Skip to content

refactor: inspector tabs bundled with actor#4670

Draft
jog1t wants to merge 1 commit into04-15-refactor_rivetkit_move_devtools_to_local_scriptfrom
04-15-refactor_inspector_tabs_bundled_with_actor
Draft

refactor: inspector tabs bundled with actor#4670
jog1t wants to merge 1 commit into04-15-refactor_rivetkit_move_devtools_to_local_scriptfrom
04-15-refactor_inspector_tabs_bundled_with_actor

Conversation

@jog1t
Copy link
Copy Markdown
Contributor

@jog1t jog1t commented Apr 15, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@railway-app
Copy link
Copy Markdown

railway-app bot commented Apr 15, 2026

🚅 Deployed to the rivet-pr-4670 environment in rivet-frontend

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web Apr 15, 2026 at 8:19 pm
frontend-cloud ❌ Build Failed (View Logs) Web Apr 15, 2026 at 8:07 pm
frontend-inspector ❌ Build Failed (View Logs) Web Apr 15, 2026 at 8:07 pm
mcp-hub ✅ Success (View Logs) Web Apr 15, 2026 at 8:07 pm
ladle ❌ Build Failed (View Logs) Web Apr 15, 2026 at 8:07 pm
kitchen-sink ❌ Build Failed (View Logs) Web Apr 15, 2026 at 8:07 pm

Copy link
Copy Markdown
Contributor Author

jog1t commented Apr 15, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude bot commented Apr 15, 2026

Code Review: refactor: inspector tabs bundled with actor

Note: This is a draft PR — reviewing to provide early feedback.

Overview

This PR moves inspector tabs (state, database, connections, queue, workflow, metadata) from inline React components into separate iframe bundles served at /ui/tabs/<tab>/. A postMessage bridge (IframeTabBridgeProvider / TabContextProvider) shuttles inspector API calls and query cache updates between the shell and each iframe. The console tab is intentionally left inline due to its dependency on TanStack Router context.

The architecture is sound and solves a real problem (tabs stay loaded across switches via forceMount), but there are several issues worth addressing before merge.


Security

allow-scripts allow-same-origin sandbox combination

In iframe-tab-content.tsx:37, the iframe uses:

sandbox="allow-scripts allow-same-origin"

The combination of allow-scripts and allow-same-origin effectively nullifies sandbox isolation — a script in the iframe can remove its own sandbox attribute and gain unrestricted same-origin access to the parent. Since these tabs are already same-origin trusted content this isn't a new vulnerability, but it means the sandbox attribute provides zero security value and is misleading. Either drop sandbox entirely or document why it's there (e.g. future cross-origin path).

sendAction falls back to "*" as postMessage target

In tab-context.tsx:608:

const target = shellOrigin.current ?? "*";
window.parent.postMessage({ type: "action-request", ... }, target);

If a tab sends an action before receiving the init message (e.g. a race on fast loads), it broadcasts action requests to any listening window. Since action-request payloads contain actor operation details, this is a low-severity but unnecessary information leak. Consider queuing actions until shellOrigin is known, or failing with a clear error instead of falling back to "*".

Wrong dashboard domain

Both tab-context.tsx:469 and router.ts:473 reference https://dashboard.rivet.dev. Per CLAUDE.md, the dashboard is at https://hub.rivet.dev. If these are two different properties, please add a comment explaining the distinction.

// tab-context.tsx
const ALLOWED_SHELL_ORIGINS = new Set([
    window.location.origin,
    "https://dashboard.rivet.dev",  // should this be hub.rivet.dev?
]);

CSP frame-ancestors header not applied to all responses

In router.ts:470:

if (response instanceof Response) {
    response.headers.set("Content-Security-Policy", "frame-ancestors ...");
}

If serveStatic returns undefined (falling through to next), no CSP header is set on the resulting response. The onNotFound fallback serves index.html via a nested serveStatic call whose response isn't captured here. This leaves the fallback path without frame-ancestors protection.


Bugs / Correctness

console entry is built but never used

entries/console/ exists with a full main.tsx / index.html, but console is not in the TABS array in vite.config.ts so it won't be built, and it isn't iframed in actors-actor-details.tsx. This is dead code that will confuse contributors. Either add it to TABS (if planned) or remove the entry files.

shellOrigin query param is passed but never read

iframe-tab-content.tsx:29-30:

const shellOrigin = encodeURIComponent(window.location.origin);
const src = `/ui/tabs/${tab}/?actorId=...&shellOrigin=${shellOrigin}`;

tab-context.tsx never reads shellOrigin from the URL — it uses the hardcoded ALLOWED_SHELL_ORIGINS set. Remove the unused param to reduce confusion.

iframe registration race with ready message

If an iframe posts "ready" before its useEffect in IframeTabContent fires and calls registerIframe, the shell's handleMessage will iterate iframeMap without finding the iframe and silently skip the init. The iframe will never receive init. This is unlikely in practice (React effects run synchronously after commit) but worth a comment or a deferred fallback.


Code Style / CLAUDE.md Violations

Em dashes in comments — CLAUDE.md says "Do not use em dashes (—). Use periods to separate sentences instead."

Several new files use em dashes:

  • tab-runtime.tsx: // Data arrives via postMessage / broadcastQueryClient —
  • iframe-tab-bridge.tsx: // Cross-origin iframe — rely on the ready postMessage instead.
  • iframe-tab-content.tsx JSDoc: * which hides inactive tabs without unmounting them —

JSX indentation in actors-actor-details.tsx

<Tabs> is not indented under <IframeTabBridgeProvider>:

return (
    <IframeTabBridgeProvider actorId={actorId}>
    <Tabs   // ← should be indented one more level
        value={value}

Minor / Suggestions

  • @tanstack/query-broadcast-client-experimental: The "experimental" label means breaking changes are possible. The PR description should acknowledge this risk and track the package's stability.

  • IframeTabContent type: actorId is typed as string but the bridge and context use the branded ActorId type. Consider aligning them.

  • Self-referential type assertions in mock-inspector-context.tsx: The api object references itself (ReturnType<typeof api.getConnections>) to infer return types. This is clever but fragile — if any method's actual type diverges, TypeScript may not catch the mismatch. A shared interface type (already exported as ActorInspectorContext) would be safer.

  • useLayoutEffect for mountedTabs: The effect only updates React state; useEffect would be sufficient and avoids the synchronous-post-paint penalty.

  • broadcastQueryClient called on every render cycle: In iframe-tab-bridge.tsx, the broadcastQueryClient call is inside useEffect with [queryClient, actorId] deps. Depending on the library internals, re-calling it on actorId change may register duplicate listeners. A useRef guard like the broadcastSetUp ref used in tab-context.tsx would be safer.


Test Coverage

The PR checklist notes no tests were added. Given the postMessage bridge is the core of this change, at minimum consider:

  • A unit test for the origin allowlist logic (ensure messages from unexpected origins are rejected)
  • A test for the action timeout / cleanup path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant