Skip to content

Commit a393365

Browse files
ToddHebebrandTodd Hebebrandclaude
authored
feat(billing): normalize the invoice editor onto the quote save grammar (#2829)
Brings the **Invoice** editor/detail/workspace onto the same save/UX grammar the **Quote** side already had, de-duplicates the two, and fixes the save-gate bugs that surfaced once both surfaces were compared side by side. Two commits: the normalization, then the fixes from a full multi-agent review of it. ## The normalization **New shared modules** - `billing/shared/saveCues.tsx` — the blur-to-save grammar (`useSavedFlash`, `SrSaved`, `fieldRing`, `seamless`, `unsavedHintId`), lifted out of `quoteEditorShared`. The dirty signal moved from a ring to border-color because the focus ring occupies the box-shadow channel and was painting over the cue on exactly the field being edited. - `billing/shared/OrgCombobox.tsx` — typeahead org picker replacing a native `<select>`, which stops scaling at MSP org counts. **Invoices** — autosave hint + last-saved indicator, undo line-delete, show/hide cost & margin, aria label on the horizontally scrolling lines table. **Quotes** — send-dialog warnings ($0 total, no Stripe, missing billing contact, deposit-without-Stripe), undo-send / send-now, save-timeout copy. All five locales updated in parity. ## The fixes The review found a pair of opposite bugs sharing one cause: the editors reported *"a field is dirty"* and *"a save is in flight"* as the same signal. - A **failed line save** left an amber "unsaved" border while its in-flight key cleared — so the editor read as quiet and Issue stayed enabled, numbering the invoice at the pre-edit money. - A **failed notes/terms save** left the field dirty forever, pinning *"Saving changes… Issue unlocks when everything is saved"* over a save that had already given up, with no way out and no pointer to the field. The gate is now two signals: `savePending` (in flight, resolves on its own → a click queues behind it) and `unsavedFieldLabel` (save failed and the field is still dirty → the click is refused and the field is named). That second signal is the **intersection** of "last attempt failed" and "still differs from the server", and needs both halves. Failure alone keeps blocking after the user reverts by hand — the commit path short-circuits on an unchanged value, so no fresh attempt ever clears the flag. Dirty alone fires during the round-trip between a successful save and its refetch, refusing an Issue that is perfectly valid. The same bug existed twice more on the quote side (`QuoteEditor` terms, `QuoteHeaderMeta` title); both fixed identically. `QuoteActions` also gains the failure nonce and the spinner rule it never received — its queued Send fired on post-failure quiescence, and it spun forever on a dirty field. **Also** - Queued Issue/Send capture `atFailureNonce` rather than relying on two effects observing two props in a lucky order. The old comment credited *"effects run in definition order"*; what actually protected it was `savePending` lagging by one commit. Right code, wrong reason, one refactor away from a race on an irreversible money action. - The queue re-checks `hasVisibleLines` before firing — flushing the deferred delete of the last visible line is itself what the click triggered. - A partial delete-flush reports what landed instead of only "canceled". - `runScoped` no longer stamps "Saved 3:14 PM" on add-line validation failures. - `loadCatalog` guards its parse; an empty picker and a broken one now read differently. - `QuoteBlockCard`'s dirty/saved cue rendered **nothing** — `fieldRing` emits border-color and the element carried no border width. `seamless(state, invalid)` stops the error branch dropping the only focus indicator on an invalid field. Five quote sites moved off `transition-shadow`. - `OrgCombobox`: the combobox role moves to the search input (the element that holds focus), `aria-activedescendant` added, `aria-selected` tracks the cursor — arrow navigation was silent to assistive tech. The clone dialog's `<label>` wrapper became a `<div>`: a click on the popover's own chrome forwarded to the trigger and closed it mid-search. `orgComboboxOptions()` replaces a duplicated prepend and its two unsound casts. - The no-billing-contact hint deep-links to `#billing`; without it the user landed on General with no field in sight. - `ContractEditor` migrates onto `shared/saveCues`, dropping a third divergent copy still on `ring-1 ring-warning` (~2.3:1, below the 3:1 non-text minimum). - Comments asserting rationale the code didn't implement are corrected: the border-width precondition, the `transition-colors` pairing, "one place it evolves", the CatalogItemPicker claim. ## Testing - **503 files / 4531 tests pass**, run twice to rule out flakes - `tsc --noEmit` clean - Locale parity: **1213 keys × 5 locales**, no drift New: `shared/saveCues.test.tsx` (the 1500ms expiry and unmount teardown shipped untested) and `QuoteHeaderMeta.title.test.tsx`. Added coverage for the same-render failure+quiescence race, the failed-delete-flush cancel, the quote 10s backstop, `DocumentWorkspace`'s slots, the margin memory mirror under blocked storage, and the catalog error state. **Reviewer note:** three existing tests encoded the old contract (dirty ⇒ "saving") and were rewritten rather than preserved. That's a deliberate behavior change and the right place to push back if you disagree with it. Scope: `apps/web` only — no API, schema, migrations, or cascade lists touched. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Todd Hebebrand <[email protected]> Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
1 parent 83556c8 commit a393365

53 files changed

Lines changed: 4181 additions & 660 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
// Save-quiescence gating on the Issue buttons (the quote editor's Send
2+
// contract, normalized onto invoices): while the editor reports pending edits,
3+
// an Issue click is never dead — it queues and fires the moment the editor goes
4+
// quiescent, with a visible hint explaining the hold.
5+
import { act, fireEvent, render, screen, waitFor } from '@testing-library/react';
6+
import { beforeEach, describe, expect, it, vi } from 'vitest';
7+
8+
import InvoiceActions from './InvoiceActions';
9+
import type { InvoiceDetail } from './invoiceTypes';
10+
import { fetchWithAuth } from '../../stores/auth';
11+
12+
vi.mock('../../stores/auth', () => ({
13+
fetchWithAuth: vi.fn(),
14+
useAuthStore: Object.assign(
15+
(selector: (s: { user: { permissions: { resource: string; action: string }[] } }) => unknown) =>
16+
selector({ user: { permissions: [{ resource: '*', action: '*' }] } }),
17+
{ getState: () => ({ tokens: null }) },
18+
),
19+
}));
20+
vi.mock('@/lib/navigation', () => ({ navigateTo: vi.fn() }));
21+
const showToastMock = vi.fn();
22+
vi.mock('../shared/Toast', () => ({ showToast: (a: unknown) => showToastMock(a) }));
23+
24+
const fetchMock = vi.mocked(fetchWithAuth);
25+
const json = (payload: unknown, ok = true, status = ok ? 200 : 500): Response =>
26+
({ ok, status, statusText: ok ? 'OK' : 'ERR', json: vi.fn().mockResolvedValue(payload) }) as unknown as Response;
27+
28+
const visibleLine: InvoiceDetail['lines'][number] = {
29+
id: 'line-1', invoiceId: 'inv-1', sourceType: 'manual', parentLineId: null, catalogItemId: null,
30+
name: null, description: 'Consulting', quantity: '2.00', unitPrice: '50.00', costBasis: null, revenueAllocation: null,
31+
taxable: false, customerVisible: true, lineTotal: '100.00', isUnapprovedTime: false, sortOrder: 1,
32+
};
33+
34+
function detail(): InvoiceDetail {
35+
return {
36+
invoice: {
37+
id: 'inv-1', invoiceNumber: null, orgId: 'org-1', siteId: null, status: 'draft',
38+
currencyCode: 'USD', issueDate: null, dueDate: null, sentAt: null, subtotal: '100.00', taxRate: null,
39+
taxTotal: '0.00', total: '100.00', amountPaid: '0.00', balance: '100.00', billToName: 'Acme',
40+
notes: '', termsAndConditions: null, sellerSnapshot: null, createdAt: '2026-06-01T00:00:00Z',
41+
},
42+
lines: [visibleLine],
43+
};
44+
}
45+
46+
const issuePosted = () =>
47+
fetchMock.mock.calls.some((c) => String(c[0]) === '/invoices/inv-1/issue' && (c[1] as RequestInit)?.method === 'POST');
48+
49+
beforeEach(() => {
50+
vi.clearAllMocks();
51+
fetchMock.mockImplementation(async () => json({ data: {} }));
52+
});
53+
54+
describe('InvoiceActions — savePending gating', () => {
55+
it('shows the saving hint and holds an Issue click until quiescence, then fires it', async () => {
56+
const { rerender } = render(<InvoiceActions detail={detail()} variant="header" savePending />);
57+
58+
// Visible reason for the hold (both a hint paragraph and the button title).
59+
expect(screen.getByTestId('invoice-issue-saving-hint')).toBeInTheDocument();
60+
expect(screen.getByTestId('invoice-issue')).toHaveAttribute(
61+
'aria-describedby', 'invoice-issue-held-hint-header',
62+
);
63+
64+
// Clicking Issue while saving queues it — nothing fires yet.
65+
fireEvent.click(screen.getByTestId('invoice-issue'));
66+
expect(issuePosted()).toBe(false);
67+
68+
// The editor goes quiescent → the queued Issue fires without a second click.
69+
rerender(<InvoiceActions detail={detail()} variant="header" savePending={false} />);
70+
await waitFor(() => expect(issuePosted()).toBe(true));
71+
});
72+
73+
it('holds an Issue & Send click until quiescence, then opens the confirm dialog (never auto-sends)', async () => {
74+
const { rerender } = render(<InvoiceActions detail={detail()} variant="header" savePending />);
75+
76+
fireEvent.click(screen.getByTestId('invoice-issue-send'));
77+
expect(screen.queryByTestId('invoice-issue-send-confirm')).not.toBeInTheDocument();
78+
expect(issuePosted()).toBe(false);
79+
80+
rerender(<InvoiceActions detail={detail()} variant="header" savePending={false} />);
81+
// Quiescence opens the CONFIRM dialog — the email still needs an explicit yes.
82+
await waitFor(() => expect(screen.getByTestId('invoice-issue-send-confirm')).toBeInTheDocument());
83+
expect(issuePosted()).toBe(false);
84+
});
85+
86+
it('renders no saving hint and issues immediately when savePending is false', async () => {
87+
render(<InvoiceActions detail={detail()} variant="header" />);
88+
expect(screen.queryByTestId('invoice-issue-saving-hint')).not.toBeInTheDocument();
89+
fireEvent.click(screen.getByTestId('invoice-issue'));
90+
await waitFor(() => expect(issuePosted()).toBe(true));
91+
});
92+
93+
it('flushes deferred deletions when Issue is clicked while saving', () => {
94+
const onIssueWhilePending = vi.fn();
95+
render(<InvoiceActions detail={detail()} variant="header" savePending onIssueWhilePending={onIssueWhilePending} />);
96+
fireEvent.click(screen.getByTestId('invoice-issue'));
97+
expect(onIssueWhilePending).toHaveBeenCalledTimes(1);
98+
});
99+
100+
it('cancels a queued Issue when a save FAILURE is reported — quiescence after a failure must not issue', async () => {
101+
// A failed delete-flush restores its rows and a failed line blur-save
102+
// clears its in-flight key: both read as "quiet". Firing the queue then
103+
// would number an invoice that contradicts what the user last saw.
104+
const { rerender } = render(
105+
<InvoiceActions detail={detail()} variant="header" savePending saveFailureNonce={0} />,
106+
);
107+
fireEvent.click(screen.getByTestId('invoice-issue'));
108+
expect(issuePosted()).toBe(false);
109+
110+
// The failure signal lands (nonce bump), then the editor goes quiescent.
111+
rerender(<InvoiceActions detail={detail()} variant="header" savePending saveFailureNonce={1} />);
112+
await waitFor(() => expect(showToastMock).toHaveBeenCalledWith(expect.objectContaining({ type: 'error' })));
113+
rerender(<InvoiceActions detail={detail()} variant="header" savePending={false} saveFailureNonce={1} />);
114+
115+
// The queue was dropped by the failure — quiescence fires nothing.
116+
await act(async () => {});
117+
await waitFor(() => expect(screen.queryByTestId('invoice-issue-saving-hint')).not.toBeInTheDocument());
118+
expect(issuePosted()).toBe(false);
119+
});
120+
121+
it('cancels a queued Issue when the failure and quiescence arrive in the SAME render', async () => {
122+
// The dangerous shape: one commit in which the nonce bumps AND savePending
123+
// goes false. Splitting these across two rerenders proves nothing — the
124+
// first is inert while savePending is still true, so the assertion passes
125+
// even if the guard is ordered wrongly. Both props must flip at once.
126+
const { rerender } = render(
127+
<InvoiceActions detail={detail()} variant="header" savePending saveFailureNonce={0} />,
128+
);
129+
fireEvent.click(screen.getByTestId('invoice-issue'));
130+
expect(issuePosted()).toBe(false);
131+
132+
rerender(<InvoiceActions detail={detail()} variant="header" savePending={false} saveFailureNonce={1} />);
133+
134+
await waitFor(() => expect(showToastMock).toHaveBeenCalledWith(expect.objectContaining({ type: 'error' })));
135+
await act(async () => {});
136+
expect(issuePosted()).toBe(false);
137+
});
138+
139+
it('refuses (does not queue) an Issue click when a field failed to save and stays dirty', async () => {
140+
// Queueing here would wait on a save that is never coming. The hint and the
141+
// toast must name the field instead of promising a save in progress.
142+
render(<InvoiceActions detail={detail()} variant="header" unsavedFieldLabel="Notes" />);
143+
144+
expect(screen.queryByTestId('invoice-issue-saving-hint')).not.toBeInTheDocument();
145+
expect(screen.getByTestId('invoice-issue-unsaved-hint')).toHaveTextContent('Notes');
146+
147+
fireEvent.click(screen.getByTestId('invoice-issue'));
148+
await waitFor(() => expect(showToastMock).toHaveBeenCalledWith(
149+
expect.objectContaining({ type: 'warning', message: expect.stringContaining('Notes') }),
150+
));
151+
// No queue, no spinner, and above all no POST.
152+
expect(issuePosted()).toBe(false);
153+
});
154+
155+
it('re-checks the unsaved-field refusal before firing a queued Issue', async () => {
156+
// The click gate tests savePending FIRST, so a click during a deferred
157+
// delete queues even while an earlier failed price save keeps a field
158+
// dirty (its nonce bump happened BEFORE the click, so the captured nonce
159+
// never changes). When the flush succeeds and the editor goes quiet, the
160+
// queue must re-check the label and refuse — firing would issue the
161+
// invoice at the pre-edit money.
162+
const { rerender } = render(
163+
<InvoiceActions detail={detail()} variant="header" savePending unsavedFieldLabel="Notes" />,
164+
);
165+
fireEvent.click(screen.getByTestId('invoice-issue'));
166+
expect(issuePosted()).toBe(false);
167+
168+
// Quiescence arrives with the field STILL flagged unsaved.
169+
rerender(
170+
<InvoiceActions detail={detail()} variant="header" savePending={false} unsavedFieldLabel="Notes" />,
171+
);
172+
173+
// The refusal surfaces exactly like an immediate click's would — a warning
174+
// naming the field — and above all no POST.
175+
await waitFor(() => expect(showToastMock).toHaveBeenCalledWith(
176+
expect.objectContaining({ type: 'warning', message: expect.stringContaining('Notes') }),
177+
));
178+
await act(async () => {});
179+
expect(issuePosted()).toBe(false);
180+
// The queue is gone (no spinner promising a fire that isn't coming).
181+
expect(screen.getByTestId('invoice-issue').querySelector('.animate-spin')).toBeNull();
182+
});
183+
184+
it('re-checks the visible-lines precondition before firing a queued Issue', async () => {
185+
// The queue can outlive the gate: flushing the deferred delete of the last
186+
// customer-visible line is itself what the Issue click triggered.
187+
const withLine = detail();
188+
const { rerender } = render(<InvoiceActions detail={withLine} variant="header" savePending />);
189+
fireEvent.click(screen.getByTestId('invoice-issue'));
190+
191+
const emptied = detail();
192+
emptied.lines = [{ ...visibleLine, customerVisible: false }];
193+
rerender(<InvoiceActions detail={emptied} variant="header" savePending={false} />);
194+
195+
await waitFor(() => expect(showToastMock).toHaveBeenCalledWith(expect.objectContaining({ type: 'warning' })));
196+
expect(issuePosted()).toBe(false);
197+
});
198+
199+
it('drops a queued Issue after a bounded wait when a save stays in flight, with an honest still-saving toast', async () => {
200+
// With failures handled by the nonce signal, the timeout is the slow-save
201+
// backstop — its copy must not claim a failure nobody saw.
202+
vi.useFakeTimers({ shouldAdvanceTime: true });
203+
try {
204+
render(<InvoiceActions detail={detail()} variant="header" savePending />);
205+
fireEvent.click(screen.getByTestId('invoice-issue'));
206+
expect(issuePosted()).toBe(false);
207+
208+
await act(async () => { await vi.advanceTimersByTimeAsync(15_000); });
209+
210+
expect(showToastMock).toHaveBeenCalledWith(expect.objectContaining({ type: 'warning' }));
211+
// The queue is gone: even if the editor later goes quiescent, nothing fires.
212+
expect(issuePosted()).toBe(false);
213+
} finally {
214+
vi.useRealTimers();
215+
}
216+
});
217+
});

0 commit comments

Comments
 (0)