From 6db2a5a08d9f1afaaed72013f94ac0a70252f3ba Mon Sep 17 00:00:00 2001 From: Ben Floyd Date: Wed, 19 Nov 2025 19:24:45 -0700 Subject: [PATCH] wire nav through a facade --- refactor-plan.md | 133 +++++++++++++----- src/features/backlinks/BacklinksSlice.ts | 11 +- src/features/navigation/NavigationFacade.ts | 62 ++++++++ src/features/navigation/NavigationSlice.ts | 13 +- .../__tests__/PluginDebugCommands.test.ts | 69 +++++++++ 5 files changed, 239 insertions(+), 49 deletions(-) create mode 100644 src/features/navigation/NavigationFacade.ts create mode 100644 src/orchestrator/__tests__/PluginDebugCommands.test.ts diff --git a/refactor-plan.md b/refactor-plan.md index 18a1a85..168080b 100644 --- a/refactor-plan.md +++ b/refactor-plan.md @@ -3,7 +3,8 @@ ## Scope - Refactor 1: Align orchestrator and slice contracts with implementation. -- Refactor 2: Factor [`CoalescePlugin`](main.ts:9) into smaller, testable modules. +- Refactor 2: Factor [`CoalescePlugin`](main.ts:10) into smaller, testable modules. +- Refactor 3: Introduce a shared navigation facade used consistently by navigation and backlinks. This file is implementation‑oriented and intended for Code mode to execute. @@ -24,18 +25,19 @@ This file is implementation‑oriented and intended for Code mode to execute. ## Refactor 2 – Factor CoalescePlugin -### Current structure (after refactor step 1–3) +### Current structure (after refactor) -- [`CoalescePlugin`](main.ts:9) now: +- [`CoalescePlugin`](main.ts:10) now: - Uses [`createAndStartOrchestrator()`](src/orchestrator/PluginBootstrap.ts:31) from [`PluginBootstrap`](src/orchestrator/PluginBootstrap.ts:1) in `onload` instead of manually constructing and starting the orchestrator. - Delegates debug commands to [`PluginDebugCommands`](src/orchestrator/PluginDebugCommands.ts:1) via [`attachDebugCommands()`](src/orchestrator/PluginDebugCommands.ts:21) and [`detachDebugCommands()`](src/orchestrator/PluginDebugCommands.ts:108), with a small local `coalesceUpdateLogging` that still calls [`Logger.setGlobalLogging`](src/features/shared-utilities/Logger.ts:1). - - Delegates DOM, workspace, and orchestrator event wiring to [`registerPluginEvents()`](src/orchestrator/PluginEvents.ts:26) in [`PluginEvents`](src/orchestrator/PluginEvents.ts:1). - - Still owns view initialization and duplicate suppression via [`shouldProcessFile()`](main.ts:100), [`updateCoalesceUIForFile()`](main.ts:122), and [`initializeExistingViews()`](main.ts:395). + - Delegates DOM, workspace, and orchestrator event wiring to [`registerPluginEvents()`](src/orchestrator/PluginEvents.ts:26) in [`PluginEvents`](src/orchestrator/PluginEvents.ts:1), passing a callback from the view initializer for UI updates. + - Delegates view initialization and duplicate suppression to [`PluginViewInitializer`](src/orchestrator/PluginViewInitializer.ts:1) instead of owning that logic directly. - New helpers: - [`PluginBootstrap`](src/orchestrator/PluginBootstrap.ts:1) – `createOrchestrator` and `createAndStartOrchestrator`. - [`PluginDebugCommands`](src/orchestrator/PluginDebugCommands.ts:1) – `attachDebugCommands(app, plugin, orchestrator, logger)` and `detachDebugCommands(app)`. - [`PluginEvents`](src/orchestrator/PluginEvents.ts:1) – `registerPluginEvents(app, plugin, orchestrator, logger, updateCoalesceUIForFile?)`. + - [`PluginViewInitializer`](src/orchestrator/PluginViewInitializer.ts:1) – `initializeExistingViews()` and `updateForFile(path)`. ### Target structure @@ -44,27 +46,27 @@ Organize plugin responsibilities into small, focused modules under `src/orchestr - `PluginBootstrap` – constructs [`PluginOrchestrator`](src/orchestrator/PluginOrchestrator.ts:20), initializes slices, and wires settings/logging. - `PluginDebugCommands` – defines functions that attach and detach all `coalesce*` debug helpers on `app`. - `PluginEvents` – encapsulates registration logic for DOM custom events (`coalesce-settings-*`, `coalesce-navigate*`), workspace events, and orchestrator events. -- `PluginViewInitializer` – planned: handles `initializeExistingViews()` and the duplicate‑processing logic for backlinks UI. +- `PluginViewInitializer` – handles initial view discovery and the duplicate‑processing logic for backlinks UI attachment. -After this refactor, [`CoalescePlugin`](main.ts:9) should be largely a thin adapter from Obsidian lifecycle (`onload`, `onunload`) to these helper modules. +After this refactor, [`CoalescePlugin`](main.ts:10) is largely a thin adapter from Obsidian lifecycle (`onload`, `onunload`) to these helper modules. ### Implementation checklist and status 1. Extract a plugin bootstrap module – **Completed** - [`PluginBootstrap`](src/orchestrator/PluginBootstrap.ts:1) provides `createAndStartOrchestrator(app, plugin, config)`. - - [`CoalescePlugin.onload`](main.ts:14) now uses `createAndStartOrchestrator` instead of manual `new PluginOrchestrator(...)` and separate `initialize()` / `start()` calls. + - [`CoalescePlugin.onload`](main.ts:16) now uses `createAndStartOrchestrator` instead of manual `new PluginOrchestrator(...)` and separate `initialize()` / `start()` calls. 2. Extract debug command wiring – **Completed** - [`PluginDebugCommands`](src/orchestrator/PluginDebugCommands.ts:1) now owns all `coalesce*` debug helpers. - - [`CoalescePlugin.setupDebugMethods`](main.ts:86) calls `attachDebugCommands(this.app, this, this.orchestrator, this.logger)` and defines only `coalesceUpdateLogging` locally to call [`Logger.setGlobalLogging`](src/features/shared-utilities/Logger.ts:1). - - [`CoalescePlugin.onunload`](main.ts:452) calls `detachDebugCommands(this.app)` instead of manually deleting each debug property. + - [`CoalescePlugin.setupDebugMethods`](main.ts:91) calls `attachDebugCommands(this.app, this, this.orchestrator, this.logger)` and defines only `coalesceUpdateLogging` locally to call [`Logger.setGlobalLogging`](src/features/shared-utilities/Logger.ts:1). + - [`CoalescePlugin.onunload`](main.ts:174) calls `detachDebugCommands(this.app)` instead of manually deleting each debug property. 3. Extract DOM+workspace+orchestrator event registration – **Completed** - [`PluginEvents`](src/orchestrator/PluginEvents.ts:1) implements `registerPluginEvents(app, plugin, orchestrator, logger, updateCoalesceUIForFile?)`, wiring: - DOM events: `coalesce-settings-collapse-changed`, `coalesce-logging-state-changed`, `coalesce-navigate`, `coalesce-navigate-complete`. - Workspace events: `file-open`, `layout-change`, `active-leaf-change`. - Orchestrator events: `file:opened`, `layout:changed`, `active-leaf:changed`. - - [`CoalescePlugin.registerEventHandlers`](main.ts:190) now delegates to: + - [`CoalescePlugin.registerEventHandlers`](main.ts:106) now delegates to: ```ts registerPluginEvents( @@ -72,38 +74,101 @@ After this refactor, [`CoalescePlugin`](main.ts:9) should be largely a thin adap this, this.orchestrator, this.logger, - this.updateCoalesceUIForFile.bind(this) + this.viewInitializer.updateForFile.bind(this.viewInitializer) ); ``` -4. Extract view initialization and duplicate‑processing logic – **Planned** - - Introduce a `PluginViewInitializer` helper, for example: +4. Extract view initialization and duplicate‑processing logic – **Completed** + - [`PluginViewInitializer`](src/orchestrator/PluginViewInitializer.ts:1) now owns: + - Duplicate suppression logic (previously `shouldProcessFile` in `main.ts`). + - Backlinks UI attachment and settings application logic (previously `updateCoalesceUIForFile`). + - Initial and delayed `file:opened` emission for all markdown views (previously `initializeExistingViews`). + - [`CoalescePlugin`](main.ts:54) creates the view initializer and delegates: + - `this.viewInitializer = createViewInitializer(this.app, this.orchestrator, this.logger);` + - Startup initialization via `this.viewInitializer.initializeExistingViews();`. + +5. Add tests for the new modules – **In progress** + - `PluginDebugCommands` – **Completed**: + - [`PluginDebugCommands.test`](src/orchestrator/__tests__/PluginDebugCommands.test.ts:1) verifies that `attachDebugCommands` attaches representative `coalesce*` helpers on a mocked `app` and that `detachDebugCommands` removes them. + - `PluginEvents` – Planned: + - Verify that firing `coalesce-*` DOM events invokes the expected orchestrator or slice methods (using spies/mocks for `settings`, `backlinks`, `viewIntegration`, and navigation behavior). + - Verify that `file:opened` drives `initializeView` + `attachToDOM` + `setOptions` as expected when slices are present. + - `PluginViewInitializer` – Planned: + - Verify that duplicate‑processing suppression works (previous `shouldProcessFile` semantics preserved). + - Verify that backlinks UI is attached only when the correct slices and active view are present. + +## Refactor 3 – Shared navigation facade + +### Current structure + +- Navigation implementation: + - [`NavigationSlice`](src/features/navigation/NavigationSlice.ts:17) constructs: + - `FileOpener` + - `LinkHandler` + - `NavigationService` + - and exposes a higher-level navigation API (`openPath`, `handleLinkClick`, history, etc.). + - [`BacklinksSlice`](src/features/backlinks/BacklinksSlice.ts:35) previously constructed its own: + - `FileOpener` + - `LinkHandler` + - `NavigationService` + - to implement [`handleNavigation()`](src/features/backlinks/BacklinksSlice.ts:352) via wiki-link text. + +- Shared navigation facade (new): + - [`NavigationFacade`](src/features/navigation/NavigationFacade.ts:1) now provides a lazily-initialized shared set of navigation components: + - `getSharedNavigation(app, baseLogger)` returns `{ navigationService, fileOpener, linkHandler }`. + - `__resetSharedNavigationForTests()` to reset the singleton in tests. + +### Target design + +- There should be exactly one `NavigationService` (and corresponding `FileOpener` / `LinkHandler`) per plugin instance, shared by: + - [`NavigationSlice`](src/features/navigation/NavigationSlice.ts:17) + - [`BacklinksSlice`](src/features/backlinks/BacklinksSlice.ts:35) + - Any future slice that needs navigation behavior. +- Validation, file existence checks, and link processing should flow through this shared stack so navigation behavior is consistent everywhere. + +### Implementation checklist and status + +1. Introduce a NavigationFacade module – **Completed** + - Created [`NavigationFacade`](src/features/navigation/NavigationFacade.ts:1) with: + - `getSharedNavigation(app, baseLogger)`: lazily creates and stores `{ navigationService, fileOpener, linkHandler }` on first call, reuses thereafter. + - `__resetSharedNavigationForTests()`: test helper to reset the shared instance. + +2. Update NavigationSlice to use the shared facade – **Completed** + - [`NavigationSlice`](src/features/navigation/NavigationSlice.ts:17) now imports `getSharedNavigation` and uses it in the constructor: ```ts - const viewInitializer = createViewInitializer(app, orchestrator, logger); - viewInitializer.updateForFile(path); - viewInitializer.initializeExistingViews(); + const shared = getSharedNavigation(app, this.logger); + this.fileOpener = shared.fileOpener; + this.linkHandler = shared.linkHandler; + this.navigationService = shared.navigationService; ``` - - Migrate [`shouldProcessFile()`](main.ts:100), [`updateCoalesceUIForFile()`](main.ts:122), and [`initializeExistingViews()`](main.ts:395) out of `main.ts` into this helper. - - Update: - - `PluginEvents.registerPluginEvents` call sites to use `viewInitializer.updateForFile(...)` instead of calling `updateCoalesceUIForFile` directly. - - `CoalescePlugin.onload` to call `viewInitializer.initializeExistingViews()` rather than its own method. + - It no longer constructs its own `FileOpener` / `LinkHandler` / `NavigationService`; it only configures performance monitoring and history on top. -5. Add tests for the new modules – **Planned** - - `PluginDebugCommands`: - - Verify that `attachDebugCommands` attaches the expected `coalesce*` functions on a mocked `app`, and that `detachDebugCommands` removes them. - - `PluginEvents`: - - Verify that firing `coalesce-*` events invokes the expected orchestrator or slice methods (using spies/mocks for `settings`, `backlinks`, `viewIntegration`, and `navigation` behaviors). - - Verify that `file:opened` drives `initializeView` + `attachToDOM` + `setOptions` as expected when slices are present. - - `PluginViewInitializer` (once added): - - Verify that duplicate‑processing suppression works (`shouldProcessFile` semantics preserved). - - Verify that backlinks UI is attached only when the correct slices and active view are present. +3. Update BacklinksSlice to use the shared facade – **Completed** + - [`BacklinksSlice`](src/features/backlinks/BacklinksSlice.ts:35) now imports `getSharedNavigation` from `../navigation/NavigationFacade` and uses it in the constructor: + + ```ts + const sharedNavigation = getSharedNavigation(this.app, this.logger); + this.navigationService = sharedNavigation.navigationService; + ``` + + - It no longer constructs its own navigation stack; [`handleNavigation()`](src/features/backlinks/BacklinksSlice.ts:352) continues to call `navigationService.openWikiLink(...)`, but now using the shared instance. + +4. Add tests for the navigation facade – **Planned** + - Add a small test module (e.g. [`NavigationFacade.test`](src/features/navigation/__tests__/NavigationFacade.test.ts:1)) that: + - Asserts that two calls to `getSharedNavigation(app, logger)` return the same `navigationService` instance. + - Asserts that `__resetSharedNavigationForTests()` causes a subsequent call to create a new instance. + - Optionally, assert that `NavigationSlice` and `BacklinksSlice` both obtain a `NavigationService` that is strictly the same object in an integration-style test. ## Suggested implementation order (updated) 1. Refactor 1 (orchestrator/slice contracts) – **Completed**. -2. Refactor 2 (CoalescePlugin) – **Partially completed**: - - Done: bootstrap extraction, debug command extraction, event wiring extraction. - - Remaining: view initializer helper and tests for new modules. -3. After these are complete and covered by tests, proceed to the next roadmap items (navigation unification, validation centralization, logging standardization), using this document as living design notes. \ No newline at end of file +2. Refactor 2 (CoalescePlugin) – **Completed**: + - Bootstrap, debug commands, event wiring, and view initialization have all been extracted into helper modules, with `main.ts` acting as a thin lifecycle adapter. +3. Refactor 3 (shared navigation facade) – **Partially completed**: + - Shared navigation facade created and wired into [`NavigationSlice`](src/features/navigation/NavigationSlice.ts:17) and [`BacklinksSlice`](src/features/backlinks/BacklinksSlice.ts:35). + - Remaining: facade-focused tests and potential further centralization of validation once SharedUtilities validation helpers are introduced into the navigation path. +4. Next steps: + - Add tests for navigation facade and remaining helper modules. + - Proceed to centralizing validation (via `SharedUtilitiesSlice.getValidationUtils`) and standardizing logging (via `SharedUtilitiesSlice.getLogger`), continuing to update this document as new refactors land. \ No newline at end of file diff --git a/src/features/backlinks/BacklinksSlice.ts b/src/features/backlinks/BacklinksSlice.ts index 2af5eb9..22c9ff8 100644 --- a/src/features/backlinks/BacklinksSlice.ts +++ b/src/features/backlinks/BacklinksSlice.ts @@ -17,8 +17,7 @@ import { HeaderUI } from './HeaderUI'; import { FilterControls } from './FilterControls'; import { SettingsControls } from './SettingsControls'; import { NavigationService } from '../navigation/NavigationService'; -import { FileOpener } from '../navigation/FileOpener'; -import { LinkHandler } from '../navigation/LinkHandler'; +import { getSharedNavigation } from '../navigation/NavigationFacade'; /** * BacklinksSlice @@ -66,11 +65,9 @@ export class BacklinksSlice implements IBacklinksSlice { this.app = app; this.logger = new Logger('BacklinksSlice'); - // Initialize navigation service used for link-style navigation - const navigationLogger = this.logger.child('Navigation'); - const fileOpener = new FileOpener(this.app, navigationLogger); - const linkHandler = new LinkHandler(this.app, navigationLogger); - this.navigationService = new NavigationService(this.app, fileOpener, linkHandler, navigationLogger); + // Initialize shared navigation service used for link-style navigation + const sharedNavigation = getSharedNavigation(this.app, this.logger); + this.navigationService = sharedNavigation.navigationService; // Set default options this.options = { diff --git a/src/features/navigation/NavigationFacade.ts b/src/features/navigation/NavigationFacade.ts new file mode 100644 index 0000000..d056e33 --- /dev/null +++ b/src/features/navigation/NavigationFacade.ts @@ -0,0 +1,62 @@ +import { App } from 'obsidian'; +import { Logger } from '../shared-utilities/Logger'; +import { NavigationService } from './NavigationService'; +import { FileOpener } from './FileOpener'; +import { LinkHandler } from './LinkHandler'; + +/** + * NavigationFacade + * + * Provides a shared NavigationService (and its underlying FileOpener and + * LinkHandler) so multiple slices (navigation, backlinks, etc.) can coordinate + * navigation through a single abstraction. + * + * This avoids each slice constructing its own NavigationService and keeps + * navigation behavior consistent across the plugin. + */ +export interface SharedNavigation { + navigationService: NavigationService; + fileOpener: FileOpener; + linkHandler: LinkHandler; +} + +let sharedNavigation: SharedNavigation | null = null; + +/** + * Get (or lazily create) the shared navigation components. + * + * The first caller provides the App and a base logger. Subsequent callers + * reuse the same NavigationService / FileOpener / LinkHandler instances. + */ +export function getSharedNavigation( + app: App, + baseLogger: Logger, +): SharedNavigation { + if (sharedNavigation) { + return sharedNavigation; + } + + const navigationLogger = + typeof (baseLogger as any).child === 'function' + ? (baseLogger as any).child('NavigationFacade') + : new Logger('NavigationFacade'); + + const fileOpener = new FileOpener(app, navigationLogger); + const linkHandler = new LinkHandler(app, navigationLogger); + const navigationService = new NavigationService(app, fileOpener, linkHandler, navigationLogger); + + sharedNavigation = { + navigationService, + fileOpener, + linkHandler, + }; + + return sharedNavigation; +} + +/** + * Test-only helper to reset the shared navigation instance. + */ +export function __resetSharedNavigationForTests(): void { + sharedNavigation = null; +} \ No newline at end of file diff --git a/src/features/navigation/NavigationSlice.ts b/src/features/navigation/NavigationSlice.ts index 0fdf72b..aa5bee7 100644 --- a/src/features/navigation/NavigationSlice.ts +++ b/src/features/navigation/NavigationSlice.ts @@ -3,6 +3,7 @@ import { INavigationSlice } from '../shared-contracts/slice-interfaces'; import { NavigationService } from './NavigationService'; import { LinkHandler } from './LinkHandler'; import { FileOpener } from './FileOpener'; +import { getSharedNavigation } from './NavigationFacade'; import { Logger } from '../shared-utilities/Logger'; import { PerformanceMonitor } from '../shared-utilities/PerformanceMonitor'; import { CommonHelpers } from '../shared-utilities/CommonHelpers'; @@ -28,14 +29,10 @@ export class NavigationSlice implements INavigationSlice { this.app = app; this.logger = new Logger('NavigationSlice'); - this.fileOpener = new FileOpener(app, this.logger); - this.linkHandler = new LinkHandler(app, this.logger); - this.navigationService = new NavigationService( - app, - this.fileOpener, - this.linkHandler, - this.logger - ); + const shared = getSharedNavigation(app, this.logger); + this.fileOpener = shared.fileOpener; + this.linkHandler = shared.linkHandler; + this.navigationService = shared.navigationService; this.performanceMonitor = new PerformanceMonitor( this.logger.child('Performance'), diff --git a/src/orchestrator/__tests__/PluginDebugCommands.test.ts b/src/orchestrator/__tests__/PluginDebugCommands.test.ts new file mode 100644 index 0000000..e856675 --- /dev/null +++ b/src/orchestrator/__tests__/PluginDebugCommands.test.ts @@ -0,0 +1,69 @@ +import { App } from 'obsidian'; +import { attachDebugCommands, detachDebugCommands } from '../PluginDebugCommands'; +import { PluginOrchestrator } from '../PluginOrchestrator'; + +describe('PluginDebugCommands', () => { + interface MockApp extends App { + [key: string]: any; + } + + const createMockApp = (): MockApp => { + const app: any = { + workspace: { + getActiveViewOfType: jest.fn().mockReturnValue(null), + getLeavesOfType: jest.fn().mockReturnValue([]), + }, + }; + return app as MockApp; + }; + + const createMockOrchestrator = (): PluginOrchestrator => { + return { + getSlice: jest.fn(), + getState: jest.fn().mockReturnValue({}), + getStatistics: jest.fn().mockReturnValue({}), + emit: jest.fn(), + on: jest.fn(), + off: jest.fn(), + getAllSlices: jest.fn().mockReturnValue({}), + cleanup: jest.fn(), + initialize: jest.fn(), + start: jest.fn(), + stop: jest.fn(), + } as unknown as PluginOrchestrator; + }; + + const createMockLogger = () => ({ + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }); + + it('attaches and detaches coalesce debug commands on the app object', () => { + const app = createMockApp(); + const orchestrator = createMockOrchestrator(); + const logger = createMockLogger(); + const plugin: any = {}; // CoalescePlugin instance is not used directly here + + attachDebugCommands(app, plugin, orchestrator, logger); + + const anyApp = app as any; + + // Spot-check a few representative commands + expect(typeof anyApp.coalesceStatus).toBe('function'); + expect(typeof anyApp.coalesceTestFocus).toBe('function'); + expect(typeof anyApp.coalesceTestDirectFocus).toBe('function'); + expect(typeof anyApp.coalesceOrchestratorStatus).toBe('function'); + expect(typeof anyApp.coalesceSliceStatus).toBe('function'); + + detachDebugCommands(app); + + // Ensure they were removed + expect(anyApp.coalesceStatus).toBeUndefined(); + expect(anyApp.coalesceTestFocus).toBeUndefined(); + expect(anyApp.coalesceTestDirectFocus).toBeUndefined(); + expect(anyApp.coalesceOrchestratorStatus).toBeUndefined(); + expect(anyApp.coalesceSliceStatus).toBeUndefined(); + }); +}); \ No newline at end of file