fix: v1.0 bug fixes — CLI integration + 5 P0 issues (#350, #354, #355, #356, #357)

[alfredo1996]

Summary

• CLI integration into setup scripts: scripts/setup.sh and scripts/setup-local-demo.sh now delegate to the @neoboard/cli package (neoboard setup / neoboard demo) instead of raw shell commands. CLI init updated to install all package deps (app, component, connection).
• fix: sidebar name doesn't update after profile change #350 — Sidebar name not updating: JWT callback now re-fetches name from DB; session callback propagates it to session.user.name.
• fix: connector selection warning should only show on action, not by default #355 — Connector warning on modal open: Warning only shows when the user has written a query without selecting a connector, not on fresh open.
• fix: Widget Lab preview tab loops indefinitely when opening template #354 — Widget Lab preview infinite loop: Moved allParamValues and previewQuery to refs inside handlePreview to break the callback/effect dependency cycle.
• fix: connector edit/create modal should show correct default values #357 — Connector modal wrong defaults: Edit mode shows a loading spinner while fetching config; "driver default" placeholders replaced with actual numbers (100, 60000).
• fix: query error handling — stop loading, meaningful message, better placement #356 — Query error handling: Full-width error banner replaced with inline icon + tooltip; api-client.ts now maps HTTP status codes to descriptive messages; useSeedQuery exposes error state.

Closes #350, #354, #355, #356, #357

Test plan

• App unit tests: 1816 passed (118 files)
• CLI unit tests: 142 passed (20 files)
• Lint: no new errors (only pre-existing warnings)
• E2E: run npm run test:e2e to verify full flows
• Manual: verify sidebar name updates after profile change
• Manual: verify Widget Lab preview does not loop on template open
• Manual: verify connector modal shows numeric defaults, not "driver default"
• Manual: verify query error appears as tooltip, not banner

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • New CLI for install/dev/status/db/demo and delegated setup flows
  • Toggleable self-registration and force-password-change flows
  • Query templates, connection-selection warnings, improved widget-editor auto-preview and widget IDs
  • Settings redirect, user-role dropdown in user management, connection edit prefill (optional password)

• Bug Fixes

  • Clearer, status-aware API error messages and API-key failure feedback
  • Chart layout safety timeout to avoid stuck loading overlays
  • Improved preview/parameter/no-connection handling and UI robustness

• Tests

  • Large expansion of E2E and unit tests across UI and CLI

• Documentation

  • New UX friction report and project/dev process docs

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds Claude agent/skill/hook infra and CLAUDE.md; introduces a new TypeScript CLI (neoboard) with many commands/libs/tests; updates auth/registration bootstrap, connection credential handling, query-editor/widget-editor UX, Card/Dashboard component behavior, numerous tests, CI/Sonar, and repo housekeeping (.gitignore/.claude).

Changes

Cohort / File(s)	Summary
Claude infra .claude/.gitignore, .claude/settings.json, .claude/agents/*, .claude/skills/**/*, .claude/hooks/*	Add Claude configuration, agents, skills, and hook scripts (boundary/query/credential checks, format/lint, coverage, e2e enforcement). Review hook patterns for false-positives and workflow impact.
CLI package & tooling cli/package.json, cli/tsconfig.json, cli/vitest.config.ts, bin/neoboard, cli/src/**, cli/src/commands/*, cli/src/lib/*, cli/src/__tests__/**	Introduce @neoboard/cli with commands (init/start/stop/dev/setup/status/doctor/demo/env + db subcommands), core libs (exec/docker/config/health/output/ports/prompt), many tests, entrypoint and build scripts. Review CLI error handling, exec/docker safety, and public exports.
Auth / registration app/src/lib/auth/config.ts, app/src/app/api/auth/bootstrap-status/*, `app/src/app/(auth)/(login	signup)/*`, tests
Connections & schemas app/src/app/(dashboard)/connections/page.tsx, app/src/app/api/connections/[id]/route.ts, app/src/lib/schemas.ts, app/src/lib/parse-utils.ts, tests	Async edit-dialog prefill via GET decryptJson; PATCH merges stored password when omitted; add updateConnectionConfigSchema and parse/map helpers. Inspect decryption error flows and security of password-merge logic.
API keys app/src/app/api/keys/route.ts, tests	Wrap API-key generation in try/catch and return role-aware 503 messages on failure; ensure secrets are not leaked to non-admins.
Widget editor & query templates app/src/components/widget-editor-modal.tsx, app/src/components/widget-editor/query-editor-panel.tsx, app/src/lib/query-templates.ts, app/src/components/widget-editor/parameter-preview.tsx, related tests	Add language-aware templates, preview debounce/autorun controls, seed-query error propagation, chart-options reset refinements, and connection-type-aware clearing. Verify debounce timings and template-language mapping.
Card / Dashboard UX & IDs app/src/components/card-container.tsx, app/src/components/dashboard-container.tsx, app/src/components/**, tests	Add optional widgetIdSuffix → effectiveWidgetId, refine missing-connection/parameter messages, add double-click edit handling, fullscreen suffix usage, spinner UI, and remove onWidgetSettingsChange prop—audit callers for compatibility.
Store & hooks app/src/stores/widget-editor-store.ts, app/src/hooks/use-seed-query.ts, tests	Add clearQueryState() action; useSeedQuery now returns error alongside options/loading. Update callers to handle new return shape.
API client & error messages app/src/lib/api-client.ts, tests	Replace generic non-envelope HTTP errors with status-specific hint messages for selected codes (400/401/403/404/408/500/502/503/504). Confirm message suitability.
Tests, E2E & CI app/e2e/*.spec.ts, many app/src/**/__tests__/**, component/src/**/__tests__/**, cli/src/__tests__/**, .github/workflows/ci.yml, sonar-project.properties	Large additions/updates to Playwright and Vitest suites; include cli in CI/test matrix and Sonar coverage. Watch for flaky/serial tests and CI resource impact.
Components & charts component/src/charts/graph-chart.tsx, component/src/components/composed/chart-settings-panel.tsx, component/src/components/composed/dashboard-mini-preview.tsx	Add 800ms layout safety timeout in GraphChart, ChartSettingsPanel.resetKey to reset tabs, thumbnail sizing and dark-mode styling updates—ensure timer cleanup and tab-reset semantics.
Repo config & scripts CLAUDE.md, .gitignore, neoboard.config.json, .env.example, scripts/*, package.json	Add CLAUDE.md, refine .gitignore to keep .claude/ tracked except local caches, document REGISTRATION_ENABLED, include CLI in scripts and Sonar, and delegate setup scripts to the CLI — validate local bootstrap and CI integration.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CLI as "neoboard CLI"
  participant Docker as "Docker/Compose"
  participant Health as "Health probes"
  participant DB as "DB (Postgres/Neo4j)"

  User->>CLI: runStart()
  CLI->>CLI: runDoctor()
  CLI->>Docker: composeUp(full?)
  Docker-->>CLI: containers started
  CLI->>Health: waitForHealth(pg)
  Health-->>CLI: pg ready
  CLI->>Health: waitForHealth(neo4j)
  Health-->>CLI: neo4j ready
  CLI->>DB: runDbMigrate()
  DB-->>CLI: migrations applied
  CLI-->>User: print completion banner (URLs, success)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related issues

• #303 — CLI doctor/health: implements doctor and health utilities present in this PR (matches added cli/src/commands/doctor.ts and health helpers).
• #311 — DB dump: CLI db dump feature implemented (cli/src/commands/db/dump.ts) aligns with this issue.
• #304 — CLI env: env generate/validate/regenerate implemented (cli/src/commands/env.ts) addressing the issue.
• #309 — CLI demo/seed: CLI seeding and demo orchestration (cli/src/commands/db/seed.ts, cli/src/commands/demo.ts) correspond to the issue.

Possibly related PRs

• #127 — Query editor/templates: overlaps query template and editor changes (app/src/lib/query-templates.ts, query-editor panel).
• #132 — Widget/editor interactions: overlaps widget-editor preview, effective widget ID, and debounce logic.
• #119 — Bootstrap-status/api: related changes to bootstrap-status response and signup/login UI.

Suggested labels

chore, pkg:app, bug

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/v1.0-bug-fixes

[coderabbitai]

Actionable comments posted: 7

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

app/src/lib/auth/config.ts (1)

100-110: ⚠️ Potential issue | 🟠 Major

Add tenant scoping to JWT re-fetch query.

This query still fetches by users.id only. In a multi-tenant app, it must also filter by tenant to prevent cross-tenant data exposure.

Suggested fix

+ import { and, eq } from "drizzle-orm";
...
- .where(eq(users.id, token.id as string))
+ .where(
+   and(
+     eq(users.id, token.id as string),
+     eq(users.tenantId, (token.tenantId as string) ?? process.env.TENANT_ID ?? "default"),
+   ),
+ )

As per coding guidelines: "Multi-tenancy: every DB query must include tenant_id filter".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/lib/auth/config.ts` around lines 100 - 110, The JWT re-fetch query
selects user by users.id only and needs tenant scoping to prevent cross-tenant
access; update the query that uses db.select(...).from(users).where(eq(users.id,
token.id as string)).limit(1) to include the tenant filter (e.g. add an AND
condition comparing users.tenantId to the token's tenant id) so the WHERE clause
enforces both eq(users.id, token.id as string) and eq(users.tenantId,
token.tenantId as string); ensure token.tenantId is read safely and used in the
same query (symbols: db.select, users, token.id, token.tenantId).

app/src/app/api/auth/bootstrap-status/__tests__/route.test.ts (1)

21-27: ⚠️ Potential issue | 🟡 Minor

Harden env cleanup to avoid cross-test leakage.

Current per-test delete process.env.REGISTRATION_ENABLED cleanup is fragile if a test exits early. Restore from a captured original value in hooks.

Suggested patch

 describe("GET /api/auth/bootstrap-status", () => {
   // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
   let GET: () => Promise<any>;
+  const originalRegistrationEnabled = process.env.REGISTRATION_ENABLED;

   beforeEach(async () => {
     vi.resetModules();
     vi.clearAllMocks();
+    process.env.REGISTRATION_ENABLED = originalRegistrationEnabled;
     const mod = await import("../route");
     GET = mod.GET;
   });
+
+  afterEach(() => {
+    process.env.REGISTRATION_ENABLED = originalRegistrationEnabled;
+  });

Also applies to: 46-107

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/app/api/auth/bootstrap-status/__tests__/route.test.ts` around lines
21 - 27, Capture the original process.env.REGISTRATION_ENABLED before tests run
(e.g. in a beforeAll or top-level const) and restore that value in an afterEach
(or afterAll) hook instead of deleting the env variable directly in tests;
update the test file that imports GET (in the beforeEach block that does
vi.resetModules/vi.clearAllMocks and imports "../route") to save the original
value and ensure afterEach restores process.env.REGISTRATION_ENABLED to the
saved value (or deletes it only if the original was undefined) so tests cannot
leak or leave mutated env state.

.github/workflows/ci.yml (1)

10-20: ⚠️ Potential issue | 🟠 Major

CLI is now in CI scope but missing from type-check job.

Since Line 10/Line 20 include cli/** in workflow triggers, CLI should also be installed and type-checked in the typecheck job; otherwise CLI TS errors can still merge.

Suggested workflow patch

       - name: Setup Node.js
         uses: actions/setup-node@v6
         with:
           node-version: '22'
           cache: 'npm'
           cache-dependency-path: |
             app/package-lock.json
             component/package-lock.json
             connection/package-lock.json
+            cli/package-lock.json
@@
       - name: Install dependencies
         run: |
           npm ci --prefix app &
           APP_CI_PID=$!
           npm ci --prefix component &
           COMP_CI_PID=$!
           npm ci --prefix connection &
           CONN_CI_PID=$!
+          npm ci --prefix cli &
+          CLI_CI_PID=$!
@@
           wait $APP_CI_PID  || FAIL=1
           wait $COMP_CI_PID || FAIL=1
           wait $CONN_CI_PID || FAIL=1
+          wait $CLI_CI_PID  || FAIL=1
           exit $FAIL
@@
       - name: Type-check app
         working-directory: app
         run: npx tsc --noEmit
+
+      - name: Type-check cli
+        working-directory: cli
+        run: npx tsc --noEmit

Also applies to: 125-143

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 10 - 20, The CI workflow now triggers
on cli/** but the typecheck job doesn't install or include the CLI code; update
the typecheck job (job name "typecheck") to install and type-check the CLI by
adding the cli/ package installation to the dependency install step (the step
that runs npm/yarn/pnpm install) and include 'cli/**' in the paths/glob that the
type-checking step runs against so TypeScript checks the CLI sources as well;
ensure any workspace/pnpm workspaces or package patterns used by the typecheck
step (e.g., workspace globs) include the cli package name or 'cli/**'.

🟠 Major comments (26)

app/src/components/dashboard-container.tsx-262-269 (1)

262-269: ⚠️ Potential issue | 🟠 Major

Broaden the interactive-element guard for double-click edit.

Line 265 only ignores button. Double-clicking other interactive targets (e.g., links, inputs, contenteditable) can incorrectly trigger edit mode.

Suggested patch

                 onDoubleClick={
                   editable && onEditWidget
                     ? (e: React.MouseEvent) => {
-                        if ((e.target as HTMLElement).closest("button")) return;
+                        const interactive = (e.target as HTMLElement).closest(
+                          "button, a, input, textarea, select, [role='button'], [contenteditable='true']",
+                        );
+                        if (interactive) return;
                         onEditWidget(widget);
                       }
                     : undefined
                 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/components/dashboard-container.tsx` around lines 262 - 269, The
double-click handler attached via the onDoubleClick prop (used when editable &&
onEditWidget for widget) only ignores clicks on "button", causing edits to
trigger when double-clicking other interactive elements; update the guard to
check for a broader set of interactive elements by using HTMLElement.closest
with a selector that includes button, a, input, textarea, select,
[contenteditable="true"], [role="button"] and other relevant interactive
selectors (and optionally check e.defaultPrevented) so the handler returns early
when the target or its ancestors are any of those interactive elements before
calling onEditWidget(widget).

.claude/skills/review/SKILL.md-12-12 (1)

12-12: ⚠️ Potential issue | 🟠 Major

Use merge-base diff to avoid missing changed files.

Line 12 fallback (git diff --name-only) can produce an empty list on a clean tree even when the branch diverges from dev. That can make the review checklist run against the wrong scope.

💡 Proposed fix

-- Changed: !`git diff origin/dev --name-only 2>/dev/null || git diff --name-only`
+- Changed: !`BASE_REF=$(git rev-parse --verify origin/dev >/dev/null 2>&1 && echo origin/dev || echo dev); MB=$(git merge-base HEAD "$BASE_REF" 2>/dev/null || echo "$BASE_REF"); git diff --name-only "$MB"...HEAD`

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/skills/review/SKILL.md at line 12, Replace the fallback diff command
in the "Changed: !`git diff origin/dev --name-only 2>/dev/null || git diff
--name-only`" line so it computes the diff from the merge-base instead of using
a plain working-tree diff; locate that exact string and change the fallback to
use the repository merge base (compute merge-base between HEAD and origin/dev
and run git diff against that merge-base) so changed files are detected even on
a clean tree when branches have diverged.

cli/src/lib/health.ts-16-21 (1)

16-21: ⚠️ Potential issue | 🟠 Major

Don’t abort health polling when check() throws.

A transient probe exception currently exits the loop immediately. Health checks should treat errors as “not ready” and keep polling until timeout.

💡 Proposed fix

   const deadline = Date.now() + timeout;
   while (Date.now() < deadline) {
-    if (check()) {
+    let ready = false;
+    try {
+      ready = check();
+    } catch {
+      ready = false;
+    }
+    if (ready) {
       spinner.succeed(`${label} is ready`);
       return;
     }
     await new Promise((r) => setTimeout(r, interval));
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/lib/health.ts` around lines 16 - 21, The health polling loop
currently exits early if check() throws; update the loop in the function
containing the while (Date.now() < deadline) block to wrap the check() call in a
try/catch so that any exception is treated as a failed probe (i.e., continue
polling until timeout) rather than breaking the loop; specifically, around the
call to check() catch errors from check() and ignore or log them (but do not
rethrow), only call spinner.succeed(`${label} is ready`) and return when check()
returns truthy normally.

cli/src/lib/exec.ts-30-34 (1)

30-34: ⚠️ Potential issue | 🟠 Major

run() replaces entire environment when opts.env is provided.

Line 33 uses fallback logic opts?.env ?? process.env, which means if opts.env is defined (even partially), it completely replaces process.env. Passing { FOO: "1" } removes PATH, NODE_OPTIONS, and other critical variables, breaking command execution. The interface permits partial environments, creating a footgun.

Merge the environment instead:

Fix

-      env: opts?.env ?? process.env,
+      env: { ...process.env, ...(opts?.env ?? {}) },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/lib/exec.ts` around lines 30 - 34, The run() function currently
passes opts?.env directly to execSync which replaces the entire process.env when
a partial env is provided; change this to merge the environments so provided
keys override defaults by creating a mergedEnv (e.g., merge process.env with
opts.env where opts.env wins) and pass mergedEnv to execSync instead of
opts?.env; update the options object in execSync (the block around execSync call
in exec.ts) to use this merged environment while preserving cwd and timeout
behavior.

cli/src/lib/exec.ts-57-63 (1)

57-63: ⚠️ Potential issue | 🟠 Major

Change dockerExec() signature to accept pre-split arguments.

Line 61 splits the cmd string on whitespace, which breaks quoted arguments. For example, psql ... -c "DROP DATABASE IF EXISTS mydb" splits into separate tokens, breaking the -c flag.

Affected calls in reset.ts, seed.ts, and migrate.ts all use quoted arguments that will be corrupted.

Suggested fix

-export function dockerExec(container: string, cmd: string): string {
+export function dockerExec(container: string, cmdArgs: string[]): string {
   try {
     const result = execFileSync(
       "docker",
-      ["exec", container, ...cmd.split(/\s+/)],
+      ["exec", container, ...cmdArgs],
       { encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] },
     );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/lib/exec.ts` around lines 57 - 63, The dockerExec function currently
takes a single cmd string and splits on whitespace which breaks quoted args;
change the signature of dockerExec(container: string, cmdArgs: string[]) to
accept a pre-split array of arguments, and update its execFileSync call to
execFileSync("docker", ["exec", container, ...cmdArgs], { encoding: "utf-8",
stdio: ["pipe","pipe","pipe"] }); then update all callers (reset.ts, seed.ts,
migrate.ts) to pass an array of arguments (preserving quoted values as single
elements, e.g. ["psql", "...", "-c", 'DROP DATABASE IF EXISTS mydb']) instead of
a single string so quoted arguments are not split.

.claude/hooks/check-query-safety.sh-23-43 (1)

23-43: ⚠️ Potential issue | 🟠 Major

Multiline template literals with unsafe interpolation bypass this guard.

Line 29 only triggers when both a query keyword and ${...} appear on the same line. Multiline templates split these across lines—for example, SELECT on line 2 and ${variable} on line 3—allowing unsafe interpolation to pass undetected.

Test case:

const query = `
  SELECT * FROM users
  WHERE id = ${userId}
`;

This pattern is not blocked by the current implementation, despite containing the exact unsafe pattern the hook is designed to prevent.

Tighten the check to detect template literals containing both patterns anywhere within them (not line-by-line). A grep -z approach or simpler three-way check (backticks present AND keywords present AND interpolation present) will reliably catch multiline unsafe queries.

🤖 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 - 43, The current check
only matches query keywords and `${...}` on the same line, allowing multiline
template literals to bypass it; update the guard that uses QUERY_KEYWORDS and
NEW_CONTENT so it detects interpolation anywhere inside backtick template
literals (not line-by-line). Concretely, change the conditional that tests
NEW_CONTENT against QUERY_KEYWORDS and '${' to a three-way check (ensure a
backtick exists AND a query keyword exists AND the literal '${' exists anywhere
in NEW_CONTENT) or use a multiline-aware grep (e.g., grep -zE /pcre-like search)
to scan across lines; keep using the existing QUERY_KEYWORDS and NEW_CONTENT
symbols and ensure the code path still emits the same BLOCKED message and exit
when all three conditions are true.

.claude/hooks/check-boundaries.sh-14-23 (1)

14-23: ⚠️ Potential issue | 🟠 Major

Boundary enforcement bypassed by relative paths and missing require() support.

Line 14 and 22 only match paths containing "/component/src/" or "/connection/src/" with a leading slash, allowing relative paths like component/src/... to skip enforcement. Additionally, the regex patterns on lines 15 and 23 won't catch require(...) syntax because they expect quotes immediately after the keyword, but require is followed by (.

Fix the path globs to match both relative and absolute paths, and update regex to properly detect require() calls.

Suggested patch

-if [[ "$FILE_PATH" == *"/component/src/"* ]]; then
-  if echo "$NEW_CONTENT" | grep -qE "(from|import|require)[[:space:]]*['\"].*/(app|connection)/|(from|import|require)[[:space:]]*['\"]@/(app|connection)"; then
+if [[ "$FILE_PATH" == *"component/src/"* ]]; then
+  if echo "$NEW_CONTENT" | grep -qE "(import[[:space:]].*from[[:space:]]*|import[[:space:]]*|require[[:space:]]*\()[\"'](.*/(app|connection)/|@/(app|connection))"; then
@@
-if [[ "$FILE_PATH" == *"/connection/src/"* ]]; then
-  if echo "$NEW_CONTENT" | grep -qE "(from|import|require)[[:space:]]*['\"]react(-dom)?['\"/]|(from|import|require)[[:space:]]*['\"].*/(app|component)/|(from|import|require)[[:space:]]*['\"]@/(app|component)"; then
+if [[ "$FILE_PATH" == *"connection/src/"* ]]; then
+  if echo "$NEW_CONTENT" | grep -qE "(import[[:space:]].*from[[:space:]]*|import[[:space:]]*|require[[:space:]]*\()[\"'](react(-dom)?([\"'/]|$)|.*/(app|component)/|@/(app|component))"; then

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/hooks/check-boundaries.sh around lines 14 - 23, The boundary checks
currently only match FILE_PATH values containing a leading slash and the regexes
miss require(...) calls; change the glob checks to match both relative and
absolute paths by replacing *"/component/src/"* and *"/connection/src/"* with
*"component/src/"* (e.g. [[ "$FILE_PATH" == *"component/src/"* ]]) and
*"connection/src/"* respectively, and update the grep -qE patterns that inspect
NEW_CONTENT to detect require(...) forms too (add alternation like
require[[:space:]]*\(\s*['\"]...['\"]\s*\) and apply the same for react and `@/`
or .*/(app|connection|component) matches) so imports via require('...') or
require("...") and paths without a leading slash are blocked appropriately.

cli/src/lib/docker.ts-21-27 (1)

21-27: ⚠️ Potential issue | 🟠 Major

Avoid raw shell interpolation in Docker/Compose commands.

Line 21, Line 27, Line 69, and Line 82 embed paths/credentials directly into shell strings. This is fragile and can be exploited if values contain shell metacharacters.

🔒 Proposed mitigation (minimal quoting helper)

+const q = (v: string) => JSON.stringify(v);

 export function composeUp(opts?: { full?: boolean }): void {
   const file = composeFile(opts?.full);
-  run(`docker compose -f ${file} up -d --build`, { cwd: paths.root });
+  run(`docker compose -f ${q(file)} up -d --build`, { cwd: paths.root });
 }

 export function composeDown(opts?: { volumes?: boolean }): void {
   const file = composeFile();
   const flags = opts?.volumes ? " -v" : "";
-  run(`docker compose -f ${file} down${flags}`, { cwd: paths.root });
+  run(`docker compose -f ${q(file)} down${flags}`, { cwd: paths.root });
 }

@@
     execInContainer(
       "neoboard-postgres",
-      `pg_isready -U ${config.postgres.user}`,
+      `pg_isready -U ${q(config.postgres.user)}`,
     );

@@
     execInContainer(
       "neoboard-neo4j",
-      `cypher-shell -u ${config.neo4j.user} -p ${config.neo4j.password} RETURN 1`,
+      `cypher-shell -u ${q(config.neo4j.user)} -p ${q(config.neo4j.password)} "RETURN 1"`,
     );

Also applies to: 69-83

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/lib/docker.ts` around lines 21 - 27, The commands currently
interpolate unescaped variables into shell strings (e.g., the run call in the
composeUp/composeDown usages and other calls at lines 69-83), which is unsafe;
update these to pass the Docker binary and arguments separately or use a minimal
quoting/escaping helper instead of template-string interpolation: stop doing
run(`docker ... ${file} ...`) and instead construct argv style calls (or run
with an args array) so composeFile(), credentials, and paths are passed as
distinct, escaped arguments; update the invocations referenced by the symbol
composeDown and the corresponding compose-up/run calls at lines ~21 and ~69-83
accordingly.

cli/src/commands/env.ts-53-53 (1)

53-53: ⚠️ Potential issue | 🟠 Major

Encode DB credentials before building DATABASE_URL.

Line 53 interpolates raw user/password/database values; special characters can produce an invalid URL and break app startup.

🐛 Proposed fix

-  const dbUrl = `postgresql://${config.postgres.user}:${config.postgres.password}@localhost:${config.ports.postgres}/${config.postgres.database}`;
+  const dbUser = encodeURIComponent(config.postgres.user);
+  const dbPassword = encodeURIComponent(config.postgres.password);
+  const dbName = encodeURIComponent(config.postgres.database);
+  const dbUrl = `postgresql://${dbUser}:${dbPassword}@localhost:${config.ports.postgres}/${dbName}`;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/commands/env.ts` at line 53, The dbUrl string built in env.ts
(variable dbUrl) interpolates raw config.postgres.user,
config.postgres.password, and config.postgres.database which can break the
connection URL if those values contain special characters; update the dbUrl
construction to URL-encode each component (use a safe encoder such as
encodeURIComponent on user, password, and database) before interpolating so the
resulting postgresql://... DATABASE_URL is always valid. Ensure you replace the
current interpolation with encoded values and keep host/port as-is.

cli/src/commands/db/dump.ts-31-37 (1)

31-37: ⚠️ Potential issue | 🟠 Major

Provide PGPASSWORD for non-interactive dumps.

Line 31–37 invokes pg_dump without password injection. In password-auth setups, this can block on prompt or fail.

🐛 Proposed fix

   if (mode === "docker") {
     sql = run(
-      `docker exec neoboard-postgres pg_dump -U ${config.postgres.user} ${config.postgres.database}${dataFlag}`,
+      `docker exec -e PGPASSWORD=${JSON.stringify(config.postgres.password)} neoboard-postgres pg_dump -U ${JSON.stringify(config.postgres.user)} ${JSON.stringify(config.postgres.database)}${dataFlag}`,
     );
   } else {
     sql = run(
-      `pg_dump -h localhost -p ${config.ports.postgres} -U ${config.postgres.user} ${config.postgres.database}${dataFlag}`,
+      `PGPASSWORD=${JSON.stringify(config.postgres.password)} pg_dump -h localhost -p ${config.ports.postgres} -U ${JSON.stringify(config.postgres.user)} ${JSON.stringify(config.postgres.database)}${dataFlag}`,
     );
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/commands/db/dump.ts` around lines 31 - 37, The pg_dump invocations in
the run(...) calls can hang or fail when Postgres requires a password; update
both calls that assign sql to inject the password non-interactively by supplying
PGPASSWORD from config.postgres.password (use an env var for the local call and
docker exec -e for the container call). Specifically, modify the two run(...)
invocations that build the pg_dump command (the assignments to sql) to include
PGPASSWORD=${config.postgres.password} (for the container use docker exec -e
PGPASSWORD=... neoboard-postgres ...) while keeping the existing -U, host/port,
database and ${dataFlag} pieces.

neoboard.config.json-8-16 (1)

8-16: ⚠️ Potential issue | 🟠 Major

Avoid committing plaintext database credentials in tracked config.

Line 10 and Line 15 hardcode passwords. Move secrets to environment-based resolution (or a non-committed local override) and keep this file non-sensitive by default.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@neoboard.config.json` around lines 8 - 16, The config currently commits
plaintext DB credentials under the "postgres" and "neo4j" objects (the
"password" fields); replace those hardcoded passwords with environment-based
resolution: remove the literal passwords and read them from environment
variables (e.g., POSTGRES_PASSWORD and NEO4J_PASSWORD) or from a non-committed
local override, update any code that loads this JSON to fallback to process.env
values for postgres.password and neo4j.password, and ensure the committed
neoboard.config.json contains safe placeholders and documentation on the
required env var names.

cli/package.json-23-23 (1)

23-23: ⚠️ Potential issue | 🟠 Major

Update @types/node to match Node 22 runtime target.

CI uses Node 22, but @types/node is pinned to ^25.5.0 (Node 25). This allows TypeScript to accept APIs unavailable in Node 22, causing runtime failures. Change to @types/node@^22 to align with the CI/runtime environment.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/package.json` at line 23, The dependency `@types/node` in package.json is
pinned to ^25.5.0 but CI/runtime uses Node 22; update the version spec for
"@types/node" to "^22" so TypeScript type checks target Node 22-compatible APIs,
then reinstall/update the lockfile (npm install or yarn) and run the TypeScript
build/tests to ensure no newer Node APIs remain referenced; locate the
"@types/node" entry in package.json to make this change.

.claude/agents/code-reviewer.md-17-17 (1)

17-17: ⚠️ Potential issue | 🟠 Major

Include CLI tests in the mandatory post-review test step.

The agent currently enforces app/component tests only; with CLI now in scope, this can miss regressions in cli/.

Proposed fix

-4. After code review, run `cd app && npx vitest run` and `cd component && npx vitest run` to verify tests pass.
+4. After code review, run `cd app && npx vitest run`, `cd component && npx vitest run`, and `cd cli && npx vitest run` to verify tests pass.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/agents/code-reviewer.md at line 17, Update the post-review test step
(the checklist item that currently runs "cd app && npx vitest run" and "cd
component && npx vitest run") to also run the CLI test suite by adding "cd cli
&& npx vitest run" (or a single combined command that runs app, component, and
cli tests). Edit the checklist text so step 4 explicitly includes running tests
in cli/ alongside app/ and component/ to ensure CLI regressions are covered.

scripts/setup-local-demo.sh-12-16 (1)

12-16: ⚠️ Potential issue | 🟠 Major

Avoid running a stale compiled CLI in demo setup.

dist existence alone is not enough; if cli/src/** changed, this script can execute old demo logic.

Proposed fix

-if [ ! -f "$CLI_BIN" ]; then
+if [ ! -f "$CLI_BIN" ] || find "$ROOT_DIR/cli/src" -type f -newer "$CLI_BIN" | grep -q .; then
   echo "==> Bootstrapping NeoBoard CLI..."
   npm install --prefix "$ROOT_DIR/cli"
   npm run build --prefix "$ROOT_DIR/cli"
   echo ""
 fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/setup-local-demo.sh` around lines 12 - 16, The current bootstrap only
checks for CLI_BIN existence and can use a stale build; update the
setup-local-demo.sh logic around CLI_BIN to also compare modification times and
rebuild when source changed: check if any files under "$ROOT_DIR/cli/src" (and
package.json) are newer than "$CLI_BIN" (e.g., using find -newer or a timestamp
compare) and if so run npm install --prefix "$ROOT_DIR/cli" and npm run build
--prefix "$ROOT_DIR/cli"; ensure the condition references CLI_BIN and the source
paths so the script rebuilds when cli/src/** or package.json changed.

scripts/setup.sh-11-15 (1)

11-15: ⚠️ Potential issue | 🟠 Major

Rebuild the CLI when sources change, not only when dist is missing.

Right now, an existing but stale cli/dist/index.js is reused, which can run outdated setup behavior.

Proposed fix

-# Bootstrap: build the CLI if it hasn't been compiled yet
-if [ ! -f "$CLI_BIN" ]; then
+# Bootstrap: build the CLI if missing or stale
+if [ ! -f "$CLI_BIN" ] || find "$ROOT_DIR/cli/src" -type f -newer "$CLI_BIN" | grep -q .; then
   echo "==> Bootstrapping NeoBoard CLI..."
   npm install --prefix "$ROOT_DIR/cli"
   npm run build --prefix "$ROOT_DIR/cli"
   echo ""
 fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/setup.sh` around lines 11 - 15, The current check only rebuilds when
CLI_BIN is missing; update the condition to also rebuild when source files are
newer than the built artifact: use CLI_BIN and ROOT_DIR (e.g., check
ROOT_DIR/cli/src) and replace the if with a test like "if [ ! -f \"$CLI_BIN\" ]
|| [ -n \"$(find \"$ROOT_DIR/cli/src\" -type f -newer \"$CLI_BIN\" -print
-quit)\" ]" so npm install and npm run build --prefix "$ROOT_DIR/cli" run when
dist is missing or sources changed.

.claude/skills/pr/SKILL.md-20-20 (1)

20-20: ⚠️ Potential issue | 🟠 Major

Pre-flight omits connection tests.

PRs affecting connection/ can pass pre-flight without running its test suite. Add cd connection && npm test (with Docker note/guard) to keep pre-flight aligned with package labels and changed-scope validation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/skills/pr/SKILL.md at line 20, Pre-flight script/README omits
running the connection package tests; update the pre-flight checklist to include
running the connection test suite by adding the step "cd connection && npm test"
(with the existing Docker guard/note pattern used for other packages) so that
packages labeled/changed in connection run their tests during pre-flight and
remain consistent with changed-scope validation.

.claude/skills/code/SKILL.md-40-44 (1)

40-44: ⚠️ Potential issue | 🟠 Major

Post-coding verification is app-only despite an “ALL coding tasks” scope.

This block skips component/ and connection/ test paths, so changes outside app/ can merge without package-level validation. Please make post-coding checks package-aware (or delegate to the test skill).

Suggested update

- cd app && npx next lint --fix
- npm run build
- cd app && npm test
+ npm run lint
+ npm run build
+ # Run package tests based on changed paths (app/component/connection),
+ # or invoke the dedicated test skill for selective execution.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/skills/code/SKILL.md around lines 40 - 44, The post-coding
verification commands only run lint/build/tests inside the app package ("cd app
&& npx next lint --fix", "npm run build", "cd app && npm test"), which misses
changes in component/ and connection/; update the block to run
workspace/package-aware checks (e.g., run lint from repo root, run build for
each package or a workspace build script, and run tests across all packages) or
delegate to the centralized test skill — replace the three commands with
package-aware equivalents such as running root-level lint, a multi-package
build, and a global test runner (or call the test skill) so component/ and
connection/ changes are validated too.

cli/src/index.ts-154-155 (1)

154-155: ⚠️ Potential issue | 🟠 Major

Use parseAsync() instead of parse() for async command actions.

Line 155 uses program.parse() while multiple command handlers are async (lines 28, 36, 45, 53, 62, 70, 78, 89, 99, 113, 126, 135, 144). This can cause unhandled promise rejections and unreliable exit semantics. Commander.js requires parseAsync() to properly await async action handlers.

Suggested fix

-if (isDirectRun) {
-  program.parse();
-}
+if (isDirectRun) {
+  program.parseAsync(process.argv).catch((error) => {
+    console.error(error);
+    process.exitCode = 1;
+  });
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/index.ts` around lines 154 - 155, The CLI currently calls
program.parse() when isDirectRun is true, but several command action handlers
are async so you must replace that call with program.parseAsync() to ensure
commander awaits async actions and avoids unhandled rejections; update the
invocation guarded by isDirectRun from program.parse() to await
program.parseAsync() (or return program.parseAsync() if top-level await isn't
used) so async handlers run to completion.

.claude/agents/feature-reviewer.md-17-40 (1)

17-40: ⚠️ Potential issue | 🟠 Major

Same Playwright CLI invocation issue.

Consistent with the other agent files: use npx playwright not npx @playwright/cli``.

🤖 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 17 - 40, Replace every usage
of the Playwright CLI invocation string "npx `@playwright/cli`" with the correct
"npx playwright" throughout the document (affecting all commands such as open,
goto, fill, click, type, select, screenshot, snapshot, console, network,
resize); update instances used with selectors and commands like 'open', 'goto',
'fill', 'click', 'type', 'select', 'screenshot', 'snapshot', 'console',
'network', and 'resize' in the .claude/agents/feature-reviewer.md content so the
examples invoke the tool via "npx playwright" consistently.

.claude/agents/user-sim-admin.md-21-32 (1)

21-32: ⚠️ Potential issue | 🟠 Major

Same Playwright CLI invocation issue.

As noted in ux-crawler.md, the commands should use npx playwright not npx @playwright/cli``.

-npx `@playwright/cli` open <url>
+npx playwright open <url>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/agents/user-sim-admin.md around lines 21 - 32, Replace all
occurrences of the incorrect Playwright CLI invocation "npx `@playwright/cli`"
with the correct "npx playwright" in the command list shown (the lines invoking
open, goto, click, fill, type, select, screenshot, snapshot, console, resize) so
the docs use the same CLI form as in ux-crawler.md.

.claude/skills/fix-pr-reviews/SKILL.md-40-50 (1)

40-50: ⚠️ Potential issue | 🟠 Major

Replace literal {owner}/{repo} placeholders with resolved values.

Line 40–50 and 95–113 are not directly executable as written. Please resolve owner/repo dynamically before API calls.

Runnable pattern

+# Resolve owner/repo once
+REPO_FULL=$(gh repo view --json nameWithOwner --jq '.nameWithOwner')
+OWNER="${REPO_FULL%/*}"
+REPO="${REPO_FULL#*/}"
+
-gh api repos/{owner}/{repo}/pulls/$ARGUMENTS/comments \
+gh api repos/$OWNER/$REPO/pulls/$ARGUMENTS/comments \
   --jq '[.[] | select(.user.login == "coderabbitai[bot]")]'
@@
-gh api repos/{owner}/{repo}/pulls/$ARGUMENTS/reviews \
+gh api repos/$OWNER/$REPO/pulls/$ARGUMENTS/reviews \
   --jq '[.[] | select(.user.login == "coderabbitai[bot]")]'

Also applies to: 95-113

🤖 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 - 50, The gh api/gh
pr commands in the SKILL.md snippet use literal placeholder "{owner}/{repo}"
which makes the commands non-runnable; update the script to resolve the
repository owner and name into variables and interpolate them into the commands
that call gh api repos/{owner}/{repo}/pulls and the other gh pr view/gh api
review endpoints (the lines containing the literal "{owner}/{repo}" and the
commands using $ARGUMENTS), ensuring the resolved owner and repo values are
derived (for example from git remote or environment) and substituted before
invoking the API so the gh calls execute correctly.

cli/src/commands/status.ts-51-63 (1)

51-63: ⚠️ Potential issue | 🟠 Major

Readiness probes fail silently in local mode.

Lines 51–52 call isPgReady() and isNeo4jReady() unconditionally, but both functions execute probes inside Docker containers only (via execInContainer()). When running in local mode, these calls fail silently and return false, causing the status command to report healthy local services as "stopped". The getMode() result at line 40 is never used to route to appropriate local-mode health checks. Either add mode-aware branching in runStatus() or make the readiness probes mode-aware.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/commands/status.ts` around lines 51 - 63, runStatus is calling
isPgReady() and isNeo4jReady() unconditionally, but those probes use
execInContainer() and return false in local mode; make readiness checks
mode-aware by using getMode() (or the existing mode variable) to branch: when
mode === "local" call local-aware probes or alternate checks (e.g., direct
TCP/http checks used by getAppHealth or a new isPgLocalReady/isNeo4jLocalReady)
and only call isPgReady()/isNeo4jReady() when in container mode so healthy local
services are not reported as "stopped"; update runStatus to use getMode() to
select the appropriate probe functions (or make isPgReady/isNeo4jReady accept a
mode param) and keep execInContainer usage limited to container mode.

cli/src/commands/db/migrate.ts-18-25 (1)

18-25: ⚠️ Potential issue | 🟠 Major

status and dry-run are not reporting pending DB work.

Both commands only read paths.journalPath, so they will list every journal entry even when those migrations were already applied. Either query applied state or rename this output to make it clear it is only the local migration catalog.

Also applies to: 27-50

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/commands/db/migrate.ts` around lines 18 - 25, The current
readJournal() and the status/dry-run commands only read paths.journalPath (via
readJournal()) so they report all cataloged migrations regardless of what the
database has applied; update the logic in the status and dry-run handlers to
query the database for applied migrations (e.g., read the migrations/applied
table or use the existing DB client) and compare those applied IDs against the
journal entries returned by readJournal(), then filter or annotate the output so
only pending migrations (journal entries not present in the applied set) are
reported as pending (or clearly mark applied vs pending). Alternatively, if you
prefer not to query the DB, rename the output/UI text to explicitly state it is
a local migration catalog rather than a pending-by-DB status; touch
readJournal(), the status command handler and dry-run handler to implement the
chosen behavior.

cli/src/commands/doctor.ts-13-30 (1)

13-30: ⚠️ Potential issue | 🟠 Major

Don’t hard-fail Docker checks in local mode.

If this workspace is configured for local mode, these Docker daemon / Compose failures are false positives. Gate them on the configured mode, or downgrade them outside docker mode.

Also applies to: 80-86

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/commands/doctor.ts` around lines 13 - 30, The Docker daemon and
Compose checks (checkDockerRunning and checkDockerComposeV2) currently
hard-fail; update them to respect the workspace mode by detecting local/docker
mode (call the existing config check e.g. isLocalMode() or similar) and if the
workspace is in local mode return a non-failing result (e.g. status "skipped" or
"warn" with a message like "Skipped in local mode") instead of "fail"; if not in
local mode keep the existing behavior. Ensure both functions use the same gating
logic so Docker failures are only hard-fails when not in local mode.

.claude/settings.json-84-90 (1)

84-90: ⚠️ Potential issue | 🟠 Major

The npm approval guard is bypassable.

This regex misses common mutating forms like npm i foo and npm install --save-dev foo, so dependency changes can still land without explicit approval.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/settings.json around lines 84 - 90, The current hook command's regex
only detects full words like "install" and "uninstall" and misses short aliases
and flagged installs; update the grep -qE pattern in the hooks' "command" (the
shell variables INPUT and CMD are used) to also match short aliases and installs
with flags—e.g. change the pattern to something like
'^npm\s+(install|i|uninstall|un|remove|rm|r|add)\b' and additionally detect
flag-driven dependency changes (match --save-dev|-D|--save|-S|--save-prod etc.)
so commands like "npm i foo" and "npm install --save-dev foo" are caught and the
script still returns exit 2 and the same BLOCKED message.

cli/src/commands/db/migrate.ts-70-74 (1)

70-74: ⚠️ Potential issue | 🟠 Major

Refuse --to until targeted migration is implemented.

Warning and then running the full migrate command anyway can apply more schema changes than the operator asked for. This should exit non-zero instead of continuing.

Suggested change

   if (opts.to) {
-    warn(
-      `--to ${opts.to}: Drizzle Kit applies all pending migrations. Version validation is not yet supported.`,
-    );
+    throw new Error(
+      `--to ${opts.to} is not supported yet. Refusing to apply all pending migrations implicitly.`,
+    );
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/commands/db/migrate.ts` around lines 70 - 74, The code currently just
warns when opts.to is provided and continues; change this to refuse the option
by returning a non-zero exit: detect when opts.to is set (the existing if
(opts.to) block), log an error (use the same logger or console.error) explaining
that targeted migration (--to) is not supported, and then exit with a non-zero
status (e.g., process.exit(1) or throw an Error) so the migrate command does not
run; update any surrounding control flow in migrate handler to ensure early
return when opts.to is present.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 28f8a570-68ef-49da-9752-2f5be012c7bd

📥 Commits

Reviewing files that changed from the base of the PR and between f31188f and 75787aa.

⛔ Files ignored due to path filters (2)

• cli/package-lock.json is excluded by !**/package-lock.json
• docker/docker-compose.prod.yml is excluded by !docker/**

📒 Files selected for processing (142)

• .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
• .env.example
• .github/workflows/ci.yml
• .gitignore
• CLAUDE.md
• app/e2e/api-keys.spec.ts
• app/e2e/auth.spec.ts
• app/e2e/charts.spec.ts
• app/e2e/connections.spec.ts
• app/e2e/settings-profile.spec.ts
• app/e2e/users.spec.ts
• app/e2e/widget-states.spec.ts
• app/e2e/widgets.spec.ts
• app/src/app/(auth)/login/__tests__/page.test.tsx
• app/src/app/(auth)/login/page.tsx
• app/src/app/(auth)/signup/__tests__/page.test.tsx
• app/src/app/(auth)/signup/page.tsx
• app/src/app/(dashboard)/__tests__/layout.test.tsx
• app/src/app/(dashboard)/connections/page.tsx
• app/src/app/(dashboard)/layout.tsx
• app/src/app/(dashboard)/settings/__tests__/page.test.ts
• app/src/app/(dashboard)/settings/page.tsx
• app/src/app/(dashboard)/widget-lab/page.tsx
• app/src/app/api/auth/bootstrap-status/__tests__/route.test.ts
• app/src/app/api/auth/bootstrap-status/route.ts
• app/src/app/api/connections/[id]/__tests__/route.test.ts
• app/src/app/api/connections/[id]/route.ts
• app/src/app/api/keys/__tests__/route.test.ts
• app/src/app/api/keys/route.ts
• app/src/components/__tests__/card-container-states.test.tsx
• app/src/components/__tests__/card-container.test.tsx
• app/src/components/__tests__/dashboard-container-dblclick.test.tsx
• app/src/components/card-container.tsx
• app/src/components/dashboard-container.tsx
• app/src/components/widget-editor-modal.tsx
• app/src/components/widget-editor/__tests__/query-editor-panel.test.tsx
• app/src/components/widget-editor/__tests__/transform-editor.test.tsx
• app/src/components/widget-editor/parameter-preview.tsx
• app/src/components/widget-editor/query-editor-panel.tsx
• app/src/components/widget-editor/transform-editor.tsx
• app/src/hooks/use-seed-query.ts
• app/src/lib/__tests__/api-client.test.ts
• app/src/lib/__tests__/parse-utils.test.ts
• app/src/lib/__tests__/schemas.test.ts
• app/src/lib/api-client.ts
• app/src/lib/auth/__tests__/config.test.ts
• app/src/lib/auth/config.ts
• app/src/lib/parse-utils.ts
• app/src/lib/query-templates.ts
• app/src/lib/schemas.ts
• app/src/stores/__tests__/widget-editor-store.test.ts
• app/src/stores/widget-editor-store.ts
• bin/neoboard
• cli/package.json
• cli/src/__tests__/commands/db/dump.test.ts
• cli/src/__tests__/commands/db/migrate.test.ts
• cli/src/__tests__/commands/db/reset.test.ts
• cli/src/__tests__/commands/db/seed.test.ts
• cli/src/__tests__/commands/demo.test.ts
• cli/src/__tests__/commands/dev.test.ts
• cli/src/__tests__/commands/doctor.test.ts
• cli/src/__tests__/commands/env.test.ts
• cli/src/__tests__/commands/init.test.ts
• cli/src/__tests__/commands/setup.test.ts
• cli/src/__tests__/commands/start.test.ts
• cli/src/__tests__/commands/status.test.ts
• cli/src/__tests__/commands/stop.test.ts
• cli/src/__tests__/lib/config.test.ts
• cli/src/__tests__/lib/docker.test.ts
• cli/src/__tests__/lib/exec.test.ts
• cli/src/__tests__/lib/health.test.ts
• cli/src/__tests__/lib/output.test.ts
• cli/src/__tests__/lib/ports.test.ts
• cli/src/__tests__/program.test.ts
• cli/src/commands/db/dump.ts
• cli/src/commands/db/migrate.ts
• cli/src/commands/db/reset.ts
• cli/src/commands/db/seed.ts
• cli/src/commands/demo.ts
• cli/src/commands/dev.ts
• cli/src/commands/doctor.ts
• cli/src/commands/env.ts
• cli/src/commands/init.ts
• cli/src/commands/setup.ts
• cli/src/commands/start.ts
• cli/src/commands/status.ts
• cli/src/commands/stop.ts
• cli/src/index.ts
• cli/src/lib/config.ts
• cli/src/lib/docker.ts
• cli/src/lib/exec.ts
• cli/src/lib/health.ts
• cli/src/lib/output.ts
• cli/src/lib/ports.ts
• cli/src/lib/prompt.ts
• cli/tsconfig.json
• cli/vitest.config.ts
• component/src/charts/__tests__/graph-chart.test.tsx
• component/src/charts/graph-chart.tsx
• component/src/components/composed/__tests__/chart-settings-panel.test.tsx
• component/src/components/composed/__tests__/dashboard-mini-preview.test.tsx
• component/src/components/composed/app-shell.tsx
• component/src/components/composed/chart-settings-panel.tsx
• component/src/components/composed/dashboard-mini-preview.tsx
• docs/ux-friction-report.md
• neoboard.config.json
• package.json
• scripts/setup-local-demo.sh
• scripts/setup.sh
• sonar-project.properties

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n .claude/hooks/check-credential-logging.sh

Repository: alfredo1996/neoboard

Length of output: 1571

---

🏁 Script executed:

# Check if the file exists and what grep version we have
file .claude/hooks/check-credential-logging.sh 2>/dev/null || echo "File not found"
grep --version | head -1

Repository: alfredo1996/neoboard

Length of output: 98

---

🏁 Script executed:

# Test the grep -zPqi syntax validity
echo "test" | grep -zPqi "test" 2>&1 && echo "Syntax valid" || echo "Syntax invalid or not supported"

Repository: alfredo1996/neoboard

Length of output: 76

---

🏁 Script executed:

# Test multiline console logging evasion with current detection
cat > /tmp/test_current.sh << 'EOF'
#!/bin/bash
CRED_PATTERN='(password|passwd|secret|credential|apiKey|api_key|encryptionKey|encryption_key|decrypted|privateKey|private_key|accessToken|access_token|refreshToken|refresh_token)'

# Test case 1: Single line (should be blocked)
echo "=== Test 1: Single line console.log with password ==="
NEW_CONTENT="console.log(password)"
if echo "$NEW_CONTENT" | grep -iE "console\.(log|warn|error|debug|info)" | grep -qiE "${CRED_PATTERN}"; then
  echo "BLOCKED (expected)"
else
  echo "PASSED (unexpected - should have been blocked)"
fi

# Test case 2: Multiline (should be blocked but likely won't be)
echo ""
echo "=== Test 2: Multiline console.log with password ==="
NEW_CONTENT=$'console.log(\n  password\n)'
if echo "$NEW_CONTENT" | grep -iE "console\.(log|warn|error|debug|info)" | grep -qiE "${CRED_PATTERN}"; then
  echo "BLOCKED (expected)"
else
  echo "PASSED (EVASION - not blocked as expected)"
fi

# Test case 3: Multiline with variable
echo ""
echo "=== Test 3: Multiline console.log with variable reference ==="
NEW_CONTENT=$'console.log(\n  "password is:",\n  userPassword\n)'
if echo "$NEW_CONTENT" | grep -iE "console\.(log|warn|error|debug|info)" | grep -qiE "${CRED_PATTERN}"; then
  echo "BLOCKED (expected)"
else
  echo "PASSED (EVASION - not blocked as expected)"
fi

# Test case 4: Variable concatenation
echo ""
echo "=== Test 4: Multiline with string concatenation ==="
NEW_CONTENT=$'console.warn(\n  "User " + username +\n  " has password: " + password\n)'
if echo "$NEW_CONTENT" | grep -iE "console\.(log|warn|error|debug|info)" | grep -qiE "${CRED_PATTERN}"; then
  echo "BLOCKED (expected)"
else
  echo "PASSED (EVASION - not blocked as expected)"
fi
EOF

bash /tmp/test_current.sh

Repository: alfredo1996/neoboard

Length of output: 434

---

🏁 Script executed:

# Now test the proposed fix with grep -zPqi
cat > /tmp/test_proposed.sh << 'EOF'
#!/bin/bash
CRED_PATTERN='(password|passwd|secret|credential|apiKey|api_key|encryptionKey|encryption_key|decrypted|privateKey|private_key|accessToken|access_token|refreshToken|refresh_token)'

# Test case 1: Single line (should be blocked)
echo "=== Test 1: Single line console.log with password ==="
NEW_CONTENT="console.log(password)"
if echo "$NEW_CONTENT" | grep -zPqi "console\.(log|warn|error|debug|info)\s*\([^)]*${CRED_PATTERN}[^)]*\)"; then
  echo "BLOCKED (expected)"
else
  echo "PASSED (unexpected - should have been blocked)"
fi

# Test case 2: Multiline (should be blocked)
echo ""
echo "=== Test 2: Multiline console.log with password ==="
NEW_CONTENT=$'console.log(\n  password\n)'
if echo "$NEW_CONTENT" | grep -zPqi "console\.(log|warn|error|debug|info)\s*\([^)]*${CRED_PATTERN}[^)]*\)"; then
  echo "BLOCKED (expected)"
else
  echo "PASSED (unexpected)"
fi

# Test case 3: Multiline with variable
echo ""
echo "=== Test 3: Multiline console.log with variable reference ==="
NEW_CONTENT=$'console.log(\n  "password is:",\n  userPassword\n)'
if echo "$NEW_CONTENT" | grep -zPqi "console\.(log|warn|error|debug|info)\s*\([^)]*${CRED_PATTERN}[^)]*\)"; then
  echo "BLOCKED (expected)"
else
  echo "PASSED (unexpected)"
fi

# Test case 4: Variable concatenation
echo ""
echo "=== Test 4: Multiline with string concatenation ==="
NEW_CONTENT=$'console.warn(\n  "User " + username +\n  " has password: " + password\n)'
if echo "$NEW_CONTENT" | grep -zPqi "console\.(log|warn|error|debug|info)\s*\([^)]*${CRED_PATTERN}[^)]*\)"; then
  echo "BLOCKED (expected)"
else
  echo "PASSED (unexpected)"
fi

# Test case 5: Nested parens (edge case - may not work with simple pattern)
echo ""
echo "=== Test 5: Nested parentheses with secret ==="
NEW_CONTENT=$'console.log(encrypt(secret))'
if echo "$NEW_CONTENT" | grep -zPqi "console\.(log|warn|error|debug|info)\s*\([^)]*${CRED_PATTERN}[^)]*\)"; then
  echo "BLOCKED (expected)"
else
  echo "PASSED (unexpected)"
fi
EOF

bash /tmp/test_proposed.sh

Repository: alfredo1996/neoboard

Length of output: 429

---

Multiline console.* calls can evade credential logging detection.

The current check on line 25 pipes two greps—matching console.log|warn|error|debug|info first, then filtering for credentials. This fails when the credential keyword appears on a different line than the console call. Multiline patterns like console.log(\n password\n) bypass the hook entirely.

Use grep -zPqi to treat the entire input as a single string and match credentials anywhere within console call parentheses:

Hardening fix

-if echo "$NEW_CONTENT" | grep -iE "console\.(log|warn|error|debug|info)" | grep -qiE "${CRED_PATTERN}"; then
+if echo "$NEW_CONTENT" | grep -zPqi "console\.(log|warn|error|debug|info)\s*\([^)]*${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 hook's two-step
grep on NEW_CONTENT misses multiline console calls; change the check that
currently pipes greps into a single grep invocation that treats the input as one
NUL-separated blob (use grep -zPqi) and use a PCRE that finds
console\.(log|warn|error|debug|info)\s*\(.*?${CRED_PATTERN}.*?\) across newlines
so credentials inside parentheses are detected; update the conditional that
references NEW_CONTENT and CRED_PATTERN to use this single -zPqi pattern instead
of the two greps.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Block secret reads too.

Read(*) and shell readers like cat, head, tail, and grep still expose .env, key, and credential files. The deny list only blocks writes/edits right now, so secrets can still be exfiltrated. Add matching read-side denies as well.

Also applies to: 21-35

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/settings.json around lines 9 - 15, The deny list currently blocks
write/edit commands but not read operations; update the .claude/settings.json
deny entries to also include read-side patterns for shell readers and generic
reads (e.g., add "Bash(cat *)", "Bash(head *)", "Bash(tail *)", "Bash(grep *)",
"Read(*)" or equivalent read-pattern entries alongside the existing write
denies) and mirror the same additions for the other block (lines 21-35) so
secrets like .env and credential files cannot be exfiltrated via read commands;
locate the array of deny strings in the file and add the read variants next to
the existing deny entries such as "Bash(ls *)" to keep writes and reads
consistently blocked.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

find . -name "SKILL.md" -type f

Repository: alfredo1996/neoboard

Length of output: 575

---

🏁 Script executed:

cat -n .claude/skills/next/SKILL.md 2>/dev/null | head -30

Repository: alfredo1996/neoboard

Length of output: 1474

---

🌐 Web query:

GitHub CLI gh issue list --json fields assignees documentation

💡 Result:

The GitHub CLI command gh issue list --json supports outputting JSON data for issues, including the assignees field. To use it specifically for assignees: gh issue list --json assignees This returns a JSON array of issues, each with an assignees array containing assignee objects (typically with fields like login). Available JSON fields for gh issue list (from official documentation) include: assignees, author, body, closed, closedAt, closedByPullRequestsReferences, comments, createdAt, id, isPinned, labels, milestone, number, projectCards, projectItems, reactionGroups, state, stateReason, title, updatedAt, url. To list assignees for all issues in CSV-like format (example from usage): gh issue list --state all --json number,state,title,assignees --jq 'map([.number, .state, .title, (.assignees | map(.login) | join(","))])[] | @csv' Official documentation: https://cli.github.com/manual/gh_issue_list

Citations:

• 1: https://cli.github.com/manual/gh_issue_list
• 2: https://stackoverflow.com/questions/76929428/how-to-list-all-issues-of-a-github-repository-with-all-assignees
• 3: https://man.archlinux.org/man/extra/github-cli/gh-issue-list.1.en
• 4: https://cli.github.com/manual/gh_issue_view

---

Request assignees in the JSON output before filtering.

The jq expression filters on .assignees, but that field is not in the --json list on line 17. Since assignees is not returned, jq receives null, causing select(.assignees | length == 0) to filter out all results. Either add assignees to the field list or use --search "no:assignee" instead.

Suggested change

-gh issue list --state open --limit 10 --json number,title,labels,milestone,body --jq '[.[] | select(.assignees | length == 0)] | sort_by(.milestone.title) | .[0:5]'
+gh issue list --state open --search "no:assignee" --limit 10 --json number,title,labels,milestone,body --jq 'sort_by(.milestone.title) | .[0:5]'

🤖 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 15 - 18, The jq filter uses
.assignees but the gh command on the earlier line doesn't request the assignees
field; update the command that builds JSON (the gh issue list call which
currently uses --json number,title,labels,milestone,body) to include assignees
so select(.assignees | length == 0) sees real data, or alternatively replace the
filtering approach by using gh's search flag (e.g., --search "no:assignee")
instead of post-filtering; ensure you change the gh issue list invocation (the
one with --json number,title,labels,milestone,body) or switch to --search to fix
the missing .assignees field.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Locate and read the config.ts file
find . -name "config.ts" -type f | grep -E "cli/src"

Repository: alfredo1996/neoboard

Length of output: 87

---

🏁 Script executed:

# Read the specific file and lines mentioned
cat -n cli/src/lib/config.ts | head -40

Repository: alfredo1996/neoboard

Length of output: 1429

---

🌐 Web query:

Node.js path.dirname behavior Windows root directory infinite loop

💡 Result:

No, Node.js path.dirname does not cause an infinite loop for the Windows root directory. It correctly handles root paths like 'C:\' by returning the drive root itself (e.g., path.win32.dirname('C:\') returns 'C:\'), preventing loops when repeatedly calling dirname.

Citations:

• 1: path.dirname used in loop nodejs/node#7461
• 2: https://nodejs.org/api/path.html
• 3: https://nodejs.org/docs/v23.4.0/api/path.html
• 4: https://stackoverflow.com/questions/75395104/using-path-dirname-to-find-path

---

Project-root traversal is not cross-platform safe.

On Windows, dirname() returns the same path at filesystem roots (e.g., path.dirname('C:\\') returns 'C:\\'). The loop condition while (dir !== "/") remains true indefinitely on Windows since the root is never /, causing an infinite loop.

Replace the POSIX-specific root check with a comparison:

Suggested fix

 export function findProjectRoot(startDir?: string): string {
   let dir = startDir ?? dirname(fileURLToPath(import.meta.url));
-  while (dir !== "/") {
+  while (true) {
     const pkgPath = join(dir, "package.json");
     if (existsSync(pkgPath)) {
       try {
         const pkg = JSON.parse(readFileSync(pkgPath, "utf-8"));
         if (pkg.name === "neoboard") return dir;
       } catch {
         /* skip */
       }
     }
-    dir = dirname(dir);
+    const parent = dirname(dir);
+    if (parent === dir) break;
+    dir = parent;
   }
   throw new Error(
     "Could not find NeoBoard project root (package.json with name 'neoboard')",
   );
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/lib/config.ts` around lines 25 - 36, The traversal loop using while
(dir !== "/") is not cross-platform safe and can infinite-loop on Windows;
change the logic to stop when you reach the filesystem root by computing the
root via path.parse(dir).root (or by checking if dirname(dir) === dir) and break
when dir === root (or when dirname(dir) === dir), updating the loop that uses
dirname, join, existsSync, readFileSync and pkgPath to use that root-aware
termination condition.

[coderabbitai]

Actionable comments posted: 3

🤖 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/components/widget-editor-modal.tsx`:
- Around line 405-426: handleConnectionChange currently only clears query state
for cross-type switches and leaves preview/derived-column state stale; update
handleConnectionChange to always invalidate/reset preview-related state in the
widget editor store (e.g., preview results, derived columns, preview status
flags) whenever connectionId changes, and also reset
autoPreviewTriggered.current so the auto-preview effect can rerun; after
invalidation, if the existing query is still valid for the new connection type
(i.e., newConnection exists and compatible with current chart/query), call the
same preview trigger used by the query-change effect to re-run the preview.
Apply the same invalidation + autoPreviewTriggered.current reset and conditional
re-run to the other connection-change path noted around the 729-765 section so
both code paths behave consistently.
- Around line 1637-1649: The current tooltip trigger is an unfocusable SVG
(AlertCircle) and the UI still shows the loading spinner whenever connectionId
&& query.trim() even after previewQuery.isError, so keyboard users can't read
errors and failed previews look like endless loading; fix by making
TooltipTrigger a focusable element (wrap the AlertCircle in a button or give the
trigger an element with tabIndex=0 and an accessible aria-label) using the
existing TooltipTrigger/asChild so keyboard users can open the TooltipContent,
and change the render logic that shows the spinner (the condition checking
connectionId && query.trim()) to also require !previewQuery.isError; when
previewQuery.isError render a settled error state (e.g., an accessible error
icon + descriptive text or the TooltipContent) instead of the spinner so
failures are visible and reachable via keyboard (reference: Tooltip,
TooltipTrigger, AlertCircle, TooltipContent, previewQuery.isError, connectionId,
query.trim()).
- Line 487: Replace the current non-null ref guard
(editInitialChartTypeRef.current) with an explicit source-tracking flag so you
can tell hydration/template changes apart from user actions: add a small
enum/ref like editChartTypeSourceRef with values 'hydrate' | 'user' | null, set
it to 'hydrate' when doing lab-edit/template hydration (where you currently set
editInitialChartTypeRef.current = widget.chartType) and set it to 'user' inside
the user-driven chart type change handler (e.g., onChartTypeChange), then update
the chartOptions reset logic to only run when the source is 'user'; finally
clear the source (null) after handling so subsequent changes behave normally
(apply same change to the other identical guard block around the 623-644
region).

🪄 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: c7d80afa-5767-4558-ae04-55d36b81cc06

📥 Commits

Reviewing files that changed from the base of the PR and between 75787aa and 627393d.

📒 Files selected for processing (2)

• app/src/components/widget-editor-modal.tsx
• app/src/lib/auth/__tests__/config.test.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Connector switches can leave the preview bound to the previous data source.

On a cross-type switch, Line 417 only clears store fields, so the UI can still fall back to the old preview state. On a same-type switch, the new auto-preview split never reruns because autoPreviewTriggered.current is already set and the query-change effect ignores connection-only changes. The preview pane and derived column list can stay stale until the user edits the query or clicks Run.

Reset/invalidate preview state on any connector change, and rerun preview when the existing query is still valid for the new connection.

Also applies to: 729-765

🤖 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 405 - 426,
handleConnectionChange currently only clears query state for cross-type switches
and leaves preview/derived-column state stale; update handleConnectionChange to
always invalidate/reset preview-related state in the widget editor store (e.g.,
preview results, derived columns, preview status flags) whenever connectionId
changes, and also reset autoPreviewTriggered.current so the auto-preview effect
can rerun; after invalidation, if the existing query is still valid for the new
connection type (i.e., newConnection exists and compatible with current
chart/query), call the same preview trigger used by the query-change effect to
re-run the preview. Apply the same invalidation + autoPreviewTriggered.current
reset and conditional re-run to the other connection-change path noted around
the 729-765 section so both code paths behave consistently.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

The chart-options reset guard still confuses hydration with user changes.

This only keys off editInitialChartTypeRef.current !== null. lab-edit hydration is never guarded, so opening a template with a different chartType can overwrite its saved chartOptions with defaults. It also leaves the ref armed when edit-mode hydration does not actually change chartType, so the first user switch can skip the reset instead.

Track the source of the chartType change (hydrate/template vs user action) instead of a non-null ref.

Also applies to: 623-644

🤖 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` at line 487, Replace the current
non-null ref guard (editInitialChartTypeRef.current) with an explicit
source-tracking flag so you can tell hydration/template changes apart from user
actions: add a small enum/ref like editChartTypeSourceRef with values 'hydrate'
| 'user' | null, set it to 'hydrate' when doing lab-edit/template hydration
(where you currently set editInitialChartTypeRef.current = widget.chartType) and
set it to 'user' inside the user-driven chart type change handler (e.g.,
onChartTypeChange), then update the chartOptions reset logic to only run when
the source is 'user'; finally clear the source (null) after handling so
subsequent changes behave normally (apply same change to the other identical
guard block around the 623-644 region).

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (4)

cli/src/lib/config.ts (1)

25-36: ⚠️ Potential issue | 🔴 Critical

Fix root traversal termination for Windows filesystem roots.

At Line 25, while (dir !== "/") is POSIX-specific and can loop forever on Windows root paths. Use a root-aware stop condition (parent === dir or dir === parse(dir).root).

Suggested fix

 export function findProjectRoot(startDir?: string): string {
   let dir = startDir ?? dirname(fileURLToPath(import.meta.url));
-  while (dir !== "/") {
+  while (true) {
     const pkgPath = join(dir, "package.json");
     if (existsSync(pkgPath)) {
       try {
         const pkg = JSON.parse(readFileSync(pkgPath, "utf-8"));
         if (pkg.name === "neoboard") return dir;
       } catch {
         /* skip */
       }
     }
-    dir = dirname(dir);
+    const parent = dirname(dir);
+    if (parent === dir) break;
+    dir = parent;
   }
   throw new Error(
     "Could not find NeoBoard project root (package.json with name 'neoboard')",
   );
 }

In Node.js on Windows, what does `path.dirname("C:\\")` return, and will `while (dir !== "/") { dir = path.dirname(dir) }` terminate?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/lib/config.ts` around lines 25 - 36, The loop in the root-detection
logic (variable dir in the function that walks up using dirname) uses a
POSIX-only check while (dir !== "/"), which can hang on Windows; change the
termination to a root-aware condition such as comparing parent === dir or using
path.parse(dir).root (import parse from path if needed) so the loop stops when
dirname(dir) equals dir or dir === parse(dir).root; update the loop condition
around dirname(dir) accordingly in the code that reads package.json.

app/src/components/widget-editor-modal.tsx (3)

405-418: ⚠️ Potential issue | 🟠 Major

Reset preview state when the connector changes.

This handler only clears store query state on cross-type switches. It leaves previewQuery, derived preview data, and autoPreviewTriggered.current untouched, so same-query connector switches stay bound to the previous connection and cross-type switches can still render the last preview until another run. Reset/invalidate preview state here whenever connectionId changes.

🤖 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 405 - 418, The
handleConnectionChange handler must also invalidate preview state whenever
connectionId changes: in handleConnectionChange (alongside setConnectionId and
connectorChanged logic) call the widget editor store actions to clear
previewQuery and any derived preview data (e.g., preview results/state), and
reset autoPreviewTriggered.current to false so auto-preview won't reuse prior
runs; ensure any preview-running flags are cleared as well (reference
handleConnectionChange, useWidgetEditorStore, previewQuery,
autoPreviewTriggered.current).

---

1748-1751: ⚠️ Potential issue | 🟠 Major

Render a settled error state here.

The tooltip trigger is fixed, but this branch still shows the loading spinner whenever connectionId && query.trim() and there is no preview data. A failed first run therefore still looks like endless loading.

Minimal fix

-                      ) : connectionId && query.trim() ? (
+                      ) : previewQuery.isError ? (
+                        <div className="h-full flex items-center justify-center text-sm text-muted-foreground">
+                          Preview unavailable. See the error icon for details.
+                        </div>
+                      ) : connectionId && query.trim() ? (

🤖 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 1748 - 1751, The
current JSX branch shows a spinner whenever connectionId && query.trim() even if
a preview run has already failed; update the render logic in
widget-editor-modal.tsx to detect a settled preview result instead of always
showing the loader: replace the simple condition using connectionId &&
query.trim() with a check that distinguishes isPreviewLoading (or
previewLoading) from a settled state (previewData === null && previewError) and
render an error/empty state when previewError exists (or when previewCompleted
&& !previewData) rather than the spinner; use the existing symbols connectionId,
query, previewData, previewError and isPreviewLoading to implement this
conditional and render the error UI in that branch.

---

239-242: ⚠️ Potential issue | 🟠 Major

This chart-type reset guard still misclassifies hydration.

applyingTemplateRef covers the add-mode template picker, but editInitialChartTypeRef is only armed for widget edit mode and only tested for non-null. lab-edit hydration still falls through to Line 642 and overwrites saved chartOptions with defaults, and if the initial setChartType() is a no-op the ref stays armed so the first real user switch skips the reset instead. Track the source of the change (hydrate vs user) instead of using a sentinel value.

Also applies to: 487-487, 623-644

🤖 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 239 - 242, The
current sentinel editInitialChartTypeRef misclassifies hydration vs user changes
and lets hydration overwrite saved chartOptions; replace it with a
source-tracking ref (e.g., chartTypeChangeSourceRef) that stores 'hydrate' |
'user' | 'template' (or similar) and update all places that change chart type:
set the ref to 'hydrate' during data hydration, to 'template' when
applyingTemplateRef logic runs, and to 'user' on explicit user interactions
(including the initial setChartType triggered by the UI); then modify the reset
logic that currently checks editInitialChartTypeRef to instead skip the
chartOptions reset only when source === 'hydrate' and ensure any no-op
setChartType still updates the source to 'user' when it originates from a user
action so the guard doesn't remain armed.

🤖 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/components/widget-editor-modal.tsx`:
- Around line 754-765: The debounced useEffect (using prevQueryRef, query and
handlePreview) is firing for programmatic updates (modal hydration/template
apply); fix by preventing programmatic setQuery from appearing as a "change":
either (A) when you perform programmatic updates (the places that call setQuery
during modal open/template apply) also set prevQueryRef.current = newQuery
immediately so the effect sees no change, or (B) add a ref like
isProgrammaticUpdateRef that you set true before calling setQuery and false
after, and update the effect to skip scheduling handlePreview when
isProgrammaticUpdateRef.current is true; update references in the modal
hydration/template-apply code paths and the useEffect that uses prevQueryRef and
handlePreview accordingly.

In `@cli/src/lib/config.ts`:
- Around line 120-135: readLocalConfig currently parses JSON without validating
its shape, so a corrupted file (e.g., { "mode": "prod" }) can return an invalid
mode; update readLocalConfig to validate the parsed object conforms to the
LocalConfig shape (specifically that parsed.mode is exactly "docker" or "local")
and otherwise return DEFAULT_LOCAL_CONFIG. Inside readLocalConfig, after
JSON.parse, check typeof parsed === "object" && (parsed.mode === "docker" ||
parsed.mode === "local") before returning; if validation fails, return
DEFAULT_LOCAL_CONFIG (optionally overwrite paths.localConfig with a sanitized
DEFAULT_LOCAL_CONFIG via writeLocalConfig). Ensure getMode still returns
readLocalConfig().mode and no other callers rely on unvalidated values.

---

Duplicate comments:
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 405-418: The handleConnectionChange handler must also invalidate
preview state whenever connectionId changes: in handleConnectionChange
(alongside setConnectionId and connectorChanged logic) call the widget editor
store actions to clear previewQuery and any derived preview data (e.g., preview
results/state), and reset autoPreviewTriggered.current to false so auto-preview
won't reuse prior runs; ensure any preview-running flags are cleared as well
(reference handleConnectionChange, useWidgetEditorStore, previewQuery,
autoPreviewTriggered.current).
- Around line 1748-1751: The current JSX branch shows a spinner whenever
connectionId && query.trim() even if a preview run has already failed; update
the render logic in widget-editor-modal.tsx to detect a settled preview result
instead of always showing the loader: replace the simple condition using
connectionId && query.trim() with a check that distinguishes isPreviewLoading
(or previewLoading) from a settled state (previewData === null && previewError)
and render an error/empty state when previewError exists (or when
previewCompleted && !previewData) rather than the spinner; use the existing
symbols connectionId, query, previewData, previewError and isPreviewLoading to
implement this conditional and render the error UI in that branch.
- Around line 239-242: The current sentinel editInitialChartTypeRef
misclassifies hydration vs user changes and lets hydration overwrite saved
chartOptions; replace it with a source-tracking ref (e.g.,
chartTypeChangeSourceRef) that stores 'hydrate' | 'user' | 'template' (or
similar) and update all places that change chart type: set the ref to 'hydrate'
during data hydration, to 'template' when applyingTemplateRef logic runs, and to
'user' on explicit user interactions (including the initial setChartType
triggered by the UI); then modify the reset logic that currently checks
editInitialChartTypeRef to instead skip the chartOptions reset only when source
=== 'hydrate' and ensure any no-op setChartType still updates the source to
'user' when it originates from a user action so the guard doesn't remain armed.

In `@cli/src/lib/config.ts`:
- Around line 25-36: The loop in the root-detection logic (variable dir in the
function that walks up using dirname) uses a POSIX-only check while (dir !==
"/"), which can hang on Windows; change the termination to a root-aware
condition such as comparing parent === dir or using path.parse(dir).root (import
parse from path if needed) so the loop stops when dirname(dir) equals dir or dir
=== parse(dir).root; update the loop condition around dirname(dir) accordingly
in the code that reads package.json.

🪄 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: ef519022-3839-4f35-ab75-bcfc9be465b3

📥 Commits

Reviewing files that changed from the base of the PR and between 627393d and 032de30.

📒 Files selected for processing (2)

• app/src/components/widget-editor-modal.tsx
• cli/src/lib/config.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd -t f "config.ts" cli/src/lib/

Repository: alfredo1996/neoboard

Length of output: 85

---

🏁 Script executed:

cat -n cli/src/lib/config.ts | head -150

Repository: alfredo1996/neoboard

Length of output: 4528

---

🏁 Script executed:

rg "type LocalConfig|interface LocalConfig|DEFAULT_LOCAL_CONFIG" -A 5 -B 1

Repository: alfredo1996/neoboard

Length of output: 1368

---

Add runtime validation to readLocalConfig() to prevent invalid mode values.

JSON.parse() at line 123 has no shape validation. A corrupted .neoboard.local file like { "mode": "prod" } would bypass the LocalConfig type contract and cause getMode() to return an invalid value, contradicting its declared return type of "docker" | "local".

Suggested fix

 export function readLocalConfig(): LocalConfig {
-  if (!existsSync(paths.localConfig)) return DEFAULT_LOCAL_CONFIG;
+  if (!existsSync(paths.localConfig)) return { ...DEFAULT_LOCAL_CONFIG };
   try {
-    return JSON.parse(readFileSync(paths.localConfig, "utf-8"));
+    const parsed: unknown = JSON.parse(readFileSync(paths.localConfig, "utf-8"));
+    if (
+      typeof parsed === "object" &&
+      parsed !== null &&
+      "mode" in parsed &&
+      ((parsed as { mode: unknown }).mode === "docker" ||
+        (parsed as { mode: unknown }).mode === "local")
+    ) {
+      return { mode: (parsed as { mode: "docker" | "local" }).mode };
+    }
+    return { ...DEFAULT_LOCAL_CONFIG };
   } catch {
-    return DEFAULT_LOCAL_CONFIG;
+    return { ...DEFAULT_LOCAL_CONFIG };
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/lib/config.ts` around lines 120 - 135, readLocalConfig currently
parses JSON without validating its shape, so a corrupted file (e.g., { "mode":
"prod" }) can return an invalid mode; update readLocalConfig to validate the
parsed object conforms to the LocalConfig shape (specifically that parsed.mode
is exactly "docker" or "local") and otherwise return DEFAULT_LOCAL_CONFIG.
Inside readLocalConfig, after JSON.parse, check typeof parsed === "object" &&
(parsed.mode === "docker" || parsed.mode === "local") before returning; if
validation fails, return DEFAULT_LOCAL_CONFIG (optionally overwrite
paths.localConfig with a sanitized DEFAULT_LOCAL_CONFIG via writeLocalConfig).
Ensure getMode still returns readLocalConfig().mode and no other callers rely on
unvalidated values.

[coderabbitai]

🧹 Nitpick comments (1)

app/src/components/widget-editor-modal.tsx (1)

1760-1770: Consider extracting nested ternary for readability.

SonarCloud flags the nested ternary here. A small helper or early-return pattern would clarify the four render branches (error → data → waiting → prompt) without changing behavior.

♻️ Optional: extract preview content to a helper

// Inside the component, above the return:
function renderPreviewContent() {
  if (previewQuery.isError && !previewQuery.data && !initialPreviewData) {
    return (
      <div className="flex flex-col items-center justify-center h-full gap-2 text-muted-foreground">
        <AlertCircle className="h-8 w-8 text-destructive" />
        <p className="text-sm font-medium text-destructive">Query failed</p>
        <p className="text-xs max-w-xs text-center">{previewQuery.error.message}</p>
      </div>
    );
  }
  if (previewQuery.data || initialPreviewData) {
    return (
      <CardContainer
        widget={{ /* ... */ }}
        previewData={(previewQuery.data ?? initialPreviewData)!.data}
        previewResultId={(previewQuery.data ?? initialPreviewData)!.resultId}
      />
    );
  }
  if (connectionId && query.trim() && !previewQuery.isError) {
    return (
      <div className="h-full flex items-center justify-center">
        <div className="h-6 w-6 animate-spin rounded-full border-2 border-primary border-t-transparent" />
      </div>
    );
  }
  return (
    <div className="h-full flex items-center justify-center text-sm text-muted-foreground">
      Run a query to see the preview
    </div>
  );
}

🤖 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 1760 - 1770, The
nested ternary rendering the preview is hard to read—extract it into a helper
(e.g., renderPreviewContent) placed inside the component above the return and
replace the ternary with a single call; inside the helper implement four
explicit branches checking previewQuery.isError (and absence of
previewQuery.data/initialPreviewData) to render the error block,
previewQuery.data || initialPreviewData to render the CardContainer with
previewData/previewResultId, connectionId && query.trim() &&
!previewQuery.isError to render the spinner, and a final default that returns
the "Run a query to see the preview" prompt, using the existing symbols
previewQuery, initialPreviewData, connectionId, query, and CardContainer.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 1760-1770: The nested ternary rendering the preview is hard to
read—extract it into a helper (e.g., renderPreviewContent) placed inside the
component above the return and replace the ternary with a single call; inside
the helper implement four explicit branches checking previewQuery.isError (and
absence of previewQuery.data/initialPreviewData) to render the error block,
previewQuery.data || initialPreviewData to render the CardContainer with
previewData/previewResultId, connectionId && query.trim() &&
!previewQuery.isError to render the spinner, and a final default that returns
the "Run a query to see the preview" prompt, using the existing symbols
previewQuery, initialPreviewData, connectionId, query, and CardContainer.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4386b08f-823f-47f8-9b7a-09ba899fd651

📥 Commits

Reviewing files that changed from the base of the PR and between 032de30 and 17db752.

📒 Files selected for processing (3)

• app/e2e/widget-states.spec.ts
• app/e2e/widgets.spec.ts
• app/src/components/widget-editor-modal.tsx

✅ Files skipped from review due to trivial changes (1)

• app/e2e/widgets.spec.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• app/e2e/widget-states.spec.ts

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

cli/src/commands/db/seed.ts (1)

8-15: ⚠️ Potential issue | 🔴 Critical

Blacklist validation here still leaves arg injection open.

assertSafeValue() rejects quotes and $, but it still allows spaces and tabs, so values like neo4j --debug are split into extra argv in both cypher-shell calls. It also rejects many otherwise valid passwords. Please pass these credentials as discrete args, or shell-escape each token before interpolation.

Also applies to: 32-35, 51-56

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/commands/db/seed.ts` around lines 8 - 15, The current
assertSafeValue(value, label) whitelist/blacklist is insufficient because it
allows whitespace and other token-delimiting characters which lets values like
"neo4j --debug" split into extra argv when later interpolated into cypher-shell
calls; update the fix by either (A) changing assertSafeValue to explicitly
reject any whitespace (e.g., test for /\s/), newlines and other token separators
so credentials can't contain spaces/tabs, and update its callers to validate via
this tightened rule, or (B) better yet, stop interpolating credentials into a
single shell string and instead invoke cypher-shell with discrete args (e.g.,
pass user, password, and URI as separate argv entries) or properly shell-escape
every token before interpolation; reference assertSafeValue and the cypher-shell
invocation sites and apply one of these two fixes consistently for all callers
(including the other occurrences noted around lines 32-35 and 51-56).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/src/commands/db/reset.ts`:
- Around line 22-29: getDatabaseHost currently returns "localhost" on parse or
read failures which allows the destructive reset to proceed; change
getDatabaseHost to fail closed by returning undefined/null when the .env is
missing or DATABASE_URL cannot be unambiguously parsed, only return "localhost"
or "127.0.0.1" when the regex explicitly matches those local hosts, and update
the reset flow that calls getDatabaseHost (the destructive reset logic in this
file) to abort immediately unless getDatabaseHost positively verifies a local
host.

In `@cli/src/commands/db/seed.ts`:
- Around line 18-26: The check in assertSafePath is insecure because startsWith
can be fooled by sibling prefixes; replace it with a boundary-safe check using
path.relative: compute const resolved = resolve(paths.root, scriptPath) and
const rel = relative(normalize(paths.root), normalize(resolved)); throw if
rel.startsWith('..') || rel === '..' or path.isAbsolute(rel) &&
rel.includes('..'); also ensure the function returns the validated absolute path
(change assertSafePath to return string instead of void) and update callers
(e.g., where node is invoked around the seed execution) to use the returned
resolved absolute path rather than constructing paths.root + config.seed.script
so the executed script cannot escape the repo.

---

Duplicate comments:
In `@cli/src/commands/db/seed.ts`:
- Around line 8-15: The current assertSafeValue(value, label)
whitelist/blacklist is insufficient because it allows whitespace and other
token-delimiting characters which lets values like "neo4j --debug" split into
extra argv when later interpolated into cypher-shell calls; update the fix by
either (A) changing assertSafeValue to explicitly reject any whitespace (e.g.,
test for /\s/), newlines and other token separators so credentials can't contain
spaces/tabs, and update its callers to validate via this tightened rule, or (B)
better yet, stop interpolating credentials into a single shell string and
instead invoke cypher-shell with discrete args (e.g., pass user, password, and
URI as separate argv entries) or properly shell-escape every token before
interpolation; reference assertSafeValue and the cypher-shell invocation sites
and apply one of these two fixes consistently for all callers (including the
other occurrences noted around lines 32-35 and 51-56).

🪄 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: d657ac52-da0b-4239-87f4-c93f30d43f0d

📥 Commits

Reviewing files that changed from the base of the PR and between 17db752 and d8b0573.

📒 Files selected for processing (3)

• cli/src/__tests__/commands/db/seed.test.ts
• cli/src/commands/db/reset.ts
• cli/src/commands/db/seed.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• cli/src/tests/commands/db/seed.test.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't default the safety check to localhost.

When .env is missing or DATABASE_URL cannot be parsed, getDatabaseHost() returns "localhost" and the destructive reset flow still proceeds. This guard should fail closed and only continue when the host was positively verified as local.

Possible fix

-function getDatabaseHost(): string {
+function getDatabaseHost(): string | null {
   try {
     const content = readFileSync(paths.envFile, "utf-8");
     const match = content.match(/DATABASE_URL=.*@([^:/]+)/);
-    return match?.[1] ?? "localhost";
+    return match?.[1] ?? null;
   } catch {
-    return "localhost";
+    return null;
   }
 }
@@
-  if (!isLocalhost(host)) {
+  if (!host || !isLocalhost(host)) {

Also applies to: 40-47

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/commands/db/reset.ts` around lines 22 - 29, getDatabaseHost currently
returns "localhost" on parse or read failures which allows the destructive reset
to proceed; change getDatabaseHost to fail closed by returning undefined/null
when the .env is missing or DATABASE_URL cannot be unambiguously parsed, only
return "localhost" or "127.0.0.1" when the regex explicitly matches those local
hosts, and update the reset flow that calls getDatabaseHost (the destructive
reset logic in this file) to abort immediately unless getDatabaseHost positively
verifies a local host.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

The project-root check is prefix-based and can be escaped.

startsWith(normalize(paths.root)) treats sibling paths like /repo/app-malicious/... as if they were under /repo/app, so ../app-malicious/seed.js passes validation. The later node ${paths.root}/${config.seed.script} call will resolve that traversal and execute a script outside the repo. Use a boundary-safe relative/canonical path check and propagate the validated absolute path.

Also applies to: 66-66

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/src/commands/db/seed.ts` around lines 18 - 26, The check in
assertSafePath is insecure because startsWith can be fooled by sibling prefixes;
replace it with a boundary-safe check using path.relative: compute const
resolved = resolve(paths.root, scriptPath) and const rel =
relative(normalize(paths.root), normalize(resolved)); throw if
rel.startsWith('..') || rel === '..' or path.isAbsolute(rel) &&
rel.includes('..'); also ensure the function returns the validated absolute path
(change assertSafePath to return string instead of void) and update callers
(e.g., where node is invoked around the seed execution) to use the returned
resolved absolute path rather than constructing paths.root + config.seed.script
so the executed script cannot escape the repo.

[sonarqubecloud]

Quality Gate failed

Failed conditions
2 Security Hotspots
74.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud