Touch devices fix, scroll to fist unread update#1788
Conversation
Signed-off-by: prxt6529 <prxt@6529.io>
📝 WalkthroughWalkthroughRemoves serialNo from navigation/state, adds UnreadDividerProvider and a scroll-to-unread FAB (WaveDropsScrollToUnreadButton) wired through drops list, refactors touch detection to guarded globalThis and event-driven listeners, and updates tests/mocks accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WaveDropsContent
participant WaveDropsMessageListSection
participant WaveDropsScrollToUnreadButton
participant UnreadDividerContext
User->>WaveDropsContent: scrolls / views drops
WaveDropsContent->>WaveDropsMessageListSection: pass scrollContainerRef, unreadDividerSerialNo, onScrollToUnread
WaveDropsMessageListSection->>WaveDropsScrollToUnreadButton: render FAB when unread visible/above/below
User->>WaveDropsScrollToUnreadButton: click
WaveDropsScrollToUnreadButton->>WaveDropsMessageListSection: onScrollToUnread(serialNo)
WaveDropsMessageListSection->>WaveDropsMessageListSection: scroll to "#drop-{serial}"
WaveDropsMessageListSection->>UnreadDividerContext: update/persist divider state
sequenceDiagram
participant Browser
participant useIsTouchDevice
participant matchMedia
participant EventTarget as addEventListener
participant User
Browser->>useIsTouchDevice: mount hook
useIsTouchDevice->>matchMedia: query "(pointer: fine)"
matchMedia-->>useIsTouchDevice: matches? (true/false)
alt matches true
useIsTouchDevice-->>Browser: return false (no touch)
else matches false
useIsTouchDevice->>EventTarget: register one-time touchstart handler
useIsTouchDevice-->>Browser: initial false
User->>Browser: touchstart event
EventTarget->>useIsTouchDevice: notify handler
useIsTouchDevice->>useIsTouchDevice: set state true, remove handler
end
Browser->>useIsTouchDevice: unmount
useIsTouchDevice->>EventTarget: ensure handler removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/waves/drops/wave-drops-all/index.tsx (1)
118-126: Stale divider state whendividerSerialNochanges for the same wave.The guard on lines 119-121 prevents re-initialization when
waveIdhasn't changed, butdividerSerialNois also in the dependency array. If a user navigates to the same wave with a differentdividerSerialNo, the effect exits early without callingsetUnreadDividerSerialNo(dividerSerialNo ?? null).Suggested fix: track both waveId and dividerSerialNo
- const initializedWaveRef = useRef<string | null>(null); + const initializedRef = useRef<{ waveId: string; dividerSerialNo: number | null | undefined } | null>(null); useEffect(() => { - if (initializedWaveRef.current === waveId) { + if ( + initializedRef.current?.waveId === waveId && + initializedRef.current?.dividerSerialNo === dividerSerialNo + ) { return; } - initializedWaveRef.current = waveId; + initializedRef.current = { waveId, dividerSerialNo }; setVisibleLatestSerial(null); prevLatestSerialNoRef.current = null; setUnreadDividerSerialNo(dividerSerialNo ?? null); }, [waveId, dividerSerialNo, setUnreadDividerSerialNo]);
🤖 Fix all issues with AI agents
In `@components/nextGen/collections/collectionParts/mint/NextGenMintWidget.tsx`:
- Around line 333-340: Guard against empty or undefined proof objects returned
by findActiveProof: when computing activeProof from originalProofs, only call
setCurrentProof({ ...activeProof, proof: activeProof.proof }) if activeProof
exists and activeProof.proof is defined; otherwise do nothing (or reset to a
safe default). Update the useEffect that references findActiveProof,
originalProofs, setCurrentProof and currentProof to short‑circuit when
activeProof?.proof is falsy so state remains consistent.
In `@hooks/useDeviceInfo.ts`:
- Around line 37-46: The check for viewport width uses win.matchMedia without
guarding for lack of support—make widthMobile use optional chaining and a safe
fallback like the earlier hasFinePointer line; update the expression that sets
widthMobile (which references win.matchMedia("(max-width: 768px)").matches) to
something that first verifies win.matchMedia exists (e.g.,
win.matchMedia?.("(max-width: 768px)")?.matches ?? false) so it won't throw in
environments where matchMedia is undefined while keeping the same boolean
semantics alongside hasFinePointer and hasTouchScreen.
🧹 Nitpick comments (3)
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx (1)
88-93: LGTM! Test expectation correctly updated for divider-based navigation.The change from
serialNotodivider: nullin thesetActiveWavecall expectation aligns with the PR's architectural shift.Consider adding an additional test case to verify the
setActiveWavebehavior whenfirstUnreadDropSerialNois present (non-null), ensuring the divider value is correctly passed on click:💡 Suggested test case
it('passes divider to setActiveWave on click when firstUnreadDropSerialNo is present', async () => { const waveWithUnread = { ...baseWave, id: '3', firstUnreadDropSerialNo: 42 }; render(<BrainLeftSidebarWave wave={waveWithUnread} onHover={onHover} />); const link = screen.getByRole('link'); await userEvent.click(link); expect(setActiveWave).toHaveBeenCalledWith('3', { isDirectMessage: false, divider: 42 }); });hooks/useIsTouchDevice.ts (1)
13-17: Redundant state update on line 15.
setIsTouchDevice(false)is unnecessary since the initial state is alreadyfalse. This call triggers a no-op re-render cycle.Suggested simplification
const hasFinePointer = window.matchMedia?.("(pointer: fine)")?.matches; if (hasFinePointer) { - setIsTouchDevice(false); return; }components/brain/my-stream/MyStreamWaveChat.tsx (1)
51-56: Render-time ref mutation is unconventional but acceptable here.The pattern of mutating
capturedDividerRefduring render works for lazy initialization but can be confusing. An alternative is using a state initializer oruseMemo. However, since this captures a value once per wave change and doesn't cause re-renders, it's acceptable.Alternative using useMemo for clarity
- const capturedDividerRef = useRef<{ waveId: string; serialNo: number | null } | null>(null); - - if (!capturedDividerRef.current || capturedDividerRef.current.waveId !== wave.id) { - capturedDividerRef.current = { waveId: wave.id, serialNo: firstUnreadSerialNo }; - } + const capturedDivider = useMemo( + () => ({ waveId: wave.id, serialNo: firstUnreadSerialNo }), + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally capture only on wave.id change + [wave.id] + );Then use
capturedDivider.serialNoinstead ofcapturedDividerRef.current.serialNo.
Signed-off-by: prxt6529 <prxt@6529.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/brain/my-stream/MyStreamWaveChat.tsx`:
- Around line 68-72: The capturedDividerRef is only initialized from
firstUnreadSerialNo and never updated from initialDropState, causing
dividerTarget to revert when URL params are cleared; add a useEffect that runs
when initialDropState or wave.id changes and, if
initialDropState?.dividerSerialNo is defined, set capturedDividerRef.current = {
waveId: wave.id, serialNo: initialDropState.dividerSerialNo } so the ref stays
synced with the actual divider value (ensure you reference capturedDividerRef,
initialDropState, wave.id, and firstUnreadSerialNo in the effect and guard
nulls); apply the same sync logic for the other occurrence noted around lines
104-108.
♻️ Duplicate comments (1)
hooks/useDeviceInfo.ts (1)
46-46: GuardmatchMediausage consistently.Line 46 does not use optional chaining for
matchMedia, while line 37 does. This could throw in test environments or browsers withoutmatchMediasupport.- const widthMobile = win.matchMedia("(max-width: 768px)").matches; + const widthMobile = win.matchMedia?.("(max-width: 768px)")?.matches ?? false;
🧹 Nitpick comments (2)
hooks/useDeviceInfo.ts (1)
64-92: Consider guardingglobalThisin useEffect for consistency.The
getInfofunction guards against undefinedglobalThis(line 19), but theuseEffectblock directly callsglobalThis.addEventListener/removeEventListenerwithout a similar guard. This inconsistency could cause issues in certain test environments.Suggested guard
useEffect(() => { + if (typeof globalThis === "undefined") { + return; + } + const update = () => setInfo((prev) => { const next = getInfo(touchDetectedRef.current); // ... }); globalThis.addEventListener("resize", update); // ... }, [getInfo]);__tests__/hooks/useIsTouchDevice.test.ts (1)
18-23: Consider cleaning upmatchMediaproperty definition inafterEach.Each test uses
Object.definePropertyto setglobalThis.matchMedia, but this isn't cleaned up inafterEach. Whilejest.restoreAllMocks()restores spies, it doesn't revert property definitions. This could cause test pollution if the order of tests changes or new tests are added.Suggested cleanup
+ let originalMatchMedia: typeof globalThis.matchMedia | undefined; + beforeEach(() => { + originalMatchMedia = globalThis.matchMedia; addEventListenerSpy = jest.spyOn(globalThis, "addEventListener").mockImplementation((event, handler) => { if (event === "touchstart") { touchStartHandler = handler as EventListener; } }); removeEventListenerSpy = jest.spyOn(globalThis, "removeEventListener"); }); afterEach(() => { addEventListenerSpy.mockRestore(); removeEventListenerSpy.mockRestore(); touchStartHandler = null; jest.restoreAllMocks(); + if (originalMatchMedia !== undefined) { + globalThis.matchMedia = originalMatchMedia; + } else { + delete (globalThis as any).matchMedia; + } });
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hooks/useDeviceInfo.ts`:
- Around line 79-90: The hook currently calls
globalThis.addEventListener/removeEventListener directly for "resize" and
"touchstart" (handlers update and onceTouch, toggling touchDetectedRef.current),
which can throw in non-DOM runtimes; add a guard checking that
globalThis.addEventListener and globalThis.removeEventListener are functions
before registering or removing events (similar to the pattern in
useWebSocketHealth.ts), and only call globalThis.addEventListener("touchstart",
onceTouch, { passive: true }) if that API exists; ensure the cleanup return also
checks for removeEventListener before attempting to remove resize and touchstart
listeners.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hooks/useDeviceInfo.ts`:
- Around line 28-38: The current hasTouchScreen calculation in useDeviceInfo
incorrectly forces false when hasFinePointer is true; update the logic that
computes hasTouchScreen (referencing hasFinePointer,
touchDetected/touchDetectedRef, and nav.maxTouchPoints) so hybrid devices are
detected: treat hasTouchScreen as true if touchDetected (or
touchDetectedRef.current) is true OR navigator.maxTouchPoints > 0, regardless of
hasFinePointer, while still using matchMedia to detect fine pointers for
hasFinePointer itself.
Signed-off-by: prxt6529 <prxt@6529.io>
|
3 participants
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.