dashboard fixes#68
Conversation
CLAUDE.md "Wishonia: Voice of the Site" + AGENTS.md "Copy and Voice" now spell out the patterns to never write: tautological "X is the Y. The Y is the road that produces X" framings; empty mechanism vocabulary (incentive layer, coordination mechanism, primitive, substrate, kernel of, fundamentally/essentially/literally); stack / rails / off-ramp / pipeline metaphors; corporate-blather openers; stacked abstract-noun lists; sweeping "every X" without numbers. Plus the read-aloud test: if it could appear unchanged in a Y Combinator launch post or Stripe pitch deck, rewrite. Wishonia is an alien mildly disappointed in your civilization, not a founder. TreatyContent.tsx: replace the canonical violation that motivated this rule. "Next: the enforcement stack. The treaty is the off-ramp. The Court is the road that produces the off-ramp." → "If your government refuses to sign / Sign the treaty here. If your government refuses to ratify it, join the class action and sue them for the 102 million people their refusal has killed." Concrete, stakes-named, zero infrastructure metaphors. TODO.md: queues two follow-up tasks so we don't re-investigate. (1) Finish neobrutalist cleanup — drop pink/cyan/yellow from BrutalCard/SectionContainer bgColor unions, update dynamic color-returning functions (getCardColor's "yellow"/"cyan" non-status branches, step.color data, accentColor / metric.tone props), delete the now-unused brutal-pink/cyan/yellow CSS vars, collapse the @theme inline redirect block, optionally rename BrutalCard → TreatySection and ArcadeTag → Eyebrow, delete dead ARCADE_LABELS dictionary. (2) Copy audit pass — sweep packages/web/src/ for the new anti-pattern grep terms and rewrite each surface to concrete-Cunk-deadpan, with the four already-found offenders (TreatyContent, TreasuryAllocationViz line 160, WishocracyLinkCard line 17, ResourcePromoSection line 12) listed explicitly so the audit doesn't need to re-find them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
📝 WalkthroughWalkthroughThis PR tightens CI visual-baseline discovery and seeding, adds a safe SQL migration and removes one-shot scripts, introduces page-render tooling and demo-session minting, ignores generated preview files, establishes terse copy-voice constraints, adds route-review metadata and many new Markdown pages, hardens task date handling and accessibility labels, updates Next/webpack externals, and adds pixel-level visual diffing and related tests. ChangesCI Visual Baseline Resolution
Tooling: page previews & scripts
DB migration & script removals
Copy voice & campaign tracking
Web app UI & behavior changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
Updates the project’s “Wishonia voice” documentation to explicitly forbid a set of empty/tautological startup-style writing patterns, replaces a canonical offender in the /treaty CTA copy with concrete stakes, and improves CI behavior around web validation and visual baseline resolution.
Changes:
- Add explicit “anti-pattern” rules + a read-aloud test to
CLAUDE.mdandAGENTS.mdfor user-facing copy. - Replace
/treaty“enforcement stack / off-ramp” copy with concrete class-action framing inTreatyContent.tsx. - Adjust CI so
web-validateruns onmainpushes and make baseline artifact download more robust; expand TODO tracking for remaining migration/copy-audit work.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| TODO.md | Updates/expands queued cleanup tasks (migration cleanup + copy audit follow-ups). |
| packages/web/src/components/treaty/TreatyContent.tsx | Rewrites the Court CTA copy on /treaty to remove the tautological metaphor and name concrete stakes. |
| CLAUDE.md | Adds a detailed anti-pattern blacklist + “read-aloud” test for Wishonia voice. |
| AGENTS.md | Mirrors the anti-pattern blacklist + test in agent-facing copy rules. |
| .github/workflows/ci.yml | Runs web-validate on main pushes and improves main-baseline artifact extraction logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
mikepsinn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
203-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t require the entire main workflow run to be
successwhen hunting for a visual baseline.This query ignores any main run where
web-validateproduced a validweb-visual-reviewartifact but a later job in the same workflow failed, notablydeploy-production. In that case PRs silently lose their freshest before-screenshots even though the baseline artifact exists.Suggested fix
mapfile -t run_ids < <(gh run list \ --workflow CI \ --branch main \ - --status success \ --limit 20 \ - --json databaseId \ - --jq '.[].databaseId') + --json databaseId,status,conclusion \ + --jq '.[] | select(.status == "completed") | .databaseId')Then keep relying on
gh run download ... --name web-visual-reviewas the real proof that a usable baseline exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 203 - 209, The current gh run list call in the mapfile (mapfile -t run_ids < <(gh run list --workflow CI --branch main --status success --limit 20 --json databaseId --jq '.[].databaseId')) filters only fully successful workflow runs and thus skips runs that produced a usable web-visual-review artifact but later failed (e.g., deploy-production); remove the strict --status success filter (or replace it with --status completed) so you still collect candidate run IDs, then continue to rely on the subsequent gh run download --name web-visual-review check to determine whether a usable baseline artifact actually exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/components/treaty/TreatyContent.tsx`:
- Around line 97-99: The inline sentence "Sign the treaty here..." should be
gated on the component prop showInlineSign (in TreatyContent.tsx) so it only
appears when showInlineSign is true; update the JSX around the sentence to
conditionally render it (or alternatively change the copy to reference the
visible "court" button when showInlineSign is false), ensuring you modify the
render that uses showInlineSign and the specific text node so users aren't
instructed to "sign here" when no inline sign control exists.
In `@TODO.md`:
- Around line 896-899: Edit the TODO.md "Confirmed offenders" list to remove or
mark fixed the entry for the TreatyContent component: locate the bullet
referencing TreatyContent (the TreatyContent.tsx offender) in the confirmed
offenders section and either delete that line or append a clear "(fixed)" note
so the audit won't flag it after this PR merges; commit the update so the TODO
reflects the rewritten copy.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 203-209: The current gh run list call in the mapfile (mapfile -t
run_ids < <(gh run list --workflow CI --branch main --status success --limit 20
--json databaseId --jq '.[].databaseId')) filters only fully successful workflow
runs and thus skips runs that produced a usable web-visual-review artifact but
later failed (e.g., deploy-production); remove the strict --status success
filter (or replace it with --status completed) so you still collect candidate
run IDs, then continue to rely on the subsequent gh run download --name
web-visual-review check to determine whether a usable baseline artifact actually
exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c876724-1ce3-44f3-852b-5b93c65a3fe6
📒 Files selected for processing (5)
.github/workflows/ci.ymlAGENTS.mdCLAUDE.mdTODO.mdpackages/web/src/components/treaty/TreatyContent.tsx
Visual review
Start with the browser gallery for a quick before/after pass over every captured route. Use the visual diff link for changed-only review when Argos is available. The downloadable artifact is the fallback copy of |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/web/src/app/tasks/[id]/page.tsx (1)
321-343: ⚡ Quick winRun anonymous Wishonia lookup concurrently with public page fetch.
In the anonymous path,
getWishoniaUserId()currently waits forgetPublicTaskPageData(id)to finish. Running them together removes avoidable latency.Proposed refactor
const session = await getServerSession(authOptions); const userId = session?.user.id ?? null; - const publicPageData = userId - ? null - : await getPublicTaskPageData(id); + const [publicPageData, publicWishoniaUserId] = userId + ? [null, null] + : await Promise.all([ + getPublicTaskPageData(id), + getWishoniaUserId().catch(() => null), + ]); const [data, commentFeed, activityTimeline, wishoniaUserId, ancestors, viewerIsAdmin] = userId ? await Promise.all([ @@ : [ publicPageData?.data ?? null, publicPageData?.commentFeed ?? { comments: [], total: 0 }, publicPageData?.activityTimeline ?? [], - await getWishoniaUserId().catch(() => null), + publicWishoniaUserId, publicPageData?.ancestors ?? [], false, ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/app/tasks/`[id]/page.tsx around lines 321 - 343, The anonymous branch currently awaits getPublicTaskPageData(id) before calling getWishoniaUserId(), adding unnecessary latency; change the anonymous path to run getPublicTaskPageData(id) and getWishoniaUserId() concurrently (e.g., start both Promises and await Promise.all) and then build publicPageData, commentFeed, activityTimeline, wishoniaUserId, ancestors, and viewerIsAdmin from the settled results; update references to publicPageData and wishoniaUserId accordingly so the same values are used when assembling the array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/db/prisma/migrations/20260509173000_rename_optimize_earth_root_task/migration.sql`:
- Around line 41-52: The migration soft-deletes the old Task root by setting
"deletedAt" on the row identified by old_id/new_id in the "Task" table, but that
does not trigger ON DELETE behavior on FK constraints so dependent tables will
still reference old_id; update all dependent FK columns (e.g., TaskClaim.taskId,
TaskComment.taskId, TaskDependency.[taskId/dependsOnId],
ReferralInvitation.taskId, ShareAttempt.taskId, and any other tables referencing
Task.id) to point to new_id or NULL as appropriate before or after moving
children (the UPDATE that sets "parentTaskId" = new_id), or explicitly
delete/clean up those dependent rows if desired, and ensure "updatedAt" is
adjusted; locate the logic around old_id/new_id and the two UPDATEs on "Task"
and add corresponding UPDATE/DELETE statements for each dependent table to
prevent dangling references.
In `@packages/web/next.config.js`:
- Around line 32-34: The serverExternalPackages array currently lists only
"@storacha/client" and "pinata" but omits "@optimitron/storage", causing
Turbopack to attempt bundling it; update the serverExternalPackages array (the
variable named serverExternalPackages) to include "@optimitron/storage" so
Turbopack externalizes it the same way webpack does, ensuring the turbopack: {
root: ... } config and the existing webpack server-side externalization logic
remain consistent with the webpack-only node: fallback handling.
In `@packages/web/scripts/render-pages-to-markdown.ts`:
- Around line 120-137: The loop over routes that calls extractPage and writes
files currently logs failures but still exits zero; introduce a failure tracker
(e.g., a boolean or counter like hadFailures) in the render-pages-to-markdown
script, set it to true whenever the catch block for the routes loop (the block
handling extractPage/routeToFilePath/writeFile) catches an error, and after the
loop (and likewise after the second similar loop around lines 143–146) throw an
Error or call process.exit(1) when hadFailures is true so the process returns a
non-zero status; update the catch blocks that build message from err to also set
the tracker when an error occurs.
- Around line 58-66: The parseRoutesFromArgs output can contain path traversal
segments (e.g., "../../") that are later used to build output file paths; to
fix, when mapping routes into output file locations (the code that joins routes
with APP_DIR in render-pages-to-markdown.ts), normalize and resolve each route
against APP_DIR (use path.normalize/path.resolve or path.join(APP_DIR, route)
then path.resolve) and verify the resolved path has APP_DIR as its prefix (e.g.,
check path.relative(APP_DIR, resolvedPath) does not start with ".."); reject or
strip routes that fail this check so no write occurs outside APP_DIR. Ensure
this validation is applied wherever parseRoutesFromArgs results are consumed
(referencing parseRoutesFromArgs, APP_DIR, and DEFAULT_ROUTES).
In `@packages/web/src/components/tasks/ProgramTaskSection.tsx`:
- Around line 41-45: Extract the duplicated date parsing/formatting helpers into
a shared module (e.g., packages/web/src/lib/tasks/task-dates.ts) and replace
local implementations: move logic for getTaskDateMs, getTaskDate (or
getTaskDateMs's Date-returning variant), formatDueDate (and any task-specific
formatters used in task-row.tsx and task-card.tsx) into exported functions like
getTaskDate, getTaskDateMs, formatTaskDueDate, formatTaskCompletedDate; then
update ProgramTaskSection.tsx, task-row.tsx, and task-card.tsx to import those
functions instead of their local duplicates so all three files call the single
canonical implementations (referencing the existing symbols getTaskDateMs,
getTaskDate, and formatDueDate when locating where to replace).
- Around line 41-45: Add unit tests for the date-parsing helpers getTaskDateMs
and getTaskDate to cover all branches: input null and undefined should return
null; valid Date input should return the Date / timestamp; valid ISO date string
should parse to the same Date / timestamp; invalid date strings and Invalid Date
objects should return null. Use your project's test runner (vitest) to create
tests that assert getTaskDate returns null/Date and getTaskDateMs returns
null/number for the cases above, including explicit checks for timestamp
equality for ISO strings and Date inputs.
---
Nitpick comments:
In `@packages/web/src/app/tasks/`[id]/page.tsx:
- Around line 321-343: The anonymous branch currently awaits
getPublicTaskPageData(id) before calling getWishoniaUserId(), adding unnecessary
latency; change the anonymous path to run getPublicTaskPageData(id) and
getWishoniaUserId() concurrently (e.g., start both Promises and await
Promise.all) and then build publicPageData, commentFeed, activityTimeline,
wishoniaUserId, ancestors, and viewerIsAdmin from the settled results; update
references to publicPageData and wishoniaUserId accordingly so the same values
are used when assembling the array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32134807-06dd-4cbb-af39-1bd611597789
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
.github/workflows/ci.yml.gitignoreAGENTS.mdCLAUDE.mdTODO.mdpackage.jsonpackages/data/src/parameters/parameters-calculations-citations.tspackages/db/package.jsonpackages/db/prisma/migrations/20260509173000_rename_optimize_earth_root_task/migration.sqlpackages/db/src/reassign-personal-tasks.tspackages/web/AGENTS.mdpackages/web/e2e/visual-regression.spec.tspackages/web/next.config.jspackages/web/package.jsonpackages/web/scripts/build-visual-review.mjspackages/web/scripts/rename-optimize-earth-root.tspackages/web/scripts/render-pages-to-markdown.tspackages/web/src/app/tasks/[id]/page.tsxpackages/web/src/components/Navbar.tsxpackages/web/src/components/site/TreatyTaskDashboardClient.tsxpackages/web/src/components/tasks/ProgramTaskSection.tsxpackages/web/src/components/tasks/task-card.tsxpackages/web/src/components/tasks/task-row.tsxpackages/web/src/components/treaty/TreatyContent.tsxpackages/web/src/lib/__tests__/site.test.tspackages/web/src/lib/routes.tspackages/web/src/lib/site.tspackages/web/src/lib/tasks/president-management.test.tspackages/web/src/lib/tasks/president-management.ts
💤 Files with no reviewable changes (2)
- packages/db/src/reassign-personal-tasks.ts
- packages/web/scripts/rename-optimize-earth-root.ts
✅ Files skipped from review due to trivial changes (8)
- packages/db/package.json
- .gitignore
- packages/web/src/lib/tasks/president-management.test.ts
- packages/web/src/components/treaty/TreatyContent.tsx
- AGENTS.md
- packages/data/src/parameters/parameters-calculations-citations.ts
- CLAUDE.md
- TODO.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/e2e/utils/static-pages.ts (1)
16-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent fallback and hardcoded host string.
The function returns
"127.0.0.1:3001"whenBASE_URLis unset but returnsnullwhenBASE_URLis set to an invalid URL. For consistency, both error conditions should fall back to the same default. Additionally, the hardcoded host string should be extracted to a named constant.🔧 Proposed fix for consistent fallback and DRY
+const DEFAULT_SMOKE_TEST_HOST = "127.0.0.1:3001"; + function getSmokeTestHost() { - if (!process.env.BASE_URL) return "127.0.0.1:3001"; + if (!process.env.BASE_URL) return DEFAULT_SMOKE_TEST_HOST; try { return new URL(process.env.BASE_URL).host; } catch { - return null; + return DEFAULT_SMOKE_TEST_HOST; } }Based on learnings, port 3001 on 127.0.0.1 is the canonical local web server for this repository.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/e2e/utils/static-pages.ts` around lines 16 - 24, Extract the hardcoded host "127.0.0.1:3001" into a named constant (e.g., DEFAULT_SMOKE_TEST_HOST) and update getSmokeTestHost to return that constant both when process.env.BASE_URL is unset and when new URL(process.env.BASE_URL) throws; keep function name getSmokeTestHost and the URL parsing logic but change the catch branch to return DEFAULT_SMOKE_TEST_HOST instead of null so both error paths use the same fallback.
🧹 Nitpick comments (7)
packages/web/src/lib/routes.ts (3)
712-723: 💤 Low valueExplicit
copyPreview: false/screenshot: falseoverrides onplaintiffsManageLinkare correct — flag the spread-then-override pattern for future readers.Spreading
plaintiffsLinkbrings incopyPreview: trueandscreenshot: true, which are then explicitly set tofalseto make the manage page auth-only in the review pipeline. That's intentional and works, but it's easy to miss. A short inline comment ("manage page is auth-only; override public review flags inherited fromplaintiffsLink") would save a future maintainer a few minutes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/routes.ts` around lines 712 - 723, The spread-then-override on plaintiffsManageLink currently resets copyPreview and screenshot inherited from plaintiffsLink to false for auth-only behavior; add a brief inline comment above the plaintiffsManageLink declaration (referencing plaintiffsManageLink and plaintiffsLink and the copyPreview/screenshot fields) stating: "manage page is auth-only; override public review flags inherited from plaintiffsLink" so future readers understand why those booleans are explicitly set to false.
1506-1512: 💤 Low value
href.split(/[?#]/, 1)[0]with limit 1 returns the prefix only when no separators precede content — confirm intent.
String.prototype.split(separator, limit)truncates the result array tolimititems but does not change splitting behavior. Forhref = "/dashboard?ref=foo", this returns["/dashboard"](correct). Forhref = "/", it returns["/"](correct). The|| ROUTES.homefallback is unreachable given thestartsWith("/")guard above, since any/-prefixed href yields a non-empty first chunk. The fallback is harmless but dead — fine to leave for defensiveness.If you want the fallback to actually mean something, drop the early-return for non-
/hrefs and let the fallback take effect; otherwise this reads cleaner without it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/routes.ts` around lines 1506 - 1512, The function getReviewPathFromNavItem contains an unreachable fallback "|| ROUTES.home" because the earlier guard (if (!href.startsWith("/")) return null) guarantees href.split(/[?#]/, 1)[0] is always a non-empty string for "/"-prefixed values; remove the redundant fallback so the function simply returns the first split chunk. Update getReviewPathFromNavItem to keep the existing startsWith("/") guard and return the href.split(/[?#]/, 1)[0] result directly, referencing the NavItem.href handling and ROUTES.home only if you later decide to change the guard semantics.
1450-1494: 💤 Low value
getRouteReviewSpecsskips items with truthyexternal, but the curated list already excludes externals.
routeReviewNavItemsis hand-curated and currently contains noexternal: trueentries. ThenavItem.externalguard at line 1478 is defensive belt-and-suspenders, which is fine, but it duplicates intent already enforced by the list itself. Consider either removing the guard (since the list is the contract) or relaxing the list to allow mixing externals (and let the guard filter them).Not blocking — current behavior is correct either way.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/routes.ts` around lines 1450 - 1494, The navItem.external check inside getRouteReviewSpecs duplicates the curated contract of routeReviewNavItems; remove the redundant guard (the "|| navItem.external" condition) from the filter so getRouteReviewSpecs only checks mode and relies on the curated list, keeping getReviewPathFromNavItem and the existing name/path construction intact for each navItem (update any tests that assumed externals were filtered if needed).packages/web/scripts/build-visual-review.mjs (1)
32-61: 💤 Low value
routeOrderis a hand-maintained list — drifts the moment someone adds ascreenshotflag inroutes.tswithout updating this file.
routeSortIndexreturnsrouteOrder.lengthfor unknown routes, so new routes silently sort to the end. That's a graceful fallback, but it means visual-review ordering androutes.tsreview opt-ins will drift over time. Consider derivingrouteOrderfromgetRouteReviewSpecs("screenshot")(and friends) at runtime so the two stay in sync, with the manual list reserved only for the non-nav exceptions (task-optimize-earth,task-one-percent-treaty,task-signer-canada,side-menu,side-menu-auth).Not blocking — current behavior is correct, just drift-prone.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/scripts/build-visual-review.mjs` around lines 32 - 61, The manual routeOrder array drifts from routes that opt into screenshots; update the build-visual-review logic so routeOrder is derived at runtime from getRouteReviewSpecs("screenshot") (and other review types if used) instead of a hard-coded list: call getRouteReviewSpecs("screenshot"), map to route names in the same order it returns, then append the explicit-exception names ("task-optimize-earth", "task-one-percent-treaty", "task-signer-canada", "side-menu", "side-menu-auth") to preserve their custom placement; ensure routeSortIndex continues to fall back to routeOrder.length for unknown routes.packages/web/e2e/visual-regression.spec.ts (2)
72-77: ⚡ Quick win
requiredTextregression guard freezes exact prose in an E2E — coding guidelines push back on this.Per the repo's testing rule:
Browser tests should assert behavior, route transitions, data contracts, analytics-critical parameters, accessibility roles, and the presence/absence of coarse UI states. Avoid exact prose, magic-number, or paragraph-level assertions unless the wording itself is the contract being tested. Put exact copy parity in focused unit/doc tests, seeded-template tests, or screenshot review instead.
The intent here (guard a president/signer task list from being silently removed) is legitimate, but the inline comment ("Do not delete without Mike's explicit approval") and the prose assertion are exactly the pattern the guideline says to avoid. Two options that keep the regression guard without freezing copy:
- Replace the text match with a structural/role assertion — e.g.
await expect(page.getByRole("list", { name: /signer/i })).toBeVisible()or assert thatpage.getByTestId("signer-task-row")has at least N entries.- Move the prose contract to a focused unit/snapshot test on the page module so the E2E stays behavior-only.
If the wording itself is genuinely the contract being tested, leave a one-liner explaining that and the comment is fine.
As per coding guidelines: "Browser tests should assert behavior, route transitions, data contracts, analytics-critical parameters, accessibility roles, and the presence/absence of coarse UI states. Avoid exact prose, magic-number, or paragraph-level assertions unless the wording itself is the contract being tested."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/e2e/visual-regression.spec.ts` around lines 72 - 77, The current regression guard uses a prose match via route.requiredText and page.getByText which freezes copy; replace this with a structural/assertion-based check (e.g., assert the presence of the signer/president task list via page.getByRole("list", { name: /signer|president/i }) or page.getByTestId("signer-task-row") and assert it has at least N entries) and remove the "Do not delete without Mike's explicit approval" prose; if the exact wording truly is a contract, convert the inline comment to a single-line justification and move the detailed copy parity check into a focused unit/snapshot test for the page module instead.
47-53: 💤 Low value
signInDemoUserfailure short-circuits viaexpect(...).toBe(true)— fine for first sign-in, but consider the cleanup story.Each test gets a fresh
pagefixture, so cookies/storage don't leak across the cases — good. Nothing to change here. Worth noting that if the seeded demo user ever fails to mint a session (e.g., themint-demo-sessionscript changes), every authenticated route fails with the same error; the assertion message ("demo user should sign in before dashboard screenshot") is generic enough to cover that, but a single upfronttest.beforeAllsmoke check onsignInDemoUserwould surface the root cause in one place instead of N failing tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/e2e/visual-regression.spec.ts` around lines 47 - 53, Add a one-time smoke check that calls signInDemoUser before the suite runs so a global authentication break fails fast: implement a test.beforeAll that creates a page/context (or reuses an appropriate fixture), calls signInDemoUser(page), and throws or fails with a clear message if it returns false; keep the existing per-route expect(...) checks but remove the need to debug N identical failures by surfacing the root signInDemoUser/session minting problem in one place.packages/web/src/lib/site.ts (1)
472-501: 💤 Low valueFooter column wiring is consistent with the new
routes.ts; double-checkfeedbackLinkbelongs in "Do Something".
feedbackLinkplaced alongsidevoteLink,plaintiffsLink,donateLinkin "Do Something" reads as a low-priority sibling to vote/register/donate. Feedback is typically a lower-tier action; consider moving it to the bottom of that list (already last, good) or to a different column. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/site.ts` around lines 472 - 501, The footer currently places feedbackLink as a direct item in the "Do Something" column next to primary actions (voteLink, plaintiffsLink, donateLink); move feedbackLink to a less-prominent position or a different column to reflect its lower priority—either append it as the last item in that column (ensure it remains last) or remove it from that array and add it to another column's items (e.g., "Learn Something" or a new "Connect" column). Update the columns array where bottomText and columns are defined and adjust the items arrays referencing feedbackLink, voteLink, plaintiffsLink, donateLink, dashboardLink, presidentManagementLink, peopleLink, tasksLink, treatyLink, humanityVGovernmentLink, onePercentTreatyPaperLink, courtLink, endorseLink, and organizationalSignatoryLinks accordingly so the footer order and grouping reflect the intended priority.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/scripts/build-visual-review.mjs`:
- Around line 496-505: The catch in comparePair currently returns {changed:
true,...} which inflates "changed" counts; change the catch to return an
explicit errored flag (e.g. {changed: false, missing: false, errored: true,
label: `diff error: ${message.slice(0,60)}`, statusClass: "error"}) and then
update summarizeGroups to count errored routes separately (increment an errored
count) and adjust routeStatusLabel / header rendering to surface an "errored"
pill with that count so errors are not rolled into the changed bucket.
In `@packages/web/src/app/court/page.md`:
- Line 34: Article V on the generated /court page still contains the forbidden
tautology ("The Treaty is the off-ramp. The Court is the road that produces the
off-ramp.") and needs to be changed; update the source for the /court content
(likely packages/web/src/app/court/page.tsx or the module that produces page.md)
to either (A) rewrite Article V to remove the off-ramp/road metaphor and state
the concrete outcomes (e.g., treaty signing, 1% military redirect, liability
caps and the 102M-deaths framing) consistent with the TreatyContent.tsx rewrite,
or (B) if you intentionally keep the legal-document register, add a short
documented exception to CLAUDE.md / AGENTS.md referencing this exact phrase so
reviewers know it is permitted; modify the corresponding source (page.tsx or the
content module generating page.md) and update CLAUDE.md/AGENTS.md if choosing
the exception path.
In `@packages/web/src/app/questions/page.md`:
- Around line 3-5: Replace the user-facing lines that contain throat-clearing
and profanity: remove the apologetic phrase "I'm very sorry to bother you, but
this is kind of the most important thing in the universe..." and the profanity
line "GO TO HELL" and replace them with concise, direct copy consistent with
Wishonia voice (e.g., a short, neutral statement or CTA and an affirmative
response option instead of "FINE"); update the page content in the questions
page markdown accordingly and do not commit these copy changes until a human
reviewer has explicitly approved the exact wording.
---
Outside diff comments:
In `@packages/web/e2e/utils/static-pages.ts`:
- Around line 16-24: Extract the hardcoded host "127.0.0.1:3001" into a named
constant (e.g., DEFAULT_SMOKE_TEST_HOST) and update getSmokeTestHost to return
that constant both when process.env.BASE_URL is unset and when new
URL(process.env.BASE_URL) throws; keep function name getSmokeTestHost and the
URL parsing logic but change the catch branch to return DEFAULT_SMOKE_TEST_HOST
instead of null so both error paths use the same fallback.
---
Nitpick comments:
In `@packages/web/e2e/visual-regression.spec.ts`:
- Around line 72-77: The current regression guard uses a prose match via
route.requiredText and page.getByText which freezes copy; replace this with a
structural/assertion-based check (e.g., assert the presence of the
signer/president task list via page.getByRole("list", { name:
/signer|president/i }) or page.getByTestId("signer-task-row") and assert it has
at least N entries) and remove the "Do not delete without Mike's explicit
approval" prose; if the exact wording truly is a contract, convert the inline
comment to a single-line justification and move the detailed copy parity check
into a focused unit/snapshot test for the page module instead.
- Around line 47-53: Add a one-time smoke check that calls signInDemoUser before
the suite runs so a global authentication break fails fast: implement a
test.beforeAll that creates a page/context (or reuses an appropriate fixture),
calls signInDemoUser(page), and throws or fails with a clear message if it
returns false; keep the existing per-route expect(...) checks but remove the
need to debug N identical failures by surfacing the root signInDemoUser/session
minting problem in one place.
In `@packages/web/scripts/build-visual-review.mjs`:
- Around line 32-61: The manual routeOrder array drifts from routes that opt
into screenshots; update the build-visual-review logic so routeOrder is derived
at runtime from getRouteReviewSpecs("screenshot") (and other review types if
used) instead of a hard-coded list: call getRouteReviewSpecs("screenshot"), map
to route names in the same order it returns, then append the explicit-exception
names ("task-optimize-earth", "task-one-percent-treaty", "task-signer-canada",
"side-menu", "side-menu-auth") to preserve their custom placement; ensure
routeSortIndex continues to fall back to routeOrder.length for unknown routes.
In `@packages/web/src/lib/routes.ts`:
- Around line 712-723: The spread-then-override on plaintiffsManageLink
currently resets copyPreview and screenshot inherited from plaintiffsLink to
false for auth-only behavior; add a brief inline comment above the
plaintiffsManageLink declaration (referencing plaintiffsManageLink and
plaintiffsLink and the copyPreview/screenshot fields) stating: "manage page is
auth-only; override public review flags inherited from plaintiffsLink" so future
readers understand why those booleans are explicitly set to false.
- Around line 1506-1512: The function getReviewPathFromNavItem contains an
unreachable fallback "|| ROUTES.home" because the earlier guard (if
(!href.startsWith("/")) return null) guarantees href.split(/[?#]/, 1)[0] is
always a non-empty string for "/"-prefixed values; remove the redundant fallback
so the function simply returns the first split chunk. Update
getReviewPathFromNavItem to keep the existing startsWith("/") guard and return
the href.split(/[?#]/, 1)[0] result directly, referencing the NavItem.href
handling and ROUTES.home only if you later decide to change the guard semantics.
- Around line 1450-1494: The navItem.external check inside getRouteReviewSpecs
duplicates the curated contract of routeReviewNavItems; remove the redundant
guard (the "|| navItem.external" condition) from the filter so
getRouteReviewSpecs only checks mode and relies on the curated list, keeping
getReviewPathFromNavItem and the existing name/path construction intact for each
navItem (update any tests that assumed externals were filtered if needed).
In `@packages/web/src/lib/site.ts`:
- Around line 472-501: The footer currently places feedbackLink as a direct item
in the "Do Something" column next to primary actions (voteLink, plaintiffsLink,
donateLink); move feedbackLink to a less-prominent position or a different
column to reflect its lower priority—either append it as the last item in that
column (ensure it remains last) or remove it from that array and add it to
another column's items (e.g., "Learn Something" or a new "Connect" column).
Update the columns array where bottomText and columns are defined and adjust the
items arrays referencing feedbackLink, voteLink, plaintiffsLink, donateLink,
dashboardLink, presidentManagementLink, peopleLink, tasksLink, treatyLink,
humanityVGovernmentLink, onePercentTreatyPaperLink, courtLink, endorseLink, and
organizationalSignatoryLinks accordingly so the footer order and grouping
reflect the intended priority.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76756ce0-b03d-4070-bd5b-6c0bee294da7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.github/workflows/ci.yml.gitignorepackages/web/AGENTS.mdpackages/web/e2e/utils/static-pages.tspackages/web/e2e/utils/visual-routes.tspackages/web/e2e/visual-regression.spec.tspackages/web/package.jsonpackages/web/scripts/build-visual-review.mjspackages/web/scripts/mint-demo-session.tspackages/web/scripts/render-pages-to-markdown.tspackages/web/src/app/court/page.mdpackages/web/src/app/donate/page.mdpackages/web/src/app/employees/page.mdpackages/web/src/app/endorse/page.mdpackages/web/src/app/feedback/page.mdpackages/web/src/app/humanity-v-government/page.mdpackages/web/src/app/legal/page.mdpackages/web/src/app/page.mdpackages/web/src/app/people/page.mdpackages/web/src/app/plaintiffs/page.mdpackages/web/src/app/privacy/page.mdpackages/web/src/app/questions/page.mdpackages/web/src/app/signatories/page.mdpackages/web/src/app/tasks/page.mdpackages/web/src/app/terms/page.mdpackages/web/src/app/treaty/page.mdpackages/web/src/app/vote/page.mdpackages/web/src/app/why/page.mdpackages/web/src/lib/routes.tspackages/web/src/lib/site.tspackages/web/src/lib/tasks/government-task-assignee.tspackages/web/src/lib/tasks/leader-accountability-network.tspackages/web/src/lib/tasks/treaty-signer-network.test.tspackages/web/src/lib/tasks/treaty-signer-network.ts
✅ Files skipped from review due to trivial changes (11)
- packages/web/src/app/vote/page.md
- packages/web/src/app/privacy/page.md
- packages/web/src/app/legal/page.md
- packages/web/src/app/plaintiffs/page.md
- packages/web/src/app/feedback/page.md
- packages/web/src/app/people/page.md
- packages/web/src/app/page.md
- packages/web/src/app/why/page.md
- .gitignore
- packages/web/src/app/tasks/page.md
- packages/web/src/app/signatories/page.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
Code reviewTwo CLAUDE.md compliance issues found: Issue 1 - Banned off-ramp copy still in runtime source File: packages/data/src/referendums/court-of-humanity.ts (line 68) The page.md snapshot committed at packages/web/src/app/court/page.md (line 34) confirms that /court still renders: "The Treaty is the off-ramp. The Court is the road that produces the off-ramp." This is the verbatim bad example cited in the no-startup-bro rule added in this same PR. The TreatyContent.tsx fix is correct but does not reach the actual runtime source — COURT_OF_HUMANITY_TEXT in court-of-humanity.ts line 68. See: optimitron/packages/data/src/referendums/court-of-humanity.ts Lines 67 to 69 in d616400 Fix: Edit line 68 of court-of-humanity.ts — e.g. "The Treaty is the standing settlement offer. The Court enforces it." Issue 2 - "We are building" corporate opener on /feedback File: packages/web/src/app/feedback/page.tsx (lines 76-77) The page.md snapshot at packages/web/src/app/feedback/page.md (line 6) shows: "We are building a decentralized to-do list for humanity: coordinate the work required to end war and disease in the least irritating way possible." "We are building" is a corporate opener banned by the rule added in this PR. CLAUDE.md: "No blather. No 'welcome to', 'let's take a moment'..." and "No startup-bro copy. No... corporate openers (We're building, Let's take a moment)." See: optimitron/packages/web/src/app/feedback/page.tsx Lines 75 to 78 in d616400 Fix: Replace with Wishonia-voice copy. Example: "102 million people died waiting for treatments already proven safe. Tell us what to fix." |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Summary
TreatyContent.tsx:"Next: the enforcement stack. The treaty is the off-ramp. The Court is the road that produces the off-ramp."→ concrete, stakes-named alternative pointing to the class action with the 102M-deaths frame.BrutalCard/SectionContainerbgColor unions, updategetCardColorand other dynamic-color functions, delete unused brutal-* CSS vars, collapse the@theme inlineredirect block, optionalBrutalCard→TreatySectionrename, delete deadARCADE_LABELS); (2) copy audit pass with the four already-found offenders listed inline (TreatyContent, TreasuryAllocationViz:160, WishocracyLinkCard:17, ResourcePromoSection:12).Test plan
pnpm --filter @optimitron/web exec tsc --noEmitclean/treatypage renders the new copy where the "enforcement stack" block was🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores