From 9256023b7a0314fe64069cc75e350c8e638807fa Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Thu, 16 Jul 2026 21:21:52 +0100 Subject: [PATCH 1/6] feat(core): report missing modules when an option or api needs one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setting an enterprise option on the free package did nothing and said nothing: the service slot is empty, every call site is `?.`, so it short-circuits silently. A user could not tell a broken feature from a missing package. Report when intent is declared — an option set, or an api command called: dockview: `overflow.mode: 'wrap'` requires the "MultiRowTabs" module, which ships in dockview-enterprise. npm install dockview-enterprise import 'dockview-enterprise'; Interaction stays silent. A right-click reaching an absent ContextMenu module doesn't mean the app wanted one, and the `?.` call site can't tell a missing module from a feature nobody asked for. - optionsModules.ts: rules table mapping options to the modules that implement them, validated at construction and on updateOptions. A flat list rather than a keyed record because options nest and gate on values — `overflow.mode: 'wrap'` needs MultiRowTabs while `overflow.search` needs AdvancedOverflow, and the message names the exact path. Reasons are grouped so each missing module reports once however many of its options are set. - modules.ts: ENTERPRISE_MODULE_NAMES + one message builder, so core modules never mention the paid package and enterprise ones always carry the fix. assertModule delegates to it. - Guard commands, not queries: api.undo() asked for something to happen; canUndo asked a question and `false` is a truthful answer. - addEdgeGroup keeps throwing (its return type is non-optional, so there is no no-op to degrade to) but now raises the same message instead of logging one and throwing a thinner second one. - enterpriseModuleNames.spec: fails if ENTERPRISE_MODULE_NAMES drifts from dockview-enterprise's Modules in either direction. Core can't import enterprise to derive the list, so this test is the only thing holding the mirror in sync. Validation reads the options the caller passed, never the merged set: every key of DockviewOptions is present as `undefined` on `this.options`, so a presence test there would fire for every rule. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/__tests__/dockview/modules.spec.ts | 42 +++- .../__tests__/dockview/optionsModules.spec.ts | 190 ++++++++++++++++++ .../src/dockview/dockviewComponent.ts | 71 +++++-- .../dockview-core/src/dockview/modules.ts | 111 ++++++++-- .../src/dockview/optionsModules.ts | 180 +++++++++++++++++ .../__tests__/enterpriseModuleNames.spec.ts | 46 +++++ 6 files changed, 608 insertions(+), 32 deletions(-) create mode 100644 packages/dockview-core/src/__tests__/dockview/optionsModules.spec.ts create mode 100644 packages/dockview-core/src/dockview/optionsModules.ts create mode 100644 packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts diff --git a/packages/dockview-core/src/__tests__/dockview/modules.spec.ts b/packages/dockview-core/src/__tests__/dockview/modules.spec.ts index 3814c86e4f..e527c7ce4c 100644 --- a/packages/dockview-core/src/__tests__/dockview/modules.spec.ts +++ b/packages/dockview-core/src/__tests__/dockview/modules.spec.ts @@ -3,6 +3,7 @@ import { ModuleRegistry, _resetMissingModuleWarnings, assertModule, + missingModuleMessage, } from '../../dockview/modules'; describe('ModuleRegistry', () => { @@ -135,11 +136,44 @@ describe('assertModule', () => { assertModule(undefined, 'PopoutWindow', 'api.addPopoutGroup'); assertModule(undefined, 'PopoutWindow', 'api.popoutRestorationPromise'); expect(consoleError).toHaveBeenCalledTimes(2); - expect(consoleError.mock.calls[0][0]).toMatch( - /for api\.addPopoutGroup/ - ); + expect(consoleError.mock.calls[0][0]).toMatch(/`api\.addPopoutGroup`/); expect(consoleError.mock.calls[1][0]).toMatch( - /for api\.popoutRestorationPromise/ + /`api\.popoutRestorationPromise`/ + ); + }); + + test('names dockview-enterprise and the fix for an enterprise module', () => { + assertModule(undefined, 'SmartGuides', 'smartGuides'); + const message = consoleError.mock.calls[0][0]; + expect(message).toMatch(/ships in dockview-enterprise/); + expect(message).toMatch(/npm install dockview-enterprise/); + expect(message).toMatch(/import 'dockview-enterprise'/); + }); + + test('does not mention enterprise for a core module', () => { + assertModule(undefined, 'PopoutWindow', 'api.addPopoutGroup'); + expect(consoleError.mock.calls[0][0]).not.toMatch(/enterprise/i); + }); +}); + +describe('missingModuleMessage', () => { + test('is the same text assertModule logs, for callers that must throw', () => { + const consoleError = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + _resetMissingModuleWarnings(); + + assertModule(undefined, 'EdgeGroup', 'api.addEdgeGroup'); + expect(consoleError.mock.calls[0][0]).toBe( + missingModuleMessage('EdgeGroup', 'api.addEdgeGroup') ); + + consoleError.mockRestore(); + }); + + test('names the module and the reason', () => { + const message = missingModuleMessage('EdgeGroup', 'api.addEdgeGroup'); + expect(message).toMatch(/EdgeGroup/); + expect(message).toMatch(/`api\.addEdgeGroup`/); }); }); diff --git a/packages/dockview-core/src/__tests__/dockview/optionsModules.spec.ts b/packages/dockview-core/src/__tests__/dockview/optionsModules.spec.ts new file mode 100644 index 0000000000..fac5f0e062 --- /dev/null +++ b/packages/dockview-core/src/__tests__/dockview/optionsModules.spec.ts @@ -0,0 +1,190 @@ +import { DockviewComponentOptions } from '../../dockview/options'; +import { + OPTION_MODULE_RULES, + validateOptionModules, +} from '../../dockview/optionsModules'; +import { + ENTERPRISE_MODULE_NAMES, + _resetMissingModuleWarnings, +} from '../../dockview/modules'; +import { AllModules } from '../../dockview/allModules'; + +const nothingRegistered = () => false; +const everythingRegistered = () => true; + +function options(o: Partial) { + return o; +} + +describe('validateOptionModules', () => { + let consoleError: jest.SpyInstance; + + beforeEach(() => { + _resetMissingModuleWarnings(); + consoleError = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + }); + + afterEach(() => { + consoleError.mockRestore(); + }); + + test('says nothing for options that request no module', () => { + validateOptionModules( + options({ theme: undefined, scrollbars: 'custom' }), + nothingRegistered + ); + expect(consoleError).not.toHaveBeenCalled(); + }); + + test('says nothing when the module is registered', () => { + validateOptionModules( + options({ overflow: { mode: 'wrap' } }), + everythingRegistered + ); + expect(consoleError).not.toHaveBeenCalled(); + }); + + test('reports the module and the exact option path that needed it', () => { + validateOptionModules( + options({ overflow: { mode: 'wrap' } }), + nothingRegistered + ); + expect(consoleError).toHaveBeenCalledTimes(1); + const message = consoleError.mock.calls[0][0]; + expect(message).toMatch(/overflow\.mode: 'wrap'/); + expect(message).toMatch(/MultiRowTabs/); + expect(message).toMatch(/npm install dockview-enterprise/); + }); + + test('one option key can require different modules by value', () => { + validateOptionModules( + options({ overflow: { mode: 'wrap', search: true } }), + nothingRegistered + ); + const messages = consoleError.mock.calls.map((c) => c[0]).join('\n'); + expect(messages).toMatch(/MultiRowTabs/); + expect(messages).toMatch(/AdvancedOverflow/); + }); + + test('one message per missing module, however many of its options are set', () => { + validateOptionModules( + options({ + overflow: { mode: 'wrap', maxRows: 3, search: true, mru: true }, + }), + nothingRegistered + ); + + // Two modules missing, four options set: two messages, not four. + expect(consoleError).toHaveBeenCalledTimes(2); + + const multiRow = consoleError.mock.calls + .map((c) => c[0] as string) + .find((m) => m.includes('MultiRowTabs'))!; + expect(multiRow).toMatch(/overflow\.mode: 'wrap'/); + expect(multiRow).toMatch(/overflow\.maxRows/); + // Grammar tracks the count, and the fix is stated once. + expect(multiRow).toMatch(/require the "MultiRowTabs" module/); + expect(multiRow.match(/npm install/g)).toHaveLength(1); + }); + + test('a single reason still reads as singular', () => { + validateOptionModules( + options({ overflow: { mode: 'wrap' } }), + nothingRegistered + ); + expect(consoleError.mock.calls[0][0]).toMatch( + /requires the "MultiRowTabs" module/ + ); + }); + + test('dockToEdgeGroups requires AutoEdgeGroup — a paid feature end to end', () => { + validateOptionModules( + options({ dockToEdgeGroups: true }), + nothingRegistered + ); + expect(consoleError.mock.calls[0][0]).toMatch(/AutoEdgeGroup/); + }); + + test('edgeGroupPeek alone does not demand enterprise', () => { + validateOptionModules( + options({ edgeGroupPeek: { animate: false } }), + nothingRegistered + ); + expect(consoleError).not.toHaveBeenCalled(); + }); + + test('an explicitly disabled feature is silent', () => { + validateOptionModules( + options({ + smartGuides: { enabled: false }, + pinnedTabs: { enabled: false }, + layoutHistory: { enabled: false }, + autoHideEdgeGroups: false, + dockToEdgeGroups: { left: false }, + }), + nothingRegistered + ); + expect(consoleError).not.toHaveBeenCalled(); + }); + + test('smartGuides is enabled by presence', () => { + validateOptionModules(options({ smartGuides: {} }), nothingRegistered); + expect(consoleError.mock.calls[0][0]).toMatch(/SmartGuides/); + }); + + test('layoutHistory is dormant until enabled', () => { + validateOptionModules( + options({ layoutHistory: {} }), + nothingRegistered + ); + expect(consoleError).not.toHaveBeenCalled(); + + validateOptionModules( + options({ layoutHistory: { enabled: true } }), + nothingRegistered + ); + expect(consoleError.mock.calls[0][0]).toMatch(/LayoutHistory/); + }); + + test('a per-edge set fires when any edge is enabled', () => { + validateOptionModules( + options({ autoHideEdgeGroups: { left: true } }), + nothingRegistered + ); + expect(consoleError.mock.calls[0][0]).toMatch(/AutoHideEdgeGroup/); + }); + + test('logs once per module+reason across repeated validation', () => { + for (let i = 0; i < 3; i++) { + validateOptionModules( + options({ overflow: { mode: 'wrap' } }), + nothingRegistered + ); + } + expect(consoleError).toHaveBeenCalledTimes(1); + }); +}); + +describe('OPTION_MODULE_RULES', () => { + test('every rule names a known module', () => { + // Derived from AllModules rather than hardcoded, so adding or renaming + // a core module can't leave this list stale. Guards against a typo'd + // module name silently never matching (and so never warning). + const coreModules = new Set(AllModules.map((m) => m.moduleName)); + + for (const rule of OPTION_MODULE_RULES) { + const known = + ENTERPRISE_MODULE_NAMES.has(rule.moduleName) || + coreModules.has(rule.moduleName); + expect([rule.reason, known]).toEqual([rule.reason, true]); + } + }); + + test('no rule fires on empty options', () => { + for (const rule of OPTION_MODULE_RULES) { + expect([rule.reason, rule.when({})]).toEqual([rule.reason, false]); + } + }); +}); diff --git a/packages/dockview-core/src/dockview/dockviewComponent.ts b/packages/dockview-core/src/dockview/dockviewComponent.ts index f6c3cceceb..f6d1cc5399 100644 --- a/packages/dockview-core/src/dockview/dockviewComponent.ts +++ b/packages/dockview-core/src/dockview/dockviewComponent.ts @@ -85,8 +85,10 @@ import { assertModule, DockviewModule, getRegisteredModules, + missingModuleMessage, ModuleRegistry, } from './modules'; +import { validateOptionModules } from './optionsModules'; import { AllModules } from './allModules'; import { IFloatingGroupHost } from './floatingGroupService'; import { IPopoutWindowHost, PopoutGroupEntry } from './popoutWindowService'; @@ -883,12 +885,20 @@ export class DockviewComponent /** Toggle Smart Guides snapping at runtime (no-op if the module is absent). */ setSmartGuidesEnabled(enabled: boolean): void { - this._smartGuidesService?.setEnabled(enabled); + assertModule( + this._smartGuidesService, + 'SmartGuides', + 'api.setSmartGuidesEnabled' + )?.setEnabled(enabled); } /** Merge a partial Smart Guides option override in at runtime. */ updateSmartGuidesOptions(options: Partial): void { - this._smartGuidesService?.updateOptions(options); + assertModule( + this._smartGuidesService, + 'SmartGuides', + 'api.updateSmartGuidesOptions' + )?.updateOptions(options); } /** Fires when a dragged float commits an alignment snap on drop. */ @@ -1157,12 +1167,20 @@ export class DockviewComponent /** Undo the previous recorded layout mutation (no-op if nothing to undo or * the LayoutHistory module is absent). Requires `layoutHistory.enabled`. */ undo(): void { - this._layoutHistoryService?.undo(); + assertModule( + this._layoutHistoryService, + 'LayoutHistory', + 'api.undo' + )?.undo(); } /** Re-apply the next layout mutation (no-op if nothing to redo). */ redo(): void { - this._layoutHistoryService?.redo(); + assertModule( + this._layoutHistoryService, + 'LayoutHistory', + 'api.redo' + )?.redo(); } get canUndo(): boolean { @@ -1416,11 +1434,12 @@ export class DockviewComponent return; } - const service = assertModule( - this.pinnedTabsService, - 'PinnedTabs', - 'setPinned' - ); + // Not routed through assertModule: reaching here means + // `pinnedTabs.enabled` is set, so the option rule in optionsModules.ts + // has already reported the missing module at construction (or at the + // updateOptions that enabled it). Warning again here would report the + // same module twice for one mistake. + const service = this.pinnedTabsService; if (!service) { return; } @@ -1484,6 +1503,8 @@ export class DockviewComponent } this._moduleRegistry.initialize(this); + this.reportMissingOptionModules(options); + // Surface popout removal symmetrically with `onDidAddPopoutGroup`. The // service is the single point every removal path funnels through, both // a genuine window close and an explicit removal, and it suppresses @@ -2763,6 +2784,11 @@ export class DockviewComponent override updateOptions(options: Partial): void { super.updateOptions(options); + // Validate what the caller passed, not the merged result: every key of + // DockviewOptions is present (as `undefined`) on `this.options`, so a + // presence test there would fire for every rule. + this.reportMissingOptionModules(options); + this._floatingGroupService?.updateBounds(options); this._rootDropTargetService?.setOptions(options); @@ -2815,6 +2841,19 @@ export class DockviewComponent this._layoutFromShell(this.gridview.width, this.gridview.height); } + /** + * Report any option the caller set whose module isn't registered. Logs + * once per module+reason; never throws, matching the module system's + * degrade-to-no-op contract. + */ + private reportMissingOptionModules( + options: Partial + ): void { + validateOptionModules(options, (moduleName) => + this._moduleRegistry.has(moduleName) + ); + } + override layout( width: number, height: number, @@ -2864,13 +2903,15 @@ export class DockviewComponent position: EdgeGroupPosition, options: AddEdgeGroupOptions ): DockviewGroupPanelApi { - const service = assertModule( - this._edgeGroupService, - 'EdgeGroup', - 'api.addEdgeGroup' - ); + const service = this._edgeGroupService; if (!service) { - throw new Error(`dockview: EdgeGroup module is not registered`); + // Throws rather than degrading to a no-op like every other module + // entry point: the return type is non-optional, so there is no + // group to hand back. Not routed through assertModule — that would + // log and then throw, reporting the same problem twice. + throw new Error( + missingModuleMessage('EdgeGroup', 'api.addEdgeGroup') + ); } if (service.has(position)) { throw new Error( diff --git a/packages/dockview-core/src/dockview/modules.ts b/packages/dockview-core/src/dockview/modules.ts index 177f02f06f..2727a169d0 100644 --- a/packages/dockview-core/src/dockview/modules.ts +++ b/packages/dockview-core/src/dockview/modules.ts @@ -103,24 +103,117 @@ export function defineModule(config: { }; } +/** + * The modules that ship in `dockview-enterprise` rather than the free + * packages. Core never imports them; it holds their names only so a + * missing-module message can name the package that provides the fix. + * + * Keep in sync with the `Modules` list exported by `dockview-enterprise`. + */ +export const ENTERPRISE_MODULE_NAMES: ReadonlySet = new Set([ + 'AdvancedOverflow', + 'AutoEdgeGroup', + 'AutoHideEdgeGroup', + 'ContextMenu', + 'DropGuide', + 'KeyboardDocking', + 'KeyboardNavigation', + 'LayoutHistory', + 'License', + 'MultiRowTabs', + 'PinnedTabs', + 'SmartGuides', +]); + const _warnedMissingModule = new Set(); /** - * For tests: clears the once-per-key dedup cache used by `assertModule`. + * For tests: clears the once-per-key dedup cache used by `logMissingModule` + * (and therefore `assertModule`). */ export function _resetMissingModuleWarnings(): void { _warnedMissingModule.clear(); } +/** + * Builds the text for a module that a caller needed but which isn't + * registered. `reason` describes what the user did to need it (an option they + * set, an api method they called) and is quoted verbatim. Pass several when one + * module is needed for several reasons at once — a single message listing them + * beats one message per reason each repeating the same install instructions. + * + * Exported so the rare caller that must throw (see `assertModule`) can raise + * the same message rather than a thinner second one. + * + * Enterprise modules get the install/import fix inline, because importing + * `dockview-enterprise` self-registers every enterprise module — there is no + * separate `registerModules` step for the user to get wrong. + */ +export function missingModuleMessage( + moduleName: string, + reason?: string | string[] +): string { + const reasons = reason === undefined ? [] : ([] as string[]).concat(reason); + const quoted = reasons.map((r) => `\`${r}\``).join(', '); + const needed = reasons.length + ? `${quoted} require${reasons.length > 1 ? '' : 's'} the "${moduleName}" module` + : `The "${moduleName}" module is required`; + + if (ENTERPRISE_MODULE_NAMES.has(moduleName)) { + return ( + `dockview: ${needed}, which ships in dockview-enterprise.\n\n` + + ` npm install dockview-enterprise\n` + + ` import 'dockview-enterprise'; // self-registers every enterprise module\n` + ); + } + + return `dockview: ${needed}, but it is not registered.`; +} + +/** + * Logs a deduplicated "module not registered" error naming what was needed and + * how to get it. Deduped per module+reasons for the lifetime of the page, so + * repeated calls (and re-validation on every `updateOptions`) log once. + */ +export function logMissingModule( + moduleName: string, + reason?: string | string[] +): void { + const reasons = reason === undefined ? [] : ([] as string[]).concat(reason); + const key = `${moduleName}|${reasons.join('|')}`; + if (_warnedMissingModule.has(key)) { + return; + } + _warnedMissingModule.add(key); + console.error(missingModuleMessage(moduleName, reason)); +} + /** * Returns the service if its module is registered, otherwise logs a - * deduplicated console error and returns `undefined`. Missing modules never - * throw; they degrade the affected feature to a no-op so consuming - * applications don't crash in production. + * deduplicated console error and returns `undefined`. This function never + * throws: the affected feature degrades to a no-op so consuming applications + * don't crash in production. + * + * The one exception is an entry point that must return a value it cannot + * synthesise — `addEdgeGroup(): DockviewGroupPanelApi` has no group to hand + * back and no `undefined` in its return type, so it throws + * `missingModuleMessage(...)` directly instead of calling this. Don't route + * such a caller through here: it would log *and* throw, reporting twice. * * Use at public-API entry points where the caller wants to surface which * module is missing. For internal/lifecycle paths, plain `?.` chaining on * the service slot is preferred: no log, a silent no-op. + * + * Guard commands, not queries. `api.undo()` asked for something to happen, so + * silence is a bug report waiting to happen; `canUndo` asked a question, and + * `false` is a truthful answer that shouldn't log. Same for event getters + * falling back to a never-firing event, and for idempotent cleanup + * (`clearHistory()` on an absent history has genuinely nothing to do). + * + * Interaction handlers are queries in this sense too: a right-click reaching an + * absent ContextMenu module means the app never asked for one, so it stays + * silent (`?.`) and the browser's own menu shows. Options are where intent is + * declared — see `optionsModules.ts`. */ export function assertModule( service: T | undefined, @@ -130,15 +223,7 @@ export function assertModule( if (service !== undefined) { return service; } - const key = `${moduleName}|${context ?? ''}`; - if (_warnedMissingModule.has(key)) { - return undefined; - } - _warnedMissingModule.add(key); - const where = context ? ` for ${context}` : ''; - console.error( - `dockview: module "${moduleName}" is not registered${where}.` - ); + logMissingModule(moduleName, context); return undefined; } diff --git a/packages/dockview-core/src/dockview/optionsModules.ts b/packages/dockview-core/src/dockview/optionsModules.ts new file mode 100644 index 0000000000..b9687dd7dd --- /dev/null +++ b/packages/dockview-core/src/dockview/optionsModules.ts @@ -0,0 +1,180 @@ +/** + * Maps dockview options to the modules that implement them, so setting an + * option for an absent module reports which module is missing instead of + * silently doing nothing. + * + * This is the "declared intent" diagnostic. It fires when the user asks for a + * feature via options — at construction and on every `updateOptions`. It + * deliberately does *not* fire on user interaction: right-clicking without the + * ContextMenu module stays silent (the browser's own menu shows), because the + * `?.` service-slot call sites can't tell a missing module from a feature the + * app never wanted. + * + * Rules are a flat list rather than a `Record` + * because dockview's options are nested and value-gated: `overflow.mode: + * 'wrap'` needs MultiRowTabs while `overflow.search` needs AdvancedOverflow, + * so one top-level key maps to several modules depending on its contents. A + * rule per gated thing also lets `reason` name the exact path the user set, + * which is the whole point of the message. + * + * ## Adding a rule + * + * A rule is just "this option needs that module". Whether that makes it paid is + * not a judgement call: a module is enterprise iff it ships in + * `dockview-enterprise`'s `Modules` list. {@link ENTERPRISE_MODULE_NAMES} + * mirrors that list, and a test in dockview-enterprise fails if the two drift, + * so naming the right module is the only thing you have to get right here. + * + * The one trap is picking the module. Don't infer it from what core does at + * runtime: core may carry a fallback that hands a paid feature to free builds, + * and reading that as "the option is free" turns one bug into two. Pick the + * module that *implements the documented feature*; if core also does it, that's + * a leak to fix in core, not a reason to drop the rule. `dockToEdgeGroups` is + * the cautionary case — core's single-band fallback made it look free. + */ + +import { logMissingModule } from './modules'; +import { DockviewComponentOptions, isAnyEdgeGroupEnabled } from './options'; + +export interface OptionModuleRule { + /** + * How the user expressed intent, quoted verbatim into the message — name + * the exact option path and value, e.g. `overflow.mode: 'wrap'`. + */ + reason: string; + /** The module that implements it. */ + moduleName: string; + /** + * Whether `options` requests the feature. Receives the options the caller + * passed, never the merged set — every key of `DockviewOptions` is present + * (as `undefined`) on the merged object, so a presence test there is always + * true. An option left out, `undefined`, or explicitly disabled must return + * false: silence is correct for a feature nobody asked for. + */ + when: (options: Partial) => boolean; +} + +export const OPTION_MODULE_RULES: OptionModuleRule[] = [ + { + reason: 'smartGuides', + moduleName: 'SmartGuides', + // Present implies enabled; `enabled: false` is an explicit opt-out. + when: (o) => o.smartGuides != null && o.smartGuides.enabled !== false, + }, + { + reason: 'layoutHistory.enabled: true', + moduleName: 'LayoutHistory', + // Defaults to false: the module is inert until asked for. + when: (o) => o.layoutHistory?.enabled === true, + }, + { + reason: 'pinnedTabs.enabled: true', + moduleName: 'PinnedTabs', + when: (o) => o.pinnedTabs?.enabled === true, + }, + { + reason: "overflow.mode: 'wrap'", + moduleName: 'MultiRowTabs', + when: (o) => o.overflow?.mode === 'wrap', + }, + { + reason: 'overflow.maxRows', + moduleName: 'MultiRowTabs', + when: (o) => o.overflow?.maxRows != null, + }, + { + reason: 'overflow.search', + moduleName: 'AdvancedOverflow', + when: (o) => !!o.overflow?.search, + }, + { + reason: 'overflow.mru', + moduleName: 'AdvancedOverflow', + when: (o) => !!o.overflow?.mru, + }, + { + reason: 'overflow.thumbnails', + moduleName: 'AdvancedOverflow', + when: (o) => !!o.overflow?.thumbnails, + }, + { + reason: 'autoHideEdgeGroups', + moduleName: 'AutoHideEdgeGroup', + // Without the module a collapsed edge group renders an empty strip and + // can't be pinned back — the activators are the module's whole job, so + // core has no fallback here. + when: (o) => isAnyEdgeGroupEnabled(o.autoHideEdgeGroups), + }, + + { + reason: 'dockToEdgeGroups', + moduleName: 'AutoEdgeGroup', + // Dock-to-edge is a paid feature end to end: features.mdx ticks only + // the Enterprise column and autoEdgeGroups.mdx is `enterprise: true`. + // Core carries a single-band fallback (see dockviewComponent's edge-drop + // handler, gated on `!autoEdgeGroupService`) that predates that + // decision and currently hands the feature to free builds — tracked + // separately; this rule reflects the intended boundary, not that leak. + when: (o) => isAnyEdgeGroupEnabled(o.dockToEdgeGroups), + }, + + // No rule for `edgeGroupPeek`: it only tunes `autoHideEdgeGroups`, and is + // read solely by AutoHideEdgeGroupService. Alone it is inert even *with* + // the module, so it can't justify a message of its own; alongside + // `autoHideEdgeGroups` the rule above already covers it. + { + reason: 'getTabContextMenuItems', + moduleName: 'ContextMenu', + when: (o) => o.getTabContextMenuItems != null, + }, + { + reason: 'getTabGroupChipContextMenuItems', + moduleName: 'ContextMenu', + when: (o) => o.getTabGroupChipContextMenuItems != null, + }, + { + reason: 'createContextMenuItemComponent', + moduleName: 'ContextMenu', + when: (o) => o.createContextMenuItemComponent != null, + }, + { + reason: 'dndGuide', + moduleName: 'DropGuide', + when: (o) => o.dndGuide != null, + }, +]; + +/** + * Reports any option in `options` whose module isn't registered. Never throws. + * + * Reasons are grouped so each missing module produces exactly one message, however + * many of its options were set: `overflow: { mode: 'wrap', maxRows: 3 }` names + * both paths in a single MultiRowTabs error rather than repeating the install + * instructions twice. Logging is deduplicated per module+reasons (see + * `logMissingModule`), so re-validating on every `updateOptions` stays quiet. + * + * Pass the options the caller supplied, not the merged set — see + * {@link OptionModuleRule.when}. + */ +export function validateOptionModules( + options: Partial, + isModuleRegistered: (moduleName: string) => boolean +): void { + const reasonsByModule = new Map(); + + for (const rule of OPTION_MODULE_RULES) { + if (!rule.when(options) || isModuleRegistered(rule.moduleName)) { + continue; + } + const reasons = reasonsByModule.get(rule.moduleName); + if (reasons) { + reasons.push(rule.reason); + } else { + reasonsByModule.set(rule.moduleName, [rule.reason]); + } + } + + for (const [moduleName, reasons] of reasonsByModule) { + logMissingModule(moduleName, reasons); + } +} diff --git a/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts b/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts new file mode 100644 index 0000000000..f4dfacbb7d --- /dev/null +++ b/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts @@ -0,0 +1,46 @@ +// Deep-imports a dockview-core internal on purpose. `ENTERPRISE_MODULE_NAMES` +// is not part of core's public API — core holds it only to word its own +// missing-module messages — and widening the published surface just to make it +// testable would be a worse trade than this one test-only reach. Specs are +// excluded from the tsc build (see tsconfig `exclude`), so this path never +// ships. +import { ENTERPRISE_MODULE_NAMES } from '../../../dockview-core/src/dockview/modules'; +import { Modules } from '../index'; + +/** + * `dockview-core` mirrors the enterprise module names so a missing-module error + * can say "this ships in dockview-enterprise, here's how to install it". Core + * can't import this package to derive that list (the whole point of the split), + * so the mirror is hand-maintained and can silently drift. + * + * Drift is not a cosmetic problem: an enterprise module missing from the set + * degrades its error from "npm install dockview-enterprise" to a bare "not + * registered", which is exactly the uninformative failure the message exists to + * prevent — and it would only surface to a user who doesn't have the package. + * This test is the only thing holding the two in sync. + */ +describe('ENTERPRISE_MODULE_NAMES mirrors dockview-enterprise', () => { + const shipped = Modules.map((m) => m.moduleName); + + test('every module this package registers is named in core', () => { + const missing = shipped + .filter((name) => !ENTERPRISE_MODULE_NAMES.has(name)) + .sort(); + + // If this fails: add the names to ENTERPRISE_MODULE_NAMES in + // dockview-core/src/dockview/modules.ts. + expect(missing).toEqual([]); + }); + + test('core names no module this package no longer ships', () => { + const shippedNames = new Set(shipped); + const stale = [...ENTERPRISE_MODULE_NAMES] + .filter((name) => !shippedNames.has(name)) + .sort(); + + // If this fails: a module was renamed or moved to core (free), and + // ENTERPRISE_MODULE_NAMES still claims it's paid — users would be told + // to install a package they don't need. + expect(stale).toEqual([]); + }); +}); From d1d3de4d09c5846817ad923b46c131625f62cec4 Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Thu, 16 Jul 2026 21:31:53 +0100 Subject: [PATCH 2/6] fix(core): stop giving dock-to-edge away to free builds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dockToEdgeGroups` is an enterprise feature — features.mdx ticks only the Enterprise column and autoEdgeGroups.mdx is `enterprise: true` — but core carried a single-band fallback in `dockToLayoutEdge` that performed the dock whenever AutoEdgeGroup was absent. A free user setting the option dropped a panel on a root edge and got an edge group: the paid feature, minus only the two-band drag-reveal affordance. Remove the fallback. The option is now inert without the module and a root-edge drop splits the grid, as it does when the option is unset. Enterprise behaviour cannot change: the removed block's own condition included `!autoEdgeGroupService`, making it dead code whenever the module is registered — those drops already returned earlier via the module's `onWillDrop.preventDefault`, or fell through to the same move below. The enterprise suite passes untouched. Add the core tests that never existed for this path: they fail against the old fallback and pass now. Not addressed here: `resolveRootOverlayModel` still widens the root drop band 10px -> 32px from the option alone, so a free build gets a wider activation band that plainly splits the grid. Gating that on the service is not safe today — RootDropTargetService resolves the band in its own constructor during `_moduleRegistry.initialize()`, and core modules initialise before the enterprise ones, so the service would read `autoEdgeGroupService` as absent and collapse the band to 10px for paying users too. It needs the check deferred to postConstruct; tracked separately. The band is cosmetic — the feature itself no longer leaks. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../dockview/dockToEdgeGroupsTier.spec.ts | 100 ++++++++++++++++++ .../src/dockview/dockviewComponent.ts | 26 ++--- 2 files changed, 107 insertions(+), 19 deletions(-) create mode 100644 packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts diff --git a/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts b/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts new file mode 100644 index 0000000000..1405a03564 --- /dev/null +++ b/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts @@ -0,0 +1,100 @@ +import { DockviewComponent } from '../../dockview/dockviewComponent'; +import { AllModules } from '../../dockview/allModules'; +import { IContentRenderer } from '../../dockview/types'; +import { LocalSelectionTransfer, PanelTransfer } from '../../dnd/dataTransfer'; +import { _resetMissingModuleWarnings } from '../../dockview/modules'; + +class TestPanel implements IContentRenderer { + element = document.createElement('div'); + init(): void { + // noop + } + layout(): void { + // noop + } + dispose(): void { + // noop + } +} + +/** + * Docking a dragged panel to a layout edge is an AutoEdgeGroup (enterprise) + * feature — `features.mdx` ticks only the Enterprise column. Core used to carry + * a single-band fallback that performed the dock whenever the module was + * absent, which handed the feature to free builds; these tests pin the tier + * boundary so it can't come back. + * + * `AllModules` is core-only, so this file always runs as a free build: the + * enterprise modules self-register on import of `dockview-enterprise`, which + * this package never imports. + */ +describe('dockToEdgeGroups is enterprise-only', () => { + const transfer = LocalSelectionTransfer.getInstance(); + + let dockview: DockviewComponent; + let container: HTMLElement; + + beforeEach(() => { + container = document.createElement('div'); + document.body.appendChild(container); + + // The missing-module dedup is page-global and outlives a test, so + // without this only the first component built in this file would log. + _resetMissingModuleWarnings(); + jest.spyOn(console, 'error').mockImplementation(() => undefined); + + dockview = new DockviewComponent(container, { + createComponent: () => new TestPanel(), + dockToEdgeGroups: true, + modules: AllModules, + } as never); + dockview.layout(1000, 800); + }); + + afterEach(() => { + transfer.clearData(PanelTransfer.prototype); + dockview.dispose(); + container.remove(); + jest.restoreAllMocks(); + }); + + function dragPanelToEdge(panelId: string, groupId: string): void { + transfer.setData( + [new PanelTransfer(dockview.id, groupId, panelId)], + PanelTransfer.prototype + ); + dockview.dockToLayoutEdge(new MouseEvent('drop') as never, 'left'); + } + + test('a root-edge drop does not create an edge group', () => { + const a = dockview.addPanel({ id: 'a', component: 'default' }); + dockview.addPanel({ id: 'b', component: 'default' }); + + dragPanelToEdge('a', a.group.id); + + // The dock is the paid behaviour: without AutoEdgeGroup the drop must + // fall through to the ordinary grid split, leaving no edge group. + expect(dockview.getEdgeGroup('left')).toBeUndefined(); + expect( + dockview.groups.filter((g) => g.model.location.type === 'edge') + ).toEqual([]); + }); + + test('the panel is still moved — the drop degrades, it does not vanish', () => { + const a = dockview.addPanel({ id: 'a', component: 'default' }); + dockview.addPanel({ id: 'b', component: 'default' }); + + dragPanelToEdge('a', a.group.id); + + expect(dockview.panels.map((p) => p.id).sort()).toEqual(['a', 'b']); + }); + + test('setting the option reports the missing module', () => { + // The option is inert here, so the diagnostic is the only thing telling + // the user why — see optionsModules.ts. + const messages = (console.error as jest.Mock).mock.calls.map( + (c) => c[0] as string + ); + expect(messages.some((m) => m.includes('AutoEdgeGroup'))).toBe(true); + }); +}); diff --git a/packages/dockview-core/src/dockview/dockviewComponent.ts b/packages/dockview-core/src/dockview/dockviewComponent.ts index f6d1cc5399..9ffd5ee30d 100644 --- a/packages/dockview-core/src/dockview/dockviewComponent.ts +++ b/packages/dockview-core/src/dockview/dockviewComponent.ts @@ -1109,25 +1109,13 @@ export class DockviewComponent const data = getPanelData(); - if ( - data && - position !== 'center' && - isEdgeGroupEnabled( - this.options.dockToEdgeGroups, - position as EdgeGroupPosition - ) && - this._edgeGroupService && - !this._moduleRegistry.services.autoEdgeGroupService - ) { - // `dockToEdgeGroups` baseline (single band): a root-edge drop reveals - // an edge group instead of splitting the grid. When the two-band - // drag-reveal affordance is registered it owns edge-drop routing: - // it preempts the outer band via `onWillDrop.preventDefault` and lets - // the inner band fall through here to the default grid split, so - // this single-band fallback is disabled. - this.revealEdgeGroupWithData(position, data); - return; - } + // No `dockToEdgeGroups` handling here: docking a dragged panel to an + // edge is an AutoEdgeGroup (enterprise) feature, and that module owns + // edge-drop routing entirely — it preempts its outer "dock as edge + // group" band via `onWillDrop.preventDefault` above and lets the inner + // "split the grid" band fall through to the move below. Without the + // module the option is inert and a root-edge drop splits the grid, as + // it does when the option is unset. if (data) { this.moveGroupOrPanel({ From 9c9a1519d142e8b127cea28b22e86bc07b9db012 Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:02:06 +0100 Subject: [PATCH 3/6] fix(core): report keyboardNavigation, and pin the rules to the modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `keyboardNavigation` is read by nothing in core and only by the enterprise keyboard modules, but had no rule — so setting it on the free package did nothing and said nothing. Exactly the failure the rules table exists to prevent, shipped in the table itself. Add the rule, and stop it recurring. The table is hand-written and core cannot derive it (it can't import the modules it might be missing), so each module now declares the options that must name it (`DockviewModule.options`), and a test in dockview-enterprise pins the two together in both directions: - an option a module claims with no rule -> the user gets silence - a rule naming a module for an option it doesn't own -> the wrong module gets named, or a free feature gets billed as paid The test reproduces this bug when the new rule is removed. Rules gain `optionKey` so the test can match them without parsing prose; `reason` still carries the precise path for the message. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../dockview-core/src/dockview/modules.ts | 21 +++++++ .../src/dockview/optionsModules.ts | 37 ++++++++++++ .../__tests__/enterpriseModuleNames.spec.ts | 55 ++++++++++++++++++ .../src/advancedOverflowService.ts | 1 + .../src/autoEdgeGroupService.ts | 1 + .../src/autoHideEdgeGroupService.ts | 1 + .../dockview-enterprise/src/contextMenu.ts | 5 ++ .../src/dropGuideService.ts | 1 + .../src/keyboardNavigationService.ts | 1 + .../src/layoutHistoryService.ts | 1 + .../src/multiRowTabsService.ts | 1 + .../src/pinnedTabsService.ts | Bin 31108 -> 31137 bytes .../src/smartGuidesService.ts | 1 + 13 files changed, 126 insertions(+) diff --git a/packages/dockview-core/src/dockview/modules.ts b/packages/dockview-core/src/dockview/modules.ts index 2727a169d0..4ef004d183 100644 --- a/packages/dockview-core/src/dockview/modules.ts +++ b/packages/dockview-core/src/dockview/modules.ts @@ -68,6 +68,24 @@ export interface DockviewModule { */ init?: (host: THost, services: ServiceCollection) => IDisposable; dependsOn?: DockviewModule[]; + /** + * Top-level option keys that must report *this* module when it isn't + * registered — the module's half of the contract with core's + * `OPTION_MODULE_RULES`, which core cannot derive because it can't import + * the modules it might be missing. + * + * Declared here rather than inferred so a test can hold the two in sync: + * see `enterpriseModuleNames.spec.ts`, which fails if an option listed here + * has no rule (the user would get silence) or a rule names this module for + * an option not listed here. + * + * List an option only if it should produce a diagnostic naming this module. + * Options this module merely *reads* don't belong here: `edgeGroupPeek` only + * tunes `autoHideEdgeGroups` and is inert alone, and where one opt-in gates + * two modules in the same package (`keyboardNavigation`) only the module the + * message should name declares it — one mistake, one message. + */ + options?: string[]; } /** @@ -84,9 +102,12 @@ export function defineModule(config: { service: NonNullable ) => IDisposable; dependsOn?: DockviewModule[]; + /** See {@link DockviewModule.options}. */ + options?: string[]; }): DockviewModule { return { moduleName: config.name, + options: config.options, services: { [config.serviceKey]: config.create, }, diff --git a/packages/dockview-core/src/dockview/optionsModules.ts b/packages/dockview-core/src/dockview/optionsModules.ts index b9687dd7dd..0c5e676865 100644 --- a/packages/dockview-core/src/dockview/optionsModules.ts +++ b/packages/dockview-core/src/dockview/optionsModules.ts @@ -37,6 +37,14 @@ import { logMissingModule } from './modules'; import { DockviewComponentOptions, isAnyEdgeGroupEnabled } from './options'; export interface OptionModuleRule { + /** + * The top-level option this rule gates on. Several rules may share one key + * (`overflow` gates both MultiRowTabs and AdvancedOverflow); `reason` carries + * the precise path. Kept separate from `reason` so the completeness test in + * dockview-enterprise can match rules to the options each module declares + * without parsing prose. + */ + optionKey: keyof DockviewComponentOptions; /** * How the user expressed intent, quoted verbatim into the message — name * the exact option path and value, e.g. `overflow.mode: 'wrap'`. @@ -56,48 +64,57 @@ export interface OptionModuleRule { export const OPTION_MODULE_RULES: OptionModuleRule[] = [ { + optionKey: 'smartGuides', reason: 'smartGuides', moduleName: 'SmartGuides', // Present implies enabled; `enabled: false` is an explicit opt-out. when: (o) => o.smartGuides != null && o.smartGuides.enabled !== false, }, { + optionKey: 'layoutHistory', reason: 'layoutHistory.enabled: true', moduleName: 'LayoutHistory', // Defaults to false: the module is inert until asked for. when: (o) => o.layoutHistory?.enabled === true, }, { + optionKey: 'pinnedTabs', reason: 'pinnedTabs.enabled: true', moduleName: 'PinnedTabs', when: (o) => o.pinnedTabs?.enabled === true, }, { + optionKey: 'overflow', reason: "overflow.mode: 'wrap'", moduleName: 'MultiRowTabs', when: (o) => o.overflow?.mode === 'wrap', }, { + optionKey: 'overflow', reason: 'overflow.maxRows', moduleName: 'MultiRowTabs', when: (o) => o.overflow?.maxRows != null, }, { + optionKey: 'overflow', reason: 'overflow.search', moduleName: 'AdvancedOverflow', when: (o) => !!o.overflow?.search, }, { + optionKey: 'overflow', reason: 'overflow.mru', moduleName: 'AdvancedOverflow', when: (o) => !!o.overflow?.mru, }, { + optionKey: 'overflow', reason: 'overflow.thumbnails', moduleName: 'AdvancedOverflow', when: (o) => !!o.overflow?.thumbnails, }, { + optionKey: 'autoHideEdgeGroups', reason: 'autoHideEdgeGroups', moduleName: 'AutoHideEdgeGroup', // Without the module a collapsed edge group renders an empty strip and @@ -107,6 +124,7 @@ export const OPTION_MODULE_RULES: OptionModuleRule[] = [ }, { + optionKey: 'dockToEdgeGroups', reason: 'dockToEdgeGroups', moduleName: 'AutoEdgeGroup', // Dock-to-edge is a paid feature end to end: features.mdx ticks only @@ -123,25 +141,44 @@ export const OPTION_MODULE_RULES: OptionModuleRule[] = [ // the module, so it can't justify a message of its own; alongside // `autoHideEdgeGroups` the rule above already covers it. { + optionKey: 'getTabContextMenuItems', reason: 'getTabContextMenuItems', moduleName: 'ContextMenu', when: (o) => o.getTabContextMenuItems != null, }, { + optionKey: 'getTabGroupChipContextMenuItems', reason: 'getTabGroupChipContextMenuItems', moduleName: 'ContextMenu', when: (o) => o.getTabGroupChipContextMenuItems != null, }, { + optionKey: 'createContextMenuItemComponent', reason: 'createContextMenuItemComponent', moduleName: 'ContextMenu', when: (o) => o.createContextMenuItemComponent != null, }, { + optionKey: 'dndGuide', reason: 'dndGuide', moduleName: 'DropGuide', when: (o) => o.dndGuide != null, }, + { + optionKey: 'keyboardNavigation', + reason: 'keyboardNavigation', + moduleName: 'KeyboardNavigation', + // Any truthy value opts in (`true` or an options object) — mirrors + // `readKeyboardNavigation` in the enterprise keyboard modules. Baseline + // a11y (ARIA, within-strip arrow keys, live region, focus indicators) + // is free and needs no option; this opt-in buys layout-level keyboard + // operability, which is the paid part. + // + // The same opt-in also gates KeyboardDocking, but one message is + // enough: both ship in dockview-enterprise, so the install fixes both, + // and naming each separately would report one mistake twice. + when: (o) => !!o.keyboardNavigation, + }, ]; /** diff --git a/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts b/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts index f4dfacbb7d..2ae8497bc6 100644 --- a/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts +++ b/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts @@ -5,6 +5,7 @@ // excluded from the tsc build (see tsconfig `exclude`), so this path never // ships. import { ENTERPRISE_MODULE_NAMES } from '../../../dockview-core/src/dockview/modules'; +import { OPTION_MODULE_RULES } from '../../../dockview-core/src/dockview/optionsModules'; import { Modules } from '../index'; /** @@ -44,3 +45,57 @@ describe('ENTERPRISE_MODULE_NAMES mirrors dockview-enterprise', () => { expect(stale).toEqual([]); }); }); + +/** + * The second half of the same problem. `OPTION_MODULE_RULES` in core says which + * option needs which module; it is hand-written, and core cannot derive it — + * it can't import the modules it might be missing. So an enterprise option can + * be added with no rule, and it fails *silently*: the free user sets it, nothing + * happens, nothing is logged. `keyboardNavigation` shipped exactly that way. + * + * Each module declares the options that must name it (`DockviewModule.options`) + * and this pins the two together: the declaration lives with the module that + * knows the answer, and core's table is checked against it. + */ +describe('OPTION_MODULE_RULES covers every gated enterprise option', () => { + const declared = Modules.flatMap((m) => + (m.options ?? []).map((optionKey) => ({ + moduleName: m.moduleName, + optionKey, + })) + ); + + const ruled = OPTION_MODULE_RULES.map((r) => ({ + moduleName: r.moduleName, + optionKey: r.optionKey as string, + })); + + const key = (p: { moduleName: string; optionKey: string }) => + `${p.optionKey} -> ${p.moduleName}`; + + test('every option a module claims has a rule naming that module', () => { + const ruledKeys = new Set(ruled.map(key)); + const uncovered = declared + .filter((d) => !ruledKeys.has(key(d))) + .map(key) + .sort(); + + // If this fails: setting that option on the free package does nothing + // and says nothing. Add a rule to optionsModules.ts. + expect(uncovered).toEqual([]); + }); + + test('every rule naming an enterprise module is claimed by it', () => { + const declaredKeys = new Set(declared.map(key)); + const unclaimed = ruled + .filter((r) => ENTERPRISE_MODULE_NAMES.has(r.moduleName)) + .filter((r) => !declaredKeys.has(key(r))) + .map(key) + .sort(); + + // If this fails: core demands a module for an option the module doesn't + // own — the `dockToEdgeGroups` class of bug, where the wrong module gets + // named (or a free feature gets billed as paid). + expect(unclaimed).toEqual([]); + }); +}); diff --git a/packages/dockview-enterprise/src/advancedOverflowService.ts b/packages/dockview-enterprise/src/advancedOverflowService.ts index ae01f48803..34d095b6d0 100644 --- a/packages/dockview-enterprise/src/advancedOverflowService.ts +++ b/packages/dockview-enterprise/src/advancedOverflowService.ts @@ -442,6 +442,7 @@ export const AdvancedOverflowModule = defineModule< IAdvancedOverflowHost >({ name: 'AdvancedOverflow', + options: ['overflow'], serviceKey: 'advancedOverflowService', create: (host) => new AdvancedOverflowService(host), init: (host, service) => { diff --git a/packages/dockview-enterprise/src/autoEdgeGroupService.ts b/packages/dockview-enterprise/src/autoEdgeGroupService.ts index f2cafa15fb..4f7d2f4502 100644 --- a/packages/dockview-enterprise/src/autoEdgeGroupService.ts +++ b/packages/dockview-enterprise/src/autoEdgeGroupService.ts @@ -289,6 +289,7 @@ export const AutoEdgeGroupModule = defineModule< IAutoEdgeGroupHost >({ name: 'AutoEdgeGroup', + options: ['dockToEdgeGroups'], serviceKey: 'autoEdgeGroupService', dependsOn: [EdgeGroupModule], create: (host) => new AutoEdgeGroupService(host), diff --git a/packages/dockview-enterprise/src/autoHideEdgeGroupService.ts b/packages/dockview-enterprise/src/autoHideEdgeGroupService.ts index 577cbb30c0..b695fa4ccf 100644 --- a/packages/dockview-enterprise/src/autoHideEdgeGroupService.ts +++ b/packages/dockview-enterprise/src/autoHideEdgeGroupService.ts @@ -735,6 +735,7 @@ export const AutoHideEdgeGroupModule = defineModule< IAutoHideEdgeGroupHost >({ name: 'AutoHideEdgeGroup', + options: ['autoHideEdgeGroups'], serviceKey: 'autoHideEdgeGroupService', dependsOn: [EdgeGroupModule], create: (host) => new AutoHideEdgeGroupService(host), diff --git a/packages/dockview-enterprise/src/contextMenu.ts b/packages/dockview-enterprise/src/contextMenu.ts index 48cb51aba4..4a6e294aed 100644 --- a/packages/dockview-enterprise/src/contextMenu.ts +++ b/packages/dockview-enterprise/src/contextMenu.ts @@ -439,6 +439,11 @@ export const ContextMenuModule = defineModule< IContextMenuHost >({ name: 'ContextMenu', + options: [ + 'getTabContextMenuItems', + 'getTabGroupChipContextMenuItems', + 'createContextMenuItemComponent', + ], serviceKey: 'contextMenuService', create: (host) => new ContextMenuController(host), }); diff --git a/packages/dockview-enterprise/src/dropGuideService.ts b/packages/dockview-enterprise/src/dropGuideService.ts index 4f44705297..14ecac5ff6 100644 --- a/packages/dockview-enterprise/src/dropGuideService.ts +++ b/packages/dockview-enterprise/src/dropGuideService.ts @@ -512,6 +512,7 @@ export class DropGuideService export const DropGuideModule = defineModule<'dropGuideService', IDropGuideHost>( { name: 'DropGuide', + options: ['dndGuide'], serviceKey: 'dropGuideService', dependsOn: [AdvancedDnDModule], create: (host) => new DropGuideService(host), diff --git a/packages/dockview-enterprise/src/keyboardNavigationService.ts b/packages/dockview-enterprise/src/keyboardNavigationService.ts index 9aeb6f8039..b594254c2f 100644 --- a/packages/dockview-enterprise/src/keyboardNavigationService.ts +++ b/packages/dockview-enterprise/src/keyboardNavigationService.ts @@ -368,6 +368,7 @@ export const KeyboardNavigationModule = defineModule< IKeyboardNavigationHost >({ name: 'KeyboardNavigation', + options: ['keyboardNavigation'], serviceKey: 'keyboardNavigationService', create: (host) => new KeyboardNavigationService(host), }); diff --git a/packages/dockview-enterprise/src/layoutHistoryService.ts b/packages/dockview-enterprise/src/layoutHistoryService.ts index 7731d5c18a..c4d73c4779 100644 --- a/packages/dockview-enterprise/src/layoutHistoryService.ts +++ b/packages/dockview-enterprise/src/layoutHistoryService.ts @@ -383,6 +383,7 @@ export const LayoutHistoryModule = defineModule< ILayoutHistoryHost >({ name: 'LayoutHistory', + options: ['layoutHistory'], serviceKey: 'layoutHistoryService', create: (host) => new LayoutHistoryService(host), }); diff --git a/packages/dockview-enterprise/src/multiRowTabsService.ts b/packages/dockview-enterprise/src/multiRowTabsService.ts index 462065a43d..f37497692a 100644 --- a/packages/dockview-enterprise/src/multiRowTabsService.ts +++ b/packages/dockview-enterprise/src/multiRowTabsService.ts @@ -560,6 +560,7 @@ export const MultiRowTabsModule = defineModule< IMultiRowTabsHost >({ name: 'MultiRowTabs', + options: ['overflow'], serviceKey: 'multiRowTabsService', create: (host) => new MultiRowTabsService(host), }); diff --git a/packages/dockview-enterprise/src/pinnedTabsService.ts b/packages/dockview-enterprise/src/pinnedTabsService.ts index b691aadff4f1604567ca5c75191b6b68026e492b..160bcdb8ced19c68d102b6e34e38f760c84292a8 100644 GIT binary patch delta 38 tcmZqq%((C~2A!D}`wFg3P?U)Rd6Kq+<2h&Hfdpi~u*@4#ofg delta 14 VcmZ4ZnX%({ name: 'SmartGuides', + options: ['smartGuides'], serviceKey: 'smartGuidesService', dependsOn: [FloatingGroupModule], create: (host) => new SmartGuidesService(host), From e1bea3cd5e566940630b8edf20a9d8cf9c38b0e5 Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:34:04 +0100 Subject: [PATCH 4/6] fix(core): stop false purchase prompts, and finish gating dock-to-edge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from a cold review of the branch. `dndGuide: false` demanded the paid package. The rule tested presence (`!= null`) on a `boolean | {...}` option, so explicitly turning the compass *off* billed the user for it — the exact bug this branch exists to prevent, introduced by it. `smartGuides: false` did the same: `false` isn't in its type, but JS callers and prop-forwarding bindings reach it, and `false != null` fell through to `false.enabled`. Both now test truthiness, matching every sibling rule and the contract the file states. The disabled-is-silent test hand-listed five rules and missed the broken one, so it is now enumerated per rule. It found the `smartGuides` case immediately. Finish the dock-to-edge gate. Removing the fallback left the option widening the root activation band 10px -> 32px with no module check, so a free build got a 3.2x larger *grid-split* trigger and nothing else — a regression, and the comment claiming the option was "inert" was false. The band is now gated on the affordance being registered. That gate has a trap worth knowing: RootDropTargetService resolves the band in its own constructor, during module initialisation, and core's modules initialise before the enterprise ones — so `hasEdgeDragReveal` is always false at that point, however the component is configured. Gating naively would collapse the band to 10px for paying users too. The re-resolve is deferred to the module's `init` (postConstruct); a test in dockview-enterprise pins it, and fails with 10 != 32 if the deferral goes. Also: - `options.ts` still advertised dockToEdgeGroups as needing only the free RootDropTarget + EdgeGroup modules — the IDE hover contradicted the diagnostic, so a user would read the error as a false positive. - Comments in moduleContracts and revealEdgeGroupWithData still described the deleted fallback; setPanelPinned still promised a warn that's gone. - assertModule now states both rules for guarding an entry point, since the branch applied opposite reasoning to setPanelPinned and undo/redo. They differ by reachability: undo() works with no option set, so no rule can have fired; setPanelPinned can't be reached until the option is set. - The completeness spec claimed to prevent silent gaps. It can't: coverage derives from a hand-written declaration, so an option with neither a declaration nor a rule still passes. Docstring now says so. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../dockview/dockToEdgeGroupsTier.spec.ts | 58 ++++++++--- .../__tests__/dockview/optionsModules.spec.ts | 21 ++++ .../src/dockview/dockviewComponent.ts | 24 +++-- .../src/dockview/moduleContracts.ts | 8 +- .../dockview-core/src/dockview/modules.ts | 25 ++++- .../dockview-core/src/dockview/options.ts | 10 +- .../src/dockview/optionsModules.ts | 10 +- .../src/dockview/rootDropTargetService.ts | 70 +++++++++---- .../src/__tests__/edgeDragRevealBand.spec.ts | 98 +++++++++++++++++++ .../__tests__/enterpriseModuleNames.spec.ts | 10 +- 10 files changed, 282 insertions(+), 52 deletions(-) create mode 100644 packages/dockview-enterprise/src/__tests__/edgeDragRevealBand.spec.ts diff --git a/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts b/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts index 1405a03564..af07bf1763 100644 --- a/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts +++ b/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts @@ -1,6 +1,7 @@ import { DockviewComponent } from '../../dockview/dockviewComponent'; import { AllModules } from '../../dockview/allModules'; import { IContentRenderer } from '../../dockview/types'; +import { DockviewComponentOptions } from '../../dockview/options'; import { LocalSelectionTransfer, PanelTransfer } from '../../dnd/dataTransfer'; import { _resetMissingModuleWarnings } from '../../dockview/modules'; @@ -32,32 +33,57 @@ describe('dockToEdgeGroups is enterprise-only', () => { const transfer = LocalSelectionTransfer.getInstance(); let dockview: DockviewComponent; - let container: HTMLElement; + const built: DockviewComponent[] = []; - beforeEach(() => { - container = document.createElement('div'); + /** A component with only core modules — i.e. the free package. */ + function freeDockview( + options: Partial + ): DockviewComponent { + const container = document.createElement('div'); document.body.appendChild(container); + const dv = new DockviewComponent(container, { + createComponent: () => new TestPanel(), + modules: AllModules, + ...options, + } as never); + dv.layout(1000, 800); + built.push(dv); + return dv; + } + beforeEach(() => { // The missing-module dedup is page-global and outlives a test, so // without this only the first component built in this file would log. _resetMissingModuleWarnings(); jest.spyOn(console, 'error').mockImplementation(() => undefined); - dockview = new DockviewComponent(container, { - createComponent: () => new TestPanel(), - dockToEdgeGroups: true, - modules: AllModules, - } as never); - dockview.layout(1000, 800); + dockview = freeDockview({ dockToEdgeGroups: true }); }); afterEach(() => { transfer.clearData(PanelTransfer.prototype); - dockview.dispose(); - container.remove(); + for (const dv of built.splice(0)) { + dv.dispose(); + } jest.restoreAllMocks(); }); + /** + * The resolved edge activation band, read off the drop target the service + * built. Reaches through privates deliberately: the band is not exposed + * anywhere public, and the alternative — simulating dragover at a measured + * offset — would assert the same thing through far more machinery. + */ + function bandActivationSize(dv: DockviewComponent): unknown { + const svc = (dv as never as { _rootDropTargetService: unknown }) + ._rootDropTargetService as { + _html5Target: { + options: { overlayModel?: { activationSize?: unknown } }; + }; + }; + return svc._html5Target.options.overlayModel?.activationSize; + } + function dragPanelToEdge(panelId: string, groupId: string): void { transfer.setData( [new PanelTransfer(dockview.id, groupId, panelId)], @@ -89,6 +115,16 @@ describe('dockToEdgeGroups is enterprise-only', () => { expect(dockview.panels.map((p) => p.id).sort()).toEqual(['a', 'b']); }); + test('the option does not widen the edge activation band', () => { + // The widened band exists to fit the affordance's outer "dock as edge + // group" sub-band. With no affordance nothing consumes it, so widening + // would only enlarge the plain grid-split trigger — a bigger target for + // behaviour the default band already gives. + expect(bandActivationSize(dockview)).toEqual( + bandActivationSize(freeDockview({ dockToEdgeGroups: undefined })) + ); + }); + test('setting the option reports the missing module', () => { // The option is inert here, so the diagnostic is the only thing telling // the user why — see optionsModules.ts. diff --git a/packages/dockview-core/src/__tests__/dockview/optionsModules.spec.ts b/packages/dockview-core/src/__tests__/dockview/optionsModules.spec.ts index fac5f0e062..cf48d87671 100644 --- a/packages/dockview-core/src/__tests__/dockview/optionsModules.spec.ts +++ b/packages/dockview-core/src/__tests__/dockview/optionsModules.spec.ts @@ -123,12 +123,33 @@ describe('validateOptionModules', () => { layoutHistory: { enabled: false }, autoHideEdgeGroups: false, dockToEdgeGroups: { left: false }, + dndGuide: false, + keyboardNavigation: false, + overflow: { search: false, mru: false }, }), nothingRegistered ); expect(consoleError).not.toHaveBeenCalled(); }); + // Enumerated rather than hand-listed: the hand-listed test above missed + // `dndGuide: false`, which fired because its rule tested presence rather + // than truthiness. A rule that can't be expressed as a plain `false` is + // skipped — every such option here nests its own opt-out, covered above. + test.each([ + ['dndGuide'], + ['keyboardNavigation'], + ['autoHideEdgeGroups'], + ['dockToEdgeGroups'], + ['smartGuides'], + ])('`%s: false` is an opt-out, never a purchase prompt', (key) => { + validateOptionModules( + options({ [key]: false } as Partial), + nothingRegistered + ); + expect(consoleError).not.toHaveBeenCalled(); + }); + test('smartGuides is enabled by presence', () => { validateOptionModules(options({ smartGuides: {} }), nothingRegistered); expect(consoleError.mock.calls[0][0]).toMatch(/SmartGuides/); diff --git a/packages/dockview-core/src/dockview/dockviewComponent.ts b/packages/dockview-core/src/dockview/dockviewComponent.ts index 9ffd5ee30d..c56d388696 100644 --- a/packages/dockview-core/src/dockview/dockviewComponent.ts +++ b/packages/dockview-core/src/dockview/dockviewComponent.ts @@ -1114,8 +1114,9 @@ export class DockviewComponent // edge-drop routing entirely — it preempts its outer "dock as edge // group" band via `onWillDrop.preventDefault` above and lets the inner // "split the grid" band fall through to the move below. Without the - // module the option is inert and a root-edge drop splits the grid, as - // it does when the option is unset. + // module a root-edge drop splits the grid, exactly as when the option + // is unset — the option's only other effect, widening the activation + // band, is gated on the same service (see rootDropTargetService). if (data) { this.moveGroupOrPanel({ @@ -1191,6 +1192,15 @@ export class DockviewComponent ); } + /** + * Whether the two-band edge drag-reveal affordance is registered. See + * {@link IRootDropTargetHost.hasEdgeDragReveal} — must not be read during + * module initialisation, only from `init`/postConstruct onwards. + */ + get hasEdgeDragReveal(): boolean { + return !!this._moduleRegistry.services.autoEdgeGroupService; + } + isGridEmpty(): boolean { return this.gridview.length === 0; } @@ -1406,8 +1416,10 @@ export class DockviewComponent /** * Pin/unpin a panel's tab. The single gated entry point behind * `panel.api.setPinned`. Dormant unless `pinnedTabs.enabled` is set (a - * silent no-op), and a warn-once no-op when the PinnedTabs module is not - * registered. When active it mutates the panel's pinned flag (which fires + * silent no-op), and a silent no-op when the PinnedTabs module is not + * registered — reaching past the `enabled` check means the option was set, + * so the option rule has already named the missing module. When active it + * mutates the panel's pinned flag (which fires * `panel.api.onDidChangePinned`) and the component-level * `onDidPanelPinnedChange`; the module reacts to enforce pinned-first * ordering. @@ -2964,8 +2976,8 @@ export class DockviewComponent * (never re-created; `addEdgeGroup` throws on a duplicate position). No-op if * the EdgeGroup module is absent. * - * This is the primitive behind the dock-to-edge groups; the two-band - * drag-reveal affordance and the `dockToEdgeGroups` baseline both route here. + * This is the primitive behind the dock-to-edge groups: the two-band + * drag-reveal affordance routes its outer-band drops here. */ revealEdgeGroupWithData( position: EdgeGroupPosition, diff --git a/packages/dockview-core/src/dockview/moduleContracts.ts b/packages/dockview-core/src/dockview/moduleContracts.ts index cf78bd3a01..b2619d3673 100644 --- a/packages/dockview-core/src/dockview/moduleContracts.ts +++ b/packages/dockview-core/src/dockview/moduleContracts.ts @@ -489,9 +489,11 @@ export interface IAutoHideEdgeGroupService extends IDisposable { * `onWillDrop` seams (no new core overlay code): on a layout-edge drag it * classifies the pointer into an outer "dock as edge group" band and an inner * "split the grid" band, drawing its own outer-band highlight and routing an - * outer-band drop to `revealEdgeGroupWithData`. The presence of this service - * also disables core's single-band `dockToEdgeGroups` fallback so the inner band - * falls through to the default grid split. + * outer-band drop to `revealEdgeGroupWithData`. The inner band falls through to + * core's default grid split. + * + * This service is what makes `dockToEdgeGroups` do anything: core implements no + * part of it, and gates the widened activation band on this service's presence. */ export interface IAutoEdgeGroupHost { readonly options: DockviewComponentOptions; diff --git a/packages/dockview-core/src/dockview/modules.ts b/packages/dockview-core/src/dockview/modules.ts index 4ef004d183..8242d7992e 100644 --- a/packages/dockview-core/src/dockview/modules.ts +++ b/packages/dockview-core/src/dockview/modules.ts @@ -225,11 +225,26 @@ export function logMissingModule( * module is missing. For internal/lifecycle paths, plain `?.` chaining on * the service slot is preferred: no log, a silent no-op. * - * Guard commands, not queries. `api.undo()` asked for something to happen, so - * silence is a bug report waiting to happen; `canUndo` asked a question, and - * `false` is a truthful answer that shouldn't log. Same for event getters - * falling back to a never-firing event, and for idempotent cleanup - * (`clearHistory()` on an absent history has genuinely nothing to do). + * Two rules decide whether an entry point guards: + * + * 1. **Commands, not queries.** `api.undo()` asked for something to happen, so + * silence is a bug report waiting to happen; `canUndo` asked a question, and + * `false` is a truthful answer that shouldn't log. Same for event getters + * falling back to a never-firing event, and for idempotent cleanup + * (`clearHistory()` on an absent history has genuinely nothing to do). + * + * 2. **Only if the option rule can't already have fired.** A command reachable + * without its gating option — `api.undo()` works on a component that never + * set `layoutHistory` — needs this, because no rule will have run. A command + * that can't be reached until the option is set doesn't: `setPanelPinned` + * returns early unless `pinnedTabs.enabled`, so by the time it could warn, + * the option rule has already named the same module. Guarding it too would + * report one mistake twice. + * + * Rule 2 tolerates one overlap: setting `layoutHistory.enabled` *and* calling + * `api.undo()` without the module reports both, since the reasons differ and + * dedup is per module+reason. That's the price of covering the far more likely + * case — calling `undo()` having never set the option at all. * * Interaction handlers are queries in this sense too: a right-click reaching an * absent ContextMenu module means the app never asked for one, so it stays diff --git a/packages/dockview-core/src/dockview/options.ts b/packages/dockview-core/src/dockview/options.ts index 4d43339550..c95d42ce01 100644 --- a/packages/dockview-core/src/dockview/options.ts +++ b/packages/dockview-core/src/dockview/options.ts @@ -609,10 +609,12 @@ export interface DockviewOptions { * panel leaves. * * A per-edge set: `true` enables dock-to-edge on all four edges, or name - * edges individually. Requires both the `RootDropTarget` (drag overlays) - * and `EdgeGroup` modules; a no-op if either is absent. Distinct from - * `dndEdges`, which only shapes the outer drop overlay (and still splits - * the grid). Off by default. + * edges individually. Off by default. + * + * Requires the `AutoEdgeGroup` module (and the `EdgeGroup` module it builds + * on); a no-op if absent, where a root-edge drop splits the grid as usual. + * Distinct from `dndEdges`, which only shapes the outer drop overlay (and + * still splits the grid). */ dockToEdgeGroups?: EdgeGroupSet; /** diff --git a/packages/dockview-core/src/dockview/optionsModules.ts b/packages/dockview-core/src/dockview/optionsModules.ts index 0c5e676865..c222a3bb73 100644 --- a/packages/dockview-core/src/dockview/optionsModules.ts +++ b/packages/dockview-core/src/dockview/optionsModules.ts @@ -68,7 +68,10 @@ export const OPTION_MODULE_RULES: OptionModuleRule[] = [ reason: 'smartGuides', moduleName: 'SmartGuides', // Present implies enabled; `enabled: false` is an explicit opt-out. - when: (o) => o.smartGuides != null && o.smartGuides.enabled !== false, + // Truthiness rather than `!= null` so a JS caller passing the untyped + // `smartGuides: false` (a reasonable guess at the off switch, and + // whatever a binding forwards) isn't billed for disabling it. + when: (o) => !!o.smartGuides && o.smartGuides.enabled !== false, }, { optionKey: 'layoutHistory', @@ -162,7 +165,10 @@ export const OPTION_MODULE_RULES: OptionModuleRule[] = [ optionKey: 'dndGuide', reason: 'dndGuide', moduleName: 'DropGuide', - when: (o) => o.dndGuide != null, + // Truthiness, not presence: `dndGuide` is `boolean | {...}`, so a + // `!= null` test would fire on `dndGuide: false` — billing a user for + // turning the compass off. + when: (o) => !!o.dndGuide, }, { optionKey: 'keyboardNavigation', diff --git a/packages/dockview-core/src/dockview/rootDropTargetService.ts b/packages/dockview-core/src/dockview/rootDropTargetService.ts index ff48bf3cdd..f61ea76230 100644 --- a/packages/dockview-core/src/dockview/rootDropTargetService.ts +++ b/packages/dockview-core/src/dockview/rootDropTargetService.ts @@ -1,4 +1,4 @@ -import { IDisposable } from '../lifecycle'; +import { Disposable, IDisposable } from '../lifecycle'; import { Event } from '../events'; import { DroptargetEvent, @@ -18,23 +18,29 @@ const DEFAULT_ROOT_OVERLAY_MODEL: DroptargetOverlayModel = { size: { type: 'pixels', value: 20 }, }; -// When `dockToEdgeGroups` is on for any edge we widen the edge activation band -// so the two-band drag-reveal affordance has room for a distinct outer ("dock -// as edge group") and inner ("split the grid") sub-band within it. The band is -// uniform across edges; per-edge gating of the actual dock happens in the drop -// handler and the (enterprise) edge resolver. +// The two-band drag-reveal affordance needs room for a distinct outer ("dock as +// edge group") and inner ("split the grid") sub-band, so the edge activation +// band widens when it is present. The band is uniform across edges; per-edge +// gating of the actual dock happens in the affordance's own edge resolver. const AUTO_EDGE_ROOT_OVERLAY_MODEL: DroptargetOverlayModel = { activationSize: { type: 'pixels', value: 32 }, size: { type: 'pixels', value: 20 }, }; +/** + * `hasEdgeDragReveal` gates the widened band, *not* the `dockToEdgeGroups` + * option alone: without the affordance nothing consumes the outer sub-band, so + * widening would only enlarge the plain grid-split trigger — a 3.2x bigger + * target for the same behaviour the default band already gives. + */ function resolveRootOverlayModel( - options: Pick + options: Pick, + hasEdgeDragReveal: boolean ): DroptargetOverlayModel { if (typeof options.dndEdges === 'object' && options.dndEdges !== null) { return options.dndEdges; } - return isAnyEdgeGroupEnabled(options.dockToEdgeGroups) + return hasEdgeDragReveal && isAnyEdgeGroupEnabled(options.dockToEdgeGroups) ? AUTO_EDGE_ROOT_OVERLAY_MODEL : DEFAULT_ROOT_OVERLAY_MODEL; } @@ -43,6 +49,16 @@ export interface IRootDropTargetHost { readonly id: string; readonly element: HTMLElement; readonly options: DockviewComponentOptions; + /** + * Whether the two-band edge drag-reveal affordance is registered. + * + * Read lazily, never at construction: module services are created in + * registration order, and this one is built before any module that could + * provide the affordance — so at construction the answer is always `false`, + * even when the affordance is on its way. The module's `init` hook + * re-applies the options once every service exists. + */ + readonly hasEdgeDragReveal: boolean; isGridEmpty(): boolean; rootDropTargetOverrideTarget(): DropTargetTargetModel | undefined; /** @@ -103,7 +119,10 @@ export class RootDropTargetService implements IRootDropTargetService { return host.dispatchUnhandledDragOver(event, position); }; - const overlayModel = resolveRootOverlayModel(host.options); + const overlayModel = resolveRootOverlayModel( + host.options, + host.hasEdgeDragReveal + ); this._html5Target = html5Backend.createDropTarget(host.element, { className: 'dv-drop-target-edge', @@ -151,16 +170,19 @@ export class RootDropTargetService implements IRootDropTargetService { // changes. Prefer the incoming partial for a changed key, falling back // to the current host options for the other. if ('dndEdges' in options || 'dockToEdgeGroups' in options) { - const model = resolveRootOverlayModel({ - dndEdges: - 'dndEdges' in options - ? options.dndEdges - : this._host.options.dndEdges, - dockToEdgeGroups: - 'dockToEdgeGroups' in options - ? options.dockToEdgeGroups - : this._host.options.dockToEdgeGroups, - }); + const model = resolveRootOverlayModel( + { + dndEdges: + 'dndEdges' in options + ? options.dndEdges + : this._host.options.dndEdges, + dockToEdgeGroups: + 'dockToEdgeGroups' in options + ? options.dockToEdgeGroups + : this._host.options.dockToEdgeGroups, + }, + this._host.hasEdgeDragReveal + ); this._html5Target.setOverlayModel(model); this._pointerTarget.setOverlayModel(model); } @@ -179,4 +201,14 @@ export const RootDropTargetModule = defineModule< name: 'RootDropTarget', serviceKey: 'rootDropTargetService', create: (host) => new RootDropTargetService(host), + init: (host, service) => { + // Re-apply the options now that every module's service exists. + // `create` above ran during module initialisation, when + // `hasEdgeDragReveal` could not yet be true however the component was + // configured, so the band it resolved may be too narrow. Every key of + // DockviewOptions is present on the merged object, so this recomputes + // unconditionally. Nothing can drag between the two phases. + service.setOptions(host.options); + return Disposable.NONE; + }, }); diff --git a/packages/dockview-enterprise/src/__tests__/edgeDragRevealBand.spec.ts b/packages/dockview-enterprise/src/__tests__/edgeDragRevealBand.spec.ts new file mode 100644 index 0000000000..c0e3baafca --- /dev/null +++ b/packages/dockview-enterprise/src/__tests__/edgeDragRevealBand.spec.ts @@ -0,0 +1,98 @@ +import { DockviewComponent, IContentRenderer } from 'dockview'; + +class TestPanel implements IContentRenderer { + element = document.createElement('div'); + init(): void { + // noop + } + layout(): void { + // noop + } + dispose(): void { + // noop + } +} + +/** + * Core gates the widened root activation band on the two-band drag-reveal + * affordance being registered, so free builds don't get a bigger grid-split + * trigger for a feature they don't have. + * + * The gate has a trap: `RootDropTargetService` resolves the band in its own + * constructor, during module initialisation — and core's modules initialise + * before this package's, so at that moment `hasEdgeDragReveal` is *always* + * false, however the component is configured. A naive gate would therefore + * collapse the band to the default for paying users too. Core defers the + * re-resolve to the module's `init` (postConstruct); this test is what proves + * that deferral works. Without it the gate silently breaks the affordance it + * exists to serve. + * + * `src/__tests__/registerModules.ts` registers every enterprise module globally, + * so a plain component here is an enterprise build. + */ +describe('edge drag-reveal activation band', () => { + const DEFAULT_ACTIVATION = 10; + const WIDENED_ACTIVATION = 32; + + const built: DockviewComponent[] = []; + + afterEach(() => { + for (const dv of built.splice(0)) { + dv.dispose(); + } + }); + + function build(options: Record): DockviewComponent { + const container = document.createElement('div'); + document.body.appendChild(container); + const dv = new DockviewComponent(container, { + createComponent: () => new TestPanel(), + ...options, + } as never); + dv.layout(1000, 800); + built.push(dv); + return dv; + } + + function activationSize(dv: DockviewComponent): unknown { + const svc = (dv as never as { _rootDropTargetService: unknown }) + ._rootDropTargetService as { + _html5Target: { + options: { + overlayModel?: { activationSize?: { value?: number } }; + }; + }; + }; + return svc._html5Target.options.overlayModel?.activationSize?.value; + } + + test('widens for dockToEdgeGroups when the affordance is registered', () => { + expect(activationSize(build({ dockToEdgeGroups: true }))).toBe( + WIDENED_ACTIVATION + ); + }); + + test('stays default when the option is off, affordance or not', () => { + expect(activationSize(build({}))).toBe(DEFAULT_ACTIVATION); + expect(activationSize(build({ dockToEdgeGroups: false }))).toBe( + DEFAULT_ACTIVATION + ); + }); + + test('widens for a per-edge set too', () => { + expect( + activationSize(build({ dockToEdgeGroups: { left: true } })) + ).toBe(WIDENED_ACTIVATION); + }); + + test('tracks a late updateOptions', () => { + const dv = build({}); + expect(activationSize(dv)).toBe(DEFAULT_ACTIVATION); + + dv.updateOptions({ dockToEdgeGroups: true }); + expect(activationSize(dv)).toBe(WIDENED_ACTIVATION); + + dv.updateOptions({ dockToEdgeGroups: false }); + expect(activationSize(dv)).toBe(DEFAULT_ACTIVATION); + }); +}); diff --git a/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts b/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts index 2ae8497bc6..db736d207f 100644 --- a/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts +++ b/packages/dockview-enterprise/src/__tests__/enterpriseModuleNames.spec.ts @@ -54,8 +54,14 @@ describe('ENTERPRISE_MODULE_NAMES mirrors dockview-enterprise', () => { * happens, nothing is logged. `keyboardNavigation` shipped exactly that way. * * Each module declares the options that must name it (`DockviewModule.options`) - * and this pins the two together: the declaration lives with the module that - * knows the answer, and core's table is checked against it. + * and this pins the declaration to core's table, in both directions. + * + * Know what this does *not* buy you: coverage is measured against the + * declaration, which is itself hand-written. An option added with **neither** a + * declaration nor a rule passes both tests and stays just as silent. This + * narrows the manual step to one place — next to the module that knows the + * answer — rather than eliminating it. Adding an option to a module means adding + * it to `options` too, and nothing here will remind you. */ describe('OPTION_MODULE_RULES covers every gated enterprise option', () => { const declared = Modules.flatMap((m) => From 3e4b2bbac7c720cc2b2e1b4b15b109dfa4713a40 Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Thu, 16 Jul 2026 23:30:13 +0100 Subject: [PATCH 5/6] fix(core): finish the dock-to-edge gating cleanup With core's single-band fallback removed, resolve the root edge overlay band honestly at construction: pass `false` rather than `host.hasEdgeDragReveal`, whose service may not exist yet during module init. The `init` hook re-resolves the real band afterwards. Update the OPTION_MODULE_RULES rationale to say core implements no part of dock-to-edge, and pin the tier test to the concrete default activation size instead of comparing two free builds. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../dockview/dockToEdgeGroupsTier.spec.ts | 21 ++++++++++++++----- .../src/dockview/optionsModules.ts | 13 ++++++------ .../src/dockview/rootDropTargetService.ts | 11 ++++++---- 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts b/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts index af07bf1763..0181ffb548 100644 --- a/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts +++ b/packages/dockview-core/src/__tests__/dockview/dockToEdgeGroupsTier.spec.ts @@ -30,6 +30,9 @@ class TestPanel implements IContentRenderer { * this package never imports. */ describe('dockToEdgeGroups is enterprise-only', () => { + /** `DEFAULT_ROOT_OVERLAY_MODEL.activationSize` in rootDropTargetService. */ + const DEFAULT_ACTIVATION = 10; + const transfer = LocalSelectionTransfer.getInstance(); let dockview: DockviewComponent; @@ -74,14 +77,16 @@ describe('dockToEdgeGroups is enterprise-only', () => { * anywhere public, and the alternative — simulating dragover at a measured * offset — would assert the same thing through far more machinery. */ - function bandActivationSize(dv: DockviewComponent): unknown { + function bandActivationSize(dv: DockviewComponent): number | undefined { const svc = (dv as never as { _rootDropTargetService: unknown }) ._rootDropTargetService as { _html5Target: { - options: { overlayModel?: { activationSize?: unknown } }; + options: { + overlayModel?: { activationSize?: { value?: number } }; + }; }; }; - return svc._html5Target.options.overlayModel?.activationSize; + return svc._html5Target.options.overlayModel?.activationSize?.value; } function dragPanelToEdge(panelId: string, groupId: string): void { @@ -120,9 +125,15 @@ describe('dockToEdgeGroups is enterprise-only', () => { // group" sub-band. With no affordance nothing consumes it, so widening // would only enlarge the plain grid-split trigger — a bigger target for // behaviour the default band already gives. - expect(bandActivationSize(dockview)).toEqual( + // + // Pinned to the concrete default rather than compared against a second + // free build: two builds agreeing proves nothing if the band ever + // resolves to undefined. 32 is the widened value this must never be — + // see the enterprise-side edgeDragRevealBand spec for the positive. + expect(bandActivationSize(dockview)).toBe(DEFAULT_ACTIVATION); + expect( bandActivationSize(freeDockview({ dockToEdgeGroups: undefined })) - ); + ).toBe(DEFAULT_ACTIVATION); }); test('setting the option reports the missing module', () => { diff --git a/packages/dockview-core/src/dockview/optionsModules.ts b/packages/dockview-core/src/dockview/optionsModules.ts index c222a3bb73..ec9fcbf41c 100644 --- a/packages/dockview-core/src/dockview/optionsModules.ts +++ b/packages/dockview-core/src/dockview/optionsModules.ts @@ -29,8 +29,9 @@ * runtime: core may carry a fallback that hands a paid feature to free builds, * and reading that as "the option is free" turns one bug into two. Pick the * module that *implements the documented feature*; if core also does it, that's - * a leak to fix in core, not a reason to drop the rule. `dockToEdgeGroups` is - * the cautionary case — core's single-band fallback made it look free. + * a leak to fix in core, not a reason to drop the rule. `dockToEdgeGroups` was + * the cautionary case: core once carried a single-band fallback that made it + * look free, and the fix was to remove the fallback, not to weaken this rule. */ import { logMissingModule } from './modules'; @@ -132,10 +133,10 @@ export const OPTION_MODULE_RULES: OptionModuleRule[] = [ moduleName: 'AutoEdgeGroup', // Dock-to-edge is a paid feature end to end: features.mdx ticks only // the Enterprise column and autoEdgeGroups.mdx is `enterprise: true`. - // Core carries a single-band fallback (see dockviewComponent's edge-drop - // handler, gated on `!autoEdgeGroupService`) that predates that - // decision and currently hands the feature to free builds — tracked - // separately; this rule reflects the intended boundary, not that leak. + // Core implements no part of it — its edge-drop handler always splits + // the grid, and the widened activation band is gated on the service — + // so without the module the option is inert and this is the only thing + // that says so. when: (o) => isAnyEdgeGroupEnabled(o.dockToEdgeGroups), }, diff --git a/packages/dockview-core/src/dockview/rootDropTargetService.ts b/packages/dockview-core/src/dockview/rootDropTargetService.ts index f61ea76230..be9422c01f 100644 --- a/packages/dockview-core/src/dockview/rootDropTargetService.ts +++ b/packages/dockview-core/src/dockview/rootDropTargetService.ts @@ -119,10 +119,13 @@ export class RootDropTargetService implements IRootDropTargetService { return host.dispatchUnhandledDragOver(event, position); }; - const overlayModel = resolveRootOverlayModel( - host.options, - host.hasEdgeDragReveal - ); + // `false`, not `host.hasEdgeDragReveal`: this runs during module + // initialisation, where the affordance's service may not exist yet, so + // the honest answer here is always "no" (see `hasEdgeDragReveal`'s + // contract). Reading the host would look like a live gate while being + // incapable of returning anything else. The band this resolves is + // provisional; the `init` hook below re-resolves it for real. + const overlayModel = resolveRootOverlayModel(host.options, false); this._html5Target = html5Backend.createDropTarget(host.element, { className: 'dv-drop-target-edge', From 766e837ad8cd7f21616c0172a6b9c1be8594bdb2 Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Thu, 16 Jul 2026 23:30:19 +0100 Subject: [PATCH 6/6] fix(dockview-enterprise): use unit separator in pinned-tab rebuild key Join pinned panel ids on the ASCII unit separator (0x1f) instead of a space when building the change-detection key, so ids containing the delimiter can't collide and silently skip a DOM rebuild. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/pinnedTabsService.ts | Bin 31137 -> 31335 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/packages/dockview-enterprise/src/pinnedTabsService.ts b/packages/dockview-enterprise/src/pinnedTabsService.ts index 160bcdb8ced19c68d102b6e34e38f760c84292a8..af9c094d3980c246ef743019fe5fa4af8573db2a 100644 GIT binary patch delta 211 zcmYj~F%AJi7=m6$#tpZ`CeIbuVr1BST6|6t0Baiw7XSbN