Fix JS/TS service call resolution and empty-graph reindex#455
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and accuracy of the codebase's graph generation and call resolution capabilities. It addresses a critical issue where incremental indexing could fail to reindex an empty graph, and it refines how JavaScript and TypeScript member calls are resolved, particularly for dependency-injected services and Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Greptile SummaryThis PR fixes two production bugs: (1) Key changes:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_process_files] --> B{force=True?}
B -->|Yes| C[old_hashes = empty]
B -->|No| D[Load hash cache from disk]
D --> E[_should_force_full_reindex]
E --> F{force or no cache?}
F -->|Yes| G[return False]
F -->|No| H[fetch_all: COUNT project symbols]
H --> I{DB query fails?}
I -->|Yes| G
I -->|No| J{results empty?}
J -->|Yes| K[return True]
J -->|No| L{symbol_count == 0?}
L -->|Yes| K
L -->|No| G
K --> M[old_hashes = empty - full reindex]
C --> N[Process all eligible files]
G --> O{old_hashes empty?}
O -->|Yes| N
O -->|No| P[Skip unchanged files via hash]
M --> N
subgraph JS_TS_Resolution[JS/TS Call Resolution]
Q[resolve_function_call] --> R{IIFE or super?}
R -->|No| S{method chain?}
S -->|No| T[_try_resolve_via_imports with class_context]
T --> U{2-part call with this prefix?}
U -->|Yes| V[_try_resolve_method using class_context]
U -->|No| W[_try_resolve_via_local_type]
T --> X{call has separator?}
X -->|No| Y[same-module and trie fallback]
X -->|Yes, unresolved| Z[return None]
end
Prompt To Fix All With AIThis is a comment left during a code review.
Path: codebase_rag/parsers/call_resolver.py
Line: 18
Comment:
**Hardcoded `"this"` string literal**
The project already has `JAVA_KEYWORD_THIS = "this"` in `constants.py` (line 1961). Using a bare `"this"` string here violates the "No Hardcoded Strings" rule. Either reuse the existing constant with a rename, or introduce a new `KEYWORD_THIS` constant.
```suggestion
_JS_INSTANCE_PREFIXES = {cs.KEYWORD_SELF, cs.JAVA_KEYWORD_THIS}
```
**Rule Used:** ## Technical Requirements
### Agentic Framework
-... ([source](https://app.greptile.com/review/custom-context?memory=d4240b05-b763-467a-a6bf-94f73e8b6859))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: codebase_rag/parsers/js_ts/type_inference.py
Line: 134-145
Comment:
**Hardcoded tree-sitter node-type string literals**
`"required_parameter"`, `"optional_parameter"`, and `"accessibility_modifier"` appear as raw string literals in `_collect_constructor_parameter_type` (and also in `_collect_parameter_types` at lines 169-174). No corresponding constants exist in `constants.py` for these three node types. Per the project's "No Hardcoded Strings" rule, they should each be added as constants (e.g. `TS_REQUIRED_PARAMETER`, `TS_OPTIONAL_PARAMETER`, `TS_ACCESSIBILITY_MODIFIER`) and referenced via `cs.`.
The same applies to `"type_annotation"` used in `_extract_type_annotation_name` (line 195).
All four new literals should be centralised in `constants.py`.
**Rule Used:** ## Technical Requirements
### Agentic Framework
-... ([source](https://app.greptile.com/review/custom-context?memory=d4240b05-b763-467a-a6bf-94f73e8b6859))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: codebase_rag/graph_updater.py
Line: 409-433
Comment:
**Inline log message strings not in `logs.py`**
Three new log-message strings are hardcoded directly inside `_should_force_full_reindex` rather than defined in `logs.py`:
- `"Incremental reindex graph-state probe failed for {name}: {error}"`
- `"No graph-state probe results for {name}; forcing full reindex"`
- `"No existing graph symbols found for {name}; ignoring hash cache and forcing full reindex"`
The project's coding standard says files that are not `logs.py` should have almost no string literals, and all log messages should live in `logs.py`. These three should be moved there and referenced via `ls.`.
**Rule Used:** ## Technical Requirements
### Agentic Framework
-... ([source](https://app.greptile.com/review/custom-context?memory=d4240b05-b763-467a-a6bf-94f73e8b6859))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: codebase_rag/parsers/call_resolver.py
Line: 57-63
Comment:
**Suffix-match fallback can resolve to wrong namespace**
The single-suffix-match fallback searches the entire function registry for any entry ending with `ClassName.method_name`. If there is exactly one such entry globally, it is returned — even if it belongs to a completely unrelated namespace.
Example: `class_qn = "proj.a.MyClass"` with method `"doWork"`. If `"proj.b.MyClass.doWork"` exists (same short class name, different package) and is the only match, this fallback silently returns it instead of falling through to `_resolve_inherited_method`. This can produce the very kind of incorrect cross-module edge the PR aims to eliminate.
The safeguard should at minimum verify that `matched_qn` shares the same module prefix as `class_qn` before trusting the single match:
```python
if len(suffix_matches) == 1:
matched_qn = suffix_matches[0]
expected_prefix = cs.SEPARATOR_DOT.join(class_qn.split(cs.SEPARATOR_DOT)[:-1])
if matched_qn.startswith(expected_prefix):
return self.function_registry[matched_qn], matched_qn
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "Fix JS/TS service ca..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| _SEPARATOR_PATTERN = re.compile(r"[.:]|::") | ||
| _CHAINED_METHOD_PATTERN = re.compile(r"\.([^.()]+)$") | ||
| _JS_INSTANCE_PREFIXES = {cs.KEYWORD_SELF, "this"} |
There was a problem hiding this comment.
"this" string literal
The project already has JAVA_KEYWORD_THIS = "this" in constants.py (line 1961). Using a bare "this" string here violates the "No Hardcoded Strings" rule. Either reuse the existing constant with a rename, or introduce a new KEYWORD_THIS constant.
| _JS_INSTANCE_PREFIXES = {cs.KEYWORD_SELF, "this"} | |
| _JS_INSTANCE_PREFIXES = {cs.KEYWORD_SELF, cs.JAVA_KEYWORD_THIS} |
Rule Used: ## Technical Requirements
Agentic Framework
-... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/parsers/call_resolver.py
Line: 18
Comment:
**Hardcoded `"this"` string literal**
The project already has `JAVA_KEYWORD_THIS = "this"` in `constants.py` (line 1961). Using a bare `"this"` string here violates the "No Hardcoded Strings" rule. Either reuse the existing constant with a rename, or introduce a new `KEYWORD_THIS` constant.
```suggestion
_JS_INSTANCE_PREFIXES = {cs.KEYWORD_SELF, cs.JAVA_KEYWORD_THIS}
```
**Rule Used:** ## Technical Requirements
### Agentic Framework
-... ([source](https://app.greptile.com/review/custom-context?memory=d4240b05-b763-467a-a6bf-94f73e8b6859))
How can I resolve this? If you propose a fix, please make it concise.| if param_node.type not in { | ||
| "required_parameter", | ||
| "optional_parameter", | ||
| cs.TS_FORMAL_PARAMETER, | ||
| }: | ||
| return | ||
|
|
||
| has_accessibility_modifier = any( | ||
| child.type == "accessibility_modifier" for child in param_node.children | ||
| ) | ||
| if not has_accessibility_modifier: | ||
| return |
There was a problem hiding this comment.
"required_parameter", "optional_parameter", and "accessibility_modifier" appear as raw string literals in _collect_constructor_parameter_type (and also in _collect_parameter_types at lines 169-174). No corresponding constants exist in constants.py for these three node types. Per the project's "No Hardcoded Strings" rule, they should each be added as constants (e.g. TS_REQUIRED_PARAMETER, TS_OPTIONAL_PARAMETER, TS_ACCESSIBILITY_MODIFIER) and referenced via cs..
The same applies to "type_annotation" used in _extract_type_annotation_name (line 195).
All four new literals should be centralised in constants.py.
Rule Used: ## Technical Requirements
Agentic Framework
-... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/parsers/js_ts/type_inference.py
Line: 134-145
Comment:
**Hardcoded tree-sitter node-type string literals**
`"required_parameter"`, `"optional_parameter"`, and `"accessibility_modifier"` appear as raw string literals in `_collect_constructor_parameter_type` (and also in `_collect_parameter_types` at lines 169-174). No corresponding constants exist in `constants.py` for these three node types. Per the project's "No Hardcoded Strings" rule, they should each be added as constants (e.g. `TS_REQUIRED_PARAMETER`, `TS_OPTIONAL_PARAMETER`, `TS_ACCESSIBILITY_MODIFIER`) and referenced via `cs.`.
The same applies to `"type_annotation"` used in `_extract_type_annotation_name` (line 195).
All four new literals should be centralised in `constants.py`.
**Rule Used:** ## Technical Requirements
### Agentic Framework
-... ([source](https://app.greptile.com/review/custom-context?memory=d4240b05-b763-467a-a6bf-94f73e8b6859))
How can I resolve this? If you propose a fix, please make it concise.| except Exception as e: | ||
| logger.debug( | ||
| "Incremental reindex graph-state probe failed for {name}: {error}", | ||
| name=self.project_name, | ||
| error=e, | ||
| ) | ||
| return False | ||
|
|
||
| if not results: | ||
| logger.info( | ||
| "No graph-state probe results for {name}; forcing full reindex", | ||
| name=self.project_name, | ||
| ) | ||
| return True | ||
|
|
||
| symbol_count = results[0].get("c", 0) | ||
| if not isinstance(symbol_count, int): | ||
| return False | ||
|
|
||
| if symbol_count == 0: | ||
| logger.info( | ||
| "No existing graph symbols found for {name}; ignoring hash cache and forcing full reindex", | ||
| name=self.project_name, | ||
| ) | ||
| return True |
There was a problem hiding this comment.
logs.py
Three new log-message strings are hardcoded directly inside _should_force_full_reindex rather than defined in logs.py:
"Incremental reindex graph-state probe failed for {name}: {error}""No graph-state probe results for {name}; forcing full reindex""No existing graph symbols found for {name}; ignoring hash cache and forcing full reindex"
The project's coding standard says files that are not logs.py should have almost no string literals, and all log messages should live in logs.py. These three should be moved there and referenced via ls..
Rule Used: ## Technical Requirements
Agentic Framework
-... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/graph_updater.py
Line: 409-433
Comment:
**Inline log message strings not in `logs.py`**
Three new log-message strings are hardcoded directly inside `_should_force_full_reindex` rather than defined in `logs.py`:
- `"Incremental reindex graph-state probe failed for {name}: {error}"`
- `"No graph-state probe results for {name}; forcing full reindex"`
- `"No existing graph symbols found for {name}; ignoring hash cache and forcing full reindex"`
The project's coding standard says files that are not `logs.py` should have almost no string literals, and all log messages should live in `logs.py`. These three should be moved there and referenced via `ls.`.
**Rule Used:** ## Technical Requirements
### Agentic Framework
-... ([source](https://app.greptile.com/review/custom-context?memory=d4240b05-b763-467a-a6bf-94f73e8b6859))
How can I resolve this? If you propose a fix, please make it concise.| class_name = class_qn.split(cs.SEPARATOR_DOT)[-1] | ||
| suffix_matches = self.function_registry.find_ending_with( | ||
| f"{class_name}{separator}{method_name}" | ||
| ) | ||
| if len(suffix_matches) == 1: | ||
| matched_qn = suffix_matches[0] | ||
| return self.function_registry[matched_qn], matched_qn |
There was a problem hiding this comment.
The single-suffix-match fallback searches the entire function registry for any entry ending with ClassName.method_name. If there is exactly one such entry globally, it is returned — even if it belongs to a completely unrelated namespace.
Example: class_qn = "proj.a.MyClass" with method "doWork". If "proj.b.MyClass.doWork" exists (same short class name, different package) and is the only match, this fallback silently returns it instead of falling through to _resolve_inherited_method. This can produce the very kind of incorrect cross-module edge the PR aims to eliminate.
The safeguard should at minimum verify that matched_qn shares the same module prefix as class_qn before trusting the single match:
if len(suffix_matches) == 1:
matched_qn = suffix_matches[0]
expected_prefix = cs.SEPARATOR_DOT.join(class_qn.split(cs.SEPARATOR_DOT)[:-1])
if matched_qn.startswith(expected_prefix):
return self.function_registry[matched_qn], matched_qnPrompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/parsers/call_resolver.py
Line: 57-63
Comment:
**Suffix-match fallback can resolve to wrong namespace**
The single-suffix-match fallback searches the entire function registry for any entry ending with `ClassName.method_name`. If there is exactly one such entry globally, it is returned — even if it belongs to a completely unrelated namespace.
Example: `class_qn = "proj.a.MyClass"` with method `"doWork"`. If `"proj.b.MyClass.doWork"` exists (same short class name, different package) and is the only match, this fallback silently returns it instead of falling through to `_resolve_inherited_method`. This can produce the very kind of incorrect cross-module edge the PR aims to eliminate.
The safeguard should at minimum verify that `matched_qn` shares the same module prefix as `class_qn` before trusting the single match:
```python
if len(suffix_matches) == 1:
matched_qn = suffix_matches[0]
expected_prefix = cs.SEPARATOR_DOT.join(class_qn.split(cs.SEPARATOR_DOT)[:-1])
if matched_qn.startswith(expected_prefix):
return self.function_registry[matched_qn], matched_qn
```
How can I resolve this? If you propose a fix, please make it concise.![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses two key issues: ensuring a full reindex when the graph is empty despite an existing hash cache, and significantly improving JS/TS member call resolution for constructor-injected services and this.method() calls. The changes introduce robust logic for graph state probing and refined type inference mechanisms. The addition of comprehensive regression tests for both behaviors is highly commendable, ensuring the stability and correctness of these improvements. Overall, the changes enhance the reliability and accuracy of the codebase's graph generation and analysis capabilities.
| ), | ||
| {"prefix": f"{self.project_name}."}, | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
Catching a broad Exception can mask underlying issues and make debugging harder. While fetch_all might be an external call, it's best practice to either catch more specific exceptions that are expected from the IngestorProtocol or add a comment explaining why a broad Exception is necessary here due to an unpredictable variety of errors.
Consider replacing Exception with a more specific exception type or a tuple of expected exceptions if possible. If not, please add a comment explaining the rationale for catching a generic Exception.
References
- When handling errors from external libraries that may raise multiple types of exceptions for configuration issues (e.g.,
ValueErrorfor invalid formats andAssertionErrorfor missing keys), catch a tuple of these specific exceptions, such as(ValueError, AssertionError), instead of a genericException. - If catching a broad
Exceptionis a pragmatic choice due to a wide and unpredictable variety of exceptions from external libraries, add a comment explaining the rationale to improve code maintainability.
revert: undo PR #455 due to 30 test regressions across multiple languages
2 participants
Summary
this.method()callsProblem
Two issues showed up in practice:
--update-graphcould leave Memgraph nearly empty if the database was cleaned but.cgr-hash-cache.jsonstill existed, because unchanged files were skipped.this.routeHistoryService.saveRoute()to a same-module method such asRoutesController.saveRoute().Changes
Incremental indexing safeguard
GraphUpdaterJS/TS call resolution
thislike an instance prefix alongsideselfthis.method()against the current class context firstthis.service.method()/ typed local service vars before any weak same-module fallback_try_resolve_method(...)to handle minor qualified-name namespace mismatches when the target is otherwise unambiguousValidation
Ran:
Result:
4 passedNotes
This prefers missing edges over incorrect edges for unresolved qualified member calls, which should improve call-graph correctness for NestJS-style DI code.