feat: Add global shortcut to export logs #2336
Conversation
was moved into AppBootstrap.tsx
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
I think the e2e test is failing in CI because the timeout may not be long enough - it may take a while for the UI to load in a resource constrained environment. You can increase the timeout for this call specifically by passing in the option: https://playwright.dev/docs/test-timeouts
| @@ -11,7 +11,7 @@ export default defineConfig(({ mode }) => { | |||
|
|
|||
| let port = Number.parseInt(env.PORT, 10); | |||
| if (Number.isNaN(port) || port <= 0) { | |||
| port = 4030; | |||
| port = 4010; | |||
There was a problem hiding this comment.
Nice catch! This was originally 4030 as we had two other packages embed-grid and embed-chart that were 4010 and 4020 respectively, but then we added embed-widget which deprecated both of those. Since they've both been removed, embed-widget has moved to 4010 but seems we missed changing it here.
| @@ -1,12 +1,11 @@ | |||
| import React, { type ReactElement } from 'react'; | |||
| import { ContextMenuRoot, ToastContainer } from '@deephaven/components'; | |||
| import { ToastContainer } from '@deephaven/components'; | |||
There was a problem hiding this comment.
You should also remove the ContextMenuRoot from App.tsx in embed-widget.
| @@ -19,6 +25,9 @@ export type AppBootstrapProps = { | |||
| /** URL of the server. */ | |||
| serverUrl: string; | |||
|
|
|||
| /** Version of front-end. */ | |||
| uiVersion: string; | |||
There was a problem hiding this comment.
This prop should be optional - it's not really required, and by adding it we're making this a breaking change for anybody consuming this package (since they'd need to provide uiVersion or TypeScript will fail:
| uiVersion: string; | |
| uiVersion?: string; |
I'd actually go one further, and just take a logMetadata?: Record<string, unknown> prop, in case we want to include any other properties in there.
| const contextActions = [ | ||
| { | ||
| // Exporting logs in a general context | ||
| action: () => { | ||
| exportLogs( | ||
| logHistory, | ||
| { | ||
| uiVersion, | ||
| userAgent: navigator.userAgent, | ||
| }, | ||
| store.getState() | ||
| ); | ||
| }, | ||
| shortcut: GLOBAL_SHORTCUTS.EXPORT_LOGS, | ||
| isGlobal: true, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
I had mentioned on our 1:1 that the action was only in two places, so we didn't need to factor it out again. But now we've got it in a few places, I'd make a method in the app-utils package to create the context action.
function createExportLogsContextAction(metadata?: ..., reduxData?: ...) {
...
}We should also memoize the contextActions here using useMemo so we're not re-rendering unnecessarily.
…n-login-or-connection-fails
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2336 +/- ##
==========================================
- Coverage 46.70% 46.69% -0.01%
==========================================
Files 704 704
Lines 39044 39052 +8
Branches 9757 9942 +185
==========================================
+ Hits 18234 18235 +1
- Misses 20799 20806 +7
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Ctrl+Alt+Shift+L/Cmd-Option-Shift-Las a global shortcut for exporting support logsCloses #1963