feat(phase2-slice1): booking decline + provider profile CRUD#5
Merged
Merged
Conversation
Phase 2 Slice 1 — Marketplace MVP kickoff: ## Booking Decline - Extend BookingStatus with 'declined' (submitted → declined transition) - DeclineSubmittedBooking input/result types in booking.repository domain - InMemoryBookingRepository.declineSubmittedBooking with idempotency guard - PostgresBookingRepository.declineSubmittedBooking (Postgres path parity) - BookingsService.declineBooking — provider-only, emits booking.declined event - POST /api/v1/bookings/:bookingId/decline endpoint - declineReason stored on BookingRecord ## Provider Profile - ProviderProfileRepository interface (upsert, getByProviderId, listPublic) - InMemoryProviderProfileRepository with partial-update merge semantics - ProvidersService.upsertProfile + getMyProfile (provider-only) - PUT /api/v1/providers/me/profile + GET /api/v1/providers/me/profile endpoints - Registered in ProvidersModule ## Domain Events - BookingDeclinedDomainEvent + worker envelope types in @quickwerk/domain - BookingDomainEventPublisher.publishBookingDeclined added to interface - LoggingBookingDomainEventPublisher + RelayBookingDomainEventPublisher updated - booking-declined.worker.ts with same retry/DLQ pattern as booking-accepted ## App Layer (@quickwerk/product-app) - bookingApiRoutes.decline + createDeclineBookingRequest in api-client - providerApiRoutes.myProfile + createUpsertProviderProfileRequest in api-client - declineBookingRequest action in product-app booking feature - ProviderProfileScreenState types + state factories in provider-state.ts - loadMyProviderProfile + saveMyProviderProfile actions in provider-screen-actions.ts ## Tests - 5 new decline tests in bookings.service.test.ts - InMemoryProviderProfileRepository tests (6 cases) - ProvidersService profile tests (7 cases) - booking-declined.worker.test.ts (7 cases) - booking-screen-actions-decline.test.ts (4 cases) - provider-screen-actions-profile.test.ts (8 cases) - provider-profile-state.test.ts (6 cases) - Fix: vitest.config.ts added to background-workers to exclude stale dist/ files ## Planning - .agent/plans/phase2-marketplace-mvp-kickoff-2026-03-28.md with full Phase 2 scope, milestones, technical approach, API changes, testing strategy, and risk log Test totals: platform-api 117/119, product-app 90/90, background-workers 11/11, admin-web 10/10 All typechecks pass: platform-api, product-app, admin-web, api-client, domain
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds provider profile CRUD and booking-decline support across client, API, domain, repositories, event publishing, and background worker plumbing, plus tests, API-client request builders, DB migration, and a Phase 2 plan document. Changes
Sequence Diagram(s)sequenceDiagram
actor Provider
participant Client as Client App
participant API as Platform API
participant Repo as Booking Repo
participant Publisher as Event Publisher
participant Worker as Background Worker
Provider->>Client: Trigger decline
Client->>API: POST /api/v1/bookings/{id}/decline
API->>API: Resolve session & auth (401/403 checks)
API->>Repo: declineSubmittedBooking(input)
Repo->>Repo: Validate transition, update status→'declined', persist declineReason
Repo-->>API: Return updated BookingRecord
API->>Publisher: publishBookingDeclined(event)
Publisher->>Worker: Relay/publish declined event (attempt loop)
Worker->>Worker: Build envelope, compute retry backoff or DLQ
API-->>Client: 200 { booking: { status: 'declined', declineReason } }
sequenceDiagram
actor Provider
participant Client as Client App
participant API as Platform API
participant Repo as Profile Repo
Provider->>Client: Submit profile (PUT)
Client->>API: PUT /api/v1/providers/me/profile
API->>API: Resolve session & validate displayName (401/400)
API->>Repo: upsertProfile(input with now)
Repo->>Repo: Normalize fields, preserve createdAt, set updatedAt
Repo-->>API: Return ProviderProfile
API-->>Client: 200 { profile }
Provider->>Client: Request profile (GET)
Client->>API: GET /api/v1/providers/me/profile
API->>Repo: getProfileByProviderId(...)
Repo-->>API: ProviderProfile | null
API-->>Client: 200 { profile | { status: 'not-set' } }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/platform-api/src/bookings/infrastructure/postgres-booking.repository.ts (1)
20-27:⚠️ Potential issue | 🟠 Major
BookingRowandloadBookingRecordalso need to includedecline_reason.To maintain parity with the in-memory implementation, the
BookingRowtype and the SELECT queries inloadBookingRecord/loadBookingByIdshould include thedecline_reasoncolumn, andmapBookingRecordshould map it todeclineReason.🔧 Proposed additions for reading declineReason
Add to
BookingRow:type BookingRow = { id: string; customer_user_id: string; provider_user_id: string | null; requested_service: string; status: BookingStatus; created_at: Date | string; + decline_reason: string | null; };Update SELECT in
loadBookingRecord(and similarly inloadBookingById):SELECT id::text, customer_user_id::text, provider_user_id::text, requested_service, status, - created_at + created_at, + decline_reason FROM bookingsUpdate
mapBookingRecord:return { bookingId: booking.id, createdAt: toIsoString(booking.created_at), customerUserId: booking.customer_user_id, providerUserId: booking.provider_user_id ?? undefined, requestedService: booking.requested_service, status: booking.status, + declineReason: booking.decline_reason ?? undefined, statusHistory, };Also applies to: 294-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/bookings/infrastructure/postgres-booking.repository.ts` around lines 20 - 27, The BookingRow type and the SQL read paths are missing the decline_reason field; update the BookingRow type to include decline_reason: string | null, modify the SELECT in loadBookingRecord and loadBookingById to include decline_reason, and update mapBookingRecord to map row.decline_reason to the domain property declineReason (preserving null handling/typing consistent with other fields).
🧹 Nitpick comments (6)
services/platform-api/src/orchestration/logging-domain-event.publisher.ts (1)
23-35: Consider extracting shared event-log payload mapping.
publishBookingAcceptedandpublishBookingDeclinednow have near-identical logging blocks; a tiny helper would reduce divergence risk as fields evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/orchestration/logging-domain-event.publisher.ts` around lines 23 - 35, Both publishBookingAccepted and publishBookingDeclined duplicate the same structured logging payload; extract a small helper (e.g., buildEventLogPayload or mapDomainEventToLog) that accepts the domain event (BookingAcceptedDomainEvent | BookingDeclinedDomainEvent) and returns the common breadcrumb payload (event, correlationId, status, details with eventName, eventId, replayed, bookingId). Replace the duplicated logStructuredBreadcrumb calls in publishBookingAccepted and publishBookingDeclined to call this helper so future field changes are made in one place.apps/product-app/src/features/booking/booking-state.ts (1)
4-4: Tightenstatustype to preserve narrowing.On Line 4,
| stringmakes the union effectively juststring, which weakens compile-time guarantees for decline/accept handling.🔧 Proposed fix
- status: 'submitted' | 'accepted' | 'declined' | string; + status: 'submitted' | 'accepted' | 'declined';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/product-app/src/features/booking/booking-state.ts` at line 4, The status field's union currently includes "| string" which collapses type narrowing; update the status declaration in booking-state.ts (the status property on the Booking/booking state type) to remove "| string" and use only the explicit literals "submitted" | "accepted" | "declined" (or add any specific allowed literals you actually support) so TypeScript can narrow checks for accepted/declined branches correctly.apps/product-app/src/features/provider/provider-profile-state.test.ts (1)
39-48: Add aserviceAreamapping assertion in the loaded-state test.Line 40 uses a profile with
serviceArea, but the loaded-state expectations never verify it reaches the form. This leaves a quiet regression gap.Suggested test addition
if (state.status === 'loaded') { expect(state.profile.displayName).toBe('Max Muster'); expect(state.form.displayName).toBe('Max Muster'); expect(state.form.bio).toBe('Expert plumber'); expect(state.form.tradeCategories).toEqual(['plumbing']); + expect(state.form.serviceArea).toBe('Vienna'); expect(state.form.isPublic).toBe(true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/product-app/src/features/provider/provider-profile-state.test.ts` around lines 39 - 48, The test for createLoadedProfileState omits verifying that profile.serviceArea is mapped into the form; update the test for createLoadedProfileState (using sampleProfile and the returned state) to assert that when state.status === 'loaded' the profile.serviceArea equals the form.serviceArea (e.g., expect(state.form.serviceArea).toEqual(state.profile.serviceArea or the expected array/value from sampleProfile)); keep this check alongside the existing displayName/bio/tradeCategories/isPublic assertions to prevent regressions in the serviceArea mapping.services/platform-api/src/bookings/bookings.service.test.ts (1)
367-385: Strengthen replay idempotency assertion with history length.In Line 377 replay flow, also assert that
statusHistorydoes not grow (e.g., remains length2) to catch accidental duplicate decline transitions.Suggested assertion
expect(replay.ok).toBe(true); if (replay.ok) { expect(replay.booking.status).toBe('declined'); + expect(replay.booking.statusHistory).toHaveLength(2); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/bookings/bookings.service.test.ts` around lines 367 - 385, Add an assertion in the declineBooking replay test to ensure the booking's statusHistory does not grow on replay: after calling service.declineBooking(provider, created.booking.bookingId, {}) and inspecting replay.booking.status, also assert that replay.booking.statusHistory.length remains the expected length (e.g., 2) to ensure idempotent transitions; locate this in the 'declineBooking — is idempotent for the same provider' test where service.declineBooking and replay are used.services/platform-api/src/bookings/bookings.service.ts (1)
332-345: Consider includingdeclineReasonin the domain event payload.The
BookingDeclinedDomainEventdoesn't includedeclineReason, which could be useful for notification workers to compose messages like "Provider declined: [reason]". If notification workers need this, they'd have to fetch the booking separately.💡 Optional enhancement to include declineReason in event
const declinedEvent: BookingDeclinedDomainEvent = { eventName: 'booking.declined', eventId: randomUUID(), occurredAt: new Date().toISOString(), correlationId, replayed: declined.replayed, booking: { bookingId: declined.booking.bookingId, customerUserId: declined.booking.customerUserId, providerUserId: declined.booking.providerUserId ?? session.userId, requestedService: declined.booking.requestedService, status: 'declined', + declineReason: declined.booking.declineReason, }, };This would require updating
BookingDeclinedDomainEventinpackages/domainas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/bookings/bookings.service.ts` around lines 332 - 345, The booking.declined domain event currently omits the declineReason; update the declinedEvent payload built in bookings.service.ts (variable declinedEvent) to include declineReason (e.g., booking.declineReason or declined.declineReason) under booking so notification workers can use it, and bump the BookingDeclinedDomainEvent type in the domain package to include declineReason on the booking shape; ensure serialization uses the same string (or nullable) type as the source booking to avoid type errors.services/platform-api/src/providers/providers.service.ts (1)
311-314: LogdisplayNamevalidation failures for observability parity.The 400 branch returns early without a breadcrumb, which makes bad-request tracing harder than other failure paths.
Proposed refinement
const displayName = input.displayName?.trim(); if (!displayName) { + logStructuredBreadcrumb({ + event: 'provider.profile.upsert', + correlationId, + status: 'failed', + details: { reason: 'invalid-display-name', actorUserId: session.userId }, + }); return { ok: false, statusCode: 400, error: 'displayName is required.' }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/providers/providers.service.ts` around lines 311 - 314, The early return when displayName is missing should record a breadcrumb for observability before returning; in the block that computes const displayName = input.displayName?.trim() and returns { ok: false, statusCode: 400, error: 'displayName is required.' }, add a breadcrumb/log entry using the module's tracing/logger (the same logger/tracing used elsewhere in providers.service.ts) that records the validation failure, the incoming input.displayName value (or that it was undefined/empty), and context like method name so bad-request traces match other failure paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/product-app/src/features/provider/provider-screen-actions.ts`:
- Around line 184-196: The profile builder in provider-screen-actions (the
object with keys profile -> providerUserId, displayName, bio, tradeCategories,
serviceArea, isPublic, createdAt, updatedAt) currently coerces missing
createdAt/updatedAt to empty strings which accepts malformed payloads; change
parsing so createdAt and updatedAt are required and the parser fails when they
are absent or not valid strings (do not default to ''), and refactor to reuse a
single parser/validator for both load and save paths (e.g., extract a
validateProviderProfile function or schema used by the code that constructs the
profile and the code at the 229-241 block) so both paths enforce the same
required timestamp rules.
In `@apps/product-app/src/features/provider/provider-state.ts`:
- Around line 47-61: createNotSetProfileState and createLoadedProfileState are
returning form objects/arrays by reference, risking shared mutable state; update
both factories to return fresh copies instead of passing references: in
createNotSetProfileState clone initialProviderProfileFormState into a new object
for the form, and in createLoadedProfileState build a new form object by copying
primitive fields and shallow-cloning arrays (e.g., tradeCategories) and any
nested objects so no reference to profile or initial objects is retained. Ensure
the returned ProviderProfileScreenState contains these new cloned structures
rather than original references.
In `@packages/api-client/src/index.ts`:
- Around line 162-168: The client-side type UpsertProviderProfileBody currently
requires displayName which conflicts with the server's partial-update contract;
update the UpsertProviderProfileBody type in packages/api-client/src/index.ts to
make displayName optional (displayName?: string) to match the API controller,
and scan for and update any callers/tests that assumed the field was required so
partial-profile PATCH/PUT requests compile and behave consistently.
In `@packages/domain/src/index.ts`:
- Around line 58-71: The domain introduces BookingDeclinedDomainEvent with
booking.status 'declined' but the canonical bookingStatuses constant/union omits
'declined'; update the bookingStatuses definition to include 'declined' so the
canonical source of truth matches BookingDeclinedDomainEvent and any derived
unions/validation (locate bookingStatuses and the BookingDeclinedDomainEvent
type in packages/domain/src/index.ts and add 'declined' to the statuses list).
In `@services/platform-api/src/bookings/bookings.controller.ts`:
- Around line 200-202: The controller currently dereferences body.declineReason
directly when calling bookingsService.declineBooking, which can throw if body is
null/undefined; first read declineReason safely (e.g. const declineReason =
body?.declineReason) and validate it before calling declineBooking,
returning/throwing a controlled HTTP 400 (BadRequest) if the payload is missing
or invalid; then call this.bookingsService.declineBooking(session, bookingId, {
declineReason }, { correlationId }) so the service always receives a
well-defined value.
In `@services/platform-api/src/bookings/domain/booking.repository.ts`:
- Line 19: BookingRecord now has a new field declineReason but the Postgres
mapper mapBookingRecord in
services/platform-api/src/bookings/infrastructure/postgres-booking.repository.ts
does not map it, causing loss of data on reads; update mapBookingRecord to read
the decline_reason (or appropriate DB column) from the DB row and assign it to
BookingRecord.declineReason, and also ensure any reverse mapper (e.g., when
building DB params for insert/update) sets the decline_reason column from the
Booking entity's declineReason so create/update and read mappers are in parity.
In
`@services/platform-api/src/bookings/infrastructure/postgres-booking.repository.ts`:
- Around line 191-197: The code references a 'declined' status and persists a
declineReason but the DB schema lacks that support; add a migration that alters
the bookings CHECK constraint to include 'declined', adds a new decline_reason
TEXT column to bookings, and updates any CHECK/foreign-key constraints on
booking_status_history to allow transitions involving 'declined' (so
booking_status_history and bookings constraints accept 'declined' as a valid
status); ensure the migration is idempotent and used by the code that writes
declineReason (declineSubmittedBooking) and any repository methods that update
bookings or booking_status_history.
In `@services/platform-api/src/orchestration/relay-domain-event.publisher.ts`:
- Around line 124-126: publishBookingDeclined currently only calls
loggingPublisher.publishBookingDeclined and thus bypasses the relay retry/DLQ
pipeline; change publishBookingDeclined to mirror the accepted-event path by
delegating the event to the relay pipeline before/after logging (i.e., call the
same relayAttemptExecutor method used for accepted events such as the
relayAttemptExecutor.execute/handle/publish method used by
publishBookingAccepted) so declined events go through retry/DLQ semantics while
still invoking loggingPublisher.publishBookingDeclined.
In
`@services/platform-api/src/providers/infrastructure/in-memory-provider-profile.repository.ts`:
- Around line 19-24: The update logic for bio and serviceArea uses truthy
fallback (input.bio?.trim() || existing?.bio) which prevents explicitly clearing
them with an empty string; change those assignments to check for undefined/null
instead of falsy so an empty trimmed string is accepted (e.g., set bio to
input.bio !== undefined ? input.bio.trim() : existing?.bio and similarly for
serviceArea), leaving tradeCategories and isPublic logic unchanged; update the
assignments in the in-memory-provider-profile.repository.ts where bio and
serviceArea are computed.
---
Outside diff comments:
In
`@services/platform-api/src/bookings/infrastructure/postgres-booking.repository.ts`:
- Around line 20-27: The BookingRow type and the SQL read paths are missing the
decline_reason field; update the BookingRow type to include decline_reason:
string | null, modify the SELECT in loadBookingRecord and loadBookingById to
include decline_reason, and update mapBookingRecord to map row.decline_reason to
the domain property declineReason (preserving null handling/typing consistent
with other fields).
---
Nitpick comments:
In `@apps/product-app/src/features/booking/booking-state.ts`:
- Line 4: The status field's union currently includes "| string" which collapses
type narrowing; update the status declaration in booking-state.ts (the status
property on the Booking/booking state type) to remove "| string" and use only
the explicit literals "submitted" | "accepted" | "declined" (or add any specific
allowed literals you actually support) so TypeScript can narrow checks for
accepted/declined branches correctly.
In `@apps/product-app/src/features/provider/provider-profile-state.test.ts`:
- Around line 39-48: The test for createLoadedProfileState omits verifying that
profile.serviceArea is mapped into the form; update the test for
createLoadedProfileState (using sampleProfile and the returned state) to assert
that when state.status === 'loaded' the profile.serviceArea equals the
form.serviceArea (e.g.,
expect(state.form.serviceArea).toEqual(state.profile.serviceArea or the expected
array/value from sampleProfile)); keep this check alongside the existing
displayName/bio/tradeCategories/isPublic assertions to prevent regressions in
the serviceArea mapping.
In `@services/platform-api/src/bookings/bookings.service.test.ts`:
- Around line 367-385: Add an assertion in the declineBooking replay test to
ensure the booking's statusHistory does not grow on replay: after calling
service.declineBooking(provider, created.booking.bookingId, {}) and inspecting
replay.booking.status, also assert that replay.booking.statusHistory.length
remains the expected length (e.g., 2) to ensure idempotent transitions; locate
this in the 'declineBooking — is idempotent for the same provider' test where
service.declineBooking and replay are used.
In `@services/platform-api/src/bookings/bookings.service.ts`:
- Around line 332-345: The booking.declined domain event currently omits the
declineReason; update the declinedEvent payload built in bookings.service.ts
(variable declinedEvent) to include declineReason (e.g., booking.declineReason
or declined.declineReason) under booking so notification workers can use it, and
bump the BookingDeclinedDomainEvent type in the domain package to include
declineReason on the booking shape; ensure serialization uses the same string
(or nullable) type as the source booking to avoid type errors.
In `@services/platform-api/src/orchestration/logging-domain-event.publisher.ts`:
- Around line 23-35: Both publishBookingAccepted and publishBookingDeclined
duplicate the same structured logging payload; extract a small helper (e.g.,
buildEventLogPayload or mapDomainEventToLog) that accepts the domain event
(BookingAcceptedDomainEvent | BookingDeclinedDomainEvent) and returns the common
breadcrumb payload (event, correlationId, status, details with eventName,
eventId, replayed, bookingId). Replace the duplicated logStructuredBreadcrumb
calls in publishBookingAccepted and publishBookingDeclined to call this helper
so future field changes are made in one place.
In `@services/platform-api/src/providers/providers.service.ts`:
- Around line 311-314: The early return when displayName is missing should
record a breadcrumb for observability before returning; in the block that
computes const displayName = input.displayName?.trim() and returns { ok: false,
statusCode: 400, error: 'displayName is required.' }, add a breadcrumb/log entry
using the module's tracing/logger (the same logger/tracing used elsewhere in
providers.service.ts) that records the validation failure, the incoming
input.displayName value (or that it was undefined/empty), and context like
method name so bad-request traces match other failure paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af937faf-a910-4f7b-bf20-c3d668c5e227
📒 Files selected for processing (31)
.agent/plans/phase2-marketplace-mvp-kickoff-2026-03-28.mdapps/product-app/src/features/booking/booking-screen-actions-decline.test.tsapps/product-app/src/features/booking/booking-screen-actions.tsapps/product-app/src/features/booking/booking-state.tsapps/product-app/src/features/provider/provider-profile-state.test.tsapps/product-app/src/features/provider/provider-screen-actions-profile.test.tsapps/product-app/src/features/provider/provider-screen-actions.tsapps/product-app/src/features/provider/provider-state.tspackages/api-client/src/index.tspackages/domain/src/index.tsservices/background-workers/src/workers/booking-declined.worker.test.tsservices/background-workers/src/workers/booking-declined.worker.tsservices/background-workers/vitest.config.tsservices/platform-api/src/bookings/bookings.controller.tsservices/platform-api/src/bookings/bookings.service.test.tsservices/platform-api/src/bookings/bookings.service.tsservices/platform-api/src/bookings/domain/booking.repository.tsservices/platform-api/src/bookings/infrastructure/in-memory-booking.repository.tsservices/platform-api/src/bookings/infrastructure/postgres-booking.repository.tsservices/platform-api/src/orchestration/domain-event.publisher.tsservices/platform-api/src/orchestration/logging-domain-event.publisher.tsservices/platform-api/src/orchestration/relay-domain-event.publisher.tsservices/platform-api/src/providers/domain/provider-profile.repository.tsservices/platform-api/src/providers/infrastructure/in-memory-provider-profile.repository.test.tsservices/platform-api/src/providers/infrastructure/in-memory-provider-profile.repository.tsservices/platform-api/src/providers/infrastructure/provider-profile-repository.provider.tsservices/platform-api/src/providers/providers.controller.tsservices/platform-api/src/providers/providers.module.tsservices/platform-api/src/providers/providers.service.profile.test.tsservices/platform-api/src/providers/providers.service.test.tsservices/platform-api/src/providers/providers.service.ts
Comment on lines
+47
to
+61
| export const createNotSetProfileState = (form: ProviderProfileFormState = initialProviderProfileFormState): ProviderProfileScreenState => ({ | ||
| status: 'not-set', | ||
| form, | ||
| }); | ||
|
|
||
| export const createLoadedProfileState = (profile: ProviderProfile): ProviderProfileScreenState => ({ | ||
| status: 'loaded', | ||
| profile, | ||
| form: { | ||
| displayName: profile.displayName, | ||
| bio: profile.bio ?? '', | ||
| serviceArea: profile.serviceArea ?? '', | ||
| tradeCategories: profile.tradeCategories, | ||
| isPublic: profile.isPublic, | ||
| }, |
Contributor
There was a problem hiding this comment.
Avoid shared mutable references in form state factories.
createNotSetProfileState and createLoadedProfileState currently pass through object/array references. If form state mutates in place anywhere, state can bleed across screens.
🧩 Proposed fix
export const createNotSetProfileState = (form: ProviderProfileFormState = initialProviderProfileFormState): ProviderProfileScreenState => ({
status: 'not-set',
- form,
+ form: {
+ ...form,
+ tradeCategories: [...form.tradeCategories],
+ },
});
@@
displayName: profile.displayName,
bio: profile.bio ?? '',
serviceArea: profile.serviceArea ?? '',
- tradeCategories: profile.tradeCategories,
+ tradeCategories: [...profile.tradeCategories],
isPublic: profile.isPublic,
},
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/product-app/src/features/provider/provider-state.ts` around lines 47 -
61, createNotSetProfileState and createLoadedProfileState are returning form
objects/arrays by reference, risking shared mutable state; update both factories
to return fresh copies instead of passing references: in
createNotSetProfileState clone initialProviderProfileFormState into a new object
for the form, and in createLoadedProfileState build a new form object by copying
primitive fields and shallow-cloning arrays (e.g., tradeCategories) and any
nested objects so no reference to profile or initial objects is retained. Ensure
the returned ProviderProfileScreenState contains these new cloned structures
rather than original references.
Comment on lines
+200
to
+202
| const result = await this.bookingsService.declineBooking(session, bookingId, { declineReason: body.declineReason }, { | ||
| correlationId, | ||
| }); |
Contributor
There was a problem hiding this comment.
Guard declineReason extraction against null-ish body payloads.
Line 200 dereferences body.declineReason directly. If request body is null/missing, this can throw before your controlled HTTP error handling.
Proposed defensive fix
- const result = await this.bookingsService.declineBooking(session, bookingId, { declineReason: body.declineReason }, {
+ const result = await this.bookingsService.declineBooking(
+ session,
+ bookingId,
+ { declineReason: body?.declineReason },
+ {
correlationId,
- });
+ },
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const result = await this.bookingsService.declineBooking(session, bookingId, { declineReason: body.declineReason }, { | |
| correlationId, | |
| }); | |
| const result = await this.bookingsService.declineBooking( | |
| session, | |
| bookingId, | |
| { declineReason: body?.declineReason }, | |
| { | |
| correlationId, | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform-api/src/bookings/bookings.controller.ts` around lines 200 -
202, The controller currently dereferences body.declineReason directly when
calling bookingsService.declineBooking, which can throw if body is
null/undefined; first read declineReason safely (e.g. const declineReason =
body?.declineReason) and validate it before calling declineBooking,
returning/throwing a controlled HTTP 400 (BadRequest) if the payload is missing
or invalid; then call this.bookingsService.declineBooking(session, bookingId, {
declineReason }, { correlationId }) so the service always receives a
well-defined value.
Comment on lines
+191
to
+197
| await client.query( | ||
| `UPDATE bookings | ||
| SET status = 'declined', | ||
| provider_user_id = $2::uuid | ||
| WHERE id = $1::uuid`, | ||
| [input.bookingId, input.providerUserId], | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the bookings table schema/migration to confirm decline_reason column exists
rg -n "decline_reason|declineReason" --type sql
rg -n "decline_reason|declineReason" -g '*.ts' services/platform-api/src/Repository: MarkoAtc/QuickWerk
Length of output: 1218
🏁 Script executed:
# Find migration files and database schema definitions
find . -type f \( -name "*migration*" -o -name "*schema*" -o -name "*.sql" \) | head -20Repository: MarkoAtc/QuickWerk
Length of output: 423
🏁 Script executed:
# Search for bookings table definition in migrations or schema files
rg -l "CREATE TABLE.*bookings|bookings.*TABLE" --type sql
rg "bookings" -g '*.ts' | grep -i "migration\|schema\|database" | head -20Repository: MarkoAtc/QuickWerk
Length of output: 125
🏁 Script executed:
# Look for TypeORM entities or database migrations patterns
find . -path "*/migrations/*" -o -path "*/database/*" | head -30Repository: MarkoAtc/QuickWerk
Length of output: 468
🏁 Script executed:
# Check the initial migration for bookings table schema
cat -n ./services/platform-api/migrations/0001_initial_auth_bookings.sqlRepository: MarkoAtc/QuickWerk
Length of output: 2730
🏁 Script executed:
# Search all migration files for decline_reason
rg -n "decline_reason|declineReason" ./services/platform-api/migrations/Repository: MarkoAtc/QuickWerk
Length of output: 44
🏁 Script executed:
# Check all migration files for bookings table modifications
for file in ./services/platform-api/migrations/*.sql; do
echo "=== $(basename $file) ===";
cat -n "$file" | head -50;
doneRepository: MarkoAtc/QuickWerk
Length of output: 6222
🏁 Script executed:
# Search for "declined" status in all migrations
rg -n "declined|DECLINED" ./services/platform-api/migrations/Repository: MarkoAtc/QuickWerk
Length of output: 44
🏁 Script executed:
# Read the declineSubmittedBooking method in postgres-booking.repository.ts to understand the context
sed -n '155,225p' ./services/platform-api/src/bookings/infrastructure/postgres-booking.repository.tsRepository: MarkoAtc/QuickWerk
Length of output: 2324
🏁 Script executed:
# Check if there are any pending/draft migrations
ls -la ./services/platform-api/migrations/ | grep -i draft
find . -name "*decline*" -o -name "*pending*" 2>/dev/null | head -20Repository: MarkoAtc/QuickWerk
Length of output: 283
🏁 Script executed:
# Verify the bookings table structure with comment about which PR added 'declined' status
git log --oneline --all | grep -i "decline\|status" | head -10Repository: MarkoAtc/QuickWerk
Length of output: 130
🏁 Script executed:
# Check the git diff for commit 98c5cc6 to see if migration was included
git show --name-only 98c5cc6 | head -30Repository: MarkoAtc/QuickWerk
Length of output: 1552
🏁 Script executed:
# Check booking_status_history constraint - does it need to allow 'declined'?
rg -n "to_status|from_status" ./services/platform-api/migrations/0001_initial_auth_bookings.sqlRepository: MarkoAtc/QuickWerk
Length of output: 303
🏁 Script executed:
# Check if there's a pending migration file that hasn't been committed
find ./services/platform-api/migrations -type f -newer ./services/platform-api/migrations/0005_operator_role_support.sql 2>/dev/nullRepository: MarkoAtc/QuickWerk
Length of output: 44
Missing database migration to support 'declined' status and declineReason persistence.
The declineSubmittedBooking implementation references a 'declined' status and attempts to persist declineReason, but the required database schema changes were never created. The bookings table schema only allows status IN ('submitted', 'accepted') and lacks a decline_reason column. Attempting to execute this code will fail with a PostgreSQL CHECK constraint violation.
Create a migration that:
- Updates the bookings table CHECK constraint to include 'declined' status
- Adds a
decline_reason TEXTcolumn - Updates the booking_status_history constraints to allow 'declined' transitions
Without this migration, the feature is non-functional in PostgreSQL mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform-api/src/bookings/infrastructure/postgres-booking.repository.ts`
around lines 191 - 197, The code references a 'declined' status and persists a
declineReason but the DB schema lacks that support; add a migration that alters
the bookings CHECK constraint to include 'declined', adds a new decline_reason
TEXT column to bookings, and updates any CHECK/foreign-key constraints on
booking_status_history to allow transitions involving 'declined' (so
booking_status_history and bookings constraints accept 'declined' as a valid
status); ensure the migration is idempotent and used by the code that writes
declineReason (declineSubmittedBooking) and any repository methods that update
bookings or booking_status_history.
Comment on lines
+19
to
+24
| bio: input.bio?.trim() || existing?.bio, | ||
| tradeCategories: (input.tradeCategories ?? existing?.tradeCategories ?? []) | ||
| .map((c) => c.trim()) | ||
| .filter(Boolean), | ||
| serviceArea: input.serviceArea?.trim() || existing?.serviceArea, | ||
| isPublic: input.isPublic ?? existing?.isPublic ?? false, |
Contributor
There was a problem hiding this comment.
Allow explicit clearing of bio and serviceArea.
Current fallback logic keeps old values when the client sends an empty string, so optional fields cannot be cleared once set. That breaks update semantics for editable optional fields.
Proposed fix
- bio: input.bio?.trim() || existing?.bio,
+ bio: input.bio !== undefined ? input.bio.trim() || undefined : existing?.bio,
tradeCategories: (input.tradeCategories ?? existing?.tradeCategories ?? [])
.map((c) => c.trim())
.filter(Boolean),
- serviceArea: input.serviceArea?.trim() || existing?.serviceArea,
+ serviceArea:
+ input.serviceArea !== undefined ? input.serviceArea.trim() || undefined : existing?.serviceArea,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bio: input.bio?.trim() || existing?.bio, | |
| tradeCategories: (input.tradeCategories ?? existing?.tradeCategories ?? []) | |
| .map((c) => c.trim()) | |
| .filter(Boolean), | |
| serviceArea: input.serviceArea?.trim() || existing?.serviceArea, | |
| isPublic: input.isPublic ?? existing?.isPublic ?? false, | |
| bio: input.bio !== undefined ? input.bio.trim() || undefined : existing?.bio, | |
| tradeCategories: (input.tradeCategories ?? existing?.tradeCategories ?? []) | |
| .map((c) => c.trim()) | |
| .filter(Boolean), | |
| serviceArea: | |
| input.serviceArea !== undefined ? input.serviceArea.trim() || undefined : existing?.serviceArea, | |
| isPublic: input.isPublic ?? existing?.isPublic ?? false, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform-api/src/providers/infrastructure/in-memory-provider-profile.repository.ts`
around lines 19 - 24, The update logic for bio and serviceArea uses truthy
fallback (input.bio?.trim() || existing?.bio) which prevents explicitly clearing
them with an empty string; change those assignments to check for undefined/null
instead of falsy so an empty trimmed string is accepted (e.g., set bio to
input.bio !== undefined ? input.bio.trim() : existing?.bio and similarly for
serviceArea), leaving tradeCategories and isPublic logic unchanged; update the
assignments in the in-memory-provider-profile.repository.ts where bio and
serviceArea are computed.
Must-fix items resolved: - postgres-booking.repository: add decline_reason to BookingRow, both SELECT queries, and mapBookingRecord -> declineReason mapping; also write decline_reason in declineSubmittedBooking UPDATE - provider-screen-actions: enforce required createdAt/updatedAt parsing (no empty-string fallback); extract shared parseProviderProfilePayload validator reused by load + save paths - provider-state factories: clone tradeCategories array in createLoadedProfileState to avoid shared mutable ref - api-client UpsertProviderProfileBody: make displayName optional to match server partial-update contract - packages/domain bookingStatuses: add 'declined' to the const array - bookings.controller decline endpoint: safe body access guard before reading declineReason; graceful handling of undefined body - migration 0006: idempotent ALTER TABLE / ADD COLUMN IF NOT EXISTS for declined status + decline_reason column on bookings and booking_status_history tables - relay-domain-event.publisher: route booking.declined through in-process retry/DLQ pipeline (consumeBookingDeclinedAttempt) mirroring booking.accepted; logging publisher still called first for audit trail - in-memory-provider-profile.repository: use explicit undefined check (input.bio !== undefined) so callers can explicitly clear bio/serviceArea High-value nitpicks included: - Remove '| string' from product-app CreatedBooking.status union; add parseBookingStatus guard in booking-screen-actions.ts - provider-profile-state.test: assert serviceArea mapping + tradeCategories clone isolation - bookings.service.test: assert statusHistory length unchanged on replay - bookings.service: include declineReason in BookingDeclinedDomainEvent booking payload (domain type already supports it) - providers.service: emit structured breadcrumb on invalid displayName - logging-domain-event.publisher: extract buildEventLogDetails helper to deduplicate payload mapping between accepted/declined publishers - background-workers: export consumeBookingDeclinedAttempt and related symbols from package index
Collaborator
Author
CodeRabbit feedback resolved (review 4025873138)Commit: aabf141 ✅ All must-fix actionable items resolved
✅ High-value nitpicks included
🧪 Test resultsAll typechecks pass for touched packages: platform-api, product-app, api-client, domain, background-workers. @coderabbitai review |
Contributor
|
Must-fix items (10/10 ✅)
High-value nitpicks ✅
Triggering a fresh review now. ✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
services/platform-api/src/orchestration/relay-domain-event.publisher.ts (2)
129-136: Consider extracting duplicate relay logic to reduce code duplication.The retry/DLQ loop pattern (initial attempt + for-loop with breadcrumb logging) is now duplicated between
publishBookingAcceptedandpublishBookingDeclined. Consider extracting a generic relay helper to improve maintainability as more event types are added.Also applies to: 151-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/orchestration/relay-domain-event.publisher.ts` around lines 129 - 136, The retry/DLQ pattern duplicated between publishBookingAccepted and publishBookingDeclined (the initial consumeBookingAcceptedAttempt/consumeBookingDeclinedAttempt call plus the for-loop using relayMaxAttempts, relayBaseBackoffMs and now for breadcrumb logging) should be extracted into a single helper (e.g., relayWithRetry or publishWithRetry) that accepts the event, a per-attempt handler function (consumeBookingAcceptedAttempt/consumeBookingDeclinedAttempt), maxAttempts, baseBackoffMs and now; replace both publishBookingAccepted and publishBookingDeclined with calls to this helper so the initial attempt, retry loop, breadcrumb logging, backoff calculation and DLQ handling are centralized.
127-136: Minor inconsistency:nowis captured once vs fresh per attempt.In
publishBookingAccepted,this.relayClock.now()is called fresh for each retry attempt (lines 61, 88). Here,nowis captured once at Line 127 and reused for all attempts. This may cause retry delay calculations to use a stale timestamp if backoff logic depends on the current time.Additionally,
shouldFailis hardcoded tofalsewhereas the accepted flow usesthis.relayAttemptPolicy.shouldFailAttempt(...), which limits chaos/fault-injection testing for declined events.♻️ Suggested alignment with accepted flow
async publishBookingDeclined(event: BookingDeclinedDomainEvent): Promise<void> { await this.loggingPublisher.publishBookingDeclined(event); - const now = this.relayClock.now(); - let finalWorkerResult = consumeBookingDeclinedAttempt({ event, attempt: 1, maxAttempts: relayMaxAttempts, baseBackoffMs: relayBaseBackoffMs, - shouldFail: false, - now, + shouldFail: this.relayAttemptPolicy.shouldFailAttempt({ + event, + attempt: 1, + maxAttempts: relayMaxAttempts, + }), + now: this.relayClock.now(), });Apply similarly in the retry loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/orchestration/relay-domain-event.publisher.ts` around lines 127 - 136, The declined-event path captures now once and hardcodes shouldFail, causing stale timestamps and disabling fault-injection; update the declined flow (calls to consumeBookingDeclinedAttempt) to mirror publishBookingAccepted by calling this.relayClock.now() inside each attempt/retry and compute shouldFail via this.relayAttemptPolicy.shouldFailAttempt(attempt, maxAttempts) (use relayMaxAttempts/relayBaseBackoffMs and the same call sites where consumeBookingDeclinedAttempt is invoked) so each attempt receives a fresh now and the proper shouldFail value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/background-workers/src/workers/index.ts`:
- Around line 13-20: Add a consumer registration for the booking-declined flow:
update the consumers array in main.ts to include an object with eventName:
'booking.declined' and handler: 'consumeBookingDeclinedAttempt' so the existing
consumeBookingDeclinedAttempt handler is wired (mirror the
booking-accepted-orchestration entry pattern); ensure the handler name exactly
matches consumeBookingDeclinedAttempt and that the eventName is
'booking.declined'.
---
Nitpick comments:
In `@services/platform-api/src/orchestration/relay-domain-event.publisher.ts`:
- Around line 129-136: The retry/DLQ pattern duplicated between
publishBookingAccepted and publishBookingDeclined (the initial
consumeBookingAcceptedAttempt/consumeBookingDeclinedAttempt call plus the
for-loop using relayMaxAttempts, relayBaseBackoffMs and now for breadcrumb
logging) should be extracted into a single helper (e.g., relayWithRetry or
publishWithRetry) that accepts the event, a per-attempt handler function
(consumeBookingAcceptedAttempt/consumeBookingDeclinedAttempt), maxAttempts,
baseBackoffMs and now; replace both publishBookingAccepted and
publishBookingDeclined with calls to this helper so the initial attempt, retry
loop, breadcrumb logging, backoff calculation and DLQ handling are centralized.
- Around line 127-136: The declined-event path captures now once and hardcodes
shouldFail, causing stale timestamps and disabling fault-injection; update the
declined flow (calls to consumeBookingDeclinedAttempt) to mirror
publishBookingAccepted by calling this.relayClock.now() inside each
attempt/retry and compute shouldFail via
this.relayAttemptPolicy.shouldFailAttempt(attempt, maxAttempts) (use
relayMaxAttempts/relayBaseBackoffMs and the same call sites where
consumeBookingDeclinedAttempt is invoked) so each attempt receives a fresh now
and the proper shouldFail value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cc0f565-e5d0-43fe-b2ea-da938921a6df
📒 Files selected for processing (19)
apps/product-app/src/features/booking/booking-screen-actions.tsapps/product-app/src/features/booking/booking-state.test.tsapps/product-app/src/features/booking/booking-state.tsapps/product-app/src/features/provider/provider-profile-state.test.tsapps/product-app/src/features/provider/provider-screen-actions.tsapps/product-app/src/features/provider/provider-state.tspackages/api-client/src/index.tspackages/domain/src/index.tsservices/background-workers/src/index.tsservices/background-workers/src/workers/index.tsservices/platform-api/migrations/0006_booking_declined_support.sqlservices/platform-api/src/bookings/bookings.controller.tsservices/platform-api/src/bookings/bookings.service.test.tsservices/platform-api/src/bookings/bookings.service.tsservices/platform-api/src/bookings/infrastructure/postgres-booking.repository.tsservices/platform-api/src/orchestration/logging-domain-event.publisher.tsservices/platform-api/src/orchestration/relay-domain-event.publisher.tsservices/platform-api/src/providers/infrastructure/in-memory-provider-profile.repository.tsservices/platform-api/src/providers/providers.service.ts
✅ Files skipped from review due to trivial changes (3)
- apps/product-app/src/features/booking/booking-state.test.ts
- services/platform-api/migrations/0006_booking_declined_support.sql
- apps/product-app/src/features/provider/provider-profile-state.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/product-app/src/features/booking/booking-state.ts
- services/platform-api/src/orchestration/logging-domain-event.publisher.ts
- services/platform-api/src/bookings/bookings.controller.ts
- packages/domain/src/index.ts
- services/platform-api/src/providers/infrastructure/in-memory-provider-profile.repository.ts
- services/platform-api/src/providers/providers.service.ts
- apps/product-app/src/features/provider/provider-state.ts
- services/platform-api/src/bookings/infrastructure/postgres-booking.repository.ts
Collaborator
Author
|
Addressed the latest actionable re-review feedback in commit 9f08fab. Changes made:
Validation rerun:
|
This was referenced Mar 28, 2026
7 tasks
This was referenced Apr 13, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Phase 2 — Slice 1: Marketplace MVP Kickoff
Phase 1 exit criteria are met (PR #4 merged). This PR kicks off Phase 2 with the first two foundational features of the marketplace loop.
What's in this PR
🔻 Booking Decline
Providers can now decline a submitted booking. Mirrors the accept flow with idempotency, audit trail, and domain event emission.
BookingStatusextended:'submitted' | 'accepted' | 'declined'POST /api/v1/bookings/:bookingId/decline(provider-only)declineReasonstored on the recordbooking.declineddomain event emitted (same structured pattern asbooking.accepted)👤 Provider Profile CRUD
Providers can create/update a public-facing profile (display name, bio, trade categories, service area, visibility).
ProviderProfileRepositoryinterface + in-memory implementation (upsert, get, list public)PUT /api/v1/providers/me/profile+GET /api/v1/providers/me/profile(provider-only)ProvidersModule⚙️ Worker: booking.declined
booking-declined.worker.ts— same retry/DLQ pattern asbooking-acceptedvitest.config.tsadded tobackground-workers(fixes pre-existing stale dist/ pickup)📦 Package updates
@quickwerk/domain:BookingDeclinedDomainEvent, worker envelope types@quickwerk/api-client:bookingApiRoutes.decline,providerApiRoutes.myProfile, typed request builders📋 Planning doc
.agent/plans/phase2-marketplace-mvp-kickoff-2026-03-28.md— full Phase 2 scope, milestones, API surface, testing strategy, risks.Test totals
All typechecks pass:
platform-api,product-app,admin-web,api-client,domain.Phase 1 behaviour preserved
What comes next (Slice 2)
GET /api/v1/providers— public provider list/filter for customer discoverybooking.declinedrelay pipeline (mirrors booking.accepted relay)Summary by CodeRabbit
New Features
Tests
Chores