Batch 1: Bootstrap reconciled release/2.1 onto 2.0 base (#758) - #768
Merged
Conversation
Port infrastructure files from release/2.1 to the new release/2.1-from-2.0 base (which inherits release/2.0's plugin work). No code changes. - .env.example — full env var documentation including SSO/OIDC, encryption rotation, rate limit, log level - .gitmodules — enterprise submodule pointer (alfredo1996/neoboard-enterprise) - enterprise/ — submodule pinned at 9461e039 (verified accessible) - docker/docker-compose.prod-full.yml — single-server compose with app + PostgreSQL, healthchecks, resource limits - .claude/hooks/check-migration-guard.sh — PreToolUse hook blocking edits to existing migration files (forward-only enforcement) Pre-existing lint errors on release/2.0 base (cli/, app/, etc.) are out-of-scope for this batch — see follow-up tasks. Part of #757 Closes #758 Co-Authored-By: Claude Opus 4.6 (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 (4)
WalkthroughAdds an env template, migration write-guard hook, enterprise submodule update, safe error-sanitization option with route/tests, dashboard timer/key changes and tests, Jest/test-runner tweaks, and CI pull_request branch filter expansion. ChangesBootstrap, safety, and CI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 task
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/hooks/check-migration-guard.sh:
- Around line 4-6: Add strict failure handling and make jq errors abort the
hook: enable shell strict mode by adding set -euo pipefail at the top, then
replace the silent jq calls (the FILE_PATH assignment that uses jq -r
'.tool_input.file_path // .tool_input.filePath // empty' and the second jq
invocation at lines 12-13) with commands that check jq's exit status and fail
the script on parse errors (e.g., capture jq output into a variable and if jq
exits non-zero or the output is invalid, print an error to stderr and exit 1).
Ensure both FILE_PATH and the other jq-derived variables use the same guarded
pattern so the hook fails closed on JSON parse failures.
In @.env.example:
- Around line 51-56: Update the SSO comment to remove the contradiction by
changing the phrase "three required vars" to "four required vars" so it matches
the later "all four must be set" line; locate the comment block that starts with
"Set all three required vars to enable a single OIDC provider via env." and edit
that sentence to say "Set all four required vars to enable a single OIDC
provider via env." so the NEOBOARD_EDITION/SSO guidance and the subsequent
"Required (all four must be set to activate SSO)" line are consistent.
In `@enterprise`:
- Line 1: The repository submodule entry in .gitmodules points to an
inaccessible remote (https://github.com/alfredo1996/neoboard-enterprise.git)
causing clone and CI failures; update the .gitmodules entry for the enterprise
submodule to the canonical org repo URL, run git submodule sync && git submodule
update --init --remote to fetch from the new remote, and update the pinned SHA
(replace the unreachable 9461e03950b94d3f449d65b76a9ddef59e5ca333) with the
correct commit SHA from the canonical repository so CI and local clones can
initialize the enterprise submodule successfully.
🪄 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: 5a109afd-2604-4ea0-a7dc-a483f76c2526
⛔ Files ignored due to path filters (1)
docker/docker-compose.prod-full.ymlis excluded by!docker/**
📒 Files selected for processing (4)
.claude/hooks/check-migration-guard.sh.env.example.gitmodulesenterprise
Address CodeRabbit findings on PR #768: - Migration guard hook now uses `set -euo pipefail` and `jq -e` to fail closed when JSON parsing fails instead of silently allowing edits - Fix contradictory SSO comment: "three" → "four" required vars Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stacked PRs targeting feature branches were not getting CI checks because the pull_request trigger only matched main, dev, and release/*. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hain The integration test files in __tests__/connection/, __tests__/neo4j/, and __tests__/postgresql/ failed at collection time with: SyntaxError: Unexpected token 'export' at .../node_modules/uuid/dist-node/index.js:1 Chain: setup.ts -> testcontainers -> dockerode -> uuid@14 (ESM-only). Two changes: 1. Add a `^.+\.m?js$` transform via ts-jest so Jest's CJS runtime can load uuid's ESM build. 2. Override transformIgnorePatterns to allow the uuid package through (default ignores all of node_modules). Also escapes the dot in the existing tsx? pattern (`^.+.tsx?$` -> `^.+\.tsx?$`) so it matches only TypeScript files, not arbitrary characters before "ts". Verified locally: 224/226 tests pass, down from ~13 collection failures. The remaining 2 failures are real Neo4j integration timeouts unrelated to this config (setField-query, list-databases) — tracked separately. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous commit added a `^.+\.m?js$` transform so Jest could load the ESM uuid package. That transform also matches compiled files in `dist/`, so jest started picking up `dist/generalized/__tests__/errors.test.js` (and its .d.ts twin) and double-running tests: FAIL dist/generalized/__tests__/errors.test.js FAIL dist/generalized/__tests__/errors.test.d.ts Add `/dist/` to testPathIgnorePatterns so only sources in `__tests__/` are collected. Tests should never be run against built output. Verified: connection suite collects 30 suites (was 32 with dist garbage), 225/226 tests pass — only the 2 pre-existing Neo4j integration timeouts remain (setField-query, list-databases). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
connection/src/generalized/__tests__/errors.test.ts imported `describe`, `it`, `expect` from "vitest", but the connection package uses Jest. The file was added in #616 (plugin hardening, Apr 2026) and has been failing silently in CI ever since with: Vitest cannot be imported in a CommonJS module using require() Switching the imports to "@jest/globals" makes all 7 assertions run green under jest. No code changes — only the import line. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks
Two unrelated bugs surfacing as 5 consistent E2E failures across PR #776: #779 — Refresh button silently no-op'd dashboard-container.tsx invalidated TanStack Query with a 4-element key (["widget-query", connectionId, query, params]) but useWidgetQuery's actual key is 6 elements (["widget-query", connectionId, database, query, params, staleTime]). Position 2 mismatched (query string vs null/database) so the prefix match failed and no refetch ever fired. Fixed in: e2e/auto-refresh.spec.ts:107 manual per-widget refresh re-fetch e2e/auto-refresh.spec.ts:221 manual refresh, auto-refresh disabled e2e/widget-states.spec.ts:222 showRefreshButton re-fetches e2e/widget-states.spec.ts:526 cacheMode 'forever' refresh Fix: align invalidation prefix to match through `query` slot. Stop at `query` because params/staleTime are merged at hook call time and not reproducible at refresh time. #778 — Write route leaked raw driver error handleRouteError -> sanitizeErrorMessage was deliberately permissive (only strips bundler internals like __TURBOPACK__) so pg/cypher syntax errors echoed user SQL into the response body. Fixed in: e2e/write-permissions.spec.ts:255 "safe 500 message" assertion expected "Write query execution failed", got 'syntax error at or near "THIS"'. Fix: opt-in `safeMessage` flag on handleRouteError that collapses untyped errors to fallbackMsg. Typed app errors (Queue/Auth/Enterprise) still get specific handling. Used by /api/query/write only — read routes preserve current behavior so users can debug their own queries. Tests: - api-utils.test.ts: 3 new safeMessage cases (44/44 unit pass) - write/route.test.ts: updated leaky-behavior assertion to verify the sanitized message + negative match against /syntax error/ - dashboard-container tests: 27/27 still pass - E2E: all 5 originally-failing tests now pass locally (20/20)
alfredo1996
added a commit
that referenced
this pull request
May 15, 2026
…n) (#774) Foundation fix that unblocks Unit & Integration CI on the entire 2.1 reconciliation stack. The connection package's jest config didn't transform ESM-only `uuid` (v14) imported transitively by `testcontainers`, causing all PRs that depend on the connection workspace to error on `import` syntax during test discovery. This fix: - Adds an ESM transform pattern for `uuid` in jest.config.js - Excludes `dist/` from testPathIgnorePatterns - Updates errors.test.ts to import from @jest/globals (was importing from vitest by mistake) E2E shard 1/5 + 5/5 failures here are the documented refresh-button flake (GH#779), unrelated to the connection package — already fixed and shipping in PR #768 (next merge in the cascade). Merging foundation despite those flakes; downstream merges land the fix.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/dashboard-container.tsx (1)
152-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuery key in
exportWidgetCsvis missingdatabase, same bug fixed below.The refresh handler fix adds
widget.database ?? nullat position 2 to matchuseWidgetQuery's key shape. This function still uses the old key withoutdatabase, so CSV export will silently find no cached data whenwidget.databaseis set.Proposed fix
const entries = queryClient.getQueriesData<{ data: unknown }>({ - queryKey: ["widget-query", widget.connectionId, widget.query], + queryKey: ["widget-query", widget.connectionId, widget.database ?? null, widget.query], });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard-container.tsx` around lines 152 - 154, exportWidgetCsv uses the old query key shape and omits widget.database, so queryClient.getQueriesData in exportWidgetCsv (the line creating entries) should include widget.database ?? null as the third element to match useWidgetQuery's key structure; update the queryKey from ["widget-query", widget.connectionId, widget.query] to ["widget-query", widget.connectionId, widget.database ?? null, widget.query] (or equivalent using widget.database) so cached data is found when database is set.
🤖 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.
Outside diff comments:
In `@app/src/components/dashboard-container.tsx`:
- Around line 152-154: exportWidgetCsv uses the old query key shape and omits
widget.database, so queryClient.getQueriesData in exportWidgetCsv (the line
creating entries) should include widget.database ?? null as the third element to
match useWidgetQuery's key structure; update the queryKey from ["widget-query",
widget.connectionId, widget.query] to ["widget-query", widget.connectionId,
widget.database ?? null, widget.query] (or equivalent using widget.database) so
cached data is found when database is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a780271d-001a-4120-b420-d7de8cd1c307
📒 Files selected for processing (10)
.claude/hooks/check-migration-guard.sh.env.example.github/workflows/ci.ymlapp/src/app/api/query/write/__tests__/route.test.tsapp/src/app/api/query/write/route.tsapp/src/components/dashboard-container.tsxapp/src/lib/__tests__/api/api-utils.test.tsapp/src/lib/api/api-utils.tsconnection/jest.config.jsconnection/src/generalized/__tests__/errors.test.ts
✅ Files skipped from review due to trivial changes (2)
- connection/src/generalized/tests/errors.test.ts
- .env.example
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/hooks/check-migration-guard.sh
… name races
Root cause of repeated "strict mode violation: getByText('Neo4j Bar Template',
{ exact: true }) resolved to 2 elements" failures on shard 5 across multiple
PRs (#770, #771): the consumption describe block creates two templates with
hardcoded names ("Neo4j Bar Template", "PostgreSQL Table Template") in
beforeEach. With fullyParallel=true and 2 CI workers, two of these tests'
beforeEach hooks race — both succeed in POSTing — leaving 2 elements with
the same text in the DOM and breaking strict-mode locators.
Fix: declare test.describe.configure({ mode: "serial" }) on the consumption
describe so its tests run sequentially within a worker. Each test's
beforeEach/afterEach now owns the shared template names exclusively.
Verified locally: all 4 consumption tests pass with --workers=2.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR #768 unit suite failed with "Test timed out in 5000ms" on "shows selection count text when rows are selected with pagination active" in data-grid-dynamic-pagination.test.tsx:349. Same root cause as #439 (commit 0ff3419): under coverage instrumentation in CI, rendering 30 rows + userEvent.click + selection state propagation exceeds the default 5s budget. The previous test in the same describe ("can select all rows on the current page") was already bumped to 15000ms for the same reason — this sibling test was missed in that PR. Match the precedent so both selection-related tests have headroom. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
May 16, 2026
PR #768 unit suite failed with "Test timed out in 5000ms" on "shows selection count text when rows are selected with pagination active" in data-grid-dynamic-pagination.test.tsx:349. Same root cause as #439 (commit 0ff3419): under coverage instrumentation in CI, rendering 30 rows + userEvent.click + selection state propagation exceeds the default 5s budget. The previous test in the same describe ("can select all rows on the current page") was already bumped to 15000ms for the same reason — this sibling test was missed in that PR. Match the precedent so both selection-related tests have headroom. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
May 16, 2026
PR #768 unit suite failed with "Test timed out in 5000ms" on "shows selection count text when rows are selected with pagination active" in data-grid-dynamic-pagination.test.tsx:349. Same root cause as #439 (commit 0ff3419): under coverage instrumentation in CI, rendering 30 rows + userEvent.click + selection state propagation exceeds the default 5s budget. The previous test in the same describe ("can select all rows on the current page") was already bumped to 15000ms for the same reason — this sibling test was missed in that PR. Match the precedent so both selection-related tests have headroom. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
May 16, 2026
PR #768 unit suite failed with "Test timed out in 5000ms" on "shows selection count text when rows are selected with pagination active" in data-grid-dynamic-pagination.test.tsx:349. Same root cause as #439 (commit 0ff3419): under coverage instrumentation in CI, rendering 30 rows + userEvent.click + selection state propagation exceeds the default 5s budget. The previous test in the same describe ("can select all rows on the current page") was already bumped to 15000ms for the same reason — this sibling test was missed in that PR. Match the precedent so both selection-related tests have headroom. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
May 16, 2026
PR #768 unit suite failed with "Test timed out in 5000ms" on "shows selection count text when rows are selected with pagination active" in data-grid-dynamic-pagination.test.tsx:349. Same root cause as #439 (commit 0ff3419): under coverage instrumentation in CI, rendering 30 rows + userEvent.click + selection state propagation exceeds the default 5s budget. The previous test in the same describe ("can select all rows on the current page") was already bumped to 15000ms for the same reason — this sibling test was missed in that PR. Match the precedent so both selection-related tests have headroom. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
May 16, 2026
PR #768 unit suite failed with "Test timed out in 5000ms" on "shows selection count text when rows are selected with pagination active" in data-grid-dynamic-pagination.test.tsx:349. Same root cause as #439 (commit 0ff3419): under coverage instrumentation in CI, rendering 30 rows + userEvent.click + selection state propagation exceeds the default 5s budget. The previous test in the same describe ("can select all rows on the current page") was already bumped to 15000ms for the same reason — this sibling test was missed in that PR. Match the precedent so both selection-related tests have headroom. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
May 16, 2026
PR #768 unit suite failed with "Test timed out in 5000ms" on "shows selection count text when rows are selected with pagination active" in data-grid-dynamic-pagination.test.tsx:349. Same root cause as #439 (commit 0ff3419): under coverage instrumentation in CI, rendering 30 rows + userEvent.click + selection state propagation exceeds the default 5s budget. The previous test in the same describe ("can select all rows on the current page") was already bumped to 15000ms for the same reason — this sibling test was missed in that PR. Match the precedent so both selection-related tests have headroom. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The deferred-ready setTimeout in DashboardContainer.openFullscreen could fire after the component unmounted, calling setState on a torn-down tree. In CI (vitest jsdom + coverage) this surfaced as an unhandled "window is not defined" exception originating from dashboard-container-branches.test.tsx, failing the run despite all 2592 tests passing. Track the timer in a ref and clear it on unmount, on close, and when re-arming via openFullscreen. Production behavior unchanged; defensive cleanup prevents the React unmounted-update warning in browsers too.
Add tests for the close, re-arm, and unmount paths of the fullscreen ready-timer ref so SonarCloud's new-coverage check on the prior fix clears the 80% threshold (was 75% on 4 uncovered lines). - close-while-pending → exercises closeFullscreen clearTimeout branch - re-arm openFullscreen → exercises previous-ref clearTimeout branch - unmount-while-pending → exercises useEffect cleanup branch Dialog mock now exposes onOpenChange via a close button so the close path is reachable from tests; vi.useFakeTimers lets us assert the timer was cleared (no setState fires on a torn-down tree).
|
This was referenced May 16, 2026
This was referenced May 16, 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.
Part of #757 — release/2.1 reconciliation onto release/2.0 base.
Summary
.env.example(71 lines) — documents OIDC, ENCRYPTION_KEY rotation, rate limit env vars.gitmodules— points enterprise submodule atalfredo1996/neoboard-enterpriseenterprise/submodule pinned at SHA9461e039docker/docker-compose.prod-full.yml(72 lines) — single-server compose with healthchecks/limits.claude/hooks/check-migration-guard.sh(26 lines) — forward-only migration guardTest plan
git submodule update --init)docker compose -f docker/docker-compose.prod-full.yml config)E2E deferred to Batch 10 (#767).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Bug Fixes / Reliability
CI / Tests