fix(core): detect style-only line changes in diff renderer#709
Merged
Conversation
github-actions
Bot
added
type:bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTerminal rendering now detects style-only changes. Screen computes per-row style fingerprints from cell attributes and snapshots them; Renderer's differential renderer skips emitting rows only when both text content and style match previous frames. ChangesStyle-aware differential rendering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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 |
|
Hi @Siddh2024 👋 ⭐ 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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR to TermUI, @Siddh2024.
Before your PR merges:
- ⭐ Star the repo. Required. The
star-checkjob blocks your merge otherwise. - ✅ All checks green:
build,test,typecheck. - 🏷 PR title follows
type: short description. Example:fix: handle empty list. - 🔗 Link your closing issue in the description.
GSSoC 2026 points come from labels after merge:
gssoc:approved. +50 base points.level:beginner/intermediate/advanced/critical. +20 / +35 / +55 / +80.quality:clean/exceptional. x 1.2 / x 1.5.type:*. Stackable bonus.
Your reviewer responds within 48 hours. Ping @Karanjot786 on Discord for urgent help.
🚀 Welcome to the cohort.
|
Hi @Siddh2024 👋 ⭐ 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. |
![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
🤖 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 154-164: The style fingerprint currently only uses the first
character of fg.type/bg.type which collapses distinct colors; update the seed
computation in Screen (around the seed/hash logic using cell, fg, bg, bits) to
incorporate the full color payload rather than charCodeAt(0) of the type:
include both the color type and the actual color value (e.g., combine fg.type
and its payload/value/string representation and same for bg) when deriving seed
so different named/hex/rgb values produce different seeds before updating hash.
🪄 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: 0bbf61fd-a54f-4b66-af46-658fb83ce7f1
📒 Files selected for processing (2)
packages/core/src/terminal/Renderer.tspackages/core/src/terminal/Screen.ts
Comment on lines
+154
to
+164
| const fg = cell.fg.type; | ||
| const bg = cell.bg.type; | ||
| const bits = | ||
| (cell.bold ? 1 : 0) | | ||
| (cell.italic ? 2 : 0) | | ||
| (cell.underline ? 4 : 0) | | ||
| (cell.dim ? 8 : 0) | | ||
| (cell.strikethrough ? 16 : 0) | | ||
| (cell.inverse ? 32 : 0); | ||
| const seed = fg.charCodeAt(0) * 65536 + bg.charCodeAt(0) * 4096 + bits; | ||
| hash = ((hash << 7) - hash + seed) | 0; |
There was a problem hiding this comment.
Style fingerprint currently misses real color changes within the same color type.
On Line 154–Line 164, the fingerprint seed only uses fg.type/bg.type (and only their first character), not the actual color payload. This means many style-only color edits still hash equal (e.g., named:red → named:blue, and even none vs named both start with 'n'), so rows can be incorrectly skipped.
Suggested fix (allocation-light, includes full color payload)
@@
getStyleLine(row: number): string {
if (row < 0 || row >= this._rows) return '';
let hash = 0;
+ const colorSeed = (color: Color): number => {
+ switch (color.type) {
+ case 'none':
+ return 1;
+ case 'named': {
+ let h = 2;
+ for (let i = 0; i < color.name.length; i++) h = ((h << 5) - h + color.name.charCodeAt(i)) | 0;
+ return h;
+ }
+ case 'ansi256':
+ return 3 + color.code;
+ case 'rgb':
+ return 4 + ((color.r << 16) | (color.g << 8) | color.b);
+ case 'hex': {
+ let h = 5;
+ for (let i = 0; i < color.hex.length; i++) h = ((h << 5) - h + color.hex.charCodeAt(i)) | 0;
+ return h;
+ }
+ }
+ };
for (const cell of this.back[row]) {
if (cell.width === 0) continue;
- const fg = cell.fg.type;
- const bg = cell.bg.type;
const bits =
(cell.bold ? 1 : 0) |
(cell.italic ? 2 : 0) |
(cell.underline ? 4 : 0) |
(cell.dim ? 8 : 0) |
(cell.strikethrough ? 16 : 0) |
(cell.inverse ? 32 : 0);
- const seed = fg.charCodeAt(0) * 65536 + bg.charCodeAt(0) * 4096 + bits;
+ const seed = colorSeed(cell.fg) * 65536 + colorSeed(cell.bg) * 4096 + bits;
hash = ((hash << 7) - hash + seed) | 0;
if (cell.link) {
for (let i = 0; i < cell.link.length; i++)
hash = ((hash << 5) - hash + cell.link.charCodeAt(i)) | 0;
}
}
return String(hash);
}🤖 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.ts` around lines 154 - 164, The style
fingerprint currently only uses the first character of fg.type/bg.type which
collapses distinct colors; update the seed computation in Screen (around the
seed/hash logic using cell, fg, bg, bits) to incorporate the full color payload
rather than charCodeAt(0) of the type: include both the color type and the
actual color value (e.g., combine fg.type and its payload/value/string
representation and same for bg) when deriving seed so different named/hex/rgb
values produce different seeds before updating hash.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/create-termui-app/src/index.ts (1)
16-29: ⚡ Quick winConsider additional validation for cross-platform robustness.
The regex and explicit "." / ".." checks effectively prevent path traversal. However, a few edge cases could improve robustness:
- Windows reserved names: Names like "CON", "PRN", "AUX", "NUL", "COM1"-"COM9", "LPT1"-"LPT9" will cause
mkdirSyncto fail on Windows with a cryptic error.- Trailing dots: The regex allows names ending with a dot (e.g., "myapp."), which can cause issues on Windows where trailing dots in filenames are sometimes ignored.
- Scoped package message: The error message mentions "start with @ for scoped packages," but the regex doesn't allow
/, so proper scoped package names like "@org/package" are rejected. Consider clarifying the message (e.g., "or start with @ for scoped names") or documenting that/is intentionally disallowed.🛡️ Suggested enhancement for Windows reserved names
+const WINDOWS_RESERVED_NAMES = /^(con|prn|aux|nul|com[1-9]|lpt[1-9])$/i; + function validateProjectName(name: string, source: string): void { if (!VALID_PROJECT_NAME_RE.test(name)) { throw new Error( `Invalid project name "${name}" (from ${source}). Use only letters, digits, hyphens, underscores, dots, or start with @ for scoped packages.` ); } if (name === "." || name === "..") { throw new Error( `Invalid project name "${name}". "." and ".." are not allowed as project names.` ); } + if (WINDOWS_RESERVED_NAMES.test(name)) { + throw new Error( + `Invalid project name "${name}". "${name.toUpperCase()}" is a reserved name on Windows.` + ); + } + if (name.endsWith(".")) { + throw new Error( + `Invalid project name "${name}". Project names cannot end with a dot.` + ); + } }🤖 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/create-termui-app/src/index.ts` around lines 16 - 29, The validateProjectName function currently uses VALID_PROJECT_NAME_RE and explicit "."/".." checks but misses Windows-reserved device names, trailing dots, and misleads about scoped names; update validateProjectName to (1) reject Windows reserved names case-insensitively (e.g., CON, PRN, AUX, NUL, COM1–COM9, LPT1–LPT9) by comparing name (and name without trailing dots) against that set, (2) reject names that end with a dot or contain trailing spaces/dots (trim and check for '.' suffix) to avoid Windows filesystem quirks, and (3) either allow scoped-style names (contain a single '/' after an '@') or change the error text to say "start with @ for scoped names" so the message matches VALID_PROJECT_NAME_RE; reference VALID_PROJECT_NAME_RE and the validateProjectName function when making these changes.
🤖 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.
Nitpick comments:
In `@packages/create-termui-app/src/index.ts`:
- Around line 16-29: The validateProjectName function currently uses
VALID_PROJECT_NAME_RE and explicit "."/".." checks but misses Windows-reserved
device names, trailing dots, and misleads about scoped names; update
validateProjectName to (1) reject Windows reserved names case-insensitively
(e.g., CON, PRN, AUX, NUL, COM1–COM9, LPT1–LPT9) by comparing name (and name
without trailing dots) against that set, (2) reject names that end with a dot or
contain trailing spaces/dots (trim and check for '.' suffix) to avoid Windows
filesystem quirks, and (3) either allow scoped-style names (contain a single '/'
after an '@') or change the error text to say "start with @ for scoped names" so
the message matches VALID_PROJECT_NAME_RE; reference VALID_PROJECT_NAME_RE and
the validateProjectName function when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2b71488-a463-4f09-8584-f71f81fcb7dd
📒 Files selected for processing (1)
packages/create-termui-app/src/index.ts
Karanjot786
added
gssoc:approved
Owner
|
Thank you so much for your contribution, @Siddh2024! Your PR #709 fixed the diff renderer to detect style-only line changes. Terminals now repaint correctly when only colors or styles change without content changes. Keep contributing. We appreciate your work. |
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
2 participants
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.
Description
The differential renderer's line-level comparison only checked character text (\getLine(r) === getPreviousLine(r)), completely ignoring style attributes. When a widget updated only colors, bold, italic, or other style attributes without changing text content, the renderer skipped the line — making style changes invisible.
Root Cause
In \Renderer._flush()\ at line 130:
\\ s
// Before: only text comparison
if (this._screen.getLine(r) === this._screen.getPreviousLine(r)) continue;
\\
\getLine()\ returns only the character text (.char) joined together. Style attributes like \ g, \�g, \�old, \italic, \underline, \dim, \strikethrough, \inverse, and \link\ are completely ignored.
The Fix
**\Screen.ts**: Added \getStyleLine()\ which computes a fast fingerprint hash of all style attributes for each cell in the row, and \getPreviousStyleLine()\ to retrieve the saved fingerprint. \saveLines()\ now stores both text and style fingerprints.
**\Renderer.ts**: The diff check now requires BOTH text AND style to be unchanged before skipping a line:
\\ s
// After: text AND style comparison
if (this._screen.getLine(r) === this._screen.getPreviousLine(r)
&& this._screen.getStyleLine(r) === this._screen.getPreviousStyleLine(r)) continue;
\\
Closes #706
Summary by CodeRabbit
Bug Fixes
Performance