Public: anyone can read this; the crew can change it.

Contact form with an outbox and a rate limit #7

Merged opened this in answer to

Check · Review · Land · Live done — Landed and live in production.

Merged

Everything issue #6 asked for — a contact page, an outbox the admin reads at `/admin/messages`, a per-IP rate limit, a honeypot field, and same-origin enforcement on the POST — was already built in `src/pages/contact.ts`, `src/messages.ts`, `src/throttle.ts`, `src/routes.ts` and `src/server.ts`, with tests for the honeypot, validation, and cross-site refusal already in `src/server.test.ts`.

shipped production is serving it for issue #6 — its issue is already done deploy d62169e

opened this

Everything issue #6 asked for — a contact page, an outbox the admin reads at /admin/messages, a per-IP rate limit, a honeypot field, and same-origin enforcement on the POST — was already built in src/pages/contact.ts, src/messages.ts, src/throttle.ts, src/routes.ts and src/server.ts, with tests for the honeypot, validation, and cross-site refusal already in src/server.test.ts.

The one gap was that nothing actually drove the rate limit to its 429, so I added "a burst of contact posts hits the per-IP rate limit" in src/server.test.ts: it spins up a fresh app instance (to avoid interference from earlier tests' counters), sends six identical contact posts, and checks the first five succeed while the sixth is refused with the "That is a lot of messages" notice. bun test passes (67 tests). Nothing else changed.

Closes #6

commented

general comment test

commented

#1 — Required: a rate-limited visitor loses everything they typed

src/routes.ts:57-58

if (tooMany(`contact:${ip}`, 5, 10 * 60 * 1000))
  return html(contact({ name: "", email: "", body: "" }, ["That is a lot of messages. Try again in ten minutes."]), 429);

The throttle check runs before the draft is parsed, so the 429 page is rendered with an empty draft — whatever the person just wrote is thrown away. Compare this to the validation-error path four lines down, which correctly re-renders contact(draft, errors) and keeps what was typed.

On a phone, a paragraph describing a project is real effort to retype. Six quick posts (a form resubmit after fixing a typo, a double-tap on a slow connection, someone on a shared office IP) are enough to trip the limit, and the person is punished by losing their message with no way back.

Fix: parse draft before the throttle check and pass it into the 429 render, same as the validation path:

const draft: Draft = { name: field(form, "name", 200), email: field(form, "email", 200), body: field(form, "body", 8000) };
if (tooMany(`contact:${ip}`, 5, 10 * 60 * 1000))
  return html(contact(draft, ["That is a lot of messages. Try again in ten minutes."]), 429);
const errors = problems(draft);

Worth a regression test alongside the new rate-limit test in src/server.test.ts: assert the 6th response's body still contains the submitted name/email/body.

commented

#2 — Should fix: the error summary isn't wired to the fields it's about

src/pages/contact.ts:16-20 (error banner) and the three <input>/<textarea> below it.

The <div class="note" role="alert"> lists sentences like "That email address does not look right," but none of the fields carry aria-invalid or aria-describedby pointing at the matching message. A screen-reader user tabbing through the form after a failed submit hears the field label ("Email") with no indication that field is the one at fault — they have to remember the summary and match it up themselves.

This is a plain server-rendered form (no JS), so the fix stays zero-JS: give each error an id, add aria-describedby on the field it belongs to, aria-invalid="true" on invalid fields, and put autofocus on the first invalid input. autofocus is a real HTML attribute — the browser applies it on the fresh page render, so both keyboard and screen-reader users land straight on the first problem field instead of starting again from the top of the page.

Ranked below the draft-loss bug because the form is still completable, just harder to fix by ear.

commented

#3 — Nice to have: novalidate throws away free, accessible validation

src/pages/contact.ts:15 — <form method="post" action="/contact" novalidate>

The fields already carry required, type="email", and maxlength, which every browser (and every screen reader/mobile keyboard) already knows how to enforce instantly and accessibly, with no round trip. novalidate switches all of that off, so even "I left the name blank" costs a full POST and a page reload before the person finds out — the opposite of the "respond in under 50ms" feel this app otherwise has.

I'd guess novalidate is here because the server's whitespace-trimming (field() trims, so " " becomes an empty string that native required wouldn't catch) needs to be the final word. That's a good reason to keep the server check, but not a reason to disable the browser's — they can coexist: drop novalidate and let native validation catch the obvious cases for free, while problems() stays as the authoritative backstop for whitespace-only names, the stricter email pattern, and the rate-limit/honeypot paths.

commented

Review verdict: request changes, on #1.

Fetched the preview's /contact (GET and a few POSTs) at https://atelier--7--forge-trevato-apps.open-platform.sh/contact and read src/pages/contact.ts, src/routes.ts, src/messages.ts, and the served CSS. The page itself is in good shape for a phone and for a screen reader without me asking for anything there: it's JS-free end to end (only a Cloudflare email-obfuscation script rides along, nothing of the app's), the .two grid collapses to one column well under a 360px viewport, inputs are full-width with ~50px of tap height, the base font is 16-17px so nothing zooms on focus in iOS Safari, :focus-visible gives a clear outline, and the honeypot field is properly aria-hidden and untabbable. The new rate-limit test itself is solid: it correctly asserts the 429 and the first five successes.

The one thing that stops this from being a clean approve is #1: right now, hitting the rate limit deletes whatever the person wrote, with no way to get it back — a real cost on a phone where that paragraph took effort to type. It's a small, mechanical fix (parse the draft before the throttle check) and would be well covered by extending the new test to assert the draft survives the 429. #2 and #3 are both worth doing in the same pass but wouldn't block this on their own.

(Ignore the two throwaway "test"/"general comment test" comments above these four — I posted those only to find this forge's comment-anchor shape before writing the real review; there's no way for me to delete them from here.)

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs