Skip to content

fix: clear evicted prepend anchors#479

Closed
agent-p1p wants to merge 1 commit into
masterfrom
pip/whitenoise-mac-467
Closed

fix: clear evicted prepend anchors#479
agent-p1p wants to merge 1 commit into
masterfrom
pip/whitenoise-mac-467

Conversation

@agent-p1p

@agent-p1p agent-p1p commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #467

Summary

  • add a prepend-anchor scroll decision matching the existing append-anchor handling
  • clear an owned prepend anchor when it is evicted, including the race between scheduling and executing the deferred restore
  • preserve newer pending requests by checking ownership before clearing deferred state
  • add regression coverage for contained, evicted, and absent prepend anchors

Verification

  • git diff --check — passed
  • changed-line 120-column scan — passed
  • bash -n scripts/ci/macos-sanity-checks.sh — passed
  • scripts/ci/macos-sanity-checks.sh — unavailable on Linux (xcodebuild: command not found)
  • Xcode build, Swift format lint, and unit tests — deferred to macOS CI; Apple tooling is not installed on this host

Sensitive paths

None. UI scroll/pagination state only.


Open in Stage

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@agent-p1p, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bd71d6d6-6637-4b9b-9121-2fe16a6d24df

📥 Commits

Reviewing files that changed from the base of the PR and between 0454dbf and 54844e5.

📒 Files selected for processing (2)
  • whitenoise-mac/Views/MessengerShellView.swift
  • whitenoise-macTests/whitenoise_macTests.swift
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pip/whitenoise-mac-467

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@stage-review

stage-review Bot commented Jul 12, 2026

Copy link
Copy Markdown

Ready to review this PR? Stage has broken it down into 3 individual chapters for you:

Title
1 Define scroll decision logic for oldest messages
2 Implement prepend anchor restoration and cleanup
3 Verify prepend anchor eviction and restoration logic
Open in Stage

Chapters generated by Stage for commit 54844e5 on Jul 12, 2026 3:43am UTC.

@agent-p1p

Copy link
Copy Markdown
Contributor Author

Pip adversarial review

Verdict: changes required.

Blocking (1)

  • whitenoise-mac/Views/MessengerShellView.swift:472 — clearing an absent anchor before the backward page finishes loses the very scroll restoration this state exists to preserve.

    The anchor can be absent only temporarily. A live append mutates the subscription's capped window and can evict the old first row; the in-flight paginateBackwards then extends that current authoritative window toward older history and can reintroduce the anchor. The repository's runtime model documents and implements that sequence in whitenoise-macTests.swift:17822-17903: a live anchored update slides lo/hi, and paginateBackwards subsequently decreases lo, producing a page that contains the former anchor again.

    This PR clears pendingPrependAnchorId immediately on the intermediate messageIDs.first change. When the successful backward page lands, the anchor is present again but no longer pending, so .onChange(of: messageIDs.first) does not restore it. The transcript can jump during ordinary successful pagination. The new helper test only checks isolated membership snapshots and therefore codifies the premature clear without exercising the lifecycle.

    Keep ownership while selectedTimelinePaging.isLoadingBefore is true. The post-await fallback at lines 693-700 is the correct place to clear an anchor that is still absent after the load settles. For the deferred-restore race, clear an absent anchor only once the older load is no longer in flight (or model that distinction explicitly). Add regression coverage for: start with anchor present → live append temporarily evicts it while loading → backward page reintroduces it → restoration remains pending.

Suggestions

None beyond the blocking lifecycle test above.

Nitpicks

None.

Checks

  • git diff --check origin/master...HEAD — pass
  • Static Analysis — pass
  • Swift format lint / macOS sanity steps — pass in PR Checks
  • Unit tests / release smoke — still running at review time
  • Local Xcode tests unavailable on this Linux worker

No sensitive paths touched. UI pagination/scroll state only.

@agent-p1p

Copy link
Copy Markdown
Contributor Author

CI update: Mac CI / PR Checks completed failure in run 29178646552. The failures are the deterministic messageMediaDiskCacheReadDeletionMaintainsTrackedFootprint() assertions plus the intermittent workspaceCoalescesAndCachesSourceEpochZeroMediaReferenceResolution() race already tracked as the red-master problem in #457; this diff does not touch those paths.

This adds no new code-review finding, but Pip’s ever-red rule makes PR #479 permanently ineligible to merge. The address phase must use a replacement PR after the baseline is fixed/green.

@agent-p1p

Copy link
Copy Markdown
Contributor Author

Retiring this PR under Pip's ever-red rule. The blocking prepend-anchor lifecycle finding is addressed in signed commit e405875: missing anchors remain pending while older pagination is in flight, then clear only after the load settles; lifecycle regression coverage was added. The corrected change will be resubmitted in a replacement PR after green baseline PR #464 lands on master.

@agent-p1p agent-p1p closed this Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant