Skip to content

fix: fix workspaces pagination #19448

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
merged 5 commits into from
Aug 21, 2025
Merged

fix: fix workspaces pagination #19448

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6b10145
fix: fix workspaces pagination
BrunoQuaresma Aug 20, 2025
c63fc7d
improve tests
BrunoQuaresma Aug 20, 2025
7c4ce4d
Make tests more reliable
BrunoQuaresma Aug 20, 2025
ed86e7f
Fix lint
BrunoQuaresma Aug 20, 2025
275116b
Improve tests
BrunoQuaresma Aug 21, 2025
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
4 changes: 2 additions & 2 deletions site/src/api/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1187,9 +1187,9 @@ class ApiMethods {
};

getWorkspaces = async (
options: TypesGen.WorkspacesRequest,
req: TypesGen.WorkspacesRequest,
): Promise<TypesGen.WorkspacesResponse> => {
const url = getURLWithSearchParams("/api/v2/workspaces", options);
const url = getURLWithSearchParams("/api/v2/workspaces", req);
const response = await this.axios.get<TypesGen.WorkspacesResponse>(url);
return response.data;
};
Expand Down
11 changes: 5 additions & 6 deletions site/src/api/queries/workspaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,14 @@ async function findMatchWorkspace(q: string): Promise<Workspace | undefined> {
}
}

function workspacesKey(config: WorkspacesRequest = {}) {
const { q, limit } = config;
return ["workspaces", { q, limit }] as const;
function workspacesKey(req: WorkspacesRequest = {}) {
return ["workspaces", req] as const;
}

export function workspaces(config: WorkspacesRequest = {}) {
export function workspaces(req: WorkspacesRequest = {}) {
return {
queryKey: workspacesKey(config),
queryFn: () => API.getWorkspaces(config),
queryKey: workspacesKey(req),
queryFn: () => API.getWorkspaces(req),
} as const satisfies QueryOptions<WorkspacesResponse>;
}

Expand Down
60 changes: 60 additions & 0 deletions site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,66 @@ describe("WorkspacesPage", () => {
MockStoppedWorkspace.latest_build.template_version_id,
);
});

it("correctly handles pagination by including pagination parameters in query key", async () => {
const totalWorkspaces = 50;
const workspacesPage1 = Array.from({ length: 25 }, (_, i) => ({
...MockWorkspace,
id: `page1-workspace-${i}`,
name: `page1-workspace-${i}`,
}));
const workspacesPage2 = Array.from({ length: 25 }, (_, i) => ({
...MockWorkspace,
id: `page2-workspace-${i}`,
name: `page2-workspace-${i}`,
}));

const getWorkspacesSpy = jest.spyOn(API, "getWorkspaces");

getWorkspacesSpy
.mockResolvedValueOnce({
workspaces: workspacesPage1,
count: totalWorkspaces,
})
.mockResolvedValueOnce({
workspaces: workspacesPage2,
count: totalWorkspaces,
});
Copy link
Member

@Parkreiner Parkreiner Aug 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like this setup is a little fragile. I've never stacked multiple calls to mockResolvedValueOnce onto the same spy before, but if I'm reading it right, it means that, no matter what we actually call the function with, we'll always get page 1 for the first call, and page 2 for the second call. But then:

  • Any future calls immediately go back to the actual non-mocked version
  • We never check the inputs for the functions to decide what to return out. If we accidentally pass page 3 in, we might get page 1 or we might get page 2
  • If we try to get page 1 multiple times, we can't

I don't know all the details, but I know that MSW has tools to mock out the API response. Maybe that's not needed and we could keep the Jest Spys, but I think I'd rather make it so that we have a mock that stays locked in for the duration of the test, and then checks the page input:

  • A page value of 1 always gives back page 1 content
  • A page value of 2 always gives back page 2 content
  • Any other page values just give back an empty array

Copy link
Member

@Parkreiner Parkreiner Aug 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I know you have the checks for the exact inputs a little bit below, but if the React testing tools ever add double-rendering to help check for correctness (like what you get with StrictMode and dev mode), that would immediately cause this test to break

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if I got it. Let me share what I understood.

The issue with this test is it is using stacked mockResolvedValueOnce to mock the responses for the first and second calls so, if react renders twice in the same page, the test would break. Is that right?

Copy link
Member

@Parkreiner Parkreiner Aug 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, if React ever double-mounts the same hook for the data fetching, this will happen if we start at page 1:

  1. The page component gets mounted, and we trigger the data fetch, using a page value of 1 as input. The function is mocked, so we always get the page 1 data back no matter what
  2. The render completes
  3. React unmounts and re-mounts the component from scratch, which potentially causes a second data fetch, still using a page value of 1 as input
  4. But because we've already popped off the first mock, calling the function with a page value of 1 actually gets us page 2's data, because the mock doesn't actually check what number gets passed in
  5. We get back page 2's data, and render that out. React considers the render "stabilized"
  6. We go back to the testing/assertion logic, and the tests fail because the page 1 content is no longer on screen
  7. If we were to keep going, and try switching the page value to 2 by clicking the UI, we have no more mocks left, and the call goes to the actual API implementation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the spyOn to use the mockImplementation, wdyt?


const user = userEvent.setup();
renderWithAuth(<WorkspacesPage />);

await waitFor(() => {
expect(screen.getByText("page1-workspace-0")).toBeInTheDocument();
});

expect(getWorkspacesSpy).toHaveBeenNthCalledWith(
1,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(getWorkspacesSpy).toHaveBeenNthCalledWith(
1,
expect(getWorkspacesSpy).toHaveBeenLastCalledWith(

one last thing, I think toHaveBeenLastCalledWith would be better here. to michael's point: how many times this function actually gets called could be considered an implementation detail of react. but we do know we want it to have been called, and that at least the most recent call should match this assertion.

https://jestjs.io/docs/expect#tohavebeenlastcalledwitharg1-arg2-

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense! 👍

expect.objectContaining({
q: "owner:me",
offset: 0,
limit: 25,
}),
);

const nextPageButton = screen.getByRole("button", { name: /next page/i });
await user.click(nextPageButton);

await waitFor(() => {
expect(screen.getByText("page2-workspace-0")).toBeInTheDocument();
});

expect(getWorkspacesSpy).toHaveBeenNthCalledWith(
2,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(getWorkspacesSpy).toHaveBeenNthCalledWith(
2,
expect(getWorkspacesSpy).toHaveBeenLastCalledWith(

same thought here

expect.objectContaining({
q: "owner:me",
offset: 25,
limit: 25,
}),
);

expect(screen.queryByText("page1-workspace-0")).not.toBeInTheDocument();
});
});

const getWorkspaceCheckbox = (workspace: Workspace) => {
Expand Down
Loading