fix/sites-polish
needs attentionviewing older commitbf60412 · fullPR #290reviewed 2026-07-09 04:33 UTC3H · 7M · 8L · 4I- Purpose
- Polish follow-up to sites 1:N (#279): fix two regressions surfaced on preview.
- Goal
- Replace full-width Agrupar switch with compact toolbar control; ensure site-split copies assignees so teammates keep visibility of moved RPUs.
- Sub-goals
- SG-1: compact GroupControl popover in toolbar replacing Switch row
- SG-2: assignee-copy on site split via bulk INSERT-SELECT in same transaction
- What
- New GroupControl component; groupBySite state lifted to page; createSiteShell extended with optional copyAssigneesFromSiteId; handler org-gates source site; new copyAllToSite bulk query in site-assignee.
- Why
- Agrupar Switch consumed a full-width row and looked inconsistent. Split path left new sites unassigned causing visibility gaps for teammates.
- Areas
- apps/platform+99−25domains/core+43−1packages/api+9−0
- Blast
- 10 files, +151/-26 across 3 areas. No schema migration. Additive optional API field.
Findings · 24
correctness5
TOCTOU: findByPublicId pre-flight outside transaction — soft-deleted source site passes org-gate
apps/platform/src/api/handlers/sites.handler.ts:71
Handler reads source site on bare database before opening the shell transaction. findByPublicId has no isNull(deletedAt) guard, so a just-soft-deleted site passes org-gate; copyAllToSite then copies 0 assignees silently.
copyAllToSite result extraction uses a driver-shape cast that may return wrong count
domains/core/src/site-assignee/site-assignee.queries.ts:244
Dual-branch cast hedges between postgres.js and node-pg. Rows are still inserted correctly; only the returned count may be wrong.
collapsedSites not reset when groupBySite toggled off then on
apps/platform/src/app/[locale]/(dashboard)/bills/contratos/_components/ContractsTable.tsx:165
Inline CreateSiteCommand intersection in handler duplicates shell type
apps/platform/src/api/handlers/sites.handler.ts:84
copyAllToSite failure aborts site creation with generic 500
domains/core/src/site/site.shells.ts:84
security3
Soft-deleted source site passes org-gate — unintended assignee copy possible
apps/platform/src/api/handlers/sites.handler.ts:71
Assignee copy permitted at member role
apps/platform/src/api/handlers/sites.handler.ts:59
SQL in copyAllToSite is fully parameterized — no injection risk
domains/core/src/site-assignee/site-assignee.queries.ts:236
conventions4
Handler calls siteQueries.findByPublicId directly — canonical form says handlers use shells only
apps/platform/src/api/handlers/sites.handler.ts:64
createSiteShell accepts anonymous inline intersection instead of named command type
domains/core/src/site/site.shells.ts:51
JSDoc on copyAllToSite partially documents the obvious
domains/core/src/site-assignee/site-assignee.queries.ts:226
GroupControl hardcodes Spanish strings instead of next-intl
apps/platform/src/app/[locale]/(dashboard)/bills/contratos/_components/GroupControl.tsx:40
tests5
copyAllToSite has zero integration test coverage
domains/core/src/site-assignee/site-assignee.queries.ts:236
Handler org-gate for copyAssigneesFromSitePublicId is untested
apps/platform/src/api/handlers/sites.handler.ts:70
createSiteShell assignee-copy path untested at shell layer
domains/core/src/site/site.shells.ts:84
ContractsTable memo test does not exercise groupBySite=true
apps/platform/src/app/[locale]/(dashboard)/bills/contratos/_components/ContractsTable.tsx:144
No assertion that copyAssigneesFromSiteId is excluded from decideCreateSite input
domains/core/src/site/site.shells.ts:65
improvement7
Inline type intersection should be a named type shared by shell and handler
apps/platform/src/api/handlers/sites.handler.ts:84
Conditional spread should be a direct field — type already accepts undefined
apps/platform/src/api/handlers/sites.handler.ts:90
copyAllToSite return count is dead weight — consider void
domains/core/src/site-assignee/site-assignee.queries.ts:231
GroupControl file-level JSDoc restates the obvious
apps/platform/src/app/[locale]/(dashboard)/bills/contratos/_components/GroupControl.tsx:3
GroupControl open state is partially redundant with Radix native close
apps/platform/src/app/[locale]/(dashboard)/bills/contratos/_components/GroupControl.tsx:23
Hardcoded 1 badge in GroupControl trigger is premature UX
apps/platform/src/app/[locale]/(dashboard)/bills/contratos/_components/GroupControl.tsx:38
Double cast in copyAllToSite should use typed execute<T> pattern
domains/core/src/site-assignee/site-assignee.queries.ts:244