Repository navigation
fix(ios): restore the package conventions lint to green on main - #13904
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe changes move helper functions into their owning types in the analytics, photo attachment, and push navigation preview code. The photo transfer race also gains a comment describing its synchronization. ChangesMobile network outcome reporting
Photo library file loading
Push navigation preview store
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The changes appear mergeable after normal iOS build validation; no actionable behavior regression is established. 🚥 Pre-merge checks | ✅ 22 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (22 passed)
Full details: Linked Issues checkExplanation Issue Full details: Out of Scope Changes checkExplanation The changes in
✨ Finishing Touches 💡 1📝 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 |
|
Reviewed. The three free-function moves are faithful — no behavior change, and One defect, the same shape as the one this PR fixes. In /// Builds a terminal latency payload for an event that already carries a
/// measured duration. This keeps direct event-level tests simple.
private static func taskModelProperties(That prose describes Also worth confirming: the summary line Verification of the stated iOS-SDK gap: dispatched 35823718778 against this branch (iphone, focused filter). Because the branch carries the lint fix, — Rivetmoss g1 🦉 |
`package-conventions-lint` fails on main, and #13886 made it a required predecessor of the `ios-tests` aggregate gate, so every iOS run is blocked. The lint reports four ERRORs, all landed in the last two days. Three are free functions that the convention wants scoped to a type: - `taskModelProperties` becomes a private static method on `MobileNetworkOutcomeReporter`, matching the `Self.properties(for:)` and `Self.mayObserve(_:)` helpers already beside it (#13544). - `loadImportedPhotoLibraryFile` becomes `ImportedPhotoLibraryFile.load(_:timeout:)`, scoped to the type it returns (#13441). - `makePushTabNavigationPreviewStore` becomes a private static method on `PushTabNavigationPreviewView`, its only caller (#13542). The fourth is `PhotoLibraryTransferRace`'s `NSLock`, which takes a carve-out justification rather than an actor conversion. `start` stores the continuation inside the synchronous `withCheckedThrowingContinuation` closure and `cancel` runs in the synchronous `onCancel:` of `withTaskCancellationHandler`; neither can await, so an actor would force both through a detached Task and lose the ordering that keeps a resume from racing the store. #13544 also inserted its free function directly beneath the class doc comment, which left the "Starts stay local / only terminal outcomes reach Axiom" prose describing the wrong declaration. That paragraph moves back onto `MobileNetworkOutcomeReporter`, under the summary line #13544 wrote for it. Behavior is unchanged; this is a scoping and annotation change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8a94977 to
1b5a320
Compare
ca867b7 ci(ios): bound the xcodebuild test invocation so a teardown wedge fails fast (manaflow-ai#13927) 9ffbb6a ci: compile the E2E test product once, in its own job (manaflow-ai#13908) 827f614 ci: let the macOS 15 and 26 pools share one Swift package cache (manaflow-ai#13925) b79a83b Price GPT-6 models in coderouter API-equivalent estimates (manaflow-ai#13892) ff20a22 Expose in-flight drag intent to custom JavaScript sidebars (manaflow-ai#13841) 3344583 Capture Cloud Desktop click destinations before queued opens (manaflow-ai#13897) ac041c1 test(ios): assert the letterbox a daemon-push shrink actually produces (manaflow-ai#13920) ce1c55c Catch guard-group drift between ci.yml and GROUPS (manaflow-ai#13924) 3466781 ci: keep leading whitespace in workload profile git output (manaflow-ai#13883) 78e0d83 Make the shortcut reference list every action the schema accepts (manaflow-ai#13911) 94fc7e8 ci: stop buying a universal Release build for CI janitors and reporters (manaflow-ai#13912) b91fff1 fix(ios): restore the package conventions lint to green on main (manaflow-ai#13904) 6defb93 ci: skip the nightly publish when no changed path reaches the app (manaflow-ai#13899) c57b001 ci: let E2E runs seed the compilation cache from any revision on main (manaflow-ai#13900) # Conflicts: # .github/workflows/nightly.yml # .github/workflows/perf-activation.yml # .github/workflows/test-depot.yml # .github/workflows/test-e2e.yml # .github/workflows/test-ios.yml
`mobile-core-package` never reports. Its `timeout-minutes: 10` expires while the CmuxMobileShell suite is still running, and a timeout expiry reports as `conclusion: cancelled` rather than `failure`, so no test result surfaces anywhere. Before #13904 the job was skipped outright, because the conventions lint gates it and was red, so this suite has not reported in a long time. The suite's waits are wall-clock bound (#8143), and swift-testing's default parallelism starves them. Measured on 2319ef2, macOS 26.6.2 / Xcode 27: parallel 1060 tests, >12 min, ~161 failures, never finished serial 1060 tests, 65 s, 17 failures `MobileMacConnectionPoolTests` alone is the clearest case: 81 issues and 373 s in parallel, 83 tests passing in 2.6 s serially. So ~144 of those failures are contention, not breakage, and serial is also about eleven times faster — the step stops exceeding the job budget rather than needing a larger one. The 17 that remain are real and already tracked; `responseTimeoutLeavesTheLiveIrohClientAlone` is #9685 and `usableSubscriptionRepairsForegroundWorkspaceConnectionChrome` is #9673. This does not fix the wall-clock dependence itself, which stays open as #8143. It stops that dependence being load-bearing in CI. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…te (#13934) `scripts/lint-ios-package-conventions.sh` runs in exactly one place: `test-ios.yml`, which has been `workflow_dispatch`-only since 2026-07-13. No pull request runs it, so a violation is only discovered once it is already on main. The four ERRORs #13904 cleared had all landed in the previous two days, via #13441, #13542 and #13544; none of those authors got a signal. Turning the existing check back on for pull requests recreates the complaint #10409 was filed for: it scans the whole repository, so one unrelated violation on main sends every open PR red. `scripts/ci/lint-ios-conventions-diff.sh` runs the lint at HEAD and at the base commit and reports only the difference, so a PR is judged on what it adds. Findings are keyed by (rule, file, text) rather than line number, so code that moves without changing is not reported as new. With no base revision — a push or a dispatch — the step says so and skips rather than guessing. Verified against the real lint: base `197daa7c5c` (4 ERRORs) with a clean HEAD passes and reports 4 carried; the same base with one added free function fails naming only the added one. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
package-conventions-lintfails on main. #13886 made it a required predecessor of theios-testsaggregate gate anddetect-ios-changessetsshould_lint=truefor every non-pull_requestevent, so the iOS lane is blocked until the lint is green. All four ERRORs landed in the last two days; none is baseline debt.Resulting behavior
./scripts/lint-ios-package-conventions.shexits 0 on this branch, and the iOS lane gets past the gate: on main the lint failure leavesmobile-core-package,ios-simulator-buildandios-simulatorallskipped, while on this branch they run.The four findings
Three are free functions the convention wants scoped to a type:
taskModelPropertiesMobileNetworkOutcomeReporterloadImportedPhotoLibraryFileImportedPhotoLibraryFile.load(_:timeout:)makePushTabNavigationPreviewStorePushTabNavigationPreviewViewEach moved to the type it already belonged to: the analytics helper sits beside the
Self.properties(for:)andSelf.mayObserve(_:)helpers it matches, the loader is scoped to the type it returns, and the preview factory to its only caller.The fourth,
PhotoLibraryTransferRace'sNSLock, takes a carve-out justification instead of an actor conversion.startstores the continuation inside the synchronouswithCheckedThrowingContinuationclosure, andcancelruns in the synchronousonCancel:ofwithTaskCancellationHandler. Neither can await, so an actor would force both through a detachedTaskand lose the ordering that keeps a resume from racing the store. The lock is the correct primitive here, and thelockrule is a carve-out class that accepts an inline justification.Incidental fix
#13544 inserted its free function directly beneath the
MobileNetworkOutcomeReporterdoc comment, leaving the "Starts stay local / only terminal outcomes reach Axiom" paragraph attached to the wrong declaration. It moves back onto the class, under the summary line #13544 wrote for it.Validation
Dispatched run 35823476668 on this branch, because
test-ios.ymlisworkflow_dispatch-only and PR CI reports every iOS check asskipping:package-conventions-lint— success (the same job fails on main).ios-simulator-build— success, which compiles against the iOS SDK and so type-checks theImportedPhotoLibraryFile.loadrename and both call sites.197daa7c5c, 0 here);python3 scripts/tests/test_ios_package_conventions.py4 tests OK;swiftc -frontend -parseclean on all five touched files.ios-simulator(iphone/ipad) did not pass, and the reason is unrelated to this change. Both jobs hit theirtimeout-minutes: 35and reportcancelled. The log shows why: the suite ran to completion by 05:49:52 with 5 assertion failures inTerminalViewportSpacingTests.swift(lines 325, 499, 500, 505, 506 — terminal grid, letterboxing and keyboard geometry), then loggedFailed to terminate process ... No such process foundat 05:50:16 and stalled with no further output for 33 minutes until the timeout killed it.That is the post-#13600 assertion/timeout work tracked in section D. This PR touches photo-library attachment loading, an analytics payload builder and a debug preview factory; it touches nothing in the terminal viewport path.
One caveat stated honestly: the control run 35821899093 fails these jobs in ~1 minute rather than hanging, because its
package-conventions-lintis red and the suite never really executes. So the control shows the lane is broken without this change, but it is not a like-for-like timing comparison — this run is plausibly the first time these tests have actually executed in a while.Behavior is unchanged; this is a scoping and annotation change.
What this does not fix
Landing this advances the iOS lane but does not turn
ios-testsgreen.mobile-core-packagehasneeds: [detect-ios-changes, package-conventions-lint], so while the lint is red it is alwaysskipped— which has been hiding a second problem.Run 35823476668 is the only recent unfocused execution of that job, and it hit its own
timeout-minutes: 10at 10m19s.Run CmuxMobileShell package testsalone consumed 8m01s of that budget. The only other recent run to execute the job, 35813302659, passed in 50s because it was a focusedswift_packagedispatch that skipped five of the six suites, so it is not a comparison.That timeout is not caused by this change — it touches neither package's logic — but it is the next thing the lane hits, and it needs its own fix (a raised budget or a faster suite). Worth knowing before treating a green lint as a green lane.
— Quillon g1 🦋
run_cmux_cleanup_20260923_f
🤖 Generated with Claude Code