Consolidate debug extractions into CmuxFeedback (no new packages) - #6224
Merged
Merged
Conversation
… (no new packages) Folds two per-sliver refactor branches into existing domain packages so the debug-group extractions land without creating any new top-level package. 1. feat-mobile-host-rpc-router extracted the privileged Mac<->phone dogfood feedback sink into a new CmuxDogfoodFeedbackSink package. That domain folds into the existing CmuxFeedback package under DogfoodSink/: DogfoodFeedbackLimits, DogfoodFeedbackOutcome, DogfoodFeedbackSubmission (Sendable value types) and the nonisolated Sendable DogfoodFeedbackService. Byte-identical logic (same caps, base64-char-cap-before-decode then byte-cap ordering, Task.detached(.utility) off-main write, ISO8601-colons-to-dash bundle naming, 0700/0600 perms, bundle.json schema/sorted-keys/pretty, lexicographic prune keeping newest 50, and the same RPC error codes/messages). TerminalController.v2MobileDogfoodFeedbackSubmit is now a thin forward that resolves the authenticated email via the main-actor MobileHostService and calls service.submit(...); the service re-enforces the @manaflow.ai gate at the trust boundary. CmuxFeedback is already imported by TerminalController and already linked to the app target, so no import or pbxproj change was needed. 2. feat-debug-windows-extraction extracted the self-contained About-titlebar debug cluster into a new CmuxDebugWindowsUI package. That UI folds into the existing CmuxAppKitSupportUI package under AboutTitlebarDebug/: the AboutWindowKind / TitlebarVisibilityOption / TitlebarToolbarStyleOption value enums, AboutTitlebarDebugOptions value type, AboutTitlebarDebugStore (@mainactor @observable, single writer), AboutTitlebarDebugWindowController, AboutTitlebarDebugView, plus the DebugWindowsCoordinator and the WindowDecorating protocol seam. AppDelegate conforms to WindowDecorating and owns the coordinator (held weakly by the coordinator/store to avoid a retain cycle); cmuxApp.swift and the About/Acknowledgments controllers forward into the app-owned coordinator/store. Byte-identical window identifiers, titles, style-mask bits, toolbar identifiers, sizes, and copy-config payload. CmuxAppKitSupportUI already exists and is already linked to the app target. Both member branches branched off an older main (5321bec), before CmuxFeedback and CmuxAppKitSupportUI existed, which is why they created standalone packages. Neither new package is created here; zero new top-level packages and zero pbxproj entries were added. Verification: scripts/lint-ios-package-conventions.sh introduces no new violation and zero lint:allow (the single pre-existing ERROR is ComposerDictationTextMerge in CmuxMobileShellUI, unrelated to this change). swift build + swift test green in both packages (CmuxFeedback 12 tests, CmuxAppKitSupportUI 8 tests). File-length budget reconciled: TerminalController 14829->14681, cmuxApp 4921->4516, AppDelegate 17894->17905 (+11 for composition-root wiring). Supersedes the per-sliver package PRs from feat-mobile-host-rpc-router and feat-debug-windows-extraction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTwo features are extracted into dedicated packages. The About Titlebar Debug subsystem ( ChangesAbout Titlebar Debug Extraction into CmuxAppKitSupportUI
DogfoodFeedbackService Extraction into CmuxFeedback
Sequence Diagram(s)sequenceDiagram
participant DebugMenu as Debug Menu / Controls
participant AppDelegate
participant DebugWindowsCoordinator
participant AboutTitlebarDebugStore
participant AboutTitlebarDebugWindowController
participant AboutWindowController
AppDelegate->>DebugWindowsCoordinator: lazy init(decorator: self)
DebugMenu->>AppDelegate: debugWindowsCoordinator.showAboutTitlebarDebugWindow()
AppDelegate->>DebugWindowsCoordinator: showAboutTitlebarDebugWindow()
DebugWindowsCoordinator->>AboutTitlebarDebugWindowController: lazy init(store:decorator:) + show()
AboutWindowController->>AppDelegate: aboutTitlebarDebugStore.applyCurrentOptions(to:for:)
AppDelegate->>AboutTitlebarDebugStore: applyCurrentOptions(to:for:)
sequenceDiagram
participant TerminalController
participant DogfoodFeedbackService
participant DetachedTask as Task.detached(priority: .utility)
participant FileSystem
TerminalController->>DogfoodFeedbackService: submit(submission, authenticatedEmail)
DogfoodFeedbackService->>DogfoodFeedbackService: isPrivilegedFeedbackEmail → .unauthorized
DogfoodFeedbackService->>DogfoodFeedbackService: cap text fields, reject oversized base64
DogfoodFeedbackService->>DetachedTask: decode base64 + writeBundle
DetachedTask->>FileSystem: mkdir 0700, write diagnostic.log + bundle.json 0600
DetachedTask->>FileSystem: pruneBundles (keep newest N)
DetachedTask-->>DogfoodFeedbackService: .written(bundlePath, diagnosticLogBytes)
DogfoodFeedbackService-->>TerminalController: DogfoodFeedbackOutcome → V2CallResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
This was referenced Jun 16, 2026
Contributor
Greptile SummaryThis PR consolidates two previously rejected per-sliver packages into the existing
Confidence Score: 5/5Safe to merge; all changed paths are straightforward extractions with byte-identical behavior and full test coverage in both packages. The dogfood feedback path is a clean mechanical lift: the privilege check, field-cap ordering, and detached task structure are preserved exactly. The debug UI path correctly migrates from ObservableObject to @observable, replaces the AppDelegate.shared singleton reach with a proper seam, and avoids retain cycles through the documented weak references. The only new finding is a trivial No files require special attention. The AboutTitlebarDebugWindowController and DogfoodFeedbackService issues already flagged in previous review threads are the main open items. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Phone as Paired Phone
participant TC as TerminalController
participant DFS as DogfoodFeedbackService
participant Disk as ~/.cache/cmux-dogfood-feedback
Phone->>TC: v2MobileDogfoodFeedbackSubmit(params)
TC->>TC: "resolve authenticatedEmail (MobileHostService, @MainActor)"
TC->>TC: build DogfoodFeedbackSubmission (wire fields)
TC->>DFS: submit(submission, authenticatedEmail)
DFS->>DFS: isPrivilegedFeedbackEmail? (caller actor)
DFS->>DFS: cap text / terminalText / buildStamp (caller actor)
DFS->>DFS: guard base64 char count (caller actor)
DFS-->>DFS: Task.detached(.utility)
DFS->>DFS: decode base64 blob
DFS->>DFS: guard decoded byte count
DFS->>Disk: createDirectory (0700)
DFS->>Disk: write diagnostic.log (chmod 0600)
DFS->>Disk: write bundle.json (chmod 0600)
DFS->>Disk: pruneBundles (keep newest 50)
DFS-->>TC: DogfoodFeedbackOutcome
TC-->>Phone: V2CallResult (.ok / .err)
%%{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 Phone as Paired Phone
participant TC as TerminalController
participant DFS as DogfoodFeedbackService
participant Disk as ~/.cache/cmux-dogfood-feedback
Phone->>TC: v2MobileDogfoodFeedbackSubmit(params)
TC->>TC: "resolve authenticatedEmail (MobileHostService, @MainActor)"
TC->>TC: build DogfoodFeedbackSubmission (wire fields)
TC->>DFS: submit(submission, authenticatedEmail)
DFS->>DFS: isPrivilegedFeedbackEmail? (caller actor)
DFS->>DFS: cap text / terminalText / buildStamp (caller actor)
DFS->>DFS: guard base64 char count (caller actor)
DFS-->>DFS: Task.detached(.utility)
DFS->>DFS: decode base64 blob
DFS->>DFS: guard decoded byte count
DFS->>Disk: createDirectory (0700)
DFS->>Disk: write diagnostic.log (chmod 0600)
DFS->>Disk: write bundle.json (chmod 0600)
DFS->>Disk: pruneBundles (keep newest 50)
DFS-->>TC: DogfoodFeedbackOutcome
TC-->>Phone: V2CallResult (.ok / .err)
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// The controller is built around an injected ``AboutTitlebarDebugStore`` (the | ||
| /// single source of truth for the options) and a ``WindowDecorating`` seam used | ||
| /// to normalize the panel's own chrome. | ||
| public final class AboutTitlebarDebugWindowController: NSWindowController, NSWindowDelegate { |
Contributor
There was a problem hiding this comment.
@MainActor on UI-bound window controller
AboutTitlebarDebugWindowController wraps AppKit, holds an @MainActor @Observable store, and calls store.applyToOpenWindows() (a @MainActor method) in show(), yet the class itself is not annotated @MainActor. All current callers are @MainActor (DebugWindowsCoordinator.showAboutTitlebarDebugWindow() is @MainActor), so runtime behavior is correct, but the missing annotation means the Swift 6 type system won't catch a future call site that constructs or invokes this controller from a background context. Adding @MainActor to the class would make the isolation contract explicit and have the compiler enforce it at every call site.
Rule Used: Flag new or materially worsened Swift 6 actor isol... (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!
Comment on lines
+147
to
+174
| let timestamp = formatter.string(from: now()).replacingOccurrences(of: ":", with: "-") | ||
| let shortID = String(UUID().uuidString.prefix(8)).lowercased() | ||
| let bundleDir = root.appendingPathComponent("\(timestamp)_\(shortID)", isDirectory: true) | ||
|
|
||
| do { | ||
| // The bundle holds visible terminal text and debug logs, which can | ||
| // contain credentials or other private data. Create the root and | ||
| // bundle dirs owner-only (0700) so no other local user can traverse | ||
| // into them, and chmod the written files to 0600. The dir is created | ||
| // 0700 first, so even the brief window before the file chmod is not | ||
| // world-readable through a traversable parent. | ||
| let dirAttributes: [FileAttributeKey: Any] = [.posixPermissions: 0o700] | ||
| try fileManager.createDirectory( | ||
| at: root, | ||
| withIntermediateDirectories: true, | ||
| attributes: dirAttributes | ||
| ) | ||
| try fileManager.createDirectory( | ||
| at: bundleDir, | ||
| withIntermediateDirectories: true, | ||
| attributes: dirAttributes | ||
| ) | ||
| let diagnosticURL = bundleDir.appendingPathComponent("diagnostic.log") | ||
| try diagnosticData.write(to: diagnosticURL) | ||
| try fileManager.setAttributes([.posixPermissions: 0o600], ofItemAtPath: diagnosticURL.path) | ||
| let manifest: [String: Any] = [ | ||
| "schema": "cmux.dogfood.feedback.v1", | ||
| "received_at": formatter.string(from: now()), |
Contributor
There was a problem hiding this comment.
received_at timestamp can differ from bundle directory name
now() is called once to build the directory name (timestamp, line 147) and then called again for the received_at field in the manifest (line 174). Between the two calls, real wall-clock time advances, so the timestamp embedded in the directory name and the received_at field in bundle.json may not match. This was the same pattern in the original TerminalController code (two bare Date() calls), so it is pre-existing behavior, but the injectable now closure makes a clean fix straightforward: capture the result of a single now() call at the top of writeBundle and reuse that value for both the directory name and received_at.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/AboutTitlebarDebugOptions.swift`:
- Around line 69-87: The windowTitle assignment in the defaults(for:) method of
AboutTitlebarDebugOptions uses a hardcoded English string "About cmux" instead
of a localized value. Replace this hardcoded string with
String(localized:defaultValue:) to enable proper localization, reusing the same
localization key as AboutWindowKind.fallbackTitle for consistency across the
codebase.
In
`@Packages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/AboutWindowKind.swift`:
- Around line 19-24: The `displayTitle` property in the switch statement returns
bare English strings instead of using localized strings. Replace the returned
string "About Window" with `String(localized:"About Window", defaultValue:"About
Window")` to enable proper localization. Apply the same fix to the
`fallbackTitle` property (also applies to lines 36-41) by wrapping its returned
string with `String(localized:defaultValue:)`. Additionally, add corresponding
entries for both localized strings to `Resources/Localizable.xcstrings` to
provide translations for all supported locales.
In
`@Packages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/TitlebarToolbarStyleOption.swift`:
- Around line 23-36: The `displayTitle` property in the
TitlebarToolbarStyleOption enum returns user-facing picker labels as bare
English strings that lack localization support. Modify each case in the switch
statement (automatic, expanded, preference, unified, unifiedCompact) to wrap the
returned string values with `String(localized:defaultValue:)` instead of
returning plain string literals, so the labels can be properly localized
according to the app's localization requirements.
In
`@Packages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/TitlebarVisibilityOption.swift`:
- Around line 17-24: The displayTitle property in the TitlebarVisibilityOption
enum contains hardcoded English string literals "Hidden" and "Visible" that are
shown in the UI picker but are not localized. Replace each of these return
statements to wrap the strings using String(localized:defaultValue:) syntax,
passing the same string as both the localized key and the defaultValue
parameter. This will enable proper localization support for the picker display
labels while maintaining the current English defaults.
In
`@Packages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Views/AboutTitlebarDebugView.swift`:
- Around line 19-128: All user-facing text strings in the AboutTitlebarDebugView
must be localized using the String(localized:defaultValue:) pattern. Replace all
bare English strings with this localization pattern, following the example
already shown in the code. Specifically, wrap the following with
String(localized:defaultValue:): the GroupBox title "Actions", all button labels
("Reset All", "Reapply to Open Windows", "Copy Config", "Apply Now"), the Toggle
label "Enable Debug Overrides", the help text explaining the disabled state, all
form labels ("Window Title", "Title Visibility", "Toolbar Style"), the section
label "Style Mask", and all style mask toggle labels ("Titled", "Closable",
"Miniaturizable", "Resizable", "Full Size Content View"). Use descriptive
localization keys (e.g., "debug.aboutTitlebarDebug.actions") and the bare
English text as the defaultValue parameter for each string.
In
`@Packages/CmuxFeedback/Sources/CmuxFeedback/DogfoodSink/DogfoodFeedbackLimits.swift`:
- Around line 34-48: The init method of DogfoodFeedbackLimits accepts limit
parameters without validating that they are valid (positive or non-zero), which
can cause traps in downstream code when methods like prefix(...) and
dropLast(keep) are called with negative values, and allows maxRetainedBundles to
be zero which incorrectly prunes bundles. Add validation checks in the
initializer to ensure all limit parameters meet their required invariants (e.g.,
greater than zero or positive as appropriate), and raise an error or
precondition failure if invalid values are provided.
In
`@Packages/CmuxFeedback/Sources/CmuxFeedback/DogfoodSink/DogfoodFeedbackService.swift`:
- Around line 143-149: The timestamp generation in DogfoodFeedbackService.swift
creates directory names using only second-level precision, which causes
non-deterministic pruning when multiple bundles are created within the same
second because the random ID becomes the tie-breaker for lexicographic ordering.
To fix this, increase the timestamp precision from second-level to include
milliseconds or microseconds in the ISO8601DateFormatter options so that burst
writes generate chronologically ordered directory names that are also
lexicographically sorted. This ensures the pruning logic in the directory name
ordering at lines 201-213 reliably deletes older bundles first, preventing
accidental deletion of newer bundles during retention cleanup.
In
`@Packages/CmuxFeedback/Tests/CmuxFeedbackTests/DogfoodFeedbackServiceTests.swift`:
- Around line 52-65: The blobByteCapRejected test verifies the rejection outcome
but does not confirm that no filesystem operations occurred. Add a fileExists
check after the outcome assertion (after the expect statement at line 64) to
verify that the expected file does not exist, confirming that rejection happens
before any I/O operations are performed. This makes the test consistent with the
pattern used in the unauthorized test and the base64CharCapRejected test.
- Around line 132-164: The test prunesOldBundles verifies that exactly 2 bundles
remain after pruning, but does not validate that they are the newest 2 as the
test name suggests. After obtaining the remaining bundle URLs from
contentsOfDirectory and confirming the count is 2, inspect those URLs to verify
they correspond to the last 2 timestamps in the sequence (the final 2 iterations
when tick had the highest values). Extract the timestamps from the remaining
directory names and assert they match the expected newest timestamps to ensure
the oldest bundles were actually pruned rather than random ones.
- Around line 38-50: Add a file existence check after the outcome assertion in
the base64CharCapRejected() test function to verify that no directory was
created when the submission was rejected. This check should follow the same
pattern used in the unauthorized test to confirm that parameter validation and
rejection occurs before any filesystem operations are attempted.
🪄 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: 46547138-9404-437c-83b6-d3722670b772
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (18)
Packages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Coordinator/DebugWindowsCoordinator.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Seams/WindowDecorating.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Stores/AboutTitlebarDebugStore.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/AboutTitlebarDebugOptions.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/AboutWindowKind.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/TitlebarToolbarStyleOption.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/TitlebarVisibilityOption.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Views/AboutTitlebarDebugView.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Windows/AboutTitlebarDebugWindowController.swiftPackages/CmuxAppKitSupportUI/Tests/CmuxAppKitSupportUITests/AboutTitlebarDebugStoreTests.swiftPackages/CmuxFeedback/Sources/CmuxFeedback/DogfoodSink/DogfoodFeedbackLimits.swiftPackages/CmuxFeedback/Sources/CmuxFeedback/DogfoodSink/DogfoodFeedbackOutcome.swiftPackages/CmuxFeedback/Sources/CmuxFeedback/DogfoodSink/DogfoodFeedbackService.swiftPackages/CmuxFeedback/Sources/CmuxFeedback/DogfoodSink/DogfoodFeedbackSubmission.swiftPackages/CmuxFeedback/Tests/CmuxFeedbackTests/DogfoodFeedbackServiceTests.swiftSources/AppDelegate.swiftSources/TerminalController.swiftSources/cmuxApp.swift
Comment on lines
+69
to
+87
| public static func defaults(for kind: AboutWindowKind) -> AboutTitlebarDebugOptions { | ||
| switch kind { | ||
| case .about: | ||
| return AboutTitlebarDebugOptions( | ||
| overridesEnabled: false, | ||
| windowTitle: "About cmux", | ||
| titleVisibility: .hidden, | ||
| titlebarAppearsTransparent: true, | ||
| movableByWindowBackground: false, | ||
| titled: true, | ||
| closable: true, | ||
| miniaturizable: true, | ||
| resizable: false, | ||
| fullSizeContentView: false, | ||
| showToolbar: false, | ||
| toolbarStyle: .automatic | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Localize the default window title.
The windowTitle default value in defaults(for:) uses a bare English string "About cmux" that becomes the displayed window title. This should use String(localized:defaultValue:) to match the localization requirement. Consider reusing the same key as AboutWindowKind.fallbackTitle for consistency.
🌐 Proposed localization fix
case .about:
return AboutTitlebarDebugOptions(
overridesEnabled: false,
- windowTitle: "About cmux",
+ windowTitle: String(localized: "window.about.title", defaultValue: "About cmux"),
titleVisibility: .hidden,🤖 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/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/AboutTitlebarDebugOptions.swift`
around lines 69 - 87, The windowTitle assignment in the defaults(for:) method of
AboutTitlebarDebugOptions uses a hardcoded English string "About cmux" instead
of a localized value. Replace this hardcoded string with
String(localized:defaultValue:) to enable proper localization, reusing the same
localization key as AboutWindowKind.fallbackTitle for consistency across the
codebase.
Source: Coding guidelines
Comment on lines
+19
to
+24
| public var displayTitle: String { | ||
| switch self { | ||
| case .about: | ||
| return "About Window" | ||
| } | ||
| } |
There was a problem hiding this comment.
Localize user-facing display strings.
The displayTitle and fallbackTitle properties return user-facing text that appears in the debug window UI and as the window title, but they use bare English strings. As per coding guidelines, all user-facing Swift text must use String(localized:defaultValue:) with matching entries in Resources/Localizable.xcstrings for all supported locales.
🌐 Proposed localization fix
public var displayTitle: String {
switch self {
case .about:
- return "About Window"
+ return String(localized: "debug.aboutTitlebarDebug.aboutWindow.title", defaultValue: "About Window")
}
}
/// Title used when the debug-overridden title resolves to empty.
public var fallbackTitle: String {
switch self {
case .about:
- return "About cmux"
+ return String(localized: "window.about.title", defaultValue: "About cmux")
}
}Also applies to: 36-41
🤖 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/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/AboutWindowKind.swift`
around lines 19 - 24, The `displayTitle` property in the switch statement
returns bare English strings instead of using localized strings. Replace the
returned string "About Window" with `String(localized:"About Window",
defaultValue:"About Window")` to enable proper localization. Apply the same fix
to the `fallbackTitle` property (also applies to lines 36-41) by wrapping its
returned string with `String(localized:defaultValue:)`. Additionally, add
corresponding entries for both localized strings to
`Resources/Localizable.xcstrings` to provide translations for all supported
locales.
Source: Coding guidelines
Comment on lines
+23
to
+36
| public var displayTitle: String { | ||
| switch self { | ||
| case .automatic: | ||
| return "Automatic" | ||
| case .expanded: | ||
| return "Expanded" | ||
| case .preference: | ||
| return "Preference" | ||
| case .unified: | ||
| return "Unified" | ||
| case .unifiedCompact: | ||
| return "Unified Compact" | ||
| } | ||
| } |
There was a problem hiding this comment.
Localize toolbar style picker labels.
The displayTitle property returns user-facing picker labels ("Automatic", "Expanded", "Preference", "Unified", "Unified Compact") as bare English strings. As per coding guidelines, all picker labels in the About Titlebar Debug UI must be localized using String(localized:defaultValue:).
🌐 Proposed localization fix
public var displayTitle: String {
switch self {
case .automatic:
- return "Automatic"
+ return String(localized: "debug.aboutTitlebarDebug.toolbarStyle.automatic", defaultValue: "Automatic")
case .expanded:
- return "Expanded"
+ return String(localized: "debug.aboutTitlebarDebug.toolbarStyle.expanded", defaultValue: "Expanded")
case .preference:
- return "Preference"
+ return String(localized: "debug.aboutTitlebarDebug.toolbarStyle.preference", defaultValue: "Preference")
case .unified:
- return "Unified"
+ return String(localized: "debug.aboutTitlebarDebug.toolbarStyle.unified", defaultValue: "Unified")
case .unifiedCompact:
- return "Unified Compact"
+ return String(localized: "debug.aboutTitlebarDebug.toolbarStyle.unifiedCompact", defaultValue: "Unified Compact")
}
}📝 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.
Suggested change
| public var displayTitle: String { | |
| switch self { | |
| case .automatic: | |
| return "Automatic" | |
| case .expanded: | |
| return "Expanded" | |
| case .preference: | |
| return "Preference" | |
| case .unified: | |
| return "Unified" | |
| case .unifiedCompact: | |
| return "Unified Compact" | |
| } | |
| } | |
| public var displayTitle: String { | |
| switch self { | |
| case .automatic: | |
| return String(localized: "debug.aboutTitlebarDebug.toolbarStyle.automatic", defaultValue: "Automatic") | |
| case .expanded: | |
| return String(localized: "debug.aboutTitlebarDebug.toolbarStyle.expanded", defaultValue: "Expanded") | |
| case .preference: | |
| return String(localized: "debug.aboutTitlebarDebug.toolbarStyle.preference", defaultValue: "Preference") | |
| case .unified: | |
| return String(localized: "debug.aboutTitlebarDebug.toolbarStyle.unified", defaultValue: "Unified") | |
| case .unifiedCompact: | |
| return String(localized: "debug.aboutTitlebarDebug.toolbarStyle.unifiedCompact", defaultValue: "Unified Compact") | |
| } | |
| } |
🤖 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/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/TitlebarToolbarStyleOption.swift`
around lines 23 - 36, The `displayTitle` property in the
TitlebarToolbarStyleOption enum returns user-facing picker labels as bare
English strings that lack localization support. Modify each case in the switch
statement (automatic, expanded, preference, unified, unifiedCompact) to wrap the
returned string values with `String(localized:defaultValue:)` instead of
returning plain string literals, so the labels can be properly localized
according to the app's localization requirements.
Source: Coding guidelines
Comment on lines
+17
to
+24
| public var displayTitle: String { | ||
| switch self { | ||
| case .hidden: | ||
| return "Hidden" | ||
| case .visible: | ||
| return "Visible" | ||
| } | ||
| } |
There was a problem hiding this comment.
Localize picker display labels.
The displayTitle values "Hidden" and "Visible" are shown in the "Title Visibility" picker UI but use bare English strings. As per coding guidelines, all picker labels must be localized using String(localized:defaultValue:).
🌐 Proposed localization fix
public var displayTitle: String {
switch self {
case .hidden:
- return "Hidden"
+ return String(localized: "debug.aboutTitlebarDebug.titleVisibility.hidden", defaultValue: "Hidden")
case .visible:
- return "Visible"
+ return String(localized: "debug.aboutTitlebarDebug.titleVisibility.visible", defaultValue: "Visible")
}
}🤖 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/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Values/TitlebarVisibilityOption.swift`
around lines 17 - 24, The displayTitle property in the TitlebarVisibilityOption
enum contains hardcoded English string literals "Hidden" and "Visible" that are
shown in the UI picker but are not localized. Replace each of these return
statements to wrap the strings using String(localized:defaultValue:) syntax,
passing the same string as both the localized key and the defaultValue
parameter. This will enable proper localization support for the picker display
labels while maintaining the current English defaults.
Source: Coding guidelines
Comment on lines
+19
to
+128
| Button("Reset All") { | ||
| store.reset(.about) | ||
| } | ||
| Button("Reapply to Open Windows") { | ||
| store.applyToOpenWindows() | ||
| } | ||
| Button("Copy Config") { | ||
| store.copyConfigToPasteboard() | ||
| } | ||
| } | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| .padding(.top, 2) | ||
| } | ||
|
|
||
| Spacer(minLength: 0) | ||
| } | ||
| .padding(16) | ||
| .frame(maxWidth: .infinity, alignment: .topLeading) | ||
| } | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .topLeading) | ||
| } | ||
|
|
||
| private func editor(for kind: AboutWindowKind) -> some View { | ||
| let overridesEnabled = binding(for: kind, keyPath: \.overridesEnabled) | ||
|
|
||
| return GroupBox(kind.displayTitle) { | ||
| VStack(alignment: .leading, spacing: 10) { | ||
| Toggle("Enable Debug Overrides", isOn: overridesEnabled) | ||
|
|
||
| Text("When disabled, cmux uses normal default titlebar behavior for this window.") | ||
| .font(.caption) | ||
| .foregroundColor(.secondary) | ||
|
|
||
| Divider() | ||
|
|
||
| VStack(alignment: .leading, spacing: 10) { | ||
| HStack(spacing: 8) { | ||
| Text("Window Title") | ||
| TextField("", text: binding(for: kind, keyPath: \.windowTitle)) | ||
| } | ||
|
|
||
| HStack(spacing: 10) { | ||
| Picker("Title Visibility", selection: binding(for: kind, keyPath: \.titleVisibility)) { | ||
| ForEach(TitlebarVisibilityOption.allCases) { option in | ||
| Text(option.displayTitle).tag(option) | ||
| } | ||
| } | ||
| Picker("Toolbar Style", selection: binding(for: kind, keyPath: \.toolbarStyle)) { | ||
| ForEach(TitlebarToolbarStyleOption.allCases) { option in | ||
| Text(option.displayTitle).tag(option) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Toggle("Show Toolbar", isOn: binding(for: kind, keyPath: \.showToolbar)) | ||
| Toggle("Transparent Titlebar", isOn: binding(for: kind, keyPath: \.titlebarAppearsTransparent)) | ||
| Toggle("Movable by Window Background", isOn: binding(for: kind, keyPath: \.movableByWindowBackground)) | ||
|
|
||
| Divider() | ||
|
|
||
| Text("Style Mask") | ||
| .font(.caption) | ||
| .foregroundColor(.secondary) | ||
|
|
||
| Toggle("Titled", isOn: binding(for: kind, keyPath: \.titled)) | ||
| Toggle("Closable", isOn: binding(for: kind, keyPath: \.closable)) | ||
| Toggle("Miniaturizable", isOn: binding(for: kind, keyPath: \.miniaturizable)) | ||
| Toggle("Resizable", isOn: binding(for: kind, keyPath: \.resizable)) | ||
| Toggle("Full Size Content View", isOn: binding(for: kind, keyPath: \.fullSizeContentView)) | ||
|
|
||
| HStack(spacing: 10) { | ||
| Button(String(localized: "debug.aboutTitlebarDebug.resetAbout", defaultValue: "Reset About")) { | ||
| store.reset(kind) | ||
| } | ||
| Button("Apply Now") { | ||
| store.applyToOpenWindows(for: kind) | ||
| } | ||
| } | ||
| } | ||
| .disabled(!overridesEnabled.wrappedValue) | ||
| .opacity(overridesEnabled.wrappedValue ? 1 : 0.75) | ||
| } | ||
| .padding(.top, 2) | ||
| } | ||
| } | ||
|
|
||
| private func binding<Value>( | ||
| for kind: AboutWindowKind, | ||
| keyPath: WritableKeyPath<AboutTitlebarDebugOptions, Value> | ||
| ) -> Binding<Value> { | ||
| Binding( | ||
| get: { store.options(for: kind)[keyPath: keyPath] }, | ||
| set: { newValue in | ||
| var updated = store.options(for: kind) | ||
| updated[keyPath: keyPath] = newValue | ||
| store.update(updated, for: kind) | ||
| } | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Localize all user-facing UI strings in the debug view.
The view contains numerous bare English strings in buttons, toggles, labels, and help text. As per coding guidelines, all user-facing Swift text must use String(localized:defaultValue:). While lines 22 and 100 correctly demonstrate the pattern, the following strings still need localization:
- Line 27:
"Actions"(GroupBox title) - Lines 29, 32, 35, 103: Button labels (
"Reset All","Reapply to Open Windows","Copy Config","Apply Now") - Line 56:
"Enable Debug Overrides"(Toggle label) - Line 58: Help text explaining disabled state
- Lines 66, 71, 76: Text field and picker labels (
"Window Title","Title Visibility","Toolbar Style") - Lines 83-85: Toggle labels for titlebar properties
- Line 89:
"Style Mask"(section label) - Lines 93-97: Style mask toggle labels (
"Titled","Closable","Miniaturizable","Resizable","Full Size Content View")
🌐 Example localization pattern
- GroupBox("Actions") {
+ GroupBox(String(localized: "debug.aboutTitlebarDebug.actions.title", defaultValue: "Actions")) {
HStack(spacing: 10) {
- Button("Reset All") {
+ Button(String(localized: "debug.aboutTitlebarDebug.resetAll", defaultValue: "Reset All")) {
store.reset(.about)
}
- Button("Reapply to Open Windows") {
+ Button(String(localized: "debug.aboutTitlebarDebug.reapplyToOpenWindows", defaultValue: "Reapply to Open Windows")) {
store.applyToOpenWindows()
}Apply this pattern to all remaining bare strings.
🤖 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/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/AboutTitlebarDebug/Views/AboutTitlebarDebugView.swift`
around lines 19 - 128, All user-facing text strings in the
AboutTitlebarDebugView must be localized using the
String(localized:defaultValue:) pattern. Replace all bare English strings with
this localization pattern, following the example already shown in the code.
Specifically, wrap the following with String(localized:defaultValue:): the
GroupBox title "Actions", all button labels ("Reset All", "Reapply to Open
Windows", "Copy Config", "Apply Now"), the Toggle label "Enable Debug
Overrides", the help text explaining the disabled state, all form labels
("Window Title", "Title Visibility", "Toolbar Style"), the section label "Style
Mask", and all style mask toggle labels ("Titled", "Closable", "Miniaturizable",
"Resizable", "Full Size Content View"). Use descriptive localization keys (e.g.,
"debug.aboutTitlebarDebug.actions") and the bare English text as the
defaultValue parameter for each string.
Source: Coding guidelines
Comment on lines
+34
to
+48
| public init( | ||
| maxTextChars: Int, | ||
| maxTerminalChars: Int, | ||
| maxBuildStampChars: Int, | ||
| maxBlobBase64Chars: Int, | ||
| maxBlobBytes: Int, | ||
| maxRetainedBundles: Int | ||
| ) { | ||
| self.maxTextChars = maxTextChars | ||
| self.maxTerminalChars = maxTerminalChars | ||
| self.maxBuildStampChars = maxBuildStampChars | ||
| self.maxBlobBase64Chars = maxBlobBase64Chars | ||
| self.maxBlobBytes = maxBlobBytes | ||
| self.maxRetainedBundles = maxRetainedBundles | ||
| } |
There was a problem hiding this comment.
Validate limit invariants in the initializer.
Lines 34-48 accept negative/zero values, but downstream code assumes valid bounds: prefix(...) in DogfoodFeedbackService (Lines 93-95) and dropLast(keep) (Line 211) can trap for negatives, and maxRetainedBundles == 0 can prune the just-written bundle while still returning .written.
Suggested fix
public init(
maxTextChars: Int,
maxTerminalChars: Int,
maxBuildStampChars: Int,
maxBlobBase64Chars: Int,
maxBlobBytes: Int,
maxRetainedBundles: Int
) {
+ precondition(maxTextChars >= 0, "maxTextChars must be >= 0")
+ precondition(maxTerminalChars >= 0, "maxTerminalChars must be >= 0")
+ precondition(maxBuildStampChars >= 0, "maxBuildStampChars must be >= 0")
+ precondition(maxBlobBase64Chars >= 0, "maxBlobBase64Chars must be >= 0")
+ precondition(maxBlobBytes >= 0, "maxBlobBytes must be >= 0")
+ precondition(maxRetainedBundles > 0, "maxRetainedBundles must be > 0")
self.maxTextChars = maxTextChars
self.maxTerminalChars = maxTerminalChars
self.maxBuildStampChars = maxBuildStampChars
self.maxBlobBase64Chars = maxBlobBase64Chars
self.maxBlobBytes = maxBlobBytes
self.maxRetainedBundles = maxRetainedBundles
}📝 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.
Suggested change
| public init( | |
| maxTextChars: Int, | |
| maxTerminalChars: Int, | |
| maxBuildStampChars: Int, | |
| maxBlobBase64Chars: Int, | |
| maxBlobBytes: Int, | |
| maxRetainedBundles: Int | |
| ) { | |
| self.maxTextChars = maxTextChars | |
| self.maxTerminalChars = maxTerminalChars | |
| self.maxBuildStampChars = maxBuildStampChars | |
| self.maxBlobBase64Chars = maxBlobBase64Chars | |
| self.maxBlobBytes = maxBlobBytes | |
| self.maxRetainedBundles = maxRetainedBundles | |
| } | |
| public init( | |
| maxTextChars: Int, | |
| maxTerminalChars: Int, | |
| maxBuildStampChars: Int, | |
| maxBlobBase64Chars: Int, | |
| maxBlobBytes: Int, | |
| maxRetainedBundles: Int | |
| ) { | |
| precondition(maxTextChars >= 0, "maxTextChars must be >= 0") | |
| precondition(maxTerminalChars >= 0, "maxTerminalChars must be >= 0") | |
| precondition(maxBuildStampChars >= 0, "maxBuildStampChars must be >= 0") | |
| precondition(maxBlobBase64Chars >= 0, "maxBlobBase64Chars must be >= 0") | |
| precondition(maxBlobBytes >= 0, "maxBlobBytes must be >= 0") | |
| precondition(maxRetainedBundles > 0, "maxRetainedBundles must be > 0") | |
| self.maxTextChars = maxTextChars | |
| self.maxTerminalChars = maxTerminalChars | |
| self.maxBuildStampChars = maxBuildStampChars | |
| self.maxBlobBase64Chars = maxBlobBase64Chars | |
| self.maxBlobBytes = maxBlobBytes | |
| self.maxRetainedBundles = maxRetainedBundles | |
| } |
🤖 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/CmuxFeedback/Sources/CmuxFeedback/DogfoodSink/DogfoodFeedbackLimits.swift`
around lines 34 - 48, The init method of DogfoodFeedbackLimits accepts limit
parameters without validating that they are valid (positive or non-zero), which
can cause traps in downstream code when methods like prefix(...) and
dropLast(keep) are called with negative values, and allows maxRetainedBundles to
be zero which incorrectly prunes bundles. Add validation checks in the
initializer to ensure all limit parameters meet their required invariants (e.g.,
greater than zero or positive as appropriate), and raise an error or
precondition failure if invalid values are provided.
Comment on lines
+143
to
+149
| let formatter = ISO8601DateFormatter() | ||
| formatter.formatOptions = [.withInternetDateTime] | ||
| // Colons are legal in HFS+/APFS but awkward in shell globs; swap for `-` | ||
| // so the directory name is paste-safe. | ||
| let timestamp = formatter.string(from: now()).replacingOccurrences(of: ":", with: "-") | ||
| let shortID = String(UUID().uuidString.prefix(8)).lowercased() | ||
| let bundleDir = root.appendingPathComponent("\(timestamp)_\(shortID)", isDirectory: true) |
There was a problem hiding this comment.
Pruning order is not reliably chronological under burst writes.
Lines 143-149 use second-level timestamps plus random IDs, but Lines 201-213 prune by lexicographic directory name. When multiple bundles land in the same second, ordering falls back to random ID, so retention can delete newer bundles (including the just-written one) non-deterministically.
Suggested fix
guard let entries = try? fileManager.contentsOfDirectory(
at: root,
- includingPropertiesForKeys: [.isDirectoryKey],
+ includingPropertiesForKeys: [.isDirectoryKey, .creationDateKey],
options: [.skipsHiddenFiles]
) else { return }
-let directories = entries
- .filter { (try? $0.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) == true }
- .sorted { $0.lastPathComponent < $1.lastPathComponent }
+let directories = entries
+ .compactMap { url -> (url: URL, createdAt: Date)? in
+ let values = try? url.resourceValues(forKeys: [.isDirectoryKey, .creationDateKey])
+ guard values?.isDirectory == true else { return nil }
+ return (url, values?.creationDate ?? .distantPast)
+ }
+ .sorted {
+ if $0.createdAt == $1.createdAt {
+ return $0.url.lastPathComponent < $1.url.lastPathComponent
+ }
+ return $0.createdAt < $1.createdAt
+ }
+ .map(\.url)Also applies to: 201-213
🤖 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/CmuxFeedback/Sources/CmuxFeedback/DogfoodSink/DogfoodFeedbackService.swift`
around lines 143 - 149, The timestamp generation in DogfoodFeedbackService.swift
creates directory names using only second-level precision, which causes
non-deterministic pruning when multiple bundles are created within the same
second because the random ID becomes the tie-breaker for lexicographic ordering.
To fix this, increase the timestamp precision from second-level to include
milliseconds or microseconds in the ISO8601DateFormatter options so that burst
writes generate chronologically ordered directory names that are also
lexicographically sorted. This ensures the pruning logic in the directory name
ordering at lines 201-213 reliably deletes older bundles first, preventing
accidental deletion of newer bundles during retention cleanup.
Comment on lines
+38
to
+50
| @Test("oversized base64 string is rejected without decoding") | ||
| func base64CharCapRejected() async throws { | ||
| let limits = DogfoodFeedbackLimits( | ||
| maxTextChars: 16, maxTerminalChars: 16, maxBuildStampChars: 16, | ||
| maxBlobBase64Chars: 4, maxBlobBytes: 1024, maxRetainedBundles: 5 | ||
| ) | ||
| let (service, _) = makeService(limits: limits) | ||
| let outcome = await service.submit( | ||
| DogfoodFeedbackSubmission(text: "", terminalText: "", buildStamp: "", diagnosticBlobBase64: "AAAAAAAA"), | ||
| authenticatedEmail: "a@manaflow.ai" | ||
| ) | ||
| #expect(outcome == .invalidParams(reason: "diagnostic_blob_base64 exceeds size limit")) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Verify no I/O occurred for consistency.
The unauthorized test (line 35) explicitly checks that no directory is created when a submission is rejected. This test should do the same to confirm the "rejected without decoding" claim and ensure rejection happens before any filesystem operations.
✅ Add fileExists check after line 49
)
`#expect`(outcome == .invalidParams(reason: "diagnostic_blob_base64 exceeds size limit"))
+ `#expect`(!FileManager.default.fileExists(atPath: root.path))
}🤖 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/CmuxFeedback/Tests/CmuxFeedbackTests/DogfoodFeedbackServiceTests.swift`
around lines 38 - 50, Add a file existence check after the outcome assertion in
the base64CharCapRejected() test function to verify that no directory was
created when the submission was rejected. This check should follow the same
pattern used in the unauthorized test to confirm that parameter validation and
rejection occurs before any filesystem operations are attempted.
Comment on lines
+52
to
+65
| @Test("decoded blob over the byte cap is dropped") | ||
| func blobByteCapRejected() async throws { | ||
| let limits = DogfoodFeedbackLimits( | ||
| maxTextChars: 16, maxTerminalChars: 16, maxBuildStampChars: 16, | ||
| maxBlobBase64Chars: 1_000_000, maxBlobBytes: 4, maxRetainedBundles: 5 | ||
| ) | ||
| let (service, _) = makeService(limits: limits) | ||
| let blob = Data(repeating: 0xAB, count: 32).base64EncodedString() | ||
| let outcome = await service.submit( | ||
| DogfoodFeedbackSubmission(text: "", terminalText: "", buildStamp: "", diagnosticBlobBase64: blob), | ||
| authenticatedEmail: "a@manaflow.ai" | ||
| ) | ||
| #expect(outcome == .invalidParams(reason: "diagnostic blob exceeds size limit")) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Verify no I/O occurred for consistency.
Same as the base64CharCapRejected test: add a fileExists check to confirm rejection happens before any filesystem operations, consistent with the unauthorized test pattern.
✅ Add fileExists check after line 64
)
`#expect`(outcome == .invalidParams(reason: "diagnostic blob exceeds size limit"))
+ `#expect`(!FileManager.default.fileExists(atPath: root.path))
}🤖 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/CmuxFeedback/Tests/CmuxFeedbackTests/DogfoodFeedbackServiceTests.swift`
around lines 52 - 65, The blobByteCapRejected test verifies the rejection
outcome but does not confirm that no filesystem operations occurred. Add a
fileExists check after the outcome assertion (after the expect statement at line
64) to verify that the expected file does not exist, confirming that rejection
happens before any I/O operations are performed. This makes the test consistent
with the pattern used in the unauthorized test and the base64CharCapRejected
test.
Comment on lines
+132
to
+164
| @Test("pruning keeps only the newest N bundle directories") | ||
| func prunesOldBundles() async throws { | ||
| let limits = DogfoodFeedbackLimits( | ||
| maxTextChars: 16, maxTerminalChars: 16, maxBuildStampChars: 16, | ||
| maxBlobBase64Chars: 1_000_000, maxBlobBytes: 1_000_000, maxRetainedBundles: 2 | ||
| ) | ||
| let root = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("cmux-dogfood-prune-\(UUID().uuidString)", isDirectory: true) | ||
| // Distinct, monotonically increasing timestamps so the lexicographic | ||
| // sort is chronological and deterministic across writes. | ||
| var tick = 1_700_000_000.0 | ||
| let payload = Data("x".utf8).base64EncodedString() | ||
| for _ in 0..<5 { | ||
| let captured = tick | ||
| let service = DogfoodFeedbackService( | ||
| limits: limits, | ||
| cacheRoot: root, | ||
| now: { Date(timeIntervalSince1970: captured) } | ||
| ) | ||
| _ = await service.submit( | ||
| DogfoodFeedbackSubmission(text: "", terminalText: "", buildStamp: "", diagnosticBlobBase64: payload), | ||
| authenticatedEmail: "dev@manaflow.ai" | ||
| ) | ||
| tick += 60 | ||
| } | ||
| let remaining = try FileManager.default.contentsOfDirectory( | ||
| at: root, | ||
| includingPropertiesForKeys: nil, | ||
| options: [.skipsHiddenFiles] | ||
| ) | ||
| #expect(remaining.count == 2) | ||
| try? FileManager.default.removeItem(at: root) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider verifying which bundles are retained.
The test confirms that 2 bundles remain after pruning, but doesn't verify they are the newest 2 (as claimed by the test name "keeps only the newest N"). You could inspect the remaining URLs to confirm they correspond to the last 2 timestamps in the sequence.
📋 Optional: verify the retained bundles are the newest
let remaining = try FileManager.default.contentsOfDirectory(
at: root,
includingPropertiesForKeys: nil,
options: [.skipsHiddenFiles]
- )
+ ).sorted(by: { $0.lastPathComponent < $1.lastPathComponent })
`#expect`(remaining.count == 2)
+ // Verify the newest 2 bundles remain (ticks 1_700_000_240 and 1_700_000_300)
+ `#expect`(remaining[0].lastPathComponent.contains("1_700_000_240"))
+ `#expect`(remaining[1].lastPathComponent.contains("1_700_000_300"))
try? FileManager.default.removeItem(at: root)📝 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.
Suggested change
| @Test("pruning keeps only the newest N bundle directories") | |
| func prunesOldBundles() async throws { | |
| let limits = DogfoodFeedbackLimits( | |
| maxTextChars: 16, maxTerminalChars: 16, maxBuildStampChars: 16, | |
| maxBlobBase64Chars: 1_000_000, maxBlobBytes: 1_000_000, maxRetainedBundles: 2 | |
| ) | |
| let root = FileManager.default.temporaryDirectory | |
| .appendingPathComponent("cmux-dogfood-prune-\(UUID().uuidString)", isDirectory: true) | |
| // Distinct, monotonically increasing timestamps so the lexicographic | |
| // sort is chronological and deterministic across writes. | |
| var tick = 1_700_000_000.0 | |
| let payload = Data("x".utf8).base64EncodedString() | |
| for _ in 0..<5 { | |
| let captured = tick | |
| let service = DogfoodFeedbackService( | |
| limits: limits, | |
| cacheRoot: root, | |
| now: { Date(timeIntervalSince1970: captured) } | |
| ) | |
| _ = await service.submit( | |
| DogfoodFeedbackSubmission(text: "", terminalText: "", buildStamp: "", diagnosticBlobBase64: payload), | |
| authenticatedEmail: "dev@manaflow.ai" | |
| ) | |
| tick += 60 | |
| } | |
| let remaining = try FileManager.default.contentsOfDirectory( | |
| at: root, | |
| includingPropertiesForKeys: nil, | |
| options: [.skipsHiddenFiles] | |
| ) | |
| #expect(remaining.count == 2) | |
| try? FileManager.default.removeItem(at: root) | |
| } | |
| let remaining = try FileManager.default.contentsOfDirectory( | |
| at: root, | |
| includingPropertiesForKeys: nil, | |
| options: [.skipsHiddenFiles] | |
| ).sorted(by: { $0.lastPathComponent < $1.lastPathComponent }) | |
| `#expect`(remaining.count == 2) | |
| // Verify the newest 2 bundles remain (ticks 1_700_000_180 and 1_700_000_240) | |
| `#expect`(remaining[0].lastPathComponent.contains("1_700_000_180")) | |
| `#expect`(remaining[1].lastPathComponent.contains("1_700_000_240")) | |
| try? FileManager.default.removeItem(at: root) |
🤖 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/CmuxFeedback/Tests/CmuxFeedbackTests/DogfoodFeedbackServiceTests.swift`
around lines 132 - 164, The test prunesOldBundles verifies that exactly 2
bundles remain after pruning, but does not validate that they are the newest 2
as the test name suggests. After obtaining the remaining bundle URLs from
contentsOfDirectory and confirming the count is 2, inspect those URLs to verify
they correspond to the last 2 timestamps in the sequence (the final 2 iterations
when tick had the highest values). Extract the timestamps from the remaining
directory names and assert they match the expected newest timestamps to ensure
the oldest bundles were actually pruned rather than random ones.
…tring The caseless enum ComposerDictationTextMerge (added by #6197, on main) trips the namespace-enum convention lint, which scans the whole iOS tree and fails package-conventions-lint on every PR. Convert the pure base+transcript merge into a receiver-natural String extension method, mergingDictation(transcript:), per the linter's recommended pattern, and update the controller call site and host tests. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The extracted store is now public package code with a unit test. Splitting the pure configSnapshot() from copyConfigToPasteboard() lets the test assert the payload without clearing the real NSPasteboard.general on the dev/CI process. No behavior change to the menu action. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Shepherd regression-check notes (no merge; lead serializes merges): CI fix pushed. Pasteboard testability. Split Regression check (extraction is behavior-preserving). Verified the AboutTitlebarDebug extraction is a 1:1 type move (AboutTitlebarDebugStore/Options/View/WindowController, AboutWindowKind, Titlebar*Option) from Consciously rejecting the remaining autoreview P3 (localize About Titlebar Debug labels): those strings moved verbatim from |
# Conflicts: # .github/swift-file-length-budget.tsv # Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/ComposerDictationController.swift # Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/ComposerDictationTextMerge.swift # Packages/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/ComposerDictationTests.swift
# Conflicts: # .github/swift-file-length-budget.tsv
# Conflicts: # Packages/macOS/CmuxAppKitSupportUI/Tests/CmuxAppKitSupportUITests/AboutTitlebarDebugStoreTests.swift # Packages/macOS/CmuxFeedback/Tests/CmuxFeedbackTests/DogfoodFeedbackServiceTests.swift
This branch was successfully deployed
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
1 participant
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.
Folds two per-sliver refactor branches into existing domain packages so the debug-group extractions land with zero new top-level packages. The owner rejected the per-sliver packages; the extractions are kept, but they now live in the existing domain packages.
What folded
feat-mobile-host-rpc-router extracted the privileged Mac<->phone dogfood feedback sink into a new
CmuxDogfoodFeedbackSinkpackage. That domain is folded into the existingCmuxFeedbackpackage underSources/CmuxFeedback/DogfoodSink/:DogfoodFeedbackLimits,DogfoodFeedbackOutcome,DogfoodFeedbackSubmission(Sendable value types) and thenonisolated Sendable DogfoodFeedbackService.TerminalController.v2MobileDogfoodFeedbackSubmitis a thin forward that resolves the authenticated email via the main-actorMobileHostServiceand callsservice.submit(...); the service re-enforces the@manaflow.aigate at the trust boundary.CmuxFeedbackwas already imported byTerminalControllerand already linked to the app target, so no import or pbxproj change was needed.feat-debug-windows-extraction extracted the self-contained About-titlebar debug cluster (~444L) into a new
CmuxDebugWindowsUIpackage. That UI is folded into the existingCmuxAppKitSupportUIpackage underSources/CmuxAppKitSupportUI/AboutTitlebarDebug/: theAboutWindowKind/TitlebarVisibilityOption/TitlebarToolbarStyleOptionvalue enums,AboutTitlebarDebugOptions,AboutTitlebarDebugStore(@MainActor @Observable, single writer),AboutTitlebarDebugWindowController,AboutTitlebarDebugView, theDebugWindowsCoordinator, and theWindowDecoratingprotocol seam.AppDelegateconforms toWindowDecoratingand owns the coordinator (held weakly to avoid a retain cycle);cmuxApp.swiftand the About/Acknowledgments controllers forward into the app-owned coordinator/store.CmuxAppKitSupportUIalready exists and is already linked to the app target.Both member branches branched off an older main (
5321becb), beforeCmuxFeedbackandCmuxAppKitSupportUIexisted, which is why they created standalone packages. Zero new top-level packages and zero pbxproj entries are added by this PR.Byte-identical
Feedback sink: same caps, base64-char-cap-before-decode then byte-cap ordering,
Task.detached(.utility)off-main write, ISO8601-colons-to-dash bundle naming, 0700/0600 perms,bundle.jsonschema/sorted-keys/pretty, lexicographic prune keeping newest 50, same RPC error codes/messages and.okpayload keys. Debug windows: same window identifiers, titles, style-mask bits, toolbar identifiers, min/max sizes, copy-config payload, anddidSet-driven reapply.Verification
scripts/lint-ios-package-conventions.sh: no new violation, zerolint:allow. The single pre-existing ERROR (ComposerDictationTextMergeinCmuxMobileShellUI) is on cleanmainand unrelated.swift build+swift testgreen in both packages:CmuxFeedback12 tests,CmuxAppKitSupportUI8 tests.TerminalController14829→14681,cmuxApp4921→4516,AppDelegate17894→17905 (+11 composition-root wiring).swift_file_length_budget.pypasses.xcodebuildleft to CI per refactor policy.Supersedes the per-sliver package PRs from
feat-mobile-host-rpc-routerandfeat-debug-windows-extraction.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Consolidates the privileged dogfood feedback sink and the About-titlebar debug UI into existing packages
CmuxFeedbackandCmuxAppKitSupportUI. No new top-level packages; behavior stays the same.Refactors
CmuxFeedback/DogfoodSink/: addedDogfoodFeedbackLimits,DogfoodFeedbackOutcome,DogfoodFeedbackSubmission,DogfoodFeedbackService. Service caps fields, rejects oversized base64 before decode, writes bundles off-main, prunes, and preserves existing RPC responses.TerminalController.v2MobileDogfoodFeedbackSubmitnow resolves the authenticated email and forwards to the service.CmuxAppKitSupportUI/AboutTitlebarDebug/: addedAboutTitlebarDebugStore,AboutTitlebarDebugView,AboutTitlebarDebugWindowController,DebugWindowsCoordinator, andWindowDecorating.AppDelegateconforms toWindowDecorating, owns the coordinator, and menu/About/Acknowledgments forward into it.configSnapshot()split fromcopyConfigToPasteboard()for testability. Byte-identical details preserved (caps/order, ISO8601-with-dashes naming, 0700/0600 perms, sorted/prettybundle.json, prune keeps newest 50; same window identifiers/titles/style bits/sizes).Tests
Written for commit 8441595. Summary will update on new commits.
Summary by CodeRabbit
Release Notes