fix(vue): F89 JSDoc fix, F90 dual-script merge, F92 lang plumbing (#1936)#2050
Conversation
|
@prajapatisparsh is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 10589 tests passed 16 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
magyargergo
left a comment
There was a problem hiding this comment.
PR #2050 tri-review — Vue SFC dual-script merge & JS/TS routing
Methods (3): GitNexus reviewer swarm + Compound-Engineering personas (both Claude) and Codex (the one independent engine). Engine breakdown: 8 Claude lanes — correctness, adversarial, maintainability, testing, risk, test/CI, plus a Vue-SFC expert and a TypeScript/tree-sitter expert added at your request (risk & test/CI ended mid-investigation; their domains were covered by the other lanes) — and Codex live. Codex ran as a foreground, not-prompted-from-Claude-output call and independently confirmed all three headline findings. Note on independence: the two Claude methods share priors, so Claude-only agreement is "consistent across personas," not independent confirmation; the strong signal here is Codex + a Claude lane + a reproduction.
What's solid (validated — credit where due): the extractVueScript merge logic and the new lang field work as intended on the sequential (<15-file) path, and the extractor unit tests pass. Two suspected problems were refuted and excluded (validation is a feature): (a) feeding a TS-grammar cached tree into emitJsScopeCaptures does not corrupt captures — it is guarded by jsCachedTreeMatchesGrammar and re-parsed; (b) the F89 lineOffset doc change is accurate — countNewlines(precedingText)+1 is genuinely 1-based and the consumer math (+ node 0-based row) is correct, no off-by-one introduced.
Headline (P1 — reproduced)
For a .vue with an explicit <script lang="js"> block followed by a <script setup lang="ts"> block, extractVueScript merges both and returns lang = blocks[0].lang = 'js', so emitVueScopeCaptures routes the combined JS+TS text to the JavaScript grammar. Reproduced with the repo's pinned grammars on that merged content: tree-sitter-javascript → 1 ERROR + 1 missing node, the interface fails to parse; tree-sitter-typescript → 0 errors, clean. This is a regression — before this PR only the <script setup> (TS) block was extracted and parsed as TS. (Trigger needs an explicit lang="js"; an absent lang routes to TS and is safe.) Codex and the adversarial lane independently confirmed/reproduced it.
Inline findings
- [P1]
captures.ts:74/vue-sfc-extractor.ts:269— mixed-lang merged content routed to one grammar viablocks[0].lang(the headline above). - [P2]
captures.ts:62-63—setupLangis read but never set by any caller (the bridge passes only{ sourceKind }; theLanguageProvidercontract type omits it; repo-wide grep finds zero setters). So in worker mode (≥15 files — the large-repo path this branch targets) every.vueroutes toemitTsScopeCaptures, silently losing the JS-only passes (CJSrequire()decomposition, JSDoc type-bindings). Not a crash and not a regression vs base — but F92's routing fix doesn't reach the worker path, and the comment "the parse worker passes langs via sourceMeta.setupLang" is actively misleading. - [P2]
vue-sfc-extractor.ts:266—lineOffset = blocks[0].lineOffsetuses source order whilecombinedContentis built fromordered(non-setup first); divergent when<script setup>precedes<script>, and the single-newline join collapses the inter-block gap so the second block's lines are off regardless. Affects user-visible line numbers (jump-to-line / call-site annotations), not graph topology. - [P2/P3] new fixtures —
vue-dual-script/App.vueandvue-js-lang/App.vueare referenced by no test and lack thesrc/mini-repo layout the integration suite uses; their natural home istest/integration/resolvers/vue-scope.test.ts(which currently runs onlyvue-composition-api/vue-options-api/vue-cross-file).
Lower-priority / notes
- Absent
lang=""routes to TS even though Vue's default script lang is JS — harmless (TS is a superset) but plain<script>JS also misses the JS-only passes; consider routing'' | 'js' | 'jsx'→ JS. lang="jsx"/"tsx"(newly advertised in the doc) route to the non-JSX TS grammar (grammar is chosen by the.vueextension) → JSX would ERROR-node. Pre-existing; consider trimming the doc claim.- Stale doc:
languages/vue/index.ts:40-42still says "only<script setup>is processed… the non-setup block is skipped" — contradicts the new merge (that file is not in this diff). - Comment at
vue-sfc-extractor.ts:267-268("the template compiler will catch it") — GitNexus never runs the Vue template compiler; mismatched-lang blocks are silently mis-parsed here.
Test gaps
No test exercises the worker (pre-extracted-script) routing, the full-pipeline JS-lang or dual-script paths (the new fixtures would, if wired), <script setup>-before-<script> ordering, or symbol line numbers for merged blocks. The merge test asserts presence (toContain) but not order.
CI: all green except Vercel (deploy authorization, not a code issue) — typecheck/lint/tests/coverage pass. Coverage of this review: the full 5-file diff was read and the headline reproduced; the PR head moved mid-review (a main merge bringing in unrelated MCP work, #2049) but no Vue file changed, so these findings apply to the current head.
Automated multi-tool digest (GitNexus swarm + Compound-Engineering personas + Codex). Verify before acting; inline anchors mark the most load-bearing lines.
| @@ -0,0 +1,7 @@ | |||
| <template> | |||
There was a problem hiding this comment.
[P2/P3 — testing, code-read] This fixture (and vue-dual-script/App.vue) is referenced by no test — a repo-wide grep finds zero consumers. The integration suite test/integration/resolvers/vue-scope.test.ts loads fixtures by explicit path and runs only vue-composition-api / vue-options-api / vue-cross-file; these new dirs also lack the src/ mini-repo layout those use. So the JS-lang routing and the dual-script merge are covered only by the pure-extractVueScript unit tests — never end-to-end, and never on the worker path (which would expose the dead setupLang).
Actionable path: add describe blocks to vue-scope.test.ts that run runPipelineFromRepo against these fixtures and assert symbols from both blocks / JS-style captures appear — or remove the fixtures. [code-read]
|
on it |
|
There are two failures |
F89: JSDoc corrected (0-based → 1-based, behavior unchanged)
F90: merge <script> + <script setup> blocks (was: only setup)
F92: plumb lang through to select JS vs TS captures
F91 done via feat(vue): migrate Vue SFC to scope-based resolution (RFC #909 Ring 3, closes #940) #1950
29 unit tests pass, 1 integration test suite blocked by pre-existing tree-sitter-dart import