clean up CI#3204
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCI switches from nx run-many to ChangesNx affected migration and library test configuration cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
[zzz-frontend] [Sat May 16 02:21:47 UTC 2026] - Deployed 6179ffc to https://genshin-optimizer-prs.github.io/pr/3204/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat May 16 02:21:58 UTC 2026] - Deployed 6179ffc to https://genshin-optimizer-prs.github.io/pr/3204/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat May 16 02:22:10 UTC 2026] - Deployed 6179ffc to https://genshin-optimizer-prs.github.io/pr/3204/sr-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat May 16 02:22:15 UTC 2026] - Deployed 6179ffc to https://genshin-optimizer-prs.github.io/pr/3204/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat May 16 02:22:38 UTC 2026] - Deployed 6179ffc to https://genshin-optimizer-prs.github.io/pr/3204/frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Sat May 16 15:07:20 UTC 2026] - Deployed 28afbdc to https://genshin-optimizer-prs.github.io/pr/3204/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat May 16 15:07:39 UTC 2026] - Deployed 28afbdc to https://genshin-optimizer-prs.github.io/pr/3204/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Sat May 16 15:09:45 UTC 2026] - Deployed 28afbdc to https://genshin-optimizer-prs.github.io/pr/3204/sr-frontend (Takes 3-5 minutes after this completes to be available) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
29-31: ⚡ Quick winGate
Fetch masterto PRs only to keep push CI faster.For push-to-master,
NX_BASE/NX_HEADalready come from the push payload, so this fetch is extra stamina drain.Suggested tweak
- name: Fetch master + if: ${{ github.event_name == 'pull_request' }} run: |Also applies to: 51-53, 98-100
🤖 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 @.github/workflows/ci.yml around lines 29 - 31, The "Fetch master" step is running on all events and should be gated to PRs only to avoid extra fetches on push; update the CI step with a conditional so it only runs when github.event_name == 'pull_request' (add an if: condition on the step named "Fetch master" that wraps the git fetch command), and apply the same change to the two other identical "Fetch master" occurrences referenced at the other locations (the steps that run git fetch --no-tags --no-recurse-submodules --filter=blob:none origin +master:refs/heads/master).
18-19: ⚡ Quick winRefactor fetch refspec for clarity, but current approach works.
The concern about
origin/masterbeing missing doesn't manifest in practice—git automatically updates remote tracking branches even with the explicitrefs/heads/mastertarget. That said, the proposed change is still worth doing for code clarity: writing directly torefs/remotes/origin/mastermakes the intent crystal-clear and avoids relying on implicit git behavior.At 3 AM (every AM when you're hooked on gacha), clarity is king. Future-you (and future maintainers) will appreciate not having to reverse-engineer whether git auto-updates the remote tracking branch or not.
- git fetch --no-tags --no-recurse-submodules --filter=blob:none origin +master:refs/heads/master + git fetch --no-tags --no-recurse-submodules --filter=blob:none origin +master:refs/remotes/origin/masterAlso applies to: 53, 100
🤖 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 @.github/workflows/ci.yml around lines 18 - 19, Replace the implicit "origin/master" refspec with the explicit remote-tracking ref "refs/remotes/origin/master" for clarity: update the NX_BASE default (and the other two analogous occurrences mentioned) so they use "refs/remotes/origin/master" instead of "origin/master" (leave NX_HEAD logic unchanged); this makes the intent explicit and avoids relying on git's implicit updating of remote-tracking branches.
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 29-31: The "Fetch master" step is running on all events and should
be gated to PRs only to avoid extra fetches on push; update the CI step with a
conditional so it only runs when github.event_name == 'pull_request' (add an if:
condition on the step named "Fetch master" that wraps the git fetch command),
and apply the same change to the two other identical "Fetch master" occurrences
referenced at the other locations (the steps that run git fetch --no-tags
--no-recurse-submodules --filter=blob:none origin +master:refs/heads/master).
- Around line 18-19: Replace the implicit "origin/master" refspec with the
explicit remote-tracking ref "refs/remotes/origin/master" for clarity: update
the NX_BASE default (and the other two analogous occurrences mentioned) so they
use "refs/remotes/origin/master" instead of "origin/master" (leave NX_HEAD logic
unchanged); this makes the intent explicit and avoids relying on git's implicit
updating of remote-tracking branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2739c93c-0d36-4a24-8d74-934aed1a22e8
📒 Files selected for processing (9)
.github/workflows/ci.ymllibs/gi/dm-localization/jest.config.tslibs/gi/dm-localization/project.jsonlibs/gi/dm-localization/src/executors/gen-locale/executor.spec.tslibs/gi/dm-localization/tsconfig.jsonlibs/gi/dm-localization/tsconfig.lib.jsonlibs/gi/dm-localization/tsconfig.spec.jsonlibs/sr/assets-data/src/executors/gen-assets-data/executor.spec.tslibs/sr/assets-data/vite.config.ts
💤 Files with no reviewable changes (5)
- libs/gi/dm-localization/tsconfig.spec.json
- libs/gi/dm-localization/src/executors/gen-locale/executor.spec.ts
- libs/gi/dm-localization/jest.config.ts
- libs/sr/assets-data/src/executors/gen-assets-data/executor.spec.ts
- libs/gi/dm-localization/tsconfig.json
| env: | ||
| NX_DAEMON: false | ||
| # PR: diff vs refs/remotes/origin/master | ||
| # Push to master: event.before→sha |
There was a problem hiding this comment.
Does this work properly when you push to a PR? Is the event_name gonna be push or pull_request?
| env: | ||
| NX_DAEMON: false | ||
| # PR: diff vs refs/remotes/origin/master | ||
| # Push to master: event.before→sha |
There was a problem hiding this comment.
Also, why not just let master run the entire suite of tests + lint + etc, we are not going to be blocked by waiting for that to finish in 99.999% of cases I can think of, and it can be helpful to get full coverage for our limited testing
2 participants
Describe your changes
This should speed up CI on PRs.
Cleanup the unit test and format CI to only run on
nx affected.Remove submodule dependency for
teststep.Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
Chores
Tests