Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions apps/code/src/main/services/auth/service.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { IPowerManager } from "@posthog/platform/power-manager";
import { OAUTH_SCOPE_VERSION } from "@shared/constants/oauth";
import { NotAuthenticatedError } from "@shared/errors";
import type { CloudRegion } from "@shared/types/regions";
import { type BackoffOptions, sleepWithBackoff } from "@shared/utils/backoff";
import { getCloudUrlFromRegion } from "@shared/utils/urls";
Expand Down Expand Up @@ -336,7 +337,7 @@ export class AuthService extends TypedEventEmitter<AuthServiceEvents> {

const storedSession = this.resolveStoredSession();
if (!storedSession) {
throw new Error("Not authenticated");
throw new NotAuthenticatedError();
}

return storedSession;
Expand Down Expand Up @@ -549,7 +550,7 @@ export class AuthService extends TypedEventEmitter<AuthServiceEvents> {
}
private requireSession(): InMemorySession {
if (!this.session) {
throw new Error("Not authenticated");
throw new NotAuthenticatedError();
}
return this.session;
}
Expand Down
7 changes: 6 additions & 1 deletion apps/code/src/renderer/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { useFocusStore } from "@renderer/stores/focusStore";
import { useThemeStore } from "@renderer/stores/themeStore";
import { initializeUpdateStore } from "@renderer/stores/updateStore";
import { trpcClient, useTRPC } from "@renderer/trpc/client";
import { isNotAuthenticatedError } from "@shared/errors";
import { ANALYTICS_EVENTS } from "@shared/types/analytics";
import { useQueryClient } from "@tanstack/react-query";
import { useSubscription } from "@trpc/tanstack-react-query";
Expand Down Expand Up @@ -284,7 +285,11 @@ function App() {
const content = renderContent();

return (
<ErrorBoundary name="App">
<ErrorBoundary
name="App"
resetKey={authState.status}
shouldSuppress={isNotAuthenticatedError}
>
{isAuthenticated ? (
<AnimatePresence mode="wait">{content}</AnimatePresence>
) : (
Expand Down
162 changes: 162 additions & 0 deletions apps/code/src/renderer/components/ErrorBoundary.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
import { Theme } from "@radix-ui/themes";
import { isNotAuthenticatedError, NotAuthenticatedError } from "@shared/errors";
import { render, screen } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import type { ReactNode } from "react";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { ErrorBoundary } from "./ErrorBoundary";

vi.mock("@utils/analytics", () => ({
captureException: vi.fn(),
}));

vi.mock("@utils/logger", () => ({
logger: {
scope: () => ({
error: vi.fn(),
warn: vi.fn(),
info: vi.fn(),
debug: vi.fn(),
}),
},
}));

import { captureException } from "@utils/analytics";

function Thrower({ error }: { error: Error | null }) {
if (error) throw error;
return <div>ok</div>;
}

function Boundary(props: {
children: ReactNode;
resetKey?: unknown;
shouldSuppress?: (e: Error) => boolean;
fallback?: ReactNode;
}) {
return (
<Theme>
<ErrorBoundary
resetKey={props.resetKey}
shouldSuppress={props.shouldSuppress}
fallback={props.fallback}
>
{props.children}
</ErrorBoundary>
</Theme>
);
}

beforeEach(() => {
vi.spyOn(console, "error").mockImplementation(() => {});
});

afterEach(() => {
vi.restoreAllMocks();
vi.mocked(captureException).mockClear();
});

describe("ErrorBoundary", () => {
it("renders children when no error is thrown", () => {
render(
<Boundary>
<Thrower error={null} />
</Boundary>,
);
expect(screen.getByText("ok")).toBeInTheDocument();
});

it("renders the default fallback UI on error and reports telemetry", () => {
render(
<Boundary>
<Thrower error={new Error("boom")} />
</Boundary>,
);
expect(screen.getByText("Something went wrong")).toBeInTheDocument();
expect(screen.getByText("boom")).toBeInTheDocument();
expect(captureException).toHaveBeenCalledTimes(1);
});

it("renders custom fallback when provided", () => {
render(
<Boundary fallback={<div>custom fallback</div>}>
<Thrower error={new Error("boom")} />
</Boundary>,
);
expect(screen.getByText("custom fallback")).toBeInTheDocument();
});

it("suppresses errors that match shouldSuppress (renders null, no telemetry)", () => {
render(
<Boundary shouldSuppress={isNotAuthenticatedError}>
<Thrower error={new NotAuthenticatedError()} />
</Boundary>,
);
expect(screen.queryByText("Something went wrong")).not.toBeInTheDocument();
expect(screen.queryByText("ok")).not.toBeInTheDocument();
expect(captureException).not.toHaveBeenCalled();
});

it("does not suppress non-matching errors", () => {
render(
<Boundary shouldSuppress={isNotAuthenticatedError}>
<Thrower error={new Error("other failure")} />
</Boundary>,
);
expect(screen.getByText("Something went wrong")).toBeInTheDocument();
expect(captureException).toHaveBeenCalledTimes(1);
});

it("clears error state when resetKey changes", () => {
const { rerender } = render(
<Boundary resetKey="a">
<Thrower error={new Error("boom")} />
</Boundary>,
);
expect(screen.getByText("Something went wrong")).toBeInTheDocument();

rerender(
<Boundary resetKey="b">
<Thrower error={null} />
</Boundary>,
);
expect(screen.queryByText("Something went wrong")).not.toBeInTheDocument();
expect(screen.getByText("ok")).toBeInTheDocument();
});

it("recovers via retry button", async () => {
const user = userEvent.setup();
const { rerender } = render(
<Boundary>
<Thrower error={new Error("boom")} />
</Boundary>,
);
expect(screen.getByText("Something went wrong")).toBeInTheDocument();

rerender(
<Boundary>
<Thrower error={null} />
</Boundary>,
);
await user.click(screen.getByRole("button", { name: /try again/i }));
expect(screen.getByText("ok")).toBeInTheDocument();
});
});

describe("isNotAuthenticatedError", () => {
it("matches NotAuthenticatedError instances", () => {
expect(isNotAuthenticatedError(new NotAuthenticatedError())).toBe(true);
});

it("matches plain objects with the same name (e.g. tRPC-serialized errors)", () => {
expect(isNotAuthenticatedError({ name: "NotAuthenticatedError" })).toBe(
true,
);
});

it("does not match unrelated errors", () => {
expect(isNotAuthenticatedError(new Error("Not authenticated"))).toBe(false);
expect(isNotAuthenticatedError(null)).toBe(false);
expect(isNotAuthenticatedError("Not authenticated")).toBe(false);
});
});
Comment on lines +136 to +162
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Prefer parameterised tests for isNotAuthenticatedError

The three it blocks below cover five distinct input/expectation pairs and would be cleaner as a single it.each table. The current structure also buries the three false assertions inside one test body, making it easy to miss a case. Using it.each maps each input to its expected result in one place and is the team's stated preference for tests like these.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/components/ErrorBoundary.test.tsx
Line: 136-162

Comment:
**Prefer parameterised tests for `isNotAuthenticatedError`**

The three `it` blocks below cover five distinct input/expectation pairs and would be cleaner as a single `it.each` table. The current structure also buries the three `false` assertions inside one test body, making it easy to miss a case. Using `it.each` maps each input to its expected result in one place and is the team's stated preference for tests like these.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

89 changes: 54 additions & 35 deletions apps/code/src/renderer/components/ErrorBoundary.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,46 @@ interface Props {
fallback?: ReactNode;
/** Optional name to identify which boundary caught the error */
name?: string;
/** When this value changes, the boundary clears its error state. */
resetKey?: unknown;
/**
* If returns true for a caught error, the boundary renders nothing,
* skips telemetry, and waits for `resetKey` to change before recovering.
* Use to handle transient errors that the surrounding tree will resolve
* (e.g. auth state about to flip to unauthenticated).
*/
shouldSuppress?: (error: Error) => boolean;
}

interface State {
hasError: boolean;
error: Error | null;
lastResetKey: unknown;
}

export class ErrorBoundary extends Component<Props, State> {
constructor(props: Props) {
super(props);
this.state = { hasError: false, error: null };
state: State = { error: null, lastResetKey: this.props.resetKey };

static getDerivedStateFromError(error: Error): Partial<State> {
return { error };
}

static getDerivedStateFromError(error: Error): State {
return { hasError: true, error };
static getDerivedStateFromProps(
props: Props,
state: State,
): Partial<State> | null {
if (props.resetKey === state.lastResetKey) return null;
return { error: null, lastResetKey: props.resetKey };
}

componentDidCatch(error: Error, errorInfo: ErrorInfo) {
if (this.props.shouldSuppress?.(error)) {
log.warn("Suppressed error in boundary", {
name: this.props.name,
error: error.message,
});
return;
}

log.error("Error caught by boundary", {
name: this.props.name,
error: error.message,
Expand All @@ -44,39 +66,36 @@ export class ErrorBoundary extends Component<Props, State> {
}

handleRetry = () => {
this.setState({ hasError: false, error: null });
this.setState({ error: null });
};

render() {
if (this.state.hasError) {
if (this.props.fallback) {
return this.props.fallback;
}
const { error } = this.state;
if (!error) return this.props.children;
if (this.props.shouldSuppress?.(error)) return null;
if (this.props.fallback) return this.props.fallback;

return (
<Box p="4">
<Callout.Root color="red" size="2">
<Callout.Icon>
<Warning weight="fill" />
</Callout.Icon>
<Callout.Text>
<Flex direction="column" gap="2">
<Text className="font-medium">Something went wrong</Text>
<Text className="text-[13px] text-gray-11">
{this.state.error?.message || "An unexpected error occurred"}
</Text>
<Flex gap="2" mt="2">
<Button size="1" variant="soft" onClick={this.handleRetry}>
Try again
</Button>
</Flex>
return (
<Box p="4">
<Callout.Root color="red" size="2">
<Callout.Icon>
<Warning weight="fill" />
</Callout.Icon>
<Callout.Text>
<Flex direction="column" gap="2">
<Text className="font-medium">Something went wrong</Text>
<Text className="text-[13px] text-gray-11">
{error.message || "An unexpected error occurred"}
</Text>
<Flex gap="2" mt="2">
<Button size="1" variant="soft" onClick={this.handleRetry}>
Try again
</Button>
</Flex>
</Callout.Text>
</Callout.Root>
</Box>
);
}

return this.props.children;
</Flex>
</Callout.Text>
</Callout.Root>
</Box>
);
}
}
3 changes: 2 additions & 1 deletion apps/code/src/renderer/features/auth/hooks/authClient.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { PostHogAPIClient } from "@renderer/api/posthogClient";
import { trpcClient } from "@renderer/trpc/client";
import { NotAuthenticatedError } from "@shared/errors";
import { getCloudUrlFromRegion } from "@shared/utils/urls";
import { useMemo } from "react";
import {
Expand Down Expand Up @@ -56,7 +57,7 @@ export function useAuthenticatedClient(): PostHogAPIClient {
const client = useOptionalAuthenticatedClient();

if (!client) {
throw new Error("Not authenticated");
throw new NotAuthenticatedError();
}

return client;
Expand Down
15 changes: 15 additions & 0 deletions apps/code/src/shared/errors.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,18 @@
export class NotAuthenticatedError extends Error {
constructor(message = "Not authenticated") {
super(message);
this.name = "NotAuthenticatedError";
}
}

export function isNotAuthenticatedError(error: unknown): boolean {
return (
typeof error === "object" &&
error !== null &&
(error as { name?: unknown }).name === "NotAuthenticatedError"
);
}

const AUTH_ERROR_PATTERNS = [
"authentication required",
"failed to authenticate",
Expand Down
Loading