Tighten the duplicate-code check and share more code across the app - #1787
Conversation
Tightens the src/scripts/e2e jscpd config (.jscpd.json) from a 23-token to a 22-token minimum. Because jscpd seeds on a fixed window, that one-token drop surfaced 116 new near-duplicate spans that were previously just under the bar. This removes every one of them so the 0% threshold still holds. Most fixes fall into three shapes, matching how the duplication was really shaped: - Shared types: where two places repeated the same field list or the same function signature, they now share one `type`/`interface`/function-type alias (e.g. WriteInputs, StatementLinks, OrderSpan, SnapshotArgsFn, the settings-registry pruning shape). - Shared helpers/components: genuinely repeated logic was lifted into one plain-language helper or curried factory, and repeated markup into one small component (e.g. errorMessage, withLazyLogger, streamChunks, nonBlankLines, HeaderRow, LabelledParas, HiddenDetailRow, AgentHeader, moneyInto/moneyOutOf). - Import blocks: the few coincidental matches that were only shared import lines are wrapped in the sanctioned jscpd:ignore. Also unifies two overlaps the split surfaced: the duplicate-group form's inputs now reuse the shared TextField (via optional autofocus/required/duplicate flags) instead of a parallel DuplicateField, and admin list tables that lead with a Name column use a namedColumns helper so that column is declared once. No behaviour changes. Existing tests cover every extraction; the attendees hardcoded-string allowlist ratchets 5 -> 4 to reflect the smaller template. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BaLTxRDzaxxQu7xm12khNK
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request consolidates repeated helpers, types, request handling, database operations, booking logic, scripts, and UI rendering across application modules. It also updates duplicate-detection configuration and adds tests for shared lookup, JSON parsing, lazy logging, and recalculation utilities. ChangesShared refactor and UI consolidation
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@scripts/bench/bunny-crypto-smoke.ts`:
- Around line 16-18: Update subtleEnc to call the existing randomIv helper
instead of constructing the 12-byte nonce inline, while preserving the current
nonce handling and encryption behavior.
In `@src/features/api/helpers.ts`:
- Line 2: Update parseApiJsonBody to validate the result of JsonBodyReader
before returning it, rejecting null and other non-record JSON values while
preserving valid plain-record payloads. Follow the existing validation pattern
in src/features/auth.ts so route schema checks receive only the promised record
shape.
In `@src/shared/env.ts`:
- Around line 14-15: Update the withLazyLogger usage in isReadOnly() so the
promise remains fire-and-forget while attaching a rejection handler for dynamic
import or callback failures. Keep isReadOnly() synchronous and preserve its
existing return behavior.
In `@src/shared/forms.tsx`:
- Around line 741-752: Move the ReturnUrlField component out of the oversized
forms module into a focused component file, preserving its public export, props,
rendering behavior, and documentation. Remove its local definition from
forms.tsx and import ReturnUrlField there from the new module.
In `@src/shared/payment-helpers.ts`:
- Around line 464-477: Replace the imperative for...of traversal in
collectPackedFields with the repository’s curried `#fp` collection helper, first
confirming the exact utility name and signature from existing usage. Preserve
PACKED_KEYS iteration, readValue invocation, keep-based filtering, and the fresh
Record<string, string> result.
In `@src/ui/templates/admin/attendees.tsx`:
- Around line 114-141: Update the registration date label in the attendee
details component to use the existing t translation function, and add the
corresponding catalog entry if missing; preserve the current label meaning and
formatting.
In `@src/ui/templates/admin/recalculate-rows.ts`:
- Around line 1-23: Update buildRecalculateRows so fields is typed with name:
Name rather than name: string, binding each field name to the snapshot keys;
then remove the name as Name cast and use field.name directly when indexing
snapshot and formatting values.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cb75ae54-b2c4-43b0-984b-9a328bd7a880
📒 Files selected for processing (167)
.jscpd.jsone2e-payments/src/flow.tse2e-payments/src/order-flow.tsscripts/bench/bunny-crypto-smoke.tsscripts/bench/cold-start/first-request-child.tsscripts/bench/cold-start/first-request.tsscripts/bench/cold-start/serve-root.tsscripts/bunny-regions.tsscripts/edge-bundle-lib.tsscripts/mutation/isolation-state.tsscripts/mutation/isolation.tsscripts/mutation/runner.tsscripts/mutation/summary.tsscripts/precommit/git.tsscripts/precommit/mutation-step.tsscripts/stream-lines.tsscripts/stripe-mock.tsscripts/stripe-mock/install.tsscripts/unit-tests-report-lib.tssrc/features/admin/actions.tssrc/features/admin/api.tssrc/features/admin/attendee-form-routes.tssrc/features/admin/attendee-logistics-routes.tssrc/features/admin/attendee-page-data.tssrc/features/admin/attendees-edit.tssrc/features/admin/attendees-merge.tssrc/features/admin/catalog-transfer/export.tssrc/features/admin/catalog-transfer/import.tssrc/features/admin/dashboard.tssrc/features/admin/find-group.tssrc/features/admin/groups.tssrc/features/admin/listing-page-data.tssrc/features/admin/listing-qr.tssrc/features/admin/listings-edit.tssrc/features/admin/listings-view.tssrc/features/admin/load-listing.tssrc/features/admin/modifiers.tssrc/features/admin/money-adjust.tssrc/features/admin/news.tssrc/features/admin/settings-email-templates.tssrc/features/admin/settings-email.tssrc/features/admin/settings-sms.tssrc/features/admin/settings-statuses.tssrc/features/admin/site-content-create.tssrc/features/admin/site-content.tssrc/features/admin/site-pages.tssrc/features/api/helpers.tssrc/features/api/json-body.tssrc/features/api/listings.tssrc/features/auth.tssrc/features/feeds.tssrc/features/flash-for-page.tssrc/features/index.tssrc/features/join.tssrc/features/public/discovery.tssrc/features/public/ticket-page-url.tssrc/features/public/ticket-payment.tssrc/features/public/ticket-submit.tssrc/features/public/ticket-submit/paths.tssrc/features/public/unsubscribe.tssrc/shared/accounting/manual-entries.tssrc/shared/app-forms.tssrc/shared/booking/capacity-tree.tssrc/shared/booking/fold-tree.tssrc/shared/booking/node-order.tssrc/shared/booking/order-lines.tssrc/shared/booking/order-span.tssrc/shared/booking/package-cap.tssrc/shared/booking/price-tree.tssrc/shared/bunny-cdn.tssrc/shared/columns/attendee-columns.tssrc/shared/dates.tssrc/shared/db/attendees/balance.tssrc/shared/db/attendees/create.tssrc/shared/db/attributes.tssrc/shared/db/client.tssrc/shared/db/groups.tssrc/shared/db/holidays.tssrc/shared/db/id-and-name-columns.tssrc/shared/db/listing-parents.tssrc/shared/db/migrations.tssrc/shared/db/migrations/2026-06-14_rename_events_to_listings.tssrc/shared/db/migrations/2026-06-18_contact_preferences.tssrc/shared/db/migrations/booking-slot-index.tssrc/shared/db/migrations/master-query.tssrc/shared/db/migrations/rename-utils.tssrc/shared/db/migrations/schema-sync.tssrc/shared/db/modifier-resolve.tssrc/shared/db/news-posts.tssrc/shared/db/query-log.tssrc/shared/db/questions/attendee-answers/save.tssrc/shared/db/settings.tssrc/shared/db/settings/raw-writes.tssrc/shared/db/site-pages.tssrc/shared/db/slugged-content-input.tssrc/shared/db/system-notes.tssrc/shared/db/users.tssrc/shared/deno-deploy-api.tssrc/shared/env.tssrc/shared/error-message.tssrc/shared/flash-fields.tssrc/shared/forms.tsxsrc/shared/images/transcode.tssrc/shared/lazy-logger.tssrc/shared/limits.tssrc/shared/lines.tssrc/shared/listing-attribute-filter.tssrc/shared/listings-actions.tssrc/shared/listings-by-id.tssrc/shared/payment-helpers.tssrc/shared/payments.tssrc/shared/refund-ledger.tssrc/shared/rest/crud-api.tssrc/shared/rest/resource.tssrc/shared/settings/registry.tssrc/shared/square.tssrc/shared/storage.tssrc/shared/stream-chunks.tssrc/shared/stripe.tssrc/shared/turso-api.tssrc/shared/webhook-verification.tssrc/ui/client/admin/markdown-editor-toolbar.tssrc/ui/client/dom.tssrc/ui/client/order.tssrc/ui/templates/admin/activityLog.tsxsrc/ui/templates/admin/admin-page.tsxsrc/ui/templates/admin/agent-header.tsxsrc/ui/templates/admin/api-keys.tsxsrc/ui/templates/admin/attendee-detail.tsxsrc/ui/templates/admin/attendee-form.tsxsrc/ui/templates/admin/attendees.tsxsrc/ui/templates/admin/builder.tsxsrc/ui/templates/admin/built-sites.tsxsrc/ui/templates/admin/built-sites/list-parts.tsxsrc/ui/templates/admin/bulk-actions.tsxsrc/ui/templates/admin/deliveries.tsxsrc/ui/templates/admin/groups.tsxsrc/ui/templates/admin/hidden-row.tsxsrc/ui/templates/admin/ledger.tsxsrc/ui/templates/admin/ledger/statement.tsxsrc/ui/templates/admin/listing-defaults.tsxsrc/ui/templates/admin/listings/aggregates.tsxsrc/ui/templates/admin/listings/details.tsxsrc/ui/templates/admin/listings/form-sections.tsxsrc/ui/templates/admin/listings/roster.tsxsrc/ui/templates/admin/logout.tsxsrc/ui/templates/admin/modifiers/aggregates.tsxsrc/ui/templates/admin/money-summary.tsxsrc/ui/templates/admin/nav.tsxsrc/ui/templates/admin/privacy.tsxsrc/ui/templates/admin/recalculate-rows.tssrc/ui/templates/attendee-table.tsxsrc/ui/templates/components/data-table.tsxsrc/ui/templates/components/error.tsxsrc/ui/templates/components/header-row.tsxsrc/ui/templates/components/heading-layout.tsxsrc/ui/templates/components/labelled-para.tsxsrc/ui/templates/components/price-input.tsxsrc/ui/templates/components/prose-heading.tsxsrc/ui/templates/components/success-complete-page.tsxsrc/ui/templates/components/text-field.tsxsrc/ui/templates/payment.tsxsrc/ui/templates/public/reservations/contact-fields.tssrc/ui/templates/public/reservations/listing-rows.tssrc/ui/templates/public/unsubscribe.tsxsrc/ui/templates/tickets.tsxtest/lib/i18n-coverage.test.ts
Follow-up to the duplication cleanup. Zooming out on the extracted helpers surfaced several idioms that were still repeated in more places than the original clone pass touched, plus a handful of small correctness/quality points from the PR review. Deeper consolidation: - Add one generic `byId` helper to #fp and route every "index items by their id" site through it: the old `listingsById`, two separate `listingsByIdMap` definitions, and several inline `new Map(x.map(i => [i.id, i]))` copies. The single-purpose `listings-by-id.ts` is deleted. - Use the shared `errorMessage` helper at the 12 remaining places that still spelled out `err instanceof Error ? err.message : String(err)` by hand. - Share the request Content-Type normaliser: a third site (the always-loaded middleware) used `|| ""` so the duplication checker never flagged it. The normaliser now lives in the middleware and every caller reuses it. Review fixes: - Reject non-record JSON bodies (e.g. `null`) in the public API parser, the same way the cookie-auth parser already does, so malformed bodies fail with a clear 400 instead of throwing deeper in. - The lazy logger no longer lets a failed load leak an unhandled rejection. - Give the recalculate-rows builder a loud, named error when a snapshot field is missing, instead of a silent cast that could crash on a bad pairing. - Wire the attendee "Registered:" label through the message catalog. - Move the return-url form field into its own file, and reuse the shared random-nonce helper in the crypto benchmark. New regression tests cover the JSON-body guard, the lazy-logger swallow, the recalculate-rows guard, and the new `byId` helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BaLTxRDzaxxQu7xm12khNK
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/shared/lazy-logger.ts`:
- Around line 12-13: Update withLazyLogger to accept callbacks returning void or
Promise<void>, then await the callback invocation inside the existing try/catch
so asynchronous rejections are handled by that catch. Preserve the current lazy
logger import and synchronous callback behavior.
In `@test/fp.test.ts`:
- Around line 520-525: Split the combined test around byId into two independent
tests: one verifying that later items with the same id win, and another
verifying that empty input returns an empty Map. Preserve the existing
assertions and behavior while giving each test a focused name.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c00046e3-e924-4945-905a-53c3243a1c7a
📒 Files selected for processing (35)
scripts/bench/bunny-crypto-smoke.tsscripts/safe-upgrade.tssrc/features/admin/attendee-form-routes.tssrc/features/admin/attendee-page-data.tssrc/features/admin/calendar-csv.tssrc/features/admin/catalog-transfer/import.tssrc/features/admin/servicing.tsxsrc/features/admin/settings-superuser.tssrc/features/api/helpers.tssrc/features/auth.tssrc/features/feeds.tssrc/features/index.tssrc/features/middleware.tssrc/features/public/discovery.tssrc/fp.tssrc/shared/accounting/store.tssrc/shared/botpoison.tssrc/shared/db/attendees/capacity.tssrc/shared/db/groups.tssrc/shared/db/migrations.tssrc/shared/email-renderer.tssrc/shared/email.tssrc/shared/forms.tsxsrc/shared/lazy-logger.tssrc/shared/logger.tssrc/shared/return-url-field.tsxsrc/shared/site-pages/core.tssrc/shared/webhook.tssrc/ui/templates/admin/attendees.tsxsrc/ui/templates/admin/recalculate-rows.tssrc/ui/templates/attendee-table.tsxtest/fp.test.tstest/lib/api-json-body.test.tstest/lib/lazy-logger.test.tstest/lib/recalculate-rows.test.ts
- withLazyLogger now accepts void | Promise<void> and awaits the callback, so a rejecting async callback is caught by the same try/catch instead of leaking an unhandled rejection. Adds a regression test for the async-reject path. - Split the combined byId test into one-behaviour-each tests (duplicate-id precedence; empty input), per the test standards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BaLTxRDzaxxQu7xm12khNK
…isfy stricter cpd Main's tightened jscpd threshold (#1787) flagged the paired thin wrappers and the duplicated attendee-by-id load once merged in. Restructure the reader layer so there's a single SQL builder: - attendeeBatchStatement is now the one place a declared query becomes SQL; getAttendees runs it, and the batch listing readers embed it. Removes the selectAttendees/selectAttendeeOrNull pair and getAttendeeRow. - getAttendeeRaw is the single attendee-by-id read; attendees-edit and attendees-merge reuse it instead of re-inlining the same getAttendees call. - Collapse the singular attendeeId/listingId filters into attendeeIds/listingIds (a single lookup is a one-element array), matching the repo's "one path for one-or-many" rule and dropping the duplicate WHERE branches. getAttendeesByIds keeps its kind='attendee' filter — that's the exact behaviour the pre-existing query had; whether the logistics run sheet should include servicing events is a separate product decision, recorded in TODO.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G24MzgR46DN4ju1srifKaq
What this does
The project has a checker that fails the build if the same code appears twice. This makes it a little stricter, then removes all the copy-paste it uncovers — and, while in there, pulls several more repeated patterns into single shared helpers.
None of this changes anything people see on the site. It's a tidy-up: the same behaviour, written once instead of many times, so there's less code to maintain and fewer places for the same bug to hide.
The stricter check
The duplicate-code checker only looks at chunks of code above a certain size. This lowers that size by one notch. Because of the way the tool scans, that one small step uncovered 116 near-identical chunks that had been sitting just under the old limit. Every one of them is now removed — the project keeps duplication at zero, so they had to be.
The fixes follow one rule: share the code, don't hide the warning. Most were one of three shapes:
Going further
Once the obvious copies were gone, a few patterns turned out to be repeated in even more places than the checker had flagged. Those got folded into single helpers too:
Review feedback
The automated reviewer raised seven points; six are folded in and one was a deliberate no-change (the loop it flagged is the style this project actually prefers). The fixes worth calling out:
null) with a clear error, instead of failing further along.Safety
All automated checks pass: type-checking, linting, the duplicate-code check (back to zero), the copy/wording check, the build, and the full test suite at 100% coverage. New tests were added for each of the behaviour fixes above.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BaLTxRDzaxxQu7xm12khNK
Summary by CodeRabbit
New Features
Bug Fixes