fix(desktop): stabilize Windows installer + beta 0.0.64#1119
Conversation
📝 WalkthroughWalkthroughAdds Windows-specific packaging and signing controls, copies icons and node-pty outside ASAR, implements robust native-module copying, adds Windows app discovery and PowerShell-based ringtone playback with main↔renderer IPC, centralizes terminal-host cross-platform paths and adapts daemon/socket behavior, and extends NSIS installer UI. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Process
participant IPC as IPC (Main↔Renderer)
participant Renderer as Renderer
participant PS as PowerShell
Note over Main,Renderer: Windows ringtone playback — IPC-first, PS fallback
Main->>IPC: sendRingtoneEvent("ringtone-play", filename)
alt renderer subscribed and receives event
IPC->>Renderer: deliver "ringtone-play" with filename
Renderer->>Renderer: set Audio.src and audio.play()
else renderer unavailable or delivery fails
Main->>PS: spawn PowerShell with player script + filename
PS->>PS: play audio via WPF MediaPlayer
PS-->>Main: exit/status
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 7
🤖 Fix all issues with AI agents
In `@apps/desktop/electron-builder.ts`:
- Around line 99-103: The copy rule for the Windows-only package
"@lydell/node-pty-win32-x64" is currently added unconditionally in the top-level
asarUnpack and files arrays; remove that rule from the global asarUnpack/files
entries in electron-builder.ts and add an equivalent copy rule inside the
existing win configuration block so it only applies to Windows builds. Locate
the global asarUnpack and files definitions and the win section in
electron-builder.ts (and reference OPTIONAL_PLATFORM_MODULES from
copy-native-modules.ts if needed) and move the entry for
"node_modules/@lydell/node-pty-win32-x64" into the win.files/asarUnpack block,
ensuring no remaining global references remain.
In `@apps/desktop/src/lib/electron-app/factories/windows/create.ts`:
- Around line 12-19: The packaged icon candidate paths are incorrect: when
app.isPackaged is true app.getAppPath() yields .../app.asar (so
join(app.getAppPath(), "resources", ...) is wrong) and process.resourcesPath
should point to the app's resources root so you must not add an extra
"resources" segment; update the candidates array in create.ts to remove the
app.getAppPath() resource path and instead use process.resourcesPath joined with
"build", "icons", "icon.ico" (keep the dev path using
app.getAppPath()/src/resources as before) so the variable candidates produces
the actual packaged icon path.
In `@apps/desktop/src/lib/trpc/routers/ringtone/index.ts`:
- Around line 182-190: The Windows branch currently calls
sendRingtoneEvent("ringtone-play", input.filename) and returns success without
checking its boolean result; update the handler in the ringtone router (the
Windows-specific branch around sendRingtoneEvent) to check the returned value
and, if it is false, either call playSoundFile(getSoundPath(input.filename)) as
a fallback or return/log an error and set success to false; apply the same
check-and-fallback logic to the other Windows branches mentioned (the ones
around lines 195-202 and 236-239) so success is only returned when playback was
actually initiated or a fallback was executed.
- Around line 131-143: The execFile callback for Windows playback currently
ignores its error parameter so PowerShell exits with non‑zero codes aren't
logged; update the execFile callback passed to currentSession.process (created
with execFile(powershellPath, buildWindowsPlayerArgs(soundPath), ...)) to accept
(error, stdout, stderr) and, if error is present, log it (including
error.message and stderr) using the domain/operation prefix pattern (e.g.
"[ringtone/play] Windows playback failed: ..."); keep the existing cleanup that
sets currentSession to null when sessionId matches.
- Around line 25-37: Replace the direct IPC pattern by adding a tRPC observable
subscription to the ringtone router and refactor sendRingtoneEvent to accept a
single params object; specifically, create a new subscribe procedure on the
ringtone router using observable from `@trpc/server/observable` that emits
play/stop events to subscribers (mirror patterns used in
auth/notifications/menu/ports/terminal/ui-state routers), update renderer to
call this tRPC subscribe instead of relying on BrowserWindow.webContents.send,
and change the sendRingtoneEvent signature from sendRingtoneEvent(channel:
"ringtone-play" | "ringtone-stop", filename?: string) to
sendRingtoneEvent(params: { channel: "ringtone-play" | "ringtone-stop";
filename?: string }) and update all callers to pass a single object.
In `@apps/desktop/src/renderer/components/RingtonePlayer/RingtonePlayer.tsx`:
- Around line 47-48: RingtonePlayer currently registers IPC listeners directly
with window.ipcRenderer for PLAY_CHANNEL/STOP_CHANNEL; replace this with tRPC
subscriptions and centralized channel defs: add play/stop channel names to
apps/desktop/src/shared/ipc-channels.ts (e.g., PLAY_CHANNEL, STOP_CHANNEL),
implement tRPC server-side observables for those channels, and in
RingtonePlayer.tsx subscribe via the tRPC client observable/subscription API
instead of window.ipcRenderer.on; wire the existing handlePlay and handleStop
callbacks to the subscription next/error handlers and remove the direct
ipcRenderer registrations.
In `@apps/desktop/src/resources/build/installer.nsh`:
- Around line 13-25: The customInstall macro currently always creates the
desktop shortcut when $newDesktopLink exists; update the logic in the
customInstall macro to check the installer flag $isNoDesktopShortcut (or its
truthiness) before creating the desktop shortcut: wrap the CreateShortCut and
subsequent ClearErrors/WinShell::SetLnkAUMI calls for $newDesktopLink in a
conditional that skips shortcut creation if $isNoDesktopShortcut is set, leaving
the existing $newStartMenuLink handling unchanged.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/components/RingtonePlayer/RingtonePlayer.tsx (1)
10-14: Wrap helper functions inuseCallbackto satisfy exhaustive deps.The
playPlaybackandstopPlaybackfunctions are used in theuseEffectbut not listed in dependencies. While the current implementation works because these functions only reference stable values (a ref and a module-level import), this pattern violates React hooks rules and can cause subtle bugs if the functions evolve.♻️ Proposed fix using useCallback
+import { useCallback, useEffect, useRef } from "react"; -import { useEffect, useRef } from "react"; import { getRingtoneUrl } from "renderer/lib/ringtone-urls"; const PLAY_CHANNEL = "ringtone-play"; const STOP_CHANNEL = "ringtone-stop"; export function RingtonePlayer() { const audioRef = useRef<HTMLAudioElement | null>(null); - const stopPlayback = () => { + const stopPlayback = useCallback(() => { if (!audioRef.current) return; audioRef.current.pause(); audioRef.current.currentTime = 0; - }; + }, []); - const playPlayback = (filename: string) => { + const playPlayback = useCallback((filename: string) => { const url = getRingtoneUrl(filename); if (!url) { console.warn("[ringtone] Missing URL for filename:", filename); return; } if (!audioRef.current) { audioRef.current = new Audio(); } const audio = audioRef.current; audio.pause(); audio.src = url; audio.currentTime = 0; audio.volume = 1; audio.play().catch((error) => { console.warn("[ringtone] Failed to play audio:", error); }); - }; + }, []); useEffect(() => { // ... handlers remain the same - }, []); + }, [playPlayback, stopPlayback]);Also applies to: 16-35
apps/desktop/src/lib/trpc/routers/external/helpers.ts (1)
170-209: Consider extracting nested directory traversal for readability.The 4-level nesting in
findJetBrainsToolboxExereflects the Toolbox directory structure but could be simplified by extracting the inner channel/build search into a helper function. This is optional given the domain complexity.♻️ Optional extraction for clarity
+const findExeInToolboxProduct = ( + productDir: string, + exeName: string, +): string | null => { + try { + const channels = fs.readdirSync(productDir, { withFileTypes: true }); + for (const channel of channels) { + if (!channel.isDirectory() || !channel.name.startsWith("ch-")) continue; + const channelDir = nodePath.join(productDir, channel.name); + const builds = fs.readdirSync(channelDir, { withFileTypes: true }) + .filter((b) => b.isDirectory()) + .map((b) => b.name) + .sort() + .reverse(); + for (const buildName of builds) { + const candidate = nodePath.join(channelDir, buildName, "bin", exeName); + if (fs.existsSync(candidate)) return candidate; + } + } + } catch { + // Ignore lookup errors + } + return null; +}; + const findJetBrainsToolboxExe = ( toolboxRoot: string, exeName: string, ): string | null => { try { if (!fs.existsSync(toolboxRoot)) return null; const products = fs.readdirSync(toolboxRoot, { withFileTypes: true }); for (const product of products) { if (!product.isDirectory()) continue; - const productDir = nodePath.join(toolboxRoot, product.name); - const channels = fs.readdirSync(productDir, { withFileTypes: true }); - // ... nested logic + const match = findExeInToolboxProduct( + nodePath.join(toolboxRoot, product.name), + exeName, + ); + if (match) return match; } } catch { // Ignore lookup errors and fall back } return null; };apps/desktop/src/lib/trpc/routers/ringtone/index.ts (1)
13-23: Extract Windows playback helpers and timing constants to a shared module.
The PowerShell script’s hard-coded waits (300 / 200 ms) are magic numbers and the helper is duplicated withsrc/main/lib/notification-sound.ts; centralizing them will prevent drift and make the timings explicit.As per coding guidelines: Avoid magic numbers by extracting them to named constants at module top.
Also applies to: 39-79
apps/desktop/src/main/lib/notification-sound.ts (2)
13-65: Consider extracting Windows playback helpers to a shared module.
The PowerShell script and timing values are duplicated with the ringtone router; centralizing them would prevent drift and allow named constants for the waits.As per coding guidelines: Avoid magic numbers by extracting them to named constants at module top.
126-139: Align Windows playback warnings with[domain/operation]logging format.
Use a prefix like[notification-sound/playback]for this warning.As per coding guidelines: Use prefixed console logging with pattern `[domain/operation] message` for all logging.🔧 Suggested tweak
- console.warn( - "[notification-sound] Windows playback failed:", - error.message, - ); + console.warn( + "[notification-sound/playback] Windows playback failed:", + error.message, + );apps/desktop/scripts/copy-native-modules.ts (1)
58-95: Replace positional/boolean args with params objects and add a log prefix.
copyModuleIfSymlink/ensureLocalModuleCopyare called withfalse, which is hard to read and violates the params-object guideline. Also, the script logs should use a consistent[domain/operation]prefix.As per coding guidelines: Functions with 2+ parameters should accept a single params object with named properties instead of positional arguments; Avoid boolean blindness: use options objects with named properties instead of `doThing(true, false, true)`; Use prefixed console logging with pattern `[domain/operation] message` for all logging.♻️ Example refactor
+const LOG_PREFIX = "[native-modules/prepare]"; -console.log("Preparing native modules for electron-builder..."); +console.log(`${LOG_PREFIX} Preparing native modules for electron-builder...`); -function copyModuleIfSymlink( - nodeModulesDir: string, - moduleName: string, - required: boolean, -): boolean { +function copyModuleIfSymlink({ + nodeModulesDir, + moduleName, + required, +}: { + nodeModulesDir: string; + moduleName: string; + required: boolean; +}): boolean { -function ensureLocalModuleCopy(moduleName: string, required: boolean) { +function ensureLocalModuleCopy({ + moduleName, + required, +}: { + moduleName: string; + required: boolean; +}) { -ensureLocalModuleCopy(moduleName, false); +ensureLocalModuleCopy({ moduleName, required: false });Also applies to: 106-154
| if (process.platform === "win32") { | ||
| sendRingtoneEvent("ringtone-play", input.filename); | ||
| return { success: true as const }; | ||
| }), | ||
| } | ||
|
|
||
| const soundPath = getSoundPath(input.filename); | ||
| playSoundFile(soundPath); | ||
| return { success: true as const }; | ||
| }), |
There was a problem hiding this comment.
Check sendRingtoneEvent result before returning success on Windows.
If no window exists, the helper returns false and these paths still return success while skipping playback. Consider falling back to playSoundFile (or logging/returning an error) when the send fails.
Also applies to: 195-202, 236-239
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/ringtone/index.ts` around lines 182 - 190,
The Windows branch currently calls sendRingtoneEvent("ringtone-play",
input.filename) and returns success without checking its boolean result; update
the handler in the ringtone router (the Windows-specific branch around
sendRingtoneEvent) to check the returned value and, if it is false, either call
playSoundFile(getSoundPath(input.filename)) as a fallback or return/log an error
and set success to false; apply the same check-and-fallback logic to the other
Windows branches mentioned (the ones around lines 195-202 and 236-239) so
success is only returned when playback was actually initiated or a fallback was
executed.
| "nanoid": "^5.1.6", | ||
| "node-addon-api": "^7.1.0", | ||
| "node-pty": "1.1.0-beta30", | ||
| "node-pty": "npm:@lydell/node-pty@^1.0.1", |
There was a problem hiding this comment.
@AXGZ21 what's the reason for using this package? I think i had to do this for onlook as well but can't recall exactly
There was a problem hiding this comment.
Good question! The @lydell/node-pty package is a fork of the original node-pty library. Based on the change, there are a few likely reasons why this fork was used:
Possible reasons:
- Bug fixes - The lydell fork may contain fixes for specific issues that haven't been merged into the main package yet
- Platform-specific compatibility - Since
node-ptydeals with pseudo-terminals, this fork might have better cross-platform support (Windows/macOS/Linux) - Electron compatibility - The fork could have better compatibility with our specific Electron version
- Maintenance - The main package might have had slower maintenance when this change was made
To confirm the exact reason, it would be helpful to:
- Check the original PR description or commit message for context
- Review the lydell/node-pty repository to see what differences exist
- Look for any related issues that prompted this change
Did you encounter this same issue with Onlook, or are you just reviewing the change for context?
AI Generated by PierrunoYT
There was a problem hiding this comment.
@Kitenite We use npm:@lydell/node-pty@^1.0.1 because it provides reliable prebuilt binaries (especially for Windows) for Electron, so terminal PTY spawning works and installer builds don’t fail on native compilation.
Also, keep only one node-pty entry in package.json (remove the duplicate).
…nt handling - Added EventEmitter for ringtone events to manage play and stop actions. - Refactored sendRingtoneEvent function to use an object parameter for better clarity. - Updated RingtonePlayer component to subscribe to ringtone events via tRPC, replacing previous IPC event handling.
- Updated the playSoundFile function to include error handling for Windows playback failures. - Removed redundant error listener and consolidated error logging within the playback callback.
- Added fallback mechanisms for both playing and stopping ringtones when no renderer window is available. - Updated sendRingtoneEvent calls to check for successful event sending before falling back to direct sound playback or stopping current sound.
- Introduced a condition to prevent the creation of a desktop shortcut if the user opts out. - Ensured that the existing logic for creating shortcuts in the Start Menu remains intact.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/desktop/electron-builder.ts`:
- Around line 107-109: The packaging config sets npmRebuild: false but
electron-rebuild is never invoked and the copy:native-modules script only copies
symlinks, so native modules will fail at runtime; either set npmRebuild: true in
the electron-builder config (change npmRebuild to true) or add an explicit
pre-pack step to run electron-rebuild (or electron-builder install-app-deps)
from your package.json build/package/release scripts so native modules (e.g.,
better-sqlite3, node-pty) are rebuilt for the Electron platform before
packaging.
In `@apps/desktop/src/lib/trpc/routers/ringtone/index.ts`:
- Around line 32-42: The sendRingtoneEvent function currently always returns
true, preventing callers from detecting whether any listeners handled the event;
change it to return the boolean result of ringtoneEvents.emit(...) and ensure
the play path returns false if params.filename is missing. Concretely, in
sendRingtoneEvent (the function handling channel "ringtone-play" |
"ringtone-stop"), call ringtoneEvents.emit("ringtone-event", ...) and return its
value for both play and stop branches, and return false for invalid play
requests (no filename) so callers can run their fallback playback.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/components/RingtonePlayer/RingtonePlayer.tsx (1)
46-50: StabilizestopPlaybackwithuseCallbackto satisfy exhaustive deps.The static analysis correctly identifies that
stopPlaybackis used inside the cleanup effect but not listed in the dependency array. SincestopPlaybackis recreated on every render, this could lead to stale closure issues. Wrap it withuseCallbackto make it stable.♻️ Proposed fix
-import { useEffect, useRef } from "react"; +import { useCallback, useEffect, useRef } from "react"; import { electronTrpc } from "renderer/lib/electron-trpc"; import { getRingtoneUrl } from "renderer/lib/ringtone-urls"; export function RingtonePlayer() { const audioRef = useRef<HTMLAudioElement | null>(null); - const stopPlayback = () => { + const stopPlayback = useCallback(() => { if (!audioRef.current) return; audioRef.current.pause(); audioRef.current.currentTime = 0; - }; + }, []);Then update the effect:
useEffect(() => { return () => { stopPlayback(); }; - }, []); + }, [stopPlayback]);apps/desktop/src/lib/trpc/routers/ringtone/index.ts (2)
44-84: PowerShell path escaping may be insufficient for edge cases.The escaping only handles single quotes (
'→''), which is correct for PowerShell single-quoted strings. However, file paths with unusual characters (backticks, dollar signs if path somehow contains them) could cause issues. Consider also validating that the path contains only expected characters, or using-LiteralPathwhere applicable.
183-220: Inconsistent indentation in procedure definitions.The
previewandstopprocedures have inconsistent indentation compared tosubscribeandlist. This appears to be a formatting issue where these procedures are at a different indent level than expected within therouter({...})block.apps/desktop/electron-builder.ts (1)
13-13: Extract the"1"flag to a named constant.This avoids a magic number and clarifies intent.
♻️ Suggested change
-const disableWinSigning = process.env.SUPERSET_DISABLE_WIN_SIGNING === "1"; +const DISABLE_WIN_SIGNING_FLAG = "1"; +const disableWinSigning = + process.env.SUPERSET_DISABLE_WIN_SIGNING === DISABLE_WIN_SIGNING_FLAG;
- Updated type assertions for legacy panes and file viewers during state migration. - Ensured proper handling of `needsAttention` and `isLocked` properties to maintain compatibility with the new schema.
… organization ID access - Added a check to ensure the user is present in the session during token verification. - Updated access to `activeOrganizationId` in multiple routers to use optional chaining for safer access.
|
will this be merged? |
- Improved the sendRingtoneEvent function to validate the presence of a filename for play requests. - Updated logic to return false for invalid requests, ensuring better error handling and clarity in event emission.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/ringtone/index.ts`:
- Around line 215-226: The Windows stop handler may skip killing the fallback
child process when sendRingtoneEvent returns true; modify the stop procedure
(stop: publicProcedure.mutation) to always call stopCurrentSound() (to kill any
fallback playback) and then, if on win32, also call sendRingtoneEvent({ channel:
"ringtone-stop" }) to notify the renderer; ensure both functions run
(stopCurrentSound first) and keep returning { success: true }.
🧹 Nitpick comments (1)
apps/desktop/electron-builder.ts (1)
60-65: Consider moving the iconsextraResourcesentry into thewinsection.The comment on line 60 says these icons are for Windows shortcuts, but the entry sits at the top level, so it will be bundled into macOS and Linux builds as well. Moving it under
win.extraResourceswould keep non-Windows artifacts leaner.
- Refactored the stop procedure to ensure fallback playback is always stopped first. - Simplified the logic for notifying the renderer to stop playback on Windows, removing unnecessary checks for renderer availability.
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)
apps/desktop/src/lib/trpc/routers/ringtone/index.ts (1)
169-173:⚠️ Potential issue | 🟡 Minor
aplayfallback silently swallows errors.If
aplayalso fails, the error is discarded and the user gets no feedback. Per coding guidelines: "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly."Proposed fix
- currentSession.process = execFile("aplay", [soundPath], () => { + currentSession.process = execFile("aplay", [soundPath], (aplayError) => { + if (aplayError) { + console.warn("[ringtone/play] Linux aplay fallback failed:", aplayError.message); + } if (currentSession?.id === sessionId) { currentSession = null; } });
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/ringtone/index.ts (2)
50-90: Extract magic numbers in the PowerShell player script to named constants.The loop bound
300, sleep interval200, and WMPplayStatevalues (3= playing,1= stopped,8= ended) are opaque inline. Consider extracting them at the module top:+const WMP_POLL_MAX_ITERATIONS = 300; +const WMP_POLL_INTERVAL_MS = 200; +// Windows Media Player playState enum values +const WMP_STATE_PLAYING = 3; +const WMP_STATE_STOPPED = 1; +const WMP_STATE_ENDED = 8;Then reference them in the script template. This makes the effective 60-second timeout (300 × 200 ms) discoverable and adjustable. As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic."
189-225: Inconsistent indentation:previewandstopare indented one level less thansubscribeandlist.
preview(Line 189) andstop(Line 215) are at one-tab indent inside therouter({})call, whilesubscribe(Line 231) andlist(Line 248) are at two-tab indent. This appears to be a formatting accident. Aligning all procedures to the same level improves readability.
|
still no updates on this? |
|
Hey — just a heads up, this was closed as part of an automated stale PR cleanup. If you think this was done in error, feel free to reopen it! |
|
Hey — this was closed by an automated cleanup of PRs with major merge conflicts that are 3+ weeks old. If you think this was done incorrectly, please feel free to reopen it. Sorry for any inconvenience! |
4 participants
What this PR does\n- Fixes Windows packaging layout so the installed app starts reliably.\n- Keeps installer/build-only resources out of app asar packaging paths.\n- Ensures Windows shortcuts use the Superset icon path consistently.\n- Bumps desktop app version to
Summary by CodeRabbit
New Features
Bug Fixes
Chores