Repository navigation
Use signals for AppKit sidebar interactions - #8383
Closed
lawrencecchen wants to merge 10 commits into
Closed
lawrencecchen wants to merge 10 commits into
lawrencecchen wants to merge 10 commits into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a fine-grained reactive signal framework for AppKit, builds a debug UI (Signal Lab) on it, refactors inline rename coordination to use session-based state instead of once-only resolution, and integrates Signal Lab display and enhanced screenshot capture into debug commands. ChangesSignal/Reactive Framework
AppKit Signal Lab Debug UI
Inline Rename Session Coordination
Debug Command Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Signal as Signal
participant Memo as SignalMemo
participant Effect as SignalEffect
participant Graph as SignalGraph
User->>Graph: createSignal(initialValue)
Graph->>Signal: create Signal
Signal-->>User: Signal
User->>Graph: createMemo(compute)
Graph->>Memo: create & prime
Memo->>Graph: track dependencies
Graph->>Signal: addObserver(Memo)
Memo-->>User: SignalMemo
User->>Graph: createEffect(body)
Graph->>Effect: create
Effect->>Graph: withObserver(effect)
Graph->>Signal: addObserver(Effect)
Effect-->>User: SignalEffect (running)
User->>Signal: set(newValue)
Signal->>Graph: schedule observers
Graph->>Memo: run()
Memo->>Graph: recompute
Memo->>Graph: schedule downstream
Graph->>Effect: run()
Effect->>Graph: track dependencies
Effect-->>User: effect executed
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 2 warnings)
✅ Passed checks (19 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b1bfe9f2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
975
to
+976
| func control(_ control: NSControl, textView: NSTextView, doCommandBy commandSelector: Selector) -> Bool { | ||
| if commandSelector == #selector(NSResponder.cancelOperation(_:)) { | ||
| onCancel?() | ||
| return true | ||
| } | ||
| if commandSelector == #selector(NSResponder.insertNewline(_:)) { | ||
| onCommit?(stringValue) | ||
| return true | ||
| } | ||
| return false | ||
| renameCoordinator.control(control, textView: textView, doCommandBy: commandSelector) |
There was a problem hiding this comment.
SidebarRowInlineRenameField is also the persistent addField in SidebarRowChecklistSection, but unlike the workspace title path it never calls resetForNewSession(). After the first checklist item is submitted or cancelled, the coordinator remains resolved; opening “Add item” again makes Enter and focus loss no-ops (and Escape cannot invoke its cancel closure), so users cannot add a second item from that reused row. Reset the coordinator when a new checklist-add activation starts.
Useful? React with 👍 / 👎.
Contributor
Greptile SummaryThis PR promotes the reusable signal core from the debug lab and wires it into three production paths: workspace custom-title state, sidebar click-vs-drag gesture lifecycle, and inline-rename phase management. The result replaces ad-hoc boolean flags and split
Confidence Score: 5/5Safe to merge. All new state machines are @MainActor-isolated, the signal graph is well-tested including diamond propagation and throwing batches, and the behavioral change (optimistic blue on mouse-up instead of mouse-down) is intentional and preflight-tested. The architectural changes are thorough: the hasResolved bool and split @published fields that caused the original bug are fully removed, the new signal paths are covered by 43 passing tests including regression cases, and the click-drag state machine prevents the prior paint-on-press ambiguity. The one outstanding item is that sidebarCustomTitleSignal and its graph remain accessible at internal scope with a public set method, but no current callsite exploits this and it is purely a hardening concern. Sources/Workspace.swift and Sources/Workspace+TitleOwnership.swift — the sidebarCustomTitleSignal lazy var is internal and its Signal.set is public, leaving a path for callers outside the extension to bypass publishSidebarCustomTitle's write ordering. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant TableViewImpl as SidebarWorkspaceTableViewImpl
participant Controller as SidebarWorkspaceTableController
participant Interaction as SignalClickDragInteraction
participant Effect as selectionInteractionEffect
User->>TableViewImpl: mouseDown
TableViewImpl->>Controller: pointerMouseDown(row, modifiers)
Controller->>Interaction: mouseDown(workspaceId, modifiers)
Note over Interaction: idle → pressed
alt Drag threshold crossed
TableViewImpl->>Controller: draggingSession(willBeginAt:)
Controller->>Interaction: dragDidBegin(workspaceId)
Note over Interaction: pressed → dragging
Note over Effect: guard .activating fails — no selection
TableViewImpl->>Controller: draggingSession(ended:)
Controller->>Interaction: dragDidEnd()
Note over Interaction: dragging → idle
else Clean click (mouse-up)
TableViewImpl->>Controller: tableView(_:shouldSelectRow:) [AppKit action]
Controller->>Interaction: mouseUpWithoutDrag(workspaceId)
Note over Interaction: pressed → activating
Interaction->>Effect: phase changed to .activating
Effect->>Controller: previewSelection(workspaceId) — paint blue
Effect->>Controller: commitSelection(workspaceId, modifiers)
Controller->>Controller: selectionCoalescer.request(updateSelection)
Controller->>Controller: configure(rows:) [authoritative update arrives]
Controller->>Interaction: activationDidReconcile(workspaceId)
Note over Interaction: activating → idle
Effect->>Controller: cleanup — restoreAuthoritativeSelectionAppearance
end
TableViewImpl->>Controller: pointerTrackingDidEnd()
Controller->>Interaction: trackingDidEnd() [no-op if not .pressed]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant TableViewImpl as SidebarWorkspaceTableViewImpl
participant Controller as SidebarWorkspaceTableController
participant Interaction as SignalClickDragInteraction
participant Effect as selectionInteractionEffect
User->>TableViewImpl: mouseDown
TableViewImpl->>Controller: pointerMouseDown(row, modifiers)
Controller->>Interaction: mouseDown(workspaceId, modifiers)
Note over Interaction: idle → pressed
alt Drag threshold crossed
TableViewImpl->>Controller: draggingSession(willBeginAt:)
Controller->>Interaction: dragDidBegin(workspaceId)
Note over Interaction: pressed → dragging
Note over Effect: guard .activating fails — no selection
TableViewImpl->>Controller: draggingSession(ended:)
Controller->>Interaction: dragDidEnd()
Note over Interaction: dragging → idle
else Clean click (mouse-up)
TableViewImpl->>Controller: tableView(_:shouldSelectRow:) [AppKit action]
Controller->>Interaction: mouseUpWithoutDrag(workspaceId)
Note over Interaction: pressed → activating
Interaction->>Effect: phase changed to .activating
Effect->>Controller: previewSelection(workspaceId) — paint blue
Effect->>Controller: commitSelection(workspaceId, modifiers)
Controller->>Controller: selectionCoalescer.request(updateSelection)
Controller->>Controller: configure(rows:) [authoritative update arrives]
Controller->>Interaction: activationDidReconcile(workspaceId)
Note over Interaction: activating → idle
Effect->>Controller: cleanup — restoreAuthoritativeSelectionAppearance
end
TableViewImpl->>Controller: pointerTrackingDidEnd()
Controller->>Interaction: trackingDidEnd() [no-op if not .pressed]
Reviews (5): Last reviewed commit: "fix: make signal propagation transaction..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+13
to
+31
| @Test | ||
| @MainActor | ||
| func appKitRowRenameEnterCommitsLiveFieldEditorText() { | ||
| let field = SidebarRowInlineRenameField() | ||
| field.stringValue = "Original" | ||
| var committedTitle: String? | ||
| field.onCommit = { committedTitle = $0 } | ||
| let editor = NSTextView() | ||
| editor.string = "Renamed" | ||
|
|
||
| let handled = field.control( | ||
| field, | ||
| textView: editor, | ||
| doCommandBy: #selector(NSResponder.insertNewline(_:)) | ||
| ) | ||
|
|
||
| #expect(handled) | ||
| #expect(committedTitle == "Renamed") | ||
| } |
Contributor
There was a problem hiding this comment.
resetForNewSession() invariant
The new test covers Enter committing live field-editor text, but there is no test for the other half of this fix: a reused AppKit table cell whose coordinator still has hasResolved = true from a previous session will silently swallow the commit on the second rename. If resetForNewSession() were accidentally dropped from beginInlineRename(), no commit would fire on subsequent renames and the regression would go undetected. A second test that calls resetForNewSession(), sets up a new commit capture, and presses Enter again would lock in that invariant.
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!
lawrencecchen
force-pushed
the
task-sidebar-appkit-workspace-rename
branch
from
July 18, 2026 01:29
2b1bfe9 to
9a067c3
Compare
lawrencecchen
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 advancement logic around Self.nextStatus so
any task whose resulting status is .complete has progress set to 100% rather
than the incremental value. Apply the same correction to both advancement paths,
including the occurrence around lines 153–160, while preserving normal progress
updates for non-completed statuses.
In
`@Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Views/AppKitSignalLabViewController.swift`:
- Around line 488-491: Update the visibleCountLabel formatting and each
filter-count label to use complete localized plural phrases rather than
concatenating localized titles with counts. Add/use catalog entries with
explicit .one and .other variants, selecting the appropriate key based on the
count while preserving the displayed count value.
In
`@Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Windows/AppKitSignalLabWindowController.swift`:
- Line 18: Add the AppKit Signal Lab identifier “cmux.appKitSignalsLab” to the
cmuxAuxiliaryWindowIdentifiers collection in cmuxApp.swift, preserving the
existing window-close ownership behavior for auxiliary windows.
In
`@Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalGraph.swift`:
- Around line 95-101: Update the pending observer processing loop in SignalGraph
to avoid calling min(by:) and removing only one observer per iteration. In each
pass, determine the highest-priority scheduling group in a single scan, remove
and run all observers sharing that priority, then continue with the remaining
groups so processing scales with the number of distinct priorities rather than
observer count.
In `@Sources/cmuxApp.swift`:
- Around line 614-621: Update openAllDebugWindows() to call
DebugWindowsCoordinator.showAppKitSignalLabWindow(), alongside the existing
debug-window show calls, so the AppKit Signals Lab window is included in the
“Open All Debug Windows” action.
🪄 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: 6e12be01-78f8-482f-90e7-b1185155c5fa
📒 Files selected for processing (34)
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/Views/AppKitSignalLabViewController.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Views/SignalLabPulseView.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Windows/AppKitSignalLabWindowController.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/Signal.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalDependency.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalEffect.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalEffectContext.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalGraph.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalMemo.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalObserver.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/WeakSignalObserver.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/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftSources/Sidebar/SidebarInlineRenameCoordinator.swiftSources/Sidebar/SidebarInlineRenameField.swiftSources/TerminalController+ControlDebugContext.swiftSources/TerminalController+DebugMethodNames.swiftSources/TerminalController.swiftSources/cmuxApp.swiftcmuxTests/SidebarInlineRenameKeyResolverTests.swiftcmuxTests/SidebarWorkspaceTableTests.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🤖 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 advancement logic around Self.nextStatus so
any task whose resulting status is .complete has progress set to 100% rather
than the incremental value. Apply the same correction to both advancement paths,
including the occurrence around lines 153–160, while preserving normal progress
updates for non-completed statuses.
In
`@Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Views/AppKitSignalLabViewController.swift`:
- Around line 488-491: Update the visibleCountLabel formatting and each
filter-count label to use complete localized plural phrases rather than
concatenating localized titles with counts. Add/use catalog entries with
explicit .one and .other variants, selecting the appropriate key based on the
count while preserving the displayed count value.
In
`@Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Windows/AppKitSignalLabWindowController.swift`:
- Line 18: Add the AppKit Signal Lab identifier “cmux.appKitSignalsLab” to the
cmuxAuxiliaryWindowIdentifiers collection in cmuxApp.swift, preserving the
existing window-close ownership behavior for auxiliary windows.
In
`@Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalGraph.swift`:
- Around line 95-101: Update the pending observer processing loop in SignalGraph
to avoid calling min(by:) and removing only one observer per iteration. In each
pass, determine the highest-priority scheduling group in a single scan, remove
and run all observers sharing that priority, then continue with the remaining
groups so processing scales with the number of distinct priorities rather than
observer count.
In `@Sources/cmuxApp.swift`:
- Around line 614-621: Update openAllDebugWindows() to call
DebugWindowsCoordinator.showAppKitSignalLabWindow(), alongside the existing
debug-window show calls, so the AppKit Signals Lab window is included in the
“Open All Debug Windows” action.
🪄 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: 6e12be01-78f8-482f-90e7-b1185155c5fa
📒 Files selected for processing (34)
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/Views/AppKitSignalLabViewController.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Views/SignalLabPulseView.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Windows/AppKitSignalLabWindowController.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/Signal.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalDependency.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalEffect.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalEffectContext.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalGraph.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalMemo.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalObserver.swiftPackages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/WeakSignalObserver.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/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftSources/Sidebar/SidebarInlineRenameCoordinator.swiftSources/Sidebar/SidebarInlineRenameField.swiftSources/TerminalController+ControlDebugContext.swiftSources/TerminalController+DebugMethodNames.swiftSources/TerminalController.swiftSources/cmuxApp.swiftcmuxTests/SidebarInlineRenameKeyResolverTests.swiftcmuxTests/SidebarWorkspaceTableTests.swift
🛑 Comments failed to post (5)
Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Model/AppKitSignalLabModel.swift (1)
93-95: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep completed tasks at 100% progress.
.reviewtransitions to.completeregardless of progress. For example, the blocked 41% fixture can become complete at 77%, after which the UI disables further advancement.Proposed fix
- updatedTask.status = Self.nextStatus(after: task.status, progress: updatedTask.progress) + updatedTask.status = Self.nextStatus(after: task.status, progress: updatedTask.progress) + if updatedTask.status == .complete { + updatedTask.progress = 1 + }Also applies to: 153-160
🤖 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/Model/AppKitSignalLabModel.swift` around lines 93 - 95, Update the task advancement logic around Self.nextStatus so any task whose resulting status is .complete has progress set to 100% rather than the incremental value. Apply the same correction to both advancement paths, including the occurrence around lines 153–160, while preserving normal progress updates for non-completed statuses.Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Views/AppKitSignalLabViewController.swift (1)
488-491: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the complete count phrases with plural variants.
"%lld visible"and"\(localizedTitle) \(count)"bypass locale-specific pluralization and word ordering. Use catalog entries with.one/.othervariants for the visible count and each filter-count label.As per coding guidelines, “User-facing text must use localized APIs and matching catalogs.” Based on learnings, pluralized Swift strings should use explicit
.oneand.otherlocalization 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 the visibleCountLabel formatting and each filter-count label to use complete localized plural phrases rather than concatenating localized titles with counts. Add/use catalog entries with explicit .one and .other variants, selecting the appropriate key based on the count while preserving the displayed count value.Sources: Coding guidelines, Path instructions, Learnings
Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AppKitSignalLab/Windows/AppKitSignalLabWindowController.swift (1)
18-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -eu rg -n -C3 'cmuxAuxiliaryWindowIdentifiers|cmux\.appKitSignalsLab' \ "$(fd -a '^cmuxApp\.swift$' . | head -n 1)"Repository: manaflow-ai/cmux
Length of output: 636
Register
cmux.appKitSignalsLabfor Cmd+W ownership
Add this identifier tocmuxAuxiliaryWindowIdentifiersinSources/cmuxApp.swift; otherwise the new lab window falls through to workspace panel close handling.🤖 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/Windows/AppKitSignalLabWindowController.swift` at line 18, Add the AppKit Signal Lab identifier “cmux.appKitSignalsLab” to the cmuxAuxiliaryWindowIdentifiers collection in cmuxApp.swift, preserving the existing window-close ownership behavior for auxiliary windows.Source: Coding guidelines
Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Signals/SignalGraph.swift (1)
95-101: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Optimize batched observer processing to avoid O(N²) repeated scans.
The
min(by:)search inside awhileloop overpendingObserversperforms a repeated full-collection scan since it processes only one observer per pass. If a batch contains many observers (e.g., during a broad UI state transition), this degrades to O(N²) complexity and can cause severe frame drops. As per path instructions, avoid repeated hot-path sorting/filtering for scalable collections in production code.Extract all observers for the highest priority in a single pass to reduce the outer loop to O(P), where P is the number of distinct priorities, making the overall flush practically O(N).
⚡ Proposed optimization
while !pendingObservers.isEmpty { - guard let observer = pendingObservers.values.min(by: { + guard let minPriority = pendingObservers.values.min(by: { $0.schedulingPriority < $1.schedulingPriority - }) else { return } - pendingObservers.removeValue(forKey: ObjectIdentifier(observer)) - observer.run() + })?.schedulingPriority else { return } + + let batch = pendingObservers.values.filter { $0.schedulingPriority == minPriority } + for observer in batch { + pendingObservers.removeValue(forKey: ObjectIdentifier(observer)) + } + + for observer in batch { + observer.run() + } }📝 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.while !pendingObservers.isEmpty { guard let minPriority = pendingObservers.values.min(by: { $0.schedulingPriority < $1.schedulingPriority })?.schedulingPriority else { return } let batch = pendingObservers.values.filter { $0.schedulingPriority == minPriority } for observer in batch { pendingObservers.removeValue(forKey: ObjectIdentifier(observer)) } for observer in batch { observer.run() } }🤖 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/Signals/SignalGraph.swift` around lines 95 - 101, Update the pending observer processing loop in SignalGraph to avoid calling min(by:) and removing only one observer per iteration. In each pass, determine the highest-priority scheduling group in a single scan, remove and run all observers sharing that priority, then continue with the remaining groups so processing scales with the number of distinct priorities rather than observer count.Source: Path instructions
Sources/cmuxApp.swift (1)
614-621: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Include the new window in
openAllDebugWindows.The "AppKit Signals Lab…" window was added to the "Debug Windows" menu, but
AppDelegate.shared?.debugWindowsCoordinator.showAppKitSignalLabWindow()was not added to theopenAllDebugWindows()helper function below. Please consider adding it so that the "Open All Debug Windows" action correctly includes this new tool.🛠️ Suggested update for
openAllDebugWindows()private func openAllDebugWindows() { // ... existing windows ... AppDelegate.shared?.debugWindowsCoordinator.showAboutTitlebarDebugWindow() AppDelegate.shared?.debugWindowsCoordinator.showAppKitSignalLabWindow() // ... }🤖 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/cmuxApp.swift` around lines 614 - 621, Update openAllDebugWindows() to call DebugWindowsCoordinator.showAppKitSignalLabWindow(), alongside the existing debug-window show calls, so the AppKit Signals Lab window is included in the “Open All Debug Windows” action.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
lawrencecchen
changed the title
lawrencecchen
added
the
stale-revisit
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Signal design
Solid-style synchronous propagation fits AppKit selection state. Concurrent transitions do not: selection identity and the blue row must commit together on MainActor. Keyed subscriptions and ownership scopes remain useful follow-ups if the sidebar adopts per-row signal effects or nested effects.
The release experiment improved a 30-write, 800-observer fan-out from about 683 ms to 8-26 ms. A six-layer diamond now settles 12 branch memos and 6 join memos exactly once, with one downstream effect. A 50-workspace, seven-field, 100-burst update fell from 35,000 to 5,000 row reruns and about 41-46 ms to 11-13 ms with batching.
Tests
swift test --package-path Packages/macOS/CmuxAppKitSupportUI(48 passed)PortScannerTests.swiftAlternative
Fixes #8224