Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe release workflow was consolidated into Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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
| check-version: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| is-prerelease: ${{ steps.version.outputs.prerelease }} | ||
| version: ${{ steps.version.outputs.current }} | ||
| steps: |
There was a problem hiding this comment.
Lock down check-version's token scope.
Line 10 starts a job without an explicit permissions block, so it inherits the repository-default GITHUB_TOKEN scope even though this job only reads repository contents.
Suggested fix
check-version:
+ permissions:
+ contents: read
runs-on: ubuntu-latest📝 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.
| check-version: | |
| runs-on: ubuntu-latest | |
| outputs: | |
| is-prerelease: ${{ steps.version.outputs.prerelease }} | |
| version: ${{ steps.version.outputs.current }} | |
| steps: | |
| check-version: | |
| permissions: | |
| contents: read | |
| runs-on: ubuntu-latest | |
| outputs: | |
| is-prerelease: ${{ steps.version.outputs.prerelease }} | |
| version: ${{ steps.version.outputs.current }} | |
| steps: |
🤖 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 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).
| # Function to compare semver versions | ||
| version_compare() { | ||
| # Strip prerelease suffix for base comparison | ||
| local v1_base="${1%%-*}" | ||
| local v2_base="${2%%-*}" | ||
| local v1_pre="${1#*-}" | ||
| local v2_pre="${2#*-}" | ||
|
|
||
| # If no prerelease, set to empty | ||
| [[ "$v1_base" == "$1" ]] && v1_pre="" | ||
| [[ "$v2_base" == "$2" ]] && v2_pre="" | ||
|
|
||
| # Compare major.minor.patch | ||
| IFS='.' read -ra V1 <<< "$v1_base" | ||
| IFS='.' read -ra V2 <<< "$v2_base" | ||
|
|
||
| for i in 0 1 2; do | ||
| local n1="${V1[$i]:-0}" | ||
| local n2="${V2[$i]:-0}" | ||
| if (( n1 > n2 )); then | ||
| echo "greater" | ||
| return | ||
| elif (( n1 < n2 )); then | ||
| echo "lesser" | ||
| return | ||
| fi | ||
| done | ||
|
|
||
| # Base versions equal, compare prerelease | ||
| # No prerelease > prerelease (1.0.0 > 1.0.0-beta) | ||
| if [[ -z "$v1_pre" && -n "$v2_pre" ]]; then | ||
| echo "greater" | ||
| elif [[ -n "$v1_pre" && -z "$v2_pre" ]]; then | ||
| echo "lesser" | ||
| elif [[ "$v1_pre" > "$v2_pre" ]]; then | ||
| echo "greater" | ||
| elif [[ "$v1_pre" < "$v2_pre" ]]; then | ||
| echo "lesser" | ||
| else | ||
| echo "equal" | ||
| fi |
There was a problem hiding this comment.
The prerelease comparison here is not semver-correct.
Lines 94-97 use plain string ordering, so versions like 1.2.3-beta.2 sort after 1.2.3-beta.10. That lets this gate reject valid bumps and permit downgrades. Please replace the hand-rolled comparison with a semver-aware implementation.
🤖 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 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.
| - name: Run Prod build | ||
| run: make build | ||
| env: | ||
| HUSKY: "0" | ||
| VITE_RUN_ENVIRONMENT: prod | ||
| RunEnvironment: prod | ||
| VITE_BUILD_HASH: ${{ env.VITE_BUILD_HASH }} | ||
| VITE_BUILD_HASH: ${{ needs.check-version.outputs.version }} |
There was a problem hiding this comment.
VITE_BUILD_HASH changed shape without updating all consumers.
Line 181 now passes a semantic version instead of a commit hash. That works for the Swagger/client generation paths, but src/ui/util/revision.ts:1-5 still truncates this env var to 7 characters as if it were a SHA, so prereleases and longer versions will render incorrectly. Either keep the hash here or update that consumer in the same change.
🤖 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 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.
| - name: Create Release | ||
| uses: actions/github-script@v8 | ||
| with: | ||
| github-token: ${{ steps.app-token.outputs.token }} | ||
| script: | | ||
| const version = '${{ needs.check-version.outputs.version }}'; | ||
| const isPrerelease = ${{ needs.check-version.outputs.is-prerelease }}; | ||
| const tagName = `v${version}`; | ||
|
|
||
| const release = await github.rest.repos.createRelease({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| tag_name: tagName, | ||
| name: tagName, | ||
| target_commitish: context.sha, | ||
| generate_release_notes: true, | ||
| prerelease: isPrerelease | ||
| }); |
There was a problem hiding this comment.
Make release creation idempotent before relying on reruns.
This step performs a non-idempotent write. If GitHub creates the release/tag and the step later fails or times out, rerunning the workflow will hit already_exists and stop instead of recovering—the exact case this PR is trying to handle.
Suggested fix
- name: Create Release
uses: actions/github-script@v8
with:
github-token: ${{ steps.app-token.outputs.token }}
script: |
const version = '${{ needs.check-version.outputs.version }}';
const isPrerelease = ${{ needs.check-version.outputs.is-prerelease }};
const tagName = `v${version}`;
+
+ try {
+ const existing = await github.rest.repos.getReleaseByTag({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ tag: tagName,
+ });
+ console.log(`Release already exists: ${existing.data.html_url}`);
+ return;
+ } catch (error) {
+ if (error.status !== 404) throw error;
+ }
const release = await github.rest.repos.createRelease({
owner: context.repo.owner,
repo: context.repo.repo,
tag_name: tagName,🤖 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 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.
1 participant
Light testing in https://github.com/acm-uiuc/core/tree/ashleyl7/test-workflow-branch
Summary by CodeRabbit