-
Couldn't load subscription status.
- Fork 240
feat(workspaces): Save and restore tabs COMPASS-9499 #7253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
packages/compass-workspaces/src/stores/workspaces-middleware.ts
Outdated
Show resolved
Hide resolved
|
haven't fixed the logging comment yet but tried to address the other comments, @gribnoysup can you lmk if this looks better? |
| loadSavedWorkspaces: async () => { | ||
| throwIfNotTestEnv(); | ||
| return Promise.reject(); | ||
| }, | ||
| restoreSavedWorkspaces: () => throwIfNotTestEnv, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a public interface of workspaces that you are adding this to, loading / restoring workspaces shouldn't be exposed through it: we shouldn't allow other parts of the app to trigger this process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work well overall and aligned with all the architecture patterns nicely, good work! Still some code changes that I think we need to make. I mentioned this in one of the new review comments: this is a pretty big chunk of work and doing it in one go is just making it harder for yourself. We should add a feature flag and split the remaining work into smaller chunks
Also the e2e test failures are legit and need to be addressed, this feature now gets in the way of expected app behavior in some scenarios. Adding a FF will temporarily help to deal with that, but moving forward we should add some explicit e2e tests for a feature like that
packages/compass-workspaces/src/stores/workspaces-middleware.ts
Outdated
Show resolved
Hide resolved
| * Schema for saving workspace tab state to user data | ||
| */ | ||
|
|
||
| // Define schema for collection subtab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost everything here is exact copy of types defined in packages/compass-workspaces/src/types.ts, to avoid these diverging we should use one of those as source. As you'd need the schemas to use with UserData, we should probalby make schemas the source of types. This will require you to split the schemas here a bit to follow the type structure that we need in the app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried to address this in 76ae816, lmk if that's better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered in #7253 (comment)
| // Define schema for a workspace tab | ||
| export const WorkspaceTabSchema = z.object({ | ||
| id: z.string(), | ||
| type: WorkspaceTabTypeSchema, | ||
| connectionId: z.string().optional(), | ||
| namespace: z.string().optional(), | ||
| initialQuery: z.record(z.any()).optional(), | ||
| initialAggregation: z.record(z.any()).optional(), | ||
| initialPipeline: z.array(z.record(z.any())).optional(), | ||
| initialPipelineText: z.string().optional(), | ||
| editViewName: z.string().optional(), | ||
| initialEvaluate: z.union([z.string(), z.array(z.string())]).optional(), | ||
| initialInput: z.string().optional(), | ||
| subTab: CollectionSubtabSchema.optional(), | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of relevant to my comment above: the schema here should be a tuple of values by workspace type, similar to how it's defined in the types file. That way zod will make sure that we will never end up with workspaces where properties are not matching the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to address this and the comment above in 76ae816; Can you lmk if that's better?
packages/compass-workspaces/src/stores/workspaces-middleware.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one question + one optional suggestion ![]()
| return; | ||
| } | ||
|
|
||
| void connections.connect(connectionInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we deal with failures to connect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the feature is behind a feature flag, nothing immediately blocking this PR from my side (there are some legit failures in the CI, so please take a look). But a couple of things definitely need follow-ups. Can you please open tickets for those?
One thing that we discussed, but is still not addressed is the "invisible" tab titles while connections are loading:
Kapture.2025-10-22.at.10.27.21.mp4
As discussed before, this is due ConnectionInfoProvider rendering nothing while connection is in progress. Instead I think we should allow some sort of fallback rendering there and do this for titles inside the workspace to make it clear that we actually restored the tabs, they are just not ready.
The e2e tests are also missing and for a feature like that we definitely want some.
|
|
||
| const CollectionSubtabSchema = z.enum(collectionSubtabValues); | ||
|
|
||
| export const WorkspaceTabSchema = z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better, but only half way there IMO. As I said before, these types are now even more of a copy of what's defined in types.ts. Can you maybe walk me through your thought process here? Is there any reason you don't want to replace existing types in types.ts with types derived from the schemas? I don't see the reason not to do it, but maybe there's one I'm missing 🙂 Like imagine next time someone needs to add a new workspace, now they need to add exactly the same types twice in two different places, is there a good reason to do so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I just don't understand what the suggestion is 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace existing types in types.ts with types derived from the schemas
How exactly do I actually do this? I think I'm getting a bit mixed up on the terminology here, can we maybe refer to specific file / class names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to follow the suggestion but I don't actually understand what the suggestion is
It sounds like you're implying that there's some way to convert a typescript type (from types.ts) "directly" into a zod schema but I'm just not aware of how you would do that
|
@gribnoysup Okay, I think I finally resolved everything. Check one last time? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements saving and restoring of session tabs in Compass, allowing users to reopen their previous workspace state when restarting the application. The feature is controlled by a new enableRestoreWorkspaces feature flag and includes a confirmation dialog before restoring tabs.
Key changes:
- Added Zod schemas for workspace type validation and serialization
- Implemented middleware to debounce and persist workspace state changes
- Created storage service providers for both desktop and web platforms
- Added thunk action to load and restore saved workspaces with connection validation
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compass/src/app/components/entrypoint.tsx | Added WorkspacesStorageServiceProviderDesktop to the provider chain |
| packages/compass-workspaces/src/types.ts | Converted workspace type definitions to Zod schemas for validation and persistence |
| packages/compass-workspaces/src/stores/workspaces.ts | Added RestoreWorkspaces action and loadSavedWorkspaces thunk for loading persisted state |
| packages/compass-workspaces/src/stores/workspaces-middleware.ts | Created middleware to debounce and save workspace state changes |
| packages/compass-workspaces/src/services/workspaces-storage.tsx | Defined storage service context and noop implementation for tests |
| packages/compass-workspaces/src/services/workspaces-storage-web.tsx | Implemented Atlas-based storage provider for web platform |
| packages/compass-workspaces/src/services/workspaces-storage-desktop.tsx | Implemented file-based storage provider for desktop platform |
| packages/compass-workspaces/src/index.ts | Integrated storage service and middleware into plugin activation |
| packages/compass-web/src/entrypoint.tsx | Added WorkspacesStorageServiceProviderWeb to the web provider chain |
| packages/compass-user-data/src/index.ts | Exported IUserData as a class rather than just a type |
| packages/compass-preferences-model/src/feature-flags.ts | Added enableRestoreWorkspaces feature flag |
| packages/atlas-service/src/atlas-service.ts | Added savedWorkspaces to the list of supported resource types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dispatch({ | ||
| type: WorkspacesActions.RestoreWorkspaces, | ||
| tabs: workspacesToRestore, | ||
| }); |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dispatch occurs unconditionally even when confirm is false and workspacesToRestore is empty. This should be moved inside the if (confirm) block or wrapped in a condition checking workspacesToRestore.length > 0.
| dispatch({ | |
| type: WorkspacesActions.RestoreWorkspaces, | |
| tabs: workspacesToRestore, | |
| }); | |
| if (confirm && workspacesToRestore.length > 0) { | |
| dispatch({ | |
| type: WorkspacesActions.RestoreWorkspaces, | |
| tabs: workspacesToRestore, | |
| }); | |
| } |
| name: 'enableRestoreWorkspaces', | ||
| stage: 'development', | ||
| atlasCloudFeatureFlagName: | ||
| 'DATA_EXPLORER_COMPASS_WEB_ENABLE_RESTORE_WORKSPACES', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6067772 to
c417488
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very neat! The check failure in CI is legit: you need to add user-data dependency to the workspaces package, but otherwise LGTM for this part of work
Description
https://jira.mongodb.org/browse/COMPASS-9499
Screen.Recording.2025-09-17.at.11.48.12.AM.mov
Implement saving and restoring of session tabs.
Filed https://jira.mongodb.org/browse/COMPASS-9913 to track a couple of potential followups that I'm omitting from this PR
TODO: add test cases:
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes