feat: simplify setup process and enhance environment configuration#41
Merged
Conversation
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-41
|
🚅 Deployed to the applirank-pr-41 environment in applirank
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Docker-first infra and developer flows: new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CI as CI
participant DockerCompose as DockerCompose
participant App as App
participant Postgres as Postgres
participant Minio as Minio
participant Adminer as Adminer
User->>DockerCompose: ./setup.sh (generate .env)
User->>DockerCompose: docker compose up --build -d
DockerCompose->>Postgres: start + healthcheck
DockerCompose->>Minio: start + healthcheck
DockerCompose->>App: build image (Dockerfile) and start
App->>Postgres: run migrations
App->>Minio: create bucket / verify readiness
CI->>App: readiness probe (HTTP /)
CI->>App: optional db:seed and sign-in smoke tests
DockerCompose->>Adminer: available via tools profile
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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
🧹 Nitpick comments (4)
Dockerfile (2)
27-35: Redundant migration copy — line 29 is superseded by line 35.Line 29 copies
server/database/migrationsinto./server/database/migrations, but line 35 then copies the entire./serverdirectory over it (same source, same destination subtree). The explicit migration copy is a no-op.Either remove line 29 (and update the comment) since line 35 covers it, or reorder so the narrower copy comes after the broader one — though that would also be a no-op. The cleanest fix is to drop lines 27-29 and add the migration rationale as a comment near line 35.
♻️ Proposed fix — remove redundant copy
-# Drizzle migrations are loaded at runtime via a relative path ("./server/database/migrations") -# They must live alongside .output so the path resolves correctly inside the container -COPY --from=builder /app/server/database/migrations ./server/database/migrations - -# Seed script support — copies node_modules, package.json, and server source +# Seed + migration support — copies node_modules, package.json, and full server source # so `docker compose exec app npm run db:seed` works inside the container +# Also ensures server/database/migrations is available at runtime (loaded via relative path) COPY --from=builder /app/package.json ./package.json COPY --from=builder /app/node_modules ./node_modules COPY --from=builder /app/server ./server🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 27 - 35, Remove the redundant explicit migration COPY that copies /app/server/database/migrations into ./server/database/migrations (the earlier COPY --from=builder for that path), and instead keep only the broader COPY --from=builder /app/server ./server; move the migration rationale comment to the block that copies /app/server so it explains why migrations must live alongside .output. Update the Dockerfile by deleting the narrow migration COPY and its comment and adding the migration rationale as a comment adjacent to the COPY --from=builder /app/server ./server line.
33-35: Copying fullnode_modulesinto the runner bloats the image significantly.The entire dev+prod
node_modulesfrom the builder is copied just to supportnpm run db:seed. This can add hundreds of MBs. Consider a lighter approach — e.g., a separate seed stage/image, or runningnpm ci --omit=devto install only production dependencies in the runner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 33 - 35, The Dockerfile currently copies the entire node_modules from the builder (lines with "COPY --from=builder /app/node_modules ./node_modules") which bloats the final image to support only the npm run db:seed step; change to a lighter approach by either adding a dedicated seed stage (e.g., create a "seeder" build stage that runs npm ci and executes npm run db:seed then exits) or modify the final runner stage to install only production dependencies (run npm ci --omit=dev or npm ci --production) and then run the seed command, removing the full node_modules COPY; update references to the COPY --from=builder /app/node_modules ./node_modules and ensure npm run db:seed is executed from the new seeder stage or after a prod-only install.setup.sh (1)
27-27: Consider restricting.envfile permissions since it contains secrets.The generated
.envcontains passwords and auth secrets. Setting restrictive permissions before writing prevents other users on shared machines from reading it.🛡️ Proposed fix
+# Create .env with restrictive permissions before writing secrets +(umask 077 && cat > .env <<EOF -cat > .env <<EOF ... EOF +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` at line 27, The script currently writes secrets directly with "cat > .env <<EOF" without setting file permissions; change the flow in setup.sh to create .env with restrictive mode (e.g., umask or chmod 600) before or immediately after writing: either set umask 177 at the top of the block or create the file with touch and chmod 600, then redirect the heredoc into that file (refer to the "cat > .env <<EOF" operation and the ".env" target) so only the owner can read/write the generated .env.docker-compose.yml (1)
39-54: No healthcheck on theappservice.Both
dbandminiodefine healthchecks, butappdoesn't. Adding one would let any futuredepends_on: app: condition: service_healthywork, and gives better visibility indocker compose ps.💡 Example healthcheck
ports: - "3000:3000" + healthcheck: + test: ["CMD-SHELL", "wget -qO- http://localhost:3000/ || exit 1"] + interval: 10s + timeout: 5s + retries: 5 depends_on:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 39 - 54, Add a Docker healthcheck for the app service so it reports healthy/unhealthy and supports future depends_on: condition: service_healthy. In the docker-compose service block for "app" add a healthcheck that probes the app (for example an HTTP check against http://localhost:3000/health or a CMD-SHELL curl -f ...), and include sensible keys: test, interval, timeout, retries (and optional start_period) to avoid false negatives; update service "app" so its healthcheck is present alongside existing services "db" and "minio".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 39-54: The app service currently exposes port mapping without a
host IP (ports: - "3000:3000"), making it reachable on all interfaces; change
the app service's ports entry to bind to localhost like the db and minio
services (use "127.0.0.1:3000:3000") to restrict external access and let the
reverse proxy handle public traffic—update the docker-compose service named app
and the ports mapping accordingly.
In `@Dockerfile`:
- Around line 18-39: The runner stage in the Dockerfile runs as root; create a
non-root user and switch to it before CMD to satisfy Trivy DS-0002. In the "FROM
node:20-alpine AS runner" stage add commands to create a dedicated user/group
(e.g., appuser), ensure /app ownership is changed to that user (chown /app), and
set USER to that non-root account so the container runs without root privileges
when executing CMD ["node", ".output/server/index.mjs"].
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 39-54: Add a Docker healthcheck for the app service so it reports
healthy/unhealthy and supports future depends_on: condition: service_healthy. In
the docker-compose service block for "app" add a healthcheck that probes the app
(for example an HTTP check against http://localhost:3000/health or a CMD-SHELL
curl -f ...), and include sensible keys: test, interval, timeout, retries (and
optional start_period) to avoid false negatives; update service "app" so its
healthcheck is present alongside existing services "db" and "minio".
In `@Dockerfile`:
- Around line 27-35: Remove the redundant explicit migration COPY that copies
/app/server/database/migrations into ./server/database/migrations (the earlier
COPY --from=builder for that path), and instead keep only the broader COPY
--from=builder /app/server ./server; move the migration rationale comment to the
block that copies /app/server so it explains why migrations must live alongside
.output. Update the Dockerfile by deleting the narrow migration COPY and its
comment and adding the migration rationale as a comment adjacent to the COPY
--from=builder /app/server ./server line.
- Around line 33-35: The Dockerfile currently copies the entire node_modules
from the builder (lines with "COPY --from=builder /app/node_modules
./node_modules") which bloats the final image to support only the npm run
db:seed step; change to a lighter approach by either adding a dedicated seed
stage (e.g., create a "seeder" build stage that runs npm ci and executes npm run
db:seed then exits) or modify the final runner stage to install only production
dependencies (run npm ci --omit=dev or npm ci --production) and then run the
seed command, removing the full node_modules COPY; update references to the COPY
--from=builder /app/node_modules ./node_modules and ensure npm run db:seed is
executed from the new seeder stage or after a prod-only install.
In `@setup.sh`:
- Line 27: The script currently writes secrets directly with "cat > .env <<EOF"
without setting file permissions; change the flow in setup.sh to create .env
with restrictive mode (e.g., umask or chmod 600) before or immediately after
writing: either set umask 177 at the top of the block or create the file with
touch and chmod 600, then redirect the heredoc into that file (refer to the "cat
> .env <<EOF" operation and the ".env" target) so only the owner can read/write
the generated .env.
Comment on lines
+39
to
+54
| app: | ||
| build: . | ||
| container_name: applirank_app | ||
| restart: unless-stopped | ||
| env_file: .env | ||
| environment: | ||
| # Override localhost URLs with Docker-internal hostnames | ||
| DATABASE_URL: postgresql://${DB_USER}:${DB_PASSWORD}@db:5432/${DB_NAME} | ||
| S3_ENDPOINT: http://minio:9000 | ||
| ports: | ||
| - "3000:3000" | ||
| depends_on: | ||
| db: | ||
| condition: service_healthy | ||
| minio: | ||
| condition: service_healthy |
There was a problem hiding this comment.
App port binds to all interfaces — consider 127.0.0.1 like db and minio.
The db and minio services explicitly bind to 127.0.0.1 with comments warning against public exposure. The app service binds to 0.0.0.0:3000 (the default when no host IP is specified), making it directly reachable from the network. If this compose file is used on a VPS (as described in the Deployment section), the app would be accessible bypassing Caddy/HTTPS.
For consistency and defense-in-depth, bind to localhost and let the reverse proxy handle external traffic.
🔒 Proposed fix
ports:
- - "3000:3000"
+ - "127.0.0.1:3000:3000"📝 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
| app: | |
| build: . | |
| container_name: applirank_app | |
| restart: unless-stopped | |
| env_file: .env | |
| environment: | |
| # Override localhost URLs with Docker-internal hostnames | |
| DATABASE_URL: postgresql://${DB_USER}:${DB_PASSWORD}@db:5432/${DB_NAME} | |
| S3_ENDPOINT: http://minio:9000 | |
| ports: | |
| - "3000:3000" | |
| depends_on: | |
| db: | |
| condition: service_healthy | |
| minio: | |
| condition: service_healthy | |
| app: | |
| build: . | |
| container_name: applirank_app | |
| restart: unless-stopped | |
| env_file: .env | |
| environment: | |
| # Override localhost URLs with Docker-internal hostnames | |
| DATABASE_URL: postgresql://${DB_USER}:${DB_PASSWORD}@db:5432/${DB_NAME} | |
| S3_ENDPOINT: http://minio:9000 | |
| ports: | |
| - "127.0.0.1:3000:3000" | |
| depends_on: | |
| db: | |
| condition: service_healthy | |
| minio: | |
| condition: service_healthy |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` around lines 39 - 54, The app service currently exposes
port mapping without a host IP (ports: - "3000:3000"), making it reachable on
all interfaces; change the app service's ports entry to bind to localhost like
the db and minio services (use "127.0.0.1:3000:3000") to restrict external
access and let the reverse proxy handle public traffic—update the docker-compose
service named app and the ports mapping accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/docker-readme-validation.yml (2)
236-245: Minor: redundant grep alternative with-iflag.Line 241 uses
grep -qi "^npm error\|unhandledRejection\|UnhandledPromiseRejection". Because-imakes the match case-insensitive,unhandledRejectionalready coversUnhandledPromiseRejection. The third alternative is redundant.Very minor nit — no functional impact.
Proposed simplification
- if echo "$output" | grep -qi "^npm error\|unhandledRejection\|UnhandledPromiseRejection"; then + if echo "$output" | grep -qi "^npm error\|unhandledRejection"; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker-readme-validation.yml around lines 236 - 245, In the "Seed idempotency — re-running seed must not crash" CI step, simplify the grep pattern by removing the redundant alternative "UnhandledPromiseRejection" from the command that checks output (the line containing grep -qi "^npm error\|unhandledRejection\|UnhandledPromiseRejection"); keep a case-insensitive match for "unhandledRejection" and "npm error" only so the check remains correct but with one fewer redundant alternative.
98-152: Health-check loops are well-structured.The timeout logic is correct—on the final iteration, a successful health state is detected before the timeout branch fires. The app readiness probe additionally checks for premature container exits, which is a nice touch.
One optional consideration: these three polling loops (db, minio, app) share the same structure. If you anticipate adding more services in the future, extracting a reusable
wait_for_healthy <container> <retries> <sleep>shell function could reduce duplication. Not necessary now, given readability is good.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker-readme-validation.yml around lines 98 - 152, Extract the repeated polling logic into a reusable shell function (e.g., wait_for_healthy) and call it for the DB and MinIO checks to remove duplication; implement wait_for_healthy to accept container name, retries, and sleep interval and perform the docker inspect loop currently duplicated in the blocks that reference applirank_db and applirank_minio, preserving the same exit/log behavior, and optionally add a separate wait_for_reachable(container, url, retries, sleep) wrapper for the app readiness probe that preserves the curl check and premature-container-exit handling for applirank_app.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/docker-readme-validation.yml:
- Around line 265-287: The post-restart check currently greps the entire
container log with `docker compose logs app | grep -q "Migration failed"`, which
can match pre-restart transient errors; before calling `docker compose restart
app` capture the current log length or timestamp (e.g. count lines via `docker
compose logs app | wc -l` or record the last log timestamp), then after the
restart only search the new log slice (use `tail -n +<previous_line_count+1>` or
filter logs newer than the recorded timestamp) and run the `grep -q "Migration
failed"` against that sliced output so only post-restart messages are
considered.
---
Nitpick comments:
In @.github/workflows/docker-readme-validation.yml:
- Around line 236-245: In the "Seed idempotency — re-running seed must not
crash" CI step, simplify the grep pattern by removing the redundant alternative
"UnhandledPromiseRejection" from the command that checks output (the line
containing grep -qi "^npm
error\|unhandledRejection\|UnhandledPromiseRejection"); keep a case-insensitive
match for "unhandledRejection" and "npm error" only so the check remains correct
but with one fewer redundant alternative.
- Around line 98-152: Extract the repeated polling logic into a reusable shell
function (e.g., wait_for_healthy) and call it for the DB and MinIO checks to
remove duplication; implement wait_for_healthy to accept container name,
retries, and sleep interval and perform the docker inspect loop currently
duplicated in the blocks that reference applirank_db and applirank_minio,
preserving the same exit/log behavior, and optionally add a separate
wait_for_reachable(container, url, retries, sleep) wrapper for the app readiness
probe that preserves the curl check and premature-container-exit handling for
applirank_app.
Comment on lines
+265
to
+287
| # ── 8. Restart resilience (migrations must be idempotent) ──────────────── | ||
| - name: Restart app and verify it comes back clean | ||
| run: | | ||
| set -euo pipefail | ||
| docker compose restart app | ||
| for i in $(seq 30); do | ||
| if curl -fs http://localhost:3000 > /dev/null 2>&1; then | ||
| echo "✅ App reachable again after restart" | ||
| break | ||
| fi | ||
| if [ "$i" -eq 30 ]; then | ||
| echo "❌ App did not come back after restart" | ||
| docker compose logs app --tail=50 | ||
| exit 1 | ||
| fi | ||
| sleep 3 | ||
| done | ||
| if docker compose logs app | grep -q "Migration failed"; then | ||
| echo "❌ Migration error found in logs after restart" | ||
| docker compose logs app | ||
| exit 1 | ||
| fi | ||
| echo "✅ Restart handled cleanly — no migration errors" |
There was a problem hiding this comment.
Post-restart migration check searches cumulative logs.
docker compose logs app (Line 282) returns all logs since the container was created, not just logs emitted after the restart. If a transient "Migration failed" message appeared during the initial startup (and was subsequently retried successfully), this grep would still flag it as a failure after the restart step.
In practice, this is mitigated by the earlier step (Line 158) that already asserts migrations succeeded, so a false positive here is unlikely. But if you want the restart-resilience check to be self-contained, you could snapshot a log line count or timestamp before docker compose restart and only grep the tail.
Possible approach: only check logs after restart
- name: Restart app and verify it comes back clean
run: |
set -euo pipefail
+ # Capture current log line count so we only inspect post-restart output
+ pre_restart_lines="$(docker compose logs app --no-color 2>/dev/null | wc -l)"
docker compose restart app
for i in $(seq 30); do
if curl -fs http://localhost:3000 > /dev/null 2>&1; then
echo "✅ App reachable again after restart"
break
fi
if [ "$i" -eq 30 ]; then
echo "❌ App did not come back after restart"
docker compose logs app --tail=50
exit 1
fi
sleep 3
done
- if docker compose logs app | grep -q "Migration failed"; then
+ if docker compose logs app --no-color 2>/dev/null | tail -n +"$((pre_restart_lines + 1))" | grep -q "Migration failed"; then
echo "❌ Migration error found in logs after restart"
docker compose logs app
exit 1
fi
echo "✅ Restart handled cleanly — no migration errors"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/docker-readme-validation.yml around lines 265 - 287, The
post-restart check currently greps the entire container log with `docker compose
logs app | grep -q "Migration failed"`, which can match pre-restart transient
errors; before calling `docker compose restart app` capture the current log
length or timestamp (e.g. count lines via `docker compose logs app | wc -l` or
record the last log timestamp), then after the restart only search the new log
slice (use `tail -n +<previous_line_count+1>` or filter logs newer than the
recorded timestamp) and run the `grep -q "Migration failed"` against that sliced
output so only post-restart messages are considered.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Dockerfile (1)
19-41: Container still runs as root — switch to a non-root user.This repeats a previously flagged issue and is still present. Please add a dedicated user/group and
USERbeforeCMDin the runner stage.
Line 19–41.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 19 - 41, The runner stage in the Dockerfile runs the app as root; create a non-root user/group in that stage (e.g., addgroup -S app && adduser -S -G app app), chown the runtime files (./.output, ./server, ./node_modules, ./package.json, ./drizzle.config.ts, ./server/database/migrations) to that user, and set USER app before the CMD so CMD ["node", ".output/server/index.mjs"] runs as the non-root user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Dockerfile`:
- Around line 19-41: The runner stage in the Dockerfile runs the app as root;
create a non-root user/group in that stage (e.g., addgroup -S app && adduser -S
-G app app), chown the runtime files (./.output, ./server, ./node_modules,
./package.json, ./drizzle.config.ts, ./server/database/migrations) to that user,
and set USER app before the CMD so CMD ["node", ".output/server/index.mjs"] runs
as the non-root user.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.vscode/settings.jsonDockerfile
💤 Files with no reviewable changes (1)
- .vscode/settings.json
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-41
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/middleware/demo-guard.ts (1)
95-110: Dev-mode bypass is silent — consider logging when guard is inactive due to unresolvable org.Line 100:
if (!isExplicitlyConfigured || import.meta.dev) returnThe
import.meta.devarm is the only path not covered by the adjacent comment. When a developer explicitly setsDEMO_ORG_SLUGbut the org doesn't exist in their local DB (common in fresh checkouts), the middleware silently passes all writes through. There's no indication in logs that the demo guard is effectively disabled, which can mask misconfiguration during local development.🪵 Proposed: add a dev-mode warning when guard is inactive
- if (!isExplicitlyConfigured || import.meta.dev) return + if (!isExplicitlyConfigured) return + if (import.meta.dev) { + console.warn( + `[demo-guard] DEMO_ORG_SLUG is set but none of the configured slugs resolved to an org: ${demoSlugs.join(', ')}. Demo guard is inactive.`, + ) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/middleware/demo-guard.ts` around lines 95 - 110, The dev-mode bypass currently returns silently at the check in the guardedOrgIds === 0 block (the if (!isExplicitlyConfigured || import.meta.dev) return line); add a warning log specifically when import.meta.dev is true so developers know the demo guard was skipped due to unresolved orgs. Modify the branch to, when import.meta.dev is true, emit a clear warning (using the available logger if present, e.g. processLogger.warn, or console.warn) that includes the demoSlugs and the value of isExplicitlyConfigured, then return; leave the createError throw for non-dev explicit configurations unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/middleware/demo-guard.ts`:
- Around line 95-110: The dev-mode bypass currently returns silently at the
check in the guardedOrgIds === 0 block (the if (!isExplicitlyConfigured ||
import.meta.dev) return line); add a warning log specifically when
import.meta.dev is true so developers know the demo guard was skipped due to
unresolved orgs. Modify the branch to, when import.meta.dev is true, emit a
clear warning (using the available logger if present, e.g. processLogger.warn,
or console.warn) that includes the demoSlugs and the value of
isExplicitlyConfigured, then return; leave the createError throw for non-dev
explicit configurations unchanged.
…seed script support
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-41
railway-app
Bot
temporarily deployed
to
applirank / applirank-pr-41
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/middleware/demo-guard.ts (1)
28-47:isRailwayPreviewEnvironmentis called twice per request on the hot path.
isRailwayPreviewEnvironment(env.RAILWAY_ENVIRONMENT_NAME)is evaluated insidegetConfiguredDemoSlugs(line 43) and again on line 100. Since it's a pure function over a stable env var the result is always identical. Surfacing the value throughDemoSlugsResultremoves the redundant call and makes the returned object fully self-contained.Additionally, the
!isExplicitlyConfiguredsub-condition in line 101 is logically covered byisPreview: whendemoSlugs.length > 0(line 89 early-return was skipped), the only wayisExplicitlyConfiguredcan befalseis if the Railway-preview branch added the default slug — meaningisPreviewis alreadytrue. The clause is harmless but technically redundant.♻️ Proposed refactor — surface
isPreviewfromDemoSlugsResultinterface DemoSlugsResult { slugs: string[] /** True only when DEMO_ORG_SLUG was explicitly set by the operator. */ isExplicitlyConfigured: boolean + /** True when running inside a Railway PR/preview environment. */ + isPreview: boolean } function getConfiguredDemoSlugs(): DemoSlugsResult { const slugs = new Set<string>() let isExplicitlyConfigured = false if (env.DEMO_ORG_SLUG) { slugs.add(env.DEMO_ORG_SLUG) isExplicitlyConfigured = true } + const isPreview = isRailwayPreviewEnvironment(env.RAILWAY_ENVIRONMENT_NAME) - if (isRailwayPreviewEnvironment(env.RAILWAY_ENVIRONMENT_NAME)) { + if (isPreview) { slugs.add(DEFAULT_PREVIEW_DEMO_ORG_SLUG) } - return { slugs: [...slugs], isExplicitlyConfigured } + return { slugs: [...slugs], isExplicitlyConfigured, isPreview } }Then at the call site:
- const { slugs: demoSlugs, isExplicitlyConfigured } = getConfiguredDemoSlugs() + const { slugs: demoSlugs, isExplicitlyConfigured, isPreview } = getConfiguredDemoSlugs() ... if (guardedOrgIds.size === 0) { - const isPreview = isRailwayPreviewEnvironment(env.RAILWAY_ENVIRONMENT_NAME) - if (!isExplicitlyConfigured || isPreview || import.meta.dev) return + if (isPreview || import.meta.dev) returnAlso applies to: 100-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/middleware/demo-guard.ts` around lines 28 - 47, getConfiguredDemoSlugs currently returns slugs and isExplicitlyConfigured but still calls isRailwayPreviewEnvironment again at the call site; change DemoSlugsResult to include an isPreview boolean, compute isPreview = isRailwayPreviewEnvironment(env.RAILWAY_ENVIRONMENT_NAME) inside getConfiguredDemoSlugs, set the default preview slug when isPreview is true, and return it; then update the caller to use result.isPreview instead of calling isRailwayPreviewEnvironment again and remove the now-redundant !isExplicitlyConfigured sub-condition (since if slugs were non-empty and not explicitly configured, isPreview will be true).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/middleware/demo-guard.ts`:
- Around line 28-47: getConfiguredDemoSlugs currently returns slugs and
isExplicitlyConfigured but still calls isRailwayPreviewEnvironment again at the
call site; change DemoSlugsResult to include an isPreview boolean, compute
isPreview = isRailwayPreviewEnvironment(env.RAILWAY_ENVIRONMENT_NAME) inside
getConfiguredDemoSlugs, set the default preview slug when isPreview is true, and
return it; then update the caller to use result.isPreview instead of calling
isRailwayPreviewEnvironment again and remove the now-redundant
!isExplicitlyConfigured sub-condition (since if slugs were non-empty and not
explicitly configured, isPreview will be true).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfileserver/middleware/demo-guard.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
9 tasks
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
1 participant
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.
Summary
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit
New Features
Documentation
Chores
Tests