Skip to content

Comment, em dash and emoji sweep (backend campaign 6) - #95

Closed
theobong wants to merge 8 commits into
fix/backend-campaign-05-correctnessfrom
chore/backend-campaign-06-comments
Closed

theobong wants to merge 8 commits into
fix/backend-campaign-05-correctnessfrom
chore/backend-campaign-06-comments

Conversation

@theobong

@theobong theobong commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

What changed

A behavior-neutral comment, em dash and emoji sweep across the backend, plus one separate commit for user-facing copy. Comments now explain why, never what: banners, commented-out code, plan and review codes, name-restating doc comments and pointers to retired docs are gone, while security, privacy, concurrency, vendor-quirk and retention reasons are kept and tightened. Every em dash is gone except three inside applied migrations. The only behavior change is wording: the messages, email, PDF title and calendar title listed below read without em dashes.

Before you start

  • Where: staging (civfix.dev, admin.civfix.dev, api.civfix.dev) after this merges to main
  • Sign in as: a citizen account of your own (emailed code; the reviewer code is mobile-only and not enabled on staging) with some volunteer hours and a slot on an event; host of an event; operator (admin.civfix.dev through Cloudflare Access)
  • Stacked on Correctness fixes (backend campaign 5) #94 (correctness fixes), which is stacked on Security fixes (backend campaign 4) #93 and Backend formatting pass with the repo's Prettier config #91. Read only this PR's diff against Correctness fixes (backend campaign 5) #94's branch.
  • Size: 10677 counted lines. Campaign PR; the author approved PRs over the 400-line cap for this cleanup. Nearly all of it is deleted or rewritten comment lines.
  • Reading order: commits 1 to 5 are the neutral sweep (source, tests, migration comments, docs, config); commit 6 is the only user-facing copy change; commit 7 applies review fixes to three comments.

Verify

[Web]

  1. On your profile open the "Hours" tab, click "Prepare transcript", then "Open PDF" — Expect: the PDF opens as before; its document title (shown by the PDF viewer) reads "civfix service hours: , "; a table cell with no value shows "–".
  2. On an event where you hold a slot, use "Add to calendar" — Expect: the calendar entry is titled " ()".
  3. As a host, try to remove yourself from your own event's team — Expect: "You can't remove yourself. Leave the event instead."

[Admin]

  1. Open a report whose jurisdiction has no routing contact and use "Send to city" — Expect: "This report's jurisdiction has no routing contact. Set one in Jurisdictions first."

Regression

Everything else — [Web] [Mobile] [Admin]

  1. Use the apps as usual (report, chat, events, host console, admin mail) — Expect: no difference; commits 1 to 5 change no code (proofs below).

Findings addressed

Campaign PR 6: the comment rule (CLAUDE.md §2) across src, tests, scripts, the email Worker and config; every em dash in code, comments, SQL comments, user-facing strings and living markdown; every emoji in comments; the two TODOs (media upload CSRF, routing provider) turned into WHY comments.

Counts (source metrics over services/api/src and services/media-worker/src, before -> after): comment lines 6947 -> 4053; lines with an em dash (whole repo, excluding licenses and changelogs) 1391 -> 3; emoji in comments 5 -> 0.

Decisions for the reviewer

  • Three em dashes remain, inside the COMMENT ON COLUMN strings of applied migrations 0049_cleanup_cohosts.sql:26, 0051_social_posts.sql:128 and 0163_event_team_tiers.sql:81. Editing them changes SQL that databases already applied, and a new migration could not remove the text from the historical files. Catalog comments never reach users. git grep -n $'\u2014' -- . ':!LICENSE*' ':!LICENSES/**' ':!**/CHANGELOG.md' prints exactly these three lines.
  • Applied migrations had only their em dashes replaced (no comment deleted or restructured), so plan codes in those historical comments stay.
  • The certificate's empty-value mark is one constant EMPTY_VALUE = "–" (en dash, matching the "start – end" period on the same page) instead of the em dash. Issued certificates still verify: the ledger fingerprint does not include these strings; only newly rendered PDFs change. Switching to "-" is one line.
  • The demo seed's own "no em dash in seed content" check now spells the character as "\u2014"; the string value and the check are unchanged.
  • Review codes inside test names (for example "H11:", "F097") are left as they are; this pass changed test names only where they held an em dash. A later cleanup PR can drop them.
  • Comments corrected where they were false: the routing-provider stub is still selected in production; /healthz does not expose the version; the deploy workflows' if: conditions and the promote-and-pull flow; a rollback hint that moved no traffic; several "not used yet" notes; the jurisdiction layer order; the reference-code lock-order note.

User-visible copy changes

Where it is shown Before After
Host removing themselves (409) You can't remove yourself — leave the event instead. You can't remove yourself. Leave the event instead.
Logging your own volunteer hours (403) You can't log volunteer hours for yourself — another host must credit you. You can't log volunteer hours for yourself. Another host must credit you.
Announcement daily limit (429) Announcement limit reached for today — use the group chat… Announcement limit reached for today. Use the group chat…
Event timeline, resource request Resources requested — {preview} Resources requested: {preview}
Calendar entry title with a slot {event} — {slot} {event} ({slot})
Home Turf confirmation email Thanks — we received your Home Turf sign-up. Thanks! We received your Home Turf sign-up.
Certificate PDF title (en / de / es / ko) civfix service hours — {name} — {code} (and the same shape per locale) civfix service hours: {name}, {code} / civfix-Einsatzstunden: {name}, {code} / Horas de servicio en civfix: {name}, {code} / civfix 봉사 시간: {name}, {code}
Certificate PDF empty cell and empty period — –
Consent fields (event sign-up) is out of date — reload and accept the current terms / privacy notice; out of date — re-accept the terms / privacy notice is out of date, so reload and accept the current terms / privacy notice; out of date, so re-accept the terms / privacy notice
Ticket capacity field …(N already allocated) — raise the event capacity first; already holding N seats — cancel registrations before lowering it …(N already allocated), so raise the event capacity first; already holding N seats, so cancel registrations before lowering it
Page block media field must reference platform media — upload the image and send its mediaId must reference platform media: upload the image and send its mediaId
Admin: send still in progress (409, two places) …Check back shortly — the outcome will appear on the outreach trail. …Check back shortly to see the outcome on the outreach trail. / …Its outcome will appear on the outreach trail once the mail server answers, so check back shortly.
Admin: report changed meanwhile (409) This report moved on while you were looking at it — reload and try again This report moved on while you were looking at it. Reload and try again.
Admin: no routing contact No routing contact for this report's jurisdiction — set one in Jurisdictions first This report's jurisdiction has no routing contact. Set one in Jurisdictions first.
Admin: discovery note Suggested contact: X — {note} Suggested contact: X (note: {note})
Admin: mail provider refusals …once DKIM is active — this covers…; Send fewer or smaller photos — the rest… …once DKIM is active. That covers…; Send fewer or smaller photos. The rest…
NOTICE title civfix — the API and media worker civfix: the API and media worker

No error code, field key or machine-matched token changed. civfix-app and civfix-admin were checked: no client matches on any of these strings (the app's own announcement-limit copy is separate text in its catalog).

Tests changed

  • Tests that pin the copy above now pin the new text: adapters-mailer-oci.test.ts, certificate-model.test.ts (uses EMPTY_VALUE), certificate-pdf.test.ts, cleanups-routes.test.ts (ICS SUMMARY).
  • Fixtures that model external text keep their value: a city reply's decoded &#8212; and a city reply body are written as the \u2014 escape; two incidental fixture dashes follow the real subject format ([civfix: <ref>] <title>) or a colon.
  • About 110 test names and describe titles lost their em dash (a colon or semicolon instead); comments in tests follow the same rule as source. No assertion was weakened.

Verification

  • Comment neutrality: a TypeScript AST fingerprint of every changed code and JSON file is identical to #94's branch except string literals; each of the 156 changed literals is an em dash rewritten in a log line, boot error, test name or internal message (153) or the comment text inside an SQL template (3). package.json differs only in its //overrides note keys (deep-equal otherwise).
  • SQL migrations: a scratch script (not committed) strips -- and /* */ comments outside quotes and $$ bodies and compares each changed file with #94's branch. Output: sql-comment-neutral: 53/53 changed SQL files differ only in comments.
  • SQL sent by the repositories: the offline transcripts of all 764 repository cases (the recorder from #92, run locally) are identical to #94's branch once SQL comments are stripped; the 5 cases that differ differ only in comment text.
  • Node 22, pnpm 9.12.0: root pnpm typecheck, pnpm lint, pnpm check:sql, prettier check pass; api unit suite by path 428 files, 7116 passed, 3 skipped; media-worker unit 300 passed, 2 failed (the known ffprobe-helper environment issue fixed in #92, not in this stack).
  • A reviewer spot-checked every deleted comment in the security, concurrency, media-sandbox, database, migration and workflow areas and 35+ rewritten comments against the code: no security or invariant comment was lost; three inaccurate comments were fixed in the last commit; workflows changed only in comment text and one echo string.
  • CI runs only on PRs based on main, so it starts here when the PRs below merge and GitHub retargets this one to main.

Staging checks

  • https://api.civfix.dev/readyz answers ready after the deploy (no migration in this PR).
  • A certificate PDF and a calendar file download and render as in the Verify steps.

Not covered

  • The German, Spanish and Korean PDF titles were rewritten by the author, not by a native speaker.
  • The new admin-console wording is visible only on the error paths listed above.

🤖 Generated with Claude Code

@byteful
byteful added this pull request to stack #122 September 24, 2026 17:53
@byteful byteful closed this Sep 24, 2026
@byteful
byteful deleted the chore/backend-campaign-06-comments branch September 24, 2026 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants