amadan
Review #24: Share's script-only buttons stay hidden; a panic through the real router
- calls-shell.css: .calls-share-actions set display:flex, which beat the hidden attribute, so Copy link and Share… showed without script and did nothing; Share… (rst-btn) did the same where the browser cannot share. Browser coverage: scriptless (red before the rule) and chrome (red with the inner rule removed). - TestARoutePanicIsTheErrorPageThroughTheRouter panics inside a real route (the return cookie's clock on /signin/local) through the production router, signed in and out; red with the router's recoverer removed, where the old middleware-only test stayed green.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+81
−0
internal/calls/errors_test.go+50 −0internal/calls/static/calls-shell.css+6 −0test/browser/call.mjs+25 −0
CI passed — run details
CI log
=== vet === go vet ./... === fmt === === test === go test ./... ok amadan.net/oficina/calls/callscore (cached) ? amadan.net/oficina/calls/cmd/calls [no test files] ok amadan.net/oficina/calls/internal/calls 1.515s ok amadan.net/oficina/calls/internal/calls/apns (cached) ok amadan.net/oficina/calls/internal/calls/hub (cached) ? amadan.net/oficina/calls/internal/calls/icons [no test files] ok amadan.net/oficina/calls/internal/calls/relay (cached) ok amadan.net/oficina/calls/internal/calls/seal (cached) ok amadan.net/oficina/calls/internal/callstest 8.525s ? amadan.net/oficina/calls/protocol [no test files] ? amadan.net/oficina/calls/scripts/writeestilo [no test files] === migration-check === go run amadan.net/rastrillo/rastrillo/cmd/rastrillo migration check rastrillo migration check: models and migrations agree === race === CGO_ENABLED=1 go test -race ./internal/... ok amadan.net/oficina/calls/internal/calls 9.786s ok amadan.net/oficina/calls/internal/calls/apns (cached) ok amadan.net/oficina/calls/internal/calls/hub (cached) ? amadan.net/oficina/calls/internal/calls/icons [no test files] ok amadan.net/oficina/calls/internal/calls/relay (cached) ok amadan.net/oficina/calls/internal/calls/seal (cached) ok amadan.net/oficina/calls/internal/callstest 25.110s
diff --git a/internal/calls/errors_test.go b/internal/calls/errors_test.go| index c6034ea..7159919 100644 |
| --- a/internal/calls/errors_test.go |
| +++ b/internal/calls/errors_test.go |
| @@ -1,10 +1,13 @@ |
| package calls |
| import ( |
| + "bytes" |
| + "log/slog" |
| "net/http" |
| "net/http/httptest" |
| "strings" |
| "testing" |
| + "time" |
| ) |
| // A panic inside the router is the 500 page with a reference and the |
| @@ -28,3 +31,50 @@ func TestAPanicIsTheErrorPageWithAReference(t *testing.T) { |
| t.Fatalf("panic for a fetch: %d %q", w.Code, w.Header().Get("Content-Type")) |
| } |
| } |
| + |
| +// Astra on #24, finding 3 (Memoria #21's shape): the test above builds |
| +// recoverToErrorPage by hand, so taking it out of the router left it |
| +// green. This one panics inside a REAL route — the return cookie's clock, |
| +// read by /signin/local?return_to= — through the production router, |
| +// signed in and out, and checks the 500 page with a reference, the |
| +// viewer's own chrome, and that the panic's value reaches neither the |
| +// page nor the log. |
| +func TestARoutePanicIsTheErrorPageThroughTheRouter(t *testing.T) { |
| + const sentinel = "secret-sentinel-title" |
| + for _, signed := range []bool{true, false} { |
| + f := newHomeRouterFixture(t) |
| + if signed { |
| + f.local(t) |
| + } |
| + var logs bytes.Buffer |
| + f.app.logger = slog.New(slog.NewTextHandler(&logs, nil)) |
| + f.app.store.clk.set(func() time.Time { panic(sentinel) }) |
| + r := httptest.NewRequest("GET", homeTestOrigin+"/signin/local?return_to=%2Fsettings", nil) |
| + r.Header.Set("Accept", "text/html") |
| + for _, c := range f.cookies { |
| + r.AddCookie(c) |
| + } |
| + w := httptest.NewRecorder() |
| + func() { |
| + defer func() { |
| + if v := recover(); v != nil { |
| + t.Fatalf("signed in=%v: the panic escaped the router: %v", signed, v) |
| + } |
| + }() |
| + f.handler.ServeHTTP(w, r) |
| + }() |
| + b := w.Body.String() |
| + if w.Code != http.StatusInternalServerError || !strings.Contains(b, "<title>Something went wrong · Calls</title>") || !strings.Contains(b, "rst-error-ref") { |
| + t.Errorf("signed in=%v: %d, want the 500 page with a reference:\n%s", signed, w.Code, b) |
| + } |
| + if strings.Contains(b, "estilo-account") != signed { |
| + t.Errorf("signed in=%v: the 500 page's chrome is not the viewer's", signed) |
| + } |
| + if strings.Contains(b, sentinel) || strings.Contains(logs.String(), sentinel) { |
| + t.Errorf("signed in=%v: the panic's value reached the page or the log", signed) |
| + } |
| + if !strings.Contains(logs.String(), "ref=") { |
| + t.Errorf("signed in=%v: no reference in the log: %s", signed, logs.String()) |
| + } |
| + } |
| +} |
diff --git a/internal/calls/static/calls-shell.css b/internal/calls/static/calls-shell.css| index 517d6d6..b68de91 100644 |
| --- a/internal/calls/static/calls-shell.css |
| +++ b/internal/calls/static/calls-shell.css |
| @@ -22,6 +22,12 @@ body { background: var(--rst-bg); } |
| .calls-option-help { margin-block: 0.25rem var(--rst-sp-3); color: var(--rst-text-muted); font-size: var(--rst-fs-sm); } |
| .calls-share-link a { overflow-wrap: anywhere; } |
| .calls-share-actions { display: flex; flex-wrap: wrap; gap: var(--rst-sp-2); } |
| +/* The row is script's (share-link.js reveals it): the display above |
| + would otherwise beat the hidden attribute and show two buttons that do |
| + nothing without script (Astra on #24). */ |
| +.calls-share-actions[hidden], .calls-share-actions > [hidden] { display: none; } |
| +/* …and Share… inside it, which share-link.js reveals only where the |
| + browser can share: rst-btn's display beats hidden the same way. */ |
| .calls-link-input { inline-size: 100%; max-inline-size: 44rem; } |
| /* The holding screen: who you'll join as, then the device panel, then |
diff --git a/test/browser/call.mjs b/test/browser/call.mjs| index b753936..bd72271 100644 |
| --- a/test/browser/call.mjs |
| +++ b/test/browser/call.mjs |
| @@ -5007,6 +5007,15 @@ const run = { |
| check(holding.cls < 0.01, 'the holding screen holds still while its devices arrive (CLS ' + holding.cls.toFixed(4) + ' ' + JSON.stringify(holding.shifts) + ')'); |
| check(await page.locator('header[estilo-bar]').count() === 1, 'the holding screen wears the estilo bar'); |
| + // With script, Copy link shows, and Share… only where the browser |
| + // can share: rst-btn's display used to beat its hidden attribute. |
| + const [share] = await Promise.all([ctx.waitForEvent('page'), page.click('form[action$="/share"] button')]); |
| + await share.waitForLoadState('load'); |
| + const canShare = await share.evaluate(() => !!navigator.share); |
| + check(await share.locator('#copy-invitation').isVisible() && (await share.locator('#share-invitation').isVisible()) === canShare, |
| + 'the Share page shows Copy link, and Share… only where the browser can share (navigator.share ' + canShare + ')'); |
| + await share.close(); |
| + |
| await Promise.all([page.waitForURL('**/m/' + sqid), page.click('#join')]); |
| await page.waitForFunction(() => !!window.meet, null, { timeout: 20000 }); |
| check(await until(async () => (await state(page)).admitted, 20000), 'joined from the holding screen'); |
| @@ -5014,6 +5023,7 @@ const run = { |
| const joined = await cls(); |
| check(joined.cls < 0.01, 'holding screen → room holds still (CLS ' + joined.cls.toFixed(4) + ' ' + JSON.stringify(joined.shifts) + ')'); |
| + |
| // The title menu: the account menu's order and words. |
| await page.mouse.move(200, 200); |
| await page.click('#call-nav > summary'); |
| @@ -5521,6 +5531,21 @@ const run = { |
| check(await holding.locator('#devices').isHidden(), |
| 'the holding screen keeps its device controls hidden with no script'); |
| + // Share link works with no script (a POST into a new tab), and the |
| + // page it opens shows the link and nothing that needs script: Copy |
| + // link and Share… are script's, hidden until share-link.js reveals |
| + // them. A display rule on their row used to beat the hidden |
| + // attribute, leaving two buttons that did nothing (Astra on #24). |
| + const [share] = await Promise.all([ |
| + ctx.waitForEvent('page'), |
| + holding.click('form[action$="/share"] button'), |
| + ]); |
| + await share.waitForLoadState('domcontentloaded'); |
| + check(await share.locator('#invitation-link').isVisible(), 'no script: the Share page shows the invitation link (at ' + where(share) + ')'); |
| + check(await share.locator('#share-actions').isHidden() && await share.locator('#copy-invitation').isHidden(), |
| + 'no script: the Share page shows no Copy link or Share… buttons'); |
| + await share.close(); |
| + |
| const room = await ctx.newPage(); |
| await room.goto(origin + '/m/' + sqid, { waitUntil: 'domcontentloaded' }); |
| const roomText = await room.locator('body').innerText(); |