Adjust diff comment token color#2522
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an editor comment color to the Ember dark theme, introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/utils/code-theme/diff-theme.ts (1)
40-40: Optional: avoid recomputing editor theme in the same path.Line 40 calls
getDiffCommentColor(theme), which recomputesgetEditorTheme(theme)after Line 39 already computed it. This is minor, but easy to simplify.♻️ Proposed refactor
+function blendDiffCommentColor(comment: string, foreground: string): string { + const mixedColor = interpolate([comment, foreground], "lab")( + DIFF_COMMENT_FOREGROUND_BLEND, + ); + const rgbColor = toRgb(mixedColor); + return rgbColor ? formatHex(rgbColor) : toHex(comment); +} + export function getDiffCommentColor(theme: Theme): string { const editorTheme = getEditorTheme(theme); - const mixedColor = interpolate( - [editorTheme.syntax.comment, editorTheme.colors.foreground], - "lab", - )(DIFF_COMMENT_FOREGROUND_BLEND); - const rgbColor = toRgb(mixedColor); - - return rgbColor ? formatHex(rgbColor) : toHex(editorTheme.syntax.comment); + return blendDiffCommentColor( + editorTheme.syntax.comment, + editorTheme.colors.foreground, + ); } function createShikiTheme(theme: Theme) { const editorTheme = getEditorTheme(theme); - const diffCommentColor = getDiffCommentColor(theme); + const diffCommentColor = blendDiffCommentColor( + editorTheme.syntax.comment, + editorTheme.colors.foreground, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/utils/code-theme/diff-theme.ts` at line 40, The code calls getDiffCommentColor(theme) which internally recomputes getEditorTheme(theme) even though you already compute editorTheme; refactor to compute const editorTheme = getEditorTheme(theme) once and then either change getDiffCommentColor to accept editorTheme (e.g., getDiffCommentColor(editorTheme)) or call a helper that uses the existing editorTheme, then assign diffCommentColor from that to avoid redundant getEditorTheme calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/utils/code-theme/diff-theme.ts`:
- Line 40: The code calls getDiffCommentColor(theme) which internally recomputes
getEditorTheme(theme) even though you already compute editorTheme; refactor to
compute const editorTheme = getEditorTheme(theme) once and then either change
getDiffCommentColor to accept editorTheme (e.g.,
getDiffCommentColor(editorTheme)) or call a helper that uses the existing
editorTheme, then assign diffCommentColor from that to avoid redundant
getEditorTheme calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e50a098-9d73-46a5-8e42-66db238b9e4b
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/utils/code-theme/diff-theme.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/utils/code-theme/diff-theme.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/main/terminal-host/session-lifecycle.test.ts (1)
50-54: DuplicatedescribeWithTimeoutdefinition.This is identical to the wrapper in
daemon.test.ts. As mentioned in the other file, consider extracting this totest-helpers.tsalongsidesupportsLocalSocketBindingto reduce duplication and centralize test configuration.♻️ Suggested shared utility in test-helpers.ts
// In test-helpers.ts import { describe } from "bun:test"; export const describeWithTimeout = describe as unknown as ( label: string, options: { timeout: number }, body: () => void, ) => void; // Common timeout for integration tests export const INTEGRATION_TEST_TIMEOUT = 15000;Then import in both test files:
import { describeWithTimeout, INTEGRATION_TEST_TIMEOUT, supportsLocalSocketBinding } from "./test-helpers";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session-lifecycle.test.ts` around lines 50 - 54, The duplicate describeWithTimeout definition should be extracted into a shared test helper; create a new test-helpers.ts exporting describeWithTimeout (the cast wrapper), INTEGRATION_TEST_TIMEOUT, and supportsLocalSocketBinding, then remove the local duplicate from session-lifecycle.test.ts and daemon.test.ts and import describeWithTimeout and INTEGRATION_TEST_TIMEOUT (and supportsLocalSocketBinding if needed) from "./test-helpers" so both tests reuse the single definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/terminal-host/daemon.test.ts`:
- Around line 47-51: The custom casted describeWithTimeout wrapper around
describe is invalid for bun:test and silently drops the timeout; remove
describeWithTimeout and replace its usages with the native describe (or plain
describe blocks) and set timeouts on individual tests (use test(name, fn,
timeoutMs)) or configure a global timeout in bunfig.toml ([test] timeout =
15000); update any references to describeWithTimeout in tests to either call
describe(...) and set timeouts on contained test(...) calls or move timeout
configuration to bunfig.toml.
---
Nitpick comments:
In `@apps/desktop/src/main/terminal-host/session-lifecycle.test.ts`:
- Around line 50-54: The duplicate describeWithTimeout definition should be
extracted into a shared test helper; create a new test-helpers.ts exporting
describeWithTimeout (the cast wrapper), INTEGRATION_TEST_TIMEOUT, and
supportsLocalSocketBinding, then remove the local duplicate from
session-lifecycle.test.ts and daemon.test.ts and import describeWithTimeout and
INTEGRATION_TEST_TIMEOUT (and supportsLocalSocketBinding if needed) from
"./test-helpers" so both tests reuse the single definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f19091e-1039-475b-a96a-dfd25669c72e
📒 Files selected for processing (2)
apps/desktop/src/main/terminal-host/daemon.test.tsapps/desktop/src/main/terminal-host/session-lifecycle.test.ts
Kitenite
force-pushed
the
kitenite/fix-the-diffs-colo
branch
from
ae31b72 to
fc640a0
Compare
z3thon
pushed a commit
to z3thon/superset
that referenced
this pull request
Mar 19, 2026
* Update colors * Refine theme tests * Lint
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
Testing
Summary by cubic
Blend diff comment tokens toward the editor foreground for better readability in diffs, using LAB interpolation (0.22) and computing the blend once per theme via
getDiffCommentColor. Added partial editor overrides and an explicit editor comment color in theembertheme so diff comments render consistently.EditorThemeOverridesand updatedTheme.editorto accept partialcolors/syntax.editor.colors.searchActivewithui.highlightActive.Written for commit f0a52ed. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests