chore: Adding create permission check for the plus button on tabs list#39672
chore: Adding create permission check for the plus button on tabs list#39672ankitakinger wants to merge 17 commits intoreleasefrom
Conversation
WalkthroughThis pull request refactors the add tab functionality in the EditorTabs component by enhancing its conditional logic with an additional permission check. It updates several import paths to use centralized focus strategy types, removes obsolete re-exports from FocusSetters, and introduces new generator methods to include branch information in URL keys. New files have been added to re-export focus setter functionalities and to define FocusStrategy types, while minor UI adjustments (such as a CSS class removal) and a simplification in the datasource saga have also been implemented. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Event
participant FS as Focus Strategy (AppIDEFocusStrategy)
participant GC as Git Selector
U->>FS: Invoke getUrlKey(url)
FS->>GC: Retrieve current branch
GC-->>FS: Return branch
FS-->>U: Return url + '#' + branch
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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
the
skip-changelog
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.
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
anytypes 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 anyyieldstatements. 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)
hetunandu
left a comment
hetunandu
left a comment
There was a problem hiding this comment.
PR has a lot of miscellaneous changes. Can we separate it out?
| ); | ||
|
|
||
| removeKeys.push(`${parentPath}#${branch}`); | ||
| if (focusEntityInfo.params.basePageId) { |
There was a problem hiding this comment.
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
ankitakinger
added
ok-to-test
There was a problem hiding this comment.
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
selectGitApplicationCurrentBranchfromselectors/gitModSelectorssupports 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.
app/client/src/ce/navigation/FocusStrategy/NoIDEFocusStrategy.ts
Outdated
Show resolved
Hide resolved
…o chore/add-create-permission-check
KelvinOm
requested review from
KelvinOm
and removed request for
a team and
KelvinOm
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts(2 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(1 hunks)app/client/src/sagas/FocusRetentionSaga.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ce/sagas/DatasourcesSagas.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/sagas/FocusRetentionSaga.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/ce/navigation/FocusStrategy/NoIDEFocusStrategy.ts
[error] 14-16: This generator function doesn't contain yield.
(lint/correctness/useYield)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (8)
app/client/src/ce/navigation/FocusStrategy/NoIDEFocusStrategy.ts (2)
1-1: Import path update looks good.The import is now referencing the new centralized types file, which improves code organization.
11-13: Method signature update is appropriate.The
getEntityParentUrlmethod has been converted to an arrow function, which is consistent with the method implementation pattern.app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts (2)
2-5: Import path update looks good.The import is now referencing the new centralized types file, which improves code organization.
294-300: Implementation of getUrlKey looks correct.The generator function properly yields to select the Git branch from Redux state and returns a URL with the branch information appended. This implementation will help maintain focused state across different branches.
app/client/src/ce/navigation/FocusStrategy/types.ts (4)
1-9: New type definitions look good.The imports and FocusPath interface are well-defined and provide good type safety.
11-13: FocusElementsConfigList type is well-structured.Good use of mapped type to create a mapping from FocusEntity to arrays of FocusElementConfig.
33-43: New getUrlKey method is appropriate.The addition of the getUrlKey method to the interface matches the implementations we've seen in the other files. The documentation comment is helpful.
44-50: waitForPathLoad method signature looks correct.The method signature for the generator function is properly defined, although it has the same TODO item about fixing the ESLint disable in the future.
| getUrlKey: function* (url: string) { | ||
| return url; | ||
| }, |
There was a problem hiding this comment.
Generator function should use yield statements.
This generator function doesn't contain any yield statements, which negates the purpose of using a generator. This should be either:
- Modified to use yield (if asynchronous behavior is needed), similar to how it's implemented in AppIDEFocusStrategy
- Changed to a regular function or arrow function if no generator behavior is needed
- getUrlKey: function* (url: string) {
- return url;
- },
+ getUrlKey: (url: string): string => {
+ return url;
+ },Or if you need to match the interface but don't need yield functionality in this specific implementation:
- getUrlKey: function* (url: string) {
- return url;
- },
+ getUrlKey: function* (url: string) {
+ // For NoIDE implementation, we don't need branch information
+ const result = url;
+ yield;
+ return result;
+ },📝 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.
| getUrlKey: function* (url: string) { | |
| return url; | |
| }, | |
| // Option 1: If generator behavior is not needed, convert to a regular arrow function. | |
| getUrlKey: (url: string): string => { | |
| return url; | |
| }, | |
| // Option 2: If you need to maintain the generator interface, add a yield. | |
| /* | |
| getUrlKey: function* (url: string) { | |
| // For NoIDE implementation, we don't need branch information | |
| const result = url; | |
| yield; | |
| return result; | |
| }, | |
| */ |
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-16: This generator function doesn't contain yield.
(lint/correctness/useYield)
| export interface FocusStrategy { | ||
| focusElements: FocusElementsConfigList; | ||
| /** based on the route change, what states need to be set in the upcoming route **/ | ||
| getEntitiesForSet: ( | ||
| previousPath: string, | ||
| currentPath: string, | ||
| state: AppsmithLocationState, | ||
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| ) => Generator<any, Array<FocusPath>, any>; | ||
| /** based on the route change, what states need to be stored for the previous route **/ | ||
| getEntitiesForStore: ( | ||
| path: string, | ||
| currentPath: string, | ||
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| ) => Generator<any, Array<FocusPath>, any>; | ||
| /** For entities with hierarchy, return the parent entity path for storing its state **/ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Interface methods are well-documented.
The FocusStrategy interface methods are clearly documented with comments explaining their purpose. The TODO comments for fixing ESLint disables should be addressed in a future update.
Keep track of these TODOs for future updates:
🌐 Web query:
What are best practices for avoiding @typescript-eslint/no-explicit-any in generator function signatures?
💡 Result:
To avoid @typescript-eslint/no-explicit-any in generator function signatures while maintaining type safety, consider these best practices:
Use Precise Type Annotations
Replace any with specific types in generator parameters and yield expressions:
function* safeGenerator(): Generator<number, void, string> {
const input: string = yield 42; // Yield number, accept string via next()
console.log(input.toUpperCase());
}This specifies:
- Yield type:
number - Return type:
void - Input type for
next():string
Leverage unknown for Unknown Types
When types aren't immediately known:
function* typeSafeGenerator(): Generator<unknown, void, unknown> {
const data = yield; // data is unknown
if (typeof data === 'string') {
// Type-checked usage
}
}Use Interface Contracts
Define explicit interfaces for complex parameters:
interface GeneratorConfig<T> {
initialValue: T;
transform: (value: T) => T;
}
function* configuredGenerator<T>({
initialValue,
transform
}: GeneratorConfig<T>): Generator<T> {
let current = initialValue;
while (true) {
current = transform(current);
yield current;
}
}Enable Complementary ESLint Rules
Strengthen type safety with these related rules:
| Rule | Purpose | Example Fix |
|---|---|---|
no-unsafe-argument |
Prevents passing any to functions |
Type callback parameters |
no-unsafe-return |
Blocks returning any from generators |
Specify yield return type |
no-unsafe-assignment |
Catches implicit any assignments |
Add type annotations |
Generator-Specific Patterns
- Typed Iterators:
function* paginatedResults<T>(urls: string[]): Generator<Promise<T>, void> {
for (const url of urls) {
yield fetch(url).then(res => res.json());
}
}- Type Narrowing:
function* typeCheckedGenerator() {
const value = yield;
if (isValidType(value)) {
// Safely use narrowed type
}
}- Generic Generators:
function* asyncWrapper<T>(promise: Promise<T>): Generator<Promise<T>, T> {
return yield promise;
}Migration Strategy
- Enable
@typescript-eslint/no-explicit-anywith strict mode - Use ESLint's
--fixwithfixToUnknown: trueoption[1] - Gradually replace
unknownwith specific types - Utilize TypeScript's
ReturnTypeandYieldTypehelpers:
type YieldType<T> = T extends Generator<infer Y> ? Y : never;By combining these techniques, you maintain generator flexibility while eliminating unsafe any usage. The TypeScript 3.6+ Generator type provides strict type checking for yield/return/next interactions[14], and when combined with proper ESLint configuration, ensures comprehensive type safety across generator lifecycle operations.
Citations:
- 1: https://typescript-eslint.io/rules/no-explicit-any/
- 2: https://www.typescriptlang.org/docs/handbook/2/functions.html
- 3: https://typescript-eslint.io/rules/
- 4: https://typescript-eslint.io/blog/avoiding-anys/
- 5: https://www.assertnotmagic.com/2020/05/12/untangling-typescript-parameters/
- 6: https://typescript-eslint.io/rules/no-unsafe-function-type/
- 7: https://www.allthingstypescript.dev/p/why-avoid-the-any-type-in-typescript
- 8: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*
- 9: https://www.benmvp.com/blog/when-use-typescript-unknown-versus-any
- 10: https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html
- 11: https://www.typescriptlang.org/docs/handbook/functions.html
- 12: https://www.npmjs.com/package/@cling/eslint-config
- 13: https://docs.datadoghq.com/security/code_security/static_analysis/static_analysis_rules/typescript-best-practices/no-explicit-any/
- 14: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-6.html
- 15: [no-explicit-any] Allow safe usages of
anytypescript-eslint/typescript-eslint#1071 - 16: https://google.github.io/styleguide/tsguide.html
- 17: https://developer.huawei.com/consumer/en/doc/harmonyos-guides/ide_no-explicit-any-V13
- 18: https://caoslab.psy.cmu.edu:32443/developers/jsexperimentslayout/-/blob/1d532da4c3a5529e3c8a5b6b18bddc278538f258/client/node_modules/@typescript-eslint/eslint-plugin/docs/rules/no-explicit-any.md
Action Required: Update Generator Signatures for Strict Typing
The interface is well-documented; however, both generator methods currently use explicit any types (temporarily suppressed by ESLint) that could be improved. In light of best practices, please consider updating the signatures for getEntitiesForSet and getEntitiesForStore as follows:
-
Replace
anywith precise types:
Instead of usingGenerator<any, Array<FocusPath>, any>, annotate the generator with clear types for the yielded value, the return value, and the value accepted by.next(). For example, if the generator yields values of typeYieldType, returns anArray<FocusPath>, and accepts inputs of typeNextType, use:Generator<YieldType, Array<FocusPath>, NextType>
-
Consider using
unknownwhen appropriate:
If a type isn’t immediately known or can vary, usingunknownin place ofanycan preserve type safety until a more specific type can be determined. -
Plan for a gradual refactor:
Address these TODO comments by replacing the ESLint suppressions with proper type annotations based on your generator’s logic. This will improve maintainability and enforce stricter type checking.
Please keep track of these TODOs for a future update.
3 participants
Description
getEntityParentUrland updatinghandleRemoveFocusHistoryin focus retention sagaFixes #39673
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13810368612
Commit: 7bc2574
Cypress dashboard.
Tags: @tag.All
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Wed, 12 Mar 2025 11:46:01 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Style