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".

firmware/stackchan/renderers-piu/effects/speech-balloon.ts (1)

91-117: The dynamic-height path is still quadratic.

When height is not fixed, setText() re-runs countWrappedLines(), which concatenates and measures a growing prefix for every character. For streamed transcripts, that makes update cost grow with the whole accumulated string again. Consider caching wrap state or line widths instead of remeasuring every prefix on each update.

Also applies to: 172-178

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@firmware/stackchan/renderers-piu/effects/speech-balloon.ts` around lines 91 -
117, countWrappedLines currently rebuilds and remeasures every growing prefix on
each call (used by resolveHeight), producing quadratic cost for
streaming/appended text; change to an incremental/cached wrap algorithm:
maintain persistent wrap state (e.g., array of lineWidths or current lines)
keyed to the existing text and update it when setText/appends occur rather than
recomputing from scratch, and have countWrappedLines (or its replacement) use
that cached state to compute the new line count in O(chars appeneded) time;
locate and update the logic in countWrappedLines and resolveHeight and any other
place calling the function (the streaming update path referenced around the
second occurrence) to use the new cached structure and update it on each append
instead of remeasuring all prefixes.

firmware/stackchan/manifest_local.json (1)

4-8: Avoid checking a workstation-specific Voicevox endpoint into the shared local manifest.

192.168.7.136 will be dead for almost everyone else pulling the branch. Prefer leaving tts.host/tts.port to mcconfig overrides or an untracked local overlay.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@firmware/stackchan/manifest_local.json` around lines 4 - 8, The
manifest_local.json currently hardcodes a workstation-specific Voicevox endpoint
(tts.host = "192.168.7.136" and tts.port = 50021); remove these hardcoded values
from the tracked manifest and make tts.host and tts.port configurable via the
existing mcconfig override or an untracked local overlay instead (leave them
null/empty or delete the keys in manifest_local.json), ensuring consumers can
set values in their local mcconfig or overlay; reference the tts.host and
tts.port fields and update any README or comments to instruct developers to set
these in mcconfig/local overlay rather than committing workstation-specific
endpoints.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

firmware/tests/renderers/default-mod-face/main.ts (1)

6-12: Exercise the registered drawer callback as well.

The test validates initial sync, but it would still pass if the toggleFace callback stopped cycling simple → dog → image → simple. Capturing kind, initialState, and invoking the callback would cover the actual drawer interaction added in firmware/stackchan/default-mods/on-robot-created.ts:46-55.

Suggested test expansion

+type DrawerButton = {
+  key?: string
+  kind?: string
+  initialState?: boolean
+  callback?: (target: {
+    renderer?: {
+      setFace?: (face: unknown) => void
+      application?: { distribute?: (event: string, payload: unknown) => void }
+    }
+  }) => void
+}
+
-const buttons: { key?: string }[] = []
+const buttons: DrawerButton[] = []
 const drawerStates: [string, boolean][] = []
 const events: [string, unknown][] = []

 const robot = {
   application: {
-    addDrawerButton: (button: { key?: string }) => {
+    addDrawerButton: (button: DrawerButton) => {
       buttons.push(button)
     },
     setDrawerButtonState: (key: string, active: boolean) => {
       drawerStates.push([key, active])
@@
 onRobotCreated(robot as never)

 equal(buttons[0]?.key, 'toggleFace', 'toggleFace button should be registered')
+equal(buttons[0]?.kind, 'toggle', 'toggleFace should be registered as a toggle button')
+equal(buttons[0]?.initialState, false, 'toggleFace initialState should be inactive')
 equal(drawerStates[0]?.[0], 'toggleFace', 'toggleFace state should be initialized')
 equal(drawerStates[0]?.[1], false, 'toggleFace should start inactive')
 equal(events[0]?.[0], 'onFaceMode', 'initial face mode should be distributed')
 equal(events[0]?.[1], 'simple', 'initial face mode should be simple')
+
+const faces: unknown[] = []
+const callbackTarget = {
+  renderer: {
+    ...robot.renderer,
+    setFace: (face: unknown) => {
+      faces.push(face)
+    },
+  },
+}
+
+buttons[0]?.callback?.(callbackTarget)
+equal(drawerStates[1]?.[0], 'toggleFace', 'toggleFace state should update after first toggle')
+equal(drawerStates[1]?.[1], true, 'toggleFace should become active after switching to dog')
+equal(events[1]?.[1], 'dog', 'first toggle should switch to dog face mode')
+
+buttons[0]?.callback?.(callbackTarget)
+equal(drawerStates[2]?.[1], true, 'toggleFace should remain active after switching to image')
+equal(events[2]?.[1], 'image', 'second toggle should switch to image face mode')
+
+buttons[0]?.callback?.(callbackTarget)
+equal(drawerStates[3]?.[1], false, 'toggleFace should become inactive after returning to simple')
+equal(events[3]?.[1], 'simple', 'third toggle should return to simple face mode')
+equal(faces.length, 3, 'face should be replaced on each toggle')

Also applies to: 42-48

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@firmware/tests/renderers/default-mod-face/main.ts` around lines 6 - 12, The
test currently registers drawer callbacks via robot.application.addDrawerButton
but never invokes them; extend the test to capture the registered button (from
the buttons array) and its callback (the addDrawerButton handler stored when
robot.application.addDrawerButton is called), extract the callback's parameters
like kind and initialState, then invoke that callback repeatedly (or with the
appropriate event payload) to assert the drawer state cycles through "simple" →
"dog" → "image" → "simple" as implemented in on-robot-created.ts; update
assertions on drawerStates and events after each invocation to verify the actual
drawer interaction logic.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

firmware/stackchan/renderers-piu/effects/speech-balloon.ts (1)

185-198: Minor: redundant updateText call on setText / onDisplaying.

ensureParts already invokes this.updateText(self, currentText) (Line 147) after a rebuild, and updatePalette(currentFace) (Line 146). So:

• setText (Lines 185–189) calls updateText a second time whenever a rebuild occurred.
• onDisplaying (Lines 195–198) calls updatePalette(currentFace) a second time after ensureParts already did.

Harmless today (palette cache short-circuits, updateText just reassigns the same string), but can be trimmed:

♻️ Proposed tweak

       setText(self: PiuContainer, text: string) {
         currentText = text ?? ''
-        this.ensureParts(self)
-        this.updateText(self, currentText)
+        // ensureParts will call updateText(currentText) if it rebuilds;
+        // otherwise we still need to push the new string through.
+        const rebuilt = !background || !bodyText || layoutWidth !== resolveWidth(self)
+        this.ensureParts(self)
+        if (!rebuilt) this.updateText(self, currentText)
       }
@@
       onDisplaying(content: PiuContainer) {
         this.ensureParts(content)
-        this.updatePalette(currentFace)
       }

(Requires hoisting resolveWidth/layoutWidth/background/bodyText visibility, which they already have in the closure.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@firmware/stackchan/renderers-piu/effects/speech-balloon.ts` around lines 185
- 198, The setText and onDisplaying methods redundantly call updateText and
updatePalette after ensureParts already performs those updates; remove the
duplicate calls in setText (remove this.updateText(self, currentText)) and in
onDisplaying (remove this.updatePalette(currentFace)) so ensureParts is the
single place that updates text and palette during a rebuild, or alternatively
wrap those calls with a cheap guard that skips if ensureParts already ran (use
currentText/currentFace state checks); update references: setText, onDisplaying,
ensureParts, updateText, updatePalette, currentText, currentFace.

firmware/stackchan/renderers-piu/effects/emoticon.ts (1)

9-14: Optional: intersecting PiuContent with PiuShape for WithSkin.

WithSkin now intersects PiuContent with Omit<PiuShape, 'fillOutline' | 'strokeOutline'>. Since all call sites construct shapes (not bare contents) via defineShapeTemplate, typing this directly as Omit<PiuShape, ...> & { skin?: PiuSkin; fillOutline?: unknown; strokeOutline?: unknown } would be more accurate and drop the need to intersect with PiuContent. Not blocking — the current intersection typechecks.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@firmware/stackchan/renderers-piu/effects/emoticon.ts` around lines 9 - 14,
The current WithSkin type unnecessarily intersects PiuContent with
Omit<PiuShape, 'fillOutline' | 'strokeOutline'>; update WithSkin to be based
directly on the shape type so it reflects actual call sites: replace the
intersection with a single type defined as Omit<PiuShape, 'fillOutline' |
'strokeOutline'> & { skin?: PiuSkin; fillOutline?: unknown; strokeOutline?:
unknown } (remove PiuContent from the union); ensure usages in
defineShapeTemplate still type-check after this change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes