Merged
Conversation
📝 WalkthroughWalkthroughAdds reusable GitHub Actions workflows (setup, deploy, PR checks); refactors artifact payload helpers into a new module; moves Prisma datasource URL into Changes
Sequence DiagramsequenceDiagram
participant GH as "GitHub Actions"
participant Setup as "setup.yml"
participant GHCR as "GHCR (images)"
participant Deploy as "deploy.yml"
participant AWS as "AWS (ECR / ECS)"
GH->>Setup: call setup workflow (checkout, cache, build/test)
GH->>GHCR: build & push images (engine + otel)
GH->>Deploy: call deploy workflow with BUILD_NUMBER, DOCKER_TAG, ECS_CLUSTER, secrets
Deploy->>AWS: configure credentials & login to ECR
Deploy->>AWS: pull GHCR images, tag to ECR, push tags (DOCKER_TAG + BUILD_NUMBER)
Deploy->>AWS: conditional ECS update (if ECS_CLUSTER != 'none')
AWS-->>Deploy: push/deploy result
Deploy-->>GH: workflow result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/lib/server/models/artifact-handle.ts (1)
20-43: Extract repeatedartifact_filesappend logic into a helper.Both branches duplicate the same string-append block; a tiny helper will reduce drift risk when behavior changes later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/models/artifact-handle.ts` around lines 20 - 43, The artifact_files string-append logic is duplicated in handleArtifact; create a small helper (e.g., appendArtifactFile or addArtifactFile) that accepts the ProviderForArtifacts instance and the artifact name and performs the comma-joined append to provider.artifact_files, then replace both duplicated blocks in handleArtifact (the branches using Build.artifactType and Release.artifactType) with calls to that helper; keep the existing version_code assignment logic for Build.Artifact.VersionCode in handleArtifact unchanged.
🤖 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/deploy.yml:
- Around line 52-57: The workflow is appending ".amazonaws.com" to the ECR
registry output causing duplicated domains; update the "Build deployment target
variables" step (id: vars) to use steps.ecr.outputs.registry directly when
setting IMAGE_URL and OTEL_URL (i.e., remove the appended ".amazonaws.com"
suffix) so IMAGE_URL and OTEL_URL become "${{ steps.ecr.outputs.registry
}}/appbuilder-buildengine-api-origin" and "${{ steps.ecr.outputs.registry
}}/appbuilder-buildengine-api-otel-collector" respectively.
In @.github/workflows/main.yml:
- Around line 40-47: The OTEL build step ("Build OTEL Docker image") uses the
wrong tag variable; change its with.tags from ${{ env.BUILD_ENGINE_BUILD_TAG }}
to ${{ env.OTEL_BUILD_TAG }} (or add OTEL_BUILD_TAG alongside existing tags) so
the later deploy workflow can find and re-tag the local OTEL image via
inputs.OTEL_BUILD_TAG; update the step that uses docker/build-push-action@v4 to
reference the OTEL_BUILD_TAG environment variable (or include both tags)
accordingly.
- Around line 71-82: The step-level "uses: ./ .github/workflows/deploy.yml" (the
"name: Deploy SIL" step) is invalid for reusable workflows and will fail; move
this reusable-workflow invocation to a job-level call (jobs.<job_id>.uses)
instead of a step, and convert the current step into either inputs to that job
or a wrapper job that calls the reusable workflow. Also ensure local Docker
images referenced by BUILD_ENGINE_BUILD_TAG and OTEL_BUILD_TAG are preserved
when calling the reusable workflow: either push those images to a registry
before the reusable workflow runs, or export/import them via docker save ->
upload as artifact (or use cache) and docker load inside the called workflow;
alternatively keep the ECR push in this job/composite action rather than relying
on the fresh runner. Apply the same changes to the other occurrences that use
"./.github/workflows/deploy.yml" (the blocks around the other noted uses).
In @.github/workflows/pr.yml:
- Around line 3-7: The workflow's pull_request trigger only lists branches
"master" and "develop" under the on: pull_request -> branches array, so PRs
targeting feature/svelte won't run; update the branches array to include
"feature/svelte" (or remove the branches filter entirely if you want the
workflow to run for all target branches) in the on: pull_request section to
ensure PR `#78` triggers the workflow.
In @.github/workflows/setup.yml:
- Around line 120-124: The fixed sleep calls that race PostgreSQL and the app
(around the ./run ci up -d db + npx prisma migrate reset and the later smoke
test) should be replaced with readiness checks: after running ./run ci up -d db,
poll PostgreSQL (use pg_isready or a small psql loop retrying until success with
a timeout) before invoking npx prisma migrate reset; similarly, before running
the smoke test replace the sleep with an HTTP health-check/retry loop against
the app's health endpoint (or use docker container-ready checks) with a sensible
timeout and retry interval; update both places mentioned (the db startup block
and the app-smoke-test block) to use these readiness loops instead of sleep.
- Around line 24-37: The "Get environment info" step (id: set-vars) currently
runs and prints the full environment via the env command, which can leak
secrets; remove the env invocation and any related "echo Environment variables"
lines from the run block while keeping the harmless diagnostics (node/npm/docker
versions) and the BACKUP_AUTH0_SECRET generation and echo to $GITHUB_OUTPUT
intact; update the run script inside the step so it no longer executes env or
prints all environment variables.
In `@src/lib/server/models/artifact-handle.ts`:
- Around line 30-32: When handling Build.Artifact.VersionCode in the artifact
parsing logic, guard the assignment to provider.version_code by validating the
contents returned from readS3File() (or the local default) before converting:
ensure contents is a non-empty string and represents a valid integer (use
parseInt/Number and check !isNaN and isFinite or a /^\s*-?\d+\s*$/ test), then
assign the parsed numeric value to provider.version_code; if validation fails,
do not assign (or throw/return an error as appropriate) so you avoid silently
writing 0/NaN from empty or non-numeric contents.
---
Nitpick comments:
In `@src/lib/server/models/artifact-handle.ts`:
- Around line 20-43: The artifact_files string-append logic is duplicated in
handleArtifact; create a small helper (e.g., appendArtifactFile or
addArtifactFile) that accepts the ProviderForArtifacts instance and the artifact
name and performs the comma-joined append to provider.artifact_files, then
replace both duplicated blocks in handleArtifact (the branches using
Build.artifactType and Release.artifactType) with calls to that helper; keep the
existing version_code assignment logic for Build.Artifact.VersionCode in
handleArtifact unchanged.
🪄 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: bd71e1f3-436f-4729-a958-2035b32ca0c5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/deploy.yml.github/workflows/main.yml.github/workflows/pr.yml.github/workflows/setup.ymlfixSourcemaps.ctspackage.jsonprisma.config.tssrc/lib/prisma/schema.prismasrc/lib/server/aws/s3.tssrc/lib/server/job-executors/release.tssrc/lib/server/models/artifact-handle.tssrc/lib/server/models/artifacts.ts
💤 Files with no reviewable changes (1)
- src/lib/server/models/artifacts.ts
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/server/models/artifact-handle.ts (1)
30-33:⚠️ Potential issue | 🟠 MajorUse one strict integer parse path for
version_code.Line 30 validates with
parseInt, but Line 33 assignsNumber(contents). A value like"123abc"passes the guard and then writesNaN. Parse once (trimmed, base-10), validate, and assign that parsed value.💡 Proposed fix
- const code = (contents && parseInt(contents)) || NaN; - - if (type === Build.Artifact.VersionCode && !isNaN(code)) { - provider.version_code = Number(contents); + const parsedVersionCode = Number.parseInt(contents.trim(), 10); + if (type === Build.Artifact.VersionCode && Number.isFinite(parsedVersionCode)) { + provider.version_code = parsedVersionCode; }#!/bin/bash # Quick behavior check for the current guard/assignment mismatch (no repo files required). node - <<'NODE' const samples = ["123", "123abc", "", " 42 ", "0x10"]; for (const s of samples) { const guarded = (s && parseInt(s)) || NaN; const assigned = Number(s); console.log({ input: s, guardPasses: !Number.isNaN(guarded), assigned }); } NODE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/models/artifact-handle.ts` around lines 30 - 33, The guard uses parseInt while the assignment uses Number(contents), causing mismatches (e.g., "123abc"); instead, parse once with a trimmed, base-10 integer parse and reuse that value: in the block that computes code and checks if type === Build.Artifact.VersionCode, replace the current two-step approach by parsing contents once with parseInt(contents.trim(), 10) (or equivalent) into the local variable (currently named code), verify !Number.isNaN(code), and assign provider.version_code = code so the validated parsed integer is what gets stored.
🧹 Nitpick comments (2)
src/lib/server/models/artifact-handle.ts (1)
24-28: Extract repeatedartifact_filesappend logic into a helper.The same CSV-append block appears in both branches. Pulling this into one helper reduces duplication and drift risk.
♻️ Suggested refactor
+function appendArtifactFile(provider: ProviderForArtifacts, name: string) { + provider.artifact_files = provider.artifact_files + ? `${provider.artifact_files},${name}` + : name; +} + export function handleArtifact(provider: ProviderForArtifacts, key: string, contents: string = '') { if ('version_code' in provider) { const [type, name] = Build.artifactType(key); if (type !== Build.Artifact.Unknown) { - if (provider.artifact_files) { - provider.artifact_files += ',' + name; - } else { - provider.artifact_files = name; - } + appendArtifactFile(provider, name); @@ } else { const [type, name] = Release.artifactType(key); if (type !== Release.Artifact.Unknown) { - if (provider.artifact_files) { - provider.artifact_files += ',' + name; - } else { - provider.artifact_files = name; - } + appendArtifactFile(provider, name); } } }Also applies to: 39-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/models/artifact-handle.ts` around lines 24 - 28, Extract the repeated CSV-append logic for provider.artifact_files into a small helper (e.g., appendCsvField(provider, 'artifact_files', name)) and replace both inline blocks with a single call; the helper should handle undefined/empty values and prepend a comma only when a value already exists, ensuring it updates provider.artifact_files consistently wherever used..github/workflows/main.yml (1)
63-68: Consider updatingdocker/login-actionto v3.The action is at v2, but v3 is the current stable version with improvements. This is a minor update for consistency with modern action versions.
🔧 Proposed fix
- name: Login to GitHub Container Registry - uses: docker/login-action@v2 + uses: docker/login-action@v3 with: registry: ghcr.io🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/main.yml around lines 63 - 68, Update the GitHub Actions step named "Login to GitHub Container Registry" to use the newer docker/login-action@v3 instead of docker/login-action@v2; locate the step that uses docker/login-action@v2 and change the version tag to `@v3` while keeping the existing inputs (registry, username, password) unchanged to preserve behavior.
🤖 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/main.yml:
- Around line 37-52: The workflow uses docker/build-push-action@v4 in the "Build
Docker image" and "Build OTEL Docker image" steps; update both action references
to the current stable release (docker/build-push-action@v7 or `@v7.0.0`) to keep
the action up-to-date and backward compatible, confirm the existing inputs
(push, tags, context, load, file) remain valid under v7 and adjust if necessary
per the v7 action docs (e.g., migration notes), then run CI to verify the builds
still behave as expected.
---
Duplicate comments:
In `@src/lib/server/models/artifact-handle.ts`:
- Around line 30-33: The guard uses parseInt while the assignment uses
Number(contents), causing mismatches (e.g., "123abc"); instead, parse once with
a trimmed, base-10 integer parse and reuse that value: in the block that
computes code and checks if type === Build.Artifact.VersionCode, replace the
current two-step approach by parsing contents once with
parseInt(contents.trim(), 10) (or equivalent) into the local variable (currently
named code), verify !Number.isNaN(code), and assign provider.version_code = code
so the validated parsed integer is what gets stored.
---
Nitpick comments:
In @.github/workflows/main.yml:
- Around line 63-68: Update the GitHub Actions step named "Login to GitHub
Container Registry" to use the newer docker/login-action@v3 instead of
docker/login-action@v2; locate the step that uses docker/login-action@v2 and
change the version tag to `@v3` while keeping the existing inputs (registry,
username, password) unchanged to preserve behavior.
In `@src/lib/server/models/artifact-handle.ts`:
- Around line 24-28: Extract the repeated CSV-append logic for
provider.artifact_files into a small helper (e.g., appendCsvField(provider,
'artifact_files', name)) and replace both inline blocks with a single call; the
helper should handle undefined/empty values and prepend a comma only when a
value already exists, ensuring it updates provider.artifact_files consistently
wherever used.
🪄 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: bd7f7dc3-555a-4a08-a9ab-0f6e4f061108
📒 Files selected for processing (4)
.github/workflows/deploy.yml.github/workflows/main.yml.github/workflows/setup.ymlsrc/lib/server/models/artifact-handle.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/deploy.yml
- .github/workflows/setup.yml
These jobs should be skipped due to needs property
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/main.yml (1)
53-58:⚠️ Potential issue | 🟡 MinorUpdate
docker/login-actionto v4.The action is pinned to
v2, but v4.0.0 is the latest available version (released 2026-03-04). Updating will provide security updates and improved runner compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/main.yml around lines 53 - 58, Update the GitHub Actions step titled "Login to GitHub Container Registry" to use docker/login-action@v4 (e.g., v4.0.0) instead of docker/login-action@v2; modify the uses field in that step so the workflow uses the v4 tag to pick up security fixes and improved runner compatibility while keeping the existing with: keys (registry, username, password) unchanged.
🧹 Nitpick comments (1)
.github/workflows/main.yml (1)
82-85: Consider alternative toif: falsefor disabled job.The static analysis flags
if: falseas a constant expression. While this pattern is valid for temporarily disabling a job, an alternative approach would be to use a repository variable or workflow input to control this:if: ${{ vars.ENABLE_FCBH_DEPLOY == 'true' && needs.push-to-ghcr.outputs.ECS_CLUSTER == 'scriptoria-prd' }}This provides an audit trail in repository settings when toggling deployments, though the current approach with the explanatory comment is also acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/main.yml around lines 82 - 85, Replace the hard-coded "if: false" used to disable the deploy-fcbh job with a conditional that references a repository variable or workflow input so toggles are auditable; update the job's condition (deploy-fcbh and its current if: false) to use something like a vars.ENABLE_FCBH_DEPLOY or a workflow input combined with the existing cluster check (i.e., require the repo var/input == 'true' && the existing needs.push-to-ghcr.outputs.ECS_CLUSTER == 'scriptoria-prd') and keep the explanatory comment about temporary disablement if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/main.yml:
- Around line 53-58: Update the GitHub Actions step titled "Login to GitHub
Container Registry" to use docker/login-action@v4 (e.g., v4.0.0) instead of
docker/login-action@v2; modify the uses field in that step so the workflow uses
the v4 tag to pick up security fixes and improved runner compatibility while
keeping the existing with: keys (registry, username, password) unchanged.
---
Nitpick comments:
In @.github/workflows/main.yml:
- Around line 82-85: Replace the hard-coded "if: false" used to disable the
deploy-fcbh job with a conditional that references a repository variable or
workflow input so toggles are auditable; update the job's condition (deploy-fcbh
and its current if: false) to use something like a vars.ENABLE_FCBH_DEPLOY or a
workflow input combined with the existing cluster check (i.e., require the repo
var/input == 'true' && the existing needs.push-to-ghcr.outputs.ECS_CLUSTER ==
'scriptoria-prd') and keep the explanatory comment about temporary disablement
if desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89b3ee34-3010-4537-8452-7b0606532462
📒 Files selected for processing (1)
.github/workflows/main.yml
FyreByrd
added a commit
that referenced
this pull request
Mar 26, 2026
* Initial draft of GHA * Move DB Url back to schema * Fix circular import in artifacts * Fix source maps * Add empty versions of needed env vars * Relocate empty env vars * Fix build output * DRY deploy * Add OTEL to deploy script * CR Feedback * Refactor Docker build * Remove unneeded if These jobs should be skipped due to needs property
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.
Changes:
Summary by CodeRabbit