fix(dashboards): handle kea-router auto-parsed query_variables in URL params#53828
Open
lawrence3699 wants to merge 1 commit intoPostHog:masterfrom
Open
fix(dashboards): handle kea-router auto-parsed query_variables in URL params#53828lawrence3699 wants to merge 1 commit intoPostHog:masterfrom
lawrence3699 wants to merge 1 commit intoPostHog:masterfrom
Conversation
… params kea-router's parseValue() auto-parses JSON-like URL parameter values into objects when there is no trailing whitespace. parseURLVariables() and parseURLFilters() unconditionally called JSON.parse() on the value, which fails silently when the value is already an object — causing dashboard query_variables to be ignored on initial page load. The fix checks typeof before parsing: if the value is already an object (auto-parsed by kea-router), use it directly. Closes PostHog#53788
Contributor
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+8
to
+35
| describe('parseURLVariables', () => { | ||
| it('parses a JSON string value from search params', () => { | ||
| const searchParams = { | ||
| [SEARCH_PARAM_QUERY_VARIABLES_KEY]: '{"card_name":"Polukranos, Unchained"}', | ||
| } | ||
| const result = parseURLVariables(searchParams) | ||
| expect(result).toEqual({ card_name: 'Polukranos, Unchained' }) | ||
| }) | ||
|
|
||
| it('handles an already-parsed object from search params (kea-router auto-parse)', () => { | ||
| // When the URL has no trailing %20, kea-router auto-parses JSON values into objects | ||
| const searchParams = { | ||
| [SEARCH_PARAM_QUERY_VARIABLES_KEY]: { card_name: 'Polukranos, Unchained' }, | ||
| } | ||
| const result = parseURLVariables(searchParams) | ||
| expect(result).toEqual({ card_name: 'Polukranos, Unchained' }) | ||
| }) | ||
|
|
||
| it('returns empty object when key is missing', () => { | ||
| expect(parseURLVariables({})).toEqual({}) | ||
| }) | ||
|
|
||
| it('returns empty object for invalid JSON string', () => { | ||
| const searchParams = { | ||
| [SEARCH_PARAM_QUERY_VARIABLES_KEY]: 'not-json', | ||
| } | ||
| expect(parseURLVariables(searchParams)).toEqual({}) | ||
| }) |
Contributor
There was a problem hiding this comment.
The two happy-path tests in each describe block (string input and already-parsed object input) exercise the same function with different input types and can be collapsed into a single it.each. The custom instructions ask for parameterised tests, and this pattern is repeated for both parseURLVariables and parseURLFilters.
Suggested change
| describe('parseURLVariables', () => { | |
| it('parses a JSON string value from search params', () => { | |
| const searchParams = { | |
| [SEARCH_PARAM_QUERY_VARIABLES_KEY]: '{"card_name":"Polukranos, Unchained"}', | |
| } | |
| const result = parseURLVariables(searchParams) | |
| expect(result).toEqual({ card_name: 'Polukranos, Unchained' }) | |
| }) | |
| it('handles an already-parsed object from search params (kea-router auto-parse)', () => { | |
| // When the URL has no trailing %20, kea-router auto-parses JSON values into objects | |
| const searchParams = { | |
| [SEARCH_PARAM_QUERY_VARIABLES_KEY]: { card_name: 'Polukranos, Unchained' }, | |
| } | |
| const result = parseURLVariables(searchParams) | |
| expect(result).toEqual({ card_name: 'Polukranos, Unchained' }) | |
| }) | |
| it('returns empty object when key is missing', () => { | |
| expect(parseURLVariables({})).toEqual({}) | |
| }) | |
| it('returns empty object for invalid JSON string', () => { | |
| const searchParams = { | |
| [SEARCH_PARAM_QUERY_VARIABLES_KEY]: 'not-json', | |
| } | |
| expect(parseURLVariables(searchParams)).toEqual({}) | |
| }) | |
| describe('parseURLVariables', () => { | |
| it.each([ | |
| ['a JSON string value', '{"card_name":"Polukranos, Unchained"}', { card_name: 'Polukranos, Unchained' }], | |
| [ | |
| 'an already-parsed object (kea-router auto-parse)', | |
| { card_name: 'Polukranos, Unchained' }, | |
| { card_name: 'Polukranos, Unchained' }, | |
| ], | |
| ])('parses %s from search params', (_, input, expected) => { | |
| const result = parseURLVariables({ [SEARCH_PARAM_QUERY_VARIABLES_KEY]: input }) | |
| expect(result).toEqual(expected) | |
| }) | |
| it('returns empty object when key is missing', () => { | |
| expect(parseURLVariables({})).toEqual({}) | |
| }) | |
| it('returns empty object for invalid JSON string', () => { | |
| const searchParams = { | |
| [SEARCH_PARAM_QUERY_VARIABLES_KEY]: 'not-json', | |
| } | |
| expect(parseURLVariables(searchParams)).toEqual({}) | |
| }) | |
| }) |
The same pattern applies to the parseURLFilters describe block.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/dashboard/dashboardUtils.test.ts
Line: 8-35
Comment:
**Prefer parameterised tests for the string-vs-object cases**
The two happy-path tests in each `describe` block (string input and already-parsed object input) exercise the same function with different input types and can be collapsed into a single `it.each`. The custom instructions ask for parameterised tests, and this pattern is repeated for both `parseURLVariables` and `parseURLFilters`.
```suggestion
describe('parseURLVariables', () => {
it.each([
['a JSON string value', '{"card_name":"Polukranos, Unchained"}', { card_name: 'Polukranos, Unchained' }],
[
'an already-parsed object (kea-router auto-parse)',
{ card_name: 'Polukranos, Unchained' },
{ card_name: 'Polukranos, Unchained' },
],
])('parses %s from search params', (_, input, expected) => {
const result = parseURLVariables({ [SEARCH_PARAM_QUERY_VARIABLES_KEY]: input })
expect(result).toEqual(expected)
})
it('returns empty object when key is missing', () => {
expect(parseURLVariables({})).toEqual({})
})
it('returns empty object for invalid JSON string', () => {
const searchParams = {
[SEARCH_PARAM_QUERY_VARIABLES_KEY]: 'not-json',
}
expect(parseURLVariables(searchParams)).toEqual({})
})
})
```
The same pattern applies to the `parseURLFilters` describe block.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+38
to
+58
| describe('parseURLFilters', () => { | ||
| it('parses a JSON string value from search params', () => { | ||
| const searchParams = { | ||
| [SEARCH_PARAM_FILTERS_KEY]: '{"date_from":"-7d"}', | ||
| } | ||
| const result = parseURLFilters(searchParams) | ||
| expect(result).toEqual({ date_from: '-7d' }) | ||
| }) | ||
|
|
||
| it('handles an already-parsed object from search params (kea-router auto-parse)', () => { | ||
| const searchParams = { | ||
| [SEARCH_PARAM_FILTERS_KEY]: { date_from: '-7d', date_to: 'now' }, | ||
| } | ||
| const result = parseURLFilters(searchParams) | ||
| expect(result).toEqual({ date_from: '-7d', date_to: 'now' }) | ||
| }) | ||
|
|
||
| it('returns empty object when key is missing', () => { | ||
| expect(parseURLFilters({})).toEqual({}) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
parseURLFilters
parseURLVariables includes an invalid-JSON test (line 31–35) to confirm the error path returns {}, but parseURLFilters does not. Since both functions share the same try/catch structure, coverage should be symmetric. Adding this case ensures the catch branch for parseURLFilters stays tested as the code evolves.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/dashboard/dashboardUtils.test.ts
Line: 38-58
Comment:
**Missing invalid JSON test for `parseURLFilters`**
`parseURLVariables` includes an invalid-JSON test (line 31–35) to confirm the error path returns `{}`, but `parseURLFilters` does not. Since both functions share the same `try/catch` structure, coverage should be symmetric. Adding this case ensures the catch branch for `parseURLFilters` stays tested as the code evolves.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes dashboard URL parsing so query_variables (and query_filters) work whether kea-router provides the param value as a JSON string or an already-parsed object (avoiding silent drops when the trailing %20 is missing).
Changes:
- Guard
parseURLVariablesandparseURLFiltersto onlyJSON.parsewhen the raw value is a string. - Add unit tests covering both string and already-parsed object inputs for variables/filters parsing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/src/scenes/dashboard/dashboardUtils.ts | Makes URL param parsing resilient to kea-router auto-parsing of JSON-like values. |
| frontend/src/scenes/dashboard/dashboardUtils.test.ts | Adds regression tests for both string and object search param inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+30
to
+35
| it('returns empty object for invalid JSON string', () => { | ||
| const searchParams = { | ||
| [SEARCH_PARAM_QUERY_VARIABLES_KEY]: 'not-json', | ||
| } | ||
| expect(parseURLVariables(searchParams)).toEqual({}) | ||
| }) |
There was a problem hiding this comment.
The "invalid JSON" test will trigger console.error inside parseURLVariables (since the function logs on parse failures). To keep test output clean and consistent with other tests in the repo, consider stubbing/spying on console.error in this test (optionally asserting the call) and restoring it afterward.
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
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.
Problem
Dashboard
query_variablesURL parameter is silently ignored when manually constructing a URL without a trailing%20.Works:
?query_variables={"card_name":"Polukranos, Unchained"}%20Fails:
?query_variables={"card_name":"Polukranos, Unchained"}Root cause
kea-router'sparseValue()has two branches for JSON-like strings:{...}(no trailing space) → auto-parses viaJSON.parse()→ returns an object{...}(trailing space) → strips the space → returns a stringparseURLVariables()unconditionally callsJSON.parse()on the search param value. When kea-router already parsed it into an object,JSON.parse(someObject)stringifies it to"[object Object]"and throws aSyntaxError, which the catch block swallows — silently dropping all query variables.The trailing
%20workaround prevents kea-router from auto-parsing the JSON, so the value stays as a string andJSON.parse()succeeds.Fix
Check
typeof raw === 'string'before callingJSON.parse(). If the value is already an object (auto-parsed by kea-router), use it directly. Applied the same fix toparseURLFilters()which has the same latent bug.Changes
frontend/src/scenes/dashboard/dashboardUtils.ts— guardparseURLVariablesandparseURLFiltersagainst pre-parsed object valuesfrontend/src/scenes/dashboard/dashboardUtils.test.ts— new unit tests covering both string and object inputsHow I verified
dashboardLogic.test.ts,dashboardQuickFiltersDebounce.test.ts, etc.)Closes #53788