fix: improve prod Docker build (#15)#971
Conversation
Summary by CodeRabbit
WalkthroughThis PR updates Docker build/runtime configuration—installing chromium, excluding .venv, and patching entrypoint migrations and SQLite schema—alongside frontend TypeScript fixes to ref types, an ElementType-based route contract, and minor build/tooling config adjustments in package.json and tsconfig.json. ChangesDocker Build and Runtime
Frontend Type Corrections and Build Config
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
application/frontend/src/pages/Explorer/visuals/force-graph/forceGraph.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. application/frontend/src/pages/GapAnalysis/GapAnalysis.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. application/frontend/src/pages/chatbot/chatbot.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
Exclude .venv/ from build context and install system chromium in the node build stage (mirrors PR #641) so yarn install succeeds on arm64. Co-authored-by: Cursor <cursoragent@cursor.com>
Exclude built frontend assets from tsconfig, fix TypeScript errors surfacing under node:lts, remove broken @types/minimatch stub on install, and harden prod-docker-entrypoint (alembic heads + document_metadata + upstream_sync) so the container serves standards data after startup. Co-authored-by: Cursor <cursoragent@cursor.com>
northdpole
force-pushed
the
fix/15-docker-prod-build
branch
from
db5e913 to
a6949de
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/prod-docker-entrypoint.sh (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
$(pwd)over legacy backticks.Shellcheck flags the use of backticks (SC2006) and recommends declaring and assigning separately to avoid masking return values (SC2155).
♻️ Proposed fix
-export FLASK_APP=`pwd`/cre.py +FLASK_APP="$(pwd)/cre.py" +export FLASK_APP🤖 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/prod-docker-entrypoint.sh` at line 5, The `prod-docker-entrypoint.sh` setup is using legacy backticks in the `FLASK_APP` assignment, which shellcheck flags and which can mask command return values. Update the `export FLASK_APP=...` line to use `$(pwd)` instead of backticks, and keep the assignment straightforward in the entrypoint script so the `FLASK_APP` value is set cleanly.Source: Linters/SAST tools
Dockerfile (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
--no-install-recommendsand clean apt cache to reduce image size.Without
--no-install-recommends,chromiumpulls in many recommended packages that bloat the image and increase the attack surface. Cleaning the apt lists afterward prevents them from being baked into the layer.♻️ Proposed fix
-RUN apt-get update && apt-get install -y chromium +RUN apt-get update && apt-get install -y --no-install-recommends chromium \ + && rm -rf /var/lib/apt/lists/*Source: Linters/SAST tools
application/frontend/src/pages/GapAnalysis/GapAnalysis.tsx (1)
176-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider resetting
timerIdRef.currenttoundefinedafter clearing.After
clearInterval, the ref still holds the stale interval ID. SubsequentstopPollingcalls pass the!== undefinedguard and callclearIntervalon an already-cleared ID (harmless no-op, but misleading). Resetting toundefinedkeeps the ref's state accurate.♻️ Suggested refactor
const stopPolling = () => { if (timerIdRef.current !== undefined) { clearInterval(timerIdRef.current); + timerIdRef.current = undefined; } };🤖 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 `@application/frontend/src/pages/GapAnalysis/GapAnalysis.tsx` around lines 176 - 178, In stopPolling, the timerIdRef.current value is left pointing to a cleared interval ID after clearInterval, so update the GapAnalysis polling cleanup to reset timerIdRef.current back to undefined immediately after clearing it. Keep the existing guard, but make the ref state accurately reflect that no interval is active so repeated stopPolling calls behave consistently.
🤖 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/prod-docker-entrypoint.sh`:
- Around line 7-18: Add a startup failure guard in the prod entrypoint so schema
patching never silently continues on error: the Python block that opens
sqlite3.connect(db_path) and loops over cre/node should first verify each table
exists before calling PRAGMA table_info and ALTER TABLE, and skip or fail
cleanly if it does not. Also add set -e at the top of the script so any failed
migration or Python exception stops startup instead of letting gunicorn launch
with a broken database schema.
---
Nitpick comments:
In `@application/frontend/src/pages/GapAnalysis/GapAnalysis.tsx`:
- Around line 176-178: In stopPolling, the timerIdRef.current value is left
pointing to a cleared interval ID after clearInterval, so update the GapAnalysis
polling cleanup to reset timerIdRef.current back to undefined immediately after
clearing it. Keep the existing guard, but make the ref state accurately reflect
that no interval is active so repeated stopPolling calls behave consistently.
In `@scripts/prod-docker-entrypoint.sh`:
- Line 5: The `prod-docker-entrypoint.sh` setup is using legacy backticks in the
`FLASK_APP` assignment, which shellcheck flags and which can mask command return
values. Update the `export FLASK_APP=...` line to use `$(pwd)` instead of
backticks, and keep the assignment straightforward in the entrypoint script so
the `FLASK_APP` value is set cleanly.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: cddaef2c-b0c3-4f56-a85f-43339ec8e01a
📒 Files selected for processing (9)
.dockerignoreDockerfileapplication/frontend/src/pages/Explorer/visuals/force-graph/forceGraph.tsxapplication/frontend/src/pages/GapAnalysis/GapAnalysis.tsxapplication/frontend/src/pages/chatbot/chatbot.tsxapplication/frontend/src/routes.tsxpackage.jsonscripts/prod-docker-entrypoint.shtsconfig.json
| python - <<'PY' | ||
| import sqlite3 | ||
|
|
||
| db_path = "/code/standards_cache.sqlite" | ||
| conn = sqlite3.connect(db_path) | ||
| for table in ("cre", "node"): | ||
| cols = {row[1] for row in conn.execute(f"PRAGMA table_info({table})")} | ||
| if "document_metadata" not in cols: | ||
| conn.execute(f"ALTER TABLE {table} ADD COLUMN document_metadata JSON") | ||
| conn.commit() | ||
| conn.close() | ||
| PY |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle missing tables and add set -e to prevent serving a broken app.
Two stability risks in this block:
-
If the
creornodetables don't exist instandards_cache.sqlite(e.g., fresh database orflask db upgrade headsfailed silently),PRAGMA table_inforeturns an empty set,"document_metadata" not in colsisTrue, andALTER TABLE cre ADD COLUMN ...throwssqlite3.OperationalError: no such table. The exception is unhandled. -
The script has no
set -e, so both the migration failure (line 6) and the Python crash are silently swallowed —gunicornstarts and serves an app with a broken database schema.
Consider guarding the ALTER TABLE with a table-existence check and adding set -e to the script so migration or schema-patch failures halt startup instead of silently serving a broken app.
🛡️ Proposed fix: guard ALTER TABLE and add set -e
#! /bin/bash
+set -e
+
export INSECURE_REQUESTS=1
export FLASK_CONFIG="production"
export FLASK_APP=`pwd`/cre.py
flask db upgrade heads
python - <<'PY'
import sqlite3
db_path = "/code/standards_cache.sqlite"
conn = sqlite3.connect(db_path)
for table in ("cre", "node"):
cols = {row[1] for row in conn.execute(f"PRAGMA table_info({table})")}
- if "document_metadata" not in cols:
+ if cols and "document_metadata" not in cols:
conn.execute(f"ALTER TABLE {table} ADD COLUMN document_metadata JSON")
conn.commit()
conn.close()
PY🤖 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/prod-docker-entrypoint.sh` around lines 7 - 18, Add a startup failure
guard in the prod entrypoint so schema patching never silently continues on
error: the Python block that opens sqlite3.connect(db_path) and loops over
cre/node should first verify each table exists before calling PRAGMA table_info
and ALTER TABLE, and skip or fail cleanly if it does not. Also add set -e at the
top of the script so any failed migration or Python exception stops startup
instead of letting gunicorn launch with a broken database schema.
1 participant
Summary
.venv/to.dockerignoreso local Python virtualenvs are not sent to the Docker build context (repo convention uses.venv, not onlyvenv/).chromiumin the prodDockerfilenode build stage beforeyarn install, mirroring PR #641 /Dockerfile-dev. This unblockspuppeteerpostinstall on arm64 Linux without affecting amd64 CI (publish.ymlruns onubuntu-latestwhere both apt chromium and puppeteer's bundled binary are available).Fixes #15 (build-context and arm64 yarn install portions).
Test plan
make docker-prodon arm64/macOS (after freeing Docker VM disk if needed)make docker-prodon linux/amd64 (matchespublish.yml).venv/is excluded:docker buildcontext size should not balloon when.venvexists locallyNotes
make lint—blackpassed;yarn lintfailed locally due to pre-existing Yarn 4 / lockfile mismatch (unrelated to this diff).Made with Cursor