Keep @papi/frontend out of the comment list's host-reachable model

extensions/src/legacy-comment-manager/src/comment-list-filters.model.test.ts

@@ -1,6 +1,10 @@
 import { beforeEach, describe, expect, it, vi } from 'vitest';
-import { logger } from '@papi/frontend';
-import type { CommentFilters, CommentPreset, ScopeFilter } from './comment-list-filters.model';
+import type {
+  CommentFilters,
+  CommentPreset,
+  ScopeFilter,
+  WarnFn,
+} from './comment-list-filters.model';
 import {
   applyFilterOverrides,
   buildCommentThreadSelector,
@@ -15,10 +19,6 @@ import {
   scopeFilterToLabelKey,
 } from './comment-list-filters.model';
 
-vi.mock('@papi/frontend', () => ({
-  logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() },
-}));
-
 const scrRef = { book: 'GEN', chapterNum: 3, verseNum: 5 };
 
 function build(preset: CommentPreset, scopeFilter: ScopeFilter = DEFAULT_SCOPE_FILTER) {
@@ -96,39 +96,41 @@ describe('presets', () => {
 });
 
 describe('applyFilterOverrides — legacy axis mapping', () => {
+  // The model takes its diagnostic sink as a parameter rather than importing a logger, so these
+  // assert against the injected sink directly. See `WarnFn`'s doc for why it is injected.
+  let warn: WarnFn;
+
   beforeEach(() => {
-    vi.clearAllMocks();
+    warn = vi.fn();
   });
 
   it('narrows type: conflicts + another active axis onto conflict, not all', () => {
     // The Send/Receive "unresolved conflicts" view: falling back to 'all' would drop the conflict
     // constraint entirely and show the complete unfiltered list, the opposite of what was asked.
-    expect(applyFilterOverrides({ type: 'conflicts', resolved: 'unresolved' })).toEqual({
+    expect(applyFilterOverrides({ type: 'conflicts', resolved: 'unresolved' }, warn)).toEqual({
       preset: 'conflict',
     });
   });
 
   it('logs a warning naming the combination when narrowing an unmatched conflicts combination', () => {
-    applyFilterOverrides({ type: 'conflicts', resolved: 'unresolved' });
-    expect(logger.warn).toHaveBeenCalledWith(
-      expect.stringContaining('unresolved|all|conflicts|all'),
-    );
+    applyFilterOverrides({ type: 'conflicts', resolved: 'unresolved' }, warn);
+    expect(warn).toHaveBeenCalledWith(expect.stringContaining('unresolved|all|conflicts|all'));
   });
 
   it('still maps the exact all|all|conflicts|all row onto conflict without logging', () => {
-    expect(applyFilterOverrides({ type: 'conflicts' })).toEqual({ preset: 'conflict' });
-    expect(logger.warn).not.toHaveBeenCalled();
+    expect(applyFilterOverrides({ type: 'conflicts' }, warn)).toEqual({ preset: 'conflict' });
+    expect(warn).not.toHaveBeenCalled();
   });
 
   it('logs a warning naming the combination when a legacy combination widens to all', () => {
     // 'team' assignment has no counterpart in the new preset model at all.
-    applyFilterOverrides({ assignment: 'team' });
-    expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('all|all|all|team'));
+    applyFilterOverrides({ assignment: 'team' }, warn);
+    expect(warn).toHaveBeenCalledWith(expect.stringContaining('all|all|all|team'));
   });
 
   it('does not log for an exactly-matched legacy combination', () => {
-    applyFilterOverrides({ resolved: 'unresolved' });
-    expect(logger.warn).not.toHaveBeenCalled();
+    applyFilterOverrides({ resolved: 'unresolved' }, warn);
+    expect(warn).not.toHaveBeenCalled();
   });
 });
 
@@ -166,8 +168,8 @@ describe('applyFilterOverrides', () => {
       // shape here so the reproduction case can be expressed at all.
       // eslint-disable-next-line no-type-assertion/no-type-assertion
       const overrides = malformed as Partial<CommentFilters>;
-      expect(() => applyFilterOverrides(overrides)).not.toThrow();
-      expect(applyFilterOverrides(overrides)).toEqual(DEFAULT_COMMENT_FILTERS);
+      expect(() => applyFilterOverrides(overrides, vi.fn())).not.toThrow();
+      expect(applyFilterOverrides(overrides, vi.fn())).toEqual(DEFAULT_COMMENT_FILTERS);
     },
   );
 });

extensions/src/legacy-comment-manager/src/comment-list-filters.model.ts

@@ -1,4 +1,3 @@
-import { logger } from '@papi/frontend';
 import type {
   LegacyCommentThreadSelector,
   CommentPreset,
@@ -11,6 +10,18 @@ import type { LocalizeKey } from 'platform-bible-utils';
 
 export type { CommentPreset, CommentFilters, ScopeFilter, LegacyCommentFilters, LegacyScopeFilter };
 
+/**
+ * Sink for a diagnostic this module emits while resolving untrusted filter input (see
+ * {@link presetFromLegacyAxes}). Injected by the caller rather than logged directly, because this
+ * module is reachable from the extension's backend entry point (`main.ts` imports
+ * {@link presetToLabelKey} and {@link scopeFilterToLabelKey} for its menu contributions) as well as
+ * from the web views. `@papi/frontend` is a webpack external the extension host's `require` shim
+ * rejects, so importing a logger here fails the whole extension's activation — see
+ * `extension-host-import-boundary.test.ts`, which pins that boundary. Every caller that can reach
+ * the legacy mapping runs in a web view and passes `logger.warn` from `@papi/frontend`.
+ */
+export type WarnFn = (message: string) => void;
+
 // Filter constants, types, and the selector mapping — shared between the presentational panel (which
 // renders the filter toolbar) and the web view (which uses the values to build its comment-thread
 // query). Kept free of React/DOM dependencies so the mapping is unit-testable.
@@ -240,10 +251,11 @@ const LEGACY_AXES_TO_PRESET: Partial<Record<string, CommentPreset>> = {
  * Any other combination with no counterpart at all — e.g. `assignment: 'team'`/`'unassigned'`
  * (dropped entirely by the new model), `type: 'comments'` (no preset excludes conflicts), or a mix
  * of active axes the table doesn't recognize — widens to `'all'` rather than throw or guess which
- * axis to drop. Either fallback logs, naming the combination: this is the direction nobody notices
- * without one, since a silent widen just shows extra rows rather than failing loudly.
+ * axis to drop. Either fallback reports through `warn`, naming the combination: this is the
+ * direction nobody notices without one, since a silent widen just shows extra rows rather than
+ * failing loudly. See {@link WarnFn} for why the sink is injected instead of logged here.
  */
-function presetFromLegacyAxes(legacy: LegacyCommentFilters): CommentPreset {
+function presetFromLegacyAxes(legacy: LegacyCommentFilters, warn: WarnFn): CommentPreset {
   const key = [
     legacy.resolved ?? 'all',
     legacy.read ?? 'all',
@@ -255,17 +267,15 @@ function presetFromLegacyAxes(legacy: LegacyCommentFilters): CommentPreset {
   if (exactMatch) return exactMatch;
 
   if (legacy.type === 'conflicts') {
-    logger.warn(
+    warn(
       `Legacy comment filter combination "${key}" has no exact preset match; narrowing to ` +
         `'conflict' (dropping the other axis) rather than widening to 'all' (which would also ` +
         `drop the conflict constraint).`,
     );
     return 'conflict';
   }
 
-  logger.warn(
-    `Legacy comment filter combination "${key}" has no matching preset; widening to 'all'.`,
-  );
+  warn(`Legacy comment filter combination "${key}" has no matching preset; widening to 'all'.`);
   return DEFAULT_COMMENT_FILTERS.preset;
 }
 
@@ -295,13 +305,15 @@ function isNewCommentFiltersShape(
  * so both kinds of untrusted input are handled here rather than repeated at each call site:
  *
  * - The deprecated {@link LegacyCommentFilters} four-axis shape (detected by the absence of a `preset`
- *   key) is mapped onto its matching preset via {@link presetFromLegacyAxes}.
+ *   key) is mapped onto its matching preset via {@link presetFromLegacyAxes}, which reports an
+ *   inexact mapping through `warn` (see {@link WarnFn}).
  * - A `preset` this build doesn't recognize — from a newer build, or a malformed value crossing the
  *   command/message bus — resolves to the default rather than reaching
  *   {@link buildCommentThreadSelector}, whose exhaustiveness guard throws on an unhandled preset.
  */
 export function applyFilterOverrides(
-  overrides?: Partial<CommentFilters> | LegacyCommentFilters,
+  overrides: Partial<CommentFilters> | LegacyCommentFilters | undefined,
+  warn: WarnFn,
 ): CommentFilters {
   if (!overrides) return { ...DEFAULT_COMMENT_FILTERS };
 
@@ -314,7 +326,7 @@ export function applyFilterOverrides(
     return { preset: resolvedPreset };
   }
 
-  return { preset: presetFromLegacyAxes(overrides) };
+  return { preset: presetFromLegacyAxes(overrides, warn) };
 }
 
 /**

extensions/src/legacy-comment-manager/src/comment-list-web-view-message.util.test.ts

@@ -1,4 +1,5 @@
 import { describe, expect, it, vi } from 'vitest';
+import { logger } from '@papi/frontend';
 import type {
   CommentPreset,
   LegacyCommentFilters,
@@ -7,8 +8,9 @@ import type {
 import { DEFAULT_COMMENT_FILTERS, DEFAULT_SCOPE_FILTER } from './comment-list-filters.model';
 import { resolveSetFiltersMessage } from './comment-list-web-view-message.util';
 
-// comment-list-filters.model.ts (imported transitively via resolveSetFiltersMessage) logs a warning
-// when a legacy combination has no matching preset -- see presetFromLegacyAxes.
+// This util is the web-view-side boundary that supplies the `warn` sink comment-list-filters.model.ts
+// takes as a parameter (the model itself must not import a logger -- see `WarnFn`'s doc and
+// extension-host-import-boundary.test.ts).
 vi.mock('@papi/frontend', () => ({
   logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() },
 }));
@@ -164,6 +166,14 @@ describe('resolveSetFiltersMessage — legacy shapes', () => {
     });
   });
 
+  it('routes an unmatched legacy combination to the logger', () => {
+    // The model reports through an injected sink rather than logging itself, so this pins the half
+    // of that arrangement the model can no longer guarantee on its own: that this web-view caller
+    // actually wires the sink up to a real logger instead of swallowing the diagnostic.
+    resolveSetFiltersMessage({ filters: { assignment: 'team' } }, current);
+    expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('all|all|all|team'));
+  });
+
   it("maps the legacy 'unfiltered' scope onto 'all-books', its exact replacement", () => {
     const resolved = resolveSetFiltersMessage({ scopeFilter: 'unfiltered' }, current);
     expect(resolved.scopeFilter).toBe('all-books');

extensions/src/legacy-comment-manager/src/comment-list-web-view-message.util.ts

@@ -1,3 +1,4 @@
+import { logger } from '@papi/frontend';
 import type {
   CommentFilters,
   LegacyCommentFilters,
@@ -32,7 +33,8 @@ export type CurrentCommentListView = {
  * ({@link LegacyCommentFilters}, {@link LegacyScopeFilter}) for backward compatibility with
  * out-of-repo senders; `applyFilterOverrides`/`resolveScopeFilter` map both onto the current model
  * and normalize anything unrecognized to its default, so this function never passes an invalid
- * preset or scope through to the caller.
+ * preset or scope through to the caller. This function runs only in a web view, so it supplies the
+ * `warn` sink `applyFilterOverrides` needs (see {@link WarnFn}).
  */
 export function resolveSetFiltersMessage(
   message: {
@@ -46,7 +48,7 @@ export function resolveSetFiltersMessage(
   scopeFilter: ScopeFilter;
   scopeFilterChanged: boolean;
 } {
-  const filters = applyFilterOverrides(message.filters);
+  const filters = applyFilterOverrides(message.filters, (warning) => logger.warn(warning));
   const scopeFilter = resolveScopeFilter(message.scopeFilter);
   return {
     filters,

extensions/src/legacy-comment-manager/src/comment-list.web-view.tsx

@@ -267,7 +267,7 @@ global.webViewComponent = function CommentListWebView({
   const initialOverrideRef = useRef<CurrentCommentListView | undefined>(
     initialFilters !== undefined || initialScopeFilter !== undefined
       ? {
-          filters: applyFilterOverrides(initialFilters),
+          filters: applyFilterOverrides(initialFilters, (warning) => logger.warn(warning)),
           scopeFilter: resolveScopeFilter(initialScopeFilter),
         }
       : undefined,