CLU-603: How we addressed Ben's review feedback (with screenshots)

clu-603-review-response-gist.md

CLU-603: How We Addressed Ben's Review Feedback

PR #2583 · final commit 1d253619

---

Ben's feedback (verbatim summary)

1. Documents has a distinct processing/syncing case — rows are empty not because there are no documents, but because uploads haven't resolved yet. That's not "no documents yet," and collapsing it there was wrong.
2. The five loosely-related props (emptyHeading, emptySubtext, emptyAction, isFiltered, onClearSearch) push inference to call sites and let filter-empty silently fall through to true-empty copy or CTAs.
3. Don't add standalone EmptyState to ExposureTable and InjuryTable — they don't use DataTable, so that creates a parallel implementation path outside the standardized boundary.
4. Add a small focused test for the shared state branching.

---

1. The processing/syncing state — modeled as a first-class variant

Before: DocumentsPanel inlined the syncing case into emptyMessage and showed "No documents yet" heading regardless:

emptyMessage={
  documentsPanel.hasSyncingDocuments
    ? "No documents found. Uploads are processing, refreshing shortly…"
    : "No documents found for this matter."
}
emptyHeading="No documents yet"          // always rendered — wrong for transient state
emptySubtext="Documents uploaded..."

After: Added { reason: "processing" } as a first-class variant in TableEmptyState:

type TableEmptyState =
  | { reason: "empty";      heading: string; subtext?: string; action?: { label: string; onClick: () => void } }
  | { reason: "filtered";   onClear?: () => void }
  | { reason: "processing"; message?: string }

DocumentsPanel now routes explicitly — three states, no bleed-through:

emptyState={
  documentsPanel.deferredSearchTerm
    ? { reason: "filtered", onClear: () => documentsPanel.setSearchTerm("") }
    : documentsPanel.hasSyncingDocuments
    ? { reason: "processing", message: "Uploads are processing, we'll refresh automatically…" }
    : {
        reason: "empty",
        heading: "No documents yet",
        subtext: "Documents uploaded to this matter will appear here.",
        action: canWrite ? { label: "Upload document", onClick: ... } : undefined,
      }
}

The processing variant renders only the transient message — no illustration, no heading, no CTA.

Processing state — transient message only, no illustration, no CTA:

(Simulated via fetch interceptor in local dev — S3 upload is unavailable locally. In production this state appears naturally while a file upload resolves.)

---

2. Loose props replaced by a discriminated union

Before: Five independent optional props:

emptyHeading?: string
emptySubtext?: string
emptyAction?: { label: string; onClick: () => void }
isFiltered?: boolean
onClearSearch?: () => void

Problems:

• isFiltered: true without onClearSearch was valid but broken
• Nothing prevented emptyAction from rendering when isFiltered was true (latent CTA-in-filter-empty bug)
• Every call site had to infer the right combination

After: Single emptyState?: TableEmptyState prop. Mutual exclusion enforced by the type:

• filtered has no action field → CTA in filter-empty is structurally impossible
• processing has no heading or action → true-empty copy can't bleed in
• All 9 panels updated

True-empty state (Documents) — illustration + heading + CTA:

Filter-empty state — "No results found" + "Clear search", no CTA:

After clicking "Clear search" — documents restored:

Read-only user — illustration + heading visible, CTA absent:

---

3. ExposureTable and InjuryTable — reverted

The first pass added a standalone EmptyState guard at the top of each component:

// Was added, then reverted:
if (!exposures || exposures.length === 0) {
  return (
    <EmptyState
      variant="empty"
      heading="No exposure data"
      subtext="Exposure records will appear here once added."
    />
  );
}

Ben's concern: these components don't use DataTable, so adding EmptyState directly creates a second rendering path outside the DataTable → TableEmptyState contract.

Resolution: Both additions were reverted. The components fall back to their pre-existing behavior — a simple text message — until there's a separate design decision.

Exposure — pre-existing simple text, no new EmptyState:

Injury — same:

---

4. Six focused unit tests added to data-table.test.tsx

Added to the existing file (not a new file), covering the state-branching logic:

Test	What it proves
empty renders heading, subtext, CTA	True-empty variant works end to end
filtered shows "No results found" + "Clear search"	Filter-empty variant renders correctly
filtered without onClear omits clear button	Optional onClear handled safely
processing renders its message, not empty/filtered copy	Processing variant is isolated
processing without message uses a default	Default message fallback works
empty without action omits the CTA	Read-only path renders correctly

All 265 unit test suites pass; tsc --noEmit clean.

---