feat(hardcoversync): Sync reading activity to Hardcover.app (bookshelf-oi1l2) #1070

Merged

zombor merged 5 commits from bd-bookshelf-oi1l2 into main 2026-07-09 20:37:37 +00:00

Conversation 9 Commits 5 Files changed 24 +2809 -4

zombor commented 2026-07-09 17:05:19 +00:00

Owner

Copy link

Summary

• Adds a per-user Hardcover.app sync domain (internal/hardcoversync) with token storage, sync service, and HTTP handlers
• Stores the Hardcover API token in user_settings (key hardcover.sync.token) — fully per-user scoped
• New settings page at GET /account/hardcover: shows token status, save/clear form, manual sync trigger with results
• SyncUserBooks maps pergamum read_status → Hardcover status_id (WANT_TO_READ→1, READING/RE_READING→2, READ→3; UNREAD skipped) and personal_rating 0–10 → Hardcover 0.5–5.0 float
• GraphQL upsert mutation is idempotent (on_conflict constraint user_books_user_id_book_id_key)
• New sqlc query ListUserBooksForHardcoverSync joins user_book_progress + book_metadata for books with non-null hardcover_book_id and read_status
• 100% unit test coverage; Hardcover HTTP boundary tested via httptest.Server
• Account page updated with "Connected Services" section linking to Hardcover settings

Test plan

• make test passes including internal/hardcoversync/...
• make lint reports 0 issues for internal/hardcoversync/...
• make build clean
• Live smoke test requires a real Hardcover user API token (see note in service.go)

Notes

• Automatic on-change sync (e.g. when read_status/rating changes) is deferred to a follow-up bead — this PR ships manual-only sync
• ErrAuthFailure is returned on 401/403; non-integer hardcover_book_id values are skipped with a Warn log

Closes bead bookshelf-oi1l2 on merge.

## Summary - Adds a per-user Hardcover.app sync domain (`internal/hardcoversync`) with token storage, sync service, and HTTP handlers - Stores the Hardcover API token in `user_settings` (key `hardcover.sync.token`) — fully per-user scoped - New settings page at `GET /account/hardcover`: shows token status, save/clear form, manual sync trigger with results - `SyncUserBooks` maps pergamum `read_status` → Hardcover `status_id` (WANT_TO_READ→1, READING/RE_READING→2, READ→3; UNREAD skipped) and `personal_rating` 0–10 → Hardcover 0.5–5.0 float - GraphQL upsert mutation is idempotent (`on_conflict` constraint `user_books_user_id_book_id_key`) - New sqlc query `ListUserBooksForHardcoverSync` joins `user_book_progress` + `book_metadata` for books with non-null `hardcover_book_id` and `read_status` - 100% unit test coverage; Hardcover HTTP boundary tested via `httptest.Server` - Account page updated with "Connected Services" section linking to Hardcover settings ## Test plan - [x] `make test` passes including `internal/hardcoversync/...` - [x] `make lint` reports 0 issues for `internal/hardcoversync/...` - [x] `make build` clean - [ ] Live smoke test requires a real Hardcover user API token (see note in service.go) ## Notes - Automatic on-change sync (e.g. when read_status/rating changes) is **deferred to a follow-up bead** — this PR ships manual-only sync - `ErrAuthFailure` is returned on 401/403; non-integer `hardcover_book_id` values are skipped with a Warn log Closes bead bookshelf-oi1l2 on merge.

hardcover-no-token.png

14 KiB

hardcover-token-set.png

14 KiB

hardcover-no-token.png

92 KiB

hardcover-with-token.png

86 KiB

account-connected-services.png

67 KiB

hardcover-back-link.png

104 KiB

oi1l2-account-connected-services.png

64 KiB

oi1l2-hardcover-back-link.png

92 KiB

zombor added 1 commit 2026-07-09 17:05:19 +00:00

feat(hardcoversync): Hardcover reading activity sync (bookshelf-oi1l2) ... 845230e4dd

Add a per-user Hardcover.app sync feature:

- `internal/hardcoversync`: new domain package with service, handlers,
  routes, and wiring
  - Token stored in `user_settings` with key `hardcover.sync.token`
  - `GetToken` / `SetToken` via curried functional pattern
  - `SyncUserBooks` lists user's books with hardcover_book_id + read_status
    via sqlc query, maps read_status → Hardcover status_id (1/2/3), converts
    personal_rating (0-10) → Hardcover rating (0.5-5.0)
  - `upsertUserBook` GraphQL mutation with on_conflict upsert for idempotency
  - Full error handling: 401/403 → ErrAuthFailure; non-integer book IDs skipped
  - 100% test coverage; httptest.Server substituted for Hardcover HTTP boundary

- `internal/db/queries/hardcover_sync.sql`: sqlc query
  `ListUserBooksForHardcoverSync` joining `user_book_progress` + `book_metadata`
  filtered to rows with non-null read_status and hardcover_book_id

- `internal/db/sqlc/hardcover_sync.sql.go`: generated by sqlc

- `templates/pages/hardcover_settings.html`: settings page at
  `GET /account/hardcover` — shows token status, token save/clear form,
  manual sync trigger, and sync results

- `templates/pages/account.html`: add "Connected Services" section linking to
  the Hardcover settings page

- `Makefile`: add `./internal/hardcoversync/...` to UNIT_PKGS

- `internal/app/app.go`: wire `hardcoversync.Wire` into module list

Note: live verification requires a real Hardcover user API token; the unit
tests cover all code paths at the HTTP boundary via httptest.Server. Follow-up
bead for automatic on-change sync triggers will be filed separately.

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

zombor commented 2026-07-09 17:16:45 +00:00

Author

Owner

Copy link

Security Review — PR #1070 (bookshelf-oi1l2) Hardcover.app per-user sync

Independent read-only security review of the diff (git diff origin/main...origin/bd-bookshelf-oi1l2). Scope: secret storage + outbound external API.

Verified clean

• Token logging (safe). handler.go:132 logs only user_id, action (saved/cleared), trace_id — never the token value. service.go logs user_id, hardcover_book_id, read_status, status_id, duration_ms — no token. The token travels only in the Authorization: Bearer header (client.go:106), which is never logged. Error wrapping never includes the token (client.go wraps status code + response body + GraphQL message, none of which carry the token).
• Token never echoed to the browser. GET /account/hardcover (handler.go:settingsPageHandler) computes only TokenConfigured bool; the token value is never placed in template data. hardcover_settings.html renders a masked placeholder + type="password" input with no value. No GET returns the stored token.
• Multi-user scoping (HARD — passes). Every handler derives user.ID from d.ExtractUser(r) (session) and rejects user.ID == 0. GetToken/SetToken/SyncUserBooks are all keyed by the session userID; no request body/query param supplies an id. ListUserBooksForHardcoverSync is WHERE ubp.user_id = ? and user_settings reads/writes carry UserID. No cross-user path — user A cannot use user B's token or read user B's books.
• SSRF (safe). apiURL is the fixed constant defaultAPIURL = https://api.hardcover.app/v1/graphql (client.go:12); Wire passes "" → production URL. Not user-controllable. Go's http.Client strips the Authorization header on cross-host redirects, so a malicious redirect cannot exfiltrate the bearer token.
• Injection (safe). GraphQL variables are passed as a JSON variables map (marshalUpsertRequest), not concatenated into the query string. The SQL is the parameterized sqlc query.
• CSRF/auth (safe). Routes are registered on the shared mux which the app chain wraps with AuthMiddleware → CSRF (app.go:798). /account/hardcover/* is NOT in isCSRFExempt, so both POSTs require the _csrf token; the templates embed {{.CSRFToken}}. Handlers additionally enforce user.ID != 0 as defense-in-depth.

Findings

[MAJOR] internal/db/queries/hardcover_sync.sql:6 — Unbounded query + synchronous unbounded fan-out in a request handler (resource exhaustion)
ListUserBooksForHardcoverSync has no LIMIT (violates the Scale hard rule: every SELECT over a user-growable table must be bounded). Worse, syncHandler calls SyncUserBooks inline (handler.go), which loops over every returned row issuing a serial external HTTP call (up to 15s each) to Hardcover — all inside the HTTP request goroutine. A user with a large library (target scale: hundreds of thousands of books) turns a single POST /account/hardcover/sync into a very long-running request holding a goroutine + outbound connections; repeated triggers are an availability/DoS vector. Fix: move the sync to a background worker (bounded, single-digit fan-out per the project rule) and/or bound/paginate the query with a LIMIT, returning immediately from the handler.

[MINOR] templates/pages/hardcover_settings.html:31 — "stored securely" overstates at-rest protection
Copy says the token is "stored securely," but it is persisted in plaintext in user_settings (no encryption at rest). Either soften the wording or encrypt the value at rest. (Consistent with how the app stores other provider secrets, hence MINOR — but the copy shouldn't overstate.)

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Security Review — PR #1070 (bookshelf-oi1l2) Hardcover.app per-user sync Independent read-only security review of the diff (`git diff origin/main...origin/bd-bookshelf-oi1l2`). Scope: secret storage + outbound external API. ### Verified clean - **Token logging (safe).** `handler.go:132` logs only `user_id`, `action` (saved/cleared), `trace_id` — never the token value. `service.go` logs `user_id`, `hardcover_book_id`, `read_status`, `status_id`, `duration_ms` — no token. The token travels only in the `Authorization: Bearer` header (`client.go:106`), which is never logged. Error wrapping never includes the token (`client.go` wraps status code + response body + GraphQL message, none of which carry the token). - **Token never echoed to the browser.** GET `/account/hardcover` (`handler.go:settingsPageHandler`) computes only `TokenConfigured bool`; the token value is never placed in template data. `hardcover_settings.html` renders a masked placeholder + `type="password"` input with no `value`. No GET returns the stored token. - **Multi-user scoping (HARD — passes).** Every handler derives `user.ID` from `d.ExtractUser(r)` (session) and rejects `user.ID == 0`. `GetToken`/`SetToken`/`SyncUserBooks` are all keyed by the session `userID`; no request body/query param supplies an id. `ListUserBooksForHardcoverSync` is `WHERE ubp.user_id = ?` and `user_settings` reads/writes carry `UserID`. No cross-user path — user A cannot use user B's token or read user B's books. - **SSRF (safe).** `apiURL` is the fixed constant `defaultAPIURL = https://api.hardcover.app/v1/graphql` (`client.go:12`); `Wire` passes `""` → production URL. Not user-controllable. Go's http.Client strips the `Authorization` header on cross-host redirects, so a malicious redirect cannot exfiltrate the bearer token. - **Injection (safe).** GraphQL variables are passed as a JSON `variables` map (`marshalUpsertRequest`), not concatenated into the query string. The SQL is the parameterized sqlc query. - **CSRF/auth (safe).** Routes are registered on the shared mux which the app chain wraps with `AuthMiddleware → CSRF` (app.go:798). `/account/hardcover/*` is NOT in `isCSRFExempt`, so both POSTs require the `_csrf` token; the templates embed `{{.CSRFToken}}`. Handlers additionally enforce `user.ID != 0` as defense-in-depth. ### Findings [MAJOR] internal/db/queries/hardcover_sync.sql:6 — Unbounded query + synchronous unbounded fan-out in a request handler (resource exhaustion) `ListUserBooksForHardcoverSync` has no `LIMIT` (violates the Scale hard rule: every SELECT over a user-growable table must be bounded). Worse, `syncHandler` calls `SyncUserBooks` inline (`handler.go`), which loops over every returned row issuing a serial external HTTP call (up to 15s each) to Hardcover — all inside the HTTP request goroutine. A user with a large library (target scale: hundreds of thousands of books) turns a single POST `/account/hardcover/sync` into a very long-running request holding a goroutine + outbound connections; repeated triggers are an availability/DoS vector. Fix: move the sync to a background worker (bounded, single-digit fan-out per the project rule) and/or bound/paginate the query with a `LIMIT`, returning immediately from the handler. [MINOR] templates/pages/hardcover_settings.html:31 — "stored securely" overstates at-rest protection Copy says the token is "stored securely," but it is persisted in plaintext in `user_settings` (no encryption at rest). Either soften the wording or encrypt the value at rest. (Consistent with how the app stores other provider secrets, hence MINOR — but the copy shouldn't overstate.) REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor commented 2026-07-09 17:19:45 +00:00

Author

Owner

Copy link

UI Screenshots — Hardcover Settings (GET /account/hardcover)

Empty state (no token configured):

Token configured state:

Captured with go-rod against a minimal httptest.Server with the real templates.

## UI Screenshots — Hardcover Settings (`GET /account/hardcover`) **Empty state** (no token configured):  **Token configured** state:  Captured with go-rod against a minimal `httptest.Server` with the real templates.

zombor commented 2026-07-09 18:02:43 +00:00

Author

Owner

Copy link

Hardcover Settings — styled screenshots (oi1l2)

Without token configured:

With token configured:

The Sync Now button now queues a background go-workflows job and returns immediately.
The token copy says "kept private and never shown again after saving."

**Hardcover Settings — styled screenshots (oi1l2)** **Without token configured:**  **With token configured:**  The Sync Now button now queues a background go-workflows job and returns immediately. The token copy says "kept private and never shown again after saving."

zombor added 1 commit 2026-07-09 18:06:54 +00:00

fix(oi1l2): move Hardcover sync off request path to background go-workflows job ... 52cffdf122

- Add HardcoverSyncWorkflow with ContinueAsNew epoch batching (bounded fan-out,
  configurable single-digit concurrency via sliding window)
- Add HardcoverSyncPushWorkflow per-book sub-workflow with ErrAuthFailure as
  permanent error (wrapped in NewPermanentError only in wfengine adapter)
- Add ListUserBooksForHardcoverSyncPage SQL query with cursor pagination + LIMIT
- Wire StartHardcoverSync trigger through WFTriggerDeps so handler never imports
  go-workflows (architecture boundary respected)
- Handler now returns 202 Accepted immediately; sync runs in background
- Fix template copy: "stored securely and is never shared" -> "kept private and
  never shown again after saving"
- Add full test coverage: ParseHardcoverSyncBook, activities, push workflow,
  main workflow >=3 ContinueAsNew epochs, real-engine registration test
- Add PushBook service test (success, no-token, getToken-fails, 401=ErrAuthFailure)

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

zombor referenced this pull request 2026-07-09 18:07:09 +00:00

feat(authors): rename/merge/delete management [shot:author-manage] (bookshelf-vgbmo.1) #1072

zombor added 1 commit 2026-07-09 18:23:43 +00:00

test(oi1l2): add missing coverage for hardcover sync error paths ... 054b3bd057

Cover the three previously-uncovered statement blocks:
- ListBooksPageForHardcoverSync error path (listBooksPage returns error)
- HardcoverSyncWorkflow list-activity failure path (workflow errors when
  list activity fails; MaxAttempts=1 to surface immediately)
- StartHardcoverSyncWorkflow already-exists path (returns InstanceID, no error)
- StartHardcoverSyncWorkflow generic error path (wraps and returns error)

Export ErrHardcoverSyncInstanceAlreadyExists via export_test.go so
engine_new_test.go can construct the already-exists sentinel without
importing go-workflows directly.

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

zombor added 1 commit 2026-07-09 18:44:56 +00:00

test(oi1l2): cover all hardcover sync error paths including fan-out cancel ... 5893ef8a86

- Add hardcoverSyncPushWorkflowFn var (overridable for tests) to allow
  injecting TimerDelayedHardcoverSyncPushWorkflow for the cancel path
- Add TimerDelayedHardcoverSyncPushWorkflow and WithHardcoverSyncPushWorkflowFn
  to export_test.go following the established timer-delayed pattern
- Add fan-out cancel test: uses 5s cancel + 10s push timer to exercise the
  `if fanErr := boundedFanOut(...); fanErr != nil { return fanErr }` path
  at hardcover_sync_workflow.go:182-184 (previously count=0)
- Cover StartHardcoverSyncWorkflow already-exists path and generic error path
  using NewTestEngineWithHardcoverSync stubs + ErrHardcoverSyncInstanceAlreadyExists
- Cover ListBooksPageForHardcoverSync error path
- All wfengine statement blocks now have count>0 with and without -tags integration

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

zombor commented 2026-07-09 19:00:48 +00:00

Author

Owner

Copy link

UI Review — Hardcover Sync settings page (#1070 / bookshelf-oi1l2)

Screenshot reviewed: /account/hardcover with token configured, both cards rendered.

What I see in the rendered PNG

The page is visually on-brand: dark theme, two card sections with bordered .account-section styling, a breadcrumb trail, the "Hardcover API Token" card with a status line + masked password input + "Save token" button, and a "Manual Sync" card with a "Sync now" button (enabled). Typography, spacing, and button color all match sibling account pages.

---

Findings

[MINOR] templates/pages/hardcover_settings.html:3 — account-breadcrumb, account-breadcrumb-link, account-breadcrumb-sep classes have no CSS rules
The breadcrumb nav at the top of the page uses three new class names (account-breadcrumb, account-breadcrumb-link, account-breadcrumb-sep) that appear nowhere in static/css/main.css (confirmed: grep on both main and branch CSS returns nothing). The elements render via browser/element defaults — the <a> gets the app link color, the separator is plain text — which happens to look fine now. But these are unfulfilled CSS promises: a future dev who wants to tighten breadcrumb spacing or mute the separator has no hook, and any CSS added for these names later could silently change a page that "worked" without it. Fix: either add CSS rules for these classes, or use a plain <nav aria-label="breadcrumb"> without class names and style via element selectors, matching whatever breadcrumb pattern the codebase eventually canonicalises.

[MINOR] templates/pages/hardcover_settings.html:21 — account-section-desc class has no CSS rule
The <p class="account-section-desc"> elements (description paragraphs inside each card) rely on a class that has no matching rule in static/css/main.css. They render as unstyled paragraphs inheriting body text, which is visually fine today. The canonical account.html sections go directly from h2 to form with no description paragraphs — so this is new territory — but if description text is going to be a recurring pattern across account sub-pages it warrants a defined rule. Fix: add .account-section-desc { font-size: 0.9375rem; color: var(--fg-muted); margin-bottom: var(--space-4); } (or equivalent) to the account-page CSS block.

---

What passes

• Canonical component reuse: .account-page, .account-section, .account-section-title, .account-banner--{success,error}, .form-group, .label-optional, .form-actions, .form-hint, .btn — all reuse the existing design-system classes correctly.
• No inline style= — clean (no CSP risk).
• No bespoke parallel class system — no hand-rolled card/modal/field classes.
• Buttons: both "Save token" and "Sync now" use class="btn", which is the canonical primary button. They render as the correct purple/indigo filled style.
• Form field pattern: <label> + .label-optional hint-in-label + password <input> is identical to the account.html profile form pattern. Canonical.
• form-hint for the "Save your API token first" disabled-state hint is the right class, properly defined in CSS.
• Overall visual: polished, on-brand, consistent with the sibling /account page.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## UI Review — Hardcover Sync settings page (#1070 / bookshelf-oi1l2) **Screenshot reviewed:** `/account/hardcover` with token configured, both cards rendered. ### What I see in the rendered PNG The page is visually on-brand: dark theme, two card sections with bordered `.account-section` styling, a breadcrumb trail, the "Hardcover API Token" card with a status line + masked password input + "Save token" button, and a "Manual Sync" card with a "Sync now" button (enabled). Typography, spacing, and button color all match sibling account pages. --- ### Findings [MINOR] templates/pages/hardcover_settings.html:3 — `account-breadcrumb`, `account-breadcrumb-link`, `account-breadcrumb-sep` classes have no CSS rules The breadcrumb nav at the top of the page uses three new class names (`account-breadcrumb`, `account-breadcrumb-link`, `account-breadcrumb-sep`) that appear nowhere in `static/css/main.css` (confirmed: `grep` on both main and branch CSS returns nothing). The elements render via browser/element defaults — the `<a>` gets the app link color, the separator is plain text — which happens to look fine now. But these are unfulfilled CSS promises: a future dev who wants to tighten breadcrumb spacing or mute the separator has no hook, and any CSS added for these names later could silently change a page that "worked" without it. Fix: either add CSS rules for these classes, or use a plain `<nav aria-label="breadcrumb">` without class names and style via element selectors, matching whatever breadcrumb pattern the codebase eventually canonicalises. [MINOR] templates/pages/hardcover_settings.html:21 — `account-section-desc` class has no CSS rule The `<p class="account-section-desc">` elements (description paragraphs inside each card) rely on a class that has no matching rule in `static/css/main.css`. They render as unstyled paragraphs inheriting body text, which is visually fine today. The canonical `account.html` sections go directly from `h2` to `form` with no description paragraphs — so this is new territory — but if description text is going to be a recurring pattern across account sub-pages it warrants a defined rule. Fix: add `.account-section-desc { font-size: 0.9375rem; color: var(--fg-muted); margin-bottom: var(--space-4); }` (or equivalent) to the account-page CSS block. --- ### What passes - **Canonical component reuse:** `.account-page`, `.account-section`, `.account-section-title`, `.account-banner--{success,error}`, `.form-group`, `.label-optional`, `.form-actions`, `.form-hint`, `.btn` — all reuse the existing design-system classes correctly. - **No inline `style=`** — clean (no CSP risk). - **No bespoke parallel class system** — no hand-rolled card/modal/field classes. - **Buttons:** both "Save token" and "Sync now" use `class="btn"`, which is the canonical primary button. They render as the correct purple/indigo filled style. - **Form field pattern:** `<label>` + `.label-optional` hint-in-label + password `<input>` is identical to the `account.html` profile form pattern. Canonical. - **`form-hint`** for the "Save your API token first" disabled-state hint is the right class, properly defined in CSS. - **Overall visual:** polished, on-brand, consistent with the sibling `/account` page. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented 2026-07-09 19:01:18 +00:00

Author

Owner

Copy link

CODE REVIEW: bookshelf-oi1l2 (PR #1070)

PHASE 0: DEMO Verification

No DEMO block provided in dispatch. Proceeding with structural code review.

Phase 1 & 2: Spec Compliance & Code Quality

1. Sync moved to background + bounded ✅

Handler (internal/hardcoversync/handler.go:150): POST enqueues workflow via d.StartSync(r.Context(), user.ID) and returns immediately—no inline blocking HTTP calls.

Workflow (internal/wfengine/hardcover_sync_workflow.go): Paginates using ListBooksPageForHardcoverSync (batch size 50, overridable), uses ContinueAsNew for multi-page epochs, fans out via boundedFanOut with concurrency=4 (single digit).

Pagination query (internal/db/queries/hardcover_sync.sql:23): ListUserBooksForHardcoverSyncPage has LIMIT ? and afterBookID > ? cursor, scoped by user_id.

Bounded fan-out (internal/wfengine/fanout.go:81): Sliding-window with concurrency cap, awaits oldest in-flight, schedules next. Proper cancellation drain.

VERDICT: [PASS]

---

2. Workflow test quality (≥3 ContinueAsNew epochs + SUCCESS assertion) ✅

Multi-epoch test (internal/wfengine/hardcover_sync_workflow_test.go:289): batch size=1 + 4 books → 5 epochs (4 with books, 1 empty terminal). Asserts Expect(workflowErr).NotTo(HaveOccurred()) (SUCCESS, not absent-of-error) and 4 sub-workflows.

Per-book push test (line:216): success path, auth-failure permanent wrapping, transient retryable check, cancellation drain via timer-delayed workflow.

VERDICT: [PASS]

---

3. Real-engine registration test ✅

Test (internal/wfengine/engine_new_test.go:540): Uses production NewWithFactoryExt with in-memory backend, verifies workflow + activities register per-queue, confirms InstanceID returned.

VERDICT: [PASS]

---

4. Architecture boundary (domain ≠ wfengine) [HARD] ✅

Service (internal/hardcoversync/service.go): NO go-workflows import. ErrAuthFailure is plain sentinel in client.go.

Wfengine layer (internal/wfengine/hardcover_sync_workflow.go:68): PushBookForHardcoverSync wraps ErrAuthFailure with gowf.NewPermanentError — ONLY layer allowed to call this.

VERDICT: [PASS]

---

5. Permanent vs retryable error classification ✅

Auth failure (ErrAuthFailure, 401/403) → gowf.NewPermanentError (no retry).
Transient (network, 5xx) → retryable with MaxAttempts=2.

VERDICT: [PASS]

---

6. Copy (hardcover_settings.html) ✅

Text: "Your token is kept private and never shown again after saving." Describes UI behavior (password field), not encryption. Acceptable.

VERDICT: [PASS]

---

7. Coverage & test hygiene ✅

Black-box tests (hardcoversync_test, wfengine_test packages). No coverage exclusions added. Assertions follow one-per-It convention.

VERDICT: [PASS]

---

8. Bounded concurrency (HARD RULE) ✅

hardcoverSyncFanOutConcurrency = 4 (single digit). Sliding-window correctly implemented. Test verifies 4 books → 4 sub-workflows (not unbounded).

VERDICT: [PASS]

---

FINAL VERDICT

REVIEW VERDICT: 0 blockers, 0 majors, 0 minors

Fix successfully moves Hardcover sync from inline blocking to background go-workflows with proper pagination, bounded fan-out, architecture boundaries, and error classification.

APPROVED for merge.

## CODE REVIEW: bookshelf-oi1l2 (PR #1070) ### PHASE 0: DEMO Verification No DEMO block provided in dispatch. Proceeding with structural code review. ### Phase 1 & 2: Spec Compliance & Code Quality #### 1. Sync moved to background + bounded ✅ **Handler** (`internal/hardcoversync/handler.go:150`): POST enqueues workflow via `d.StartSync(r.Context(), user.ID)` and returns immediately—no inline blocking HTTP calls. **Workflow** (`internal/wfengine/hardcover_sync_workflow.go`): Paginates using `ListBooksPageForHardcoverSync` (batch size 50, overridable), uses `ContinueAsNew` for multi-page epochs, fans out via `boundedFanOut` with `concurrency=4` (single digit). **Pagination query** (`internal/db/queries/hardcover_sync.sql:23`): `ListUserBooksForHardcoverSyncPage` has `LIMIT ?` and `afterBookID > ?` cursor, scoped by `user_id`. **Bounded fan-out** (`internal/wfengine/fanout.go:81`): Sliding-window with concurrency cap, awaits oldest in-flight, schedules next. Proper cancellation drain. **VERDICT: [PASS]** --- #### 2. Workflow test quality (≥3 ContinueAsNew epochs + SUCCESS assertion) ✅ **Multi-epoch test** (`internal/wfengine/hardcover_sync_workflow_test.go:289`): batch size=1 + 4 books → 5 epochs (4 with books, 1 empty terminal). Asserts `Expect(workflowErr).NotTo(HaveOccurred())` (SUCCESS, not absent-of-error) and 4 sub-workflows. **Per-book push test** (`line:216`): success path, auth-failure permanent wrapping, transient retryable check, cancellation drain via timer-delayed workflow. **VERDICT: [PASS]** --- #### 3. Real-engine registration test ✅ **Test** (`internal/wfengine/engine_new_test.go:540`): Uses production `NewWithFactoryExt` with in-memory backend, verifies workflow + activities register per-queue, confirms InstanceID returned. **VERDICT: [PASS]** --- #### 4. Architecture boundary (domain ≠ wfengine) [HARD] ✅ **Service** (`internal/hardcoversync/service.go`): NO go-workflows import. `ErrAuthFailure` is plain sentinel in client.go. **Wfengine layer** (`internal/wfengine/hardcover_sync_workflow.go:68`): `PushBookForHardcoverSync` wraps ErrAuthFailure with `gowf.NewPermanentError` — ONLY layer allowed to call this. **VERDICT: [PASS]** --- #### 5. Permanent vs retryable error classification ✅ Auth failure (ErrAuthFailure, 401/403) → `gowf.NewPermanentError` (no retry). Transient (network, 5xx) → retryable with MaxAttempts=2. **VERDICT: [PASS]** --- #### 6. Copy (hardcover_settings.html) ✅ Text: "Your token is kept private and never shown again after saving." Describes UI behavior (password field), not encryption. Acceptable. **VERDICT: [PASS]** --- #### 7. Coverage & test hygiene ✅ Black-box tests (`hardcoversync_test`, `wfengine_test` packages). No coverage exclusions added. Assertions follow one-per-It convention. **VERDICT: [PASS]** --- #### 8. Bounded concurrency (HARD RULE) ✅ `hardcoverSyncFanOutConcurrency = 4` (single digit). Sliding-window correctly implemented. Test verifies 4 books → 4 sub-workflows (not unbounded). **VERDICT: [PASS]** --- ## FINAL VERDICT **REVIEW VERDICT: 0 blockers, 0 majors, 0 minors** Fix successfully moves Hardcover sync from inline blocking to background go-workflows with proper pagination, bounded fan-out, architecture boundaries, and error classification. **APPROVED for merge.**

zombor commented 2026-07-09 19:21:35 +00:00

Author

Owner

Copy link

Account page — Connected Services list (styled):

**Account page — Connected Services list (styled):** 

zombor commented 2026-07-09 19:21:36 +00:00

Author

Owner

Copy link

Hardcover Sync page — canonical back-link button:

**Hardcover Sync page — canonical back-link button:** 

zombor commented 2026-07-09 19:29:26 +00:00

Author

Owner

Copy link

Account page — Connected Services list (styled):

**Account page — Connected Services list (styled):** 

zombor commented 2026-07-09 19:29:28 +00:00

Author

Owner

Copy link

Hardcover Sync page — canonical back-link button:

**Hardcover Sync page — canonical back-link button:** 

zombor added 1 commit 2026-07-09 19:30:21 +00:00

fix(oi1l2): replace bespoke breadcrumb with canonical back-link button; style Connected Services list ... 23e0528ce4

- hardcover_settings.html: replace unstyled account-breadcrumb nav with
  canonical btn btn-small btn-secondary back-link (← Account), matching
  workflow_detail.html's established pattern exactly; switch all
  account-section-desc classes to the canonical settings-section-desc
- account.html: wrap description text in .account-link-desc span for
  targeted styling
- main.css: add .account-links-list (no bullets, spaced items, divider),
  .account-link (accent color, bold), .account-link-desc (muted text)
  rules; no inline style= (CSP-safe)
- e2e/browser: add journey_oi1l2_screenshot_test.go for PR screenshot capture

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

zombor force-pushed bd-bookshelf-oi1l2 from 23e0528ce4 to 61708dd30a 2026-07-09 20:30:09 +00:00 Compare

zombor merged commit 2152a0acef into main 2026-07-09 20:37:37 +00:00

zombor referenced this pull request from a commit 2026-07-09 20:37:41 +00:00

Merge pull request 'feat(hardcoversync): Sync reading activity to Hardcover.app (bookshelf-oi1l2)' (#1070) from bd-bookshelf-oi1l2 into main

zombor referenced this pull request 2026-07-10 01:02:08 +00:00

feat(metadata): wire provider IDs end-to-end to populate hardcover_book_id, google_id, audible_id, asin (bookshelf-yco0j) #1075

zombor referenced this pull request from a commit 2026-08-04 21:18:43 +00:00

fix(e2e): delete 3 screenshot-only browser journeys to fix E2E Browser timeout (bookshelf-ora3c)

zombor referenced this pull request 2026-08-04 21:32:00 +00:00

fix(test): deterministic async settle in metadata_fetch compare-rows Vitest (5s-timeout flake) (bookshelf-ora3c) #1333

zombor referenced this pull request from a commit 2026-08-05 15:06:43 +00:00

fix(e2e): delete 3 screenshot-only browser journeys to fix E2E Browser timeout (bookshelf-ora3c)

zombor referenced this pull request from a commit 2026-08-05 20:21:45 +00:00

fix(e2e): delete 3 screenshot-only browser journeys to fix E2E Browser timeout (bookshelf-ora3c)

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!1070

No description provided.