Polish macOS dashboard UI#6262
Conversation
kodjima33
force-pushed
the
nik/macos-ui-polish
branch
from
27f7dad to
37246c1
Compare
Greptile SummaryThis PR is a macOS desktop UI polish pass that tightens spacing, upgrades corner radii, and introduces a shared design-token layer ( Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App Launch] -->|"--export-views / --export-fullpages flag"| B[ViewExporter.run]
A -->|normal launch| Z[DesktopHomeView]
B --> C{Mode?}
C -->|--export-views| D[runBatch standalone\ncount=15]
C -->|--export-fullpages| E[runBatch fullpage\ncount=11]
C -->|--export-single i dir| F[exportView single standalone]
C -->|--export-fullpage-single i dir| G[exportView single fullpage]
D -->|"spawn subprocess per view\n(0..14)"| F
E -->|"spawn subprocess per view\n(0..10)"| G
F --> H[NSHostingView render]
G --> H
H --> I[Write PNG]
H --> J[Write PDF]
D --> K[convertPDFsToSVG\n/opt/homebrew/bin/pdf2svg]
E --> K
Z --> L[SidebarView]
Z --> M[Content Area\nOmiChrome.windowRadius=26pt\ngradient background]
M --> N[MemoriesPage\nMemoryGraphInlineCard above list\n2-line truncated cards]
M --> O[ChatPage\nomiPanel input area]
M --> P[ConversationsPage\nomiControlSurface chips]
M --> Q[DashboardPage\nspacing 28pt / padding 30pt]
Reviews (1): Last reviewed commit: "Refine memories page density" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for i in 0..<count { | ||
| let viewName: String | ||
| if flag == "--export-single" { | ||
| viewName = standaloneViewAt(i)?.0 ?? "unknown-\(i)" | ||
| } else { | ||
| viewName = fullPageViewAt(i)?.0 ?? "unknown-\(i)" | ||
| } | ||
| NSLog("ViewExporter: [\(i+1)/\(count)] Spawning subprocess for \(viewName)...") | ||
|
|
||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: executablePath) | ||
| process.arguments = [flag, "\(i)", dir] | ||
| process.standardError = Pipe() | ||
| process.standardOutput = FileHandle.nullDevice | ||
|
|
||
| do { | ||
| try process.run() | ||
| process.waitUntilExit() | ||
|
|
||
| if process.terminationStatus == 0 { | ||
| NSLog("ViewExporter: [\(i+1)/\(count)] \(viewName) OK") | ||
| exportedCount += 1 | ||
| } else { | ||
| NSLog( | ||
| "ViewExporter: [\(i+1)/\(count)] \(viewName) FAILED (exit \(process.terminationStatus))" | ||
| ) | ||
| failedCount += 1 | ||
| crashedViews.append(viewName) | ||
| } | ||
| } catch { | ||
| NSLog("ViewExporter: [\(i+1)/\(count)] \(viewName) FAILED (\(error.localizedDescription))") | ||
| failedCount += 1 | ||
| crashedViews.append(viewName) | ||
| } |
There was a problem hiding this comment.
process.standardError = Pipe() captures stderr but the Pipe's fileHandleForReading is never read. On macOS a Pipe has a 64 KB kernel buffer. If any subprocess writes more than that to stderr (e.g. from SwiftUI layout warnings or added logging), the subprocess blocks on write() while the parent is blocked on process.waitUntilExit(), causing a permanent hang.
For a quick fix, drain stderr on a background thread or just send it to /dev/null:
| for i in 0..<count { | |
| let viewName: String | |
| if flag == "--export-single" { | |
| viewName = standaloneViewAt(i)?.0 ?? "unknown-\(i)" | |
| } else { | |
| viewName = fullPageViewAt(i)?.0 ?? "unknown-\(i)" | |
| } | |
| NSLog("ViewExporter: [\(i+1)/\(count)] Spawning subprocess for \(viewName)...") | |
| let process = Process() | |
| process.executableURL = URL(fileURLWithPath: executablePath) | |
| process.arguments = [flag, "\(i)", dir] | |
| process.standardError = Pipe() | |
| process.standardOutput = FileHandle.nullDevice | |
| do { | |
| try process.run() | |
| process.waitUntilExit() | |
| if process.terminationStatus == 0 { | |
| NSLog("ViewExporter: [\(i+1)/\(count)] \(viewName) OK") | |
| exportedCount += 1 | |
| } else { | |
| NSLog( | |
| "ViewExporter: [\(i+1)/\(count)] \(viewName) FAILED (exit \(process.terminationStatus))" | |
| ) | |
| failedCount += 1 | |
| crashedViews.append(viewName) | |
| } | |
| } catch { | |
| NSLog("ViewExporter: [\(i+1)/\(count)] \(viewName) FAILED (\(error.localizedDescription))") | |
| failedCount += 1 | |
| crashedViews.append(viewName) | |
| } | |
| let process = Process() | |
| process.executableURL = URL(fileURLWithPath: executablePath) | |
| process.arguments = [flag, "\(i)", dir] | |
| process.standardError = FileHandle.nullDevice | |
| process.standardOutput = FileHandle.nullDevice |
If you want to capture subprocess output for debugging, use process.standardError = Pipe() and consume pipe.fileHandleForReading.readDataToEndOfFile() after waitUntilExit(), or read it asynchronously before waiting.
| return (entry.0, entry.1(), entry.2) | ||
| } | ||
|
|
||
| static var standaloneViewCount: Int { 15 } |
There was a problem hiding this comment.
standaloneViewCount = 15 and fullPageCount = 11 (line 307) are magic numbers that must match the actual number of entries in standaloneViewAt / fullPageViewAt. If someone adds a view to one of those arrays without updating the matching count, the batch exporter silently skips the new view — the out-of-range index causes the subprocess to return 1 and crashedViews gets a spurious entry.
Consider deriving the counts from the arrays themselves. One approach is to move each registry into a static stored property so .count is accessible:
// Instead of a local `let views: [...] = [...]` inside the function,
// make it a static stored property:
private static let standaloneViews: [(String, () -> AnyView, CGSize)] = [...]
static var standaloneViewCount: Int { standaloneViews.count }
static func standaloneViewAt(_ index: Int) -> (String, AnyView, CGSize)? { ... }At minimum, add an assert(standaloneViewCount == /* expected */ 15) or a unit test that validates the count.
| let pdf2svgPath = "/opt/homebrew/bin/pdf2svg" | ||
| guard FileManager.default.fileExists(atPath: pdf2svgPath) else { |
There was a problem hiding this comment.
pdf2svg path hard-codes Apple Silicon Homebrew prefix
/opt/homebrew/bin/pdf2svg is the Homebrew prefix for Apple Silicon. Intel Macs use /usr/local/bin/pdf2svg, so the SVG conversion step is silently skipped on Intel hardware with the log message "pdf2svg not found".
| let pdf2svgPath = "/opt/homebrew/bin/pdf2svg" | |
| guard FileManager.default.fileExists(atPath: pdf2svgPath) else { | |
| let pdf2svgPath: String = { | |
| // Homebrew on Apple Silicon lives in /opt/homebrew; Intel Macs use /usr/local | |
| let asSilicon = "/opt/homebrew/bin/pdf2svg" | |
| let intel = "/usr/local/bin/pdf2svg" | |
| return FileManager.default.fileExists(atPath: asSilicon) ? asSilicon : intel | |
| }() |
| static let warning = Color(hex: 0xF59E0B) // Amber | ||
| static let error = Color(hex: 0xEF4444) // Red | ||
| static let info = Color(hex: 0x3B82F6) // Blue | ||
| static let amber = Color(hex: 0xF59E0B) // Same as warning, for starred items |
There was a problem hiding this comment.
amber duplicates warning — same hex value 0xF59E0B
Both OmiColors.warning (line 32) and the new OmiColors.amber resolve to the same colour. Having two names for the same semantic value creates confusion about which to use and risks them drifting in the future.
Consider either re-using warning for starred items and documenting the dual purpose, or defining amber as an alias:
static let amber: Color = warning // same amber/warning yellowThis makes the relationship explicit and avoids silent divergence.
* Polish macOS dashboard UI * Refine memories page density
1 participant
Summary
Verification
/Applications/Omi Dev.applocallyfull-memories.pngexportNotes
main; PR path only