Automattic / Automattic/newspack-rolling-coverage

Pin ordering left active on "load more", breaking the date-cursor contract

Open
#20 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
PHP
Stars
1
Forks
1
Avg merge
2d 18h
Merged PRs (30d)
3

Description

The forward/polling branch correctly opts out of pinned-first ordering ($args[ Post_Type::SKIP_PIN_ORDER_VAR ] = true; at line 694), but this backward-pagination branch does not, and neither does render_block() at line 169.

That matters because both paths derive the next cursor from the last element of the results array, on the assumption that it is the page's oldest row — $oldest_iso at line 198 and $next_before at line 766. Pinned-first ordering breaks that assumption: pinned rows are hoisted to the front regardless of date, so the last element stops being the oldest row as soon as pinning reorders the page.

Two things follow. A pinned entry older than the page-1 cutoff is rendered at the top of page 1 and then again on whichever later page its date falls in — and view.ts:325 appends with insertAdjacentHTML( 'beforeend', … ) with no de-duplication by entry ID, so the reader genuinely sees it twice. And when pins fill a page, the cursor is taken from a pinned row whose date may be far older than rows already shown, so the next request skips everything in between.

Setting the skip var on this query and on the initial render, and deriving the cursor from the minimum post_date across the returned set rather than from the last array element, addresses both.

### Consider

Concrete threshold for this, from the caller side. While the number of pinned entries at or below the cursor stays under the page size, the last array element is still the oldest unpinned row and nothing is skipped — the damage is limited to pinned entries re-rendering on every page. The moment that count reaches the page size, the page fills entirely with pinned rows, $next_before is taken from the last pinned entry in pin order, and every unpinned entry published after that timestamp is dropped from the reader's view for good. entriesPerPage is settable from 1 to 100 in the block sidebar, so at a page size of 1 a single pin triggers it. Two changes together close it: set SKIP_PIN_ORDER_VAR on this query the way the polling branch does at line 694, and derive the cursor from the minimum post_date across the returned set rather than from the last array element — in render_block() at line 198 as well as here.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with the backward-pagination branch around lines 694 and 766, compare its query with the forward/polling branch, then inspect render_block() around lines 169 and 198 and view.ts:325. Set the skip-pin-order variable on both relevant queries and derive each cursor from the minimum post_date in the returned set; verify pinned entries are not duplicated or skipped during load more.

Written by the indexing model from the issue text.

Assessment

Tech stack
php, typescript
Domain
backend, frontend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.