Skip to content

refactor: inspector tabs bundled with actor#4670

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

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 26, 2026 at 2:04 pm
frontend-inspector ❌ Build Failed (View Logs) Web Apr 24, 2026 at 2:04 pm
frontend-cloud ❌ Build Failed (View Logs) Web Apr 24, 2026 at 2:00 pm
ladle ❌ Build Failed (View Logs) Web Apr 24, 2026 at 1:59 pm
kitchen-sink ❌ Build Failed (View Logs) Web Apr 24, 2026 at 1:59 pm
mcp-hub ✅ Success (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

Review: refactor: inspector tabs bundled with actor

This PR introduces a postMessage-based iframe architecture for inspector tabs, where each tab (workflow, database, state, queue, connections, metadata) is built as a separate Vite bundle and rendered inside a sandboxed iframe. The shell communicates with iframes via a structured message protocol to bridge the inspector API and sync query cache. This is a well-thought-out architectural change.

Bugs / Correctness

Dead code: entries/console/ is never built

The entries/console/ directory (with index.html and main.tsx) is added by the PR but console is intentionally absent from the TABS array in vite.config.ts. The inline comment explains console stays inline due to TanStack Router context requirements. This directory and its files should either be removed to avoid confusion, or documented more explicitly as a work-in-progress.

shellOrigin URL param is passed but never consumed

In iframe-tab-content.tsx the iframe src is constructed with shellOrigin=${encodeURIComponent(window.location.origin)}, but tab-context.tsx never reads this param. The tab records the origin from the first valid postMessage instead. The parameter is harmless but misleading. Either remove it, or read it at mount time to pre-populate shellOrigin.current to solve the next issue.

sendAction targets "*" before the init message arrives

In tab-context.tsx sendAction sends to shellOrigin.current ?? "*" while shellOrigin.current is still null (before the first postMessage from the shell). If a component fires an action before init, the postMessage leaks to any listener. Reading the shellOrigin URL param at mount time would initialize the ref and remove the "*" fallback.

broadcastQueryClient may be called multiple times in the shell

IframeTabBridgeProvider calls broadcastQueryClient({ queryClient, broadcastChannel }) inside a useEffect that re-runs on actorId or queryClient changes. The API is not idempotent; repeated calls on the same QueryClient may register duplicate BroadcastChannel subscribers and cause duplicate cache updates. Guard the call with a ref (similar to the broadcastSetUp.current pattern already used in tab-context.tsx).

Security

CSP frame-ancestors and allowed origins use an unrecognized domain

In router.ts:

frame-ancestors 'self' https://dashboard.rivet.dev

Per CLAUDE.md the dashboard is at https://hub.rivet.dev. Similarly, tab-context.tsx has ALLOWED_SHELL_ORIGINS set to "https://dashboard.rivet.dev". If the production cloud dashboard is at hub.rivet.dev, both should be updated to that domain. If dashboard.rivet.dev is intentional, it should be documented.

Action dispatch via dynamic key access

In iframe-tab-bridge.tsx the bridge casts the inspector API to Record<string, (...args) => Promise<unknown>> and calls api[action.name](...action.args). The ALLOWED_ACTIONS set provides the guard and the model is sound for same-origin iframes. A brief comment noting that all callers are same-origin and that the allowlist is the security boundary would help future reviewers.

Code Quality

<IframeTabBridgeProvider> JSX indentation in actors-actor-details.tsx

The <Tabs> element is not indented relative to <IframeTabBridgeProvider>, making the nesting hard to read at a glance.

Inline comment about console is slightly imprecise

The comment says "console stays inline: ActorWorkerContextProvider uses TanStack Router context unavailable in iframes" -- the actual dependency is useDataProvider() inside actor-worker-context.tsx, which calls useRouteContext. ActorWorkerContextProvider itself does not import from @tanstack/react-router directly.

Summary

The core architecture is solid: structured message protocol with origin allowlisting, query cache hydration via BroadcastChannel, iframe lifecycle deferred to first tab activation, and an explicit action allowlist at the bridge boundary. Main actionable items:

  1. Remove the dead entries/console/ files or document their intent
  2. Eliminate the "*" target-origin race by reading shellOrigin from the URL param at mount
  3. Confirm and fix the dashboard.rivet.dev vs hub.rivet.dev domain in the CSP and allowed-origins set
  4. Guard broadcastQueryClient in IframeTabBridgeProvider against duplicate registration

@jog1t jog1t force-pushed the 04-15-refactor_inspector_tabs_bundled_with_actor branch from 1fe66d4 to a190d13 Compare April 16, 2026 18:01
@jog1t jog1t force-pushed the 04-15-refactor_rivetkit_move_devtools_to_local_script branch from 986ef51 to 5a4e315 Compare April 16, 2026 18:01
@jog1t jog1t force-pushed the 04-15-refactor_inspector_tabs_bundled_with_actor branch from a190d13 to f7625ea Compare April 16, 2026 21:46
@jog1t jog1t force-pushed the 04-15-refactor_rivetkit_move_devtools_to_local_script branch from 5a4e315 to d5a6b40 Compare April 16, 2026 21:46
@jog1t jog1t marked this pull request as ready for review April 16, 2026 21:47
@jog1t jog1t force-pushed the 04-15-refactor_rivetkit_move_devtools_to_local_script branch from d5a6b40 to b5b770f Compare April 24, 2026 13:58
@jog1t jog1t force-pushed the 04-15-refactor_inspector_tabs_bundled_with_actor branch from f7625ea to 4d42d30 Compare April 24, 2026 13:58
@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

PR Review: refactor: inspector tabs bundled with actor

Summary

This PR refactors the inspector UI by moving several actor detail tabs (workflow, database, state, queue, connections, metadata) from inline React components into separately-bundled iframe apps. The shell communicates with each iframe via postMessage and BroadcastChannel for query cache sync. The architecture is well-conceived, but there are a few issues worth addressing before merge.


Critical Issues

1. Inspector-tabs bundle is never built

frontend/vite.inspector-tabs.config.ts is added but no package.json script or turbo task calls it. The build pipeline invokes build:inspector (Dockerfiles reference it), but that task is not defined in frontend/package.json or frontend/turbo.json, and nothing ever invokes vite build --config vite.inspector-tabs.config.ts. The tabs will be absent from any deployed tarball. A build:inspector script needs to run both vite.inspector.config.ts and vite.inspector-tabs.config.ts.

2. @tanstack/query-core version mismatch breaks broadcastQueryClient

@tanstack/query-broadcast-client-experimental was resolved to 5.99.0, which pulls in @tanstack/query-core@5.99.0 as a second copy alongside the existing @tanstack/query-core@5.87.1. The broadcastQueryClient() call on the shell side passes a QueryClient from @tanstack/react-query (using query-core@5.87.1), while the broadcast internals use query-core@5.99.0. Two separate module instances will break cache sync. Pin all @tanstack/query-* packages to the same minor version.


Medium Issues

3. Dead entries/console/ files

frontend/apps/inspector-tabs/entries/console/ exists but "console" is absent from the TABS const in vite.config.ts, so it is never built. These files are dead code and should be removed.

4. Incorrect queryKey cast in tab-context.tsx

queryClient.setQueryData(msg.queryKey as string[], msg.data) -- msg.queryKey is unknown[] (correct for TanStack Query keys), but cast to string[]. The cast should be unknown[] or the proper QueryKey type from @tanstack/react-query.

5. isInitialized exported but never consumed

tab-context.tsx defines and exports isInitialized in the context value, but no component in the PR reads it. If tabs render before the init message arrives from the shell, they show stale data with no loading state. Either use isInitialized to show a loading indicator or remove the field.

6. sendAction posts to "*" before shell origin is established

In tab-context.tsx: const target = shellOrigin.current ?? "*". If sendAction is called before the first valid message arrives, the action request is posted to "*". Any window listening to postMessages could intercept the response. The tabs should queue or reject actions until shellOrigin is known, or derive the origin from the shellOrigin URL param already passed in the iframe src.

7. getQueryCacheSnapshot uses fragile string inclusion check

if (keyStr.includes(actorId)) matches any query key whose JSON serialization contains the actorId string. A more precise check should compare against known key prefixes using the actorInspectorQueriesKeys factory.


Minor Issues

8. dashboard.rivet.dev vs hub.rivet.dev in CSP and origin allowlist

The CSP in router.ts and ALLOWED_SHELL_ORIGINS in tab-context.tsx use https://dashboard.rivet.dev, but CLAUDE.md defines the canonical dashboard URL as https://hub.rivet.dev. If hub users embed the inspector from hub.rivet.dev, the origin will be rejected. Consider adding both origins or confirming which domain is correct.

9. broadcastQueryClient called on every remount without cleanup guard

IframeTabBridgeProvider calls broadcastQueryClient(...) unconditionally on mount. If the provider remounts (navigating between actors), the old BroadcastChannel listener may persist. Consider adding a guard similar to the broadcastSetUp.current check used on the tab side, and/or a cleanup on unmount.

10. Misleading try/catch for same-origin contentDocument access

In iframe-tab-bridge.tsx, the try/catch around iframe.contentDocument?.readyState has a comment saying "Cross-origin iframe", but all iframes in this architecture are same-origin -- accessing contentDocument never throws. This try/catch is dead code and the comment is misleading.

11. JSX indentation regression in actors-actor-details.tsx

<Tabs> and its contents are not indented inside <IframeTabBridgeProvider>, making the JSX hierarchy misleading.


Security Observations

  • The ALLOWED_ACTIONS allowlist in iframe-tab-bridge.tsx is a good defense against arbitrary inspector API calls from iframes. The dynamic dispatch api[action.name](...action.args) being guarded by this allowlist is the right approach.
  • The sandbox="allow-scripts allow-same-origin" combination on tab iframes provides no sandboxing benefit for same-origin content. This is expected for BroadcastChannel/postMessage to work, but worth noting.

Summary

Severity Issue
Critical Inspector-tabs bundle is never built (no build script/task)
Critical @tanstack/query-core version mismatch breaks BroadcastChannel sync
Medium Dead entries/console/ files that cannot be built
Medium Incorrect queryKey as string[] cast in tab-context.tsx
Medium isInitialized exported but never consumed (no loading state)
Medium sendAction posts to "*" before shell origin is established
Medium getQueryCacheSnapshot uses fragile string-include actorId filter
Minor dashboard.rivet.dev vs hub.rivet.dev in CSP/allowlist
Minor broadcastQueryClient remount without cleanup guard
Minor Misleading try/catch for same-origin contentDocument access
Minor JSX indentation inside IframeTabBridgeProvider

The overall architecture (iframe tabs, postMessage bridge, BroadcastChannel cache sync) is sound. The two critical issues (missing build step and query-core version mismatch) need to be resolved before this can ship correctly.

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