crueber/walhub

Watch

Fork 0

Explore: order owner sections by most recently committed repository (owners list has no activity ordering — name proxy only) #283

New issue

Closed

opened 2026-09-10 12:13:50 +00:00 by crueber · 5 comments

crueber commented

2026-09-10 12:13:50 +00:00

Owner

Follow-up to #247 (repo rows on /explore are now ordered by most recent commit — done). The remaining gap: the owner sections themselves are still ordered by a name proxy, not by activity.

Current state (code evidence)

Each owner's repo list on /explore is correctly activity-ordered: Owners.jsx:31-33 fetches GET /api/v1/owners/{owner}/repos/detailed?sort=activity&order=desc and stabilizes client-side via orderByActivity (lib/owners.js:44). Done in #247.
The owner sections are not: Owners.jsx:110 orders owner names via newestFirst(owners()) — reverse-lexicographic on the name (lib/owners.js:27-30, whose own comment says "Owner sections stay name-proxied (newestFirst — the owners list carries no timestamps)"). A dead owner whose name sorts late appears above an active owner whose name sorts early.
GET /api/v1/owners (internal/api/discovery.go:115) returns sorted name strings only — no timestamps, no activity. newestFirst's doc comment calls it "the closest deterministic newest-first proxy available" and names itself the function to replace when the backend carries times.

What's needed

Order owner sections on /explore by their most recently committed repository — the owner whose repo had the newest last_commit_time first, owners with no activity last.

Proposed design

The data to compute this already exists server-side per owner (the repos/detailed rows carry last_commit_time), but scoring every owner on page load means N parallel detailed fetches before rendering — the page currently renders owner sections progressively and that's good behavior to keep. Two shapes:

Server-side rollup (recommended). Extend the owners listing (or the #247/#248 catalog — same aggregate object that already carries per-repo last_commit_time and size_bytes) with a per-owner last_commit_time rollup = max over the owner's repos. GET /api/v1/owners?sort=activity returns owners in that order (and the field per row). The catalog maintenance pass already walks every repo; a per-owner max is one comparison per repo in the same pass. Client change: replace newestFirst(owners()) with ordering on the returned field, falling back to name order when absent — orderByActivity is reusable nearly verbatim (it already defines the total order: known times desc, unknown last, deterministic name tiebreak).
Client-side lazy re-rank (fallback). Render owner sections as today (name-proxied), then re-rank sections as each repos:{owner} detailed doc arrives. Simple, no API change, but the visible order shifts as data lands — janky, and the first paint still lies. Only if the rollup is unwanted.

Prefer (1): /explore is the page whose whole point is "what's alive on this instance," and the catalog pass makes the rollup ~free.

Acceptance criteria

GET /api/v1/owners (all three twins) supports sort=activity&order=desc and returns last_commit_time per owner (max over the owner's repos; null when the owner has no commits).
/explore owner sections are ordered by that value, newest first; owners with no activity sort last (deterministic name tiebreak); unknown/missing values degrade to today's behavior without error.
newestFirst is retired from the explore page (deleted or repurposed with an updated comment — its own doc comment anticipates exactly this replacement).
The catalog/maintenance pass maintains the rollup incrementally (a new push updates its owner's rollup without a full rescan).
Headless test for the owner-ordering function covering: mixed known/unknown times, ties, single-owner, empty list.
All three route twins serve the new param/field.

Follow-up to #247 (repo rows on `/explore` are now ordered by most recent commit — done). The remaining gap: **the owner sections themselves** are still ordered by a name proxy, not by activity. ## Current state (code evidence) - Each owner's *repo list* on `/explore` is correctly activity-ordered: `Owners.jsx:31-33` fetches `GET /api/v1/owners/{owner}/repos/detailed?sort=activity&order=desc` and stabilizes client-side via `orderByActivity` (`lib/owners.js:44`). Done in #247. - The **owner sections** are not: `Owners.jsx:110` orders owner names via `newestFirst(owners())` — reverse-lexicographic on the name (`lib/owners.js:27-30`, whose own comment says "Owner sections stay name-proxied (newestFirst — the owners list carries no timestamps)"). A dead owner whose name sorts late appears above an active owner whose name sorts early. - `GET /api/v1/owners` (`internal/api/discovery.go:115`) returns sorted name strings only — no timestamps, no activity. `newestFirst`'s doc comment calls it "the closest deterministic newest-first proxy available" and names itself the function to replace when the backend carries times. ## What's needed Order owner sections on `/explore` by their **most recently committed repository** — the owner whose repo had the newest `last_commit_time` first, owners with no activity last. ## Proposed design The data to compute this already exists server-side per owner (the `repos/detailed` rows carry `last_commit_time`), but scoring every owner on page load means N parallel detailed fetches before rendering — the page currently renders owner sections progressively and that's good behavior to keep. Two shapes: 1. **Server-side rollup (recommended).** Extend the owners listing (or the #247/#248 catalog — same aggregate object that already carries per-repo `last_commit_time` and `size_bytes`) with a per-owner `last_commit_time` rollup = max over the owner's repos. `GET /api/v1/owners?sort=activity` returns owners in that order (and the field per row). The catalog maintenance pass already walks every repo; a per-owner max is one comparison per repo in the same pass. Client change: replace `newestFirst(owners())` with ordering on the returned field, falling back to name order when absent — `orderByActivity` is reusable nearly verbatim (it already defines the total order: known times desc, unknown last, deterministic name tiebreak). 2. **Client-side lazy re-rank (fallback).** Render owner sections as today (name-proxied), then re-rank sections as each `repos:{owner}` detailed doc arrives. Simple, no API change, but the visible order shifts as data lands — janky, and the first paint still lies. Only if the rollup is unwanted. Prefer (1): `/explore` is the page whose whole point is "what's alive on this instance," and the catalog pass makes the rollup ~free. ## Acceptance criteria - [ ] `GET /api/v1/owners` (all three twins) supports `sort=activity&order=desc` and returns `last_commit_time` per owner (max over the owner's repos; null when the owner has no commits). - [ ] `/explore` owner sections are ordered by that value, newest first; owners with no activity sort last (deterministic name tiebreak); unknown/missing values degrade to today's behavior without error. - [ ] `newestFirst` is retired from the explore page (deleted or repurposed with an updated comment — its own doc comment anticipates exactly this replacement). - [ ] The catalog/maintenance pass maintains the rollup incrementally (a new push updates its owner's rollup without a full rescan). - [ ] Headless test for the owner-ordering function covering: mixed known/unknown times, ties, single-owner, empty list. - [ ] All three route twins serve the new param/field.

crueber referenced this issue

2026-09-10 16:28:23 +00:00

Explore: show only the top 5 most active owners, plus instance owner/repo totals (currently 50 name-sorted sections, no totals) #295

crueber referenced this issue from a commit

2026-09-10 18:40:34 +00:00

Fix #283: explore owner sections ordered by most recent commit

crueber referenced this issue from a pull request that will close it,

2026-09-10 18:40:36 +00:00

Fix #283: explore ordered by recent commit #299

crueber commented

2026-09-10 18:40:38 +00:00

Author

Owner

Fixed by #299 (#299) — explore owner sections now order by most-recent-commit repo (client-side rank over the #247 detailed docs, zero extra GETs, unknowns last). No backend change; server-side rollup noted as a possible follow-up. Not merging — needs review.

Fixed by #299 (https://git.packden.us/crueber/walhub/pulls/299) — explore owner sections now order by most-recent-commit repo (client-side rank over the #247 detailed docs, zero extra GETs, unknowns last). No backend change; server-side rollup noted as a possible follow-up. Not merging — needs review.

crueber commented

2026-09-10 18:50:20 +00:00

Author

Owner

Review of PR #299 (fix/issue-283, client-side re-rank) — verdict at bottom.

SCOPE (load-bearing): the client-side approach does NOT satisfy #283 as written — blocked, with precise unblock below. What I verified:

Zero extra GETs: CONFIRMED. Owners.jsx OwnerSection reuses the existing repos:${owner} useData key (same key the /:owner page uses); the createEffect only reads that doc and reports ownerActivity upward. No new useData, no new endpoint/SDK call. web/src/pages/Owners.jsx:38-48.
Tie-breaks deterministic: CONFIRMED. orderOwnersByActivity: known desc, unknown last, name-ascending tiebreak (codepoint compare). web/src/lib/owners.js:107-127. Tests pin it.
Slice-after-rank: operation order CORRECT (orderOwnersByActivity then pageSlice, Owners.jsx:132-133), BUT ranking data is incomplete when total owners > MAX_OWNERS (50): only rendered sections mount and report activity, so an active owner at name-position 51+ never mounts, never reports, and can never rise into the top 50. Stable-wrong, not just first-paint flash. This is the structural gap the server rollup (rank over all owners before slice) exists to close.
First-paint flash: real (unknowns trail by name, sections shift once as docs land). Documented in code + doc — acceptable as a transient, but #283 explicitly called this shape 'janky... first paint still lies' and preferred the rollup.
All-unknown behavior change (old reverse-lexicographic proxy → name-ascending): real change, DOCUMENTED in code comment + 12_web_ui.md entry. Acceptable (old order was an admitted proxy), but it is a visible change worth a line in the PR description.
newestFirst legacy export: clean — page no longer imports it (Owners.jsx:27-34), no other importers (only unrelated same-name locals in thread-order.js/Issue.jsx), comment says 'New callers want orderOwnersByActivity'. #283 allowed 'deleted or repurposed with an updated comment' — repurpose chosen, fine.
No new deps: CONFIRMED (no package.json/lock diff; only solid-js signals already in use).
Doc entries: accurate except two nits I fixed and pushed (see below).

SMALL FIXES PUSHED to origin/fix/issue-283 (commit 'Review #299: doc stale-clause cleanup, most-active overflow copy', re-tested):

docs/go/12_web_ui.md: Order bullet kept the stale 'newestFirst stays for owner sections only' clause directly contradicted by the #283 parenthetical that followed it — collapsed to one clean sentence.
web/src/pages/Owners.jsx:144 + doc Caps line: overflow line said 'showing newest N of M owners' — wrong under activity ordering; now 'showing most active …'.

TESTS (scratch worktree /tmp/pr299, since removed): owners.test.js 16/16 incl. 5 new #283 cases; full node suite 576 pass / 0 fail (matches PR claim; the 1 'cancelled' entry is the pre-existing smoke.test.js pending-promise artifact, also present without this PR); vite build green (only the pre-existing >500kB chunk warning). Per instructions: no browser (node tests + reasoning; browser proof remains open), no docker/compose, no system packages. Main worktree untouched (still clean on main apart from untracked .opencode/).

MERGE RECOMMENDATION: blocked — client-side re-rank is correct for ≤50 owners but cannot satisfy #283's acceptance criteria (server sort=activity + per-owner last_commit_time on all three /api/v1/owners twins; catalog pass maintains the rollup incrementally; three-twins coverage). To unblock, either: (a) implement the rollup — extend the #247/#248 catalog aggregate with per-owner max(last_commit_time) in the same maintenance pass (one comparison per repo), serve sort=activity&order=desc + the field on all three twins (internal/api/discovery.go owners listing + twins), keep orderOwnersByActivity as the client fallback for missing values; or (b) maintainer amends #283's acceptance criteria to bless client-side as the fix and files the rollup as a follow-up (noting the >50-owner tail limitation). Happy to re-review either path.

Review of PR #299 (fix/issue-283, client-side re-rank) — verdict at bottom. SCOPE (load-bearing): the client-side approach does NOT satisfy #283 as written — blocked, with precise unblock below. What I verified: 1. Zero extra GETs: CONFIRMED. Owners.jsx OwnerSection reuses the existing `repos:${owner}` useData key (same key the /:owner page uses); the createEffect only reads that doc and reports ownerActivity upward. No new useData, no new endpoint/SDK call. web/src/pages/Owners.jsx:38-48. 2. Tie-breaks deterministic: CONFIRMED. orderOwnersByActivity: known desc, unknown last, name-ascending tiebreak (codepoint compare). web/src/lib/owners.js:107-127. Tests pin it. 3. Slice-after-rank: operation order CORRECT (orderOwnersByActivity then pageSlice, Owners.jsx:132-133), BUT ranking data is incomplete when total owners > MAX_OWNERS (50): only rendered sections mount and report activity, so an active owner at name-position 51+ never mounts, never reports, and can never rise into the top 50. Stable-wrong, not just first-paint flash. This is the structural gap the server rollup (rank over all owners before slice) exists to close. 4. First-paint flash: real (unknowns trail by name, sections shift once as docs land). Documented in code + doc — acceptable as a transient, but #283 explicitly called this shape 'janky... first paint still lies' and preferred the rollup. 5. All-unknown behavior change (old reverse-lexicographic proxy → name-ascending): real change, DOCUMENTED in code comment + 12_web_ui.md entry. Acceptable (old order was an admitted proxy), but it is a visible change worth a line in the PR description. 6. newestFirst legacy export: clean — page no longer imports it (Owners.jsx:27-34), no other importers (only unrelated same-name locals in thread-order.js/Issue.jsx), comment says 'New callers want orderOwnersByActivity'. #283 allowed 'deleted or repurposed with an updated comment' — repurpose chosen, fine. 7. No new deps: CONFIRMED (no package.json/lock diff; only solid-js signals already in use). 8. Doc entries: accurate except two nits I fixed and pushed (see below). SMALL FIXES PUSHED to origin/fix/issue-283 (commit 'Review #299: doc stale-clause cleanup, most-active overflow copy', re-tested): - docs/go/12_web_ui.md: Order bullet kept the stale '`newestFirst` stays for owner sections only' clause directly contradicted by the #283 parenthetical that followed it — collapsed to one clean sentence. - web/src/pages/Owners.jsx:144 + doc Caps line: overflow line said 'showing newest N of M owners' — wrong under activity ordering; now 'showing most active …'. TESTS (scratch worktree /tmp/pr299, since removed): owners.test.js 16/16 incl. 5 new #283 cases; full node suite 576 pass / 0 fail (matches PR claim; the 1 'cancelled' entry is the pre-existing smoke.test.js pending-promise artifact, also present without this PR); vite build green (only the pre-existing >500kB chunk warning). Per instructions: no browser (node tests + reasoning; browser proof remains open), no docker/compose, no system packages. Main worktree untouched (still clean on main apart from untracked .opencode/). MERGE RECOMMENDATION: blocked — client-side re-rank is correct for ≤50 owners but cannot satisfy #283's acceptance criteria (server sort=activity + per-owner last_commit_time on all three /api/v1/owners twins; catalog pass maintains the rollup incrementally; three-twins coverage). To unblock, either: (a) implement the rollup — extend the #247/#248 catalog aggregate with per-owner max(last_commit_time) in the same maintenance pass (one comparison per repo), serve sort=activity&order=desc + the field on all three twins (internal/api/discovery.go owners listing + twins), keep orderOwnersByActivity as the client fallback for missing values; or (b) maintainer amends #283's acceptance criteria to bless client-side as the fix and files the rollup as a follow-up (noting the >50-owner tail limitation). Happy to re-review either path.

crueber referenced this issue from a commit

2026-09-10 19:15:22 +00:00

Forgejo #283 follow-up: server-side owner activity rollup (07 §8, 12 §2.3.1, 14 amendment)

crueber commented

2026-09-10 19:15:31 +00:00

Author

Owner

Unblocks #299 review (option a — server-side rollup): pushed 96fe40f to origin/fix/issue-283 (PR #299, fast-forward, no force).

WHAT IT DOES

Derived per-owner max-commit rollup: sizecatalog.OwnerRollups folds max(LastCommitTime) per owner over the in-memory catalog at request time (one comparison per repo). NOT stored: no new bucket keys, no proto/codec/fixture change (law 5 untouched). Incremental structurally — the #247 per-repo activity was already incremental (blind push write + sweep heal), so a push moves its owner's max without a rescan (pinned by TestOwnerRollupHealsIncrementally).
GET /api/v1/owners?sort=activity&order=desc → the FROZEN []string, activity-ordered over ALL owners before the client's MAX_OWNERS slice (the >50 structural gap is closed; first paint ordered). Default (no query) byte-identical, zero added trips. Absent catalog → name order; corrupt → 503.
NEW GET /api/v1/owners/detailed (triple twins /api-v1 + /api-browser-v1 + /services/api, discovery endpoints[], SDK owners.listDetailed) → {owners:[{name, last_commit_sha|null, last_commit_time|null}]} with the same sort query (the #248 row-shape rule — the field could not ride the string list).
Client rank KEPT as fallback/enhancement (decided, documented): Owners.jsx fetches sort=activity&order=desc, orderOwnersByActivity still re-ranks as section docs land (covers missing/stale catalog values, same total order so they never disagree). newestFirst stays a legacy export.
Docs (law 12): Decisions entries in 07_api.md (§8), 12_web_ui.md (§2.3.1 + follow-up entry superseding the deliberately-out-of-scope tail), 14_extensibility.md (derived-rollup amendment).

TESTS

Go: new table-driven suites (rollup values, sort incl. ties/unknowns/bogus params, backfill; handler matrix incl. all three twins, discovery, degrade-without-catalog, corrupt→503, nil-store/registry). gofmt/vet clean; go test -race clean (internal/api, internal/sizecatalog). Coverage: api 95.3%, sizecatalog 97.7% (new code 96–100%).
JS: sdk-surface (list query + listDetailed shapes) + sdk-nostore (both no-store) pins; owners.test.js untouched (fallback pinned there). Touched files 32/32 green.
NOT run (noted, not hidden): full node suite — 8 unrelated files fail in this fresh worktree on missing web/node_modules (marked/solid-js, pre-existing env gap; my files pass standalone). No vite build (same gap). No browser (per instructions; remains open). No docker/compose, no system packages. internal/server asset tests fail here on empty web/dist (500 ui shell missing — run make build; same fresh-worktree gap, pre-existing); dist-independent server tests + cmd/walhub discovery tests pass. Main worktree untouched (still clean on main apart from untracked .opencode/).

DEVIATIONS/OBSERVATIONS

No brain change: derived instead of stored per-owner aggregate (rationale in the 07/14 entries).
Stale field lesson: AGENTS.md says web/dist/.keep is tracked — it is in NEITHER main nor this branch (deleted in a1134a6; .gitignore still carries the !web/dist/.keep exception). Fresh worktrees cannot go build ./... without a placeholder. Left my placeholder untracked (not committed); flagging in case you want a .keep restore as its own change.

Ready for re-review.

Unblocks #299 review (option a — server-side rollup): pushed 96fe40f to origin/fix/issue-283 (PR #299, fast-forward, no force). WHAT IT DOES - Derived per-owner max-commit rollup: sizecatalog.OwnerRollups folds max(LastCommitTime) per owner over the in-memory catalog at request time (one comparison per repo). NOT stored: no new bucket keys, no proto/codec/fixture change (law 5 untouched). Incremental structurally — the #247 per-repo activity was already incremental (blind push write + sweep heal), so a push moves its owner's max without a rescan (pinned by TestOwnerRollupHealsIncrementally). - GET /api/v1/owners?sort=activity&order=desc → the FROZEN []string, activity-ordered over ALL owners before the client's MAX_OWNERS slice (the >50 structural gap is closed; first paint ordered). Default (no query) byte-identical, zero added trips. Absent catalog → name order; corrupt → 503. - NEW GET /api/v1/owners/detailed (triple twins /api-v1 + /api-browser-v1 + /services/api, discovery endpoints[], SDK owners.listDetailed) → {owners:[{name, last_commit_sha|null, last_commit_time|null}]} with the same sort query (the #248 row-shape rule — the field could not ride the string list). - Client rank KEPT as fallback/enhancement (decided, documented): Owners.jsx fetches sort=activity&order=desc, orderOwnersByActivity still re-ranks as section docs land (covers missing/stale catalog values, same total order so they never disagree). newestFirst stays a legacy export. - Docs (law 12): Decisions entries in 07_api.md (§8), 12_web_ui.md (§2.3.1 + follow-up entry superseding the deliberately-out-of-scope tail), 14_extensibility.md (derived-rollup amendment). TESTS - Go: new table-driven suites (rollup values, sort incl. ties/unknowns/bogus params, backfill; handler matrix incl. all three twins, discovery, degrade-without-catalog, corrupt→503, nil-store/registry). gofmt/vet clean; go test -race clean (internal/api, internal/sizecatalog). Coverage: api 95.3%, sizecatalog 97.7% (new code 96–100%). - JS: sdk-surface (list query + listDetailed shapes) + sdk-nostore (both no-store) pins; owners.test.js untouched (fallback pinned there). Touched files 32/32 green. - NOT run (noted, not hidden): full node suite — 8 unrelated files fail in this fresh worktree on missing web/node_modules (marked/solid-js, pre-existing env gap; my files pass standalone). No vite build (same gap). No browser (per instructions; remains open). No docker/compose, no system packages. internal/server asset tests fail here on empty web/dist (500 ui shell missing — run make build; same fresh-worktree gap, pre-existing); dist-independent server tests + cmd/walhub discovery tests pass. Main worktree untouched (still clean on main apart from untracked .opencode/). DEVIATIONS/OBSERVATIONS - No brain change: derived instead of stored per-owner aggregate (rationale in the 07/14 entries). - Stale field lesson: AGENTS.md says web/dist/.keep is tracked — it is in NEITHER main nor this branch (deleted in a1134a6; .gitignore still carries the !web/dist/.keep exception). Fresh worktrees cannot go build ./... without a placeholder. Left my placeholder untracked (not committed); flagging in case you want a .keep restore as its own change. Ready for re-review.

crueber commented

2026-09-10 19:20:45 +00:00

Author

Owner

Re-review of PR #299 delta (96fe40f server-side rollup + my fix e395d38 below). Scope first: full PR file list == union of the 4 commits on the branch, nothing unexpected. Prior review's passes (fallback semantics, newestFirst legacy, zero GETs) not re-litigated.

FINDINGS (file:line + resolution):

Derived-not-stored rollup, zero new bucket keys — CONFIRMED. git diff main..branch -- internal/store/ is empty (no proto/codec/testdata touched). internal/sizecatalog/owners_activity.go:43-64 is a pure fold (one comparison per repo, in-memory); splitID (sizecatalog.go:624) correctly skips malformed ids (pinned).
sort=activity on owners listing — CONFIRMED. Default (no query) byte-identical: catalog read only under sort=activity (internal/api/discovery.go:137), zero added trips. Unknowns last in BOTH directions, ties name-ascending (owners_activity.go:87-99). Bogus sort/order degrade, never 400 (owners_activity.go:50-60, incl. case-insensitive — pinned). Corrupt catalog -> 503 via mapViewErr default (env.go:701-710, pinned incl. default-still-200); absent catalog / nil store -> name order + null rows, never 404/500 (pinned); nil registry -> 503 both surfaces (pinned).
/detailed rows + twins + discovery + SDK — SANE. Explicit nulls, no omitempty (owners_activity.go:35-44); [] never null (pinned asserts). Triple twins wired (routes.go:46-51, shared handlers incl. query on all three); discovery derived from table (pinned). SDK owners.list(query)/listDetailed(query) both no-store per #200 (core.js:259-306, pinned in sdk-surface + sdk-nostore-304).
Client/server coexistence — BUG FOUND AND FIXED. orderOwnersByActivity over an empty activityMap resorts to name order, discarding the server ranking BEFORE the MAX_OWNERS slice: demonstrated server [bob,alice,amy,zed] -> first paint [alice,amy,bob,zed], cap-2 slice [alice,amy] vs server top-2 [bob,alice]. The >50 structural gap (the original blocker) was NOT closed. Fix pushed as e395d38: hasKnownActivity gate (owners.js + Owners.jsx:139-147) — server order passes through until a real time lands, same total order after, so they never fight. Pinned by 2 new headless tests.
First paint past caps — NOT resolved before fix, RESOLVED after (sim + test: first-paint slice keeps server top-N). Residual transient (pending sections trail until reports land; stale catalog heals via fresher section times) is bounded and documented in code.
Catalog-absent fallback — CONFIRMED name order + null rows both surfaces; agrees with client all-unknown order.
Coverage — api 95.3% (gate holds), sizecatalog 97.7%; new code 96-100% (OwnerRollups/SortOwners/owners/ownerSortParams/ownerRollups 100%, ownersDetailed 96%).
Deps — go.mod/sum + package.json/lock diffs all empty (law 1 clean).
Docs — 07 §8, 12 §2.3.1 follow-up, 14 amendment accurate; note: the 12 'trade-off retired / first paint ordered' claim was FALSE before e395d38 and is TRUE after (no doc edit needed). 14 has a whitespace-only reflow of adjacent mirror lines — cosmetic, harmless.

TESTS (scratch worktree /tmp/pr299b, to be removed): go test -race clean (internal/api, internal/sizecatalog); gofmt/vet clean; JS 34/34 (owners 18 incl. 2 new, sdk-surface, sdk-nostore-304). Per instructions: no browser (remains open, noted), no docker/compose, no system packages. Main worktree untouched (clean on main apart from untracked .opencode/).

MERGE RECOMMENDATION: ready to merge (after CI).

Re-review of PR #299 delta (96fe40f server-side rollup + my fix e395d38 below). Scope first: full PR file list == union of the 4 commits on the branch, nothing unexpected. Prior review's passes (fallback semantics, newestFirst legacy, zero GETs) not re-litigated. FINDINGS (file:line + resolution): 1. Derived-not-stored rollup, zero new bucket keys — CONFIRMED. git diff main..branch -- internal/store/ is empty (no proto/codec/testdata touched). internal/sizecatalog/owners_activity.go:43-64 is a pure fold (one comparison per repo, in-memory); splitID (sizecatalog.go:624) correctly skips malformed ids (pinned). 2. sort=activity on owners listing — CONFIRMED. Default (no query) byte-identical: catalog read only under sort=activity (internal/api/discovery.go:137), zero added trips. Unknowns last in BOTH directions, ties name-ascending (owners_activity.go:87-99). Bogus sort/order degrade, never 400 (owners_activity.go:50-60, incl. case-insensitive — pinned). Corrupt catalog -> 503 via mapViewErr default (env.go:701-710, pinned incl. default-still-200); absent catalog / nil store -> name order + null rows, never 404/500 (pinned); nil registry -> 503 both surfaces (pinned). 3. /detailed rows + twins + discovery + SDK — SANE. Explicit nulls, no omitempty (owners_activity.go:35-44); [] never null (pinned asserts). Triple twins wired (routes.go:46-51, shared handlers incl. query on all three); discovery derived from table (pinned). SDK owners.list(query)/listDetailed(query) both no-store per #200 (core.js:259-306, pinned in sdk-surface + sdk-nostore-304). 4. Client/server coexistence — BUG FOUND AND FIXED. orderOwnersByActivity over an empty activityMap resorts to name order, discarding the server ranking BEFORE the MAX_OWNERS slice: demonstrated server [bob,alice,amy,zed] -> first paint [alice,amy,bob,zed], cap-2 slice [alice,amy] vs server top-2 [bob,alice]. The >50 structural gap (the original blocker) was NOT closed. Fix pushed as e395d38: hasKnownActivity gate (owners.js + Owners.jsx:139-147) — server order passes through until a real time lands, same total order after, so they never fight. Pinned by 2 new headless tests. 5. First paint past caps — NOT resolved before fix, RESOLVED after (sim + test: first-paint slice keeps server top-N). Residual transient (pending sections trail until reports land; stale catalog heals via fresher section times) is bounded and documented in code. 6. Catalog-absent fallback — CONFIRMED name order + null rows both surfaces; agrees with client all-unknown order. 7. Coverage — api 95.3% (gate holds), sizecatalog 97.7%; new code 96-100% (OwnerRollups/SortOwners/owners/ownerSortParams/ownerRollups 100%, ownersDetailed 96%). 8. Deps — go.mod/sum + package.json/lock diffs all empty (law 1 clean). 9. Docs — 07 §8, 12 §2.3.1 follow-up, 14 amendment accurate; note: the 12 'trade-off retired / first paint ordered' claim was FALSE before e395d38 and is TRUE after (no doc edit needed). 14 has a whitespace-only reflow of adjacent mirror lines — cosmetic, harmless. TESTS (scratch worktree /tmp/pr299b, to be removed): go test -race clean (internal/api, internal/sizecatalog); gofmt/vet clean; JS 34/34 (owners 18 incl. 2 new, sdk-surface, sdk-nostore-304). Per instructions: no browser (remains open, noted), no docker/compose, no system packages. Main worktree untouched (clean on main apart from untracked .opencode/). MERGE RECOMMENDATION: ready to merge (after CI).

crueber closed this issue

2026-09-10 19:21:14 +00:00

crueber referenced this issue from a commit

2026-09-10 19:21:15 +00:00

Merge pull request 'Fix #283: explore ordered by recent commit' (#299) from fix/issue-283 into main

crueber commented

2026-09-10 19:21:15 +00:00

Author

Owner

Fixed by PR #299 incl. server rollup rework + review server-order fix (sort=activity, derived rollup, coexistence correct; gates green), merged. Closing.

crueber referenced this issue from a commit

2026-09-10 20:41:09 +00:00

Fix #295: explore top-5 most-active owners + instance owner total

crueber referenced this issue

2026-09-10 20:41:11 +00:00

Fix #295: explore top-5 active + totals #306

crueber referenced this issue

2026-09-10 20:41:16 +00:00

Explore: show only the top 5 most active owners, plus instance owner/repo totals (currently 50 name-sorted sections, no totals) #295

crueber referenced this issue

2026-09-10 20:52:42 +00:00

Explore instance repo total needs a backend rail (follow-up to #295) #307