app/client/src/ce/pages/Applications/WorkspaceMenu.tsx (1)

169-181:Use RouteBuilder + add test id/a11y role.

• Prefer central URL helper to avoid hard-coded paths.
• Add data-testid and role="menuitem" for QA/a11y parity with other items.

Apply within this block:

-  <CustomMenuItem-    className="workspace-menu-item"+  <CustomMenuItem+    className="workspace-menu-item"+    data-testid="t--workspace-datasources"+    role="menuitem"     onClick={() =>-      getOnSelectAction(DropdownOnSelectActions.REDIRECT, {-        path: `/workspace/${workspace.id}/datasources`,-      })+      getOnSelectAction(DropdownOnSelectActions.REDIRECT, {+        path: workspaceDatasourcesURL({ workspaceId: workspace.id }),+      })     }   >

And add import at top:

+import { workspaceDatasourcesURL } from "ce/RouteBuilder";

app/client/src/pages/workspace/index.tsx (1)

20-29:Prefer Route.render to avoid remounts and re-created component functions.
Keeps props typing simple and avoids new component identity per render.

-        <SentryRoute-          component={({-            match,-          }: {-            match: { params: { workspaceId: string } };-          }) => (-            <WorkspaceDatasourcesPage workspaceId={match.params.workspaceId} />-          )}-          path={`${path}/:workspaceId/datasources`}-        />+        <SentryRoute+          path={`${path}/:workspaceId/datasources`}+          render={({ match }: { match: { params: { workspaceId: string } } }) => (+            <WorkspaceDatasourcesPage workspaceId={match.params.workspaceId} />+          )}+        />

app/client/src/ce/RouteBuilder.ts (1)

200-206:Consider aligning with the existing URL building pattern.

These functions use template literals directly, while most other URL builders in this file useurlBuilder.build(). For consistency and to leverage any URL handling logic (encoding, normalization) thaturlBuilder provides, consider using the established pattern.

Apply this diff to align with existing patterns:

-export const workspaceDatasourcesURL = (workspaceId: string): string =>-  `/workspace/${workspaceId}/datasources`;--export const workspaceDatasourceEditorURL = (-  workspaceId: string,-  datasourceId: string,-): string => `/workspace/${workspaceId}/datasources/${datasourceId}`;+export const workspaceDatasourcesURL = (workspaceId: string): string =>+  urlBuilder.build({+    suffix: `workspace/${workspaceId}/datasources`,+  });++export const workspaceDatasourceEditorURL = (+  workspaceId: string,+  datasourceId: string,+): string =>+  urlBuilder.build({+    suffix: `workspace/${workspaceId}/datasources/${datasourceId}`,+  });

Note: Review urlBuilder.build implementation to ensure it handles absolute paths starting with "workspace" correctly, or adjust the pattern accordingly.

app/client/src/ce/sagas/workspaceIDESagas.ts (2)

24-28:Await editor reset before initialization to avoid race conditions.

RESET_EDITOR_REQUEST may have async side-effects. Wait for RESET_EDITOR_SUCCESS before proceeding.

-import { all, put, call, takeLatest } from "redux-saga/effects";+import { all, put, call, take, takeLatest } from "redux-saga/effects";...   yield put(resetEditorRequest());+  yield take(ReduxActionTypes.RESET_EDITOR_SUCCESS);   yield call(workspaceEngine.setupEngine);

---

39-43:Simplify watcher.

all([...]) with a single effect is unnecessary. Yield takeLatest directly.

-export default function* watchWorkspaceIDESagas() {-  yield all([-    takeLatest(ReduxActionTypes.INITIALIZE_WORKSPACE_IDE, startWorkspaceIDE),-  ]);-}+export default function* watchWorkspaceIDESagas() {+  yield takeLatest(ReduxActionTypes.INITIALIZE_WORKSPACE_IDE, startWorkspaceIDE);+}

app/client/src/ce/reducers/uiReducers/editorReducer.tsx (1)

45-62:Reset workspace IDE flag on RESET_EDITOR_REQUEST.

Keeps flags consistent across editor switches.

 [ReduxActionTypes.RESET_EDITOR_REQUEST]: (state: EditorReduxState) => {   return {     ...state,+    isWorkspaceEditorInitialized: false,     currentPageId: undefined,

app/client/src/ce/actions/workspaceIDEActions.ts (1)

7-10:LGTM. Consider typing the action.

Optionally return ReduxAction for stricter typing.

app/client/src/pages/workspace/WorkspaceDatasourceEditor.tsx (1)

16-24:Guard selector result and avoid magic string.

• Use optional chaining to avoid reading id from undefined.
• Replace "unused_env" with a shared constant.

 useEffect(() => {-  if (-    currentEnvironmentDetails.id &&-    currentEnvironmentDetails.id !== "unused_env"-  ) {-    dispatch(setCurrentEditingEnvironmentID(currentEnvironmentDetails.id));-  }-}, [dispatch, currentEnvironmentDetails.id]);+  const envId = currentEnvironmentDetails?.id;+  if (envId && envId !== UNUSED_ENV_ID) {+    dispatch(setCurrentEditingEnvironmentID(envId));+  }+}, [dispatch, currentEnvironmentDetails?.id]);

Add/import:const UNUSED_ENV_ID = "unused_env"; (or import an existing constant).

app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (1)

95-106:Add error fallback UI.

Show an error state if initialization fails (pair with reducer error flag).

-if (!isWorkspaceEditorInitialized) {+const initError = useSelector(+  (state: DefaultRootState) => state.ui.editor.workspaceEditorInitError,+);+if (initError) {+  return (+    <PageContainer>+      <CenteredWrapper style={{ height: `calc(100vh - ${IDE_HEADER_HEIGHT}px)` }}>+        <div>Failed to initialize Workspace Datasources. Please retry.</div>+      </CenteredWrapper>+    </PageContainer>+  );+}+if (!isWorkspaceEditorInitialized) {   return (

app/client/src/ce/entities/Engine/WorkspaceEditorEngine.ts (1)

104-108:Optional: include workspaceId in SUCCESS payload.

Helps consumers correlate which workspace initialized (useful for telemetry or race-proofing).

   *completeChore() {     yield put({-      type: ReduxActionTypes.INITIALIZE_WORKSPACE_IDE_SUCCESS,+      type: ReduxActionTypes.INITIALIZE_WORKSPACE_IDE_SUCCESS,+      // payload: { workspaceId }, // if you propagate it here     });   }

app/client/src/pages/workspace/WorkspaceDataSidePane.tsx (4)

170-179:Add alt text to datasource icons (a11y).

Provide meaningful alt; fallback to plugin or datasource name.

-  startIcon: (-    <DatasourceIcon-      src={getAssetUrl(pluginImages[data.pluginId])}-    />-  ),+  startIcon: (+    <DatasourceIcon+      alt={`${data.name} icon`}+      src={getAssetUrl(pluginImages[data.pluginId])}+    />+  ),

---

36-40:Optional: avoid magic numbers in heights.

Use a shared header height token to compute PaneBody height for consistency with IDE.

-  height: calc(100vh - 120px);+  /* import { IDE_HEADER_HEIGHT } from "@appsmith/ads"; */+  height: calc(100vh - var(--ide-header-height, 120px));

And set the CSS variable at a higher container if available.

---

70-75:useCallback deps completeness.

goToDatasource depends on history. It’s stable, but include it to silence exhaustive-deps lint (if enabled).

-  const goToDatasource = useCallback(+  const goToDatasource = useCallback(     (id: string) => {       history.push(workspaceDatasourceEditorURL(workspaceId, id));     },-    [workspaceId],+    [workspaceId, history],   );

---

124-137:Show spinner copy via i18n message key.

Replace hardcoded “Loading workspace permissions...” with a messages constant for consistency.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (3)

71-73:Pass IDE_TYPE.Workspace for correct usage labels.

The selector generates usage strings like "N queries in this app" when passedIDE_TYPE.App. Since this is a workspace context, passIDE_TYPE.Workspace so labels read "…in this workspace".

-const dsUsageMap = useSelector((state) =>-  getDatasourceUsageCountForApp(state, IDE_TYPE.App),-);+const dsUsageMap = useSelector((state) =>+  getDatasourceUsageCountForApp(state, IDE_TYPE.Workspace),+);

---

81-86:Initialization won't re-run when switching workspaces.

The!isWorkspaceEditorInitialized guard prevents re-initialization when navigating from one workspace to another. The flag stays true from the first workspace, so workspace B never initializes.

-useEffect(() => {-  if (workspaceId && !isWorkspaceEditorInitialized) {-    dispatch(initWorkspaceIDE({ workspaceId }));-  }-}, [dispatch, workspaceId, isWorkspaceEditorInitialized]);+useEffect(() => {+  if (workspaceId) {+    dispatch(initWorkspaceIDE({ workspaceId }));+  }+}, [dispatch, workspaceId]);

---

120-124:Route path case mismatch with navigation calls.

This route uses "/new" (lowercase), but WorkspaceDataSidePane pushes "/NEW" (uppercase) on lines 43 and 102. Navigation will fail. Standardize on lowercase "/new" in WorkspaceDataSidePane.tsx.

app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (2)

89-106:Consider adding error handling for failed initialization.

Currently there's no feedback if workspace fetch or IDE initialization fails. Users might see an indefinite spinner. Consider adding error boundaries or timeout handling.

---

123-135:Inconsistent workspaceId prop.

Line 123 rendersWorkspaceCreateNewDatasourceTab without passingworkspaceId, but line 135 passes it explicitly. If the component extracts it from route params, the explicit prop is redundant. If not, line 123 is missing it.

For consistency, either remove the explicit prop on line 135:

-<WorkspaceCreateNewDatasourceTab workspaceId={workspaceId} />+<WorkspaceCreateNewDatasourceTab />

Or add it to line 123:

-render={() => <WorkspaceCreateNewDatasourceTab />}+render={() => <WorkspaceCreateNewDatasourceTab workspaceId={workspaceId} />}

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (2)

70-73:Use Workspace context for usage strings.

Pass IDE_TYPE.Workspace so labels read “…in this workspace”.

-  const dsUsageMap = useSelector((state) =>-    getDatasourceUsageCountForApp(state, IDE_TYPE.App),-  );+  const dsUsageMap = useSelector((state) =>+    getDatasourceUsageCountForApp(state, IDE_TYPE.Workspace),+  );

---

81-86:Re-initialize IDE on workspace change.

The guard can skip init when switching workspaces. Dispatch on every workspaceId change.

-useEffect(() => {-  if (workspaceId && !isWorkspaceEditorInitialized) {-    dispatch(initWorkspaceIDE({ workspaceId }));-  }-}, [dispatch, workspaceId, isWorkspaceEditorInitialized]);+useEffect(() => {+  if (workspaceId) {+    dispatch(initWorkspaceIDE({ workspaceId }));+  }+}, [dispatch, workspaceId]);

app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (2)

132-135:Make the default list route exact.

Prevents accidental matches if route order shifts.

-            <SentryRoute-              path="/workspace/:workspaceId/datasources"-              render={() => <WorkspaceCreateNewDatasourceTab />}-            />+            <SentryRoute+              exact+              path="/workspace/:workspaceId/datasources"+              render={() => <WorkspaceCreateNewDatasourceTab />}+            />

---

121-124:Avoid magic “NEW” segment.

Extract to a constant or reuse a RouteBuilder helper to keep callers and routes in sync.

-              path="/workspace/:workspaceId/datasources/NEW"+              path={`/workspace/:workspaceId/datasources/${NEW_SEGMENT}`}

Consider adding workspaceDatasourceNewURL(workspaceId) in RouteBuilder and using it here.

app/client/src/ce/entities/URLRedirect/URLAssembly.ts (3)

254-263:Don’t infer context from window.location. Prefer explicit params.

Coupling to window pathname makes URL building dependent on the current page and breaks SSR/tests. Consider treating presence of builderParams.workspaceId as the workspace context switch, falling back to location only if absent.

Minimal approach: add an optional parameter or internal flag set in build():

• Determineconst inWorkspace = !!builderParams.workspaceId || this.isWorkspaceContext();
• Pass workspaceId explicitly to generateBasePathForWorkspace rather than re-parsing location.

---

341-349:Suffix rewrite for “datasource/” is brittle.

This special-case logic is easy to miss for other workspace routes and mixes concerns. Prefer dedicated helpers or route constants for workspace URLs and keep URLBuilder generic.

• Move workspace-datasource path assembly to RouteBuilder (already added) and remove this special-case.
• If retained, also handle exact "datasource" (no trailing slash) to avoid edge cases.

---

30-32:Document workspaceId usage.

Clarify that providing workspaceId opts into workspace routing so callers aren’t surprised.

Add JSDoc on URLBuilderParams.workspaceId explaining precedence over window location and expected consumers.

app/client/src/ce/RouteBuilder.ts (1)

200-207:Unify with route constants and add a helper for "NEW".

• ConstantsWORKSPACE_URL andWORKSPACE_DATASOURCES_PAGE_URL exist in baseRoutes and should be imported to avoid drift.
• AddworkspaceDatasourceNewURL helper to consolidate the three hardcoded/datasources/NEW strings currently in WorkspaceDatasourcesPage.tsx and WorkspaceDataSidePane.tsx.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (2)

75-78:Use Workspace context for usage strings.

As previously noted, passingIDE_TYPE.App causes usage labels to read "in this app". Update toIDE_TYPE.Workspace so the UI displays "in this workspace" for consistency with the workspace context.

-  const dsUsageMap = useSelector((state) =>-    getDatasourceUsageCountForApp(state, IDE_TYPE.App),-  );+  const dsUsageMap = useSelector((state) =>+    getDatasourceUsageCountForApp(state, IDE_TYPE.Workspace),+  );

---

86-91:Re-initialize when workspaceId changes.

As previously noted, the current guard prevents re-initialization when navigating between different workspaces. TheisWorkspaceEditorInitialized flag remains true after the first workspace is loaded, so switching to a different workspace won't trigger initialization.

-  useEffect(() => {-    if (workspaceId && !isWorkspaceEditorInitialized) {-      dispatch(initWorkspaceIDE({ workspaceId }));-    }-  }, [dispatch, workspaceId, isWorkspaceEditorInitialized]);+  useEffect(() => {+    if (workspaceId) {+      dispatch(initWorkspaceIDE({ workspaceId }));+    }+  }, [dispatch, workspaceId]);

app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (1)

330-335:Optional: Consider removing unnecessary optional chaining.

The Plugin interface (fromentities/Plugin/index.ts) showspackageName is a required field, so the optional chainingplugin?.packageName is unnecessary. The variable namefinalFilteredMostPopularPlugins could be more descriptive (e.g.,workspaceFilteredPlugins), though the current name is acceptable.

-  // Filter out REST API from most popular plugins when on workspace datasources page-  const finalFilteredMostPopularPlugins = isWorkspaceDatasourcesPage-    ? filteredMostPopularPlugins.filter(-        (plugin: Plugin) => plugin?.packageName !== PluginPackageName.REST_API,-      )-    : filteredMostPopularPlugins;+  // Filter out REST API from most popular plugins when on workspace datasources page+  const workspaceFilteredPlugins = isWorkspaceDatasourcesPage+    ? filteredMostPopularPlugins.filter(+        (plugin: Plugin) => plugin.packageName !== PluginPackageName.REST_API,+      )+    : filteredMostPopularPlugins;

app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (1)

368-371:Consider using route matching utilities for robustness.

The current implementation usingincludes() will work for the expected paths but could theoretically match unintended URLs. For more robust path matching, consider using React Router'smatchPath or theuseLocation hook with explicit path comparison.

Example refactor:

+import { useLocation, matchPath } from "react-router";+import { WORKSPACE_DATASOURCES_PAGE_URL, WORKSPACE_DATASOURCE_EDITOR_PAGE_URL } from "constants/routes";...-  // Check if we're on workspace datasources page-  const isWorkspaceDatasourcesPage =-    window.location.pathname.includes("/workspace/") &&-    window.location.pathname.includes("/datasources");+  const location = useLocation();+  const isWorkspaceDatasourcesPage = Boolean(+    matchPath(location.pathname, { path: WORKSPACE_DATASOURCES_PAGE_URL }) ||+    matchPath(location.pathname, { path: WORKSPACE_DATASOURCE_EDITOR_PAGE_URL })+  );

app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (1)

123-141:Consider using lowercase "new" for route consistency.

The route uses/NEW (uppercase), which matches the navigation inWorkspaceDataSidePane. While this works, the convention in web routing typically favors lowercase path segments. Consider standardizing on/new across both the route definition and all navigation calls for better consistency with common web practices.

-            path={`${WORKSPACE_DATASOURCES_PAGE_URL}/NEW`}+            path={`${WORKSPACE_DATASOURCES_PAGE_URL}/new`}

And update navigation calls inWorkspaceDataSidePane.tsx:

-history.push(`/workspace/${workspaceId}/datasources/NEW`);+history.push(`/workspace/${workspaceId}/datasources/new`);

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

app/client/src/pages/workspace/WorkspaceDatasourceHeader.tsx (2)

34-36:Remove or clarify the empty div.

The empty div in IDEHeader.Right serves no clear purpose. Either remove it if IDEHeader.Right can be empty, or add a comment if it's a placeholder for future functionality.

         <IDEHeader.Right>-          <div />         </IDEHeader.Right>

---

9-16:WorkspaceDatasourceHeader height should be explicitly set to match padding-top.

The page correctly appliespadding-top: ${IDE_HEADER_HEIGHT}px onPageContainer to offset the fixed header. However,WorkspaceDatasourceHeader doesn't explicitly set its own height property. For alignment consistency, addheight: ${IDE_HEADER_HEIGHT}px; toStyledWorkspaceHeader to ensure the header and content spacing remain in sync ifIDE_HEADER_HEIGHT changes.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

app/client/src/pages/AppIDE/layouts/components/Header/index.tsx (2)

165-168:Consider adding a defensive check forcurrentWorkspace.id.

The code checkscurrentWorkspace.name on line 163 but directly accessescurrentWorkspace.id without validation. If the id is undefined, the URL becomes malformed (/applications?workspaceId=undefined).

Consider either:

1. AddingcurrentWorkspace.id to the condition on line 163
2. Using optional chaining with a fallback

-            <>-              <Link-                className="mr-1.5"-                to={`${APPLICATIONS_URL}?workspaceId=${currentWorkspace.id}`}-              >+            {currentWorkspace.id ? (+              <>+                <Link+                  className="mr-1.5"+                  to={`${APPLICATIONS_URL}?workspaceId=${currentWorkspace.id}`}+                >+                  {currentWorkspace.name}+                </Link>+                {"/"}+                <EditorName+                  applicationId={applicationId}+                  className="t--application-name editable-application-name max-w-48"+                  defaultSavingState={+                    isSavingName ? SavingState.STARTED : SavingState.NOT_STARTED+                  }+                  defaultValue={currentApplication?.name || ""}+                  editInteractionKind={EditInteractionKind.SINGLE}+                  editorName="Application"+                  fill+                  getNavigationMenu={useNavigationMenuData}+                  isError={isErroredSavingName}+                  isNewEditor={+                    applicationList.filter((el) => el.id === applicationId)+                      .length > 0+                  }+                  isPopoverOpen={isPopoverOpen}+                  onBlur={(value: string) =>+                    updateApplicationDispatch(applicationId || "", {+                      name: value,+                      currentApp: true,+                    })+                  }+                  setIsPopoverOpen={setIsPopoverOpen}+                />+              </>+            ) : null}-                {currentWorkspace.name}-              </Link>-              {"/"}-              <EditorName-                applicationId={applicationId}-                className="t--application-name editable-application-name max-w-48"-                defaultSavingState={-                  isSavingName ? SavingState.STARTED : SavingState.NOT_STARTED-                }-                defaultValue={currentApplication?.name || ""}-                editInteractionKind={EditInteractionKind.SINGLE}-                editorName="Application"-                fill-                getNavigationMenu={useNavigationMenuData}-                isError={isErroredSavingName}-                isNewEditor={-                  applicationList.filter((el) => el.id === applicationId)-                    .length > 0-                }-                isPopoverOpen={isPopoverOpen}-                onBlur={(value: string) =>-                  updateApplicationDispatch(applicationId || "", {-                    name: value,-                    currentApp: true,-                  })-                }-                setIsPopoverOpen={setIsPopoverOpen}-              />-            </>

---

167-167:Consider using a URL builder utility for query parameter construction.

Manually constructing query parameters via template literals lacks type safety and proper URL encoding. Consider usingURLSearchParams or a URL builder utility consistent with patterns elsewhere in the codebase (e.g.,viewerURL on line 144).

Example withURLSearchParams:

constapplicationsUrl=`${APPLICATIONS_URL}?${newURLSearchParams({workspaceId:currentWorkspace.id}).toString()}`;

Or define a URL builder function similar to other route builders in the codebase.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (1)

78-83:Re-initialize when workspaceId changes.

The!isWorkspaceEditorInitialized guard prevents re-initialization when navigating between different workspaces. Once initialized for workspace A, switching to workspace B will skip initialization because the flag is already true.

Apply this diff to ensure initialization occurs for each workspace:

 useEffect(() => {-  if (workspaceId && !isWorkspaceEditorInitialized) {+  if (workspaceId) {     dispatch(initWorkspaceIDE({ workspaceId }));   }-}, [dispatch, workspaceId, isWorkspaceEditorInitialized]);+}, [dispatch, workspaceId]);

Note: If initialization should only occur once per workspace, track the initialized workspaceId separately rather than using a boolean flag.

app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (1)

114-118:Consider lowercase route convention.

The route uses uppercase/NEW, which is unconventional (most routes use lowercase/new). While this is consistent with the navigation inWorkspaceDataSidePane.tsx, lowercase is the standard convention for URLs.

To align with web conventions, consider refactoring to lowercase:

-            path={`${WORKSPACE_DATASOURCES_PAGE_URL}/NEW`}+            path={`${WORKSPACE_DATASOURCES_PAGE_URL}/new`}

And update the corresponding navigation inWorkspaceDataSidePane.tsx:

history.push(`/workspace/${workspaceId}/datasources/new`)

Alternatively, extract to a shared constant to prevent future mismatches:

// In constants/routes.tsexportconstDATASOURCE_NEW_PATH='/new';// Usagepath={`${WORKSPACE_DATASOURCES_PAGE_URL}${DATASOURCE_NEW_PATH}`}

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: Aishwarya-U-RRepo: appsmithorg/appsmith PR: 29405File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41Timestamp: 2024-10-08T15:32:39.374ZLearning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.

Learnt from: Aishwarya-U-RRepo: appsmithorg/appsmith PR: 29405File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41Timestamp: 2024-07-26T21:12:57.228ZLearning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.

Learnt from: brayn003Repo: appsmithorg/appsmith PR: 38171File: app/client/src/git/components/GitContextProvider/index.tsx:60-70Timestamp: 2024-12-16T19:43:14.764ZLearning: Prefer not to throw errors when dispatching actions in the `setImportWorkspaceId` function in `app/client/src/git/components/GitContextProvider/index.tsx`.

Learnt from: brayn003Repo: appsmithorg/appsmith PR: 38088File: app/client/src/git/components/GitQuickActions/BranchButton/index.tsx:72-74Timestamp: 2024-12-11T08:33:24.352ZLearning: In the 'BranchButton' component in 'app/client/src/git/components/GitQuickActions/BranchButton/index.tsx' (TypeScript, React), the `useEffect` hook that checks for label ellipsis does not need to include `currentBranch` in its dependency array.

Learnt from: ankitakingerRepo: appsmithorg/appsmith PR: 29965File: app/client/src/utils/storage.ts:129-129Timestamp: 2024-10-08T15:32:34.115ZLearning: The `migrateAppIdToEditorId` function in `app/client/src/utils/storage.ts` is intended to be called only when `appId` is present and `editorId` is not in the payload. This context should be considered in future reviews related to this function.

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/utils/helpers.tsx:1124-1136Timestamp: 2025-10-30T07:15:20.287ZLearning: In app/client/src/utils/helpers.tsx, within the getUpdateRouteForSlugPath function, the pageSlug parameter extracted from the route includes the trailing hyphen (e.g., "home-page-" not "home-page"). Additionally, when the static slug conversion branch is executed, params.basePageId is guaranteed to be defined because the code path requires successful page data fetch from the server as a precondition.

app/client/src/entities/Engine/WorkspaceEditorEngine.ts (1)

58-102:LGTM with minor suggestion.

The plugin and datasource loading logic is correct, with proper airgap handling and error flows. ThefailFastApiCalls pattern is used appropriately.

Consider includingworkspaceId in error messages for easier debugging:

   if (!initActionCalls)-    throw new PluginsNotFoundError("Unable to fetch plugins");+    throw new PluginsNotFoundError(`Unable to fetch plugins for workspace ${workspaceId}`);   // ... later ...   if (!pluginFormCall)     throw new PluginFormConfigsNotFoundError(-      "Unable to fetch plugin form configs",+      `Unable to fetch plugin form configs for workspace ${workspaceId}`,     );

Based on learnings, workspace IDs are obfuscated and safe to log.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: Aishwarya-U-RRepo: appsmithorg/appsmith PR: 29405File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41Timestamp: 2024-10-08T15:32:39.374ZLearning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.

Learnt from: Aishwarya-U-RRepo: appsmithorg/appsmith PR: 29405File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41Timestamp: 2024-07-26T21:12:57.228ZLearning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.

Learnt from: brayn003Repo: appsmithorg/appsmith PR: 36622File: app/client/src/entities/Engine/AppEditorEngine.ts:0-0Timestamp: 2024-10-11T09:28:32.636ZLearning: In the file `app/client/src/entities/Engine/AppEditorEngine.ts`, the IDs (`currentApplication.baseId`) and branches (`currentBranch`) are obfuscated, so it's acceptable to log them for debugging purposes.

app/client/src/pages/workspace/WorkspaceDataSidePane.tsx (1)

94-95:Fix route case: "NEW" → "new".

Routes are case-sensitive. The route inWorkspaceDatasourcesPage.tsx uses lowercase/NEW at line 127. Standardize to consistent casing.

Also applies to: 158-160

app/client/src/ce/entities/URLRedirect/URLAssembly.ts (1)

314-324:Validate workspaceId; don't fall back to empty string.

Returning"" yields/workspace//… URLs. Already flagged previously - ensure this is addressed.

app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (1)

125-129:Route path case must match callers.

Route uses/NEW (uppercase). Callers inWorkspaceDataSidePane.tsx also push/NEW. Standardize to lowercase/new for URL consistency.

app/client/cypress/e2e/Regression/ClientSide/Workspace/WorkspaceDatasourceSettings_spec.ts (1)

92-92:Reconsider excessive use of{ force: true } when enabling tests.

The commented tests use{ force: true } extensively in click operations. While not explicitly prohibited, forcing clicks can mask real actionability issues like elements being covered or not properly visible. Consider removing force flags and ensuring elements are properly interactable before clicking.

Example improvement:

-        cy.get(".t--add-datasource-button").first().click({ force: true });+        cy.get(".t--add-datasource-button").first().should("be.visible").should("be.enabled").click();

Also applies to: 97-97, 141-141, 145-145, 147-147, 175-175, 180-180

app/client/src/pages/workspace/index.tsx (2)

15-22:Avoid inline arrow functions incomponent prop.

Usingcomponent={() => <Component />} creates a new component instance on every render, causing unnecessary unmounting/remounting. Userender prop instead for inline JSX.

-      <SentryRoute-        component={() => (-          <PageWrapper displayName="Workspace Settings">-            <Settings />-          </PageWrapper>-        )}-        path={`${path}/:workspaceId/settings`}-      />+      <SentryRoute+        path={`${path}/:workspaceId/settings`}+        render={() => (+          <PageWrapper displayName="Workspace Settings">+            <Settings />+          </PageWrapper>+        )}+      />

Apply the same fix to the default route (lines 35-41).

Also applies to: 35-41

---

23-34:Consider consolidating duplicate routes.

Both/datasources and/datasource routes render identical components. If intentional for backward compatibility, add a comment. Otherwise, consolidate.

-      <SentryRoute-        path={`${path}/:workspaceId/datasources`}-        render={({ match }: { match: { params: { workspaceId: string } } }) => (-          <WorkspaceDatasourcesPage workspaceId={match.params.workspaceId} />-        )}-      />-      <SentryRoute-        path={`${path}/:workspaceId/datasource`}-        render={({ match }: { match: { params: { workspaceId: string } } }) => (-          <WorkspaceDatasourcesPage workspaceId={match.params.workspaceId} />-        )}-      />+      {/* /datasources for list view, /datasource/:id for editor - both use same page */}+      <SentryRoute+        path={[`${path}/:workspaceId/datasources`, `${path}/:workspaceId/datasource`]}+        render={({ match }: { match: { params: { workspaceId: string } } }) => (+          <WorkspaceDatasourcesPage workspaceId={match.params.workspaceId} />+        )}+      />

app/client/src/pages/workspace/WorkspaceDataSidePane.tsx (2)

122-135:Consider using Spinner for consistent loading UX.

WorkspaceDatasourcesPage uses<Spinner size="lg" /> during loading. This component shows plain text. Consider aligning for consistent UX.

---

169-201:Short-circuit EntityGroupsList when no datasources.

EntityGroupsList renders even whendatasources.length === 0, producing empty group containers. Short-circuit to avoid rendering empty structure.

       {datasources.length === 0 ? (          <EmptyState            button={blankStateButtonProps}            description={createMessage(DATASOURCE_LIST_BLANK_DESCRIPTION)}            icon={"datasource-v3"}          />-        ) : null}-        <EntityGroupsList+        ) : (+          <EntityGroupsList

And close the ternary properly with) after theEntityGroupsList block.

app/client/src/ce/entities/URLRedirect/URLAssembly.ts (2)

256-260:isWorkspaceContext() is called multiple times per build.

Consider caching the result withinbuild() to avoid repeatedwindow.location.pathname reads and regex matching.

 build(builderParams: URLBuilderParams, mode: APP_MODE = APP_MODE.EDIT) {+    const isWorkspace = this.isWorkspaceContext();     // ... use isWorkspace instead of this.isWorkspaceContext() throughout

---

262-264:Misleading parameter name ingenerateBasePathForWorkspace.

Parameter is namedworkspaceId but the caller passesentityId. While functionally correct in workspace context, consider renaming for clarity.

-  generateBasePathForWorkspace(workspaceId: string) {-    return `/workspace/${workspaceId}/datasources`;+  generateBasePathForWorkspace(entityId: string) {+    // In workspace context, entityId is the workspaceId+    return `/workspace/${entityId}/datasources`;   }