-
Notifications
You must be signed in to change notification settings - Fork 497
Fix/terminal background color detection #2461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix/terminal background color detection #2461
Conversation
The Preview block's CodeEditPreview component was not passing file information to Monaco editor, preventing automatic language detection from file extensions. Changes: - Add fileName prop to CodeEditor component - Pass actual file path to Monaco instead of random UUID - Monaco now auto-detects language from file extension (.js, .py, .ts, etc.) Fixes syntax highlighting for all text files opened in Preview blocks while maintaining backward compatibility with existing CodeEditor usage.
Terminal applications like Neovim query the terminal's background colorto determine which color scheme to use for syntax highlighting. Previously, the background color in the XTerm.js theme was being set to transparent (#00000000), which caused these applications to fail to detect the actual background color. This fix preserves the actual background color in the XTerm.js theme while maintaining the transparency feature, which is applied via the applyTransparencyToColor() function. Fixes syntax highlighting in Neovim and other terminal applications that rely on background color detection.
WalkthroughThis pull request enhances the CodeEditor component with an optional Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes involve minor prop additions, straightforward variable assignments, and simple logic adjustments across three files. The edits follow consistent patterns—adding optional props and adjusting background preservation logic—with no complex control flow or dense logic. File spread is moderate, but individual changes in each file require only localized reasoning about correctness. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/app/view/preview/preview-edit.tsx (1)
19-19: Consider handling empty string edge case.If
fileInfo.pathandfileInfo.nameboth exist but are empty strings,fileNamewill be"". An empty string path might cause unexpected behavior in Monaco's model management.Consider a more defensive approach:
- const fileName = fileInfo?.path || fileInfo?.name; + const fileName = fileInfo?.path || fileInfo?.name || undefined;This ensures
fileNameis either a non-empty string orundefined, allowing CodeEditor's fallback to generate a proper random UUID.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/view/codeeditor/codeeditor.tsx(1 hunks)frontend/app/view/preview/preview-edit.tsx(2 hunks)frontend/app/view/term/termutil.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/view/codeeditor/codeeditor.tsx (2)
frontend/app/store/global.ts (1)
useOverrideConfigAtom(858-858)frontend/util/util.ts (1)
boundNumber(411-411)
🔇 Additional comments (2)
frontend/app/view/term/termutil.ts (1)
32-34: LGTM! Background color preservation implemented correctly.The removal of the forced transparent override allows terminal applications like Neovim to properly detect the background color for syntax highlighting, while still applying user-configured transparency through
applyTransparencyToColor(). The comment clearly documents the intent.frontend/app/view/codeeditor/codeeditor.tsx (1)
111-111: LGTM! Optional fileName prop added.The addition of the optional
fileNameprop enables file-path-aware editor initialization, improving Monaco's model management.
| const fontSize = boundNumber(useOverrideConfigAtom(blockId, "editor:fontsize"), 6, 64); | ||
| const theme = "wave-theme-dark"; | ||
| const editorPath = useRef(crypto.randomUUID()).current; | ||
| const editorPath = useRef(fileName ?? crypto.randomUUID()).current; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: editorPath doesn't update when fileName prop changes.
The current implementation uses useRef which only evaluates its initial value on the first render. If the fileName prop changes on subsequent renders, editorPath will retain its original value, causing Monaco Editor to use the wrong model identity.
Scenario demonstrating the bug:
- Component mounts with
fileName="file-a.txt"→editorPath="file-a.txt" - Props update to
fileName="file-b.txt"→editorPathstill"file-a.txt"❌
This results in Monaco using the model for file A when editing file B.
Apply this diff to make editorPath reactive to fileName changes while maintaining a stable UUID fallback:
+ const fallbackPath = useRef(crypto.randomUUID()).current;
- const editorPath = useRef(fileName ?? crypto.randomUUID()).current;
+ const editorPath = fileName ?? fallbackPath;This ensures:
- When
fileNameis provided,editorPathupdates immediately - When
fileNameisundefined, a stable random UUID is used across renders
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const editorPath = useRef(fileName ?? crypto.randomUUID()).current; | |
| const fallbackPath = useRef(crypto.randomUUID()).current; | |
| const editorPath = fileName ?? fallbackPath; |
🤖 Prompt for AI Agents
In frontend/app/view/codeeditor/codeeditor.tsx at line 124, replace the
useRef-based initialization so editorPath reacts to changes in the fileName prop
while keeping a stable UUID fallback: create a persistent fallback ID using
useRef at component init (e.g., assign crypto.randomUUID() once into a ref) and
then derive editorPath on every render as fileName ?? fallbackRef.current,
ensuring fileName updates immediately and the generated UUID remains stable when
fileName is undefined.
| <CodeEditor | ||
| blockId={model.blockId} | ||
| text={fileContent} | ||
| fileName={fileName} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify fileName prop integration with CodeEditor.
The fileName prop is correctly passed to CodeEditor, enabling file-path-aware editor initialization.
Confirm that the CodeEditor component properly handles the case when fileName is undefined or changes between renders:
🏁 Script executed:
#!/bin/bash
# Verify CodeEditor's handling of fileName prop and editorPath stability
# Check how editorPath is used and whether it updates when fileName changes
ast-grep --pattern $'const editorPath = $$$'
# Search for Monaco Editor path prop usage
rg -nP --type=tsx --type=ts -C3 '\bpath\s*=\s*\{?\s*editorPath'Length of output: 147
I found that editorPath is defined in CodeEditor, but the search had some issues. Let me refine the verification with better commands.
🏁 Script executed:
#!/bin/bash
# Examine the CodeEditor component to understand fileName prop and editorPath handling
# View the CodeEditor component around the editorPath definition
echo "=== CodeEditor component (lines 110-150) ==="
sed -n '110,150p' frontend/app/view/codeeditor/codeeditor.tsx
# Search for all uses of editorPath within CodeEditor to see if it updates reactively
echo -e "\n=== All editorPath usages in CodeEditor ==="
grep -n "editorPath" frontend/app/view/codeeditor/codeeditor.tsx
# Check if fileName appears in dependency arrays
echo -e "\n=== useEffect/useCallback dependency arrays with fileName or editorPath ==="
grep -n -B2 -A2 "fileName\|editorPath" frontend/app/view/codeeditor/codeeditor.tsx | grep -A2 -B2 "\[.*\]"
# View the preview-edit.tsx file around line 69 to see how fileName is passed
echo -e "\n=== preview-edit.tsx context (lines 60-80) ==="
sed -n '60,80p' frontend/app/view/preview/preview-edit.tsxLength of output: 2286
I found a critical issue. Let me verify how fileName changes over time:
🏁 Script executed:
#!/bin/bash
# Check if fileName changes dynamically in preview-edit.tsx
echo "=== Full preview-edit.tsx to understand fileName lifecycle ==="
cat frontend/app/view/preview/preview-edit.tsx | grep -n "fileName" -B2 -A2
echo -e "\n=== Check for fileName state updates ==="
grep -n "setFileName\|fileName.*=" frontend/app/view/preview/preview-edit.tsxLength of output: 667
I've identified a potential bug. Let me check if fileInfo can change after the component mounts:
🏁 Script executed:
#!/bin/bash
# Search for updates to model.statFile atom to determine if fileInfo can change
echo "=== Check atom updates and fileInfo reactivity ==="
rg -n "statFile" frontend/app/view/preview/preview-edit.tsx
echo -e "\n=== Search broader codebase for statFile mutations ==="
rg -n "statFile.*=" --type ts --type tsx -C2 | head -40
echo -e "\n=== Verify if editorPath should be reactive ==="
# Check if Monaco editor's path prop expects to update dynamically
grep -n "path=" frontend/app/view/codeeditor/codeeditor.tsx -B3 -A3Length of output: 522
Fix editorPath reactivity: editorPath should update when fileName changes.
The current implementation freezes editorPath on mount due to the .current extraction in the initial value assignment. When fileName changes (via fileInfo atom updates in preview-edit.tsx), the CodeEditor component re-renders with a new prop, but editorPath remains stale because it's locked in a useRef.
Issue location: frontend/app/view/codeeditor/codeeditor.tsx:124
The fix should ensure editorPath updates reactively. Replace:
const editorPath = useRef(fileName ?? crypto.randomUUID()).current;With a pattern that maintains the UUID fallback while allowing updates:
const editorPathRef = useRef(fileName ?? crypto.randomUUID());
const editorPath = editorPathRef.current;Or if the path should update with fileName changes:
const editorPath = fileName ?? useRef(crypto.randomUUID()).current;🤖 Prompt for AI Agents
In frontend/app/view/codeeditor/codeeditor.tsx around line 124, editorPath is
currently frozen by assigning useRef(...).current on init, so it never updates
when fileName changes; replace that pattern with a reactive approach — either
keep a ref that stores a UUID fallback but read its .current separately (i.e.
create editorPathRef = useRef(fileName ?? crypto.randomUUID()) and then use
editorPathRef.current), or make editorPath derive from fileName with a ref only
for the UUID fallback (i.e. editorPath = fileName ??
useRef(crypto.randomUUID()).current) so editorPath will update when fileName
changes. Ensure you only change the initialization pattern and use the new
editorPath value everywhere the component expects it.
Terminal applications like Neovim query the terminal's background colorto determine which color scheme to use for syntax highlighting.
Previously, the background color in the XTerm.js theme was being set to transparent (#00000000), which caused these applications to fail to detect the actual background color.
This fix preserves the actual background color in the XTerm.js theme while maintaining the transparency feature, which is applied via the applyTransparencyToColor() function.
Fixes syntax highlighting in Neovim and other terminal applications that rely on background color detection.