chore(devex): fail-fast HMAC, seed-from-host fix, dev DNS warning (#907, #898, #899) - #939
Merged
Merged
Conversation
Findings from pre-polish review (umbrella #895): - code: add E2E to after-coding; release/1.1 branch awareness - next: auto-detect release/X.Y as base branch (instead of hard-coded dev) - github-workflow: fix frontmatter name mismatch (was 'github'); expand label list to match repo - issue: expand label list to match real GH labels (a11y, design, devex, etc.) - code-reviewer: add E2E test step (was unit-only) - test-runner: sharpen Docker conditional; clarify destroy-before-E2E rule - ux-crawler + user-sim-creator: replace ghost user 'bob@example.com' with seeded 'creator@neoboard.local' (pending #921) - CLAUDE.md: /github -> /github-workflow skill ref; clarify code-reviewer test scope - NEW: skills/deploy — production-readiness audit skill (capture-don't-fix, 5 sections, destructive-step approval gate) Pre-polish review issues filed: #921, #922, #923, #924, #925. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves the gap where the SSO management UI rendered fully on community
installs. The existing inline NEOBOARD_EDITION check on /api/sso-providers
was kept (already returned 403 forbidden) — this PR replaces it with the
canonical requireFeature("sso") guard (returns 402 ENTERPRISE_REQUIRED)
and adds defense-in-depth + a client-side gate so the UI itself never
renders on community.
Changes:
- /api/sso-providers (admin CRUD): replace inline edition check with
requireFeature("sso"); now returns 402 ENTERPRISE_REQUIRED instead of 403
- /api/auth/sso-providers (public login route): short-circuit to empty
response on community before any DB read (defense-in-depth — even stale
rows or env-provider misconfig can't leak)
- New useFeatures()/useFeature() hook: TanStack Query, 5-min staleTime,
reads /api/features
- New <FeatureGate feature="..."> component for declarative client gating
- New <EnterpriseRequiredEmptyState feature="..."> reusable empty state
with auto-generated copy per feature + upgrade CTA
- Settings layout: filter Authentication tab by sso feature; hidden on
community (avoids dead-end and UI flicker during initial load)
- Settings/authentication page: wrap content in FeatureGate; community
users see the EnterpriseRequiredEmptyState directly
Tests:
- 49 unit tests pass (6 new useFeatures + 6 FeatureGate + 1 new community-
edition page test + updated route tests + all pre-existing tests)
- New E2E spec app/e2e/sso-gating.spec.ts covers community-mode UI +
API gating
- Enterprise-mode E2E coverage filed as #933 (requires second Playwright
worker — substantial global-setup overhaul, out of scope)
Drill brief: claude_code_docs/plans/issue-906.md
Closes #906
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three fixes from the CodeRabbit review: 1. POST + PATCH beforeEach in route.test.ts now pin NEOBOARD_EDITION=enterprise (deterministic across ambient envs; previously could flip to 402 if community env leaked in) 2. Added 402 ENTERPRISE_REQUIRED contract tests for POST, DELETE, PATCH — previously only GET covered the new contract; now full CRUD surface is locked 3. sso-gating.spec.ts: /api/sso-providers test uses page.request.get(...) instead of the global request context. Fixes the CI ECONNRESET seen on E2E shard 4/5 (global request raced with server startup) and matches the authenticated journey set up in the describe block's beforeEach All 28 route tests pass. Local E2E deferred per user direction (Docker teardown cost outweighs benefit for this small fix-up; CI will verify). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eadonly Addresses two SonarCloud findings on #934 (rule typescript:S6759, MINOR): - app/src/components/feature-gate.tsx:34 - app/src/components/enterprise-required-empty-state.tsx:78 Per the codebase pattern (e.g. save-template-dialog.tsx, dashboard-picker-dialog.tsx), mark each prop interface field with the `readonly` modifier. 6 feature-gate tests pass. Local E2E deferred to CI (no functional change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
security(auth): gate SSO settings page + API by enterprise edition
Closes #916. Unblocks #915. ## Server (app/src/app/api/dashboards/import/route.ts) - Accept `connectionMapping` + `skippedConnections` for BOTH formats - NeoDash imports now honor a `neodash-default` placeholder mapping - Cross-tenant safety: mapping validation now scoped to (userId, tenantId) - Response envelope adds `notes: string[]` (additive — existing clients reading only `id` continue to work) - Notes thread through from the converter (chart-type downgrades) plus new import-time notes (skipped connections, unmapped widget counts) ## Converter (app/src/lib/dashboard/neodash-converter.ts) - `convertNeoDash(json, defaultConnectionId?)` — accepts a connection id to stamp on every widget. Falls back to "" (legacy behavior) when omitted. - `convertNeoDashWithNotes` mirrors the signature ## Dialog (app/src/app/(dashboard)/page.tsx) - NeoDash imports synthesize a single placeholder `neodash-default` with type `neo4j` and surface it in the mapping UI (no longer silently uses empty connectionId) - New "Skip" checkbox per mapping row — widgets using a skipped key import with connectionId="" and a note in the result - Empty-targets UX: when no compatible connection exists for the placeholder type, the select is disabled and a helper line offers "Create one" (opens /connections in a new tab) or "Skip" - Post-success view replaces the form: dashboard name + notes list + "Stay here" / "View dashboard" buttons (no longer auto-redirects so users can read import notes carefully) ## Hook (app/src/hooks/use-dashboards.ts) - `ImportDashboardResult extends DashboardDetail` adds `notes: string[]` - `ImportDashboardInput` accepts optional `skippedConnections` ## Tests (app/src/app/api/dashboards/import/__tests__/route.test.ts) - 5 new contract tests: - notes envelope is always present - NeoDash with mapped connection - NeoDash with skipped placeholder → warning note - NeoBoard with skipped → unmapped-widget note - cross-tenant mapping rejected (400) - All 54 unit tests pass ## Out of scope (filed for follow-up) - Inline "Create new connection" affordance inside the import dialog — current "open /connections in new tab" is the fallback flagged in the drill - NeoDash converter content fidelity (params, markdown body) — that's #915 which now has the notes infra it needs to land cleanly ## Drill brief claude_code_docs/plans/issue-916.md Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…h mapping The PR #935 dialog redesign changed two behaviors that the existing import E2E tests assumed: 1. No auto-redirect after Import — dialog now shows a notes summary with View/Stay buttons; tests must click "View dashboard" before waitForURL 2. NeoDash imports now show a synthesized Neo4j placeholder mapping row instead of skipping the mapping step entirely Fixes 4 failing E2E tests on shards 2/5 and 3/5: - dashboard-portability.spec.ts:52 — NeoBoard format import - dashboard-portability.spec.ts:141 — NeoDash chart-type mapping - dashboard-portability.spec.ts:189 — NeoDash unsupported-type fallback - import-validation.spec.ts:124 — multi-connection mapping happy path For the NeoDash tests, the new placeholder is skipped via the Skip checkbox (these tests assert chart-type behavior, not connection wiring — widgets render regardless of connection presence). Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The synthesized placeholder name was "Neo4j connection (<title>)", which
duplicated the dashboard title that's already shown above in the parsed-
preview box. In strict-mode E2E selectors this caused dashboard-portability
spec line 145 to fail: dialog.getByText("E2E NeoDash Import Test") resolved
to 2 elements (the preview header AND the placeholder row).
Placeholder name is now just "Neo4j connection" — semantically just as
clear (user sees the type "neo4j" beneath it) and avoids the collision.
Local E2E deferred to CI per session pattern.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…edesign feat(import): unify NeoBoard + NeoDash flow with mapping UI + notes
Closes #915. Three NeoDash converter bugs fixed in one place. ## Bug A — top-level params dropped NeoDash stores dashboard-wide params in `nd.settings.parameters`; NeoBoard has no global params (they're outputs of parameter-select widgets). The converter now: 1. Regex-scans every converted widget query for `$param_<name>` references 2. For each defined param that's referenced → creates a parameter-select widget with inferred type + default value on a NEW "Filters" page (prepended as page 1) 3. For each defined param that's NOT referenced → skip + note 4. For each referenced-but-undefined param → create with no default + warn Type inference: array→multi-select, finite number→number-range, empty string→text, otherwise→select (NeoDash's most common case). Strips the legacy "neodash_" prefix from param names so the generated widget produces `$param_<name>` matching what queries reference (paired with `convertParamSyntax` which already rewrites `$neodash_X` → `$param_X` in queries before scanning). Filter widgets tile 4-per-row at w=3 h=2, connectionId="" (no data). ## Bug B — markdown content dropped NeoDash stored markdown body in `report.query`. Markdown widget reads from `settings.content`. Converter now: - Routes `report.query` into `settings.content` when chartType is markdown - Clears widget.query (markdown is content-only — no query path needed) - Emits per-widget note: 'Imported markdown content for "<title>"' ## Bug C — silent failure mode Uses the existing notes infrastructure from #916 / PR #935 to surface every conversion decision. Notes per the drill (#915 brief): per-param explicit notes so user knows exactly what happened. Acceptable verbosity trade-off — terse summary alternative was considered and rejected. ## Tests 30 new pure-function unit tests cover: - isNeoDashFormat (4) - inferParameterType — all branches (6) - extractParamReferences — happy + edges (5) - Markdown content routing (4) - Filters-page generation (8) - defaultConnectionId behavior (3) Plus all 54 existing tests across the converter / route / dashboard suite continue to pass. Build + type-check green. ## Out of scope (per drill) - Reference detection beyond queries (titles, click-action params, styling rules) — first pass scans queries only - Auto-wiring seed queries for select-typed params — user configures in the editor - Markdown that contains `$param_*` substitutions — NeoDash didn't do inline substitution; literal copy Drill brief: claude_code_docs/plans/issue-915.md Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Extract buildFiltersPage() helper (Sonar S3776: cognitive complexity 20 → 15) - Drop unnecessary type assertion on nd.settings?.parameters (Sonar S4325) - number-range rangeMin = min(default, 0) — supports negative defaults (CR) - Update test fixtures to use $neodash_* syntax so tests exercise the full conversion path instead of bypassing it (CR — 4 tests) - Add original-widget query-rewrite assertion (CR nitpick) - Add explicit negative-default test for rangeMin widening - Fix 2 pre-existing tests at app/src/lib/__tests__/dashboard/ to expect the Filters page at pages[0] (original page now at pages[1] when params are referenced) Local: 2834/2834 tests pass; build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ter-fidelity fix(import): preserve NeoDash markdown + auto-generate parameter widgets
…h hierarchical Closes #917. ## Bug fix (root) `app/src/plugins/graph/settings.ts:layout` now accepts `"hierarchical"` — the type was already in `component/src/charts/graph-chart.tsx:25` but the Zod enum was missing it, causing widgets to crash on previously-saved hierarchical-layout configs. ## Resilience pattern (cross-cutting) New helper `app/src/lib/plugin/safe-parse-settings.ts`: - Wraps `schema.safeParse` with a fallback to schema defaults - Logs a structured warning via `console.warn` on failure (browser-safe; pino is server-only — bundling it into plugin components blows up webpack with `node:crypto` unhandled scheme) - Re-throws only when the schema ITSELF is broken (schema.parse({}) fails) ## Adoption (mechanical, all 20 plugins) Every plugin component migrated from: const settings = <X>SettingsSchema.parse(raw); to: const settings = safeParseSettings(<X>SettingsSchema, raw, "<plugin-id>"); Includes `single-value` which had a manual safeParse fallback — replaced with the helper for consistency + logging. ## Schema audit Cross-checked Zod enums in all 20 plugin settings against chart-side TS types where the chart exports a named union. Only one drift found: graph layout (the root finding). Other plugins don't export named unions, so the safeParse helper provides defense-in-depth. ## Tests - 9 helper unit tests cover: success, failure with defaults, structured log payload, passthrough preservation, undefined/null, missing fields, broken-schema propagation, pluginId in payload - 2843/2843 total tests pass (+9 new) - Build + type-check green ## Out of scope (per drill) - UI badge on widget header when fallback fires (silent log decided) - Compile-time `satisfies` enforcement of schema ⊆ chart-type (deferred; filed as a possible follow-up if drift recurs) Drill brief: claude_code_docs/plans/issue-917.md Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ugins Adds a single parameterized render test that exercises every plugin's component with deliberately-invalid settings, covering the safeParseSettings call site in each of the 20 plugin component files. Why: SonarCloud new_coverage gate failed on #937 — the 20 mechanical 1-line plugin migrations counted as "new code" with no direct coverage. Plugin components don't have unit tests by convention (they're covered via E2E), but the gate doesn't know that. This test lifts new_coverage above the 80% threshold by exercising each plugin's component once. Each test: - Renders plugin.component with garbage settings via @testing-library/react - Asserts no throw (proves safeParseSettings caught the validation failure and returned defaults instead of crashing) Heavy deps stubbed: @neoboard/components widgets, next/dynamic, the heavier internal components that use TanStack Query (table-renderer, form-widget- renderer, graph-exploration-wrapper). 21 new tests pass (20 plugins + 1 sanity check on the list). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rse-pattern fix(plugins): graceful settings fallback via safeParseSettings + graph hierarchical layout
Closes #904. Final Phase 2 PR. ## Root cause `app/src/app/(dashboard)/[id]/page.tsx` (lines 148-160) uses sessionStorage to baseline the dashboard version, then fires a "Dashboard updated by X" banner whenever the refetched server version exceeds the stored baseline. That comparison fires on every SELF-save: after a successful PUT, the server bumps version N → N+1; the refetch sees N+1; sessionStorage still says N; banner fires with the user's own name. Then on revisit the banner triggers again or stays stale. ## Fix Update `useUpdateDashboard.onSuccess` to write the new version to sessionStorage BEFORE invalidating the cache. TanStack Query guarantees onSuccess runs before invalidateQueries' refetch lands, so the baseline is in place by the time the detail page's effect reads it. Other-user saves still trigger the banner correctly — they don't run through this user's mutation onSuccess. ## Defense-in-depth via updatedBy === userId (NOT in this PR) Considered during drill but rejected: would require exposing `dashboard.updatedBy` (user UUID) in the API response, which isn't there today. The primary fix solves the actual race; defense-in-depth is unnecessary for the realistic threat model. ## Tests - Unit (4 new cases on `useUpdateDashboard`): - PUT call shape (mutationFn) - onSuccess writes new version to sessionStorage - onSuccess skips write when result has no version field - onSuccess skips write when version is non-numeric - E2E (`dashboard-states.spec.ts`): full flow — create dashboard, save, navigate away, navigate back, assert no "Dashboard updated by" banner 2868/2868 unit tests pass; build green. Drill brief: claude_code_docs/plans/issue-904.md Local E2E deferred to CI per session pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ot list CI E2E shard 2/5 failed because the test expected `page.waitForURL(/\/dashboards/)` after clicking "Back", but Back actually navigates to /<id> (view mode), not the dashboards list. View mode is where the version-bump effect runs anyway, so the simpler flow exercises the bug directly: 1. Create dashboard → edit mode (version=1) 2. Save → server bumps to version=2; onSuccess writes 2 to sessionStorage 3. Click Back → /<id> view mode 4. View page's effect: refetch sees version=2, sessionStorage says 2 → NO banner Also added per-test unique dashboard name (timestamp suffix) and cleanup at the end to avoid polluting later tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix(dashboard): suppress self-save 'updated by' banner on revisit
Closes #907 — promote API_KEY_HMAC_SECRET from optional to required so the app refuses to start without it. Previously API key creation would surface a cryptic runtime error; now we fail fast at startup like ENCRYPTION_KEY and NEXTAUTH_SECRET. CLI's `neoboard env init` now generates this secret alongside ENCRYPTION_KEY so a fresh setup is still one command. Closes #898 — hardcode `localhost` in scripts/seed-demo.mjs. Previously the script honoured NEO4J_HOST/PG_HOST env vars; when seeding ran inside the docker-app container, those resolved to container names (e.g. `db`) which the host-side dev server then couldn't reach. Docker compose publishes the ports to localhost anyway, so the host-form is correct everywhere. Closes #899 — add a dev-only DNS-resolution check that warns about seeded connections whose URIs point to unreachable hosts. Fire-and-forget so startup never blocks; falls back to a no-op outside development. Warns once per affected connection with a concrete fix hint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds feature-flag hooks/components and SSO gating, redesigns dashboard import (NeoDash conversion, skipped-mapping, notes, UI), rolls out safe plugin-settings parsing across plugins, requires API_KEY_HMAC_SECRET and CLI generation, adds dev hostname checks, and mandates Playwright E2E in CI/docs. ChangesFeature Gating & SSO
Dashboard Import & NeoDash Conversion
Plugin Settings Safety
Env & Dev Startup Hardening
Workflows & Docs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
CI's TypeScript type-check rejected `vi.fn()` assigned to a `(message: string) => void` slot. Use the typed `vi.fn<T>()` overload so the mock satisfies the callable signature while still exposing `.mock`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.claude/skills/next/SKILL.md (2)
83-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPR creation still hardcodes
devinstead of the detected base.Line 83 conflicts with Line 43 and can target the wrong base branch during active release cycles.
Suggested fix
- --base dev \ + --base "$BASE" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/next/SKILL.md around lines 83 - 84, The PR creation command in SKILL.md currently hardcodes "--base dev" which can target the wrong branch; replace the hardcoded token "--base dev" with the detected base variable used elsewhere (e.g., the detected base placeholder or environment variable like "--base \"$BASE_BRANCH\"" or the project’s {{base}} placeholder) so the command uses the computed base branch rather than "dev" (update the string that contains "--base dev" to reference the detected base variable).
17-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnassigned-issue filter references a field not requested in
--json.The jq expression checks
.assignees, butassigneesis missing from the selected fields, so this filter is unreliable.Suggested fix
-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 --limit 10 --json number,title,labels,milestone,body,assignees --jq '[.[] | select((.assignees | length) == 0)] | sort_by(.milestone.title) | .[0:5]'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/next/SKILL.md at line 17, The GH CLI invocation builds a jq filter that inspects .assignees but the --json list (number,title,labels,milestone,body) does not include assignees, so the select(.assignees | length == 0) will fail; update the command used in the file (the gh issue list ... --json ... string) to include assignees in the --json fields (add "assignees") OR change the jq predicate to check a field that is actually requested (e.g., use labels or assignee count already present), ensuring the jq expression and the --json selection stay consistent (refer to the gh issue list invocation and the jq select(.assignees | length == 0) expression).app/src/__tests__/instrumentation.test.ts (1)
144-170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
API_KEY_HMAC_SECRETin fail-fast stderr output.The missing-vars test now deletes this required variable but never verifies it is reported, so regressions in the startup error message can slip through.
Suggested test assertion
expect(stderrOutput).toContain("ENCRYPTION_KEY"); expect(stderrOutput).toContain("NEXTAUTH_SECRET"); expect(stderrOutput).toContain("DATABASE_URL"); + expect(stderrOutput).toContain("API_KEY_HMAC_SECRET");Based on learnings: "Every new behavior, bug fix, and edge case must have a test."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/__tests__/instrumentation.test.ts` around lines 144 - 170, The test "calls process.exit(1) when required vars are missing" currently deletes API_KEY_HMAC_SECRET but doesn't assert it appears in the stderr fail-fast output; update the assertions after building stderrOutput (used with stderrSpy.mock.calls) to include expect(stderrOutput).toContain("API_KEY_HMAC_SECRET") so the test verifies that register() reports the missing API_KEY_HMAC_SECRET alongside ENCRYPTION_KEY, NEXTAUTH_SECRET and DATABASE_URL.
🧹 Nitpick comments (1)
app/src/lib/__tests__/dashboard/neodash-converter.test.ts (1)
1-7: 💤 Low valueNon-standard test file location.
Per coding guidelines, test files should be co-located in
__tests__/next to the file under test. This file is atlib/__tests__/dashboard/neodash-converter.test.tsbut there's already a co-located test atlib/dashboard/__tests__/neodash-converter.test.ts. Consider consolidating to avoid duplicate maintenance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/__tests__/dashboard/neodash-converter.test.ts` around lines 1 - 7, This test file is a duplicate in a non-standard location; remove or consolidate it into the co-located test under the same module to avoid duplicate coverage and maintenance. Delete this file (app/src/lib/__tests__/dashboard/neodash-converter.test.ts) or move its unique assertions into the existing co-located test that already imports isNeoDashFormat, convertNeoDash, and convertNeoDashWithNotes (from neodash-converter) and ensure any unique cases here are merged into lib/dashboard/__tests__/neodash-converter.test.ts; after merging, run the test suite to confirm no import paths or duplicate-test issues remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/agents/test-runner.md:
- Line 12: Update the guidance that currently says “Destroy Docker before E2E”:
instead of instructing to kill all containers globally, limit teardown to this
repo’s compose stack by running a docker-compose down with volume removal in the
repository context or by specifying the project name/project directory so only
NeoBoard resources are removed; replace the global "docker ps" / "destroy all
containers" wording with this scoped teardown instruction and keep the
subsequent step to bring the compose stack up and wait for healthchecks.
In @.claude/skills/code/SKILL.md:
- Line 51: The sentence referencing a machine-specific memory link for the
"release/X.Y" exception is not portable; remove the absolute path link and
either replace it with a repo-relative documentation path or drop the link
entirely and keep only the command-based check (`git branch -r | grep
'origin/release/'`), updating the line that mentions the `release/X.Y` branch so
it no longer points to `.claude/projects/-Users-...` but to a stable repo doc or
the command check.
In @.claude/skills/deploy/SKILL.md:
- Line 248: Update the broken relative link in .claude/skills/deploy/SKILL.md
that currently points to "../issue/skill.md" — change the target to the correct
case-sensitive path "../issue/SKILL.md" so the "issue skill" link resolves
properly on case-sensitive filesystems; locate the link text "issue skill" and
replace the href string "../issue/skill.md" with "../issue/SKILL.md".
- Around line 228-246: The fenced code block that begins with "## Deployment
audit findings (YYYY-MM-DD)" is missing a language identifier, triggering MD040;
update the opening fence from ``` to include a language tag such as ```markdown
(or ```text) so the block becomes a labeled fenced code block; locate the block
in .claude/skills/deploy/SKILL.md (look for the "Deployment audit findings
(YYYY-MM-DD)" heading and the following triple-backtick fence) and change only
the opening fence to include the language identifier.
In @.claude/skills/next/SKILL.md:
- Around line 34-37: The BASE assignment strips the release/ prefix (awk -F/
'{print $NF}') causing git fetch/checkout to use a non-existent branch; update
the extraction to preserve the full branch name (e.g., replace awk -F/ '{print
$NF}' with either sed 's#.*refs/heads/##' or awk -F'/' '{print $(NF-1)"/"$NF}')
so BASE becomes "release/1.2" and the subsequent git fetch origin "$BASE" && git
checkout "$BASE" && git pull origin "$BASE" work correctly.
In `@app/src/lib/dev/verify-connection-hosts.ts`:
- Around line 83-90: The message currently logs full decrypted connection URIs
(unresolvable.map c.uri), which may contain credentials; change it to redact
credentials by extracting and logging only host-level info (e.g., use the URL
parser on each c.uri and log url.host or `${url.hostname}${url.port ?
':'+url.port : ''}`) instead of the raw c.uri so usernames/passwords are never
written to logs; update the string construction that references unresolvable and
PROMPT_HINT accordingly (keep connection name and type, replace c.uri with the
parsed host-only value).
- Around line 106-120: The current fetchConnections passed into
verifyConnectionHostsImpl queries all rows from schema.connections without
tenant scoping; update the fetchConnections implementation to restrict by tenant
(e.g., add a .where(schema.connections.tenantId, '=', tenantId) or equivalent)
so only the current tenant's rows are returned, and ensure tenantId is obtained
and propagated (either capture it in the async fetchConnections closure or
extend verifyConnectionHostsImpl to accept a tenantId argument); update any
calling code and types (SeededConnection usage) accordingly so every db.select
on schema.connections includes the tenant filter.
---
Outside diff comments:
In @.claude/skills/next/SKILL.md:
- Around line 83-84: The PR creation command in SKILL.md currently hardcodes
"--base dev" which can target the wrong branch; replace the hardcoded token
"--base dev" with the detected base variable used elsewhere (e.g., the detected
base placeholder or environment variable like "--base \"$BASE_BRANCH\"" or the
project’s {{base}} placeholder) so the command uses the computed base branch
rather than "dev" (update the string that contains "--base dev" to reference the
detected base variable).
- Line 17: The GH CLI invocation builds a jq filter that inspects .assignees but
the --json list (number,title,labels,milestone,body) does not include assignees,
so the select(.assignees | length == 0) will fail; update the command used in
the file (the gh issue list ... --json ... string) to include assignees in the
--json fields (add "assignees") OR change the jq predicate to check a field that
is actually requested (e.g., use labels or assignee count already present),
ensuring the jq expression and the --json selection stay consistent (refer to
the gh issue list invocation and the jq select(.assignees | length == 0)
expression).
In `@app/src/__tests__/instrumentation.test.ts`:
- Around line 144-170: The test "calls process.exit(1) when required vars are
missing" currently deletes API_KEY_HMAC_SECRET but doesn't assert it appears in
the stderr fail-fast output; update the assertions after building stderrOutput
(used with stderrSpy.mock.calls) to include
expect(stderrOutput).toContain("API_KEY_HMAC_SECRET") so the test verifies that
register() reports the missing API_KEY_HMAC_SECRET alongside ENCRYPTION_KEY,
NEXTAUTH_SECRET and DATABASE_URL.
---
Nitpick comments:
In `@app/src/lib/__tests__/dashboard/neodash-converter.test.ts`:
- Around line 1-7: This test file is a duplicate in a non-standard location;
remove or consolidate it into the co-located test under the same module to avoid
duplicate coverage and maintenance. Delete this file
(app/src/lib/__tests__/dashboard/neodash-converter.test.ts) or move its unique
assertions into the existing co-located test that already imports
isNeoDashFormat, convertNeoDash, and convertNeoDashWithNotes (from
neodash-converter) and ensure any unique cases here are merged into
lib/dashboard/__tests__/neodash-converter.test.ts; after merging, run the test
suite to confirm no import paths or duplicate-test issues remain.
🪄 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: a9038f7c-71cd-4eb0-a9f2-8a6c3e932909
📒 Files selected for processing (67)
.claude/agents/code-reviewer.md.claude/agents/test-runner.md.claude/agents/user-sim-creator.md.claude/agents/ux-crawler.md.claude/skills/code/SKILL.md.claude/skills/deploy/SKILL.md.claude/skills/github-workflow/SKILL.md.claude/skills/issue/SKILL.md.claude/skills/next/SKILL.mdCLAUDE.mdapp/e2e/dashboard-portability.spec.tsapp/e2e/dashboard-states.spec.tsapp/e2e/import-validation.spec.tsapp/e2e/sso-gating.spec.tsapp/src/__tests__/instrumentation.test.tsapp/src/app/(dashboard)/page.tsxapp/src/app/(dashboard)/settings/authentication/__tests__/page.test.tsxapp/src/app/(dashboard)/settings/authentication/page.tsxapp/src/app/(dashboard)/settings/layout.tsxapp/src/app/api/auth/sso-providers/__tests__/route.test.tsapp/src/app/api/auth/sso-providers/route.tsapp/src/app/api/dashboards/import/__tests__/route.test.tsapp/src/app/api/dashboards/import/route.tsapp/src/app/api/sso-providers/__tests__/route.test.tsapp/src/app/api/sso-providers/route.tsapp/src/components/__tests__/feature-gate.test.tsxapp/src/components/enterprise-required-empty-state.tsxapp/src/components/feature-gate.tsxapp/src/hooks/__tests__/use-dashboards.test.tsapp/src/hooks/__tests__/use-features.test.tsapp/src/hooks/use-dashboards.tsapp/src/hooks/use-features.tsapp/src/instrumentation.tsapp/src/lib/__tests__/dashboard/neodash-converter.test.tsapp/src/lib/__tests__/env-config.test.tsapp/src/lib/dashboard/__tests__/neodash-converter.test.tsapp/src/lib/dashboard/neodash-converter.tsapp/src/lib/dev/__tests__/verify-connection-hosts.test.tsapp/src/lib/dev/verify-connection-hosts.tsapp/src/lib/env-config.tsapp/src/lib/plugin/__tests__/safe-parse-settings.test.tsapp/src/lib/plugin/safe-parse-settings.tsapp/src/plugins/__tests__/safe-parse-adoption.test.tsxapp/src/plugins/bar/component.tsxapp/src/plugins/choropleth/component.tsxapp/src/plugins/circle-packing/component.tsxapp/src/plugins/form/component.tsxapp/src/plugins/gantt/component.tsxapp/src/plugins/gauge/component.tsxapp/src/plugins/graph/component.tsxapp/src/plugins/graph/settings.tsapp/src/plugins/iframe/component.tsxapp/src/plugins/json/component.tsxapp/src/plugins/line/component.tsxapp/src/plugins/map/component.tsxapp/src/plugins/markdown/component.tsxapp/src/plugins/parameter-select/component.tsxapp/src/plugins/pie/component.tsxapp/src/plugins/radar/component.tsxapp/src/plugins/sankey/component.tsxapp/src/plugins/single-value/component.tsxapp/src/plugins/sunburst/component.tsxapp/src/plugins/table/component.tsxapp/src/plugins/treemap/component.tsxcli/src/__tests__/commands/env.test.tscli/src/commands/env.tsscripts/seed-demo.mjs
|
|
||
| 1. Run `git diff --name-only HEAD` and `git diff --cached --name-only` to detect changed files. | ||
| 2. Check that Docker is running. | ||
| 2. Check Docker state: `docker ps --format '{{.Names}}: {{.Status}}'`. If E2E will run, first destroy all containers (memory rule: "Destroy Docker before E2E") then `docker compose up -d` and wait for healthchecks. |
There was a problem hiding this comment.
Scope Docker teardown to NeoBoard compose resources only.
Line 12’s “destroy all containers” guidance is too broad and can kill unrelated local workloads. Limit teardown to this project’s compose stack/volumes (for example, docker compose down -v in the repo context) instead of global docker ps scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/agents/test-runner.md at line 12, Update the guidance that currently
says “Destroy Docker before E2E”: instead of instructing to kill all containers
globally, limit teardown to this repo’s compose stack by running a
docker-compose down with volume removal in the repository context or by
specifying the project name/project directory so only NeoBoard resources are
removed; replace the global "docker ps" / "destroy all containers" wording with
this scoped teardown instruction and keep the subsequent step to bring the
compose stack up and wait for healthchecks.
| ## Branching | ||
|
|
||
| - Default base: `dev` | ||
| - **Exception**: when a `release/X.Y` branch is active (see [memory](../../../.claude/projects/-Users-alfredorubin-Desktop-public/memory/project_release_1_1_active.md) or check `git branch -r | grep 'origin/release/'`), branch from and PR into the active release branch instead of `dev`. |
There was a problem hiding this comment.
Replace machine-specific memory link with repo-stable guidance.
Line 51 links to a user-specific path (.claude/projects/-Users-...) that won’t resolve for other contributors. Use a repo-relative doc path or keep only the command-based check (git branch -r ...).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/code/SKILL.md at line 51, The sentence referencing a
machine-specific memory link for the "release/X.Y" exception is not portable;
remove the absolute path link and either replace it with a repo-relative
documentation path or drop the link entirely and keep only the command-based
check (`git branch -r | grep 'origin/release/'`), updating the line that
mentions the `release/X.Y` branch so it no longer points to
`.claude/projects/-Users-...` but to a stable repo doc or the command check.
Comment on lines
+228
to
+246
| ``` | ||
| ## Deployment audit findings (YYYY-MM-DD) | ||
|
|
||
| ### Phase 1 — Cold start | ||
| - [ ] #NNN — <title> | ||
| ... | ||
|
|
||
| ### Phase 2 — Checklist | ||
| ... | ||
|
|
||
| ### Phase 3 — Drills | ||
| ... | ||
|
|
||
| ### Phase 4 — Observability | ||
| ... | ||
|
|
||
| ### Phase 5 — TLS/proxy/multi-tenancy | ||
| ... | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced block.
Line 228 starts a fenced code block without language, which trips markdown lint (MD040). Add ```markdown (or ```text) for the findings template block.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 228-228: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/deploy/SKILL.md around lines 228 - 246, The fenced code block
that begins with "## Deployment audit findings (YYYY-MM-DD)" is missing a
language identifier, triggering MD040; update the opening fence from ``` to
include a language tag such as ```markdown (or ```text) so the block becomes a
labeled fenced code block; locate the block in .claude/skills/deploy/SKILL.md
(look for the "Deployment audit findings (YYYY-MM-DD)" heading and the following
triple-backtick fence) and change only the opening fence to include the language
identifier.
| ... | ||
| ``` | ||
|
|
||
| File each as a GH issue using the [issue skill](../issue/skill.md): |
There was a problem hiding this comment.
Fix issue-skill link casing/path.
Line 248 links to ../issue/skill.md, but this repo uses SKILL.md naming. Update the link to the actual path to avoid dead navigation on case-sensitive systems.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/deploy/SKILL.md at line 248, Update the broken relative link
in .claude/skills/deploy/SKILL.md that currently points to "../issue/skill.md" —
change the target to the correct case-sensitive path "../issue/SKILL.md" so the
"issue skill" link resolves properly on case-sensitive filesystems; locate the
link text "issue skill" and replace the href string "../issue/skill.md" with
"../issue/SKILL.md".
Comment on lines
+34
to
37
| BASE=$(git ls-remote --heads origin 'release/*' 2>/dev/null | awk -F/ '{print $NF}' | sort -V | tail -1) | ||
| BASE="${BASE:-dev}" | ||
| git fetch origin "$BASE" && git checkout "$BASE" && git pull origin "$BASE" | ||
| git checkout -b <type>/<short-description> |
There was a problem hiding this comment.
BASE detection drops the release/ prefix and breaks checkout.
Line 34 extracts only the last path token (1.2), so Line 36 may try to fetch/checkout a non-existent branch instead of release/1.2.
Suggested fix
-BASE=$(git ls-remote --heads origin 'release/*' 2>/dev/null | awk -F/ '{print $NF}' | sort -V | tail -1)
+BASE=$(git ls-remote --heads origin 'refs/heads/release/*' 2>/dev/null \
+ | sed -E 's#^.*refs/heads/##' \
+ | sort -V \
+ | tail -1)
BASE="${BASE:-dev}"
git fetch origin "$BASE" && git checkout "$BASE" && git pull origin "$BASE"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| BASE=$(git ls-remote --heads origin 'release/*' 2>/dev/null | awk -F/ '{print $NF}' | sort -V | tail -1) | |
| BASE="${BASE:-dev}" | |
| git fetch origin "$BASE" && git checkout "$BASE" && git pull origin "$BASE" | |
| git checkout -b <type>/<short-description> | |
| BASE=$(git ls-remote --heads origin 'refs/heads/release/*' 2>/dev/null \ | |
| | sed -E 's#^.*refs/heads/##' \ | |
| | sort -V \ | |
| | tail -1) | |
| BASE="${BASE:-dev}" | |
| git fetch origin "$BASE" && git checkout "$BASE" && git pull origin "$BASE" | |
| git checkout -b <type>/<short-description> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/next/SKILL.md around lines 34 - 37, The BASE assignment
strips the release/ prefix (awk -F/ '{print $NF}') causing git fetch/checkout to
use a non-existent branch; update the extraction to preserve the full branch
name (e.g., replace awk -F/ '{print $NF}' with either sed 's#.*refs/heads/##' or
awk -F'/' '{print $(NF-1)"/"$NF}') so BASE becomes "release/1.2" and the
subsequent git fetch origin "$BASE" && git checkout "$BASE" && git pull origin
"$BASE" work correctly.
Two CodeRabbit findings on the new dev-only DNS checker: - The warning included the full decrypted URI, which can be `scheme://user:password@host/...` — that violates the repo rule "NEVER log decrypted credentials." Print only the parsed hostname. - The diagnostic query selected from `connections` without a tenant filter, violating the multi-tenancy rule that every DB query include one. Scope to `process.env.TENANT_ID ?? "default"`. Test gains a credential-leak guard that fails if any URI-embedded username/password reaches the warn sink. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three DevEx unblockers clustered together — all touch first-run / fresh-clone setup and would otherwise each need their own 30-line PR.
Closes #907 — API_KEY_HMAC_SECRET required at startup
Promoted from optional to required. Without it,
POST /api/api-keysfailed with a cryptic message at create time. NowvalidateEnvConfigrejects it at cold start, alongside ENCRYPTION_KEY / NEXTAUTH_SECRET / DATABASE_URL.neoboard env initgenerates the secret in the same pass as ENCRYPTION_KEY, so a fresh setup is still one command.Closes #898 — Seed against localhost, not container names
scripts/seed-demo.mjshonouredNEO4J_HOST/PG_HOSTenv vars. When the script ran inside thedocker-appcontainer, those resolved to compose service names (e.g.db) and the host-side dev server later couldn't reach them. Docker compose publishes ports tolocalhost, so hardcodinglocalhostis correct in every run mode.Closes #899 — Dev-only DNS warning for unreachable seeded connections
Fire-and-forget DNS lookup of each seeded connection's hostname at startup. Warns once per affected row with a concrete fix hint. Guards
NODE_ENV === "development"; never blocks or crashes startup.Test plan
npm -w app run test— 2879/2879 pass (added 11 new tests: 9 forverify-connection-hosts, 2 forAPI_KEY_HMAC_SECRETvalidation)npm -w cli run test— 263/263 pass (3 fixtures updated)npm run lint— no new errors (pre-existinggraph-exploration-wrapper.tsxerrors unchanged)npm run build— passescd app && npx playwright test) — deferred to CI per session pattern; gated by Docker🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation