feat: switch tree-sitter from native N-API to web-tree-sitter (WASM)#76
Merged
Conversation
HrushiBorhade
added
phase-2
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR switches Tree-sitter from native N-API to web-tree-sitter (WASM): root pnpm build allowlist updated, core package deps replaced with Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Chunker
participant Runtime as web-tree-sitter
participant Loader as tree-sitter-wasms
participant Parser as ParserInstance
Caller->>Runtime: ensureInit() (ParserClass.init())
Runtime-->>Caller: ready
Caller->>Loader: resolve/load .wasm for language
Loader-->>Caller: wasm path / buffer
Caller->>Runtime: Language.load(wasm)
Runtime-->>Caller: Language instance
Caller->>Parser: instantiate ParserClass().setLanguage(Language)
Caller->>Parser: parse(source)
Parser-->>Caller: SyntaxTree
Caller->>Caller: extract chunks
Caller->>Parser: parser.delete()
Caller->>Runtime: tree.delete()
Caller-->>Caller: return chunks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/chunker/ast.ts (2)
96-99:⚠️ Potential issue | 🟡 MinorUse the Pino logger instead of
console.warnin this module.
console.warnis used here while the file already usescreateLogger. Keep warning output consistent with structured logging.🧹 Proposed fix
- console.warn( - `[chunker] No AST node types configured for "${language}", using fallback: ${filePath}`, - ); + log.warn( + `[chunker] No AST node types configured for "${language}", using fallback: ${filePath}`, + ); @@ - console.warn( - `[chunker] AST parsed but found 0 matching nodes for "${language}" in: ${filePath}`, - ); + log.warn( + `[chunker] AST parsed but found 0 matching nodes for "${language}" in: ${filePath}`, + );As per coding guidelines
**/*.{ts,tsx}: Use Pino for logging throughout the application.Also applies to: 137-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/chunker/ast.ts` around lines 96 - 99, Replace the console.warn calls with the module's Pino logger so warnings are emitted as structured logs: locate the console.warn usages in this file (the warning that starts with `[chunker] No AST node types configured for "${language}"` and the similar calls around the 137-139 range) and change them to use the existing logger instance created via createLogger (e.g., logger.warn) and pass the same message and context (language and filePath) as structured fields where possible; ensure you use logger.warn(...) consistently throughout the module so all warnings come through Pino.
102-113:⚠️ Potential issue | 🟠 MajorEnsure parser cleanup runs on parse failures.
Parser allocated at line 102 with
new ParserClass()is not freed when parse fails and early returns at line 112, skipping cleanup at lines 133-134. With native tree-sitter N-API bindings, this causes a memory leak.♻️ Proposed fix (move cleanup to
finally)- let tree; - try { - tree = parser.parse(source); - } catch (err: unknown) { - log.error( - `tree-sitter parse failed for ${filePath}: ${err instanceof Error ? err.message : err}`, - ); - return [fallbackChunk(source, filePath, language)]; - } - - const chunks: Chunk[] = []; - - for (const node of tree.rootNode.children) { - if (!nodeTypes.has(node.type)) continue; - - const content = node.text.trim(); - if (content.length === 0) continue; - - chunks.push({ - content, - filePath, - lineStart: node.startPosition.row + 1, - lineEnd: node.endPosition.row + 1, - language, - type: 'ast', - }); - } - - parser.delete(); - tree.delete(); + let tree; + const chunks: Chunk[] = []; + try { + tree = parser.parse(source); + + for (const node of tree.rootNode.children) { + if (!nodeTypes.has(node.type)) continue; + + const content = node.text.trim(); + if (content.length === 0) continue; + + chunks.push({ + content, + filePath, + lineStart: node.startPosition.row + 1, + lineEnd: node.endPosition.row + 1, + language, + type: 'ast', + }); + } + } catch (err: unknown) { + log.error( + `tree-sitter parse failed for ${filePath}: ${err instanceof Error ? err.message : err}`, + ); + return [fallbackChunk(source, filePath, language)]; + } finally { + tree?.delete(); + parser.delete(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/chunker/ast.ts` around lines 102 - 113, Parser instance created with ParserClass and assigned to parser must be cleaned up even on parse failure; move the parser cleanup (the code currently at lines 133-134) into a finally block so that after calling parser.parse(source) (and on the early return that returns fallbackChunk(source, filePath, language)) the parser is always released. Specifically, ensure the new try/catch/finally around parser.parse calls log.error on failure (keeping the existing message using filePath) and then in finally call the parser cleanup method used in this module (e.g., parser.delete(), parser.free(), or parser.close()) so no native tree-sitter resources leak.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/chunker/ast.ts`:
- Around line 17-23: ensureInit currently always imports web-tree-sitter (WASM);
change it to select the parser based on environment by first trying to load the
native Node N-API binding (e.g., require/import 'tree-sitter' or the native
package your project uses) for CLI/node environments and only fall back to
import('web-tree-sitter') for Lambda/container/trigger environments; update the
logic inside ensureInit (referencing ParserClass, initialized and the
ParserClass.init() call) to attempt native load, call ParserClass.init() if
needed, and set initialized=true, with a clear fallback to the WASM import when
native loading fails or an environment variable indicates a container/lambda
runtime.
---
Outside diff comments:
In `@packages/core/src/chunker/ast.ts`:
- Around line 96-99: Replace the console.warn calls with the module's Pino
logger so warnings are emitted as structured logs: locate the console.warn
usages in this file (the warning that starts with `[chunker] No AST node types
configured for "${language}"` and the similar calls around the 137-139 range)
and change them to use the existing logger instance created via createLogger
(e.g., logger.warn) and pass the same message and context (language and
filePath) as structured fields where possible; ensure you use logger.warn(...)
consistently throughout the module so all warnings come through Pino.
- Around line 102-113: Parser instance created with ParserClass and assigned to
parser must be cleaned up even on parse failure; move the parser cleanup (the
code currently at lines 133-134) into a finally block so that after calling
parser.parse(source) (and on the early return that returns fallbackChunk(source,
filePath, language)) the parser is always released. Specifically, ensure the new
try/catch/finally around parser.parse calls log.error on failure (keeping the
existing message using filePath) and then in finally call the parser cleanup
method used in this module (e.g., parser.delete(), parser.free(), or
parser.close()) so no native tree-sitter resources leak.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4592548-5e31-4bf2-bc72-c997352fab23
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.jsonpackages/core/package.jsonpackages/core/src/chunker/ast.ts
Comment on lines
+17
to
+23
| async function ensureInit(): Promise<void> { | ||
| if (initialized) return; | ||
| const mod = await import('web-tree-sitter'); | ||
| ParserClass = mod.default; | ||
| await ParserClass.init(); | ||
| initialized = true; | ||
| } |
There was a problem hiding this comment.
Add environment-aware parser selection instead of always forcing WASM.
Current flow unconditionally initializes WASM parser for all environments. This drops the CLI native-N-API path required by project policy and can regress CLI performance/startup characteristics.
As per coding guidelines **/chunker/**/*.{ts,tsx}: Use tree-sitter with native N-API bindings for AST chunking in CLI, but switch to web-tree-sitter (WASM) for Trigger.dev Lambda/container environments.
Also applies to: 85-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/chunker/ast.ts` around lines 17 - 23, ensureInit currently
always imports web-tree-sitter (WASM); change it to select the parser based on
environment by first trying to load the native Node N-API binding (e.g.,
require/import 'tree-sitter' or the native package your project uses) for
CLI/node environments and only fall back to import('web-tree-sitter') for
Lambda/container/trigger environments; update the logic inside ensureInit
(referencing ParserClass, initialized and the ParserClass.init() call) to
attempt native load, call ParserClass.init() if needed, and set
initialized=true, with a clear fallback to the WASM import when native loading
fails or an environment variable indicates a container/lambda runtime.
- Replaced native tree-sitter + 6 grammar packages with web-tree-sitter + tree-sitter-wasms - WASM grammars loaded at runtime via Language.load() (TypeScript, TSX, JS, Python, Rust, Go, CSS) - Parser initialized async via Parser.init() — chunkAST is now async - Removed native N-API dependencies from pnpm.onlyBuiltDependencies - Compatible versions: web-tree-sitter@0.22.6 + tree-sitter-wasms@0.1.13 - All 32 chunker tests pass, 132/132 non-embedder tests pass - WASM runs anywhere: Node.js, Linux containers, Trigger.dev workers Closes #69
HrushiBorhade
force-pushed
the
phase-2/web-tree-sitter
branch
from
8be27de to
e985ae9
Compare
- Wrapped parser/tree in try/finally to ensure cleanup on all code paths - Replaced console.warn with log.warn for consistent structured logging
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
1 participant
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.
Summary
Replaces native tree-sitter (N-API .node binaries) with web-tree-sitter (WASM) so AST parsing works in any environment — Node.js, Linux containers, Trigger.dev workers.
Changes
packages/core/src/chunker/ast.ts— complete rewrite: dynamicimport('web-tree-sitter'),Parser.init(),Language.load()from WASM filespackages/core/package.json— removed 7 native deps (tree-sitter,tree-sitter-typescript, etc.), addedweb-tree-sitter@0.22.6+tree-sitter-wasms@0.1.13package.json— removed native tree-sitter frompnpm.onlyBuiltDependencieschunkASTis nowasync(was sync) — callers already async, no breaking changeWhy web-tree-sitter@0.22.6?
Pre-built WASM grammar files from
tree-sitter-wasms@0.1.13are compiled against tree-sitter 0.22.x. Usingweb-tree-sitter@0.26.7(latest) causesgetDylinkMetadataerrors due to WASM format incompatibility. v0.22.6 is stable and widely used.Test plan
pnpm typecheckpassesCloses #69
Summary by CodeRabbit
Refactor
Chores