Conversation
Note that it requires ScrollProvider to exist, otherwise, the status will stay forever as 'should_scroll_into_view'. It's OK for now.
There was a problem hiding this comment.
Pull request overview
This PR refactors the scroll and focus coordination mechanism to ensure that cell focus is properly executed after scrolling completes. The previous implementation used simple boolean flags (shouldScroll, shouldFocus) to coordinate these operations, which could lead to race conditions. The new implementation uses a state machine with explicit states and transitions to ensure proper sequencing.
Changes:
- Replaced boolean flags with a state machine (
FocusState) to coordinate scroll and focus operations - Introduced
FocusActiontypes to manage state transitions explicitly - Simplified
useInputStatehook by removinguseEffectEventand callingnotifyChangesynchronously - Updated
ScrollProviderto dispatch focus actions based on scroll events and status
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/contexts/CellNavigationContext.ts | Added FocusState interface and FocusAction union type; replaced shouldScroll and acknowledgeScroll with focusState and focusDispatch |
| src/providers/CellNavigationProvider.tsx | Implemented focus state reducer with state machine logic; replaced boolean state with reducer-based state management |
| src/providers/ScrollProvider.tsx | Integrated focus state machine with scroll operations; dispatches focus actions based on scroll status |
| src/hooks/useInputState.ts | Simplified by removing useEffectEvent and calling notifyChange synchronously in the setState callback |
| test/providers/CellNavigationProvider.test.tsx | Updated tests to use new focusState and focusDispatch API instead of focusCurrentCell checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // one extra render is needed for local scrolling, or at start if the scroll parameters are not ready yet | ||
| // To avoid infinite loops in case of unexpected issues, we use a counter to stop trying to scroll after 3 attempts (2 should be enough). | ||
| if (state.counter >= 3) { | ||
| // console.warn('Cannot scroll to the cell after 5 attempts. To avoid infinite loops, try to focus it now.') |
There was a problem hiding this comment.
The comment mentions "5 attempts" but the code checks for 3 attempts. This inconsistency between the comment and the actual code logic should be corrected. The comment should say "3 attempts" to match the code.
| // console.warn('Cannot scroll to the cell after 5 attempts. To avoid infinite loops, try to focus it now.') | |
| // console.warn('Cannot scroll to the cell after 3 attempts. To avoid infinite loops, try to focus it now.') |
| // update the state | ||
| dispatch(action) | ||
| }, [shouldScroll, acknowledgeScroll, cellPosition, scrollTo, scrollTopAnchor, localOffset, scale]) | ||
| }, [cellPosition, scrollTo, scrollTopAnchor, localOffset, scale, focusDispatch, focusState]) |
There was a problem hiding this comment.
The useEffect depends on the entire focusState object, but only uses focusState.status. This causes the effect to re-run whenever focusState.counter changes, even though the counter is not used in the effect. Consider destructuring focusState.status outside the effect and only including that in the dependency array, or using focusState.status directly in the dependency array if the linter allows it. This will prevent unnecessary re-executions of the effect when only the counter changes.
| focusDispatch?.({ type: 'SCROLLED_EVENT_RECEIVED' }) | ||
| }, [focusDispatch]) |
There was a problem hiding this comment.
The 'SCROLLED_EVENT_RECEIVED' action is dispatched on every scroll event, but the reducer only transitions to 'should_focus' when the state is 'scrolling_into_view'. This means that during user-initiated scrolling (when the state is 'idle'), these actions will be processed but have no effect. While this is functionally correct, it could be more efficient to only dispatch this action when focusState.status is 'scrolling_into_view' to avoid unnecessary reducer calls.
| focusDispatch?.({ type: 'SCROLLED_EVENT_RECEIVED' }) | |
| }, [focusDispatch]) | |
| if (focusDispatch && focusState.status === 'scrolling_into_view') { | |
| focusDispatch({ type: 'SCROLLED_EVENT_RECEIVED' }) | |
| } | |
| }, [focusDispatch, focusState]) |
severo
force-pushed
the
ensure-focus-is-run-after-scrolling
branch
from
a876300 to
cd8afce
Compare
No description provided.