fix(gui): CI activation order + review-bot findings

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.
This commit is contained in:
imccyu
2026-07-24 01:42:29 +08:00
parent c8c43fb399
commit d5cd73a9d9
8 changed files with 58 additions and 9 deletions

View File

@@ -29,6 +29,10 @@ const defaultExecuteBundle = (code: string, url: string): void => {
// sourceURL comment keeps devtools stack frames attributed to the bundle. // sourceURL comment keeps devtools stack frames attributed to the bundle.
el.textContent = `${code}\n//# sourceURL=${url}` el.textContent = `${code}\n//# sourceURL=${url}`
document.head.appendChild(el) 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 => { 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 // Execution URL of the bundle currently being executed (bound into the
// factory registration so diagnostics can name the source). // factory registration so diagnostics can name the source).
private executingUrl = '' 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<string> private readonly fetchBundle: (url: string) => Promise<string>
private readonly executeBundle: (code: string, url: string) => void 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 // Registration is keyed by the handoff id; a duplicate means a bundle
// executed twice without an invalidate — always a bug, always loud. // 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?)`) 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 }) 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 url = urlOf(row)
const code = await this.fetchBundle(url) const code = await this.fetchBundle(url)
this.executingUrl = url this.executingUrl = url
this.executingId = id
try { try {
this.executeBundle(code, url) this.executeBundle(code, url)
} finally { } finally {
this.executingUrl = '' this.executingUrl = ''
this.executingId = ''
} }
if (!this.factories.has(id)) { if (!this.factories.has(id)) {
throw new Error(`client-modules: bundle ${url} executed without registering "${id}" via __ModuleLoader__.load`) throw new Error(`client-modules: bundle ${url} executed without registering "${id}" via __ModuleLoader__.load`)

View File

@@ -293,8 +293,9 @@ describe('default transport seams', () => {
;(document as unknown as Record<string, unknown>).__realmBridge = win.__ModuleLoader__ ;(document as unknown as Record<string, unknown>).__realmBridge = win.__ModuleLoader__
const surface = await loader.import('dee', '', {}) const surface = await loader.import('dee', '', {})
expect((surface as { marker: string }).marker).toBe('via-script') expect((surface as { marker: string }).marker).toBe('via-script')
const script = [...document.querySelectorAll('script')].at(-1) // The script node is removed right after its synchronous execution —
expect(script?.textContent).toContain('//# sourceURL=/plugins/dee/client.js?rev=0') // 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 () => { it('a non-ok bundle response is loud with the status', async () => {

View File

@@ -14,8 +14,13 @@ import { QuestionComposer } from './QuestionComposer.tsx'
export { PendingQuestion } from './contract/slots.ts' export { PendingQuestion } from './contract/slots.ts'
export type { QuestionAnswer, QuestionComposerProps, QuestionWait } 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). */ /** Chain routing: claim the composer while a question wait is pending (pure — owner props only). */
function selectQuestion({ interactions }: ComposerChainProps): QuestionWait | null { function selectQuestion({ interactions }: ComposerChainProps): QuestionWait | null {

View File

@@ -23,17 +23,23 @@ async function bench() {
{ name: 'root', children: { 'conversation.composer': { kind: 'chain', scope: 'session' } } } as never, { name: 'root', children: { 'conversation.composer': { kind: 'chain', scope: 'session' } } } as never,
() => null, () => 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 } return { ctx, slots }
} }
describe('apply', () => { describe('apply', () => {
it('declares the services it binds', () => { 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 () => { it('fails loud when no live entry has declared the composer slot', async () => {
const ctx = new Context() const ctx = new Context()
await ctx.plugin(SlotsService).await() 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 })) await expect(ctx.plugin({ inject: [...inject], apply }))
.rejects.toThrow(/slot "conversation.composer" is not declared/) .rejects.toThrow(/slot "conversation.composer" is not declared/)
}) })

View File

@@ -12,8 +12,14 @@ import type {} from '@deepseek-ai/dsh-client-ui-conversation/client'
import { TrajectoryView } from './TrajectoryView.tsx' import { TrajectoryView } from './TrajectoryView.tsx'
import { WaterfallView } from './WaterfallView.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 * Client plugin body: register the trajectory and waterfall view tabs. The

View File

@@ -59,7 +59,7 @@ describe('tsdown client artifact', () => {
const { handoff, surface } = await loadArtifact() const { handoff, surface } = await loadArtifact()
expect(handoff.id).toBe(PLUGIN_ID) expect(handoff.id).toBe(PLUGIN_ID)
expect(surface.apply).toBeTypeOf('function') 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 () => { 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', name: 'root',
children: { 'conversation.view': { kind: 'list', scope: 'session' } }, children: { 'conversation.view': { kind: 'list', scope: 'session' } },
}, (_p: { renderSlot?: unknown }) => null) }, (_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 }) const fiber = ctx.plugin(surface as { apply: (ctx: Context) => void })
await fiber.await() await fiber.await()
expect(slots.entries('conversation.view').map(e => e.options.id)).toEqual(['trajectory', 'waterfall']) expect(slots.entries('conversation.view').map(e => e.options.id)).toEqual(['trajectory', 'waterfall'])

View File

@@ -83,6 +83,9 @@ async function bench() {
const chatBody = vi.fn(() => <div data-testid="chat-body" />) const chatBody = vi.fn(() => <div data-testid="chat-body" />)
slots.register( slots.register(
{ name: 'conversation.view', id: 'chat', order: 0, label: 'Chat' } as never, chatBody as never) { 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 }) const fiber = ctx.plugin({ inject: [...inject], apply })
await fiber.await() await fiber.await()
return { ctx, slots, fiber } return { ctx, slots, fiber }

View File

@@ -247,7 +247,15 @@ export function createHostWebPluginRegistry(deps: WebPluginRegistryDeps): HostWe
return return
} }
if (rev === undefined || rev === before) 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) watchFile(record.clientPath, { interval: watchInterval, persistent: false }, listener)
watched.set(id, { path: record.clientPath, listener }) watched.set(id, { path: record.clientPath, listener })