Move magnifier to external package, hue-hunter#2509
Conversation
📝 WalkthroughWalkthroughThis PR removes the Rust-based magnifier/pixel-sampler subsystem and associated magnifier UI, tests, and build wiring, and replaces the in-app picker launcher with a hue-hunter-based color picker integration. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant MB as Menubar (main)
participant Picker as hue-hunter ColorPicker
participant IPC as Electron IPC / Renderer
MB->>MB: hide menubar window
MB->>Picker: launch color picker
Picker-->>MB: returns selected color or cancel
MB->>IPC: send picked color via IPC (changeColor / pickContrast*)
MB->>MB: show menubar window
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
27-49: Align Wayland requirements with the new “no external tools” note.The install block still recommends
grim, which conflicts with the new Wayland note saying no external tools are needed. Consider removing those lines to avoid confusion.📝 Suggested doc tweak
# Debian / Ubuntu sudo apt-get install libxcb1 libxrandr2 libdbus-1-3 -# For Wayland (recommended) -sudo apt-get install grim - # For X11 (fallback) sudo apt-get install imagemagick xdotool # OR sudo apt-get install scrot xdotool @@ # Alpine -sudo apk add libxcb libxrandr dbus grim +sudo apk add libxcb libxrandr dbus
🤖 Fix all issues with AI agents
In `@electron-app/src/color-picker.ts`:
- Around line 14-18: The colorNameFn currently assumes getColorName(rgb).name
exists; guard against getColorName returning a string by checking the return
value from getColorName(rgb) inside colorNameFn (e.g., test typeof result ===
'object' && 'name' in result) and return the object.name when present, otherwise
fall back to a sensible string (the returned string/hex or a formatted rgb/empty
string). Update the ColorPicker instantiation (colorNameFn) to perform this
check so undefined names are avoided.
In `@WARP.md`:
- Line 124: Replace bare URLs in the WARP.md text (e.g., the line that currently
reads "Uses the `hue-hunter` npm package
(https://github.com/RobbieTheWagner/hue-hunter)") with a markdown link or
angle-bracketed URL; for example change the parenthetical raw URL to either
"[hue-hunter](https://github.com/RobbieTheWagner/hue-hunter)" or
"<https://github.com/RobbieTheWagner/hue-hunter>". Also update the other
occurrence noted (the similar bare URL later in the file) the same way so all
raw URLs conform to MD034.
| // Create ColorPicker instance with color naming | ||
| const picker = new ColorPicker({ | ||
| colorNameFn: (rgb) => getColorName(rgb).name, | ||
| initialDiameter: 180, | ||
| initialSquareSize: 20, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E 'color-picker|color-picker.ts'Repository: shipshapecode/swach
Length of output: 165
🏁 Script executed:
fd 'color-picker.ts' -t fRepository: shipshapecode/swach
Length of output: 95
🏁 Script executed:
ls -la | head -20
fd 'electron-app' -t dRepository: shipshapecode/swach
Length of output: 1305
🏁 Script executed:
git ls-files | grep '\.ts$' | head -30Repository: shipshapecode/swach
Length of output: 903
🏁 Script executed:
find . -maxdepth 3 -type d | head -30Repository: shipshapecode/swach
Length of output: 552
🏁 Script executed:
git ls-files | head -50Repository: shipshapecode/swach
Length of output: 1285
🏁 Script executed:
git ls-files | grep -i colorRepository: shipshapecode/swach
Length of output: 816
🏁 Script executed:
rg 'getColorName' -t ts -t tsx -t js -t jsxRepository: shipshapecode/swach
Length of output: 90
🏁 Script executed:
cat -n electron-app/src/color-picker.tsRepository: shipshapecode/swach
Length of output: 2297
🏁 Script executed:
rg 'getColorName' -t ts -A 10Repository: shipshapecode/swach
Length of output: 1063
🏁 Script executed:
cat -n types/nearest-color/index.d.tsRepository: shipshapecode/swach
Length of output: 238
🏁 Script executed:
rg 'nearest-color' package.json -A 2Repository: shipshapecode/swach
Length of output: 151
🌐 Web query:
nearest-color npm package documentation return type from method
💡 Result:
If you call from(colors) (or nearestFrom for the TypeScript variant) you get a function that, when passed a color, returns an object with this shape:
{ name: string, value: string (hex), rgb: { r: number, g: number, b: number }, distance: number }
(If you request multiple nearest colors the TypeScript/nearest-colors variant can return an array of such objects.) [1][2]
Sources:
- nearest-color (npm README). [1]
- nearest-colors (TypeScript variant README). [2]
Guard against getColorName returning a non-object value.
The nearest-color library's type definition indicates getColorName() can return either a ColorMatch object or a string. If it returns a string (hex value), accessing .name will return undefined rather than a color name string. The ColorPicker may behave unexpectedly or fail silently with undefined names.
🛡️ Suggested guard
-const picker = new ColorPicker({
- colorNameFn: (rgb) => getColorName(rgb).name,
+const picker = new ColorPicker({
+ colorNameFn: (rgb) => {
+ const match = getColorName(rgb as { r: number; g: number; b: number });
+ return match?.name ?? 'Unknown';
+ },
initialDiameter: 180,
initialSquareSize: 20,
});📝 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.
| // Create ColorPicker instance with color naming | |
| const picker = new ColorPicker({ | |
| colorNameFn: (rgb) => getColorName(rgb).name, | |
| initialDiameter: 180, | |
| initialSquareSize: 20, | |
| // Create ColorPicker instance with color naming | |
| const picker = new ColorPicker({ | |
| colorNameFn: (rgb) => { | |
| const match = getColorName(rgb as { r: number; g: number; b: number }); | |
| return match?.name ?? 'Unknown'; | |
| }, | |
| initialDiameter: 180, | |
| initialSquareSize: 20, |
🤖 Prompt for AI Agents
In `@electron-app/src/color-picker.ts` around lines 14 - 18, The colorNameFn
currently assumes getColorName(rgb).name exists; guard against getColorName
returning a string by checking the return value from getColorName(rgb) inside
colorNameFn (e.g., test typeof result === 'object' && 'name' in result) and
return the object.name when present, otherwise fall back to a sensible string
(the returned string/hex or a formatted rgb/empty string). Update the
ColorPicker instantiation (colorNameFn) to perform this check so undefined names
are avoided.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/ember.yml:
- Line 16: Remove the obsolete Rust toolchain action by deleting the workflow
step that contains the string "uses: dtolnay/rust-toolchain@stable"; this line
should be removed from all CI workflow YAMLs (ember.yml, electron.yml, lint.yml,
release.yml) so the workflows no longer invoke the Rust toolchain action and
avoid unnecessary CI overhead.
In @.github/workflows/release.yml:
- Line 15: Remove the unnecessary Rust toolchain setup steps by deleting any
workflow steps that use "uses: dtolnay/rust-toolchain@stable" (both occurrences
referenced in the diff) from the release workflow; ensure only Node.js/pnpm
setup steps remain (do not add new Rust-related actions or files), and verify
the job continues to run with the existing Node/pnpm steps and environment
variables unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
170-170:nearest-colorshould be independencies, notdevDependencies.The
nearest-colorpackage is imported in bothelectron-app/src/color-picker.ts(Electron main process) andapp/services/nearest-color.ts(Ember app service), which are production code. Placing it indevDependenciesmay cause the packaged application to fail if the bundler doesn't include dev dependencies.🐛 Proposed fix
Move
nearest-colorfromdevDependenciestodependencies:"color-name-list": "^14.27.0", "decorator-transforms": "^2.3.1", + "nearest-color": "^0.4.4", "electron-dl": "^4.0.0",And remove from devDependencies:
- "nearest-color": "^0.4.4", "postcss": "^8.5.6",
♻️ Duplicate comments (1)
electron-app/src/color-picker.ts (1)
16-21: Guard againstgetColorNamereturning a non-object value.As flagged previously, the
nearest-colorlibrary can return either aColorMatchobject or potentiallyundefined/falsy when no match is found. Accessing.namedirectly may result inundefinedbeing passed to the picker.🛡️ Suggested guard
const picker = new ColorPicker({ - colorNameFn: (rgb) => getColorName(rgb).name, + colorNameFn: (rgb) => { + const match = getColorName(rgb as { r: number; g: number; b: number }); + return match?.name ?? 'Unknown'; + }, initialDiameter: 180, initialSquareSize: 20, });
🧹 Nitpick comments (1)
electron-app/src/color-picker.ts (1)
36-38: RedundanthideWindowcall when window is unavailable.The else branch executes when
mb.windowis falsy or destroyed, yet it still callsmb.hideWindow(). This is at best redundant and at worst could cause unexpected behavior if the menubar implementation doesn't handle this gracefully.♻️ Consider removing the else branch
if (mb.window && !mb.window.isDestroyed()) { const hidePromise = new Promise<void>((resolve) => { if (mb.window?.isVisible()) { mb.window?.once('hide', () => resolve()); mb.hideWindow(); } else { resolve(); } }); await hidePromise; - } else { - mb.hideWindow(); }
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.