fix(cli): setup scripts use docker mode + increase health timeout - #397
Closed
alfredo1996 wants to merge 6 commits into
Closed
fix(cli): setup scripts use docker mode + increase health timeout#397alfredo1996 wants to merge 6 commits into
alfredo1996 wants to merge 6 commits into
Conversation
- scripts/setup.sh and setup-local-demo.sh now use docker mode (default) instead of --mode local which skips Docker containers - Health check timeout increased from 60s to 120s — Neo4j cold start from empty volumes takes 90-120s - Health check interval increased from 1s to 2s — less polling noise Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review WalkthroughConsolidates startup/mode handling and DB access: removes explicit Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "CLI (runStart)"
participant Compose as "composeUp"
participant Docker as "Docker Engine"
participant Neo4j as "neoboard-neo4j"
User->>CLI: runStart(opts?)
CLI->>Compose: composeUp({ full: opts?.full ?? false })
Compose->>Docker: docker-compose (DB-only or full)
Docker->>Neo4j: create/start container
CLI->>Docker: docker inspect --format={{.State.Health.Status}} neoboard-neo4j
Docker-->>CLI: "healthy" / "starting"
alt healthy
CLI->>User: report Neo4j ready
else not healthy
CLI->>User: waiting / eventual timeout
end
sequenceDiagram
participant User
participant CLI as "CLI (db:migrate)"
participant FS as "Filesystem (.env.local)"
participant Config as "Project config"
participant Shell as "child process (npx drizzle-kit)"
participant Postgres as "Postgres"
User->>CLI: runDbMigrate()
CLI->>FS: read paths.envFile
alt .env.local exists
FS-->>CLI: DATABASE_URL (strip quotes)
else missing
CLI->>Config: readProjectConfig()
Config-->>CLI: build postgres URL (URI-encoded creds)
end
CLI->>Shell: run("npx drizzle-kit migrate", { cwd: paths.appDir, env: {...process.env, DATABASE_URL} })
Shell->>Postgres: connect using DATABASE_URL
Postgres-->>Shell: migration result
Shell-->>CLI: exit status
CLI->>User: report success/failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/setup-local-demo.sh (1)
20-20: Use explicit Docker mode for demo setup as well.Line 20 also relies on CLI defaults. Pinning mode here keeps demo setup behavior stable and aligned with the PR intent.
Proposed change
-node "$CLI_BIN" demo +node "$CLI_BIN" demo --mode docker🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-local-demo.sh` at line 20, The demo setup call currently relies on CLI defaults (node "$CLI_BIN" demo); update this invocation to explicitly pin the Docker mode so behavior is stable—call the CLI with the mode flag used elsewhere (e.g., add the same --mode docker or --docker flag your CLI expects) when invoking the demo command to ensure consistent Docker-mode behavior for the demo setup.scripts/setup.sh (1)
19-19: Make Docker mode explicit to avoid default drift.Line 19 now depends on the CLI default. For setup reliability, prefer explicit Docker mode so behavior won’t change if defaults change later.
Proposed change
-node "$CLI_BIN" setup +node "$CLI_BIN" setup --mode docker🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup.sh` at line 19, The setup invocation relies on the CLI default mode; change the call to node "$CLI_BIN" setup to explicitly request Docker mode (e.g., add the CLI flag that sets mode to docker such as --mode docker or the project-specific --docker flag) so the script no longer depends on CLI defaults; update the invocation in scripts/setup.sh (the node "$CLI_BIN" setup line) to include the explicit Docker-mode flag and ensure any documentation or downstream automation uses the same explicit flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/setup-local-demo.sh`:
- Line 20: The demo setup call currently relies on CLI defaults (node "$CLI_BIN"
demo); update this invocation to explicitly pin the Docker mode so behavior is
stable—call the CLI with the mode flag used elsewhere (e.g., add the same --mode
docker or --docker flag your CLI expects) when invoking the demo command to
ensure consistent Docker-mode behavior for the demo setup.
In `@scripts/setup.sh`:
- Line 19: The setup invocation relies on the CLI default mode; change the call
to node "$CLI_BIN" setup to explicitly request Docker mode (e.g., add the CLI
flag that sets mode to docker such as --mode docker or the project-specific
--docker flag) so the script no longer depends on CLI defaults; update the
invocation in scripts/setup.sh (the node "$CLI_BIN" setup line) to include the
explicit Docker-mode flag and ensure any documentation or downstream automation
uses the same explicit flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0dfd9bc-c26f-4d32-a0cf-3ceee71b47a7
📒 Files selected for processing (3)
cli/src/lib/health.tsscripts/setup-local-demo.shscripts/setup.sh
…st health Root causes of slow/broken setup: 1. start.ts used full=true → built entire Next.js Docker image (minutes) 2. db/migrate.ts used docker exec into neoboard-app (doesn't exist) 3. Neo4j health used cypher-shell (slow JVM startup per poll) 4. Scripts hardcoded --mode local (skipped Docker) 5. Health timeout 60s too short for Neo4j cold start Fixes: - start.ts: full=false → docker-compose.yml (DBs only, ~30s) - db/migrate.ts: always runs locally with DATABASE_URL resolved from .env.local (priority) or neoboard.config.json (fallback). Works whether DB is Docker, local, or remote. - docker.ts: isNeo4jReady uses docker inspect (instant) not cypher-shell - health.ts: timeout 60s→120s, interval 1s→2s - scripts: removed --mode local (uses docker default) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cli/src/__tests__/lib/docker.test.ts (1)
177-184: Add one trim-behavior test for robustness.Since
isNeo4jReady()trims output, consider adding a case like"healthy\n"to prevent regressions in shell-output handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/__tests__/lib/docker.test.ts` around lines 177 - 184, Add a test case to cover trimmed shell output: update the test suite for isNeo4jReady to mock mockRunOrNull returning a value with trailing newline (e.g., "healthy\n") and assert that isNeo4jReady() still returns true; reference the existing test helpers/mockRunOrNull and the isNeo4jReady() function so the new case mirrors the "returns true when docker inspect reports healthy" test but uses a newline-terminated string to verify trimming behavior.cli/src/__tests__/commands/db/migrate.test.ts (1)
102-121: Add edge-case tests for URL parsing behavior.Please add cases for quoted values (e.g.,
DATABASE_URL="...") and special-character credentials in fallback URL construction. This will protect the new resolver from common.envformatting variations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/__tests__/commands/db/migrate.test.ts` around lines 102 - 121, Add two new test cases in migrate.test.ts alongside the existing runDbMigrate tests: one where the .env.local DATABASE_URL is quoted (e.g., DATABASE_URL="postgresql://user:pass@host:5432/db") to ensure the resolver strips surrounding quotes when calling runDbMigrate, and another where the fallback config-derived URL uses credentials containing special characters (e.g., password with symbols) to verify proper encoding/handling when constructing DATABASE_URL. Use the existing helpers/mock patterns (runDbMigrate, mockExistsSync, mockRun) to set up .env content or config inputs and assert mockRun is called with env containing the expected normalized DATABASE_URL for each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/commands/db/migrate.ts`:
- Around line 15-25: The DATABASE_URL parsing currently returns the raw value
(including surrounding quotes) and the fallback builds a connection string
without URI-encoding credentials; update the parsing loop in migrate.ts to strip
any surrounding single or double quotes from the extracted url (trim() then
remove leading/trailing "'" or '"' if present) before returning it, and change
the fallback that uses readProjectConfig() to URI-encode the username and
password (e.g., via encodeURIComponent on config.postgres.user and
config.postgres.password) when constructing the `postgresql://...` string so
reserved characters don’t break the connection string.
- Around line 103-106: Wrap the existing run("npx drizzle-kit migrate", ...)
call with an advisory lock: create a Postgres client, connect, call SELECT
pg_advisory_lock(<constant_bigint>) to acquire a session lock, then execute the
run(...) command while the lock is held, and finally release the lock (SELECT
pg_advisory_unlock(...)) and close the client in a try/finally so the lock is
always released; reference the existing run(...) invocation in migrate.ts and
use a single fixed bigint key (e.g., 1234567890) for pg_advisory_lock/unlock to
serialize concurrent migrations.
---
Nitpick comments:
In `@cli/src/__tests__/commands/db/migrate.test.ts`:
- Around line 102-121: Add two new test cases in migrate.test.ts alongside the
existing runDbMigrate tests: one where the .env.local DATABASE_URL is quoted
(e.g., DATABASE_URL="postgresql://user:pass@host:5432/db") to ensure the
resolver strips surrounding quotes when calling runDbMigrate, and another where
the fallback config-derived URL uses credentials containing special characters
(e.g., password with symbols) to verify proper encoding/handling when
constructing DATABASE_URL. Use the existing helpers/mock patterns (runDbMigrate,
mockExistsSync, mockRun) to set up .env content or config inputs and assert
mockRun is called with env containing the expected normalized DATABASE_URL for
each case.
In `@cli/src/__tests__/lib/docker.test.ts`:
- Around line 177-184: Add a test case to cover trimmed shell output: update the
test suite for isNeo4jReady to mock mockRunOrNull returning a value with
trailing newline (e.g., "healthy\n") and assert that isNeo4jReady() still
returns true; reference the existing test helpers/mockRunOrNull and the
isNeo4jReady() function so the new case mirrors the "returns true when docker
inspect reports healthy" test but uses a newline-terminated string to verify
trimming behavior.
🪄 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: 06804a05-36e2-4260-a686-9ea9a0ac4a8c
📒 Files selected for processing (6)
cli/src/__tests__/commands/db/migrate.test.tscli/src/__tests__/commands/start.test.tscli/src/__tests__/lib/docker.test.tscli/src/commands/db/migrate.tscli/src/commands/start.tscli/src/lib/docker.ts
✅ Files skipped from review due to trivial changes (1)
- cli/src/tests/commands/start.test.ts
Comment on lines
+103
to
+106
| run("npx drizzle-kit migrate", { | ||
| cwd: paths.appDir, | ||
| env: { ...process.env, DATABASE_URL: dbUrl }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the drizzle-kit version used by this repository, does drizzle-kit migrate acquire a PostgreSQL advisory lock (or equivalent concurrency lock) to prevent concurrent migration runners?
💡 Result:
I can’t tell what drizzle-kit version “this repository” uses unless you share the repo URL or paste the drizzle-kit entry from package.json / lockfile.
That said, Drizzle Kit’s migrate has historically not taken a PostgreSQL advisory lock (e.g. pg_advisory_lock / pg_advisory_xact_lock) to serialize concurrent runners—the Drizzle team explicitly described a future “proper lock mechanism” as something they will implement, implying it wasn’t present at the time of that design note/discussion. [1] The locks people commonly observe during migrate are standard PostgreSQL DDL locks (e.g., from CREATE TABLE IF NOT EXISTS ... __drizzle_migrations and other DDL), not an explicit migration-runner mutex. [2]
If you paste your repo’s drizzle-kit version, I can verify more precisely for that version and (if needed) point you to how to confirm by searching the installed CLI code for pg_advisory_*.
Sources
- Drizzle discussion noting “Proper Lock Mechanism” as a planned improvement for simultaneous
migrate()executions: “We will implement a proper lock mechanism…” [1] - Report of DDL locking behavior during Drizzle migrations (table creation causing locks), not advisory-lock coordination: [2]
🏁 Script executed:
fd -t f "package.json" | head -5Repository: alfredo1996/neoboard
Length of output: 162
🏁 Script executed:
cat package.json | grep -A 2 -B 2 "drizzle"Repository: alfredo1996/neoboard
Length of output: 46
🏁 Script executed:
# Also check for any advisory lock usage in the codebase
rg "pg_advisory|advisory.*lock" -iRepository: alfredo1996/neoboard
Length of output: 46
🏁 Script executed:
cat cli/package.json | grep -E "(drizzle|migration)" -iRepository: alfredo1996/neoboard
Length of output: 46
🏁 Script executed:
# Check for lockfiles to get exact version
fd -t f "(pnpm-lock|package-lock|yarn.lock)" | head -1Repository: alfredo1996/neoboard
Length of output: 85
🏁 Script executed:
# Search for drizzle across all package files
rg "drizzle-kit" --type jsonRepository: alfredo1996/neoboard
Length of output: 561
🏁 Script executed:
# Check cli/package.json for drizzle dependencies
cat cli/package.jsonRepository: alfredo1996/neoboard
Length of output: 636
🏁 Script executed:
# Check the full context of the migration command at lines 103-106
cat -n cli/src/commands/db/migrate.ts | sed -n '95,115p'Repository: alfredo1996/neoboard
Length of output: 643
🏁 Script executed:
# Search for any migration-related files that might have custom locking
fd -t f "migrat" cli/src/ | head -10Repository: alfredo1996/neoboard
Length of output: 140
🏁 Script executed:
cat -n cli/src/__tests__/commands/db/migrate.test.ts | head -50Repository: alfredo1996/neoboard
Length of output: 1635
Add advisory locking to prevent concurrent migration races.
drizzle-kit migrate (version 0.31.10) does not implement PostgreSQL advisory locking. Running migrations concurrently can cause race conditions. Wrap the migration call with pg_advisory_xact_lock() or implement an equivalent serialization mechanism to comply with NeoBoard migration requirements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/commands/db/migrate.ts` around lines 103 - 106, Wrap the existing
run("npx drizzle-kit migrate", ...) call with an advisory lock: create a
Postgres client, connect, call SELECT pg_advisory_lock(<constant_bigint>) to
acquire a session lock, then execute the run(...) command while the lock is
held, and finally release the lock (SELECT pg_advisory_unlock(...)) and close
the client in a try/finally so the lock is always released; reference the
existing run(...) invocation in migrate.ts and use a single fixed bigint key
(e.g., 1234567890) for pg_advisory_lock/unlock to serialize concurrent
migrations.
- start.ts accepts { full } option — full=true uses docker-compose.full.yml
(app + DBs), full=false uses docker-compose.yml (DBs only)
- demo.ts always passes full=true — users get the complete experience
- setup.ts passes full=false by default — developers start DBs,
then run the app locally with npm run dev
neoboard demo: ~3.5min → full stack in Docker, seeded, ready
neoboard setup + neoboard dev: ~30s → DBs in Docker, app locally
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…entials Addresses CodeRabbit review: - Strip surrounding quotes from DATABASE_URL in .env.local - URI-encode user/password/database in config fallback URL - Add tests for both edge cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
Owner
Author
|
Superseded by #414 (combined release 1.1 fixes) |
|
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.
Scripts used --mode local (skipped Docker). Neo4j cold start timed out at 60s. Fixed both.
Summary by CodeRabbit
Chores
Behavior Changes
Tests