Conversation
WalkthroughThis pull request refactors several modules by updating type definitions and function signatures to use generics, enhancing flexibility. It also improves robustness in the block execution hook with optional chaining. Additionally, it removes outdated tab management and redirection logic from the saga file and replaces it with new helper functions in dedicated utility files. Import statements have been updated to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Action
participant S as IDE Saga
participant R as Redirect Helper
U->>S: Remove entity (removedId)
S->>R: getNextEntityAfterRemove(removedId, tabs)
R-->>S: Return RedirectAction (NA, LIST, or ITEM)
S->>U: Redirect based on action
sequenceDiagram
participant U as User Action
participant S as Tab Manager
participant T as Tab Helper
U->>S: Create entity (newId)
S->>T: getUpdatedTabs(newId, currentTabs)
T-->>S: Return updated tab list
S->>U: Update UI with new tabs
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (7)
🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
IDE Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/src/IDE/utils/groupAndSortEntitySegmentList.ts(1 hunks)app/client/src/ce/PluginActionEditor/hooks/useBlockExecution.ts(1 hunks)app/client/src/sagas/IDESaga.tsx(2 hunks)app/client/src/sagas/getNextEntityAfterRemove.test.ts(1 hunks)app/client/src/sagas/helper.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/sagas/getNextEntityAfterRemove.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/sagas/helper.ts
[error] 330-336: This generator function doesn't contain yield.
(lint/correctness/useYield)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (8)
app/client/src/ce/PluginActionEditor/hooks/useBlockExecution.ts (1)
24-24: Good safeguard against undefined configuration
Using optional chaining effectively prevents a potential null reference error here.app/client/src/IDE/utils/groupAndSortEntitySegmentList.ts (4)
2-2: Efficient lodash usage
The import appears correct and consistent with established patterns.
4-4: Updated type definition is beneficial
DeclaringEditorSegmentListas generic enhances reusability across various entity types.
6-6: Good extension of the generic parameter
Allowingitemsto be typed asT[]solidifies type safety for future usage.
9-11: Generic function improves flexibility
ConvertinggroupAndSortEntitySegmentListto a generic function is a solid step toward making this utility more reusable.app/client/src/sagas/helper.ts (1)
338-401: Navigation logic for entity removal looks solid
Centralizing removal handling and redirect decisions in this helper clarifies responsibilities and simplifies maintenance.app/client/src/sagas/IDESaga.tsx (2)
2-2: Explicit import
Clear indication of usingFocusEntityfrom the same file, maintaining code clarity.
31-34: Refactored imports
Importing tab and redirect utilities from the helper module streamlines the saga and improves maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/IDE/utils/getUpdatedTabs.ts (1)
1-7: Add JSDoc comments and return type annotation.The function logic is correct, but it would benefit from JSDoc comments explaining its purpose and parameters. Also, explicitly define the return type for better maintainability.
-export function getUpdatedTabs(newId: string, currentTabs: string[]) { +/** + * Updates the tabs array to include the new tab ID if it doesn't already exist + * @param newId The ID of the tab to add + * @param currentTabs Current array of tab IDs + * @returns Updated array of tab IDs + */ +export function getUpdatedTabs(newId: string, currentTabs: string[]): string[] {app/client/src/IDE/utils/getNextEntityAfterRemove.ts (1)
38-66: Consider simplifying the redirection logic.The switch statement implementation works correctly, but could be simplified. Also, the TODO comment on lines 48-49 indicates potential design changes needed.
const indexOfTab = tabs.findIndex((item) => item.key === removedId); - switch (indexOfTab) { - case -1: - // If no other action is remaining, navigate to the creation url - return { - action: RedirectAction.LIST, - }; - case 0: - // if the removed item is first item, then if tabs present, tabs + 1 - // else otherItems[0] -> TODO: consider changing this logic after discussion with - // design team. May be new listing UI for side by side - if (tabs.length > 1) { - return { - action: RedirectAction.ITEM, - payload: tabs[1], - }; - } else { - return { - action: RedirectAction.LIST, - }; - } - default: - return { - action: RedirectAction.ITEM, - payload: tabs[indexOfTab - 1], - }; - } + // If tab not found or it's the last remaining tab, go to list view + if (indexOfTab === -1 || tabs.length <= 1) { + return { + action: RedirectAction.LIST, + }; + } + + // Choose next tab if available, otherwise previous tab + const nextTabIndex = indexOfTab === 0 ? 1 : indexOfTab - 1; + return { + action: RedirectAction.ITEM, + payload: tabs[nextTabIndex], + };Note: Resolve the TODO comment (lines 48-49) after discussion with the design team.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
app/client/src/IDE/utils/getNextEntityAfterRemove.ts(1 hunks)app/client/src/IDE/utils/getUpdatedTabs.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (2)
app/client/src/IDE/utils/getNextEntityAfterRemove.ts (2)
1-20: Well-structured enum and interface implementation with proper documentation.The implementation of the RedirectAction enum and RedirectActionDescription interface is clean and well-documented. Good use of generic typing for flexibility.
22-37: Strong implementation of redirection logic with proper type safety.The function correctly determines whether navigation is required based on the currently selected entity. Good use of generics with the EntityItem constraint.
2 participants
Description
Refactoring code by taking out the common logic for re-using it on other IDEs
Fixes #38904
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13692417401
Commit: caef1fa
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts
List of identified flaky tests.Thu, 06 Mar 2025 07:19:07 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor