fix(activity): prevent ui flicker on silent reload - #145
Conversation
- Introduce `shouldShowLoading` to control loading state visibility - Use `loadedRangeRef` to track previously loaded range for comparison - Skip `setSelectedKey(undefined)` on silent reloads to preserve user selection - Add test for `shouldShowLoading` function - Add test for session key stability during reloads This change addresses the UX regression where the activity view would flicker and clear user selections during background refreshes triggered by the workspace monitor. Now, only range changes or initial loads will show the loading state and clear selections, while token-only refreshes will update data silently and maintain the selected session. Refs: #42
📝 WalkthroughWalkthroughAdds a ChangesSilent reload behavior
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceMonitor
participant ActivityCalendarView
participant shouldShowLoading
WorkspaceMonitor->>ActivityCalendarView: reload token increment
ActivityCalendarView->>shouldShowLoading: loadedRangeRef.current, loadRange
shouldShowLoading-->>ActivityCalendarView: showLoading boolean
alt showLoading true (range changed)
ActivityCalendarView->>ActivityCalendarView: setLoading(true)
ActivityCalendarView->>ActivityCalendarView: setSelectedKey(undefined)
else showLoading false (same range)
ActivityCalendarView->>ActivityCalendarView: keep loading state and selection
end
ActivityCalendarView->>ActivityCalendarView: update loadedRangeRef.current
Related PRs: None mentioned. Suggested labels: companion, bugfix Suggested reviewers: None determined from provided context. 🐰 A calendar block, once picked, stays true, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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)
apps/companion/src/activity/ActivityCalendarView.tsx (1)
353-383: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep background reload failures non-destructive. In
apps/companion/src/activity/ActivityCalendarView.tsx, thecatchpath still doessetError(...)andsetEvents([])even whenshowLoadingis false, so a token-only refresh failure clears the current calendar and drops into the error branch. Only clear the view when the reload is actually taking over the screen, or surface the error without discarding the existing events.🤖 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 `@apps/companion/src/activity/ActivityCalendarView.tsx` around lines 353 - 383, The reload handling in ActivityCalendarView should not clear the current calendar on background refresh failures. In the loadEvents promise chain, adjust the catch path so that when showLoading is false it does not call setEvents([]), and only sets the error state or clears the view when the refresh is actually replacing the screen. Use the existing showLoading flag, loadEvents, and setError/setEvents logic to keep token-only refresh failures non-destructive.
🧹 Nitpick comments (1)
apps/companion/tests/activity-calendar.test.tsx (1)
314-339: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for silent-reload failure behavior.
Given the
catchhandling inActivityCalendarView.tsxcurrently clearseventsregardless ofshowLoading(see companion comment on that file), a test simulating a token-only reload whereloadEventsrejects would catch that regression and guard the intended silent-reload UX going forward.🤖 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 `@apps/companion/tests/activity-calendar.test.tsx` around lines 314 - 339, Add a test that covers the silent-reload failure path in shouldShowLoading/ActivityCalendarView by simulating a reload where only the reload token changes for the same visible range and loadEvents rejects. Verify the component does not clear existing events or fall back to the loading placeholder in this token-only reload case, so the catch behavior in ActivityCalendarView.tsx is guarded against regressing the silent-reload UX.
🤖 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 `@apps/companion/src/activity/ActivityCalendarView.tsx`:
- Around line 353-383: The reload handling in ActivityCalendarView should not
clear the current calendar on background refresh failures. In the loadEvents
promise chain, adjust the catch path so that when showLoading is false it does
not call setEvents([]), and only sets the error state or clears the view when
the refresh is actually replacing the screen. Use the existing showLoading flag,
loadEvents, and setError/setEvents logic to keep token-only refresh failures
non-destructive.
---
Nitpick comments:
In `@apps/companion/tests/activity-calendar.test.tsx`:
- Around line 314-339: Add a test that covers the silent-reload failure path in
shouldShowLoading/ActivityCalendarView by simulating a reload where only the
reload token changes for the same visible range and loadEvents rejects. Verify
the component does not clear existing events or fall back to the loading
placeholder in this token-only reload case, so the catch behavior in
ActivityCalendarView.tsx is guarded against regressing the silent-reload UX.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8c31098-5ff8-4baf-a9d4-d2a632cdb4b2
📒 Files selected for processing (3)
apps/companion/src/activity/ActivityCalendarView.tsxapps/companion/tests/activity-calendar.test.tsxdocs/plans/0042-companion-activity-silent-reload.md
1 participant
shouldShowLoadingto control loading state visibilityloadedRangeRefto track previously loaded range for comparisonsetSelectedKey(undefined)on silent reloads to preserve user selectionshouldShowLoadingfunctionThis change addresses the UX regression where the activity view would flicker and clear user selections during background refreshes triggered by the workspace monitor. Now, only range changes or initial loads will show the loading state and clear selections, while token-only refreshes will update data silently and maintain the selected session.
Refs: #42
Summary by CodeRabbit