refactor(imports): untangle ChoiceView's circular deps so settings views are CI-testable#1254
Merged
Merged
Conversation
…ews are CI-testable ChoiceView and the other settings views could not be mounted in a vitest component test: their import graph formed a 43-module strongly-connected component that esbuild's bundle tolerates but vitest's ESM evaluation order does not, throwing "Class extends value undefined". Two structural causes, both broken here: - 11 leaf modules value-imported main.ts purely to read the static singleton QuickAdd.instance, dragging the entire app into one cycle. Add a leaf holder src/quickAddInstance.ts (imports main as a TYPE only); main.onload publishes the instance via setQuickAddInstance and the leaf modules read it via getQuickAddInstance, so nothing value-imports main anymore. - completeFormatter statically imported three engines that transitively import it back; load them lazily with `await import()` at their async call-sites. The 43-node cycle is gone; only benign recursive-component and mutual-function cycles remain, none with cross-cycle class inheritance. Tests: - ChoiceView.test.ts: render(ChoiceView) in vitest, plus the conditional Then/Else persistence flow through the real save path, complementing the local obsidian-e2e test (which CI cannot run). - importCycles.test.ts: structural guard that fails if any class extends a base value-imported from its own import cycle (proven to catch a regression of either fix, which a behavioural test alone does not). Behaviour-preserving: full vitest suite (1457), tsc, svelte-check, eslint and a production build all pass, and the flow was verified live in the dev vault (plugin loads clean, settings view renders, conditional then-branch persists plain JSON to disk, lazily imported engines execute). Closes #1249
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces static QuickAdd.instance with a runtime accessor (get/set); migrates consumers to call getQuickAddInstance(); defers engine imports in CompleteFormatter via dynamic imports; adds component and import-cycle regression tests ensuring ChoiceView mounts in Vitest. ChangesBreaking circular imports via instance accessor pattern
Circular import regression and validation
sequenceDiagram
participant Plugin as QuickAdd Plugin
participant Accessor as quickAddInstance
participant Formatter as CompleteFormatter
participant Consumer as ChoiceView / UI
Plugin->>Accessor: setQuickAddInstance(this)
Accessor->>Consumer: getQuickAddInstance() when needed
Consumer->>Formatter: create CompleteFormatter(getQuickAddInstance())
Formatter->>Formatter: await import(Single*Engine)
Consumer->>Consumer: render(ChoiceView) succeeds in Vitest
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs:
Suggested labels:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Deploying quickadd with
|
| Latest commit: |
3decb7e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9ac09afe.quickadd.pages.dev |
| Branch Preview URL: | https://fix-1249-untangle-choiceview.quickadd.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main.ts (1)
193-207: ⚖️ Poor tradeoffClear the QuickAdd singleton on
onunload(requires a clearing API).
quickAddInstance.tsstores the active plugin in a module-levelinstance: QuickAdd | undefined, andgetQuickAddInstance()only errors when that value isundefined.setQuickAddInstance(plugin: QuickAdd)can set but cannot clear it (no “clear”/undefinedpath), andsrc/main.ts’sonunload()doesn’t reset it—leaving a stale reference after disable. AddclearQuickAddInstance()(or allowsetQuickAddInstance(plugin: QuickAdd | undefined)) and call it fromonunload().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.ts` around lines 193 - 207, Add a way to clear the module-level QuickAdd singleton and call it from the plugin teardown: update quickAddInstance.ts to export either a new clearQuickAddInstance() that sets the internal instance: QuickAdd | undefined to undefined, or change setQuickAddInstance(plugin: QuickAdd) to accept QuickAdd | undefined so callers can clear it; ensure getQuickAddInstance() still errors when undefined. Then update src/main.ts:onunload() to invoke clearQuickAddInstance() (or setQuickAddInstance(undefined)) alongside the existing cleanup so the stale QuickAdd reference is released on disable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main.ts`:
- Around line 193-207: Add a way to clear the module-level QuickAdd singleton
and call it from the plugin teardown: update quickAddInstance.ts to export
either a new clearQuickAddInstance() that sets the internal instance: QuickAdd |
undefined to undefined, or change setQuickAddInstance(plugin: QuickAdd) to
accept QuickAdd | undefined so callers can clear it; ensure
getQuickAddInstance() still errors when undefined. Then update
src/main.ts:onunload() to invoke clearQuickAddInstance() (or
setQuickAddInstance(undefined)) alongside the existing cleanup so the stale
QuickAdd reference is released on disable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45172d33-c2e2-4a02-91fc-f1c9fef01f65
📒 Files selected for processing (20)
src/engine/MacroChoiceEngine.tssrc/formatters/completeFormatter.tssrc/gui/AIAssistantSettingsModal.tssrc/gui/InputPrompt.test.tssrc/gui/InputPrompt.tssrc/gui/MacroGUIs/AIAssistantCommandSettingsModal.tssrc/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.tssrc/gui/MacroGUIs/UserScriptSettingsModal.tssrc/gui/MathModal.tssrc/gui/choiceList/ChoiceView.test.tssrc/gui/choiceList/persistenceBoundary.svelte.tssrc/gui/choiceList/persistenceBoundary.test.tssrc/gui/suggesters/LaTeXSuggester.tssrc/gui/suggesters/fileSuggester.test.tssrc/gui/suggesters/fileSuggester.tssrc/gui/suggesters/tagSuggester.tssrc/importCycles.test.tssrc/main.tssrc/quickAddInstance.tssrc/utils/macroHelpers.ts
…filter) The Svelte <script>-block extraction regex in the import-cycle guard used `</script>`, which CodeQL's js/bad-tag-filter flags because it misses valid end tags like `</script >`. Allow optional whitespace before `>` (`</script\s*>`). Behaviour-preserving (only widens matches; verified the guard test still passes) and resolves the PR #1254 CodeQL alert. The test parses only our own trusted in-repo Svelte sources, so there was no real security exposure.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/importCycles.test.ts (1)
78-80:⚠️ Potential issue | 🟡 Minor | 💤 Low valueCodeQL warning: regex doesn't match all malformed closing tags.
The comment says this was added "to satisfy CodeQL", but the warning persists. The regex
<\/script\s*>only matches</script>with trailing whitespace, but CodeQL wants it to handle malformed tags like</script [junk]>.For this use case (parsing trusted Svelte files), the current pattern is sufficient, but you can silence the warning by making the closing tag more permissive.
🔧 Proposed fix to satisfy CodeQL
- // Closing tag tolerates whitespace (`</script >`) to satisfy CodeQL - // js/bad-tag-filter; we only parse our own trusted Svelte sources here. - const m = text.match(/<script[^>]*>([\s\S]*?)<\/script\s*>/i); + // Closing tag tolerates any content before `>` to satisfy CodeQL js/bad-tag-filter. + // We only parse our own trusted Svelte sources, so this permissiveness is safe. + const m = text.match(/<script[^>]*>([\s\S]*?)<\/script[^>]*>/i);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/importCycles.test.ts` around lines 78 - 80, Update the closing-tag part of the regex used when matching script content (currently in the expression assigned to m via text.match(/<script[^>]*>([\s\S]*?)<\/script\s*>/i)) to make the closing tag permissive for malformed variants (e.g. allow junk between the tag name and '>'). Replace the `</script\s*>` fragment with a pattern such as `</script\b[^>]*>` so the full regex becomes something like /<script[^>]*>([\s\S]*?)<\/script\b[^>]*>/i to satisfy CodeQL while still safely parsing trusted Svelte sources.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/importCycles.test.ts`:
- Around line 78-80: Update the closing-tag part of the regex used when matching
script content (currently in the expression assigned to m via
text.match(/<script[^>]*>([\s\S]*?)<\/script\s*>/i)) to make the closing tag
permissive for malformed variants (e.g. allow junk between the tag name and
'>'). Replace the `</script\s*>` fragment with a pattern such as
`</script\b[^>]*>` so the full regex becomes something like
/<script[^>]*>([\s\S]*?)<\/script\b[^>]*>/i to satisfy CodeQL while still safely
parsing trusted Svelte sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2f68749-8dd2-4c18-bf02-40b10827c3ae
📒 Files selected for processing (1)
src/importCycles.test.ts
…deQL) CodeQL js/bad-tag-filter re-flagged the closing-tag regex twice (`</script>` then `</script\s*>`) because an HTML end tag can carry ignored attributes up to `>` (e.g. `</script\t\n bar>`). Widening the regex just invites another alert, so drop the tag-matching regex entirely and locate the block with indexOf — js/bad-tag-filter is a regex-only query and now has nothing to bind to. The indexOf scan is also strictly more correct: it concatenates every <script> body (instance + `<script module>`/`<script context="module">`), which the old single-match regex missed, so module-context imports are now seen by the import-cycle guard. Verified byte-identical extraction on all existing .svelte files; guard test, tsc and eslint pass. Resolves the two CodeQL alerts on PR #1254.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1249.
Problem
ChoiceView(and the other settings views) could not be mounted in a vitest component test. Their import graph formed a 43-module strongly-connected component that esbuild's production bundle tolerates but vitest's ESM evaluation order does not — it threwClass extends value undefined(the fatal edge wasVDateInputPrompt extends GenericInputPrompt, reached viaquickAddApi). As a result the ChoiceView save/persistence flow was only covered by a localobsidian-e2etest that CI can't run.Root causes & fix
Two structural causes, identified with an import-graph SCC analysis and broken here:
main.tsgod-module singleton. 11 leaf modules didimport QuickAdd from "./main"purely to read the static singletonQuickAdd.instance— andmain.tsimports the whole app, dragging everything into one cycle. New leaf modulesrc/quickAddInstance.tsholds the instance (importsmainas a type only, fully erased).main.onloadpublishes it viasetQuickAddInstance(this); the 11 leaf modules read it viagetQuickAddInstance(). Nothing value-importsmainanymore, so it drops out of every cycle.completeFormatter ⇄ engines.completeFormatterstatically importedSingleMacroEngine/SingleTemplateEngine/SingleInlineScriptEngine, which transitively import it back. They're now loaded lazily withawait import()at their async call-sites (esbuild inlines these into the single-file bundle).The 43-node cycle is gone — only benign recursive-component (
MultiChoiceListItem ⇄ ChoiceList) and mutual-function (choiceSuggester ⇄ choiceExecutor) cycles remain, none with cross-cycle class inheritance.Tests
src/gui/choiceList/ChoiceView.test.ts—render(ChoiceView)now mounts in vitest, plus the conditional Then/Else persistence flow through ChoiceView's real save path (full nested-fidelity + plain-snapshot assertions). Complements the local e2e test.src/importCycles.test.ts— structural regression guard: builds the value-import graph, runs Tarjan SCC, and fails if anyclass X extends YwhereYis value-imported fromX's own SCC. Proven to catch a regression of either fix (a behavioural test alone does not — the removedcompleteFormatter ⇄ enginecycle is benign-but-fragile today, so reverting that half leaves the whole suite green).persistenceBoundary.test.tswith nested-branch detachment coverage and corrected its now-stale header comment.Verification
Behaviour-preserving. Full vitest suite 1457 passing (was 1451; +6),
tsc --noEmit,svelte-check(0 errors), ESLint, and a production esbuild build all pass.Also verified live in the dev vault (real Obsidian):
ChoiceView(filter, all rows, AddChoiceBox).thenCommands(length 1,elseCommandsempty) todata.jsonas plain JSON with no$stateProxy artifacts.SingleInlineScriptEngineandSingleTemplateEngineexecute correctly at runtime viaapi.format.Release impact
None — behaviour-preserving refactor + tests (no
feat/fix/perf, so no release).main.jsis gitignored and not part of the diff.Summary by CodeRabbit
Refactor
Bug Fixes
Tests