guest servers persist at login#1696
Open
ignaciojimenezr wants to merge 11 commits intoMCPJam:mainfrom
Open
Conversation
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
ignaciojimenezr
force-pushed
the
servers-persist-auth-state
branch
from
2258bc2 to
dfd60ff
Compare
dosubot
Bot
added
size:XXL
ignaciojimenezr
requested review from
chelojimenez and
prathmeshpatel
and removed request for
chelojimenez
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughApp reads Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/hooks/use-workspace-state.ts`:
- Around line 458-467: The timeout/rejection handler currently calls
finishWorkspaceBootstrap() which discards guest/local workspaces and can leave
fresh accounts with no workspaces; change the timeout and failure paths (the
workspaceBootstrapTimeoutRef timeout handler and the rejection branch around the
import logic at the other location) to avoid calling finishWorkspaceBootstrap()
unconditionally — instead set a "bootstrapTimedOut" or "deferFinishUntilRemote"
flag (or call finishWorkspaceBootstrap(preserveLocal = true)) so local/guest
workspaces remain active until a remote workspace is actually observed, and only
call finishWorkspaceBootstrap to clear guest data after a confirmed remote
workspace materializes. Ensure to update both the timeout block that currently
logs and toasts and the reject/catch branch referenced around lines 675-684 to
use the same preservation logic.
- Around line 423-429: The current auth-reset useEffect clears shared boolean
refs (migrationInFlightRef, bootstrapMutationInFlightRef, carryForwardAbortRef,
carryForwardCompletedRef, setHasCompletedBootstrapImport) which allows stale
async tasks to resume or race after sign-out; replace this with a request-scoped
generation/token: add an operationGenerationRef (e.g., numeric or UUID) and
increment it inside the useEffect when auth drops or local fallback activates,
stop resetting the shared booleans there, and update all async flows that start
migrations/bootstraps (the code paths currently guarded at the checks referenced
around lines 516, 535, 621) to capture the current generation at start and
early-abort if the captured generation no longer matches operationGenerationRef
before performing any writes (sharedWorkspaceId, convexActiveWorkspaceId,
localStorage or initiating a second import); ensure carryForwardAbort/completed
behavior is derived from comparing generations rather than global booleans so
each request is scoped to its own token.
- Line 17: The import list at the top of use-workspace-state.ts removed
serializeServersForSharing causing type-check failures where
serializeServersForSharing is used (lines calling serializeServersForSharing).
Restore the named import by adding serializeServersForSharing back to the import
from "@/lib/workspace-serialization" alongside deserializeServersFromConvex so
the calls to serializeServersForSharing resolve correctly.
In `@mcpjam-inspector/client/src/lib/persisted-server-payload.ts`:
- Line 57: The transportType determination currently uses truthiness and
misclassifies falsy string commands; change the logic that sets transportType so
it checks the type of config.command (e.g., typeof config.command === 'string')
rather than its truthiness, ensuring transportType is "stdio" whenever a command
field exists (even if empty) and "http" only when command is absent; update the
expression that assigns transportType (the const transportType = ... statement)
accordingly so the persisted payload shape remains consistent with the presence
of config.command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38ee8107-7a89-4d99-911a-5d0e24911794
📒 Files selected for processing (12)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsxmcpjam-inspector/client/src/components/LoadingScreen.tsxmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsxmcpjam-inspector/client/src/hooks/use-app-state.tsmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/use-workspace-state.tsmcpjam-inspector/client/src/hooks/useWorkspaces.tsmcpjam-inspector/client/src/lib/__tests__/persisted-server-payload.test.tsmcpjam-inspector/client/src/lib/persisted-server-payload.ts
💤 Files with no reviewable changes (1)
- mcpjam-inspector/client/src/components/ServersTab.tsx
| >, | ||
| ): PersistedServerPayload { | ||
| const config = serverEntry.config as Record<string, unknown>; | ||
| const transportType = config.command ? "stdio" : "http"; |
Contributor
There was a problem hiding this comment.
Derive transportType from command type, not truthiness.
On Line 57, falsy string commands can be misclassified as "http" while Line 68 still persists a command, creating an inconsistent payload shape.
Proposed fix
- const transportType = config.command ? "stdio" : "http";
+ const transportType = typeof config.command === "string" ? "stdio" : "http";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const transportType = config.command ? "stdio" : "http"; | |
| const transportType = typeof config.command === "string" ? "stdio" : "http"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/persisted-server-payload.ts` at line 57, The
transportType determination currently uses truthiness and misclassifies falsy
string commands; change the logic that sets transportType so it checks the type
of config.command (e.g., typeof config.command === 'string') rather than its
truthiness, ensuring transportType is "stdio" whenever a command field exists
(even if empty) and "http" only when command is absent; update the expression
that assigns transportType (the const transportType = ... statement) accordingly
so the persisted payload shape remains consistent with the presence of
config.command.
prathmeshpatel
requested changes
Apr 8, 2026
Comment on lines
+583
to
+594
| @@ -460,12 +589,121 @@ export function useWorkspaceState({ | |||
| logger, | |||
| workspaceOrganizationId, | |||
| canManageBillingForWorkspaceActions, | |||
| appState.activeWorkspaceId, | |||
| finishWorkspaceBootstrap, | |||
| hasCarryForwardCandidates, | |||
Collaborator
There was a problem hiding this comment.
[do] hasCarryForwardCandidates shows up twice, remove one
| ); | ||
| } | ||
|
|
||
| export function buildRemoteServerFromPersistedPayload(args: { |
Collaborator
There was a problem hiding this comment.
[do] is this used anywhere? if not lets remove
Comment on lines
+154
to
+156
| const bootstrapMutationInFlightRef = useRef(false); | ||
| const carryForwardAbortRef = useRef(false); | ||
| const carryForwardCompletedRef = useRef(false); |
Collaborator
There was a problem hiding this comment.
[try] Could you add claude code/codex to add some comments here, this is hard to follow
Comment on lines
+119
to
+123
| const shouldPauseRemoteBootstrapQueries = | ||
| isAuthenticated && | ||
| !useLocalFallback && | ||
| hasCarryForwardCandidates && | ||
| !hasCompletedBootstrapImport; |
Collaborator
There was a problem hiding this comment.
[query] what does shouldPauseRemoteBootstrapQueries even mean
Collaborator
Author
There was a problem hiding this comment.
We pause loading workspace data from Convex because we're importing the guest servers into the workspace first, if we don't wait, the UI would briefly show an empty server list before the import finishes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx (3)
1219-1225: Redundant assertion—already verified above.Lines 1209-1216 make the same
UPDATE_WORKSPACEdispatch assertion. This duplicate can be removed.🧹 Remove redundant assertion
expect(vi.mocked(toast.error)).not.toHaveBeenCalled(); - expect(dispatch).toHaveBeenCalledWith({ - type: "UPDATE_WORKSPACE", - workspaceId: "default", - updates: { - sharedWorkspaceId: "remote-1", - }, - }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx` around lines 1219 - 1225, Remove the duplicate assertion that checks the same dispatch payload for the "UPDATE_WORKSPACE" action: locate the second expect(dispatch).toHaveBeenCalledWith({... type: "UPDATE_WORKSPACE", workspaceId: "default", updates: { sharedWorkspaceId: "remote-1" } ...}) in the use-workspace-state.test.tsx test and delete that redundant expect since the same assertion is already made earlier; leave the original single assertion intact.
1243-1254: Repeated inline type—consider extracting.This bootstrap result shape appears verbatim at lines 1664-1673, 1743-1752, and 1753-1762. A shared type alias would reduce duplication and ease future schema changes.
♻️ Extract shared type
// Near the top of the file, after imports type BootstrapImportResult = { targetWorkspaceId: string; targetOrganizationId?: string; createdWorkspace: boolean; importedServerNames: string[]; skippedExistingNameServerNames: string[]; failedServerNames: string[]; importedSourceWorkspaceIds: string[]; timedOut: boolean; }; // Then use throughout: let resolveBootstrapImport: ((value: BootstrapImportResult) => void) | null = null; const deferred = createDeferred<BootstrapImportResult>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx` around lines 1243 - 1254, The anonymous repeated inline type for the bootstrap import result should be extracted into a shared type alias (e.g., BootstrapImportResult) and then used wherever that shape appears; update declarations like resolveBootstrapImport and any createDeferred<T>() usages to reference BootstrapImportResult instead of repeating the object literal type so changes to the schema are centralized (ensure the alias is declared near the top of the file after imports and replace all verbatim occurrences at the sites around resolveBootstrapImport and the other deferred/createDeferred usages).
923-990: Near-duplicate test case with lines 854-921.This test shares identical setup, mock response, and assertions with
"treats a same-name remote server as already imported". The distinct intent (header differences being tolerated) isn't exercised by any unique assertions.Consider consolidating into a single parameterized test or adding an assertion that actually validates header-difference behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx` around lines 923 - 990, The test "silently skips a same-name remote server even when headers differ" duplicates setup and assertions from the existing "treats a same-name remote server as already imported"; either remove this duplicate or consolidate by parameterizing the test, and if you intend to validate header-tolerance keep the test but add an assertion that verifies header differences are ignored (e.g., ensure bootstrapGuestServerImportMock was called with the local server config that differs only in requestInit.headers or assert no import attempted for server "linear" despite different headers). Update the test referencing bootstrapGuestServerImportMock, the local workspace created via createLocalWorkspace, and the dispatch expectation for UPDATE_WORKSPACE/sharedWorkspaceId to reflect the intended unique behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx`:
- Around line 1219-1225: Remove the duplicate assertion that checks the same
dispatch payload for the "UPDATE_WORKSPACE" action: locate the second
expect(dispatch).toHaveBeenCalledWith({... type: "UPDATE_WORKSPACE",
workspaceId: "default", updates: { sharedWorkspaceId: "remote-1" } ...}) in the
use-workspace-state.test.tsx test and delete that redundant expect since the
same assertion is already made earlier; leave the original single assertion
intact.
- Around line 1243-1254: The anonymous repeated inline type for the bootstrap
import result should be extracted into a shared type alias (e.g.,
BootstrapImportResult) and then used wherever that shape appears; update
declarations like resolveBootstrapImport and any createDeferred<T>() usages to
reference BootstrapImportResult instead of repeating the object literal type so
changes to the schema are centralized (ensure the alias is declared near the top
of the file after imports and replace all verbatim occurrences at the sites
around resolveBootstrapImport and the other deferred/createDeferred usages).
- Around line 923-990: The test "silently skips a same-name remote server even
when headers differ" duplicates setup and assertions from the existing "treats a
same-name remote server as already imported"; either remove this duplicate or
consolidate by parameterizing the test, and if you intend to validate
header-tolerance keep the test but add an assertion that verifies header
differences are ignored (e.g., ensure bootstrapGuestServerImportMock was called
with the local server config that differs only in requestInit.headers or assert
no import attempted for server "linear" despite different headers). Update the
test referencing bootstrapGuestServerImportMock, the local workspace created via
createLocalWorkspace, and the dispatch expectation for
UPDATE_WORKSPACE/sharedWorkspaceId to reflect the intended unique behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fec132b-2df6-4c21-80a3-a926e21fa0ed
📒 Files selected for processing (4)
mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsxmcpjam-inspector/client/src/hooks/use-workspace-state.tsmcpjam-inspector/client/src/lib/__tests__/persisted-server-payload.test.tsmcpjam-inspector/client/src/lib/persisted-server-payload.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- mcpjam-inspector/client/src/lib/tests/persisted-server-payload.test.ts
- mcpjam-inspector/client/src/lib/persisted-server-payload.ts
- mcpjam-inspector/client/src/hooks/use-workspace-state.ts
ignaciojimenezr
changed the title
ignaciojimenezr
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx (1)
1342-1360: Duplicate assertion in test.Lines 1353-1359 repeat the exact assertion from lines 1342-1350. Consider removing the redundancy.
✂️ Proposed cleanup
await waitFor(() => { expect(dispatch).toHaveBeenCalledWith({ type: "UPDATE_WORKSPACE", workspaceId: "default", updates: { sharedWorkspaceId: "remote-1", }, }); }); expect(vi.mocked(toast.error)).not.toHaveBeenCalled(); - expect(dispatch).toHaveBeenCalledWith({ - type: "UPDATE_WORKSPACE", - workspaceId: "default", - updates: { - sharedWorkspaceId: "remote-1", - }, - }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx` around lines 1342 - 1360, The test contains a duplicated assertion: the expect(dispatch).toHaveBeenCalledWith({ type: "UPDATE_WORKSPACE", workspaceId: "default", updates: { sharedWorkspaceId: "remote-1" } }) is asserted twice; remove the second redundant expect block (the one after expect(vi.mocked(toast.error)).not.toHaveBeenCalled()) so the test only asserts the dispatch call once while keeping the toast.error check and the first waitFor assertion intact; look for the duplicated assertion referencing dispatch, "UPDATE_WORKSPACE", and sharedWorkspaceId to locate and delete the extra lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx`:
- Around line 1342-1360: The test contains a duplicated assertion: the
expect(dispatch).toHaveBeenCalledWith({ type: "UPDATE_WORKSPACE", workspaceId:
"default", updates: { sharedWorkspaceId: "remote-1" } }) is asserted twice;
remove the second redundant expect block (the one after
expect(vi.mocked(toast.error)).not.toHaveBeenCalled()) so the test only asserts
the dispatch call once while keeping the toast.error check and the first waitFor
assertion intact; look for the duplicated assertion referencing dispatch,
"UPDATE_WORKSPACE", and sharedWorkspaceId to locate and delete the extra lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bb5c22d-4465-49ef-aeaa-77bfeb055520
📒 Files selected for processing (3)
mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsxmcpjam-inspector/client/src/hooks/use-workspace-state.tsmcpjam-inspector/client/src/hooks/useWorkspaces.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/hooks/use-workspace-state.ts
dosubot
Bot
added
size:XL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
this makes guest-created servers persist after sign-in by carrying them forward during bootstrap.
What changed