Repository navigation
feat: add feedback, a Star CTA and one-click error reports - #797
Conversation
30403fd to
a02540d
Compare
HugoXK-bot
left a comment
There was a problem hiding this comment.
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.
POST /feedback(src/app.ts:4148) is behindapprovedCsrfGuard(:4149) and a feedback rate limit; the message isredact()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.- 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 israndomToken(18)and is the only thing the page carries. 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 andclaimErrorReport(: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 includednode scripts/risk-gate.mjs --base 46e686047bc0a6210a006943975f61ea11ee5e3e --head HEAD—risk-tier: T3; basis: src/config.tsnode scripts/pr-lint.mjs --pr 797—pr-lint: ok (#797, T3, 19 files)node scripts/changelog.mjs check— 47 fragments oknode scripts/version.mjs check— 0.3.6 oknode 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
left a comment
There was a problem hiding this comment.
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-lintFAILS 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
queuedsince 11:40:58Z and there is noci-okorreview-gateconclusion, so this PASS is the first conclusion here. Triggerci-okbefore merge. - The head is
dirtyagainst 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_versionmoves to 40"; the tree sets 43 atsrc/db.ts:2126with refusal at:2100(41 feedback and error reports, 42 triage, 43 Lead delivery, astest/scheduleSchema.test.tsstates). 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 /feedbacktakesapprovedCsrfGuard, redacts and bounds the message at 2000 chars, audits id/kind/length only, and mailsconfig.mail.feedbackEmail(fallback toADMIN_EMAILatsrc/config.ts:334).src/app.ts:4184-4241—POST /errors/:ref/reportis public by design: the unguessablerefis the capability, the consent field is required,claimErrorReportis 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/:refandPOST .../triagerequire an HMAC triage token scoped to the ref plussafeEqual(src/feedback/lead.ts:44-50), and the triage body is validated bytriageProblems(src/feedback/lifecycle.ts:54-75): nonew/released, every user-facing status needs a message,fixedneeds 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 afterSCHEMAruns, so a pre-existing database and a fresh one converge; the rest of the schema is additiveCREATE ... 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 oknode scripts/pr-lint.mjs --pr 797— FAIL (theLarge-PR:line above)node scripts/changelog.mjs check— 48 fragments oknode scripts/version.mjs check— 0.3.6 oknode 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
left a comment
There was a problem hiding this comment.
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-lintis green (ok (#797, T3, 25 files), run 12:52:13Z) and the body carriesLarge-PR: 25 files, 1502 changed lines.- The body now says
PRAGMA user_versionmoves 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-gatefailure 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 oknode scripts/pr-lint.mjs --pr 797— ok (#797, T3, 25 files)node scripts/changelog.mjs check— 67 fragments oknode scripts/version.mjs check— 0.3.6 oknode scripts/secret-scan.mjs— no suspected value in the tracked files
Not run: Docker, CodeQL, the visual matrix, a real SMTP or Lead bridge.
Written-by: HugoXK
What changed
POST /feedback. The route requires approved + CSRF, stores afeedbackrow, audits the id/kind/length, mailsFEEDBACK_EMAIL(falling back toADMIN_EMAIL), and flashes the outcome.★ Star Fledge on GitHub, and Settings has an About card with the same link. No star-count API is called.error_reportsrow with a randomrefbefore 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_EMAILis optional and falls back toADMIN_EMAIL.feedbackanderror_reports;PRAGMA user_versionmoves to 43.docs/architecture.md,docs/security.md,docs/deploy.md,.env.exampleand 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
refis 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 passnode scripts/changelog.mjs check— 44 fragments oknode scripts/version.mjs check— 0.3.6 okNot run:
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.