fix(core): correctly render badge recipe background/text#271
Merged
RtlZeroMemory merged 4 commits intomainfrom Mar 7, 2026
Merged
fix(core): correctly render badge recipe background/text#271RtlZeroMemory merged 4 commits intomainfrom
RtlZeroMemory merged 4 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughRefactors badge/tag color resolution and rendering so recipes' bg/text tokens drive pill backgrounds and text colors; replaces Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant Renderer as renderTextWidgets
participant Recipe as badge/tag Recipe & Token Resolver
participant Drawlist as DrawList Builder
UI->>Renderer: render badge("LABEL", variant)
Renderer->>Recipe: resolveTagColor / badgeRecipe(theme, variant)
Recipe-->>Renderer: { text: {fg, bold}, bg: {bg} } or token fallbacks
alt recipe.bg present
Renderer->>Drawlist: fillRect(bg) <!-- background painted -->
end
Renderer->>Drawlist: drawText("( LABEL )", fg: text.fg, bold)
Drawlist-->>Renderer: ops recorded (fillRect?, drawText)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
Comment |
![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 (1)
packages/core/src/renderer/__tests__/sliderRecipeRendering.test.ts (1)
44-59: Add the explicitprops.styleprecedence case in this test file.This covers the recipe-only path, but the PR also changes how badge background is carried onto
drawText. A second case withui.badge(..., { variant: "info", style: { fg, bg } })would lock down the stated guarantee that explicit style overrides still win over recipe values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/sliderRecipeRendering.test.ts` around lines 44 - 59, Add a second test that verifies explicit props.style wins over recipe colors: call renderOps(ui.badge("Info", { variant: "info", style: { fg: /* explicit fg */, bg: /* explicit bg */ } }), { viewport: { cols: 24, rows: 2 }, theme: dsTheme }), then locate the fillRect (ops.find(...)) and drawText (firstDrawText(...)) as in the existing test and assert the fill.style?.bg and text.style?.bg equal the explicit bg, text.style?.fg equals the explicit fg, the text includes "( Info )", and bold remains true; use unique symbols ui.badge, renderOps, dsTheme, fillRect, and firstDrawText to find the right places to assert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/renderer/__tests__/sliderRecipeRendering.test.ts`:
- Around line 49-57: The current test picks the first fillRect from ops which
may be unrelated; update the find for the badge fill (the variable `fill`
created from `ops.find(...)`) to match the badge's expected background (e.g.
require `op.style?.bg === dsTheme.colors.info`) and, if available, also verify
the badge geometry by matching the expected `op.rect` (or calculated badge rect)
so the test fails when the pill background or position disappears; keep the
`text` checks using `firstDrawText` as-is to confirm the "( Info )" label.
In `@packages/core/src/renderer/renderToDrawlist/widgets/renderTextWidgets.ts`:
- Around line 906-909: The background fill currently merges the entire ownStyle
into bgStyle (via mergeTextStyle(bgBaseStyle, ownStyle)), which allows text
attributes like fg, bold, or inverse to affect builder.fillRect; instead, only
merge the bg attribute from ownStyle into bgBaseStyle before calling
builder.fillRect. Locate the block that computes bgBaseStyle and bgStyle (using
mergeTextStyle, parentStyle, recipeResult.bg, and ownStyle) and change the
second merge to use an object containing only ownStyle.bg (e.g., merge only {
bg: ownStyle.bg } when ownStyle exists) so builder.fillRect receives a style
with background overrides only.
---
Nitpick comments:
In `@packages/core/src/renderer/__tests__/sliderRecipeRendering.test.ts`:
- Around line 44-59: Add a second test that verifies explicit props.style wins
over recipe colors: call renderOps(ui.badge("Info", { variant: "info", style: {
fg: /* explicit fg */, bg: /* explicit bg */ } }), { viewport: { cols: 24, rows:
2 }, theme: dsTheme }), then locate the fillRect (ops.find(...)) and drawText
(firstDrawText(...)) as in the existing test and assert the fill.style?.bg and
text.style?.bg equal the explicit bg, text.style?.fg equals the explicit fg, the
text includes "( Info )", and bold remains true; use unique symbols ui.badge,
renderOps, dsTheme, fillRect, and firstDrawText to find the right places to
assert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cfc8c72-ea5e-4348-9d79-0313b47a81f8
📒 Files selected for processing (2)
packages/core/src/renderer/__tests__/sliderRecipeRendering.test.tspackages/core/src/renderer/renderToDrawlist/widgets/renderTextWidgets.ts
packages/core/src/renderer/__tests__/sliderRecipeRendering.test.ts
Outdated
Show resolved
Hide resolved
packages/core/src/renderer/renderToDrawlist/widgets/renderTextWidgets.ts
Show resolved
Hide resolved
Owner
Author
|
Addressed the CodeRabbit review feedback in 3c5e448:\n- Scoped badge fill assertion to the badge background color in the regression test.\n- Added explicit props.style precedence test for badge fg/bg overrides.\n- Narrowed badge fillRect style merge to background-only overrides () to avoid carrying text attributes into fill operations.\n\nValidation rerun:\n- npm run build\n- npm run lint\n- npm run typecheck\n- node --test packages/core/dist/renderer/tests/sliderRecipeRendering.test.js\n- node scripts/run-tests.mjs |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts`:
- Around line 203-210: The test currently only verifies a single paused
snapshot; update it to assert the value stays frozen across a real paused
interval by advancing frames/time and re-checking that progress does not change.
Specifically, after the initial waitFor that captures entry from
h.render((hooks) => useParallel(hooks, paused)) (referencing useParallel,
paused, pausedValue, next.pendingEffects, h.runPending), run several subsequent
frames (e.g., call h.runPending(next.pendingEffects) repeatedly or advance
timers) and assert each time that Math.abs(entry.value - pausedValue) <= 0.1 and
entry.isAnimating === false so the test proves no drift over time while paused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5272056-ed1c-420c-99e0-a1d07c762932
📒 Files selected for processing (1)
packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts
Comment on lines
+203
to
+210
| await waitFor(() => { | ||
| const next = h.render((hooks) => useParallel(hooks, paused)); | ||
| render = next; | ||
| h.runPending(next.pendingEffects); | ||
| const entry = next.result[0]; | ||
| if (!entry) return false; | ||
| return Math.abs(entry.value - pausedValue) <= 0.1 && entry.isAnimating === false; | ||
| }); |
There was a problem hiding this comment.
Keep a real paused interval in this regression test.
This only proves that one render produced a paused snapshot. It no longer verifies that progress stays frozen over time while paused, so a regression that still advances for a few frames can pass here.
Suggested change
await waitFor(() => {
const next = h.render((hooks) => useParallel(hooks, paused));
render = next;
h.runPending(next.pendingEffects);
const entry = next.result[0];
if (!entry) return false;
return Math.abs(entry.value - pausedValue) <= 0.1 && entry.isAnimating === false;
});
+
+ await sleep(80);
+ render = h.render((hooks) => useParallel(hooks, paused));
+ h.runPending(render.pendingEffects);
+ assert.ok(Math.abs((render.result[0]?.value ?? 0) - pausedValue) <= 0.1);
+ assert.equal(render.result[0]?.isAnimating, false);Based on learnings, "Read target file tests before changing behavior; expected behavior and edge cases are documented in test files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts`
around lines 203 - 210, The test currently only verifies a single paused
snapshot; update it to assert the value stays frozen across a real paused
interval by advancing frames/time and re-checking that progress does not change.
Specifically, after the initial waitFor that captures entry from
h.render((hooks) => useParallel(hooks, paused)) (referencing useParallel,
paused, pausedValue, next.pendingEffects, h.runPending), run several subsequent
frames (e.g., call h.runPending(next.pendingEffects) repeatedly or advance
timers) and assert each time that Math.abs(entry.value - pausedValue) <= 0.1 and
entry.isAnimating === false so the test proves no drift over time while paused.
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
1 participant
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
Root Cause
Badge rendering was taking recipe background color and using it as foreground text color, while not reliably preserving background under text draw.
Changes
Validation
Closes #270
Summary by CodeRabbit
Tests
Refactor