feat(desktop): color dock icon and label per workspace#1478
Conversation
Adds workspace-specific dock icon coloring and labeling so developers can visually distinguish multiple Electron dev instances running from different worktrees. - Add dock-icon.ts: draws a colored rounded-rect border on the app icon using an SDF approach, with hue derived from the workspace name - Rename Electron.app to "Superset (<workspace>).app" and update path.txt so macOS resolves the correct bundle name for the dock label - Patch CFBundleName/CFBundleDisplayName in the dev Electron plist - Set window title to include workspace name
📝 WalkthroughWalkthroughAdds macOS workspace-aware behavior: patches Electron .app and Info.plist at runtime for workspace-specific bundle metadata and path, renders a deterministic workspace-colored border onto the macOS dock icon in development, and shows workspace names in window titles (dev-only). Changes
Sequence Diagram(s)sequenceDiagram
actor App as Electron App (macOS)
participant Workspace as Workspace Configuration
participant FS as File System / Icon Resource
participant Renderer as Bitmap Renderer
participant Dock as macOS Dock Display
App->>Workspace: request workspace name
Workspace-->>App: workspace name
App->>App: hashToHue(workspace name)
App->>FS: load app icon bitmap
FS-->>App: bitmap (pixels, width, height)
App->>Renderer: compute content bounds & border params
Renderer-->>App: modified bitmap with colored border
App->>Dock: app.dock.setIcon(modified bitmap)
Dock-->>Dock: update dock icon display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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
🤖 Fix all issues with AI agents
In `@apps/desktop/scripts/patch-dev-protocol.ts`:
- Around line 186-194: The code can create a dangling symlink when oldTargetPath
is missing; modify the block around unlinkSync/renameSync/symlinkSync so you
only create the symlink when the target directory exists (or create the
directory first). Specifically, in the else branch handling a workspace-name
change, after computing oldTargetPath and before calling
symlinkSync(ELECTRON_APP_PATH,...), check existsSync(desiredAppPath) (or call
mkdirSync(desiredAppPath, { recursive: true }) to ensure it exists) and only
then call symlinkSync(DESIRED_APP_NAME, ELECTRON_APP_PATH) and set actualAppPath
= desiredAppPath; otherwise skip creating the symlink (or fail early) to avoid
leaving a dangling link.
🧹 Nitpick comments (5)
apps/desktop/scripts/patch-dev-protocol.ts (3)
130-147: Shell-injection risk from workspace name interpolated into shell commands.
DISPLAY_NAME(which containsworkspaceName) is interpolated directly intoexecSynccommand strings. If a workspace name ever contains characters like",$, or backticks, PlistBuddy commands will break or behave unexpectedly.Workspace names today are likely safe (alphanumeric + hyphens), but this is fragile. Consider sanitizing or at least validating the workspace name before interpolation.
🛡️ Suggested defensive guard (near line 68)
const PROTOCOL_SCHEME = `superset-${workspaceName}`; + +if (!/^[\w-]+$/.test(workspaceName)) { + console.warn(`[patch-dev-protocol] Unsafe workspace name "${workspaceName}", skipping`); + process.exit(0); +} + const BUNDLE_ID = `com.superset.desktop.${workspaceName}`;
98-112: DRY:electronPkgDir/electronPkgCheckis resolved twice.Lines 102-106 compute
electronPkgCheckandpathTxtCheck, and Lines 230-232 compute the same paths again aselectronPkgDirandpathTxtPath. Consider hoisting these to module-level constants alongsideELECTRON_DIST_DIR.Also applies to: 230-232
139-147: Emptycatch {}on PlistBuddy delete silently swallows all errors.Line 144's empty catch is intentional for the "key doesn't exist" case, but it also hides unexpected failures (e.g., plist corruption, permission errors). A brief log at
warnlevel would help debugging.Suggested improvement
try { execSync( `/usr/libexec/PlistBuddy -c "Delete :CFBundleDisplayName" "${PLIST_PATH}" 2>/dev/null`, ); -} catch {} +} catch { + // Expected when key doesn't exist yet +}apps/desktop/src/main/lib/dock-icon.ts (2)
226-232: Extract magic numbers for border styling to named constants.Lines 227, 231, and 232 use inline magic numbers for saturation (75), lightness (55), border thickness ratio (0.038), and corner radius ratio (0.22). Per coding guidelines, these should be named constants at module top for clarity and easy tuning.
Suggested extraction
+const BORDER_SATURATION = 75; +const BORDER_LIGHTNESS = 55; +const BORDER_THICKNESS_RATIO = 0.038; +const BORDER_CORNER_RADIUS_RATIO = 0.22; + export function setWorkspaceDockIcon(): void {Then reference them in the function body:
- const rgb = hslToRgb(hue, 75, 55); + const rgb = hslToRgb(hue, BORDER_SATURATION, BORDER_LIGHTNESS); const bounds = findContentBounds(bitmap, size.width, size.height); - const thickness = Math.round(size.width * 0.038); - const cornerRadius = Math.round(size.width * 0.22); + const thickness = Math.round(size.width * BORDER_THICKNESS_RATIO); + const cornerRadius = Math.round(size.width * BORDER_CORNER_RADIUS_RATIO);As per coding guidelines: "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic."
107-129: Edge case:findContentBoundsreturns inverted bounds when the icon is fully transparent.If every pixel has alpha ≤ 10, the returned bounds will be
{ top: height, left: width, bottom: 0, right: 0 }. ThedrawBorderloop is safe (it won't iterate), but the caller has no signal that the bounds are invalid. Sinceicon.isEmpty()is checked earlier insetWorkspaceDockIcon, this is unlikely to be hit in practice.
| } else { | ||
| // Different workspace name from previous run — update | ||
| const oldTargetPath = resolve(ELECTRON_DIST_DIR, currentTarget); | ||
| unlinkSync(ELECTRON_APP_PATH); | ||
| if (existsSync(oldTargetPath)) { | ||
| renameSync(oldTargetPath, desiredAppPath); | ||
| } | ||
| symlinkSync(DESIRED_APP_NAME, ELECTRON_APP_PATH); | ||
| actualAppPath = desiredAppPath; |
There was a problem hiding this comment.
Dangling symlink when the previous workspace's app directory is missing.
If oldTargetPath doesn't exist (Line 190 is false), the renameSync is skipped but symlinkSync on Line 193 still creates a symlink pointing to DESIRED_APP_NAME — which won't exist either. This leaves a dangling symlink and the rest of the script (lsregister, path.txt) will operate on a nonexistent path.
🛡️ Suggested fix
const oldTargetPath = resolve(ELECTRON_DIST_DIR, currentTarget);
unlinkSync(ELECTRON_APP_PATH);
if (existsSync(oldTargetPath)) {
renameSync(oldTargetPath, desiredAppPath);
+ } else if (!existsSync(desiredAppPath)) {
+ console.warn("[patch-dev-protocol] Previous Electron.app not found and desired target missing — re-install electron");
+ process.exit(1);
}
symlinkSync(DESIRED_APP_NAME, ELECTRON_APP_PATH);📝 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.
| } else { | |
| // Different workspace name from previous run — update | |
| const oldTargetPath = resolve(ELECTRON_DIST_DIR, currentTarget); | |
| unlinkSync(ELECTRON_APP_PATH); | |
| if (existsSync(oldTargetPath)) { | |
| renameSync(oldTargetPath, desiredAppPath); | |
| } | |
| symlinkSync(DESIRED_APP_NAME, ELECTRON_APP_PATH); | |
| actualAppPath = desiredAppPath; | |
| } else { | |
| // Different workspace name from previous run — update | |
| const oldTargetPath = resolve(ELECTRON_DIST_DIR, currentTarget); | |
| unlinkSync(ELECTRON_APP_PATH); | |
| if (existsSync(oldTargetPath)) { | |
| renameSync(oldTargetPath, desiredAppPath); | |
| } else if (!existsSync(desiredAppPath)) { | |
| console.warn("[patch-dev-protocol] Previous Electron.app not found and desired target missing — re-install electron"); | |
| process.exit(1); | |
| } | |
| symlinkSync(DESIRED_APP_NAME, ELECTRON_APP_PATH); | |
| actualAppPath = desiredAppPath; |
🤖 Prompt for AI Agents
In `@apps/desktop/scripts/patch-dev-protocol.ts` around lines 186 - 194, The code
can create a dangling symlink when oldTargetPath is missing; modify the block
around unlinkSync/renameSync/symlinkSync so you only create the symlink when the
target directory exists (or create the directory first). Specifically, in the
else branch handling a workspace-name change, after computing oldTargetPath and
before calling symlinkSync(ELECTRON_APP_PATH,...), check
existsSync(desiredAppPath) (or call mkdirSync(desiredAppPath, { recursive: true
}) to ensure it exists) and only then call symlinkSync(DESIRED_APP_NAME,
ELECTRON_APP_PATH) and set actualAppPath = desiredAppPath; otherwise skip
creating the symlink (or fail early) to avoid leaving a dangling link.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Superset (<workspace>).appwith symlink + path.txt update so macOS shows the correct dock label (e.g. "Superset (color-electron)")CFBundleName/CFBundleDisplayNamein the dev Electron plist and registers with LaunchServicesTest plan
bun devfrom a worktree and verify the dock icon has a colored borderbun devfrom a second worktree and verify it gets a different border colorbun installthenbun devand verify everything still works (handles fresh Electron.app)Summary by CodeRabbit
New Features
Improvements