fix(import): preserve NeoDash markdown + auto-generate parameter widgets - #936
Merged
alfredo1996 merged 2 commits intoJun 4, 2026
Merged
Conversation
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>
alfredo1996
added
bug
|
Warning Review limit reached
More reviews will be available in 41 minutes and 46 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR adds parameter extraction and type inference utilities to the NeoDash converter, auto-generates a Filters page with parameter-select widgets for all referenced dashboard parameters, migrates markdown widget content from query to settings, and stamps connectionId onto non-filter widgets. Comprehensive tests validate all conversion scenarios and parameter widget layout. ChangesNeoDash Parameter Import and Markdown Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
8 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
app/src/lib/dashboard/__tests__/neodash-converter.test.ts (1)
225-245: ⚡ Quick winVerify query syntax conversion in converted widgets.
The test confirms that a filter widget is created for
$neodash_year, but doesn't assert that the original widget's query was converted from$neodash_yearto$param_year. Add an assertion to verify the full conversion flow:// After line 244, add: const originalWidget = exp.layout.pages[1].widgets[0]; expect(originalWidget.query).toContain('$param_year'); expect(originalWidget.query).not.toContain('$neodash_year');This ensures both filter creation AND query transformation are working correctly.
🤖 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/dashboard/__tests__/neodash-converter.test.ts` around lines 225 - 245, Add assertions in the "creates a parameter-select widget for each referenced + defined param" test to verify the original report widget's query was rewritten: after calling convertNeoDashWithNotes(nd) and extracting exp, access the converted original widget via exp.layout.pages[1].widgets[0] (e.g., originalWidget) and assert originalWidget.query contains the new parameter token "$param_year" and does not contain the legacy "$neodash_year"; this uses the existing test helpers (makeNeoDash, makeReport, convertNeoDashWithNotes) and variables (exp, notes) so you can place the two expect checks directly after the existing filter-related assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/lib/dashboard/__tests__/neodash-converter.test.ts`:
- Around line 327-349: The test uses unrealistic parameter tokens `$param_p${i}`
in the queryParts; update the fixture to use real NeoDash parameter syntax
`$neodash_p${i}` so makeReport and convertNeoDashWithNotes exercise real parsing
logic—modify the loop that builds queryParts in neodash-converter.test.ts (where
makeNeoDash is called with makeReport({ query: queryParts.join(" ") })) to push
`$neodash_p${i}` instead of `$param_p${i}`.
- Around line 389-398: The test uses an inconsistent parameter name "$param_x"
but the NeoDash parameters are provided under "neodash_x", so update the input
to use the realistic NeoDash variable name; in the test that calls makeNeoDash
and makeReport, replace "$param_x" with "$neodash_x" so makeReport, makeNeoDash,
and convertNeoDashWithNotes operate against the correct parameter and the
connectionId stamping assertions remain valid.
- Around line 351-367: The test fixture uses wrong NeoDash parameter
placeholders; update the report query in the test that calls makeNeoDash and
makeReport so it references "$neodash_small $neodash_big" instead of
"$param_small $param_big" so the parameters object ({ neodash_small: 5,
neodash_big: 500 }) matches the query; locate the test case named "number-range
pre-populates rangeMin=0 and rangeMax=max(default, 100)" and change the query
string passed into makeReport, then run the neodash converter functions
(convertNeoDashWithNotes) to verify the assertions for byName.small and
byName.big still hold.
- Around line 276-306: The test currently uses NeoDash parameters in the query
with the wrong prefix ($param_*) which bypasses the conversion logic; update the
test fixture in the "infers types correctly per default value" case so the
report query string uses the correct NeoDash parameter prefixes ($neodash_str,
$neodash_emp, $neodash_arr, $neodash_num, $neodash_yn) to match the parameters
object and exercise the prefix-conversion code paths in makeNeoDash / makeReport
and convertNeoDashWithNotes.
- Around line 257-274: The test uses pre-converted syntax ($param_undeclared)
which bypasses the converter; update the fixture in the "creates
parameter-select for referenced-but-undefined params with a warning note" test
to use the real NeoDash input syntax by changing the query in makeReport from
"MATCH (n) WHERE n.name = $param_undeclared RETURN n" to use
"$neodash_undeclared" (e.g. "MATCH (n) WHERE n.name = $neodash_undeclared RETURN
n") so convertNeoDashWithNotes runs the $neodash_* → $param_* transformation;
keep the existing assertions against convertNeoDashWithNotes output (expect
parameterName "undeclared", defaultValue undefined, and the warning note)
unchanged.
In `@app/src/lib/dashboard/neodash-converter.ts`:
- Around line 516-519: The code sets settings.rangeMin = 0 for parameterType ===
"number-range" which breaks negative defaultValue; update the logic in the block
handling parameterType "number-range" (where defaultValue is checked and
settings.rangeMin/settings.rangeMax are assigned) so that settings.rangeMin is
set to Math.min(defaultValue, 0) (or otherwise computed to ensure the
defaultValue is within [rangeMin, rangeMax]) and settings.rangeMax uses
Math.max(defaultValue, 100), ensuring the computed range always includes the
defaultValue (refer to symbols parameterType, defaultValue, settings.rangeMin,
settings.rangeMax).
---
Nitpick comments:
In `@app/src/lib/dashboard/__tests__/neodash-converter.test.ts`:
- Around line 225-245: Add assertions in the "creates a parameter-select widget
for each referenced + defined param" test to verify the original report widget's
query was rewritten: after calling convertNeoDashWithNotes(nd) and extracting
exp, access the converted original widget via exp.layout.pages[1].widgets[0]
(e.g., originalWidget) and assert originalWidget.query contains the new
parameter token "$param_year" and does not contain the legacy "$neodash_year";
this uses the existing test helpers (makeNeoDash, makeReport,
convertNeoDashWithNotes) and variables (exp, notes) so you can place the two
expect checks directly after the existing filter-related assertions.
🪄 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: 5f00bbcd-21fa-4f6a-920e-8cd95c697af6
📒 Files selected for processing (2)
app/src/lib/dashboard/__tests__/neodash-converter.test.tsapp/src/lib/dashboard/neodash-converter.ts
- 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>
Owner
Author
Addressed CR + Sonar feedback (commit dc30bd8)
Local: 2834/2834 tests pass, build green. Watching CI. |
|
Merged
9 tasks
5 tasks
alfredo1996
added a commit
that referenced
this pull request
Jun 5, 2026
…, #898, #899) (#939) * chore(.claude): polish skills, agents, CLAUDE.md for release/1.1 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> * security(auth): gate SSO settings page + API by enterprise edition 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> * test(auth): address CodeRabbit feedback on #934 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> * fix(auth): mark FeatureGate + EnterpriseRequiredEmptyState props as readonly 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> * feat(import): unify NeoBoard + NeoDash flow with mapping UI + notes 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> * test(e2e): update import E2E specs for new post-success view + NeoDash 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> * fix(import): drop dashboard title from NeoDash placeholder name 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> * fix(import): preserve NeoDash markdown + auto-generate parameter widgets 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> * fix(import): address CR + Sonar findings on #936 - 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> * fix(plugins): graceful settings fallback via safeParseSettings + graph 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> * test(plugins): smoke test safeParseSettings adoption across all 20 plugins 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> * fix(dashboard): suppress self-save 'updated by' banner on revisit 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> * test(e2e): fix dashboard-states #904 test — Back goes to view mode, not 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> * chore(devex): fail-fast HMAC, seed-from-host fix, dev DNS warning 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> * fix(test): type warn mock so app tsc accepts the assignment 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> * fix(dev): redact URI in DNS warn, add tenant scope to diagnostic query 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> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three NeoDash converter bugs that silently dropped data on import:
settings.parameterswere lost entirelytext/markdownwidget content was placed in the wrong field, rendering empty markdown widgetsAll three resolved here. Closes #915. Stacks on the notes infrastructure landed in #935 (merged).
Drill brief:
claude_code_docs/plans/issue-915.md.What changed
Bug A — Top-level parameters → auto-generated parameter-select widgets
The converter now:
$param_<name>references (afterconvertParamSyntaxrewrites$neodash_X→$param_X)nd.settings.parameters:parameter-selectwidget with inferred type + default valueneodash_prefix from param names so the widget produces$param_<name>matching what queries referenceType inference for
parameter-select.parameterType[]/ arraymulti-selectnumber(finite)number-range(rangeMin: 0,rangeMax: max(value, 100))""(empty string)textselectNaN/Infinityselect(defensive fallback)Layout for auto-generated widgets
Tile 4 per row at
w=3, h=2. Sequential placement:x = (i % 4) * 3,y = Math.floor(i / 4) * 2. WidgetconnectionId=""(no data fetch).Bug B — Markdown content routing
When
chartType === "markdown":settings.content = report.query(where the widget actually reads from)widget.query = ""(markdown is content-only)Imported markdown content for "<title>"Bug C — Notes infrastructure
Uses the
notes: string[]already returned byconvertNeoDashWithNotes. Per the drill: verbose per-param notes (user requested explicit traceability over summary brevity).Tests
30 new unit tests in
app/src/lib/dashboard/__tests__/neodash-converter.test.ts:isNeoDashFormat— happy + edges (4)inferParameterType— every branch incl. NaN/Infinity (6)extractParamReferences— uniqueness, empty queries, false positives (5)defaultConnectionIdinteraction with filter widgets (3)All 54 existing tests pass (route, dashboard, demo-showcases). Build + type-check green.
Files
app/src/lib/dashboard/neodash-converter.tsapp/src/lib/dashboard/__tests__/neodash-converter.test.tsOut of scope (per drill — explicit decisions)
$param_*inline substitution — NeoDash didn't do this; literal copyPhase 2 sequence
This is PR #3 of 5 in the Phase 2 security & data-loss sweep:
Risk
idand store the param name insettings.parameterName— no collisionCloses #915
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests