fix(#411): remediate frontend review findings — wire fetchWithRetry, fix error handling

- Wire fetchWithRetry into login page config fetch (was dead code)
- Remove duplicate ERROR_CODE_MESSAGES, use parseAuthError from auth-errors.ts
- Fix OAuth sign-in fire-and-forget: add .catch() with PDA error + loading reset
- Fix credential login catch: use parseAuthError for better error messages
- Add user feedback when auth config fetch fails (was silent degradation)
- Fix sign-out failure: use logAuthError and set authError state
- Enable fetchWithRetry production logging for retry visibility

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Jason Woltje
2026-02-16 12:33:25 -06:00
parent 7ead8b1076
commit 9696e45265
5 changed files with 89 additions and 49 deletions

View File

@@ -35,19 +35,34 @@ vi.mock("@/lib/config", () => ({
API_BASE_URL: "http://localhost:3001",
}));
// Mock fetchWithRetry to behave like fetch for test purposes
const { mockFetchWithRetry } = vi.hoisted(() => ({
mockFetchWithRetry: vi.fn(),
}));
vi.mock("@/lib/auth/fetch-with-retry", () => ({
fetchWithRetry: mockFetchWithRetry,
}));
// Mock parseAuthError to use the real implementation
vi.mock("@/lib/auth/auth-errors", async () => {
const actual = await vi.importActual<typeof import("@/lib/auth/auth-errors")>("@/lib/auth/auth-errors");
return actual;
});
/* ------------------------------------------------------------------ */
/* Helpers */
/* ------------------------------------------------------------------ */
function mockFetchConfig(config: AuthConfigResponse): void {
(global.fetch as Mock).mockResolvedValueOnce({
mockFetchWithRetry.mockResolvedValueOnce({
ok: true,
json: (): Promise<AuthConfigResponse> => Promise.resolve(config),
});
}
function mockFetchFailure(): void {
(global.fetch as Mock).mockRejectedValueOnce(new Error("Network error"));
mockFetchWithRetry.mockRejectedValueOnce(new Error("Network error"));
}
const OAUTH_ONLY_CONFIG: AuthConfigResponse = {
@@ -72,15 +87,16 @@ const BOTH_PROVIDERS_CONFIG: AuthConfigResponse = {
describe("LoginPage", (): void => {
beforeEach((): void => {
vi.clearAllMocks();
global.fetch = vi.fn();
// Reset search params to empty for each test
mockSearchParams.delete("error");
// Default: OAuth2 returns a resolved promise (fire-and-forget redirect)
mockOAuth2.mockResolvedValue(undefined);
});
it("renders loading state initially", (): void => {
// Never resolve fetch so it stays in loading state
// eslint-disable-next-line @typescript-eslint/no-empty-function -- intentionally never-resolving promise to test loading state
(global.fetch as Mock).mockReturnValueOnce(new Promise(() => {}));
mockFetchWithRetry.mockReturnValueOnce(new Promise(() => {}));
render(<LoginPage />);
@@ -105,13 +121,13 @@ describe("LoginPage", (): void => {
expect(main).toHaveClass("flex", "min-h-screen");
});
it("fetches /auth/config on mount", async (): Promise<void> => {
it("fetches /auth/config on mount using fetchWithRetry", async (): Promise<void> => {
mockFetchConfig(EMAIL_ONLY_CONFIG);
render(<LoginPage />);
await waitFor((): void => {
expect(global.fetch).toHaveBeenCalledWith("http://localhost:3001/auth/config");
expect(mockFetchWithRetry).toHaveBeenCalledWith("http://localhost:3001/auth/config");
});
});
@@ -164,7 +180,7 @@ describe("LoginPage", (): void => {
expect(screen.queryByText(/or continue with email/i)).not.toBeInTheDocument();
});
it("falls back to email-only on fetch failure", async (): Promise<void> => {
it("falls back to email-only on fetch failure and shows unavailability message", async (): Promise<void> => {
mockFetchFailure();
render(<LoginPage />);
@@ -175,10 +191,13 @@ describe("LoginPage", (): void => {
expect(screen.getByLabelText(/password/i)).toBeInTheDocument();
expect(screen.queryByRole("button", { name: /continue with/i })).not.toBeInTheDocument();
// Should show the unavailability banner (fix #5)
expect(screen.getByText("Some sign-in options may be temporarily unavailable.")).toBeInTheDocument();
});
it("falls back to email-only on non-ok response", async (): Promise<void> => {
(global.fetch as Mock).mockResolvedValueOnce({
mockFetchWithRetry.mockResolvedValueOnce({
ok: false,
status: 500,
});
@@ -210,6 +229,26 @@ describe("LoginPage", (): void => {
});
});
it("shows error when OAuth sign-in fails", async (): Promise<void> => {
mockFetchConfig(OAUTH_ONLY_CONFIG);
mockOAuth2.mockRejectedValueOnce(new Error("Provider unavailable"));
const user = userEvent.setup();
render(<LoginPage />);
await waitFor((): void => {
expect(screen.getByRole("button", { name: /continue with authentik/i })).toBeInTheDocument();
});
await user.click(screen.getByRole("button", { name: /continue with authentik/i }));
await waitFor((): void => {
expect(
screen.getByText("Unable to connect to the sign-in provider. Please try again in a moment.")
).toBeInTheDocument();
});
});
it("calls signIn.email and redirects on success", async (): Promise<void> => {
mockFetchConfig(EMAIL_ONLY_CONFIG);
mockSignInEmail.mockResolvedValueOnce({ data: { user: {} } });
@@ -261,9 +300,9 @@ describe("LoginPage", (): void => {
expect(mockPush).not.toHaveBeenCalled();
});
it("shows generic error on unexpected sign-in exception", async (): Promise<void> => {
it("shows parseAuthError message on unexpected sign-in exception", async (): Promise<void> => {
mockFetchConfig(EMAIL_ONLY_CONFIG);
mockSignInEmail.mockRejectedValueOnce(new Error("Network failure"));
mockSignInEmail.mockRejectedValueOnce(new TypeError("Failed to fetch"));
const user = userEvent.setup();
render(<LoginPage />);
@@ -277,8 +316,9 @@ describe("LoginPage", (): void => {
await user.click(screen.getByRole("button", { name: /continue/i }));
await waitFor((): void => {
// parseAuthError maps TypeError("Failed to fetch") to network_error message
expect(
screen.getByText("Something went wrong. Please try again in a moment.")
screen.getByText("Unable to connect. Check your network and try again.")
).toBeInTheDocument();
});
});
@@ -333,7 +373,7 @@ describe("LoginPage", (): void => {
it("loading spinner has role=status", (): void => {
// Never resolve fetch so it stays in loading state
// eslint-disable-next-line @typescript-eslint/no-empty-function -- intentionally never-resolving promise
(global.fetch as Mock).mockReturnValueOnce(new Promise(() => {}));
mockFetchWithRetry.mockReturnValueOnce(new Promise(() => {}));
render(<LoginPage />);