amadan

oficina / sheets Public

fix: close the last-owner orphan, and the review's findings on this branch

Two Fable review passes, both REQUEST CHANGES. This is what they found.

**BLOCKER, and it is already live.** A sheet could be left with no owner
in one form submission. `wouldOrphan` guarded revoke and nothing else,
but `grant` upserts on (sheet_id, member_id) — re-sharing with an
address that already has access CHANGES their role, and counted nobody.
So the sole owner typing their own address and picking "viewer" demoted
themselves: zero owner grants, `mayShare` false for everyone including
them, and, as revoke's own comment says, no admin path to repair it.
`TestTheLastOwnerCannotBeRemoved` asserted this was impossible while
testing only the other door.

Both doors now refuse it, and the check sits inside the transaction that
does the write rather than in the handler above it. That is the second
finding: check-then-act across a request boundary let two owners
revoking each other concurrently both count two owners, both pass, and
both delete. SQLite serialises the writes, not a read against someone
else's later write. `lastOwnerGoing` moved to its own file — grants.go
crossed the 300-line cap, which the guard caught.

**The badge that swallows prose came back.** rst-help was fixed on the
sign-in screen and reappeared within days on the share and history
screens, written by another session reaching for the obvious name for
help text. It is a 28px square badge; given a sentence it keeps the box
and spills the text. Both fixed, and now guarded — the name is a trap
and prose is the bait, so the trap is what gets tested.

The guard then earned itself during the rebase onto current core: it
failed on two MORE instances that had landed in the meantime, in the
sort/filter notice and the comments screen. Five instances of one
mistake in a few days, four of them written after the first was fixed.
Both are fixed here too. That is the whole argument for the test — a
name this easy to reach for is not going to be remembered around.

**The table view told viewers a lie.** handleTable admits anyone who may
read; handleTableWrite 404s a viewer or commenter. So the subtitle this
branch had just rewritten — "every cell here is editable" — was shown to
people the server would refuse, above a link per cell that opened a form
that 404d on submit. The claim, the anchors and ?edit= are all gated on
MayWrite now, expressed as a coordinate no cell can equal rather than a
flag the template is trusted to honour.

**The share default test was anchored on the wrong select.** There are
two named "role" on that page and the link one also leads with viewer,
so a reordering of the forms would have retargeted the test at the one
that was right and kept passing while the person default regressed. It
anchors on id now, and also asserts no option is marked selected, which
is the other way the default moves.

Remaining review findings are on core rather than this branch and are
recorded in discussion #2 rather than fixed here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XtQKC2QWg1cP9RQ5zAL72J
Paul Campbell pushed by paul@keymail.dev c4a395435ab8229df041dca4c7fbd8c43bb60ee2 parent f5f063e
13 files changed, +398 −23
  • internal/guardstest/markup_test.go +61 −0
  • internal/sheets/grants.go +32 −6
  • internal/sheets/grants_orphan_test.go +94 −0
  • internal/sheets/handlers_share.go +18 −4
  • internal/sheets/handlers_share_test.go +21 −2
  • internal/sheets/handlers_table.go +21 −0
  • internal/sheets/handlers_table_viewer_test.go +69 −0
  • internal/sheets/lastowner.go +57 −0
  • internal/sheets/static/sheets.css +5 −3
  • internal/sheets/templates/comments.html +1 −1
  • internal/sheets/templates/history.html +2 −1
  • internal/sheets/templates/share.html +5 −2
  • internal/sheets/templates/table.html +12 −4

CI passed — run details

Intent

Attributed to this commit: 270 g CO₂e (34505464 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on design-fixes, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.

full ledger: design-fixes

diff --git a/internal/guardstest/markup_test.go b/internal/guardstest/markup_test.go
index c4bf893..cd67ffa 100644
--- a/internal/guardstest/markup_test.go
+++ b/internal/guardstest/markup_test.go
@@ -112,3 +112,64 @@ func templateFiles(t *testing.T, root string) []string {
}
return out
}
+
+// badgesThatSwallowProse are rastrillo attributes whose rule gives the
+// element a FIXED width and height. Wrapped around a sentence they keep
+// the box and let the text spill out of it.
+//
+// rst-help is the case that produced this test. It is the little "?"
+// beside a label — tokens.css gives it inline-flex, 28px square, a
+// border and a radius. Given a paragraph it rendered one or two words
+// per line with the badge's border struck through the middle, and ran
+// off the left edge of a phone.
+//
+// It is here rather than in a review checklist because it came back.
+// Fixed on the sign-in screen, it reappeared within days on the share
+// and history screens, written by a different session reaching for the
+// obvious name for help text. The name is a trap and prose is the bait,
+// so the trap is what gets guarded.
+//
+// The prose token is rst-field-help.
+var badgesThatSwallowProse = []string{"rst-help"}
+
+// TestBadgeAttributesNeverWrapProse fails on a badge holding a sentence.
+//
+// The threshold is deliberately generous: a badge legitimately holds an
+// icon, a glyph, or a very short label, and this test must not argue
+// with that. What it catches is prose — anything past a handful of
+// characters, which no 28px box can show.
+func TestBadgeAttributesNeverWrapProse(t *testing.T) {
+ root := repoRoot(t)
+ dir := filepath.Join(root, "internal/sheets/templates")
+ entries, err := os.ReadDir(dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ for _, name := range badgesThatSwallowProse {
+ re := regexp.MustCompile(`(?s)<(\w+)\s[^>]*\b` +
+ regexp.QuoteMeta(name) + `\b[^>]*>(.*?)</`)
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasSuffix(e.Name(), ".html") {
+ continue
+ }
+ body, err := os.ReadFile(filepath.Join(dir, e.Name()))
+ if err != nil {
+ t.Fatal(err)
+ }
+ // Template comments first: they discuss this attribute by
+ // name, and matching inside one would fail on the very
+ // prose that explains the rule.
+ clean := regexp.MustCompile(`(?s)\{\{/\*.*?\*/\}\}`).
+ ReplaceAllString(string(body), "")
+ for _, m := range re.FindAllStringSubmatch(clean, -1) {
+ inner := strings.TrimSpace(m[2])
+ if len(inner) > 24 {
+ t.Errorf("%s: [%s] wraps %d characters of prose. It is a "+
+ "28px square badge — the box stays and the text "+
+ "spills out of it. Use rst-field-help.\n %.60s",
+ e.Name(), name, len(inner), inner)
+ }
+ }
+ }
+ }
+}
diff --git a/internal/sheets/grants.go b/internal/sheets/grants.go
index 4c1e0cb..2b4b025 100644
--- a/internal/sheets/grants.go
+++ b/internal/sheets/grants.go
@@ -183,10 +183,22 @@ func (a *App) grant(sheetID uint64, address string, role SheetRole, by Member) (
// their role rather than failing on the unique index, which is what
// a person doing it expects.
g := Grant{SheetID: sheetID, MemberID: m.ID, Role: string(role), GrantedBy: by.ID}
- if err := a.db.Clauses(clause.OnConflict{
- Columns: []clause.Column{{Name: "sheet_id"}, {Name: "member_id"}},
- DoUpdates: clause.AssignmentColumns([]string{"role", "granted_by", "updated_at"}),
- }).Create(&g).Error; err != nil {
+ // One transaction, because the check and the write are the same
+ // decision. Scoped to tx throughout, never the outer handle
+ // (AGENTS.md) — the one-connection writer pool deadlocks otherwise.
+ if err := a.db.Transaction(func(tx *gorm.DB) error {
+ going, err := lastOwnerGoing(tx, sheetID, m.ID, role)
+ if err != nil {
+ return err
+ }
+ if going {
+ return errLastOwner
+ }
+ return tx.Clauses(clause.OnConflict{
+ Columns: []clause.Column{{Name: "sheet_id"}, {Name: "member_id"}},
+ DoUpdates: clause.AssignmentColumns([]string{"role", "granted_by", "updated_at"}),
+ }).Create(&g).Error
+ }); err != nil {
return Member{}, err
}
return m, nil
@@ -199,8 +211,22 @@ func (a *App) revoke(sheetID, memberID uint64) error {
if a.db == nil {
return errNoDB
}
- return a.db.Where("sheet_id = ? AND member_id = ?", sheetID, memberID).
- Delete(&Grant{}).Error
+ // The orphan check lives in here, inside the transaction that does
+ // the delete, rather than in the handler above it. A handler-side
+ // check is a read separated from its write by a request's worth of
+ // time: two owners revoking each other at once both saw two owners
+ // and both deleted.
+ return a.db.Transaction(func(tx *gorm.DB) error {
+ going, err := lastOwnerGoing(tx, sheetID, memberID, SheetViewer)
+ if err != nil {
+ return err
+ }
+ if going {
+ return errLastOwner
+ }
+ return tx.Where("sheet_id = ? AND member_id = ?", sheetID, memberID).
+ Delete(&Grant{}).Error
+ })
}
// Admits is the instance's admission gate: who may hold a session here.
diff --git a/internal/sheets/grants_orphan_test.go b/internal/sheets/grants_orphan_test.go
new file mode 100644
index 0000000..747ea6a
--- /dev/null
+++ b/internal/sheets/grants_orphan_test.go
@@ -0,0 +1,94 @@
+package sheets
+
+import (
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "strings"
+ "testing"
+)
+
+// A sheet can never be left without an owner — through ANY door.
+//
+// TestTheLastOwnerCannotBeRemoved already covers revoke. This covers the
+// other one, which was open: `grant` upserts on (sheet_id, member_id),
+// so re-sharing with somebody who already has access CHANGES their role.
+// Nothing counted owners on that path.
+//
+// So the sole owner typing their own address into the share form and
+// picking "viewer" demoted themselves. Zero owner grants remain, mayShare
+// is false for everybody, and — as the revoke path's own comment says —
+// there is no admin path to repair it. One request, no undo, sheet
+// permanently unshareable.
+//
+// It is a one-line form submission, not a contrived sequence, which is
+// what makes it worth a test rather than a note.
+func TestAnOwnerCannotDemoteThemselvesToNobody(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Only owner")
+
+ form := url.Values{}
+ form.Set("address", "paul@example.com")
+ form.Set("role", string(SheetViewer))
+ req := httptest.NewRequest(http.MethodPost,
+ fmt.Sprintf("/sheets/%d/share", s.ID), strings.NewReader(form.Encode()))
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ rec := do(t, app, as(req, cookies))
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("share POST = %d, want 303", rec.Code)
+ }
+
+ // Whatever the screen says, the sheet must still have an owner.
+ people, err := app.grantsOn(s.ID)
+ if err != nil {
+ t.Fatal(err)
+ }
+ owners := 0
+ for _, p := range people {
+ if p.Role == SheetOwner {
+ owners++
+ }
+ }
+ if owners == 0 {
+ t.Fatal("the only owner demoted themselves: the sheet now has no " +
+ "owner, nobody can ever share it again, and there is no path " +
+ "to repair it")
+ }
+
+ // And the screen must still be reachable by them.
+ rec = do(t, app, as(httptest.NewRequest(http.MethodGet,
+ fmt.Sprintf("/sheets/%d/share", s.ID), nil), cookies))
+ if rec.Code != http.StatusOK {
+ t.Fatalf("the owner can no longer open the share screen: %d", rec.Code)
+ }
+}
+
+// Demoting one of two owners is fine, and must stay fine — the guard
+// above must refuse the LAST owner, not every demotion.
+func TestOneOfTwoOwnersCanBeDemoted(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Two owners")
+ guestOn(t, app, s.ID, "ada@elsewhere.example", SheetOwner)
+
+ form := url.Values{}
+ form.Set("address", "ada@elsewhere.example")
+ form.Set("role", string(SheetEditor))
+ req := httptest.NewRequest(http.MethodPost,
+ fmt.Sprintf("/sheets/%d/share", s.ID), strings.NewReader(form.Encode()))
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ if rec := do(t, app, as(req, cookies)); rec.Code != http.StatusSeeOther {
+ t.Fatalf("share POST = %d, want 303", rec.Code)
+ }
+
+ people, err := app.grantsOn(s.ID)
+ if err != nil {
+ t.Fatal(err)
+ }
+ for _, p := range people {
+ if p.Member.Address == "ada@elsewhere.example" && p.Role != SheetEditor {
+ t.Errorf("ada is %q, want editor: demoting a second owner is "+
+ "allowed and the guard has been drawn too wide", p.Role)
+ }
+ }
+}
diff --git a/internal/sheets/handlers_share.go b/internal/sheets/handlers_share.go
index 599151c..dcd2795 100644
--- a/internal/sheets/handlers_share.go
+++ b/internal/sheets/handlers_share.go
@@ -1,6 +1,7 @@
package sheets
import (
+ "errors"
"net/http"
"strconv"
"time"
@@ -130,6 +131,14 @@ func (a *App) handleShareGrant(w http.ResponseWriter, r *http.Request) {
return
}
if _, err := a.grant(s.ID, r.FormValue("address"), role, viewer); err != nil {
+ // Demoting the last owner is refused, and says so. Told
+ // "that does not look like an email address" instead, a person
+ // would retype their own address and try again — the one
+ // response that cannot help.
+ if errors.Is(err, errLastOwner) {
+ a.shareRedirect(w, r, s, "last-owner")
+ return
+ }
// The address is not echoed back. It is somebody's email, and
// an error string is a log line waiting to happen.
a.shareRedirect(w, r, s, "bad-address")
@@ -152,11 +161,16 @@ func (a *App) handleShareRevoke(w http.ResponseWriter, r *http.Request) {
// A sheet with no owner is a sheet nobody can ever share again, and
// there is no admin path to repair it. Refusing the last one is the
// difference between a mistake and an unrecoverable mistake.
- if a.wouldOrphan(s.ID, memberID) {
- a.shareRedirect(w, r, s, "last-owner")
- return
- }
+ //
+ // The check is inside revoke's own transaction rather than here.
+ // Here it was a read separated from its delete by the rest of this
+ // handler, so two owners revoking each other at the same moment both
+ // saw two owners and both deleted.
if err := a.revoke(s.ID, memberID); err != nil {
+ if errors.Is(err, errLastOwner) {
+ a.shareRedirect(w, r, s, "last-owner")
+ return
+ }
a.fail(w, r, err)
return
}
diff --git a/internal/sheets/handlers_share_test.go b/internal/sheets/handlers_share_test.go
index 617a3f4..e45a42c 100644
--- a/internal/sheets/handlers_share_test.go
+++ b/internal/sheets/handlers_share_test.go
@@ -245,9 +245,15 @@ func TestTheShareFormOffersViewerFirst(t *testing.T) {
body := do(t, app,
as(httptest.NewRequest(http.MethodGet, sharePath(s), nil), cookies)).Body.String()
- i := strings.Index(body, `name="role"`)
+ // id="role", NOT name="role". There are two selects named "role" on
+ // this page — the person share and the link share — and the link
+ // one also leads with viewer. Anchored on the name, a reordering of
+ // the two forms would silently retarget this test at the select
+ // that happens to be right, and it would keep passing while the
+ // person default regressed to owner.
+ i := strings.Index(body, `id="role"`)
if i < 0 {
- t.Fatal("no role select on the share screen")
+ t.Fatal("no person-share role select on the share screen")
}
first := strings.Index(body[i:], "<option")
if first < 0 {
@@ -261,4 +267,17 @@ func TestTheShareFormOffersViewerFirst(t *testing.T) {
t.Errorf("the first role option is %q, want the viewer one: the "+
"browser selects the first option, so this is the default grant", opt)
}
+
+ // The other way the default moves. First-option-wins only holds
+ // while nothing is marked selected; one `selected` on a later
+ // option overrides it, and every assertion above would still pass.
+ sel := body[i:]
+ if end := strings.Index(sel, "</select>"); end >= 0 {
+ sel = sel[:end]
+ }
+ if strings.Contains(sel, "selected") {
+ t.Errorf("the role select marks an option selected: that beats "+
+ "first-option-wins, so the default is no longer the first "+
+ "entry of shareableRoles.\n%s", sel)
+ }
}
diff --git a/internal/sheets/handlers_table.go b/internal/sheets/handlers_table.go
index 7f2e9b8..cc2a848 100644
--- a/internal/sheets/handlers_table.go
+++ b/internal/sheets/handlers_table.go
@@ -51,6 +51,11 @@ type tableRow struct {
// tableView is the page's payload.
type tableView struct {
+ // MayWrite gates every claim and affordance only an editor can act
+ // on. handleTable admits anyone who may READ, while
+ // handleTableWrite 404s a viewer — so without it the page told
+ // every reader the cells were editable, then refused them.
+ MayWrite bool
Sheet SheetSummary
Rows []tableRow
Cols []string
@@ -83,6 +88,13 @@ func (a *App) handleTable(w http.ResponseWriter, r *http.Request) {
}
defer h.Release()
+ // The viewer may be absent: a signed-out reader can reach a sheet
+ // through a share link, and that reader can never write.
+ mayWrite := false
+ if actor, okActor := a.actor(r); okActor {
+ mayWrite = a.mayWrite(s.ID, actor)
+ }
+
page := intParam(r, "page")
if page < 1 {
page = 1
@@ -92,6 +104,14 @@ func (a *App) handleTable(w http.ResponseWriter, r *http.Request) {
page = pages
}
editR, editC := parseEdit(r.URL.Query().Get("edit"))
+ if !mayWrite {
+ // Read-only expressed as a coordinate no cell can equal, which
+ // is the same shape the anonymous link view uses. A viewer or
+ // commenter who types ?edit=0,0 by hand gets the plain table
+ // rather than a form that would 404 on submit, and the template
+ // is not trusted to remember the rule.
+ editR, editC = -1, -1
+ }
first := (page - 1) * tableRowsPerPage
@@ -99,6 +119,7 @@ func (a *App) handleTable(w http.ResponseWriter, r *http.Request) {
Sheet: s, Page: page, Pages: pages,
PrevPage: page - 1, NextPage: page + 1,
HasPrev: page > 1, HasNext: page < pages,
+ MayWrite: mayWrite,
}
for c := 0; c < tableCols; c++ {
view.Cols = append(view.Cols, colLabel(c))
diff --git a/internal/sheets/handlers_table_viewer_test.go b/internal/sheets/handlers_table_viewer_test.go
new file mode 100644
index 0000000..6570700
--- /dev/null
+++ b/internal/sheets/handlers_table_viewer_test.go
@@ -0,0 +1,69 @@
+package sheets
+
+import (
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "strings"
+ "testing"
+)
+
+// A viewer is not offered an edit affordance the server would refuse.
+//
+// handleTable admits anyone who may READ — sheetFromURL takes any role —
+// while handleTableWrite 404s a viewer or a commenter. So the page told
+// every reader "every cell here is editable", gave them a link per cell
+// that opened a form, and answered the submit with a 404. Offering an
+// affordance and then refusing it is worse than not offering it.
+//
+// The claim, the anchors and the ?edit= form are all gated now, and the
+// gate is a coordinate no cell can equal rather than a flag the template
+// is trusted to honour — the same shape the anonymous link view uses, so
+// the read-only view cannot grow an edit form by somebody adding a
+// branch to the template.
+//
+// The last assertion is the one that keeps this honest: an owner must
+// still get everything, or a gate that is simply stuck closed would pass
+// every check above.
+func TestTheTableViewOffersAViewerNothingToEdit(t *testing.T) {
+ app, cookies := signIn(t)
+ s, _ := newSheet(t, app, cookies, "Read only")
+ guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer)
+ guestCookies := signInAs(t, app, "ada@elsewhere.example")
+
+ path := fmt.Sprintf("/sheets/%d/table", s.ID)
+ rec := do(t, app, as(httptest.NewRequest(http.MethodGet, path, nil), guestCookies))
+ if rec.Code != http.StatusOK {
+ t.Fatalf("a viewer cannot open the table view: %d", rec.Code)
+ }
+ body := rec.Body.String()
+
+ if strings.Contains(body, "editable") {
+ t.Error("the table view tells a viewer the cells are editable, and " +
+ "the write route answers that viewer with a 404")
+ }
+ if strings.Contains(body, "edit%3D") || strings.Contains(body, "edit=") {
+ t.Error("the table view gives a viewer per-cell edit links")
+ }
+
+ // Asking for the form by hand does not produce one either.
+ forced := path + "?edit=0,0"
+ rec = do(t, app, as(httptest.NewRequest(http.MethodGet, forced, nil), guestCookies))
+ if rec.Code != http.StatusOK {
+ t.Fatalf("GET the table view with an edit coordinate as a viewer = %d", rec.Code)
+ }
+ if strings.Contains(rec.Body.String(), `name="raw"`) {
+ t.Error("an edit coordinate handed a viewer a cell form; read-only " +
+ "must be expressed in the data, not left to the template")
+ }
+
+ // The owner still gets all of it, or the gate is too wide.
+ rec = do(t, app, as(httptest.NewRequest(http.MethodGet, path, nil), cookies))
+ owner := rec.Body.String()
+ if !strings.Contains(owner, "editable") {
+ t.Error("the owner lost the editable claim too — the gate is too wide")
+ }
+ if !strings.Contains(owner, "edit=") && !strings.Contains(owner, "edit%3D") {
+ t.Error("the owner lost the edit links too — the gate is too wide")
+ }
+}
diff --git a/internal/sheets/lastowner.go b/internal/sheets/lastowner.go
new file mode 100644
index 0000000..5ee9ca1
--- /dev/null
+++ b/internal/sheets/lastowner.go
@@ -0,0 +1,57 @@
+package sheets
+
+import (
+ "errors"
+
+ "gorm.io/gorm"
+)
+
+// errLastOwner refuses the change that would leave a sheet with nobody
+// who can share it.
+//
+// A sheet with no owner cannot be shared, cannot be un-shared, and
+// cannot be given an owner — mayShare is false for everybody, including
+// the person who did it, and there is no admin path to repair it. That
+// makes it the one state in this package worth an explicit sentinel
+// rather than a boolean: both doors have to refuse it, and both have to
+// say the same thing to the person.
+//
+// The doors are revoke and grant. Only revoke was guarded at first;
+// grant upserts on (sheet_id, member_id), so re-sharing with an address
+// that already had access changed their role and counted nothing. The
+// sole owner picking "viewer" for their own address orphaned the sheet
+// in one form submission.
+var errLastOwner = errors.New("sheets: a sheet needs an owner")
+
+// lastOwnerGoing reports whether changing this member's role away from
+// owner would take the sheet's last owner with it.
+//
+// It runs inside the caller's transaction, and that is the point rather
+// than tidiness: read-then-write outside one is check-then-act, and two
+// owners demoting each other concurrently both counted two, both passed,
+// and both wrote. SQLite serialises the writes; it does not serialise a
+// read against someone else's later write.
+func lastOwnerGoing(tx *gorm.DB, sheetID, memberID uint64, newRole SheetRole) (bool, error) {
+ if newRole == SheetOwner {
+ return false, nil // not going anywhere
+ }
+ var current Grant
+ err := tx.Where("sheet_id = ? AND member_id = ?", sheetID, memberID).
+ First(&current).Error
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ return false, nil // holds nothing, so takes nothing with them
+ }
+ if err != nil {
+ return false, err
+ }
+ if SheetRole(current.Role) != SheetOwner {
+ return false, nil
+ }
+ var owners int64
+ if err := tx.Model(&Grant{}).
+ Where("sheet_id = ? AND role = ?", sheetID, string(SheetOwner)).
+ Count(&owners).Error; err != nil {
+ return false, err
+ }
+ return owners <= 1, nil
+}
diff --git a/internal/sheets/static/sheets.css b/internal/sheets/static/sheets.css
index 132bd44..d5a439e 100644
--- a/internal/sheets/static/sheets.css
+++ b/internal/sheets/static/sheets.css
@@ -202,9 +202,11 @@
* spreadsheet, and this is the view the spec calls the product.
*
* So the cell reads as a cell and the affordance shows on approach. The
- * anchor still fills the cell, so the target is the whole box rather
- * than the few pixels of the glyph — which is also what lets an empty
- * cell be clickable without a &nbsp; standing in for content.
+ * anchor takes the cell's full width and a line of height, so the
+ * target is the whole line rather than the few pixels of the glyph —
+ * which is also what lets an empty cell be clickable without a &nbsp;
+ * standing in for content. It does not stretch to a row made taller by
+ * a wrapped neighbour; that was true of the &nbsp; it replaces too.
*
* :focus-visible carries the same treatment as :hover deliberately.
* Keyboard is the primary way through this view, and a focus ring alone
diff --git a/internal/sheets/templates/comments.html b/internal/sheets/templates/comments.html
index 90b1e1a..fe86cb4 100644
--- a/internal/sheets/templates/comments.html
+++ b/internal/sheets/templates/comments.html
@@ -31,7 +31,7 @@
<button rst-btn="primary" type="submit">Add comment</button>
</div>
</form>
-<p rst-help>Row and column are counted from zero, the way the grid
+<p rst-field-help>Row and column are counted from zero, the way the grid
addresses them: row 0, column 0 is A1.</p>
{{end}}
diff --git a/internal/sheets/templates/history.html b/internal/sheets/templates/history.html
index d185d32..6a7ca3b 100644
--- a/internal/sheets/templates/history.html
+++ b/internal/sheets/templates/history.html
@@ -15,7 +15,8 @@
<form method="post" action="/sheets/{{.Data.Sheet.ID}}/undo">
<button rst-btn="primary" type="submit">Undo my last edit</button>
</form>
-<p rst-help>Undo only ever touches your own edits, one cell at a time. If
+{{/* rst-field-help, not rst-help — see share.html and the guard. */}}
+<p rst-field-help>Undo only ever touches your own edits, one cell at a time. If
somebody has written over that cell since, your undo takes it back — and
you can undo that too.</p>
{{end}}
diff --git a/internal/sheets/templates/share.html b/internal/sheets/templates/share.html
index 396c0b3..53733c5 100644
--- a/internal/sheets/templates/share.html
+++ b/internal/sheets/templates/share.html
@@ -51,8 +51,11 @@ stored, so this page cannot show it to you again.</p>
{{end}}
<h2>Link sharing</h2>
-<p rst-help>Off unless you make one. Anybody holding the link can read
-this sheet without an account — so it is exactly as private as the
+{{/* rst-field-help, not rst-help: rst-help is a 28x28 icon badge and
+ swallows prose. See signin.html and the guard test in
+ internal/guardstest/markup_test.go. */}}
+<p rst-field-help>Off unless you make one. Anybody holding the link can
+read this sheet without an account — so it is exactly as private as the
people you send it to. A link can never edit.</p>
<form rst-form method="post" action="/sheets/{{.Data.Sheet.ID}}/share/link">
diff --git a/internal/sheets/templates/table.html b/internal/sheets/templates/table.html
index ab34b2b..2eb99de 100644
--- a/internal/sheets/templates/table.html
+++ b/internal/sheets/templates/table.html
@@ -7,8 +7,8 @@
The trailing "with no JavaScript at all" went with it: this view is
the product rather than the fallback, and a subtitle that leads with
what the page lacks argues the opposite. */}}
-<p rst-page-header-sub>Page {{.Data.Page}} of {{.Data.Pages}}. Every cell
-here is editable.</p>
+<p rst-page-header-sub>Page {{.Data.Page}} of {{.Data.Pages}}.{{if .Data.MayWrite}}
+Every cell here is editable.{{end}}</p>
</div>
<a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/history">History</a>
<a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/comments">Comments</a>
@@ -55,7 +55,7 @@ Descending</label>
</form>
{{if .Data.View.Active}}
-<p rst-help>This view is sorted or filtered. It changes what everybody
+<p rst-field-help>This view is sorted or filtered. It changes what everybody
sees on this tab, and it changes nothing about the sheet itself — the
rows are where they always were.</p>
{{end}}
@@ -73,6 +73,7 @@ rows are where they always were.</p>
<tbody>
{{$sheet := .Data.Sheet}}
{{$page := .Data.Page}}
+{{$mayWrite := .Data.MayWrite}}
{{range .Data.Rows}}
<tr>
<th scope="row">{{.Number}}</th>
@@ -90,12 +91,19 @@ rows are where they always were.</p>
<input rst-input id="cell-{{.Ref}}" name="raw" type="text" value="{{.Raw}}" autofocus>
<button rst-btn="primary compact" type="submit">Save</button>
</form>
-{{else}}
+{{else if $mayWrite}}
{{/* The link is the affordance: it turns this cell into the form
above. The cell shows its computed value, and the title carries
the input, so a formula is readable without opening it. */}}
<a href="/sheets/{{$sheet.ID}}/table?page={{$page}}&amp;edit={{.R}},{{.C}}"
aria-label="Edit {{.Ref}}"{{if .Raw}} title="{{.Raw}}"{{end}}>{{.Display}}</a>
+{{else}}
+{{/* A viewer or a commenter. No anchor, because the destination would
+ answer their submit with a 404 — offering the affordance and then
+ refusing it is worse than not offering it. The &nbsp; comes back
+ here for the same reason it stays on the link view: with no anchor
+ there is nothing holding an empty cell open. */}}
+{{if .Display}}{{.Display}}{{else}}&nbsp;{{end}}
{{end}}
</td>
{{end}}