feat: Made changes to support datasources configuration page for workspace #41324
Conversation
… without needing an entity like applications.
WalkthroughAdds workspace-scoped IDE support: new IDE type and base paths, workspace URL builders and routes, Redux actions/reducer flags and sagas, a WorkspaceEditorEngine and workspace-aware URL assembly, plus several workspace-scoped datasource UI components, pages, and routing integrations. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant Page as WorkspaceDatasourcesPage
participant Store as Redux Store
participant Saga as Workspace IDE Saga
participant Engine as WorkspaceEditorEngine
participant API as Backend/API
User->>Browser: Navigate /workspace/:workspaceId/datasources
Browser->>Page: render
Page->>Store: dispatch initWorkspaceIDE(workspaceId)
Store->>Saga: INITIALIZE_WORKSPACE_IDE
Saga->>Engine: new WorkspaceEditorEngine()
Saga->>Engine: call setupEngine()
Engine->>Store: dispatch START_EVALUATION
Engine->>API: fetch environments, plugins, datasources (parallel)
API-->>Engine: responses
Engine->>Saga: completeChore() / throw error
Saga->>Store: dispatch INITIALIZE_WORKSPACE_IDE_SUCCESS or INITIALIZE_WORKSPACE_IDE_ERROR
Store->>Page: isWorkspaceEditorInitialized updated
Page->>Browser: render side pane & editor routes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tomjose92
changed the title
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/18841755423. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (14)
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 use
urlBuilder.build(). For consistency and to leverage any URL handling logic (encoding, normalization) thaturlBuilderprovides, 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.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
app/client/src/ce/IDE/Interfaces/IDETypes.ts(1 hunks)app/client/src/ce/IDE/constants/routes.ts(1 hunks)app/client/src/ce/RouteBuilder.ts(1 hunks)app/client/src/ce/actions/workspaceIDEActions.ts(1 hunks)app/client/src/ce/constants/ReduxActionConstants.tsx(2 hunks)app/client/src/ce/entities/Engine/WorkspaceEditorEngine.ts(1 hunks)app/client/src/ce/entities/URLRedirect/URLAssembly.ts(7 hunks)app/client/src/ce/pages/Applications/WorkspaceMenu.tsx(1 hunks)app/client/src/ce/reducers/uiReducers/editorReducer.tsx(3 hunks)app/client/src/ce/sagas/index.tsx(2 hunks)app/client/src/ce/sagas/workspaceIDESagas.ts(1 hunks)app/client/src/constants/routes/baseRoutes.ts(1 hunks)app/client/src/ee/actions/workspaceIDEActions.ts(1 hunks)app/client/src/ee/entities/Engine/WorkspaceEditorEngine.ts(1 hunks)app/client/src/ee/sagas/workspaceIDESagas.ts(1 hunks)app/client/src/pages/workspace/WorkspaceCreateNewDatasourceTab.tsx(1 hunks)app/client/src/pages/workspace/WorkspaceDataSidePane.tsx(1 hunks)app/client/src/pages/workspace/WorkspaceDatasourceEditor.tsx(1 hunks)app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx(1 hunks)app/client/src/pages/workspace/index.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
app/client/src/ce/actions/workspaceIDEActions.ts (1)
app/client/src/ce/constants/ReduxActionConstants.tsx (1)
ReduxActionTypes(1302-1346)
app/client/src/ce/IDE/constants/routes.ts (1)
app/client/src/ce/IDE/Interfaces/IDETypes.ts (1)
IDE_TYPE(1-6)
app/client/src/ce/sagas/workspaceIDESagas.ts (4)
app/client/src/actions/ReduxActionTypes.ts (1)
ReduxAction(9-12)app/client/src/ce/actions/workspaceIDEActions.ts (1)
InitWorkspaceIDEPayload(3-5)app/client/src/ce/entities/Engine/WorkspaceEditorEngine.ts (1)
WorkspaceEditorEngine(22-109)app/client/src/actions/initActions.ts (1)
resetEditorRequest(51-53)
app/client/src/ce/reducers/uiReducers/editorReducer.tsx (1)
app/client/src/ce/constants/ReduxActionConstants.tsx (1)
ReduxActionTypes(1302-1346)
app/client/src/pages/workspace/WorkspaceDatasourceEditor.tsx (1)
app/client/src/ce/actions/environmentAction.ts (1)
setCurrentEditingEnvironmentID(7-10)
app/client/src/pages/workspace/WorkspaceDataSidePane.tsx (6)
app/client/src/ce/selectors/entitiesSelector.ts (4)
getDatasources(86-88)getDatasourcesGroupedByPluginCategory(114-144)getPluginImages(605-623)getPlugins(90-91)app/client/src/ce/entities/FeatureFlag.ts (1)
FEATURE_FLAG(2-67)app/client/src/ce/RouteBuilder.ts (1)
workspaceDatasourceEditorURL(203-206)app/client/src/ce/utils/BusinessFeatures/permissionPageHelpers.tsx (1)
getHasCreateDatasourcePermission(56-62)app/client/src/ce/constants/messages.ts (3)
DATASOURCE_BLANK_STATE_CTA(2360-2360)DATA_PANE_TITLE(2356-2356)DATASOURCE_LIST_BLANK_DESCRIPTION(2357-2358)app/client/src/entities/Datasource/index.ts (1)
Datasource(154-162)
app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (5)
app/client/src/ce/selectors/entitiesSelector.ts (1)
getDatasourceUsageCountForApp(1769-1787)app/client/src/ce/IDE/Interfaces/IDETypes.ts (1)
IDE_TYPE(1-6)app/client/src/ce/actions/workspaceIDEActions.ts (1)
initWorkspaceIDE(7-10)app/client/src/pages/workspace/WorkspaceDataSidePane.tsx (1)
WorkspaceDataSidePane(52-197)app/client/src/components/SentryRoute.tsx (1)
SentryRoute(3-3)
app/client/src/ce/RouteBuilder.ts (1)
app/client/cypress/support/commands.js (1)
workspaceId(396-396)
app/client/src/ce/entities/URLRedirect/URLAssembly.ts (1)
app/client/cypress/support/commands.js (1)
workspaceId(396-396)
app/client/src/ce/entities/Engine/WorkspaceEditorEngine.ts (5)
app/client/src/ce/actions/environmentAction.ts (1)
fetchingEnvironmentConfigs(13-26)app/client/src/actions/ReduxActionTypes.ts (1)
ReduxAction(9-12)app/client/src/sagas/InitSagas.ts (1)
failFastApiCalls(147-167)app/client/src/entities/Engine/index.ts (2)
PluginsNotFoundError(61-61)PluginFormConfigsNotFoundError(62-62)app/client/src/actions/pluginActions.ts (1)
fetchPluginFormConfigs(22-29)
app/client/src/pages/workspace/index.tsx (2)
app/client/src/components/SentryRoute.tsx (1)
SentryRoute(3-3)app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (1)
WorkspaceDatasourcesPage(59-145)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
🔇 Additional comments (12)
app/client/src/ce/sagas/index.tsx (1)
60-60: Saga wiring looks good.
Placement alongside workspaceSagas is reasonable. No changes needed.Also applies to: 79-79
app/client/src/ce/entities/URLRedirect/URLAssembly.ts (1)
245-252: Workspace URL resolution needs refinement, but proposed fixes require design clarification.The concerns about empty workspaceId and context-coupled suffix rewriting are valid. However, the proposed diffs have structural issues that need addressing:
Confirmed issues:
resolveEntityIdForWorkspace()returns""instead of throwing—creates/workspace//datasourcesrisk- Suffix rewrite gates on
isWorkspaceContext()only, not explicitworkspaceIdintentIssues with proposed solution:
generateBasePath()receives onlybasePageIdandmode; diffs assume it receivesbuilderParams—misaligned with actual signature- No evidence of
datasource/suffix pattern in RouteBuilder (uses"datasource","datasources","datasource/{id}")- Diff E duplicates suffix logic; unclear whether
generateBasePath()should make the workspace vs. app decisionRecommended approach:
- Fix
resolveEntityIdForWorkspace()to throw on missing ID (align withresolveEntityIdForApp())- Clarify method responsibilities: Should
generateBasePath()be refactored to receivebuilderParamsand handle the workspace/app decision, or shouldbuild()handle routing logic?- Unify suffix handling: Gate on
!!builderParams.workspaceId || isWorkspaceContext()consistently—not just in one locationRedesign the routing layer to pass explicit intent (workspaceId) through method calls rather than relying on URL parsing alone.
app/client/src/constants/routes/baseRoutes.ts (1)
31-31: LGTM!The new workspace datasources route constant follows the established naming pattern and integrates cleanly with existing workspace routes.
app/client/src/ce/IDE/constants/routes.ts (1)
45-45: LGTM!Adding the Workspace IDE type to the IDEBasePaths mapping is consistent with the existing pattern for None and UIPackage types.
app/client/src/ee/actions/workspaceIDEActions.ts (1)
1-1: LGTM!Standard EE/CE facade pattern that allows for potential future extensions while maintaining a clean architecture boundary.
app/client/src/ee/entities/Engine/WorkspaceEditorEngine.ts (1)
1-1: LGTM!The EE facade for WorkspaceEditorEngine follows the established pattern and maintains clean architectural boundaries.
app/client/src/ee/sagas/workspaceIDESagas.ts (1)
1-2: LGTM!The EE saga facade follows the established pattern and properly re-exports both named and default exports from the CE layer.
app/client/src/ce/constants/ReduxActionConstants.tsx (2)
414-415: LGTM!The new workspace IDE action constants follow the established naming convention and are consistent with existing IDE initialization patterns (e.g., INITIALIZE_EDITOR).
483-483: LGTM!The error action constant is properly placed in IDEActionErrorTypes and follows the naming pattern.
app/client/src/pages/workspace/WorkspaceCreateNewDatasourceTab.tsx (1)
4-6: No issues found. CreateNewDatasourceTab properly accesses workspace context via Redux.The component is Redux-connected through
connect(mapStateToProps), which automatically injects workspace data (viagetCurrentAppWorkspace,getCurrentPageId, and other selectors) as props. The wrapper component correctly omits props since the child component obtains all required context from Redux state.app/client/src/ce/entities/Engine/WorkspaceEditorEngine.ts (1)
40-47: No action required—empty type is intentional.The
typeproperty in thefetchingEnvironmentConfigsaction creator is intentionally left as an empty string to imitate the functionality of the enterprise edition. The action properly propagates through sagas that handle environment configuration fetching. The code at lines 40–47 is correct as written.app/client/src/pages/workspace/WorkspaceDatasourceEditor.tsx (1)
5-5: Fix empty Redux action type in setCurrentEditingEnvironmentID.The action type is confirmed empty in
app/client/src/ce/actions/environmentAction.tsline 8. Redux actions require a non-emptytypeproperty for proper routing and reducer handling. Replace the empty string with a descriptive action type constant.⛔ Skipped due to learnings
Learnt from: AmanAgarwal041 PR: appsmithorg/appsmith#29266 File: app/client/src/ce/actions/environmentAction.ts:15-16 Timestamp: 2024-07-26T21:12:57.228Z Learning: The `type` property in the `fetchingEnvironmentConfigs` action creator is intentionally left as an empty string to imitate the functionality of the enterprise edition, as per the user's clarification.
| None: "None", | ||
| App: "App", | ||
| UIPackage: "UIPackage", | ||
| Workspace: "Workspace", |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM — new IDE type addition is consistent.
No concerns here. Please ensure downstream switch/route maps cover IDE_TYPE.Workspace.
Run to spot any missing cases:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts '\bIDE_TYPE\.Workspace\b|:\s*IDEType\b' -C2Length of output: 8283
---
🏁 Script executed:
```shell
#!/bin/bash
# Find switch statements handling IDE_TYPE
rg -nP --type=ts 'switch\s*\(\s*(type|ideType|.*IDE_TYPE)' -A 15 | head -100
Length of output: 8750
🏁 Script executed:
#!/bin/bash
# Find other enum-like mappings that use IDE_TYPE keys
rg -nP --type=ts '\[IDE_TYPE\.' -C 1 | head -150Length of output: 498
🏁 Script executed:
#!/bin/bash
# Check if there are any references to IDE_TYPE.Workspace that might be missing
rg -nP --type=ts 'IDE_TYPE\.Workspace' -C 3Length of output: 386
🏁 Script executed:
#!/bin/bash
# Get full getIDEFocusStrategy function to check if Workspace case is handled
cat app/client/src/ce/navigation/FocusStrategy/index.tsLength of output: 528
🏁 Script executed:
#!/bin/bash
# Search for getIDETypeByUrl to see if it handles Workspace
cat app/client/src/ce/entities/IDE/utils/getIDETypeByUrl.tsLength of output: 434
🏁 Script executed:
#!/bin/bash
# Search for EE FocusStrategy implementations
fd -t f 'FocusStrategy' -path '*/ee/*'Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check if UIPackageFocusStrategy or WorkspaceFocusStrategy exists
rg -nP --type=ts 'UIPackageFocusStrategy|WorkspaceFocusStrategy' -lLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find AppIDEFocusStrategy to understand implementation pattern
rg -nP --type=ts 'class AppIDEFocusStrategy|export.*AppIDEFocusStrategy' -C 3Length of output: 784
🏁 Script executed:
#!/bin/bash
# Search more broadly for strategy files in EE folder
find . -path "*/ee/*" -name "*FocusStrategy*" 2>/dev/null | head -20Length of output: 179
🏁 Script executed:
#!/bin/bash
# Check if there's an EE version of the focus strategy index
find . -path "*/ee/*" -name "index.ts" | xargs grep -l "getIDEFocusStrategy\|FocusStrategy" 2>/dev/null | head -10Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check the EE version of getIDEFocusStrategy
cat app/client/src/ee/navigation/FocusStrategy/index.tsLength of output: 44
🏁 Script executed:
#!/bin/bash
# Check if getIDEFocusStrategy is called with Workspace anywhere
rg -nP --type=ts 'getIDEFocusStrategy' -C 3Length of output: 2489
Add case for IDE_TYPE.Workspace in getIDEFocusStrategy or update default handler.
The switch statement in app/client/src/ce/navigation/FocusStrategy/index.ts only handles IDE_TYPE.None and IDE_TYPE.App, but IDE_TYPE.Workspace is now in IDEBasePaths. Since FocusRetentionSaga calls getIDEFocusStrategy(ideType) where ideType comes from getIDETypeByUrl(), this will throw an error if Workspace paths are configured.
Currently, IDE_TYPE.Workspace has an empty path array, so it won't trigger at runtime yet—but this is a latent bug waiting to happen.
🤖 Prompt for AI Agents
In app/client/src/ce/IDE/Interfaces/IDETypes.ts around line 5,
IDE_TYPE.Workspace was added but getIDEFocusStrategy (in
app/client/src/ce/navigation/FocusStrategy/index.ts) only handles IDE_TYPE.None
and IDE_TYPE.App; add a case for IDE_TYPE.Workspace to the switch in
getIDEFocusStrategy (or update the switch default to handle unknown/added IDE
types safely) so FocusRetentionSaga won't throw when getIDETypeByUrl() returns
Workspace; implement the Workspace case to return the appropriate focus strategy
(match App behavior or a no-op strategy) and ensure the default branch logs a
warning and returns a safe fallback strategy.
| [ReduxActionTypes.INITIALIZE_WORKSPACE_IDE]: (state: EditorReduxState) => { | ||
| return { ...state, isWorkspaceEditorInitialized: false }; | ||
| }, | ||
| [ReduxActionTypes.INITIALIZE_WORKSPACE_IDE_SUCCESS]: ( | ||
| state: EditorReduxState, | ||
| ) => { | ||
| return { ...state, isWorkspaceEditorInitialized: true }; | ||
| }, |
There was a problem hiding this comment.
Handle initialization error to avoid infinite spinner.
Add INITIALIZE_WORKSPACE_IDE_ERROR handler and store the error so UI can show a fallback.
[ReduxActionTypes.INITIALIZE_WORKSPACE_IDE]: (state: EditorReduxState) => {
return { ...state, isWorkspaceEditorInitialized: false };
},
[ReduxActionTypes.INITIALIZE_WORKSPACE_IDE_SUCCESS]: (
state: EditorReduxState,
) => {
return { ...state, isWorkspaceEditorInitialized: true };
},
+ [ReduxActionErrorTypes.INITIALIZE_WORKSPACE_IDE_ERROR]: (
+ state: EditorReduxState,
+ action: ReduxAction<{ error: unknown }>,
+ ) => {
+ return {
+ ...state,
+ isWorkspaceEditorInitialized: false,
+ workspaceEditorInitError: action.payload?.error,
+ };
+ },Additionally add the field to state shape and initial state:
// initialState
onLoadActionExecution: false,
+ workspaceEditorInitError: undefined, export interface EditorReduxState {
...
onLoadActionExecution?: boolean;
+ workspaceEditorInitError?: unknown;📝 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.
| [ReduxActionTypes.INITIALIZE_WORKSPACE_IDE]: (state: EditorReduxState) => { | |
| return { ...state, isWorkspaceEditorInitialized: false }; | |
| }, | |
| [ReduxActionTypes.INITIALIZE_WORKSPACE_IDE_SUCCESS]: ( | |
| state: EditorReduxState, | |
| ) => { | |
| return { ...state, isWorkspaceEditorInitialized: true }; | |
| }, | |
| [ReduxActionTypes.INITIALIZE_WORKSPACE_IDE]: (state: EditorReduxState) => { | |
| return { ...state, isWorkspaceEditorInitialized: false }; | |
| }, | |
| [ReduxActionTypes.INITIALIZE_WORKSPACE_IDE_SUCCESS]: ( | |
| state: EditorReduxState, | |
| ) => { | |
| return { ...state, isWorkspaceEditorInitialized: true }; | |
| }, | |
| [ReduxActionErrorTypes.INITIALIZE_WORKSPACE_IDE_ERROR]: ( | |
| state: EditorReduxState, | |
| action: ReduxAction<{ error: unknown }>, | |
| ) => { | |
| return { | |
| ...state, | |
| isWorkspaceEditorInitialized: false, | |
| workspaceEditorInitError: action.payload?.error, | |
| }; | |
| }, |
🤖 Prompt for AI Agents
In app/client/src/ce/reducers/uiReducers/editorReducer.tsx around lines 69 to
76, add a new ReduxActionTypes.INITIALIZE_WORKSPACE_IDE_ERROR case that sets
isWorkspaceEditorInitialized to false and stores the error payload on state
(e.g., workspaceInitError or initializeWorkspaceError) so the UI can stop the
spinner and show a fallback; also add that error field to the EditorReduxState
type/shape and initialize it in the reducer's initial state (null or undefined
by default), and ensure the SUCCESS and INITIALIZE handlers clear/reset the
error.
| yield put({ | ||
| type: ReduxActionErrorTypes.INITIALIZE_WORKSPACE_IDE_ERROR, | ||
| payload: { | ||
| error, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Surface error state for UI.
INITIALIZE_WORKSPACE_IDE_ERROR is dispatched, but there’s no reducer handling to unblock or show an error. Pair this with a reducer flag and UI fallback. See reducer comment for concrete diff.
🤖 Prompt for AI Agents
In app/client/src/ce/sagas/workspaceIDESagas.ts around lines 30 to 36, the saga
dispatches INITIALIZE_WORKSPACE_IDE_ERROR but the app has no reducer or state
flag to surface this error to the UI; add a reducer case and state flag (e.g.,
workspaceIDEInitializationError or isWorkspaceIDEInitError) to store the error
payload and clear any "initializing" flag, update the reducer's initial state
accordingly, and ensure components read this flag to show a fallback/error UI
and stop loading spinners; also add an action handler to clear the error when
retrying or on successful init.
| const addButtonClickHandler = useCallback(() => { | ||
| history.push(`/workspace/${workspaceId}/datasources/NEW`); | ||
| }, [workspaceId]); | ||
|
|
There was a problem hiding this comment.
Fix route case: “NEW” → “new”.
Routes are case-sensitive. Align with WorkspaceDatasourcesPage.
- history.push(`/workspace/${workspaceId}/datasources/NEW`);
+ history.push(`/workspace/${workspaceId}/datasources/new`);Also update header button below (Line 148-151):
- onClick={() =>
- history.push(`/workspace/${workspaceId}/datasources/NEW`)
- }
+ onClick={() =>
+ history.push(`/workspace/${workspaceId}/datasources/new`)
+ }📝 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.
| const addButtonClickHandler = useCallback(() => { | |
| history.push(`/workspace/${workspaceId}/datasources/NEW`); | |
| }, [workspaceId]); | |
| const addButtonClickHandler = useCallback(() => { | |
| history.push(`/workspace/${workspaceId}/datasources/new`); | |
| }, [workspaceId]); |
🤖 Prompt for AI Agents
In app/client/src/pages/workspace/WorkspaceDataSidePane.tsx around lines 95 to
98, the route pushed uses an uppercase "NEW" which is case-sensitive and should
be "new"; change history.push(`/workspace/${workspaceId}/datasources/NEW`) to
use "new". Also update the header button route at lines ~148-151 to use "new"
instead of "NEW" so both places match WorkspaceDatasourcesPage routing.
| return ( | ||
| <DataSourceEditor | ||
| applicationId={workspaceApplicationId} | ||
| datasourceId={datasourceId} | ||
| isWorkspaceContext | ||
| pageId={workspacePageId} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify DataSourceEditor workspace mode assumptions.
We pass synthetic applicationId/pageId with isWorkspaceContext. Confirm DataSourceEditor suppresses app/page fetches in this mode.
🏁 Script executed:
#!/bin/bash
# Ensure DataSourceEditor gates app/page dependent fetches behind `isWorkspaceContext`.
rg -nP --type=ts --type=tsx -C3 '\bfunction\s+DataSourceEditor\b|\bconst\s+DataSourceEditor\b' app/client/src \
| sed -n '1,120p'
rg -nP --type=ts --type=tsx -C3 'isWorkspaceContext' app/client/src/pages/Editor/DataSourceEditor*Length of output: 55
🏁 Script executed:
#!/bin/bash
# Find DataSourceEditor component files
fd -e ts -e tsx 'DataSourceEditor' app/client/src
# Search for DataSourceEditor definition without type flag
rg -n 'function DataSourceEditor|const DataSourceEditor' app/client/src -A 5
# Search for isWorkspaceContext usage in Editor directory
rg -n 'isWorkspaceContext' app/client/src/pages/Editor -C 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Broader search for DataSource-related files
find app/client/src -type f -name "*DataSource*" 2>/dev/null | head -20
# List structure of app/client/src if it exists
ls -la app/client/src/ 2>/dev/null | head -20
# Search broadly for isWorkspaceContext across entire codebase
rg 'isWorkspaceContext' -n 2>/dev/null | head -30
# Search for any Editor directory
find app/client -type d -name "Editor" 2>/dev/nullLength of output: 2312
🏁 Script executed:
#!/bin/bash
# Check what WorkspaceDatasourceEditor imports and uses
cat app/client/src/pages/workspace/WorkspaceDatasourceEditor.tsx
# Search for component definitions accepting isWorkspaceContext
rg 'isWorkspaceContext' app/client/src/pages/Editor -B 5 -A 10
# Look for Datasource related components in Editor directories
find app/client/src/pages/Editor -type f \( -name "*.tsx" -o -name "*.ts" \) | xargs grep -l "Datasource" 2>/dev/nullLength of output: 5898
🏁 Script executed:
#!/bin/bash
# View DataSourceEditor component definition
cat app/client/src/pages/Editor/DataSourceEditor/index.tsx
# Search for isWorkspaceContext usage in DataSourceEditor directory
rg 'isWorkspaceContext' app/client/src/pages/Editor/DataSourceEditor -B 3 -A 3
# Look for useEffect and fetch patterns in the directory
rg 'useEffect|fetch|applicationId|pageId' app/client/src/pages/Editor/DataSourceEditor/index.tsx -nLength of output: 40922
DataSourceEditor must handle isWorkspaceContext to suppress app/page dependency fetches.
The isWorkspaceContext prop passed from WorkspaceDatasourceEditor is never declared in the component's ReduxStateProps interface and never checked. The component unconditionally calls getApplicationByIdFromWorkspaces(state, applicationId) and passes both applicationId and pageId to subforms without guarding based on workspace context. This means synthetic IDs intended to avoid unnecessary API calls are still processed as legitimate application/page references, potentially triggering fetches.
Add isWorkspaceContext to the ReduxStateProps interface, extract it in mapStateToProps, and conditionally skip app/page lookups when true. Alternatively, prevent the synthetic IDs from reaching selectors that trigger API calls by gating the affected logic.
| // Get the same Redux state that app editor components expect | ||
| const dsUsageMap = useSelector((state) => | ||
| getDatasourceUsageCountForApp(state, IDE_TYPE.App), | ||
| ); |
There was a problem hiding this comment.
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),
+);📝 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.
| // Get the same Redux state that app editor components expect | |
| const dsUsageMap = useSelector((state) => | |
| getDatasourceUsageCountForApp(state, IDE_TYPE.App), | |
| ); | |
| // Get the same Redux state that app editor components expect | |
| const dsUsageMap = useSelector((state) => | |
| getDatasourceUsageCountForApp(state, IDE_TYPE.Workspace), | |
| ); |
🤖 Prompt for AI Agents
In app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx around lines 70
to 73, the selector currently passes IDE_TYPE.App which causes usage labels to
read as “in this app”; update the call to pass IDE_TYPE.Workspace so the fetched
usage strings and labels reflect “in this workspace” instead. Ensure the
useSelector invocation imports/uses the IDE_TYPE enum and replace IDE_TYPE.App
with IDE_TYPE.Workspace so the UI displays workspace-scoped usage text.
| // Initialize workspace IDE once when workspaceId changes | ||
| useEffect(() => { | ||
| if (workspaceId && !isWorkspaceEditorInitialized) { | ||
| dispatch(initWorkspaceIDE({ workspaceId })); | ||
| } | ||
| }, [dispatch, workspaceId, isWorkspaceEditorInitialized]); |
There was a problem hiding this comment.
Re-initialize when workspaceId changes.
Current guard skips init if a previous workspace initialized the flag. Re-dispatch on workspace change.
-useEffect(() => {
- if (workspaceId && !isWorkspaceEditorInitialized) {
- dispatch(initWorkspaceIDE({ workspaceId }));
- }
-}, [dispatch, workspaceId, isWorkspaceEditorInitialized]);
+useEffect(() => {
+ if (workspaceId) {
+ dispatch(initWorkspaceIDE({ workspaceId }));
+ }
+}, [dispatch, workspaceId]);🤖 Prompt for AI Agents
In app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx around lines 81
to 86, the useEffect currently skips initializing the IDE when
isWorkspaceEditorInitialized is true from a previous workspace; change the logic
so initWorkspaceIDE is dispatched whenever workspaceId changes. Either track the
initialized workspaceId in state and only skip when it equals the current
workspaceId, or remove the isWorkspaceEditorInitialized guard and always
dispatch initWorkspaceIDE({ workspaceId }) when workspaceId is present; ensure
the effect dependencies include workspaceId and dispatch.
| <Switch> | ||
| {/* Create new datasource - use the exact same component */} | ||
| <SentryRoute | ||
| exact | ||
| path="/workspace/:workspaceId/datasources/new" | ||
| render={() => ( | ||
| <WorkspaceCreateNewDatasourceTab workspaceId={workspaceId} /> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
Route path case must match callers.
This route uses “/new” (lowercase). Calls elsewhere push “/NEW”. Standardize on lowercase “new”. See SidePane comment for fix.
🤖 Prompt for AI Agents
In app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx around lines 118
to 126 the route path is defined as "/workspace/:workspaceId/datasources/new"
(lowercase) but callers elsewhere use "/NEW"; update all callers (start with
SidePane where push/navigation uses "/NEW") to use the lowercase "/new" so the
pushed paths match this route exactly; search the repo for usages of "/NEW" and
replace them with "/new" (or refactor to a shared constant) and run the app to
verify navigation works.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/18842120808. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
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 passed
IDE_TYPE.App. Since this is a workspace context, passIDE_TYPE.Workspaceso 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
!isWorkspaceEditorInitializedguard 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.
🧹 Nitpick comments (2)
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 renders
WorkspaceCreateNewDatasourceTabwithout 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} />}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (5)
app/client/src/ce/selectors/entitiesSelector.ts (1)
getDatasourceUsageCountForApp(1769-1787)app/client/src/ce/IDE/Interfaces/IDETypes.ts (1)
IDE_TYPE(1-6)app/client/src/ce/actions/workspaceIDEActions.ts (1)
initWorkspaceIDE(7-10)app/client/src/pages/workspace/WorkspaceDataSidePane.tsx (1)
WorkspaceDataSidePane(52-197)app/client/src/components/SentryRoute.tsx (1)
SentryRoute(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
…r datasource configuration
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
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]);
🧹 Nitpick comments (6)
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():
- Determine
const 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".
- Constants
WORKSPACE_URLandWORKSPACE_DATASOURCES_PAGE_URLexist in baseRoutes and should be imported to avoid drift.- Add
workspaceDatasourceNewURLhelper to consolidate the three hardcoded/datasources/NEWstrings currently in WorkspaceDatasourcesPage.tsx and WorkspaceDataSidePane.tsx.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/ce/RouteBuilder.ts(1 hunks)app/client/src/ce/entities/URLRedirect/URLAssembly.ts(6 hunks)app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx(1 hunks)app/client/src/pages/workspace/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/workspace/index.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
app/client/src/ce/RouteBuilder.ts (1)
app/client/cypress/support/commands.js (1)
workspaceId(396-396)
app/client/src/ce/entities/URLRedirect/URLAssembly.ts (1)
app/client/cypress/support/commands.js (1)
workspaceId(396-396)
app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (5)
app/client/src/ce/selectors/entitiesSelector.ts (1)
getDatasourceUsageCountForApp(1769-1787)app/client/src/ce/IDE/Interfaces/IDETypes.ts (1)
IDE_TYPE(1-6)app/client/src/ce/actions/workspaceIDEActions.ts (1)
initWorkspaceIDE(7-10)app/client/src/pages/workspace/WorkspaceDataSidePane.tsx (1)
WorkspaceDataSidePane(52-197)app/client/src/components/SentryRoute.tsx (1)
SentryRoute(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
| resolveEntityIdForWorkspace(builderParams: URLBuilderParams): string { | ||
| // Extract workspaceId from current URL if not provided | ||
| if (builderParams?.workspaceId) { | ||
| return builderParams.workspaceId; | ||
| } | ||
|
|
||
| const currentUrl = window.location.pathname; | ||
| const workspaceMatch = currentUrl.match(/^\/workspace\/([^\/]+)/); | ||
|
|
||
| return workspaceMatch ? workspaceMatch[1] : ""; | ||
| } | ||
|
|
There was a problem hiding this comment.
Validate workspaceId; don’t fall back to empty string.
Returning "" yields “/workspace//…” URLs. Align with app path behavior and throw.
- const workspaceMatch = currentUrl.match(/^\/workspace\/([^\/]+)/);
-
- return workspaceMatch ? workspaceMatch[1] : "";
+ const workspaceMatch = currentUrl.match(/^\/workspace\/([^\/]+)/);
+ const id = workspaceMatch ? workspaceMatch[1] : "";
+ if (!id) {
+ throw new URIError("Missing workspaceId. Provide builderParams.workspaceId or navigate under /workspace/:workspaceId.");
+ }
+ return id;🤖 Prompt for AI Agents
In app/client/src/ce/entities/URLRedirect/URLAssembly.ts around lines 312 to
323, the resolver returns an empty string when no workspaceId is found which
produces malformed paths like "/workspace//…"; change the logic to validate and
require a workspaceId: if builderParams.workspaceId is present return it;
otherwise attempt to extract from window.location.pathname and if extraction
fails throw a clear Error (or assert) indicating missing workspaceId instead of
returning ""; ensure the thrown error message includes context (e.g., "Missing
workspaceId in URL or builderParams") so callers can handle or surface it.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/18848305560. |
|
Deploy-Preview-URL: https://ce-41324.dp.appsmith.com |
…components. Also handled the header for WorkspaceDatasources. Also hiding REST API and GRAPHQL API from datasources list on workspaces page
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (2)
75-78: Use Workspace context for usage strings.As previously noted, passing
IDE_TYPE.Appcauses usage labels to read "in this app". Update toIDE_TYPE.Workspaceso 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. The
isWorkspaceEditorInitializedflag 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]);
🧹 Nitpick comments (3)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (1)
330-335: Optional: Consider removing unnecessary optional chaining.The Plugin interface (from
entities/Plugin/index.ts) showspackageNameis a required field, so the optional chainingplugin?.packageNameis unnecessary. The variable namefinalFilteredMostPopularPluginscould 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 using
includes()will work for the expected paths but could theoretically match unintended URLs. For more robust path matching, consider using React Router'smatchPathor theuseLocationhook 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/newacross 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 in
WorkspaceDataSidePane.tsx:-history.push(`/workspace/${workspaceId}/datasources/NEW`); +history.push(`/workspace/${workspaceId}/datasources/new`);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/src/ce/pages/common/AppHeader.tsx(2 hunks)app/client/src/constants/routes/baseRoutes.ts(1 hunks)app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx(1 hunks)app/client/src/pages/workspace/WorkspaceDatasourceHeader.tsx(1 hunks)app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx(1 hunks)app/client/src/pages/workspace/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/constants/routes/baseRoutes.ts
🧰 Additional context used
🧬 Code graph analysis (6)
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (2)
app/client/src/pages/Editor/IntegrationEditor/util.ts (1)
filterSearch(11-20)app/client/src/ce/constants/messages.ts (1)
CREATE_NEW_DATASOURCE_REST_API(402-402)
app/client/src/pages/workspace/WorkspaceDatasourceHeader.tsx (2)
app/client/src/pages/Editor/AppsmithLink.tsx (1)
AppsmithLink(29-67)app/client/src/constants/routes/baseRoutes.ts (1)
APPLICATIONS_URL(8-8)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (1)
app/client/src/entities/Plugin/index.ts (1)
Plugin(71-87)
app/client/src/pages/workspace/index.tsx (3)
app/client/src/components/SentryRoute.tsx (1)
SentryRoute(3-3)app/client/src/pages/common/PageWrapper.tsx (1)
PageWrapper(63-89)app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (1)
WorkspaceDatasourcesPage(64-146)
app/client/src/ce/pages/common/AppHeader.tsx (1)
app/client/src/constants/routes/baseRoutes.ts (2)
WORKSPACE_DATASOURCES_PAGE_URL(31-31)WORKSPACE_DATASOURCE_EDITOR_PAGE_URL(32-32)
app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (6)
app/client/src/ce/selectors/entitiesSelector.ts (1)
getDatasourceUsageCountForApp(1769-1787)app/client/src/ce/IDE/Interfaces/IDETypes.ts (1)
IDE_TYPE(1-6)app/client/src/ce/actions/workspaceIDEActions.ts (1)
initWorkspaceIDE(7-10)app/client/src/pages/workspace/WorkspaceDataSidePane.tsx (1)
WorkspaceDataSidePane(52-197)app/client/src/components/SentryRoute.tsx (1)
SentryRoute(3-3)app/client/src/constants/routes/baseRoutes.ts (2)
WORKSPACE_DATASOURCES_PAGE_URL(31-31)WORKSPACE_DATASOURCE_EDITOR_PAGE_URL(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
🔇 Additional comments (15)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (1)
338-338: LGTM!The plugin selection correctly uses the workspace-filtered plugin list when displaying most popular plugins.
app/client/src/ce/pages/common/AppHeader.tsx (2)
19-20: LGTM!The new imports for workspace datasource routes and header component are correctly placed and follow the existing import patterns.
Also applies to: 25-25
49-56: LGTM!The new workspace datasource routes are properly integrated into the Switch statement and follow the existing routing patterns.
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (1)
373-386: LGTM!The visibility conditions correctly hide REST and GraphQL API creation options when on workspace datasources pages. The logic is consistently applied to both API types.
app/client/src/pages/workspace/WorkspaceDatasourceHeader.tsx (3)
1-7: LGTM!All imports are correctly placed and necessary for the component implementation.
9-16: LGTM!The fixed header styling is appropriate and uses the correct CSS variables for z-index and background color.
18-38: LGTM!The header component is well-structured and correctly implements the IDEHeader pattern with appropriate conditional rendering for the workspace name.
app/client/src/pages/workspace/index.tsx (4)
6-6: LGTM!The import for WorkspaceDatasourcesPage is correctly placed.
15-22: LGTM!The Settings route maintains the existing PageWrapper pattern and is correctly structured.
23-42: LGTM!The workspace datasources routes are correctly configured. Both routes properly extract
workspaceIdfrom route parameters and pass it toWorkspaceDatasourcesPage. The component handles its own layout, so not wrapping it inPageWrapperis appropriate.
43-49: LGTM!The default workspace page route maintains the existing PageWrapper pattern and is correctly positioned as the catch-all route.
app/client/src/pages/workspace/WorkspaceDatasourcesPage.tsx (4)
1-24: LGTM!All imports are appropriately organized and necessary for the component implementation.
27-58: LGTM!The styled components create an appropriate two-pane IDE layout that mirrors the AppIDE structure.
80-84: LGTM!The workspace selector correctly retrieves the current workspace by ID.
93-98: LGTM!The workspace fetching logic correctly ensures workspace data is loaded when needed.
| // Check if we're on workspace datasources page | ||
| const isWorkspaceDatasourcesPage = | ||
| window.location.pathname.includes("/workspace/") && | ||
| window.location.pathname.includes("/datasources"); |
There was a problem hiding this comment.
Fragile URL detection could match unintended paths.
The string inclusion checks includes("/workspace/") and includes("/datasources") will match unintended URLs like /workspaces/datasources, /foo/workspace/bar/datasources, or any path containing these substrings. Additionally, directly accessing window.location in mapStateToProps is inconsistent with the pattern used at line 258 where location.pathname is passed as a parameter.
Consider using the existing IDE type detection pattern or more precise path matching:
- // Check if we're on workspace datasources page
- const isWorkspaceDatasourcesPage =
- window.location.pathname.includes("/workspace/") &&
- window.location.pathname.includes("/datasources");
+ // Check if we're on workspace datasources page
+ const pathname = window.location.pathname;
+ const isWorkspaceDatasourcesPage =
+ /^\/workspace\/[^\/]+\/datasources/.test(pathname);Alternatively, leverage the IDE type detection if workspace has a distinct IDE type:
- // Check if we're on workspace datasources page
- const isWorkspaceDatasourcesPage =
- window.location.pathname.includes("/workspace/") &&
- window.location.pathname.includes("/datasources");
+ const ideType = getIDETypeByUrl(window.location.pathname);
+ const isWorkspaceDatasourcesPage =
+ ideType === IDEType.Workspace && window.location.pathname.endsWith("/datasources");📝 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.
| // Check if we're on workspace datasources page | |
| const isWorkspaceDatasourcesPage = | |
| window.location.pathname.includes("/workspace/") && | |
| window.location.pathname.includes("/datasources"); | |
| // Check if we're on workspace datasources page | |
| const pathname = window.location.pathname; | |
| const isWorkspaceDatasourcesPage = | |
| /^\/workspace\/[^\/]+\/datasources/.test(pathname); |
| // Check if we're on workspace datasources page | |
| const isWorkspaceDatasourcesPage = | |
| window.location.pathname.includes("/workspace/") && | |
| window.location.pathname.includes("/datasources"); | |
| const ideType = getIDETypeByUrl(window.location.pathname); | |
| const isWorkspaceDatasourcesPage = | |
| ideType === IDEType.Workspace && window.location.pathname.endsWith("/datasources"); |
🤖 Prompt for AI Agents
In app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx
around lines 318 to 321, the URL detection is fragile because it uses
window.location.pathname.includes(...) and can match unintended paths and also
contradicts the pattern at line 258 where location.pathname is passed in;
replace the window.location usage with the location.pathname parameter passed
into mapStateToProps (or accept it if not present), and perform a stricter match
by checking path segments or using a precise regex (e.g., match ^/workspace(/|$)
and /datasources(/|$) or split pathname and verify segments) or, better, use the
existing IDE/workspace type detection if workspace has a distinct IDE type;
update mapStateToProps signature to use the provided location and remove direct
window access.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
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 applies
padding-top: ${IDE_HEADER_HEIGHT}pxonPageContainerto offset the fixed header. However,WorkspaceDatasourceHeaderdoesn't explicitly set its own height property. For alignment consistency, addheight: ${IDE_HEADER_HEIGHT}px;toStyledWorkspaceHeaderto ensure the header and content spacing remain in sync ifIDE_HEADER_HEIGHTchanges.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/workspace/WorkspaceDatasourceHeader.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/client/src/pages/workspace/WorkspaceDatasourceHeader.tsx (2)
app/client/src/pages/Editor/AppsmithLink.tsx (1)
AppsmithLink(29-67)app/client/src/constants/routes/baseRoutes.ts (1)
APPLICATIONS_URL(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (1)
app/client/src/pages/workspace/WorkspaceDatasourceHeader.tsx (1)
1-7: LGTM!Imports are clean and all used.
…age, it takes the user to the workspace on the applications page.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/pages/AppIDE/layouts/components/Header/index.tsx (2)
165-168: Consider adding a defensive check forcurrentWorkspace.id.The code checks
currentWorkspace.nameon line 163 but directly accessescurrentWorkspace.idwithout validation. If the id is undefined, the URL becomes malformed (/applications?workspaceId=undefined).Consider either:
- Adding
currentWorkspace.idto the condition on line 163- 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 using
URLSearchParamsor a URL builder utility consistent with patterns elsewhere in the codebase (e.g.,viewerURLon line 144).Example with
URLSearchParams:const applicationsUrl = `${APPLICATIONS_URL}?${new URLSearchParams({ workspaceId: currentWorkspace.id }).toString()}`;Or define a URL builder function similar to other route builders in the codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/pages/AppIDE/layouts/components/Header/index.tsx(1 hunks)app/client/src/pages/workspace/WorkspaceDatasourceHeader.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/workspace/WorkspaceDatasourceHeader.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/client/src/pages/AppIDE/layouts/components/Header/index.tsx (1)
app/client/src/constants/routes/baseRoutes.ts (1)
APPLICATIONS_URL(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
1 participant
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 3f3d190 yet
Wed, 29 Oct 2025 10:28:20 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
UI Behavior