Conversation
|
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:
WalkthroughThe deploy workflow was reworked: it now triggers on pushes to Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/deploy-prod.yml (1)
376-384:⚠️ Potential issue | 🟡 MinorInclude
check-versionin the notification fan-in.Line 377 omits the new gatekeeper job from
needs, so version-check failures never show up inneeds_jsonand the Discord alert path misses the real root cause.Suggested fix
- needs: [deploy-prod, publish-node-client, test-unit, build, test-e2e, make-release] + needs: [check-version, deploy-prod, publish-node-client, test-unit, build, test-e2e, make-release]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-prod.yml around lines 376 - 384, The notify job's needs array is missing the new gatekeeper job (check-version), so failures there aren't included in needs_json and Discord alerts can miss the true cause; update the notify job's needs list (the needs value used to build needs_json for the lacherogwu/failed-jobs-discord-notification-action) to include "check-version" alongside deploy-prod, publish-node-client, test-unit, build, test-e2e, and make-release so the notification fan-in captures check-version failures.
🤖 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-prod.yml:
- Around line 175-181: The CI changed VITE_BUILD_HASH to a semantic version but
src/ui/util/revision.ts still assumes a git SHA and truncates to 7 chars; update
the consumer instead of reverting CI: modify the logic in
src/ui/util/revision.ts (the function/constant that reads
process.env.VITE_BUILD_HASH) to stop unconditionally slicing to 7 characters and
instead return the full env value or apply conditional truncation only when the
value matches a SHA pattern (e.g., hex of length >=7); ensure prerelease/long
semver strings are returned intact and update any exported symbol names used by
the UI accordingly.
- Around line 60-100: The current version_compare function uses plain string
comparison for prerelease parts (v1_pre, v2_pre) which misorders identifiers
like beta.2 vs beta.10; update version_compare to perform semver-correct
prerelease comparison by splitting v1_pre and v2_pre on '.' and comparing each
identifier: numeric identifiers compare numerically, numeric < non-numeric,
non-numeric compare lexically in ASCII order, and longer identifier lists win
only if all shared identifiers are equal; alternatively replace the hand-rolled
logic in version_compare with a proven semver-aware comparator (e.g., use sort
-V, a small Python/Node helper using packaging.version/semver library, or embed
the algorithm above) so prerelease ordering is semver compliant.
- Around line 10-15: The check-version job currently lacks an explicit
permissions block and inherits the full repo GITHUB_TOKEN scope; update the job
definition for check-version to declare minimal permissions (e.g., permissions:
contents: read) so the GITHUB_TOKEN only has read access to repository contents
for the steps that read the version outputs (keep the outputs keys is-prerelease
and version unchanged).
- Around line 327-344: The Create Release step is non-idempotent because it
always calls github.rest.repos.createRelease (tagName) and will fail on reruns
if the release/tag already exists; change the script to first check for an
existing release by tag (use github.rest.repos.getReleaseByTag or list releases
and match tagName) and only call createRelease when none exists, or wrap
createRelease in a try/catch and on a 422/already_exists error call
getReleaseByTag and reuse/update that release instead of failing; update the
script around the variables tagName, isPrerelease and the createRelease call to
implement this logic so the workflow becomes safe to rerun.
---
Outside diff comments:
In @.github/workflows/deploy-prod.yml:
- Around line 376-384: The notify job's needs array is missing the new
gatekeeper job (check-version), so failures there aren't included in needs_json
and Discord alerts can miss the true cause; update the notify job's needs list
(the needs value used to build needs_json for the
lacherogwu/failed-jobs-discord-notification-action) to include "check-version"
alongside deploy-prod, publish-node-client, test-unit, build, test-e2e, and
make-release so the notification fan-in captures check-version failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d6f003b-3254-4c61-9f01-2eeaaa2fc343
📒 Files selected for processing (2)
.github/workflows/deploy-prod.yml.github/workflows/release.yml
💤 Files with no reviewable changes (1)
- .github/workflows/release.yml
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/deploy-prod.yml (1)
1-7:⚠️ Potential issue | 🔴 CriticalSerialize this workflow before relying on the latest-tag gate.
Because
check-versiononly compares against the latest existing tag, twomainpushes with the samepackage.jsonversion can both pass if the first run has not reachedmake-releaseyet. The later run can then deploy a different commit whilemake-releaseexits early becausev${version}already exists, leaving prod ahead of the tagged release.Suggested fix
name: Deploy all resources to PROD run-name: PROD deploy - @${{ github.actor }} +concurrency: + group: prod-release-${{ github.ref }} + cancel-in-progress: false + on: push: branches: - 'main'Also applies to: 31-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-prod.yml around lines 1 - 7, The workflow can race around the latest-tag gate; add a GitHub Actions concurrency block to serialize deploys so only one run can proceed at a time (preventing a later push from passing check-version before make-release finishes). Edit the deploy-prod workflow to add a top-level concurrency: set a stable group name (e.g., "deploy-prod-${{ github.ref }}" or a repo-wide "deploy-prod") and set cancel-in-progress: false so runs queue instead of running concurrently; keep references to the existing check-version and make-release steps unchanged but now protected by the serialized concurrency group.
🧹 Nitpick comments (1)
.github/workflows/deploy-prod.yml (1)
419-427: Includecheck-versionin the notifier context.
needs_jsonis built fromneeds, so the new gate job will never appear in the Discord payload as written. If version-gate failures are actionable, add it here too.Suggested fix
notify: permissions: {} - needs: [deploy-prod, publish-node-client, test-unit, build, test-e2e, make-release] + needs: [check-version, deploy-prod, publish-node-client, test-unit, build, test-e2e, make-release]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-prod.yml around lines 419 - 427, The notify job's needs array (in the notify job definition) is missing the version gate job, so its needs_json won't include that gate; update the notify job's needs (the needs array referenced in the notify job named "notify") to include "check-version" (or the exact version-gate job name used in your workflow) alongside deploy-prod, publish-node-client, test-unit, build, test-e2e, and make-release so failures from the version gate appear in the Discord payload.
🤖 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/deploy-prod.yml:
- Around line 1-7: The workflow can race around the latest-tag gate; add a
GitHub Actions concurrency block to serialize deploys so only one run can
proceed at a time (preventing a later push from passing check-version before
make-release finishes). Edit the deploy-prod workflow to add a top-level
concurrency: set a stable group name (e.g., "deploy-prod-${{ github.ref }}" or a
repo-wide "deploy-prod") and set cancel-in-progress: false so runs queue instead
of running concurrently; keep references to the existing check-version and
make-release steps unchanged but now protected by the serialized concurrency
group.
---
Nitpick comments:
In @.github/workflows/deploy-prod.yml:
- Around line 419-427: The notify job's needs array (in the notify job
definition) is missing the version gate job, so its needs_json won't include
that gate; update the notify job's needs (the needs array referenced in the
notify job named "notify") to include "check-version" (or the exact version-gate
job name used in your workflow) alongside deploy-prod, publish-node-client,
test-unit, build, test-e2e, and make-release so failures from the version gate
appear in the Discord payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d276943-c000-4f3e-9b0c-1f0b6bbdd808
📒 Files selected for processing (2)
.github/workflows/deploy-prod.ymlsrc/ui/util/revision.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/deploy-prod.yml (1)
184-185: Keep QA deployment gated on unit tests.This graph change removes the transitive
test-unitgate fromtest-e2e, so a unit-test failure will still build, deploy to QA, and consume the serialized QA slot. Either re-addtest-unithere, or add it directly totest-e2e.needsif you want to keep the build itself parallelized.♻️ Minimal change in this segment
needs: - check-version + - test-unit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-prod.yml around lines 184 - 185, The QA deployment workflow removed the transitive `test-unit` dependency causing QA to proceed even if unit tests fail; restore the gate by adding `test-unit` to the `needs:` list alongside `check-version` (or alternately add `test-unit` to the `needs` array of the `test-e2e` job) so that either the QA deployment job or the `test-e2e` job explicitly depends on `test-unit` and will be blocked when unit tests fail.
🤖 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-prod.yml:
- Around line 423-425: The notify job's fan-in is missing the check-version job,
so when check-version is the only failing job the Discord alert can't surface
the real root cause; update the notify job's needs list (the notify job block
referenced as "notify") to include check-version alongside deploy-prod,
publish-node-client, test-unit, build, test-e2e, and make-release so needs_json
will contain the failing check-version job and alerts report the correct root
cause.
---
Nitpick comments:
In @.github/workflows/deploy-prod.yml:
- Around line 184-185: The QA deployment workflow removed the transitive
`test-unit` dependency causing QA to proceed even if unit tests fail; restore
the gate by adding `test-unit` to the `needs:` list alongside `check-version`
(or alternately add `test-unit` to the `needs` array of the `test-e2e` job) so
that either the QA deployment job or the `test-e2e` job explicitly depends on
`test-unit` and will be blocked when unit tests fail.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d832ab46-6902-4b6f-9026-5a9f2669f3d1
📒 Files selected for processing (1)
.github/workflows/deploy-prod.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-prod.yml:
- Around line 41-44: The current logic uses git's --sort=version:refname to set
LATEST_TAG and PREVIOUS_VERSION which misorders prereleases vs releases; replace
that by enumerating all tags (the output feeding LATEST_TAG), strip the leading
"v" (as PREVIOUS_VERSION does), and then determine the highest semver using the
same semver comparator used elsewhere in this workflow (not git's sort).
Concretely: collect tags into a list instead of relying on git --sort, normalize
them by removing the "v" prefix (the same normalization used to set
PREVIOUS_VERSION), and pick the max with your semver comparator function so
LATEST_TAG/PREVIOUS_VERSION reflect true semver ordering (release > prerelease).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 172ffe93-043b-4c91-b930-63f520b1f983
📒 Files selected for processing (1)
.github/workflows/deploy-prod.yml
| # Get previous version | ||
| LATEST_TAG=$(git tag -l 'v*' --sort=-version:refname | head -n 1) | ||
| PREVIOUS_VERSION=${LATEST_TAG#v} | ||
| PREVIOUS_VERSION=${PREVIOUS_VERSION:-0.0.0} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
export HOME="$tmp/home"
mkdir -p "$HOME"
repo="$tmp/repo"
mkdir -p "$repo"
cd "$repo"
git init -q
git -c user.name=test -c user.email=test@example.com commit --allow-empty -m init -q
git tag v1.2.3
git tag v1.2.3-beta.10
git tag -l 'v*' --sort=-version:refnameRepository: acm-uiuc/core
Length of output: 157
Don't use Git's version sort for semver comparison.
git tag --sort=version:refname doesn't correctly order same-base prereleases vs. releases. Git sorts suffixes lexicographically by default, but SemVer requires release versions to outrank their prereleases. With tags v1.2.3 and v1.2.3-beta.10, Git's sort puts the prerelease first—so head -n 1 picks v1.2.3-beta.10 as the "latest" tag instead of v1.2.3. This breaks PREVIOUS_VERSION extraction and can allow downgrades past the later semver check (e.g., deploying 1.2.3-beta.11 when 1.2.3 already exists). Use the same semver comparator you implement below instead of relying on Git's sort.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy-prod.yml around lines 41 - 44, The current logic
uses git's --sort=version:refname to set LATEST_TAG and PREVIOUS_VERSION which
misorders prereleases vs releases; replace that by enumerating all tags (the
output feeding LATEST_TAG), strip the leading "v" (as PREVIOUS_VERSION does),
and then determine the highest semver using the same semver comparator used
elsewhere in this workflow (not git's sort). Concretely: collect tags into a
list instead of relying on git --sort, normalize them by removing the "v" prefix
(the same normalization used to set PREVIOUS_VERSION), and pick the max with
your semver comparator function so LATEST_TAG/PREVIOUS_VERSION reflect true
semver ordering (release > prerelease).
There was a problem hiding this comment.
Git's version:refname sorts by version number rather than lexicographically (--sort=refname)
There was a problem hiding this comment.
@ashleyyli, you're correct that --sort=version:refname performs numeric-aware version ordering (e.g., correctly placing 1.10.0 after 1.2.0), unlike --sort=refname which is purely lexicographic.
However, the concern about prerelease vs. release ordering still stands. Even when version sort is used in git-tag, tagnames with the same base version but different suffixes are still sorted lexicographically, resulting in prerelease tags appearing after the main release (e.g., "1.0-rc1" after "1.0"). This is the opposite of what SemVer requires — when major, minor, and patch are equal, a pre-release version has lower precedence than a normal version (e.g., 1.0.0-alpha < 1.0.0).
The practical impact: if both v1.2.3 and v1.2.3-beta.10 exist, --sort=-version:refname | head -n 1 returns v1.2.3-beta.10 (the prerelease sorts lexicographically higher due to the - suffix), not v1.2.3 (the true semver maximum). This would set PREVIOUS_VERSION to 1.2.3-beta.10, incorrectly blocking a deployment of 1.2.3.
The version:refname sort order can be affected by the versionsort.suffix configuration variable — and as shown in the Git source, you'd need to configure that with - as the suffix to make prereleases sort before their base release. Without it, the default behaviour misaligns with semver.
ashleyyli
changed the title
|
1 participant
Addresses #613
Light testing in https://github.com/acm-uiuc/core/tree/ashleyl7/test-workflow-branch
Summary by CodeRabbit