Skip to content

ui: nuqs for URL state management#6312

Open
manojVivek wants to merge 10 commits intomainfrom
nuqs
Open

ui: nuqs for URL state management#6312
manojVivek wants to merge 10 commits intomainfrom
nuqs

Conversation

@manojVivek
Copy link
Copy Markdown
Contributor

@manojVivek manojVivek commented Apr 7, 2026

No description provided.

@manojVivek manojVivek requested a review from a team as a code owner April 7, 2026 10:21
@alwaysmeticulous
Copy link
Copy Markdown

alwaysmeticulous bot commented Apr 7, 2026 •
edited
Loading

🔴 Meticulous spotted visual differences in 2 of 256 screens tested: view and approve differences detected.

Meticulous evaluated ~5 hours of user flows against your PR.

Last updated for commit d804240 Dashboard items into a hook with default value from ParcaContext. This comment will update as new commits are pushed.

@pre-commit-ci-lite pre-commit-ci-lite bot requested review from a team as code owners April 7, 2026 10:31
yomete
yomete approved these changes Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@yomete yomete left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm just one comment about color preference.

yomete
yomete reviewed Apr 7, 2026
View reviewed changes
ui/packages/shared/profile/src/ProfileView/hooks/useVisualizationState.ts Outdated
[setRawCurPathArrow]
);
const [colorStackLegend] = useQueryState('color_stack_legend', stringParam);
const [colorBy, setStoreColorBy] = useQueryState('color_by', colorByParser);
Copy link
Copy Markdown
Contributor

@yomete yomete Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like colorByParser here hardcodes a default of 'binary', but the old code used the stored user preference as the default.

This means users who change their “Color by” setting will have it reset on every fresh page load and i think Meticulous maybe caught this change too.

Copy link
Copy Markdown
Contributor Author

@manojVivek manojVivek Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I made binary the default, didn't realise it is part of the user preferences. Thanks for catching this! 🙏🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yomete yomete yomete approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@manojVivek @yomete