Skip to content

ui: move latestQuery, latestFormattedQuery from redux to local state#88510

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
xinhaoz:remove-redux-latestquery
Sep 27, 2022
Merged

ui: move latestQuery, latestFormattedQuery from redux to local state#88510
craig[bot] merged 1 commit intocockroachdb:masterfrom
xinhaoz:remove-redux-latestquery

Conversation

@xinhaoz
Copy link
Copy Markdown
Contributor

@xinhaoz xinhaoz commented Sep 22, 2022

Previously, the fields latestQuery and latestFormattedQuery representing the latest non-empty query string for a statement viewed from the detaisl page was being stored in redux. The purpose of these fields was to preserve the query when changing tabs in the stmt details page. Saving this in the redux store was unnecessary and so this commit moves these fields to the stmt details local state.

Release note: None

https://www.loom.com/share/a28d412fb83a429391210935982404de

@xinhaoz xinhaoz requested a review from a team September 22, 2022 20:42
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

The idea was not only to save between tab switch, but also if you select a period where the data is not available, we want to at least show the query. With your change, doesn't the state change and the query gets cleanup? Can you test this scenario (go to the details with a period that has info, then select a new period where there is no data on it)?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@xinhaoz
Copy link
Copy Markdown
Contributor Author

xinhaoz commented Sep 23, 2022

Yes, that behaviour is still preserved here, we're doing the exact same thing but in the component state instead of redux since this info isn't needed anywhere else in the app. The state is only updated if we receive a different, non-empty query. Included a loom!

Copy link
Copy Markdown
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

You need to cleanup pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.sagas.spec.ts:670:11 to remove the references for latestQuery you remove from SQLDetailsStatsReducerState

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

Previously, the fields `latestQuery` and `latestFormattedQuery`
representing the latest non-empty query string for a statement viewed
from the detaisl page was being stored in redux. The purpose of these
fields was to preserve the query when changing tabs in the stmt
details page. Saving this in the redux store was unnecessary and so
this commit moves these fields to the stmt details local state.

Release note: None
@xinhaoz xinhaoz force-pushed the remove-redux-latestquery branch from a29af4a to 27299e0 Compare September 26, 2022 14:38
@xinhaoz xinhaoz requested a review from a team September 26, 2022 17:10
Copy link
Copy Markdown
Contributor Author

@xinhaoz xinhaoz left a comment

Choose a reason for hiding this comment

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

Done

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

Copy link
Copy Markdown
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 7 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)

@xinhaoz
Copy link
Copy Markdown
Contributor Author

xinhaoz commented Sep 27, 2022

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Sep 27, 2022

Build succeeded:

@craig craig bot merged commit 03b8afb into cockroachdb:master Sep 27, 2022
@xinhaoz xinhaoz deleted the remove-redux-latestquery branch September 27, 2022 18:56
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.

3 participants