feat(home): Discover rail — scale-safe random-id-anchor seek (bookshelf-0zpr.2) #320

Merged

zombor merged 1 commit from bd-bookshelf-0zpr.2 into main 2026-06-03 18:44:02 +00:00

Conversation 2 Commits 1 Files changed 5 +702 -10

zombor commented 2026-06-03 17:25:06 +00:00

Owner

Copy link

Summary

• Adds a Discover rail to the home dashboard surfacing random books from the library
• Scale-safe: uses a date-seeded random anchor in [min_id, max_id] for a PK index seek (WHERE id >= anchor ORDER BY id LIMIT 12) — no ORDER BY RAND()
• Anchor is derived from UTC day number so the rail feels fresh each day but is stable within a day
• Handles wrap-around: when anchor lands near max_id (fewer than 12 rows), does a second seek from min_id and deduplicates
• Rail is omitted when the library is empty or the query returns no results
• Wired via ListDiscover(queryRow, query, listFormats) following the curried-fn pattern; added as 6th arg to Get()

Test plan

• DiscoverSQL whitebox: no ORDER BY RAND(), has id >= ?, has LIMIT ?, filters deleted = 0
• DiscoverIDRangeSQL whitebox: uses MIN/MAX, filters deleted = 0
• DiscoverDateSeed: same value same day, different value next day
• ListDiscover: empty library (NULL min/max), normal fetch (12 books), wrap-around dedup, ErrNoRows, ID range error, query error, scan error, rows.Err, wrap-around query error, enrichAuthors error, formats error, authors populated
• Get integration: Discover rail present when non-empty, omitted when empty, error propagated
• All existing tests updated for the new 6th listDiscover arg (default stub returns empty)
• make test green
• make coverage 100%

Closes bead bookshelf-0zpr.2 on merge.

## Summary - Adds a **Discover** rail to the home dashboard surfacing random books from the library - Scale-safe: uses a date-seeded random anchor in [min_id, max_id] for a PK index seek (`WHERE id >= anchor ORDER BY id LIMIT 12`) — **no `ORDER BY RAND()`** - Anchor is derived from UTC day number so the rail feels fresh each day but is stable within a day - Handles wrap-around: when anchor lands near max_id (fewer than 12 rows), does a second seek from min_id and deduplicates - Rail is omitted when the library is empty or the query returns no results - Wired via `ListDiscover(queryRow, query, listFormats)` following the curried-fn pattern; added as 6th arg to `Get()` ## Test plan - [x] `DiscoverSQL` whitebox: no `ORDER BY RAND()`, has `id >= ?`, has `LIMIT ?`, filters `deleted = 0` - [x] `DiscoverIDRangeSQL` whitebox: uses `MIN/MAX`, filters `deleted = 0` - [x] `DiscoverDateSeed`: same value same day, different value next day - [x] `ListDiscover`: empty library (NULL min/max), normal fetch (12 books), wrap-around dedup, ErrNoRows, ID range error, query error, scan error, rows.Err, wrap-around query error, enrichAuthors error, formats error, authors populated - [x] `Get` integration: Discover rail present when non-empty, omitted when empty, error propagated - [x] All existing tests updated for the new 6th `listDiscover` arg (default stub returns empty) - [x] `make test` green - [x] `make coverage` 100% Closes bead bookshelf-0zpr.2 on merge.

zombor added 1 commit 2026-06-03 17:25:06 +00:00

feat(home): add Discover rail with scale-safe random-id-anchor seek ... 0f0885ae47

Adds a 'Discover' rail to the home dashboard that surfaces random books
from the library using a PK-seek strategy: derive a date-seeded anchor in
[min_id, max_id] for non-deleted books, then SELECT WHERE id >= anchor
ORDER BY id LIMIT 12. No ORDER BY RAND(). Wraps around from min_id when
anchor is near max_id. Rail is omitted for an empty library.

Closes bead bookshelf-0zpr.2 on merge.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-06-03 17:34:37 +00:00

Author

Owner

Copy link

SECURITY REVIEW — bookshelf-0zpr.2 (PR #320)

Scope: SQL injection, data exposure, unbounded queries, anchor arithmetic overflow.

---

1. SQL Injection — CLEAN

Both discoverSQL and discoverIDRangeSQL use only ? bind parameters. The computed anchor (int64) and limit (int) are passed to query(ctx, discoverSQL, anchor, limit) — no string interpolation anywhere in the call path. No injection surface.

2. Soft-delete filter — CLEAN

discoverSQL: WHERE b.id >= ? AND b.deleted = 0 — soft-deleted books are excluded from results.
discoverIDRangeSQL: WHERE deleted = 0 — the range query also excludes deleted books, so the anchor is always derived from the live book population. No deleted books leak through.

3. Library-wide scope (not per-user) — INTENTIONAL, NO SENSITIVE DATA

The Discover rail is explicitly library-wide (no user_id filter). This is a "random sample" discovery surface, analogous to a bookstore browsing table, so the design intent is correct. The query selects only: b.id, bm.title (via COALESCE), b.book_cover_hash, and a boolean has_cover flag — no user data, no reading progress, no PII, no private metadata is exposed. Nothing sensitive leaks from the absence of a user filter.

4. LIMIT present — CLEAN

discoverSQL has LIMIT ? with the bound limit parameter (set to discoverLimit = 12). The wrap-around path passes need = limit - len(result) as its limit argument. Both paths are bounded.

5. Anchor arithmetic overflow — CLEAN

• span = maxID.Int64 - minID.Int64 + 1: both are sql.NullInt64 (64-bit signed). With auto-increment PKs the practical max gap is well under int64 range. A library of 2^63 books is not a plausible scenario.
• rng.Int63n(span): rand.Int63n requires span > 0. The only way span == 0 would be if max == min - 1 (impossible since min <= max from MIN/MAX semantics) — so this is safe. If min == max, span == 1, Int63n(1) returns 0, and anchor = minID, which is correct.
• anchor = minID.Int64 + rng.Int63n(span): result is in [minID, maxID] inclusive — always a valid PK seek value. No overflow risk with realistic book counts.

6. Wrap-around: minID seek as fallback — CLEAN

The second seek uses minID.Int64 as anchor with need as limit (both validated). The dedup set correctly prevents returning the same book twice. No privacy concern since all results come from the same non-deleted, non-sensitive column set.

---

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## SECURITY REVIEW — bookshelf-0zpr.2 (PR #320) **Scope:** SQL injection, data exposure, unbounded queries, anchor arithmetic overflow. --- **1. SQL Injection — CLEAN** Both `discoverSQL` and `discoverIDRangeSQL` use only `?` bind parameters. The computed `anchor` (`int64`) and `limit` (`int`) are passed to `query(ctx, discoverSQL, anchor, limit)` — no string interpolation anywhere in the call path. No injection surface. **2. Soft-delete filter — CLEAN** `discoverSQL`: `WHERE b.id >= ? AND b.deleted = 0` — soft-deleted books are excluded from results. `discoverIDRangeSQL`: `WHERE deleted = 0` — the range query also excludes deleted books, so the anchor is always derived from the live book population. No deleted books leak through. **3. Library-wide scope (not per-user) — INTENTIONAL, NO SENSITIVE DATA** The Discover rail is explicitly library-wide (no `user_id` filter). This is a "random sample" discovery surface, analogous to a bookstore browsing table, so the design intent is correct. The query selects only: `b.id`, `bm.title` (via `COALESCE`), `b.book_cover_hash`, and a boolean `has_cover` flag — no user data, no reading progress, no PII, no private metadata is exposed. Nothing sensitive leaks from the absence of a user filter. **4. LIMIT present — CLEAN** `discoverSQL` has `LIMIT ?` with the bound `limit` parameter (set to `discoverLimit = 12`). The wrap-around path passes `need = limit - len(result)` as its limit argument. Both paths are bounded. **5. Anchor arithmetic overflow — CLEAN** - `span = maxID.Int64 - minID.Int64 + 1`: both are `sql.NullInt64` (64-bit signed). With auto-increment PKs the practical max gap is well under `int64` range. A library of 2^63 books is not a plausible scenario. - `rng.Int63n(span)`: `rand.Int63n` requires `span > 0`. The only way `span == 0` would be if `max == min - 1` (impossible since `min <= max` from MIN/MAX semantics) — so this is safe. If `min == max`, `span == 1`, `Int63n(1)` returns 0, and `anchor = minID`, which is correct. - `anchor = minID.Int64 + rng.Int63n(span)`: result is in `[minID, maxID]` inclusive — always a valid PK seek value. No overflow risk with realistic book counts. **6. Wrap-around: minID seek as fallback — CLEAN** The second seek uses `minID.Int64` as anchor with `need` as limit (both validated). The dedup set correctly prevents returning the same book twice. No privacy concern since all results come from the same non-deleted, non-sensitive column set. --- No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-03 17:35:53 +00:00

Author

Owner

Copy link

Code Review — bookshelf-0zpr.2 (PR #320)

Phase 0: DEMO Verification

No DEMO block was found in the bead comments or PR body. The PR contains a test plan checklist but no runnable command whose output can be re-executed and verified. For a UI rail feature, a valid DEMO would be a curl against the rendered home page or a browser screenshot showing the Discover rail. Per the review standard, absence of a DEMO block is a blocking finding.

Phase 1 & 2: Scale, Determinism, Correctness

Scale (PASS)

• discoverSQL uses WHERE b.id >= ? ... ORDER BY b.id LIMIT ? — a pure PK forward-range seek, O(LIMIT) at any scale. Confirmed: no ORDER BY RAND().
• discoverIDRangeSQL is SELECT MIN(id), MAX(id) FROM book WHERE deleted = 0 — reads two index extremes only, constant cost regardless of table size.
• Both seek queries have explicit LIMIT parameters. No unbounded query.
• Authors and formats are batch-fetched (single IN-query each). No N+1.

Determinism (PASS)

• discoverDateSeed computes t.UTC().Unix() / 86400 — integer UTC day number, stable for all timestamps on the same calendar day, changes at midnight UTC. Determinism is correct.
• time.Now() is called inside the Get returned closure at request time (service.go:85), not at wiring time. The seed is therefore per-request but stable within a day — exactly the intended behaviour.
• anchor = minID.Int64 + rng.Int63n(span) where span = maxID - minID + 1 >= 1. Int63n range is [0, span-1], so anchor is always in [minID, maxID]. No out-of-bounds risk.
• Single-book library: span = 1, Int63n(1) = 0, anchor = minID. Seek returns the one book. Correct.

Correctness (PASS with one minor)

• Empty library (NULL MIN/MAX): sql.NullInt64.Valid check at service.go:219 returns empty slice. Correct.
• sql.ErrNoRows guard at service.go:551 also returns empty slice. Correct.
• Wrap-around deduplication: the seen map is built from bookIDs (first-seek IDs), then extra rows with matching IDs are skipped. Parallel result/bookIDs slices stay in sync through the append loop. enrichAuthors receives the correctly unified slices. Logic is sound.
• Wrap-around second seek uses need (not limit) as the SQL LIMIT, so it requests only the remaining gap. Correct.
• All error paths in fetchDiscoverBooks, enrichAuthors, and listFormats are propagated with wrapped errors. No silent swallowing.
• Curried-fn pattern followed throughout. Wired correctly in wire.go with d.Conn.QueryRowContext and d.Conn.QueryContext.

Test quality

[MINOR] internal/home/service_test.go:461 — It("selects MIN and MAX of id") contains two Expect calls in one It block, violating the one-Expect-per-It convention (project-conventions.md). Split into two It blocks: It("contains MIN(id)") and It("contains MAX(id)").

[MINOR] internal/home/service_test.go:572 — Expect(ids).NotTo(ContainElements(int64(4), int64(4))) only verifies id=4 is not duplicated. id=5 is also a potential duplicate from the second seek but is not checked. Consider Expect(ids).To(HaveLen(5)) paired with ConsistOf to be exhaustive, or at minimum add a check for id=5.

---

REVIEW VERDICT: 1 blocker, 0 major, 2 minor

The blocker is the absent DEMO block — the Discover rail is a visible UI feature and the implementer did not provide a runnable command or screenshot showing it rendered on the home page. The two minors are test-quality nits that do not affect correctness or security.

## Code Review — bookshelf-0zpr.2 (PR #320) ### Phase 0: DEMO Verification No DEMO block was found in the bead comments or PR body. The PR contains a test plan checklist but no runnable command whose output can be re-executed and verified. For a UI rail feature, a valid DEMO would be a curl against the rendered home page or a browser screenshot showing the Discover rail. Per the review standard, absence of a DEMO block is a blocking finding. ### Phase 1 & 2: Scale, Determinism, Correctness **Scale (PASS)** - `discoverSQL` uses `WHERE b.id >= ? ... ORDER BY b.id LIMIT ?` — a pure PK forward-range seek, O(LIMIT) at any scale. Confirmed: no `ORDER BY RAND()`. - `discoverIDRangeSQL` is `SELECT MIN(id), MAX(id) FROM book WHERE deleted = 0` — reads two index extremes only, constant cost regardless of table size. - Both seek queries have explicit `LIMIT` parameters. No unbounded query. - Authors and formats are batch-fetched (single IN-query each). No N+1. **Determinism (PASS)** - `discoverDateSeed` computes `t.UTC().Unix() / 86400` — integer UTC day number, stable for all timestamps on the same calendar day, changes at midnight UTC. Determinism is correct. - `time.Now()` is called inside the `Get` returned closure at request time (`service.go:85`), not at wiring time. The seed is therefore per-request but stable within a day — exactly the intended behaviour. - `anchor = minID.Int64 + rng.Int63n(span)` where `span = maxID - minID + 1 >= 1`. `Int63n` range is `[0, span-1]`, so anchor is always in `[minID, maxID]`. No out-of-bounds risk. - Single-book library: `span = 1`, `Int63n(1) = 0`, `anchor = minID`. Seek returns the one book. Correct. **Correctness (PASS with one minor)** - Empty library (NULL MIN/MAX): `sql.NullInt64.Valid` check at `service.go:219` returns empty slice. Correct. - `sql.ErrNoRows` guard at `service.go:551` also returns empty slice. Correct. - Wrap-around deduplication: the `seen` map is built from `bookIDs` (first-seek IDs), then `extra` rows with matching IDs are skipped. Parallel `result`/`bookIDs` slices stay in sync through the append loop. `enrichAuthors` receives the correctly unified slices. Logic is sound. - Wrap-around second seek uses `need` (not `limit`) as the SQL LIMIT, so it requests only the remaining gap. Correct. - All error paths in `fetchDiscoverBooks`, enrichAuthors, and listFormats are propagated with wrapped errors. No silent swallowing. - Curried-fn pattern followed throughout. Wired correctly in `wire.go` with `d.Conn.QueryRowContext` and `d.Conn.QueryContext`. **Test quality** [MINOR] `internal/home/service_test.go:461` — `It("selects MIN and MAX of id")` contains two `Expect` calls in one `It` block, violating the one-Expect-per-It convention (`project-conventions.md`). Split into two `It` blocks: `It("contains MIN(id)")` and `It("contains MAX(id)")`. [MINOR] `internal/home/service_test.go:572` — `Expect(ids).NotTo(ContainElements(int64(4), int64(4)))` only verifies id=4 is not duplicated. id=5 is also a potential duplicate from the second seek but is not checked. Consider `Expect(ids).To(HaveLen(5))` paired with `ConsistOf` to be exhaustive, or at minimum add a check for id=5. --- REVIEW VERDICT: 1 blocker, 0 major, 2 minor The blocker is the absent DEMO block — the Discover rail is a visible UI feature and the implementer did not provide a runnable command or screenshot showing it rendered on the home page. The two minors are test-quality nits that do not affect correctness or security.

zombor force-pushed bd-bookshelf-0zpr.2 from 0f0885ae47 to 1f35a45ed6 2026-06-03 18:36:42 +00:00 Compare

zombor merged commit d7cf03ff66 into main 2026-06-03 18:44:02 +00:00

zombor referenced this pull request from a commit 2026-06-03 18:44:04 +00:00

Merge pull request 'feat(home): Discover rail — scale-safe random-id-anchor seek (bookshelf-0zpr.2)' (#320) from bd-bookshelf-0zpr.2 into main

zombor referenced this pull request from a commit 2026-08-08 02:34:05 +00:00

fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa)

zombor referenced this pull request 2026-08-08 02:34:32 +00:00

fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa) #1397

zombor referenced this pull request from a commit 2026-08-08 15:28:50 +00:00

fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!320

No description provided.