-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: reset table selection when navigating away from index page #9140
base: main
Are you sure you want to change the base?
fix: reset table selection when navigating away from index page #9140
Conversation
- Add RecordIndexResetSelectionEffect component to handle selection cleanup - Integrate effect into RecordIndexPage - Ensures checkbox selections are cleared when navigating away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Adds a new cleanup effect component to reset table row selections when navigating away from index pages, fixing the issue of persisting selections between page navigations.
- Added new
packages/twenty-front/src/modules/object-record/record-index/components/RecordIndexResetSelectionEffect.tsx
to handle selection cleanup on unmount - Integrated
RecordIndexResetSelectionEffect
intoRecordIndexPage.tsx
within the component hierarchy - Uses
useResetTableRowSelection
hook to clear row selections and close action menu dropdown - Leverages React's useEffect cleanup function for proper selection state management
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you :)
@samyakpiya I have approved first but we actually would like to test a different pattern. We are trying to get rid of the useEffects completely on the codebase The right way to do that is to let the caller synchronously reset the table selection. But how could the link in RecordDetailSectionHeader know that it should reset the table selection: let's introduce a component that will include a <Link to={...} onClick{resetTableSelection} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above
@charlesBochet Thanks for the feedback! I understand we want to move away from useEffect and handle the selection reset synchronously through the Link component's onClick. I'll update the PR to implement this pattern instead. |
@charlesBochet I've considered the pattern you suggested but have some technical concerns to discuss. The current suggestion to handle reset via
Additionally, implementing this through Links would require lifting state management up the component tree to make the reset function available to both index and show pages. Would it make more sense to handle this at the routing/navigation level instead? This would ensure consistent behavior regardless of how the user leaves the index page. I'm happy to explore alternative patterns that don't use useEffect while still covering all these edge cases. |
Fixes #9132
Purpose
Currently, when navigating away from an index page and returning, previously checked selections remain checked. This fix ensures the selection context is properly reset on navigation.
Changes
RecordIndexResetSelectionEffect
component to handle selection cleanupRecordIndexPage
useResetTableRowSelection
hook to clear selectionsTesting
Demo
I've recorded a short video demonstrating how this PR fixes the issue:
Loom Video Link
Before fix: Selections persist after navigation
After fix: Selections are properly reset