Skip to content

fix(marko-virtual): keep latest pinned through prepend race - #1273

Open
ousamabenyounes wants to merge 1 commit into
TanStack:mainfrom
ousamabenyounes:fix/issue-1267
Open

fix(marko-virtual): keep latest pinned through prepend race#1273
ousamabenyounes wants to merge 1 commit into
TanStack:mainfrom
ousamabenyounes:fix/issue-1267

Conversation

@ousamabenyounes

@ousamabenyounes ousamabenyounes commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

🎯 Changes

  • Re-enable the chat-pretext Latest/prepend race coverage with four deterministic attempts.
  • Keep the example's pending Latest pin alive through the history-load commit using bounded animation-frame retries and cleanup on destroy.

Fixes #1267.

✅ Checklist

  • I have read the contributing guidelines
  • I have added tests for this change
  • I have run the full test suite locally
  • Changeset not required: this only changes an example and its e2e coverage, with no package release surface

📦 Release Impact

None. Example/e2e-only change.

Test verification (RED → GREEN)

  • RED: CI=1 pnpm --dir packages/marko-virtual/e2e/app exec playwright test e2e/chat-pretext.spec.ts --grep "Latest returns" failed 4/4 before the fix with distanceFromEnd = 870.
  • Revert proof: the final test still failed 4/4 after reverting only the example fix, captured in issue_1267_final_revert_red.log.
  • GREEN: the same focused command passed 4/4 after restoring the fix, captured in issue_1267_final_green_after_revert.log.
  • Focused file: CI=1 pnpm --dir packages/marko-virtual/e2e/app exec playwright test e2e/chat-pretext.spec.ts passed 12/12.

Full local suite

  • Baseline main: RUN_CI_LOG_DIR=.../baseline-ci ./run-ci.sh passed on 789f5c2c.
  • Final branch: RUN_CI_LOG_DIR=.../final-ci-after-review ./run-ci.sh passed; Nx reported all configured targets passing for 69 projects, including 85 Marko e2e tests.

Summary by CodeRabbit

  • Bug Fixes

    • Improved the “Latest” chat behavior during in-flight history loading, keeping the view reliably pinned to the newest messages.
    • Added bounded retry handling to ensure the chat reaches and remains at the latest position.
  • Tests

    • Added coverage for repeated race-condition scenarios involving history loading and returning to the latest messages.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 00bc8849-b498-4a81-aa2c-8f9203081ecf

📥 Commits

Reviewing files that changed from the base of the PR and between 789f5c2 and a291976.

📒 Files selected for processing (2)
  • examples/marko/chat-pretext/src/routes/+page.marko
  • packages/marko-virtual/e2e/app/e2e/chat-pretext.spec.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The chat example now uses bounded animation-frame retries to keep the viewport pinned during late history prepends. The skipped race test is replaced with four active parameterized tests that verify the bottom position and At latest status.

Changes

Chat latest-position handling

Layer / File(s) Summary
Bounded pin retry flow
examples/marko/chat-pretext/src/routes/+page.marko
The example tracks retry frames, settled frames, and retry attempts. It retries scrollToEnd() until the end holds for two frames or 60 attempts. The Latest action resets counters, and cleanup cancels pending frames.
In-flight prepend race coverage
packages/marko-virtual/e2e/app/e2e/chat-pretext.spec.ts
Four parameterized tests replace the skipped race test. They detect content growth after the prepend and verify the bottom position, At latest status, and newest message visibility.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to a2919

This change keeps the chat viewport pinned to the latest messages when history prepends complete after Latest is selected, with active race coverage confirming the expected bottom position and status. It is ready to merge.

Sequence Diagram(s)

sequenceDiagram
  participant LatestButton
  participant PinRetry
  participant Messages
  participant HistoryLoader
  LatestButton->>PinRetry: reset counters and call scrollToEnd()
  HistoryLoader->>Messages: prepend history rows
  PinRetry->>Messages: retry scrollToEnd() on animation frames
  Messages-->>PinRetry: report settled end position
  PinRetry-->>LatestButton: restore At latest state
Loading

Suggested reviewers: piecyk

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the Marko virtualizer fix for keeping the Latest position pinned during a prepend race.
Description check ✅ Passed The description includes the change summary, checklist, release impact, issue reference, focused test results, and full-suite verification. It is complete and relevant to the repository template.
Linked Issues check ✅ Passed The changes address issue [#1267] by preserving the pending Latest pin through late history prepends, adding bounded retry and cleanup logic, and re-enabling deterministic race coverage.
Out of Scope Changes check ✅ Passed The changes are limited to the chat-pretext example and its related end-to-end tests. They directly support issue [#1267] and introduce no unrelated scope.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

marko-virtual: scrollToEnd during an in-flight prepend strands the view one prepend above the bottom (chat-pretext e2e fails on CI)

1 participant