amadan
Review nits from the shared-chrome and rastrillo-path reviews
Fixes for the non-blocking findings on the merged branches (see this branch's discussion). The coordinating session checked them, on Paul's standing instruction to roll in fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Squashed from chore/review-nits into main.
3 files changed,
+109
−5
internal/calendar/static/calendar.css+7 −0internal/calendartest/quickadd_browser_test.go+87 −0internal/calendartest/schedule_recovery_test.go+15 −5
CI passed — run details
CI log
=== vet === go vet ./... go vet -tags browser ./internal/calendartest/ === fmt === === test === go test ./... ? amadan.net/oficina/calendar/cmd/calendar [no test files] ok amadan.net/oficina/calendar/internal/calendar 4.857s ? amadan.net/oficina/calendar/internal/calendar/icons [no test files] ? amadan.net/oficina/calendar/internal/calendar/seal [no test files] ok amadan.net/oficina/calendar/internal/calendartest 2.341s ok amadan.net/oficina/calendar/internal/guardstest 0.055s ? amadan.net/oficina/calendar/scripts/writeestilo [no test files] === migration-check === go tool rastrillo migration check rastrillo migration check: models and migrations agree
diff --git a/internal/calendar/static/calendar.css b/internal/calendar/static/calendar.css| index 9868d72..f1a207c 100644 |
| --- a/internal/calendar/static/calendar.css |
| +++ b/internal/calendar/static/calendar.css |
| @@ -472,6 +472,12 @@ html { scroll-padding-block-start: var(--estilo-bar-block-size); } |
| * shared-chrome spec §11), not a hand-built 6px and shadow of its own. */ |
| border-radius: var(--estilo-float-radius); |
| box-shadow: var(--rst-shadow-pop); |
| + /* Border-box, here and on the textarea below: both are width: 100% |
| + * on a phone, and content-box added the padding and border on top, |
| + * so the open panel ran ~15px past the right edge at 390px and the |
| + * page scrolled sideways. TestQuickAddOnANarrowScreen (make browser) |
| + * measures it. */ |
| + box-sizing: border-box; |
| /* Anchored to the END edge, so the panel opens back across the page |
| * instead of off the side of it. The box sits at the far right of the |
| * header, beside New event: anchored at the start edge a 30rem panel |
| @@ -490,6 +496,7 @@ html { scroll-padding-block-start: var(--estilo-bar-block-size); } |
| background: var(--rst-bg); |
| border: 1px solid var(--rst-line); |
| border-radius: var(--rst-radius-sm); |
| + box-sizing: border-box; |
| color: var(--rst-text); |
| display: block; |
| font: inherit; |
diff --git a/internal/calendartest/quickadd_browser_test.go b/internal/calendartest/quickadd_browser_test.go| new file mode 100644 |
| index 0000000..d7f3fcf |
| --- /dev/null |
| +++ b/internal/calendartest/quickadd_browser_test.go |
| @@ -0,0 +1,87 @@ |
| +//go:build browser |
| + |
| +package calendartest |
| + |
| +import ( |
| + "context" |
| + "io" |
| + "log/slog" |
| + "net/http" |
| + "path/filepath" |
| + "testing" |
| + "time" |
| + |
| + "amadan.net/rastrillo/rastrillo/harness" |
| + "github.com/chromedp/cdproto/emulation" |
| + "github.com/chromedp/cdproto/network" |
| + "github.com/chromedp/chromedp" |
| + |
| + calendar "amadan.net/oficina/calendar/internal/calendar" |
| +) |
| + |
| +// TestQuickAddOnANarrowScreen: on a phone the quick-add panel stops |
| +// floating and drops into the page (calendar.css, max-width 40rem), and |
| +// at 390px it ran about 15px past the right edge of the screen — the |
| +// textarea's and the Add button's right ends were off the side, and the |
| +// page scrolled sideways. Every string check in the Go suite passes a |
| +// panel like that. This opens it at 320 and 390 and fails if anything |
| +// inside it reaches past the viewport, or the page grows wider than the |
| +// screen. |
| +func TestQuickAddOnANarrowScreen(t *testing.T) { |
| + var app *calendar.App |
| + rig := harness.New(t, func(origin string) http.Handler { |
| + a, err := calendar.New(calendar.Config{ |
| + DBPath: filepath.Join(t.TempDir(), "calendar.db"), |
| + Env: "development", |
| + Origin: origin, |
| + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), |
| + }) |
| + if err != nil { |
| + t.Fatalf("calendar.New: %v", err) |
| + } |
| + t.Cleanup(func() { a.Close() }) |
| + app = a |
| + return a.Handler() |
| + }) |
| + session := signInOverHTTP(t, app, rig.Origin, "host@example.com") |
| + |
| + for _, width := range []int64{320, 390} { |
| + var problems []string |
| + ctx, cancel := context.WithTimeout(rig.Context(), 20*time.Second) |
| + err := chromedp.Run(ctx, |
| + network.SetCookie(session.Name, session.Value).WithURL(rig.Origin+"/"). |
| + WithHTTPOnly(session.HttpOnly).WithSameSite(network.CookieSameSiteLax), |
| + emulation.SetDeviceMetricsOverride(width, 844, 1, true), |
| + chromedp.Navigate(rig.Origin+"/"), |
| + chromedp.WaitVisible(`[cal-quick] > summary`, chromedp.ByQuery), |
| + chromedp.Click(`[cal-quick] > summary`, chromedp.ByQuery), |
| + chromedp.WaitVisible(`[cal-quick-text]`, chromedp.ByQuery), |
| + chromedp.Evaluate(quickAddProblems, &problems), |
| + ) |
| + cancel() |
| + if err != nil { |
| + t.Fatalf("%dpx: %v", width, err) |
| + } |
| + for _, p := range problems { |
| + t.Errorf("%dpx, the open quick-add panel: %s", width, p) |
| + } |
| + } |
| +} |
| + |
| +// quickAddProblems lists every visible element of the open quick-add |
| +// panel that reaches past either side of the viewport, and the page |
| +// itself if it scrolls sideways. |
| +const quickAddProblems = `(() => { |
| + const vw = document.documentElement.clientWidth, out = [] |
| + const els = [document.querySelector('[cal-quick]'), ...document.querySelectorAll('[cal-quick] *')] |
| + for (const el of els) { |
| + const r = el.getBoundingClientRect() |
| + if (r.width === 0 && r.height === 0) continue |
| + if (r.right > vw + 0.5 || r.left < -0.5) { |
| + const name = el.tagName.toLowerCase() + [...el.attributes].filter(a => a.name.startsWith('cal-')).map(a => '[' + a.name + ']').join('') |
| + out.push(name + ' spans ' + Math.round(r.left) + '-' + Math.round(r.right) + ' in a ' + vw + 'px viewport') |
| + } |
| + } |
| + if (document.documentElement.scrollWidth > vw) out.push('the page is ' + document.documentElement.scrollWidth + 'px wide in a ' + vw + 'px viewport') |
| + return out |
| +})()` |
diff --git a/internal/calendartest/schedule_recovery_test.go b/internal/calendartest/schedule_recovery_test.go| index 4c2699d..1e1f420 100644 |
| --- a/internal/calendartest/schedule_recovery_test.go |
| +++ b/internal/calendartest/schedule_recovery_test.go |
| @@ -9,16 +9,26 @@ import ( |
| calendar "amadan.net/oficina/calendar/internal/calendar" |
| ) |
| +// The work is in the database before the failing boot, as it is when an |
| +// instance restarts. It used to be inserted straight into the database |
| +// after boot, which raced the retry loop: when the first retry ran before |
| +// the INSERT it found nothing due, succeeded, and nothing woke it again |
| +// (a raw INSERT bypasses the request middleware that wakes it), so about |
| +// one run in ten failed "timer was not armed". |
| func TestTransientControlFailureAtBootRecovers(t *testing.T) { |
| control := controlForTest(t) |
| - control.mu.Lock() |
| - control.fail = true |
| - control.mu.Unlock() |
| - app := calendar.NewForTest(t, t.TempDir()) |
| + dir := t.TempDir() |
| + seed := calendar.NewForTest(t, dir) |
| due := time.Now().Add(time.Hour).Unix() |
| - if _, err := app.DBForTest().Exec(`INSERT INTO reminder_queues (reminder_id,occurrence_key,due_at,attempts,late_by_sec) VALUES (1,'boot-retry',?,0,0)`, due); err != nil { |
| + if _, err := seed.DBForTest().Exec(`INSERT INTO reminder_queues (reminder_id,occurrence_key,due_at,attempts,late_by_sec) VALUES (1,'boot-retry',?,0,0)`, due); err != nil { |
| t.Fatal(err) |
| } |
| + seed.Close() |
| + control.mu.Lock() |
| + control.at = time.Time{} |
| + control.fail = true |
| + control.mu.Unlock() |
| + calendar.NewForTest(t, dir) |
| control.mu.Lock() |
| control.fail = false |
| control.mu.Unlock() |