Conversation
|
WalkthroughAdds broad macOS Dashboard features: Home view, multi-pane Settings overlay, branch/session management, editor/coding-agent and terminal integrations, gateway token management, tmux/zellij session support, runtime/container metadata, UI styling refactors, CLI extensions, and many model/service API additions. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant User
end
participant Dashboard as DashboardView
participant Model as DashboardModel
participant CLI as HackCLIClient
participant Terminal as TerminalIntegration
participant Editor as EditorIntegration
participant Agent as CodingAgentIntegration
User->>Dashboard: select project / open home
Dashboard->>Model: handleSelectedItemChange(selection)
Model->>CLI: fetchProjects(), fetchGlobalStatus()
CLI-->>Model: projects + status
Model-->>Dashboard: updated state
User->>Dashboard: "Open in Editor" / "Open in Agent"
Dashboard->>Editor: resolve editor for project
Editor-->>Dashboard: app path or terminal command
alt GUI editor
Dashboard->>Editor: NSWorkspace.open(projectURL)
else terminal editor
Dashboard->>Terminal: openExternalTerminalWithCommand(command, app)
end
Dashboard->>Agent: resolveBinaryPath(agent, override)
Agent-->>Dashboard: binary path
Dashboard->>Terminal: openTerminalWithCommand(launchCommand(projectPath, agent))
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/daemon/routes/sessions.ts (1)
95-98:⚠️ Potential issue | 🟠 MajorValidate
sessionIdagainstSESSION_NAME_PATTERNbefore use in tmux commands.
sessionIdfrom the URL path is passed directly totmux -t(lines 204, 243, 288) without validation, whileSESSION_NAME_PATTERNis enforced only at creation time. Tmux's-targument interprets special characters (e.g.,:for window/pane targeting), so a craftedsessionIdcould target unintended sessions or panes.Proposed fix
const sessionId = segments[2]; if (!sessionId) { return jsonResponse({ error: "missing_session_id" }, 400); } + if (!SESSION_NAME_PATTERN.test(sessionId)) { + return jsonResponse({ error: "invalid_session_id" }, 400); + }src/commands/ssh.ts (2)
375-389:⚠️ Potential issue | 🔴 CriticalCommand injection:
sessionNameis interpolated into a shell string without validation.
opts.sessionNameis embedded in a single-quoted shell command via template literal (line 377). The positional argument path (line 218→228) passessessionArgdirectly toconnectToSessionwithout validating againstSESSION_NAME_PATTERN. A value likefoo'; malicious_cmd; echo 'would break out of the quotes.Validate before use:
Proposed fix
async function connectToSession(opts: { readonly hostname: string; readonly user?: string; readonly port?: number; readonly sessionName: string; }): Promise<number> { + if (!SESSION_NAME_PATTERN.test(opts.sessionName)) { + p.log.error("Invalid session name: only alphanumeric, dash, underscore, or dot allowed"); + return 1; + } + p.log.step(`Connecting to ${opts.sessionName}...`);
51-56:⚠️ Potential issue | 🟡 MinorRemoval of
-dshort flag for--directis a breaking change for existing CLI users.If anyone scripted or habitually uses
hack ssh -d, this will silently fail or behave unexpectedly. Consider documenting this in release notes.
🤖 Fix all issues with AI agents
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/GlassCard.swift`:
- Around line 48-56: The else branch is adding a duplicate RoundedRectangle
stroke overlay on top of content while cardBackground already applies
.stroke(borderColor, lineWidth: 1); remove the outer .overlay(...) in the else
branch so only cardBackground provides the border; locate the else branch
wrapping content (the view builder using
content.background(cardBackground).overlay(RoundedRectangle(cornerRadius: 16,
style: .continuous).stroke(borderColor, lineWidth: 1))) and delete the
.overlay(...) call that creates the second stroke, leaving
content.background(cardBackground) intact.
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectDetailView.swift`:
- Around line 1755-1760: Multiple copies of the shellQuote function
(shellQuote(_: String)) exist with inconsistent implementations; extract a
single shared implementation (e.g., a ShellQuoter.shellQuote(_:) static method
or String extension) into a common module/utility and replace the duplicated
functions in ProjectDetailView, CodingAgentIntegration, EditorIntegration and
SettingsOverlayView to call that shared function; pick one POSIX-safe quoting
strategy (use the existing "'\"'\"'" variant consistently), remove the local
copies, and ensure all callers reference the new utility symbol
(ShellQuoter.shellQuote or String.shellQuoted) so behavior is centralized and
consistent.
- Around line 496-506: The current flow calls model.addBranch(...) then
unconditionally calls model.startBranch(...), so startBranch may run even if
addBranch failed; update the call site in the Button action to only call
model.startBranch when addBranch actually succeeded: either change addBranch to
return a success value (Bool/Result) or make it throw and handle errors, or
invoke the existing runActionResult pattern to perform addBranch and inspect its
success before calling startBranch; ensure you reference the model.addBranch and
model.startBranch calls (and preserve resetBranchDraft()/showAddBranchSheet
behavior only after a successful add/start) so the UI state is only cleared when
branch creation/start completes successfully.
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swift`:
- Around line 676-722: The savePreferences function currently ignores the Bool
result from model.setGlobalConfig calls so partial failures are possible; update
savePreferences to check the return value of each await model.setGlobalConfig
(for keys like "controlPlane.preferences.appearance.theme",
"controlPlane.preferences.terminal.defaultApp",
"controlPlane.preferences.agents.binaryPath", etc.) and if any call returns
false set an error state (or throw/return early) before calling await
model.refresh() and await loadConfigFromDisk(), ensuring failures are surfaced
to the UI; preserve the existing defer that clears isLoadingConfig and surface a
concise user-visible error or boolean result from savePreferences to indicate
success/failure.
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/TerminalDrawerModel.swift`:
- Around line 73-88: The titleOverride can be whitespace-only and produce a
blank tab title; in openOrSelect normalize titleOverride like branch/command by
trimming whitespace and treating empty string as nil (e.g. create a
normalizedTitle/ resolvedTitle from titleOverride), then use that resolvedTitle
when computing title (replace titleOverride with the normalized value in the Tab
creation and title assignment); ensure you reference the existing openOrSelect
function and the title assignment line that builds title with
Self.tabBaseTitle(for: project) so the change is localized.
In
`@apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLIClient.swift`:
- Around line 143-152: In inspectTailscale(), the broad catch block swallows
CancellationError and prevents cooperative cancellation; update the catch in
inspectTailscale() to rethrow CancellationError (e.g., check `error` for
CancellationError or use Task.isCancelled/Task.checkCancellation) before falling
back to inspectTailscaleDirect(), so cancellation propagates from
run()/Task.checkCancellation() while non-cancellation failures still use the
direct fallback.
- Around line 575-646: The runExecutable implementation duplicates run; extract
the shared process execution flow (Process setup after executable/args
resolution, with Task.checkCancellation, Pipe creation,
withTaskCancellationHandler that runs the process, reads stdout/stderr async,
waits for exit, handles cancellation by terminating process and closing pipes,
and constructs CLIResult or throws HackCLIError) into a single private helper
(e.g., runProcess(using: Process, allowNonZeroExit:) or
runProcess(executableURL: URL, args: [String], allowNonZeroExit: Bool)) and have
both run and runExecutable resolve the executablePath/args, create or configure
the Process, then delegate to that helper; preserve existing behaviors:
environment via HackCLILocator.buildEnvironment(), cwd handling, proper
pipe/fileHandle closing, cancellation checks, and error mapping so no behavior
changes occur.
In
`@apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLILocator.swift`:
- Around line 40-44: The code currently ignores an explicit env["HACK_CLI_PATH"]
override when normalizeHackCandidate(...) returns nil; update the logic in
HackCLILocator (the block that checks env["HACK_CLI_PATH"] and
fileManager.isExecutableFile(atPath:)) to surface this failure instead of
silently discarding it — either log a warning (including the original override
value and reason) or return an explicit error/result indicating normalization
failed for the provided path (use the same logger or error flow used elsewhere
in this module). Ensure the message references the override value and that
normalizeHackCandidate was the failing step so users see why their HACK_CLI_PATH
was rejected.
In `@src/control-plane/extensions/gateway/commands.ts`:
- Around line 111-123: The token-revoke command writes JSON to stdout when
parsed.value.json is true but fails to return early, causing subsequent
ctx.logger.warn or ctx.logger.success output to corrupt the JSON; update the
token-revoke path (the block that checks parsed.value.json) to immediately
return 0 after writing JSON (mirroring token-create and token-list behavior) so
no further logging (ctx.logger.warn / ctx.logger.success) runs and stdout
contains only the JSON payload.
🧹 Nitpick comments (24)
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ToolbarIconButton.swift (2)
56-57: Redundant frame sizing.
setFrameSize(_:)on Line 56 already updatesframe.size, so the assignment on Line 57 is a no-op.Suggested fix
focusRingType = .none setFrameSize(Self.hitTargetSize) - frame.size = Self.hitTargetSize
105-143:isDarkAppearancecomputed twice per update cycle.
updateAppearance()(Line 106) andupdateSymbolAppearance()(Line 131) each independently queryeffectiveAppearance. Since the latter is always called from the former, you can compute it once and pass it through.Suggested refactor
private func updateAppearance() { let isDarkAppearance = effectiveAppearance.bestMatch(from: [.darkAqua, .aqua]) == .darkAqua // ... background/border logic unchanged ... - updateSymbolAppearance() + updateSymbolAppearance(isDarkAppearance: isDarkAppearance) } - private func updateSymbolAppearance() { - let isDarkAppearance = effectiveAppearance.bestMatch(from: [.darkAqua, .aqua]) == .darkAqua + private func updateSymbolAppearance(isDarkAppearance: Bool) { let symbolName = (isHovered ? hoverSystemImage : nil) ?? normalSystemImagesrc/daemon/routes/sessions.ts (1)
349-363:parseTmuxSessionFieldsis duplicated between this file andsrc/commands/ssh.ts.Both files define identical
parseTmuxSessionFieldsand use the same separator constant. Consider extracting these into a shared module (e.g.,src/lib/tmux.ts) to avoid divergence.apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLIClient.swift (2)
278-294: Quadratic worst-case inextractJsonSnippets.Every
{and[in the text triggers a full forward scan viaextractBalancedJson. For typical CLI output this is fine, but deeply nested or large outputs (e.g., verbose logging prefixed to JSON) could degrade to O(n²). Something to keep in mind — if performance is ever a concern, scanning once with a single pass would eliminate this.No action needed now, just flagging for awareness.
629-630: Prefer failableString(bytes:encoding:)overString(decoding:as:).Per the SwiftLint
optional_data_string_conversionrule,String(decoding:as:)always succeeds (replacing invalid bytes with U+FFFD), which can silently mask corrupt output. The failableString(bytes:encoding:)initializer lets you detect and handle encoding failures. The same applies to lines 405-406 inrun.Proposed fix (runExecutable, lines 629-630)
- let stdout = String(decoding: stdoutBytes ?? Data(), as: UTF8.self) - let stderr = String(decoding: stderrBytes ?? Data(), as: UTF8.self) + let stdout = String(bytes: stdoutBytes ?? Data(), encoding: .utf8) ?? "" + let stderr = String(bytes: stderrBytes ?? Data(), encoding: .utf8) ?? ""src/commands/session.ts (1)
883-934: Robust session parsing with custom separator and fallback — nicely done.The
|||HACK_SESSION_FIELD|||separator avoids collisions with session names that could contain tabs or colons, and the tab fallback inparseTmuxSessionFieldsadds resilience. Blank-line skipping (Line 900) is a good defensive addition.One minor type-safety note:
parseTmuxSessionFieldsreturnsreadonly string[]but TypeScript can't narrow array length from the.length === expectedCountcheck, so the destructuredname,attached,pathat Line 907 are typedstring | undefined. The existing guards (if (name)at Line 908,=== "1",|| null) handle this safely at runtime, but if you want full type-level safety you could use a tuple return:Optional: tuple return type for stricter inference
function parseTmuxSessionFields( line: string, separator: string, expectedCount: number -): readonly string[] | null { +): readonly [string, string, string] | null { const bySeparator = line.split(separator); if (bySeparator.length === expectedCount) { - return bySeparator; + return bySeparator as [string, string, string]; } const byTab = line.split("\t"); if (byTab.length === expectedCount) { - return byTab; + return byTab as [string, string, string]; } return null; }This would lose the generic
expectedCountflexibility, so only worthwhile if the function stays single-use.src/control-plane/extensions/gateway/commands.ts (1)
129-154: Consider a shared generic parse-result type to reduce repetition.
TokenCreateParseResult,TokenListParseResult, andTokenRevokeParseResultare identical in shape apart from thevaluetype. A single generic would DRY this up:type ParseResult<T> = | { readonly ok: true; readonly value: T } | { readonly ok: false; readonly error: string };Then:
ParseResult<TokenCreateArgs>,ParseResult<TokenListArgs>, etc.Also, per coding guidelines,
interfaceis preferred overtypefor plain object shapes (TokenCreateArgs,TokenListArgs,TokenRevokeArgs). As per coding guidelines, "Preferinterfacefor defining object shapes in TypeScript".src/control-plane/extensions/tailscale/commands.ts (3)
209-225: Consider using the resolvedbinaryPathin theexeccall.Line 210 resolves
binaryPathviafindExecutableInPath("tailscale"), but line 223 still invokesexec(["tailscale", ...])using the bare command name. WhileBun.which(used internally) ensures the binary is on PATH, using the resolvedbinaryPathwould be more explicit and avoid any edge case where PATH changes between the check and the exec.♻️ Suggested change
- const result = await exec(["tailscale", "status", "--json"], { + const result = await exec([binaryPath, "status", "--json"], { stdin: "ignore", });
146-167: Preferinterfaceovertypefor object shapes.
TailscaleInspectPeer,TailscaleInspectSelf, and other object-shape definitions usetypeinstead ofinterface. The project coding guidelines preferinterfacefor defining object shapes in TypeScript. Since these are purely structural (no unions or mapped types),interfacewould be more idiomatic here.As per coding guidelines: "Prefer
interfacefor defining object shapes in TypeScript".
323-354: Near-duplicate logic betweenparsePeerandparseSelfPeer.These two functions share identical field extraction for all properties except
isExitNodeOption. You could extract a shared base and extend it, or haveparseSelfPeercallparsePeerand omit the extra field. This is a minor DRY observation — the duplication is small enough to be acceptable as-is.tests/config-command.test.ts (1)
31-48: Dynamicimport()ofrun.tswill be cached after the first test.Each test does
await import("../src/cli/run.ts"), but Bun caches dynamic imports. Since these tests don't usemock.modulefor the config dependencies, this works fine — the environment variables are read at call time, not import time. Just noting this is safe in the current setup but would become an issue if any test needed to mock modules imported byrun.ts.Also applies to: 50-67, 69-86, 88-105
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/CodingAgentIntegration.swift (1)
84-89:shellQuoteis duplicated across four files with inconsistent implementations.This exact helper appears in
EditorIntegration.swift,ProjectDetailView.swift, andCodingAgentIntegration.swift(all using'\"'\"'quoting), whileSettingsOverlayView.swiftuses a different escaping approach ('\\''). Consider extracting a single sharedshellQuoteutility to avoid drift and reduce duplication.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/DashboardModel.swift (3)
105-110: Ordering ofruntimeHealthStatecases deserves a brief note.If
runtimeOverallOk == trueis checked beforeglobalInfraDown, then a scenario where runtime reports OK but global infra is actually down would return.healthy. In practice this likely can't happen becauseruntimeOverallOkincorporatesglobalStatus?.summary.ok, but the logic is non-obvious. A short comment here would help future readers.
560-578:scheduleStatusClear()was extracted but only used byrunGlobalCommand;runActionandrunActionResultstill inline the same pattern.Consider replacing the duplicated status-clear blocks at lines 575–578 and 592–595 with a call to
scheduleStatusClear()for consistency.♻️ Example refactor in runAction
await refresh() - statusClearTask = Task { [weak self] in - try? await Task.sleep(for: .seconds(2)) - self?.statusMessage = nil - } + scheduleStatusClear()Also applies to: 592-595, 640-644
630-638:shouldFallbackToTerminalrelies on fragile string matching againstlocalizedDescription.Matching substrings like
"sudo","password", or"not permitted"in localized error descriptions is fragile — the exact wording can change across OS locales and CLI versions. Consider matching against a structured error code or exit status fromHackCLIError.commandFailed(exitCode:stderr:)instead, if available. If string matching is the only viable path here, consider matching against the raw stderr rather thanlocalizedDescription.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SetupAssistantView.swift (1)
188-210: LGTM — Permissions callout with automation request and settings link.Clean implementation. The
terminalAutomationToneprovides appropriate visual feedback after the user attempts the permission request.One minor observation: the callout is always visible even if the user has already granted automation permission in a prior session, since
terminalAutomationGrantedresets tonileach time the sheet opens. IfTerminalIntegrationcan probe the current permission state, initializing with that value on appear would make this feel more polished, but it's not blocking.tests/project-views.test.ts (1)
2-2: Minor:writeFilefromnode:fs/promisescould useBun.writeper project conventions.The test setup at line 54 uses
writeFilefromnode:fs/promises. Per coding guidelines,Bun.file/Bun.writeis preferred overnode:fswrite methods. That said, the otherfs/promisesimports (mkdir,mkdtemp,rm,symlink) have no Bun equivalents, so the mixed usage is pragmatic here. Based on learnings, "PreferBun.fileovernode:fs's readFile/writeFile for file operations".apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/GatewayDetailView.swift (2)
498-515: Formatter objects are allocated on every call — consider caching.
ISO8601DateFormatter(),DateFormatter(), andRelativeDateTimeFormatter()are expensive to create. These functions are called per-row intokenTableRow, meaning N×3 allocations per render for N tokens.Proposed fix — use static formatters
+ private static let iso8601Formatter: ISO8601DateFormatter = { + let f = ISO8601DateFormatter() + return f + }() + + private static let dateTimeFormatter: DateFormatter = { + let f = DateFormatter() + f.dateStyle = .medium + f.timeStyle = .short + return f + }() + + private static let relativeDateFormatter: RelativeDateTimeFormatter = { + let f = RelativeDateTimeFormatter() + f.unitsStyle = .short + return f + }() + private func formatTimestamp(_ value: String?) -> String { - guard let value, let date = ISO8601DateFormatter().date(from: value) else { + guard let value, let date = Self.iso8601Formatter.date(from: value) else { return value ?? "—" } - let formatter = DateFormatter() - formatter.dateStyle = .medium - formatter.timeStyle = .short - return formatter.string(from: date) + return Self.dateTimeFormatter.string(from: date) } private func formatRelativeTimestamp(_ value: String?) -> String { - guard let value, let date = ISO8601DateFormatter().date(from: value) else { + guard let value, let date = Self.iso8601Formatter.date(from: value) else { return "—" } - let formatter = RelativeDateTimeFormatter() - formatter.unitsStyle = .short - return formatter.localizedString(for: date, relativeTo: Date()) + return Self.relativeDateFormatter.localizedString(for: date, relativeTo: Date()) }Note:
GatewayDetailViewis a struct, so useprivate static letor extract into a companionenumnamespace.
427-432: Token revocation has no confirmation — destructive action is one-click.Revoking a token is irreversible. Consider adding a confirmation dialog or at minimum a
.confirmationDialogmodifier to prevent accidental revocations.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/HomeDashboardView.swift (1)
102-104: Minor duplication of "is running" logic betweenisProjectRunningandProjectListRow.statusColor.Both check
project.status == .running || project.runtimeStatus == .running. This is acceptable sinceProjectListRowis private and tightly scoped, but if the running-check logic evolves, both sites need updating.Also applies to: 199-203
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectDetailView.swift (1)
356-394: Repeated content wrapper pattern acrossbranchesContent,sessionsContent, andoverviewContent.All three use the same ScrollView → VStack → padding → RoundedRectangle background → stroke pattern. This is a low-priority DRY observation — could be extracted into a helper, but not urgent.
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/EditorIntegration.swift (1)
44-59: Bundle identifiers may become stale — consider documenting the source.Hardcoded bundle IDs like
"com.todesktop.230313mzl4w4u92"(Cursor) are correct today but fragile. A brief TSDoc/comment linking to where these were sourced would help future maintainers verify them.src/lib/runtime-projects.ts (2)
14-68: Prefer interfaces for object shapes.These type definitions are simple object shapes that should use
interfaceinstead oftypealiases, per project guidelines.♻️ Suggested refactor
-export type RuntimeContainer = { +export interface RuntimeContainer { readonly id: string; readonly project: string; readonly service: string; readonly state: string; readonly status: string; readonly name: string; readonly ports: string; readonly workingDir: string | null; readonly image: string | null; readonly labels: Readonly<Record<string, string>> | null; readonly mounts: readonly RuntimeContainerMount[]; readonly networks: readonly RuntimeContainerNetwork[]; -}; +} -export type RuntimeContainerMount = { +export interface RuntimeContainerMount { readonly type: string; readonly source: string; readonly destination: string; readonly mode: string; readonly rw: boolean | null; -}; +} -export type RuntimeContainerNetwork = { +export interface RuntimeContainerNetwork { readonly name: string; readonly ipAddress: string | null; readonly gateway: string | null; readonly aliases: readonly string[]; -}; +} -type ContainerInspectData = { +interface ContainerInspectData { readonly labels: Record<string, string>; readonly image: string | null; readonly mounts: readonly RuntimeContainerMount[]; readonly networks: readonly RuntimeContainerNetwork[]; -}; +}
365-443: Use named-parameter objects for the parseInspect helpers.*These helper functions use positional parameters, which conflicts with the repo standard of using named parameters for clarity and consistency. Refactor all four functions and their call sites to use named-parameter objects.
♻️ Suggested refactor
- const labels = parseInspectLabels(config); - const image = parseInspectImage(config); - const mounts = parseInspectMounts(item.Mounts); - const networks = parseInspectNetworks(item.NetworkSettings); + const labels = parseInspectLabels({ config }); + const image = parseInspectImage({ config }); + const mounts = parseInspectMounts({ raw: item.Mounts }); + const networks = parseInspectNetworks({ raw: item.NetworkSettings }); -function parseInspectLabels(config: unknown): Record<string, string> { +function parseInspectLabels(opts: { config: unknown }): Record<string, string> { + const { config } = opts if (!isRecord(config)) { return {}; } const labels = config.Labels; if (!isRecord(labels)) { return {}; } const out: Record<string, string> = {}; for (const [key, value] of Object.entries(labels)) { if (typeof value === "string") { out[key] = value; } } return out; } -function parseInspectImage(config: unknown): string | null { +function parseInspectImage(opts: { config: unknown }): string | null { + const { config } = opts if (!isRecord(config)) { return null; } return getString(config, "Image") ?? null; } -function parseInspectMounts(raw: unknown): RuntimeContainerMount[] { +function parseInspectMounts(opts: { raw: unknown }): RuntimeContainerMount[] { + const { raw } = opts if (!Array.isArray(raw)) { return []; } const mounts: RuntimeContainerMount[] = []; for (const value of raw) { if (!isRecord(value)) { continue; } const source = getString(value, "Source") ?? ""; const destination = getString(value, "Destination") ?? ""; if (!(source && destination)) { continue; } const rwValue = value.RW; mounts.push({ type: getString(value, "Type") ?? "", source, destination, mode: getString(value, "Mode") ?? "", rw: typeof rwValue === "boolean" ? rwValue : null, }); } return mounts; } -function parseInspectNetworks(raw: unknown): RuntimeContainerNetwork[] { +function parseInspectNetworks(opts: { raw: unknown }): RuntimeContainerNetwork[] { + const { raw } = opts if (!isRecord(raw)) { return []; } const networks = raw.Networks; if (!isRecord(networks)) { return []; } const out: RuntimeContainerNetwork[] = []; for (const [name, value] of Object.entries(networks)) { if (!isRecord(value)) { continue; } const aliasesRaw = value.Aliases; const aliases = Array.isArray(aliasesRaw) ? aliasesRaw.filter((alias): alias is string => typeof alias === "string") : []; out.push({ name, ipAddress: getString(value, "IPAddress") ?? null, gateway: getString(value, "Gateway") ?? null, aliases, }); } return out; }
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/GlassCard.swift
Show resolved
Hide resolved
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectDetailView.swift
Show resolved
Hide resolved
| private func shellQuote(_ value: String) -> String { | ||
| if value.isEmpty { | ||
| return "''" | ||
| } | ||
| return "'\(value.replacingOccurrences(of: "'", with: "'\"'\"'"))'" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
shellQuote is duplicated 4 times across the codebase with inconsistent implementations.
This exact function exists in:
ProjectDetailView.swift(here) — uses'\"'\"'CodingAgentIntegration.swift:83— uses'\"'\"'EditorIntegration.swift:179— uses'\"'\"'SettingsOverlayView.swift:1935— uses'\\''
Both quoting strategies are valid POSIX, but having four copies risks divergence. Consider extracting into a shared utility.
#!/bin/bash
# Verify all shellQuote implementations in the codebase
rg -n "func shellQuote" --type swift -A 5🤖 Prompt for AI Agents
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectDetailView.swift`
around lines 1755 - 1760, Multiple copies of the shellQuote function
(shellQuote(_: String)) exist with inconsistent implementations; extract a
single shared implementation (e.g., a ShellQuoter.shellQuote(_:) static method
or String extension) into a common module/utility and replace the duplicated
functions in ProjectDetailView, CodingAgentIntegration, EditorIntegration and
SettingsOverlayView to call that shared function; pick one POSIX-safe quoting
strategy (use the existing "'\"'\"'" variant consistently), remove the local
copies, and ensure all callers reference the new utility symbol
(ShellQuoter.shellQuote or String.shellQuoted) so behavior is centralized and
consistent.
.../macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swift
Show resolved
Hide resolved
.../macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/TerminalDrawerModel.swift
Show resolved
Hide resolved
apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLIClient.swift
Show resolved
Hide resolved
| private func runExecutable( | ||
| executablePath: String, | ||
| args: [String], | ||
| allowNonZeroExit: Bool, | ||
| cwd: String? | ||
| ) async throws -> CLIResult { | ||
| try Task.checkCancellation() | ||
|
|
||
| let process = Process() | ||
| process.environment = HackCLILocator.buildEnvironment() | ||
| if let cwd, !cwd.isEmpty { | ||
| process.currentDirectoryURL = URL(fileURLWithPath: cwd) | ||
| } | ||
| process.executableURL = URL(fileURLWithPath: executablePath) | ||
| process.arguments = args | ||
|
|
||
| let stdoutPipe = Pipe() | ||
| let stderrPipe = Pipe() | ||
| process.standardOutput = stdoutPipe | ||
| process.standardError = stderrPipe | ||
|
|
||
| return try await withTaskCancellationHandler(operation: { | ||
| do { | ||
| try process.run() | ||
| } catch { | ||
| stdoutPipe.fileHandleForReading.closeFile() | ||
| stderrPipe.fileHandleForReading.closeFile() | ||
| throw HackCLIError.commandFailed(exitCode: 127, stderr: error.localizedDescription) | ||
| } | ||
|
|
||
| async let stdoutData = stdoutPipe.fileHandleForReading.readToEnd() | ||
| async let stderrData = stderrPipe.fileHandleForReading.readToEnd() | ||
| let exitCode = await Task.detached(priority: nil) { | ||
| process.waitUntilExit() | ||
| return Int(process.terminationStatus) | ||
| }.value | ||
|
|
||
| let stdoutBytes: Data? | ||
| let stderrBytes: Data? | ||
|
|
||
| do { | ||
| stdoutBytes = try await stdoutData | ||
| } catch { | ||
| stdoutBytes = nil | ||
| } | ||
|
|
||
| do { | ||
| stderrBytes = try await stderrData | ||
| } catch { | ||
| stderrBytes = nil | ||
| } | ||
|
|
||
| try Task.checkCancellation() | ||
|
|
||
| let stdout = String(decoding: stdoutBytes ?? Data(), as: UTF8.self) | ||
| let stderr = String(decoding: stderrBytes ?? Data(), as: UTF8.self) | ||
| if exitCode != 0 && !allowNonZeroExit { | ||
| throw HackCLIError.commandFailed( | ||
| exitCode: exitCode, | ||
| stderr: stderr.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| ) | ||
| } | ||
|
|
||
| return CLIResult(stdout: stdout, stderr: stderr, exitCode: exitCode) | ||
| }, onCancel: { | ||
| if process.isRunning { | ||
| process.terminate() | ||
| } | ||
| stdoutPipe.fileHandleForReading.closeFile() | ||
| stderrPipe.fileHandleForReading.closeFile() | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
runExecutable is a near-complete duplicate of run.
The body of runExecutable (lines 575-646) is functionally identical to run (lines 345-423) — the only difference is how the executable path and arguments are resolved. This ~70-line duplication is a maintenance burden: any bug fix or improvement (e.g., cancellation handling, pipe management) must be applied in both places.
Consider extracting the shared process-execution logic into a single private helper and having both run and runExecutable delegate to it after resolving the executable.
Sketch
+ private func executeProcess(
+ executableURL: URL,
+ arguments: [String],
+ environment: [String: String],
+ allowNonZeroExit: Bool,
+ cwd: String?
+ ) async throws -> CLIResult {
+ // … shared pipe/process/cancellation logic …
+ }
+
private func run(…) async throws -> CLIResult {
let environment = HackCLILocator.buildEnvironment()
let (url, args) = // resolve hack executable
- // … inline process logic …
+ return try await executeProcess(executableURL: url, arguments: args, environment: environment, …)
}
private func runExecutable(…) async throws -> CLIResult {
- // … duplicated process logic …
+ return try await executeProcess(executableURL: URL(fileURLWithPath: executablePath), arguments: args, …)
}🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 629-629: Prefer failable String(bytes:encoding:) initializer when converting Data to String
(optional_data_string_conversion)
[Warning] 630-630: Prefer failable String(bytes:encoding:) initializer when converting Data to String
(optional_data_string_conversion)
🤖 Prompt for AI Agents
In
`@apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLIClient.swift`
around lines 575 - 646, The runExecutable implementation duplicates run; extract
the shared process execution flow (Process setup after executable/args
resolution, with Task.checkCancellation, Pipe creation,
withTaskCancellationHandler that runs the process, reads stdout/stderr async,
waits for exit, handles cancellation by terminating process and closing pipes,
and constructs CLIResult or throws HackCLIError) into a single private helper
(e.g., runProcess(using: Process, allowNonZeroExit:) or
runProcess(executableURL: URL, args: [String], allowNonZeroExit: Bool)) and have
both run and runExecutable resolve the executablePath/args, create or configure
the Process, then delegate to that helper; preserve existing behaviors:
environment via HackCLILocator.buildEnvironment(), cwd handling, proper
pipe/fileHandle closing, cancellation checks, and error mapping so no behavior
changes occur.
| if let override = env["HACK_CLI_PATH"], fileManager.isExecutableFile(atPath: override) { | ||
| return override | ||
| if let resolvedOverride = normalizeHackCandidate(override, env: env) { | ||
| return resolvedOverride | ||
| } | ||
| } |
There was a problem hiding this comment.
User-specified HACK_CLI_PATH override is silently discarded when normalization fails.
If the user sets HACK_CLI_PATH to a bun-wrapper script but bun isn't installed and no dist binary exists, normalizeHackCandidate returns nil and the override is ignored without any logging. This could make debugging difficult for users who explicitly configured this env var.
Consider logging a warning or returning early with a descriptive error when an explicit override fails normalization.
🤖 Prompt for AI Agents
In
`@apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLILocator.swift`
around lines 40 - 44, The code currently ignores an explicit
env["HACK_CLI_PATH"] override when normalizeHackCandidate(...) returns nil;
update the logic in HackCLILocator (the block that checks env["HACK_CLI_PATH"]
and fileManager.isExecutableFile(atPath:)) to surface this failure instead of
silently discarding it — either log a warning (including the original override
value and reason) or return an explicit error/result indicating normalization
failed for the provided path (use the same logger or error flow used elsewhere
in this module). Ensure the message references the override value and that
normalizeHackCandidate was the failing step so users see why their HACK_CLI_PATH
was rejected.
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 (2)
src/commands/ssh.ts (1)
419-419:⚠️ Potential issue | 🟡 MinorShell injection risk: validate
sessionNameat the call site.
opts.sessionNameis interpolated directly into a shell command string. While upstream callers currently validate againstSESSION_NAME_PATTERN,connectToSessionitself is a public-enough function that a future caller could bypass that check. Adding a guard here would provide defense-in-depth.🛡️ Proposed fix
async function connectToSession(opts: { readonly hostname: string; readonly user?: string; readonly port?: number; readonly sessionName: string; }): Promise<number> { + if (!SESSION_NAME_PATTERN.test(opts.sessionName)) { + p.log.error("Invalid session name"); + return 1; + } p.log.step(`Connecting to ${opts.sessionName}...`);src/daemon/routes/sessions.ts (1)
380-407:⚠️ Potential issue | 🟡 Minor
_opts.sessionNameis accepted but never used.
getConnectionInfoaccepts an optionalsessionNameparameter (line 381) and it's passed byhandleGetSession(line 185), but it's never referenced in the function body. This looks like an incomplete feature — the SSH command could include atmux attach -t <sessionName>suffix. Either implement it or remove the parameter to avoid confusion.
🤖 Fix all issues with AI agents
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/GlobalStatusStrip.swift`:
- Around line 126-128: The menu item Button("Home") sets model.selectedItem =
.home but selectorLabel (the computed property/function that maps selectedItem
to display text) returns "Dashboard" for the .home case; make them consistent by
either renaming the Button label to "Dashboard" or changing selectorLabel's
mapping for .home to return "Home" (or otherwise unify the terminology if you
intentionally want a distinction), updating the selectorLabel implementation and
the Button("Home") usage so both show the same user-facing label.
- Around line 413-438: stripLayoutSignature currently includes lastUpdatedText
which changes every clock tick and triggers the easeInOut layout animation
unnecessarily; update stripLayoutSignature (used by the layout/animation
identity) to exclude lastUpdatedText so only structural changes drive the
animation (keep "\(selectedKey)|\(selectorLabel)|\(actionKey)" as the
signature), or alternatively move lastUpdatedText into a separate view identity
that is not tied to the strip's animated signature; locate the computed property
stripLayoutSignature and remove lastUpdatedText from the returned string
(references: stripLayoutSignature, lastUpdatedText, selectorLabel,
model.selectedItem, model.projectLifecycleActions).
🧹 Nitpick comments (8)
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/GhosttyTerminalSession.swift (2)
66-76: Branch normalization doesn't apply whenModeis constructed externally.The normalization on lines 68-69 is good, but
init(project:mode:)(line 78) andinit(project:mode:initialCommand:)(line 84) accept a pre-builtModethat may containbranch: "". The defensive!branch.isEmptyguard inresolveCommandcovers this at the command-building layer, so there's no bug — but consider extracting the normalization into aModefactory or static method so all construction sites produce a consistently normalized value.Also, the expression on line 69 reads a bit indirectly. A more idiomatic Swift alternative:
♻️ Optional: clearer normalization
- let normalizedBranch = branch?.trimmingCharacters(in: .whitespacesAndNewlines) - let resolvedBranch = (normalizedBranch?.isEmpty == false) ? normalizedBranch : nil + let resolvedBranch = branch + .map { $0.trimmingCharacters(in: .whitespacesAndNewlines) } + .flatMap { $0.isEmpty ? nil : $0 }
309-343: Branch-aware argument construction is duplicated across the two code paths.Lines 322-325 and 334-337 repeat the same conditional
--branchappend logic. This is fine for now, but if more flags are added later (e.g.,--follow,--since), maintaining two copies will be error-prone. Consider building the args array once before theif let hackPathbranch:♻️ Optional: deduplicate argument construction
case let .logs(path, branch): if path.isEmpty { return TerminalCommand( executableURL: URL(fileURLWithPath: "/usr/bin/env"), arguments: ["echo", "Missing project path"], environment: environment, workingDirectory: nil ) } + var logArgs = ["logs", "--pretty", "--path", path] + if let branch, !branch.isEmpty { + logArgs.append(contentsOf: ["--branch", branch]) + } + if let hackPath = HackCLILocator.resolveHackExecutable(in: environment) { - var args = ["logs", "--pretty", "--path", path] - if let branch, !branch.isEmpty { - args.append(contentsOf: ["--branch", branch]) - } return TerminalCommand( executableURL: URL(fileURLWithPath: hackPath), - arguments: args, + arguments: logArgs, environment: environment, workingDirectory: URL(fileURLWithPath: path) ) } - var args = ["hack", "logs", "--pretty", "--path", path] - if let branch, !branch.isEmpty { - args.append(contentsOf: ["--branch", branch]) - } return TerminalCommand( executableURL: URL(fileURLWithPath: "/usr/bin/env"), - arguments: args, + arguments: ["hack"] + logArgs, environment: environment, workingDirectory: URL(fileURLWithPath: path) )src/daemon/routes/sessions.ts (3)
95-98: ValidatesessionIdformat before use as defense-in-depth.The
namefield is validated againstSESSION_NAME_PATTERNduring creation, butsessionIdextracted from the URL path is used as-is in all subsequent tmux commands. WhilefindSessionguards against operating on non-existent sessions, adding an early format check here would reject malformed input sooner and keep validation consistent.Proposed fix
const sessionId = segments[2]; if (!sessionId) { return jsonResponse({ error: "missing_session_id" }, 400); } + if (!SESSION_NAME_PATTERN.test(sessionId)) { + return jsonResponse({ error: "invalid_session_id" }, 400); + }
106-118: Sub-route matching doesn't enforce exact segment count.Lines 106, 111, and 116 check
segments[3]without also verifyingsegments.length === 4. A request to/v1/sessions/foo/stop/extra/segmentswould still match thestoproute. This is unlikely to cause issues in practice but tightening the checks prevents unexpected matches.Proposed fix
- if (segments[3] === "stop" && opts.req.method === "POST") { + if (segments.length === 4 && segments[3] === "stop" && opts.req.method === "POST") { return await handleStopSession({ sessionId }); } - if (segments[3] === "exec" && opts.req.method === "POST") { + if (segments.length === 4 && segments[3] === "exec" && opts.req.method === "POST") { return await handleExecSession({ req: opts.req, sessionId }); } - if (segments[3] === "input" && opts.req.method === "POST") { + if (segments.length === 4 && segments[3] === "input" && opts.req.method === "POST") { return await handleInputSession({ req: opts.req, sessionId }); }
158-162: Consider usingletor building args inline since the array is mutated.
argsis declaredconstbut conditionally mutated via.push(). While technically valid in JS, building the full command inline is clearer about intent and avoids mutation.Proposed fix
- const args = ["tmux", "new-session", "-d", "-s", name]; - if (cwd) { - args.push("-c", cwd); - } - - const result = await exec(args, { stdin: "ignore" }); + const result = await exec( + [ + "tmux", "new-session", "-d", "-s", name, + ...(cwd ? ["-c", cwd] : []), + ], + { stdin: "ignore" } + );apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/GlobalStatusStrip.swift (3)
21-21: Redundant ternary — both branches return10.HStack(spacing: placement == .titlebar ? 10 : 10)This condition always evaluates to
10. Either simplify to a constant or differentiate the spacing values if different spacing was intended for each placement.Proposed fix
- HStack(spacing: placement == .titlebar ? 10 : 10) { + HStack(spacing: 10) {
377-390:RelativeDateTimeFormatteris re-allocated on every access.This computed property is evaluated at least twice per body pass (directly in the view and via
stripLayoutSignature). Consider hoisting the formatter to astatic letto avoid repeated allocation.Proposed fix
+ private static let relativeFormatter: RelativeDateTimeFormatter = { + let f = RelativeDateTimeFormatter() + f.unitsStyle = .abbreviated + return f + }() + private var lastUpdatedText: String? { guard let date = model.lastUpdated else { return nil } - let formatter = RelativeDateTimeFormatter() - formatter.unitsStyle = .abbreviated let now = Date() if date > now { return "Updated just now" } let delta = now.timeIntervalSince(date) if delta < 5 { return "Updated just now" } - return "Updated \(formatter.localizedString(for: date, relativeTo: now))" + return "Updated \(Self.relativeFormatter.localizedString(for: date, relativeTo: now))" }
97-114: Titlebar-prefixed colors applied to the ellipsis icon in all placements.
titlebarIconForegroundandtitlebarIconStroke(Lines 100, 105) are used for the ellipsis menu button, which is rendered in both.contentand.titlebarplacements. If these colors are intentionally shared, consider dropping the "titlebar" prefix to avoid confusion. If they were meant to be titlebar-only, the styling should be conditional onplacement.
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/GlobalStatusStrip.swift
Outdated
Show resolved
Hide resolved
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/GlobalStatusStrip.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swift`:
- Around line 931-946: applyGlobalSettings currently ignores the Bool results
from model.setGlobalConfig (like savePreferences does), so failures silently
continue to model.refresh() and loadConfigFromDisk(); change applyGlobalSettings
to await both setGlobalConfig calls, collect their Bool returns (e.g. into
[result1, result2]) and use allSatisfy { $0 } to verify success, and if any fail
avoid calling model.refresh() / loadConfigFromDisk() (and optionally surface an
error/return early); mirror the savePreferences pattern and keep isLoadingConfig
lifecycle (defer) around the new logic.
- Around line 1082-1101: saveSupervisorSettings currently awaits three
model.setGlobalConfig calls but ignores their return results and proceeds to
await model.refresh() and loadConfigFromDisk even if a save failed; update
saveSupervisorSettings to capture each await model.setGlobalConfig(...) result
(for keys "controlPlane.supervisor.enabled",
"controlPlane.supervisor.maxConcurrentJobs",
"controlPlane.supervisor.logsMaxBytes"), check for failure (e.g., returned false
or an error), and only call model.refresh() and loadConfigFromDisk() if all
saves succeeded, otherwise set an appropriate error state or early-return so
failures are handled before refreshing.
In `@src/daemon/routes/sessions.ts`:
- Around line 346-352: The code that sets createdAt uses
Number.parseInt(created, 10) and then new Date(...).toISOString(), which will
throw a RangeError if created is non-numeric; update the logic around the
created variable in the sessions route so you validate the parsed value (e.g.,
const ts = Number.parseInt(created, 10)) and only call new Date(ts *
1000).toISOString() when Number.isFinite(ts) (or !Number.isNaN(ts)); otherwise
set createdAt to null (or a safe fallback). Modify the block that constructs the
session object (the createdAt assignment) to use this validated parsed timestamp
to prevent the RangeError.
🧹 Nitpick comments (10)
src/control-plane/extensions/gateway/commands.ts (2)
130-155: Consider usinginterfacefor the Args object shapes.The coding guidelines prefer
interfacefor object shapes. The union parse-result types must remaintypealiases, butTokenCreateArgs,TokenListArgs, andTokenRevokeArgsare plain object shapes.Additionally, the three parse-result types are structurally identical — a generic would reduce duplication:
♻️ Suggested refactor
-type TokenCreateArgs = { +interface TokenCreateArgs { readonly label?: string; readonly scope: GatewayTokenScope; readonly json: boolean; -}; +} -type TokenListArgs = { +interface TokenListArgs { readonly json: boolean; -}; +} -type TokenRevokeArgs = { +interface TokenRevokeArgs { readonly tokenId: string; readonly json: boolean; -}; +} -type TokenListParseResult = - | { readonly ok: true; readonly value: TokenListArgs } - | { readonly ok: false; readonly error: string }; - -type TokenRevokeParseResult = - | { readonly ok: true; readonly value: TokenRevokeArgs } - | { readonly ok: false; readonly error: string }; - -type TokenCreateParseResult = - | { readonly ok: true; readonly value: TokenCreateArgs } - | { readonly ok: false; readonly error: string }; +type ParseResult<T> = + | { readonly ok: true; readonly value: T } + | { readonly ok: false; readonly error: string };As per coding guidelines, "Prefer
interfacefor defining object shapes in TypeScript".
164-172:_tokenparameter intakeValueis unused — remove it.The
_tokenparameter serves no purpose. Removing it simplifies the helper.♻️ Suggested fix
- const takeValue = ( - _token: string, - value: string | undefined - ): string | null => { + const takeValue = (value: string | undefined): string | null => { if (!value || value.startsWith("-")) { return null; } return value; };Then update call sites (lines 205, 224):
- const value = takeValue(token, opts.args[i + 1]); + const value = takeValue(opts.args[i + 1]);apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLIClient.swift (1)
280-296: Potential quadratic cost on large or brace-heavy output.
extractJsonSnippetsattemptsextractBalancedJsonfrom every{and[in the text. For typical CLI output this is fine, but if a command ever emits large or deeply nested output, this could become expensive. Consider short-circuiting after the first successful decode indecodeLenient(you alreadyreturnon success, so this is mostly fine), or adding an early break/limit on the number of snippets tried.As-is this is acceptable for the expected output sizes.
apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLILocator.swift (2)
57-65: PATH-scan candidate silently dropped when normalization returnsnil.When
normalizeHackCandidatereturnsnilfor a PATH candidate (bun required but unavailable and no dist binary), the loop continues without logging. If every candidate in PATH is a bun wrapper without a dist fallback, the function returnsnilon Line 54 with no diagnostic info. This may make it difficult for users to understand why the CLI isn't found.Consider logging the skipped candidate or collecting diagnostics to surface the reason for failure.
68-94: File is read twice for wrapper scripts: once inbunIsRequiredByWrapperand once inresolveWrapperDistBinary.When
normalizeHackCandidatedetermines that bun is required (reads the file at Line 69) and then falls through toresolveWrapperDistBinary(reads the same file again at Line 76), the wrapper script content is read from disk twice. This is a minor I/O inefficiency.Consider refactoring to read the file once and pass the content to both checks.
♻️ Sketch
- private static func normalizeHackCandidate(_ candidate: String, env: [String: String]) -> String? { - if bunIsRequiredByWrapper(candidate), - resolveExecutable(named: "bun", in: env) == nil { - if let fallback = resolveWrapperDistBinary(candidate), FileManager.default.isExecutableFile(atPath: fallback) { + private static func normalizeHackCandidate(_ candidate: String, env: [String: String]) -> String? { + guard let content = try? String(contentsOfFile: candidate, encoding: .utf8) else { + return candidate + } + if content.contains("exec bun "), + resolveExecutable(named: "bun", in: env) == nil { + if let fallback = resolveWrapperDistBinary(content: content), FileManager.default.isExecutableFile(atPath: fallback) { return fallback } return nil } return candidate }apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/DashboardModel.swift (1)
632-640: Terminal fallback heuristic relies on English substrings in error messages.
shouldFallbackToTerminalmatches against English keywords like"sudo","permission denied", etc. If the CLI ever emits localized or differently-worded errors, this heuristic could miss them. Since these are likely from system-level CLI commands (not user-facing localization), this is acceptable for now but worth noting.src/daemon/routes/sessions.ts (1)
377-382:findSessionlists all sessions to find one by name.This works but performs a full
tmux list-sessionsfor every lookup. Each handler (create, get, stop, exec, input) callsfindSession, so a single request to e.g./v1/sessions/:id/execinvokes two full session listings (once for existence check, once potentially elsewhere). For typical session counts this is fine, but if performance becomes a concern,tmux has-session -t <name>is a lighter probe.src/commands/ssh.ts (1)
15-16:SESSION_NAME_PATTERNandparseTmuxSessionFieldsare duplicated withsrc/daemon/routes/sessions.ts.
SESSION_NAME_PATTERNdiffers:/^[\w.-]+$/here (allows dots) vs/^[\w-]+$/insessions.ts(no dots). If this is intentional (CLI is more permissive, daemon API is stricter), consider documenting the rationale. If not, they should be aligned.parseTmuxSessionFieldsis identical in both files and could be extracted to a shared module (e.g.,src/lib/tmux.ts).TmuxSessioninterface is also duplicated with different shapes (3 fields here, 5 insessions.ts).Also applies to: 487-501
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectDetailView.swift (1)
898-912:serviceNamesrecomputesserviceStatus(for:)on every sort comparison.
serviceStatus(for:)performs dictionary lookups and container filtering. SinceserviceNamesis a computed property accessed from multiple call sites (header pills,servicesSection,runningServiceCount), the sort work multiplies. For typical service counts this is fine, but if the list grows, consider caching the statuses.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swift (1)
2653-2709:GlobalConfigSnapshot.load()performs synchronous file I/O.This is called from
asyncfunctions on the main actor (via@Environment(DashboardModel.self)). For a small JSON config file this is likely acceptable, but worth noting — if the file grows or lives on a slow volume, it could cause UI hitches.
| private func applyGlobalSettings() async { | ||
| isLoadingConfig = true | ||
| defer { isLoadingConfig = false } | ||
|
|
||
| await model.setGlobalConfig( | ||
| key: "controlPlane.daemon.launchd.runAtLoad", | ||
| value: launchAtLoad ? "true" : "false" | ||
| ) | ||
| await model.setGlobalConfig( | ||
| key: "controlPlane.gateway.bind", | ||
| value: gatewayBind | ||
| ) | ||
|
|
||
| await model.refresh() | ||
| await loadConfigFromDisk() | ||
| } |
There was a problem hiding this comment.
applyGlobalSettings ignores setGlobalConfig return values, unlike savePreferences.
savePreferences was updated to check all return values via allSatisfy, but applyGlobalSettings and saveSupervisorSettings still fire-and-forget. This means failed saves silently proceed to refresh() + loadConfigFromDisk(), giving the user a false impression of success.
Proposed fix for applyGlobalSettings
- await model.setGlobalConfig(
+ let didSaveLaunch = await model.setGlobalConfig(
key: "controlPlane.daemon.launchd.runAtLoad",
value: launchAtLoad ? "true" : "false"
)
- await model.setGlobalConfig(
+ let didSaveBind = await model.setGlobalConfig(
key: "controlPlane.gateway.bind",
value: gatewayBind
)
+ guard didSaveLaunch, didSaveBind else { return }
await model.refresh()
await loadConfigFromDisk()📝 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.
| private func applyGlobalSettings() async { | |
| isLoadingConfig = true | |
| defer { isLoadingConfig = false } | |
| await model.setGlobalConfig( | |
| key: "controlPlane.daemon.launchd.runAtLoad", | |
| value: launchAtLoad ? "true" : "false" | |
| ) | |
| await model.setGlobalConfig( | |
| key: "controlPlane.gateway.bind", | |
| value: gatewayBind | |
| ) | |
| await model.refresh() | |
| await loadConfigFromDisk() | |
| } | |
| private func applyGlobalSettings() async { | |
| isLoadingConfig = true | |
| defer { isLoadingConfig = false } | |
| let didSaveLaunch = await model.setGlobalConfig( | |
| key: "controlPlane.daemon.launchd.runAtLoad", | |
| value: launchAtLoad ? "true" : "false" | |
| ) | |
| let didSaveBind = await model.setGlobalConfig( | |
| key: "controlPlane.gateway.bind", | |
| value: gatewayBind | |
| ) | |
| guard didSaveLaunch, didSaveBind else { return } | |
| await model.refresh() | |
| await loadConfigFromDisk() | |
| } |
🤖 Prompt for AI Agents
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swift`
around lines 931 - 946, applyGlobalSettings currently ignores the Bool results
from model.setGlobalConfig (like savePreferences does), so failures silently
continue to model.refresh() and loadConfigFromDisk(); change applyGlobalSettings
to await both setGlobalConfig calls, collect their Bool returns (e.g. into
[result1, result2]) and use allSatisfy { $0 } to verify success, and if any fail
avoid calling model.refresh() / loadConfigFromDisk() (and optionally surface an
error/return early); mirror the savePreferences pattern and keep isLoadingConfig
lifecycle (defer) around the new logic.
| private func saveSupervisorSettings() async { | ||
| isLoadingConfig = true | ||
| defer { isLoadingConfig = false } | ||
|
|
||
| await model.setGlobalConfig( | ||
| key: "controlPlane.supervisor.enabled", | ||
| value: enabled ? "true" : "false" | ||
| ) | ||
| await model.setGlobalConfig( | ||
| key: "controlPlane.supervisor.maxConcurrentJobs", | ||
| value: String(maxConcurrentJobs) | ||
| ) | ||
| await model.setGlobalConfig( | ||
| key: "controlPlane.supervisor.logsMaxBytes", | ||
| value: String(logsMaxBytesValue) | ||
| ) | ||
|
|
||
| await model.refresh() | ||
| await loadConfigFromDisk() | ||
| } |
There was a problem hiding this comment.
saveSupervisorSettings also ignores setGlobalConfig return values.
Same pattern as applyGlobalSettings — three config saves with no failure checking before calling refresh().
Proposed fix
- await model.setGlobalConfig(
+ let a = await model.setGlobalConfig(
key: "controlPlane.supervisor.enabled",
value: enabled ? "true" : "false"
)
- await model.setGlobalConfig(
+ let b = await model.setGlobalConfig(
key: "controlPlane.supervisor.maxConcurrentJobs",
value: String(maxConcurrentJobs)
)
- await model.setGlobalConfig(
+ let c = await model.setGlobalConfig(
key: "controlPlane.supervisor.logsMaxBytes",
value: String(logsMaxBytesValue)
)
+ guard a, b, c else { return }
await model.refresh()
await loadConfigFromDisk()🤖 Prompt for AI Agents
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swift`
around lines 1082 - 1101, saveSupervisorSettings currently awaits three
model.setGlobalConfig calls but ignores their return results and proceeds to
await model.refresh() and loadConfigFromDisk even if a save failed; update
saveSupervisorSettings to capture each await model.setGlobalConfig(...) result
(for keys "controlPlane.supervisor.enabled",
"controlPlane.supervisor.maxConcurrentJobs",
"controlPlane.supervisor.logsMaxBytes"), check for failure (e.g., returned false
or an error), and only call model.refresh() and loadConfigFromDisk() if all
saves succeeded, otherwise set an appropriate error state or early-return so
failures are handled before refreshing.
| path: path || null, | ||
| windows: Number.parseInt(windows ?? "1", 10), | ||
| createdAt: created | ||
| ? new Date(Number.parseInt(created, 10) * 1000).toISOString() | ||
| : null, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential RangeError if session_created field is non-numeric.
Number.parseInt(created, 10) on a non-numeric string returns NaN, and new Date(NaN * 1000).toISOString() throws a RangeError. The created ? truthiness check guards against empty/null but not against a malformed value.
🛡️ Proposed fix
- createdAt: created
- ? new Date(Number.parseInt(created, 10) * 1000).toISOString()
- : null,
+ const epoch = created ? Number.parseInt(created, 10) : NaN
+ ...
+ createdAt: Number.isFinite(epoch)
+ ? new Date(epoch * 1000).toISOString()
+ : null,📝 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.
| path: path || null, | |
| windows: Number.parseInt(windows ?? "1", 10), | |
| createdAt: created | |
| ? new Date(Number.parseInt(created, 10) * 1000).toISOString() | |
| : null, | |
| }); | |
| } | |
| path: path || null, | |
| windows: Number.parseInt(windows ?? "1", 10), | |
| createdAt: | |
| created && Number.isFinite(Number.parseInt(created, 10)) | |
| ? new Date(Number.parseInt(created, 10) * 1000).toISOString() | |
| : null, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/daemon/routes/sessions.ts` around lines 346 - 352, The code that sets
createdAt uses Number.parseInt(created, 10) and then new
Date(...).toISOString(), which will throw a RangeError if created is
non-numeric; update the logic around the created variable in the sessions route
so you validate the parsed value (e.g., const ts = Number.parseInt(created, 10))
and only call new Date(ts * 1000).toISOString() when Number.isFinite(ts) (or
!Number.isNaN(ts)); otherwise set createdAt to null (or a safe fallback). Modify
the block that constructs the session object (the createdAt assignment) to use
this validated parsed timestamp to prevent the RangeError.
## <small>1.6.1 (2026-02-13)</small> * Audit large repo changes ([a2d2788](a2d2788)) * Find Xcode simulator skill ([60d9ef9](60d9ef9)) * Find Xcode simulator skill ([8bc37b2](8bc37b2)) * Fix project pill layout and tabs ([750682c](750682c)) * Fix review-agent findings ([8cad0ee](8cad0ee)) * Fix ticket card layout and startup ([9b16700](9b16700)) * Fix ticket card layout and startup ([ffb1958](ffb1958)) * Merge main into review-session-management-workflow ([17636d9](17636d9)) * Merge pull request #8 from hack-dance/review-session-management-workflow ([6760c72](6760c72)), closes [#8](#8) * Merge pull request #9 from hack-dance/app-updates ([c30a589](c30a589)), closes [#9](#9) * Merge remote-tracking branch 'origin/app-updates' into app-updates ([031a226](031a226)) * Restore global terminal panel ([d7bf346](d7bf346)) * Update CLI docs and commands ([faeb629](faeb629)) * fix: address PR review findings and stabilize builds ([07c21ea](07c21ea)) * fix: resolve rebase integration regressions ([e1c5cce](e1c5cce)) * fix(macos): resolve dashboard model mismatch build break ([f229ffa](f229ffa)) * fix(macos): stabilize status strip identity and labels ([8d2ab8c](8d2ab8c)) * test: cover runtime meta failures ([c2b56f6](c2b56f6)) * test: satisfy ProjectMeta type ([5ae817a](5ae817a)) * chore(macos): add SwiftUI previews + force setup flag ([56d4386](56d4386))
1 participant
Summary by CodeRabbit
New Features
Improvements