ref(settings): Use nuqs to manage project key query state #102082
Conversation
github-actions
bot
added
the
Scope: Frontend
| to="" | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| setShowDeprecatedDsn(showDeprecatedDsn ? null : true); |
There was a problem hiding this comment.
Not sure if this is the best pattern, let me know if there's a better alternative!
There was a problem hiding this comment.
I would keep the Link as it was and just not use the setter. The setter can be used instead of manually using navigate, but for links, this is fine:
<Link
to={{
query: {
...location.query,
showDeprecated: showDeprecatedDsn ? undefined : 'true',
},
}}
>
Note that you don’t need to spread ...location into to, but you need to spread ...location.query. Long term I’d want to have a better pattern for this.
There was a problem hiding this comment.
Hmm CI seems to be failing because of re-rendering issues.
Cannot update a component (`ProjectKeyCredentials`) while rendering a different component (`NuqsTestingAdapter`). To locate the bad setState() call inside `NuqsTestingAdapter`, follow the stack trace as described in https://react.dev/link/setstate-in-render
I can't reproduce this locally though, so I'm not sure what the issue could be.
There was a problem hiding this comment.
It's an issue with our testing adapter. I think @scttcper has a PR up to fix it though, maybe we should just merge that one?
There was a problem hiding this comment.
sure - I'll wait to merge this PR until that is fixed then.
There was a problem hiding this comment.
huh I tried the test locally and it works fine for me on your PR 🤔
There was a problem hiding this comment.
Should be fixed on master, please update 🙏
There was a problem hiding this comment.
thanks for the reminder @TkDodo! Will update and merge
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
| defaultValue: availableTabs[0]?.key ?? 'otlp', | ||
| }), | ||
| [availableTabs] | ||
| ); |
There was a problem hiding this comment.
Bug: Inconsistent Default Value Breaks Parser Behavior
The tabParser's defaultValue is 'otlp' when availableTabs is empty, but parseAsStringLiteral is configured with no valid keys. This inconsistency means the default value isn't accepted by the parser, potentially causing unexpected useQueryState behavior.
|
@AbhiPrasad tests are ✅ now, I think it’s good to merge |
A follow up to #102010 (comment) that converts the query param logic to use `nuqs`. Much nicer! --------- Co-authored-by: Dominik Dorfmeister <dominik.dorfmeister@sentry.io>
A follow up to #102010 (comment) that converts the query param logic to use `nuqs`. Much nicer! --------- Co-authored-by: Dominik Dorfmeister <dominik.dorfmeister@sentry.io>
A follow up to #102010 (comment) that converts the query param logic to use
nuqs. Much nicer!