amadan
feat: the homepage is the entry page, not a signpost
Signed in, "/" rendered a line naming you and a button reading "Open your sheets" — a page whose entire content was a link to the next page. That was correct when it was written and stopped being correct the day /sheets landed. auth redirects a fresh session to SignedInPath, which defaults to "/", so every magic-link click arrived here; the page had to say something, because the first real click landed on a version that said there was nothing to reach at this address, and a sign-in that looks identical to a failed one is the bug worth fixing. The button was that fix. Nobody went back to collapse the hop once there was somewhere to hop to. Both halves are closed, and neither alone is enough: auth is now configured with SignedInPath "/sheets" so a click never comes through here, and handleIndex redirects the person who later types the bare address. "/" keeps the anonymous state — the way in, and the callout about what a live host can read. Nothing is lost by redirecting, which is what made this safe: the layout's account menu already carries the address, the sign-out form and the theme switcher on every signed-in screen, so the landing page names you at least as clearly as the signpost did. Two tests used "/" as a stand-in for "a signed-in page" — the affordance gate and the mode switcher. Both check layout chrome rather than anything about the root, so both moved to /sheets. The end-to-end sign-in test keeps its original question, asked of the page you actually land on: can the person who clicked tell it worked?
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqAkePcWa7dNbXFr91nwLv
6 files changed,
+109
−27
internal/sheets/auth.go+7 −2internal/sheets/handlers_table_test.go+4 −2internal/sheets/page.go+25 −10internal/sheets/page_test.go+61 −4internal/sheets/templates/index.html+8 −8internal/sheets/theme_test.go+4 −1
CI passed — run details
diff --git a/internal/sheets/auth.go b/internal/sheets/auth.go| index 63a0477..4d4303c 100644 |
| --- a/internal/sheets/auth.go |
| +++ b/internal/sheets/auth.go |
| @@ -151,8 +151,13 @@ func NewAuth(db *sql.DB, env AuthEnv, logger *slog.Logger, authorize func(string |
| Mailer: env.Mailer, |
| Authorize: authorize, |
| SigninPath: "/signin", |
| - Subject: "Your Sheets sign-in link", |
| - Logger: logger, |
| + // Where a fresh session lands. auth defaults this to "/", |
| + // which is the anonymous entry page — so without this line |
| + // every magic-link click arrives on a page that exists to |
| + // offer sign-in, and has to be told it worked. |
| + SignedInPath: "/sheets", |
| + Subject: "Your Sheets sign-in link", |
| + Logger: logger, |
| }) |
| } |
diff --git a/internal/sheets/handlers_table_test.go b/internal/sheets/handlers_table_test.go| index 61d06d6..c6d4dad 100644 |
| --- a/internal/sheets/handlers_table_test.go |
| +++ b/internal/sheets/handlers_table_test.go |
| @@ -126,8 +126,10 @@ func TestEveryPageWorksWithoutScript(t *testing.T) { |
| path string |
| want []string |
| }{ |
| - {"/", []string{`action="/signout"`}}, |
| - {"/sheets", []string{`action="/sheets"`, `name="title"`}}, |
| + // Sign-out is checked on /sheets rather than on "/": it is |
| + // layout chrome, present on every signed-in page, and "/" |
| + // stopped being one when it started redirecting onward. |
| + {"/sheets", []string{`action="/sheets"`, `name="title"`, `action="/signout"`}}, |
| {fmt.Sprintf("/sheets/%d", s.ID), []string{fmt.Sprintf("/sheets/%d/table", s.ID)}}, |
| {fmt.Sprintf("/sheets/%d/table", s.ID), []string{"?page=1&edit=0,0", "Next rows"}}, |
| {fmt.Sprintf("/sheets/%d/table?page=1&edit=0,0", s.ID), |
diff --git a/internal/sheets/page.go b/internal/sheets/page.go| index aa018b7..3687ca8 100644 |
| --- a/internal/sheets/page.go |
| +++ b/internal/sheets/page.go |
| @@ -2,18 +2,33 @@ package sheets |
| import "net/http" |
| -// The root page has two states, and the signed-in one is the whole |
| -// point: a magic link lands here, so if this page cannot say "you are |
| -// signed in, as this address", then a successful sign-in and a silently |
| -// failed one look identical to the person who clicked. That is exactly |
| -// what happened the first time a link was clicked here — auth's |
| -// SignedInPath defaults to "/", and this page told the arriving user |
| -// there was nothing to reach at this address. |
| +// The root page is the anonymous entry page, and nothing else. |
| // |
| -// It was assembled from string constants until the view layer arrived; |
| -// it is a template now, and the reasoning above is the only part worth |
| -// keeping from that version. |
| +// It used to have a signed-in state too, and that state was the whole |
| +// point of it: a magic link lands here, so if the page could not say |
| +// "you are signed in, as this address", then a successful sign-in and a |
| +// silently failed one looked identical to the person who clicked. That |
| +// is exactly what happened the first time a link was clicked here — |
| +// auth's SignedInPath defaults to "/", and this page told the arriving |
| +// user there was nothing to reach at this address. |
| +// |
| +// The fix at the time was a line naming you and a button reading "Open |
| +// your sheets", because /sheets did not exist yet. Once it did, that |
| +// state was a page whose entire content was a link to the next page, |
| +// and the layout's account menu names you on every screen anyway. So |
| +// the signed-in half is a redirect now, and the original requirement is |
| +// met better: you land on your sheets, which could not be mistaken for |
| +// a failed sign-in. |
| +// |
| +// Both halves of the hop are closed — auth is configured with |
| +// SignedInPath "/sheets" so a fresh click never comes through here at |
| +// all, and this redirect covers somebody who later types the bare |
| +// address. Neither alone is enough. |
| func (a *App) handleIndex(w http.ResponseWriter, r *http.Request) { |
| + if _, ok := a.identity(r); ok { |
| + http.Redirect(w, r, "/sheets", http.StatusSeeOther) |
| + return |
| + } |
| a.render(w, r, "index", "Oficina Sheets", http.StatusOK, nil) |
| } |
diff --git a/internal/sheets/page_test.go b/internal/sheets/page_test.go| index 17ac33a..c99a5c3 100644 |
| --- a/internal/sheets/page_test.go |
| +++ b/internal/sheets/page_test.go |
| @@ -109,17 +109,37 @@ func TestSignInEndToEnd(t *testing.T) { |
| verify.Code, verify.Header().Get("Location")) |
| } |
| - // 4. The root page now says who you are, and offers a way out. |
| + // 4. The root page sends you on to your sheets rather than |
| + // rendering a signpost, and that page says who you are. |
| + // |
| + // This assertion used to read the root page itself, because the |
| + // root page was the whole proof that a click had worked. It is |
| + // the same proof either way: the question is whether the person |
| + // who clicked can tell they are signed in, and the page they |
| + // actually land on is the one that has to answer it. |
| after := httptest.NewRecorder() |
| req := httptest.NewRequest(http.MethodGet, "/", nil) |
| for _, c := range cookies { |
| req.AddCookie(c) |
| } |
| app.Router().ServeHTTP(after, req) |
| + if after.Code != http.StatusSeeOther { |
| + t.Fatalf("GET / signed in = %d, want 303", after.Code) |
| + } |
| + if loc := after.Header().Get("Location"); loc != "/sheets" { |
| + t.Fatalf("GET / signed in redirects to %q, want /sheets", loc) |
| + } |
| + |
| + landing := httptest.NewRecorder() |
| + onward := httptest.NewRequest(http.MethodGet, "/sheets", nil) |
| + for _, c := range cookies { |
| + onward.AddCookie(c) |
| + } |
| + app.Router().ServeHTTP(landing, onward) |
| - body := after.Body.String() |
| - if !strings.Contains(body, "Signed in as") || !strings.Contains(body, "paul@example.com") { |
| - t.Errorf("root page after sign-in does not name the user:\n%s", body) |
| + body := landing.Body.String() |
| + if !strings.Contains(body, "paul@example.com") { |
| + t.Errorf("page after sign-in does not name the user:\n%s", body) |
| } |
| if !strings.Contains(body, `action="/signout"`) { |
| t.Error("no way to sign out") |
| @@ -129,6 +149,43 @@ func TestSignInEndToEnd(t *testing.T) { |
| } |
| } |
| +// TestSignedInRootIsNotASignpost: the root page had two states, and the |
| +// signed-in one was a button reading "Open your sheets" — a page whose |
| +// entire content for a signed-in person was a link to the next page. |
| +// That existed because /sheets did not when the root page was written, |
| +// and auth lands a fresh session on SignedInPath, which defaults to "/". |
| +// Both halves are fixed: auth lands on /sheets, and anyone who types the |
| +// bare address is sent there too. |
| +func TestSignedInRootIsNotASignpost(t *testing.T) { |
| + app, cookies := signIn(t) |
| + |
| + rec := httptest.NewRecorder() |
| + req := httptest.NewRequest(http.MethodGet, "/", nil) |
| + for _, c := range cookies { |
| + req.AddCookie(c) |
| + } |
| + app.Router().ServeHTTP(rec, req) |
| + |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("GET / signed in = %d, want 303", rec.Code) |
| + } |
| + if body := rec.Body.String(); strings.Contains(body, "Open your sheets") { |
| + t.Error("root page still renders the signpost") |
| + } |
| +} |
| + |
| +// TestAnonymousRootStillOffersAWayIn: the redirect above must not cost |
| +// the anonymous visitor the only page that offers sign-in. |
| +func TestAnonymousRootStillOffersAWayIn(t *testing.T) { |
| + rec := get(t, "/") |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("GET / anonymous = %d, want 200", rec.Code) |
| + } |
| + if !strings.Contains(rec.Body.String(), `<a href="/signin">`) { |
| + t.Error("anonymous root page offers no way in") |
| + } |
| +} |
| + |
| // TestSignInIsSingleUse: the same link must not work twice. |
| func TestSignInIsSingleUse(t *testing.T) { |
| app, mail := appWithRecorder(t) |
diff --git a/internal/sheets/templates/index.html b/internal/sheets/templates/index.html| index 17a1c9c..13fc274 100644 |
| --- a/internal/sheets/templates/index.html |
| +++ b/internal/sheets/templates/index.html |
| @@ -6,16 +6,16 @@ |
| </div> |
| </header> |
| -{{if .SignedIn}} |
| -{{/* The signed-in state is the whole point of this page: a magic link |
| - lands here, so if it cannot say who you are, a successful sign-in |
| - and a silently failed one look identical. */}} |
| -<p>Signed in as <strong>{{.Address}}</strong>.</p> |
| -<p><a rst-btn="primary" href="/sheets">Open your sheets</a></p> |
| -{{else}} |
| +{{/* One state only. This page used to branch on .SignedIn, because a |
| + magic link landed here and a page that could not say who you are |
| + made a successful sign-in and a silently failed one look identical. |
| + That branch was a line naming you and a button to /sheets, written |
| + before /sheets existed. handleIndex redirects a signed-in visitor |
| + there now and auth lands them there directly, so this page is only |
| + ever rendered for somebody who is not signed in — and the layout's |
| + account menu is what names you, on every screen. */}} |
| <p>You are not signed in. <a href="/signin">Sign in</a> — we email you a |
| link instead of asking for a password.</p> |
| -{{end}} |
| <div rst-callout> |
| <div rst-callout-body> |
diff --git a/internal/sheets/theme_test.go b/internal/sheets/theme_test.go| index 9a39ab7..c2c7cdc 100644 |
| --- a/internal/sheets/theme_test.go |
| +++ b/internal/sheets/theme_test.go |
| @@ -171,8 +171,11 @@ func TestModeReturnIsNotAnOpenRedirect(t *testing.T) { |
| // The switcher is chrome, and chrome that needs JavaScript is chrome |
| // somebody who needs dark mode cannot reach. |
| func TestModeSwitcherIsAForm(t *testing.T) { |
| + // Asked of /sheets, not of "/": the switcher lives in the layout's |
| + // account menu, so any signed-in page proves it, and "/" is no |
| + // longer one — it redirects a signed-in visitor onward. |
| app, cookies := signIn(t) |
| - body := do(t, app, as(httptest.NewRequest(http.MethodGet, "/", nil), cookies)).Body.String() |
| + body := do(t, app, as(httptest.NewRequest(http.MethodGet, "/sheets", nil), cookies)).Body.String() |
| if !strings.Contains(body, `action="/mode"`) { |
| t.Fatal("the signed-in page offers no display-mode form") |
| } |