fix(core): include full color value in getStyleLine hash to prevent collision#1664
fix(core): include full color value in getStyleLine hash to prevent collision#1664atul-upadhyay-7 wants to merge 1 commit into
Conversation
…ollision The diff renderer uses getStyleLine to detect style-only changes. The previous hash only used the first character of the color type string (e.g. 'a' for 'ansi256'), so two colors of the same type but different codes produced the same hash. This silently skipped color updates. Add hashColor helper that fingerprints the complete color value (type + code/name/r,g,b/hex) so different colors always produce distinct hashes. Closes Karanjot786#1659
github-actions
Bot
added
area:core
|
Hi @atul-upadhyay-7 👋 ⭐ Star this repo before your PR merges. Why? GSSoC 2026 contributors who star get priority review and points credit. After you star, push any commit (or re-run this check). The Thanks for your contribution to TermUI. |
📝 WalkthroughWalkthroughA new internal ChangeshashColor fix for getStyleLine fingerprinting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/terminal/Screen.test.ts (1)
154-163: ⚡ Quick winConsider testing named colors that could collide.
The test uses
'red'vs'blue'which produce different hashes (3634 vs 3139). IfNamedColorincludes bright variants, adding a test like'blue'vs'brightBlue'would verify collision resistance for all valid named colors.it('bright and non-bright named colors produce different hashes', () => { const screen = new Screen(5, 1); screen.setCell(0, 0, { char: 'A', fg: { type: 'named', name: 'blue' } }); const hash1 = screen.getStyleLine(0); screen.setCell(0, 0, { char: 'A', fg: { type: 'named', name: 'brightBlue' } }); const hash2 = screen.getStyleLine(0); expect(hash1).not.toBe(hash2); });This test would currently fail if
brightBlueis a validNamedColor, catching the collision bug inhashColor.🤖 Prompt for 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. In `@packages/core/src/terminal/Screen.test.ts` around lines 154 - 163, Add a new test case after the existing test in the Screen.test.ts file that specifically checks for hash collisions between bright and non-bright variants of the same color. Create a test named something like "bright and non-bright named colors produce different hashes" that follows the same pattern as the current test, using a Screen instance and calling setCell twice with 'blue' and 'brightBlue' as the named color values, then comparing the hashes returned by getStyleLine to ensure they are different. This will verify that the hashColor function properly distinguishes between color variants that differ only in brightness.
🤖 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 20-21: The hash function for named colors in the 'named' case of
Screen.ts only considers the first and last character of the color name, causing
collisions between different colors like blue/brightBlue/brightWhite which all
hash to the same value. This breaks style-change detection in getStyleLine().
Replace the current hash formula with a proper string hash function that
processes the entire color name string to ensure unique hash values for
different colors and prevent incorrect style-change detection.
---
Nitpick comments:
In `@packages/core/src/terminal/Screen.test.ts`:
- Around line 154-163: Add a new test case after the existing test in the
Screen.test.ts file that specifically checks for hash collisions between bright
and non-bright variants of the same color. Create a test named something like
"bright and non-bright named colors produce different hashes" that follows the
same pattern as the current test, using a Screen instance and calling setCell
twice with 'blue' and 'brightBlue' as the named color values, then comparing the
hashes returned by getStyleLine to ensure they are different. This will verify
that the hashColor function properly distinguishes between color variants that
differ only in brightness.
🪄 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: e1eeb64c-99fa-43f2-ab61-00d625eb811a
📒 Files selected for processing (2)
packages/core/src/terminal/Screen.test.tspackages/core/src/terminal/Screen.ts
1 participant
Summary
Fixes a critical bug where
Screen.getStyleLine()produced hash collisions that caused the diff renderer to silently skip color-only updates. Two cells with the same color type but different codes (e.g.ansi256code 1 vs code 9) generated the same hash, so the renderer treated the row as unchanged.Closes #1659
Root Cause
getStyleLine()computed a hash seed using onlyfg.charCodeAt(0)andbg.charCodeAt(0)on the color type string. Since'ansi256'.charCodeAt(0) === 97regardless of the actual code value, allansi256colors hashed identically. Same issue forrgb,hex, andnamedcolors.Fix
Added a
hashColor(c: Color): numberhelper that fingerprints the complete color value:none0namedname.charCodeAt(0) * 31 + name.charCodeAt(name.length - 1)ansi256code * 7 + 1rgb(r << 16) | (g << 8) | bhexgetStyleLine()now useshashColor(cell.fg)andhashColor(cell.bg)instead of the type string's first character.Tests added
Verification
Summary by CodeRabbit
Tests
Bug Fixes