fix(viewer): handle sessions missing ids#366
Conversation
|
@honor2030 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR fixes critical viewer rendering failures when session IDs are missing or undefined by introducing four helper functions to safely derive session identifiers and labels, then applying them across all session-facing UI components (Dashboard, Timeline, Activity, Sessions list, detail view, and Replay selector) with explicit fallback behavior. ChangesSession ID Resilience
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 3
🧹 Nitpick comments (1)
test/viewer-session-id.test.ts (1)
97-100: 💤 Low valueThe auto-start regex pattern is fragile and tightly coupled to script output format.
This pattern will break if function names, order, whitespace, or additional auto-start calls change in the viewer script. Consider extracting the expected pattern as a named constant with a comment explaining the dependency, or use a more flexible approach like multiple small replacements for each function call.
♻️ More maintainable approach
- const scriptWithoutAutoStart = scriptMatch[1].replace( - /\n\s*loadTab\('dashboard'\);\n\s*connectWs\(\);\n\s*startDashboardAutoRefresh\(\);\s*$/, - "\n", - ); + // Strip viewer auto-start calls that would interfere with test control + let scriptWithoutAutoStart = scriptMatch[1]; + scriptWithoutAutoStart = scriptWithoutAutoStart.replace(/\bloadTab\s*\([^)]*\)\s*;?\s*/g, ""); + scriptWithoutAutoStart = scriptWithoutAutoStart.replace(/\bconnectWs\s*\([^)]*\)\s*;?\s*/g, ""); + scriptWithoutAutoStart = scriptWithoutAutoStart.replace(/\bstartDashboardAutoRefresh\s*\([^)]*\)\s*;?\s*/g, "");This approach removes each function call independently, making it resilient to order changes and whitespace variations.
🤖 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 `@test/viewer-session-id.test.ts` around lines 97 - 100, The current fragile single-regex removal in scriptWithoutAutoStart tightly couples to exact order/whitespace of loadTab('dashboard'); connectWs(); startDashboardAutoRefresh(); — replace it with a more resilient approach: extract the expectation into a named constant (e.g., AUTO_START_CALLS_PATTERN) and/or perform separate, small regex replacements that each target loadTab\('dashboard'\)\s*;, connectWs\(\)\s*; and startDashboardAutoRefresh\(\)\s*; independently (using flexible whitespace flags) so order changes or extra whitespace won't break the test; update the code that constructs scriptWithoutAutoStart to use these constants/replacements and add a short comment explaining why this is necessary.
🤖 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 `@src/viewer/index.html`:
- Around line 2586-2590: The selection mismatch comes from normalizing IDs with
sessionId(s) when rendering (used for data-session-id) but still comparing raw
x.id === state.sessions.selectedId when opening details; fix by using the same
normalization function for the detail lookup (i.e., compare sessionId(x) ===
state.sessions.selectedId) or otherwise convert both sides to the same canonical
string form before comparing so numeric/typed IDs match; update any logic that
sets or compares state.sessions.selectedId to consistently use sessionId(...)
(references: sessionId, state.sessions.selectedId, the detail lookup that
compares x.id, and the session rendering that uses data-session-id).
- Around line 2224-2227: The default timeline selection currently assigns
state.timeline.sessionId = sorted[0].id which uses the raw upstream id and can
be invalid; change the initialization to use the normalized sessionId helper (or
pick the first sorted entry whose sessionId(s) returns a non-empty value) and
assign that normalized id (or '' fallback) to state.timeline.sessionId so the
initial selected option matches the normalized/disabled logic used when
rendering options (see sessionId, sorted, sessionLabel, and
state.timeline.sessionId).
In `@test/viewer-session-id.test.ts`:
- Around line 41-57: The mock document's querySelectorAll currently returns []
which prevents the viewer script from attaching event listeners; update the mock
in test/viewer-session-id.test.ts so document.querySelectorAll(selector) returns
an array of appropriate mock elements for selectors used by the viewer (at least
'.tab-bar button', '.view', and 'input[type="checkbox"]'). Each returned mock
element should implement addEventListener, getAttribute/setAttribute, classList
toggle/contains (or className), and for checkbox inputs a checked property;
reuse the existing createElement/getElement behaviors to construct these mocks
so the forEach loops in the viewer script execute and event handlers can be
attached.
---
Nitpick comments:
In `@test/viewer-session-id.test.ts`:
- Around line 97-100: The current fragile single-regex removal in
scriptWithoutAutoStart tightly couples to exact order/whitespace of
loadTab('dashboard'); connectWs(); startDashboardAutoRefresh(); — replace it
with a more resilient approach: extract the expectation into a named constant
(e.g., AUTO_START_CALLS_PATTERN) and/or perform separate, small regex
replacements that each target loadTab\('dashboard'\)\s*;, connectWs\(\)\s*; and
startDashboardAutoRefresh\(\)\s*; independently (using flexible whitespace
flags) so order changes or extra whitespace won't break the test; update the
code that constructs scriptWithoutAutoStart to use these constants/replacements
and add a short comment explaining why this is necessary.
🪄 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
Run ID: 15c16191-1c09-4467-a7a9-2bf8ead94fd8
📒 Files selected for processing (2)
src/viewer/index.htmltest/viewer-session-id.test.ts
| var id = sessionId(s); | ||
| var disabled = id ? '' : ' disabled'; | ||
| html += '<option value="' + esc(id) + '"' + (id && state.timeline.sessionId === id ? ' selected' : '') + disabled + '>' + esc(sessionLabel(s)) + '</option>'; | ||
| }); |
There was a problem hiding this comment.
Default timeline session selection still uses raw id upstream.
These options correctly normalize/disable IDs, but initial selection still sets state.timeline.sessionId = sorted[0].id (Line 2212). If the newest session has a missing/invalid id, timeline starts empty even when later sessions are valid.
Suggested fix
- if (sessions.length > 0 && !state.timeline.sessionId) {
- var sorted = sessions.slice().sort(function(a, b) { return (b.startedAt || '').localeCompare(a.startedAt || ''); });
- state.timeline.sessionId = sorted[0].id;
- }
+ if (sessions.length > 0 && !state.timeline.sessionId) {
+ var sorted = sessions.slice().sort(function(a, b) { return (b.startedAt || '').localeCompare(a.startedAt || ''); });
+ var firstWithId = sorted.find(function(s) { return sessionId(s); });
+ state.timeline.sessionId = firstWithId ? sessionId(firstWithId) : '';
+ }📝 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.
| var id = sessionId(s); | |
| var disabled = id ? '' : ' disabled'; | |
| html += '<option value="' + esc(id) + '"' + (id && state.timeline.sessionId === id ? ' selected' : '') + disabled + '>' + esc(sessionLabel(s)) + '</option>'; | |
| }); | |
| if (sessions.length > 0 && !state.timeline.sessionId) { | |
| var sorted = sessions.slice().sort(function(a, b) { return (b.startedAt || '').localeCompare(a.startedAt || ''); }); | |
| var firstWithId = sorted.find(function(s) { return sessionId(s); }); | |
| state.timeline.sessionId = firstWithId ? sessionId(firstWithId) : ''; | |
| } |
🤖 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 `@src/viewer/index.html` around lines 2224 - 2227, The default timeline
selection currently assigns state.timeline.sessionId = sorted[0].id which uses
the raw upstream id and can be invalid; change the initialization to use the
normalized sessionId helper (or pick the first sorted entry whose sessionId(s)
returns a non-empty value) and assign that normalized id (or '' fallback) to
state.timeline.sessionId so the initial selected option matches the
normalized/disabled logic used when rendering options (see sessionId, sorted,
sessionLabel, and state.timeline.sessionId).
| var id = sessionId(s); | ||
| var selected = id && state.sessions.selectedId === id; | ||
| html += '<div class="session-item' + (selected ? ' selected' : '') + '"' + (id ? ' data-action="select-session" data-session-id="' + esc(id) + '"' : '') + '>'; | ||
| html += '<div class="session-top"><span class="session-project">' + esc(sessionDisplayName(s)) + '</span>'; | ||
| html += '<span class="badge ' + statusBadge + '">' + esc(s.status) + '</span></div>'; |
There was a problem hiding this comment.
Selection ID normalization can break detail lookup for non-string IDs.
selectedId is now normalized via sessionId(s) (string), but detail lookup still compares raw x.id === state.sessions.selectedId (Line 2616). Numeric/typed IDs won’t match, so selecting a row can fail to open details.
Suggested fix
- var s = state.sessions.items.find(function(x) { return x.id === state.sessions.selectedId; });
+ var s = state.sessions.items.find(function(x) { return sessionId(x) === state.sessions.selectedId; });🤖 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 `@src/viewer/index.html` around lines 2586 - 2590, The selection mismatch comes
from normalizing IDs with sessionId(s) when rendering (used for data-session-id)
but still comparing raw x.id === state.sessions.selectedId when opening details;
fix by using the same normalization function for the detail lookup (i.e.,
compare sessionId(x) === state.sessions.selectedId) or otherwise convert both
sides to the same canonical string form before comparing so numeric/typed IDs
match; update any logic that sets or compares state.sessions.selectedId to
consistently use sessionId(...) (references: sessionId,
state.sessions.selectedId, the detail lookup that compares x.id, and the session
rendering that uses data-session-id).
| const document = { | ||
| documentElement: { dataset: {} }, | ||
| createElement: () => { | ||
| let text = ""; | ||
| return { | ||
| set textContent(value: unknown) { | ||
| text = String(value ?? ""); | ||
| }, | ||
| get innerHTML() { | ||
| return htmlEscape(text); | ||
| }, | ||
| }; | ||
| }, | ||
| getElementById: getElement, | ||
| querySelectorAll: () => [], | ||
| addEventListener: () => {}, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check DOM operations used in viewer script to ensure mocks are sufficient
# Search for DOM methods called on document or created elements
rg -n --type=ts --type=js -C2 '\b(appendChild|setAttribute|getAttribute|querySelector|querySelectorAll|insertBefore|removeChild|replaceChild|cloneNode)\s*\(' src/viewer/
# Search for createElement usage patterns
echo "=== createElement usage ==="
rg -n --type=ts --type=js -A5 'createElement\(' src/viewer/Repository: rohitg00/agentmemory
Length of output: 92
🏁 Script executed:
# Check if src/viewer/ directory exists and explore the repo structure
find . -type d -name "viewer" 2>/dev/null | head -20Repository: rohitg00/agentmemory
Length of output: 77
🏁 Script executed:
# List the test file to understand what's being tested
git ls-files test/ | grep -E "\.test\.ts$|\.spec\.ts$" | head -20Repository: rohitg00/agentmemory
Length of output: 584
🏁 Script executed:
# Read the test file to understand setup and what's imported
head -80 test/viewer-session-id.test.tsRepository: rohitg00/agentmemory
Length of output: 2318
🏁 Script executed:
# List files in src/viewer/
ls -la src/viewer/Repository: rohitg00/agentmemory
Length of output: 372
🏁 Script executed:
# Read the document.js file to understand what renderViewerDocument returns
head -100 src/viewer/document.jsRepository: rohitg00/agentmemory
Length of output: 146
🏁 Script executed:
# Search for actual DOM operations in the viewer script
rg -n 'createElement|appendChild|setAttribute|querySelector|getAttribute|insertBefore|removeChild|textContent|innerHTML' src/viewer/ --type=js --type=tsRepository: rohitg00/agentmemory
Length of output: 46
🏁 Script executed:
# Read the document.ts file
cat -n src/viewer/document.tsRepository: rohitg00/agentmemory
Length of output: 1545
🏁 Script executed:
# Search for DOM operations in the viewer code (correct extensions)
rg -n 'createElement|appendChild|setAttribute|querySelector|getAttribute|insertBefore|removeChild|textContent|innerHTML' src/viewer/Repository: rohitg00/agentmemory
Length of output: 7758
🏁 Script executed:
# Check what's in index.html - likely contains the viewer script
head -200 src/viewer/index.html | tail -150Repository: rohitg00/agentmemory
Length of output: 4478
querySelectorAll() returning an empty array prevents event listener attachment and DOM queries.
The mock document.querySelectorAll() always returns [], which breaks the viewer script's event handling. The script uses querySelectorAll('.tab-bar button').forEach(...), querySelectorAll('.view').forEach(...), and querySelectorAll('input[type="checkbox"]').forEach(...) (at lines 1105–1108, 1537 in src/viewer/index.html) to attach event listeners and manipulate elements. With an empty array, these forEach loops never execute, so button clicks and UI interactions won't work in the test even though they would in the real DOM.
The mock does correctly support getAttribute(), setAttribute(), textContent, and innerHTML, and the viewer script doesn't use appendChild(), querySelector(), or other methods originally mentioned. However, the empty array from querySelectorAll() is a critical gap that makes the mock insufficient for testing interactive viewer behavior.
🤖 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 `@test/viewer-session-id.test.ts` around lines 41 - 57, The mock document's
querySelectorAll currently returns [] which prevents the viewer script from
attaching event listeners; update the mock in test/viewer-session-id.test.ts so
document.querySelectorAll(selector) returns an array of appropriate mock
elements for selectors used by the viewer (at least '.tab-bar button', '.view',
and 'input[type="checkbox"]'). Each returned mock element should implement
addEventListener, getAttribute/setAttribute, classList toggle/contains (or
className), and for checkbox inputs a checked property; reuse the existing
createElement/getElement behaviors to construct these mocks so the forEach loops
in the viewer script execute and event handlers can be attached.
1 participant
Summary
Unknown session/missing idinstead of throwing.undefinedaction payloads.Red / Green
PATH=/opt/homebrew/bin:$PATH npm test -- test/viewer-session-id.test.ts --reporter=verbosefailed before the implementation withTypeError.PATH=/opt/homebrew/bin:$PATH npm test -- test/viewer-session-id.test.ts test/viewer-security.test.ts --reporter=verbose— 2 files / 4 tests passed.PATH=/opt/homebrew/bin:$PATH npm run build— passed.PATH=/opt/homebrew/bin:$PATH npm test -- --reporter=dot— 84 files / 905 tests passed.Closes #340
Closes #347
Summary by CodeRabbit
Release Notes
New Features
Tests