diff --git a/src/client/native/index.ts b/src/client/native/index.ts index c126034..aa70b08 100644 --- a/src/client/native/index.ts +++ b/src/client/native/index.ts @@ -156,6 +156,21 @@ interface Registration { readonly dispose: () => void } +/** + * Run one registration disposer, reporting a failure instead of propagating + * it: releasing a registration must never mask the error being handled, and + * one broken entry must not leave the others registered. + * @param dispose - the disposer to run. + * @param what - the registration's name, for the log line. + */ +function disposeSafely(dispose: () => void, what: string): void { + try { + dispose() + } catch (error) { + console.error(`[dsh-better-sidebar] ${what} release failed:`, error) + } +} + /** Everything the registrations need. */ export interface NativeSurfaceDeps { readonly ctx: Context @@ -208,23 +223,44 @@ export function registerNativeSurface(deps: NativeSurfaceDeps): () => void { return address !== undefined && address.scope === 'session' ? address.sessionId : undefined } - /** Register the body + chip-title slots for one native implementation id. */ + /** + * Register the body + chip-title slots for one native implementation id. + * + * A slot registration can fail long after the TYPE registration succeeded + * (`ctx.effect` on a context that is already inactive, i.e. during a + * plugin reload/disposal), so the disposers are collected as they are + * handed out and the partial set is released on the way out: the caller + * must be able to roll its type registration back WITHOUT losing track of + * a slot that did register. + */ const registerSlots = ( id: string, injected: Omit, params: Pick, - ): Array<() => void> => [ - ctx.slots.inject('sidebar.right.pane.tab', () => ctx.slots.register({ - name: 'sidebar.right.pane.tab', - key: id, - inject: (sessionId: string) => ({ ...injected, ...params, sessionId }), - }, NativeTabBody)), - ctx.slots.inject('sidebar.right.pane.tab.title', () => ctx.slots.register({ - name: 'sidebar.right.pane.tab.title', - key: id, - inject: () => ({ records, service, descriptorId: injected.descriptorId }), - }, NativeTabTitle)), - ] + ): Array<() => void> => { + // Collected AS THEY ARE HANDED OUT: a slot registration can fail long + // after the TYPE registration succeeded (a plugin reload makes + // `ctx.slots.inject` throw on the now-inactive context), and the caller + // must then be able to release the type again WITHOUT losing track of a + // slot that did register. + const disposers: Array<() => void> = [] + try { + disposers.push(ctx.slots.inject('sidebar.right.pane.tab', () => ctx.slots.register({ + name: 'sidebar.right.pane.tab', + key: id, + inject: (sessionId: string) => ({ ...injected, ...params, sessionId }), + }, NativeTabBody))) + disposers.push(ctx.slots.inject('sidebar.right.pane.tab.title', () => ctx.slots.register({ + name: 'sidebar.right.pane.tab.title', + key: id, + inject: () => ({ records, service, descriptorId: injected.descriptorId }), + }, NativeTabTitle))) + } catch (error) { + for (const dispose of disposers) disposeSafely(dispose, `native tab slot "${id}"`) + throw error + } + return disposers + } /** One descriptor's native type + body + title, as one disposable. */ const registerDescriptor = (descriptor: TabDescriptor): (() => void) => { @@ -271,14 +307,25 @@ export function registerNativeSurface(deps: NativeSurfaceDeps): () => void { }], }), }) - const slots = registerSlots( - id, - { ctx, store, service, records, descriptorId: descriptor.id }, - isEditor ? { paramsOf: fileParamsOf, sessionIdOf: fileSessionIdOf } : {}, - ) + // The type is in the host's registry the moment `tabs.register` + // returns; a slot failure must release it here, or the id stays taken + // for the rest of the page's life (the host refuses a second + // registration of the same id) and the tab kind renders the host's + // "nothing can view this" face forever. + let slots: Array<() => void> + try { + slots = registerSlots( + id, + { ctx, store, service, records, descriptorId: descriptor.id }, + isEditor ? { paramsOf: fileParamsOf, sessionIdOf: fileSessionIdOf } : {}, + ) + } catch (error) { + disposeSafely(disposeType, `native tab type "${id}"`) + throw error + } return () => { - for (const dispose of slots.reverse()) dispose() - disposeType() + for (const dispose of slots.reverse()) disposeSafely(dispose, `native tab slot "${id}"`) + disposeSafely(disposeType, `native tab type "${id}"`) } } @@ -304,10 +351,16 @@ export function registerNativeSurface(deps: NativeSurfaceDeps): () => void { ...guideIconOf(editor?.icon), }], }) - const slots = registerSlots(id, { ctx, store, service, records, descriptorId: EDITOR_KIND }, {}) + let slots: Array<() => void> + try { + slots = registerSlots(id, { ctx, store, service, records, descriptorId: EDITOR_KIND }, {}) + } catch (error) { + disposeSafely(disposeType, `native tab type "${id}"`) + throw error + } return () => { - for (const dispose of slots.reverse()) dispose() - disposeType() + for (const dispose of slots.reverse()) disposeSafely(dispose, `native tab slot "${id}"`) + disposeSafely(disposeType, `native tab type "${id}"`) } } @@ -319,14 +372,23 @@ export function registerNativeSurface(deps: NativeSurfaceDeps): () => void { wanted.set(descriptor.id, () => registerDescriptor(descriptor)) } for (const [descriptorId, registration] of live) { - if (wanted.has(descriptorId)) continue - registration.dispose() + // `files` is this module's OWN takeover, never a descriptor: it is + // owned by the editor-type switch below. Re-creating it here on every + // notification used to put its host-side type through a + // tear-down/re-register window on each store commit — and a + // re-registration attempted on an already-inactive context (a plugin + // reload) left the type registered with nobody holding its disposer, + // so the same id could never be registered again for the rest of the + // page's life. + if (descriptorId === FILES_KIND || wanted.has(descriptorId)) continue + disposeSafely(() => registration.dispose(), `native tab type "${descriptorId}"`) live.delete(descriptorId) } for (const [descriptorId, create] of wanted) { if (live.has(descriptorId)) continue // One descriptor must not take the rest of the surface down with it: - // report and continue so the remaining types still register. + // report and continue so the remaining types still register. `create` + // has already released whatever it managed to install. try { live.set(descriptorId, { dispose: create() }) } catch (error) { @@ -345,17 +407,20 @@ export function registerNativeSurface(deps: NativeSurfaceDeps): () => void { } } if (!wantsFiles && hasFiles) { - live.get(FILES_KIND)?.dispose() + const takeover = live.get(FILES_KIND) live.delete(FILES_KIND) + if (takeover !== undefined) disposeSafely(() => takeover.dispose(), 'the native "files" takeover') } } const disposeSubscriptions = [service.subscribe(sync), store.subscribe(sync)] sync() return () => { - for (const registration of live.values()) registration.dispose() + for (const registration of live.values()) { + disposeSafely(() => registration.dispose(), 'a native tab registration') + } live.clear() - for (const dispose of disposeSubscriptions.reverse()) dispose() + for (const dispose of disposeSubscriptions.reverse()) disposeSafely(dispose, 'a native tab subscription') } }) return () => { diff --git a/tests/native-registration.spec.ts b/tests/native-registration.spec.ts new file mode 100644 index 0000000..33d2b5c --- /dev/null +++ b/tests/native-registration.spec.ts @@ -0,0 +1,197 @@ +// @vitest-environment jsdom +/** + * The native tab registration LIFECYCLE (`src/client/native/index.ts`): what a + * store/service notification may do to the host's tab-type registry, and what + * a failed registration is allowed to leave behind. + * + * The failure this locks is a plugin RELOAD: cordis tears the fiber down, each + * built-in descriptor is unregistered one by one, every unregistration notifies + * the plugin's service, and `sync()` therefore runs again on a context that is + * already inactive. A registration attempted there throws + * (`ctx.effect` on an inactive context) — and if the host-side type was already + * registered at that point, its disposer is lost, the id stays taken for the + * rest of the page's life, and every later attempt is refused with + * `tab type id "…" is already registered`. The kind then renders the host's + * "nothing here can view this kind of content" face until a page refresh. + * + * Two properties keep that impossible, and both are asserted here: + * 1. an unrelated notification never re-creates the `files` takeover (the + * tear-down/re-register window the reload used to walk into); + * 2. a registration that fails after its type was registered releases the + * type again, so the id stays registrable and one broken descriptor cannot + * take the others down with it. + */ +import { describe, expect, it, vi } from 'vitest' +import { registerNativeSurface } from '../src/client/native/index.ts' +import { createNativeTabRecords } from '../src/client/native/tab-adapter.tsx' +import { createBetterSidebarService } from '../src/client/service.ts' +import { createSidebarStore } from '../src/client/state.ts' + +/** + * The fake `sidebarRightTabs`, mirroring the host's two hard rules: one + * registration per id, and the returned disposer is the only way to free it. + */ +function createRegistry() { + const registered = new Map void>() + const events: string[] = [] + let failReleaseOf: string | undefined + return { + registered, + events, + failRelease(id: string): void { failReleaseOf = id }, + register(definition: { id: string }): () => void { + if (registered.has(definition.id)) { + throw new Error(`sidebarRight: tab type id "${definition.id}" is already registered`) + } + events.push(`register ${definition.id}`) + const dispose = (): void => { + if (failReleaseOf === definition.id) throw new Error(`cannot release ${definition.id}`) + registered.delete(definition.id) + events.push(`release ${definition.id}`) + } + registered.set(definition.id, dispose) + return dispose + }, + } +} + +/** + * A fake client context: `slots.inject` runs its callback synchronously (the + * real one resolves the declaration and calls `ctx.effect`, which throws while + * `inactive`), `slots.register` throws for a key listed in `failKeys`, and + * `inject` hands back the registry while honouring the callback's own teardown. + */ +function createHost() { + const registry = createRegistry() + // One mutable state object the ctx closures read: the test flips it after + // the host was built. + const state = { + /** True while the plugin's context is being torn down (the reload case). */ + inactive: false, + /** Native ids whose slot registration fails. */ + failKeys: new Set(), + } + const ctx = { + inject: ( + _deps: readonly string[], + callback: (injected: { get: (name: string) => unknown }) => (() => void) | void, + ) => { + const teardown = callback({ get: () => registry }) + return { dispose: () => { if (typeof teardown === 'function') teardown() } } + }, + get: () => registry, + slots: { + inject: (_name: string, callback: () => Array<() => void> | (() => void)) => { + if (state.inactive) throw new Error('cannot create effect on inactive context') + return typeof callback === 'function' ? callback() : () => {} + }, + register: (options: { key?: string }) => { + if (options.key !== undefined && state.failKeys.has(options.key)) { + throw new Error(`slot registration for "${options.key}" failed`) + } + return () => {} + }, + }, + } + return { registry, state, ctx } +} + +/** A store + service with two enabled descriptors, plus the record registry. */ +function createPlugins() { + const store = createSidebarStore() + store.setSession('s1') + const service = createBetterSidebarService(store) + service.registerTab({ id: 'editor', title: 'Files', component: () => null, icon: () => null }) + service.registerTab({ id: 'git', title: 'Changes', component: () => null }) + return { store, service, records: createNativeTabRecords() } +} + +/** Silence (and count) the failure logs the fixed code reports on purpose. */ +function silenceFailures(): { restore: () => void; messages: string[] } { + const messages: string[] = [] + const spy = vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + messages.push(args.map(String).join(' ')) + }) + return { restore: () => { spy.mockRestore() }, messages } +} + +describe('native tab registration lifecycle', () => { + it('releases a type whose slot registration failed, and can register it again later', () => { + const host = createHost() + const { store, service, records } = createPlugins() + const log = silenceFailures() + try { + // The reload: the context goes inactive while notifications are still + // arriving, so every slot registration fails after its type registered. + host.state.inactive = true + expect(() => registerNativeSurface({ ctx: host.ctx as never, store, service, records })).not.toThrow() + expect([...host.registry.registered.keys()], 'a failed registration leaves no type behind').toEqual([]) + + // The context comes back (the reload's next activation): the same ids + // must be registrable — no "already registered" from the orphan. + host.state.inactive = false + host.state.failKeys.clear() + store.setPrefs(store.getPrefs()) + expect([...host.registry.registered.keys()].sort()).toEqual([ + 'dsh-better-sidebar:editor', + 'dsh-better-sidebar:files', + 'dsh-better-sidebar:git', + ]) + } finally { + log.restore() + } + }) + + it('never re-creates the "files" takeover on an unrelated notification', () => { + const host = createHost() + const { store, service, records } = createPlugins() + const dispose = registerNativeSurface({ ctx: host.ctx as never, store, service, records }) + // The takeover is this module's own entry (never a descriptor), so no + // store/service notification has any business touching it. Before the fix + // every notification disposed and re-created it. + host.registry.events.length = 0 + for (let round = 0; round < 3; round++) store.setPrefs(store.getPrefs()) + expect(host.registry.events, 'no notification may tear the "files" takeover down').toEqual([]) + expect(host.registry.registered.has('dsh-better-sidebar:files')).toBe(true) + dispose() + }) + + it('keeps the other tab types when one descriptor cannot register', () => { + const host = createHost() + const { store, service, records } = createPlugins() + host.state.failKeys.add('dsh-better-sidebar:git') + const log = silenceFailures() + try { + expect(() => registerNativeSurface({ ctx: host.ctx as never, store, service, records })).not.toThrow() + expect([...host.registry.registered.keys()].sort()).toEqual([ + 'dsh-better-sidebar:editor', + 'dsh-better-sidebar:files', + ]) + // The broken descriptor is retried on the next sync instead of being + // written off, and the retry succeeds once it can register. + host.state.failKeys.clear() + store.setPrefs(store.getPrefs()) + expect([...host.registry.registered.keys()].sort()).toEqual([ + 'dsh-better-sidebar:editor', + 'dsh-better-sidebar:files', + 'dsh-better-sidebar:git', + ]) + } finally { + log.restore() + } + }) + + it('releases every registration on teardown even when one release throws', () => { + const host = createHost() + const { store, service, records } = createPlugins() + const log = silenceFailures() + try { + const dispose = registerNativeSurface({ ctx: host.ctx as never, store, service, records }) + host.registry.failRelease('dsh-better-sidebar:editor') + expect(() => dispose()).not.toThrow() + expect([...host.registry.registered.keys()]).toEqual(['dsh-better-sidebar:editor']) + } finally { + log.restore() + } + }) +})