fix: include actual color values in the style fingerprint to prevent renderer from skipping color-only changes#1807
Conversation
…event renderer from skipping color-only changes
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesColor fingerprint fix in Screen.ts
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/terminal/Screen.ts`:
- Around line 105-113: Update hashColor in Screen.ts so each color variant
contributes a non-zero tag before mixing in its value, preventing none,
ansi256(0), and rgb(0,0,0) from producing the same fingerprint from an initial
hash. Rework the named case to hash the full color.name string rather than only
the first and last characters, and apply the same variant-tag-first pattern
consistently across all switch branches.
🪄 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 Plus
Run ID: 0d1951fc-9047-430e-b905-0096506b7eed
📒 Files selected for processing (1)
packages/core/src/terminal/Screen.ts
|
Thanks — you've correctly identified a real bug: the old Issues:
Note: this overlaps with #1646, which makes the same getStyleLine/hashColor change and hits the same bench regression. Coordinate so only one lands. Checklist before re-requesting review:
|
The style fingerprint's hashColor() looped over every character of the hex string per cell (fg + bg, every dirty row, every frame), regressing render throughput ~20-32% and failing the bench CI gate. Decode the hex digits directly with charCodeAt/bit-shifts into r/g/b and combine them numerically, matching the existing O(1) rgb path. No behavior change to the fingerprint's collision-avoidance property. Co-authored-by: Karanjot786 <karanjots801@gmail.com>
|
Optimized the per-cell color hash in Note: #2150 appears to fix the same underlying bug — worth reconciling/closing one in favor of the other. |
Description
This PR fixes the issue where the renderer skips color-only changes because the
getStyleLine()fingerprint ignored actual color values and only hashed the color's type string. A newhashColor()function is introduced to incorporate the specific color values (named name, ansi code, rgb values, or hex code) into the line hash so that foreground or background color changes trigger a re-render.Related Issue
Closes #1773
Which package(s)?
@termuijs/core
Type of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/anshika1179Screenshots / Recordings (UI changes)
Notes for the Reviewer
Added
hashColorfunction to handle hashing fornone,named,ansi256,rgb, andhexcolor types, properly shifting and merging the actual color definition fields with the rolling row hash.Summary by CodeRabbit