fix: tolerate partial dashboard payloads#365
Conversation
|
@NgoQuocViet2001 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
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 (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThis PR hardens the viewer dashboard against null/undefined or unexpected API response shapes by adding normalization helpers, normalizing dashboard state fields during load, updating rendering to use filtered normalized arrays, and adding a sandboxed Vitest that verifies rendering with malformed data. ChangesDashboard rendering resilience
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
|
Thanks — the defensive normalization ( This branch now conflicts with main after #366 (#366) landed earlier today. #366 added session-id null-guard helpers ( Could you rebase against main and resolve? The two patches are complementary — yours handles missing-array fields, #366 handles missing-id session records. Likely the merged shape is: keep your I'll merge once it rebases clean + tests pass against current main (956 baseline). |
NgoQuocViet2001
force-pushed
the
fix/347-dashboard-shape
branch
from
73ac45e to
486f07f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/viewer-dashboard.test.ts (1)
39-42: ⚡ Quick winHarden script bootstrap stripping to avoid brittle test failures.
This replacement only works when those three calls appear in the exact order/format at EOF. A small bootstrap edit can make the test execute real init paths unintentionally.
Proposed diff
- return match[1].replace( - /\s+loadTab\('dashboard'\);\s+connectWs\(\);\s+startDashboardAutoRefresh\(\);\s*$/, - "\n", - ); + return match[1].replace( + /\n?\s*(loadTab\('dashboard'\)|connectWs\(\)|startDashboardAutoRefresh\(\));\s*$/gm, + "\n", + );🤖 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-dashboard.test.ts` around lines 39 - 42, The current replace uses a brittle regex anchored to a specific order of loadTab('dashboard'); connectWs(); startDashboardAutoRefresh(); at EOF (match[1].replace(...)), which breaks if bootstrap ordering/whitespace changes; update the replacement to robustly strip those init calls regardless of order/spacing by either performing three separate replaces on match[1] (e.g., remove /\s*loadTab\('dashboard'\);\s*/g, /\s*connectWs\(\);\s*/g, /\s*startDashboardAutoRefresh\(\);\s*/g) or use a single regex that matches any combination/order before EOF (e.g., replace(/(?:\s*(?:loadTab\('dashboard'\)|connectWs\(\)|startDashboardAutoRefresh\(\));?)+\s*$/,'\n')), so the test no longer depends on exact call order or formatting.
🤖 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.
Nitpick comments:
In `@test/viewer-dashboard.test.ts`:
- Around line 39-42: The current replace uses a brittle regex anchored to a
specific order of loadTab('dashboard'); connectWs();
startDashboardAutoRefresh(); at EOF (match[1].replace(...)), which breaks if
bootstrap ordering/whitespace changes; update the replacement to robustly strip
those init calls regardless of order/spacing by either performing three separate
replaces on match[1] (e.g., remove /\s*loadTab\('dashboard'\);\s*/g,
/\s*connectWs\(\);\s*/g, /\s*startDashboardAutoRefresh\(\);\s*/g) or use a
single regex that matches any combination/order before EOF (e.g.,
replace(/(?:\s*(?:loadTab\('dashboard'\)|connectWs\(\)|startDashboardAutoRefresh\(\));?)+\s*$/,'\n')),
so the test no longer depends on exact call order or formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b3c2fea-c63e-4c84-81c4-8d8dd49625d5
📒 Files selected for processing (2)
src/viewer/index.htmltest/viewer-dashboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/viewer/index.html
NgoQuocViet2001
force-pushed
the
fix/347-dashboard-shape
branch
from
486f07f to
d90246d
Compare
|
Thanks! I rebased this branch onto current I kept the Validation: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/viewer-dashboard.test.ts (1)
120-167: 💤 Low valueConsider asserting no console errors to catch silent failures.
The test correctly verifies
renderDashboarddoesn't throw and renders expected content. Adding an assertion thatconsole.errorwasn't called would strengthen confidence that malformed data is handled gracefully without silent errors.♻️ Optional improvement
expect(() => (sandbox.renderDashboard as () => void)()).not.toThrow(); + expect(sandbox.console.error).not.toHaveBeenCalled(); const dashboard = elements.get("view-dashboard");🤖 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-dashboard.test.ts` around lines 120 - 167, The test should also assert that no console errors were emitted when rendering malformed dashboard data: before calling (sandbox.renderDashboard as () => void)(), spy or mock console.error (e.g., replace with a jest.spyOn or sinon.stub tied to the test's sandbox/context), call renderDashboard, then assert the spy/stub was not called; finally restore the original console.error. Target symbols: sandbox, renderDashboard, and the test's lifecycle so the console.error mock is cleaned up after the assertion.
🤖 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.
Nitpick comments:
In `@test/viewer-dashboard.test.ts`:
- Around line 120-167: The test should also assert that no console errors were
emitted when rendering malformed dashboard data: before calling
(sandbox.renderDashboard as () => void)(), spy or mock console.error (e.g.,
replace with a jest.spyOn or sinon.stub tied to the test's sandbox/context),
call renderDashboard, then assert the spy/stub was not called; finally restore
the original console.error. Target symbols: sandbox, renderDashboard, and the
test's lifecycle so the console.error mock is cleaned up after the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd6cc7fe-d57e-4e71-9f96-40ae557c116d
📒 Files selected for processing (2)
src/viewer/index.htmltest/viewer-dashboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/viewer/index.html
Signed-off-by: NgoQuocViet2001 <ngoquocviet2001@gmail.com>
NgoQuocViet2001
force-pushed
the
fix/347-dashboard-shape
branch
from
d90246d to
911873e
Compare
|
Rebased this onto current Validation:
|
2 participants
Summary
projectandid; renderunknowninstead.Fixes #347.
Refs #340.
Why
Windows users reported the Dashboard tab failing to load with
Cannot read properties of undefined (reading 'slice')after v0.9.12. The viewer assumed several dashboard payload fields were always arrays and that every session had anid, so one partial record could stop the whole dashboard from rendering.Tests
npx vitest run test/viewer-dashboard.test.ts test/viewer-security.test.tsnpm run buildwithnpm_config_script_shell="C:\\Program Files\\Git\\bin\\bash.exe"on Windows because the package build script uses POSIX shell commandsnpm testwas also attempted locally; the new viewer test passed, but the full suite still has unrelated local Windows failures in existing path/Python-dependent tests (compress-file,obsidian-export,integration-plaintext-http).Summary by CodeRabbit