fix: address code review on #27

- The empty state is about an empty library, not a brand-new Reader:
  listView.Fresh becomes EmptyLibrary and moves behind the tab-specific
  branches, so "No favourites yet" is no longer shadowed for a Reader whose
  library happens to be empty. The action key stays put — hiding it was
  never asked for.
- The owner is not a revocable Reader: their row offers no button and
  POST /readers/{owner}/revoke is a 404, so the one row where the control
  would sign out the tapping browser cannot be reached by a hand-rolled
  POST either.
- Modify isolation is asserted in both directions, and the owner's own
  sign-in through the OAuth callback is pinned to the seeded row.
- CUTOVER.md and REDEPLOY.md still grepped API_TOKEN out of .env for their
  smoke tests, which the last commit deleted; both now take the acting
  Reader's derived credential.
- Roster type follows the machine-fact spec (500 10-11px mono, tracked),
  and PRODUCT.md names the Readers panel instead of claiming there is no
  owner surface at all.
This commit is contained in:
2026-08-08 20:14:21 +07:00
parent b0bf6fe770
commit f4f6c9c9e9
10 changed files with 115 additions and 40 deletions
+4 -1
View File
@@ -271,7 +271,10 @@ would catch a correct import behind a broken join:
```bash
API=https://bookmark-api.violetcrown.my.id
TOKEN=$(grep -E '^API_TOKEN=' .env | cut -d= -f2) # grace-window credential
# Your own Reader credential: sign in to the web UI and take it from the
# Userscripts panel's install link, or read the API_TOKEN constant out of an
# already-installed script. There is no credential in .env to grep.
TOKEN=<your Reader credential>
curl -s -H "Authorization: Bearer $TOKEN" $API/bookmarks |
python3 -c 'import json,sys; print(len(json.load(sys.stdin)))' # -> 29
```
+1 -1
View File
@@ -54,7 +54,7 @@ Not a public reading tracker or social app — a private, self-hosted sync layer
- Mobile is the primary target; desktop is an enhancement, not the design center.
- Progress data integrity over visual flourish: `updated_at`/list-ordering behavior is a correctness constraint the UI must respect, not decorate over.
- A leak between Readers fails silently and looks like working software — isolation is asserted from both directions, never inferred from counting one Reader's rows.
- No roles, no admin console, no org chrome: one owner capability and otherwise every Reader's view is the same.
- No roles, no org chrome: the owner's Readers panel is one list with one button (revoke someone's sessions), not an admin console, and otherwise every Reader's view is the same.
- Prefer native platform affordances (system dark/light, native touch targets) over custom widgetry — this is a lean self-hosted tool, not a product to demo.
## Accessibility & Inclusion
+4 -3
View File
@@ -231,9 +231,10 @@ Same four API checks as `DEPLOY.md` §3, plus the web UI. Set the host names onc
```bash
API=https://bookmark-api.violetcrown.my.id
WEB=https://bookmark.violetcrown.my.id
# During the grace window the retired global credential still resolves to the
# owner; afterwards it is 401 like any other wrong credential.
TOKEN=$(grep -E '^API_TOKEN=' .env | cut -d= -f2)
# Your own Reader credential - derived, never stored in .env. Take it from the
# Userscripts panel's install link after signing in, or from an installed
# script's API_TOKEN constant.
TOKEN=<your Reader credential>
curl -s $API/healthz # -> ok
curl -s -o /dev/null -w '%{http_code}\n' $API/bookmarks # -> 401
+8 -3
View File
@@ -301,10 +301,15 @@ button { cursor: pointer; }
border-top: 1px solid var(--rule);
}
.readerlist form { margin: 0 0 0 auto; }
.reader-id { font: 400 13px/1.4 var(--font-mono); color: var(--paper); }
.reader-sessions {
font: 400 11px/1.4 var(--font-mono);
.reader-id {
font: 500 13px/1.4 var(--font-mono);
letter-spacing: .04em;
color: var(--paper);
}
.reader-sessions {
font: 500 10px/1 var(--font-mono);
letter-spacing: .14em;
text-transform: uppercase;
color: var(--mute);
}
/* Revocation cuts someone off, so it wears --danger. Ember stays reserved for
+2 -4
View File
@@ -30,11 +30,9 @@
{{/* The action key. The icon strip on a card is unlabelled, so one permanent
line under the tabs names every glyph. It follows the tab rather than the
row: the archived and finished buckets swap Archive for Restore, and a
finished series has no Done to offer. A Reader with no cards at all has
nothing for it to name, so it hides rather than disappearing — see the
note above about out-of-band swaps needing their target to exist. */}}
finished series has no Done to offer. */}}
{{define "keyrow"}}
<div class="keyrow" id="keyrow" aria-label="Action key"{{if .OOB}} hx-swap-oob="true"{{end}}{{if .Fresh}} hidden{{end}}>
<div class="keyrow" id="keyrow" aria-label="Action key"{{if .OOB}} hx-swap-oob="true"{{end}}>
<span class="pair"><svg viewBox="0 0 24 24" aria-hidden="true"><use href="#i-play"/></svg><span>Read</span></span>
<span class="pair brass"><svg viewBox="0 0 24 24" aria-hidden="true"><use href="#i-star"/></svg><span>Fav</span></span>
<span class="pair"><svg viewBox="0 0 24 24" aria-hidden="true"><use href="#i-pencil"/></svg><span>Chapter</span></span>
+11 -12
View File
@@ -8,18 +8,6 @@
<strong>No titles match “<span class="no-match-q"></span>”.</strong>
<button type="button" class="clear-search">Clear search</button>
</div>
{{else if .Fresh}}
{{/* Nothing anywhere, not an empty bucket: this Reader has just registered,
so the empty state is the setup instruction rather than a filter
report. Both scripts, because the two libraries are separate installs. */}}
<div class="empty">
<strong>Your library is empty.</strong>
<p>Install both userscripts, then open a series and read a chapter — bookmarks arrive on their own.</p>
<p class="setup-links">
<a class="ghost" href="/install/manga-bookmark.user.js">Install Manga script</a>
<a class="ghost" href="/install/novel-bookmark.user.js">Install Novels script</a>
</p>
</div>
{{else if eq .Tab "fav"}}
<div class="empty"><strong>No favourites yet.</strong><p>Star a series to pin it here.</p></div>
{{else if eq .Tab "new"}}
@@ -28,6 +16,17 @@
<div class="empty"><strong>Nothing archived.</strong><p>Shelve a series to park it here — it keeps getting checked for new chapters.</p></div>
{{else if eq .Tab "finished"}}
<div class="empty"><strong>Nothing finished yet.</strong><p>Mark a series finished and it moves out of your reading list.</p></div>
{{else if .EmptyLibrary}}
{{/* Nothing in either library, so the links are the only thing this page can
usefully say. Both scripts: the two libraries are separate installs. */}}
<div class="empty">
<strong>Nothing here yet.</strong>
<p>Install the userscripts, then open a series and read a chapter — bookmarks arrive on their own.</p>
<p class="setup-links">
<a class="ghost" href="/install/manga-bookmark.user.js">Install Manga script</a>
<a class="ghost" href="/install/novel-bookmark.user.js">Install Novels script</a>
</p>
</div>
{{else}}
<div class="empty"><strong>Nothing here yet.</strong><p>Bookmarks appear once the userscript records a chapter.</p></div>
{{end}}
+4 -1
View File
@@ -13,7 +13,10 @@
<li>
<span class="reader-id">{{.DiscordID}}</span>
<span class="reader-sessions">{{.Sessions}} session{{if ne .Sessions 1}}s{{end}}</span>
{{if .Sessions}}
{{/* The owner's own row never offers Revoke: it is the one row where the
button would sign the tapping browser out, and the endpoint refuses
it anyway. Logout is the deliberate way to do that. */}}
{{if and .Sessions (ne .ID $.OwnerID)}}
<form hx-post="/readers/{{.ID}}/revoke" hx-target="#readers" hx-swap="outerHTML"
hx-confirm="Revoking signs this Reader out on every device immediately. Revoke?">
<button type="submit" class="ghost danger">Revoke sessions</button>
+23 -12
View File
@@ -72,16 +72,19 @@ type listView struct {
// Rotated marks the setup panel as having just rotated the credential:
// it swaps the reinstall warning in over the button row.
Rotated bool
// Fresh means this Reader has no bookmarks at all, in either library — a
// brand-new registration rather than an empty bucket. The empty state then
// explains how a library gets filled instead of describing a filter.
Fresh bool
// EmptyLibrary means this Reader holds no bookmarks in either library, so
// the empty state can offer the installs instead of reporting on a filter.
// It is not "newly registered": a Reader who deletes their last bookmark is
// in the same position and needs the same links.
EmptyLibrary bool
// Owner marks the acting Reader as the deployment's owner, which unlocks
// the Readers panel. Nothing else in the UI differs.
Owner bool
// Readers is the owner's roster, populated only for the owner's own page
// render and the revocation fragment.
// render and the revocation fragment. OwnerID travels with it so the roster
// can tell the owner's own row apart from the Readers they may revoke.
Readers []store.ReaderSummary
OwnerID int64
}
// PageURL and ListURL are the two link shapes every tab needs. Building them
@@ -238,7 +241,7 @@ func (h *Handler) index(w http.ResponseWriter, r *http.Request) {
return
}
if readerID == h.store.OwnerID() {
view.Owner = true
view.Owner, view.OwnerID = true, readerID
if view.Readers, err = h.store.Readers(); err != nil {
log.Printf("index readers: %v", err)
http.Error(w, "internal error", http.StatusInternalServerError)
@@ -292,10 +295,10 @@ func (h *Handler) buildListView(readerID int64, lib, tab string) (listView, erro
if err != nil {
return listView{}, err
}
// Fresh is about the Reader, not the library, so it is taken before the
// filter narrows the slice: it decides whether an empty list reads as
// "install the scripts" or "this bucket is empty".
fresh := len(all) == 0
// Taken before the filter narrows the slice: a Reader with novels but no
// manga has a working install already, and does not need to be told to go
// and get one.
emptyLibrary := len(all) == 0
// Narrow to one library first: reading, withNew and recent all derive from
// this slice, so doing it later would let the other library's rows into the
// strip and the Updated badge.
@@ -340,7 +343,7 @@ func (h *Handler) buildListView(readerID int64, lib, tab string) (listView, erro
}
}
return listView{Lib: lib, Tab: tab, Recent: recent, Items: items,
NewCount: len(withNew), Fresh: fresh}, nil
NewCount: len(withNew), EmptyLibrary: emptyLibrary}, nil
}
func (h *Handler) uiList(w http.ResponseWriter, r *http.Request) {
@@ -631,6 +634,14 @@ func (h *Handler) revokeReaderSessions(w http.ResponseWriter, r *http.Request) {
http.Error(w, "bad reader id", http.StatusBadRequest)
return
}
// The owner is not one of the Readers this endpoint reaches: revoking
// themselves would sign out the browser making the request, which is what
// logout is for. The roster hides the button; this refuses the hand-rolled
// POST behind it.
if target == h.store.OwnerID() {
http.NotFound(w, r)
return
}
if err := h.store.DeleteReaderSessions(target); err != nil {
log.Printf("revoke sessions: %v", err)
http.Error(w, "internal error", http.StatusInternalServerError)
@@ -642,5 +653,5 @@ func (h *Handler) revokeReaderSessions(w http.ResponseWriter, r *http.Request) {
http.Error(w, "internal error", http.StatusInternalServerError)
return
}
h.render(w, http.StatusOK, "readers", listView{Owner: true, Readers: readers})
h.render(w, http.StatusOK, "readers", listView{Owner: true, Readers: readers, OwnerID: h.store.OwnerID()})
}
+21 -2
View File
@@ -119,8 +119,27 @@ func TestPerReaderIsolation(t *testing.T) {
if err := json.Unmarshal(rr.Body.Bytes(), &owners); err != nil {
t.Fatalf("decode: %v", err)
}
if len(owners) != 1 || owners[0].Title != "Solo Leveling" {
t.Fatalf("owner list = %+v, want their own row", owners)
if len(owners) != 1 || owners[0].Title != "Solo Leveling" || owners[0].LastChapterNum != 0 {
t.Fatalf("owner list = %+v, want their own row at their own progress", owners)
}
// The mirror: the owner's write does not move the other Reader's progress
// either. Without it, isolation is only asserted in one direction.
req = credRequest(http.MethodPut, "/bookmarks/"+ownerKey, ownerCredential())
req.Header.Set("Content-Type", "application/json")
rr = httptest.NewRecorder()
srv.ServeHTTP(rr, withBody(req, `{"key":"asura:solo","site":"asura","series_id":"solo","title":"Solo Leveling","last_chapter_num":9}`))
if rr.Code != http.StatusOK {
t.Fatalf("owner put: status = %d, want 200", rr.Code)
}
rr = httptest.NewRecorder()
srv.ServeHTTP(rr, credRequest(http.MethodGet, "/bookmarks", readerCredential("other-reader")))
var theirs3 []store.Bookmark
if err := json.Unmarshal(rr.Body.Bytes(), &theirs3); err != nil {
t.Fatalf("decode: %v", err)
}
if len(theirs3) != 1 || theirs3[0].LastChapterNum != 3 {
t.Fatalf("other reader list = %+v, want progress 3 after the owner's write", theirs3)
}
// DELETE is scoped to its caller too, asserted in both directions: each
+37 -1
View File
@@ -485,6 +485,23 @@ func TestGuildMemberRegistersOnFirstLoginAndReusesIt(t *testing.T) {
}
}
// The seeded owner signs in through the same path: their row is found, not
// created a second time.
func TestOwnerLoginReusesTheSeededReader(t *testing.T) {
stub, stubSrv := newDiscordStub(t)
stub.ownerID = testDiscordID
cfg := testConfig()
cfg.Discord = discordConfig(stubSrv.URL)
router, st := newWebTestServer(t, cfg)
if got := signedInReader(t, router, st); got != st.OwnerID() {
t.Fatalf("owner's sign-in landed on Reader %d, want the seeded %d", got, st.OwnerID())
}
if n := len(storeReaders(t, st)); n != 1 {
t.Fatalf("readers after the owner's login = %d, want 1 (the seed was duplicated)", n)
}
}
// A brand-new Reader's page explains how a library gets filled and offers both
// install links, and the script it serves carries their credential — not the
// owner's.
@@ -514,7 +531,7 @@ func TestNewReaderSeesEmptyLibraryAndTheirOwnScript(t *testing.T) {
}
body := rr.Body.String()
for _, want := range []string{
"Your library is empty",
"Nothing here yet",
`href="/install/manga-bookmark.user.js"`,
`href="/install/novel-bookmark.user.js"`,
} {
@@ -570,6 +587,20 @@ func TestOwnerRevokesAnotherReadersSessions(t *testing.T) {
t.Fatal("a non-owner's revoke attempt still killed the owner's session")
}
// The owner is not a revocable Reader: the button would sign out the browser
// making the request, so both the roster and the endpoint refuse it.
req = httptest.NewRequest(http.MethodPost,
"/readers/"+strconv.FormatInt(st.OwnerID(), 10)+"/revoke", nil)
req.AddCookie(ownerCookie)
rr = httptest.NewRecorder()
router.ServeHTTP(rr, req)
if rr.Code != http.StatusNotFound {
t.Fatalf("owner revoking themselves: status = %d, want 404", rr.Code)
}
if _, ok, _ := st.GetSession(ownerCookie.Value, time.Now()); !ok {
t.Fatal("the owner signed themselves out through the revoke endpoint")
}
req = httptest.NewRequest(http.MethodPost,
"/readers/"+strconv.FormatInt(theirSession.ReaderID, 10)+"/revoke", nil)
req.AddCookie(ownerCookie)
@@ -614,6 +645,11 @@ func TestOwnerSeesReadersPanel(t *testing.T) {
if !strings.Contains(body, "Revoke sessions") {
t.Fatal("the roster offers no revocation control for a signed-in Reader")
}
// Exactly one revocable row: the other Reader's. The owner's own row carries
// the same session count and no button.
if n := strings.Count(body, "/revoke"); n != 1 {
t.Fatalf("roster has %d revoke controls, want 1 (the owner's own row must have none):\n%s", n, body)
}
}
func TestDiscordLoginTokenEndpointDown(t *testing.T) {