search-service cleanup: drop cache metrics, rename RoomIDs, dead const#133
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR standardizes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
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)
pkg/model/model_test.go (1)
1297-1319:⚠️ Potential issue | 🟡 MinorKeep a wire-key assertion for
SearchMessagesRequest.This round-trip still passes if the JSON tag regresses, because it never inspects the marshaled key. Add an assertion on the populated payload so this rename stays wire-compatible.
🧪 Suggested assertion
data, err := json.Marshal(&req) require.NoError(t, err) + var raw map[string]any + require.NoError(t, json.Unmarshal(data, &raw)) + _, hasRoomIDs := raw["roomIds"] + assert.True(t, hasRoomIDs, "roomIds must remain the wire key when populated") var dst model.SearchMessagesRequest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/model/model_test.go` around lines 1297 - 1319, The test TestSearchMessagesRequestJSON currently only round-trips the struct and checks omission of roomIds but doesn't assert the actual JSON wire keys; add an assertion that verifies the marshaled JSON uses the expected field names (e.g., "searchText" and "roomIds") so future tag renames will fail the test. Modify the subtests in TestSearchMessagesRequestJSON to unmarshal the marshaled bytes into a map[string]any (as already done in the "global" case) and add explicit assertions that raw contains "searchText" with the expected value in the "full" case and that "roomIds" is present with the expected slice, and keep the "global" case assertion that "roomIds" is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/model/model_test.go`:
- Around line 1297-1319: The test TestSearchMessagesRequestJSON currently only
round-trips the struct and checks omission of roomIds but doesn't assert the
actual JSON wire keys; add an assertion that verifies the marshaled JSON uses
the expected field names (e.g., "searchText" and "roomIds") so future tag
renames will fail the test. Modify the subtests in TestSearchMessagesRequestJSON
to unmarshal the marshaled bytes into a map[string]any (as already done in the
"global" case) and add explicit assertions that raw contains "searchText" with
the expected value in the "full" case and that "roomIds" is present with the
expected slice, and keep the "global" case assertion that "roomIds" is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edb35731-ad37-438a-b73c-73c19729b64e
📒 Files selected for processing (8)
pkg/model/model_test.gopkg/model/search.gosearch-service/handler.gosearch-service/handler_test.gosearch-service/metrics.gosearch-service/query_messages.gosearch-service/query_messages_test.gosearch-sync-worker/handler.go
…const)
- search-service/metrics.go + handler.go: remove restricted-rooms Valkey
cache hit/miss counters (search_service_cache_hits_total,
search_service_cache_misses_total). They tracked cache events for the
per-user restricted-rooms blob lookup and weren't actionable signals;
the operationally useful tail-latency view stays via
search_service_es_duration_seconds{op="user_room_get"}. Drop the now-
unused `kind` parameter from loadRestricted; metricKind* constants
stay (still used by request-path metrics).
- pkg/model/search.go: rename SearchMessagesRequest.RoomIds → RoomIDs to
follow Go's initialism convention. JSON tag stays `roomIds` so the
wire format is unchanged. Update all callers + tests.
- search-service/query_messages.go: remove the sort.Strings calls on the
user-provided unrestricted/restrictedSubset slices in
scopedAccessClauses. Tests use ElementsMatch; emission order matches
the input order. The sortedRIDs helper stays (operates on map keys
for global search and is documented as load-bearing for ES query-plan
cacheability).
- search-sync-worker/handler.go: drop the unused esErrIndexNotFound
constant. Only its sibling esErrDocumentMissing is referenced;
collapse the const block to a single line and update the doc comment
to call out that other 404 error types are still treated as failures.
https://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
Joey0538
force-pushed
the
claude/search-service-cleanup
branch
from
ddead4c to
9f9a6f4
Compare
|
Extended the cleanup to also drop Why: the Final metric inventory (all 4-5/5 importance):
Tests + lint green on Generated by Claude Code |
3 participants
Summary
Four small unrelated cleanups in search-service and one stray dead constant in search-sync-worker:
Remove restricted-rooms cache hit/miss Prometheus counters.
search_service_cache_hits_total{kind}andsearch_service_cache_misses_total{kind}tracked Valkey lookups for the per-user restricted-rooms blob. The hit/miss view wasn't actionable in practice and the operationally useful tail-latency view stays viasearch_service_es_duration_seconds{op="user_room_get"}. Thekindparameter onloadRestricted(only plumbed to those counters) is dropped. ThemetricKindMessages/metricKindRoomsconstants stay — still used by request-path metrics.Rename
SearchMessagesRequest.RoomIds→RoomIDs. Go-style initialism. JSON tag staysroomIdsso the wire format is unchanged.Drop the
sort.Stringson user-input slices inscopedAccessClauses. The sort was only there for deterministic test output; tests useassert.ElementsMatchso ordering didn't matter. Slices come from caller-suppliedRoomIDs(already deterministic). ThesortedRIDshelper stays — it sorts map iteration inglobalAccessClauses, which is a different concern (and load-bearing per its existing comment).Delete the unused
esErrIndexNotFoundconstant insearch-sync-worker/handler.go. OnlyesErrDocumentMissingis referenced; collapse the const block and tighten the doc comment to call out that other 404 error types stay treated as failures.Test plan
make test— green across the monorepomake lint— 0 issuesgrep RoomIdsrepo-wide returns no Go field hits (only the JSON tag string"roomIds"and lowercaseroomIdsin prose comments referring to the wire-format key)grep esErrIndexNotFoundrepo-wide returns 0 matchesmodel_test.goround-trip assertsroomIdsis the marshalled key/metricsoutput no longer exposessearch_service_cache_*after deployhttps://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
Generated by Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor