fix(ui): Stabilize WatchButton text #814
Conversation
Prevents layout shift by keeping text static while loading spinner shows.
timothyfroehlich
added
the
jules:copilot-review
timothyfroehlich
added
the
jules:copilot-review
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes layout shift in the WatchButton component by removing dynamic text changes during loading states and relying solely on the loading spinner for visual feedback.
Changes:
- Removed conditional text ("Watching..." / "Unwatching...") in favor of static labels ("Watch Issue" / "Unwatch Issue")
- Added unit tests for the WatchButton component to verify rendering and action behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/components/issues/WatchButton.tsx | Simplified button text to remain static during loading, eliminating layout shift |
| src/components/issues/WatchButton.test.tsx | Added unit tests covering basic rendering states and action invocation |
| @@ -0,0 +1,40 @@ | |||
| import { render, screen, fireEvent, waitFor } from "@testing-library/react"; | |||
| import { describe, it, expect, vi } from "vitest"; | |||
| import { WatchButton } from "./WatchButton"; | |||
There was a problem hiding this comment.
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.
| import { WatchButton } from "./WatchButton"; | |
| import { WatchButton } from "~/components/issues/WatchButton"; |
| }); | ||
|
|
||
| it("calls toggleWatcherAction on click", async () => { | ||
| (toggleWatcherAction as any).mockResolvedValue({ |
There was a problem hiding this comment.
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.
| Unwatch Issue | ||
| </> | ||
| ) : ( | ||
| <> | ||
| {!isPending && <Eye className="size-4" />} | ||
| {isPending ? "Watching..." : "Watch Issue"} | ||
| Watch Issue |
There was a problem hiding this comment.
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.
| 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"; | ||
|
|
There was a problem hiding this comment.
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.
| 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(), | |
| })); |
| it("calls toggleWatcherAction on click", async () => { | ||
| (toggleWatcherAction as any).mockResolvedValue({ | ||
| ok: true, | ||
| value: { isWatching: true }, | ||
| }); | ||
|
|
||
| render(<WatchButton issueId="123" initialIsWatching={false} />); | ||
|
|
||
| fireEvent.click(screen.getByRole("button")); | ||
|
|
||
| await waitFor(() => { | ||
| expect(toggleWatcherAction).toHaveBeenCalledWith("123"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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.
- Move test to src/test/unit/components/issues/\n- Use vi.mocked instead of any\n- Verify loading spinner and static text stability
Fixes layout shift in WatchButton by removing dynamic text changes (Watching.../Unwatching...) and relying on the loading spinner state.