fix(connectors): connectors polish bundle (#1043) - #1080
Conversation
…1043 items 1,5) - Route the boolean-false connection check and the [id]/test catch path through classifyConnectionError; replace the dead-end 'Connection check returned false' with an actionable message + code. - Add mapPreviewError: map blocked-write driver errors (pg wrapped-write syntax error, Neo4j read-access-mode, read-only transaction) to a clear 'Writes aren't allowed from widget queries' message in the preview panel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s 2,3,4,6,8,9) - Edit dialog gains a Name field; rename persists via the existing PATCH (name) and fires a 'Connection updated' toast. - Create form is noValidate and reports all missing required fields at once (extracted missingRequiredConnectionFields) instead of native one-at-a-time tooltips. - Client-side URI format validation (validateConnectionUri) blocks save of malformed URIs on both create and edit, with an inline error. - Preview panel shows a 'Preview shows up to 25 rows' hint so the silent LIMIT is visible. - Connections list renders a distinct Neo4j / PostgreSQL logo via a new optional ConnectionCard `icon` prop (app passes the asset; library stays asset-free). Item 7 (stale-query auto-run on connection switch) was already prevented by clearQueryState on connector-type change — no code change needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 47 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR implements a comprehensive set of improvements to connection management, addressing validation, error messaging, UI/UX polish, and renaming support. Client-side form and URI validation are added to prevent invalid connections from being saved, shared error-classification helpers standardize test-route responses, and the connections UI gains rename support, connector-specific icons, toast confirmations, and improved preview error messaging for write-attempt detection. ChangesConnections Polish & Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
…1043) SonarCloud flagged 4.3% duplication on new code: the [id]/test and test-inline routes had near-identical false/catch handling. Extract connectionCheckFalseResult + connectionTestErrorResult so both routes build the result identically, with a direct unit test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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
🧹 Nitpick comments (4)
app/src/components/widget-editor/widget-preview-panel.tsx (1)
168-182: 💤 Low valueError mapping logic is correct; IIFE is slightly less readable.
The code correctly maps preview errors via
mapPreviewError, displays "Writes not allowed" for blocked-write attempts, and falls back to "Query failed" with the raw message otherwise. Optional chaining on line 170 safely handles null errors.The IIFE pattern works but extracting
writeMsgbefore the return (e.g.,const writeMsg = mapPreviewError(previewQuery.error?.message);) would be marginally clearer. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/widget-editor/widget-preview-panel.tsx` around lines 168 - 182, Extract the IIFE result into a local variable for clarity: call mapPreviewError(previewQuery.error?.message) once at the top of the component (e.g., const writeMsg = mapPreviewError(previewQuery.error?.message);) and then replace the immediately-invoked function expression with the JSX that uses writeMsg (the AlertCircle block that shows the title {writeMsg ? "Writes not allowed" : "Query failed"} and the message {writeMsg ?? previewQuery.error?.message}). This preserves the existing logic and optional chaining but removes the IIFE for improved readability.app/src/lib/query/preview-error.ts (2)
32-38: ⚡ Quick winThe phrase
"cannot execute"might be too broad and could match non-write errors.Line 37 includes
"cannot execute"to catch PostgreSQL read-only transaction errors like"cannot execute DELETE in a read-only transaction"(verified by the test on line 31). However, this phrase is generic and could match unrelated errors such as"cannot execute query: connection timeout"or"cannot execute query due to insufficient permissions", incorrectly mapping them to the write-not-allowed message.If false positives become an issue, consider narrowing the phrase to
"in a read-only transaction"or matching the full pattern"cannot execute .* in a read-only transaction".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/query/preview-error.ts` around lines 32 - 38, The READ_ONLY_PHRASES entry "cannot execute" is too broad and causes false positives; narrow it to target read-only transaction messages by replacing that entry with either the specific substring "in a read-only transaction" or a regex pattern like "cannot execute .* in a read-only transaction" and update the lookup logic that matches READ_ONLY_PHRASES (wherever READ_ONLY_PHRASES is used) to support regex matching if you choose the pattern approach so only messages explicitly about read-only transactions are caught.
19-30: 💤 Low valueTrailing spaces on
"set "and"remove "are trimmed away and provide no precision benefit.Lines 28-29 include trailing spaces on
"set "and"remove ", likely to avoid false matches. However, the regex on line 42 captures[a-z]+(no spaces), and line 44 applies.trim()to each keyword before comparison, so the trailing spaces are discarded. The intent might have been to distinguishSET(session config) from data writes, but the trim removes that distinction.Additionally,
SETin PostgreSQL is typically a session-level command (SET search_path = ...), not a data write. Wrapping it triggers a syntax error, but mapping it to "Writes aren't allowed" could be misleading since it's config, not data mutation.Consider either removing the trailing spaces for clarity or being more specific about which SET/REMOVE patterns constitute writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/query/preview-error.ts` around lines 19 - 30, The WRITE_KEYWORDS array contains entries "set " and "remove " whose trailing spaces are discarded by the existing regex capture ([a-z]+) and the subsequent .trim(), so remove the trailing spaces and make the match precise: update WRITE_KEYWORDS to use "set" and "remove" (no trailing spaces) and tighten the keyword extraction regex to use word boundaries (e.g., \b([a-z]+)\b) so keywords are matched as whole words; leave the existing .trim() in place or remove it if you rely on the word-boundary regex.app/src/lib/query/__tests__/preview-error.test.ts (1)
7-47: ⚡ Quick winTest coverage is solid for the main cases.
The suite covers the primary scenarios: PostgreSQL wrapped-write syntax errors (DELETE/UPDATE/INSERT), Neo4j read-access violations, PostgreSQL read-only transaction errors, and appropriate null returns for non-write and empty inputs.
For completeness, consider adding assertions for the other
WRITE_KEYWORDS(MERGE, CREATE, DROP, ALTER, TRUNCATE, SET, REMOVE) andREAD_ONLY_PHRASES("write operations are not allowed", "read-only transaction", "read only transaction"). Not critical since the logic is straightforward, but broader coverage would guard against future regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/query/__tests__/preview-error.test.ts` around lines 7 - 47, Add tests to preview-error.test.ts to assert that mapPreviewError returns PREVIEW_WRITE_NOT_ALLOWED_MESSAGE for the remaining WRITE_KEYWORDS (MERGE, CREATE, DROP, ALTER, TRUNCATE, SET, REMOVE) and for additional read-only phrases from READ_ONLY_PHRASES such as "write operations are not allowed", "read-only transaction", and "read only transaction"; keep using the same test style as existing cases (call mapPreviewError with the phrase and expect PREVIEW_WRITE_NOT_ALLOWED_MESSAGE) and reference the existing symbols mapPreviewError and PREVIEW_WRITE_NOT_ALLOWED_MESSAGE so the test suite covers these extra inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/app/`(dashboard)/connections/page.tsx:
- Around line 415-427: The validation and payload assembly use raw editForm
fields so whitespace-only values can be treated as truthy and overwrite stored
creds; before validating or building the PATCH payload, normalize (trim)
editForm.name, editForm.uri, editForm.username and editForm.password and use
those trimmed values for the checks and for buildEditConfig() so "blank keeps
existing" behavior is preserved—specifically, replace truthiness checks on
editForm.uri/username/password with checks on their trimmed counterparts, pass
the trimmed values into validateConnectionUri(editForm.uri, editTarget.type) and
into buildEditConfig() (or have buildEditConfig accept/derive trimmed values) so
whitespace-only inputs are ignored and not sent.
---
Nitpick comments:
In `@app/src/components/widget-editor/widget-preview-panel.tsx`:
- Around line 168-182: Extract the IIFE result into a local variable for
clarity: call mapPreviewError(previewQuery.error?.message) once at the top of
the component (e.g., const writeMsg =
mapPreviewError(previewQuery.error?.message);) and then replace the
immediately-invoked function expression with the JSX that uses writeMsg (the
AlertCircle block that shows the title {writeMsg ? "Writes not allowed" : "Query
failed"} and the message {writeMsg ?? previewQuery.error?.message}). This
preserves the existing logic and optional chaining but removes the IIFE for
improved readability.
In `@app/src/lib/query/__tests__/preview-error.test.ts`:
- Around line 7-47: Add tests to preview-error.test.ts to assert that
mapPreviewError returns PREVIEW_WRITE_NOT_ALLOWED_MESSAGE for the remaining
WRITE_KEYWORDS (MERGE, CREATE, DROP, ALTER, TRUNCATE, SET, REMOVE) and for
additional read-only phrases from READ_ONLY_PHRASES such as "write operations
are not allowed", "read-only transaction", and "read only transaction"; keep
using the same test style as existing cases (call mapPreviewError with the
phrase and expect PREVIEW_WRITE_NOT_ALLOWED_MESSAGE) and reference the existing
symbols mapPreviewError and PREVIEW_WRITE_NOT_ALLOWED_MESSAGE so the test suite
covers these extra inputs.
In `@app/src/lib/query/preview-error.ts`:
- Around line 32-38: The READ_ONLY_PHRASES entry "cannot execute" is too broad
and causes false positives; narrow it to target read-only transaction messages
by replacing that entry with either the specific substring "in a read-only
transaction" or a regex pattern like "cannot execute .* in a read-only
transaction" and update the lookup logic that matches READ_ONLY_PHRASES
(wherever READ_ONLY_PHRASES is used) to support regex matching if you choose the
pattern approach so only messages explicitly about read-only transactions are
caught.
- Around line 19-30: The WRITE_KEYWORDS array contains entries "set " and
"remove " whose trailing spaces are discarded by the existing regex capture
([a-z]+) and the subsequent .trim(), so remove the trailing spaces and make the
match precise: update WRITE_KEYWORDS to use "set" and "remove" (no trailing
spaces) and tighten the keyword extraction regex to use word boundaries (e.g.,
\b([a-z]+)\b) so keywords are matched as whole words; leave the existing .trim()
in place or remove it if you rely on the word-boundary regex.
🪄 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: 7db1ffb9-7fa1-4353-8d6f-add15005d6b2
📒 Files selected for processing (18)
app/e2e/connections.spec.tsapp/src/app/(dashboard)/connections/page.tsxapp/src/app/api/connections/[id]/test/__tests__/route.test.tsapp/src/app/api/connections/[id]/test/route.tsapp/src/app/api/connections/test-inline/__tests__/route.test.tsapp/src/app/api/connections/test-inline/route.tsapp/src/components/widget-editor/widget-preview-panel.tsxapp/src/lib/connector/__tests__/connection-form-validation.test.tsapp/src/lib/connector/__tests__/connection-test-result.test.tsapp/src/lib/connector/__tests__/validate-connection-uri.test.tsapp/src/lib/connector/connection-error-classifier.tsapp/src/lib/connector/connection-form-validation.tsapp/src/lib/connector/connection-test-result.tsapp/src/lib/connector/validate-connection-uri.tsapp/src/lib/query/__tests__/preview-error.test.tsapp/src/lib/query/preview-error.tscomponent/src/components/composed/__tests__/connection-card.test.tsxcomponent/src/components/composed/connection-card.tsx
Address CodeRabbit: with the form now noValidate, whitespace-only uri/username/password were truthy and could overwrite stored credentials with blanks. Gate buildEditConfig inclusion on the trimmed value so 'blank keeps existing' holds for whitespace-only input too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
2 participants
Closes #1043
Dogfood session 2 (#895) P2 connectors polish bundle — one change per checklist item.
Changes
[id]/testroute now classifies thrown driver errors (mirrorstest-inline), and both routes replace the dead-end "Connection check returned false" with an actionable message +codewhen the check returns false without throwing.mapPreviewErrormaps blocked-write driver errors (pg wrapped-writesyntax error at or near "DELETE", Neo4j "Writing in read access mode not allowed", read-only transaction) to a clear "Writes aren't allowed from widget queries" message in the preview panel.name) and fires a "Connection updated" toast.noValidateand reports all missing required fields at once (extractedmissingRequiredConnectionFields) instead of native one-at-a-time tooltips.validateConnectionUriblocks save of a malformed URI (e.g.not-a-uri) on create and edit with an inline error, before it can persist as an Error-badge connection.LIMIT.ConnectionCardiconprop (app passes the asset; the library stays asset-free).Tests
classifyConnectionErrorboolean-false + thrown-error classification (both test routes);mapPreviewError(pg/Neo4j/read-only/negatives);validateConnectionUri;missingRequiredConnectionFields. PATCH-name rename already covered.ConnectionCardrenders a custom connector-type icon.All app (169) + component unit suites, tsc, and root lint green.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests