chore: dev from dump improvement#1131
Conversation
|
Warning Review limit reached
More reviews will be available in 59 minutes and 2 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces a new development workflow mode called "dev-from-dump" that allows developers to initialize the application state from database dumps rather than running seeds. It includes orchestration targets, dump-aware database import logic with progress tracking, and application behavior that adapts to skip cache rebuilds and run production-style migrations. ChangesDev-from-dump Development Workflow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 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.
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
| export const getPaymentList = async (userId: string): Promise<Payment[]> => { | ||
| const uncachedAddressStream = getUserUncachedAddresses(userId) | ||
| for await (const address of uncachedAddressStream) { | ||
| void await CacheSet.addressCreation(address) | ||
| if (process.env.SKIP_CACHE_REBUILD === undefined) { | ||
| const uncachedAddressStream = getUserUncachedAddresses(userId) | ||
| for await (const address of uncachedAddressStream) { | ||
| void await CacheSet.addressCreation(address) | ||
| } | ||
| } | ||
| return await getCachedPaymentsForUser(userId) |
There was a problem hiding this comment.
Don't turn first-run cache misses into empty payment history.
With SKIP_CACHE_REBUILD=true in .env.from-dump and a fresh Redis container in docker-compose-from-dump.yml, the first dev-from-dump boot has no payment cache yet. These guards now skip both the eager rebuild and the per-address lazy init, so getPaymentList / getPaymentStream can return zero payments for real production data instead of falling back to MariaDB.
Keep the bulk warmup disabled if needed, but uncached addresses still need a lazy DB-backed path.
Also applies to: 309-323
🤖 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 `@redis/paymentCache.ts` around lines 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.
| RUN apt-get update || echo && \ | ||
| apt-get install -y gettext || echo | ||
| apt-get install -y gettext pv || echo |
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.
| @@ -3,7 +3,7 @@ FROM mariadb:latest | |||
| RUN mkhomedir_helper mysql | |||
|
|
|||
| RUN apt-get update || echo && \ | |||
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
| for file in *.sql; do | ||
| mariadb -u root -p"$MYSQL_ROOT_PASSWORD" < "$file" | ||
| if [[ "$file" == *dump* ]]; then | ||
| filesize=$(stat -c%s "$file" 2>/dev/null || stat -f%z "$file" 2>/dev/null) |
There was a problem hiding this comment.
I don't get the second part of the subcommand stat -f%z, what does this do ?
| 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 |
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
| paybutton-config.json | ||
|
|
||
| dump.sql* | ||
| .forever |
There was a problem hiding this comment.
out of curiosity, what is this?
chedieck
force-pushed
the
chore/dev-from-dump-improvement
branch
from
18879a6 to
85a2ba2
Compare
2 participants
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
Release Notes
Chores
Performance