From d5cd73a9d9e248ec3a2cfc925593f5e8f81d0ffe Mon Sep 17 00:00:00 2001 From: imccyu <276526105+imccyu@users.noreply.github.com> Date: Fri, 24 Jul 2026 01:42:29 +0800 Subject: [PATCH] fix(gui): CI activation order + review-bot findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The session-title snapshot exposed a real activation race: ui-trajectory and ui-question register into conversation-declared slots but only injected 'slots', so nothing ordered their applies after ui-conversation's — register() into the undeclared slot threw and the entry FAILED. Both now inject 'conversation' as an ordering edge (documented as such; specs stub the service where the bench declares the slot itself). Review-bot findings, all three applied: the module loader's load sink cross-checks the handoff id against the arriving row (a mis-stamped bundle can no longer register under another entry's identity); the default execute seam removes the inline script node right after its synchronous execution (repeated HMR rebuilds no longer accumulate dead nodes); a throwing onRebuilt subscriber is contained per-listener and routed to onError instead of escaping the fs.watchFile callback. --- packages/client/modules/src/loader.ts | 16 ++++++++++++++++ packages/client/modules/tests/loader.spec.ts | 5 +++-- packages/client/ui-question/src/client/index.ts | 9 +++++++-- .../ui-question/tests/browser-plugin.spec.ts | 8 +++++++- .../client/ui-trajectory/src/client/index.ts | 10 ++++++++-- .../ui-trajectory/tests/client-bundle.spec.ts | 6 +++++- .../client/ui-trajectory/tests/views.spec.tsx | 3 +++ packages/host/webserver/src/web-plugins.ts | 10 +++++++++- 8 files changed, 58 insertions(+), 9 deletions(-) diff --git a/packages/client/modules/src/loader.ts b/packages/client/modules/src/loader.ts index 97e29bcf18..b2682a4bcd 100644 --- a/packages/client/modules/src/loader.ts +++ b/packages/client/modules/src/loader.ts @@ -29,6 +29,10 @@ const defaultExecuteBundle = (code: string, url: string): void => { // sourceURL comment keeps devtools stack frames attributed to the bundle. el.textContent = `${code}\n//# sourceURL=${url}` document.head.appendChild(el) + // Execution is synchronous for inline scripts: the factory is registered by + // now, so the node (and its source text) has no further job. Removing it + // keeps repeated HMR rebuilds from accumulating dead script nodes. + el.remove() } const urlOf = (row: WebBootEntry): string => { @@ -84,6 +88,10 @@ export class ClientModuleLoaderImpl implements ClientModuleLoader { // Execution URL of the bundle currently being executed (bound into the // factory registration so diagnostics can name the source). private executingUrl = '' + // Graph id of the row currently being executed ('' outside arrive): + // the load sink cross-checks the handoff id against it so a mis-stamped + // bundle cannot register under another entry's identity. + private executingId = '' private readonly fetchBundle: (url: string) => Promise private readonly executeBundle: (code: string, url: string) => void @@ -109,6 +117,12 @@ export class ClientModuleLoaderImpl implements ClientModuleLoader { // Registration is keyed by the handoff id; a duplicate means a bundle // executed twice without an invalidate — always a bug, always loud. if (this.factories.has(handoff.id)) throw new Error(`client-modules: duplicate factory registration for "${handoff.id}" (bundle executed twice without invalidate?)`) + // A fetched row's bundle must register the id its row names — a + // mis-stamped bundle registering under another entry's identity + // would let that entry silently materialize foreign exports. + if (this.executingId !== '' && handoff.id !== this.executingId) { + throw new Error(`client-modules: bundle ${this.executingUrl} registered "${handoff.id}" while arriving for "${this.executingId}" (mis-stamped bundle id)`) + } this.factories.set(handoff.id, { factory: handoff.factory, url: this.executingUrl }) }, } @@ -124,10 +138,12 @@ export class ClientModuleLoaderImpl implements ClientModuleLoader { const url = urlOf(row) const code = await this.fetchBundle(url) this.executingUrl = url + this.executingId = id try { this.executeBundle(code, url) } finally { this.executingUrl = '' + this.executingId = '' } if (!this.factories.has(id)) { throw new Error(`client-modules: bundle ${url} executed without registering "${id}" via __ModuleLoader__.load`) diff --git a/packages/client/modules/tests/loader.spec.ts b/packages/client/modules/tests/loader.spec.ts index ba1ae2de88..ccd001da6d 100644 --- a/packages/client/modules/tests/loader.spec.ts +++ b/packages/client/modules/tests/loader.spec.ts @@ -293,8 +293,9 @@ describe('default transport seams', () => { ;(document as unknown as Record).__realmBridge = win.__ModuleLoader__ const surface = await loader.import('dee', '', {}) expect((surface as { marker: string }).marker).toBe('via-script') - const script = [...document.querySelectorAll('script')].at(-1) - expect(script?.textContent).toContain('//# sourceURL=/plugins/dee/client.js?rev=0') + // The script node is removed right after its synchronous execution — + // repeated HMR rebuilds must not accumulate dead script nodes. + expect([...document.querySelectorAll('script')]).toEqual([]) }) it('a non-ok bundle response is loud with the status', async () => { diff --git a/packages/client/ui-question/src/client/index.ts b/packages/client/ui-question/src/client/index.ts index de8f481aad..328fa6c6ce 100644 --- a/packages/client/ui-question/src/client/index.ts +++ b/packages/client/ui-question/src/client/index.ts @@ -14,8 +14,13 @@ import { QuestionComposer } from './QuestionComposer.tsx' export { PendingQuestion } from './contract/slots.ts' export type { QuestionAnswer, QuestionComposerProps, QuestionWait } from './contract/slots.ts' -/** Required services (cordis fiber inject — the loader passes the whole export surface as an object plugin). */ -export const inject = ['slots'] +/** + * Required services (cordis fiber inject). 'conversation' is an ordering + * edge, not a call dependency: the 'conversation.composer' chain slot is + * declared by ui-conversation's apply, and register() into an undeclared + * slot throws — service waiting orders this apply after the declaring one. + */ +export const inject = ['slots', 'conversation'] /** Chain routing: claim the composer while a question wait is pending (pure — owner props only). */ function selectQuestion({ interactions }: ComposerChainProps): QuestionWait | null { diff --git a/packages/client/ui-question/tests/browser-plugin.spec.ts b/packages/client/ui-question/tests/browser-plugin.spec.ts index 1c4801b96a..09f71c9b5f 100644 --- a/packages/client/ui-question/tests/browser-plugin.spec.ts +++ b/packages/client/ui-question/tests/browser-plugin.spec.ts @@ -23,17 +23,23 @@ async function bench() { { name: 'root', children: { 'conversation.composer': { kind: 'chain', scope: 'session' } } } as never, () => null, ) + // 'conversation' inject is an ordering edge (the declaring plugin provides + // it after declaring the chain); the bench declares the chain itself. + ctx.provide('conversation', {}) return { ctx, slots } } describe('apply', () => { it('declares the services it binds', () => { - expect(inject).toEqual(['slots']) + expect(inject).toEqual(['slots', 'conversation']) }) it('fails loud when no live entry has declared the composer slot', async () => { const ctx = new Context() await ctx.plugin(SlotsService).await() + // Satisfy the ordering inject without declaring the chain: apply must + // then hit the undeclared-slot throw, not sit waiting on the service. + ctx.provide('conversation', {}) await expect(ctx.plugin({ inject: [...inject], apply })) .rejects.toThrow(/slot "conversation.composer" is not declared/) }) diff --git a/packages/client/ui-trajectory/src/client/index.ts b/packages/client/ui-trajectory/src/client/index.ts index 55b44857af..3979bfd91b 100644 --- a/packages/client/ui-trajectory/src/client/index.ts +++ b/packages/client/ui-trajectory/src/client/index.ts @@ -12,8 +12,14 @@ import type {} from '@deepseek-ai/dsh-client-ui-conversation/client' import { TrajectoryView } from './TrajectoryView.tsx' import { WaterfallView } from './WaterfallView.tsx' -/** Required services (cordis fiber inject — the loader passes the whole export surface as an object plugin). */ -export const inject = ['slots'] +/** + * Required services (cordis fiber inject). 'conversation' is an ordering + * edge, not a call dependency: the 'conversation.view' slot is declared by + * ui-conversation's apply (which then provides the service), and register() + * into an undeclared slot throws — service waiting is what orders this + * apply after the declaring one. + */ +export const inject = ['slots', 'conversation'] /** * Client plugin body: register the trajectory and waterfall view tabs. The diff --git a/packages/client/ui-trajectory/tests/client-bundle.spec.ts b/packages/client/ui-trajectory/tests/client-bundle.spec.ts index 4b317d9d84..edaf34a2cb 100644 --- a/packages/client/ui-trajectory/tests/client-bundle.spec.ts +++ b/packages/client/ui-trajectory/tests/client-bundle.spec.ts @@ -59,7 +59,7 @@ describe('tsdown client artifact', () => { const { handoff, surface } = await loadArtifact() expect(handoff.id).toBe(PLUGIN_ID) expect(surface.apply).toBeTypeOf('function') - expect(surface.inject).toEqual(['slots']) + expect(surface.inject).toEqual(['slots', 'conversation']) }) it.skipIf(code === undefined)('mounted as an object plugin, apply registers both view tabs on the real ring', async () => { @@ -71,6 +71,10 @@ describe('tsdown client artifact', () => { name: 'root', children: { 'conversation.view': { kind: 'list', scope: 'session' } }, }, (_p: { renderSlot?: unknown }) => null) + // The plugin injects 'conversation' as an ordering edge (the declaring + // plugin provides it after declaring the ring); the bench declares the + // ring itself, so a stub satisfies the wait. + ctx.provide('conversation', {}) const fiber = ctx.plugin(surface as { apply: (ctx: Context) => void }) await fiber.await() expect(slots.entries('conversation.view').map(e => e.options.id)).toEqual(['trajectory', 'waterfall']) diff --git a/packages/client/ui-trajectory/tests/views.spec.tsx b/packages/client/ui-trajectory/tests/views.spec.tsx index ab75da20b2..a3ac84738e 100644 --- a/packages/client/ui-trajectory/tests/views.spec.tsx +++ b/packages/client/ui-trajectory/tests/views.spec.tsx @@ -83,6 +83,9 @@ async function bench() { const chatBody = vi.fn(() =>
) slots.register( { name: 'conversation.view', id: 'chat', order: 0, label: 'Chat' } as never, chatBody as never) + // 'conversation' inject is an ordering edge; the bench declares the ring + // itself, so a stub satisfies the wait. + ctx.provide('conversation', {}) const fiber = ctx.plugin({ inject: [...inject], apply }) await fiber.await() return { ctx, slots, fiber } diff --git a/packages/host/webserver/src/web-plugins.ts b/packages/host/webserver/src/web-plugins.ts index 7998576f4e..9e32cfc012 100644 --- a/packages/host/webserver/src/web-plugins.ts +++ b/packages/host/webserver/src/web-plugins.ts @@ -247,7 +247,15 @@ export function createHostWebPluginRegistry(deps: WebPluginRegistryDeps): HostWe return } if (rev === undefined || rev === before) return - for (const notify of rebuildListeners) notify(id, rev) + for (const notify of rebuildListeners) { + // A throwing subscriber must not escape the fs.watchFile callback + // (that would skip later subscribers and can kill the process). + try { + notify(id, rev) + } catch (error) { + deps.onError(error instanceof Error ? error : new Error(String(error))) + } + } } watchFile(record.clientPath, { interval: watchInterval, persistent: false }, listener) watched.set(id, { path: record.clientPath, listener })