-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: The case of the missing tabs #33066
Conversation
# Conflicts: # app/client/src/ce/sagas/JSActionSagas.ts
/build-deploy-preview skip-test=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8919201028. |
Deploy-Preview-URL: https://ce-33066.dp.appsmith.com |
WalkthroughWalkthroughThe recent modifications focus on enhancing IDE tab management by introducing a Changes
Assessment against linked issues
Possibly related issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 6
export const closeQueryActionTab = (payload: { | ||
id: string; | ||
parentId: string; | ||
}) => { |
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.
Ensure consistent parameter naming for parentId
.
- parentId: string;
+ parentPageId: string;
The parameter parentId
should be renamed to parentPageId
to clarify that it refers to the ID of a page, aligning with the changes in other parts of the system where parentId
is used to denote the page ID.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
export const closeQueryActionTab = (payload: { | |
id: string; | |
parentId: string; | |
}) => { | |
export const closeQueryActionTab = (payload: { | |
id: string; | |
parentPageId: string; | |
}) => { |
@@ -655,7 +656,7 @@ export function* deleteActionSaga( | |||
}); | |||
|
|||
yield put(deleteActionSuccess({ id })); | |||
yield put(closeQueryActionTabSuccess({ id })); | |||
yield put(closeQueryActionTabSuccess({ id, parentId: pageId })); |
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.
Ensure consistency in handling parentId
for action tabs.
- yield put(closeQueryActionTabSuccess({ id, parentId: pageId }));
+ yield put(closeQueryActionTabSuccess({ id, parentId }));
This change ensures that the parentId
passed to closeQueryActionTabSuccess
is consistent with the rest of the function's logic, where parentId
is directly used from the function's parameters.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
yield put(closeQueryActionTabSuccess({ id, parentId: pageId })); | |
yield put(closeQueryActionTabSuccess({ id, parentId })); |
export const closeQueryActionTabSuccess = (payload: { | ||
id: string; | ||
parentId: string; | ||
}) => { |
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.
Ensure consistent parameter naming for parentId
.
- parentId: string;
+ parentPageId: string;
The parameter parentId
should be renamed to parentPageId
in the closeQueryActionTabSuccess
function as well, to maintain consistency and clarity across the codebase.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
export const closeQueryActionTabSuccess = (payload: { | |
id: string; | |
parentId: string; | |
}) => { | |
export const closeQueryActionTabSuccess = (payload: { | |
id: string; | |
parentPageId: string; | |
}) => { |
yield call( | ||
closeActionTabSaga, | ||
closeQueryActionTab({ | ||
id: action.payload.id, | ||
parentId: action.payload.originalPageId, | ||
}), | ||
); |
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.
Refactor to use a dedicated saga for closing action tabs.
- yield call(
- closeActionTabSaga,
- closeQueryActionTab({
- id: action.payload.id,
- parentId: action.payload.originalPageId,
- }),
- );
+ yield put(closeQueryActionTab({
+ id: action.payload.id,
+ parentId: action.payload.originalPageId,
+ }));
This change simplifies the flow by dispatching the closeQueryActionTab
action directly, which will be handled by the saga watching for this action type. This avoids the need for an explicit call to closeActionTabSaga
and aligns with Redux-Saga best practices for handling side effects.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
yield call( | |
closeActionTabSaga, | |
closeQueryActionTab({ | |
id: action.payload.id, | |
parentId: action.payload.originalPageId, | |
}), | |
); | |
yield put(closeQueryActionTab({ | |
id: action.payload.id, | |
parentId: action.payload.originalPageId, | |
})); |
yield call( | ||
closeJSActionTabSaga, | ||
closeJSActionTab({ | ||
id: action.payload.id, | ||
parentId: actionObject.pageId, | ||
}), | ||
); |
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.
Ensure proper handling of JS action tab closure during JS collection move.
- closeJSActionTabSaga,
- closeJSActionTab({
- id: action.payload.id,
- parentId: actionObject.pageId,
- }),
+ closeJSActionTab({
+ id: action.payload.id,
+ parentId: action.payload.destinationPageId,
+ }),
The parentId
should be updated to destinationPageId
when closing the JS action tab, as the JS collection is being moved to a new page. This ensures that the tab is correctly associated with the new page.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
yield call( | |
closeJSActionTabSaga, | |
closeJSActionTab({ | |
id: action.payload.id, | |
parentId: actionObject.pageId, | |
}), | |
); | |
yield call( | |
closeJSActionTab({ | |
id: action.payload.id, | |
parentId: action.payload.destinationPageId, | |
}), | |
); |
@@ -326,7 +332,7 @@ | |||
}, | |||
}); | |||
yield put(deleteJSCollectionSuccess({ id })); | |||
yield put(closeJsActionTabSuccess({ id })); | |||
yield put(closeJsActionTabSuccess({ id, parentId: pageId })); |
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.
Ensure correct parentId
when closing JS action tab on JS collection deletion.
- yield put(closeJsActionTabSuccess({ id, parentId: pageId }));
+ yield put(closeJsActionTabSuccess({ id, parentId: getCurrentPageId() }));
When a JS collection is deleted, the JS action tab should be closed with the current page ID as parentId
, ensuring that the tab closure is associated with the correct page context.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
yield put(closeJsActionTabSuccess({ id, parentId: pageId })); | |
yield put(closeJsActionTabSuccess({ id, parentId: getCurrentPageId() })); |
Suggestion for major improvement: Renaming the PR to "fix: The curious case of missing tabs". |
Description
Fixes #33032
Change the data structure of the tabs to now keep account different tabs for each page instead of just one page at a time. Also removed tabs state for focus retention.
Fixes #33045
Handles removing and navigating post move of js and queries to avoid being in a bad state
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9000257489
Commit: b39c8d7
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?