fix: omit zero total from chat search#2035
Conversation
sa: none doc: none cfg: none test: unit test
github-actions
Bot
added
domain/im
📝 WalkthroughWalkthroughThe chat search shortcut conditionally includes the ChangesChat Search Total Handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/im/im_chat_search.go`:
- Around line 110-111: The availability predicate in util.ToFloat64 handling
within shortcuts/im/im_chat_search.go lines 110-111 must use int(total) > 0 so
fractional totals such as 0.5 are unavailable after output truncation. In
shortcuts/im/im_chat_search_total_test.go lines 22-25 and 80-82, add coverage
for total: 0.5 in both JSON and pretty output modes, asserting the resulting
zero output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4b392d9-7365-48b2-8859-df5246012611
📒 Files selected for processing (3)
shortcuts/im/im_chat_search.goshortcuts/im/im_chat_search_total_test.gotests/cli_e2e/im/im_chat_search_dryrun_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/im/im_chat_search.go (1)
203-203: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFall back to the displayed-chat count when the upstream total is unavailable.
Line 203 still prints
int(total)unconditionally, so missing, invalid, zero, or sub-unit totals produce0 chat(s) foundeven whenitemscontains displayed chats. Uselen(items)unlesshasPositiveTotalis true.Proposed fix
- fmt.Fprintf(w, "\n%d chat(s) found%s\n", int(total), moreHint) + displayedTotal := len(items) + if hasPositiveTotal { + displayedTotal = int(total) + } + fmt.Fprintf(w, "\n%d chat(s) found%s\n", displayedTotal, moreHint)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/im/im_chat_search.go` at line 203, Update the result-count output around the Fprintf call to use the upstream total only when hasPositiveTotal is true; otherwise use len(items). Preserve the existing moreHint formatting and pluralized message while ensuring displayed chats are counted when the upstream total is unavailable or invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@shortcuts/im/im_chat_search.go`:
- Line 203: Update the result-count output around the Fprintf call to use the
upstream total only when hasPositiveTotal is true; otherwise use len(items).
Preserve the existing moreHint formatting and pluralized message while ensuring
displayed chats are counted when the upstream total is unavailable or invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec49791e-dcff-493c-8f7d-2af4a79c0424
📒 Files selected for processing (2)
shortcuts/im/im_chat_search.goshortcuts/im/im_chat_search_total_test.go
💤 Files with no reviewable changes (1)
- shortcuts/im/im_chat_search_total_test.go
| "page_token": pageToken, | ||
| } | ||
| if hasPositiveTotal { | ||
| outData["total"] = int(total) |
There was a problem hiding this comment.
[P1] Apply the fallback to pretty output as well. This only omits data.total from structured output. The pretty formatter still converts total unconditionally, so a response containing one chat with a missing or zero total still prints a row followed by 0 chat(s) found. That is the same inconsistency described by this PR. Please use len(items) when hasPositiveTotal is false and restore pretty-output regression coverage for missing and zero totals.
There was a problem hiding this comment.
Using len(items) as a fallback is not appropriate here. It represents only the number of items returned on the current page, not the total number of matching chats, so displaying it as the total would be inaccurate (especially with pagination). This PR is intentionally scoped to omitting the zero-valued total field from structured output; it preserves the existing pretty-output behavior rather than substituting a different count with different semantics.
2 participants
Summary
Fix
im +chat-searchoutput when the upstream API omitstotalor returnszero. The CLI now avoids reporting
total: 0alongside non-empty results andkeeps positive upstream totals unchanged.
Changes
data.totalwhen the upstream value is missing, invalid, or zero.positive upstream total is available.
Test Plan
shortcuts/imcontract tests pass.go vet ./...passes.gofmt -landgit diff --checkpass.make unit-testis fully green. The affectedshortcuts/impackagepasses, but the full suite is blocked by unrelated host-environment failures:
the test Git user config is overridden by the global config, and the system
Git does not support
git init --initial-branch.Related Issues
sa: none
doc: none
cfg: none
test: unit test
Summary by CodeRabbit
Bug Fixes
im +chat-searchhandling of the API-providedtotalwhen it’s missing, invalid, zero, or fractional.totalis now omitted from structured output unless it’s a positive value.total.Tests
totalfield contract acrossjsonandprettymodes.