Skip to content

fix(ui): Stabilize WatchButton text #814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
timothyfroehlich merged 4 commits into from
Jan 19, 2026
Merged

fix(ui): Stabilize WatchButton text #814

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
01d95c7
chore: remove accidental package-lock.json
timothyfroehlich Jan 19, 2026
920b2a4
Merge branch 'main' of https://github.com/timothyfroehlich/PinPoint
timothyfroehlich Jan 19, 2026
858f4d6
fix(ui): stabilize watch button text on toggle
timothyfroehlich Jan 19, 2026
0716313
chore: move WatchButton test and address review comments
timothyfroehlich Jan 19, 2026
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
40 changes: 40 additions & 0 deletions src/components/issues/WatchButton.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { render, screen, fireEvent, waitFor } from "@testing-library/react";
import { describe, it, expect, vi } from "vitest";
import { WatchButton } from "./WatchButton";
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The test file should be located in src/test/unit/components/issues/ instead of directly alongside the component file. This follows the established pattern seen in src/test/unit/components/auth/login-form.test.tsx and maintains clear separation between source and test code as documented in src/test/README.md.

Suggested change
import { WatchButton } from "./WatchButton";
import { WatchButton } from "~/components/issues/WatchButton";

Copilot uses AI. Check for mistakes.
import React from "react";

// Mock the server action
vi.mock("~/app/(app)/issues/watcher-actions", () => ({
toggleWatcherAction: vi.fn(),
}));

import { toggleWatcherAction } from "~/app/(app)/issues/watcher-actions";

Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The React import on line 4 is unnecessary since React is already implicitly available in JSX transform. Additionally, line 4 appears after the mock setup (lines 6-11), which places imports out of standard order. Consider removing the explicit React import or moving all imports to the top before any mock setup code.

Suggested change
import React from "react";
// Mock the server action
vi.mock("~/app/(app)/issues/watcher-actions", () => ({
toggleWatcherAction: vi.fn(),
}));
import { toggleWatcherAction } from "~/app/(app)/issues/watcher-actions";
import { toggleWatcherAction } from "~/app/(app)/issues/watcher-actions";
// Mock the server action
vi.mock("~/app/(app)/issues/watcher-actions", () => ({
toggleWatcherAction: vi.fn(),
}));

Copilot uses AI. Check for mistakes.
describe("WatchButton", () => {
it("renders correctly when not watching", () => {
render(<WatchButton issueId="123" initialIsWatching={false} />);
expect(screen.getByRole("button")).toHaveTextContent("Watch Issue");
expect(screen.queryByText("Unwatch Issue")).not.toBeInTheDocument();
});

it("renders correctly when watching", () => {
render(<WatchButton issueId="123" initialIsWatching={true} />);
expect(screen.getByRole("button")).toHaveTextContent("Unwatch Issue");
expect(screen.queryByText("Watch Issue")).not.toBeInTheDocument();
});

it("calls toggleWatcherAction on click", async () => {
(toggleWatcherAction as any).mockResolvedValue({
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

Using any type violates the TypeScript strictest patterns guideline. Instead, mock the function with proper typing. For example: vi.mocked(toggleWatcherAction).mockResolvedValue({ ok: true, value: { isWatching: true } }) after importing the function, or create a properly typed mock variable.

Copilot generated this review using guidance from repository custom instructions.
ok: true,
value: { isWatching: true },
});

render(<WatchButton issueId="123" initialIsWatching={false} />);

fireEvent.click(screen.getByRole("button"));

await waitFor(() => {
expect(toggleWatcherAction).toHaveBeenCalledWith("123");
});
});
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The test should verify that the loading spinner is shown during the pending state, similar to how login-form.test.tsx validates the spinner presence (lines 59-61 in that file). Since the PR's purpose is to rely on the loading spinner instead of dynamic text, validating spinner behavior is important to ensure the change achieves its stated goal of indicating loading state.

Copilot uses AI. Check for mistakes.
});
4 changes: 2 additions & 2 deletions src/components/issues/WatchButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ export function WatchButton({
{isWatching ? (
<>
{!isPending && <EyeOff className="size-4" />}
{isPending ? "Unwatching..." : "Unwatch Issue"}
Unwatch Issue
</>
) : (
<>
{!isPending && <Eye className="size-4" />}
{isPending ? "Watching..." : "Watch Issue"}
Watch Issue
Comment on lines +51 to +56
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The change to static text in WatchButton creates an inconsistency with InviteUserDialog (line 224-225), which still uses dynamic text: {isPending ? "Inviting..." : "Invite User"}. Consider updating InviteUserDialog to follow the same pattern for consistency, or document why different components handle loading states differently. The auth forms (login, signup, etc.) already use static text with the loading spinner, which aligns with this change.

Copilot uses AI. Check for mistakes.
</>
)}
</Button>
Expand Down
Loading