Repository navigation
Retain iOS app and network log history - #10082
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAppLog now supports bounded timestamped archives for app and network logs. It preserves generations across launches, migrates legacy ChangesLog archive lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟡 Moderate · up to This PR preserves and shares historical logs, but the current implementation can delete or omit retained generations in some filesystem states and can repeatedly scan log files while the Settings screen renders. Merge should wait for these bounded retention and performance issues to be addressed. Sequence Diagram(s)sequenceDiagram
participant SettingsView
participant AppLog
participant LogFile
SettingsView->>AppLog: request appLogFileURLs or networkLogFileURLs
AppLog->>LogFile: discover active and archived URLs
LogFile-->>AppLog: return available generation URLs
AppLog-->>SettingsView: return log URL collection
SettingsView->>SettingsView: create ShareLink for all URLs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 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 |
![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/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift`:
- Around line 814-816: In MobileSettingsView, add `@State` properties for appURLs
and networkURLs, populate both once in a section-level .task by reading
AppLog.appLogFileURLs and AppLog.networkLogFileURLs off the main actor, and
replace the inline accessor calls at MobileSettingsView.swift:814-816 and
MobileSettingsView.swift:830-832 with the stored state values.
In `@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/AppLog.swift`:
- Around line 306-313: Update AppLog.append(_:) to reuse the single encoded Data
value for both rotation sizing and output. Add or adapt a private write method
that accepts Data, then have append pass its existing data to it instead of
calling the String-based write(_:) path, while preserving newline and rotation
behavior.
- Around line 133-142: Update the archive sorting in logFileURLs(for:) to use
the parsed stamp from makeArchiveURL as the primary descending key, adding the
private archiveStamp(of:prefix:) helper alongside the archive helpers. Use
contentModificationDateKey only when either archive name cannot be parsed, and
preserve a deterministic filename tie-breaker; ensure unparseable names do not
silently outrank or replace valid stamped archives.
- Around line 128-132: Update the archive candidate filter in archiveURLs to
compare the candidate’s filename suffix with the source fileURL’s relevant
suffix rather than comparing parsed pathExtension values. Preserve the existing
prefix and file-existence checks, and ensure extension-less fileURL values match
archives produced by makeArchiveURL so pruning and diagnostic exports discover
them.
In `@Packages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/AppLogTests.swift`:
- Around line 180-206: Update boundsTimestampedArchiveCount to set
maxRetainedBytes below the archive-count-bounded total so pruneArchives
exercises retained-byte pruning. Replace the fragile combined totalBytes
assertion with separate assertions for the archive count and retained-byte
limit, preserving the expectation that archives remain present.
🪄 Autofix
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 Plus
Run ID: 34627409-caf4-4ad4-9ea3-25fcb414159c
📒 Files selected for processing (3)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/AppLog.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/AppLogTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift
Comment on lines
+128
to
+132
| .filter { candidate in | ||
| candidate.lastPathComponent.hasPrefix(prefix) | ||
| && candidate.pathExtension == fileURL.pathExtension | ||
| && fileManager.fileExists(atPath: candidate.path) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Archive discovery fails for a log URL without a path extension.
makeArchiveURL produces "<stem>.archive-<stamp>-<uuid>" when fileURL.pathExtension is empty. The pathExtension of that name is "archive-<stamp>-<uuid>", so candidate.pathExtension == fileURL.pathExtension is never true. archiveURLs then returns an empty array for such a location. Two consequences follow: pruneArchives returns early at Line 321 and retention limits are never enforced, and logFileURLs(for:) omits every archive from a diagnostic export. logFileURLs(for:) is public and accepts a caller-supplied URL, so this is reachable outside the default .log filenames.
Compare the suffix instead of the parsed extension.
🐛 Proposed fix for extension-less locations
+ let suffix = fileURL.pathExtension.isEmpty ? "" : ".\(fileURL.pathExtension)"
return names
.filter { candidate in
- candidate.lastPathComponent.hasPrefix(prefix)
- && candidate.pathExtension == fileURL.pathExtension
- && fileManager.fileExists(atPath: candidate.path)
+ let name = candidate.lastPathComponent
+ return name.hasPrefix(prefix)
+ && name.hasSuffix(suffix)
+ && fileManager.fileExists(atPath: candidate.path)
}📝 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
| .filter { candidate in | |
| candidate.lastPathComponent.hasPrefix(prefix) | |
| && candidate.pathExtension == fileURL.pathExtension | |
| && fileManager.fileExists(atPath: candidate.path) | |
| } | |
| let suffix = fileURL.pathExtension.isEmpty ? "" : ".\(fileURL.pathExtension)" | |
| return names | |
| .filter { candidate in | |
| let name = candidate.lastPathComponent | |
| return name.hasPrefix(prefix) | |
| && name.hasSuffix(suffix) | |
| && fileManager.fileExists(atPath: candidate.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/Shared/CMUXMobileCore/Sources/CMUXMobileCore/AppLog.swift` around
lines 128 - 132, Update the archive candidate filter in archiveURLs to compare
the candidate’s filename suffix with the source fileURL’s relevant suffix rather
than comparing parsed pathExtension values. Preserve the existing prefix and
file-existence checks, and ensure extension-less fileURL values match archives
produced by makeArchiveURL so pruning and diagnostic exports discover them.
Comment on lines
306
to
313
| mutating func append(_ line: String) { | ||
| guard handle != nil else { return } | ||
| let data = Data((line + "\n").utf8) | ||
| if bytesWritten + data.count > rotationThreshold { | ||
| try? handle?.close() | ||
| handle = nil | ||
| openFreshGeneration(rotatingExisting: true) | ||
| guard handle != nil else { return } | ||
| _ = rotate() | ||
| } | ||
| write(line) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Build the line data once per append.
append(_:) encodes line + "\n" to measure the rotation threshold, then write(_:) at Line 349 encodes the same string again. Every logged line pays two allocations and two UTF-8 conversions. Pass the encoded data to a private write that accepts Data.
🤖 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/Shared/CMUXMobileCore/Sources/CMUXMobileCore/AppLog.swift` around
lines 306 - 313, Update AppLog.append(_:) to reuse the single encoded Data value
for both rotation sizing and output. Add or adapt a private write method that
accepts Data, then have append pass its existing data to it instead of calling
the String-based write(_:) path, while preserving newline and rotation behavior.
Comment on lines
+180
to
+206
| @Test func boundsTimestampedArchiveCount() async throws { | ||
| let dir = try makeTempDirectory() | ||
| defer { try? FileManager.default.removeItem(at: dir) } | ||
| let appURL = dir.appendingPathComponent("app.log") | ||
| let log = AppLog( | ||
| appFileURL: appURL, | ||
| networkFileURL: nil, | ||
| maxFileBytes: 160, | ||
| buildStamp: "test", | ||
| maxArchiveCount: 2, | ||
| maxRetainedBytes: 480 | ||
| ) | ||
|
|
||
| for index in 0..<100 { | ||
| log.mirrorAppLine("bounded line \(index) 0123456789") | ||
| } | ||
| try await waitForProcessed(log, 100) | ||
|
|
||
| let archives = AppLog.logFileURLs(for: appURL).filter { $0 != appURL } | ||
| #expect(archives.count <= 2) | ||
| #expect(!archives.isEmpty) | ||
| let totalBytes = AppLog.logFileURLs(for: appURL).reduce(0) { result, url in | ||
| let size = try? url.resourceValues(forKeys: [.fileSizeKey]).fileSize | ||
| return result + (size ?? 0) | ||
| } | ||
| #expect(totalBytes <= 480) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The retained-byte limit is not actually exercised, and the assertion is fragile.
With maxFileBytes: 160, a generation holds the header (about 55 bytes) plus one line (about 52 bytes, including the 24-character ISO timestamp) and rotates at roughly 159 bytes. maxArchiveCount: 2 caps the archives at about 318 bytes, so the total stays near 477 bytes. The 480-byte ceiling is never reached. Two problems follow: the retained-byte branch of pruneArchives never runs, and #expect(totalBytes <= 480) passes with a margin of a few bytes, so a change to the header text or timestamp format breaks the test for an unrelated reason.
Set maxRetainedBytes below the count-bounded total so the byte prune runs, and assert the archive count separately.
💚 Proposed test change
maxArchiveCount: 2,
- maxRetainedBytes: 480
+ // Below the count-bounded total, so the retained-byte prune runs.
+ maxRetainedBytes: 320
)
@@
- `#expect`(totalBytes <= 480)
+ `#expect`(totalBytes <= 320 + 160)📝 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 func boundsTimestampedArchiveCount() async throws { | |
| let dir = try makeTempDirectory() | |
| defer { try? FileManager.default.removeItem(at: dir) } | |
| let appURL = dir.appendingPathComponent("app.log") | |
| let log = AppLog( | |
| appFileURL: appURL, | |
| networkFileURL: nil, | |
| maxFileBytes: 160, | |
| buildStamp: "test", | |
| maxArchiveCount: 2, | |
| maxRetainedBytes: 480 | |
| ) | |
| for index in 0..<100 { | |
| log.mirrorAppLine("bounded line \(index) 0123456789") | |
| } | |
| try await waitForProcessed(log, 100) | |
| let archives = AppLog.logFileURLs(for: appURL).filter { $0 != appURL } | |
| #expect(archives.count <= 2) | |
| #expect(!archives.isEmpty) | |
| let totalBytes = AppLog.logFileURLs(for: appURL).reduce(0) { result, url in | |
| let size = try? url.resourceValues(forKeys: [.fileSizeKey]).fileSize | |
| return result + (size ?? 0) | |
| } | |
| #expect(totalBytes <= 480) | |
| } | |
| @Test func boundsTimestampedArchiveCount() async throws { | |
| let dir = try makeTempDirectory() | |
| defer { try? FileManager.default.removeItem(at: dir) } | |
| let appURL = dir.appendingPathComponent("app.log") | |
| let log = AppLog( | |
| appFileURL: appURL, | |
| networkFileURL: nil, | |
| maxFileBytes: 160, | |
| buildStamp: "test", | |
| maxArchiveCount: 2, | |
| // Below the count-bounded total, so the retained-byte prune runs. | |
| maxRetainedBytes: 320 | |
| ) | |
| for index in 0..<100 { | |
| log.mirrorAppLine("bounded line \(index) 0123456789") | |
| } | |
| try await waitForProcessed(log, 100) | |
| let archives = AppLog.logFileURLs(for: appURL).filter { $0 != appURL } | |
| #expect(archives.count <= 2) | |
| #expect(!archives.isEmpty) | |
| let totalBytes = AppLog.logFileURLs(for: appURL).reduce(0) { result, url in | |
| let size = try? url.resourceValues(forKeys: [.fileSizeKey]).fileSize | |
| return result + (size ?? 0) | |
| } | |
| #expect(totalBytes <= 320 + 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/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/AppLogTests.swift`
around lines 180 - 206, Update boundsTimestampedArchiveCount to set
maxRetainedBytes below the archive-count-bounded total so pruneArchives
exercises retained-byte pruning. Replace the fragile combined totalBytes
assertion with separate assertions for the archive count and retained-byte
limit, preserving the expectation that archives remain present.
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.
Persist the active app and network logs across launches, rotate them into uniquely named archives at the size limit, and enforce bounded count and byte retention.
Share every retained generation from Settings so diagnostics include history instead of only the current file. Legacy .1 rotations are migrated without losing their contents.
Tests: swift test --package-path Packages/Shared/CMUXMobileCore --filter AppLogTests
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Retains iOS app and network log history across launches and includes it in diagnostics. Previously we rotated to .1 and Settings shared only the active file; now we reopen the active generation, rotate to timestamped archives, bound retention by count and total bytes, and share all generations.
CMUXMobileCore:AppLogadds timestamped archive rotation, bounded retention (defaults: 5 MB per file, 3 archives, 12 MB total), and reopens the active generation on launch. Rotation failures append to the current file instead of truncating. Legacy<name>.1files migrate into the archive namespace without data loss. Retained generations are discovered safely and ordered by embedded generation stamp (with modification-date fallback).CMUXMobileCore: New APIsAppLog.appLogFileURLs,AppLog.networkLogFileURLs, andlogFileURLs(for:)expose the active file plus archives (including any unmigrated legacy.1). The initializer adds optionalmaxArchiveCount,maxRetainedBytes, andnowparameters; existing call sites do not need changes.CmuxMobileShellUI: Settings shares all retained generations viaShareLink(items:), loading URL lists off the main thread so exported diagnostics include recent history.Written for commit ab43b7d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes