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 GuideAdds end-to-end support for cancelling reservation requests, including a new application-service cancel command, GraphQL mutation and resolver, shared CancelReservationPopconfirm UI, updated listing and my-reservations flows to use confirmation and loading states, plus comprehensive Storybook and BDD tests. Sequence diagram for cancelling a reservation request via listing informationsequenceDiagram
actor User
participant ListingInformationContainer
participant CancelReservationPopconfirm
participant ApolloClient
participant GraphQLServer
participant ReservationRequestResolvers
participant ReservationRequestApplicationService
participant ReservationRequestUnitOfWork
participant ReservationRequestRepository
participant DomainReservationRequest
User->>ListingInformationContainer: click Cancel Request button
ListingInformationContainer->>CancelReservationPopconfirm: render with onConfirm handleCancelClick and loading cancelLoading
User->>CancelReservationPopconfirm: click trigger button
CancelReservationPopconfirm->>CancelReservationPopconfirm: show popconfirm
User->>CancelReservationPopconfirm: click Yes
CancelReservationPopconfirm->>CancelReservationPopconfirm: handleConfirm(loading)
alt loading is true
CancelReservationPopconfirm-->>ListingInformationContainer: do nothing
else loading is false
CancelReservationPopconfirm->>ListingInformationContainer: onConfirm()
ListingInformationContainer->>ListingInformationContainer: handleCancelClick
alt cancelLoading is true
ListingInformationContainer-->>CancelReservationPopconfirm: return early
else cancelLoading is false
ListingInformationContainer->>ListingInformationContainer: validate userReservationRequest.id
alt missing id
ListingInformationContainer->>User: message.error No reservation request to cancel
else id present
ListingInformationContainer->>ApolloClient: mutate HomeListingInformationCancelReservationRequest(id)
ApolloClient->>GraphQLServer: cancelReservation(input)
GraphQLServer->>ReservationRequestResolvers: Mutation.cancelReservation
ReservationRequestResolvers->>ReservationRequestResolvers: verify authenticated user
ReservationRequestResolvers->>ReservationRequestApplicationService: cancel({ id })
ReservationRequestApplicationService->>ReservationRequestUnitOfWork: withScopedTransaction(callback)
ReservationRequestUnitOfWork->>ReservationRequestRepository: getById(id)
ReservationRequestRepository-->>ReservationRequestUnitOfWork: DomainReservationRequest
ReservationRequestUnitOfWork->>DomainReservationRequest: set state = Cancelled
DomainReservationRequest-->>ReservationRequestUnitOfWork: updated entity
ReservationRequestUnitOfWork->>ReservationRequestRepository: save(entity)
ReservationRequestRepository-->>ReservationRequestUnitOfWork: saved entity reference
ReservationRequestUnitOfWork-->>ReservationRequestApplicationService: ReservationRequestEntityReference
ReservationRequestApplicationService-->>ReservationRequestResolvers: ReservationRequestEntityReference
ReservationRequestResolvers-->>ApolloClient: GraphQL payload cancelReservation
ApolloClient-->>ListingInformationContainer: onCompleted
ListingInformationContainer->>User: message.success Reservation request cancelled successfully
ListingInformationContainer->>ApolloClient: refetchQueries ViewListingActiveReservationRequestForListing
end
end
end
Class diagram for reservation cancellation application service and UI componentsclassDiagram
class ReservationRequestCancelCommand {
+string id
}
class ReservationRequestApplicationService {
+queryById(command ReservationRequestQueryByIdCommand) Promise ReservationRequestEntityReference
+queryActiveByReserverId(command ReservationRequestQueryActiveByReserverIdCommand) Promise ReservationRequestEntityReference[]
+queryPastByReserverId(command ReservationRequestQueryPastByReserverIdCommand) Promise ReservationRequestEntityReference[]
+queryActiveByReserverIdAndListingId(command ReservationRequestQueryActiveByReserverIdAndListingIdCommand) Promise ReservationRequestEntityReference
+queryOverlapByListingIdAndReservationPeriod(command ReservationRequestQueryOverlapByListingIdAndReservationPeriodCommand) Promise ReservationRequestEntityReference[]
+queryActiveByListingId(command ReservationRequestQueryActiveByListingIdCommand) Promise ReservationRequestEntityReference[]
+queryListingRequestsBySharerId(command ReservationRequestQueryListingRequestsBySharerIdCommand) Promise ReservationRequestEntityReference[]
+create(command ReservationRequestCreateCommand) Promise ReservationRequestEntityReference
+cancel(command ReservationRequestCancelCommand) Promise ReservationRequestEntityReference
}
class ReservationRequestApplicationServiceFactory {
+ReservationRequest(dataSources DataSources) ReservationRequestApplicationService
}
class CancelReservationPopconfirmProps {
+function onConfirm()
+boolean loading
+ReactNode children
}
class CancelReservationPopconfirm {
+CancelReservationPopconfirm(props CancelReservationPopconfirmProps)
-handleConfirm()
}
class ListingInformationProps {
+ItemListing listing
+boolean userIsSharer
+boolean isAuthenticated
+ReservationRequest userReservationRequest
+function onReserveClick()
+function onCancelClick()
+function onLoginClick()
+function onSignUpClick()
+object reservationDates
+function onReservationDatesChange(dates)
+boolean reservationLoading
+boolean cancelLoading
+boolean otherReservationsLoading
+Error otherReservationsError
+ReservationRequest[] otherReservations
}
class ListingInformation {
+ListingInformation(props ListingInformationProps)
}
class ListingInformationContainerProps {
+ItemListing listing
+boolean userIsSharer
+boolean isAuthenticated
+ReservationRequest userReservationRequest
+function onLoginClick()
+function onSignUpClick()
+string className
}
class ListingInformationContainer {
+ListingInformationContainer(props ListingInformationContainerProps)
-handleReserveClick()
-handleCancelClick()
}
class ReservationActionsProps {
+string status
+boolean cancelLoading
+boolean closeLoading
+function onCancel()
+function onClose()
+function onMessage()
}
class ReservationActions {
+ReservationActions(props ReservationActionsProps)
}
class ReservationActionButton {
+ReservationActionButton(action string, loading boolean)
}
ReservationRequestApplicationServiceFactory ..> ReservationRequestApplicationService : creates
ReservationRequestApplicationService ..> ReservationRequestCancelCommand : uses
ListingInformationContainer ..> ListingInformation : renders
ListingInformationContainer ..> CancelReservationPopconfirm : passes onCancelClick and cancelLoading via props
ListingInformation ..> CancelReservationPopconfirm : uses when reservationRequestStatus Requested
ReservationActions ..> CancelReservationPopconfirm : wraps ReservationActionButton for cancel
CancelReservationPopconfirm o--> CancelReservationPopconfirmProps
ReservationActions ..> ReservationActionButton : composes
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 |
…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
jasonmorais
reviewed
Jan 6, 2026
| } | ||
|
|
||
| const reserver = await reservationRequest.loadReserver(); | ||
| if (reserver.id !== command.callerId) { |
Contributor
There was a problem hiding this comment.
you do this check in the setter that updates the field in the aggregate not at the application service level
lian-jung
force-pushed
the
cancel-reservation-request
branch
from
8beaa2e to
fa3dcac
Compare
lian-jung
force-pushed
the
cancel-reservation-request
branch
from
19ef665 to
fdc12fa
Compare
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 new
ReservationRequestCancelCommandincludes acallerIdand the GraphQL resolver passes it down, butcancel.tsnever uses it when mutating the entity; either wire the caller identity into the domain permission check (instead of relying solely on the entity setter) or removecallerIdfrom the command to avoid a misleading API surface. - Several Storybook play functions (e.g. in
listing-information.container.stories.tsxand the popconfirm test utils) reach into the DOM using Ant Design CSS selectors like.ant-picker-cell-innerand.ant-btn-primary, which will be brittle to upstream UI changes; where possible, consider usinggetByRole/getByTextqueries or wrapping these selectors behind higher-level helpers as you did for popconfirms. - The new listing-information container stories add many very similar scenarios that differ only in mock setup and a small variation in play steps; consider extracting a smaller set of parametrized helpers or meta-factories to reduce duplication and make future changes to the reservation/cancellation flow easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `ReservationRequestCancelCommand` includes a `callerId` and the GraphQL resolver passes it down, but `cancel.ts` never uses it when mutating the entity; either wire the caller identity into the domain permission check (instead of relying solely on the entity setter) or remove `callerId` from the command to avoid a misleading API surface.
- Several Storybook play functions (e.g. in `listing-information.container.stories.tsx` and the popconfirm test utils) reach into the DOM using Ant Design CSS selectors like `.ant-picker-cell-inner` and `.ant-btn-primary`, which will be brittle to upstream UI changes; where possible, consider using `getByRole`/`getByText` queries or wrapping these selectors behind higher-level helpers as you did for popconfirms.
- The new listing-information container stories add many very similar scenarios that differ only in mock setup and a small variation in play steps; consider extracting a smaller set of parametrized helpers or meta-factories to reduce duplication and make future changes to the reservation/cancellation flow easier to maintain.
## Individual Comments
### Comment 1
<location> `packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/cancel.ts:9-16` </location>
<code_context>
+ callerId: string;
+}
+
+export const cancel = (dataSources: DataSources) => {
+ return async (
+ command: ReservationRequestCancelCommand,
+ ): Promise<Domain.Contexts.ReservationRequest.ReservationRequest.ReservationRequestEntityReference> => {
+ let reservationRequestToReturn:
+ | Domain.Contexts.ReservationRequest.ReservationRequest.ReservationRequestEntityReference
+ | undefined;
+ await dataSources.domainDataSource.ReservationRequest.ReservationRequest.ReservationRequestUnitOfWork.withScopedTransaction(
+ async (repo) => {
+ const reservationRequest = await repo.getById(command.id);
</code_context>
<issue_to_address>
**🚨 issue (security):** The `callerId` in `ReservationRequestCancelCommand` is never used, so authorization cannot be enforced here.
`cancel` ignores `callerId` and always sets `reservationRequest.state = 'Cancelled'`, so any caller reaching this service is effectively authorized. Given the intent that “only the reserver can cancel”, you should either validate `command.callerId` against the reserver (e.g., via the entity/repo or a domain method) before mutating state, or, if authorization is handled elsewhere, remove `callerId` from the command to avoid implying checks happen here.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.stories.tsx:911` </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 extracting shared helpers for the repeated date-selection/reserve flow and simplifying the layered mock builders so each story’s behavior is clearer and the file is less complex.
You’ve added good coverage, but a few pieces are now more complex than they need to be and can be simplified without losing behavior.
### 1. Repeated date‑selection + reserve logic in `play` functions
`ReserveWithDatesSuccess`, `ReserveWithDatesError`, `CreateOnCompletedCallback`, and `CreateOnErrorCallback` (and to a lesser extent `CreateReservationSuccess`) all contain very similar sequences:
- wait for date inputs
- click first input
- wait for calendar cells
- select two non‑disabled cells
- wait for reserve button to be enabled
- click reserve
You can pull this into a small helper to reduce nesting/boilerplate and make each story’s focus clearer.
For example, in this stories file (or a small local test-utils module):
```ts
const pickFirstAvailableRangeAndReserve = async (canvas: ReturnType<typeof within>) => {
// Wait for date inputs
await waitFor(() => {
const dateInputs = canvas.queryAllByPlaceholderText(/date/i);
expect(dateInputs.length).toBeGreaterThan(0);
});
// Open date picker
const [startDateInput] = canvas.getAllByPlaceholderText(/date/i);
if (startDateInput) {
await userEvent.click(startDateInput);
}
// Wait for calendar
await waitFor(() => {
const calendarCells = document.querySelectorAll('.ant-picker-cell-inner');
expect(calendarCells.length).toBeGreaterThan(0);
});
// Pick two available dates
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 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');
});
const reserveButton = canvas.getByRole('button', { name: /reserve/i });
await userEvent.click(reserveButton);
};
```
Then stories become much leaner and easier to scan:
```ts
export const ReserveWithDatesSuccess: Story = {
// ...args/parameters unchanged...
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await pickFirstAvailableRangeAndReserve(canvas);
},
};
export const CreateOnErrorCallback: Story = {
// ...args/parameters unchanged...
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await pickFirstAvailableRangeAndReserve(canvas);
},
};
```
You still exercise the same code paths, but the per‑story `play` now highlights what’s different (mocks/args), not the mechanics of date picking and clicking.
If you need variants that only pick dates but do *not* click reserve (e.g. `CreateReservationSuccess` vs others), you can split the helper:
```ts
const selectReservationRange = async (canvas: ReturnType<typeof within>) => {
// common date selection logic (no Reserve click)
};
const clickReserveWhenEnabled = async (canvas: ReturnType<typeof within>) => {
// wait for reserve and click
};
```
and compose them as needed.
### 2. Mock helpers are currently double‑abstracted
You now have:
- low‑level builders:
`buildCancelReservationMocks`, `buildCreateReservationMocks`
- thin scenario wrappers:
`buildCancelSuccessMocks`, `buildCancelErrorMocks`, `buildCreateSuccessMocks`, `buildCreateErrorMocks`
The wrappers only set 2–3 fields, so a story often has to mentally resolve two layers to understand what the mock does.
To reduce indirection, either:
**Option A – keep only the scenario helpers**
Inline the configuration they need and delete usages of the generic builders in stories.
```ts
const buildCancelSuccessMocks = (id: string) => [
...buildBaseListingMocks(),
{
request: {
query: HomeListingInformationCancelReservationRequestDocument,
variables: { input: { id } },
},
result: {
data: {
cancelReservation: {
__typename: 'ReservationRequest',
id,
state: 'Cancelled',
},
},
},
},
{
request: {
query: ViewListingActiveReservationRequestForListingDocument,
variables: { listingId: '1', reserverId: mockCurrentUser.id },
},
result: {
data: { myActiveReservationForListing: null },
},
},
];
```
Stories then only use `buildCancelSuccessMocks`/`buildCancelErrorMocks`/`buildCreateSuccessMocks`/`buildCreateErrorMocks`, and the “shape” of each mock is visible in one place.
**Option B – keep only the generic builders**
If you prefer one flexible builder, you can inline configuration in each story and drop the thin wrappers:
```ts
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,
}),
},
},
// play unchanged...
};
```
Either approach removes a layer of naming/indirection and makes it easier to see, from the story, exactly what each mock is doing.
---
Addressing these two points (shared date‑selection helper + simplifying mock helper layering) should noticeably reduce complexity in this file while preserving the coverage and behaviors you’ve added.
</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 5 issues, and left some high level feedback:
- The new Storybook stories for
ListingInformationContainerandListingInformationrepeat fairly complex date-picker interaction logic several times; consider extracting a shared helper (e.g.,selectReservationRange(canvas)) to reduce duplication and make future Ant Design date-picker changes easier to accommodate. - In
reservation-request.resolvers.test.tsthe "Cancel reservation when user not found" scenario name and step text talk about a missing user, but the resolver implementation only checks authentication and delegates permission failures to the domain layer; consider renaming the scenario/steps to align with the actual behaviour (permission/authorization failure) to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new Storybook stories for `ListingInformationContainer` and `ListingInformation` repeat fairly complex date-picker interaction logic several times; consider extracting a shared helper (e.g., `selectReservationRange(canvas)`) to reduce duplication and make future Ant Design date-picker changes easier to accommodate.
- In `reservation-request.resolvers.test.ts` the "Cancel reservation when user not found" scenario name and step text talk about a missing user, but the resolver implementation only checks authentication and delegates permission failures to the domain layer; consider renaming the scenario/steps to align with the actual behaviour (permission/authorization failure) to avoid confusion.
## Individual Comments
### Comment 1
<location> `packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/features/cancel.feature:37-41` </location>
<code_context>
+ When the cancel command is executed
+ Then an error "Cannot cancel reservation in current state" should be thrown
+
+ Scenario: Authorization failure when caller is not the reserver
+ Given a reservation request ID "reservation-789"
+ And the reservation request belongs to a different user
+ When the cancel command is executed
+ Then an error "Only the reserver can cancel their reservation request" should be thrown
</code_context>
<issue_to_address>
**issue (testing):** The expected error message in the feature text does not match the message asserted in the cancel tests.
In `cancel.feature` the last scenario expects an error containing `"Only the reserver can cancel their reservation request"`, but `cancel.test.ts` asserts `"You do not have permission to cancel this reservation request"`. Please standardize on one message so the feature file and tests stay in sync.
</issue_to_address>
### Comment 2
<location> `packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/cancel.ts:15-24` </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');
+ }
+
+ // State setter delegates to domain entity's private cancel() method
+ // which handles state validation and permission checks via visa
+ reservationRequest.state = 'Cancelled';
+ reservationRequestToReturn = await repo.save(reservationRequest);
+ },
+ );
+ if (!reservationRequestToReturn) {
+ throw new Error('Reservation request not cancelled');
+ }
</code_context>
<issue_to_address>
**suggestion:** Consider surfacing domain-specific errors instead of generic ones to make debugging and client handling clearer.
This function currently throws plain `Error('Reservation request not found')` and `Error('Reservation request not cancelled')`. Since this is at an application–service boundary, consider using structured errors (e.g., custom error subclasses or error codes / discriminated union) so callers like the GraphQL layer can reliably distinguish not-found, validation, and persistence issues instead of relying on generic messages.
Suggested implementation:
```typescript
export class ReservationRequestNotFoundError extends Error {
public readonly code = 'RESERVATION_REQUEST_NOT_FOUND';
constructor(public readonly reservationRequestId: string) {
super(`Reservation request not found: ${reservationRequestId}`);
this.name = 'ReservationRequestNotFoundError';
}
}
export class ReservationRequestCancellationError extends Error {
public readonly code = 'RESERVATION_REQUEST_CANCELLATION_FAILED';
constructor(public readonly reservationRequestId: string) {
super(`Reservation request not cancelled: ${reservationRequestId}`);
this.name = 'ReservationRequestCancellationError';
}
}
export const cancel = (dataSources: DataSources) => {
```
```typescript
const reservationRequest = await repo.getById(command.id);
if (!reservationRequest) {
throw new ReservationRequestNotFoundError(command.id);
}
```
```typescript
if (!reservationRequestToReturn) {
throw new ReservationRequestCancellationError(command.id);
}
```
` about wiring these into the GraphQL / error-handling layer (e.g., mapping `code` or `instanceof` to appropriate responses).
Here are the edits:
<file_operations>
<file_operation operation="edit" file_path="packages/sthrift/application-services/src/contexts/reservation-request/reservation-request/cancel.ts">
<<<<<<< SEARCH
export const cancel = (dataSources: DataSources) => {
=======
export class ReservationRequestNotFoundError extends Error {
public readonly code = 'RESERVATION_REQUEST_NOT_FOUND';
constructor(public readonly reservationRequestId: string) {
super(`Reservation request not found: ${reservationRequestId}`);
this.name = 'ReservationRequestNotFoundError';
}
}
export class ReservationRequestCancellationError extends Error {
public readonly code = 'RESERVATION_REQUEST_CANCELLATION_FAILED';
constructor(public readonly reservationRequestId: string) {
super(`Reservation request not cancelled: ${reservationRequestId}`);
this.name = 'ReservationRequestCancellationError';
}
}
export const cancel = (dataSources: DataSources) => {
>>>>>>> REPLACE
<<<<<<< SEARCH
const reservationRequest = await repo.getById(command.id);
if (!reservationRequest) {
throw new Error('Reservation request not found');
}
=======
const reservationRequest = await repo.getById(command.id);
if (!reservationRequest) {
throw new ReservationRequestNotFoundError(command.id);
}
>>>>>>> REPLACE
<<<<<<< SEARCH
if (!reservationRequestToReturn) {
throw new Error('Reservation request not cancelled');
}
=======
if (!reservationRequestToReturn) {
throw new ReservationRequestCancellationError(command.id);
}
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
1. If the project already has a shared error base class or error utilities (e.g., `DomainError`, `ApplicationError`, or a central error codes enum), adapt the new error classes to extend or use those instead of raw `Error`.
2. Update the GraphQL/error handling layer to:
- Check `instanceof ReservationRequestNotFoundError` and map to a not-found / 404-style response.
- Check `instanceof ReservationRequestCancellationError` and map to a conflict/validation-style response.
- Optionally surface the `code` field (`RESERVATION_REQUEST_NOT_FOUND`, `RESERVATION_REQUEST_CANCELLATION_FAILED`) in the GraphQL error extensions for clients to branch on.
3. If there are other application-services around reservation requests (e.g., create, update), consider aligning them to use the same structured error pattern for consistency.
</issue_to_address>
### Comment 3
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.stories.tsx:143-154` </location>
<code_context>
+ {
+ request: {
+ query: HomeListingInformationCreateReservationRequestDocument,
+ variables: {
+ input: {
+ listingId,
</code_context>
<issue_to_address>
**suggestion:** Using `expect.any(String)` inside the mock variables object is misleading because the `variableMatcher` overrides variable matching.
In `buildCreateReservationMocks`, `reservationPeriodStart: expect.any(String)` and `reservationPeriodEnd: expect.any(String)` sit alongside `variableMatcher: () => true`. Apollo doesn’t interpret Jest/Vitest matchers in plain objects, and the always-true matcher means the variables object is effectively unused. Consider replacing `expect.any` with simple placeholder values or removing the variables object entirely to avoid misleading future readers.
```suggestion
{
request: {
query: HomeListingInformationCreateReservationRequestDocument,
},
variableMatcher: () => true,
```
</issue_to_address>
### Comment 4
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.container.stories.tsx:766` </location>
<code_context>
+};
+
+// Exercise success path of handleReserveClick (successful reservation creation)
+export const CreateReservationSuccess: Story = {
+ args: {
+ ...baseAuthedBorrowerArgs,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated date-picker and Reserve-button interaction logic into reusable helper functions to simplify these stories and make their intent clearer.
You’ve already reduced some duplication with `build*Mocks` and `clickCancelThenConfirm`, but the new stories add quite a bit of repeated interaction logic around the date picker and Reserve button. That makes the file harder to scan and maintain.
You can keep all current behaviors and test coverage but centralize the interaction flow into a couple of helpers.
### 1. Extract a reusable helper for selecting a date range
`ReserveWithDatesSuccess`, `ReserveWithDatesError`, `CreateOnCompletedCallback`, and `CreateOnErrorCallback` (and similar) all manually:
- wait for date inputs
- open the picker
- wait for calendar cells
- select two enabled cells
That can be encapsulated:
```ts
// Near the other helpers
const openDatePickerAndSelectRange = async (
canvas: ReturnType<typeof within>,
{
inputIndex = 0,
startCellIndex = 10,
endCellIndex = 15,
}: { inputIndex?: number; startCellIndex?: number; endCellIndex?: number } = {},
) => {
// Wait for the date picker to be available
await waitFor(() => {
const dateInputs = canvas.queryAllByPlaceholderText(/date/i);
expect(dateInputs.length).toBeGreaterThan(0);
});
// Click on date picker to open it
const dateInputs = canvas.getAllByPlaceholderText(/date/i);
const startDateInput = dateInputs[inputIndex];
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 a future date range (non‑disabled cells)
const availableCells = document.querySelectorAll(
'.ant-picker-cell:not(.ant-picker-cell-disabled) .ant-picker-cell-inner',
);
if (availableCells.length > Math.max(startCellIndex, endCellIndex)) {
const startCell = availableCells[startCellIndex];
const endCell = availableCells[endCellIndex];
if (startCell && endCell) {
await userEvent.click(startCell as HTMLElement);
await userEvent.click(endCell as HTMLElement);
}
}
};
```
### 2. Extract a helper to “reserve with selected dates”
The “wait until Reserve enabled, then click” block is also repeated:
```ts
const clickReserveWhenEnabled = async (canvas: ReturnType<typeof within>) => {
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);
};
const reserveWithSelectedDateRange = async (canvasElement: HTMLElement) => {
const canvas = within(canvasElement);
await expect(canvasElement).toBeTruthy();
await openDatePickerAndSelectRange(canvas);
await clickReserveWhenEnabled(canvas);
};
```
### 3. Use the helpers in the stories
Now the `play` functions that currently duplicate this flow can become trivial one-liners:
```ts
export const ReserveWithDatesSuccess: Story = {
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: null,
},
parameters: {
apolloClient: {
mocks: buildCreateSuccessMocks('1', 'new-res-with-dates'),
},
},
play: async ({ canvasElement }) => {
await reserveWithSelectedDateRange(canvasElement);
},
};
export const ReserveWithDatesError: Story = {
args: {
...baseAuthedBorrowerArgs,
userReservationRequest: null,
},
parameters: {
apolloClient: {
mocks: buildCreateErrorMocks('1', 'Failed to create reservation request'),
},
},
play: async ({ canvasElement }) => {
await reserveWithSelectedDateRange(canvasElement);
},
};
export const CreateOnCompletedCallback: Story = {
// ...args/parameters stay the same
play: async ({ canvasElement }) => {
await reserveWithSelectedDateRange(canvasElement);
},
};
export const CreateOnErrorCallback: Story = {
// ...args/parameters stay the same
play: async ({ canvasElement }) => {
await reserveWithSelectedDateRange(canvasElement);
},
};
```
This keeps all your specific mocks and callbacks intact while:
- removing the copy‑pasted DOM/query logic,
- making each story’s intent clearer (the difference is in mocks/args, not the interaction script),
- and making future changes to the interaction flow (e.g., different date selection strategy) a single‑point edit.
</issue_to_address>
### Comment 5
<location> `packages/sthrift/ui-components/src/components/cancel-reservation-popconfirm/cancel-reservation-popconfirm.stories.tsx:48` </location>
<code_context>
+ * Tests clicking the popconfirm trigger and confirming.
+ * Covers lines: handleConfirm function, onConfirm callback invocation.
+ */
+export const ConfirmCancellation: Story = {
+ args: {
+ onConfirm: fn(),
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared play helpers and small test harnesses so stories only configure scenarios instead of re-implementing similar interaction logic repeatedly.
You can reduce duplication and keep all current behavior by extracting a small shared `play` helper and parameterizing the flows, and by separating “test-utils validation” from UX stories.
### 1. Extract a shared play helper for confirm/cancel/loading/no-callback variants
Most stories are doing:
- open popconfirm (`triggerPopconfirmAnd`)
- optionally assert `onConfirm` called / not called
- optionally pass different trigger options
You can centralize that:
```ts
// cancel-reservation-popconfirm.stories.tsx (top-level or near stories)
type FlowAction = 'confirm' | 'cancel';
type PlayOptions = {
action: FlowAction;
expectConfirmCalled?: boolean;
triggerOptions?: Parameters<typeof triggerPopconfirmAnd>[2];
};
const runPopconfirmFlow = async (
canvasElement: HTMLElement,
args: { onConfirm?: jest.Mock | ReturnType<typeof fn> },
{ action, expectConfirmCalled, triggerOptions }: PlayOptions,
) => {
const canvas = within(canvasElement);
await triggerPopconfirmAnd(canvas, action, {
triggerButtonLabel: /Cancel Reservation/i,
...triggerOptions,
});
if (typeof expectConfirmCalled === 'boolean' && args.onConfirm) {
if (expectConfirmCalled) {
expect(args.onConfirm).toHaveBeenCalled();
} else {
expect(args.onConfirm).not.toHaveBeenCalled();
}
}
};
```
Then your stories become much smaller:
```ts
export const ConfirmCancellation: Story = {
args: {
onConfirm: fn(),
loading: false,
children: <Button danger>Cancel Reservation</Button>,
},
play: async ({ canvasElement, args }) => {
await runPopconfirmFlow(canvasElement, args, {
action: 'confirm',
expectConfirmCalled: true,
triggerOptions: {
expectedTitle: 'Cancel Reservation Request',
expectedDescription: 'Are you sure you want to cancel this request?',
},
});
},
};
export const CancelPopconfirm: Story = {
args: {
onConfirm: fn(),
loading: false,
children: <Button danger>Cancel Reservation</Button>,
},
play: async ({ canvasElement, args }) => {
await runPopconfirmFlow(canvasElement, args, {
action: 'cancel',
expectConfirmCalled: false,
});
},
};
export const LoadingState: Story = {
args: {
onConfirm: fn(),
loading: true,
children: <Button danger>Cancel Reservation</Button>,
},
play: async ({ canvasElement, args }) => {
await runPopconfirmFlow(canvasElement, args, {
action: 'confirm',
expectConfirmCalled: false,
});
},
};
export const NoConfirmCallback: Story = {
args: {
loading: false,
children: <Button danger>Cancel Reservation</Button>,
},
play: async ({ canvasElement }) => {
await runPopconfirmFlow(canvasElement, { onConfirm: undefined }, {
action: 'confirm',
});
expect(within(canvasElement).getByRole('button')).toBeVisible();
},
};
```
This keeps the same flows but makes each story focused and easy to skim.
You can apply the same helper to `TestTriggerByIndex` and `TestTriggerWithoutOptions` by only changing `triggerOptions`:
```ts
export const TestTriggerByIndex: Story = {
args: {
onConfirm: fn(),
loading: false,
children: <Button danger>Cancel Reservation</Button>,
},
play: async ({ canvasElement, args }) => {
await runPopconfirmFlow(canvasElement, args, {
action: 'confirm',
expectConfirmCalled: true,
triggerOptions: { triggerButtonIndex: 0 },
});
},
};
```
### 2. Isolate test-utils–oriented behavior behind a tiny wrapper
The stories `TestGetLoadingIndicators`, `TestClickCancelThenConfirm`, `TestTriggerByIndex`, and `TestTriggerWithoutOptions` are really exercising `popconfirm-test-utils`. You can keep that behavior but reduce visual-noise by:
- Wrapping those in a small “test harness” helper so the stories themselves contain almost no logic.
- Optionally moving the bulk of assertions into unit tests near `popconfirm-test-utils.ts` later.
Example harness for test-utils-specific stories:
```ts
const runLoadingIndicatorTest = async (canvasElement: HTMLElement) => {
const canvas = within(canvasElement);
const { userEvent, waitFor } = await import('storybook/test');
const button = canvas.getByRole('button', { name: /Cancel Reservation/i });
await userEvent.click(button);
await waitFor(() => {
const loadingIndicators = getLoadingIndicators(document.body);
expect(loadingIndicators.length).toBeGreaterThan(0);
});
};
export const TestGetLoadingIndicators: Story = {
args: {
onConfirm: fn(),
loading: true,
children: <Button danger>Cancel Reservation</Button>,
},
play: ({ canvasElement }) => runLoadingIndicatorTest(canvasElement),
};
```
Likewise for `clickCancelThenConfirm`:
```ts
const runClickCancelThenConfirmTest = async (
canvasElement: HTMLElement,
args: { onConfirm: ReturnType<typeof fn> },
) => {
await clickCancelThenConfirm(canvasElement);
expect(args.onConfirm).toHaveBeenCalled();
};
export const TestClickCancelThenConfirm: Story = {
args: {
onConfirm: fn(),
loading: false,
children: <Button danger>Cancel Reservation</Button>,
},
play: ({ canvasElement, args }) => runClickCancelThenConfirmTest(canvasElement, args),
};
```
This keeps all current behavior and coverage, but collapses the per-story complexity down to a single helper call plus args, making the file much easier to 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.
...s/home/components/view-listing/listing-information/listing-information.container.stories.tsx
Show resolved
Hide resolved
jasonmorais
reviewed
Jan 20, 2026
Contributor
There was a problem hiding this comment.
For snyk, look at this pr for what was required in this file https://github.com/simnova/sharethrift/pull/338/files
anything else i believe was upgradable, my pr is good for vulnerabilities
| reservationDates, | ||
| onReservationDatesChange, | ||
| reservationLoading = false, | ||
| cancelLoading = false, |
Contributor
There was a problem hiding this comment.
Is there a reason to seperate loading between data and cancel, or could they be combined into a single value, do they need to be passed into this component or could that be handled by componentqueryloader? i would think about those two things depending on the usecase
Contributor
There was a problem hiding this comment.
How many places is this actually used? If its just 1-2/ maybe 3 places we dont want to extract logic out.
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 cancelling reservation requests from the UI through application services and GraphQL, and enhance reservation-related UI interactions and tests.
New Features:
Enhancements:
Tests:
Chores: