implement button and logic for canceling reservation request #327
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideImplements end-to-end cancellation of reservation requests: adds an application-service cancel command wired through a new GraphQL mutation, exposes a shared CancelReservationPopconfirm UI component, updates listing/reservation flows to use it with proper loading/early-return safeguards, and adds extensive Storybook and BDD-style tests plus minor coverage/config tweaks. Sequence diagram for cancelling a reservation request via listing pagesequenceDiagram
actor User
participant ListingInformationContainer
participant ListingInformation
participant CancelReservationPopconfirm
participant ApolloClient
participant GraphQLAPI
participant ReservationRequestApplicationService
participant ReservationRequestRepository
User->>ListingInformation: click Cancel Request button
ListingInformation->>CancelReservationPopconfirm: open popconfirm
User->>CancelReservationPopconfirm: click Confirm
CancelReservationPopconfirm->>ListingInformation: onCancelClick()
ListingInformation->>ListingInformationContainer: onCancelClick()
alt cancelLoading true
ListingInformationContainer-->>ListingInformation: return (early)
else cancelLoading false and userReservationRequest.id missing
ListingInformationContainer-->>ListingInformation: message.error("No reservation request to cancel")
else cancelLoading false and id present
ListingInformationContainer->>ApolloClient: mutate cancelReservation(id)
ApolloClient->>GraphQLAPI: cancelReservation(input)
GraphQLAPI->>ReservationRequestApplicationService: cancel(id,callerId)
ReservationRequestApplicationService->>ReservationRequestRepository: getById(id)
ReservationRequestRepository-->>ReservationRequestApplicationService: reservationRequest
ReservationRequestApplicationService->>ReservationRequest: loadReserver()
ReservationRequest-->>ReservationRequestApplicationService: reserver(id)
ReservationRequestApplicationService->>ReservationRequest: setState("Cancelled")
ReservationRequestApplicationService->>ReservationRequestRepository: save(reservationRequest)
ReservationRequestRepository-->>ReservationRequestApplicationService: savedReservation
ReservationRequestApplicationService-->>GraphQLAPI: savedReservation
GraphQLAPI-->>ApolloClient: savedReservation
ApolloClient-->>ListingInformationContainer: onCompleted()
ListingInformationContainer->>ListingInformationContainer: message.success()
ListingInformationContainer->>ApolloClient: refetchQueries(ViewListingActiveReservationRequestForListing)
ApolloClient-->>ListingInformationContainer: updated active reservation
ListingInformationContainer-->>ListingInformation: props userReservationRequest=null,cancelLoading=false
end
Class diagram for cancel reservation application-service, GraphQL, and UI componentsclassDiagram
class ReservationRequestApplicationService {
+queryById(command)
+queryActiveByReserverId(command)
+queryPastByReserverId(command)
+queryActiveByReserverIdAndListingId(command)
+queryOverlapByListingIdAndReservationPeriod(command)
+queryActiveByListingId(command)
+queryListingRequestsBySharerId(command)
+create(command)
+cancel(command)
}
class ReservationRequestCancelCommand {
+string id
+string callerId
}
class CancelFunction {
+cancel(dataSources)
}
class ReservationRequestRepository {
+getById(id)
+save(reservationRequest)
}
class ReservationRequestEntity {
+string id
+string state
+loadReserver()
}
class ReservationRequestResolvers {
+cancelReservation(parent,args,context,info)
}
class GraphContext {
+applicationServices
}
class CancelReservationInput {
+string id
}
class ListingInformationContainer {
+listing
+userIsSharer
+isAuthenticated
+userReservationRequest
+reservationDates
+handleReserveClick()
+handleCancelClick()
}
class ListingInformation {
+listing
+userIsSharer
+isAuthenticated
+userReservationRequest
+reservationDates
+reservationLoading
+cancelLoading
+onReserveClick()
+onCancelClick()
}
class ReservationActions {
+status
+cancelLoading
+closeLoading
+onCancel()
+onClose()
+onMessage()
}
class CancelReservationPopconfirm {
+onConfirm()
+loading
+children
}
class PopconfirmTestUtils {
+triggerPopconfirmAnd(canvas,action,options)
+clickCancelThenConfirm(canvasElement)
+getLoadingIndicators(rootElement)
}
ReservationRequestApplicationService --> ReservationRequestRepository : uses
CancelFunction --> ReservationRequestApplicationService : constructs
ReservationRequestApplicationService --> ReservationRequestEntity : returns
ReservationRequestResolvers --> ReservationRequestApplicationService : calls cancel
ReservationRequestResolvers --> GraphContext : uses
ReservationRequestResolvers --> CancelReservationInput : args.input
ListingInformationContainer --> ListingInformation : renders
ListingInformationContainer --> CancelReservationInput : builds for mutation
ListingInformationContainer --> CancelReservationPopconfirm : via ListingInformation
ListingInformation --> CancelReservationPopconfirm : composes
ReservationActions --> CancelReservationPopconfirm : wraps buttons
CancelReservationPopconfirm --> ReservationActions : contains button children
PopconfirmTestUtils --> CancelReservationPopconfirm : tests
PopconfirmTestUtils --> ReservationActions : tests
PopconfirmTestUtils --> ListingInformation : tests
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The
cancelReservationGraphQL resolver only checks for authentication but not that the current user is the owner of the reservation request being canceled; consider enforcing an authorization check (e.g., reserver ID match) before calling the application service. - The
cancelapplication service unconditionally setsreservationRequest.state = 'Cancelled'; if your domain has rules about which states can transition toCancelled, it would be safer to enforce them here (or delegate to a domain method) rather than directly mutating the state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `cancelReservation` GraphQL resolver only checks for authentication but not that the current user is the owner of the reservation request being canceled; consider enforcing an authorization check (e.g., reserver ID match) before calling the application service.
- The `cancel` application service unconditionally sets `reservationRequest.state = 'Cancelled'`; if your domain has rules about which states can transition to `Cancelled`, it would be safer to enforce them here (or delegate to a domain method) rather than directly mutating the state.
## Individual Comments
### Comment 1
<location> `packages/sthrift/graphql/src/schema/types/reservation-request/reservation-request.resolvers.ts:222-231` </location>
<code_context>
},
);
},
+ cancelReservation: async (
+ _parent: unknown,
+ args: {
+ input: {
+ id: string;
+ };
+ },
+ context: GraphContext,
+ _info: GraphQLResolveInfo,
+ ) => {
+ const verifiedJwt = context.applicationServices.verifiedUser?.verifiedJwt;
+ if (!verifiedJwt) {
+ throw new Error(
+ 'User must be authenticated to cancel a reservation request',
+ );
+ }
+
+ return await context.applicationServices.ReservationRequest.ReservationRequest.cancel(
+ {
+ id: args.input.id,
</code_context>
<issue_to_address>
**🚨 issue (security):** Cancel resolver should validate that the caller is authorized to cancel the specific reservation request, not just authenticated.
The resolver only checks for a present `verifiedJwt` before calling `ReservationRequest.cancel` with just the `id`, so any authenticated user could cancel any reservation request if they know/guess its ID.
Pass the caller identity from `verifiedJwt` into the application service and enforce ownership/role checks there, or verify in the resolver that the reservation belongs to the current user (or that they are the listing sharer) before cancelling.
</issue_to_address>
### Comment 2
<location> `packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/cancel.ts:15-22` </location>
<code_context>
+ let reservationRequestToReturn:
+ | Domain.Contexts.ReservationRequest.ReservationRequest.ReservationRequestEntityReference
+ | undefined;
+ await dataSources.domainDataSource.ReservationRequest.ReservationRequest.ReservationRequestUnitOfWork.withScopedTransaction(
+ async (repo) => {
+ const reservationRequest = await repo.getById(command.id);
+ if (!reservationRequest) {
+ throw new Error('Reservation request not found');
+ }
+
+ reservationRequest.state = 'Cancelled';
+ reservationRequestToReturn = await repo.save(reservationRequest);
+ },
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Cancelling blindly based on ID ignores reservation state and may allow invalid state transitions.
`reservationRequest.state = 'Cancelled'` is set unconditionally for any reservation found by ID. This permits cancelling reservations in terminal or otherwise invalid states.
Please gate cancellation on allowed source states (e.g., only when `Requested`/pending) and raise a domain-level error otherwise. If the aggregate should enforce this, prefer a domain method like `reservationRequest.cancel()` over directly mutating `state`.
Suggested implementation:
```typescript
const reservationRequest = await repo.getById(command.id);
if (!reservationRequest) {
throw new Error('Reservation request not found');
}
// Only allow cancellation from specific, non-terminal states
const cancellableStates = new Set<string>(['Requested']); // align with domain model / enum
if (!cancellableStates.has(reservationRequest.state)) {
// Prefer raising a domain-level error rather than a generic Error
throw new Error(
`Reservation request cannot be cancelled from state "${reservationRequest.state}"`,
);
}
// Prefer using a domain method over mutating state directly, if available
if (typeof (reservationRequest as any).cancel === 'function') {
(reservationRequest as any).cancel();
} else {
reservationRequest.state = 'Cancelled';
}
reservationRequestToReturn = await repo.save(reservationRequest);
```
To fully align with your comment and the domain model, you should also:
1. Add a `cancel()` method to the ReservationRequest aggregate in the domain layer that:
- Enforces allowed source states (e.g. `Requested`/pending).
- Throws a domain-specific error (e.g. `InvalidReservationRequestStateError`) when cancellation is not permitted.
- Applies the state transition to `Cancelled` using domain invariants/events where appropriate.
2. Replace the generic `Error` throws in this application-service with those domain-specific errors (imported from your domain error module).
3. Replace the local `cancellableStates` check in this file with a call to `reservationRequest.cancel()` only, once that method is implemented, and remove the direct `state` mutation fallback.
4. Ensure the error handling at higher layers (e.g. GraphQL/REST) maps the new domain error into the appropriate HTTP/GraphQL error responses.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
packages/sthrift/graphql/src/schema/types/reservation-request/reservation-request.resolvers.ts
Show resolved
Hide resolved
…servation request
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The Storybook play functions for Popconfirm flows rely on
setTimeoutplusdocument.querySelector; consider switching to Testing Library async queries (e.g.findByText/waitFor) so the stories are less timing‑sensitive and more robust against UI changes. - In the reservation-request cancel application service you directly mutate
reservationRequest.state = 'Cancelled'; if the domain model offers behavior methods or typed state transitions, prefer invoking those instead of setting the string field to keep invariants enforced in one place. - The
ReservationRequestCancelCommandinterface requirescallerId, but some test scenarios (e.g. the non-existent reservation case) construct the command without it; either makecallerIdrequired in all tests or update the type to reflect when it can be omitted to avoid type/behavior divergence.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Storybook play functions for Popconfirm flows rely on `setTimeout` plus `document.querySelector`; consider switching to Testing Library async queries (e.g. `findByText` / `waitFor`) so the stories are less timing‑sensitive and more robust against UI changes.
- In the reservation-request cancel application service you directly mutate `reservationRequest.state = 'Cancelled'`; if the domain model offers behavior methods or typed state transitions, prefer invoking those instead of setting the string field to keep invariants enforced in one place.
- The `ReservationRequestCancelCommand` interface requires `callerId`, but some test scenarios (e.g. the non-existent reservation case) construct the command without it; either make `callerId` required in all tests or update the type to reflect when it can be omitted to avoid type/behavior divergence.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:88-91` </location>
<code_context>
+ await userEvent.click(buttons[0]);
+ // Verify the callback was called
+ const callbacks = [args.onCancel, args.onClose, args.onMessage];
+ const called = callbacks.some(
+ (cb) => cb && (cb as any).mock?.calls?.length > 0,
+ );
+ expect(called || true).toBe(true); // Allow pass if callbacks are called
+ }
+ },
</code_context>
<issue_to_address>
**issue (testing):** The assertion in `ButtonInteraction` is effectively a no-op and will always pass.
`expect(called || true).toBe(true)` will always pass, even if no callbacks are invoked, because `called || true` is always `true`. This makes the interaction test ineffective. Instead, assert directly on `called` (e.g. `expect(called).toBe(true)`) or, preferably, assert that the specific callback expected from clicking the first button was called.
</issue_to_address>
### Comment 2
<location> `packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/cancel.test.ts:49-58` </location>
<code_context>
+
+ Scenario(
+ 'Attempting to cancel a non-existent reservation request',
+ ({ Given, And, When, Then }) => {
+ Given('a reservation request ID "reservation-999"', () => {
+ command = { id: 'reservation-999' };
+ });
</code_context>
<issue_to_address>
**issue (testing):** `command` is assigned without `callerId`, which violates the `ReservationRequestCancelCommand` type.
Here `command` is set to `{ id: 'reservation-999' }`, but `ReservationRequestCancelCommand` requires both `id` and `callerId`, so this will not type-check. Please include a `callerId` in the test data (e.g. `{ id: 'reservation-999', callerId: 'user-123' }`) to satisfy the type and keep the test compiling.
</issue_to_address>
### Comment 3
<location> `packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/cancel.ts:23-30` </location>
<code_context>
+ throw new Error('Reservation request not found');
+ }
+
+ const reserver = await reservationRequest.loadReserver();
+ if (reserver.id !== command.callerId) {
+ throw new Error(
+ 'Only the reserver can cancel their reservation request',
+ );
+ }
+
+ reservationRequest.state = 'Cancelled';
+ reservationRequestToReturn = await repo.save(reservationRequest);
+ },
</code_context>
<issue_to_address>
**issue (bug_risk):** Cancelling does not enforce reservation state, allowing cancellation from any status.
The feature specifies cancelling a *requested* reservation, but this implementation only validates the caller and ignores the current state. This permits cancelling reservations in any state (e.g. Accepted/Rejected/Cancelled/Closed). If cancellation should be limited (e.g. to `Requested`), add a state check before setting `state = 'Cancelled'` and cover this with tests.
</issue_to_address>
### Comment 4
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:135` </location>
<code_context>
+ },
+};
+
+export const RequestedWithPopconfirm: Story = {
+ args: {
+ status: 'REQUESTED',
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared DOM-access, waiting, and assertion logic into reusable helper functions to simplify and de-duplicate the new Storybook stories while preserving behavior.
You can reduce the complexity and duplication in the new stories by extracting the repeated patterns into a few small helpers, while keeping behavior identical.
### 1. Centralize DOM access & waits
Add helpers near the top of the file (after imports, before `meta`):
```ts
const POPOVER_SELECTORS = {
title: '.ant-popconfirm-title',
description: '.ant-popconfirm-description',
confirmButton: '.ant-popconfirm-buttons .ant-btn-primary',
cancelButton: '.ant-popconfirm-buttons .ant-btn:not(.ant-btn-primary)',
};
const getButtons = (canvas: ReturnType<typeof within>) =>
canvas.getAllByRole('button');
const queryButtons = (canvas: ReturnType<typeof within>) =>
canvas.queryAllByRole('button');
const getFirstButton = (canvas: ReturnType<typeof within>) =>
getButtons(canvas)[0];
const waitForPopconfirm = () =>
new Promise<void>((resolve) => setTimeout(resolve, 100));
const getPopconfirmElements = () => ({
title: document.querySelector(POPOVER_SELECTORS.title),
description: document.querySelector(POPOVER_SELECTORS.description),
confirmButton: document.querySelector(
POPOVER_SELECTORS.confirmButton,
) as HTMLElement | null,
cancelButton: document.querySelector(
POPOVER_SELECTORS.cancelButton,
) as HTMLElement | null,
});
const expectNoButtons = (canvas: ReturnType<typeof within>) => {
const buttons = queryButtons(canvas);
expect(buttons.length).toBe(0);
};
const expectEmptyCanvas = (canvasElement: HTMLElement) => {
expect(canvasElement.children.length).toBe(0);
};
```
### 2. Use helpers in the stories
For example, `RequestedWithPopconfirm` becomes:
```ts
export const RequestedWithPopconfirm: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const cancelButton = getFirstButton(canvas);
expect(cancelButton).toBeTruthy();
if (cancelButton) {
await userEvent.click(cancelButton);
await waitForPopconfirm();
const { title, description, confirmButton } = getPopconfirmElements();
expect(title?.textContent).toContain('Cancel Reservation Request');
expect(description?.textContent).toContain('Are you sure');
if (confirmButton) {
await userEvent.click(confirmButton);
expect(args.onCancel).toHaveBeenCalled();
}
}
},
};
```
`PopconfirmCancelAction`:
```ts
export const PopconfirmCancelAction: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const cancelButton = getFirstButton(canvas);
expect(cancelButton).toBeTruthy();
if (cancelButton) {
await userEvent.click(cancelButton);
await waitForPopconfirm();
const { cancelButton: cancelPopconfirmButton } = getPopconfirmElements();
if (cancelPopconfirmButton) {
await userEvent.click(cancelPopconfirmButton);
expect(args.onCancel).not.toHaveBeenCalled();
}
}
},
};
```
`RejectedNoActions`:
```ts
export const RejectedNoActions: Story = {
args: {
status: 'REJECTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
expectNoButtons(canvas);
expectEmptyCanvas(canvasElement);
},
};
```
Similarly, `CancelledNoActions`, `ClosedNoActions`, `AcceptedActions`, `CancelLoadingState`, and `CloseLoadingState` can all delegate to `getButtons`, `getFirstButton`, and `expectNoButtons` instead of open‑coded `canvas.getAllByRole` / `queryAllByRole` and array indexing. This keeps all current behavior but significantly lowers repetition and makes the tests’ intent clearer.
</issue_to_address>
### Comment 5
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.stories.tsx:156` </location>
<code_context>
},
};
+
+export const CancelReservationSuccess: Story = {
+ args: {
+ listing: mockListing,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared Apollo mocks and the cancel/confirm play logic into reusable helpers to simplify these new Storybook stories and avoid duplication.
You can reduce the added complexity by centralizing the shared mocks and the cancel/confirm interaction, while keeping all scenarios and behavior intact.
### 1. Deduplicate Apollo mocks
All three stories rebuild the same two initial mocks. Extract a helper and compose per-story mocks:
```ts
const buildBaseListingMocks = () => [
{
request: {
query: ViewListingCurrentUserDocument,
},
result: {
data: {
currentUser: mockCurrentUser,
},
},
},
{
request: {
query: ViewListingQueryActiveByListingIdDocument,
variables: { listingId: '1' },
},
result: {
data: {
queryActiveByListingId: [],
},
},
},
];
```
Then reuse it:
```ts
export const CancelReservationSuccess: Story = {
// ...args unchanged...
parameters: {
apolloClient: {
mocks: [
...buildBaseListingMocks(),
{
request: {
query: HomeListingInformationCancelReservationRequestDocument,
variables: {
input: {
id: 'res-cancel-1',
},
},
},
result: {
data: {
cancelReservation: {
__typename: 'ReservationRequest',
id: 'res-cancel-1',
state: 'Cancelled',
},
},
},
},
{
request: {
query: ViewListingActiveReservationRequestForListingDocument,
},
result: {
data: {
myActiveReservationForListing: null,
},
},
},
],
},
},
};
```
Similarly for `CancelReservationError` and `CancelReservationLoading` (only the cancel mock differs).
### 2. Deduplicate cancel + confirm interaction and avoid fixed sleeps
The success and error stories share almost identical `play` flows with hardcoded `setTimeout` calls. You can wrap this in a helper using `waitFor`/`findByRole` (from `@storybook/testing-library`) to avoid brittle timing logic:
```ts
import { expect, userEvent, within, waitFor } from 'storybook/test';
const clickCancelThenConfirm = async (canvasElement: HTMLElement) => {
const canvas = within(canvasElement);
// Wait for the Cancel button to be in the DOM
const cancelButton = await canvas.findByRole('button', { name: /Cancel/i });
await userEvent.click(cancelButton);
// Wait for the Popconfirm confirm button, then click it
const confirmButton = await waitFor(() => {
const btn = document.querySelector(
'.ant-popconfirm-buttons .ant-btn-primary',
) as HTMLElement | null;
if (!btn) throw new Error('Confirm button not found yet');
return btn;
});
await userEvent.click(confirmButton);
};
```
Then the `play` functions become focused and less repetitive:
```ts
export const CancelReservationSuccess: Story = {
// ...args/parameters...
play: async ({ canvasElement }) => {
await expect(canvasElement).toBeTruthy();
await clickCancelThenConfirm(canvasElement);
// any success-specific assertions can go here
},
};
export const CancelReservationError: Story = {
// ...args/parameters...
play: async ({ canvasElement }) => {
await expect(canvasElement).toBeTruthy();
await clickCancelThenConfirm(canvasElement);
// any error-specific assertions can go here
},
};
```
`CancelReservationLoading` can keep its minimal `play` or reuse the helper if you want to actually trigger the loading state visually.
</issue_to_address>
### Comment 6
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.stories.tsx:324` </location>
<code_context>
},
};
+export const CancelButtonWithPopconfirm: Story = {
+ args: {
+ userReservationRequest: {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared Popconfirm-related helpers and common reservation-request setup so each story play function focuses on scenario behavior instead of repeating low-level mechanics.
The new Popconfirm stories add quite a bit of duplicated “mechanical” logic (building similar `userReservationRequest` args, clicking the cancel button, `setTimeout` waits, raw CSS selectors). You can keep all behavior but reduce complexity by extracting a few small helpers.
For example:
```ts
// at top of file (or a small test-utils module in same folder)
const POPCONFIRM_TITLE_SELECTOR = '.ant-popconfirm-title';
const POPCONFIRM_CONFIRM_SELECTOR = '.ant-popconfirm-buttons .ant-btn-primary';
const POPCONFIRM_CANCEL_SELECTOR =
'.ant-popconfirm-buttons .ant-btn:not(.ant-btn-primary)';
const baseReservationRequest = {
__typename: 'ReservationRequest' as const,
id: 'res-1',
reservationPeriodStart: '1738368000000',
reservationPeriodEnd: '1739145600000',
};
const createUserReservationRequest = (state: 'Requested' | 'Accepted') => ({
...baseReservationRequest,
state,
});
const waitForPopconfirm = async () => {
// keep behavior identical, just centralized
await new Promise((resolve) => setTimeout(resolve, 100));
};
const openCancelRequestPopconfirm = async (canvas: ReturnType<typeof within>) => {
const cancelButton = canvas.queryByRole('button', { name: /Cancel Request/i });
expect(cancelButton).toBeTruthy();
if (cancelButton) {
await userEvent.click(cancelButton);
await waitForPopconfirm();
}
};
const confirmPopconfirm = async () => {
const confirmButton = document.querySelector(
POPCONFIRM_CONFIRM_SELECTOR,
) as HTMLElement | null;
if (confirmButton) {
await userEvent.click(confirmButton);
}
};
const cancelPopconfirm = async () => {
const cancelButton = document.querySelector(
POPCONFIRM_CANCEL_SELECTOR,
) as HTMLElement | null;
if (cancelButton) {
await userEvent.click(cancelButton);
}
};
```
Then your stories become much smaller and more focused on behavior:
```ts
export const CancelButtonWithPopconfirm: Story = {
args: {
userReservationRequest: createUserReservationRequest('Requested'),
onCancelClick: fn(),
cancelLoading: false,
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await openCancelRequestPopconfirm(canvas);
const title = document.querySelector(POPCONFIRM_TITLE_SELECTOR);
expect(title?.textContent).toContain('Cancel Reservation Request');
await confirmPopconfirm();
expect(args.onCancelClick).toHaveBeenCalled();
},
};
export const CancelButtonLoading: Story = {
args: {
userReservationRequest: createUserReservationRequest('Requested'),
cancelLoading: true,
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
const cancelButton = canvas.queryByRole('button', { name: /Cancel Request/i });
expect(cancelButton).toBeTruthy();
},
};
export const NoCancelButtonForAcceptedReservation: Story = {
args: {
userReservationRequest: createUserReservationRequest('Accepted'),
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
const cancelButton = canvas.queryByRole('button', { name: /Cancel/i });
expect(cancelButton).toBeNull();
},
};
export const PopconfirmCancelButton: Story = {
args: {
userReservationRequest: createUserReservationRequest('Requested'),
onCancelClick: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await openCancelRequestPopconfirm(canvas);
await cancelPopconfirm();
expect(args.onCancelClick).not.toHaveBeenCalled();
},
};
```
This keeps all existing behavior, but:
- Centralizes the Popconfirm selectors and `setTimeout`-based wait.
- Deduplicates the `userReservationRequest` construction.
- Makes each `play` function read as “scenario description” instead of reimplementing the low-level flow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...rethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx
Outdated
Show resolved
Hide resolved
...rethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx
Show resolved
Hide resolved
...s/home/components/view-listing/listing-information/listing-information.container.stories.tsx
Show resolved
Hide resolved
...nts/layouts/home/components/view-listing/listing-information/listing-information.stories.tsx
Show resolved
Hide resolved
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
cancelapplication-service currently mutatesreservationRequest.state = 'Cancelled'directly without checking the current state; consider delegating to a domain method or explicitly validating allowed state transitions (e.g., only fromRequested) so real behavior aligns with the "cannot cancel reservation in current state" scenario you test for. - Popconfirm-related helpers and CSS selectors (e.g.,
POPCONFIRM_SELECTORS,waitForPopconfirm, confirm/cancel utilities) are duplicated across multiple Storybook files; extracting these into a shared test utility would reduce duplication and keep Popconfirm interaction logic consistent. - Several Storybook interaction tests rely on Ant Design-specific CSS selectors like
.ant-popconfirm-titleand.ant-popconfirm-buttons; where possible, prefer queries by role/label/text to make the tests less brittle to UI library markup changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `cancel` application-service currently mutates `reservationRequest.state = 'Cancelled'` directly without checking the current state; consider delegating to a domain method or explicitly validating allowed state transitions (e.g., only from `Requested`) so real behavior aligns with the "cannot cancel reservation in current state" scenario you test for.
- Popconfirm-related helpers and CSS selectors (e.g., `POPCONFIRM_SELECTORS`, `waitForPopconfirm`, confirm/cancel utilities) are duplicated across multiple Storybook files; extracting these into a shared test utility would reduce duplication and keep Popconfirm interaction logic consistent.
- Several Storybook interaction tests rely on Ant Design-specific CSS selectors like `.ant-popconfirm-title` and `.ant-popconfirm-buttons`; where possible, prefer queries by role/label/text to make the tests less brittle to UI library markup changes.
## Individual Comments
### Comment 1
<location> `packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/cancel.ts:30` </location>
<code_context>
+ );
+ }
+
+ reservationRequest.state = 'Cancelled';
+ reservationRequestToReturn = await repo.save(reservationRequest);
+ },
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider validating allowed state transitions before setting the reservation to `Cancelled`.
The service currently sets `reservationRequest.state = 'Cancelled'` for any reservation the caller owns, so states like `Accepted` or `Closed` can also be cancelled, and the “Cancellation fails when reservation is in Accepted state” behavior relies on the repository throwing. Instead, add explicit state-transition checks here (e.g., only allow `Requested` → `Cancelled`) and throw a clear error when the current state is not cancellable, so the rule is enforced at the domain level rather than by persistence side effects.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.stories.tsx:332` </location>
<code_context>
+ },
+ },
+ },
+ delay: 3000, // Simulate slow response
+ result: {
+ data: {
</code_context>
<issue_to_address>
**suggestion (performance):** The 3s artificial delay in the loading story can slow down the Storybook/test run unnecessarily.
In `CancelReservationLoading`, the mock sets `delay: 3000`, but the `play` function only checks that `canvasElement` is truthy and never triggers the cancel mutation. As a result, the delay is paid on every run without being tied to any behavior. Consider either reducing the delay (e.g. to 200–300ms) or updating `play` to actually click the cancel button so the delayed response is meaningfully exercised.
```suggestion
delay: 200, // Simulate slow response without slowing tests too much
```
</issue_to_address>
### Comment 3
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:14` </location>
<code_context>
+
+type Canvas = ReturnType<typeof within>;
+
+const getButtons = (canvas: Canvas) => canvas.getAllByRole('button');
+const queryButtons = (canvas: Canvas) => canvas.queryAllByRole('button');
+const getFirstButton = (canvas: Canvas) => getButtons(canvas)[0];
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new story helpers and stories to consolidate repeated patterns (buttons, popconfirm flow, no-actions, loading) and inline tiny one-off utilities to keep the file smaller and easier to scan.
You can keep all the new coverage but reduce surface area by consolidating the repeated patterns and inlining the one‑off helpers.
### 1. Inline or group tiny helpers
`getButtons`, `queryButtons`, `getFirstButton`, `expectNoButtons`, `expectEmptyCanvas` are simple and used in very few places. Either inline them or group them in a single local utility to cut down on globals:
```ts
// near top of file
const canvasUtils = {
getButtons: (canvas: Canvas) => canvas.getAllByRole('button'),
getFirstButton: (canvas: Canvas) => canvas.getAllByRole('button')[0],
assertNoButtons: (canvas: Canvas) =>
expect(canvas.queryAllByRole('button').length).toBe(0),
};
```
Then use:
```ts
const buttons = canvasUtils.getButtons(canvas);
canvasUtils.assertNoButtons(canvas);
```
For `expectEmptyCanvas`, it’s only used once; inlining keeps mental overhead lower:
```ts
// instead of expectEmptyCanvas(canvasElement);
expect(canvasElement.children.length).toBe(0);
```
### 2. Consolidate Popconfirm flow
`RequestedWithPopconfirm` and `PopconfirmCancelAction` share the same “click first button → wait for popconfirm → click X” flow. A small helper with a parameter for confirm vs cancel keeps behavior identical but centralizes the control flow:
```ts
type PopconfirmAction = 'confirm' | 'cancel';
const triggerPopconfirmAnd = async (
canvas: Canvas,
action: PopconfirmAction,
) => {
const cancelButton = canvas.getAllByRole('button')[0];
expect(cancelButton).toBeTruthy();
if (!cancelButton) return;
await userEvent.click(cancelButton);
await waitForPopconfirm();
const { title, description, confirmButton, cancelButton: cancelPopBtn } =
getPopconfirmElements();
expect(title?.textContent).toContain('Cancel Reservation Request');
expect(description?.textContent).toContain('Are you sure');
const target =
action === 'confirm' ? confirmButton : cancelPopBtn;
if (target) {
await userEvent.click(target);
}
};
```
Stories become thinner and more symmetric:
```ts
export const RequestedWithPopconfirm: Story = {
args: { status: 'REQUESTED', onCancel: fn(), onMessage: fn() },
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await triggerPopconfirmAnd(canvas, 'confirm');
expect(args.onCancel).toHaveBeenCalled();
},
};
export const PopconfirmCancelAction: Story = {
args: { status: 'REQUESTED', onCancel: fn(), onMessage: fn() },
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await triggerPopconfirmAnd(canvas, 'cancel');
expect(args.onCancel).not.toHaveBeenCalled();
},
};
```
### 3. Table‑drive “no actions” stories
`RejectedNoActions`, `CancelledNoActions`, `ClosedNoActions` are nearly identical. You can generate them from a small helper to avoid repeating `play` logic and keep expectations consistent:
```ts
const createNoActionsStory = (status: 'REJECTED' | 'CANCELLED' | 'CLOSED', checkEmptyCanvas = false): Story => ({
args: {
status,
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.queryAllByRole('button').length).toBe(0);
if (checkEmptyCanvas) {
expect(canvasElement.children.length).toBe(0);
}
},
});
export const RejectedNoActions = createNoActionsStory('REJECTED', true);
export const CancelledNoActions = createNoActionsStory('CANCELLED');
export const ClosedNoActions = createNoActionsStory('CLOSED');
```
### 4. Consolidate loading state assertions
`LoadingStates`, `CancelLoadingState`, and `CloseLoadingState` all assert “buttons exist” and then check specific loading/first button behaviors. A shared helper makes intent clearer:
```ts
const assertButtonsWithLoading = (canvas: Canvas) => {
const buttons = canvas.getAllByRole('button');
expect(buttons.length).toBeGreaterThan(0);
return buttons;
};
// usage
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = assertButtonsWithLoading(canvas);
const cancelButton = buttons[0];
expect(cancelButton).toBeTruthy();
},
```
This way you keep all the states and expectations but reduce the number of small globals and repeated patterns, making the file easier to scan and reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… and shared popconfirm test utilities
… and vitest-config, sthrift ui-components
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the
cancelapplication-service, you mutatereservationRequest.state = 'Cancelled'directly and hard-codeCANCELLABLE_STATES; if the domain entity already exposes behavior (e.g. acancel()method or state machine), consider delegating to that to avoid duplicating business rules and to keep invariants centralized. - The new
cancelReservationresolver throws genericErrorinstances with user-facing messages ('User must be authenticated...','User not found'); consider using a structured error (e.g.GraphQLErrorwith extensions or a shared error helper) so that the client can distinguish auth vs. not-found vs. domain errors without relying on message text. - The Popconfirm Storybook test utilities use a 1s timeout in
waitForPopconfirmand inclickCancelThenConfirm; depending on CI performance this might be a bit tight, so you may want to slightly increase or make the timeout configurable to reduce flakiness in interaction tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `cancel` application-service, you mutate `reservationRequest.state = 'Cancelled'` directly and hard-code `CANCELLABLE_STATES`; if the domain entity already exposes behavior (e.g. a `cancel()` method or state machine), consider delegating to that to avoid duplicating business rules and to keep invariants centralized.
- The new `cancelReservation` resolver throws generic `Error` instances with user-facing messages (`'User must be authenticated...'`, `'User not found'`); consider using a structured error (e.g. `GraphQLError` with extensions or a shared error helper) so that the client can distinguish auth vs. not-found vs. domain errors without relying on message text.
- The Popconfirm Storybook test utilities use a 1s timeout in `waitForPopconfirm` and in `clickCancelThenConfirm`; depending on CI performance this might be a bit tight, so you may want to slightly increase or make the timeout configurable to reduce flakiness in interaction tests.
## Individual Comments
### Comment 1
<location> `packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/cancel.ts:9` </location>
<code_context>
+ callerId: string;
+}
+
+const CANCELLABLE_STATES = ['Requested', 'Rejected'] as const;
+
+export const cancel = (dataSources: DataSources) => {
</code_context>
<issue_to_address>
**suggestion:** Consider deriving cancellable states from the domain model instead of hardcoding literal strings here.
`CANCELLABLE_STATES` redefines state names as string literals and then relies on casting `reservationRequest.state` back into that union. If the domain model exposes an enum or a “can cancel” API (e.g. `reservationRequest.canCancel()`), using that here would keep this layer aligned with domain rules and prevent drift when states are added or renamed. If such an API doesn’t exist yet, consider adding it to centralize cancellation rules in the domain model and consuming it here.
Suggested implementation:
```typescript
export const cancel = (dataSources: DataSources) => {
```
```typescript
const reservationRequest = await repo.getById(command.id);
if (!reservationRequest) {
throw new Error(`ReservationRequest with id ${command.id} not found`);
}
if (!reservationRequest.canCancel?.(command.callerId)) {
// Prefer a domain-specific error type if available
throw new Error(
`ReservationRequest with id ${command.id} cannot be cancelled in its current state`,
);
}
const cancelledReservationRequest = await reservationRequest.cancel(command.callerId);
await repo.save(cancelledReservationRequest);
reservationRequestToReturn = cancelledReservationRequest.toReference();
```
To fully align with the suggestion and make this compile, the following domain-layer changes (and possibly minor repository API adjustments) are required:
1. **ReservationRequest domain model**
- Add a domain API that centralizes cancellation rules, for example:
- `canCancel(callerId: string): boolean` (instance method), and
- `cancel(callerId: string): ReservationRequest` (instance method that performs state transition and enforces invariants).
- Inside `canCancel`, use the domain’s own state enum/type (e.g. `ReservationRequestState`) instead of string literals to determine which states are cancellable (e.g. `Requested`, `Rejected`, etc).
- Inside `cancel`, call `canCancel` and either:
- throw a domain-specific error (e.g. `ReservationRequestCannotBeCancelledError`), or
- return a `Result`/`Either` type if that’s the existing pattern.
2. **Repository / Unit of Work API**
- Ensure `ReservationRequestUnitOfWork`’s `repo` exposes the methods used above:
- `getById(id: string)` (already present),
- `save(reservationRequest)` (or equivalent like `update` / `store` / `commitChanges`).
- If the repository already tracks changes inside the unit of work, you may not need an explicit `save` call; in that case, remove `await repo.save(...)` and rely on the unit of work’s commit semantics.
3. **Error handling / result mapping**
- If the application services layer typically maps domain errors to a result type or an error DTO instead of throwing, update the error handling here accordingly:
- Replace the `throw new Error(...)` statements with the appropriate error construction / return pattern so API consumers receive consistent errors.
- If `ReservationRequestEntityReference` is normally obtained via a mapper (e.g. `reservationRequest.toReference()` or a dedicated mapper service), ensure `cancelledReservationRequest.toReference()` exists or adjust to use the existing mapping utility.
4. **Remove any remaining state-string usages**
- If there are other usages of string literals like `'Requested'` / `'Rejected'` for cancellation decisions in this context (or nearby services), refactor them to use the centralized domain API (`canCancel`/`cancel`) to avoid duplication and prevent drift when states change.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/test-utils/popconfirm-test-utils.stories.tsx:160-176` </location>
<code_context>
+ },
+};
+
+export const CancelPopconfirmSuccess: Story = {
+ args: {
+ onCancel: fn(),
+ onConfirm: fn(),
+ },
+ play: async ({ canvasElement, args }) => {
+ const canvas = within(canvasElement);
+
+ const triggerButton = canvasUtils.getFirstButton(canvas);
+ await userEvent.click(triggerButton);
+ await waitForPopconfirm();
+
+ await cancelPopconfirm();
+ // onConfirm should NOT be called when cancelling
+ expect(args.onConfirm).not.toHaveBeenCalled();
+ },
+};
</code_context>
<issue_to_address>
**suggestion (testing):** Popconfirm cancel-flow test should also assert `onCancel` is invoked
Since this helper is meant to validate Popconfirm behavior, we should also assert that `onCancel` is called after `cancelPopconfirm()`. That will ensure the cancel handler continues to fire and improve this story’s usefulness for catching regressions.
```suggestion
export const CancelPopconfirmSuccess: Story = {
args: {
onCancel: fn(),
onConfirm: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const triggerButton = canvasUtils.getFirstButton(canvas);
await userEvent.click(triggerButton);
await waitForPopconfirm();
await cancelPopconfirm();
// onCancel should be called when cancelling
expect(args.onCancel).toHaveBeenCalled();
// onConfirm should NOT be called when cancelling
expect(args.onConfirm).not.toHaveBeenCalled();
},
};
```
</issue_to_address>
### Comment 3
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:146` </location>
<code_context>
+ },
+};
+
+export const RequestedWithPopconfirm: Story = {
+ args: {
+ status: 'REQUESTED',
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated popconfirm flows, no-action story patterns, and button presence checks into small shared helpers to simplify and deduplicate these Storybook stories.
You can reduce the new complexity by extracting the repeated flows into small helpers and using them across stories. This keeps all behaviours but makes the stories much easier to scan.
### 1. Extract shared Popconfirm flows
The `RequestedWithPopconfirm`, `PopconfirmCancelAction`, and `RejectedWithCancel` stories all reimplement the same “click cancel → wait for popconfirm → assert elements → click confirm/cancel” logic with small variations.
You can centralise this:
```ts
// keep near top of file
const openCancelPopconfirm = async (canvas: Canvas) => {
const cancelButton = getFirstButton(canvas);
expect(cancelButton).toBeTruthy();
if (!cancelButton) return null;
await userEvent.click(cancelButton);
await waitForPopconfirm();
const elements = getPopconfirmElements();
const { title, description } = elements;
expect(title?.textContent).toContain('Cancel Reservation Request');
expect(description?.textContent).toContain('Are you sure');
return elements;
};
const confirmCancellation = async (elements: ReturnType<typeof getPopconfirmElements> | null, onCancel: jest.Mock) => {
if (!elements?.confirmButton) return;
await userEvent.click(elements.confirmButton);
expect(onCancel).toHaveBeenCalled();
};
const cancelPopconfirmOnly = async (elements: ReturnType<typeof getPopconfirmElements> | null, onCancel: jest.Mock) => {
if (!elements?.cancelButton) return;
await userEvent.click(elements.cancelButton);
expect(onCancel).not.toHaveBeenCalled();
};
```
Then your stories collapse to the intent only:
```ts
export const RequestedWithPopconfirm: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const elements = await openCancelPopconfirm(canvas);
await confirmCancellation(elements, args.onCancel);
},
};
export const PopconfirmCancelAction: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const elements = await openCancelPopconfirm(canvas);
await cancelPopconfirmOnly(elements, args.onCancel);
},
};
export const RejectedWithCancel: Story = {
args: {
status: 'REJECTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
// REJECTED status should have Cancel button
const buttons = getButtons(canvas);
expect(buttons.length).toBe(1);
const elements = await openCancelPopconfirm(canvas);
await confirmCancellation(elements, args.onCancel);
},
};
```
This removes the repeated popconfirm wiring while preserving all assertions.
### 2. Consolidate “no actions” stories
`CancelledNoActions` and `ClosedNoActions` are identical apart from `status`. You already have `expectNoButtons`; you can take one small step further with a tiny helper factory:
```ts
const statusWithNoActions = (status: 'CLOSED' | 'CANCELLED'): Story => ({
args: {
status,
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expectNoButtons(canvas);
},
});
export const CancelledNoActions: Story = statusWithNoActions('CANCELLED');
export const ClosedNoActions: Story = statusWithNoActions('CLOSED');
```
This keeps two stories in Storybook (for discoverability) but removes duplicated play logic.
### 3. Reuse button presence / loading checks
`LoadingStates`, `CancelLoadingState`, `CloseLoadingState`, and parts of `Requested`/`Accepted` all do “get buttons, assert length > 0, maybe assert first button truthy”. You can capture the common pattern:
```ts
const expectButtonsPresent = (canvas: Canvas, expectedCount?: number) => {
const buttons = getButtons(canvas);
expect(buttons.length).toBeGreaterThan(0);
if (expectedCount !== undefined) {
expect(buttons.length).toBe(expectedCount);
}
return buttons;
};
```
Then e.g.:
```ts
export const AcceptedActions: Story = {
args: {
status: 'ACCEPTED',
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = expectButtonsPresent(canvas, 2);
// buttons[0] is close, buttons[1] is message
},
};
export const CancelLoadingState: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
cancelLoading: true,
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = expectButtonsPresent(canvas);
const cancelButton = buttons[0];
expect(cancelButton).toBeTruthy();
},
};
```
These small helpers keep functionality intact but make the story matrix much easier to scan and maintain, and they align with the existing `canvasUtils` / popconfirm utilities you’ve already introduced.
</issue_to_address>
### Comment 4
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/components/reservation-actions.tsx:27` </location>
<code_context>
- onClick={onCancel}
- loading={cancelLoading}
- />,
+ <Popconfirm
+ key="cancel-confirm"
+ title="Cancel Reservation Request"
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated Popconfirm-based cancel button into a reusable helper component to simplify the reservation status switch branches.
You can remove the duplicated `Popconfirm` block by extracting a small helper component or function. That keeps the behaviour the same while simplifying the switch branches and reducing the chance of future divergence.
For example, inside this file:
```tsx
const CancelWithConfirm: React.FC<{
onConfirm?: () => void;
loading?: boolean;
}> = ({ onConfirm, loading }) => (
<Popconfirm
key="cancel-confirm"
title="Cancel Reservation Request"
description="Are you sure you want to cancel this request?"
onConfirm={onConfirm}
okText="Yes"
cancelText="No"
>
<span>
<ReservationActionButton
key="cancel"
action="Cancel"
loading={loading}
/>
</span>
</Popconfirm>
);
```
Then your `switch` becomes:
```tsx
const getActionsForStatus = () => {
switch (status) {
case 'REQUESTED':
return [
<CancelWithConfirm
key="cancel-confirm"
onConfirm={onCancel}
loading={cancelLoading}
/>,
<ReservationActionButton
key="message"
action="Message"
onClick={onMessage}
/>,
];
case 'REJECTED':
return [
<CancelWithConfirm
key="cancel-confirm"
onConfirm={onCancel}
loading={cancelLoading}
/>,
];
// ...
}
};
```
This keeps the confirmation UI identical for both states while making the logic easier to read and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...rethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the application-service
cancelimplementation, the state transition and allowed states are hard-coded string literals; consider delegating the transition (including which states are cancellable) to the domain aggregate or a shared enum to avoid future drift if the reservation state machine changes. - The
cancelReservationGraphQL resolver reimplements JWT/current-user lookup and genericErrorthrowing inline; if there are existing helpers or a common error type for authenticated mutations, reusing them here would reduce duplication and keep authentication/authorization error handling consistent across resolvers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the application-service `cancel` implementation, the state transition and allowed states are hard-coded string literals; consider delegating the transition (including which states are cancellable) to the domain aggregate or a shared enum to avoid future drift if the reservation state machine changes.
- The `cancelReservation` GraphQL resolver reimplements JWT/current-user lookup and generic `Error` throwing inline; if there are existing helpers or a common error type for authenticated mutations, reusing them here would reduce duplication and keep authentication/authorization error handling consistent across resolvers.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:93-94` </location>
<code_context>
+ play: async ({ canvasElement, args }) => {
+ const canvas = within(canvasElement);
+
+ // Get all buttons
+ const buttons = getButtons(canvas);
+ expect(buttons.length).toBeGreaterThan(0);
+
</code_context>
<issue_to_address>
**suggestion (testing):** Guard against assumptions about button ordering in `ButtonInteraction`.
This relies on `buttons[1]` always being the message button in the `REQUESTED` state, which makes the story brittle if button order or layout changes. Prefer selecting the button by accessible name (role + label), or at least assert `buttons.length > 1` and clearly document the ordering assumption in the test.
Suggested implementation:
```typescript
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
// Guard against assumptions about button ordering by asserting count
// and selecting the message button via its accessible name.
const buttons = getButtons(canvas);
expect(buttons.length).toBeGreaterThan(1);
const messageButton = canvas.getByRole('button', { name: /message/i });
// Use `messageButton` for interactions below instead of relying on array indices.
},
type Canvas = ReturnType<typeof within>;
```
If elsewhere in the same `play` function you currently have something like `await userEvent.click(buttons[1]);`, update it to instead use the accessible-name-selected button:
- Replace `buttons[1]` with `messageButton`.
For example:
- `await userEvent.click(buttons[1]);` → `await userEvent.click(messageButton);`
Ensure `userEvent` is imported (e.g., from `@storybook/testing-library` or `@testing-library/user-event`) if it is not already.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.tsx:125-134` </location>
<code_context>
}
};
+ const handleCancelClick = async () => {
+ if (!userReservationRequest?.id) {
+ message.error('No reservation request to cancel');
+ return;
+ }
+ try {
+ await cancelReservationRequestMutation({
+ variables: {
+ input: {
+ id: userReservationRequest.id,
+ },
+ },
+ });
+ } catch (error) {
+ console.error('Error cancelling reservation request:', error);
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Prevent duplicate cancel mutations when a cancel request is already in flight.
`handleCancelClick` doesn’t guard on `cancelLoading`, so rapid clicks or multiple triggers can enqueue duplicate `cancelReservationRequestMutation` calls. Consider an early return when `cancelLoading` is true (similar to `handleReserveClick`) to avoid repeated side effects and extra error toasts.
</issue_to_address>
### Comment 3
<location> `apps/ui-sharethrift/src/test-utils/popconfirm-test-utils.stories.tsx:160-51` </location>
<code_context>
+ },
+};
+
+export const CancelPopconfirmSuccess: Story = {
+ args: {
+ onCancel: fn(),
+ onConfirm: fn(),
+ },
+ play: async ({ canvasElement, args }) => {
+ const canvas = within(canvasElement);
</code_context>
<issue_to_address>
**suggestion (testing):** CancelPopconfirmSuccess does not verify that the onCancel handler is invoked
The story already checks that `onConfirm` is not called on cancel, but it should also assert that `onCancel` is invoked, e.g. `expect(args.onCancel).toHaveBeenCalled()`, to ensure `cancelPopconfirm` correctly triggers the cancel handler.
Suggested implementation:
```typescript
await cancelPopconfirm();
expect(args.onCancel).toHaveBeenCalled();
expect(args.onConfirm).not.toHaveBeenCalled();
```
If the `CancelPopconfirmSuccess` story uses a different helper name than `cancelPopconfirm` or if there is additional logic between the cancel action and the existing `expect(args.onConfirm).not.toHaveBeenCalled();` assertion, adjust the SEARCH block accordingly so that the new `expect(args.onCancel).toHaveBeenCalled();` line is inserted immediately before the assertion that `onConfirm` was not called.
</issue_to_address>
### Comment 4
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:146` </location>
<code_context>
+ },
+};
+
+export const RequestedWithPopconfirm: Story = {
+ args: {
+ status: 'REQUESTED',
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared Storybook play logic into small reusable helpers for Popconfirm flows, no-action states, and loading states to avoid repetition and keep stories focused on their unique assertions.
You can keep all the new coverage but cut a lot of repetition by extracting a few small helpers and reusing them across stories.
### 1. Popconfirm flows: factor out shared logic
These stories share most of their `play` body:
- `RequestedWithPopconfirm`
- `PopconfirmCancelAction`
- `RejectedWithCancel`
You can extract common operations (get cancel button, open Popconfirm, assert title/description, confirm/cancel) into tiny helpers:
```ts
// local helpers near the top of the file
const openCancelPopconfirm = async (canvas: Canvas) => {
const cancelButton = getFirstButton(canvas);
expect(cancelButton).toBeTruthy();
if (!cancelButton) return null;
await userEvent.click(cancelButton);
await waitForPopconfirm();
return getPopconfirmElements();
};
const confirmCancellation = async (elements: ReturnType<typeof getPopconfirmElements>) => {
const { confirmButton } = elements;
if (confirmButton) {
await userEvent.click(confirmButton);
}
};
const cancelCancellation = async (elements: ReturnType<typeof getPopconfirmElements>) => {
const { cancelButton } = elements;
if (cancelButton) {
await userEvent.click(cancelButton);
}
};
```
Then each story `play` becomes much smaller and focused:
```ts
export const RequestedWithPopconfirm: Story = {
// ...args
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const elements = await openCancelPopconfirm(canvas);
if (!elements) return;
const { title, description } = elements;
expect(title?.textContent).toContain('Cancel Reservation Request');
expect(description?.textContent).toContain('Are you sure');
await confirmCancellation(elements);
expect(args.onCancel).toHaveBeenCalled();
},
};
export const PopconfirmCancelAction: Story = {
// ...args
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const elements = await openCancelPopconfirm(canvas);
if (!elements) return;
await cancelCancellation(elements);
expect(args.onCancel).not.toHaveBeenCalled();
},
};
export const RejectedWithCancel: Story = {
// ...args
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const buttons = getButtons(canvas);
expect(buttons.length).toBe(1);
const elements = await openCancelPopconfirm(canvas);
if (!elements) return;
const { title, description } = elements;
expect(title?.textContent).toContain('Cancel Reservation Request');
expect(description?.textContent).toContain('Are you sure');
await confirmCancellation(elements);
expect(args.onCancel).toHaveBeenCalled();
},
};
```
This removes repeated Popconfirm setup while preserving the status‑specific expectations.
---
### 2. “No actions” states: reuse a generic `play`
`CancelledNoActions` and `ClosedNoActions` are identical except for `status`. You can define a shared `play` and reuse it:
```ts
const playExpectNoButtons: Story['play'] = ({ canvasElement }) => {
const canvas = within(canvasElement);
expectNoButtons(canvas);
};
export const CancelledNoActions: Story = {
args: {
status: 'CANCELLED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: playExpectNoButtons,
};
export const ClosedNoActions: Story = {
args: {
status: 'CLOSED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: playExpectNoButtons,
};
```
If more “no actions” states appear, they can plug into the same `play`.
---
### 3. Loading states: share the loading assertions
`CancelLoadingState` and `CloseLoadingState` also share their `play` logic (buttons exist, first button present). A small helper keeps them in sync:
```ts
const playLoadingState: Story['play'] = ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = getButtons(canvas);
expect(buttons.length).toBeGreaterThan(0);
const primaryButton = getFirstButton(canvas);
expect(primaryButton).toBeTruthy();
};
export const CancelLoadingState: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
cancelLoading: true,
},
play: playLoadingState,
};
export const CloseLoadingState: Story = {
args: {
status: 'ACCEPTED',
onClose: fn(),
onMessage: fn(),
closeLoading: true,
},
play: playLoadingState,
};
```
This keeps the behavior the same but reduces duplication and makes future UI changes easier to apply in one place.
</issue_to_address>
### Comment 5
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/components/reservation-actions.tsx:27` </location>
<code_context>
- onClick={onCancel}
- loading={cancelLoading}
- />,
+ <Popconfirm
+ key="cancel-confirm"
+ title="Cancel Reservation Request"
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated cancel Popconfirm markup into a shared helper component to keep behavior consistent and avoid duplication in the switch cases.
You can reduce duplication and keep behavior identical by extracting the cancel+Popconfirm into a small helper component or function.
For example, inside the same file:
```tsx
const CancelWithPopconfirm: React.FC<{
onConfirm?: () => void;
loading?: boolean;
}> = ({ onConfirm, loading }) => (
<Popconfirm
title="Cancel Reservation Request"
description="Are you sure you want to cancel this request?"
onConfirm={onConfirm}
okText="Yes"
cancelText="No"
>
<span>
<ReservationActionButton action="Cancel" loading={loading} />
</span>
</Popconfirm>
);
```
Then simplify the switch:
```tsx
const getActionsForStatus = () => {
switch (status) {
case 'REQUESTED':
return [
<CancelWithPopconfirm
key="cancel"
onConfirm={onCancel}
loading={cancelLoading}
/>,
<ReservationActionButton
key="message"
action="Message"
onClick={onMessage}
/>,
];
case 'ACCEPTED':
return [
<ReservationActionButton
key="close"
action="Close"
onClick={onClose}
loading={closeLoading}
/>,
<ReservationActionButton
key="message"
action="Message"
onClick={onMessage}
/>,
];
case 'REJECTED':
return [
<CancelWithPopconfirm
key="cancel"
onConfirm={onCancel}
loading={cancelLoading}
/>,
];
default:
return [];
}
};
```
This keeps all current behavior (including Popconfirm) but removes the duplicated Popconfirm markup and config, lowering the chance of the REQUESTED/REJECTED paths drifting apart.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...s/layouts/home/components/view-listing/listing-information/listing-information.container.tsx
Show resolved
Hide resolved
...rethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx
Show resolved
Hide resolved
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- There are several bespoke helpers for opening/clicking Popconfirms in stories (e.g.,
openCancelPopconfirmandexpectNoButtonsinreservation-actions.stories.tsx, similar logic inlisting-information.stories.tsx) that overlap with the newpopconfirm-test-utils; consider consolidating those call sites to use the shared utilities to reduce duplication and keep interactions consistent. - In the reservation request application service
cancelimplementation you mutatereservationRequest.statedirectly; if the domain model exposes behavior for state transitions (e.g., acancel()method or similar), it would be safer to delegate to that instead of assigning the state string to keep all invariants and side effects enforced at the domain layer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are several bespoke helpers for opening/clicking Popconfirms in stories (e.g., `openCancelPopconfirm` and `expectNoButtons` in `reservation-actions.stories.tsx`, similar logic in `listing-information.stories.tsx`) that overlap with the new `popconfirm-test-utils`; consider consolidating those call sites to use the shared utilities to reduce duplication and keep interactions consistent.
- In the reservation request application service `cancel` implementation you mutate `reservationRequest.state` directly; if the domain model exposes behavior for state transitions (e.g., a `cancel()` method or similar), it would be safer to delegate to that instead of assigning the state string to keep all invariants and side effects enforced at the domain layer.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:178-180` </location>
<code_context>
+ const buttons = canvas.getAllByRole('button');
+ expect(buttons.length).toBeGreaterThan(0);
+
+ // Check if any button shows loading state (might be disabled)
+ const disabledButtons = buttons.filter((b) => b.hasAttribute('disabled'));
+ expect(disabledButtons.length).toBeGreaterThanOrEqual(0);
+ },
</code_context>
<issue_to_address>
**suggestion (testing):** Assertion `toBeGreaterThanOrEqual(0)` on disabledButtons length is ineffective
`expect(disabledButtons.length).toBeGreaterThanOrEqual(0);` is a no-op, since the length is never negative and the assertion will always pass. If you want to assert that at least one button is in a loading/disabled state, use `toBeGreaterThan(0)` or assert directly on the loading UI (specific role, label, or spinner) so the test actually fails when the loading behavior regresses.
```suggestion
// Verify loading state is rendered
const buttons = canvas.getAllByRole('button');
expect(buttons.length).toBeGreaterThan(0);
// Check if any button shows loading state (might be disabled)
const disabledButtons = buttons.filter((b) => b.hasAttribute('disabled'));
expect(disabledButtons.length).toBeGreaterThan(0);
```
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:14` </location>
<code_context>
+
+const { getButtons, queryButtons, getFirstButton } = canvasUtils;
+
+const expectNoButtons = (canvas: Canvas) => {
+ const buttons = queryButtons(canvas);
+ expect(buttons.length).toBe(0);
</code_context>
<issue_to_address>
**issue (complexity):** Consider consolidating the repeated popconfirm and story behaviors into a few parameterized helpers so the stories only describe what varies (status, loading flag, and expected callbacks).
You can keep all the new coverage but reduce mental overhead by consolidating a few helpers and parameterizing the repeated story logic.
### 1. Collapse tiny popconfirm helpers into one utility
`openCancelPopconfirm`, `confirmPopconfirmAction`, and `cancelPopconfirmAction` all wrap the same “open popconfirm on first button, then click confirm/cancel” behavior.
You can keep the behavior but expose a single flow helper locally (or better: move it into `popconfirm-test-utils`) to remove duplication:
```ts
// popconfirm helper – local or in popconfirm-test-utils
type PopconfirmElements = ReturnType<typeof getPopconfirmElements>;
const openAndActOnFirstButtonPopconfirm = async (
canvas: Canvas,
action: 'confirm' | 'cancel',
): Promise<PopconfirmElements | null> => {
const triggerButton = getFirstButton(canvas);
expect(triggerButton).toBeTruthy();
if (!triggerButton) return null;
await userEvent.click(triggerButton);
await waitForPopconfirm();
const elements = getPopconfirmElements();
const buttonToClick =
action === 'confirm' ? elements.confirmButton : elements.cancelButton;
if (buttonToClick) {
await userEvent.click(buttonToClick);
}
return elements;
};
```
Then the stories become simpler and don’t need separate confirm/cancel helpers:
```ts
export const RequestedWithPopconfirm: Story = {
args: { status: 'REQUESTED', onCancel: fn(), onMessage: fn() },
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const elements = await openAndActOnFirstButtonPopconfirm(canvas, 'confirm');
if (!elements) return;
const { title, description } = elements;
expect(title?.textContent).toContain('Cancel Reservation Request');
expect(description?.textContent).toContain('Are you sure');
expect(args.onCancel).toHaveBeenCalled();
},
};
export const PopconfirmCancelAction: Story = {
args: { status: 'REQUESTED', onCancel: fn(), onMessage: fn() },
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await openAndActOnFirstButtonPopconfirm(canvas, 'cancel');
expect(args.onCancel).not.toHaveBeenCalled();
},
};
```
### 2. Unify “no buttons” stories
`expectNoButtons` + `playExpectNoButtons` + duplicated logic in `CancelledNoActions` and `ClosedNoActions` can be reduced to a single reusable `play` implementation and a small helper to create such stories:
```ts
const playExpectNoButtons: Story['play'] = ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = queryButtons(canvas);
expect(buttons.length).toBe(0);
};
const createNoActionsStory = (status: Story['args']['status']): Story => ({
args: {
status,
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: playExpectNoButtons,
});
export const CancelledNoActions = createNoActionsStory('CANCELLED');
export const ClosedNoActions = createNoActionsStory('CLOSED');
```
This keeps both stories but makes the “no actions for this status” rule obvious and central.
### 3. Unify loading-state stories
`LoadingStates`, `CancelLoadingState`, and `CloseLoadingState` plus `playLoadingState` all assert a very similar pattern.
You can drive them from a factory that sets the loading flag and shares the play function:
```ts
const playLoadingState: Story['play'] = ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = getButtons(canvas);
expect(buttons.length).toBeGreaterThan(0);
const primaryButton = getFirstButton(canvas);
expect(primaryButton).toBeTruthy();
};
const createLoadingStory = (
status: Story['args']['status'],
loadingType: 'cancelLoading' | 'closeLoading',
): Story => ({
args: {
status,
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
[loadingType]: true,
},
play: playLoadingState,
});
export const CancelLoadingState = createLoadingStory('REQUESTED', 'cancelLoading');
export const CloseLoadingState = createLoadingStory('ACCEPTED', 'closeLoading');
```
This eliminates duplicated `play` logic and makes it clear that the stories differ only by status + which loading flag is active.
---
These refactors keep all existing stories and assertions but make the file more declarative: shared behavior lives in a couple of helpers, and the stories mainly encode “what differs” (status, loading flag, expected callback behavior) instead of re-implementing small variants of the same flows.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...rethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the reservation-request cancel application service, consider using a domain-level API (e.g., a method on the aggregate or a domain enum) rather than directly assigning
reservationRequest.state = 'Cancelled'and comparing raw strings, so that state transitions stay aligned with domain invariants and types. - The new Storybook stories for reservation actions and listing information duplicate some of the generic button-count and no-button assertions already provided by
canvasUtils; you could simplify and centralize these stories by reusingcanvasUtils.assertNoButtons/assertHasButtonsinstead of custom helpers likeexpectNoButtonsandplayLoadingState.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the reservation-request cancel application service, consider using a domain-level API (e.g., a method on the aggregate or a domain enum) rather than directly assigning `reservationRequest.state = 'Cancelled'` and comparing raw strings, so that state transitions stay aligned with domain invariants and types.
- The new Storybook stories for reservation actions and listing information duplicate some of the generic button-count and no-button assertions already provided by `canvasUtils`; you could simplify and centralize these stories by reusing `canvasUtils.assertNoButtons`/`assertHasButtons` instead of custom helpers like `expectNoButtons` and `playLoadingState`.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/components/reservation-actions.tsx:11-26` </location>
<code_context>
+ loading?: boolean;
+}
+
+const CancelWithPopconfirm: React.FC<CancelWithPopconfirmProps> = ({
+ onConfirm,
+ loading,
+}) => (
+ <Popconfirm
+ title="Cancel Reservation Request"
+ description="Are you sure you want to cancel this request?"
+ onConfirm={onConfirm}
+ okText="Yes"
+ cancelText="No"
+ >
+ <span>
+ <ReservationActionButton action="Cancel" loading={loading} />
+ </span>
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guard against multiple cancel invocations while loading or debounce at the Popconfirm level.
Because `CancelWithPopconfirm` always renders an active `Popconfirm` and only applies `loading` to `ReservationActionButton`, users can still trigger multiple confirms (e.g., double-click) if `onConfirm` is async and `cancelLoading` is set later. Consider either guarding `onConfirm` to no-op while a cancel is in progress, or passing `loading` through to the Popconfirm OK button (e.g., `okButtonProps={{ loading }}`) to prevent retriggering while the mutation is in flight.
```suggestion
const CancelWithPopconfirm: React.FC<CancelWithPopconfirmProps> = ({
onConfirm,
loading,
}) => {
const handleConfirm = () => {
if (loading) {
return;
}
onConfirm?.();
};
return (
<Popconfirm
title="Cancel Reservation Request"
description="Are you sure you want to cancel this request?"
onConfirm={handleConfirm}
okText="Yes"
cancelText="No"
okButtonProps={{ loading }}
>
<span>
<ReservationActionButton action="Cancel" loading={loading} />
</span>
</Popconfirm>
);
};
```
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/test-utils/popconfirm-test-utils.ts:62-71` </location>
<code_context>
+ return cancelButton;
+};
+
+export const triggerPopconfirmAnd = async (
+ canvas: Canvas,
+ action: PopconfirmAction,
+ options?: {
+ triggerButtonIndex?: number;
+ expectedTitle?: string;
+ expectedDescription?: string;
+ },
+) => {
+ const {
+ triggerButtonIndex = 0,
+ expectedTitle,
+ expectedDescription,
+ } = options ?? {};
+
+ const buttons = canvas.getAllByRole('button');
+ const triggerButton = buttons[triggerButtonIndex];
+ expect(triggerButton).toBeTruthy();
+
+ if (!triggerButton) return;
+
+ await userEvent.click(triggerButton);
+ await waitForPopconfirm();
+
+ const { title, description, confirmButton, cancelButton } =
+ getPopconfirmElements();
+
+ if (expectedTitle) {
+ expect(title?.textContent).toContain(expectedTitle);
+ }
+ if (expectedDescription) {
+ expect(description?.textContent).toContain(expectedDescription);
+ }
+
+ const target = action === 'confirm' ? confirmButton : cancelButton;
+
+ if (target) {
+ await userEvent.click(target);
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Make `triggerPopconfirmAnd` more resilient by allowing selection of the trigger button by accessible name
This helper currently relies on `triggerButtonIndex` (defaulting to 0), which makes stories like `RequestedWithPopconfirm` and `RejectedWithCancel` depend on button order. To make these tests less brittle, consider allowing an optional `triggerButtonLabel` (or matcher) and using `canvas.getByRole('button', { name: /label/i })` when provided, falling back to `triggerButtonIndex` otherwise.
Suggested implementation:
```typescript
export const triggerPopconfirmAnd = async (
canvas: Canvas,
action: PopconfirmAction,
options?: {
/**
* Optional accessible name (or matcher) for the trigger button.
* When provided, this takes precedence over triggerButtonIndex.
*/
triggerButtonLabel?: string | RegExp;
/**
* Fallback to index-based selection when no label is provided.
*/
triggerButtonIndex?: number;
expectedTitle?: string;
expectedDescription?: string;
},
) => {
const {
triggerButtonLabel,
triggerButtonIndex = 0,
expectedTitle,
expectedDescription,
} = options ?? {};
let triggerButton: HTMLElement | undefined;
if (triggerButtonLabel) {
// Prefer accessible-name based lookup when a label matcher is provided
triggerButton = (await canvas.findByRole('button', {
name: triggerButtonLabel,
})) as HTMLElement;
} else {
// Fall back to index-based lookup to preserve existing behavior
const buttons = canvas.getAllByRole('button');
triggerButton = buttons[triggerButtonIndex] as HTMLElement | undefined;
}
expect(triggerButton).toBeTruthy();
if (!triggerButton) return;
await userEvent.click(triggerButton);
await waitForPopconfirm();
const { title, description, confirmButton, cancelButton } =
getPopconfirmElements();
if (expectedTitle) {
expect(title?.textContent).toContain(expectedTitle);
}
if (expectedDescription) {
expect(description?.textContent).toContain(expectedDescription);
}
const target = action === 'confirm' ? confirmButton : cancelButton;
if (target) {
await userEvent.click(target);
}
```
- If your project already uses a shared `Matcher` type from Testing Library (e.g. `import type { Matcher } from '@testing-library/dom'`), you can tighten the `triggerButtonLabel` type by replacing `string | RegExp` with `Matcher` to better align with existing conventions.
- Update any call sites of `triggerPopconfirmAnd` that would benefit from the more resilient behavior to pass `triggerButtonLabel` (e.g. `{ triggerButtonLabel: /request/i }`) instead of relying on `triggerButtonIndex`.
</issue_to_address>
### Comment 3
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:11` </location>
<code_context>
+
+type Canvas = ReturnType<typeof within>;
+
+const { getButtons, queryButtons, getFirstButton } = canvasUtils;
+
+const expectNoButtons = (canvas: Canvas) => {
</code_context>
<issue_to_address>
**issue (complexity):** Consider further unifying the story helpers (button queries, Popconfirm flows, and action-story factories) so tests share a single, consistent pattern and less duplicated logic.
You’ve already moved toward helpers (`canvasUtils`, `createNoActionsStory`, `createLoadingStory`), but there’s still a mix of patterns that keeps the mental model a bit fragmented. You can reduce complexity further by:
1. **Standardizing button queries**
`Requested`, `Accepted` still use `canvas.getAllByRole('button')` while other stories use `getButtons`. Centralizing the queries will shrink duplication and keep expectations consistent.
```ts
// shared helpers at top of file
const expectButtonsVisible = (canvas: Canvas, expectedCount?: number) => {
const buttons = getButtons(canvas);
expect(buttons.length).toBeGreaterThan(0);
if (expectedCount !== undefined) {
expect(buttons.length).toBe(expectedCount);
}
buttons.forEach(button => expect(button).toBeVisible());
};
```
Then in stories:
```ts
export const Requested: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expectButtonsVisible(canvas);
},
};
export const Accepted: Story = {
args: {
status: 'ACCEPTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expectButtonsVisible(canvas);
},
};
export const AcceptedActions: Story = {
args: {
status: 'ACCEPTED',
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expectButtonsVisible(canvas, 2);
},
};
```
This removes the raw `getAllByRole('button')` usage and keeps all button‑presence expectations going through one helper.
2. **Factoring a tiny Popconfirm DSL**
Your Popconfirm stories share the same `triggerPopconfirmAnd` call with slightly different expectations. Extracting a small helper keeps each story focused on “what” rather than “how”.
```ts
type PopconfirmExpectation = {
kind: 'confirm' | 'cancel';
expectedTitle?: string;
expectedDescription?: string;
assertCalled: (args: any) => void;
};
const runCancelPopconfirmFlow = async (
canvas: Canvas,
args: any,
{ kind, expectedTitle, expectedDescription, assertCalled }: PopconfirmExpectation,
) => {
await triggerPopconfirmAnd(canvas, kind, {
expectedTitle,
expectedDescription,
});
assertCalled(args);
};
```
Then stories become:
```ts
export const RequestedWithPopconfirm: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await runCancelPopconfirmFlow(canvas, args, {
kind: 'confirm',
expectedTitle: 'Cancel Reservation Request',
expectedDescription: 'Are you sure',
assertCalled: a => expect(a.onCancel).toHaveBeenCalled(),
});
},
};
export const PopconfirmCancelAction: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await runCancelPopconfirmFlow(canvas, args, {
kind: 'cancel',
assertCalled: a => expect(a.onCancel).not.toHaveBeenCalled(),
});
},
};
export const RejectedWithCancel: Story = {
args: {
status: 'REJECTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
const buttons = getButtons(canvas);
expect(buttons.length).toBe(1);
await runCancelPopconfirmFlow(canvas, args, {
kind: 'confirm',
expectedTitle: 'Cancel Reservation Request',
expectedDescription: 'Are you sure',
assertCalled: a => expect(a.onCancel).toHaveBeenCalled(),
});
},
};
```
3. **Reusing factories a bit more**
You already have `createNoActionsStory` and `createLoadingStory`. With the `expectButtonsVisible` helper, it’s straightforward to add an `createActionsStory` factory for the “standard presence” cases and cut down on per‑story play logic.
```ts
const createActionsStory = (
status: string,
args: Partial<Story['args']> = {},
expectedCount?: number,
): Story => ({
args: {
status,
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
...args,
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expectButtonsVisible(canvas, expectedCount);
},
});
export const RequestedActions: Story = createActionsStory('REQUESTED');
export const AcceptedActions: Story = createActionsStory('ACCEPTED', { onCancel: undefined }, 2);
```
These tweaks keep all the functionality you’ve added but remove the remaining mixed patterns and repeated button/Popconfirm assertions, making the file easier to scan and extend.
</issue_to_address>
### Comment 4
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.stories.tsx:222` </location>
<code_context>
- reservationPeriodStart: '1738368000000',
- reservationPeriodEnd: '1739145600000',
- },
+ userReservationRequest: createUserReservationRequest('Requested'),
},
play: async ({ canvasElement, args }) => {
</code_context>
<issue_to_address>
**issue (complexity):** Consider reducing duplication in the stories by unifying reservation request creation, tightening the popconfirm helpers, and centralizing shared cancel-flow logic.
You can keep the new coverage while trimming some of the complexity by consolidating patterns and tightening the helpers.
### 1. Normalize reservation request creation
Right now you have both inline objects (old stories) and `createUserReservationRequest`. You can make the factory a bit more flexible and migrate all stories to it, so there’s only one representation pattern.
```ts
const baseReservationRequest = {
__typename: 'ReservationRequest' as const,
id: 'res-1',
reservationPeriodStart: '1738368000000',
reservationPeriodEnd: '1739145600000',
};
type ReservationState = 'Requested' | 'Accepted';
const createUserReservationRequest = (
state: ReservationState,
overrides: Partial<typeof baseReservationRequest> = {},
) => ({
...baseReservationRequest,
state,
...overrides,
});
```
Then in stories (including older ones):
```ts
args: {
userReservationRequest: createUserReservationRequest('Requested', { id: 'res-2' }),
},
```
This removes the mixed “inline object vs factory” pattern and keeps all variation explicit in arguments.
---
### 2. Align `openCancelRequestPopconfirm` with actual usage
`openCancelRequestPopconfirm` is only used for its side effects and the returned button is never used. You can simplify it to better reflect intent:
```ts
const openCancelRequestPopconfirm = async (canvas: ReturnType<typeof within>) => {
const cancelButton = canvas.queryByRole('button', {
name: /Cancel Request/i,
});
expect(cancelButton).toBeTruthy();
if (cancelButton) {
await userEvent.click(cancelButton);
await waitForPopconfirm();
}
};
```
Existing calls stay the same:
```ts
await openCancelRequestPopconfirm(canvas);
await confirmPopconfirm();
```
This removes the “helper returns a value but isn’t really about the value” smell without changing behavior.
---
### 3. Extract a helper for popconfirm title
To keep stories focused on behavior and avoid reaching for `document.querySelector` directly, wrap the selector in a tiny helper:
```ts
const getPopconfirmTitleText = () =>
document.querySelector(POPCONFIRM_SELECTORS.title)?.textContent ?? '';
```
Usage in `CancelButtonWithPopconfirm`:
```ts
await openCancelRequestPopconfirm(canvas);
expect(getPopconfirmTitleText()).toContain('Cancel Reservation Request');
await confirmPopconfirm();
expect(args.onCancelClick).toHaveBeenCalled();
```
This keeps the selector details in one place and makes the story more readable.
---
### 4. Deduplicate the “cancel flow” stories
`ClickCancelButton`, `CancelButtonWithPopconfirm`, and `PopconfirmCancelButton` all drive the same flow (open cancel popconfirm, then confirm or cancel, and assert callback behavior). You can factor that into a small helper to keep each story’s `play` minimal while preserving separate stories:
```ts
type CancelFlowOptions = {
confirm: boolean;
expectCallback: boolean;
};
const runCancelFlow = async (
canvasElement: HTMLElement,
args: { onCancelClick?: jest.Mock },
{ confirm, expectCallback }: CancelFlowOptions,
) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await openCancelRequestPopconfirm(canvas);
if (confirm) {
await confirmPopconfirm();
} else {
await cancelPopconfirm();
}
if (expectCallback) {
expect(args.onCancelClick).toHaveBeenCalled();
} else {
expect(args.onCancelClick).not.toHaveBeenCalled();
}
};
```
Then the stories become:
```ts
export const ClickCancelButton: Story = {
args: {
onCancelClick: fn(),
userReservationRequest: createUserReservationRequest('Requested'),
},
play: async ({ canvasElement, args }) => {
await runCancelFlow(canvasElement, args, {
confirm: true,
expectCallback: true,
});
},
};
export const PopconfirmCancelButton: Story = {
args: {
userReservationRequest: createUserReservationRequest('Requested'),
onCancelClick: fn(),
},
play: async ({ canvasElement, args }) => {
await runCancelFlow(canvasElement, args, {
confirm: false,
expectCallback: false,
});
},
};
```
`CancelButtonWithPopconfirm` can either call `runCancelFlow` and then additionally assert the title via `getPopconfirmTitleText`, or you can keep just the title-specific assertion in that story and reuse `runCancelFlow` for the rest.
These changes keep all existing behaviors and stories, but reduce duplication and mixed patterns, making the file easier to scan and reason about.
</issue_to_address>
### Comment 5
<location> `packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/cancel.test.ts:47` </location>
<code_context>
+ error = undefined;
+ });
+
+ Scenario(
+ 'Successfully cancelling a requested reservation',
+ ({ Given, And, When, Then }) => {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared helpers for mocking transactions, executing the cancel command, and building reservation objects to keep the BDD tests readable while avoiding repeated setup code.
You can keep the BDD style and coverage while trimming a lot of duplication by extracting small helpers for the transaction mock and the cancel execution. This keeps all behavior but makes the scenarios much easier to scan and maintain.
### 1. Extract a `mockTransaction` helper
All scenarios re-implement `withScopedTransaction` with only `getById` and `save` differing. Centralize that:
```ts
// test-local helper
function mockTransaction({
dataSources,
reservationRequest,
getByIdReturn = reservationRequest,
saveReturn,
}: {
dataSources: DataSources;
reservationRequest?: unknown;
getByIdReturn?: unknown;
saveReturn?: unknown;
}) {
(
dataSources.domainDataSource as any
).ReservationRequest.ReservationRequest.ReservationRequestUnitOfWork
.withScopedTransaction.mockImplementation(
async (callback: (repo: any) => Promise<void>) => {
const mockRepo = {
getById: vi.fn().mockResolvedValue(getByIdReturn),
save: vi.fn().mockResolvedValue(saveReturn),
};
await callback(mockRepo);
},
);
}
```
Then each scenario just states its differences:
```ts
And('the reservation request exists and is in requested state', () => {
const mockReservationRequest = {
id: 'reservation-123',
state: 'Requested',
loadReserver: vi.fn().mockResolvedValue({ id: 'user-123' }),
};
mockTransaction({
dataSources: mockDataSources,
reservationRequest: mockReservationRequest,
saveReturn: { ...mockReservationRequest, state: 'Cancelled' },
});
});
And('the reservation request does not exist', () => {
mockTransaction({
dataSources: mockDataSources,
reservationRequest: undefined,
getByIdReturn: undefined,
saveReturn: undefined,
});
});
```
This removes the repeated long `withScopedTransaction.mockImplementation` blocks and focuses each scenario on *what* happens, not *how* it’s wired.
### 2. Extract a `runCancel` helper for `When` steps
All `When` blocks repeat the same try/catch on `cancel(mockDataSources)`:
```ts
async function runCancel(
dataSources: DataSources,
command: ReservationRequestCancelCommand,
) {
const cancelFn = cancel(dataSources);
try {
const result = await cancelFn(command);
return { result, error: undefined };
} catch (err) {
return { result: undefined, error: err };
}
}
```
Use it in `When` and reuse `result` / `error`:
```ts
When('the cancel command is executed', async () => {
const outcome = await runCancel(mockDataSources, command);
result = outcome.result;
error = outcome.error;
});
```
Now every scenario shares one `When` implementation, reducing boilerplate and the chance of divergence.
### 3. Reuse common `Given` shapes
Where the only difference is IDs and states, you can factor small helpers to keep the intent clear:
```ts
function buildReservation({
id,
state,
reserverId,
}: {
id: string;
state: 'Requested' | 'Rejected' | 'Accepted';
reserverId: string;
}) {
return {
id,
state,
loadReserver: vi.fn().mockResolvedValue({ id: reserverId }),
};
}
// Example usage in multiple scenarios
And('the reservation request exists and is in requested state', () => {
const mockReservationRequest = buildReservation({
id: command.id,
state: 'Requested',
reserverId: command.callerId,
});
mockTransaction({
dataSources: mockDataSources,
reservationRequest: mockReservationRequest,
saveReturn: { ...mockReservationRequest, state: 'Cancelled' },
});
});
```
This keeps the full BDD structure and all assertions intact but removes a lot of repeated setup code, making the file shorter and more readable without changing behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...rethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx
Outdated
Show resolved
Hide resolved
...ift/application-services/src/contexts/reservation-request/reservation-request/cancel.test.ts
Show resolved
Hide resolved
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
ListingInformation, the cancel Popconfirm currently only usescancelLoadingon the button; consider also passingokButtonProps={{ loading: cancelLoading }}(and/or disabling confirm while loading) so users can’t double-submit the cancel mutation while it’s in-flight. - In the reservation request
cancelapplication-service, you rely on mutatingreservationRequest.state = 'Cancelled'and an outer-scopedreservationRequestToReturn; ifwithScopedTransactionsupports returning a value, you could return the saved entity from the callback instead, which would avoid the extra mutable variable and make the control flow clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ListingInformation`, the cancel Popconfirm currently only uses `cancelLoading` on the button; consider also passing `okButtonProps={{ loading: cancelLoading }}` (and/or disabling confirm while loading) so users can’t double-submit the cancel mutation while it’s in-flight.
- In the reservation request `cancel` application-service, you rely on mutating `reservationRequest.state = 'Cancelled'` and an outer-scoped `reservationRequestToReturn`; if `withScopedTransaction` supports returning a value, you could return the saved entity from the callback instead, which would avoid the extra mutable variable and make the control flow clearer.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/test-utils/popconfirm-test-utils.ts:3-6` </location>
<code_context>
+import { expect, userEvent, waitFor, within } from 'storybook/test';
+
+const POPCONFIRM_SELECTORS = {
+ title: '.ant-popconfirm-title',
+ description: '.ant-popconfirm-description',
+ confirmButton: '.ant-popconfirm-buttons .ant-btn-primary',
+ cancelButton: '.ant-popconfirm-buttons .ant-btn:not(.ant-btn-primary)',
+} as const;
</code_context>
<issue_to_address>
**suggestion (testing):** Popconfirm test utilities rely on implementation-specific CSS selectors, which may make tests brittle
These helpers depend on Ant Design’s internal class names (e.g. `.ant-popconfirm-title`, `.ant-popconfirm-buttons .ant-btn-primary`), which are likely to change between versions. Prefer queries based on roles/names or stable `data-testid` attributes instead of CSS classes. If you decide to keep this, please add a brief TODO noting the trade-off and upgrade risk.
Suggested implementation:
```typescript
import { expect, userEvent, waitFor, within } from 'storybook/test';
// TODO: These selectors rely on Ant Design's internal CSS class names and may be brittle across library upgrades.
// Prefer refactoring to use testing-library queries based on roles, accessible names, or stable data-testid attributes.
const POPCONFIRM_SELECTORS = {
```
```typescript
title: '.ant-popconfirm-title',
description: '.ant-popconfirm-description',
confirmButton: '.ant-popconfirm-buttons .ant-btn-primary',
cancelButton: '.ant-popconfirm-buttons .ant-btn:not(.ant-btn-primary)',
} as const;
```
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:27` </location>
<code_context>
+ }
+};
+
+// Shared helper for popconfirm cancel flow
+type PopconfirmExpectation = {
+ kind: 'confirm' | 'cancel';
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the popconfirm flows, button expectations, and story factories so each story remains self-contained and readable without extra helper abstractions.
The abstractions around popconfirm, buttons, and factories add indirection without much reuse. You can keep all behaviours while simplifying the file by inlining the logic where it’s used.
### 1. Inline popconfirm expectations per story
`PopconfirmExpectation` + `runCancelPopconfirmFlow` hide the important details of each story. You can call `triggerPopconfirmAnd` directly and assert on `args` inline.
For example, replace:
```ts
type PopconfirmExpectation = {
kind: 'confirm' | 'cancel';
expectedTitle?: string;
expectedDescription?: string;
assertCalled: (args: Record<string, unknown>) => void;
};
const runCancelPopconfirmFlow = async (
canvas: Canvas,
args: Record<string, unknown>,
{
kind,
expectedTitle,
expectedDescription,
assertCalled,
}: PopconfirmExpectation,
) => {
await triggerPopconfirmAnd(canvas, kind, {
triggerButtonLabel: /cancel/i,
expectedTitle,
expectedDescription,
});
assertCalled(args);
};
export const RequestedWithPopconfirm: Story = {
// ...
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await runCancelPopconfirmFlow(canvas, args, {
kind: 'confirm',
expectedTitle: 'Cancel Reservation Request',
expectedDescription: 'Are you sure',
assertCalled: (a) => expect(a['onCancel']).toHaveBeenCalled(),
});
},
};
```
with something like:
```ts
export const RequestedWithPopconfirm: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await triggerPopconfirmAnd(canvas, 'confirm', {
triggerButtonLabel: /cancel/i,
expectedTitle: 'Cancel Reservation Request',
expectedDescription: 'Are you sure',
});
expect(args.onCancel).toHaveBeenCalled();
},
};
```
Similarly:
```ts
export const PopconfirmCancelAction: Story = {
// ...
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await triggerPopconfirmAnd(canvas, 'cancel', {
triggerButtonLabel: /cancel/i,
});
expect(args.onCancel).not.toHaveBeenCalled();
},
};
export const RejectedWithCancel: Story = {
// ...
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
assertButtonCount(canvas, 1);
await triggerPopconfirmAnd(canvas, 'confirm', {
triggerButtonLabel: /cancel/i,
expectedTitle: 'Cancel Reservation Request',
expectedDescription: 'Are you sure',
});
expect(args.onCancel).toHaveBeenCalled();
},
};
```
You keep all the behaviour but each story is self‑contained and readable without jumping to helpers.
### 2. Reduce button helper indirection
`expectButtonsVisible` + `canvasUtils.assertButtonCount` is a double abstraction over simple queries. For small stories, an inline query is easier to follow.
For instance, instead of:
```ts
const { getButtons, assertNoButtons, assertHasButtons, assertButtonCount } =
canvasUtils;
const expectButtonsVisible = (canvas: Canvas, expectedCount?: number) => {
const buttons = getButtons(canvas);
expect(buttons.length).toBeGreaterThan(0);
if (expectedCount !== undefined) {
expect(buttons.length).toBe(expectedCount);
}
for (const button of buttons) {
expect(button).toBeVisible();
}
};
export const Requested: Story = {
// ...
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expectButtonsVisible(canvas);
},
};
```
you can inline:
```ts
export const Requested: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = canvas.getAllByRole('button');
expect(buttons.length).toBeGreaterThan(0);
buttons.forEach((button) => expect(button).toBeVisible());
},
};
```
Likewise for `Accepted` / `AcceptedActions`:
```ts
export const AcceptedActions: Story = {
args: {
status: 'ACCEPTED',
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = canvas.getAllByRole('button');
expect(buttons.length).toBe(2);
buttons.forEach((button) => expect(button).toBeVisible());
},
};
```
And for the “no actions” cases you can replace `assertNoButtons` with a direct expectation:
```ts
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(() => canvas.getByRole('button')).toThrow();
},
```
### 3. Inline the story factories
`createNoActionsStory` and `createLoadingStory` introduce generics and dynamic props for a tiny number of stories. Explicit story objects are clearer and don’t require reading the factory.
Instead of:
```ts
const createNoActionsStory = (status: ReservationActionStatus): Story => ({
args: {
status,
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: playExpectNoButtons,
});
export const CancelledNoActions: Story = createNoActionsStory('CANCELLED');
export const ClosedNoActions: Story = createNoActionsStory('CLOSED');
const createLoadingStory = (
status: ReservationActionStatus,
loadingProp: 'cancelLoading' | 'closeLoading',
): Story => ({
args: {
status,
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
[loadingProp]: true,
},
play: playLoadingState,
});
export const CancelLoadingState: Story = createLoadingStory(
'REQUESTED',
'cancelLoading',
);
export const CloseLoadingState: Story = createLoadingStory(
'ACCEPTED',
'closeLoading',
);
```
make them explicit:
```ts
export const CancelledNoActions: Story = {
args: {
status: 'CANCELLED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(() => canvas.getByRole('button')).toThrow();
},
};
export const ClosedNoActions: Story = {
args: {
status: 'CLOSED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(() => canvas.getByRole('button')).toThrow();
},
};
export const CancelLoadingState: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
cancelLoading: true,
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = canvas.getAllByRole('button');
expect(buttons.length).toBeGreaterThan(0);
const loadingIndicators = canvasElement.querySelectorAll(
'.ant-btn-loading, [aria-busy="true"]',
);
expect(loadingIndicators.length).toBeGreaterThan(0);
},
};
export const CloseLoadingState: Story = {
args: {
status: 'ACCEPTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
closeLoading: true,
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = canvas.getAllByRole('button');
expect(buttons.length).toBeGreaterThan(0);
const loadingIndicators = canvasElement.querySelectorAll(
'.ant-btn-loading, [aria-busy="true"]',
);
expect(loadingIndicators.length).toBeGreaterThan(0);
},
};
```
This keeps all scenarios and assertions but makes each story self‑describing, with less indirection and type gymnastics.
</issue_to_address>
### Comment 3
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.stories.tsx:30` </location>
<code_context>
+ expectCallback: boolean;
+};
+
+const runCancelFlow = async (
+ canvasElement: HTMLElement,
+ args: Record<string, unknown>,
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the new cancel-flow and reservation helper logic in the relevant stories to avoid unnecessary indirection, casting, and unused generic options.
You can simplify the new helpers and remove some indirection without losing any functionality.
### 1. Inline `runCancelFlow` logic per story
`runCancelFlow` plus `Record<string, unknown>` casting and boolean flags adds indirection with very little reuse. Both usages are straightforward and can be expressed inline with better typing.
**Before:**
```ts
type CancelFlowOptions = {
confirm: boolean;
expectCallback: boolean;
};
const runCancelFlow = async (
canvasElement: HTMLElement,
args: Record<string, unknown>,
{ confirm, expectCallback }: CancelFlowOptions,
) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await triggerPopconfirmAnd(canvas, confirm ? 'confirm' : 'cancel', {
triggerButtonLabel: /Cancel Request/i,
});
if (expectCallback) {
expect(args['onCancelClick']).toHaveBeenCalled();
} else {
expect(args['onCancelClick']).not.toHaveBeenCalled();
}
};
export const ClickCancelButton: Story = {
args: {
onCancelClick: fn(),
userReservationRequest: createUserReservationRequest('Requested'),
},
play: async ({ canvasElement, args }) => {
await runCancelFlow(
canvasElement,
args as unknown as Record<string, unknown>,
{
confirm: true,
expectCallback: true,
},
);
},
};
export const PopconfirmCancelButton: Story = {
args: {
userReservationRequest: createUserReservationRequest('Requested'),
onCancelClick: fn(),
},
play: async ({ canvasElement, args }) => {
await runCancelFlow(
canvasElement,
args as unknown as Record<string, unknown>,
{
confirm: false,
expectCallback: false,
},
);
},
};
```
**After (same behaviour, less indirection, no casts):**
```ts
export const ClickCancelButton: Story = {
args: {
onCancelClick: fn(),
userReservationRequest: createUserReservationRequest('Requested'),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await triggerPopconfirmAnd(canvas, 'confirm', {
triggerButtonLabel: /Cancel Request/i,
});
expect(args.onCancelClick).toHaveBeenCalled();
},
};
export const PopconfirmCancelButton: Story = {
args: {
userReservationRequest: createUserReservationRequest('Requested'),
onCancelClick: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await triggerPopconfirmAnd(canvas, 'cancel', {
triggerButtonLabel: /Cancel Request/i,
});
expect(args.onCancelClick).not.toHaveBeenCalled();
},
};
```
Once both stories are inlined like this, you can safely remove `runCancelFlow` and `CancelFlowOptions` entirely, and every cancel-related story consistently uses `triggerPopconfirmAnd` directly.
### 2. Simplify or inline `createUserReservationRequest`
The `overrides` parameter isn’t currently used, and having to jump to a helper to see three fields can be more cognitive overhead than inlining.
If you expect more fields later but not overrides, you can at least drop the generic path:
**Simpler helper:**
```ts
const createUserReservationRequest = (state: ReservationState) => ({
...baseReservationRequest,
state,
});
```
If you don’t need the helper at all, consider inlining where it’s used; for example:
```ts
export const CancelButtonWithPopconfirm: Story = {
args: {
userReservationRequest: {
...baseReservationRequest,
state: 'Requested' as const,
},
onCancelClick: fn(),
cancelLoading: false,
},
// ...
};
```
Either approach keeps behaviour identical but reduces genericity and mental hops.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The Popconfirm configuration for cancellation (title/description/button texts/loading behavior) is duplicated between
ReservationActions(CancelWithPopconfirm) andListingInformation; consider reusing a shared component or helper so the UX and behavior stay consistent in one place. - The Storybook popconfirm helpers (
triggerPopconfirmAnd/clickCancelThenConfirm) rely on.ant-*class selectors and a 1s timeout, which can be brittle; if possible, prefer role/text-based queries and/or a slightly more robust wait strategy to reduce flakiness when Ant Design or timing changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Popconfirm configuration for cancellation (title/description/button texts/loading behavior) is duplicated between `ReservationActions` (`CancelWithPopconfirm`) and `ListingInformation`; consider reusing a shared component or helper so the UX and behavior stay consistent in one place.
- The Storybook popconfirm helpers (`triggerPopconfirmAnd`/`clickCancelThenConfirm`) rely on `.ant-*` class selectors and a 1s timeout, which can be brittle; if possible, prefer role/text-based queries and/or a slightly more robust wait strategy to reduce flakiness when Ant Design or timing changes.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.stories.tsx:181` </location>
<code_context>
},
};
+
+export const CancelReservationSuccess: Story = {
+ args: {
+ listing: mockListing,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new stories to share common Apollo mock builders, base args, and clearer scenario descriptions to reduce duplication and improve maintainability.
You can reduce the added complexity notably by factoring out the repeated Apollo mocks and some shared args. This keeps all behaviour intact but makes each story’s intent clearer and easier to maintain.
### 1. Extract reusable mock builders
You already added `buildBaseListingMocks`. Extend that pattern for cancel/create flows so each story just specifies the “difference” (success vs error vs loading).
```ts
const buildCancelReservationMocks = ({
id,
result,
error,
delay,
includeActiveReservationRefetch = false,
activeReservationResult = null,
}: {
id: string;
result?: { id: string; state: string };
error?: Error;
delay?: number;
includeActiveReservationRefetch?: boolean;
activeReservationResult?: unknown;
}) => {
const mocks = [
...buildBaseListingMocks(),
{
request: {
query: HomeListingInformationCancelReservationRequestDocument,
variables: { input: { id } },
},
...(result
? {
result: {
data: {
cancelReservation: {
__typename: 'ReservationRequest',
...result,
},
},
},
}
: {}),
...(error ? { error } : {}),
...(delay ? { delay } : {}),
},
];
if (includeActiveReservationRefetch) {
mocks.push({
request: { query: ViewListingActiveReservationRequestForListingDocument },
result: {
data: { myActiveReservationForListing: activeReservationResult },
},
});
}
return mocks;
};
```
Usage in stories:
```ts
export const CancelReservationSuccess: Story = {
args: {
// ...same args as today...
},
parameters: {
apolloClient: {
mocks: buildCancelReservationMocks({
id: 'res-cancel-1',
result: { id: 'res-cancel-1', state: 'Cancelled' },
includeActiveReservationRefetch: true,
activeReservationResult: null,
}),
},
},
// play unchanged
};
export const CancelReservationError: Story = {
args: {
// ...same args...
},
parameters: {
apolloClient: {
mocks: buildCancelReservationMocks({
id: 'res-cancel-error',
error: new Error('Only the reserver can cancel their reservation request'),
}),
},
},
};
export const CancelReservationLoading: Story = {
args: {
// ...same args...
},
parameters: {
apolloClient: {
mocks: buildCancelReservationMocks({
id: 'res-cancel-loading',
result: { id: 'res-cancel-loading', state: 'Cancelled' },
delay: 200,
}),
},
},
};
```
Similarly for create mutation:
```ts
const buildCreateReservationMocks = ({
listingId,
result,
error,
activeReservation,
}: {
listingId: string;
result?: { id: string };
error?: Error;
activeReservation?: {
id: string;
state: string;
reservationPeriodStart: string;
reservationPeriodEnd: string;
} | null;
}) => {
const mocks: any[] = [
...buildBaseListingMocks(),
{
request: {
query: HomeListingInformationCreateReservationRequestDocument,
variables: {
input: {
listingId,
reservationPeriodStart: expect.any(String),
reservationPeriodEnd: expect.any(String),
},
},
},
variableMatcher: () => true,
...(result
? {
result: {
data: {
createReservationRequest: {
__typename: 'ReservationRequest',
...result,
},
},
},
}
: {}),
...(error ? { error } : {}),
},
];
if (activeReservation !== undefined) {
mocks.push({
request: { query: ViewListingActiveReservationRequestForListingDocument },
result: {
data: {
myActiveReservationForListing: activeReservation
? { __typename: 'ReservationRequest', ...activeReservation }
: null,
},
},
});
}
return mocks;
};
```
Usage in stories:
```ts
export const CreateReservationSuccess: Story = {
args: {
// ...same args...
},
parameters: {
apolloClient: {
mocks: buildCreateReservationMocks({
listingId: '1',
result: { id: 'new-res-1' },
activeReservation: {
id: 'new-res-1',
state: 'Requested',
reservationPeriodStart: String(new Date('2025-03-01').getTime()),
reservationPeriodEnd: String(new Date('2025-03-10').getTime()),
},
}),
},
},
// play unchanged
};
export const CreateReservationError: Story = {
args: {
// ...same args...
},
parameters: {
apolloClient: {
mocks: buildCreateReservationMocks({
listingId: '1',
error: new Error('Failed to create reservation request'),
}),
},
},
};
```
This pattern can also be applied to `CreateReservationOnCompleted` / `CreateReservationOnError` with different `result` / `error` values.
### 2. Extract shared args to reduce story noise
The args for many stories are almost identical. Pull out a base to highlight the actual differences (id, reservation state, etc.):
```ts
const baseAuthedBorrowerArgs = {
listing: mockListing,
userIsSharer: false,
isAuthenticated: true,
};
const makeUserReservationRequest = (overrides: Partial<typeof baseReservation> = {}) => ({
__typename: 'ReservationRequest' as const,
state: 'Requested' as const,
reservationPeriodStart: '2025-02-01',
reservationPeriodEnd: '2025-02-10',
...overrides,
});
```
Usage:
```ts
export const CancelReservationNoId: Story = {
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: makeUserReservationRequest({ id: '' }),
},
// parameters and play unchanged
};
export const CancelReservationSuccess: Story = {
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: makeUserReservationRequest({ id: 'res-cancel-1' }),
},
// ...
};
```
### 3. Decouple comments from line numbers
The line-number‑based comments are brittle. You can keep the behavioural intent without coupling to specific lines:
```ts
// Exercise success path of handleReserveClick (successful reservation creation)
export const CreateReservationSuccess: Story = { /* ... */ };
// Exercise error path of handleReserveClick (mutation failure)
export const CreateReservationError: Story = { /* ... */ };
// Exercise early-return path when cancelling without a reservation id
export const CancelReservationNoId: Story = { /* ... */ };
```
These changes should significantly reduce duplication and perceived complexity while keeping all current scenarios and behaviours intact.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:33` </location>
<code_context>
export default meta;
type Story = StoryObj<typeof meta>;
export const Requested: Story = {
- args: {
- status: 'REQUESTED',
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared Storybook play-function logic into reusable helper functions to remove repetitive assertions and popconfirm handling across these stories.
You can keep all current coverage but cut down duplication and complexity with a couple of small abstractions.
### 1. Extract helpers for repeated DOM expectations
`Requested`, `Accepted`, `AcceptedActions` all do “buttons exist and are visible”; `CancelledNoActions` and `ClosedNoActions` both assert no buttons; `CancelLoadingState` and `CloseLoadingState` both check loading indicators. These can be expressed once and reused:
```ts
// stories/helpers.ts
import { within } from 'storybook/test';
export const expectButtonsVisible = (canvasElement: HTMLElement, expectedCount?: number) => {
const canvas = within(canvasElement);
const buttons = canvas.getAllByRole('button');
if (expectedCount !== undefined) {
expect(buttons.length).toBe(expectedCount);
} else {
expect(buttons.length).toBeGreaterThan(0);
}
for (const button of buttons) {
expect(button).toBeVisible();
}
};
export const expectNoButtons = (canvasElement: HTMLElement) => {
const canvas = within(canvasElement);
expect(canvas.queryAllByRole('button').length).toBe(0);
};
export const expectLoadingIndicator = (canvasElement: HTMLElement) => {
const loadingIndicators = canvasElement.querySelectorAll(
'.ant-btn-loading, [aria-busy="true"]',
);
expect(loadingIndicators.length).toBeGreaterThan(0);
};
```
Then your stories become simpler and more consistent:
```ts
export const Requested: Story = {
args: { status: 'REQUESTED', onCancel: fn(), onClose: fn(), onMessage: fn() },
play: ({ canvasElement }) => {
expectButtonsVisible(canvasElement);
},
};
export const Accepted: Story = {
args: { status: 'ACCEPTED', onCancel: fn(), onClose: fn(), onMessage: fn() },
play: ({ canvasElement }) => {
expectButtonsVisible(canvasElement);
},
};
export const AcceptedActions: Story = {
args: { status: 'ACCEPTED', onClose: fn(), onMessage: fn() },
play: ({ canvasElement }) => {
expectButtonsVisible(canvasElement, 2);
},
};
export const CancelledNoActions: Story = {
args: { status: 'CANCELLED', onCancel: fn(), onClose: fn(), onMessage: fn() },
play: ({ canvasElement }) => {
expectNoButtons(canvasElement);
},
};
export const ClosedNoActions: Story = {
args: { status: 'CLOSED', onCancel: fn(), onClose: fn(), onMessage: fn() },
play: ({ canvasElement }) => {
expectNoButtons(canvasElement);
},
};
export const CancelLoadingState: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
cancelLoading: true,
},
play: ({ canvasElement }) => {
expectButtonsVisible(canvasElement);
expectLoadingIndicator(canvasElement);
},
};
export const CloseLoadingState: Story = {
args: {
status: 'ACCEPTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
closeLoading: true,
},
play: ({ canvasElement }) => {
expectButtonsVisible(canvasElement);
expectLoadingIndicator(canvasElement);
},
};
```
This preserves every assertion you currently have while making the file easier to scan and maintain.
### 2. Factor a small helper for popconfirm stories
`RequestedWithPopconfirm`, `PopconfirmCancelAction`, `RejectedWithCancel`, and `CancelLoadingPreventsDoubleSubmit` all differ by status and expectation around `onCancel`. You can keep the separate stories but reduce duplication with a tiny factory:
```ts
// stories/helpers.ts
import { triggerPopconfirmAnd } from '../../../../../test-utils/popconfirm-test-utils.ts';
import type { RenderContext } from '@storybook/react';
type PopconfirmMode = 'confirm' | 'cancel';
export const playPopconfirm = async (
mode: PopconfirmMode,
ctx: RenderContext<any>,
{ expectCalled, withTextChecks }: { expectCalled: boolean; withTextChecks?: boolean },
) => {
const { canvasElement, args } = ctx;
const canvas = within(canvasElement);
await triggerPopconfirmAnd(canvas, mode, {
triggerButtonLabel: /cancel/i,
...(withTextChecks && {
expectedTitle: 'Cancel Reservation Request',
expectedDescription: 'Are you sure',
}),
});
if (expectCalled) {
expect(args.onCancel).toHaveBeenCalled();
} else {
expect(args.onCancel).not.toHaveBeenCalled();
}
};
```
Then reuse it in the individual stories:
```ts
export const RequestedWithPopconfirm: Story = {
args: { status: 'REQUESTED', onCancel: fn(), onMessage: fn() },
play: (ctx) => playPopconfirm('confirm', ctx, { expectCalled: true, withTextChecks: true }),
};
export const PopconfirmCancelAction: Story = {
args: { status: 'REQUESTED', onCancel: fn(), onMessage: fn() },
play: (ctx) => playPopconfirm('cancel', ctx, { expectCalled: false }),
};
export const RejectedWithCancel: Story = {
args: { status: 'REJECTED', onCancel: fn(), onClose: fn(), onMessage: fn() },
play: async (ctx) => {
const { canvasElement } = ctx;
const canvas = within(canvasElement);
expect(canvas.getAllByRole('button').length).toBe(1);
await playPopconfirm('confirm', ctx, { expectCalled: true, withTextChecks: true });
},
};
export const CancelLoadingPreventsDoubleSubmit: Story = {
args: { status: 'REQUESTED', onCancel: fn(), onMessage: fn(), cancelLoading: true },
play: (ctx) => playPopconfirm('confirm', ctx, { expectCalled: false }),
};
```
You keep each explicit story (useful for Storybook navigation and documentation), but the logic is centralized, which should address the “large matrix of granular scenarios with overlapping logic” feedback without removing any behaviour.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…nent - Extract reusable mock builders (buildCancelReservationMocks, buildCreateReservationMocks) to reduce duplication - Extract shared args (baseAuthedBorrowerArgs, makeUserReservationRequest) for story configurations - Update story comments to focus on behavior instead of line numbers - Create shared CancelReservationPopconfirm component in @sthrift/ui-components - Update ReservationActions and ListingInformation to use shared component - Ensures consistent UX and behavior for cancellation across the application
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The Popconfirm test helpers in
popconfirm-test-utils.tsrely on Ant Design CSS class selectors; consider switching to role/text-based queries (like buttons with specific labels) so tests are less tightly coupled to library implementation details. - The list of cancellable states in
cancel.tsis hard-coded (CANCELLABLE_STATES); if similar state rules exist elsewhere (e.g., in UI logic or domain rules), consider centralizing this configuration to avoid state divergence between layers. - In
ListingInformationContainer.handleCancelClick, errors are already surfaced via the mutationonErrorhandler, so the surrounding try/catch that only logs toconsole.erroris likely redundant and could be simplified to avoid duplicated error handling paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Popconfirm test helpers in `popconfirm-test-utils.ts` rely on Ant Design CSS class selectors; consider switching to role/text-based queries (like buttons with specific labels) so tests are less tightly coupled to library implementation details.
- The list of cancellable states in `cancel.ts` is hard-coded (`CANCELLABLE_STATES`); if similar state rules exist elsewhere (e.g., in UI logic or domain rules), consider centralizing this configuration to avoid state divergence between layers.
- In `ListingInformationContainer.handleCancelClick`, errors are already surfaced via the mutation `onError` handler, so the surrounding try/catch that only logs to `console.error` is likely redundant and could be simplified to avoid duplicated error handling paths.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:205-214` </location>
<code_context>
+ },
+};
+
+export const CancelLoadingState: Story = {
+ args: {
+ status: 'REQUESTED',
+ onCancel: fn(),
+ onClose: fn(),
+ onMessage: fn(),
+ cancelLoading: true,
+ },
+ play: ({ canvasElement }) => {
+ const canvas = within(canvasElement);
+ const buttons = canvas.getAllByRole('button');
+ expect(buttons.length).toBeGreaterThan(0);
+
+ // Ant Design loading buttons have aria-busy attribute or loading class
+ const loadingIndicators = canvasElement.querySelectorAll(
+ '.ant-btn-loading, [aria-busy="true"]',
+ );
</code_context>
<issue_to_address>
**suggestion:** Stories assert on Ant Design implementation details, which may be brittle across library updates.
These stories rely on `.ant-btn-loading` / `[aria-busy="true"]`, which tightly couples them to Ant Design’s DOM and class names. A small library change could break the stories without a real regression. Consider asserting on behavior instead (e.g., button disabled, visible loading indicator), or at least centralizing the selector in a helper so it’s easy to change in one place later.
Suggested implementation:
```typescript
};
const getLoadingIndicators = (root: HTMLElement) =>
root.querySelectorAll<HTMLElement>('.ant-btn-loading, [aria-busy="true"]');
export const CancelLoadingState: Story = {
```
```typescript
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
const buttons = canvas.getAllByRole('button');
expect(buttons.length).toBeGreaterThan(0);
// Centralized selector for loading indicators to avoid scattering Ant Design details
const loadingIndicators = getLoadingIndicators(canvasElement as HTMLElement);
expect(loadingIndicators.length).toBeGreaterThan(0);
},
};
```
If other stories or tests in this file (or nearby files) also rely on Ant Design-specific loading selectors, they should be updated to use the new `getLoadingIndicators` helper to keep all such coupling localized and easier to change in the future. You might also consider extending `getLoadingIndicators` later to assert more on behavior (e.g., checking `disabled` or `aria-disabled` on the parent button) without having to touch individual stories.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/test-utils/popconfirm-test-utils.ts:13-20` </location>
<code_context>
+type Canvas = ReturnType<typeof within>;
+type PopconfirmAction = 'confirm' | 'cancel';
+
+const waitForPopconfirm = async () =>
+ waitFor(
+ () => {
+ const title = document.querySelector(POPCONFIRM_SELECTORS.title);
+ if (!title) throw new Error('Popconfirm not found');
+ return title;
+ },
+ { timeout: 1000 },
+ );
+
</code_context>
<issue_to_address>
**suggestion (testing):** Revisit the short timeout in `waitForPopconfirm` to reduce flakiness
Using a 1000ms timeout here is likely to be brittle under heavier CI load and may cause intermittent test failures when the popconfirm renders slowly. Consider increasing the timeout (e.g., 2000–3000ms) or making it configurable to improve test stability without meaningfully impacting local runs.
Suggested implementation:
```typescript
type Canvas = ReturnType<typeof within>;
type PopconfirmAction = 'confirm' | 'cancel';
const waitForPopconfirm = async (timeoutMs = 3000) =>
waitFor(
() => {
const title = document.querySelector(POPCONFIRM_SELECTORS.title);
if (!title) throw new Error('Popconfirm not found');
return title;
},
{ timeout: timeoutMs },
);
```
If `waitForPopconfirm` is already being called elsewhere, you can keep existing call sites unchanged because the `timeoutMs` parameter is optional and defaults to 3000ms. If you have specific tests that you know may need more or less time, you can now pass a custom value, e.g. `await waitForPopconfirm(5000)` for a slower-flow test.
</issue_to_address>
### Comment 3
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.stories.tsx:68` </location>
<code_context>
+/**
+ * Build mocks for cancel reservation mutation with optional refetch
+ */
+const buildCancelReservationMocks = ({
+ id,
+ result,
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing scenario-focused helper functions and simpler factories around your generic mock builders so stories express clear success/error/loading states without directly managing all low-level options.
You can keep all functionality but reduce perceived complexity by hiding the generic “kitchen‑sink” builders behind focused scenario helpers and tightening the type/branch surface.
### 1. Wrap the generic cancel/create helpers in scenario‑specific functions
Right now stories call the highly parameterized builders directly:
```ts
mocks: buildCancelReservationMocks({
id: 'res-cancel-1',
result: { id: 'res-cancel-1', state: 'Cancelled' },
includeActiveReservationRefetch: true,
activeReservationResult: null,
});
```
Instead, keep `buildCancelReservationMocks` as an internal low‑level utility and export focused scenario helpers that encode the combinations you actually use:
```ts
// keep existing implementation but mark as internal
const buildCancelReservationMocks = (/* unchanged */) => { /* ... */ };
// scenario-focused helpers used in stories
const buildCancelSuccessMocks = (id: string) =>
buildCancelReservationMocks({
id,
result: { id, state: 'Cancelled' },
includeActiveReservationRefetch: true,
activeReservationResult: null,
});
const buildCancelErrorMocks = (id: string, message: string) =>
buildCancelReservationMocks({
id,
error: new Error(message),
});
const buildCancelLoadingMocks = (id: string, delay = 200) =>
buildCancelReservationMocks({
id,
result: { id, state: 'Cancelled' },
delay,
});
```
Then your stories become much more declarative:
```ts
parameters: {
apolloClient: {
mocks: buildCancelSuccessMocks('res-cancel-1'),
},
}
```
Same idea for create reservation:
```ts
const buildCreateReservationMocks = (/* unchanged */) => { /* ... */ };
const buildCreateReservationSuccessMocks = (listingId: string, reservationId: string) =>
buildCreateReservationMocks({
listingId,
result: { id: reservationId },
activeReservation: {
id: reservationId,
state: 'Requested',
reservationPeriodStart: String(new Date('2025-03-01').getTime()),
reservationPeriodEnd: String(new Date('2025-03-10').getTime()),
},
});
const buildCreateReservationErrorMocks = (listingId: string, message: string) =>
buildCreateReservationMocks({
listingId,
error: new Error(message),
});
```
Stories then read as “success/error” instead of “result + error + activeReservation + …”:
```ts
parameters: {
apolloClient: {
mocks: buildCreateReservationSuccessMocks('1', 'new-res-1'),
},
}
```
### 2. Replace `activeReservation` tri‑state with explicit helpers
Currently `activeReservation` in `buildCreateReservationMocks` supports three states: “omit refetch”, “refetch null”, and “refetch with reservation”. That’s powerful but hard to scan.
Instead of encoding that as `activeReservation?: { … } | null`, keep the low‑level API but expose explicit helpers that describe intent:
```ts
const buildCreateWithoutRefetchMocks = (listingId: string) =>
buildCreateReservationMocks({ listingId });
const buildCreateWithReservationRefetchMocks = (listingId: string, reservationId: string) =>
buildCreateReservationMocks({
listingId,
result: { id: reservationId },
activeReservation: {
id: reservationId,
state: 'Requested',
reservationPeriodStart: '2025-03-01',
reservationPeriodEnd: '2025-03-10',
},
});
const buildCreateWithEmptyRefetchMocks = (listingId: string) =>
buildCreateReservationMocks({
listingId,
result: { id: 'unused' },
activeReservation: null,
});
```
Stories then choose exactly one of these instead of manipulating `activeReservation` directly.
### 3. Tighten variable matching for create mutation
`variableMatcher: () => true` + `expect.any(String)` hides mismatches. You can still tolerate dynamic dates but constrain the rest of the shape:
```ts
const buildCreateReservationMocks = ({ listingId, /* ... */ }: { listingId: string; /* ... */ }) => {
const mocks: any[] = [
...buildBaseListingMocks(),
{
request: {
query: HomeListingInformationCreateReservationRequestDocument,
variables: {
input: {
listingId,
reservationPeriodStart: expect.any(String),
reservationPeriodEnd: expect.any(String),
},
},
},
// drop variableMatcher: we already allow dynamic dates via expect.any
...(result ? { result: { data: { /* unchanged */ } } } : {}),
...(error ? { error } : {}),
},
];
// ...
};
```
This keeps the dynamic‑date flexibility but makes it easier to debug when the listingId or shape is wrong.
### 4. Simplify reservation factories for common states
`makeUserReservationRequest` is flexible, but most stories only need “requested vs accepted vs cancelled” baselines. You can keep `makeUserReservationRequest` but add named presets that hide the literal union and overrides in most call sites:
```ts
const makeUserReservationRequest = (overrides: Partial<{
id: string;
state: 'Requested' | 'Accepted' | 'Rejected' | 'Cancelled';
reservationPeriodStart: string;
reservationPeriodEnd: string;
}> = {}) => ({
__typename: 'ReservationRequest' as const,
id: 'res-1',
state: 'Requested' as const,
reservationPeriodStart: '2025-02-01',
reservationPeriodEnd: '2025-02-10',
...overrides,
});
// scenario-friendly helpers
const requestedReservation = (id = 'res-1') =>
makeUserReservationRequest({ id, state: 'Requested' });
const acceptedReservation = (id = 'res-accepted-1') =>
makeUserReservationRequest({ id, state: 'Accepted' });
```
Then stories become:
```ts
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: requestedReservation('res-cancel-1'),
},
```
These changes keep all existing functionality but push most of the complexity into a small internal layer, while stories use simple, intention‑revealing helpers with minimal branching and parameters.
</issue_to_address>
### Comment 4
<location> `packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/cancel.test.ts:54` </location>
<code_context>
+ );
+}
+
+async function runCancel(
+ dataSources: DataSources,
+ command: ReservationRequestCancelCommand,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the test suite by inlining the `cancel` calls, sharing a single mock repository per scenario, and removing no-op or redundant helper steps to make each scenario’s behavior clearer and less indirect.
You can simplify these tests and reduce indirection without losing coverage.
**1. Remove `runCancel` and use standard `expect(...).rejects`**
Instead of wrapping `cancel` in `runCancel` with `{ result, error }`, call it directly and use vitest’s built‑in patterns:
```ts
// Before
When('the cancel command is executed', async () => {
const outcome = await runCancel(mockDataSources, command);
result = outcome.result;
error = outcome.error;
});
Then('an error "Reservation request not cancelled" should be thrown', () => {
expect(error).toBeDefined();
expect(error.message).toBe('Reservation request not cancelled');
});
// After
When('the cancel command is executed', async () => {
const cancelFn = cancel(mockDataSources);
// store the promise for assertions
result = cancelFn(command);
});
Then('an error "Reservation request not cancelled" should be thrown', async () => {
await expect(result).rejects.toThrow('Reservation request not cancelled');
});
```
For success cases:
```ts
When('the cancel command is executed', async () => {
const cancelFn = cancel(mockDataSources);
result = await cancelFn(command);
});
Then('the reservation request should be cancelled', () => {
expect(result.state).toBe('Cancelled');
});
```
With this, you can delete `runCancel` entirely and drop the `error` variable.
---
**2. Avoid remocking `withScopedTransaction` on every scenario**
You already set up `withScopedTransaction` in `BeforeEachScenario`. Instead of `mockTransaction` re‑mocking it and creating a new `mockRepo`, keep a reference to the repo and control `getById`/`save` per scenario:
```ts
let mockRepo: { getById: any; save: any };
BeforeEachScenario(() => {
mockRepo = {
getById: vi.fn(),
save: vi.fn(),
};
(mockDataSources.domainDataSource as any)
.ReservationRequest.ReservationRequest.ReservationRequestUnitOfWork
.withScopedTransaction = vi.fn(async (callback: any) => {
await callback(mockRepo);
});
command = { id: 'reservation-123', callerId: 'user-123' };
result = undefined;
});
```
Then configure behavior directly in each scenario, removing `mockTransaction`:
```ts
And('the reservation request exists and is in requested state', () => {
const mockReservationRequest = buildReservation({
id: command.id,
state: 'Requested',
reserverId: command.callerId,
});
mockRepo.getById.mockResolvedValue(mockReservationRequest);
mockRepo.save.mockResolvedValue({ ...mockReservationRequest, state: 'Cancelled' });
});
```
Similarly for error scenarios:
```ts
And('the reservation request does not exist', () => {
mockRepo.getById.mockResolvedValue(undefined);
mockRepo.save.mockResolvedValue(undefined);
});
```
After this, `mockTransaction` becomes unnecessary and can be removed.
---
**3. Drop Given steps that don’t configure behavior**
The step:
```ts
And('the reservation request exists', () => {
// Reservation request exists check
});
```
only adds noise. You can either:
- Remove it from the feature and test, or
- Move its behavior into the step (e.g. configure `mockRepo.getById`), so it’s not just a comment.
For example:
```ts
And('the reservation request exists', () => {
const mockReservationRequest = buildReservation({
id: command.id,
state: 'Requested',
reserverId: command.callerId,
});
mockRepo.getById.mockResolvedValue(mockReservationRequest);
});
```
(and keep the “save returns undefined” step responsible only for `mockRepo.save`).
---
These changes keep all functional behavior but reduce jumping through helpers, making each scenario’s setup and expectations visible in place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...rethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx
Show resolved
Hide resolved
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The popconfirm test utilities are currently duplicated between
apps/ui-sharethrift/src/test-utils/popconfirm-test-utils.tsandpackages/sthrift/ui-components/src/test-utils/popconfirm-test-utils.ts; consider consolidating to a single shared implementation (e.g., inui-components) and importing it where needed to avoid divergence. - In the
cancelapplication-service command, the state transition is implemented by directly mutatingreservationRequest.stateand hard-codingCANCELLABLE_STATES; if your domain model exposes behavior for state transitions, it might be clearer and safer to delegate cancellation and allowed-state logic to the domain entity instead of duplicating this logic in the application layer. - Several Storybook
playfunctions (e.g.,ReserveWithDatesSuccess,ReserveWithDatesError,CreateOnCompletedCallback) rely ondocument.querySelectorAllwith hard-coded indices into calendar cells (availableCells[10],availableCells[15]), which can be brittle against Ant Design or markup changes; consider selecting dates via semantics (e.g., specific text, roles, or relative positions) to make these interactions more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The popconfirm test utilities are currently duplicated between `apps/ui-sharethrift/src/test-utils/popconfirm-test-utils.ts` and `packages/sthrift/ui-components/src/test-utils/popconfirm-test-utils.ts`; consider consolidating to a single shared implementation (e.g., in `ui-components`) and importing it where needed to avoid divergence.
- In the `cancel` application-service command, the state transition is implemented by directly mutating `reservationRequest.state` and hard-coding `CANCELLABLE_STATES`; if your domain model exposes behavior for state transitions, it might be clearer and safer to delegate cancellation and allowed-state logic to the domain entity instead of duplicating this logic in the application layer.
- Several Storybook `play` functions (e.g., `ReserveWithDatesSuccess`, `ReserveWithDatesError`, `CreateOnCompletedCallback`) rely on `document.querySelectorAll` with hard-coded indices into calendar cells (`availableCells[10]`, `availableCells[15]`), which can be brittle against Ant Design or markup changes; consider selecting dates via semantics (e.g., specific text, roles, or relative positions) to make these interactions more robust.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.stories.tsx:560` </location>
<code_context>
+ * Exercise handleReserveClick with dates selected and successful mutation.
+ * This covers lines 104-123 (the full handleReserveClick flow).
+ */
+export const ReserveWithDatesSuccess: Story = {
+ args: {
+ ...baseAuthedBorrowerArgs,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the stories to extract shared date-picker/reserve interactions and reuse your existing reservation presets and mock builders to avoid repetition and keep each scenario focused on intent rather than wiring details.
You can reduce the added complexity without losing any behavior by extracting the repeated date‑picker / reserve flow and reusing your existing scenario helpers more consistently.
### 1. Extract shared date‑picker interaction logic
The `play` functions for `ReserveWithDatesSuccess`, `ReserveWithDatesError`, `CreateOnCompletedCallback`, and `CreateOnErrorCallback` all repeat the same sequence: wait for date inputs, open picker, select two cells, wait for Reserve enabled, click.
You can centralize this into a small helper and call it from each story:
```ts
// in this stories file or a shared test util
const selectReservationDateRangeAndReserve = async (
canvas: ReturnType<typeof within>,
) => {
// Wait for the date picker to be available
await waitFor(() => {
const dateInputs = canvas.queryAllByPlaceholderText(/date/i);
expect(dateInputs.length).toBeGreaterThan(0);
});
// Open date picker
const dateInputs = canvas.getAllByPlaceholderText(/date/i);
const startDateInput = dateInputs[0];
if (startDateInput) {
await userEvent.click(startDateInput);
}
// Wait for calendar to open
await waitFor(() => {
const calendarCells = document.querySelectorAll('.ant-picker-cell-inner');
expect(calendarCells.length).toBeGreaterThan(0);
});
// Select range
const availableCells = document.querySelectorAll(
'.ant-picker-cell:not(.ant-picker-cell-disabled) .ant-picker-cell-inner',
);
if (availableCells.length >= 2) {
const startCell = availableCells[10];
const endCell = availableCells[15];
if (startCell && endCell) {
await userEvent.click(startCell as HTMLElement);
await userEvent.click(endCell as HTMLElement);
}
}
// Wait for Reserve button to be enabled and click
await waitFor(
() => {
const reserveButton = canvas.queryByRole('button', { name: /reserve/i });
if (reserveButton && !reserveButton.hasAttribute('disabled')) {
return reserveButton;
}
throw new Error('Reserve button not enabled yet');
},
{ timeout: 3000 },
);
const reserveButton = canvas.getByRole('button', { name: /reserve/i });
await userEvent.click(reserveButton);
};
```
Then each story’s `play` becomes much smaller and the intent clearer:
```ts
export const ReserveWithDatesSuccess: Story = {
// ...args, parameters unchanged...
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await selectReservationDateRangeAndReserve(canvas);
},
};
export const ReserveWithDatesError: Story = {
// ...args, parameters unchanged...
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await selectReservationDateRangeAndReserve(canvas);
},
};
```
And similarly for `CreateOnCompletedCallback` / `CreateOnErrorCallback` where you need the same interaction.
If some stories only need “open date picker but don’t click Reserve”, you can split this into two helpers (`openAndSelectDateRange`, `clickReserveWhenEnabled`) and compose them in the stories that need both.
### 2. Reuse reservation presets consistently
You already introduced `requestedReservation` and `makeUserReservationRequest`, but some stories still inline variants that could use these factories, which would make the stories more declarative and consistent.
For example, instead of:
```ts
export const CancelReservationSuccess: Story = {
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: makeUserReservationRequest({ id: 'res-cancel-1' }),
},
// ...
};
```
you can lean on the preset:
```ts
export const CancelReservationSuccess: Story = {
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: requestedReservation('res-cancel-1'),
},
// ...
};
```
Same idea for other cancel stories (`CancelReservationError`, `CancelReservationLoading`, etc.) so your stories read more like “requested reservation with id X” instead of duplicating the object shape.
### 3. Thin wrappers around your mock builders
You already created `buildCancelSuccessMocks`, `buildCancelErrorMocks`, `buildCreateSuccessMocks`, and `buildCreateErrorMocks`. You can also use them in the earlier stories (e.g. `CreateReservationSuccess`, `CreateReservationError`) to avoid re-describing the same mock structures:
```ts
export const CreateReservationSuccess: Story = {
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: null,
},
parameters: {
apolloClient: {
mocks: buildCreateSuccessMocks('1', 'new-res-1'),
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
// if all you need is opening the date picker:
await waitFor(() => {
const dateInputs = canvas.queryAllByPlaceholderText(/date/i);
expect(dateInputs.length).toBeGreaterThan(0);
});
},
};
```
This keeps behavior identical but removes repeated mock configuration and makes each story more about the scenario name and less about wiring details.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/my-reservations/stories/reservation-actions.stories.tsx:108` </location>
<code_context>
+ },
+};
+
+export const RequestedWithPopconfirm: Story = {
+ args: {
+ status: 'REQUESTED',
</code_context>
<issue_to_address>
**issue (complexity):** Consider consolidating these Storybook stories by extracting shared play-logic helpers and grouping related scenarios (per status, popconfirm behavior, and loading states) into fewer stories.
You can keep all the coverage you’ve added while reducing story count and duplication by grouping related assertions into fewer stories and centralizing the “scenario variants” inside `play` helpers.
### 1. Group popconfirm confirm/cancel into one story
`RequestedWithPopconfirm` and `PopconfirmCancelAction` differ only by the interaction (“confirm” vs “cancel”) and the expectation on `onCancel`. You can merge them into a single story with a small helper:
```ts
// helper (in same file or a small test-utils file)
const assertCancelPopconfirm = async (
canvas: ReturnType<typeof within>,
args: { onCancel: jest.Mock },
) => {
// confirm path
await triggerPopconfirmAnd(canvas, 'confirm', {
triggerButtonLabel: /cancel/i,
expectedTitle: 'Cancel Reservation Request',
expectedDescription: 'Are you sure',
});
expect(args.onCancel).toHaveBeenCalledTimes(1);
// cancel path
await triggerPopconfirmAnd(canvas, 'cancel', {
triggerButtonLabel: /cancel/i,
});
expect(args.onCancel).toHaveBeenCalledTimes(1);
};
export const RequestedCancelPopconfirm: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);
await assertCancelPopconfirm(canvas, args as any);
},
};
```
This keeps both branches covered but removes one whole story and the duplicated wiring.
### 2. Consolidate loading stories and double-submit prevention
`CancelLoadingState`, `CloseLoadingState`, and `CancelLoadingPreventsDoubleSubmit` all assert variations of “loading visible” and “callbacks prevented”. You can keep the behavioral coverage in a single “loading” story per action type:
```ts
const assertLoadingAndNoDoubleSubmit = async ({
canvasElement,
args,
triggerLabel,
interaction = 'confirm',
}: {
canvasElement: HTMLElement;
args: { onCancel?: jest.Mock; onClose?: jest.Mock };
triggerLabel: RegExp;
interaction?: 'confirm' | 'cancel';
}) => {
const canvas = within(canvasElement);
// loading indicator visible
const loadingIndicators = getLoadingIndicators(canvasElement);
expect(loadingIndicators.length).toBeGreaterThan(0);
// popconfirm + callback prevention due to loading
await triggerPopconfirmAnd(canvas, interaction, {
triggerButtonLabel: triggerLabel,
});
if (args.onCancel) {
expect(args.onCancel).not.toHaveBeenCalled();
}
if (args.onClose) {
expect(args.onClose).not.toHaveBeenCalled();
}
};
export const CancelLoading: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onMessage: fn(),
cancelLoading: true,
},
play: async ({ canvasElement, args }) => {
await assertLoadingAndNoDoubleSubmit({
canvasElement,
args: args as any,
triggerLabel: /cancel/i,
});
},
};
export const CloseLoading: Story = {
args: {
status: 'ACCEPTED',
onClose: fn(),
onMessage: fn(),
closeLoading: true,
},
play: async ({ canvasElement, args }) => {
await assertLoadingAndNoDoubleSubmit({
canvasElement,
args: args as any,
triggerLabel: /close/i,
});
},
};
```
This maintains:
- loading indicators present for both `cancelLoading` and `closeLoading`,
- double-submit prevention for each action,
but removes one story and centralizes the logic.
### 3. Use one story per status for basic action presence
You currently have `Requested`, `Accepted`, `AcceptedActions`, `CancelledNoActions`, `ClosedNoActions` all asserting button count/visibility. You can keep a single story per status and move the assertions into small shared helpers to avoid duplication:
```ts
const expectButtonsVisible = (canvas: ReturnType<typeof within>, count?: number) => {
const buttons = canvas.queryAllByRole('button');
if (typeof count === 'number') {
expect(buttons.length).toBe(count);
} else {
expect(buttons.length).toBeGreaterThan(0);
}
for (const button of buttons) {
expect(button).toBeVisible();
}
};
export const Requested: Story = {
args: {
status: 'REQUESTED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expectButtonsVisible(canvas); // any >0
},
};
export const Accepted: Story = {
args: {
status: 'ACCEPTED',
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expectButtonsVisible(canvas, 2); // close + message
},
};
export const Cancelled: Story = {
args: {
status: 'CANCELLED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.queryAllByRole('button').length).toBe(0);
},
};
export const Closed: Story = {
args: {
status: 'CLOSED',
onCancel: fn(),
onClose: fn(),
onMessage: fn(),
},
play: ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.queryAllByRole('button').length).toBe(0);
},
};
```
This preserves the same behavioral checks while:
- reducing total stories,
- removing repeated “button count + visibility” logic,
- making it easier to see at a glance what each status is expected to render.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- There’s quite a bit of duplicated date-picker interaction logic in the new ListingInformation stories (waiting for inputs, opening the calendar, picking start/end cells, waiting for the Reserve button, etc.); consider extracting this into a shared helper similar to the new popconfirm test utilities so that future changes to the date UI only need to be updated in one place.
- In the ReservationRequest cancel application-service, you’re relying on setting
reservationRequest.state = 'Cancelled'and the entity’s setter to enforce business rules; if the domain model exposes an explicitcancel()method it might be clearer and safer to call that directly instead of mutatingstatewith a string literal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s quite a bit of duplicated date-picker interaction logic in the new ListingInformation stories (waiting for inputs, opening the calendar, picking start/end cells, waiting for the Reserve button, etc.); consider extracting this into a shared helper similar to the new popconfirm test utilities so that future changes to the date UI only need to be updated in one place.
- In the ReservationRequest cancel application-service, you’re relying on setting `reservationRequest.state = 'Cancelled'` and the entity’s setter to enforce business rules; if the domain model exposes an explicit `cancel()` method it might be clearer and safer to call that directly instead of mutating `state` with a string literal.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.stories.tsx:624` </location>
<code_context>
+ export const ReserveWithDatesSuccess: Story = {
</code_context>
<issue_to_address>
**suggestion:** The play functions rely on brittle DOM class selectors for the date picker, which may make the stories flaky.
In `ReserveWithDatesSuccess`, `ReserveWithDatesError`, `CreateOnCompletedCallback`, and `CreateOnErrorCallback`, the play functions use `document.querySelectorAll('.ant-picker-cell-inner')` and `.ant-picker-cell:not(.ant-picker-cell-disabled) .ant-picker-cell-inner`. These Ant Design internal class names are not stable API and may change on upgrade, making the stories flaky. Prefer Testing Library queries based on roles/labels (e.g., grid cells or accessible date labels) or drive interactions through the date picker’s input values to decouple the stories from UI-library internals.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.stories.tsx:68` </location>
<code_context>
+/**
+ * Build mocks for cancel reservation mutation with optional refetch
+ */
+const buildCancelReservationMocks = ({
+ id,
+ result,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new story helpers by inlining the scenario-specific mock builders and consolidating repeated date‑picker/reserve interactions into a single shared helper function.
You can keep all the scenarios you’ve added but significantly reduce complexity by (1) removing one layer of indirection in the mocks and (2) extracting the repeated date‑picker interactions into a single helper.
### 1. Remove the extra mock helper layer
Right now you have:
- A highly‑parameterized `buildCancelReservationMocks` / `buildCreateReservationMocks`
- Plus thin wrappers like `buildCancelSuccessMocks`, `buildCancelErrorMocks`, `buildCreateSuccessMocks`, `buildCreateErrorMocks`
This makes it hard to see what a story is doing, because behavior is spread across two helpers.
You can keep the generic builders (for flexibility) and delete the wrapper layer, inlining the call where you need the scenario. That keeps all functionality but flattens the mental model to “story → mocks builder” directly.
Example:
```ts
// Before
const buildCancelSuccessMocks = (id: string) =>
buildCancelReservationMocks({
id,
result: { id, state: 'Cancelled' },
includeActiveReservationRefetch: true,
activeReservationResult: null,
});
export const CancelOnCompletedCallback: Story = {
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: requestedReservation('res-completed-test'),
},
parameters: {
apolloClient: {
mocks: buildCancelSuccessMocks('res-completed-test'),
},
},
// ...
};
```
Inline the scenario directly into the story:
```ts
// After (same behavior, less indirection)
export const CancelOnCompletedCallback: Story = {
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: requestedReservation('res-completed-test'),
},
parameters: {
apolloClient: {
mocks: buildCancelReservationMocks({
id: 'res-completed-test',
result: { id: 'res-completed-test', state: 'Cancelled' },
includeActiveReservationRefetch: true,
activeReservationResult: null,
}),
},
},
// ...
};
```
You can do the same for the create stories:
```ts
// Before
const buildCreateErrorMocks = (listingId: string, message: string) =>
buildCreateReservationMocks({
listingId,
error: new Error(message),
});
export const CreateOnErrorCallback: Story = {
parameters: {
apolloClient: {
mocks: buildCreateErrorMocks('1', 'Database connection failed'),
},
},
// ...
};
```
```ts
// After
export const CreateOnErrorCallback: Story = {
parameters: {
apolloClient: {
mocks: buildCreateReservationMocks({
listingId: '1',
error: new Error('Database connection failed'),
}),
},
},
// ...
};
```
This removes one whole layer of helpers without changing any flows or mocks.
---
### 2. Extract reusable date‑selection + reserve click helper
Several stories (`ReserveWithDatesSuccess`, `ReserveWithDatesError`, `CreateOnCompletedCallback`, `CreateOnErrorCallback`, etc.) repeat almost the same `play` logic:
- Wait for date inputs
- Click start date input to open calendar
- Wait for calendar cells
- Pick two indices (`availableCells[10]`, `availableCells[15]`)
- Wait for Reserve button and click
That’s both brittle and hard to maintain. You can centralize this behavior into a single helper and reuse it across stories.
Example helper in the same stories file:
```ts
const selectAnyReservationRangeAndClickReserve = async (canvasElement: HTMLElement) => {
const canvas = within(canvasElement);
await waitFor(() => {
const dateInputs = canvas.queryAllByPlaceholderText(/date/i);
expect(dateInputs.length).toBeGreaterThan(0);
});
const [startDateInput] = canvas.getAllByPlaceholderText(/date/i);
if (startDateInput) {
await userEvent.click(startDateInput);
}
await waitFor(() => {
const calendarCells = document.querySelectorAll('.ant-picker-cell-inner');
expect(calendarCells.length).toBeGreaterThan(0);
});
const availableCells = document.querySelectorAll(
'.ant-picker-cell:not(.ant-picker-cell-disabled) .ant-picker-cell-inner',
);
if (availableCells.length >= 2) {
const startCell = availableCells[10];
const endCell = availableCells[15];
if (startCell && endCell) {
await userEvent.click(startCell as HTMLElement);
await userEvent.click(endCell as HTMLElement);
}
}
await waitFor(
() => {
const reserveButton = canvas.queryByRole('button', { name: /reserve/i });
if (reserveButton && !reserveButton.hasAttribute('disabled')) return reserveButton;
throw new Error('Reserve button not enabled yet');
},
{ timeout: 3000 },
);
const reserveButton = canvas.getByRole('button', { name: /reserve/i });
await userEvent.click(reserveButton);
};
```
Then your stories shrink to the scenario setup and a one‑liner in `play`:
```ts
export const ReserveWithDatesSuccess: Story = {
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: null,
},
parameters: {
apolloClient: {
mocks: buildCreateReservationMocks({
listingId: '1',
result: { id: 'new-res-with-dates' },
activeReservation: {
id: 'new-res-with-dates',
state: 'Requested',
reservationPeriodStart: String(new Date('2025-03-01').getTime()),
reservationPeriodEnd: String(new Date('2025-03-10').getTime()),
},
}),
},
},
play: async ({ canvasElement }) => {
await expect(canvasElement).toBeTruthy();
await selectAnyReservationRangeAndClickReserve(canvasElement);
},
};
```
And similarly:
```ts
export const CreateOnErrorCallback: Story = {
// args + parameters...
play: async ({ canvasElement }) => {
await expect(canvasElement).toBeTruthy();
await selectAnyReservationRangeAndClickReserve(canvasElement);
},
};
```
This keeps all the existing flows and interaction coverage, while:
- Making each story’s intent clearer (no more repeated low-level DOM querying)
- Localizing the brittle selectors (`.ant-picker-cell-inner`, indices `10` and `15`) to a single place
- Reducing the length and perceived complexity of the stories file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...s/home/components/view-listing/listing-information/listing-information.container.stories.tsx
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add end-to-end support for borrowers to cancel reservation requests from the listing and reservations views, including backend mutation, shared UI confirmation component, and extensive test coverage.
New Features:
Enhancements:
Build:
Tests: