Repository navigation
Lazy load feed activity history - #3457
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds stable, byte-cursor pagination for persisted workstream history: persistence returns a new public ChangesLazy-Loading Persisted History Pagination
Sequence Diagram(s)sequenceDiagram
participant UI as User / UI
participant VM as FeedPanelViewModel
participant Store as WorkstreamStore
participant Persist as WorkstreamPersistence
participant File as JSONL File
UI->>VM: arm() / init
VM->>Store: start()
Store->>Persist: loadPage(endingBefore: nil, limit: N)
Persist->>File: read tail, compute newline byte ranges
Persist-->>Store: Page(items:[recent], hasMoreBefore:true, startOffset:X)
Store-->>VM: update items, hasMorePersistedItems
VM-->>UI: render feed (show load-more)
rect rgba(100, 200, 100, 0.5)
UI->>VM: loadOlderItems() (appear / tap)
VM->>Store: loadOlderItems()
Store->>Persist: loadPage(endingBefore: X, limit: N)
Persist->>File: read prior ranges, compute offsets
Persist-->>Store: Page(items:[older], hasMoreBefore:bool, startOffset:Y)
Store-->>VM: prepend items, update cursor/flags
VM-->>UI: refresh feed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 6/8 reviews remaining, refill in 11 minutes and 24 seconds.Comment |
9f46179 to
5848881
Compare
Greptile SummaryThis PR introduces byte-cursor-based pagination for the JSONL activity feed, loading only the most recent 300 rows at startup and fetching older history on demand via a sentinel row in All findings are P2. The most notable: Confidence Score: 4/5Safe to merge; all findings are P2 style/design concerns with no definitive current breakage. No P0 or P1 issues found. Four P2 findings: ring capacity not enforced during older-item prepend, chronological-order assumption in
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as FeedHistoryLoadMoreRow
participant VM as FeedPanelViewModel
participant Store as WorkstreamStore
participant Persist as WorkstreamPersistence
Note over Store: start() — initial load
Store->>Persist: loadPage(limit: min(300, ringCapacity))
Persist-->>Store: Page(items, hasMoreBefore, startOffset)
Store-->>VM: items, hasMorePersistedItems, oldestLoadedPersistenceOffset
Note over UI: Row scrolls into view
UI->>VM: onAppear → loadOlderItems()
VM->>Store: loadOlderItems()
Store->>Store: guard !isLoadingOlderItems
Store->>Persist: loadPage(endingBefore: oldestOffset, limit: 300)
Persist-->>Store: Page(olderItems, hasMoreBefore, startOffset)
Store->>Store: deduplicate & items.insert(olderItems, at: 0)
Store->>Store: update oldestLoadedPersistenceOffset, hasMorePersistedItems
Store-->>VM: arm() triggers re-observation
VM-->>UI: updated isLoadingOlderItems / hasMorePersistedItems
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let existingIds = Set(items.map(\.id)) | ||
| let olderItems = page.items.filter { !existingIds.contains($0.id) } | ||
| if !olderItems.isEmpty { | ||
| items.insert(contentsOf: olderItems, at: 0) | ||
| } | ||
| self.oldestLoadedPersistenceOffset = page.startOffset ?? oldestLoadedPersistenceOffset | ||
| hasMorePersistedItems = page.hasMoreBefore | ||
| rebuildContextIndex() |
There was a problem hiding this comment.
loadOlderItems inserts directly into items at position 0, bypassing the private insert(_:) helper that enforces the ring-buffer eviction. A user with 2,288 persisted rows (the issue's exact scenario) loading all 8 pages at 300 items/page would accumulate ~2,700 items — 35% above ringCapacity = 2,000 — with no eviction until the next live ingest call. The live-event eviction removes oldest items from the front, which are the freshly-loaded history rows, creating counter-productive churn.
Consider capping total items.count after prepending, or document that ringCapacity is intentionally not enforced for on-demand history pages.
| @@ -66,26 +99,32 @@ public actor WorkstreamPersistence { | |||
| break | |||
| } | |||
| tail.insert(contentsOf: chunk, at: 0) | |||
| let newlineCount = tail.reduce(0) { $1 == 0x0A ? $0 + 1 : $0 } | |||
| if newlineCount > limit { | |||
| lineRanges = Self.lineRanges(in: tail, baseOffset: offset) | |||
| if lineRanges.count > limit { | |||
| break | |||
| } | |||
| } | |||
There was a problem hiding this comment.
lineRanges rescans the entire accumulated tail on every chunk iteration
Self.lineRanges(in: tail, baseOffset: offset) walks tail from index 0 on each loop iteration, but tail grows by prepending 64 KiB chunks each pass. For a file where the desired limit lines are spread across many chunks, this is O(k²) in the number of chunks read before lineRanges.count > limit fires. With default historyPageSize = 300 and large rows, this can consume significant CPU per loadPage call. An incremental approach — tracking which prefix has already been scanned and only computing new ranges for the prepended chunk — would keep the inner cost O(chunk) per iteration.
| .frame(maxWidth: .infinity) | ||
| .padding(.vertical, 10) | ||
| } | ||
| .buttonStyle(.plain) |
There was a problem hiding this comment.
.onAppear and button tap both fire the same action
.onAppear(perform: action) makes this an auto-loading sentinel (infinite-scroll style): loading begins as soon as the row scrolls into view without any user tap. The button labeled "Load older activity" is therefore never actually needed to initiate a load — it's redundant. If SwiftUI rebuilds the historyList while this row is in the viewport (e.g., after items are inserted at the top of items), .onAppear may fire again; the isLoadingOlderItems guard prevents a concurrent double-load but a back-to-back second load can still start immediately after the first completes.
If the intent is purely auto-load on scroll, consider removing the tap target entirely (or making the button a non-interactive progress indicator) to avoid misleading the user about the need to click it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Packages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamStore.swift`:
- Around line 93-123: loadOlderItems currently appends persisted pages to items
which can exceed ringCapacity and later be trimmed by insert(_:) when live
events arrive, making previously-paged history permanently inaccessible because
hasMorePersistedItems stays false; fix by pinning or accounting for paged rows
so they are not discarded by insert(_:), or by making insert(_:) respect a
pinned/paged prefix: update loadOlderItems to mark newly loaded items (e.g.,
pinnedIds or pinnedCount) and ensure insert(_:) will not trim those pinned items
(or if trimming occurs, set hasMorePersistedItems = true and adjust
oldestLoadedPersistenceOffset to reflect lost persisted rows), and keep
rebuildContextIndex() consistent after these adjustments (touch loadOlderItems,
insert(_:), items, ringCapacity, hasMorePersistedItems,
oldestLoadedPersistenceOffset, rebuildContextIndex).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1dd20bc-9d4f-41c5-a570-f53aeba5607a
📒 Files selected for processing (6)
Packages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamPersistence.swiftPackages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamStore.swiftPackages/CMUXWorkstream/Tests/CMUXWorkstreamTests/WorkstreamPersistenceTests.swiftPackages/CMUXWorkstream/Tests/CMUXWorkstreamTests/WorkstreamStoreTests.swiftResources/Localizable.xcstringsSources/Feed/FeedPanelView.swift
5848881 to
f807533
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Packages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamStore.swift`:
- Around line 97-103: The guard using try? on persistence.loadPage conflates
transient I/O/decoding errors with an empty-page EOF; replace the try? pattern
around persistence.loadPage(endingBefore: oldestLoadedPersistenceOffset, limit:
historyPageSize) with explicit do-catch so you only set hasMorePersistedItems =
false when the call succeeds and page.items.isEmpty, and on errors either
propagate or log and keep hasMorePersistedItems true to allow retries (handle
specific persistence errors if needed); ensure the surrounding logic that relies
on page is updated to use the successfully loaded page variable from the do
block.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83406eab-ae70-40ca-9830-6f037965d51d
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojPackages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamPersistence.swiftPackages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamStore.swiftResources/Localizable.xcstringsSources/Feed/FeedPanelView.swiftSources/Feed/FeedPanelViewModel.swiftSources/TerminalController.swift
✅ Files skipped from review due to trivial changes (2)
- Resources/Localizable.xcstrings
- Sources/Feed/FeedPanelView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Packages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamPersistence.swift
| guard let page = try? await persistence.loadPage( | ||
| endingBefore: oldestLoadedPersistenceOffset, | ||
| limit: historyPageSize | ||
| ), !page.items.isEmpty else { | ||
| hasMorePersistedItems = false | ||
| return | ||
| } |
There was a problem hiding this comment.
Don’t treat persistence read failures as end-of-history.
try? here conflates I/O/decode errors with EOF and immediately sets hasMorePersistedItems = false, which removes retry ability for transient failures.
Suggested fix
- guard let page = try? await persistence.loadPage(
- endingBefore: oldestLoadedPersistenceOffset,
- limit: historyPageSize
- ), !page.items.isEmpty else {
- hasMorePersistedItems = false
- return
- }
+ let page: WorkstreamPersistence.Page
+ do {
+ page = try await persistence.loadPage(
+ endingBefore: oldestLoadedPersistenceOffset,
+ limit: historyPageSize
+ )
+ } catch {
+ // Preserve pagination state so UI can retry.
+ return
+ }
+ guard !page.items.isEmpty else {
+ hasMorePersistedItems = false
+ return
+ }📝 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.
| guard let page = try? await persistence.loadPage( | |
| endingBefore: oldestLoadedPersistenceOffset, | |
| limit: historyPageSize | |
| ), !page.items.isEmpty else { | |
| hasMorePersistedItems = false | |
| return | |
| } | |
| let page: WorkstreamPersistence.Page | |
| do { | |
| page = try await persistence.loadPage( | |
| endingBefore: oldestLoadedPersistenceOffset, | |
| limit: historyPageSize | |
| ) | |
| } catch { | |
| // Preserve pagination state so UI can retry. | |
| return | |
| } | |
| guard !page.items.isEmpty else { | |
| hasMorePersistedItems = false | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Packages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamStore.swift` around
lines 97 - 103, The guard using try? on persistence.loadPage conflates transient
I/O/decoding errors with an empty-page EOF; replace the try? pattern around
persistence.loadPage(endingBefore: oldestLoadedPersistenceOffset, limit:
historyPageSize) with explicit do-catch so you only set hasMorePersistedItems =
false when the call succeeds and page.items.isEmpty, and on errors either
propagate or log and keep hasMorePersistedItems true to allow retries (handle
specific persistence errors if needed); ensure the surrounding logic that relies
on page is updated to use the successfully loaded page variable from the do
block.
This branch was successfully deployed
1 participant
Summary
Testing
./scripts/setup.shsucceeded./scripts/reload.sh --tag feedvirtsucceededIssues
Summary by cubic
Lazy-loads Activity feed history using JSONL byte-cursor paging to cut initial load and tab-switch time. Adds a localized “Load older activity” footer that auto-fetches previous pages with a spinner.
New Features
loadPage(endingBefore:limit:)with a stable byte cursor;loadRecentnow uses it.WorkstreamStoreloads a small recent slice at start, exposeshasMorePersistedItemsandisLoadingOlderItems, and addsloadOlderItems(); newWorkstreamDefaultInitialLoadLimitandWorkstreamDefaultHistoryPageSize.FeedHistoryLoadMoreRowthat auto-fetches on appear and after each page; localized “Load older activity” and “Loading older activity…” (en, ja).Refactors
TerminalControllersidebar helpers by removing main-thread sync wrappers.Written for commit f807533. Summary will update on new commits.
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Localization