fix(url): update scenes, remove json and line filter from service url#1977
fix(url): update scenes, remove json and line filter from service url#1977L2D2Grafana wants to merge 8 commits into
Conversation
Bundle Size ChangesHello! 👋 This comment was generated by a Github Action to help you and reviewers understand the impact of your PR on frontend bundle sizes. Great job! Bundle size increase of 0% is now below threshold of 5%. Whenever this PR is updated, this comment will update to reflect the latest changes. |
303bc18 to
09e16fd
Compare
…e scene
SceneAppPage caches the drilldown scene by pathname (ignoring the query
string), so navigating away from a service and back re-shows the cached
scene with its original variable state. Stale jsonFields/lineFormat values
left in that cached scene leaked into the query (`| json trueundefined""`,
`| line_format "{{.true}}"`), producing a malformed query (#1976).
resetVariablesIfNotInUrl now also covers jsonFields, lineFormat and
lineFilters, and treats an absent OR empty URL param as "no filters" (the
re-show URL has e.g. `var-jsonFields=` present-but-empty), so re-entry
re-syncs the stale cached variables to the URL. On a fresh URL load
AdHocFiltersVariable already drops the invalid filter on parse, so the two
paths together keep the query well-formed.
Adds an e2e test asserting that landing on a service URL with invalid
jsonFields/lineFormat values loads without a malformed-query error.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
09e16fd to
99df5bf
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a navigation/state bug in the Logs Drilldown plugin where drilldown-only URL params could persist on the service-selection route, leading to malformed LogQL interpolation (and a parse/JSON.parse error) on the first “Show logs” click.
Changes:
- Filter drilldown-only params out of service-selection URLs and proactively scrub them on
IndexSceneactivation when on the service-selection page. - Re-sync cached scene variables with the URL by clearing ad-hoc variables when the corresponding URL param is absent or empty.
- Update unit/E2E coverage and bump
@grafana/scenes/@grafana/scenes-reactto8.9.0(plus pnpm version bump inpackage.json).
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/invalidUrl.spec.ts | Adds E2E coverage to ensure landing on a URL with invalid jsonFields/lineFormat does not surface parse errors. |
| tests/exploreServices.spec.ts | Updates E2E assertions to match current service-selection behavior and reduces query-count flakiness. |
| tests/breakdowns/jsonView.spec.ts | Adjusts E2E assertions for shared log-line links to use DOM/ARIA signals rather than viewport geometry. |
| src/services/routing.ts | Adds excluded URL keys for service-selection URLs and applies filtering while building service routes. |
| src/services/routing.test.ts | Adds a unit test ensuring excluded keys are dropped when building the explore/service-selection URL. |
| src/Components/IndexScene/IndexScene.tsx | Scrubs excluded URL keys on activation for service-selection and clears cached ad-hoc variables when URL params are absent/empty. |
| package.json | Bumps @grafana/scenes/@grafana/scenes-react and updates packageManager pnpm version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!this.state.embedded && this.userInServiceSelection()) { | ||
| const searchParams = urlUtil.getUrlSearchParams(); | ||
| const hasExcludedServiceUrlKey = SERVICE_URL_EXCLUDED_KEYS.some((key) => | ||
| Object.prototype.hasOwnProperty.call(searchParams, key) |
There was a problem hiding this comment.
Why not
| Object.prototype.hasOwnProperty.call(searchParams, key) | |
| searchParams.hasOwnProperty(key) |
?
| if (cleanExploreUrl !== currentUrl) { | ||
| locationService.replace(cleanExploreUrl); | ||
| } | ||
| } |
There was a problem hiding this comment.
Additionally, why not using the current URL and search params, but removing SERVICE_URL_EXCLUDED_KEYS from the current search params?
That would mean, location.pathname + search params from line 290 but without the keys from SERVICE_URL_EXCLUDED_KEYS
There was a problem hiding this comment.
Actually my bad. If it's a search params instance, you check for existing keys using .get() or .has().
matyax
left a comment
There was a problem hiding this comment.
LGTM! Just a couple of questions.
Summary 📝
Fixes #1976 — clicking "Show logs" on the service-selection page failed on the first attempt but worked after a page refresh.
Drilldown-only URL params (
var-jsonFields/var-lineFormat) were being left in the URL on the service-selection page. On the first "Show logs" click they interpolated a malformeddetected_fieldsquery (| json trueundefined"",| line_format "{{.true}}"), surfacing aJSON.parse: unexpected end of data/ parse error until a hard refresh cleared the state.What changed 🔧
Core fix
SERVICE_URL_EXCLUDED_KEYS(var-jsonFields,var-lineFormat) and filter them out of the service URL inbuildServicesRoute—src/services/routing.ts.IndexSceneactivation (when not embedded and on the service-selection page), scrub any stuck excluded keys by replacing the location with a clean explore URL —src/Components/IndexScene/IndexScene.tsx.resetVariablesIfNotInUrlnow also clearslineFilters/jsonFields/lineFormat, and treats an empty param (e.g.var-jsonFields=) as "absent".SceneAppPagecaches scenes by pathname (not query string), so re-entering a cached scene could keep stale variable values while the URL says empty — this re-syncs them.Dependencies
@grafana/scenesand@grafana/scenes-reactfrom7.4.2→8.9.0.11.8.0→11.9.0.pnpm-lock.yaml(everyresolution:/engines:entry), which accounts for the very large lockfile diff — the only functional dependency change is the scenes bump.Tests 🧪
src/services/routing.test.ts: assertsbuildServicesUrl(ROUTES.explore())dropsvar-jsonFields/var-lineFormat.tests/invalidUrl.spec.ts: landing directly on a URL carrying invalidjsonFields/lineFormatnow loads the logs view without a parse error.skipUnlessLatestGrafana) and needed updating for current behavior:exploreServices.spec.ts› should add labels to favorites: returning to service selection now resets the search, so we assert the search is cleared rather than clicking a clear button that is no longer present.exploreServices.spec.ts› navigating back will re-run volume query: simplified to tracklogsVolumeCount(1 → 2 → 3), which is what the test title verifies. Dropped the flakylogsQueryCountupper-bound checks — that count races async panel rendering, and the panel-query count is already covered by the sibling refreshing time range test.jsonView.spec.ts› can share link to log line: assert the restored row'sdata-scrolled/aria-expandedattributes instead of atoBeInViewport()geometry check, which is unreliable for the tall expanded row in the short default viewport.Manual verification
detected_fieldsrequest (notrueundefined/ duplicate| json) and loads without theJSON.parseerror.var-jsonFields/var-lineFormatin the URL strips them (URL replaced with a clean explore URL) without breaking back/forward navigation.