Fix to put workspaceId in parameter to connect DB via UI#23
Merged
Conversation
📝 WalkthroughWalkthroughThreads workspaceId through frontend data-source components and types to enable workspace-scoped creation and listing; updates dialog and selector to call workspace-scoped APIs. Adds an end-to-end backend test that creates a temporary Postgres table, inserts rows, and verifies listing and querying via DataSourceService. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DocumentDetail as Document Detail
participant DataSourceSelector as DataSource Selector
participant DataSourceDialog as DataSource Dialog
participant API as Workspace API
participant Backend as Backend Service
User->>DocumentDetail: open document (has workspaceId)
DocumentDetail->>DataSourceSelector: pass workspaceId
User->>DataSourceSelector: click "New Data Source"
DataSourceSelector->>DataSourceDialog: open with workspaceId
User->>DataSourceDialog: configure + save
DataSourceDialog->>API: createWorkspaceDataSource(workspaceId, config)
API->>Backend: create data source scoped to workspace
Backend-->>API: created
API-->>DataSourceDialog: success
DataSourceDialog-->>DataSourceSelector: refresh list (workspace scope)
DataSourceSelector-->>User: show new data source
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 78.3s
Verification: verify:integrationResult: ✅ PASS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend/src/app/documents/document-detail.tsx (1)
450-457:⚠️ Potential issue | 🟠 MajorDon’t pass an empty workspace ID into the selector.
Falling back to
""turns “document workspace not loaded yet” into a real API parameter. If the user opens the datasource flow beforedocumentDataresolves, the dialog will try to create against/workspaces//datasourcesinstead of the document’s workspace.Suggested fix
- {showDsSelector && ( + {showDsSelector && documentData?.workspaceId && ( <Suspense fallback={null}> <DataSourceSelector - workspaceId={documentData?.workspaceId ?? ""} + workspaceId={documentData.workspaceId} open={showDsSelector} onOpenChange={setShowDsSelector} onSelect={addDataSourceTab} /> </Suspense> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/documents/document-detail.tsx` around lines 450 - 457, The DataSourceSelector is being passed an empty string when documentData is not yet loaded, causing API calls to use a malformed workspace ID; change the rendering or prop so you never pass "" into DataSourceSelector.workspaceId — e.g., compute const workspaceId = documentData?.workspaceId ?? undefined and only render the DataSourceSelector (or open it) when workspaceId is truthy, or gate the showDsSelector flag until documentData?.workspaceId exists; update references to DataSourceSelector, workspaceId, showDsSelector, and addDataSourceTab accordingly so the selector never receives an empty string.
🧹 Nitpick comments (1)
packages/backend/test/database.e2e-spec.ts (1)
119-141: Wrap raw client operations in try-finally to prevent connection leaks.If
rawClient.connect()succeeds but a subsequent query or assertion fails,rawClient.end()won't execute, leaving an orphaned connection. The same applies tocleanupClient. In CI environments with parallel tests, leaked connections can exhaust the pool and cause flakiness.♻️ Proposed fix using try-finally
const rawClient = new Client({ host: pgConfig.host, port: pgConfig.port, database: pgConfig.database, user: pgConfig.username, password: pgConfig.password, }); await rawClient.connect(); - await rawClient.query(` - CREATE TABLE IF NOT EXISTS _test_products ( - id SERIAL PRIMARY KEY, - name TEXT NOT NULL, - price NUMERIC(10, 2) NOT NULL - ) - `); - await rawClient.query(`TRUNCATE _test_products RESTART IDENTITY`); - await rawClient.query(` - INSERT INTO _test_products (name, price) VALUES - ('Widget', 9.99), - ('Gadget', 24.50), - ('Doohickey', 3.75) - `); - await rawClient.end(); + try { + await rawClient.query(` + CREATE TABLE IF NOT EXISTS _test_products ( + id SERIAL PRIMARY KEY, + name TEXT NOT NULL, + price NUMERIC(10, 2) NOT NULL + ) + `); + await rawClient.query(`TRUNCATE _test_products RESTART IDENTITY`); + await rawClient.query(` + INSERT INTO _test_products (name, price) VALUES + ('Widget', 9.99), + ('Gadget', 24.50), + ('Doohickey', 3.75) + `); + } finally { + await rawClient.end(); + }Apply the same pattern to the cleanup client (lines 178-187).
Also applies to: 177-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/test/database.e2e-spec.ts` around lines 119 - 141, The test creates a rawClient (new Client(...)) and performs connect/query/end sequences without ensuring end() runs on errors; wrap the rawClient.connect(), subsequent queries, and rawClient.end() in a try { ... } finally { await rawClient.end(); } block to guarantee the connection is always closed, and apply the same try-finally pattern to the cleanupClient usage (the cleanupClient connect/query/end block) so both rawClient and cleanupClient cannot leak connections if an assertion or query throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/frontend/src/app/datasources/datasource-list.tsx`:
- Around line 57-58: The component DataSourceList currently infers create scope
from the first data row via resolvedWorkspaceId = workspaceId ??
data[0]?.workspaceId ?? "", which can create in the wrong workspace or produce
/workspaces//datasources when data is empty; change the logic to only accept an
explicit workspace context (use workspaceId prop or null/undefined) and stop
falling back to data[0]?.workspaceId; update any code paths that open
DataSourceDialog or construct create URLs (references: DataSourceList,
DataSourceDialog, and the create button handlers around the other occurrence at
the same pattern) to require a non-empty workspaceId (disable the create button
or show a workspace selector when none is provided) and ensure you never
interpolate an empty string into /workspaces/{id}/datasources.
- Around line 57-58: The workspace-scoped datasource list isn't invalidated
after mutations; update the three mutation handlers in DataSourceList (the
delete handler, the onCreate callback, and the onSaved/onSave handler) to
invalidate both the global key ["datasources"] and the workspace-scoped key
["workspaces", resolvedWorkspaceId, "datasources"] (use the existing
resolvedWorkspaceId variable) so the workspace page is refreshed after
create/edit/delete; call your queryClient.invalidateQueries for both keys in
each handler.
In `@packages/frontend/src/components/datasource-selector.tsx`:
- Around line 17-18: The component DataSourceSelector (type
DataSourceSelectorProps) currently uses the global fetchDataSources() call which
ignores workspace scoping; replace that with the workspace-scoped query (e.g.,
fetchDataSourcesForWorkspace / the existing workspace-scoped hook/query) and
pass workspaceId into the query so the selector only loads datasources for the
current workspace; update any refetch/invalidate logic (including after creation
in DataSourceDialog) to use the same workspaceId-scoped query so creation and
selection operate on the same scope and the modal cannot show datasources from
other workspaces.
---
Outside diff comments:
In `@packages/frontend/src/app/documents/document-detail.tsx`:
- Around line 450-457: The DataSourceSelector is being passed an empty string
when documentData is not yet loaded, causing API calls to use a malformed
workspace ID; change the rendering or prop so you never pass "" into
DataSourceSelector.workspaceId — e.g., compute const workspaceId =
documentData?.workspaceId ?? undefined and only render the DataSourceSelector
(or open it) when workspaceId is truthy, or gate the showDsSelector flag until
documentData?.workspaceId exists; update references to DataSourceSelector,
workspaceId, showDsSelector, and addDataSourceTab accordingly so the selector
never receives an empty string.
---
Nitpick comments:
In `@packages/backend/test/database.e2e-spec.ts`:
- Around line 119-141: The test creates a rawClient (new Client(...)) and
performs connect/query/end sequences without ensuring end() runs on errors; wrap
the rawClient.connect(), subsequent queries, and rawClient.end() in a try { ...
} finally { await rawClient.end(); } block to guarantee the connection is always
closed, and apply the same try-finally pattern to the cleanupClient usage (the
cleanupClient connect/query/end block) so both rawClient and cleanupClient
cannot leak connections if an assertion or query throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0eef7165-e79c-4667-b686-a5a651c49683
📒 Files selected for processing (7)
packages/backend/test/database.e2e-spec.tspackages/frontend/src/app/datasources/datasource-list.tsxpackages/frontend/src/app/documents/document-detail.tsxpackages/frontend/src/app/workspaces/workspace-datasources.tsxpackages/frontend/src/components/datasource-dialog.tsxpackages/frontend/src/components/datasource-selector.tsxpackages/frontend/src/types/datasource.ts
Address CodeRabbit review feedback: stop inferring workspaceId from the first data row, use workspace-scoped fetch in DataSourceSelector, guard against empty workspaceId in document detail, invalidate both global and workspace-scoped query keys after mutations, and wrap raw pg clients in try-finally to prevent connection leaks in tests. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend/src/app/datasources/datasource-list.tsx (1)
305-311:⚠️ Potential issue | 🟠 MajorEmpty state button bypasses the workspaceId guard.
The header button (line 254) correctly disables when
!resolvedWorkspaceId, but this empty state button lacks the same guard. On the global datasources page, which rendersDataSourceListwithout passingworkspaceId, users with an empty list could click this button and openDataSourceDialogwithout a valid workspace context, leading to an API call to/workspaces/undefined/datasources.🐛 Proposed fix
<Button size="sm" onClick={() => setShowCreate(true)} + disabled={!resolvedWorkspaceId} > <Plus className="w-4 h-4 mr-1" /> New DataSource </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/datasources/datasource-list.tsx` around lines 305 - 311, The empty-state "New DataSource" Button in DataSourceList currently calls setShowCreate(true) without checking resolvedWorkspaceId, allowing the DataSourceDialog to open with an undefined workspace; update the Button (the one rendering <Plus ... /> and calling setShowCreate) to match the header button guard by disabling it when !resolvedWorkspaceId (add disabled={!resolvedWorkspaceId}) and/or wrap the onClick to no-op unless resolvedWorkspaceId is truthy (e.g., onClick={() => resolvedWorkspaceId && setShowCreate(true)}), ensuring DataSourceDialog creation is only possible when resolvedWorkspaceId exists.
🧹 Nitpick comments (1)
packages/frontend/src/app/datasources/datasource-list.tsx (1)
57-58: Good fix for past review concern - consider removing redundant variable.The previous issue of inferring workspaceId from
data[0]?.workspaceIdis properly addressed. However,resolvedWorkspaceId = workspaceIdis now a redundant assignment—you could useworkspaceIddirectly throughout the component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/datasources/datasource-list.tsx` around lines 57 - 58, The component DataSourceList contains a redundant assignment const resolvedWorkspaceId = workspaceId;—remove this variable and replace all uses of resolvedWorkspaceId with the existing workspaceId parameter (update any JSX/logic referencing resolvedWorkspaceId), keeping the function signature DataSourceList({ data, workspaceId }) unchanged so there is no behavioral change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/frontend/src/app/datasources/datasource-list.tsx`:
- Around line 305-311: The empty-state "New DataSource" Button in DataSourceList
currently calls setShowCreate(true) without checking resolvedWorkspaceId,
allowing the DataSourceDialog to open with an undefined workspace; update the
Button (the one rendering <Plus ... /> and calling setShowCreate) to match the
header button guard by disabling it when !resolvedWorkspaceId (add
disabled={!resolvedWorkspaceId}) and/or wrap the onClick to no-op unless
resolvedWorkspaceId is truthy (e.g., onClick={() => resolvedWorkspaceId &&
setShowCreate(true)}), ensuring DataSourceDialog creation is only possible when
resolvedWorkspaceId exists.
---
Nitpick comments:
In `@packages/frontend/src/app/datasources/datasource-list.tsx`:
- Around line 57-58: The component DataSourceList contains a redundant
assignment const resolvedWorkspaceId = workspaceId;—remove this variable and
replace all uses of resolvedWorkspaceId with the existing workspaceId parameter
(update any JSX/logic referencing resolvedWorkspaceId), keeping the function
signature DataSourceList({ data, workspaceId }) unchanged so there is no
behavioral change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f49617e1-f9e1-4100-82cb-b45280ff3fe2
📒 Files selected for processing (4)
packages/backend/test/database.e2e-spec.tspackages/frontend/src/app/datasources/datasource-list.tsxpackages/frontend/src/app/documents/document-detail.tsxpackages/frontend/src/components/datasource-selector.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/frontend/src/components/datasource-selector.tsx
- packages/backend/test/database.e2e-spec.ts
hackerwins
self-requested a review
March 6, 2026 15:40
hackerwins
approved these changes
Mar 6, 2026
hackerwins
left a comment
hackerwins
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for your contribution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix to put workspaceId in API parameter to connect PostgreSQL instance via UI
Why
Previously, it can not connect PostgreSQL instance via
Data Sourcestab UI with below error.Linked Issues
Fixes #
Verification
CI automatically posts a verification summary comment on this PR with
per-lane results for both
verify:selfandverify:integration.Skip reason (if applicable):
Risk Assessment
Notes for Reviewers
Summary by CodeRabbit
New Features
Tests