fix(search): serve search.rooms from the ES spotlight index + correct frontend search contract#201
Merged
Merged
Conversation
search.rooms previously matched in ES then re-hydrated each room from
the Mongo `subscriptions` collection. The spotlight index already
carries roomId/roomName/roomType/siteId per (account, room), so the
Mongo hop was redundant — it added a cross-store dependency, a round
trip, and a consistency window for fields ES already has. Serve the
response directly from the spotlight hit; drop HydrateRooms and the
subscriptions collection (added solely for this — apps does its own
$lookup). Add SiteID to model.SearchRoom.
Frontend: correct the search.rooms / search.messages wire contract.
The payload field is `query` (was incorrectly sending `searchText`),
the rooms filter is `roomType` (was `scope`, and dropped the
server-rejected `app` value), responses are `{messages,total}` /
`{rooms}` (were `results`), and the room hit field is `name` (was
`roomName`). Drop phantom fields; keep siteId now that the backend
returns it. Update consumers, tests, and docs/client-api.md.
The `// #nosec G402 -- ...` line directly above already states the justification; the trailing `//nolint:gosec // ...` restated it in slightly different words. Both directives remain (standalone gosec and golangci-lint are independent mechanisms) — only the duplicated prose is removed. Addresses the pending simplify nit from #197.
simplify review: parseRooms always returns a non-nil slice (nil only with a non-nil error, handled above), so the `if rooms == nil` guard in searchRooms was dead defensive code — removed. Renamed TestSearchRoomsResponseJSON_EmptySubscriptions → _EmptyRooms now that the path no longer touches subscriptions.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR migrates room search from MongoDB subscription hydration to direct Elasticsearch spotlight index projection, with corresponding updates to frontend search API contracts and consumers. Frontend search APIs now use explicit request payloads and renamed response fields ( ChangesSearch API refactor and spotlight ES integration
Sequence DiagramsequenceDiagram
participant Client
participant Frontend
participant SearchService
participant Elasticsearch
Client->>Frontend: user search (query, roomType?)
Frontend->>SearchService: `.search.rooms` / `.search.messages` RPC with {query, roomType, size}
SearchService->>Elasticsearch: spotlight index search request
Elasticsearch-->>SearchService: ES hits (ordered)
SearchService-->>Frontend: SearchRoomsResponse { rooms: [...] } or SearchMessagesResponse { messages: [...] }
Frontend-->>Client: render results (hit.name / message hits)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 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)
docs/client-api.md (1)
1578-1633:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSearch Rooms error docs still contain stale references.
In this updated Search Rooms section, the error anchor and backend wording are inconsistent with the current contract:
#5-error-envelope-referenceshould point to section 6.internalreason still saysES or MongoDB backend failure, but this endpoint is now ES-only.As per coding guidelines: "`docs/client-api.md`: If changes touch a client-facing handler ... update `docs/client-api.md` in the same PR to reflect the new request/response schema, error cases, and triggered events."📄 Suggested doc patch
-See [Error envelope](`#5-error-envelope-reference`). +See [Error envelope](`#6-error-envelope-reference`). -| `internal` | ES or MongoDB backend failure (transient or permanent). The raw error is never leaked to the client. | +| `internal` | ES backend failure (transient or permanent). The raw error is never leaked to the client. |🤖 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 `@docs/client-api.md` around lines 1578 - 1633, Update the Search Rooms docs to fix stale references and backend wording: replace the `#5-error-envelope-reference` anchor with the correct `#6-error-envelope-reference` and change the `internal` error reason text from "ES or MongoDB backend failure (transient or permanent)." to reflect ES-only (e.g., "Elasticsearch backend failure (transient or permanent). The raw error is never leaked to the client."). Ensure the error table entries for `bad_request` and `internal` remain accurate (keep mention that raw errors are not leaked) and that `roomType` validation notes still reject `"app"` and unrecognized values.
🧹 Nitpick comments (2)
chat-frontend/src/components/MainApp/SearchResultsPane/SearchResultsPane.test.jsx (1)
40-43: ⚡ Quick winStrengthen request contract assertion for room search.
Line 42 only checks
query; it won’t catch regressions inroomType/sizefor this path.Proposed test update
expect(request).toHaveBeenCalledWith( 'chat.user.alice.request.search.rooms', - expect.objectContaining({ query: 'gen' }) + { query: 'gen', roomType: 'all', size: 50 } )🤖 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 `@chat-frontend/src/components/MainApp/SearchResultsPane/SearchResultsPane.test.jsx` around lines 40 - 43, The test currently asserts that the mocked request was called with 'chat.user.alice.request.search.rooms' and only checks { query: 'gen' }, which misses regressions in roomType/size; update the assertion in SearchResultsPane.test.jsx to expect the request payload to include the full contract (e.g., include roomType and size alongside query) by replacing expect.objectContaining({ query: 'gen' }) with an object matcher that asserts { query: 'gen', roomType: <expectedValue>, size: <expectedValue> } (or use expect.objectContaining with all three keys) for the 'chat.user.alice.request.search.rooms' call to ensure room search parameters are validated.search-service/integration_test.go (1)
1051-1057: ⚡ Quick winAdd a cross-account leakage assertion to this ES-only path.
Now that
search.roomsno longer re-hydrates through Mongo subscriptions, the auth boundary lives entirely in the spotlight query. This test only seeds Alice's docs, so it won't catch a missinguserAccountfilter.🧪 Suggested test hardening
func TestIntegration_SearchRooms_HappyPath(t *testing.T) { f := setupRoomsFixture(t) const account = "alice" now := time.Now().UTC() // Seed spotlight docs for two rooms alice is in. seedDoc(t, f.esURL, "spotlight-subs-test", "spot-r1", map[string]any{ "roomId": "r1", "roomName": "engineering-announcements", "roomType": "channel", "userAccount": account, "siteId": "site-local", "joinedAt": now.Add(-48 * time.Hour).Format(time.RFC3339), }) seedDoc(t, f.esURL, "spotlight-subs-test", "spot-r2", map[string]any{ "roomId": "r2", "roomName": "engineering-random", "roomType": "channel", "userAccount": account, "siteId": "site-local", "joinedAt": now.Add(-24 * time.Hour).Format(time.RFC3339), }) + seedDoc(t, f.esURL, "spotlight-subs-test", "spot-r3", map[string]any{ + "roomId": "r3", + "roomName": "engineering-secret", + "roomType": "channel", + "userAccount": "mallory", + "siteId": "site-local", + "joinedAt": now.Add(-12 * time.Hour).Format(time.RFC3339), + }) reqBytes, err := json.Marshal(model.SearchRoomsRequest{Query: "engineering"}) require.NoError(t, err) msg, err := f.clientNATS.Request(subject.SearchRooms(account), reqBytes, 10*time.Second) require.NoError(t, err) var resp model.SearchRoomsResponse require.NoError(t, json.Unmarshal(msg.Data, &resp)) require.Len(t, resp.Rooms, 2, "both rooms matching 'engineering' must be returned") byID := map[string]model.SearchRoom{} for _, r := range resp.Rooms { byID[r.RoomID] = r } assert.Equal(t, model.SearchRoom{RoomID: "r1", Name: "engineering-announcements", RoomType: "channel", SiteID: "site-local"}, byID["r1"]) assert.Equal(t, model.SearchRoom{RoomID: "r2", Name: "engineering-random", RoomType: "channel", SiteID: "site-local"}, byID["r2"]) + _, leaked := byID["r3"] + assert.False(t, leaked, "rooms from other accounts must not leak") }As per coding guidelines, "Tests must cover happy path, error paths, edge cases (empty collections, boundary conditions), and invalid input."
🤖 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 `@search-service/integration_test.go` around lines 1051 - 1057, The ES-only test currently seeds only Alice's docs and asserts two rooms are returned, but doesn't guard against cross-account leakage; update the test that inspects resp.Rooms (model.SearchRoom) to also assert that every returned room is owned by Alice's account (e.g., check a userAccount/account field on each resp.Rooms entry or ensure no room with another account ID appears), or alternatively seed a room belonging to a different account and assert it is NOT returned; specifically modify the assertions around resp.Rooms/byID (and the spotlight query setup) so the test fails if results lack the required userAccount filter.
🤖 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 `@docs/client-api.md`:
- Around line 1578-1633: Update the Search Rooms docs to fix stale references
and backend wording: replace the `#5-error-envelope-reference` anchor with the
correct `#6-error-envelope-reference` and change the `internal` error reason
text from "ES or MongoDB backend failure (transient or permanent)." to reflect
ES-only (e.g., "Elasticsearch backend failure (transient or permanent). The raw
error is never leaked to the client."). Ensure the error table entries for
`bad_request` and `internal` remain accurate (keep mention that raw errors are
not leaked) and that `roomType` validation notes still reject `"app"` and
unrecognized values.
---
Nitpick comments:
In
`@chat-frontend/src/components/MainApp/SearchResultsPane/SearchResultsPane.test.jsx`:
- Around line 40-43: The test currently asserts that the mocked request was
called with 'chat.user.alice.request.search.rooms' and only checks { query:
'gen' }, which misses regressions in roomType/size; update the assertion in
SearchResultsPane.test.jsx to expect the request payload to include the full
contract (e.g., include roomType and size alongside query) by replacing
expect.objectContaining({ query: 'gen' }) with an object matcher that asserts {
query: 'gen', roomType: <expectedValue>, size: <expectedValue> } (or use
expect.objectContaining with all three keys) for the
'chat.user.alice.request.search.rooms' call to ensure room search parameters are
validated.
In `@search-service/integration_test.go`:
- Around line 1051-1057: The ES-only test currently seeds only Alice's docs and
asserts two rooms are returned, but doesn't guard against cross-account leakage;
update the test that inspects resp.Rooms (model.SearchRoom) to also assert that
every returned room is owned by Alice's account (e.g., check a
userAccount/account field on each resp.Rooms entry or ensure no room with
another account ID appears), or alternatively seed a room belonging to a
different account and assert it is NOT returned; specifically modify the
assertions around resp.Rooms/byID (and the spotlight query setup) so the test
fails if results lack the required userAccount filter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fca3973-1374-41da-8764-e974c6e9492c
📒 Files selected for processing (25)
chat-frontend/src/api/searchMessages/index.tschat-frontend/src/api/searchRooms/index.tschat-frontend/src/components/MainApp/AppHeader/SearchBar/SearchBar.jsxchat-frontend/src/components/MainApp/AppHeader/SearchBar/SearchBar.test.jsxchat-frontend/src/components/MainApp/ChatPage/InRoomSearch/InRoomSearch.jsxchat-frontend/src/components/MainApp/ChatPage/InRoomSearch/InRoomSearch.test.jsxchat-frontend/src/components/MainApp/ChatPage/ManageMembersDialog/MemberPicker/MemberPicker.jsxchat-frontend/src/components/MainApp/ChatPage/ManageMembersDialog/MemberPicker/MemberPicker.test.jsxchat-frontend/src/components/MainApp/SearchResultsPane/SearchResultsPane.jsxchat-frontend/src/components/MainApp/SearchResultsPane/SearchResultsPane.test.jsxchat-frontend/src/lib/roomFormat.jschat-frontend/src/lib/roomFormat.test.jsdocs/client-api.mdpkg/model/model_test.gopkg/model/search.gopkg/oidc/oidc.gopkg/searchengine/factory.gosearch-service/handler.gosearch-service/handler_test.gosearch-service/integration_test.gosearch-service/mock_store_test.gosearch-service/response.gosearch-service/response_test.gosearch-service/store.gosearch-service/store_mongo.go
💤 Files with no reviewable changes (2)
- search-service/store.go
- search-service/mock_store_test.go
- docs/client-api.md (search.rooms): fix stale #5 → #6 error-envelope anchor (every other ref uses #6); the `internal` reason no longer says "ES or MongoDB" — this endpoint is ES-only now. - SearchResultsPane.test.jsx: assert the full search.rooms wire payload {query,roomType,size}, not just query. - integration_test.go: seed a room owned by another account and assert it does not leak. With Mongo hydration removed, the spotlight userAccount term filter is the sole access boundary — this guards that regression directly.
The "Detect affected integration targets" step did `git fetch --depth=1 origin <base>` then a three-dot `origin/<base>...HEAD` diff. Three-dot needs a merge base, but a depth-1 base fetch has no history, so the step intermittently failed with "no merge base" (exit 128), blocking every downstream job. Drop --depth=1; checkout already uses fetch-depth: 0, so a full base fetch resolves the merge base reliably.
GITMateuszCharczuk
approved these changes
May 19, 2026
Collaborator
GITMateuszCharczuk
left a comment
GITMateuszCharczuk
left a comment
There was a problem hiding this comment.
LGTM, confirmed its running just fine on codespaces
Service containers run as non-root (uid 10001, since the runtime hardening in #197) and bind-mount docker-local/backend.creds read-only at /etc/nats/backend.creds. setup.sh wrote it 0600, so the in-container user got "permission denied" on `make up`. Generate it 0644 instead. Safe only because this is a throwaway local-dev credential created by this script; the .env file stays 0600 (read by the compose CLI on the host, never mounted).
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
3 participants
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
Two related fixes to room/message search:
Backend —
search.roomsserved directly from ES (no Mongo hop)search.roomspreviously matched in the ES spotlight index, then re-hydrated every room from the Mongosubscriptionscollection. The spotlight index is per-(account, room)and already carriesroomId/roomName/roomType/siteId(search-sync-worker/spotlight.go), so the Mongo hop was redundant — a cross-store dependency, an extra round-trip, and a consistency window for data ES already had. Now the response is built directly from the spotlight hit.pkg/model/search.go— addSiteIDtoSearchRoom(now an ES/wire projection, not a Mongo doc)search-service/response.go—parseRoomIDs→parseRooms+toSearchRoom(reuses the existing genericrawResponse[T]envelope, mirroring theparseMessagesResponse/toSearchMessagepair)search-service/handler.go—searchRoomsreturns ES-parsed rooms; Mongo path removedsearch-service/store.go/store_mongo.go—HydrateRoomsand thesubscriptionscollection removed (added solely for this;searchAppsdoes its own$lookup)docs/client-api.md— updated (required: client-facing handler) — addssiteId, drops the "hydrated from MongoDB" wordingFrontend — corrected search wire contract
The chat-frontend was sending/parsing the wrong shapes against the backend:
query— the client was sendingsearchText, so every search submitted an empty queryroomType(wasscope; dropped the server-rejectedappvalue){messages,total}/{rooms}(wereresults)name(wasroomName)total/userId; addededitedAt?/updatedAt?; keptsiteId(now legitimately returned)roomFromSearchHit, the 4 consumer components, and all affected testsAlso
//nolint:gosecprose inpkg/oidc/oidc.go/pkg/searchengine/factory.go(pending simplify nit from ci: add blocking SAST gate (gosec, govulncheck, semgrep) #197)Validation
make lint0 issues;make test(all services, race) green;gosecPASSgo vet -tags=integration ./search-service/clean (rooms integration tests converted to seed only the spotlight index — Mongo container removed from that fixture)npm run typecheckclean;npm test540/540 pass/simplifyrun on the branch: reuse clean, efficiency a net win (removes a per-request cross-store hop); the one must-fix (an unreachable nil guard) and a stale test name were addressedvuln.go.devaccess; broken Python) — CI validates theseTest plan
sastjob green (gosec + govulncheck + semgrep)make test-integration SERVICE=search-servicegreen (validates the ES-onlysearch.roomspath end-to-end)Generated by Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation