Skip to content

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

Merged
mattgetgen merged 1 commit into
mainfrom
chore/legacy-comment-manager-host-import-boundary
Sep 23, 2026
Merged

mattgetgen merged 1 commit into
mainfrom
chore/legacy-comment-manager-host-import-boundary

Conversation

@mattgetgen

@mattgetgen mattgetgen commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Since #2840 merged, legacyCommentManager fails to activate in development builds. The extension host throws Requiring other than papi is not allowed in extensions! Rejected require('@papi/frontend'), so the comment list and the Comments panel never register their commands or web views.

comment-list-filters.model.ts is shared: main.ts imports presetToLabelKey and scopeFilterToLabelKey for its menu contributions, while the web views import the rest. #2840 added a top-level import { logger } from '@papi/frontend' for two logger.warn calls in presetFromLegacyAxes, which only the web-view side reaches. @papi/frontend is a webpack external the host's require shim rejects, so the import landed in the backend bundle.

Production builds are fine — the minifier drops the unreachable function and its import — which is why CI never noticed: test.yml, package-main.yml and publish.yml all run build:extensions:production. The plain npm run build the README documents builds extensions in development mode, so anyone following it gets a dead comment list.

Why review this

Not part of current epic. It unblocks the three cross-kind zoom e2e specs in #2849 (PT-4585), which cannot run until the comment list opens again, and fixes the pre-existing comment-list-content-zoom.spec.ts failing the same way. Until it lands, anyone developing against the comment list in a dev build is working blind — the only signs are one error-level log line and a small "extension failed to start" notification.

Changes

  • presetFromLegacyAxes / applyFilterOverrides take a WarnFn ((message: string) => void) instead of importing a logger. The two web-view callers — comment-list.web-view.tsx and comment-list-web-view-message.util.ts — supply logger.warn.
  • Adds extension-host-import-boundary.test.ts, copied from platform-scripture (the newer of the two existing copies) and adapted. It walks main.ts's transitive value-import graph against an allowlist with no build step.
  • Test updates: the model's test injects a vi.fn() sink rather than mocking @papi/frontend; the message-util test gains a case pinning that the caller actually wires the sink to a real logger.

Testing

  • The new guard fails on the unfixed tree, naming exactly comment-list-filters.model.ts -> @papi/frontend, and passes after.
  • Verified against the real dev bundle, not just the static test. extensions/dist/legacy-comment-manager/src/main.js built with npm run build:extensions:
    • before — @papi/backend, @papi/frontend, platform-bible-utils
    • after — @papi/backend, platform-bible-utils (both supplied by the shim's EXTENSION_INTERFACE_MODULES)
  • All 268 legacy-comment-manager tests pass; extensions lint and prettier clean; npm run typecheck passes.
  • The new wiring test was mutation-checked — it goes red if the caller passes a no-op sink.

Two notes for reviewers:

  • Extension *.test.ts(x) files are excluded from every extension tsconfig, so npm run typecheck never sees them. I typechecked the changed test files separately against a temporary config; they are clean.
  • The guard also flagged @eten-tech-foundation/scripture-utilities, which I allowlisted rather than "fixed": it is not a webpack external (so it is bundled, never reaching the shim) and has no dependency on react or any other external. This is the same entry, with the same justification, that platform-scripture already carries.

Scope

Only 3 of the 10 extensions with a main.ts now carry this guard. I deliberately did not extend it to the other seven — a sweep of every extension's dev backend bundle found no other real instance, so it would be a pure regression guard with nothing to fix today. (hello-rock3 and hello-someone flag under a naive bundle grep, but those matches sit inside inlined HTML web-view <script> strings, which run in the renderer.) Happy to add it more widely in a follow-up if reviewers want it.

AI Involvement

AI-assisted. Claude diagnosed the dev-vs-production divergence, made the WarnFn change and the test updates, and adapted the guard from platform-scripture. I reviewed the diff and the verification evidence above, including the before/after dev bundle comparison.

Risk Level

Low — one extension, no behavior change. The legacy-axes mapping and its two warning messages are byte-identical; only the sink they are written to moved from a module-level import to a parameter. The rest is test-only.

🤖 Generated with Claude Code


This change is Reviewable

comment-list-filters.model.ts is shared: main.ts imports presetToLabelKey
and scopeFilterToLabelKey for its menu contributions, while the web views
import the rest. Its top-level `import { logger } from '@papi/frontend'`
therefore landed in the backend bundle, and @papi/frontend is a webpack
external the extension host's require shim rejects — so activation threw
"Requiring other than papi is not allowed in extensions!" and neither the
comment list nor the Comments panel registered its commands or web views.

Only development builds were affected: the production minifier drops the
unreachable presetFromLegacyAxes and its import, so CI (which builds
extensions in production mode) stayed green while the plain `npm run build`
the README documents produced a dead comment list.

presetFromLegacyAxes/applyFilterOverrides now take a WarnFn parameter, and
the two web-view callers supply logger.warn. Verified against the real dev
bundle: legacy-comment-manager/src/main.js required @papi/frontend before
and requires only @papi/backend and platform-bible-utils after.

Also copies extension-host-import-boundary.test.ts from platform-scripture,
which walks main.ts's value-import graph against an allowlist with no build
step. It fails on the unfixed tree naming this exact import, and passes
after. A sweep of every extension's dev backend bundle confirmed this was
the only real instance.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mattgetgen
mattgetgen merged commit 38aad34 into main Sep 23, 2026
6 checks passed
@mattgetgen
mattgetgen deleted the chore/legacy-comment-manager-host-import-boundary branch September 23, 2026 12:30
jolierabideau added a commit that referenced this pull request Sep 23, 2026
Picks up PT-4575's per-pane content zoom (#2821) and #2851. All source merged
cleanly; only the generated platform-bible-react bundles conflicted, and those
were regenerated rather than hand-merged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants