fix(connectors): clear query on type switch, pre-fill edit dialog - #345
Closed
alfredo1996 wants to merge 23 commits into
Closed
fix(connectors): clear query on type switch, pre-fill edit dialog#345alfredo1996 wants to merge 23 commits into
alfredo1996 wants to merge 23 commits into
Conversation
…ry are set (#315) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…or parameters..." (#316) When a widget has no connectionId, the query is disabled and TanStack Query returns isPending + idle. Previously this always showed "Waiting for parameters..." which was misleading. Now the idle state distinguishes three cases: missing connection, missing query, and genuine unresolved parameters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…313) Two root causes addressed: 1. NVL layout timeout — When the graph chart mounts inside a CSS-animated dialog (fullscreen expand), the container starts at ~0 size during the zoom-in-95 animation. NVL's force layout can fail to converge in this state and never fire onLayoutDone, leaving the loading spinner visible indefinitely. Added a safety timeout (800ms) that forces layoutReady if onLayoutDone hasn't fired, then calls fitGraph to re-center. 2. Zustand store conflict — The fullscreen dialog renders a second CardContainer for the same widget, creating two GraphExplorationWrapper instances that both read/write the same graph widget store slot. Added a widgetIdSuffix prop so the fullscreen instance uses a distinct store key (widget.id--fullscreen), preventing re-render cascades between the normal and fullscreen views. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…right testing agents Restored from git history (deleted in e8ce8f8): - CLAUDE.md — project conventions and architecture guide - .claude/agents/ — code-reviewer, code-simplifier, codebase-search, lint-fix, pr-check, pr-reviewer, project-architect, test-runner - .claude/skills/ — 16 skills (code, commit, components, drill, review, etc.) - .claude/hooks/ — 6 pre/post hooks (boundaries, coverage, credentials, etc.) - .claude/settings.json — permissions and hook configuration New additions: - .claude/agents/feature-reviewer.md — Playwright CLI-powered feature testing agent - .claude/agents/ux-crawler.md — Playwright CLI-powered full-app UX audit agent - .gitignore updated to track .claude/ (except worktrees, plans, image-cache) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…date CLAUDE.md Removed (redundant with new Playwright agents or built-in Claude Code features): - code-simplifier agent (covered by /polish skill → also removed) - codebase-search agent (built-in Explore agent) - pr-check agent (covered by code-reviewer) - pr-reviewer agent (merged into code-reviewer) - screenshot-review skill (replaced by feature-reviewer agent) - ui-audit skill (replaced by ux-crawler agent) - polish skill (code-reviewer covers simplification) Updated: - code-reviewer: now runs tests, recommends feature-reviewer for UI changes - CLAUDE.md: added Agent Pipeline section documenting the develop→review→assess flow Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…gate Cover the 800ms safety timeout in GraphChart that prevents infinite loading when onLayoutDone never fires, and the widgetIdSuffix prop in CardContainer that prevents graph store conflicts between normal and fullscreen views. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…/1.0-integration-test
…1.0-integration-test
…-integration-test
…rough The "Loading…" text placeholder was briefly visible behind the rendered chart in the fullscreen dialog. Replace with a subtle spinner that doesn't compete visually with the chart content. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- user-sim-admin: power user session (dashboards, connections, users, settings, dark mode) - user-sim-creator: first-time user onboarding (learnability, guidance gaps, confusion points) Both produce structured UX friction reports with screenshots, severity ratings, and improvement suggestions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two agent personas tested the live app via Playwright CLI: - Admin power user (Alex): full feature session, 55 screenshots - First-time creator (Jordan): onboarding experience, 58 steps Combined findings: 5 P0 issues, 6 P1 issues, 7 P2 issues, 3 P3 items. Key gaps: login page context, sign-up loop, query persistence on connection switch, connection edit blank fields, missing onboarding. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, #326) - #325: Clear query, fields, and transforms when switching connection types (neo4j ↔ postgresql) in widget editor. Same-type switches preserve the query. - #326: GET /api/connections/[id] now returns decrypted config (sans password). Edit dialog pre-fills URI, username, database, and advanced settings. Password field is optional — omit to keep existing. Closes #325 Closes #326 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
added
bug
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThe PR pre-fills connection edit dialogs by fetching/decrypting stored config (password omitted), updates PATCH to merge existing passwords when omitted, makes password optional in schemas, clears widget-editor query state when connection type changes, and adds/updates tests and hooks/docs across the repo. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditDialog as Edit Dialog
participant API as /api/connections/[id]
participant DB as Database
participant Form as Connection Form
User->>EditDialog: Click Edit
EditDialog->>API: GET /api/connections/:id
API->>DB: SELECT configEncrypted, metadata
DB-->>API: Return row with configEncrypted
API->>API: decryptJson(configEncrypted)
API->>API: strip password from decrypted config
API-->>EditDialog: Return metadata + config (password omitted)
EditDialog->>Form: Pre-fill fields (uri, username, database, advanced settings)
Form-->>User: Display edit form (password empty)
sequenceDiagram
participant User
participant Editor as Widget Editor Modal
participant Store as Widget Editor Store
participant Prev as PrevConnectionInfo
User->>Editor: Select different connection
Editor->>Prev: Read previous connection type
Editor->>Editor: Compare new.type vs prev.type
alt Type changed
Editor->>Store: clearQueryState()
Store->>Store: set query = "", availableFields = [], transforms = []
Store-->>Editor: State updated
Editor-->>User: Query editor cleared / preview reset
else Type same
Editor-->>User: No query reset
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/app/`(dashboard)/connections/page.tsx:
- Around line 296-317: The fetch that prefills the form can race and overwrite
the wrong editor; guard or cancel it by tying the response to the active
connection ID or using an AbortController. Specifically, when calling
fetch(`/api/connections/${conn.id}`) ensure you only call setEditForm if the
current active conn.id still matches (e.g., compare to a ref/prop holding the
latest active connection ID) or abort any previous request before starting a new
one (use AbortController and pass its signal to fetch, abort on dialog close or
conn change). Update the logic around setEditForm, the fetch call, and any
dialog open/close handlers so stale responses cannot mutate the editForm state.
In `@app/src/app/api/connections/`[id]/route.ts:
- Around line 91-112: The code currently re-encrypts and writes finalConfig even
when no password is present, losing any previously stored credentials; update
the logic in the route handler where finalConfig is built (the block using
existing, decryptJson, and updates.configEncrypted) to: attempt to recover
prev.password via decryptJson(existing.configEncrypted) and only merge it into
finalConfig if prev.password exists; if the incoming finalConfig omits password
and no prev.password was recovered, return a 4xx error (or throw) instead of
proceeding; finally, only call encryptJson(finalConfig) and set
updates.configEncrypted when finalConfig.password is present (i.e., after
successful recovery or when provided).
- Around line 90-103: Before performing any mutation logic (including the
password-merge branch that uses finalConfig and the subsequent db.select on
connections), enforce the canWrite permission and short-circuit with a 403 if
false. Add a guard near the start of the PATCH handler in route.ts that checks
the existing canWrite/auth flag for the current userId/tenantId and returns
immediately if not allowed; ensure the same guard is present for the other
mutation block referenced (the code around where db.select({ configEncrypted:
connections.configEncrypted }) and the update flow run). This prevents read-only
owners from reaching the finalConfig merge, db.select, or update paths.
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 399-403: When detecting a connector type switch in the same branch
that calls useWidgetEditorStore.getState().clearQueryState() (i.e., where
prevConnection.type !== newConnection.type), also reset the preview payload so
the old columns/row aren't rebuilt: clear previewQuery.data and
initialPreviewData and reset availableFields and sampleRow in the widget editor
store (or add a new store method like clearPreviewState() and call it there).
Ensure this runs immediately in the same type-switch block so previewQuery.data
/ initialPreviewData can't repopulate availableFields/sampleRow after the
switch.
🪄 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: 94bcae99-a835-4a59-80fe-3ccc7ff36751
📒 Files selected for processing (7)
app/src/app/(dashboard)/connections/page.tsxapp/src/app/api/connections/[id]/__tests__/route.test.tsapp/src/app/api/connections/[id]/route.tsapp/src/components/widget-editor-modal.tsxapp/src/lib/schemas.tsapp/src/stores/__tests__/widget-editor-store.test.tsapp/src/stores/widget-editor-store.ts
Comment on lines
+296
to
+317
| // Fetch existing config (sans password) and pre-fill the form | ||
| try { | ||
| const res = await fetch(`/api/connections/${conn.id}`); | ||
| const body = await res.json(); | ||
| const config = body?.data?.config; | ||
| if (config) { | ||
| setEditForm((prev) => ({ | ||
| ...prev, | ||
| uri: config.uri ?? "", | ||
| username: config.username ?? "", | ||
| database: config.database ?? "", | ||
| connectionTimeout: config.connectionTimeout?.toString() ?? "", | ||
| queryTimeout: config.queryTimeout?.toString() ?? "", | ||
| maxPoolSize: config.maxPoolSize?.toString() ?? "", | ||
| connectionAcquisitionTimeout: | ||
| config.connectionAcquisitionTimeout?.toString() ?? "", | ||
| idleTimeout: config.idleTimeout?.toString() ?? "", | ||
| statementTimeout: config.statementTimeout?.toString() ?? "", | ||
| sslRejectUnauthorized: config.sslRejectUnauthorized, | ||
| })); | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
Ignore stale edit-prefill responses.
This fetch always applies its result to editForm, even if the dialog was closed, another connection was opened, or the user already started typing. A slow response for connection A can overwrite connection B's form and then save A's config into B. Guard the update with the active conn.id or cancel the previous request.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/app/`(dashboard)/connections/page.tsx around lines 296 - 317, The
fetch that prefills the form can race and overwrite the wrong editor; guard or
cancel it by tying the response to the active connection ID or using an
AbortController. Specifically, when calling fetch(`/api/connections/${conn.id}`)
ensure you only call setEditForm if the current active conn.id still matches
(e.g., compare to a ref/prop holding the latest active connection ID) or abort
any previous request before starting a new one (use AbortController and pass its
signal to fetch, abort on dialog close or conn change). Update the logic around
setEditForm, the fetch call, and any dialog open/close handlers so stale
responses cannot mutate the editForm state.
Comment on lines
+90
to
+103
| let finalConfig = result.data.config; | ||
| if (finalConfig && !finalConfig.password) { | ||
| // Password omitted — merge with existing encrypted config | ||
| const [existing] = await db | ||
| .select({ configEncrypted: connections.configEncrypted }) | ||
| .from(connections) | ||
| .where( | ||
| and( | ||
| eq(connections.id, id), | ||
| eq(connections.userId, userId), | ||
| eq(connections.tenantId, tenantId), | ||
| ), | ||
| ) | ||
| .limit(1); |
There was a problem hiding this comment.
Enforce canWrite before this PATCH path.
This handler still never checks canWrite, so a read-only user who owns the connection can hit the new select/update flow and modify credentials anyway. Gate the request before any of this mutation logic runs.
As per coding guidelines app/src/app/api/**: can_write enforced before any mutation.
Also applies to: 114-123
🤖 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 90 - 103, Before
performing any mutation logic (including the password-merge branch that uses
finalConfig and the subsequent db.select on connections), enforce the canWrite
permission and short-circuit with a 403 if false. Add a guard near the start of
the PATCH handler in route.ts that checks the existing canWrite/auth flag for
the current userId/tenantId and returns immediately if not allowed; ensure the
same guard is present for the other mutation block referenced (the code around
where db.select({ configEncrypted: connections.configEncrypted }) and the update
flow run). This prevents read-only owners from reaching the finalConfig merge,
db.select, or update paths.
Comment on lines
+91
to
+112
| if (finalConfig && !finalConfig.password) { | ||
| // Password omitted — merge with existing encrypted config | ||
| const [existing] = await db | ||
| .select({ configEncrypted: connections.configEncrypted }) | ||
| .from(connections) | ||
| .where( | ||
| and( | ||
| eq(connections.id, id), | ||
| eq(connections.userId, userId), | ||
| eq(connections.tenantId, tenantId), | ||
| ), | ||
| ) | ||
| .limit(1); | ||
| if (existing?.configEncrypted) { | ||
| const prev = decryptJson<Record<string, unknown>>( | ||
| existing.configEncrypted, | ||
| ); | ||
| finalConfig = { ...finalConfig, password: prev.password as string }; | ||
| } | ||
| } | ||
|
|
||
| if (finalConfig) updates.configEncrypted = encryptJson(finalConfig); |
There was a problem hiding this comment.
Don't re-encrypt a config that still has no password.
If config.password is omitted and the existing row has no usable stored password, Line 112 still writes encryptJson(finalConfig) without credentials. That silently drops the saved password instead of preserving it; fail the request unless a previous password was actually recovered.
🤖 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 91 - 112, The code
currently re-encrypts and writes finalConfig even when no password is present,
losing any previously stored credentials; update the logic in the route handler
where finalConfig is built (the block using existing, decryptJson, and
updates.configEncrypted) to: attempt to recover prev.password via
decryptJson(existing.configEncrypted) and only merge it into finalConfig if
prev.password exists; if the incoming finalConfig omits password and no
prev.password was recovered, return a 4xx error (or throw) instead of
proceeding; finally, only call encryptJson(finalConfig) and set
updates.configEncrypted when finalConfig.password is present (i.e., after
successful recovery or when provided).
Comment on lines
+399
to
+403
| // Clear query state when switching between different connection types | ||
| // (e.g. neo4j → postgresql) since the query language is incompatible. | ||
| if (prevConnection && prevConnection.type !== newConnection.type) { | ||
| useWidgetEditorStore.getState().clearQueryState(); | ||
| } |
There was a problem hiding this comment.
Reset the preview payload when switching connector types.
Line 402 clears store state, but Lines 819-851 immediately rebuild availableFields and sampleRow from previewQuery.data / initialPreviewData. The old columns and preview stay visible after a type switch, so the editor can still be configured against the previous connector's schema.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/widget-editor-modal.tsx` around lines 399 - 403, When
detecting a connector type switch in the same branch that calls
useWidgetEditorStore.getState().clearQueryState() (i.e., where
prevConnection.type !== newConnection.type), also reset the preview payload so
the old columns/row aren't rebuilt: clear previewQuery.data and
initialPreviewData and reset availableFields and sampleRow in the widget editor
store (or add a new store method like clearPreviewState() and call it there).
Ensure this runs immediately in the same type-switch block so previewQuery.data
/ initialPreviewData can't repopulate availableFields/sampleRow after the
switch.
- api-keys.spec.ts: navigate to API Keys tab after Settings (sidebar now defaults to Profile tab) - connections.spec.ts: use seeded error connection instead of creating one, scope alert locator to card wrapper to avoid matching toast alerts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract parseOptionalInt to app/src/lib/parse-utils.ts with full unit tests - Add updateConnectionConfigSchema tests (optional password for edit) - Add PATCH route tests: password merge fallback, prefetchSchema conditionals - Add widget-editor-store tests: setConnectorChanged, loadFromWidget edge cases (parameter-select variants, form fields, cache settings, transforms, navigate click action, clickableColumns) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (12)
.claude/skills/release-plan/SKILL.md (1)
77-77: Make output generation deterministic and safe for first run.Line 77 assumes
claude_code_docs/exists and doesn’t define overwrite behavior. Specify “create directory if missing” and whether to overwrite vs append so repeated runs are predictable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/release-plan/SKILL.md at line 77, The save instruction "Save to `claude_code_docs/release-plan.md`" should be made deterministic: ensure the code that writes the output checks for and creates the directory `claude_code_docs` if it does not exist, and explicitly choose and document the write mode (e.g., overwrite the file by opening with truncate/write or append) so repeated runs behave predictably; update the logic that performs the save (the routine that writes release-plan.md) to create the directory when missing and to use a clear overwrite vs append policy and reflect that behavior in any comments or logs..claude/skills/drill/SKILL.md (1)
17-19: Add language specifier to fenced code block.The code block should specify
bashfor syntax highlighting and consistency with other skill files.Suggested fix
-``` +```bash gh issue view <number> --repo alfredo1996/neoboard</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.claude/skills/drill/SKILL.md around lines 17 - 19, Update the fenced code
block containing the gh command to include the bash language specifier for
syntax highlighting; locate the triple-backtick block that wraps "gh issue view
--repo alfredo1996/neoboard" and change the opening fence to "```bash"
so the snippet is rendered consistently with other skill files.</details> </blockquote></details> <details> <summary>.claude/agents/ux-crawler.md (1)</summary><blockquote> `151-151`: **Add language specifier to report format code block.** Use `markdown` for syntax highlighting. <details> <summary>Suggested fix</summary> ```diff -``` +```markdown ## NeoBoard UX Audit Report ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.claude/agents/ux-crawler.md at line 151, Update the code fence for the
report header to include a language specifier by changing the opening triple
backticks to ```markdown so the "## NeoBoard UX Audit Report" block uses
Markdown syntax highlighting; locate the code block containing the "## NeoBoard
UX Audit Report" heading in .claude/agents/ux-crawler.md and modify the opening
fence accordingly.</details> </blockquote></details> <details> <summary>.claude/skills/next/SKILL.md (1)</summary><blockquote> `36-36`: **Branch prefix mapping incomplete.** This line only maps 4 labels to branch prefixes, but the commit skill defines 8 commit types. Consider documenting the full mapping or defaulting unmapped labels to `chore/`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.claude/skills/next/SKILL.md at line 36, The documented branch-prefix mapping currently only shows "Branch prefix from labels: bug → fix/, enhancement → feat/, security → security/, docs → docs/." but the commit skill defines 8 commit types; update this line to include mappings for all commit types (e.g., add mappings for test → test/, refactor → refactor/, build → build/, chore → chore/ or whatever names your commit types use) or explicitly state a default behavior (e.g., "unmapped labels default to chore/"); ensure the SKILL.md entry and the exact "Branch prefix from labels: ..." string reflect the full mapping or the default rule so readers and automation have a single authoritative source. ``` </details> </blockquote></details> <details> <summary>app/e2e/connections.spec.ts (1)</summary><blockquote> `169-169`: **Fragile selector relies on CSS class naming.** `page.locator("[class*='cursor-pointer']")` is brittle — it depends on Tailwind class presence which could change. Consider using a more stable selector like `[data-testid="connection-card"]` if available, or filtering by a semantic attribute. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/e2e/connections.spec.ts` at line 169, The locator firstCard uses a brittle CSS-class-based selector (page.locator("[class*='cursor-pointer']")); replace it with a stable semantic selector such as page.locator('[data-testid="connection-card"]') or another purpose-built attribute (e.g., data-test or role) and, if necessary, add that data-testid to the component that renders connection cards so tests can reliably select the first connection card via firstCard = page.locator('[data-testid="connection-card"]').first(). ``` </details> </blockquote></details> <details> <summary>.claude/hooks/check-query-safety.sh (1)</summary><blockquote> `37-43`: **String concatenation check may also produce false positives.** The pattern `["\"][[:space:]]*\+[[:space:]]` could match legitimate cases like string constants being built from parts that don't involve user input. However, this is less likely to cause issues than the template literal check above. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.claude/hooks/check-query-safety.sh around lines 37 - 43, The current concatenation check (using NEW_CONTENT and QUERY_KEYWORDS) is too broad and can flag legitimate adjacent quoted strings; tighten the grep/regex so it only blocks cases where a quoted query fragment is concatenated with a non-quoted token (likely a variable/user input) rather than another quoted literal. Update the detection around QUERY_KEYWORDS to require a pattern like: a query keyword inside quotes followed by + and then a non-quote character/identifier (e.g., require \+ followed by [^"'\s] or ${/[$A-Za-z_]/}), or use a single Perl-compatible regex that ensures the right-hand side of the + is not a quoted string; apply this change to the block that reads NEW_CONTENT and exits with code 2 so only true concatenations with variables are blocked. ``` </details> </blockquote></details> <details> <summary>.claude/agents/code-reviewer.md (1)</summary><blockquote> `61-80`: **Add a language tag to the fenced output example** The code fence starting at Line 61 should declare a language (e.g., `markdown`) to satisfy linting and improve readability. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.claude/agents/code-reviewer.md around lines 61 - 80, The fenced code block
in the markdown example (the triple backticks that begin the "Code Review"
snippet) is missing a language tag; update that opening fence to include a
language identifier (for example changetomarkdown) so the example is
lint-compliant and renders with proper syntax highlighting in
.claude/agents/code-reviewer.md.</details> </blockquote></details> <details> <summary>.claude/agents/project-architect.md (1)</summary><blockquote> `50-100`: **Specify language for the output-format fenced block** At Line 50, add a language label (e.g., `markdown`) to the code fence to satisfy markdownlint and improve editor rendering. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.claude/agents/project-architect.md around lines 50 - 100, The fenced code
block starting at the "# Implementation Plan" header is missing a language
label; change the opening fence fromtomarkdown so markdownlint and
editors recognize it—update the code block that contains the Requirements
Summary / Implementation Steps / Testing Strategy content (the block under the
"# Implementation Plan" heading) to start with ```markdown and leave the closing
fence unchanged.</details> </blockquote></details> <details> <summary>.claude/hooks/session-context.sh (1)</summary><blockquote> `45-46`: **Use direct command status check instead of `$?`** Line 46 can be simplified and made safer by testing `gh pr view` directly in the `if` condition. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.claude/hooks/session-context.sh around lines 45 - 46, Replace the external `$?` check with a direct command-status test by running `gh pr view --json ...` inside the `if` condition and assigning its output to `PR_INFO` there; update the `if` that currently references `PR_INFO` and `$?` so it directly checks the command success and non-empty `PR_INFO` in one expression (referencing the `PR_INFO` variable, the `gh pr view` command, and the `if` conditional). ``` </details> </blockquote></details> <details> <summary>.claude/hooks/check-coverage.sh (1)</summary><blockquote> `27-35`: **Newline handling in `WARNING_MSG` may not render correctly.** Line 28 appends `\n` as a literal two-character string. When output via `printf '%s'`, these won't become actual newlines. Consider using actual newlines: <details> <summary>🔧 Suggested fix</summary> ```diff if [ "$INT_PCT" -lt 80 ] 2>/dev/null; then LOW_COVERAGE=true - WARNING_MSG="${WARNING_MSG} $(echo "$line" | xargs)\n" + WARNING_MSG="${WARNING_MSG} $(echo "$line" | xargs) +" fi ``` Or use `$'\n'` syntax for explicit newlines. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.claude/hooks/check-coverage.sh around lines 27 - 35, The WARNING_MSG is being built with the two-character sequence "\n" (see where WARNING_MSG is appended) so final printf won't render real newlines; update the append to add an actual newline (e.g., use $'\n' when concatenating or use printf -v to append a real newline) so WARNING_MSG contains real line breaks, and keep the final printf that emits the JSON payload intact (LOW_COVERAGE logic stays the same) so the output string from printf '{"hookSpecificOutput"...}' contains real newlines rather than literal backslash-n sequences. ``` </details> </blockquote></details> <details> <summary>.claude/agents/user-sim-creator.md (1)</summary><blockquote> `36-36`: **Test credentials appear in agent config.** `bob@example.com / password123` are hardcoded here. Ensure these are dev-only fixtures and never used in production. Consider referencing environment variables or a seed data doc instead. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.claude/agents/user-sim-creator.md at line 36, The README/fixture line containing the hardcoded credential "bob@example.com / password123" (the "Login as creator" example) must be converted to a dev-only fixture reference: remove the plaintext credential and instead reference an environment variable or seed-data entry (e.g., MDEV_CREATOR_EMAIL and MDEV_CREATOR_PASSWORD or a named seed "creator@example.dev" in your seed docs). Update the doc to show how to obtain these dev credentials (point to the seed data or .env example) and add a clear note that these values are for development only and must never be used in production. ``` </details> </blockquote></details> <details> <summary>app/src/components/__tests__/card-container-states.test.tsx (1)</summary><blockquote> `133-140`: **Consider using `toBeInTheDocument()` for clearer assertions.** The tests use `toBeDefined()` which works, but `toBeInTheDocument()` from `@testing-library/jest-dom` is the idiomatic choice and provides better error messages: ```diff -expect(screen.getByText("No connection configured")).toBeDefined(); +expect(screen.getByText("No connection configured")).toBeInTheDocument(); ``` This applies throughout the test file. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/src/components/__tests__/card-container-states.test.tsx` around lines 133 - 140, Replace assertions using toBeDefined() with the idiomatic toBeInTheDocument() (e.g., change expect(screen.getByText("No connection configured")).toBeDefined() and the other getByText assertion to .toBeInTheDocument()), and change the negative assertion using queryByText from .toBeNull() to .not.toBeInTheDocument() for clearer failure messages; ensure `@testing-library/jest-dom` matchers are available in this test run (import or confirm setupTests includes them) so toBeInTheDocument() is recognized. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/agents/feature-reviewer.md:
- Around line 104-125: The fenced code block in the feature review template is
missing a language identifier which triggers markdownlint; update the opening
fence (the triple-backticks that surround the template starting before "##
Feature Review: [Feature Name]") to include a language such as markdown or text
(e.g., changetomarkdown) so the block is lint-clean while preserving
the existing template content and formatting.In @.claude/agents/ux-crawler.md:
- Around line 49-56: The file exposes hardcoded test credentials in the Persona
blocks; remove the literal credentials under "Persona 1: Admin (full access)"
and "Persona 2: Creator (standard user)" and replace them with references to
environment variables or a shared test-fixtures source (e.g.,
ADMIN_EMAIL/ADMIN_PASSWORD and CREATOR_EMAIL/CREATOR_PASSWORD) so tests read
credentials from process.env or a fixtures loader. Update any documentation
lines that show concrete values to use placeholders and add a note pointing to
the test fixtures or env var names so maintainers know where to configure them.- Around line 17-43: The Playwright CLI invocation examples use the wrong
command (npx@playwright/cli``); update all examples to use the installed binary
nameplaywright-cliinstead (so replace occurrences like `npx `@playwright/cli`
open`, `npx `@playwright/cli` goto`, `npx `@playwright/cli` click`, etc. with
`playwright-cli open`, `playwright-cli goto`, `playwright-cli click`, and
likewise for `fill`, `type`, `select`, `hover`, `check`, `uncheck`,
`screenshot`, `snapshot`, `console`, `network`, `resize`, and `wait-for`) and
add a short note that the package should be installed globally (e.g., via npm)
if the global binary is required.In @.claude/hooks/check-credential-logging.sh:
- Line 25: The current check in the hook uses NEW_CONTENT and a one-line grep
that only detects credentials on the same line as console.* (if echo
"$NEW_CONTENT" | grep -iE "console.(log|warn|error|debug|info)" ...), which
allows multiline console(...) calls to bypass the guard; update the check to
normalize or join multiline console calls before testing against
CRED_PATTERN—e.g., collapse newlines inside parentheses or run grep with
NUL-aware matching (using grep -Pz or equivalent) so the pattern
"console.(log|warn|error|debug|info)\s*(" can match across lines, then test
the flattened/zero-terminated content against "${CRED_PATTERN}" using the same
flow that references NEW_CONTENT and CRED_PATTERN.In @.claude/hooks/check-query-safety.sh:
- Around line 23-35: The hook currently blocks any
${...}near SQL keywords by
scanning QUERY_KEYWORDS in NEW_CONTENT, which falsely flags Drizzle tagged
templates like sql...${...}; update the logic to skip/allow matches when the
interpolation is inside a known-safe tagged template by adding an allowlist
check before blocking: inspect NEW_CONTENT for tagged-template patterns such as
'\b(sql|mysql|sqlite|drizzle)\s*' that encompass the${...}and if found, do not treat it as unsafe (i.e., only echo the BLOCKED message and exit when a${...}` near QUERY_KEYWORDS is not inside one of the allowlisted tags).In @.claude/hooks/format-and-lint.sh:
- Around line 23-25: The lint commands fail when FILE_PATH is relative; update
the component and connection branches to run eslint with the correct path by
either cd'ing to the repo root and passing REL_PATH or by constructing the full
path (e.g., "$PROJECT_DIR/$REL_PATH") instead of "$FILE_PATH"; change both
occurrences where you currently do cd "$PROJECT_DIR/component" && npx eslint
--fix "$FILE_PATH" and cd "$PROJECT_DIR/connection" && npx eslint --fix
"$FILE_PATH" to use the package-scoped REL_PATH (or "$PROJECT_DIR/$REL_PATH") so
eslint receives a valid path for relative hook payloads.In @.claude/hooks/session-context.sh:
- Line 7: Guard the cd "$PROJECT_DIR" call so the script aborts if changing
directory fails: after attempting cd "$PROJECT_DIR" check its exit status and if
non-zero print an error to stderr (including the target path) and exit with a
non-zero code to prevent subsequent git commands from running in the wrong
directory.In @.claude/settings.json:
- Around line 4-23: The allowlist currently includes mutating Bash commands like
"Bash(git *)", "Bash(node *)", "Bash(cp *)", and "Bash(mv *)" but the branch,
boundary, query-safety, and credential-logging checks are only applied for
"Edit" and "Write" actions, so these Bash entries bypass safety hooks; modify
the .claude/settings.json rules so the same checks that run for Edit|Write are
also enforced for Bash mutating commands (either by adding Bash(...) to the
protected action groups or by applying the
branch/boundary/query-safety/credential-logging checks to the Bash entries),
ensuring commands like "Bash(git *)", "Bash(cp *)", "Bash/mv *)", and "Bash(node
*)" cannot execute without those validations.- Around line 9-16: The settings currently allow unrestricted reads via Read()
and Bash reader commands like Bash(cat ), Bash(grep ), Bash(head ), Bash(tail
), while the deny list only blocks write patterns (".env", ".pem", ".key",
"credentials"); update the deny list to also block read patterns (e.g., add
Read(.env), Read(.pem), Read(*.key), Read(credentials)) and tighten the Bash
allowlist by removing or constraining reader entries (remove or replace Bash(cat
*), Bash(grep *), Bash(head *), Bash(tail ) with safer patterns that restrict
arguments to non-secret paths) so secret files cannot be exfiltrated via Read()
or Bash readers.In @.claude/skills/commit/SKILL.md:
- Line 19: The commit types list in the SKILL.md line "Types: feat, fix, chore,
docs, refactor, test, perf, security" includes test and perf but the
github-workflow SKILL.md only defines branch prefixes for feat/, fix/, chore/,
docs/, refactor/, security/, causing a mismatch; update consistency by either
adding branch prefixes "test/" and "perf/" to the branch-prefixes list in
.claude/skills/github-workflow/SKILL.md or remove "test" and "perf" from the
Types line in .claude/skills/commit/SKILL.md so both files list the same set of
commit types.In @.claude/skills/design-review/skill.md:
- Around line 88-97: The CSS custom properties for the chart palette (--chart-1
through --chart-10) are malformed: variables are concatenated without semicolons
and proper line breaks. Fix by separating each declaration with a trailing
semicolon and place each --chart-N: hsl(...) with its comment on its own line
(or at least separated by semicolons) so the declarations parse correctly;
update the block that defines --chart-1 .. --chart-10 to use one declaration per
line with semicolons.In @.claude/skills/fix-pr-reviews/SKILL.md:
- Line 112: The GraphQL call is passing literal placeholders -f owner="{owner}"
and -f repo="{repo}" instead of actual values; compute OWNER and REPO (e.g. via
gh repo view --json owner -q .owner.login and --json name -q .name) into shell
variables and substitute them into the gh api/graphql invocation using -f
owner="$OWNER" -f repo="$REPO" so the GraphQL variables are populated
dynamically.- Around line 40-49: The GH CLI calls use literal "{owner}/{repo}" in the API
paths (e.g. the commands containing "gh api
repos/{owner}/{repo}/pulls/$ARGUMENTS/comments" and "gh api
repos/{owner}/{repo}/pulls/$ARGUMENTS/reviews"), which won't resolve
dynamically; change these to use a dynamically-resolved repo or GH CLI's
resolver (for example resolve REPO=$(gh repo view --json nameWithOwner -q
.nameWithOwner) and substitute $REPO in the gh api calls, or switch the paths to
use "gh api repos/:owner/:repo/..." so the CLI auto-resolves owner/repo) and
update all occurrences (including the "gh pr view $ARGUMENTS --comments --json
comments" section if it builds paths) to use the chosen dynamic approach.In @.claude/skills/next/SKILL.md:
- Around line 50-54: Update the workflow/docs so Step 5 uses a valid script:
either add a "lint:fix" script to package.json (script name "lint:fix") that
runs eslint with --fix, or replace the SKILL.md commandnpm run lint:fixwith
the existingnpm run lint -- --fix; ensure references to "lint:fix" in the
workflow/README are updated accordingly so the CI step no longer fails.In @.claude/skills/plan/SKILL.md:
- Around line 11-19: The policy text conflicts: it forbids writing
implementation code ("planning-only agent" / "NEVER write implementation code")
but simultaneously requires before/after code snippets; update the SKILL.md
policy language to resolve this by explicitly allowing only high-level
pseudocode or structural diffs for the requested "before/after code snippets"
(not runnable implementation), clarify the allowed granularity and formatting,
and replace the hard ban with a constrained rule referencing the phrases "NEVER
write implementation code" and "before/after code snippets" so readers know to
produce non-executable, descriptive diffs and examples only.In @.claude/skills/release-plan/SKILL.md:
- Around line 47-56: Add idempotency: before calling the gh API to create
milestones (the Step 4 gh api repos/{owner}/{repo}/milestones call) first query
existing milestones and match by title to skip creation if a milestone with the
same title exists; similarly for Step 5 where you call gh issue create, search
issues by normalized title and milestone (e.g., list issues in the target
milestone or use the GitHub search API) and only invoke gh issue create for
missing ones, ensuring title normalization and milestone-id matching to avoid
duplicates.
Nitpick comments:
In @.claude/agents/code-reviewer.md:
- Around line 61-80: The fenced code block in the markdown example (the triple
backticks that begin the "Code Review" snippet) is missing a language tag;
update that opening fence to include a language identifier (for example change
tomarkdown) so the example is lint-compliant and renders with proper
syntax highlighting in .claude/agents/code-reviewer.md.In @.claude/agents/project-architect.md:
- Around line 50-100: The fenced code block starting at the "# Implementation
Plan" header is missing a language label; change the opening fence from ``` tocontains the Requirements Summary / Implementation Steps / Testing Strategy content (the block under the "# Implementation Plan" heading) to start with ```markdown and leave the closing fence unchanged. In @.claude/agents/user-sim-creator.md: - Line 36: The README/fixture line containing the hardcoded credential "bob@example.com / password123" (the "Login as creator" example) must be converted to a dev-only fixture reference: remove the plaintext credential and instead reference an environment variable or seed-data entry (e.g., MDEV_CREATOR_EMAIL and MDEV_CREATOR_PASSWORD or a named seed "creator@example.dev" in your seed docs). Update the doc to show how to obtain these dev credentials (point to the seed data or .env example) and add a clear note that these values are for development only and must never be used in production. In @.claude/agents/ux-crawler.md: - Line 151: Update the code fence for the report header to include a language specifier by changing the opening triple backticks to ```markdown so the "## NeoBoard UX Audit Report" block uses Markdown syntax highlighting; locate the code block containing the "## NeoBoard UX Audit Report" heading in .claude/agents/ux-crawler.md and modify the opening fence accordingly. In @.claude/hooks/check-coverage.sh: - Around line 27-35: The WARNING_MSG is being built with the two-character sequence "\n" (see where WARNING_MSG is appended) so final printf won't render real newlines; update the append to add an actual newline (e.g., use $'\n' when concatenating or use printf -v to append a real newline) so WARNING_MSG contains real line breaks, and keep the final printf that emits the JSON payload intact (LOW_COVERAGE logic stays the same) so the output string from printf '{"hookSpecificOutput"...}' contains real newlines rather than literal backslash-n sequences. In @.claude/hooks/check-query-safety.sh: - Around line 37-43: The current concatenation check (using NEW_CONTENT and QUERY_KEYWORDS) is too broad and can flag legitimate adjacent quoted strings; tighten the grep/regex so it only blocks cases where a quoted query fragment is concatenated with a non-quoted token (likely a variable/user input) rather than another quoted literal. Update the detection around QUERY_KEYWORDS to require a pattern like: a query keyword inside quotes followed by + and then a non-quote character/identifier (e.g., require \+ followed by [^"'\s] or ${/[$A-Za-z_]/}), or use a single Perl-compatible regex that ensures the right-hand side of the + is not a quoted string; apply this change to the block that reads NEW_CONTENT and exits with code 2 so only true concatenations with variables are blocked. In @.claude/hooks/session-context.sh: - Around line 45-46: Replace the external `$?` check with a direct command-status test by running `gh pr view --json ...` inside the `if` condition and assigning its output to `PR_INFO` there; update the `if` that currently references `PR_INFO` and `$?` so it directly checks the command success and non-empty `PR_INFO` in one expression (referencing the `PR_INFO` variable, the `gh pr view` command, and the `if` conditional). In @.claude/skills/drill/SKILL.md: - Around line 17-19: Update the fenced code block containing the gh command to include the bash language specifier for syntax highlighting; locate the triple-backtick block that wraps "gh issue view <number> --repo alfredo1996/neoboard" and change the opening fence to "```bash" so the snippet is rendered consistently with other skill files. In @.claude/skills/next/SKILL.md: - Line 36: The documented branch-prefix mapping currently only shows "Branch prefix from labels: bug → fix/, enhancement → feat/, security → security/, docs → docs/." but the commit skill defines 8 commit types; update this line to include mappings for all commit types (e.g., add mappings for test → test/, refactor → refactor/, build → build/, chore → chore/ or whatever names your commit types use) or explicitly state a default behavior (e.g., "unmapped labels default to chore/"); ensure the SKILL.md entry and the exact "Branch prefix from labels: ..." string reflect the full mapping or the default rule so readers and automation have a single authoritative source. In @.claude/skills/release-plan/SKILL.md: - Line 77: The save instruction "Save to `claude_code_docs/release-plan.md`" should be made deterministic: ensure the code that writes the output checks for and creates the directory `claude_code_docs` if it does not exist, and explicitly choose and document the write mode (e.g., overwrite the file by opening with truncate/write or append) so repeated runs behave predictably; update the logic that performs the save (the routine that writes release-plan.md) to create the directory when missing and to use a clear overwrite vs append policy and reflect that behavior in any comments or logs. In `@app/e2e/connections.spec.ts`: - Line 169: The locator firstCard uses a brittle CSS-class-based selector (page.locator("[class*='cursor-pointer']")); replace it with a stable semantic selector such as page.locator('[data-testid="connection-card"]') or another purpose-built attribute (e.g., data-test or role) and, if necessary, add that data-testid to the component that renders connection cards so tests can reliably select the first connection card via firstCard = page.locator('[data-testid="connection-card"]').first(). In `@app/src/components/__tests__/card-container-states.test.tsx`: - Around line 133-140: Replace assertions using toBeDefined() with the idiomatic toBeInTheDocument() (e.g., change expect(screen.getByText("No connection configured")).toBeDefined() and the other getByText assertion to .toBeInTheDocument()), and change the negative assertion using queryByText from .toBeNull() to .not.toBeInTheDocument() for clearer failure messages; ensure `@testing-library/jest-dom` matchers are available in this test run (import or confirm setupTests includes them) so toBeInTheDocument() is recognized.🪄 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:
3a36ed3e-8259-4230-b899-3ab2e2ad396e📒 Files selected for processing (49)
.claude/.gitignore.claude/agents/code-reviewer.md.claude/agents/feature-reviewer.md.claude/agents/lint-fix.md.claude/agents/project-architect.md.claude/agents/test-runner.md.claude/agents/user-sim-admin.md.claude/agents/user-sim-creator.md.claude/agents/ux-crawler.md.claude/hooks/check-boundaries.sh.claude/hooks/check-coverage.sh.claude/hooks/check-credential-logging.sh.claude/hooks/check-query-safety.sh.claude/hooks/enforce-e2e.sh.claude/hooks/format-and-lint.sh.claude/hooks/session-context.sh.claude/settings.json.claude/skills/code/SKILL.md.claude/skills/commit/SKILL.md.claude/skills/components/SKILL.md.claude/skills/design-review/skill.md.claude/skills/drill/SKILL.md.claude/skills/fix-pr-reviews/SKILL.md.claude/skills/github-workflow/SKILL.md.claude/skills/harden/SKILL.md.claude/skills/issue/SKILL.md.claude/skills/next/SKILL.md.claude/skills/plan/SKILL.md.claude/skills/pr/SKILL.md.claude/skills/prioritize/SKILL.md.claude/skills/release-plan/SKILL.md.claude/skills/review/SKILL.md.claude/skills/test/SKILL.md.gitignoreCLAUDE.mdapp/e2e/api-keys.spec.tsapp/e2e/connections.spec.tsapp/src/components/__tests__/card-container-states.test.tsxapp/src/components/__tests__/card-container.test.tsxapp/src/components/card-container.tsxapp/src/components/dashboard-container.tsxapp/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/__tests__/query-editor-panel.test.tsxapp/src/components/widget-editor/query-editor-panel.tsxcomponent/src/charts/__tests__/graph-chart.test.tsxcomponent/src/charts/graph-chart.tsxcomponent/src/components/composed/app-shell.tsxcomponent/src/components/composed/dashboard-mini-preview.tsxdocs/ux-friction-report.md✅ Files skipped from review due to trivial changes (13)
- .claude/.gitignore
- component/src/components/composed/app-shell.tsx
- component/src/components/composed/dashboard-mini-preview.tsx
- .gitignore
- .claude/skills/issue/SKILL.md
- .claude/skills/github-workflow/SKILL.md
- .claude/skills/code/SKILL.md
- .claude/skills/components/SKILL.md
- .claude/agents/test-runner.md
- .claude/skills/pr/SKILL.md
- .claude/skills/test/SKILL.md
- .claude/skills/review/SKILL.md
- .claude/skills/prioritize/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/components/widget-editor-modal.tsx
Comment on lines
+104
to
+125
| ``` | ||
| ## Feature Review: [Feature Name] | ||
|
|
||
| ### Summary | ||
| [1-2 sentence verdict: pass/fail/needs-work] | ||
|
|
||
| ### Test Results | ||
| | # | Test Case | Result | Notes | | ||
| |---|-----------|--------|-------| | ||
| | 1 | Happy path: [description] | PASS/FAIL | [details] | | ||
| | 2 | Edge case: [description] | PASS/FAIL | [details] | | ||
| | ... | ... | ... | ... | | ||
|
|
||
| ### UX Issues | ||
| - [severity] [description] — [screenshot reference] | ||
|
|
||
| ### Screenshots | ||
| [Reference screenshots taken during testing] | ||
|
|
||
| ### Recommendations | ||
| - [Actionable improvement suggestions] | ||
| ``` |
There was a problem hiding this comment.
Add a language to the report template fence.
markdownlint is already warning on this block. Use markdown or text so the agent doc stays lint-clean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/feature-reviewer.md around lines 104 - 125, The fenced code
block in the feature review template is missing a language identifier which
triggers markdownlint; update the opening fence (the triple-backticks that
surround the template starting before "## Feature Review: [Feature Name]") to
include a language such as markdown or text (e.g., change ``` to ```markdown) so
the block is lint-clean while preserving the existing template content and
formatting.
Comment on lines
+17
to
+43
| You interact with the browser using the **Playwright CLI** (`npx @playwright/cli`). Key commands: | ||
|
|
||
| ```bash | ||
| # Session management | ||
| npx @playwright/cli open http://localhost:3000 # start browser | ||
| npx @playwright/cli goto <url> # navigate | ||
| npx @playwright/cli close # close browser | ||
|
|
||
| # Interactions | ||
| npx @playwright/cli click '<selector>' # click element | ||
| npx @playwright/cli fill '<selector>' '<text>' # fill input | ||
| npx @playwright/cli type '<text>' # type into focused element | ||
| npx @playwright/cli select '<selector>' '<value>' # select dropdown | ||
| npx @playwright/cli hover '<selector>' # hover element | ||
| npx @playwright/cli check '<selector>' # check checkbox | ||
| npx @playwright/cli uncheck '<selector>' # uncheck checkbox | ||
|
|
||
| # Inspection | ||
| npx @playwright/cli screenshot # capture screenshot | ||
| npx @playwright/cli snapshot # accessibility tree | ||
| npx @playwright/cli console # JS console messages | ||
| npx @playwright/cli network # network requests | ||
|
|
||
| # Browser state | ||
| npx @playwright/cli resize 1280 720 # set viewport | ||
| npx @playwright/cli wait-for '<selector>' # wait for element | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the correct package name and CLI invocation for Playwright browser automation CLI?
💡 Result:
The primary package for Playwright browser automation is 'playwright' (npm install playwright), which provides the core library for Node.js scripts automating browsers via code. After installation, use 'npx playwright install' to download browsers, and invoke CLI tools like 'npx playwright open URL' to launch a browser, 'npx playwright codegen URL' for recording scripts, or 'npx playwright test' for running tests. For the dedicated token-efficient CLI aimed at browser automation (especially for AI/coding agents), use '@playwright/cli' (npm install -g @playwright/cli@latest), invoked as 'playwright-cli open URL' for interactive automation commands like type, click, snapshot.
Citations:
- 1: https://www.npmjs.com/package/playwright
- 2: https://playwright.dev/docs/intro
- 3: https://playwright.dev/
- 4: https://github.com/microsoft/playwright-cli
- 5: https://playwright.dev/docs/test-cli
- 6: https://playwright.dev/docs/getting-started-cli
- 7: https://registry.npmjs.org/%40playwright%2Fcli
Correct the Playwright CLI invocation pattern.
The @playwright/cli package is valid, but the invocation in the code is incorrect. After installing with npm install -g @playwright/cli@latest, invoke commands as playwright-cli (not npx @playwright/cli``). For example: playwright-cli open http://localhost:3000 instead of `npx `@playwright/cli` open http://localhost:3000`.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/ux-crawler.md around lines 17 - 43, The Playwright CLI
invocation examples use the wrong command (`npx `@playwright/cli``); update all
examples to use the installed binary name `playwright-cli` instead (so replace
occurrences like `npx `@playwright/cli` open`, `npx `@playwright/cli` goto`, `npx
`@playwright/cli` click`, etc. with `playwright-cli open`, `playwright-cli goto`,
`playwright-cli click`, and likewise for `fill`, `type`, `select`, `hover`,
`check`, `uncheck`, `screenshot`, `snapshot`, `console`, `network`, `resize`,
and `wait-for`) and add a short note that the package should be installed
globally (e.g., via npm) if the global binary is required.
Comment on lines
+49
to
+56
| ### Persona 1: Admin (full access) | ||
|
|
||
| - Login: `admin@neoboard.local` / `admin123` | ||
| - Tests: Everything — user management, connections, settings, all dashboards | ||
|
|
||
| ### Persona 2: Creator (standard user) | ||
|
|
||
| - Login: `bob@example.com` / `password123` |
There was a problem hiding this comment.
Hardcoded credentials in skill file.
Test credentials are exposed in this file. While these appear to be dev/test credentials, consider referencing environment variables or a separate test fixtures file instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/ux-crawler.md around lines 49 - 56, The file exposes
hardcoded test credentials in the Persona blocks; remove the literal credentials
under "Persona 1: Admin (full access)" and "Persona 2: Creator (standard user)"
and replace them with references to environment variables or a shared
test-fixtures source (e.g., ADMIN_EMAIL/ADMIN_PASSWORD and
CREATOR_EMAIL/CREATOR_PASSWORD) so tests read credentials from process.env or a
fixtures loader. Update any documentation lines that show concrete values to use
placeholders and add a note pointing to the test fixtures or env var names so
maintainers know where to configure them.
| CRED_PATTERN='(password|passwd|secret|credential|apiKey|api_key|encryptionKey|encryption_key|decrypted|privateKey|private_key|accessToken|access_token|refreshToken|refresh_token)' | ||
|
|
||
| # Detect console.log/warn/error/debug containing credential identifiers | ||
| if echo "$NEW_CONTENT" | grep -iE "console\.(log|warn|error|debug|info)" | grep -qiE "${CRED_PATTERN}"; then |
There was a problem hiding this comment.
Credential logging guard can be bypassed by multiline console.*(...) calls
Line 25 only matches credentials on the same line as console.*. A call split across lines can slip through and log secrets.
Suggested fix
-# Detect console.log/warn/error/debug containing credential identifiers
-if echo "$NEW_CONTENT" | grep -iE "console\.(log|warn|error|debug|info)" | grep -qiE "${CRED_PATTERN}"; then
+# Detect console.log/warn/error/debug/info containing credential identifiers (including multiline calls)
+FLATTENED_CONTENT=$(printf '%s' "$NEW_CONTENT" | tr '\n' ' ')
+if echo "$FLATTENED_CONTENT" | grep -qiE "console\.(log|warn|error|debug|info)[^;]*${CRED_PATTERN}"; then
echo "BLOCKED: Detected logging of credential-related variable." >&2
echo "Rule: NEVER log decrypted credentials. Remove the log statement or redact sensitive data." >&2
exit 2
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/check-credential-logging.sh at line 25, The current check in
the hook uses NEW_CONTENT and a one-line grep that only detects credentials on
the same line as console.* (if echo "$NEW_CONTENT" | grep -iE
"console\.(log|warn|error|debug|info)" ...), which allows multiline console(...)
calls to bypass the guard; update the check to normalize or join multiline
console calls before testing against CRED_PATTERN—e.g., collapse newlines inside
parentheses or run grep with NUL-aware matching (using grep -Pz or equivalent)
so the pattern "console\.(log|warn|error|debug|info)\s*\(" can match across
lines, then test the flattened/zero-terminated content against "${CRED_PATTERN}"
using the same flow that references NEW_CONTENT and CRED_PATTERN.
Comment on lines
+23
to
+35
| # Detect template literals with interpolation that look like queries | ||
| # Check for SQL/Cypher keywords near ${...} interpolation | ||
| QUERY_KEYWORDS='(SELECT|INSERT|UPDATE|DELETE|MERGE|MATCH|CREATE|DROP|ALTER|CALL|RETURN|WITH|UNWIND)' | ||
| if echo "$NEW_CONTENT" | grep -qiE "${QUERY_KEYWORDS}" && echo "$NEW_CONTENT" | grep -qF '${'; then | ||
| # Confirm it's interpolation inside a template literal (backtick string), not just a standalone ${ | ||
| # Look for lines that have both a query keyword and ${...} pattern | ||
| if echo "$NEW_CONTENT" | grep -iE "${QUERY_KEYWORDS}" | grep -qF '${'; then | ||
| echo "BLOCKED: Detected string interpolation (\${...}) near a query keyword." >&2 | ||
| echo "Rule: ALWAYS use parameterized queries. NEVER interpolate user input into query strings." >&2 | ||
| echo "Use query parameters (\$1, \$2 for PostgreSQL or \$paramName for Neo4j) instead." >&2 | ||
| exit 2 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all Drizzle sql tagged template usages that would trigger this hook
rg -n --type=ts 'sql`[^`]*\$\{' app/src/app/api/ connection/src/Repository: alfredo1996/neoboard
Length of output: 327
🏁 Script executed:
#!/bin/bash
# Check the actual context around the flagged lines
echo "=== query/route.ts around line 142 ==="
sed -n '138,148p' app/src/app/api/query/route.ts
echo ""
echo "=== dashboards/route.ts around line 96 ==="
sed -n '92,102p' app/src/app/api/dashboards/route.ts
echo ""
echo "=== dashboards/route.ts around line 101 ==="
sed -n '97,107p' app/src/app/api/dashboards/route.tsRepository: alfredo1996/neoboard
Length of output: 1410
Hook will block legitimate Drizzle ORM parameterized queries.
The pattern matching ${ near SQL keywords will incorrectly flag Drizzle's safe tagged template literals. For example, app/src/app/api/query/route.ts:149 contains sql\...SELECT...${...}`` which is parameterized by the ORM framework, not dangerous string interpolation.
The hook's current logic cannot distinguish between unsafe patterns like `SELECT * WHERE id = ${id}` and safe ORM patterns like sql`...${column}...`.
Exclude Drizzle sql/mysql/sqlite tagged templates or add an allowlist for known-safe ORM constructs to avoid blocking legitimate code.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 26-26: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
[info] 29-29: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/check-query-safety.sh around lines 23 - 35, The hook currently
blocks any `${...}` near SQL keywords by scanning QUERY_KEYWORDS in NEW_CONTENT,
which falsely flags Drizzle tagged templates like sql`...${...}`; update the
logic to skip/allow matches when the interpolation is inside a known-safe tagged
template by adding an allowlist check before blocking: inspect NEW_CONTENT for
tagged-template patterns such as '\b(sql|mysql|sqlite|drizzle)\s*`' that
encompass the `${...}` and if found, do not treat it as unsafe (i.e., only echo
the BLOCKED message and exit when a `${...}` near QUERY_KEYWORDS is not inside
one of the allowlisted tags).
Comment on lines
+40
to
+49
| gh api repos/{owner}/{repo}/pulls/$ARGUMENTS/comments \ | ||
| --jq '[.[] | select(.user.login == "coderabbitai[bot]")]' | ||
|
|
||
| # Top-level PR comments | ||
| gh pr view $ARGUMENTS --comments --json comments \ | ||
| --jq '[.comments[] | select(.author.login == "coderabbitai[bot]")]' | ||
|
|
||
| # Review bodies | ||
| gh api repos/{owner}/{repo}/pulls/$ARGUMENTS/reviews \ | ||
| --jq '[.[] | select(.user.login == "coderabbitai[bot]")]' |
There was a problem hiding this comment.
Placeholders {owner}/{repo} need dynamic resolution.
The GitHub API paths use literal {owner}/{repo} placeholders. These should be resolved dynamically:
# Resolve owner/repo dynamically
REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner)
gh api repos/$REPO/pulls/$ARGUMENTS/comments ...Or use gh api repos/:owner/:repo/... which GitHub CLI auto-resolves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/fix-pr-reviews/SKILL.md around lines 40 - 49, The GH CLI
calls use literal "{owner}/{repo}" in the API paths (e.g. the commands
containing "gh api repos/{owner}/{repo}/pulls/$ARGUMENTS/comments" and "gh api
repos/{owner}/{repo}/pulls/$ARGUMENTS/reviews"), which won't resolve
dynamically; change these to use a dynamically-resolved repo or GH CLI's
resolver (for example resolve REPO=$(gh repo view --json nameWithOwner -q
.nameWithOwner) and substitute $REPO in the gh api calls, or switch the paths to
use "gh api repos/:owner/:repo/..." so the CLI auto-resolves owner/repo) and
update all occurrences (including the "gh pr view $ARGUMENTS --comments --json
comments" section if it builds paths) to use the chosen dynamic approach.
| } | ||
| } | ||
| } | ||
| ' -f owner="{owner}" -f repo="{repo}" -F number=$ARGUMENTS |
There was a problem hiding this comment.
GraphQL variables use literal placeholders.
Line 112 passes -f owner="{owner}" -f repo="{repo}" as literals. These need dynamic resolution:
OWNER=$(gh repo view --json owner -q .owner.login)
REPO=$(gh repo view --json name -q .name)
gh api graphql ... -f owner="$OWNER" -f repo="$REPO" ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/fix-pr-reviews/SKILL.md at line 112, The GraphQL call is
passing literal placeholders -f owner="{owner}" and -f repo="{repo}" instead of
actual values; compute OWNER and REPO (e.g. via gh repo view --json owner -q
.owner.login and --json name -q .name) into shell variables and substitute them
into the gh api/graphql invocation using -f owner="$OWNER" -f repo="$REPO" so
the GraphQL variables are populated dynamically.
Comment on lines
+50
to
+54
| ```bash | ||
| npm run lint:fix | ||
| npm run build | ||
| npm run test | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify lint:fix script existence
echo "Root package.json scripts:"
cat package.json | jq '.scripts | keys'
echo -e "\nSearching for lint:fix in any package.json:"
fd package.json --exec grep -l "lint:fix" {} \; 2>/dev/null || echo "No lint:fix found"Repository: alfredo1996/neoboard
Length of output: 336
npm run lint:fix script does not exist; workflow will fail at Step 5.
The root package.json defines lint but not lint:fix. Add the script to package.json:
"lint:fix": "eslint . --fix"Or use an existing script: npm run lint -- --fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/next/SKILL.md around lines 50 - 54, Update the workflow/docs
so Step 5 uses a valid script: either add a "lint:fix" script to package.json
(script name "lint:fix") that runs eslint with --fix, or replace the SKILL.md
command `npm run lint:fix` with the existing `npm run lint -- --fix`; ensure
references to "lint:fix" in the workflow/README are updated accordingly so the
CI step no longer fails.
Comment on lines
+11
to
+19
| You are a **planning-only** agent. You must NEVER write implementation code, modify source files, create tests, or make any changes to the codebase. Your ONLY job is to read, analyze, and produce a thorough written plan. | ||
|
|
||
| Use ultrathink. Analyze: requirements, architecture impact, security, scalability, dependencies. | ||
| Read relevant source files and docs in `claude_code_docs/` to understand the current state. | ||
|
|
||
| For each task in the plan, provide: | ||
|
|
||
| - The exact file(s) to modify and what to change (with code snippets showing the before/after) | ||
| - Why the change is needed |
There was a problem hiding this comment.
Planning-only rule conflicts with required before/after code snippets
Line 11 says “NEVER write implementation code,” but Line 18 asks for before/after code snippets. Please clarify that snippets must be high-level pseudocode or structural diffs only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/plan/SKILL.md around lines 11 - 19, The policy text
conflicts: it forbids writing implementation code ("planning-only agent" /
"NEVER write implementation code") but simultaneously requires before/after code
snippets; update the SKILL.md policy language to resolve this by explicitly
allowing only high-level pseudocode or structural diffs for the requested
"before/after code snippets" (not runnable implementation), clarify the allowed
granularity and formatting, and replace the hard ban with a constrained rule
referencing the phrases "NEVER write implementation code" and "before/after code
snippets" so readers know to produce non-executable, descriptive diffs and
examples only.
Comment on lines
+47
to
+56
| ## Step 4 — Create milestones on GitHub | ||
|
|
||
| ```bash | ||
| gh api repos/{owner}/{repo}/milestones -f title='v0.1 — Core Foundation' -f description='...' | ||
| ``` | ||
|
|
||
| ## Step 5 — Create issues on GitHub | ||
|
|
||
| For each issue, use `gh issue create` with title, body, labels, and milestone. | ||
| Add dependency notes in the body (e.g. 'Depends on #12'). |
There was a problem hiding this comment.
Add idempotency checks before creating milestones/issues.
Current steps create resources unconditionally. Re-running the skill will likely duplicate milestones/issues. Add a pre-check step (find existing milestone by title, search issue by normalized title+milestone) and only create missing ones.
Suggested doc update
## Step 4 — Create milestones on GitHub
+Before creating, check whether the milestone already exists (by exact title).
+If it exists, reuse it; if not, create it.
+
```bash
gh api repos/{owner}/{repo}/milestones -f title='v0.1 — Core Foundation' -f description='...'Step 5 — Create issues on GitHub
For each issue, use gh issue create with title, body, labels, and milestone.
Add dependency notes in the body (e.g. 'Depends on #12').
+Before creating, search for an existing issue with the same normalized title in the same milestone; skip create if found.
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
## Step 4 — Create milestones on GitHub
Before creating, check whether the milestone already exists (by exact title).
If it exists, reuse it; if not, create it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/release-plan/SKILL.md around lines 47 - 56, Add idempotency:
before calling the gh API to create milestones (the Step 4 gh api
repos/{owner}/{repo}/milestones call) first query existing milestones and match
by title to skip creation if a milestone with the same title exists; similarly
for Step 5 where you call gh issue create, search issues by normalized title and
milestone (e.g., list issues in the target milestone or use the GitHub search
API) and only invoke gh issue create for missing ones, ensuring title
normalization and milestone-id matching to avoid duplicates.
Extract mapConfigToEditForm utility from inline page logic and add tests. Add PATCH validation failure test for connections route. Brings new code coverage above 80% SonarCloud gate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Owner
Author
|
Merged into aggregated PR #349 (release/1.0-all-fixes) |
alfredo1996
pushed a commit
that referenced
this pull request
May 10, 2026
- Extract parseOptionalInt to app/src/lib/parse-utils.ts with full unit tests - Add updateConnectionConfigSchema tests (optional password for edit) - Add PATCH route tests: password merge fallback, prefetchSchema conditionals - Add widget-editor-store tests: setConnectorChanged, loadFromWidget edge cases (parameter-select variants, form fields, cache settings, transforms, navigate click action, clickableColumns) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
May 10, 2026
Extract mapConfigToEditForm utility from inline page logic and add tests. Add PATCH validation failure test for connections route. Brings new code coverage above 80% SonarCloud gate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
/api/connections/[id]now returns decrypted config (sans password). Edit dialog pre-fills URI, username, database, and all advanced settings. Password field is optional — leave blank to keep existing.Closes #325
Closes #326
Changes
widget-editor-store.ts— newclearQueryState()actionwidget-editor-modal.tsx—handleConnectionChangecompares connection types before clearingconnections/[id]/route.ts— GET returns decrypted config (no password), PATCH merges existing password when omittedschemas.ts—updateConnectionConfigSchemawith optional passwordconnections/page.tsx—openEditDialogfetches and pre-fills config, password placeholder hintTest plan
clearQueryState()store tests (2 new)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation