Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions mcpjam-inspector/client/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ import {
getChatboxPathTokenFromLocation,
} from "./components/hosted/ChatboxChatPage";
import { useHostedApiContext } from "./hooks/hosted/use-hosted-api-context";
import { useLocalStateMigration } from "./hooks/use-local-state-migration";
import { AppReadyProvider } from "./hooks/use-app-ready";
import { useInspectorCommandBus } from "./hooks/use-inspector-command-bus";
import { HOSTED_MODE, NON_PROD_LOCKDOWN } from "./lib/config";
Expand Down Expand Up @@ -802,6 +803,13 @@ export default function App() {
: undefined,
});
useInspectorCommandBus();
// One-time migration from legacy localStorage state to Convex. No-op in
// hosted mode and after the first successful run; safe to keep in the tree.
useLocalStateMigration({
isAuthenticated,
isUserBootstrapping: isEnsuringUser,
organizationId: activeOrganizationId,
});
Comment on lines +808 to +812
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Disable the old project migration path

When a local signed-in or guest session has legacy local projects and Convex initially returns no remote projects, this new migration hook runs in the same App mount as the existing use-project-state migration effect (client/src/hooks/use-project-state.ts:760-823). That older effect still calls projects:createProject for every unshared local project and has no awareness of MIGRATION_FLAG_KEY, while this hook also calls runLocalStateMigration from localStorage, so the first boot after this change can create each local project twice before either subscription observes the new remote rows. Gate or remove the old effect before enabling this one to avoid duplicate Convex projects/servers.

Useful? React with 👍 / 👎.

const oauthDebuggerServersRef = useRef(appState.servers);
oauthDebuggerServersRef.current = appState.servers;
const projectServersRef = useRef(projectServers);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ vi.mock("@/lib/oauth/mcp-oauth", () => ({

vi.mock("@/lib/apis/web/context", () => ({
injectHostedServerMapping: vi.fn(),
tryGetHostedServerDisplayName: vi.fn(),
tryResolveProjectServer: vi.fn(() => null),
}));

vi.mock("@/lib/session-token", () => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ vi.mock("@/lib/oauth/mcp-oauth", () => ({

vi.mock("@/lib/apis/web/context", () => ({
injectHostedServerMapping: vi.fn(),
tryGetHostedServerDisplayName: vi.fn(),
tryResolveProjectServer: vi.fn(() => null),
}));

vi.mock("@/lib/session-token", () => ({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { useLayoutEffect } from "react";
import { HOSTED_MODE } from "@/lib/config";
import { setHostedApiContext } from "@/lib/apis/web/context";

interface UseHostedApiContextOptions {
Expand Down Expand Up @@ -35,11 +34,6 @@ export function useHostedApiContext({
// between this effect's cleanup (which nulls the context) and its setup,
// causing "Hosted server not found" errors for shared-chat OAuth servers.
useLayoutEffect(() => {
if (!HOSTED_MODE) {
setHostedApiContext(null);
return;
}

if (!enabled) {
return;
}
Expand Down
268 changes: 268 additions & 0 deletions mcpjam-inspector/client/src/hooks/use-local-state-migration.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,268 @@
import { useEffect, useRef, useState } from "react";
import { useMutation } from "convex/react";
import {
hasMigrationCompleted,
runLocalStateMigration,
} from "@/lib/local-state-migration";
import { HOSTED_MODE } from "@/lib/config";
import { useLogger } from "./use-logger";

/**
* localStorage key holding a short-lived lease (timestamp ms) so two
* inspector tabs opened before either marks the migration complete don't
* both call `projects:createProject` for the same legacy project. The lease
* is bounded by `MIGRATION_LEASE_TTL_MS` so a tab that crashed mid-migration
* doesn't block another tab forever.
*/
const MIGRATION_LEASE_KEY = "mcp-inspector-migration-lease";
const MIGRATION_LEASE_TTL_MS = 5 * 60 * 1000;

/**
* Backoff used when a migration attempt returns `ok: false` (e.g., Convex
* was briefly unreachable, or one project failed) or the cross-tab lease
* is currently held. Without an explicit retry trigger the `useEffect`
* dependencies are stable and the failed migration would effectively wait
* for a page reload.
*/
const RETRY_DELAY_MS = 30 * 1000;

/**
* Hard cap on retry attempts so a permanent error (billing limit reached,
* forbidden, validation) doesn't spam the console every 30s forever. After
* this many retries the hook stops scheduling new ones; the user can still
* trigger a fresh attempt by reloading the tab.
*/
const MAX_RETRY_ATTEMPTS = 3;

/**
* Convex error codes that the migration cannot recover from by retrying.
* If any per-project failure carries one of these codes, the hook treats
* the whole batch as terminal — no retry, no log spam. The user reloads
* (or fixes the underlying account state) to try again.
*/
const PERMANENT_ERROR_CODES = new Set([
"billing_limit_reached",
"FORBIDDEN",
"UNAUTHORIZED",
"VALIDATION_ERROR",
]);

function looksPermanent(errorMessage: string): boolean {
for (const code of PERMANENT_ERROR_CODES) {
if (errorMessage.includes(code)) return true;
}
return false;
}

interface UseLocalStateMigrationOptions {
/** True when Convex auth has resolved (signed-in user OR guest). */
isAuthenticated: boolean;
/**
* True while `users:ensureUser` is still running. The migration depends
* on the actor's `users` row + default org existing in Convex; firing
* before bootstrap can race the row creation and surface as a
* `createProject` failure with no automatic retry. Wait until this is
* false before attempting migration.
*/
isUserBootstrapping: boolean;
/**
* Optional org id to migrate into. Undefined defers to Convex's
* resolveProjectOrganizationId which falls back to the actor's default
* organization (provisioned by `users:ensureUser`).
*/
organizationId?: string;
}

function tryAcquireMigrationLease(): boolean {
if (typeof window === "undefined") return false;
try {
const now = Date.now();
const existing = localStorage.getItem(MIGRATION_LEASE_KEY);
if (existing) {
const ts = Number(existing);
if (Number.isFinite(ts) && now - ts < MIGRATION_LEASE_TTL_MS) {
return false;
}
}
localStorage.setItem(MIGRATION_LEASE_KEY, String(now));
return true;
} catch {
// localStorage blocked — caller falls back to in-memory ref guard.
return true;
}
}

function releaseMigrationLease(): void {
if (typeof window === "undefined") return;
try {
localStorage.removeItem(MIGRATION_LEASE_KEY);
} catch {
// best-effort
}
}
Comment on lines +76 to +102
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make the migration lease owner-aware.

This lease is only a timestamp. Two tabs can both read “no lease”, both set it, and both continue. releaseMigrationLease() then removes the key unconditionally, so one tab can clear another tab’s active lease. That leaves the duplicate-project race intact.

💡 Suggested direction
+type MigrationLease = { token: string; ts: number };
+
-function tryAcquireMigrationLease(): boolean {
+function tryAcquireMigrationLease(): string | null {
   if (typeof window === "undefined") return false;
   try {
     const now = Date.now();
-    const existing = localStorage.getItem(MIGRATION_LEASE_KEY);
-    if (existing) {
-      const ts = Number(existing);
-      if (Number.isFinite(ts) && now - ts < MIGRATION_LEASE_TTL_MS) {
+    const existingRaw = localStorage.getItem(MIGRATION_LEASE_KEY);
+    if (existingRaw) {
+      const existing = JSON.parse(existingRaw) as MigrationLease;
+      if (Number.isFinite(existing.ts) && now - existing.ts < MIGRATION_LEASE_TTL_MS) {
         return false;
       }
     }
-    localStorage.setItem(MIGRATION_LEASE_KEY, String(now));
-    return true;
+    const lease = { token: crypto.randomUUID(), ts: now };
+    const serialized = JSON.stringify(lease);
+    localStorage.setItem(MIGRATION_LEASE_KEY, serialized);
+    return localStorage.getItem(MIGRATION_LEASE_KEY) === serialized
+      ? lease.token
+      : null;
   } catch {
-    return true;
+    return "in-memory-fallback";
   }
 }
 
-function releaseMigrationLease(): void {
+function releaseMigrationLease(token: string | null): void {
   if (typeof window === "undefined") return;
   try {
-    localStorage.removeItem(MIGRATION_LEASE_KEY);
+    const currentRaw = localStorage.getItem(MIGRATION_LEASE_KEY);
+    if (!currentRaw) return;
+    const current = JSON.parse(currentRaw) as MigrationLease;
+    if (current.token === token) {
+      localStorage.removeItem(MIGRATION_LEASE_KEY);
+    }
   } catch {
     // best-effort
   }
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mcpjam-inspector/client/src/hooks/use-local-state-migration.ts` around lines
48 - 74, The lease is just a timestamp and not owner-aware, so concurrent tabs
can overwrite each other and release other tabs' leases; update
tryAcquireMigrationLease and releaseMigrationLease to use an owner id: generate
a unique ownerId per tab (e.g., UUID stored in-memory), store the lease value as
a JSON string { ts, owner } under MIGRATION_LEASE_KEY, and when acquiring
perform the same TTL check but write the JSON and then re-read the key to
confirm the stored owner matches our ownerId (treat mismatch as acquisition
failure); when releasing, only remove the key if the current stored owner equals
our ownerId; preserve existing try/catch fallbacks for localStorage errors and
keep behavior of returning booleans/void as before, and reference
MIGRATION_LEASE_TTL_MS for TTL logic.


/**
* Runs the legacy-localStorage → Convex migration exactly once per install.
*
* Skips when:
* - HOSTED_MODE (hosted users never had localStorage state)
* - The migration flag is already set
* - Convex auth hasn't resolved
* - User bootstrap (`users:ensureUser`) is still running
* - Migration is already in flight (in-tab) or another tab holds the lease
*
* On `ok: false` (partial failure) or contention with another tab,
* schedules a retry tick after `RETRY_DELAY_MS`. Because `useMutation`
* returns a stable reference and the gate inputs rarely change, the prior
* "retry on next render" approach effectively waited for a page reload —
* the explicit tick state forces the effect to re-evaluate.
*/
export function useLocalStateMigration({
isAuthenticated,
isUserBootstrapping,
organizationId,
}: UseLocalStateMigrationOptions): void {
const logger = useLogger("LocalStateMigration");
const inFlightRef = useRef(false);
const doneRef = useRef(false);
const retryTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const retryCountRef = useRef(0);
const [retryTick, setRetryTick] = useState(0);
const createProject = useMutation("projects:createProject" as any);

useEffect(() => {
// Per-effect-run cancel flag. The migration is async — the component can
// unmount (or the effect can re-run on dep change) while the promise is
// still in flight. Without this flag, the trailing `.then/.catch/.finally`
// would call `scheduleRetry()` after unmount, scheduling a `setTimeout`
// that the cleanup function has already had its chance to clear. Result:
// an orphaned 30s timer that fires after the component is gone and calls
// `setRetryTick` on a dead instance.
let cancelled = false;

// Defined inside the effect so it closes over the same `logger` reference
// the effect itself uses. Hoisting it to the component body would let the
// async .then/.catch callbacks reach a stale `logger` if `useLogger`
// returns a new object on a subsequent render.
const scheduleRetry = (): void => {
if (cancelled) return;
if (retryTimerRef.current !== null) return;
if (retryCountRef.current >= MAX_RETRY_ATTEMPTS) {
logger.warn(
"Local state migration retry limit reached; will not retry until reload",
{ attempts: retryCountRef.current },
);
doneRef.current = true;
return;
}
retryCountRef.current += 1;
retryTimerRef.current = setTimeout(() => {
retryTimerRef.current = null;
setRetryTick((t) => t + 1);
}, RETRY_DELAY_MS);
};

if (HOSTED_MODE) return;
if (!isAuthenticated) return;
if (isUserBootstrapping) return;
if (doneRef.current) return;
if (inFlightRef.current) return;
if (hasMigrationCompleted()) {
doneRef.current = true;
Comment on lines +165 to +171
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Wait for organization selection before migrating

In signed-in local sessions, this effect can run as soon as Convex auth and ensureUser finish, while useOrganizationQueries is still loading and activeOrganizationId is still undefined. In that window runLocalStateMigration calls createProject with organizationId: undefined, so Convex falls back to the actor's default org, then clears the legacy keys and sets the migration flag; users with a stored/route-selected different org have their local projects migrated to the wrong organization permanently. Gate this on organization loading/selection resolution for signed-in users before acquiring the migration lease.

Useful? React with 👍 / 👎.

return;
}

if (!tryAcquireMigrationLease()) {
logger.info("Another tab holds the migration lease; will retry", {
retryDelayMs: RETRY_DELAY_MS,
});
scheduleRetry();
return () => {
cancelled = true;
};
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing timer cleanup in lease-contention effect cleanup

Low Severity

When tryAcquireMigrationLease() returns false (another tab holds the lease), scheduleRetry() is called which sets retryTimerRef.current via setTimeout. However, the cleanup function returned from this branch only sets cancelled = true and does not clear the retry timer. The main async path's cleanup correctly clears the timer with clearTimeout(retryTimerRef.current). This inconsistency means a dep-change or unmount during the lease-contention wait leaves an orphaned 30-second timer that fires setRetryTick after the effect instance is stale.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 937ed9c. Configure here.


inFlightRef.current = true;
runLocalStateMigration({
createProject: createProject as any,
organizationId,
logger,
})
.then((result) => {
if (cancelled) return;
if (result.ok) {
doneRef.current = true;
if (result.projectsMigrated > 0) {
logger.info("Local state migration completed", {
projectsMigrated: result.projectsMigrated,
});
}
return;
}
// Permanent backend errors (billing limit, forbidden, validation)
// won't recover by retrying. Mark done so the user can fix the
// account state and reload, instead of retrying every 30s.
const hasPermanentError = result.errors.some((e) =>
looksPermanent(e.error),
);
if (hasPermanentError) {
logger.error(
"Local state migration hit a permanent error; not retrying",
{ errors: result.errors },
);
doneRef.current = true;
return;
}
Comment thread
cursor[bot] marked this conversation as resolved.
logger.warn(
"Local state migration partially failed; scheduling retry",
{
errors: result.errors,
retryDelayMs: RETRY_DELAY_MS,
attempt: retryCountRef.current + 1,
},
);
scheduleRetry();
})
.catch((error) => {
if (cancelled) return;
const message = error instanceof Error ? error.message : String(error);
if (looksPermanent(message)) {
logger.error(
"Local state migration threw a permanent error; not retrying",
{ error: message },
);
doneRef.current = true;
return;
}
logger.error("Local state migration threw; scheduling retry", {
error: message,
retryDelayMs: RETRY_DELAY_MS,
attempt: retryCountRef.current + 1,
});
scheduleRetry();
})
.finally(() => {
inFlightRef.current = false;
releaseMigrationLease();
});

return () => {
// Block any trailing async resolutions from scheduling new work, and
// clear the retry timer if one was already scheduled. The other refs
// (`inFlightRef`, `doneRef`) intentionally persist so a re-mount or a
// dep-change re-run picks up where we left off.
cancelled = true;
if (retryTimerRef.current !== null) {
clearTimeout(retryTimerRef.current);
retryTimerRef.current = null;
}
};
}, [
isAuthenticated,
isUserBootstrapping,
organizationId,
createProject,
logger,
retryTick,
]);
Comment thread
cursor[bot] marked this conversation as resolved.
}
Loading
Loading