Skip to content

chore: Adding create permission check for the plus button on tabs list #39672

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.

Sign up for GitHub

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

Closed
ankitakinger wants to merge 17 commits into from
Closed

chore: Adding create permission check for the plus button on tabs list #39672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
56ceaf6
adding permission check for the plus button in entity tabs list
ankitakinger Mar 10, 2025
d205485
minor change
ankitakinger Mar 10, 2025
b401fbc
updating file names
ankitakinger Mar 10, 2025
4b6aa3a
removing unused classname
ankitakinger Mar 11, 2025
3ef148c
adding check for focus retention remove history
ankitakinger Mar 11, 2025
582e567
updating entity item storybook
ankitakinger Mar 11, 2025
65984cd
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
ankitakinger Mar 11, 2025
10f4a6d
separating out types to resolve cyclic dependencies on EE
ankitakinger Mar 11, 2025
1103ce8
updating import
ankitakinger Mar 11, 2025
6d8086b
updating import
ankitakinger Mar 11, 2025
b15dd32
addressing review comments
ankitakinger Mar 12, 2025
eab2050
addressing review comments
ankitakinger Mar 12, 2025
d0df6c6
minor change
ankitakinger Mar 12, 2025
0df8c33
updating URL
ankitakinger Mar 12, 2025
f26e5f4
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
ankitakinger Mar 12, 2025
f26de0a
fixing focus retention
ankitakinger Mar 12, 2025
7bc2574
updating focus strategies
ankitakinger Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
0 app/client/src/ce/navigation/FocusSetters.ts → ...t/src/ce/navigation/AppIDEFocusSetters.ts
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions app/client/src/ce/navigation/FocusElements/AppIDE.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,10 @@ import {
setSelectedEntityUrl,
setSelectedJSObject,
setSelectedQuery,
} from "ee/navigation/FocusSetters";
} from "ee/navigation/AppIDEFocusSetters";
import { getFirstDatasourceId } from "selectors/datasourceSelectors";
import { FocusElement, FocusElementConfigType } from "navigation/FocusElements";
import type { FocusElementsConfigList } from "sagas/FocusRetentionSaga";
import type { FocusElementsConfigList } from "ee/navigation/FocusStrategy/types";
import { ActionExecutionResizerHeight } from "PluginActionEditor/components/PluginActionResponse/constants";
import {
getPluginActionConfigSelectedTab,
Expand Down
12 changes: 11 additions & 1 deletion app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { all, select, take } from "redux-saga/effects";
import type { FocusPath, FocusStrategy } from "sagas/FocusRetentionSaga";
import type {
FocusPath,
FocusStrategy,
} from "ee/navigation/FocusStrategy/types";
import type { AppsmithLocationState } from "utils/history";
import { NavigationMethod } from "utils/history";
import type { FocusEntityInfo } from "navigation/FocusEntity";
Expand Down Expand Up @@ -288,6 +291,13 @@ export const AppIDEFocusStrategy: FocusStrategy = {
// We do not have to add any query params because this url is used as the key
return parentUrl.split("?")[0];
},
getUrlKey: function* (url: string) {
const branch: string | undefined = yield select(
selectGitApplicationCurrentBranch,
);

return `${url}#${branch}`;
},
*waitForPathLoad(currentPath: string, previousPath?: string) {
if (previousPath) {
// When page is changing, there may be some items still not loaded,
Expand Down
7 changes: 5 additions & 2 deletions app/client/src/ce/navigation/FocusStrategy/NoIDEFocusStrategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { FocusStrategy } from "sagas/FocusRetentionSaga";
import type { FocusStrategy } from "ee/navigation/FocusStrategy/types";
import NoIDEFocusElements from "../FocusElements/NoIDE";
export const NoIDEFocusStrategy: FocusStrategy = {
focusElements: NoIDEFocusElements,
Expand All @@ -8,8 +8,11 @@ export const NoIDEFocusStrategy: FocusStrategy = {
*getEntitiesForStore() {
return [];
},
getEntityParentUrl(): string {
getEntityParentUrl: (): string => {
return "";
},
getUrlKey: function* (url: string) {
return url;
},
Comment on lines +14 to +16
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)

*waitForPathLoad() {},
};
50 changes: 50 additions & 0 deletions app/client/src/ce/navigation/FocusStrategy/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import type { FocusEntityInfo } from "navigation/FocusEntity";
import type { FocusEntity } from "navigation/FocusEntity";
import type { FocusElementConfig } from "navigation/FocusElements";
import type { AppsmithLocationState } from "utils/history";

export interface FocusPath {
key: string;
entityInfo: FocusEntityInfo;
}

export type FocusElementsConfigList = {
[key in FocusEntity]?: FocusElementConfig[];
};

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 **/
Comment on lines +15 to +32
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

  1. Typed Iterators:
function* paginatedResults<T>(urls: string[]): Generator<Promise<T>, void> {
  for (const url of urls) {
    yield fetch(url).then(res => res.json());
  }
}
  1. Type Narrowing:
function* typeCheckedGenerator() {
  const value = yield;
  if (isValidType(value)) {
    // Safely use narrowed type
  }
}
  1. Generic Generators:
function* asyncWrapper<T>(promise: Promise<T>): Generator<Promise<T>, T> {
  return yield promise;
}

Migration Strategy

  1. Enable @typescript-eslint/no-explicit-any with strict mode
  2. Use ESLint's --fix with fixToUnknown: true option[1]
  3. Gradually replace unknown with specific types
  4. Utilize TypeScript's ReturnType and YieldType helpers:
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:

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 any with precise types:
    Instead of using Generator<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 type YieldType, returns an Array<FocusPath>, and accepts inputs of type NextType, use:

    Generator<YieldType, Array<FocusPath>, NextType>

  • Consider using unknown when appropriate:
    If a type isn’t immediately known or can vary, using unknown in place of any can 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.

getEntityParentUrl: (
entityInfo: FocusEntityInfo,
parentEntity: FocusEntity,
) => string;
/** Get focus history key for the URL */
getUrlKey: (
url: string,
// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
) => Generator<any, string, any>;
/** Define a wait (saga) before we start setting states **/
waitForPathLoad: (
currentPath: string,
previousPath: string,
// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
) => Generator<any, void, any>;
}
2 changes: 1 addition & 1 deletion app/client/src/ce/sagas/DatasourcesSagas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ export function* deleteDatasourceSaga(
const isValidResponse: boolean = yield validateResponse(response);

if (isValidResponse) {
const currentUrl = `${window.location.pathname}`;
const currentUrl = window.location.pathname;

yield call(handleDatasourceDeleteRedirect, id);
yield call(FocusRetention.handleRemoveFocusHistory, currentUrl);
Expand Down
1 change: 1 addition & 0 deletions app/client/src/ee/navigation/AppIDEFocusSetters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "ce/navigation/AppIDEFocusSetters";
1 change: 0 additions & 1 deletion app/client/src/ee/navigation/FocusSetters.ts
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions app/client/src/ee/navigation/FocusStrategy/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "ce/navigation/FocusStrategy/types";
16 changes: 15 additions & 1 deletion app/client/src/pages/AppIDE/layouts/components/EditorTabs/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ import { ScreenModeToggle } from "./ScreenModeToggle";
import { EditableTab } from "./EditableTab";
import { TabSelectors } from "./constants";
import { AddTab } from "./AddTab";
import { useFeatureFlag } from "utils/hooks/useFeatureFlag";
import { FEATURE_FLAG } from "ee/entities/FeatureFlag";
import { getPagePermissions } from "selectors/editorSelectors";
import { getHasCreateActionPermission } from "ee/utils/BusinessFeatures/permissionPageHelpers";

const EditorTabs = () => {
const location = useLocation();
Expand All @@ -42,9 +46,19 @@ const EditorTabs = () => {
const [showNudge, dismissNudge] = useShowSideBySideNudge();
const { addClickHandler } = useIDETabClickHandlers();
const isJSLoading = useIsJSAddLoading();
const hideAdd = segmentMode === EditorEntityTabState.Add || !files.length;

const currentEntity = identifyEntityFromPath(location.pathname);
const isFeatureEnabled = useFeatureFlag(FEATURE_FLAG.license_gac_enabled);
const pagePermissions = useSelector(getPagePermissions);
const canCreateActions = getHasCreateActionPermission(
isFeatureEnabled,
pagePermissions,
);
const hideAdd =
segmentMode === EditorEntityTabState.Add ||
!files.length ||
!canCreateActions;

const showEntityListButton =
ideViewMode === EditorViewMode.SplitScreen && files.length > 0;

Expand Down
2 changes: 1 addition & 1 deletion app/client/src/pages/Editor/gitSync/components/PushFailedWarning.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const Container = styled.div`
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export default function PushFailedWarning({ closeHandler, error }: any) {
return (
<Container className="ankita">
<Container>
<Callout isClosable kind="error" onClose={closeHandler}>
<>
<Text type={TextType.H5}>{error.errorType}</Text>
Expand Down
61 changes: 14 additions & 47 deletions app/client/src/sagas/FocusRetentionSaga.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,47 +21,10 @@ import type { Plugin } from "entities/Plugin";
import { getIDETypeByUrl } from "ee/entities/IDE/utils";
import { getIDEFocusStrategy } from "ee/navigation/FocusStrategy";
import { IDE_TYPE } from "ee/IDE/Interfaces/IDETypes";
import { selectGitApplicationCurrentBranch } from "selectors/gitModSelectors";

export interface FocusPath {
key: string;
entityInfo: FocusEntityInfo;
}

export type FocusElementsConfigList = {
[key in FocusEntity]?: FocusElementConfig[];
};

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 **/
getEntityParentUrl: (
entityInfo: FocusEntityInfo,
parentEntity: FocusEntity,
) => string;
/** Define a wait (saga) before we start setting states **/
waitForPathLoad: (
currentPath: string,
previousPath: string,
// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
) => Generator<any, void, any>;
}
import type {
FocusPath,
FocusStrategy,
} from "ee/navigation/FocusStrategy/types";

/**
* Context switching works by restoring the states of ui elements to as they were
Expand Down Expand Up @@ -123,23 +86,27 @@ class FocusRetention {
}

public *handleRemoveFocusHistory(url: string) {
const branch: string | undefined = yield select(
selectGitApplicationCurrentBranch,
);
const removeKeys: string[] = [];
const entityKey: string = yield call(this.focusStrategy.getUrlKey, url);
const focusEntityInfo = identifyEntityFromPath(url);

removeKeys.push(`${url}#${branch}`);
removeKeys.push(`${entityKey}`);

const parentElement = FocusStoreHierarchy[focusEntityInfo.entity];

if (parentElement) {
const parentPath = this.focusStrategy.getEntityParentUrl(
const parentUrl: string = yield call(
this.focusStrategy.getEntityParentUrl,
focusEntityInfo,
parentElement,
);

removeKeys.push(`${parentPath}#${branch}`);
const parentEntityKey: string = yield call(
this.focusStrategy.getUrlKey,
parentUrl,
);

removeKeys.push(parentEntityKey);
}

for (const key of removeKeys) {
Expand Down
Loading