feat: add TypeScript and TSX analyzer#610
feat: add TypeScript and TSX analyzer#610davidsuarezcdo wants to merge 1 commit intoFalkorDB:stagingfrom
Conversation
|
@davidsuarezcdo is attempting to deploy a commit to the FalkorDB Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughTypeScript language support is integrated into the source analyzer framework. A new TypeScriptAnalyzer class implements parsing, entity extraction, and symbol resolution for .ts and .tsx files using tree-sitter. The analyzer is registered in the main analyzer dispatcher and configured with LSP support. A tree-sitter-typescript dependency is added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![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
🧹 Nitpick comments (2)
api/analyzers/source_analyzer.py (1)
30-32: Consider sharing a single TypeScriptAnalyzer instance for.tsand.tsx.Currently, two separate
TypeScriptAnalyzer()instances are created. While functionally correct, sharing a single instance (like Python does for.py) would be more memory efficient.However, note that the more significant concern is whether the TypeScript grammar correctly parses TSX files (flagged separately in
analyzer.py).♻️ Proposed fix to share instance
+_ts_analyzer = TypeScriptAnalyzer() analyzers: dict[str, AbstractAnalyzer] = { # '.c': CAnalyzer(), # '.h': CAnalyzer(), '.py': PythonAnalyzer(), '.java': JavaAnalyzer(), - '.cs': CSharpAnalyzer(), - '.ts': TypeScriptAnalyzer(), - '.tsx': TypeScriptAnalyzer()} + '.cs': CSharpAnalyzer(), + '.ts': _ts_analyzer, + '.tsx': _ts_analyzer}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/analyzers/source_analyzer.py` around lines 30 - 32, The mapping currently constructs two separate TypeScriptAnalyzer() instances for '.ts' and '.tsx'; change it to create a single TypeScriptAnalyzer instance (e.g., ts_analyzer = TypeScriptAnalyzer()) and reuse that instance for both keys so both '.ts' and '.tsx' reference the same TypeScriptAnalyzer object (update the dict entries that reference TypeScriptAnalyzer() accordingly).api/analyzers/typescript/analyzer.py (1)
5-5: Replace star import with explicit imports.The star import makes it unclear which names are being used and triggers static analysis warnings (F403, F405). Based on usage in this file, explicitly import the required entities.
♻️ Proposed fix
-from ...entities import * +from ...entities import Entity, File🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/analyzers/typescript/analyzer.py` at line 5, Replace the wildcard import "from ...entities import *" in analyzer.py with an explicit import list containing only the symbols actually referenced in this file; open analyzer.py, identify every name used from the entities module (types, classes, constants, e.g., any Entity/Result/Error types you reference) and change the import to "from ...entities import NameA, NameB, NameC" accordingly, then run the linter/pytest to ensure F403/F405 warnings are resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/analyzers/typescript/analyzer.py`:
- Around line 19-23: The add_dependencies function currently ignores the files
parameter, calls npm without check=True so failures are silent, and uses the
bare "npm" executable which is a security risk; update add_dependencies to (1)
rename or use the files parameter (or change it to _files) to satisfy linting,
(2) resolve the full npm path via shutil.which and raise/log a clear error if
not found, and (3) call subprocess.run with check=True (and optionally
capture_output=True) inside a try/except to handle and surface installation
failures (log or re-raise with context), referencing the add_dependencies method
and the path argument to locate where to implement these changes.
---
Nitpick comments:
In `@api/analyzers/source_analyzer.py`:
- Around line 30-32: The mapping currently constructs two separate
TypeScriptAnalyzer() instances for '.ts' and '.tsx'; change it to create a
single TypeScriptAnalyzer instance (e.g., ts_analyzer = TypeScriptAnalyzer())
and reuse that instance for both keys so both '.ts' and '.tsx' reference the
same TypeScriptAnalyzer object (update the dict entries that reference
TypeScriptAnalyzer() accordingly).
In `@api/analyzers/typescript/analyzer.py`:
- Line 5: Replace the wildcard import "from ...entities import *" in analyzer.py
with an explicit import list containing only the symbols actually referenced in
this file; open analyzer.py, identify every name used from the entities module
(types, classes, constants, e.g., any Entity/Result/Error types you reference)
and change the import to "from ...entities import NameA, NameB, NameC"
accordingly, then run the linter/pytest to ensure F403/F405 warnings are
resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c0d19f3-2266-4cdf-9632-367d1b34a8a5
📒 Files selected for processing (4)
api/analyzers/source_analyzer.pyapi/analyzers/typescript/__init__.pyapi/analyzers/typescript/analyzer.pypyproject.toml
| def add_dependencies(self, path: Path, files: list[Path]): | ||
| if Path(f"{path}/node_modules").is_dir(): | ||
| return | ||
| if Path(f"{path}/package.json").is_file(): | ||
| subprocess.run(["npm", "install"], cwd=str(path)) |
There was a problem hiding this comment.
Add error handling for npm install and consider security implications.
- The
filesparameter is unused (Ruff ARG002). subprocess.runwithoutcheck=Truesilently ignores failures.- Using partial executable path "npm" (Ruff S607) could be a security concern in certain environments.
🛡️ Proposed fix with error handling
- def add_dependencies(self, path: Path, files: list[Path]):
+ def add_dependencies(self, path: Path, files: list[Path]): # noqa: ARG002
if Path(f"{path}/node_modules").is_dir():
return
if Path(f"{path}/package.json").is_file():
- subprocess.run(["npm", "install"], cwd=str(path))
+ try:
+ subprocess.run(["npm", "install"], cwd=str(path), check=True)
+ except subprocess.CalledProcessError as e:
+ logger.warning(f"npm install failed: {e}")🧰 Tools
🪛 Ruff (0.15.5)
[warning] 19-19: Unused method argument: files
(ARG002)
[error] 23-23: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/analyzers/typescript/analyzer.py` around lines 19 - 23, The
add_dependencies function currently ignores the files parameter, calls npm
without check=True so failures are silent, and uses the bare "npm" executable
which is a security risk; update add_dependencies to (1) rename or use the files
parameter (or change it to _files) to satisfy linting, (2) resolve the full npm
path via shutil.which and raise/log a clear error if not found, and (3) call
subprocess.run with check=True (and optionally capture_output=True) inside a
try/except to handle and surface installation failures (log or re-raise with
context), referencing the add_dependencies method and the path argument to
locate where to implement these changes.
1 participant
Summary
TypeScriptAnalyzerfollowing the exact same pattern asPythonAnalyzer,JavaAnalyzer, andCSharpAnalyzer.tsand.tsxfile extensions (shared analyzer instance)class_declaration,interface_declaration,function_declaration, andmethod_definitionentitiesextends), interface implementation (implements), method calls, parameters, and return types for LSP symbol resolutiontypescript-language-serverviamultilspyinsecond_pass()for cross-file symbol resolutiontree-sitter-typescript>=0.23.2,<0.24.0topyproject.tomlTest plan
TypeScriptAnalyzerinstantiates correctly (from api.analyzers.typescript.analyzer import TypeScriptAnalyzer).tsand.tsxappear inSourceAnalyzer.supported_types()analyze_sourcesagainst a TypeScript project and verify classes/functions/interfaces appear in the graphtypescript-language-serverresolves cross-file symbols (requirestypescript-language-serverinstalled globally)Summary by CodeRabbit
New Features
Chores