feat: surface session search hits in browser and preview#75
feat: surface session search hits in browser and preview#75awsl233777 wants to merge 2 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🔇 Additional comments (3)
📝 WalkthroughWalkthroughAdds session search/match support: helpers to normalize match payloads and locate matched messages, decorates computed session list with match metadata, marks and jumps to matched messages when loading session details, and updates UI/templates/styles and tests to surface matches. Changes
Sequence DiagramsequenceDiagram
participant UI as User/UI Layer
participant SB as Session Browser
participant Logic as Session Logic
participant Nav as Timeline Navigation
UI->>SB: select active session
SB->>SB: reset pendingSessionMatchKey
SB->>SB: load session messages helper
SB->>Logic: normalizeSessionMatch(activeSession)
Logic-->>SB: { primarySnippet, count, snippets }
SB->>Logic: findSessionMessageMatchKey(messages, snippets)
Logic-->>SB: nextMatchKey
SB->>SB: pendingSessionMatchKey = nextMatchKey
alt match found
SB->>SB: mark message isSearchMatch / matchSnippet
SB->>SB: $nextTick...
SB->>Nav: jumpToSessionTimelineNode(nextMatchKey)
Nav-->>UI: focus/scroll to record
end
SB-->>UI: return helper result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web-ui/partials/index/panel-sessions.html (1)
349-349: Consider adding match fields to v-memo dependencies.The
v-memoon line 349 tracks[msg.text, msg.timestamp, msg.roleLabel, msg.normalizedRole]but doesn't includemsg.isSearchMatchormsg.matchSnippet. If match status changes independently of these fields, the message won't re-render.In practice this may not be an issue if messages are fully replaced when loading, but adding these fields would be more robust.
💡 Optional: Add match fields to v-memo
-v-memo="[msg.text, msg.timestamp, msg.roleLabel, msg.normalizedRole]" +v-memo="[msg.text, msg.timestamp, msg.roleLabel, msg.normalizedRole, msg.isSearchMatch, msg.matchSnippet]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/partials/index/panel-sessions.html` at line 349, The v-memo dependency list on the message element currently includes [msg.text, msg.timestamp, msg.roleLabel, msg.normalizedRole] but misses search-related fields so a change in match state won't trigger re-render; update the v-memo array used in the message rendering (the v-memo attribute) to also include msg.isSearchMatch and msg.matchSnippet so messages re-evaluate when match status or snippet content changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/session-search-ui.test.mjs`:
- Around line 60-121: The test file is orphaned and will never run and also
misses a required mock helper: when calling createSessionBrowserMethods you must
provide loadActiveSessionDetailHelper (the implementation expected by
loadActiveSessionDetail) in the test's mock options; to fix either (A) register
the test file in the test runner entry so it executes and update the test's mock
creation to pass loadActiveSessionDetailHelper (implement a simple stub matching
the real helper signature) alongside api, or (B) if the test is no longer
desired, remove the orphaned test file; ensure the test that remains calling
methods.loadActiveSessionDetail uses createSessionBrowserMethods with the
loadActiveSessionDetailHelper stub and then verify pendingSessionMatchKey,
activeSessionMessages matchSnippet/isSearchMatch and jumpToSessionTimelineNode
behavior.
In `@web-ui/modules/app.methods.session-browser.mjs`:
- Around line 439-465: Before awaiting loadActiveSessionDetailHelper in
loadActiveSessionDetail, capture the identity/state used to compute the match
(e.g., const currentSession = this.activeSession or const seq =
this.sessionDetailRequestSeq) and after the await, verify that the captured
value still matches (compare currentSession === this.activeSession or seq ===
this.sessionDetailRequestSeq) before computing match and mutating
this.activeSessionMessages/pendingSessionMatchKey/jumpToSessionTimelineNode; if
the check fails, skip applying the match annotations to avoid the race. Ensure
you reference loadActiveSessionDetail, loadActiveSessionDetailHelper,
this.activeSession, and sessionDetailRequestSeq when implementing the guard.
---
Nitpick comments:
In `@web-ui/partials/index/panel-sessions.html`:
- Line 349: The v-memo dependency list on the message element currently includes
[msg.text, msg.timestamp, msg.roleLabel, msg.normalizedRole] but misses
search-related fields so a change in match state won't trigger re-render; update
the v-memo array used in the message rendering (the v-memo attribute) to also
include msg.isSearchMatch and msg.matchSnippet so messages re-evaluate when
match status or snippet content changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de5425d2-e1c4-45f0-ad5f-28481657a4f4
📒 Files selected for processing (7)
tests/unit/session-search-ui.test.mjsweb-ui/logic.sessions.mjsweb-ui/modules/app.computed.session.mjsweb-ui/modules/app.methods.session-browser.mjsweb-ui/partials/index/panel-sessions.htmlweb-ui/styles/sessions-list.cssweb-ui/styles/sessions-preview.css
📜 Review details
🔇 Additional comments (15)
web-ui/logic.sessions.mjs (3)
61-80: LGTM!The
normalizeSessionMatchfunction correctly normalizes session match data with proper defensive handling for missing/invalid inputs. The coercion ofcountto a non-negative integer and the filtering/trimming of snippets are well implemented.
82-105: LGTM!The
buildSessionMessageSearchTextfunction comprehensively extracts searchable text from multiple possible message formats (text,content, andpartsarray with both string and object entries). Good defensive coding with proper null/type checks.
107-130: LGTM!The
findSessionMessageMatchKeyfunction correctly implements case-insensitive snippet matching with proper fallback key generation. The key format matchesgetRecordRenderKeyfrom the relevant code snippet (record-${recordLineIndex}orrecord-fallback-${idx}-${timestamp}), ensuring consistency.web-ui/modules/app.methods.session-browser.mjs (3)
1-8: LGTM!The new imports for
findSessionMessageMatchKeyandnormalizeSessionMatchare correctly added to support the search-hit functionality.
328-338: LGTM!The
normalizeSessionMessagemethod now correctly extracts and normalizesmatchSnippet, derivingisSearchMatchfrom its presence. This aligns with the UI template's conditional rendering of match indicators.
386-386: LGTM!Resetting
pendingSessionMatchKeywhen selecting a new session prevents stale jump callbacks from executing after session changes.web-ui/modules/app.computed.session.mjs (2)
4-6: LGTM!The new imports for
isSessionQueryEnabledandnormalizeSessionMatchare correctly added.
34-54: LGTM!The
sortedSessionsListcomputed property now correctly decorates each session with normalized match data for both pinned and non-pinned paths. ThenormalizeSessionMatchfunction is lightweight (no I/O, simple object transformations), so the per-session overhead is acceptable.web-ui/partials/index/panel-sessions.html (2)
255-259: LGTM!The session list item correctly displays the match count badge and primary snippet when search results are available. The conditional rendering with
v-ifguards and the tooltip via:titleare well implemented.
359-361: LGTM!The message-level match highlighting with the conditional
session-msg-content-matchclass and the "搜索命中" chip are correctly implemented.web-ui/styles/sessions-preview.css (1)
252-268: LGTM!The new CSS classes for match highlighting are well-implemented:
.session-msg-content-matchprovides subtle visual emphasis with a gradient background.session-msg-match-chipfollows the existing pill/badge pattern with consistent brand colorsBoth classes integrate well with the existing design system.
web-ui/styles/sessions-list.css (1)
381-405: LGTM!The new CSS classes for session list match display are well-designed:
.session-item-matchprovides proper snippet styling with 2-line clamping and word-break handling.session-match-countfollows the existing badge pattern with consistent brand colorsBoth classes integrate seamlessly with the existing session list styling.
tests/unit/session-search-ui.test.mjs (3)
1-16: LGTM!The test setup correctly uses dynamic imports with
pathToFileURLto load the modules under test. This ESM-compatible approach works well for Node.js test environments.
18-43: LGTM!Good unit test coverage for the core helper functions:
normalizeSessionMatchtest verifies snippet trimming, empty filtering, and derived fieldsfindSessionMessageMatchKeytest verifies case-insensitive partial matching and correct key generation
45-58: LGTM!The
sortedSessionsListcomputed property test correctly verifies that match data is normalized and included in the output, validating the integration between the computed property andnormalizeSessionMatch.
| test('loadActiveSessionDetail annotates matched message and schedules jump', async () => { | ||
| const methods = createSessionBrowserMethods({ | ||
| api: async () => ({ | ||
| messages: [ | ||
| { recordLineIndex: 1, role: 'user', text: 'hello world' }, | ||
| { recordLineIndex: 2, role: 'assistant', text: 'needle is right here in the answer' } | ||
| ], | ||
| clipped: false, | ||
| messageLimit: 80, | ||
| totalMessages: 2 | ||
| }) | ||
| }); | ||
|
|
||
| const jumps = []; | ||
| const context = { | ||
| activeSession: { | ||
| source: 'codex', | ||
| sessionId: 's1', | ||
| filePath: '/tmp/demo.jsonl', | ||
| match: { | ||
| hit: true, | ||
| count: 1, | ||
| snippets: ['needle is right here'] | ||
| } | ||
| }, | ||
| mainTab: 'sessions', | ||
| sessionPreviewRenderEnabled: true, | ||
| sessionDetailRequestSeq: 0, | ||
| sessionDetailInitialMessageLimit: 80, | ||
| sessionDetailMessageLimit: 80, | ||
| sessionPreviewPendingVisibleCount: 0, | ||
| sessionPreviewVisibleCount: 0, | ||
| sessionMessageRefMap: Object.create(null), | ||
| sessionMessageRefBinderMap: Object.create(null), | ||
| resetSessionDetailPagination() {}, | ||
| resetSessionPreviewMessageRender() {}, | ||
| primeSessionPreviewMessageRender() { | ||
| this.sessionPreviewVisibleCount = 2; | ||
| }, | ||
| cancelSessionTimelineSync() {}, | ||
| clearSessionTimelineRefs() {}, | ||
| syncActiveSessionMessageCount() {}, | ||
| invalidateSessionTimelineMeasurementCache() {}, | ||
| $nextTick(fn) { fn(); }, | ||
| jumpToSessionTimelineNode(key) { jumps.push(key); }, | ||
| getRecordKey(message) { | ||
| return String(message.recordLineIndex); | ||
| }, | ||
| getRecordRenderKey(message, idx) { | ||
| return `record-${message.recordLineIndex || idx}`; | ||
| }, | ||
| normalizeSessionMessage: methods.normalizeSessionMessage, | ||
| pendingSessionMatchKey: '' | ||
| }; | ||
|
|
||
| await methods.loadActiveSessionDetail.call(context); | ||
|
|
||
| assert.strictEqual(context.pendingSessionMatchKey, 'record-2'); | ||
| assert.strictEqual(context.activeSessionMessages[1].isSearchMatch, true); | ||
| assert.strictEqual(context.activeSessionMessages[1].matchSnippet, 'needle is right here'); | ||
| assert.deepStrictEqual(jumps, ['record-2']); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if the test actually runs successfully
cd tests/unit
node --test session-search-ui.test.mjs 2>&1 || echo "Test execution failed"Repository: SakuraByteCore/codexmate
Length of output: 620
🏁 Script executed:
cat -n tests/unit/session-search-ui.test.mjs | head -70Repository: SakuraByteCore/codexmate
Length of output: 3112
🏁 Script executed:
# Find where createSessionBrowserMethods is defined
rg -l "createSessionBrowserMethods" --type js --type mjsRepository: SakuraByteCore/codexmate
Length of output: 95
🏁 Script executed:
# Look for loadActiveSessionDetail in production code
rg -l "loadActiveSessionDetail" --type js --type mjsRepository: SakuraByteCore/codexmate
Length of output: 95
🏁 Script executed:
# Find the actual file without mjs type filter
find . -name "app.methods.session-browser.mjs" -type fRepository: SakuraByteCore/codexmate
Length of output: 116
🏁 Script executed:
# Search for loadActiveSessionDetailHelper without mjs type
rg "loadActiveSessionDetailHelper" -A 5Repository: SakuraByteCore/codexmate
Length of output: 2121
🏁 Script executed:
# Check if there's a test setup file or package.json with test runner config
cat package.json | head -50Repository: SakuraByteCore/codexmate
Length of output: 1461
🏁 Script executed:
# Read the test runner to understand how tests work
cat tests/unit/run.mjsRepository: SakuraByteCore/codexmate
Length of output: 3842
🏁 Script executed:
# Read createSessionBrowserMethods in detail
sed -n '1,50p' web-ui/modules/app.methods.session-browser.mjsRepository: SakuraByteCore/codexmate
Length of output: 1630
🏁 Script executed:
# Look for the function definition and what it returns
rg "export.*createSessionBrowserMethods|function createSessionBrowserMethods" -A 30Repository: SakuraByteCore/codexmate
Length of output: 2512
🏁 Script executed:
# Verify session-search-ui.test.mjs is not in the test runner imports
grep "session-search-ui" tests/unit/run.mjsRepository: SakuraByteCore/codexmate
Length of output: 50
🏁 Script executed:
# List all test files that ARE being run
grep "await import" tests/unit/run.mjs | grep -oE "'[^']+\.test\.mjs'" | sortRepository: SakuraByteCore/codexmate
Length of output: 1218
Test file is not registered in the test suite and will never execute.
The file session-search-ui.test.mjs is not imported in tests/unit/run.mjs, so this test is dead code. Either add it to the test runner or remove the file. Additionally, if this test were to run, it would fail because createSessionBrowserMethods destructures loadActiveSessionDetailHelper from options (line 12 of app.methods.session-browser.mjs), but the test only passes api on line 61. To fix: add the test to tests/unit/run.mjs and provide the missing helper in the mock setup, or delete the orphaned test file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/session-search-ui.test.mjs` around lines 60 - 121, The test file
is orphaned and will never run and also misses a required mock helper: when
calling createSessionBrowserMethods you must provide
loadActiveSessionDetailHelper (the implementation expected by
loadActiveSessionDetail) in the test's mock options; to fix either (A) register
the test file in the test runner entry so it executes and update the test's mock
creation to pass loadActiveSessionDetailHelper (implement a simple stub matching
the real helper signature) alongside api, or (B) if the test is no longer
desired, remove the orphaned test file; ensure the test that remains calling
methods.loadActiveSessionDetail uses createSessionBrowserMethods with the
loadActiveSessionDetailHelper stub and then verify pendingSessionMatchKey,
activeSessionMessages matchSnippet/isSearchMatch and jumpToSessionTimelineNode
behavior.
| async loadActiveSessionDetail(options = {}) { | ||
| return loadActiveSessionDetailHelper.call(this, api, options); | ||
| const result = await loadActiveSessionDetailHelper.call(this, api, options); | ||
| const match = normalizeSessionMatch(this.activeSession); | ||
| const nextMatchKey = findSessionMessageMatchKey(this.activeSessionMessages, match.snippets); | ||
| this.pendingSessionMatchKey = nextMatchKey; | ||
| if (nextMatchKey) { | ||
| this.activeSessionMessages = Object.freeze( | ||
| this.activeSessionMessages.map((message, index) => { | ||
| const key = this.getRecordRenderKey(message, index); | ||
| if (key !== nextMatchKey) { | ||
| return message; | ||
| } | ||
| return Object.freeze({ | ||
| ...message, | ||
| matchSnippet: match.primarySnippet | ||
| }); | ||
| }) | ||
| ); | ||
| this.$nextTick(() => { | ||
| if (this.pendingSessionMatchKey !== nextMatchKey) return; | ||
| if (typeof this.jumpToSessionTimelineNode === 'function') { | ||
| this.jumpToSessionTimelineNode(nextMatchKey); | ||
| } | ||
| }); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Potential race condition between match computation and async helper result.
The match is computed from this.activeSession after loadActiveSessionDetailHelper completes, but this.activeSession may have changed if the user selected a different session while the async call was in-flight. The helper uses sessionDetailRequestSeq to guard its writes, but this code path doesn't verify that the active session is still the same one that was loaded.
Consider capturing this.activeSession before the await and comparing after, or checking sessionDetailRequestSeq consistency before applying match annotations.
🛡️ Suggested guard
async loadActiveSessionDetail(options = {}) {
+ const sessionSnapshot = this.activeSession;
const result = await loadActiveSessionDetailHelper.call(this, api, options);
+ // Guard against session change during async load
+ if (this.activeSession !== sessionSnapshot) {
+ return result;
+ }
const match = normalizeSessionMatch(this.activeSession);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async loadActiveSessionDetail(options = {}) { | |
| return loadActiveSessionDetailHelper.call(this, api, options); | |
| const result = await loadActiveSessionDetailHelper.call(this, api, options); | |
| const match = normalizeSessionMatch(this.activeSession); | |
| const nextMatchKey = findSessionMessageMatchKey(this.activeSessionMessages, match.snippets); | |
| this.pendingSessionMatchKey = nextMatchKey; | |
| if (nextMatchKey) { | |
| this.activeSessionMessages = Object.freeze( | |
| this.activeSessionMessages.map((message, index) => { | |
| const key = this.getRecordRenderKey(message, index); | |
| if (key !== nextMatchKey) { | |
| return message; | |
| } | |
| return Object.freeze({ | |
| ...message, | |
| matchSnippet: match.primarySnippet | |
| }); | |
| }) | |
| ); | |
| this.$nextTick(() => { | |
| if (this.pendingSessionMatchKey !== nextMatchKey) return; | |
| if (typeof this.jumpToSessionTimelineNode === 'function') { | |
| this.jumpToSessionTimelineNode(nextMatchKey); | |
| } | |
| }); | |
| } | |
| return result; | |
| } | |
| async loadActiveSessionDetail(options = {}) { | |
| const sessionSnapshot = this.activeSession; | |
| const result = await loadActiveSessionDetailHelper.call(this, api, options); | |
| // Guard against session change during async load | |
| if (this.activeSession !== sessionSnapshot) { | |
| return result; | |
| } | |
| const match = normalizeSessionMatch(this.activeSession); | |
| const nextMatchKey = findSessionMessageMatchKey(this.activeSessionMessages, match.snippets); | |
| this.pendingSessionMatchKey = nextMatchKey; | |
| if (nextMatchKey) { | |
| this.activeSessionMessages = Object.freeze( | |
| this.activeSessionMessages.map((message, index) => { | |
| const key = this.getRecordRenderKey(message, index); | |
| if (key !== nextMatchKey) { | |
| return message; | |
| } | |
| return Object.freeze({ | |
| ...message, | |
| matchSnippet: match.primarySnippet | |
| }); | |
| }) | |
| ); | |
| this.$nextTick(() => { | |
| if (this.pendingSessionMatchKey !== nextMatchKey) return; | |
| if (typeof this.jumpToSessionTimelineNode === 'function') { | |
| this.jumpToSessionTimelineNode(nextMatchKey); | |
| } | |
| }); | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/modules/app.methods.session-browser.mjs` around lines 439 - 465,
Before awaiting loadActiveSessionDetailHelper in loadActiveSessionDetail,
capture the identity/state used to compute the match (e.g., const currentSession
= this.activeSession or const seq = this.sessionDetailRequestSeq) and after the
await, verify that the captured value still matches (compare currentSession ===
this.activeSession or seq === this.sessionDetailRequestSeq) before computing
match and mutating
this.activeSessionMessages/pendingSessionMatchKey/jumpToSessionTimelineNode; if
the check fails, skip applying the match annotations to avoid the race. Ensure
you reference loadActiveSessionDetail, loadActiveSessionDetailHelper,
this.activeSession, and sessionDetailRequestSeq when implementing the guard.
|
@coderabbitai if I merge this PR directly, will it introduce any regression risk? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the actual code changes to give you a thorough answer. 🏁 Script executed: Length of output: 69684
🟡 Risk 1 —
|
3 participants
Summary
Testing
Summary by CodeRabbit
New Features
Tests