fix(plugins): keep the plugin query tests off the plugin registry - #1888
Conversation
The read-only query API added in #1784 is exercised through `createAppAPI`, which lives in `usePlugins.ts`. That module imports the whole built-in plugin registry, so loading it in a Node test pulled in MapCanvas, CesiumCanvas, and every `maplibre-*` plugin: 39 browser-only modules, none of them meaningfully exercised, all of them newly counted by the coverage reporter. Coverage is reported only over files a test actually imports, so those 39 files landed in the denominator at 1-30% function coverage and dropped the total from 72.90% to 60.36%, under the 63% floor. CI has been red on `main` since, and the report is measuring module reachability rather than how well the code is tested. Move the six query methods and `readPluginSelection` into `lib/plugin-layer-queries.ts`, which needs only the store, and have `createAppAPI` spread them in. The test imports that module directly and no longer stubs `maplibre-gl`, `window`, `sessionStorage`, or `localStorage` to get off the ground. Same reasoning, and the same shape, as `geo-editor-geometry.ts` in `@geolibre/plugins`, which is already kept free of the Geoman/MapLibre runtime so it can be unit-tested under Node. `SKETCHES_SOURCE_KIND` gets a subpath export alongside the five already in that package, so the test can share the constant instead of repeating the string literal as it did before. The wiring into the plugin-facing API is now a typed spread rather than something this test asserts; `tsc` covers it on every build. Coverage returns to 82.84% lines / 84.45% branches / 72.55% functions (baseline before #1784: 82.89 / 84.46 / 72.90), with the counted file set back from 444 to 406. The one addition is the extracted module itself, at 100% lines and 95.45% functions. All 8 tests still pass.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change moves plugin layer-query and selection methods into ChangesPlugin query API
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR moves plugin query logic out of the browser-only registry path and updates its test coverage; the reported checks pass, and no actionable merge-blocking risk remains beyond normal review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code reviewI reviewed the diff ( Bugs: None found. The extracted Security: None found. Performance: None found. This is a pure code-motion refactor with no new allocations or hot-path changes. Quality:
CLAUDE.md: The change follows the documented pattern of isolating store-only logic from browser/runtime-heavy modules (explicitly citing the No inline comments were posted — this PR is a clean, well-scoped mechanical refactor that does exactly what its description claims, with no correctness, security, or performance concerns identified. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
🔍 Cloudflare PR preview
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
mainhas been red since #1784. This restores it.What broke
#1784 added
tests/plugin-query-api.test.ts, which reaches the new read-only query API throughcreateAppAPIinusePlugins.ts. That module imports the entire built-in plugin registry, so loading it dragged inMapCanvas,CesiumCanvas, and everymaplibre-*plugin.Coverage is reported only over files a test actually imports (as
CLAUDE.mdnotes, a module with no test does not appear at all rather than as 0%). So those 39 browser-only modules entered the denominator at 1-30% function coverage:3697baf8)6093aa80)That is under the 63% floor, so
npm run cifails. Worst offenders:MapCanvas.tsxat 2.50% functions,maplibre-duckdb.tsat 1.67%,maplibre-openaerialmap.tsat 2.38%.Nothing got less tested. #1784 added 256 lines of tests. The metric moved because the denominator grew, so the gate was measuring module reachability rather than test quality.
The fix
Move the six query methods and
readPluginSelectionintoapps/geolibre-desktop/src/lib/plugin-layer-queries.ts, which needs only the store, and havecreateAppAPIspread them in. The test imports that module directly.This follows the precedent already in the tree:
geo-editor-geometry.tsis documented as "Kept free of the Geoman/MapLibre runtime imports ... so they can be unit-tested under Node without a browser environment."Knock-on cleanups:
maplibre-glmodule-hook shim and itswindow/location/sessionStorage/localStorageglobals. It no longer needs a browser to load, and runs in ~7ms.SKETCHES_SOURCE_KINDgets a subpath export alongside the five already in@geolibre/plugins, so the test shares the constant instead of repeating the"geoeditor-sketches"literal as it did before.Trade-off
The test now covers
createPluginLayerQueriesrather than asserting thatcreateAppAPIexposes these methods. That wiring is a typed spread, sotscenforces it on every build. Testing it at runtime is what requires loading the whole registry, which is the thing this PR is removing.Result
The single added file is the extracted module, at 100% lines / 95.45% functions. All 8 tests pass,
npm run lintis unchanged (0 errors, the same 40 pre-existing warnings),npm run buildis clean, andpre-commit run --files ...passes including thenpm-buildhook.Note on a separate flake
While diagnosing this I found that line coverage is nondeterministic run to run on identical sources: 81.82% (
main's CI), 76.47% (a PR's CI on the same tree), 79.78% locally. Per file, the function column is identical across runs while the line column swings, which is why one run reported an extra "76.47% line coverage does not meet threshold of 78%" error that another did not. The 78% line floor can therefore redden unrelated PRs. Out of scope here; worth its own issue.Summary by CodeRabbit
New Features
Tests