feat: deterministic plugin cover image via README order#202
Open
marekh19 wants to merge 2 commits into
Open
Conversation
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
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.
👋 First off — I'm a big fan of dotfyle and really appreciate everything you've built for the Neovim community. This one started as a small personal itch: my colorscheme's thumbnail on dotfyle was showing the wrong image, and digging into why turned into this PR.
What
Plugin cover image is random when a README has multiple images. Media is ordered only by
orderBy: { thumbnail: 'desc' }— with no thumbnail set every row ties, so Postgres returns them in arbitrary heap order and the cover (and og:image) is whatever lands at index 0.Adds a
positioncolumn set to each image's index in the parser's output during sync, and orders by it. Cover now defaults to the first image the parser emits (see caveat below). The manualthumbnailflag still wins.Fixes #186. Also addresses the ordering half of #136.
Why
How
Media.position Int?— nullable, additive migration.syncMediawrites the parser index, deduped by URL keeping the first occurrence.mediaCoverOrderBy=[{ thumbnail: desc }, { position: asc, nulls: last }, { id: asc }], used by the plugin page, RSS feed, and the media endpoint.syncMediaupserts were wrapped in an extra array (Promise.all([arr.map(...)])) and never awaited — fixed.Ordering caveat — group order, not strict document order
positionfollows the orderfindMediaUrlsemits URLs, which groups by source type (absolutegithubusercontent>/assetsuploads > relative markdown > relative HTML) and is document-ordered only within a group. So the cover is the first image of the highest-priority group present — e.g. a markdown screenshot can outrank an<img>banner placed above it.Kept as-is on purpose. This preserves the parser's existing emit order, which predates test coverage and has no documented rationale (matcher flagged for refactor in #97). The real win here is determinism — same cover every sync, authors can influence it by ordering their README — not literal document order. True document order is a small follow-up (sort parser matches on their string index); happy to add it if you'd prefer that behavior.
Self-heals existing plugins, no stale-media delete
Rows dropped from a README but never re-synced keep
position = nulland sort last, so the correct cover surfaces on the next weekly sync without deleting anything. This deliberately avoids thedeleteManypath from #140 that crashed the seeder. Stale-media cleanup (#128) stays a separate issue.Not included
parser.ts; this PR doesn't touch it.Test
sync.spec.ts— drives the realsyncMedia+ real parser against a multi-image README, stubbing onlyfetchand the DBupsert(no DB, no network, matches the existingvi.mocktest style). Asserts: first README image → position 0, repeats collapse to one row keeping first position, relative paths resolve to raw URLs, and thethumbnailflag is never written so a manual cover choice survives re-sync.parser.spec.ts— adds a case pinning the group-priority emit order (an absolute URL is emitted before a relative one that appears earlier in the README), so the caveat above is locked in and a future rework has to update it consciously.