fix(hub-ui): initialize dock page scripts before activation - #387
fix(hub-ui): initialize dock page scripts before activation#387dvcolomban wants to merge 4 commits into
Conversation
◈ PR Lens
Architecture 7 components touched across 3 lanes. Inside the changed components — 2 viewsComponent view — Hub UI script lifecycle Internal script coordination, trust gating, and per-RPC caching inside Hub UI Component view — Headless runtime scripts Client script initialization, trust verification, and caching in createDevframeClientRuntime Data flow
The other flows — 1 sequence
View
Tip Untick Architecture lens or Data flow lens under View to hide a diagram, or tick Expand every detail to open every section. The comment redraws in a few seconds. 🪧 More tips
Thanks for using PR Lens! It's built by Coldtea, free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
|
@dvcolomban is attempting to deploy a commit to the NuxtLabs Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues affect runtime script initialization and activation behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates dock page-script initialization so trusted scripts load before activation, including JSON-render entries.
Changes:
- Moves
clientScriptto the common dock entry type. - Adds per-RPC setup caching and retry behavior.
- Expands regression and integration coverage.
File summaries
| File | Review findings |
|---|---|
packages/hub/src/types/docks.ts |
Critical (2 votes): The headless runtime can skip the base clientScript when an activation script exists. |
packages/hub-ui/src/client/state/setup-script.ts |
Critical (1 vote): The cache key does not distinguish page scripts from activation renderers. |
packages/hub-ui/src/client/state/context.ts |
Moderate (2 votes): Action entries with both action and clientScript skip the page script. Moderate (1 vote): Page setup is fire-and-forget and may complete after the panel becomes active. |
packages/hub-ui/src/client/state/context.test.ts |
No final review findings. |
packages/hub-ui/src/client/state/client-script.integration.test.ts |
No final review findings. |
docs/content/8.references/6.hub-api.md |
Nit (3 votes): The linked guide contradicts the new initialization contract and should cover JSON-render entries and trust/activation lifetime. |
Review details
Suppressed comments (1)
packages/hub-ui/src/client/state/context.ts:254
- The setup is only started fire-and-forget here. A slow import or async initializer can therefore leave the panel visible while its page commands are still unregistered:
switchEntry()publishes the selected/open state before awaiting its setup, and for an entry with a separate pageclientScriptplus renderer it awaits only the renderer setup. Track the page-setup promise and await it before exposing the active panel so this notification race is actually eliminated.
void executeSetupScript(entry, scriptContext(entry), entry.clientScript).catch(() => {})
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6cfb504 to
1fd6ca9
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate issues remain in script lifecycle handling, runtime support, and documentation.
Review details
Suppressed comments (5)
docs/content/8.references/6.hub-api.md:134
- Please update the linked guide as well:
docs/content/1.guide/17.client-context.md:70still describesclientScriptas an iframe-only field and omits the JSON-render/renderer-backed cases and the trust/activation lifetime added here. The new reference row otherwise contradicts the guide users are sent to.
| `iframe` and renderer-backed dock entries | `clientScript` (optional) | inside the host page, after trust and before dock activation |
packages/hub-ui/src/client/state/context.ts:251
- Because
clientScriptnow lives onDevframeDockEntryBase, an action entry can declare both its activationactionand an independent page-levelclientScript. This condition skips every action entry before checking the common field, so that page script never initializes before activation even though the new field's contract says it does. Skip only the activation script (the explicitentry.clientScriptpassed below is still separate), or otherwise make this combination impossible in the type.
if (entry.type === '~builtin' || entry.type === 'action' || !entry.clientScript)
packages/hub-ui/src/client/state/context.ts:243
- When a custom-render entry has both
clientScriptandrenderer, an eager page-script failure is evicted from the cache, but this activation path calls onlyclientScriptOf(entry), which resolves torenderer. Opening the dock therefore never retries the failed page script despite the retry behavior promised byexecuteSetupScript; invoke the common page script separately before the renderer.
async function runDockSetupScript(entry: DevframeDockEntry): Promise<void> {
if (entry.type === '~builtin' || !clientScriptOf(entry))
return
await executeSetupScript(entry, scriptContext(entry))
packages/hub-ui/src/client/state/setup-script.ts:65
- The cache key does not encode the script's role. A custom-render entry may carry both a page-level
clientScriptand an activation-timerenderer; if those descriptors point to the same module/export, the page initialization stores this key and activation returns that promise without invoking the renderer. Include the page-versus-activation role (or use separate caches) so these two lifecycles cannot collide.
const key = JSON.stringify([entry.id, script?.importFrom, script?.importName ?? 'default'])
packages/hub/src/types/docks.ts:85
clientScriptis now permitted on every dock variant, including an entry that also hasactionorrenderer, but the headless runtime still resolves onlyaction ?? renderer ?? clientScript(packages/hub/src/client/host.ts:527). Such an entry therefore works inhub-uias two scripts but silently loses its page script forcreateDevframeClientRuntime; load both declared scripts there or constrain the public contract.
/** Page script initialized in the host page independently of dock activation or renderer type. */
clientScript?: ClientScriptEntry
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
I wonder if we should have ClientScriptEntry as |
1fd6ca9 to
6591da9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical trust-revocation gaps and activation reliability issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
docs/content/8.references/6.hub-api.md:134
- This table is the API's timing contract, but the iframe row says the script runs on first activation without qualifying the new
eager: truepath, which runs it after trust before activation. Please make the row conditional so readers do not infer that eager page scripts are still lazy.
| `iframe` | `clientScript` (optional) | inside the host page on first activation |
packages/hub-ui/src/client/state/context.ts:311
- The same stale-activation race exists here:
runPageScript(entry)can finish after a later iframe selection and then this call commits the older entry. Recheck the latest requested id/token after the await before settingselectedDockIdand opening the panel.
await runPageScript(entry)
packages/hub/src/client/host.ts:450
- Because this awaits page setup before committing
selectedId, two rapidswitchEntry()calls for different iframe docks can resolve out of order: the slower first setup will overwrite the selection made by the later click. Capture and validate an activation request (or serialize/cancel pending switches) after this await before mutating selection.
if (entry?.type === 'iframe' && entry.clientScript && loadScriptsEnabled && !await preparePageScript(entry))
return false
packages/hub/src/client/host.ts:610
- When trust is revoked during the dynamic import, this branch returns normally, so the promise stays cached as a successful setup. After trust is restored,
loadClientScripts()reuses that resolved promise and never invokes the eager script, leaving its commands/subscriptions uninitialized for the new trusted session. Invalidate the cached attempt on trust loss (or make it reject/retry) before allowing re-authentication to proceed.
if (!current || disposed || !rpc.isTrusted)
return
packages/hub/src/client/host.ts:622
- Rethrowing here makes activation failures retryable, but the host's existing fire-and-forget callers (
activateHandlerand theselectedIdsetter) callvoid switchEntry(...)without a rejection handler. A lazy script failure now produces an unhandled promise rejection whenever activation is triggered through those paths; preserve the rejection for awaited callers but catch/log it at each fire-and-forget call site.
throw error
- Files reviewed: 11/13 changed files
- Comments generated: 3
- Review effort level: Lite
|
Isn't this implementing the same as #376 ? |
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved lifecycle and error-handling issues remain around custom-render timing, stale descriptors, repeated action clicks, and unhandled rejections.
Review details
Suppressed comments (7)
docs/content/1.guide/17.client-context.md:70
- With
eager: true,clientScriptOfincludesrendererandstartPageScripts()invokes it before the entry is selected, so the unconditional statement that a custom-render renderer initializes after selection is inaccurate. Qualify this as the default/lazy behavior and note that eager custom-render scripts cannot assumecurrent.domElements.panelis mounted yet.
A client script is a `ClientScriptEntry`: `{ importFrom, importName?, eager? }`. `importName` defaults to `'default'` and `eager` defaults to `false`. An `iframe` entry's optional `clientScript` runs inside the host page when the dock entry is first activated. An `action` entry runs its `action` on each activation, while a `custom-render` entry initializes its `renderer` after selection so it can mount into the panel.
packages/hub-ui/src/client/state/context.ts:261
- The shared eager path also runs
custom-renderrenderers beforeViewCustomRendererhas mounted the panel, socurrent.domElements.panelis unavailable. Because activation reuses the cached setup promise, a renderer that mounts into that panel will not run again after selection. Keep custom-render execution activation-gated, or only preload its module eagerly and execute it once the panel exists.
if (!clientScriptOf(entry)?.eager)
continue
/** Setup reports failures and allows the next activation or publication to retry. */
void executeSetupScript(entry, scriptContext(entry), true).catch(() => {})
packages/hub-ui/src/client/state/setup-script.ts:40
- An entry can be updated or disposed while its eager import is pending, but this path only rechecks trust before invoking the function. The old descriptor can therefore run after a replacement has started its own setup (or after the dock was removed), leaving stale commands/subscriptions behind. Check that the current dock still carries this descriptor before calling
fn, and do not cache a skipped setup.
/** Trust may change while the module is loading; rejection keeps setup retryable. */
if (!context.rpc.isTrusted)
throw new Error('[@devframes/hub-ui] RPC client is no longer trusted')
await fn(context)
packages/hub-ui/src/client/state/setup-script.ts:54
- Although this cache bypass makes direct
executeSetupScriptcalls re-run action scripts, the reference UI's selected action button still routes a second click throughDockEntries.toggleDockEntry, which callsswitchEntry(null)instead of this activation path. The second click therefore closes the dock without executingaction, contrary to the documented “action clicks execute on every activation” behavior and preventing an immediate retry after a failure. Make a selected action dispatch its id rather than toggling closed, and cover repeated clicks.
/** Cache setup per RPC connection and dock; explicit action clicks always run again. */
packages/hub/src/client/host.ts:610
- If a dock is updated or replaced while this import is pending,
currentonly verifies that the id still exists, so the old descriptor can still invokefnagainst the replacement state. Since the replacement gets a different cache key, both scripts may run and leave stale commands or subscriptions installed. Before invoking the function, verify that the current entry still carries this descriptor (and treat a skipped setup as non-cacheable).
const current = entryToStateMap.get(entryId)
if (!current || disposed)
return
packages/hub/src/client/host.ts:565
- This eagerly executes a
custom-renderrenderer beforeViewCustomRendererhas mounted its panel, socurrent.domElements.panelis still unset. Activation then reuses the cached promise instead of running the renderer again, meaning a renderer that mounts into the panel never gets a usable container. Keep custom-render setup activation-gated, or separate eager module preloading from the panel-dependent execution.
else if (entry.type === 'custom-render')
startEagerScript(entry.id, entry.renderer)
packages/hub/src/client/host.ts:625
- Re-throwing here makes lazy import/setup failures reject
switchEntry(), but the runtime's internal fire-and-forget paths (activateHandlerand theselectedIdsetter) do not attach a catch. A failed script triggered through either path therefore becomes an unhandled rejection in addition to this logged error. Keep the rejection so failed cache entries can be retried, but handle it at those fire-and-forget call sites (and audit other internal activations).
throw error
- Files reviewed: 11/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Hello, indeed, the eager client-script part overlaps, I should have checked open PR before opening this one 😅 This PR is more targeted to just expanding script injection timing and the trusted race to solve a bug I hit in a downstream implementation. Would it make sense to split the fixes and address the narrower fix first ? If not we can close this one in favor of #376 indeed |
|
After running a small check, I think your PR seems to be better, it's also more focused, so I would say no, don't close this one in favor of #376 😄 |
|
I found only one bug not covered by the copilot review, I will send a PR to your branch |
There was a problem hiding this comment.
Found a bug in the playgrounds.
Adding eager: true fix it:
examples/custom-hub-vite/vite.config.ts:187action: { importFrom: 'demo-dock-client', eager: true },
examples/custom-hub-next/src/client/devframe/next-devframe-hub.ts:299action: { importFrom: demoDockClient.importFrom, eager: true },
To verify you can run pnpm run --filter custom-hub-vite dev and click on Client Script Demo on the sidebar, it will now add messages (right now it doesn't)
Background (Why)
An iframe dock's page script can register commands used by notifications before the user opens its panel. The reference hub UI waits for activation, leaving those commands unavailable until then.
Changes (What)
Add optional
eager: trueto the existingClientScriptEntrydescriptor, defaulting to lazy initialization as discussed in review. Both the reference hub UI and the headless runtime wait for RPC trust before eager initialization. Activation shares pending setup, and failed setup remains retryable. Cache setup per RPC connection and import descriptor; action clicks still execute each time.Keep the existing script fields:
clientScripton iframe docks,actionon action docks, andrendereron custom-render docks. The common dock base and JSON-render API are unchanged.Verification (Testing)
53 focused tests pass across the headless runtime, renderer fixtures, and reference UI context/script suites. Coverage includes opt-in eager initialization, default lazy behavior, trust gating, separate RPC connections, concurrent activation and setup failure/retry. Scoped hub/hub-ui type checks and changed-file lint pass.
The rebuilt assets at
51d41004were checked in the in-app Chromium browser before the subsequent trust-race correction. Embedded iframe setup remained lazy unless opted in; eager setup ran before panel activation, stayed deduplicated on activation, and registered a working page-local command. A standalone page initialized its own script instance and its command remained functional after closing the embedded page.Commit
91291de9adds six regression cases for trust revocation during dynamic import and while iframe setup completes. Both runtimes recheck trust before invocation/activation, and interrupted setup remains retryable after re-authentication. All 53 focused tests, changed-file lint and both package type checks pass. CI is running on this revision. The previous revision failed two Node 22plugin-code-serverdeclaration snapshots; that failure is retained pending the new run. The PR remains draft.