Skip to content

Conversation

@AdityaHegde
Copy link
Collaborator

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 goto for 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:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!
Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@AdityaHegde
Copy link
Collaborator Author

AdityaHegde commented Dec 17, 2025

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.

@AdityaHegde AdityaHegde merged commit 658c3c8 into main Dec 17, 2025
12 checks passed
@AdityaHegde AdityaHegde deleted the fix/citation-urls-not-working-outside-explore branch December 17, 2025 04:20
AdityaHegde added a commit that referenced this pull request Dec 18, 2025
* fix: citation urls not working outside of target explore

* Update tests
k-anshul pushed a commit that referenced this pull request Dec 18, 2025
* fix: citation urls not working outside of target explore

* Update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants