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

A tell sent from an app's forge toolbar that fails keeps the typed words and says why, in one plain line #11348

Merged opened this

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

Merged

A tell from the forge toolbar that fails now keeps the typed words and gives one plain reason under the textarea. The new tests, the toolbar's other suites, `bun x tsc --noEmit` and `bun scripts/gate.ts` pass. I did not run the whole suite.

shipped production is serving it deploy 3d07d6f

opened this

A tell from the forge toolbar that fails now keeps the typed words and gives one plain reason under the textarea. The new tests, the toolbar's other suites, bun x tsc --noEmit and bun scripts/gate.ts pass. I did not run the whole suite.

What changed

  • Rate limit (429): the line says "Too many notes just now. Your words are still here; try again in N seconds." N comes from the retry-after header.
  • Offline or network failure: the line says "You are offline. Your words are still here; send again when you are back." This shows when navigator.onLine is false. A network failure while the browser reports itself online keeps the existing "That did not go through. Try again."
  • Other refusals: the server's own message is shown, as before, for example when a visitor is banned.
  • Answer that is not JSON: an error page from a proxy no longer skips the line. The reply is read as nothing, so the generic "That did not go through" line shows.
  • Success: unchanged. The field clears and the receipt shows.

The code changes are in src/apps/toolbar.ts (a few lines) and src/apps/note_words.ts (two new constants, NOTE_OFFLINE and NOTE_WAIT). The text is set with textContent, so the reason stays escaped. The tell route, CSP and same-origin check are untouched. The existing test for the toolbar script's 24 KB size limit still passes.

Tests

  • test/apps-toolbar-tell.test.ts now boots the toolbar script against a stubbed fetch. It checks that a 429 names the seconds, a 403 shows the server's reason, a 502 with an unreadable body shows the generic line, and an offline failure shows the offline line. In each case the textarea keeps its text.
  • test/helpers/toolbar-boot.ts gained a no-op removeAttribute so the shared stub can run the send path.
  • I did not confirm that the new tests fail without the change. With src/ stashed, the test file errors on the missing NOTE_OFFLINE import, so the check says nothing about the individual tests.

Not done

  • No "not signed in" line. The toolbar's note box takes anonymous visitors, so a note is never refused for that. The sign-in case belongs to the separate "Tell the crew" link, which opens the forge's own tell page. That page already sends a refused tell back with the words kept.
  • The server's 429 message is not reworded.

To try it, open an app with the forge pill, go offline in devtools and send a note: the words stay and the offline line shows.

🤖 Generated with Claude Code

Closes #11336

commented

A tell from the forge toolbar that fails now keeps the typed words and gives one plain reason under the textarea. The new tests, the toolbar's other suites, bun x tsc --noEmit and bun scripts/gate.ts pass. I did not run the whole suite.

What changed

  • Rate limit (429): the line says "Too many notes just now. Your words are still here; try again in N seconds." N comes from the retry-after header.
  • Offline or network failure: the line says "You are offline. Your words are still here; send again when you are back." This shows when navigator.onLine is false. A network failure while the browser reports itself online keeps the existing "That did not go through. Try again."
  • Other refusals: the server's own message is shown, as before, for example when a visitor is banned.
  • Answer that is not JSON: an error page from a proxy no longer skips the line. The reply is read as nothing, so the generic "That did not go through" line shows.
  • Success: unchanged. The field clears and the receipt shows.

The code changes are in src/apps/toolbar.ts (a few lines) and src/apps/note_words.ts (two new constants, NOTE_OFFLINE and NOTE_WAIT). The text is set with textContent, so the reason stays escaped. The tell route, CSP and same-origin check are untouched. The existing test for the toolbar script's 24 KB size limit still passes.

Tests

  • test/apps-toolbar-tell.test.ts now boots the toolbar script against a stubbed fetch. It checks that a 429 names the seconds, a 403 shows the server's reason, a 502 with an unreadable body shows the generic line, and an offline failure shows the offline line. In each case the textarea keeps its text.
  • test/helpers/toolbar-boot.ts gained a no-op removeAttribute so the shared stub can run the send path.
  • I did not confirm that the new tests fail without the change. With src/ stashed, the test file errors on the missing NOTE_OFFLINE import, so the check says nothing about the individual tests.

Not done

  • No "not signed in" line. The toolbar's note box takes anonymous visitors, so a note is never refused for that. The sign-in case belongs to the separate "Tell the crew" link, which opens the forge's own tell page. That page already sends a refused tell back with the words kept.
  • The server's 429 message is not reworded.

To try it, open an app with the forge pill, go offline in devtools and send a note: the words stay and the offline line shows.

🤖 Generated with Claude Code

commented

The check failed on two tests, neither caused by the toolbar change. I fixed one and could not reproduce the other. The fix is pushed. I ran test/job-slices.test.ts and test/apps-toolbar-tell.test.ts after the merge, and both pass. I did not rerun bun x tsc --noEmit after the merge. I did not run the full suite.

  • job-slices test: it failed because #11310 put a daily entitlements job into src/main.ts without timing it or naming it in UNTIMED. It failed the same way on main. I added entitlements to UNTIMED in test/job-slices.test.ts, with the reason "the fixture has no Stripe" and a follow-up note. The job is still untimed.
  • "The three windows" test (test/traffic-history.test.ts): it passes when I run it alone, before and after merging main. It looks like a flake under shard load, but I can't confirm that from here. If it fails again on the next check, send me that log.
  • Push: I had rebased onto main, so a plain push was refused as a non-fast-forward. Instead I merged origin/main into the pushed branch and put the fix on top, with no force push.

I added no tests this round, so no other test should now fail without this change.

commented

The type error is fixed and pushed. Main had added its own entitlements entry to UNTIMED in test/job-slices.test.ts, so my entry from the last round was a duplicate. I merged main and deleted mine.

bun x tsc --noEmit is clean. test/job-slices.test.ts and test/apps-toolbar-tell.test.ts pass (16 tests). I did not run the full suite.

Approved

Approved. A failed toolbar tell now keeps the words and shows one plain line: the wait in seconds on a 429, an offline line when navigator.onLine is false, the server message otherwise, and the generic line when the reply is not JSON. Text goes in via textContent, so it is escaped, and the added script is a few lines. I read the head and the tests. I did not run the preview. Not-signed-in is not a case here: feedback.ts takes anonymous senders and never answers 401. Two things I would do next: (1) drop the doubled blank line in note_words.ts before NOTE_OFFLINE; (2) a 429 with no retry-after falls back to the server message, so give it a plain line with no number too.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs