fix(viewer): preserve dashboard scroll and show API errors#694
Conversation
|
Someone 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 adds a toast notification system to surface API errors, implements a 10-second request timeout with AbortController, refactors dashboard loading to preserve scroll position during live updates, and integrates both polling and websocket streams to use the scroll-preserving refresh flow. Tests verify scroll preservation and toast display for HTTP errors, connection failures, and request timeouts. ChangesViewer Toast Notifications and Dashboard Refresh
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)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/viewer/index.html (1)
1346-1399:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential race condition in concurrent refreshes may reset scroll position.
The scroll preservation works correctly in the common case, but if
refreshDashboard()is called twice in quick succession (e.g., from both auto-refresh timer and an incoming websocket message), the second call may savescrollTopafter the first call has already setinnerHTML(which resets scroll to 0), causing the scroll position to reset rather than preserve.The race window is small but possible given that both the 30s auto-refresh timer (line 3667) and websocket updates (line 3637) can trigger
refreshDashboard()without coordination.Consider adding a simple in-progress guard:
🛡️ Proposed fix to prevent concurrent refresh race
var dashboardTimer = null; +var dashboardRefreshing = false; function refreshDashboard() { + if (dashboardRefreshing) return Promise.resolve(); + dashboardRefreshing = true; state.dashboard.loaded = false; - return loadDashboard({ preserveScroll: true }); + return loadDashboard({ preserveScroll: true }) + .finally(function() { dashboardRefreshing = false; }); }🤖 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 1346 - 1399, loadDashboard can race when called concurrently (e.g., by refreshDashboard from timer and websocket) so preserveScroll/priorScrollTop can be captured after innerHTML has been reset; add a simple in-progress guard (e.g., a module-level boolean like dashboardRefreshInProgress) in refreshDashboard/loadDashboard to short-circuit or serialize concurrent invocations: check the flag at the start of refreshDashboard/loadDashboard, set it before mutating DOM (before setting el.innerHTML or starting Promise.all), and clear it in the finally block after renderDashboard completes or on error so subsequent calls either return early or wait/queue, ensuring the saved priorScrollTop reflects the DOM state and avoids resetting scroll.
🤖 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 `@src/viewer/index.html`:
- Around line 1346-1399: loadDashboard can race when called concurrently (e.g.,
by refreshDashboard from timer and websocket) so preserveScroll/priorScrollTop
can be captured after innerHTML has been reset; add a simple in-progress guard
(e.g., a module-level boolean like dashboardRefreshInProgress) in
refreshDashboard/loadDashboard to short-circuit or serialize concurrent
invocations: check the flag at the start of refreshDashboard/loadDashboard, set
it before mutating DOM (before setting el.innerHTML or starting Promise.all),
and clear it in the finally block after renderDashboard completes or on error so
subsequent calls either return early or wait/queue, ensuring the saved
priorScrollTop reflects the DOM state and avoids resetting scroll.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4b5112c-69e9-448d-8fd2-0300b7e8df3e
📒 Files selected for processing (2)
src/viewer/index.htmltest/viewer-session-id.test.ts
1 participant
Summary:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests