Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ui] Global AMP: Fix timeline pagination #26353

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Conversation

hellendag
Copy link
Member

@hellendag hellendag commented Dec 9, 2024

Summary & Motivation

In the global AMP view, the tick timeline is broken when paginating to older ticks. The (paginated) table view sets the parent's time range, but the time range isn't being used as a signal to refetch the list of ticks used by the timeline. Fix this with an effect.

How I Tested These Changes

View AMP overview for Discord. Page backward in time, verify that the timeline updates with the correct ticks displayed.

Changelog

[ui] Fix global auto-materialize tick timeline when paginating.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@hellendag hellendag marked this pull request as ready for review December 9, 2024 18:09
@@ -72,6 +73,11 @@ export const GlobalAutomaterializationContent = () => {
[fetch, getVariables],
);

// When the variables have changed (e.g. due to pagination), refresh.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I think I was relying on useRefreshAtInterval doing this... I think it should but also okay with requiring callsites to do it themselves. i'll leave it up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I had noticed that the useRefreshAtInterval is disabled (below) when a time range is applied, so I understood it to mean that the query only refreshes on the first page. (Which I think makes sense, since viewing an older page means the data should be static.)

Copy link

github-actions bot commented Dec 9, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-qnrvwge7u-elementl.vercel.app
https://dish-amp-pagination.core-storybook.dagster-docs.io

Built with commit 1182f1c.
This pull request is being automatically deployed with vercel-action

Copy link
Member Author

hellendag commented Dec 9, 2024

Merge activity

  • Dec 9, 12:49 PM CST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 9, 12:50 PM CST: A user merged this pull request with Graphite.

@hellendag hellendag merged commit c64a0f8 into master Dec 9, 2024
2 checks passed
@hellendag hellendag deleted the dish/amp-pagination branch December 9, 2024 18:50
pskinnerthyme pushed a commit to pskinnerthyme/dagster that referenced this pull request Dec 16, 2024
## Summary & Motivation

In the global AMP view, the tick timeline is broken when paginating to older ticks. The (paginated) table view sets the parent's time range, but the time range isn't being used as a signal to refetch the list of ticks used by the timeline. Fix this with an effect.

## How I Tested These Changes

View AMP overview for Discord. Page backward in time, verify that the timeline updates with the correct ticks displayed.

## Changelog

[ui] Fix global auto-materialize tick timeline when paginating.
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.

2 participants