-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
chore: Adding create permission check for the plus button on tabs list #39672
base: release
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request updates the logic for rendering the "Add" tab in the IDE by refining permission checks and modifying the control flow. It also reorganizes focus-related type imports and declarations, moving them to a centralized module. New files and exports are introduced to facilitate this modularization, while unused exports are removed. Additionally, several saga functions have been updated to incorporate the current Git branch when handling focus history, and a minor DOM class removal and a storybook property addition have been made for styling and UI enhancements. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🪧 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
|
…o chore/add-create-permission-check
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts (1)
1-731
: 🛠️ Refactor suggestionReplace Sleep and Wait calls with proper Cypress waiting mechanisms.
The test uses numerous instances of
agHelper.Sleep()
andcy.wait()
which violate the coding guidelines and can lead to flaky tests. These fixed time delays should be replaced with Cypress's built-in waiting mechanisms.Example patterns to use instead:
- agHelper.Sleep(2000); + cy.get('.selector').should('be.visible');- cy.wait('@postExecute'); + cy.wait('@postExecute').its('response.statusCode').should('eq', 200);
🧹 Nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java (1)
280-293
: Good implementation of metadata setting.The method correctly retrieves and sets organization and instance IDs in the metadata map.
Consider adding error handling for cases where either Mono might fail, as this could affect the datasource storage flow.
private Mono<DatasourceStorage> setAdditionalMetadataInDatasourceStorage(DatasourceStorage datasourceStorage) { Mono<String> organizationIdMono = organizationService.getCurrentUserOrganizationId(); Mono<String> instanceIdMono = configService.getInstanceId(); Map<String, Object> metadata = new HashMap<>(); - return organizationIdMono.zipWith(instanceIdMono).map(tuple -> { + return organizationIdMono.zipWith(instanceIdMono) + .onErrorResume(error -> { + log.error("Error retrieving metadata for datasource storage: {}", error.getMessage()); + return Mono.just(Tuples.of("", "")); + }) + .map(tuple -> { // Change this to ORGANIZATION_ID once we have the organizationId field in the datasource storage metadata.put(ORGANIZATION_ID, tuple.getT1()); metadata.put(INSTANCE_ID, tuple.getT2()); datasourceStorage.setMetadata(metadata); return datasourceStorage; - }); + }); }app/client/src/ce/navigation/FocusStrategy/types.ts (3)
22-24
: Address eslint-disabled type issues nowConsider fixing the
any
types now rather than postponing. Using more specific types would improve type safety and make future maintenance easier.- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ) => Generator<any, Array<FocusPath>, any>; + ) => Generator<unknown, Array<FocusPath>, unknown>;
29-31
: Address eslint-disabled type issues nowSame issue here - consider addressing these type issues now rather than later.
- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ) => Generator<any, Array<FocusPath>, any>; + ) => Generator<unknown, Array<FocusPath>, unknown>;
41-43
: Address eslint-disabled type issues nowAnd here as well - consider addressing these type issues now rather than later.
- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ) => Generator<any, void, any>; + ) => Generator<unknown, void, unknown>;
🛑 Comments failed to post (1)
app/client/src/ce/sagas/moduleInterfaceSagas.ts (1)
17-21:
⚠️ Potential issueGenerator function isn't using yield
The function is declared as a generator with the
function*
syntax, but it doesn't contain anyyield
statements. If this function doesn't need to yield control during execution, consider using a regular function instead.-export function* handleModuleWidgetCreationSaga( - props: HandleModuleWidgetCreationSagaPayload, -) { - return props.widgets; -} +export function handleModuleWidgetCreationSaga( + props: HandleModuleWidgetCreationSagaPayload, +) { + return props.widgets; +}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.export function handleModuleWidgetCreationSaga( props: HandleModuleWidgetCreationSagaPayload, ) { return props.widgets; }
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-21: This generator function doesn't contain yield.
(lint/correctness/useYield)
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.
PR has a lot of miscellaneous changes. Can we separate it out?
@@ -139,7 +103,11 @@ class FocusRetention { | |||
parentElement, | |||
); | |||
|
|||
removeKeys.push(`${parentPath}#${branch}`); | |||
if (focusEntityInfo.params.basePageId) { |
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.
This adds a implicit configuration which does something separately for AppIDE vs other IDEs. Since other IDEs could also have git, we should find a better way to identify this.
I this the focusStategy
should define this instead of us doing this in this Saga
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: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts
(3 hunks)app/client/src/ce/navigation/FocusStrategy/NoIDEFocusStrategy.ts
(2 hunks)app/client/src/ce/navigation/FocusStrategy/types.ts
(1 hunks)app/client/src/ce/sagas/DatasourcesSagas.ts
(3 hunks)app/client/src/ce/sagas/JSActionSagas.ts
(3 hunks)app/client/src/sagas/ActionSagas.ts
(4 hunks)app/client/src/sagas/FocusRetentionSaga.ts
(2 hunks)app/client/src/sagas/WidgetDeletionSagas.ts
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/sagas/FocusRetentionSaga.ts
- app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts
- app/client/src/ce/navigation/FocusStrategy/types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/ce/navigation/FocusStrategy/NoIDEFocusStrategy.ts
[error] 11-13: This generator function doesn't contain yield.
(lint/correctness/useYield)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / ci-test-result
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (12)
app/client/src/ce/navigation/FocusStrategy/NoIDEFocusStrategy.ts (1)
1-1
: Update to import source for FocusStrategy type looks good.The import statement has been updated to use the centralized type definition from the EE module, which aligns with the architectural changes mentioned in the PR objectives.
app/client/src/ce/sagas/DatasourcesSagas.ts (3)
187-187
: Import added for Git branch selectionThe import for
selectGitApplicationCurrentBranch
fromselectors/gitModSelectors
supports the focus history enhancement.
445-447
: Branch selection for focus history trackingGood addition of branch context for focus history management.
457-460
: Enhanced URL with branch informationThe URL now includes the branch information as a fragment, improving focus history tracking across branches.
app/client/src/sagas/WidgetDeletionSagas.ts (3)
61-61
: Import added for Git branch selectionThe import for Git branch selector is consistent with other sagas.
342-349
: Branch-aware focus history in widget deletionProperly integrates branch context into focus history management for widget deletion.
495-503
: Branch context added to bulk widget deletionCorrectly handles branch context for focus history in the bulk deletion saga.
app/client/src/ce/sagas/JSActionSagas.ts (2)
80-80
: Import added for Git branch selectionThe import for Git branch selector maintains consistency with other sagas.
337-349
: Branch context added for JS collection deletionGood integration of branch information in focus history for JS collection deletion.
app/client/src/sagas/ActionSagas.ts (3)
147-147
: Import added for Git branch selectionConsistent with other sagas, the selector import enhances focus history functionality.
581-633
: Branch context added to action deletionProper integration of branch information in focus history for action deletion.
1269-1276
: Branch context added to action tab closureThe implementation correctly includes branch information for focus history tracking.
…o chore/add-create-permission-check
Description
getEntityParentUrl
and updatinghandleRemoveFocusHistory
in focus retention sagaFixes #39673
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13806079072
Commit: f26e5f4
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
Wed, 12 Mar 2025 09:14:58 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Style