Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
c67e833 to
8b0d42b
Compare
f7ef6a8 to
7533bba
Compare
8b0d42b to
e31183b
Compare
7533bba to
359d05b
Compare
45aaa95 to
05e09e2
Compare
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/code/src/main/services/enrichment/service.ts:357-383
`processInBatches` is a generic results-collecting utility, but the callback here always returns `null` and the `R[]` return value is immediately discarded — all real work is done as a side effect on the `files` closure. This makes the signature misleading and violates the "no superfluous parts" rule. A plain batched loop expresses the intent directly.
```suggestion
const files = new Map<string, ParsedRepoEntry>();
for (let i = 0; i < toParse.length; i += SCAN_BATCH_SIZE) {
const batch = toParse.slice(i, i + SCAN_BATCH_SIZE);
await Promise.all(
batch.map(async (candidate) => {
let content: string;
try {
content = await fs.readFile(
path.join(repoPath, candidate.relPath),
"utf-8",
);
} catch {
return;
}
try {
const result = await enricher.parse(content, candidate.langId);
files.set(candidate.relPath, { langId: candidate.langId, result });
} catch (err) {
log.debug("enricher.parse threw during repo scan, skipping file", {
file: candidate.relPath,
error: err instanceof Error ? err.message : String(err),
});
files.set(candidate.relPath, {
langId: candidate.langId,
result: null,
});
}
}),
);
await yieldToEventLoop();
}
```
### Issue 2 of 2
apps/code/src/main/services/enrichment/service.ts:322-326
**`null` scan results are not memoised**
When `runScan` fails (e.g. `git grep` throws because the directory is not a git repo, or the process is unavailable), the `null` result is never written to `repoScanCache`. After the in-flight promise settles, a subsequent call to `scanRepo` for the same path will start a brand-new `runScan` and fail again. For persistent error conditions this means every call pair to `detectPosthogInstallState` / `findStaleFlagSuggestions` retries an already-known-failing git operation. Caching the `null` would be consistent with the "concurrent callers wait on the same promise" contract.
Reviews (1): Last reviewed commit: "feat(code): enricher run" | Re-trigger Greptile |
| const files = new Map<string, ParsedRepoEntry>(); | ||
| await processInBatches(toParse, SCAN_BATCH_SIZE, async (candidate) => { | ||
| let content: string; | ||
| try { | ||
| content = await fs.readFile( | ||
| path.join(repoPath, candidate.relPath), | ||
| "utf-8", | ||
| ); | ||
| } catch { | ||
| return null; | ||
| } | ||
| try { | ||
| const result = await enricher.parse(content, candidate.langId); | ||
| files.set(candidate.relPath, { langId: candidate.langId, result }); | ||
| return null; | ||
| } catch (err) { | ||
| log.debug("enricher.parse threw during repo scan, skipping file", { | ||
| file: candidate.relPath, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| files.set(candidate.relPath, { | ||
| langId: candidate.langId, | ||
| result: null, | ||
| }); | ||
| return null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
processInBatches is a generic results-collecting utility, but the callback here always returns null and the R[] return value is immediately discarded — all real work is done as a side effect on the files closure. This makes the signature misleading and violates the "no superfluous parts" rule. A plain batched loop expresses the intent directly.
| const files = new Map<string, ParsedRepoEntry>(); | |
| await processInBatches(toParse, SCAN_BATCH_SIZE, async (candidate) => { | |
| let content: string; | |
| try { | |
| content = await fs.readFile( | |
| path.join(repoPath, candidate.relPath), | |
| "utf-8", | |
| ); | |
| } catch { | |
| return null; | |
| } | |
| try { | |
| const result = await enricher.parse(content, candidate.langId); | |
| files.set(candidate.relPath, { langId: candidate.langId, result }); | |
| return null; | |
| } catch (err) { | |
| log.debug("enricher.parse threw during repo scan, skipping file", { | |
| file: candidate.relPath, | |
| error: err instanceof Error ? err.message : String(err), | |
| }); | |
| files.set(candidate.relPath, { | |
| langId: candidate.langId, | |
| result: null, | |
| }); | |
| return null; | |
| } | |
| }); | |
| const files = new Map<string, ParsedRepoEntry>(); | |
| for (let i = 0; i < toParse.length; i += SCAN_BATCH_SIZE) { | |
| const batch = toParse.slice(i, i + SCAN_BATCH_SIZE); | |
| await Promise.all( | |
| batch.map(async (candidate) => { | |
| let content: string; | |
| try { | |
| content = await fs.readFile( | |
| path.join(repoPath, candidate.relPath), | |
| "utf-8", | |
| ); | |
| } catch { | |
| return; | |
| } | |
| try { | |
| const result = await enricher.parse(content, candidate.langId); | |
| files.set(candidate.relPath, { langId: candidate.langId, result }); | |
| } catch (err) { | |
| log.debug("enricher.parse threw during repo scan, skipping file", { | |
| file: candidate.relPath, | |
| error: err instanceof Error ? err.message : String(err), | |
| }); | |
| files.set(candidate.relPath, { | |
| langId: candidate.langId, | |
| result: null, | |
| }); | |
| } | |
| }), | |
| ); | |
| await yieldToEventLoop(); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/enrichment/service.ts
Line: 357-383
Comment:
`processInBatches` is a generic results-collecting utility, but the callback here always returns `null` and the `R[]` return value is immediately discarded — all real work is done as a side effect on the `files` closure. This makes the signature misleading and violates the "no superfluous parts" rule. A plain batched loop expresses the intent directly.
```suggestion
const files = new Map<string, ParsedRepoEntry>();
for (let i = 0; i < toParse.length; i += SCAN_BATCH_SIZE) {
const batch = toParse.slice(i, i + SCAN_BATCH_SIZE);
await Promise.all(
batch.map(async (candidate) => {
let content: string;
try {
content = await fs.readFile(
path.join(repoPath, candidate.relPath),
"utf-8",
);
} catch {
return;
}
try {
const result = await enricher.parse(content, candidate.langId);
files.set(candidate.relPath, { langId: candidate.langId, result });
} catch (err) {
log.debug("enricher.parse threw during repo scan, skipping file", {
file: candidate.relPath,
error: err instanceof Error ? err.message : String(err),
});
files.set(candidate.relPath, {
langId: candidate.langId,
result: null,
});
}
}),
);
await yieldToEventLoop();
}
```
How can I resolve this? If you propose a fix, please make it concise.| const promise = this.runScan(repoPath).finally(() => { | ||
| this.repoScanInflight.delete(repoPath); | ||
| }); | ||
| this.repoScanInflight.set(repoPath, promise); | ||
| return promise; |
There was a problem hiding this comment.
null scan results are not memoised
When runScan fails (e.g. git grep throws because the directory is not a git repo, or the process is unavailable), the null result is never written to repoScanCache. After the in-flight promise settles, a subsequent call to scanRepo for the same path will start a brand-new runScan and fail again. For persistent error conditions this means every call pair to detectPosthogInstallState / findStaleFlagSuggestions retries an already-known-failing git operation. Caching the null would be consistent with the "concurrent callers wait on the same promise" contract.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/enrichment/service.ts
Line: 322-326
Comment:
**`null` scan results are not memoised**
When `runScan` fails (e.g. `git grep` throws because the directory is not a git repo, or the process is unavailable), the `null` result is never written to `repoScanCache`. After the in-flight promise settles, a subsequent call to `scanRepo` for the same path will start a brand-new `runScan` and fail again. For persistent error conditions this means every call pair to `detectPosthogInstallState` / `findStaleFlagSuggestions` retries an already-known-failing git operation. Caching the `null` would be consistent with the "concurrent callers wait on the same promise" contract.
How can I resolve this? If you propose a fix, please make it concise.05e09e2 to
5f2249e
Compare

Problem
first task discovery is great, but it takes 3-4 minutes
let's give users something faster
Changes
uses the enricher to scan the user's codebase and suggest "tasks" (bundled skills):
How did you test this?
manually
Publish to changelog?
sure