fix(ui): keep the stored company when the company request fails (#11477)

An error is not an answer, and this branch is destructive.

`companiesListQueryOptions` sets `retry: false`, so a request that fails before
ever succeeding leaves `data` undefined - which `CompanyProvider` defaults to
`{ companies: [], unauthorized: false }`. That is indistinguishable from "this
account was asked, and owns nothing", so `shouldClearStoredCompanySelection`
returned true and the effect removed the customer's stored company. With
`refetchOnWindowFocus: true`, a blip on focus during a cold load was enough,
and the next visit drops them onto whichever company sorts first.

The predicate now takes `errored` and refuses to clear on it. Required rather
than optional, so the compiler made both existing call sites state their
answer instead of inheriting a default.

Not clearing costs nothing: a stored id that no longer resolves is ignored by
`resolveBootstrapCompanySelection`, which checks it against the current list
before using it. Clearing wrongly costs the customer's selection, which cannot
be recovered.

Scoped deliberately. This file had been described as carrying the same defect
as the onboarding draft gate and the sign-out sweep, and that was overstated.
Those two *trusted* a stale list to answer "does this account own this
company?". This one validates membership against the current list and only
picks a default, so a stale list here self-corrects rather than leaking. The
failed-request branch is the part that is genuinely wrong, and it is the only
part changed. The transient re-decision during a background refetch is real,
self-correcting, and left alone.

Tested at both levels, because the predicate alone would not have caught it:
the provider is what defaults a failed request to an empty list, so the wiring
is where the decision goes wrong. Removing the guard fails both.

ui typecheck clean; full ui suite 4016 pass, with only the timezone-dependent
IssueProperties failure already present on master.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tonio 2026-08-16 09:58:46 -07:00 committed by GitHub
parent 0023c5c4a6
commit e384d0a2bd
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 64 additions and 2 deletions

View File

@ -137,6 +137,7 @@ describe("shouldClearStoredCompanySelection", () => {
companies: [],
isLoading: false,
unauthorized: true,
errored: false,
})).toBe(false);
});
@ -145,8 +146,23 @@ describe("shouldClearStoredCompanySelection", () => {
companies: [],
isLoading: false,
unauthorized: false,
errored: false,
})).toBe(true);
});
it("does not clear the stored company selection when the request failed", () => {
// `companiesListQueryOptions` sets `retry: false`, and a failure before any
// success leaves `data` undefined — which the provider defaults to an empty
// list. That is indistinguishable from "asked, and owns nothing", so
// without this a single failed request on a cold load would drop the
// customer's stored company.
expect(shouldClearStoredCompanySelection({
companies: [],
isLoading: false,
unauthorized: false,
errored: true,
})).toBe(false);
});
});
describe("CompanyProvider", () => {
@ -176,6 +192,30 @@ describe("CompanyProvider", () => {
vi.clearAllMocks();
});
it("keeps the stored company when the company request fails", async () => {
// The seam the predicate test above cannot reach: the provider defaults a
// failed request to an empty list, so the effect has to be told the
// request errored or it reads that as "asked, and owns nothing" and clears
// the customer's stored company.
localStorage.setItem("paperclip.selectedCompanyId", "company-a");
mockCompaniesApi.list.mockRejectedValue(new Error("companies unavailable"));
await act(async () => {
root.render(
<QueryClientProvider client={queryClient}>
<CompanyProvider>
<Probe onSelectedCompanyId={() => {}} />
</CompanyProvider>
</QueryClientProvider>,
);
});
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 0));
});
expect(localStorage.getItem("paperclip.selectedCompanyId")).toBe("company-a");
});
it("does not expose a stale stored company id before companies load", async () => {
localStorage.setItem("paperclip.selectedCompanyId", "stale-company");
mockCompaniesApi.list.mockImplementation(() => new Promise(() => {}));

View File

@ -67,7 +67,24 @@ export function shouldClearStoredCompanySelection(input: {
companies: Array<Pick<Company, "id">>;
isLoading: boolean;
unauthorized: boolean;
/**
* Whether the company request failed. An error is not an answer, and this
* branch is destructive.
*
* `companiesListQueryOptions` sets `retry: false`, and a request that fails
* before ever succeeding leaves `data` undefined - which the provider
* defaults to `{ companies: [], unauthorized: false }`. That is
* indistinguishable from "this account was asked, and owns nothing", so a
* single failed request on a cold load would clear the customer's stored
* company and drop them onto whichever company sorts first next time.
*
* Not clearing costs nothing: a stored id that no longer resolves is
* ignored by {@link resolveBootstrapCompanySelection}, which checks it
* against the current list before using it.
*/
errored: boolean;
}) {
if (input.errored) return false;
return !input.isLoading && !input.unauthorized && input.companies.length === 0;
}
@ -89,7 +106,12 @@ export function CompanyProvider({ children }: { children: ReactNode }) {
useEffect(() => {
if (isLoading) return;
if (companies.length === 0) {
if (shouldClearStoredCompanySelection({ companies, isLoading: false, unauthorized: companyListUnauthorized })) {
if (shouldClearStoredCompanySelection({
companies,
isLoading: false,
unauthorized: companyListUnauthorized,
errored: error !== null,
})) {
if (selectedCompanyId !== null) {
setSelectedCompanyIdState(null);
}
@ -108,7 +130,7 @@ export function CompanyProvider({ children }: { children: ReactNode }) {
setSelectedCompanyIdState(next);
setSelectionSource("bootstrap");
localStorage.setItem(STORAGE_KEY, next);
}, [companies, companyListUnauthorized, isLoading, selectedCompanyId, sidebarCompanies]);
}, [companies, companyListUnauthorized, error, isLoading, selectedCompanyId, sidebarCompanies]);
const setSelectedCompanyId = useCallback((companyId: string, options?: CompanySelectionOptions) => {
setSelectedCompanyIdState(companyId);