Fix multi-line TS/JS imports getting dropped from the dep graph - #3
Fix multi-line TS/JS imports getting dropped from the dep graph#3nsxdavid wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR fixes a bug where multi-line TypeScript and JavaScript import statements with the ChangesMulti-line TS/JS Import Path Capture
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7eea083689
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/explore.zig`:
- Around line 3613-3623: The continuation-line branch that currently matches any
line with ` from "…"` / ` from '…'` is too permissive and creates false
dependencies; update the conditional so it only runs when the line is actually
part of import/export syntax (e.g., require that the line also contains an
import/export token such as "import " or "export " before extracting the
string). In the block that uses containsAny(line, &.{ " from \"", " from '" }),
augment the check to also assert containsAny(line, &.{ "import ", "export " })
(or an equivalent check that the line belongs to an import/export statement)
before calling extractStringLiteral and appendImportPath so outline.imports and
the dep_graph are only populated for real import/export continuations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 565311fa-bcb0-4efe-a858-50516250caa9
📒 Files selected for processing (2)
src/explore.zigsrc/test_parser.zig
parseTsLine only extracted an import's `from "..."` specifier when it appeared on the same line as `import `. Multi-line ES imports place the `from "..."` clause on a line beginning with `}`, so those dependency paths were dropped (only single-line imports were captured). Add a continuation branch that extracts the specifier from `} from "..."` lines; this also captures `export ... from` re-export dependencies. Adds a red-to-green parser test.
7eea083 to
6a56e61
Compare
|
Moved upstream: justrach#542 |
#3 #6) Three behavior-preserving optimizations on the per-query search/rank path, identified by the perf audit and adversarially verified: #1 (mcp.zig, appendSearchSymbolNudge): the bare-identifier nudge did a full O(U+S) symbol scan (searchSymbols scans the whole symbol_index AND outlines every call) just to test existence. Switched to findAllSymbols — the same helper codedb_symbol uses: ensureSymbolIndex + O(1) symbol_index.get, with the outline scan only when !symbol_index_complete. Output byte-identical (the nudge only checks results.len and prints the query string). #3 (explore.zig, BM25 posting loop): dropped the doc_tf hashmap. doc_best_line already accumulates .count identically per posting, so df = doc_best_line.count() and tf = entry.value_ptr.count — one map instead of two, and the redundant per-doc .get() re-lookup is gone. Values bit-identical. #6 (explore.zig, tier-1 sort): the comparator hashed 4 strings + touched 2 atomics per comparison. Precompute (count, len) per candidate once in O(C) (mirroring tier0_order), then sort a struct-key array — identical predicate (count desc, len asc) and input order, so ranking is unchanged. Also stops the comparator corrupting ContentCache hit/miss stats. zig build test: full suite green (0 failures). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Fixes #1.
parseTsLineonly grabbed the import path off lines that containedimport. Multi-line imports put thefrom "..."on the closing}line, so the dependency never made it into the graph. This adds a small continuation branch that pulls the path off} from "..."lines too. It also catchesexport ... fromre-exports, which are real deps that were getting dropped the same way.The Go parser already handles multi-line import blocks with its own state. TS never got the equivalent. This is the cheap version of that, no cross-line state needed, since the
from "..."clause is the only thing carrying the path.Red to green: the new test asserts a multi-line import lands in
outline.imports. It fails before the change (path missing), passes after. The single-line case sits in the same test as a regression guard.zig build testis green. The only failures are the threetest_mcp--helptests that spawn a child binary and hiterror.SpawnFailedunder WSL/9p, which is environmental and fails on a clean checkout too.Files:
src/explore.zig(parseTsLine),src/test_parser.zig.One function plus a test. No generated artifacts. Branch is off current main. Read CONTRIBUTING and followed it.
Summary by CodeRabbit