Skip to content

feat: add feedback, a Star CTA and one-click error reports - #797

Merged
HugoXK merged 3 commits into
mainfrom
feat/796-feedback-error-report
Nov 15, 2026
Merged

HugoXK merged 3 commits into
mainfrom
feat/796-feedback-error-report

Conversation

@HugoXK

@HugoXK HugoXK commented Nov 15, 2026 •

Copy link
Copy Markdown
Owner

Written-by: HugoXK

What changed

  • Settings → Feedback: a bilingual card (kind, optional page, message) posts to POST /feedback. The route requires approved + CSRF, stores a feedback row, audits the id/kind/length, mails FEEDBACK_EMAIL (falling back to ADMIN_EMAIL), and flashes the outcome.
  • Star: the footer now says ★ Star Fledge on GitHub, and Settings has an About card with the same link. No star-count API is called.
  • Unexpected 5xx: the error handler writes an error_reports row with a random ref before rendering the page. The page shows what the report contains, requires a data-access consent checkbox, then posts once to /errors/:ref/report. The row keeps only method, pathname, status, error name, redacted message, redacted stack, version, commit, the signed-in login, and the consent version/time. No query, body, headers, cookies, email or token. The claim is atomic, the route is rate-limited and works signed-out, and a 4xx never shows the button.
  • FEEDBACK_EMAIL is optional and falls back to ADMIN_EMAIL.
  • New tables feedback and error_reports; PRAGMA user_version moves to 43. docs/architecture.md, docs/security.md, docs/deploy.md, .env.example and one changelog fragment are updated.

Fixes #796

Why

The console had no user-facing feedback path, and an unexpected 5xx only wrote a log line; the user saw a generic page with no way to tell the maintainer what happened. The GitHub repository was a bare footer link, with no Star call-to-action.

Privacy and security

Feedback text is redacted and bounded before storage and mail. Error reports store a pathname only and a redacted, truncated error message; the ref is an unguessable capability, the report route is idempotent and rate-limited, and the mail states what is not included. A 4xx is user-caused and never offers the report button.

Verification

Ran:

  • npm run check — pass (2208 tests, typecheck, build)
  • npx tsx --test test/feedback.test.ts — 6/6 pass
  • node scripts/changelog.mjs check — 44 fragments ok
  • node scripts/version.mjs check — 0.3.6 ok

Not run:

  • Real SMTP delivery
  • Production deploy
  • A live GitHub star button

Changelog: changelog.d/796-feedback-error-report.added.md.

Large-PR: 25 files, 1502 changed lines

The issue is currently needs-review; this pull request is ready for the Issue-review PASS and the bot's review at the tip.

@HugoXK-bot HugoXK-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: PASS at a02540d
Written-by: HugoXK-bot
Posted-by: HugoXK-bot

Two new surfaces, both on the narrow side, plus the storage and mail behind them.

  1. POST /feedback (src/app.ts:4148) is behind approvedCsrfGuard (:4149) and a feedback rate limit; the message is redact()ed and capped at 2000 characters before it is stored or mailed, and a mail failure does not lose the stored report — the redirect says it was saved.
  2. The 5xx capture (src/app.ts:5536-5572) stores the method, the path with the query string stripped and capped at 500, the status, the error name, the redacted message and stack, and the build version/commit — no request body, no query values, no headers. The ref is randomToken(18) and is the only thing the page carries.
  3. POST /errors/:ref/report (src/app.ts:4177) is deliberately session-free so a failure on the sign-in page can still be reported, and it is bounded instead by the ref shape, the row lookup, the consent checkbox and claimErrorReport (:4190), which is atomic so a double click sends one mail. Unknown or malformed refs are 404 and 4xx pages never render the button.

Non-blocking: src/db.ts:2048 sets user_version = max(previous, 41). #831 already took main to 41, so this is a no-op here and the tables are still created because the schema is executed on every boot; the next migration must start at 42. The report route has no CSRF token by design — the ref is the capability — which is acceptable because the claim is one-shot and the action sends only stored data.

Ran at a02540d:

  • npm run check — pass, 2042/2042 tests, typecheck and build included
  • node scripts/risk-gate.mjs --base 46e686047bc0a6210a006943975f61ea11ee5e3e --head HEAD — risk-tier: T3; basis: src/config.ts
  • node scripts/pr-lint.mjs --pr 797 — pr-lint: ok (#797, T3, 19 files)
  • node scripts/changelog.mjs check — 47 fragments ok
  • node scripts/version.mjs check — 0.3.6 ok
  • node scripts/secret-scan.mjs — no suspected value

Not run: real SMTP delivery, and a browser submit of the feedback form.

Conclusion: PASS. The capture is minimal, redacted and ref-scoped; the report action is consent-gated and idempotent.

@HugoXK-bot HugoXK-bot added the review:approved Issue reviewed and approved by the Fledge Platform Reviewer label Nov 15, 2026

@HugoXK-bot HugoXK-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: PASS at 95eb04b
Written-by: HugoXK-bot
Posted-by: HugoXK-bot

Read the tip 95eb04ba950a3d9f26f72f18de56e43a97c25f39 (25 files, +1502/−23; this is the merge of #816 into the #797 branch, src/app.ts blob 832dcd7e and src/db.ts blob 55565043 match the contents API). The verdict binds this code; the PR metadata needs work before merge, listed below.

Blocking for merge, all fixable without a new sha:

  • pr-lint FAILS at this tip: a PR over 20 files or 800 changed lines needs a "Large-PR:" line (25 files, 1525 changed lines). The body is the author's; add the line.
  • No check suite has ever run at this tip: the one suite is queued since 11:40:58Z and there is no ci-ok or review-gate conclusion, so this PASS is the first conclusion here. Trigger ci-ok before merge.
  • The head is dirty against main (mergeable: false), so any forward merge changes the tip and voids this PASS — name the new sha and I judge that one.
  • The body still says "PRAGMA user_version moves to 40"; the tree sets 43 at src/db.ts:2126 with refusal at :2100 (41 feedback and error reports, 42 triage, 43 Lead delivery, as test/scheduleSchema.test.ts states). It also describes only #796 while the tip carries #814/#816 — see the routes below.

What I checked in the code, none blocking:

  • src/app.ts:4152-4182 — POST /feedback takes approvedCsrfGuard, redacts and bounds the message at 2000 chars, audits id/kind/length only, and mails config.mail.feedbackEmail (fallback to ADMIN_EMAIL at src/config.ts:334).
  • src/app.ts:4184-4241 — POST /errors/:ref/report is public by design: the unguessable ref is the capability, the consent field is required, claimErrorReport is atomic before the mail, and the stored path is the pathname only (request.url.split('?')[0], :5649). Rate limits are 5/h and 10/h per client (:600-611).
  • src/app.ts:4243-4291 — GET /api/feedback/:ref and POST .../triage require an HMAC triage token scoped to the ref plus safeEqual (src/feedback/lead.ts:44-50), and the triage body is validated by triageProblems (src/feedback/lifecycle.ts:54-75): no new/released, every user-facing status needs a message, fixed needs a SemVer version, and issue/PR links must be github.com objects in the configured public repo.
  • src/feedback/lead.ts:79-100 — the packet sends the triage bearer token (7-day TTL) only to the configured Lead repo/bridge, never to the requester or the page.
  • src/db.ts:2337-2342 — the in-place migration only ADDs the four Lead-delivery columns after SCHEMA runs, so a pre-existing database and a fresh one converge; the rest of the schema is additive CREATE ... IF NOT EXISTS.
  • src/views/pages.ts:2570-2590 — the error page offers the report form only when a 5xx ref exists, requires the consent checkbox, and adds the publish checkbox only for a signed-in viewer; a 4xx carries no ref.

Ran (exact tip tree; github.com:443 is unreachable from this box, so the tree came from the API tarball and the two blobs above were hash-checked):

  • npm run check — lint 11 baseline warnings, typecheck clean, 2052/2052 tests, build ok
  • node scripts/pr-lint.mjs --pr 797 — FAIL (the Large-PR: line above)
  • node scripts/changelog.mjs check — 48 fragments ok
  • node scripts/version.mjs check — 0.3.6 ok
  • node scripts/secret-scan.mjs — no suspected value in the tracked files

Not run: Docker, CodeQL, the visual matrix, a real SMTP or Lead bridge.

@HugoXK-bot HugoXK-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: PASS at dc2f515
Written-by: HugoXK-bot
Posted-by: HugoXK-bot

Read the tip dc2f515a9fe71d4ca8fe9df7e5debdc730a47c06 (merge of 95eb04ba + live main 9f2ef3aa; 25 files, +1502/−23). This supersedes my PASS at 95eb04ba950a3d9f26f72f18de56e43a97c25f39 only because the forward merge moved the tip.

What the merge changed, checked rather than assumed: I hashed all 720 blobs of tree 574424ea81f32606cd2f1cc8ad26d4987b272146 against the API tree object (720/720 match), and compared this PR's added/deleted line sets with the tip I passed at 95eb04ba. 19 of 25 files are line-identical. The six that differ changed only where main itself moved: src/db.ts replaces main's > 41 / Math.max(previousVersion, 41) refusal lines with the branch's 43 (src/db.ts:2269, :2296), and src/views/pages.ts gains the two imports main added (PageNotification, FLEDGE_GITHUB_URL). No branch content was added or dropped by the rebase.

The three merge blockers I named at the old tip are fixed at this tip:

  • pr-lint is green (ok (#797, T3, 25 files), run 12:52:13Z) and the body carries Large-PR: 25 files, 1502 changed lines.
  • The body now says PRAGMA user_version moves to 43, which is what the tree does (src/db.ts:2296, refusal at :2269).
  • The tip has a full CI run: 28 named checks, every one success except review-gate failure 12:50:54Z, which is the pre-verdict state this review replaces.

One documentation gap remains, non-blocking: the body says "New tables feedback and error_reports", but the tree adds four (feedback src/db.ts:2005, error_reports :2016, feedback_triage :2040, feedback_events :2066), and the whole #816 Lead-delivery/triage half is not described in the body (src/feedback/lead.ts, src/app.ts:4479, :4494). One sentence naming it before squash keeps the commit message honest; the code, the docs (docs/architecture.md, docs/secrets.md) and the fragments (changelog.d/796-*, changelog.d/814-*) already cover it.

Ran (exact tip tree; github.com:443 is unreachable from this box, so the tree came from the API tarball and every blob was hash-checked):

  • npm run check — lint 11 baseline warnings, typecheck clean, 2208/2208 tests, build ok
  • node scripts/pr-lint.mjs --pr 797 — ok (#797, T3, 25 files)
  • node scripts/changelog.mjs check — 67 fragments ok
  • node scripts/version.mjs check — 0.3.6 ok
  • node scripts/secret-scan.mjs — no suspected value in the tracked files

Not run: Docker, CodeQL, the visual matrix, a real SMTP or Lead bridge.

@HugoXK
HugoXK merged commit f3ddda1 into main Nov 15, 2026
34 of 37 checks passed
@HugoXK
HugoXK deleted the feat/796-feedback-error-report branch November 15, 2026 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review:approved Issue reviewed and approved by the Fledge Platform Reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add a feedback card, a Star CTA and one-click 5xx reports

2 participants