fix: citation urls not working outside of target explore#8516
fix: citation urls not working outside of target explore#8516AdityaHegde merged 2 commits intomainfrom
Conversation
ericpgreen2
left a comment
There was a problem hiding this comment.
Observations
Type casting in parse-time-range-from-filters.ts
The new code returns objects cast to DashboardTimeControls that only have name populated:
return {
name: `${start.toISO()} to watermark`,
} as DashboardTimeControls;Since DashboardTimeControls extends TimeRange (which requires start and end), this could cause runtime errors if any intermediate code accesses those properties. The approach works because toTimeRangeParam checks for a non-CUSTOM name first and passes it through as the URL param, and the strings are valid Rill time expressions that the backend can resolve.
Have you verified that no code paths between here and URL serialization access .start or .end on these objects?
Relationship to structured citations (#8502)
This fix adds another layer of URL parsing and transformation that the structured citations proposal aims to eliminate. I'm fine merging this as a tactical fix for the immediate bug, but we should keep momentum on #8502 since it would clean up this complexity.
Developed in collaboration with Claude Code
|
I think this is a long standing issue. We always resolve start and end based on the name for non-custom ranges. We should update the type sometime in the future. Since we converting to a URL here we are fine not populating it. Honestly we dont need time range summary in url conversation as well, but that might be a bigger refactor. |
* fix: citation urls not working outside of target explore * Update tests
* fix: citation urls not working outside of target explore * Update tests
Citation url rewrite always used the active explore and metrics view to resolve the query. This will not work when the active explore is not the same.
Refactoring to instead use the ListResource query to get explore and metrics view. It also uses
gotofor any local links.We also need the time range summary. This PR tries to get it from cache, if not present it will route to
/-/open-query.Checklist: