-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix: unsaved changes in query editor #2026
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses unsaved changes in the query editor by updating the changeUserInput function signature and its usages across multiple components to accept an optional isDirty flag.
- Updated the changeUserInput interface in several components (QueriesHistory, SavedQueries, Query, QueryEditor, YqlEditor, TopQueries) to include an optional isDirty property.
- Modified calls to changeUserInput to explicitly set isDirty to false or true as needed.
- Updated the query reducer and related selectors to incorporate the isDirty state.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/containers/Tenant/Query/QueriesHistory/QueriesHistory.tsx | Updated changeUserInput interface and applyQueryClick to pass isDirty flag. |
src/containers/Tenant/Query/SavedQueries/SavedQueries.tsx | Modified changeUserInput interface and applyQueryClick to include isDirty flag. |
src/containers/Tenant/Diagnostics/TenantOverview/TenantCpu/TopQueries.tsx | Updated dispatch of changeUserInput to pass isDirty flag. |
src/containers/Tenant/Query/Query.tsx | Adjusted handleUserInputChange to accept isDirty flag. |
src/store/reducers/query/query.ts | Updated changeUserInput reducer and selectors to handle the new isDirty property. |
src/containers/Tenant/Diagnostics/TopQueries/TopQueries.tsx | Updated dispatch of changeUserInput to include isDirty flag. |
src/containers/Tenant/Query/QueryEditor/QueryEditor.tsx | Updated changeUserInput in props to include isDirty flag. |
src/containers/Tenant/Query/QueryEditor/YqlEditor.tsx | Updated keybinding and onChange calls to pass isDirty flag. |
src/store/reducers/query/types.ts | Updated QueryState interface to include isDirty property. |
src/utils/hooks/withConfirmation/useChangeInputWithConfirmation.tsx | Modified hook to include selectIsDirty and adjust behavior accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where unsaved changes in the query editor were not properly handled by adding a new isDirty flag and updating confirmation logic. Key changes include:
- Updating the confirmation hook to only trigger when there are unsaved changes.
- Dispatching setIsDirty updates across various query-related components.
- Adding the isDirty flag to the query state and its corresponding reducer and selectors.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/utils/hooks/withConfirmation/useChangeInputWithConfirmation.tsx | Modified to check both userInput and isDirty before triggering confirmation. |
src/containers/Tenant/Diagnostics/TenantCpu/TopQueries.tsx | Added dispatch(setIsDirty(false)) to reset dirty status on query selection. |
src/containers/Tenant/Query/QueryEditor/YqlEditor.tsx | Replaced snippet insertion logic with changeUserInput and setIsDirty update. |
src/containers/Tenant/Diagnostics/TopQueries/TopQueries.tsx | Reset the dirty flag upon user query selection. |
src/containers/Tenant/ObjectSummary/SchemaTree/SchemaTree.tsx | Updated confirmation trigger condition to include isDirty. |
src/containers/Tenant/Query/QueriesHistory/QueriesHistory.tsx | Reset the dirty flag upon selecting a query from history. |
src/containers/Tenant/Query/SavedQueries/SavedQueries.tsx | Reset the dirty flag when a saved query is applied. |
src/store/reducers/query/types.ts and src/store/reducers/query/query.ts | Introduced the isDirty state field with corresponding reducer and selectors. |
Comments suppressed due to low confidence (2)
src/containers/Tenant/Query/QueryEditor/YqlEditor.tsx:96
- The original snippet insertion logic using the snippetController2 has been removed and replaced by a changeUserInput call. Confirm that this behavior is intentional and that no functionality relying on snippet insertion will be affected.
changeUserInput({input});
src/store/reducers/query/query.ts:50
- [nitpick] Consider providing an explicit default value for the newly added isDirty property in the query state to ensure consistent behavior across the application.
const slice = createSlice({
You should also Probably it should be here (but it's better to check): https://github.com/ydb-platform/ydb-embedded-ui/blob/main/src/store/reducers/queryActions/queryActions.ts#L77 Or maybe in Save query dialog itself |
@artemmufazalov fixed |
@@ -173,6 +174,7 @@ export default function QueryEditor(props: QueryEditorProps) { | |||
if (!partial) { | |||
if (text !== historyQueries[historyCurrentIndex]?.queryText) { | |||
dispatch(saveQueryToHistory({queryText: text, queryId})); | |||
dispatch(setIsDirty(false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I run some query twice, for the second run it won't be saved in history again and confirmation will be shown. This row should be if(!partial)
block
Closes #1984
CI Results
Test Status: ✅ PASSED
📊 Full Report
Test Changes Summary ✨7 ⏭️1
✨ New Tests (7)
⏭️ Skipped Tests (1)
Bundle Size: ✅
Current: 83.22 MB | Main: 83.22 MB
Diff: +1.75 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information