feat: adjust default config for crawl and set concurrency to 5 - #50
Conversation
Greptile OverviewGreptile SummaryThis PR successfully fixes the critical bug in issue #44 where link discovery was incorrectly limited to the What Changed:
Issues Found: Critical:
Minor:
Note: The PR title mentions "set concurrency to 5" and "adjust default config," but the code changes don't actually modify any default configuration values. The defaults remain in Confidence Score: 3/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant CS as CrawlingService
participant Cache as CacheService
participant Browser as Playwright Browser
participant Queue as Crawl Queue
CLI->>CS: crawlWebsite(request)
alt skipCache is false
CS->>Cache: get(cacheKey)
alt cache hit
Cache-->>CS: cached result
CS-->>CLI: return cached result
else cache miss
Cache-->>CS: null
end
else skipCache is true
Note over CS,Cache: Skip cache lookup
end
CS->>Browser: initialize()
CS->>Queue: queue.push({url, depth: 0})
loop while queue not empty AND results < maxUrls
CS->>Queue: batch = queue.splice(0, concurrency)
par Process batch concurrently
CS->>Browser: crawlPage(item)
Browser->>Browser: goto(url)
Browser->>Browser: extract title, description
CS->>CS: extractLinks($, url, baseUrl, rules)
Note over CS: BUG FIX: Previously limited<br/>nextItems.length < concurrency<br/>Now: unlimited link discovery
loop for each link found
alt not visited
CS->>Queue: nextItems.push({url: link, depth+1})
end
end
Browser-->>CS: return nextItems
end
CS->>Queue: queue.push(...nextItems)
end
alt skipCache implementation issue
Note over CS,Cache: Currently always caches result<br/>even when skipCache=true
CS->>Cache: set(cacheKey, result)
end
CS-->>CLI: return CrawlResponse
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Caknoooo
changed the title
|
@Caknoooo Can you check the greptile comments? |
|
@Caknoooo So what was the problem, the maxUrl? WHy do we need to update the rest? |
After I try error, the problem was not The condition |
Additional Comments (5)
Consider wrapping the cache write operation with the same Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: src/services/crawling.ts
Line: 287:298
Comment:
When `skipCache` is true, the cache retrieval is correctly skipped, but the results are still being cached at the end. This creates an inconsistency: if a user explicitly skips the cache, they likely don't want to write to it either. This could lead to unexpected behavior where a `--skip-cache` request still updates the cache, affecting subsequent cached requests.
Consider wrapping the cache write operation with the same `skipCache` check:
```suggestion
if (!request.skipCache) {
try {
await this.cacheService.set(
{
ruleId: cacheKey,
projectContext: {},
},
result
);
logger.info('Cached crawl result', { url: request.url });
} catch (error) {
logger.warn('Failed to cache crawl result', { error });
}
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
There's no
This is a critical consistency issue mentioned in the previous review thread. Consider either:
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/services/crawling.ts
Line: 69:69
Comment:
The `maxDepth` is hardcoded to `2` when `request.max_depth` is not provided. This creates inconsistency across the codebase:
1. This cache key generation uses hardcoded `2`
2. Line 129 also uses hardcoded `2`
3. The CLI defaults to `2` (src/cli.ts line 63, src/cli/commands/crawl.ts line 19)
4. But the env configuration has `CRAWL_CONCURRENCY` and `CRAWL_MAX_URLS` defaults defined
There's no `CRAWL_MAX_DEPTH` environment variable defined in `src/utils/env.ts`, which means:
- Users cannot configure the default `maxDepth` via environment variables
- The cache key generation and actual crawling logic have the default hardcoded in two places, creating maintenance burden
This is a **critical consistency issue** mentioned in the previous review thread. Consider either:
1. Adding `CRAWL_MAX_DEPTH` to env.ts with a default value and using `env.CRAWL_MAX_DEPTH` here, OR
2. Extracting this default to a constant that's reused in both locations
```suggestion
maxDepth: request.max_depth || parseInt(env.CRAWL_MAX_DEPTH || '2'),
```
How can I resolve this? If you propose a fix, please make it concise.
The proper fix requires adding Prompt To Fix With AIThis is a comment left during a code review.
Path: src/services/crawling.ts
Line: 129:129
Comment:
Same hardcoded `maxDepth` default issue as line 69. This is the second location where `maxDepth` defaults to `2`, creating a maintenance burden and inconsistency with how other configuration values are handled (which use `env.CRAWL_*` variables).
The proper fix requires adding `CRAWL_MAX_DEPTH` to `src/utils/env.ts` and using it here:
```suggestion
const maxDepth = request.max_depth || parseInt(env.CRAWL_MAX_DEPTH || '2');
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/services/crawling.ts
Line: 65:76
Comment:
The `generateCacheKey` method is called unconditionally even when `skipCache` is true. While this doesn't cause errors, it's inefficient to generate a cache key (including MD5 hash computation) when it won't be used. Consider checking `request.skipCache` before calling this method, or passing `skipCache` as a parameter and returning early if it's true.
How can I resolve this? If you propose a fix, please make it concise.
While the outer loop checks if (depth < maxDepth && results.length < maxUrls && queue.length + nextItems.length < maxUrls * 10) {
const links = this.extractLinks($, url, baseUrl, rules);
const remainingSlots = Math.max(0, (maxUrls * 10) - queue.length - nextItems.length);
for (const link of links.slice(0, remainingSlots)) {
if (!visited.has(link)) {
nextItems.push({ url: link, depth: depth + 1 });
}
}
}This isn't critical for typical websites, but could prevent issues with sites that have extensive link networks. Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: src/services/crawling.ts
Line: 211:219
Comment:
The removal of `nextItems.length < concurrency` correctly fixes the bug where link discovery was artificially limited. However, this introduces a potential memory concern: if a page contains thousands of links, all of them will be added to `nextItems` and subsequently to the queue without any limit.
While the outer loop checks `results.length < maxUrls` (line 255), the queue itself can grow very large before those URLs are processed. Consider adding a safeguard to limit the total queue size or the number of links extracted per page. For example:
```typescript
if (depth < maxDepth && results.length < maxUrls && queue.length + nextItems.length < maxUrls * 10) {
const links = this.extractLinks($, url, baseUrl, rules);
const remainingSlots = Math.max(0, (maxUrls * 10) - queue.length - nextItems.length);
for (const link of links.slice(0, remainingSlots)) {
if (!visited.has(link)) {
nextItems.push({ url: link, depth: depth + 1 });
}
}
}
```
This isn't critical for typical websites, but could prevent issues with sites that have extensive link networks.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
2 participants
Bug Fix PR
Bug Summary
Linked #44
Root Cause
Summarize the underlying cause identified.
Fix
Describe the approach taken and alternatives considered.
Repro & Verification Steps
Steps to reproduce the bug and verify the fix.
Risk & Rollback Plan
Potential side effects and the rollback plan.
Checklist