fix: avoid brand asset churn during dev launch - #378
Conversation
📝 WalkthroughWalkthroughThe PR refactors the brand icon generation workflow to support configurable output paths via CLI arguments ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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.
🧹 Nitpick comments (3)
scripts/test_generate_brand_icons.py (2)
22-27: Pre-conditiongit diff --quietwill spuriously fail the test when a developer has unrelated local edits to brand assets.This assertion treats any pre-existing dirty state on the tracked brand paths as a test failure, even when the developer legitimately has WIP changes to those files. That produces confusing red test runs that aren't about the code under test.
A more robust approach is to snapshot file hashes (or
git diffoutput) before/after and compare them for equality, which tests “the run didn’t change things” regardless of the starting state:Proposed refactor
- before = subprocess.run( - ["git", "diff", "--quiet", "--", *GENERATED_BRAND_PATHS], - cwd=REPO_ROOT, - check=False, - ) - self.assertEqual(before.returncode, 0, "generated brand assets must start clean for this test") + def snapshot() -> str: + return subprocess.run( + ["git", "diff", "--", *GENERATED_BRAND_PATHS], + cwd=REPO_ROOT, + check=True, + capture_output=True, + text=True, + ).stdout + + before_diff = snapshot() ... - self.assertEqual(after.returncode, 0, "temporary icns generation must not dirty tracked brand outputs") + self.assertEqual(snapshot(), before_diff, "temporary icns generation must not dirty tracked brand outputs")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_generate_brand_icons.py` around lines 22 - 27, The current pre-check uses subprocess.run([... "git", "diff", "--quiet", "--", *GENERATED_BRAND_PATHS], ...) and asserts before.returncode == 0, which fails if a developer has unrelated local edits; change this to record a snapshot of the tracked files’ state before the test (e.g., capture content hashes or the output of git diff -- <paths> into a string) and then after running the generation compare the before and after snapshots for equality instead of asserting a clean working tree; update the variable names around before and the assertEqual call (e.g., before_snapshot and after_snapshot) so the test verifies “no changes produced by this run” regardless of starting dirty state.
31-43: Usesys.executableinstead of a bare"python3".Invoking the generator via
"python3"picks up whateverpython3is first onPATH, which may not be the interpreter running the test (e.g. venv vs. system Python, or CI images wherepython3resolves to a different version than the unittest runner). Usingsys.executableguarantees the subprocess uses the same interpreter and avoids surprisingModuleNotFoundError: PILfailures when the active venv has Pillow but the systempython3does not.Proposed fix
+import sys ... - result = subprocess.run( - [ - "python3", - str(SCRIPT_PATH), + result = subprocess.run( + [ + sys.executable, + str(SCRIPT_PATH), "--output-root", str(output_root), "--icns-only", ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_generate_brand_icons.py` around lines 31 - 43, The subprocess invocation in the test uses the literal "python3" which can point to a different interpreter; change the argv in the subprocess.run call that references SCRIPT_PATH to use sys.executable instead of "python3" and add an import sys at the top of scripts/test_generate_brand_icons.py so sys is available; keep the rest of the subprocess.run args (cwd=REPO_ROOT, capture_output=True, text=True, check=False) unchanged.scripts/launch-dev-app.sh (1)
18-25: Minor:trapis installed aftermktemp, so a failure between Line 18 and Line 25 would leak the temp dir.With
set -e, if any command between themktempon Line 18 and thetrap cleanup EXITon Line 25 were to fail (currently just the assignment on Line 19, so in practice safe), the temp directory wouldn't be cleaned up. Low risk today since only a simple assignment sits between them, but as the prelude grows this becomes a footgun. Consider installing the trap immediately aftermktemp:Proposed reorder
brand_temp_dir="$(mktemp -d "${TMPDIR:-/tmp}/open-island-brand.XXXXXX")" -brand_icon="$brand_temp_dir/OpenIsland.icns" - cleanup() { rm -rf "$brand_temp_dir" } - trap cleanup EXIT + +brand_icon="$brand_temp_dir/OpenIsland.icns"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/launch-dev-app.sh` around lines 18 - 25, The temp-dir leak risk comes from installing the trap after creating brand_temp_dir and before other commands; move the trap setup so that immediately after creating brand_temp_dir with mktemp you install trap cleanup EXIT (keeping the cleanup() function and the subsequent brand_icon assignment where they are), and ensure mktemp failure is handled (e.g., exit if mktemp fails) so cleanup only runs when brand_temp_dir exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/launch-dev-app.sh`:
- Around line 18-25: The temp-dir leak risk comes from installing the trap after
creating brand_temp_dir and before other commands; move the trap setup so that
immediately after creating brand_temp_dir with mktemp you install trap cleanup
EXIT (keeping the cleanup() function and the subsequent brand_icon assignment
where they are), and ensure mktemp failure is handled (e.g., exit if mktemp
fails) so cleanup only runs when brand_temp_dir exists.
In `@scripts/test_generate_brand_icons.py`:
- Around line 22-27: The current pre-check uses subprocess.run([... "git",
"diff", "--quiet", "--", *GENERATED_BRAND_PATHS], ...) and asserts
before.returncode == 0, which fails if a developer has unrelated local edits;
change this to record a snapshot of the tracked files’ state before the test
(e.g., capture content hashes or the output of git diff -- <paths> into a
string) and then after running the generation compare the before and after
snapshots for equality instead of asserting a clean working tree; update the
variable names around before and the assertEqual call (e.g., before_snapshot and
after_snapshot) so the test verifies “no changes produced by this run”
regardless of starting dirty state.
- Around line 31-43: The subprocess invocation in the test uses the literal
"python3" which can point to a different interpreter; change the argv in the
subprocess.run call that references SCRIPT_PATH to use sys.executable instead of
"python3" and add an import sys at the top of
scripts/test_generate_brand_icons.py so sys is available; keep the rest of the
subprocess.run args (cwd=REPO_ROOT, capture_output=True, text=True, check=False)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6880e75f-e1e5-426d-82d9-35d90c9da45a
📒 Files selected for processing (4)
Assets/Brand/README.mdscripts/generate_brand_icons.pyscripts/launch-dev-app.shscripts/test_generate_brand_icons.py
|
Thanks for this, and apologies for the long silence. The problem you identified — dev launch rewriting the tracked brand PNGs on every run — just landed via #680, which takes a smaller route: This review was generated by AI and may contain mistakes in judgement. If anything here is wrong or unclear, please reply directly to this comment. |
2 participants
Summary
Test Plan
Summary by CodeRabbit
Tests
Refactor
Documentation