Skip to content

fix(hub-ui): initialize dock page scripts before activation - #387

Open
dvcolomban wants to merge 4 commits into
devframes:mainfrom
dvcolomban:dvcol/dock-page-scripts
Open

fix(hub-ui): initialize dock page scripts before activation#387
dvcolomban wants to merge 4 commits into
devframes:mainfrom
dvcolomban:dvcol/dock-page-scripts

Conversation

@dvcolomban

@dvcolomban dvcolomban commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

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: true to the existing ClientScriptEntry descriptor, 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: clientScript on iframe docks, action on action docks, and renderer on 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 51d41004 were 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 91291de9 adds 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 22 plugin-code-server declaration snapshots; that failure is retained pending the new run. The PR remains draft.

Copilot AI lite review requested due to automatic review settings September 13, 2026 23:58
@coldtea-pr-lens

coldtea-pr-lens Bot commented Sep 13, 2026

Copy link
Copy Markdown

◈ PR Lens

🟢 +0 new · 🟠 ~7 changed · 🔴 -0 removed · 2 flows · 5 files · commit 91291de


Architecture

Architecture diagram for devframes/devframe at 91291de

7 components touched across 3 lanes.

Open the interactive canvas


Inside the changed components — 2 views

Component view — Hub UI script lifecycle

Internal script coordination, trust gating, and per-RPC caching inside Hub UI

Architecture view of Component view — Hub UI script lifecycle in devframes/devframe

Component view — Headless runtime scripts

Client script initialization, trust verification, and caching in createDevframeClientRuntime

Architecture view of Component view — Headless runtime scripts in devframes/devframe

Data flow

Data flow diagram for devframes/devframe at 91291de

Initializing eager dock scripts on RPC trust · Activating dock entry and running page script

Open the interactive canvas


The other flows — 1 sequence

Activating dock entry and running page script

Sequence diagram of Activating dock entry and running page script in devframes/devframe

View

  • Architecture lens
  • Data flow lens
  • Expand every detail

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
  • Run npx skills add coldteadotai/pr-lens, then tell your coding agent: "Diagram the change you just made with PR Lens and attach it to the pull request."
  • Run npx @coldtea/pr-lens-cli analyze --base origin/main on a branch, then npx @coldtea/pr-lens-cli render .pr-lens/graph.json. Same lenses, your own model key, before the pull request exists.
  • Click the link under each diagram to open it on a canvas you can zoom, pan and step through.
  • The diagrams are links. Click one to open it on the canvas, then press W or click play to walk through the change.
  • Open a diagram on the canvas, then press W or click play to walk through the change one step at a time.
  • The CLI's render reads .github/pr-lens.yml and applies your renames, exclusions and lane pins at draw time.
  • Set github.comment.collapsed: true in .github/pr-lens.yml to fold the comment behind one View architecture and data flow row. Drawing still runs on every push.
  • Add .github/workflows/pr-lens.yml with coldteadotai/pr-lens/packages/action@v0 and your model provider's key as its api-key to run PR Lens from your own CI. Any /chat/completions endpoint works.
  • Push a commit and the comment redraws for the new head. A slow older run never overwrites a newer one.
  • Switch GitHub to dark mode and the diagrams follow. The moving dots are this pull request's data in motion.

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.

❤️ Share

@vercel

vercel Bot commented Sep 13, 2026

Copy link
Copy Markdown

@dvcolomban is attempting to deploy a commit to the NuxtLabs Team on Vercel.

A member of the Team first needs to authorize it.

Copilot AI left a comment

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.

🟡 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 clientScript to 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 page clientScript plus 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.

Comment thread packages/hub-ui/src/client/state/setup-script.ts Outdated
Comment thread packages/hub/src/types/docks.ts Outdated
Comment thread packages/hub-ui/src/client/state/context.ts Outdated
Comment thread docs/content/8.references/6.hub-api.md Outdated

Copilot AI left a comment

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.

🔵 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:70 still describes clientScript as 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 clientScript now lives on DevframeDockEntryBase, an action entry can declare both its activation action and an independent page-level clientScript. 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 explicit entry.clientScript passed 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 clientScript and renderer, an eager page-script failure is evicted from the cache, but this activation path calls only clientScriptOf(entry), which resolves to renderer. Opening the dock therefore never retries the failed page script despite the retry behavior promised by executeSetupScript; 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 clientScript and an activation-time renderer; 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

  • clientScript is now permitted on every dock variant, including an entry that also has action or renderer, but the headless runtime still resolves only action ?? renderer ?? clientScript (packages/hub/src/client/host.ts:527). Such an entry therefore works in hub-ui as two scripts but silently loses its page script for createDevframeClientRuntime; 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

@antfu

antfu commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

I wonder if we should have ClientScriptEntry as { importFrom, importName?, eager?: boolean }, which defaults to lazy (eager: false), for people to opt in to eager: true if they want to initialize the script immediately. WDYT?

@dvcolomban
dvcolomban marked this pull request as draft September 14, 2026 07:02
@dvcolomban
dvcolomban force-pushed the dvcol/dock-page-scripts branch from 1fd6ca9 to 6591da9 Compare September 14, 2026 09:27
@dvcolomban
dvcolomban marked this pull request as ready for review September 14, 2026 13:14
Copilot AI review requested due to automatic review settings September 14, 2026 13:14

Copilot AI left a comment

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.

🟡 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: true path, 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 setting selectedDockId and opening the panel.
    await runPageScript(entry)

packages/hub/src/client/host.ts:450

  • Because this awaits page setup before committing selectedId, two rapid switchEntry() 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 (activateHandler and the selectedId setter) call void 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

Comment thread packages/hub-ui/src/client/state/context.ts
Comment thread packages/hub-ui/src/client/state/setup-script.ts
Comment thread packages/hub/src/client/host.ts Outdated
Copilot AI review requested due to automatic review settings September 14, 2026 13:29
@posva

posva commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Isn't this implementing the same as #376 ?

Copilot AI left a comment

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.

🔵 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, clientScriptOf includes renderer and startPageScripts() 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 assume current.domElements.panel is 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-render renderers before ViewCustomRenderer has mounted the panel, so current.domElements.panel is 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 executeSetupScript calls re-run action scripts, the reference UI's selected action button still routes a second click through DockEntries.toggleDockEntry, which calls switchEntry(null) instead of this activation path. The second click therefore closes the dock without executing action, 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, current only verifies that the id still exists, so the old descriptor can still invoke fn against 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-render renderer before ViewCustomRenderer has mounted its panel, so current.domElements.panel is 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 (activateHandler and the selectedId setter) 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

@dvcolomban

Copy link
Copy Markdown
Contributor Author

@posva

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

@posva

posva commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

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 think I could base that other PR on this one once it gets merged

@posva

posva commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

I found only one bug not covered by the copilot review, I will send a PR to your branch

@posva posva left a comment

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.

Found a bug in the playgrounds.
Adding eager: true fix it:

  • examples/custom-hub-vite/vite.config.ts:187
    action: { importFrom: 'demo-dock-client', eager: true },
  • examples/custom-hub-next/src/client/devframe/next-devframe-hub.ts:299
    action: { 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)

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.

4 participants