[FEAT] Add Light theme inspired by MergeNB logo#86
Conversation
Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
Check this out. Some notes: Have the MergeNB logo match the old html <title>MergeNB Logo - Fanned Out</title>then, make sure the cell boundaries/padding (which are currently white) is removed/ just make everything transparent or opaque/viewed against the background grid canvas. Also, the whole thing about making text from unified cells greyer than conflicted/unmapped is odd. Let's get rid of that and instead just make it cleaerer (somehow) that a cell is conflicted. Overall though, good start! |
Co-authored-by: Avni2000 <77120766+Avni2000@users.noreply.github.com>
Done! Changes in commit 251d6cb: ✅ Logo: Added fanned-out cards matching the HTML design - soft cyan (#A4D4DE) and purple (#C3C9F2) with 20° rotation
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
- rename elegant -> light
- Logo changes - Textarea fixes - Align dark with light as a reference point
|
@coderabbitai everything looks good on my end, but could I get a review? |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces dark/light theme support by adding a configuration option that flows from VS Code settings through backend data structures and web server sessions to the frontend web UI, with corresponding CSS refactoring to enable dynamic theme injection. Changes
Sequence DiagramsequenceDiagram
participant VSCode as VS Code
participant Settings as Settings Module
participant Resolver as Conflict Resolver
participant WebServer as Web Server
participant Browser as Browser
participant ReactApp as React App
participant Styles as Styles Module
VSCode->>Settings: User configures theme
Settings->>Settings: getSettings() reads ui.theme
Settings-->>Resolver: Settings with theme
Resolver->>Resolver: Add theme to UnifiedConflict
Resolver-->>WebServer: Pass conflict with theme
WebServer->>WebServer: Store theme in SessionData
WebServer->>Styles: getHtmlShell(theme)
Styles-->>WebServer: HTML with theme variables
WebServer-->>Browser: Serve HTML shell
Browser->>Browser: Extract __MERGENB_INITIAL_THEME
Browser->>ReactApp: Load with initialTheme
ReactApp->>ReactApp: Receive conflictData.theme
ReactApp->>Styles: injectStyles(conflictData.theme)
Styles->>Styles: getStyles(theme) builds CSS
Styles-->>Browser: Apply theme-specific CSS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/web/client/styles.ts (1)
308-309:⚠️ Potential issue | 🟡 MinorHardcoded dark-theme hover colors break the light theme.
Several hover states use hardcoded dark colors (
#3a3a3a,#3c3c3c) that will look wrong against the light palette. These should use theme-aware CSS variables or computed values.Affected selectors:
.history-item:hover(Line 309):background:#3a3a3a``.btn-secondary:hover(Line 352):background:#3c3c3c``♻️ Suggested approach
.history-item:hover { - background: `#3a3a3a`; + background: var(--bg-tertiary); }.btn-secondary:hover:not(:disabled) { - background: `#3c3c3c`; + background: var(--bg-primary); }Also applies to: 351-353
🤖 Fix all issues with AI agents
In `@src/web/client/styles.ts`:
- Line 84: The CSS currently imports Google Fonts via the external line "@import
url('https://fonts.googleapis.com/...')" which leaks user data and fails
offline; replace that external import by bundling the font files into the
extension (e.g., dist/web/fonts) and reference them via local `@font-face`
declarations in src/web/client/styles.ts (or fall back to system fonts like
sans-serif/serif when fonts aren't available), and update your build
(webpack/asset pipeline or copy step) and extension packaging so the font files
are included and served locally instead of calling fonts.googleapis.com.
- Around line 401-404: The .merge-row.conflict-row selector is defined twice
with conflicting border/background declarations causing mixed borders (3px on
three sides and 4px on the left); fix by consolidating into a single rule for
.merge-row.conflict-row (or remove the earlier/duplicate rule) and ensure you
only use the intended properties (either a full border shorthand OR explicit
border-left/top/right/bottom) so no partial overrides occur; update the rule
that currently sets border-left and background (the second occurrence) to be the
single source of truth or merge its values into the first occurrence so the
selector has one consistent border and background.
In `@src/web/webServer.ts`:
- Around line 481-483: The theme string is interpolated directly into the HTML
script (window.__MERGENB_INITIAL_THEME = '${theme}';) without runtime
validation; add a small validation step (e.g., a sanitizeTheme or normalizeTheme
function) that checks the value returned by config.get() (the theme variable)
and clamps it to either 'dark' or 'light' (default to 'light' on invalid input),
then embed that validated value into the script; to avoid injection entirely,
embed the validated value using a safe serializer (e.g., via JSON.stringify on
the final value) when writing window.__MERGENB_INITIAL_THEME so only a
guaranteed 'dark'|'light' string can be interpolated.
🧹 Nitpick comments (1)
src/web/webServer.ts (1)
337-354: 404 page uses hardcoded dark theme, inconsistent with the new theming system.The "Session Not Found" error page always renders with a dark background (
#1e1e1e), which will look jarring if the user has selected the light theme. Consider using the same theme-aware approach, or at minimum a neutral style.
src/web/client/styles.ts
Outdated
| const hasBackgroundImage = colors.bodyBackgroundImage !== 'none'; | ||
|
|
||
| return ` | ||
| @import url('https://fonts.googleapis.com/css2?family=DM+Sans:wght@700&family=Playfair+Display:ital,wght@1,500&display=swap'); |
There was a problem hiding this comment.
External Google Fonts request leaks user data and breaks offline usage.
Loading fonts from fonts.googleapis.com inside a VS Code extension's UI means every user's browser makes a request to Google's servers, leaking IP and user-agent. It also fails when offline. Consider bundling the fonts locally in dist/web/ or falling back to system fonts.
🤖 Prompt for AI Agents
In `@src/web/client/styles.ts` at line 84, The CSS currently imports Google Fonts
via the external line "@import url('https://fonts.googleapis.com/...')" which
leaks user data and fails offline; replace that external import by bundling the
font files into the extension (e.g., dist/web/fonts) and reference them via
local `@font-face` declarations in src/web/client/styles.ts (or fall back to
system fonts like sans-serif/serif when fonts aren't available), and update your
build (webpack/asset pipeline or copy step) and extension packaging so the font
files are included and served locally instead of calling fonts.googleapis.com.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for webpack or build configuration files
echo "=== Build configuration files ==="
fd -t f 'webpack|esbuild|rollup|tsconfig' --max-depth 2
echo -e "\n=== Package.json scripts ==="
cat package.json | jq '.scripts'
echo -e "\n=== Check for existing asset/font directories ==="
fd -t d 'assets|fonts|static|public' --max-depth 3
echo -e "\n=== Check how web assets are currently served ==="
rg -n "express.static|serveStatic|\.use.*static" --type ts
echo -e "\n=== Check dist or build output structure ==="
fd -t d 'dist|out|build' --max-depth 2
Length of output: 1469
🏁 Script executed:
#!/bin/bash
# Check esbuild configuration
echo "=== esbuild.js configuration ==="
cat esbuild.js
echo -e "\n=== serveStaticFile implementation ==="
rg -A 30 "private serveStaticFile" src/web/webServer.ts
echo -e "\n=== Check current web client structure ==="
tree src/web/client -L 2 || find src/web/client -maxdepth 2 -type f
echo -e "\n=== Check if there's any existing asset copying in build ==="
rg -n "copy|asset|static" esbuild.js
Length of output: 4909
@Avni2000, great question! Your project already has the perfect pattern set up with KaTeX. Here's how to bundle the fonts locally:
Step-by-step guide:
1. Download the fonts
Visit Google Fonts and download both fonts:
- DM Sans - select weight 700
- Playfair Display - select italic, weight 500
Extract the .woff2 files (modern browsers only need woff2) and create this structure:
src/web/client/fonts/
├── DMSans-Bold.woff2
└── PlayfairDisplay-MediumItalic.woff2
2. Update esbuild.js to copy fonts
Add a copyFonts() function right after copyKatex():
/**
* Copy fonts to dist/web directory
*/
function copyFonts() {
const srcDir = path.join(__dirname, 'src', 'web', 'client', 'fonts');
const destDir = path.join(__dirname, 'dist', 'web', 'fonts');
if (!fs.existsSync(destDir)) {
fs.mkdirSync(destDir, { recursive: true });
}
if (fs.existsSync(srcDir)) {
copyDir(srcDir, destDir);
console.log('[fonts] Copied font files');
} else {
console.warn(`[fonts] Warning: ${srcDir} not found`);
}
}Then call it in the main() function after the KaTeX copy:
// Copy KaTeX after build
copyKatex();
copyFonts(); // Add this line3. Update styles.ts with local @font-face
Replace the external @import line (line 84 in the diff) with local font declarations:
return `
`@font-face` {
font-family: 'DM Sans';
font-style: normal;
font-weight: 700;
font-display: swap;
src: url('/fonts/DMSans-Bold.woff2') format('woff2');
}
`@font-face` {
font-family: 'Playfair Display';
font-style: italic;
font-weight: 500;
font-display: swap;
src: url('/fonts/PlayfairDisplay-MediumItalic.woff2') format('woff2');
}
:root {
--bg-primary: ${colors.bgPrimary};
// ... rest of your CSS
`;4. Update web server content types (if needed)
Check if src/web/webServer.ts includes .woff2 in its contentTypes mapping. If not, add:
const contentTypes: Record<string, string> = {
'.js': 'application/javascript',
'.woff2': 'font/woff2', // Add this
// ... other types
};5. Build and test
npm run compileThe fonts will now load locally, work offline, and won't leak data to Google! 🎉
🧠 Learnings used
Learnt from: CR
Repo: Avni2000/MergeNB PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T02:45:32.680Z
Learning: Applies to src/webview/**/*.{ts,tsx} : Provide a unified conflict resolution UI in `ConflictResolverPanel` webview supporting both textual and semantic conflict resolution with 3-way diff view
Learnt from: CR
Repo: Avni2000/MergeNB PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-10T19:24:17.581Z
Learning: Applies to **/web/client/**/*.{ts,tsx} : Develop the conflict resolution UI using React in the `web/client/` directory
Learnt from: CR
Repo: Avni2000/MergeNB PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-10T19:24:17.581Z
Learning: Applies to **/diffUtils.ts : Implement LCS-based text diffing with inline change detection in `diffUtils.ts`
There was a problem hiding this comment.
Did not know you could bundle fonts - thanks!
There was a problem hiding this comment.
@Avni2000 You're welcome! Happy to help. Bundling fonts locally is a great practice for VS Code extensions—it keeps everything self-contained, works offline, and respects user privacy. Feel free to reach out if you have any questions while implementing it! 😊
🧠 Learnings used
Learnt from: CR
Repo: Avni2000/MergeNB PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T02:45:32.680Z
Learning: Applies to src/webview/**/*.{ts,tsx} : Provide a unified conflict resolution UI in `ConflictResolverPanel` webview supporting both textual and semantic conflict resolution with 3-way diff view
Learnt from: CR
Repo: Avni2000/MergeNB PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-10T19:24:17.581Z
Learning: Applies to **/web/client/**/*.{ts,tsx} : Develop the conflict resolution UI using React in the `web/client/` directory
Learnt from: CR
Repo: Avni2000/MergeNB PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-10T19:24:17.581Z
Learning: Applies to **/diffUtils.ts : Implement LCS-based text diffing with inline change detection in `diffUtils.ts`
| .merge-row.conflict-row { | ||
| border: 2px solid var(--border-color); | ||
| background: var(--bg-secondary); | ||
| border: 3px solid rgba(244, 135, 113, 0.6); | ||
| background: rgba(244, 135, 113, 0.08); | ||
| } |
There was a problem hiding this comment.
Duplicate .merge-row.conflict-row selectors with conflicting styles.
This selector is defined at Line 401 (with a 3px solid border all around + salmon tint) and again at Line 1040 (with a 4px left-only border + different background). Due to CSS cascade, the second definition's border-left and background override the first, but the first definition's border shorthand still partially applies to top/right/bottom — resulting in a 3px salmon border on three sides and a 4px #f48771 border on the left.
This is likely unintentional. Consolidate into a single rule or ensure one set is clearly scoped/overridden.
Also applies to: 1039-1044
🤖 Prompt for AI Agents
In `@src/web/client/styles.ts` around lines 401 - 404, The .merge-row.conflict-row
selector is defined twice with conflicting border/background declarations
causing mixed borders (3px on three sides and 4px on the left); fix by
consolidating into a single rule for .merge-row.conflict-row (or remove the
earlier/duplicate rule) and ensure you only use the intended properties (either
a full border shorthand OR explicit border-left/top/right/bottom) so no partial
overrides occur; update the rule that currently sets border-left and background
(the second occurrence) to be the single source of truth or merge its values
into the first occurrence so the selector has one consistent border and
background.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Avni2000
changed the title
Implements a light theme with beige color palette and checkered background pattern matching the MergeNB logo aesthetic. Theme is configurable via VS Code settings.
Changes
Settings & Configuration
mergeNB.ui.themesetting with enum values:"elegant"(default),"dark"MergeNBSettingsinterface with theme fieldsettings.ts→resolver.ts→WebConflictPanel.ts→ clientTheme System
getStyles(theme)function with conditional color palettes#F9F7F1background with 20px grid,#A4D4DE(cyan) for current,#C3C9F2(purple) for incomingApp.tsxinjects theme styles reactively viauseEffectwhen conflict data loadsLogo Icon
Transparent Cells
Improved Conflict Visibility
Type Safety
UnifiedConflict,WebConflictData,UnifiedConflictDatainterfacesisAutoResolveEnabled()to accept only auto-resolve setting keys (not UI settings)Example
Users change theme via VS Code settings:
{ "mergeNB.ui.theme": "elegant" // or "dark" }Dynamic style injection in client:
Screenshot
Original prompt
Created from VS Code.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit