fix: correct file count display for Grep and Glob tool results#562
Open
zuikusanxun wants to merge 4 commits intositeboon:mainfrom
Open
fix: correct file count display for Grep and Glob tool results#562zuikusanxun wants to merge 4 commits intositeboon:mainfrom
zuikusanxun wants to merge 4 commits intositeboon:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded shared file-output helpers ( Changes
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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/chat/tools/configs/toolConfigs.ts`:
- Around line 188-191: The code uses "fromStructured as number" which only
affects TypeScript types and doesn't coerce runtime values (so strings like "1"
will break pluralization); update the logic in toolConfigs.ts (look for the
fromStructured variable set from toolData.numFiles and
toolData.filenames?.length, and the later similar block around lines 228-231) to
coerce and validate the numeric value (e.g., use Number(...) or parseInt and
check Number.isFinite or isNaN) before assigning to count, defaulting to 0 or
length when invalid, and then use that numeric count for the `Found ${count}
${count === 1 ? 'file' : 'files'}` string so pluralization is correct at
runtime.
- Around line 193-194: Guard against non-string result?.content before
splitting: derive a normalized string (if Array.isArray(result?.content) join
with '\n', else use String(result?.content ?? '')). Then split on '\n', trim
each line before checks, and apply the existing filters against the trimmedLine
(e.g. check startsWith('Found '), equal 'No files found' / 'No matches found').
Update the occurrences where lines are computed (the lines variable and
subsequent count) so they use this normalized-and-trimmed pipeline to avoid
exceptions when content is an object/array and to correctly ignore status lines
with surrounding whitespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 53f00a4d-2709-4950-bd92-d124ba68b704
📒 Files selected for processing (1)
src/components/chat/tools/configs/toolConfigs.ts
… guard - Replace `as number` with `Number()` + `Number.isFinite()` to coerce and validate structured numFiles at runtime - Extract `parseToolFileLines()` helper to safely handle non-string content (array/object) and trim lines before filtering status lines - Deduplicate parsing logic across Grep and Glob tool configs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/chat/tools/configs/toolConfigs.ts (1)
220-220:modeis currently dead data in file-list content props.
mode: 'grep' | 'glob'is returned but not consumed by the file-list rendering path, so it adds surface area without effect. Consider removing it, or wiring it intoFileListContentif intended.Also applies to: 261-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/tools/configs/toolConfigs.ts` at line 220, The returned object from the file-list config includes a dead "mode" property ('grep' | 'glob') that is not used by the FileListContent rendering path; either remove the mode property from the return values in the functions in toolConfigs.ts that produce { files, mode: ... } (refer to the return sites that currently emit mode) or thread the mode into the FileListContent props and its consumer components (update FileListContent's prop type and its render/handler logic to respect mode), and remove the duplicate unused occurrences (also adjust the second occurrence analogous to the first).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/chat/tools/configs/toolConfigs.ts`:
- Around line 217-220: The code assigns files using toolData.filenames?.length
which can be truthy for non-array values and later breaks consumers expecting an
array; update the selection logic around the files variable to ensure
toolData.filenames is an actual array (use Array.isArray(toolData.filenames))
before using it, otherwise fall back to parseToolFileLines(result?.content);
apply the same guard where files is set again (the other occurrence referencing
toolData.filenames and parseToolFileLines) so downstream code that calls
files.map always receives a true array.
- Around line 205-212: The current branch that handles fromStructured (the
variable fromStructured) returns "Found 0 files" when Number(fromStructured) is
not finite, which prevents falling back to parsing actual file lines; change the
logic in the block that uses fromStructured so that if Number(fromStructured) is
not a finite number you do not return early but instead fall through to use
parseToolFileLines(result?.content) and compute the count from parsed lines
(same fix needed in the other identical block around the parseToolFileLines
usage); update the branches referencing fromStructured and parseToolFileLines to
prefer the numeric structured count only when Number.isFinite(count) is true,
otherwise compute count via parseToolFileLines(result?.content) and return the
correct "Found X file(s)" string.
---
Nitpick comments:
In `@src/components/chat/tools/configs/toolConfigs.ts`:
- Line 220: The returned object from the file-list config includes a dead "mode"
property ('grep' | 'glob') that is not used by the FileListContent rendering
path; either remove the mode property from the return values in the functions in
toolConfigs.ts that produce { files, mode: ... } (refer to the return sites that
currently emit mode) or thread the mode into the FileListContent props and its
consumer components (update FileListContent's prop type and its render/handler
logic to respect mode), and remove the duplicate unused occurrences (also adjust
the second occurrence analogous to the first).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a5a1adab-e263-4d95-970d-f75712a81009
📒 Files selected for processing (1)
src/components/chat/tools/configs/toolConfigs.ts
- Extract shared `getFileCountTitle()` and `getFileList()` helpers to eliminate duplicated logic across Grep and Glob tool configs - Use `Array.isArray()` guard on `toolData.filenames` before accessing `.length`, preventing strings/objects from being treated as arrays - Replace `as number` with `Number()` + `Number.isFinite()` for runtime type coercion; fall through to content parsing when value is invalid instead of returning a hardcoded "Found 0 files" - Remove unused `mode` property from `getContentProps` return value as it is not consumed by `FileListContent`
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
toolUseResultstructured data (numFiles/filenames) is unavailable, fall back to parsing the content string to count actual file linesDetails
The Grep and Glob tool configs in
toolConfigs.tspreviously relied solely ontoolUseResult.numFilesortoolUseResult.filenames.lengthfor the file count. When these structured fields were missing or empty (which happens in certain response formats), the count would incorrectly show 0.This fix:
numFiles/filenames) when available (using??instead of||)result.contentline-by-line, filtering out summary/status linestitle()andgetContentProps()to keep count and file list consistentTest
npm run buildpassesSummary by CodeRabbit
Bug Fixes
Refactor