Skip to content

Conversation

@briangregoryholmes
Copy link
Contributor

@briangregoryholmes briangregoryholmes commented Nov 6, 2025

  • Prevents queries from being fired with an aggregation granularity that is incompatible with the smallest_time_grain
  • Displays error message in aggregation selector
  • Adds derivation of grain for additional RillTime subclasses
  • Adds getAggregationGrain function
  • Fixes an issue where using new Rill Time syntax in YAML defaults would cause the grain to be parsed incorrectly
Screenshot 2025-11-12 at 12 34 01 PM

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!

@briangregoryholmes briangregoryholmes self-assigned this Nov 6, 2025
@briangregoryholmes briangregoryholmes changed the title fix: rill time followup fix: enforce smallest_time_grain Nov 6, 2025
@briangregoryholmes briangregoryholmes marked this pull request as ready for review November 12, 2025 16:10
Copy link
Collaborator

@AdityaHegde AdityaHegde left a comment

Choose a reason for hiding this comment

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

This adds a weird state where the data displayed is not for the grain present in the url. Is there a hard requirement to show the warning? If not, would be great if we just correct it in the url as well.

@briangregoryholmes
Copy link
Contributor Author

briangregoryholmes commented Nov 13, 2025

This is intentional on my part, but it's not a hard requirement, so I'm happy to consider what others think about it during the applications sync.

The thinking is that I want to avoid ever changing the URL without user input. We are not there yet on Explore, but I think this is a step in the right direction.

I want the URL to be a reliable indicator of what the user has asked for and I think changing it only leads to hiding problems and making things difficult to debug. I want to follow the principle of what you see is what you get it. Sometimes, what you ask for is not what you're allowed to see, but at least we still follow the principle by acknowledging that it's being asked for and displaying a non-blocking error.

You can imagine a situation where a user has manually changed the URL to a grain value that is too small. I think this new UX state is more transparent about what is happening and does a better job of showing that we're guarding against this kind of stuff. Also, if you use a bookmark that has an out of date value, this would make it clear that the bookmark should be updated.

@briangregoryholmes briangregoryholmes changed the title fix: enforce smallest_time_grain [APP-421] fix: enforce smallest_time_grain Dec 10, 2025
Copy link
Collaborator

@AdityaHegde AdityaHegde left a comment

Choose a reason for hiding this comment

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

Looks good from code POV. @ericokuma @nishantmonu51 please approve if we are ok with the UX

@briangregoryholmes briangregoryholmes merged commit cab45e1 into main Jan 7, 2026
10 checks passed
@briangregoryholmes briangregoryholmes deleted the bgh/rill-time-followup branch January 7, 2026 17:32
briangregoryholmes added a commit that referenced this pull request Jan 8, 2026
* wip

* remove logs

* cleanup

* test fixes

* remove logs

* quality check

* test fix

* remove logs

* test fixes

* tooltip adjustment

* test fixes

* rilltime cleanup

* test updates

* test fix

* test fix

* test fix

* feedback

* comment

* ux feedback

* use DEFAULT_TIME_RANGES map

* type fix

* test fix

* test fix

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants