step 1/6: LED基盤とCI/型設定の土台を追加 [dev/v1.0 corrected]#388
Conversation
remove local config
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…into feature/light
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Feature/light
Fix typo in GUIDELINE_ja.md
…363-resolve # Conflicts: # firmware/stackchan/main.ts
📝 WalkthroughWalkthroughThis PR updates the build infrastructure to support fontbm installation, refactors face rendering with a new palette-driven theming system and Die-based clipping, switches LED configuration to use preferences instead of config, adds a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdc8d0a4d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const currentFace = this.face | ||
| this.face = face | ||
| if (currentFace) this.main.remove(currentFace) | ||
| if (this.effects) this.main.insert(this.face, this.effects) | ||
| else this.main.add(this.face) | ||
| } | ||
|
|
||
| private applyTheme(faceContext: Readonly<FaceContext>) { | ||
| if (!this.autoTheme || !this.main) return | ||
| const secondary = faceContext.theme.secondary | ||
| if (secondary === this.lastSecondary) return | ||
| this.lastSecondary = secondary | ||
| this.main.skin = new Skin({ fill: secondary }) | ||
| if (currentFace) this.faceRegion.remove(currentFace) | ||
| this.faceRegion.add(this.face) |
There was a problem hiding this comment.
Update setFace to re-layout the replacement face inside FACE_REGION (and resize/reposition the region if needed). In this commit, FaceMainTemplate computes FACE_REGION geometry from the initial face and rewrites that face’s coordinates, but setFace now only does remove/add on the existing region. When RendererCompat.setFace() is called with a fresh face (e.g., normal left/top/width/height coordinates or different dimensions), the new face keeps its original coordinates relative to the region and can be offset or clipped by the stale region bounds.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
firmware/stackchan/renderers-piu/face-view.ts (1)
154-157: Direct mutation offace.coordinatesmay cause unintended side effects.The face object's
coordinatesproperty is mutated directly to offset it within the Die region. If the same face instance is reused elsewhere or inspected for its original position, this could lead to unexpected behavior. Consider whether this mutation is intentional or if a wrapper container would be more appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/stackchan/renderers-piu/face-view.ts` around lines 154 - 157, Avoid mutating the shared face object: instead of modifying face.coordinates in place, create a new coordinates object or a shallow clone of the face (e.g., newFace = { ...face, coordinates: { left: breathPad, top: breathPad } }) or wrap the face in a container that applies the offset; update uses of face in renderers-piu/face-view.ts to reference the newFace/container so the original face.coordinates remains unchanged (look for occurrences of face and face.coordinates to replace)..github/actions/setup/action.yml (1)
67-67: MakeFONTBMexport idempotent to avoid duplicate env entries.Repeated appends can accumulate duplicate
export FONTBM=...lines in the same file.Suggested idempotent write
- printf '\nexport FONTBM=%s\n' "$HOME/.local/share/fontbm/build/fontbm" >> $HOME/.local/share/xs-dev-export.sh + grep -q '^export FONTBM=' "$HOME/.local/share/xs-dev-export.sh" \ + && sed -i "s|^export FONTBM=.*|export FONTBM=$HOME/.local/share/fontbm/build/fontbm|" "$HOME/.local/share/xs-dev-export.sh" \ + || printf '\nexport FONTBM=%s\n' "$HOME/.local/share/fontbm/build/fontbm" >> "$HOME/.local/share/xs-dev-export.sh"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup/action.yml at line 67, Make the FONTBM export idempotent by ensuring we don't append duplicate "export FONTBM=..." lines to $HOME/.local/share/xs-dev-export.sh: check for an existing FONTBM export (matching the literal "export FONTBM=" or the exact path) and either replace it or remove it before appending the new export, or write the export only if grep -q fails; update the action line that currently appends FONTBM so it performs that existence check/replacement on $HOME/.local/share/xs-dev-export.sh instead of unconditional printf append.
🤖 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/actions/setup/action.yml:
- Around line 64-66: The git clone of
https://github.com/vladimirgamalyan/fontbm.git should be pinned to an immutable
ref instead of cloning the default branch; update the clone command in
action.yml (the line with "git clone --depth 1
https://github.com/vladimirgamalyan/fontbm.git $HOME/.local/share/fontbm") to
clone a specific tag or commit SHA (or reference an input/variable like
FONTBM_REF and default it to a release tag/commit) so CI uses a fixed,
reproducible ref before running the cmake/make steps.
In `@firmware/stackchan/renderers-piu/face-view.ts`:
- Around line 1-8: The class FaceMainTemplate extends Behavior but Behavior is
not imported from 'piu/MC', causing a runtime ReferenceError; fix by adding
Behavior to the import list from 'piu/MC' alongside Container, Die, Skin (i.e.,
include Behavior in the existing import statement) so that the FaceMainTemplate
class can extend it without error.
---
Nitpick comments:
In @.github/actions/setup/action.yml:
- Line 67: Make the FONTBM export idempotent by ensuring we don't append
duplicate "export FONTBM=..." lines to $HOME/.local/share/xs-dev-export.sh:
check for an existing FONTBM export (matching the literal "export FONTBM=" or
the exact path) and either replace it or remove it before appending the new
export, or write the export only if grep -q fails; update the action line that
currently appends FONTBM so it performs that existence check/replacement on
$HOME/.local/share/xs-dev-export.sh instead of unconditional printf append.
In `@firmware/stackchan/renderers-piu/face-view.ts`:
- Around line 154-157: Avoid mutating the shared face object: instead of
modifying face.coordinates in place, create a new coordinates object or a
shallow clone of the face (e.g., newFace = { ...face, coordinates: { left:
breathPad, top: breathPad } }) or wrap the face in a container that applies the
offset; update uses of face in renderers-piu/face-view.ts to reference the
newFace/container so the original face.coordinates remains unchanged (look for
occurrences of face and face.coordinates to replace).
🪄 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: e891ce30-7ccb-4755-8411-b09bc68b8c40
📒 Files selected for processing (6)
.github/actions/setup/action.ymlfirmware/stackchan/main.tsfirmware/stackchan/renderers-piu/face-view.tsfirmware/stackchan/robot.tsfirmware/stackchan/utilities/consts.tsfirmware/tsconfig.json
💤 Files with no reviewable changes (1)
- firmware/tsconfig.json
| git clone --depth 1 https://github.com/vladimirgamalyan/fontbm.git $HOME/.local/share/fontbm | ||
| cd $HOME/.local/share/fontbm && mkdir -p build && cd build | ||
| cmake .. -DCMAKE_BUILD_TYPE=Release && make -j$(nproc) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all workflow/action files for unpinned git clone usage.
rg -n --type=yaml 'git clone\s+https://github.com/' .github -C2Repository: stack-chan/stack-chan
Length of output: 47
🏁 Script executed:
# First, check if the action.yml file exists and examine its content
fd -t f 'action.yml' .github
# Then read the specific file to check lines 64-66
if [ -f .github/actions/setup/action.yml ]; then
echo "=== Lines 60-70 of action.yml ==="
sed -n '60,70p' .github/actions/setup/action.yml | cat -n
echo ""
echo "=== Full file for context ==="
wc -l .github/actions/setup/action.yml
fiRepository: stack-chan/stack-chan
Length of output: 945
🏁 Script executed:
# Search for all git clone patterns in .github (with different approach)
rg -n 'git clone' .github --type=yaml -A1 -B1
# Also check for any pinned references to see the convention
rg -n '@|ref=|checkout' .github --type=yaml | head -20Repository: stack-chan/stack-chan
Length of output: 1632
Pin fontbm to an immutable ref for reproducible and safer CI builds.
This is the only unpinned external dependency in the CI workflow. Cloning the default branch at runtime makes builds non-deterministic and increases supply-chain risk, which contradicts the pinned versions used for all other external actions (e.g., actions/checkout@v4).
Suggested fix
sudo apt-get install -y cmake libfreetype6-dev
- git clone --depth 1 https://github.com/vladimirgamalyan/fontbm.git $HOME/.local/share/fontbm
+ FONTBM_REF="<reviewed-commit-sha-or-tag>"
+ git clone https://github.com/vladimirgamalyan/fontbm.git "$HOME/.local/share/fontbm"
+ git -C "$HOME/.local/share/fontbm" checkout --detach "$FONTBM_REF"
cd $HOME/.local/share/fontbm && mkdir -p build && cd build📝 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.
| git clone --depth 1 https://github.com/vladimirgamalyan/fontbm.git $HOME/.local/share/fontbm | |
| cd $HOME/.local/share/fontbm && mkdir -p build && cd build | |
| cmake .. -DCMAKE_BUILD_TYPE=Release && make -j$(nproc) | |
| FONTBM_REF="<reviewed-commit-sha-or-tag>" | |
| git clone https://github.com/vladimirgamalyan/fontbm.git "$HOME/.local/share/fontbm" | |
| git -C "$HOME/.local/share/fontbm" checkout --detach "$FONTBM_REF" | |
| cd $HOME/.local/share/fontbm && mkdir -p build && cd build | |
| cmake .. -DCMAKE_BUILD_TYPE=Release && make -j$(nproc) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/actions/setup/action.yml around lines 64 - 66, The git clone of
https://github.com/vladimirgamalyan/fontbm.git should be pinned to an immutable
ref instead of cloning the default branch; update the clone command in
action.yml (the line with "git clone --depth 1
https://github.com/vladimirgamalyan/fontbm.git $HOME/.local/share/fontbm") to
clone a specific tag or commit SHA (or reference an input/variable like
FONTBM_REF and default it to a release tag/commit) so CI uses a fixed,
reproducible ref before running the cmake/make steps.
| import { | ||
| Container, | ||
| Die, | ||
| Skin, | ||
| type Container as PiuContainer, | ||
| type Content as PiuContent, | ||
| type Skin as PiuSkin, | ||
| } from 'piu/MC' |
There was a problem hiding this comment.
Missing Behavior import causes runtime error.
Behavior is used at line 166 (extends Behavior) but is not imported from 'piu/MC'. This will throw a ReferenceError at runtime when FaceMainTemplate is instantiated.
🐛 Proposed fix
import {
+ Behavior,
Container,
Die,
Skin,
type Container as PiuContainer,
type Content as PiuContent,
type Skin as PiuSkin,
} from 'piu/MC'📝 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.
| import { | |
| Container, | |
| Die, | |
| Skin, | |
| type Container as PiuContainer, | |
| type Content as PiuContent, | |
| type Skin as PiuSkin, | |
| } from 'piu/MC' | |
| import { | |
| Behavior, | |
| Container, | |
| Die, | |
| Skin, | |
| type Container as PiuContainer, | |
| type Content as PiuContent, | |
| type Skin as PiuSkin, | |
| } from 'piu/MC' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/stackchan/renderers-piu/face-view.ts` around lines 1 - 8, The class
FaceMainTemplate extends Behavior but Behavior is not imported from 'piu/MC',
causing a runtime ReferenceError; fix by adding Behavior to the import list from
'piu/MC' alongside Container, Die, Skin (i.e., include Behavior in the existing
import statement) so that the FaceMainTemplate class can extend it without
error.
1 participant
This PR recreates the logical diff from #363 on top of dev/v1.0.
Context:
This branch is a bridge branch:
That keeps the diff equivalent to #363 while preserving ancestry for later stacked PRs.
Supersedes the base-branch intent of #363.
Summary by CodeRabbit
New Features
Chores