chore: dev from dump improvement#1131
Open
chedieck wants to merge 7 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a "dev-from-dump" mode that initializes app state from database dumps: environment flags and Makefile targets, a dump-aware Docker Compose with a persistent DB volume, dump import scripting with progress streaming, and runtime changes to skip cache rebuilds and run production-style Prisma migrations. ChangesDev-from-dump Development Workflow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker-compose-from-dump.yml (1)
10-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude the base
.env/.env.localinpaybutton(or ensure prod loads dotenv)
docker-compose-from-dump.ymlsetspaybuttonto onlyenv_file: - .env.from-dump, whilescripts/paybutton-server-start.shsources.env/.env.localinsh..envuses plainKEY=...assignments (noexport), so those values aren’t reliably inherited by PM2 child processes.yarn prismaand thedev/init scripts load.envviadotenv-cli, butENVIRONMENT=productionrunsyarn prod, which does not load dotenv;.env.localoverrides also won’t be applied to those children.Suggested fix
env_file: + - .env + - .env.local - .env.from-dump🤖 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 `@docker-compose-from-dump.yml` around lines 10 - 11, The paybutton service only references .env.from-dump which causes production children (PM2 / yarn prod started by scripts/paybutton-server-start.sh) to miss variables from .env/.env.local; update the paybutton docker-compose service to include the base env files (add .env and .env.local to env_file alongside .env.from-dump) or ensure the startup script/supervisor (scripts/paybutton-server-start.sh / the PM2 start command invoked) explicitly loads dotenv before launching yarn prod so that non-exported KEY= values are available to child processes; locate the paybutton service block in docker-compose-from-dump.yml and the startup in scripts/paybutton-server-start.sh to apply the change.
🤖 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 `@redis/paymentCache.ts`:
- Around line 33-40: The current guard around cache warmup in getPaymentList
prevents the per-address lazy initialization when SKIP_CACHE_REBUILD is set,
causing a first-run cache miss to return an empty payment list; change the logic
so SKIP_CACHE_REBUILD only disables the bulk/warmup path but still iterates
getUserUncachedAddresses and calls CacheSet.addressCreation for each address (so
the DB-backed lazy path runs), updating the same behavior in the corresponding
getPaymentStream area (the code referencing getUserUncachedAddresses,
CacheSet.addressCreation, getCachedPaymentsForUser/getPaymentStream) to ensure
uncached addresses fallback to MariaDB rather than producing an empty result.
In `@scripts/db/Dockerfile`:
- Around line 5-6: The Dockerfile's RUN line uses "apt-get update || echo" and
"apt-get install -y gettext pv || echo" which hides failures and lets the image
build without envsubst/pv causing runtime errors in scripts/db-entrypoint.sh;
remove the "|| echo" fallbacks and make the RUN step fail on error (so apt-get
update and apt-get install return non-zero on failure), ensuring missing
packages like gettext (envsubst) and pv cause the build to fail early and
visibly.
---
Outside diff comments:
In `@docker-compose-from-dump.yml`:
- Around line 10-11: The paybutton service only references .env.from-dump which
causes production children (PM2 / yarn prod started by
scripts/paybutton-server-start.sh) to miss variables from .env/.env.local;
update the paybutton docker-compose service to include the base env files (add
.env and .env.local to env_file alongside .env.from-dump) or ensure the startup
script/supervisor (scripts/paybutton-server-start.sh / the PM2 start command
invoked) explicitly loads dotenv before launching yarn prod so that non-exported
KEY= values are available to child processes; locate the paybutton service block
in docker-compose-from-dump.yml and the startup in
scripts/paybutton-server-start.sh to apply the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ab7adb4-4c1c-4e91-a31f-c56f99442413
📒 Files selected for processing (8)
.env.from-dump.gitignoreMakefiledocker-compose-from-dump.ymlredis/paymentCache.tsscripts/db-entrypoint.shscripts/db/Dockerfilescripts/paybutton-server-start.sh
Comment on lines
+5
to
+6
| RUN apt-get update || echo && \ | ||
| apt-get install -y gettext || echo | ||
| apt-get install -y gettext pv || echo |
Contributor
There was a problem hiding this comment.
Fail the image build when gettext/pv installation fails.
The || echo clauses swallow apt-get failures, so this image can build without envsubst or pv and then fail much later inside scripts/db-entrypoint.sh. That makes the dump workflow flaky and much harder to diagnose.
Suggested fix
-RUN apt-get update || echo && \
- apt-get install -y gettext pv || echo
+RUN apt-get update && \
+ apt-get install -y --no-install-recommends gettext pv && \
+ rm -rf /var/lib/apt/lists/*🧰 Tools
🪛 Trivy (0.69.3)
[error] 5-6: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update || echo && apt-get install -y gettext pv || echo'
Rule: DS-0029
(IaC/Dockerfile)
🤖 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 `@scripts/db/Dockerfile` around lines 5 - 6, The Dockerfile's RUN line uses
"apt-get update || echo" and "apt-get install -y gettext pv || echo" which hides
failures and lets the image build without envsubst/pv causing runtime errors in
scripts/db-entrypoint.sh; remove the "|| echo" fallbacks and make the RUN step
fail on error (so apt-get update and apt-get install return non-zero on
failure), ensuring missing packages like gettext (envsubst) and pv cause the
build to fail early and visibly.
Fabcien
reviewed
Jun 2, 2026
| @@ -3,7 +3,7 @@ FROM mariadb:latest | |||
| RUN mkhomedir_helper mysql | |||
|
|
|||
| RUN apt-get update || echo && \ | |||
Collaborator
There was a problem hiding this comment.
Not sure I understand the use of || echo here, if the goal is to avoid failures then || true seems more appropriated
| if [ "$ENVIRONMENT" = "production" ]; then | ||
| pm2 start yarn --time --interpreter ash --name next --output logs/next.log --error logs/next.log -- prod || exit 1 | ||
| else | ||
| pm2 start yarn --time --interpreter ash --name next --output logs/next.log --error logs/next.log -- dev || exit 1 |
Collaborator
There was a problem hiding this comment.
This is unreachable, you're already in a if [ "$ENVIRONMENT" = "production" ]; branch
Edit: I see this is fixed in the follow up commit. Better squash in this case to make the review easier
chedieck
force-pushed
the
chore/dev-from-dump-improvement
branch
from
18879a6 to
85a2ba2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/db/Dockerfile (1)
5-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when installing required packages.
|| truehides install failures, so the image may build withoutgettext/pvand then fail during dump import runtime.Suggested fix
-RUN apt-get update || true && \ - apt-get install -y gettext pv || true +RUN apt-get update && \ + apt-get install -y --no-install-recommends gettext pv && \ + rm -rf /var/lib/apt/lists/*🤖 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 `@scripts/db/Dockerfile` around lines 5 - 6, The Dockerfile currently masks failures by appending "|| true" to the apt-get commands; remove those "|| true" tokens so the build fails on package-install errors and replace the line with a safe, failing-on-error install such as using apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends gettext pv && rm -rf /var/lib/apt/lists/* (keep the RUN instruction that contains the apt-get update/install sequence).Source: Linters/SAST tools
🤖 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 `@scripts/db-entrypoint.sh`:
- Line 27: The echo line uses unquoted $filesize when invoking numfmt which can
cause word-splitting or empty-argument issues; update the call in
scripts/db-entrypoint.sh so numfmt receives a single argument by quoting the
variable (use "$filesize" instead of $filesize) in the echo/numfmt pipeline
where the import message is printed.
- Around line 24-34: The loop that imports SQL files (the for file in *.sql; do
block) ignores failures from the mariadb and pv pipelines and still prints "Done
importing $file", risking partial DB initialization; modify the script to enable
strict failure handling (e.g., set -euo pipefail) or explicitly check the
command exit statuses after each import command (both pv "$file" | mariadb -u
root -p"$MYSQL_ROOT_PASSWORD" and mariadb -u root -p"$MYSQL_ROOT_PASSWORD" <
"$file") and if a command fails, print an error with the file name and exit
non‑zero immediately instead of printing "Done importing $file".
---
Duplicate comments:
In `@scripts/db/Dockerfile`:
- Around line 5-6: The Dockerfile currently masks failures by appending "||
true" to the apt-get commands; remove those "|| true" tokens so the build fails
on package-install errors and replace the line with a safe, failing-on-error
install such as using apt-get update && DEBIAN_FRONTEND=noninteractive apt-get
install -y --no-install-recommends gettext pv && rm -rf /var/lib/apt/lists/*
(keep the RUN instruction that contains the apt-get update/install sequence).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed7edc70-2290-4b93-8aba-d73c86ff544a
📒 Files selected for processing (8)
.env.from-dump.gitignoreMakefiledocker-compose-from-dump.ymlredis/paymentCache.tsscripts/db-entrypoint.shscripts/db/Dockerfilescripts/paybutton-server-start.sh
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- docker-compose-from-dump.yml
- Makefile
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/paybutton-server-start.sh
- redis/paymentCache.ts
Comment on lines
24
to
34
| for file in *.sql; do | ||
| mariadb -u root -p"$MYSQL_ROOT_PASSWORD" < "$file" | ||
| if [[ "$file" == *dump* ]]; then | ||
| filesize=$(stat -Lc%s "$file" 2>/dev/null || stat -Lf%z "$file" 2>/dev/null) | ||
| echo "Importing $file ($(numfmt --to=iec $filesize)) ..." | ||
| pv "$file" | mariadb -u root -p"$MYSQL_ROOT_PASSWORD" | ||
| else | ||
| echo "Importing $file ..." | ||
| mariadb -u root -p"$MYSQL_ROOT_PASSWORD" < "$file" | ||
| fi | ||
| echo "Done importing $file" | ||
| done |
Contributor
There was a problem hiding this comment.
Stop on import errors to avoid partial DB initialization.
This loop continues even if mariadb/pv fails, and still logs Done importing ..., which can leave a partially imported database while returning success.
Suggested fix
+set -euo pipefail
+
for file in *.sql; do
if [[ "$file" == *dump* ]]; then
filesize=$(stat -Lc%s "$file" 2>/dev/null || stat -Lf%z "$file" 2>/dev/null)
echo "Importing $file ($(numfmt --to=iec $filesize)) ..."
- pv "$file" | mariadb -u root -p"$MYSQL_ROOT_PASSWORD"
+ pv "$file" | mariadb -u root -p"$MYSQL_ROOT_PASSWORD"
else
echo "Importing $file ..."
mariadb -u root -p"$MYSQL_ROOT_PASSWORD" < "$file"
fi
echo "Done importing $file"
done📝 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
| for file in *.sql; do | |
| mariadb -u root -p"$MYSQL_ROOT_PASSWORD" < "$file" | |
| if [[ "$file" == *dump* ]]; then | |
| filesize=$(stat -Lc%s "$file" 2>/dev/null || stat -Lf%z "$file" 2>/dev/null) | |
| echo "Importing $file ($(numfmt --to=iec $filesize)) ..." | |
| pv "$file" | mariadb -u root -p"$MYSQL_ROOT_PASSWORD" | |
| else | |
| echo "Importing $file ..." | |
| mariadb -u root -p"$MYSQL_ROOT_PASSWORD" < "$file" | |
| fi | |
| echo "Done importing $file" | |
| done | |
| set -euo pipefail | |
| for file in *.sql; do | |
| if [[ "$file" == *dump* ]]; then | |
| filesize=$(stat -Lc%s "$file" 2>/dev/null || stat -Lf%z "$file" 2>/dev/null) | |
| echo "Importing $file ($(numfmt --to=iec $filesize)) ..." | |
| pv "$file" | mariadb -u root -p"$MYSQL_ROOT_PASSWORD" | |
| else | |
| echo "Importing $file ..." | |
| mariadb -u root -p"$MYSQL_ROOT_PASSWORD" < "$file" | |
| fi | |
| echo "Done importing $file" | |
| done |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 27-27: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@scripts/db-entrypoint.sh` around lines 24 - 34, The loop that imports SQL
files (the for file in *.sql; do block) ignores failures from the mariadb and pv
pipelines and still prints "Done importing $file", risking partial DB
initialization; modify the script to enable strict failure handling (e.g., set
-euo pipefail) or explicitly check the command exit statuses after each import
command (both pv "$file" | mariadb -u root -p"$MYSQL_ROOT_PASSWORD" and mariadb
-u root -p"$MYSQL_ROOT_PASSWORD" < "$file") and if a command fails, print an
error with the file name and exit non‑zero immediately instead of printing "Done
importing $file".
| mariadb -u root -p"$MYSQL_ROOT_PASSWORD" < "$file" | ||
| if [[ "$file" == *dump* ]]; then | ||
| filesize=$(stat -Lc%s "$file" 2>/dev/null || stat -Lf%z "$file" 2>/dev/null) | ||
| echo "Importing $file ($(numfmt --to=iec $filesize)) ..." |
Contributor
There was a problem hiding this comment.
Quote filesize when calling numfmt.
Unquoted $filesize can trigger argument-splitting/empty-arg behavior; quote it to keep parsing deterministic.
Suggested fix
- echo "Importing $file ($(numfmt --to=iec $filesize)) ..."
+ echo "Importing $file ($(numfmt --to=iec "$filesize")) ..."📝 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
| echo "Importing $file ($(numfmt --to=iec $filesize)) ..." | |
| echo "Importing $file ($(numfmt --to=iec "$filesize")) ..." |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 27-27: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@scripts/db-entrypoint.sh` at line 27, The echo line uses unquoted $filesize
when invoking numfmt which can cause word-splitting or empty-argument issues;
update the call in scripts/db-entrypoint.sh so numfmt receives a single argument
by quoting the variable (use "$filesize" instead of $filesize) in the
echo/numfmt pipeline where the import message is printed.
Source: Linters/SAST tools
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.
Description
make dev-from-dumpworkflow to spin up a local dev env with a prod database dumppvprogress bar during dump importmigrate deploysince dump already has schema + migration history)New make targets
make stop-dev-from-dump— stop without losing DBmake reset-dev-from-dump— full restartmake reset-dev-from-dump-keep-db— restart dev container only, DB staysmake nuke-dev-from-dump— destroy everything including DB volumeRemarks
dump.sqlin the repo root (frommariadb-dumpon prod)Test Plan
make dev-from-dump— watch db logs for pv progressmake stop-dev-from-dump && make dev-from-dump— should skip import (instant start)make nuke-dev-from-dump && make dev-from-dump— should reimport from scratchSummary by CodeRabbit
Chores
Performance
Usability