feat: sync table sort state to the URL - #2029
Open
mentonin wants to merge 3 commits into
Open
Conversation
|
|
||
| const sortingUpdater: OnChangeFn<SortingState> = useCallback( | ||
| updater => { | ||
| navigate({ |
Contributor
There was a problem hiding this comment.
prefer to use replace instead of push here, or we might end up hurting navigation stack
| ]; | ||
| }; | ||
|
|
||
| export function HardwareTable({ |
Contributor
There was a problem hiding this comment.
This seems to be happening only on hardware listing. But if we select a sorting strategy, and later click in a row, the sorting parameter seems to be carried in the url.
Contributor
Author
There was a problem hiding this comment.
I think the new commit fixes this everywhere now
Member
|
code is looking good, I want to test it before approving |
mentonin
requested review from
felipebergamin
and
a lite review from Copilot
and removed request for
Copilot
August 4, 2026 21:09
Comment on lines
+93
to
+95
|
|
||
| export const useSortingState = ( | ||
| options: UseSortingStateOptions = {}, |
Member
There was a problem hiding this comment.
Suggested change
| export const useSortingState = ( | |
| options: UseSortingStateOptions = {}, | |
| const EMPTY_OBJECT = {}; | |
| export const useSortingState = ( | |
| options: UseSortingStateOptions = EMPTY_OBJECT, |
| [defaultSorting, sortValue], | ||
| ); | ||
|
|
||
| const sortingUpdater: OnChangeFn<SortingState> = useCallback( |
Member
There was a problem hiding this comment.
just to follow the convention I'd name it onUpdateSorting or handleSortingUpdate
| .default(DEFAULT_LISTING_ITEMS); | ||
|
|
||
| /** Sentinel for an explicit unsorted state when the table default is not unsorted. */ | ||
| export const TABLE_SORT_UNSORTED = 'none'; |
Member
There was a problem hiding this comment.
Suggested change
| export const TABLE_SORT_UNSORTED = 'none'; | |
| export const TABLE_SORT_UNSORTED = 'none' as const; |
Make sortable tables shareable via search params: flat s=… for single tables, keyed s|… when multiple tables share a page. Clear sort when switching details tabs so inactive tables do not keep stale ordering. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Luiz Georg <luiz.georg@profusion.mobi>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Felipe Bergamin <felipebergamin@profusion.mobi>
felipebergamin
force-pushed
the
sort-url
branch
from
August 7, 2026 20:08
3eca0dd to
e04b83d
Compare
Member
|
just rebased the branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useSortingStatehook and roottableSortsearch param, so sort order is shareable and bookmarkable.s=…; use keyed params (s|b=…,s|t=…) only on pages with multiple tracked tables (issue details).tableSortwhen switching tree/hardware details tabs so inactive tables do not keep stale sort.Test plan
s/tableSortparam is omitteds|b,s|t)useSortingState.test.ts,search.test.tsRelated issues