ref(dashboards): Migrate useWidgetBuilderState to nuqs#115345
Conversation
…e to nuqs Replace 14 useQueryParamState calls in useWidgetBuilderState with a single useQueryStates call from nuqs, removing the dependency on UrlParamBatchProvider. Update all widget builder test files to match the nuqs navigate format.
github-actions
Bot
added
the
Scope: Frontend
📊 Type Coverage Diff
🔍 2 new type safety issues introducedType assertions (
This is informational only and does not block the PR. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const parseAsColumns = createParser<Column[]>({ | ||
| parse(value) { | ||
| return deserializeFields(value.split(',')); | ||
| }, | ||
| serialize(columns) { | ||
| return serializeFields(columns).join(','); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Bug: The parse functions in parseAsColumns and parseAsLinkedDashboards incorrectly split URL parameters by commas, breaking the JSON structure of aliased fields and linked dashboards, leading to corrupted or lost state.
Severity: HIGH
Suggested Fix
Replace the naive value.split(',') logic with a more robust serialization and deserialization method. For example, serialize the entire array of items into a single JSON string with JSON.stringify() and deserialize it with JSON.parse(). This avoids issues with delimiters that might appear within the serialized items themselves.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx#L307-L314
Potential issue: The `parseAsColumns` and `parseAsLinkedDashboards` functions serialize
arrays of JSON objects by joining them with commas. However, the deserialization logic
splits the resulting string by commas. Since the JSON objects for aliased fields (e.g.,
`{"field":"count()","alias":"my_alias"}`) and linked dashboards contain internal commas,
the split fragments the JSON. This causes `JSON.parse` to fail. For aliased fields, this
results in corrupted column state. For linked dashboards, the entries are dropped
entirely, causing them to disappear when loading a widget's state from a URL.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 575fa58. Configure here.
| const selectedAggregate = merged.selectedAggregate ?? undefined; | ||
| const thresholds = merged.thresholds ?? undefined; | ||
| const linkedDashboards = merged.linkedDashboards ?? undefined; | ||
| const axisRange = merged.axisRange ?? undefined; |
There was a problem hiding this comment.
RELEASE sort default not applied after nuqs migration
Medium Severity
The condition sort?.length === 0 on line 785 no longer triggers because sort is now undefined instead of [] when absent. With nuqs, setSort([]) writes null to the URL, and the derived sort state returns undefined (line 534-536). Since undefined?.length === 0 evaluates to false, the default RELEASE sort is never applied when switching from a table with non-sortable fields to a timeseries chart. The test was updated to expect the broken behavior instead of fixing the condition. The fix is to check (!sort || sort.length === 0).
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 575fa58. Configure here.
| serialize(values) { | ||
| return values.join(','); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Comma-based array serialization corrupts multi-argument function fields
High Severity
parseAsColumns and parseAsStringList use comma as the array separator via value.split(',') and .join(','). This breaks any field containing commas in its arguments, such as count_if(transaction.duration,equals,value), count_web_vitals(measurements.lcp, good), trace metric functions like sum(value,my.metric,counter,-), JSON-aliased fields, and linked dashboards. On page reload or URL sharing, the split produces garbage entries. The tests were modified to avoid comma-containing fields rather than fixing the parser.
Reviewed by Cursor Bugbot for commit 575fa58. Configure here.
|
Be extremely careful with this, please! The last time I did this it tripled the number of re-renders of the Widget Builder per every field update, which caused horrible performance problems. Do the React Profiler traces look okay with this change? If you're feeling brave we can merge and check the metrics of the duration of opening the builder, but I'd rather be sure ahead of time |
2 participants
Summary
useQueryParamStatecalls inuseWidgetBuilderStatewith a singleuseQueryStatescall from nuqsUrlParamBatchProviderwrapper fromWidgetBuilderProvider(nuqs handles throttled URL writes natively withthrottleMs: 300)localOverridesstate pattern to support theupdateUrl: falseoption (local state buffer without URL sync)Test plan
pnpm test-ci static/app/views/dashboards/widgetBuilder/)