fix: address CodeRabbit + SonarCloud review issues - #594
Conversation
Security: enforce canWrite on reassign route, fix invalid 'owner' share role in SQL, wrap jsonb_agg with COALESCE for NULL safety, add WITH ORDINALITY to preserve page order, sanitize plugin codegen inputs. Bugs: extract render helpers in widget-preview-panel to reduce cognitive complexity from 50 to within limits (S3358 nested ternaries). Code smells: mark props Readonly (S6759), flip negated condition (S7735), remove unnecessary type assertions (S4325), evict cached connections on credential update/delete (#582). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughAPI routes implement credential caching eviction and authorization enforcement. Components refactor type declarations and render logic. Query execution removes unnecessary type casts. Plugin validation enhances input handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/app/api/connections/[id]/reassign/route.ts (1)
74-88:⚠️ Potential issue | 🔴 CriticalTighten target connection authorization.
For non-admins, the target lookup accepts any connection in the tenant. That can let a user reassign editable widgets to another user’s private connection if they know its ID. Require ownership/admin, or an explicit “shared/usable by this user” predicate, before accepting
targetConnectionId.🛡️ Minimal safer ownership check
- // Target must exist in the same tenant; owner doesn't have to match - // (admin can point at anyone's connection, and non-admins can point - // at a connection shared to them via a dashboard — we only enforce - // tenant isolation here). The type check below is the real safety. + // Target must exist and be usable by the caller. const [target] = await db .select({ id: connections.id, type: connections.type }) .from(connections) .where( - and( - eq(connections.id, targetConnectionId), - eq(connections.tenantId, tenantId), - ), + isAdmin + ? and( + eq(connections.id, targetConnectionId), + eq(connections.tenantId, tenantId), + ) + : and( + eq(connections.id, targetConnectionId), + eq(connections.userId, userId), + eq(connections.tenantId, tenantId), + ), )
🧹 Nitpick comments (3)
app/src/components/widget-editor/widget-preview-panel.tsx (2)
236-265:renderPreviewContentdispatcher reads cleanly.Flag precedence (
isMarkdown→isIframe→isParamSelect→isForm→ chart) matches the original inline conditional. No behavior drift.Minor nit (optional): defining
renderPreviewContentinside the component recreates the closure per render. Harmless here, but you could hoist it and pass the needed values as an argument object if you want parity with the other extracted helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/widget-preview-panel.tsx` around lines 236 - 265, renderPreviewContent recreates a closure on every render; to avoid that allocate it outside the component and pass a single props object containing all referenced values (isMarkdown, isIframe, isParamSelect, isForm, chartOptions, paramUIType, dateSub, multiSelect, paramWidgetName, seedPreviewOptions, seedQueryPending, seedQueryError, formFields, chartType, connectionId, query, title, colorScales, transforms, transformsEnabled, buildStylingConfig, previewQuery, initialPreviewData) and then call the appropriate helper (renderMarkdown, renderIframe, renderParamSelect, renderForm, renderChart) based on the flags so the function is stable across renders.
64-78: Consider a typedchartOptionsshape instead of repeatedas string | undefinedcasts.
renderMarkdown/renderIframerepeatedly narrow individual fields offRecord<string, unknown>. A small discriminated type (e.g.MarkdownOptions,IframeOptions) passed into these helpers would remove the per-field assertions and centralize the contract. Optional — not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/widget-preview-panel.tsx` around lines 64 - 78, The helpers renderMarkdown and renderIframe accept chartOptions: Record<string, unknown> and repeatedly cast fields to string|undefined; define explicit types (e.g. interface MarkdownOptions { content?: string } and interface IframeOptions { url?: string; iframeTitle?: string; sandbox?: string }) or a discriminated union, update renderMarkdown(chartOptions: MarkdownOptions) and renderIframe(chartOptions: IframeOptions) to use those types, remove the per-field "as string | undefined" assertions, and ensure callers pass objects conforming to the new types so MarkdownWidget and IframeWidget receive properly typed props.app/src/app/api/connections/[id]/route.ts (1)
231-260: Consolidate SELECT + DELETE via.returning()to avoid the extra round-trip and TOCTOU window.Drizzle supports
RETURNINGon DELETE, so you can fetchtypeandconfigEncryptedatomically with the delete and drop the precedingselect. Besides saving a round-trip, this eliminates the window where another request mutates/deletes the row between theSELECTand theDELETE.♻️ Proposed consolidation
- // Fetch credentials before deletion so we can evict the cached driver - const [toDelete] = await db - .select({ - type: connections.type, - configEncrypted: connections.configEncrypted, - }) - .from(connections) - .where(whereClause) - .limit(1); - const deleted = await db .delete(connections) .where(whereClause) - .returning({ id: connections.id }); + .returning({ + id: connections.id, + type: connections.type, + configEncrypted: connections.configEncrypted, + }); if (deleted.length === 0) { return notFound(); } + const [{ type, configEncrypted }] = deleted; // Evict the cached driver so the connection pool is closed - if (toDelete?.configEncrypted) { + if (configEncrypted) { try { - const creds = decryptJson<ConnectionCredentials>( - toDelete.configEncrypted, - ); - closeConnection(toDelete.type as ConnectorType, creds); + const creds = decryptJson<ConnectionCredentials>(configEncrypted); + closeConnection(type as ConnectorType, creds); } catch { // Corrupted credentials — nothing to evict } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/app/api/connections/`[id]/route.ts around lines 231 - 260, The current code does a separate select into toDelete then a delete, creating a TOCTOU window; instead use db.delete(connections).where(whereClause).returning({ id: connections.id, type: connections.type, configEncrypted: connections.configEncrypted }) to atomically get the deleted row(s), drop the prior select, assign the returned row to a variable (e.g. deletedRow or deleted[0]), check deleted.length === 0 and call notFound() if empty, then use deletedRow.configEncrypted with decryptJson<ConnectionCredentials> and closeConnection(deletedRow.type as ConnectorType, creds) inside the existing try/catch to evict the cached driver.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/lib/db/connection-reassign.ts`:
- Around line 132-140: The inner widget aggregation lacks a deterministic order:
update the nested jsonb_array_elements call and its jsonb_agg so widget order is
preserved by adding WITH ORDINALITY to jsonb_array_elements (aliasing the ord
column, e.g., widget_ord) and include ORDER BY widget_ord in the nested
jsonb_agg; adjust the FROM clause aliasing for
jsonb_array_elements(page->'widgets') AS widget to use WITH ORDINALITY and
reference that ordinal in the ORDER BY of the jsonb_agg that wraps the
CASE/jsonb_set (keeping the existing CASE and jsonb_set logic intact).
---
Nitpick comments:
In `@app/src/app/api/connections/`[id]/route.ts:
- Around line 231-260: The current code does a separate select into toDelete
then a delete, creating a TOCTOU window; instead use
db.delete(connections).where(whereClause).returning({ id: connections.id, type:
connections.type, configEncrypted: connections.configEncrypted }) to atomically
get the deleted row(s), drop the prior select, assign the returned row to a
variable (e.g. deletedRow or deleted[0]), check deleted.length === 0 and call
notFound() if empty, then use deletedRow.configEncrypted with
decryptJson<ConnectionCredentials> and closeConnection(deletedRow.type as
ConnectorType, creds) inside the existing try/catch to evict the cached driver.
In `@app/src/components/widget-editor/widget-preview-panel.tsx`:
- Around line 236-265: renderPreviewContent recreates a closure on every render;
to avoid that allocate it outside the component and pass a single props object
containing all referenced values (isMarkdown, isIframe, isParamSelect, isForm,
chartOptions, paramUIType, dateSub, multiSelect, paramWidgetName,
seedPreviewOptions, seedQueryPending, seedQueryError, formFields, chartType,
connectionId, query, title, colorScales, transforms, transformsEnabled,
buildStylingConfig, previewQuery, initialPreviewData) and then call the
appropriate helper (renderMarkdown, renderIframe, renderParamSelect, renderForm,
renderChart) based on the flags so the function is stable across renders.
- Around line 64-78: The helpers renderMarkdown and renderIframe accept
chartOptions: Record<string, unknown> and repeatedly cast fields to
string|undefined; define explicit types (e.g. interface MarkdownOptions {
content?: string } and interface IframeOptions { url?: string; iframeTitle?:
string; sandbox?: string }) or a discriminated union, update
renderMarkdown(chartOptions: MarkdownOptions) and renderIframe(chartOptions:
IframeOptions) to use those types, remove the per-field "as string | undefined"
assertions, and ensure callers pass objects conforming to the new types so
MarkdownWidget and IframeWidget receive properly typed props.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af68ba10-be3e-4291-9930-47926434568c
📒 Files selected for processing (13)
app/src/app/(dashboard)/connections/page.tsxapp/src/app/api/connections/[id]/__tests__/route.test.tsapp/src/app/api/connections/[id]/reassign/route.tsapp/src/app/api/connections/[id]/route.tsapp/src/app/api/query/route.tsapp/src/app/api/query/write/route.tsapp/src/components/widget-editor/template-browser.tsxapp/src/components/widget-editor/widget-preview-panel.tsxapp/src/lib/db/connection-reassign.tsapp/src/lib/log-anonymizer.tscli/src/commands/demo.tsscripts/__tests__/generate-plugin-imports.test.mjsscripts/generate-plugin-imports.mjs
| SELECT jsonb_agg( | ||
| CASE | ||
| WHEN widget->>'connectionId' = ${fromConnectionId} | ||
| THEN jsonb_set(widget, '{connectionId}', to_jsonb(${toConnectionId}::text)) | ||
| ELSE widget | ||
| END | ||
| ) | ||
| FROM jsonb_array_elements(page->'widgets') AS widget | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PostgreSQL jsonb_agg ORDER BY aggregate input order unspecified documentation
💡 Result:
Yes, the official PostgreSQL documentation explicitly states that the ordering of input values for jsonb_agg is unspecified by default. This applies across multiple versions, including the current documentation (version 18). The aggregate functions array_agg, json_agg, jsonb_agg, and similar functions produce meaningfully different result values depending on the order of the input values. "This ordering is unspecified by default, but can be controlled by writing an ORDER BY clause within the aggregate call" (as shown in Section 4.2.7). Alternatively, input values can be supplied from a sorted subquery, though this may not always work if the outer query reorders rows. To specify order: jsonb_agg(expression ORDER BY sort_expression). Sources: Official PostgreSQL docs for versions 10, 16, and current (18).
Citations:
- 1: https://www.postgresql.org/docs/10/functions-aggregate.html
- 2: https://www.postgresql.org/docs/current/interactive/functions-aggregate.html
- 3: https://www.postgresql.org/docs/16/functions-aggregate.html
🏁 Script executed:
# Check the file size first
wc -l app/src/lib/db/connection-reassign.tsRepository: alfredo1996/neoboard
Length of output: 105
🏁 Script executed:
# Read the relevant section and surrounding context
sed -n '120,150p' app/src/lib/db/connection-reassign.tsRepository: alfredo1996/neoboard
Length of output: 986
🏁 Script executed:
# Search for page_ord usage in this file
rg "page_ord|page->>" app/src/lib/db/connection-reassign.ts -A 2 -B 2Repository: alfredo1996/neoboard
Length of output: 237
🏁 Script executed:
# Check the schema or any migration files that define the page/widget structure
fd -i "migration\|schema" app/src --type f | head -20Repository: alfredo1996/neoboard
Length of output: 46
🏁 Script executed:
# Search for database schema or migration files
find app -type f \( -name "*.sql" -o -name "*migration*" -o -name "*schema*" \) | head -20Repository: alfredo1996/neoboard
Length of output: 770
🏁 Script executed:
# Search for other references to widget structure in the codebase
rg "widgets" app/src/lib/db -A 2 -B 2 --type ts | head -50Repository: alfredo1996/neoboard
Length of output: 3020
🏁 Script executed:
# Check if there's any documentation about the layoutJson structure
rg "layoutJson\|widget.*ord" app/src -A 1 -B 1 --type ts | head -30Repository: alfredo1996/neoboard
Length of output: 46
Add ORDER BY clause to inner jsonb_agg for widgets to preserve widget order within pages.
The outer jsonb_agg correctly uses WITH ORDINALITY and ORDER BY page_ord, but the nested jsonb_agg for widgets lacks ordering. PostgreSQL's jsonb_agg produces unspecified ordering without an explicit ORDER BY clause, which can cause widgets to be reordered during reassignment.
🐛 Proposed fix
- SELECT jsonb_agg(
+ SELECT jsonb_agg(
CASE
WHEN widget->>'connectionId' = ${fromConnectionId}
THEN jsonb_set(widget, '{connectionId}', to_jsonb(${toConnectionId}::text))
ELSE widget
END
+ ORDER BY widget_ord
)
- FROM jsonb_array_elements(page->'widgets') AS widget
+ FROM jsonb_array_elements(page->'widgets') WITH ORDINALITY AS w(widget, widget_ord)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/db/connection-reassign.ts` around lines 132 - 140, The inner
widget aggregation lacks a deterministic order: update the nested
jsonb_array_elements call and its jsonb_agg so widget order is preserved by
adding WITH ORDINALITY to jsonb_array_elements (aliasing the ord column, e.g.,
widget_ord) and include ORDER BY widget_ord in the nested jsonb_agg; adjust the
FROM clause aliasing for jsonb_array_elements(page->'widgets') AS widget to use
WITH ORDINALITY and reference that ordinal in the ORDER BY of the jsonb_agg that
wraps the CASE/jsonb_set (keeping the existing CASE and jsonb_set logic intact).
|
2 participants
Summary
canWriteon reassign route (feat(connections): re-assign widgets to a different connection (#510) #588), fix invalid'owner'share role to'editor'in SQL, wrapjsonb_aggwithCOALESCE(..., '[]'::jsonb)for NULL safety on empty widget arrays, addWITH ORDINALITYto preserve page orderJSON.stringifyfor import specifiers (feat(plugins): external chart plugin loading via manifest (#423) #589)widget-preview-panel.tsxto reduce cognitive complexity from 50 to within SonarCloud limits (fixes S3358 nested ternary issues)Readonly<>(S6759), flip negated condition (S7735), remove 4 unnecessary type assertions (S4325)closeConnectionwhen credentials are updated (PATCH) or connection is deleted (DELETE) to evict stale cached drivers (fix(hardening): TTL-based eviction for connection driver cache (#574) #582)Files changed (13)
app/src/app/api/connections/[id]/reassign/route.tscanWrite+forbiddenguardapp/src/lib/db/connection-reassign.ts'owner'role, COALESCE + ORDINALITYscripts/generate-plugin-imports.mjsJSON.stringifyspecifiersscripts/__tests__/generate-plugin-imports.test.mjsapp/src/components/widget-editor/widget-preview-panel.tsxapp/src/components/widget-editor/template-browser.tsxapp/src/app/(dashboard)/connections/page.tsxcli/src/commands/demo.ts!assertionapp/src/lib/log-anonymizer.tsascastapp/src/app/api/query/route.tsas DbTypeapp/src/app/api/query/write/route.tsas DbTypeapp/src/app/api/connections/[id]/route.tscloseConnectionon PATCH/DELETEapp/src/app/api/connections/[id]/__tests__/route.test.tscloseConnectionTest plan
npm run buildsucceeds with no type errorsnpm run lintshows no new errors🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores