feat: add arsenal - #268
Closed
LiquidatorCoder wants to merge 21 commits into
Closed
Conversation
codenameakshay
previously approved these changes
Feb 18, 2026
📝 WalkthroughWalkthroughThis PR adds golden snapshot testing to the CI pipeline, introduces new Make targets for hook installation and golden verification, adds an arsenal dependency, and refines test file discovery to exclude certain test directories. ChangesGolden Snapshot Testing Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 6/8 reviews remaining, refill in 13 minutes and 29 seconds.Comment |
LiquidatorCoder
dismissed
codenameakshay’s stale review
May 2, 2026 07:01
The base branch was changed.
Android App Size ReportSkipped: no app-size relevant files changed in this PR. |
![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
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
73-101: ⚡ Quick win
golden-testswastes expensive macOS minutes: add a path filter,needs, and draft guard.Three quick wins:
- Path filter — macOS runners are ~10× the cost of Ubuntu. Without a filter, any documentation or CI-only commit spins up the full Flutter test suite on macOS.
needs: [ci]— ifformat-checkoranalyzefails in the cheapercijob, the macOS runner still starts and runs for its full duration.- Draft guard —
app_sizealready gates ongithub.event.pull_request.draft == false;golden-testsshould do the same.♻️ Proposed changes
golden-tests: + if: github.event.pull_request.draft == false + needs: [ci] runs-on: macos-latest + # Only run when Arsenal source, tests, or golden files change steps: + - name: Detect Arsenal-relevant changes + id: changes + uses: dorny/paths-filter@v3 + with: + filters: | + arsenal: + - 'lib/core/arsenal/**' + - 'test/core/arsenal/**' + - 'pubspec.yaml' + - 'pubspec.lock' - name: Checkout code + if: steps.changes.outputs.arsenal == 'true' uses: actions/checkout@v4 ... - name: Verify golden snapshots + if: steps.changes.outputs.arsenal == 'true' run: make verify-goldens CI=trueAlso note that golden PNG snapshots committed on a non-macOS machine may not match
macos-latestrenders (font rasterization differences). Consider documenting that goldens must always be regenerated on macOS to keep CI green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 73 - 101, The golden-tests job currently runs on every push and wastes macOS minutes; update the job named golden-tests to (1) add a paths filter so the workflow only triggers this job for relevant files (e.g., UI, lib/, assets/, test/, and golden images) using the workflow-level or job-level path include, (2) add needs: [ci] so golden-tests only runs after the cheaper CI job succeeds, and (3) add the same draft guard used by app_size (if: github.event.pull_request.draft == false) so the macOS job is skipped for draft PRs; keep the runner as macos-latest and ensure the job still performs the existing steps (Checkout, Setup Flutter, Install deps, Generate Firebase stub, Verify golden snapshots).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 273-276: The install-hooks Makefile target currently
unconditionally overwrites .git/hooks/pre-commit; update the install-hooks
recipe to first check if .git/hooks/pre-commit exists and if so do not clobber
it (either skip installation and echo a warning or back up the existing hook
before copying), then only copy tool/pre_commit_goldens.sh to
.git/hooks/pre-commit and chmod +x when safe. Update the lines that reference
the install-hooks target and the cp/chmod commands so they perform a test -e
".git/hooks/pre-commit" (or similar shell conditional) and handle the existing
file (skip or mv to .git/hooks/pre-commit.bak) instead of blindly replacing it.
In `@pubspec.yaml`:
- Around line 26-27: The pubspec dependency entry "arsenal: path: ../arsenal"
will break CI and local users because actions/checkout clones only this repo and
the sibling ../arsenal won't exist; fix by replacing the relative sibling path
with one of the recommended options: preferably move the Arsenal package into
the monorepo (e.g., add it under packages/arsenal and update the pubspec
"arsenal" entry to use path: packages/arsenal), or alternatively add Arsenal as
a git submodule at ../arsenal and enable submodules in actions/checkout, or add
an extra checkout step in CI to clone the arsenal repo before running make get,
or publish Arsenal and switch the pubspec to a version constraint; update the
"arsenal" dependency in pubspec.yaml accordingly and ensure CI workflows
(actions/checkout) reflect the chosen approach.
In `@tool/pre_commit_goldens.sh`:
- Line 7: The script currently uses strict mode (set -euo pipefail) but calls
fvm unguarded, which will abort commits for devs without fvm; update the
pre-commit script to detect whether fvm exists (e.g., using command -v or which)
and set a FLUTTER_CMD variable to either "fvm flutter" when fvm is available or
"flutter" when not, emit a benign warning when falling back, and replace direct
invocations of fvm flutter (the symbol "fvm" and any direct "fvm flutter" usages
around the fvm invocation block) with the FLUTTER_CMD variable so the script
degrades gracefully while keeping strict mode enabled.
In `@tool/update_goldens.sh`:
- Around line 36-48: The golden-update scripts reference missing test files and
an incorrect mapping: update_goldens.sh and pre_commit_goldens.sh map
lib/core/arsenal/components/ar_bottom_sheet.dart to ar_scaffold_test.dart and
expect many tests under test/core/arsenal/ that don't exist; either restore the
moved Arsenal test package or create the missing test directory and files. Fix
by adding test/core/arsenal/ with the expected test files (e.g.,
ar_scaffold_test.dart, ar_button_test.dart, ar_progress_steps_test.dart,
ar_chip_test.dart, ar_avatar_test.dart, ar_tag_test.dart,
ar_bottom_sheet_test.dart, ar_app_bar_test.dart, ar_bottom_nav_test.dart and
example tests ar_example_operator_profile_test.dart,
ar_example_mission_control_test.dart) or, if tests live elsewhere, update
update_goldens.sh and pre_commit_goldens.sh to point to the correct test paths
and correct the mapping for ar_bottom_sheet.dart to ar_bottom_sheet_test.dart;
ensure file names exactly match the script patterns used in the case statement
so golden workflows run.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 73-101: The golden-tests job currently runs on every push and
wastes macOS minutes; update the job named golden-tests to (1) add a paths
filter so the workflow only triggers this job for relevant files (e.g., UI,
lib/, assets/, test/, and golden images) using the workflow-level or job-level
path include, (2) add needs: [ci] so golden-tests only runs after the cheaper CI
job succeeds, and (3) add the same draft guard used by app_size (if:
github.event.pull_request.draft == false) so the macOS job is skipped for draft
PRs; keep the runner as macos-latest and ensure the job still performs the
existing steps (Checkout, Setup Flutter, Install deps, Generate Firebase stub,
Verify golden snapshots).
🪄 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: e87a01d2-5506-4bbd-b505-5b005a987124
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.claude/commands/arsenal-flow.md.github/workflows/ci.ymlMakefilepubspec.yamltool/pre_commit_goldens.shtool/update_goldens.sh
Comment on lines
+273
to
+276
| install-hooks: ## Install git hooks (run once after cloning) | ||
| @cp tool/pre_commit_goldens.sh .git/hooks/pre-commit | ||
| @chmod +x .git/hooks/pre-commit | ||
| @echo "Git pre-commit hook installed." |
There was a problem hiding this comment.
install-hooks silently clobbers any existing pre-commit hook.
Using bare cp overwrites .git/hooks/pre-commit without checking if one already exists, permanently destroying a developer's custom hook on make setup.
🛡️ Proposed fix: guard before copying
install-hooks: ## Install git hooks (run once after cloning)
+ `@if` [ -f .git/hooks/pre-commit ] && ! diff -q tool/pre_commit_goldens.sh .git/hooks/pre-commit >/dev/null 2>&1; then \
+ echo "Warning: existing .git/hooks/pre-commit differs — backing up to .git/hooks/pre-commit.bak"; \
+ cp .git/hooks/pre-commit .git/hooks/pre-commit.bak; \
+ fi
`@cp` tool/pre_commit_goldens.sh .git/hooks/pre-commit
`@chmod` +x .git/hooks/pre-commit
`@echo` "Git pre-commit hook installed."📝 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
| install-hooks: ## Install git hooks (run once after cloning) | |
| @cp tool/pre_commit_goldens.sh .git/hooks/pre-commit | |
| @chmod +x .git/hooks/pre-commit | |
| @echo "Git pre-commit hook installed." | |
| install-hooks: ## Install git hooks (run once after cloning) | |
| `@if` [ -f .git/hooks/pre-commit ] && ! diff -q tool/pre_commit_goldens.sh .git/hooks/pre-commit >/dev/null 2>&1; then \ | |
| echo "Warning: existing .git/hooks/pre-commit differs — backing up to .git/hooks/pre-commit.bak"; \ | |
| cp .git/hooks/pre-commit .git/hooks/pre-commit.bak; \ | |
| fi | |
| `@cp` tool/pre_commit_goldens.sh .git/hooks/pre-commit | |
| `@chmod` +x .git/hooks/pre-commit | |
| `@echo` "Git pre-commit hook installed." |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 273 - 276, The install-hooks Makefile target currently
unconditionally overwrites .git/hooks/pre-commit; update the install-hooks
recipe to first check if .git/hooks/pre-commit exists and if so do not clobber
it (either skip installation and echo a warning or back up the existing hook
before copying), then only copy tool/pre_commit_goldens.sh to
.git/hooks/pre-commit and chmod +x when safe. Update the lines that reference
the install-hooks target and the cp/chmod commands so they perform a test -e
".git/hooks/pre-commit" (or similar shell conditional) and handle the existing
file (skip or mv to .git/hooks/pre-commit.bak) instead of blindly replacing it.
| # If any staged Arsenal source files have changed, regenerate the affected | ||
| # golden PNGs and stage them so they're included in the commit automatically. | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
Missing fvm guard will hard-block commits for developers without fvm installed.
With set -euo pipefail in effect, if fvm is absent the shell will exit non-zero at Line 65, aborting the commit entirely — with no clear message. This will surprise contributors who don't use fvm locally (e.g., those who invoke flutter directly).
🛡️ Proposed fix: check for fvm and degrade gracefully
+# Gracefully skip if fvm is unavailable (contributors not using fvm).
+if ! command -v fvm >/dev/null 2>&1; then
+ echo "Warning: fvm not found — skipping golden regeneration. Run 'make update-goldens' manually."
+ exit 0
+fi
+
run_tests_and_stage() {
fvm flutter test "$@" --update-goldens
git add "$GOLDENS_DIR"
}Also applies to: 64-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tool/pre_commit_goldens.sh` at line 7, The script currently uses strict mode
(set -euo pipefail) but calls fvm unguarded, which will abort commits for devs
without fvm; update the pre-commit script to detect whether fvm exists (e.g.,
using command -v or which) and set a FLUTTER_CMD variable to either "fvm
flutter" when fvm is available or "flutter" when not, emit a benign warning when
falling back, and replace direct invocations of fvm flutter (the symbol "fvm"
and any direct "fvm flutter" usages around the fvm invocation block) with the
FLUTTER_CMD variable so the script degrades gracefully while keeping strict mode
enabled.
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
2 participants
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 by CodeRabbit