refactor: standardize import formatting and improve code readability in listing-information component #257
Conversation
…in listing-information component
Reviewer's GuideThis PR refactors the ListingInformation component by standardizing import formatting and string quoting, reformatting function and JSX indentation, and improving overall code readability without changing any business logic. Class diagram for ListingInformationProps interface (refactored formatting)classDiagram
class ListingInformationProps {
+ItemListing listing
+boolean userIsSharer
+boolean isAuthenticated
+ViewListingActiveReservationRequestForListingQuery["myActiveReservationForListing"] userReservationRequest
+function onReserveClick?
+function onLoginClick?
+function onSignUpClick?
+function onCancelClick?
+string className?
+object reservationDates? (startDate: Date|null, endDate: Date|null)
+function onReservationDatesChange?
+boolean reservationLoading?
+boolean otherReservationsLoading?
+Error otherReservationsError?
+ViewListingQueryActiveByListingIdQuery["queryActiveByListingId"] otherReservations?
}
class ItemListing
class ViewListingActiveReservationRequestForListingQuery
class ViewListingQueryActiveByListingIdQuery
ListingInformationProps --> ItemListing
ListingInformationProps --> ViewListingActiveReservationRequestForListingQuery
ListingInformationProps --> ViewListingQueryActiveByListingIdQuery
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Looks like there are leftover console.log calls for debugging; please remove them before merging.
- The lengthy IIFE blocks for rendering the reservation period and button could be extracted into smaller subcomponents to make the JSX more readable and maintainable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Looks like there are leftover console.log calls for debugging; please remove them before merging.
- The lengthy IIFE blocks for rendering the reservation period and button could be extracted into smaller subcomponents to make the JSX more readable and maintainable.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/components/view-listing/listing-information/listing-information.tsx:93-95` </location>
<code_context>
- 'user reservation request',
- userReservationRequest?.reservationPeriodEnd,
- );
+ console.log("is authenticated", isAuthenticated);
+ console.log(
+ "user reservation request",
+ userReservationRequest?.reservationPeriodEnd
+ );
</code_context>
<issue_to_address>
**suggestion:** Remove or replace console.log statements with a more robust logging solution.
Console.log statements in production may expose sensitive information and clutter logs. Please remove them or switch to a secure logging method.
Suggested implementation:
```typescript
import logger from 'utils/logger';
logger.info("is authenticated", isAuthenticated);
logger.info(
"user reservation request",
userReservationRequest?.reservationPeriodEnd
);
```
- Ensure you have a `logger` utility available at `utils/logger`. If not, you may need to create one or use a logging library such as `loglevel`, `winston`, or similar.
- If you do not want any logging in production, you can conditionally log based on environment variables or remove these lines entirely.
- Remove the import and usage if you decide not to log at all.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| console.log("is authenticated", isAuthenticated); | ||
| console.log( | ||
| "user reservation request", |
There was a problem hiding this comment.
suggestion: Remove or replace console.log statements with a more robust logging solution.
Console.log statements in production may expose sensitive information and clutter logs. Please remove them or switch to a secure logging method.
Suggested implementation:
import logger from 'utils/logger';
logger.info("is authenticated", isAuthenticated);
logger.info(
"user reservation request",
userReservationRequest?.reservationPeriodEnd
);- Ensure you have a
loggerutility available atutils/logger. If not, you may need to create one or use a logging library such asloglevel,winston, or similar. - If you do not want any logging in production, you can conditionally log based on environment variables or remove these lines entirely.
- Remove the import and usage if you decide not to log at all.
|
Perfect 👌 — here’s a ready-to-paste PR body template that combines both:
You can paste this entire block into your Pull Request description for #257. 🧭 Pull Request #257 – Implement “Accept Request / Reject Request” Functionality🔗 References
🧩 GoalSharers must be able to accept or reject incoming reservation requests. UI status → actions mapping
When a reservation is accepted, all other overlapping pending requests for the same listing must automatically become Rejected. 🏗️ Implementation Plan1. Domain Layer
|
| Status | Actions |
|---|---|
| Pending | Accept • Reject |
| Accepted | Close • Message |
| Rejected | Delete • Archive |
| Expired | Archive |
When a reservation is accepted, all other overlapping pending requests for the same listing must automatically become Rejected.
🏗️ Implementation Plan
1. Domain Layer packages/sthrift/domain/src/domain/contexts/reservation-request/
-
Extend
reservation-request.ts- Verify
accept()→REQUESTED → ACCEPTED - Add
reject()→REQUESTED → REJECTED - Emit
ReservationAccepted/ReservationRejectedevents - Preserve idempotency and permission checks via
ReservationRequestPassport
- Verify
2. Application Services packages/sthrift/application-services/.../reservation-request/
Create:
accept.ts
reject.ts
reject-overlapping.ts
Each should:
- Load reservation from repo
- Verify sharer authorization
- Prevent overlapping accepted reservations
- Persist via UoW transaction
- Emit domain events
- Trigger overlap rejection logic (
reject-overlapping.ts)
3. GraphQL Layer packages/sthrift/graphql/src/schema/types/reservation-request/
Add mutations:
extend type Mutation {
acceptReservationRequest(reservationRequestId: ID!): AcceptReservationRequestResult!
rejectReservationRequest(reservationRequestId: ID!): RejectReservationRequestResult!
}Resolvers must:
- Authenticate via
context.applicationServices.verifiedUser.verifiedJwt.email - Call application services
- Return
{ success, reservationRequest, errorMessage }
4. UI Layer apps/ui-sharethrift/src/pages/my-listings.tsx
- Display status → actions table as shown above
- Wire buttons to GraphQL mutations
- Add loading states + optimistic updates
- Auto-mark expired reservations (
end < today)
5. Testing
- Extend
reservation-request.aggregate.test.tsfor accept/reject and overlap scenarios - Add unit tests for
accept.tsandreject.ts - Verify GraphQL mutation responses
6. Quality and Instrumentation
- Enforce Biome lint + strict TS
- Add OpenTelemetry spans
- Use kebab-case file names, camelCase variables
- Document new services in domain README
✅ Acceptance Criteria
- Domain transitions work as intended
- Overlapping requests auto-rejected
- GraphQL mutations exposed and tested
- UI actions match status
- All unit and integration tests pass
🧠 Development Notes
- Run
pnpm run build --ws --if-presentthenpnpm test - Local dev uses Azurite storage emulator
- Use VS Code tasks
func: host startandnpm watch (functions)
🤖 Copilot Assignment Block
Paste this comment separately on the PR thread (not inside the PR body):
@copilot #196
#fetch https://github.com/orgs/simnova/projects/5/views/1?filterQuery=week%3A%22Week+2%22&pane=issue&itemId=133173606&issue=simnova%7Csharethrift%7C196
#fetch https://github.com/simnova/sharethrift/pull/257
@copilot /plan
Implement the “Accept Request” and “Reject Request” functionality (Issue #196) using DDD and CellixJS patterns:
- Domain updates for accept()/reject()
- New application services `accept.ts`, `reject.ts`, `reject-overlapping.ts`
- GraphQL mutations for accept/reject
- UI (My Listings → Requests) dynamic actions based on status
- Auto-reject overlapping requests
- Add unit and integration tests
- Follow Biome + strict TS + OpenTelemetry instrumentation|
@dani-vaibhav I've opened a new pull request, #258, to work on those changes. Once the pull request is ready, I'll request review from you. |
Summary by Sourcery
Standardize import formatting and code style in the ListingInformation component
Enhancements: