Repository navigation
Add AppKit signal todo lab - #8388
lawrencecchen wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds an AppKit Signals Lab with reactive state, task simulation, metrics, filtering, inspector controls, localized UI, menu/debug-command access, and targeted screenshot capture for the auxiliary window. ChangesAppKit Signals Lab
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DebugClient
participant ControlCommandCoordinator
participant TerminalController
participant DebugWindowsCoordinator
participant AppKitSignalLabWindowController
DebugClient->>ControlCommandCoordinator: debug.appkit_signal_lab.show
ControlCommandCoordinator->>TerminalController: controlDebugShowAppKitSignalLab()
TerminalController->>DebugWindowsCoordinator: showAppKitSignalLabWindow()
DebugWindowsCoordinator->>AppKitSignalLabWindowController: create or reuse and show
AppKitSignalLabWindowController-->>DebugClient: shown: true
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryAdds a DEBUG-only native AppKit experiment built on a Solid-style fine-grained reactive graph (signals, memos, effects with cleanup, batched propagation) and a three-pane operations dashboard that drives AppKit controls directly. No SwiftUI, Observation, or Combine is used. Entry points are gated by
Confidence Score: 5/5No new correctness bugs were found in this review; issues raised in earlier threads (missing guards and incomplete locale coverage) remain open but are unchanged. The reactive graph implementation is internally consistent — signals update their stored value immediately, memos detach and retrack dependencies on each run, effects dispose cleanly, and batch propagation correctly defers flush until batchDepth reaches zero. The control-socket integration for No files require special attention beyond what was already flagged in prior review threads. Important Files Changed
Reviews (2): Last reviewed commit: "Turn signals lab into todo app" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Owns synchronous dependency tracking and batched propagation for signals. | ||
| /// | ||
| /// `SignalGraph` follows Solid's fine-grained model: reading a signal inside an | ||
| /// effect or memo records the dependency automatically, and writing a distinct | ||
| /// value schedules only the observers that read it. All access is main-actor | ||
| /// isolated so AppKit controls can be updated directly without locks. | ||
| @MainActor | ||
| public final class SignalGraph { |
There was a problem hiding this comment.
#if DEBUG guards in production Sources
The PR description explicitly calls this a "DEBUG-only native AppKit experiment," and all entry points (DebugWindowsCoordinator.showAppKitSignalLabWindow(), controlDebugShowAppKitSignalLab(), the Debug menu item in cmuxApp.swift) are properly #if DEBUG-gated. However, the underlying implementation — SignalGraph, Signal, SignalMemo, SignalEffect, SignalEffectContext, SignalObserver, SignalDependency, WeakSignalObserver, AppKitSignalLabModel, AppKitSignalLabWindowController, and AppKitSignalLabViewController — all compile into the release binary as dead code because their source files in production Sources/ carry no #if DEBUG guard (only #if canImport(AppKit) where present). Per cmux-no-test-debug-seam-in-production-source, a facility with no production caller should be isolated in a dedicated debug file or folder behind a #if DEBUG guard; the folder exists here but the file-level guards are absent. The fix is to wrap each file in #if DEBUG … #endif, matching the pattern used in DebugWindowsCoordinator.swift.
Rule Used: Flag Swift files under a production Sources path (... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| "debug.signalLab.activity.advanced": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": {"stringUnit": {"state": "translated", "value": "Selected work item advanced in one batch."}}, | ||
| "ja": {"stringUnit": {"state": "translated", "value": "選択した作業項目を1回のバッチで進めました。"}} | ||
| } |
There was a problem hiding this comment.
The new debug.signalLab.* keys added to this catalog only provide en and ja entries. The existing catalog supports many additional locales (ar, bs, da, de, es, fr, it, ko, nl, pl, pt-BR, ru, tr, uk, zh-Hans, zh-Hant, and others visible in surrounding keys like about.appName). Per cmux-full-internationalization, catalog additions must include translated entries for every locale already supported by the touched catalog. All ~45 newly added keys follow the same two-locale pattern. If these strings are intended exclusively for the debug experiment and should never appear in production, the clean fix is to also add #if DEBUG guards to the implementation files (so the keys never ship in production catalog builds), otherwise translations for all supported locales are required.
Rule Used: Flag production user-facing text that is not fully... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Model/AppKitSignalLabModel.swift`:
- Around line 93-95: Update the task transition logic around Self.nextStatus in
AppKitSignalLabModel so any task whose next status is .complete has its progress
normalized to 1. Preserve the incremental progress for non-completed statuses
and ensure status and progress remain consistent.
In
`@Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Views/AppKitSignalLabViewController.swift`:
- Around line 488-491: Update visibleCountLabel and the filter-label formatting
around the affected view-controller code to use complete localized plural
messages, with explicit .one and .other catalog entries rather than appending
counts outside localization. Add matching entries for the visible-count and
every filter label in all supported locale catalogs, preserving the count value
and allowing locale-specific grammar and word order.
In `@Sources/TerminalController.swift`:
- Around line 12840-12848: Update the window selection logic around
explicitlyTargetedWindow so that when windowIdentifier is provided, only the
matching window is used; if no match exists, return or otherwise fail closed
without falling back to preferredWindow, candidateWindows heuristics,
NSApp.mainWindow, or NSApp.windows. Preserve the existing fallback chain only
when no explicit identifier is supplied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a48189ff-9a81-4c70-bde9-8f5711b2e4dd
📒 Files selected for processing (29)
Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Coordinator/DebugWindowsCoordinator.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Model/AppKitSignalLabFilter.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Model/AppKitSignalLabMetrics.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Model/AppKitSignalLabModel.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Model/AppKitSignalLabStatus.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Model/AppKitSignalLabTask.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Reactive/Signal.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Reactive/SignalDependency.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Reactive/SignalEffect.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Reactive/SignalEffectContext.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Reactive/SignalGraph.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Reactive/SignalMemo.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Reactive/SignalObserver.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Reactive/WeakSignalObserver.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Views/AppKitSignalLabViewController.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Views/SignalLabPulseView.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Windows/AppKitSignalLabWindowController.swiftPackages/macOS/CmuxAppKitSupportUI/Tests/CmuxAppKitSupportUITests/SignalGraphTests.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Debug/ControlCommandCoordinator+Debug.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Debug/ControlCommandCoordinator+Debug2.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Debug/ControlCommandCoordinator+DebugV1.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Debug/ControlDebugContext.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs+Debug.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorDebugV1Tests.swiftResources/Localizable.xcstringsSources/TerminalController+ControlDebugContext.swiftSources/TerminalController+DebugMethodNames.swiftSources/TerminalController.swiftSources/cmuxApp.swift
| visibleCountLabel.stringValue = String( | ||
| format: String(localized: "debug.signalLab.visibleCount", defaultValue: "%lld visible"), | ||
| Int64(tasks.count) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Localize the complete count labels with plural variants.
visibleCountLabel uses one key for all counts, while the filter labels append counts outside localization. Add complete .one/.other catalog entries for the visible count and each filter so translators control plural grammar and word order across every supported locale.
As per coding guidelines, user-facing text must use localized APIs and matching catalogs. As per path instructions, touched catalogs must cover every supported locale. Based on learnings, count strings should use explicit .one and .other keys.
Also applies to: 514-517
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Views/AppKitSignalLabViewController.swift`
around lines 488 - 491, Update visibleCountLabel and the filter-label formatting
around the affected view-controller code to use complete localized plural
messages, with explicit .one and .other catalog entries rather than appending
counts outside localization. Add matching entries for the visible-count and
every filter label in all supported locale catalogs, preserving the count value
and allowing locale-specific grammar and word order.
Sources: Coding guidelines, Path instructions, Learnings
| let explicitlyTargetedWindow = windowIdentifier.flatMap { identifier in | ||
| candidateWindows.first { $0.identifier?.rawValue == identifier } | ||
| } | ||
| let preferredWindow = [NSApp.keyWindow, NSApp.mainWindow] | ||
| .compactMap { $0 } | ||
| .first { candidateWindows.contains($0) } | ||
| let window = preferredWindow ?? candidateWindows.max { lhs, rhs in | ||
| let window = explicitlyTargetedWindow ?? preferredWindow ?? candidateWindows.max { lhs, rhs in | ||
| (lhs.frame.width * lhs.frame.height) < (rhs.frame.width * rhs.frame.height) | ||
| } ?? NSApp.mainWindow ?? NSApp.windows.first |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when an explicit window identifier is not found.
When windowIdentifier is provided but no matching window is found, the current logic falls back to capturing preferredWindow or another heuristic. This violates the path instruction to treat the explicit windowIdentifier as the single authoritative source of truth and fail closed if it is unavailable, avoiding "best effort" fallbacks that capture the wrong window.
🐛 Proposed fix
let explicitlyTargetedWindow = windowIdentifier.flatMap { identifier in
candidateWindows.first { $0.identifier?.rawValue == identifier }
}
+
+ if windowIdentifier != nil && explicitlyTargetedWindow == nil {
+ captureError = "No window found for explicit identifier"
+ return
+ }
+
let preferredWindow = [NSApp.keyWindow, NSApp.mainWindow]
.compactMap { $0 }
.first { candidateWindows.contains($0) }
let window = explicitlyTargetedWindow ?? preferredWindow ?? candidateWindows.max { lhs, rhs in
(lhs.frame.width * lhs.frame.height) < (rhs.frame.width * rhs.frame.height)
} ?? NSApp.mainWindow ?? NSApp.windows.first📝 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.
| let explicitlyTargetedWindow = windowIdentifier.flatMap { identifier in | |
| candidateWindows.first { $0.identifier?.rawValue == identifier } | |
| } | |
| let preferredWindow = [NSApp.keyWindow, NSApp.mainWindow] | |
| .compactMap { $0 } | |
| .first { candidateWindows.contains($0) } | |
| let window = preferredWindow ?? candidateWindows.max { lhs, rhs in | |
| let window = explicitlyTargetedWindow ?? preferredWindow ?? candidateWindows.max { lhs, rhs in | |
| (lhs.frame.width * lhs.frame.height) < (rhs.frame.width * rhs.frame.height) | |
| } ?? NSApp.mainWindow ?? NSApp.windows.first | |
| let explicitlyTargetedWindow = windowIdentifier.flatMap { identifier in | |
| candidateWindows.first { $0.identifier?.rawValue == identifier } | |
| } | |
| if windowIdentifier != nil && explicitlyTargetedWindow == nil { | |
| captureError = "No window found for explicit identifier" | |
| return | |
| } | |
| let preferredWindow = [NSApp.keyWindow, NSApp.mainWindow] | |
| .compactMap { $0 } | |
| .first { candidateWindows.contains($0) } | |
| let window = explicitlyTargetedWindow ?? preferredWindow ?? candidateWindows.max { lhs, rhs in | |
| (lhs.frame.width * lhs.frame.height) < (rhs.frame.width * rhs.frame.height) | |
| } ?? NSApp.mainWindow ?? NSApp.windows.first |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TerminalController.swift` around lines 12840 - 12848, Update the
window selection logic around explicitlyTargetedWindow so that when
windowIdentifier is provided, only the matching window is used; if no match
exists, return or otherwise fail closed without falling back to preferredWindow,
candidateWindows heuristics, NSApp.mainWindow, or NSApp.windows. Preserve the
existing fallback chain only when no explicit identifier is supplied.
Source: Path instructions
lawrencecchen
added
the
stale-revisit
2 participants
Implements a DEBUG-only native AppKit experiment based on Solid’s signal model: https://docs.solidjs.com/concepts/signals
The new main-actor signal graph provides writable signals, cached memos, effects with cleanup, equality suppression, dynamic dependency replacement, and batched propagation. A three-pane operations dashboard exercises the graph through filters, search, metrics, task selection, controls, a pulse chart, and an activity feed. The experiment contains no SwiftUI, Observation, or Combine dependencies.
Adds a Debug menu entry and
debug.appkit_signal_lab.showcommand.debug.window.screenshotnow acceptswindow_identifierso auxiliary AppKit windows can be captured directly.Verification:
swift test --package-path Packages/macOS/CmuxAppKitSupportUI --filter SignalGraphTestsswift test --package-path Packages/macOS/CmuxControlSocketsiglabNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a DEBUG-only AppKit Signals Lab as a native “Signal Todo List” driven by a main-actor reactive graph (Solid-style signals) for fine-grained UI updates. Adds a debug menu item,
debug.appkit_signal_lab.show, and targeted window screenshots viadebug.window.screenshot.DebugWindowsCoordinator.showAppKitSignalLabWindow()and localized “Signal Todo List…” menu entry; control commanddebug.appkit_signal_lab.show;debug.window.screenshotacceptswindow_identifierto capture auxiliary windows.SignalGraphTests,AppKitSignalLabModelTests, and coordinator tests for command routing and screenshot targeting.Written for commit cd43859. Summary will update on new commits.
Summary by CodeRabbit