feat(reschedule): dedicated pages, withdrawal, and an auto-confirm that cannot half-write - #1064
Merged
Conversation
…mits A consultee's proposal could leave a booking permanently un-reschedulable. The old sequence stamped the proposed times onto the released slot rows, ran the allocator in "requested" mode so it would read them back, and restored the originals from an in-memory snapshot if validation rejected them. Two ways that broke. The finalize step runs in its own transaction. Failing it left the booking confirmed at the new times while the proposal stayed PENDING_REVIEW with openForAppointmentId still set — and that nullable-unique then blocked every future reschedule of the appointment. Nothing surfaced it; the booking just quietly stopped being movable. Worse, the originals only ever existed in RAM. A crash between the stamp and the restore left the rows holding proposed times with no allocation to justify them and no way back. Manual mode already accepts explicit times, and on a reschedule deleteExistingAppointments removes only TENTATIVE slots — exactly the released ones — so confirmed sessions elsewhere in the booking survive. Handing the allocator the times directly means nothing is written until it commits, so neither failure has anywhere to happen. That deletes the stamp transaction, the restore transaction, the snapshot and the one-to-one pairing: 97 lines out, 33 in. Pairing only ever made sense while each proposed time was written onto a specific row; the allocator takes them as a set, so two non-contiguous proposed cells now fail validation instead of passing as one moved session. Manual mode shards its Redis lock by day (#860) so same-consultant allocations on different days run in parallel, with #440's GiST constraint backstopping overlap. But GiST sees overlaps, not counts — two sharded confirmations could each pass a per-week cap on the same stale read and take a 4-session week to 5. These times were not picked per-day by a human, so auto-confirm asks for the consultant-wide lock via a new `wideLock` flag. The consultant's own UI keeps its sharding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was no way to take back a reschedule you had asked for. The other party
could Decline; the person who opened it could only wait for expiry.
Withdrawal is the initiator's alone, whichever side they are on. The recipient
already has Decline, which ends the same request with a different meaning —
giving them this too would be a second Decline wearing a friendlier word.
The two outcomes differ in what happens to the released slots, which is why
WITHDRAWN is its own status rather than a reuse of DECLINED:
withdraw the person who asked no longer wants it, so nothing should have
moved — the booking returns to its original times
decline the consultee still wants to move and the consultant has not
agreed a time, so the slots stay released for their queue
expiry same as decline
Collapsing them would leave the audit trail unable to say who ended it, and
force every consumer to infer intent from resolvedById.
Restoring is cheap for one reason worth stating: a reschedule never rewrites
startsAt. The released rows still carry their original times, so this flips
two flags rather than replaying data from a snapshot.
WITHDRAWN is terminal, which is what releases openForAppointmentId — miss that
and a withdrawn request holds the nullable-unique forever, blocking every
later reschedule of the booking. There is a test for exactly that.
The CAS on the transition is the concurrency guard: if the other party
answered while this was in flight it matches zero rows and throws, rather than
un-releasing slots a concurrent accept has already re-confirmed.
Also adds RescheduleRequest.resolutionNote — the answering party's reply.
`reason` runs consultee to consultant; this is the return leg, and it is the
difference between "your sessions moved" and "moved to Thursdays, I have
blocked Tuesdays from September".
Both schema changes are additive and already applied; migrate diff reports no
drift.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MAX_PROPOSAL_ROUNDS and mayCounter existed, and the transition map has a PENDING_REVIEW <- COUNTERED edge. But nothing anywhere ever wrote COUNTERED: no route, no component, no job. The round-2 path was specified and never implemented. So this removes dead specification rather than a feature. Propose -> accept or decline is the whole flow, and a decline already falls back to the consultant allocating, so nothing dead-ends without it. The enum value stays. Removing it needs a migration for no benefit, and leaving it documents a path that was considered and rejected rather than forgotten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every defect found reviewing the reschedule and allocate dialogs traced back to width: labels overflowing their column, a legend that would not fit, a selection lost on reload. They were pages wearing modal costumes, the same shape the four planner dialogs had. Three routes now run over one component: consultee .../appointments/[appointmentId]/reschedule consultant .../appointments/[appointmentId]/reschedule (new) consultant .../requests/[requestId]/allocate The consultant reschedule route is a prerequisite, not an extra. That surface borrowed the CONSULTEE's modal, and the consultee route 403s a consultant — so deleting the dialogs without it would have left consultants unable to reschedule at all. Differences are data, not flags. A policy carries the rules (lead time, released slots shown, submit label, whether releasing without a time is allowed); a separate subject carries what is being placed (ids, durations, window, counterpart). That split removed two booleans outright: MANAGE_TIMINGS runs no consultee-conflict check because it has no counterpart, not because a flag says so, and "is this a fresh allocation" is subject data. SlotPicker never branches on which surface it is. EventTimingsCalendar is the fourth caller and now runs the same component under its own policy rather than being a fifth calendar to keep in sync. Consultants gained a times picker they never had — previously a confirm-only dialog. Safe by construction: only a CONSULTEE proposal auto-confirms, so a consultant's is always an offer the other side must accept. The sessions-then-times step machine is gone. A page can show the release picker above the grid, which was the point of moving off modals. Mobile has two gates answering different questions. CSS decides what is VISIBLE, so there is no hydration flash and the page is not client-only. matchMedia decides what is MOUNTED, so below lg the calendar subtree never mounts and the availability fetch never fires — the CSS-only version hid the grid while still fetching for it. The route segment is [requestId], not [appointmentId]: it receives the consultation/subscription id, and the Appointment row is downstream of it — it does not exist yet for a request that was never scheduled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two pre-existing bugs on dev. FLICKER. Two causes, of the three suspected. weeklySlotCount was a dependency of the very effect that fetches the data it derives from, so every navigation double-fetched: navigate, fetch, state updates, dependency changes, fetch again. And nothing tracked which window a response belonged to, so a slow week-N reply could land after the user had moved on and repaint the week they had left. Requests are now stamped and stale replies dropped. The third suspected cause did not hold and is worth recording: there was no clear-to-empty on navigation. The hook keeps the previous week rendered until new data arrives, and the full-grid spinner only fires on the initial load. No keep-previous-data shim was needed. ORG-ADMIN 403. The requests tab mounts the calendar in allocate mode, which asks for appointment details; the route authorised that on consultant ownership or isPrivileged — which is platform ADMIN/STAFF, not org admins. An org admin allocating for a member consultant lost the ENTIRE calendar, because the route 403s rather than downgrading. They are now neither authorised nor refused. The detail payload carries plan titles and participant names, including for the consultant's personal bookings with unrelated consultees — ADR 20 gives an org metadata, not content. So an org OWNER or MAINTAINER (checked against an ACTIVE membership in an org where the consultant is an ACTIVE EXPERT) gets the busy/free grid a buyer gets, and allocation works. The 403 was costing them the whole calendar over a tooltip they must not see anyway. Ordinary org members are deliberately excluded: only the two governance roles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
✅ Deploy Preview for familiarise ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe PR adds shared policy-driven scheduling components, routed allocation and rescheduling pages, calendar authorization, stale-request protection, centralized slot-status styling, consultant-wide allocation locks, and reschedule withdrawal handling. ChangesScheduling and rescheduling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
utils/slotAllocation/SlotAllocationService.ts (1)
827-834: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the trailing optional positionals with an options object.
manualAllocatenow ends withidempotencyKey?: string, initialAllocation?: boolean, wideLock?: boolean. Two adjacent optional booleans are easy to transpose, and the compiler accepts the transposition. There is one internal call site, so the change is contained.♻️ Proposed refactor
private static async manualAllocate( eventType: EventType, eventId: string, slotStrings: string[], - idempotencyKey?: string, - initialAllocation?: boolean, - wideLock?: boolean, + opts: { + idempotencyKey?: string; + initialAllocation?: boolean; + wideLock?: boolean; + } = {}, ): Promise<AllocationResult> { + const { idempotencyKey, initialAllocation, wideLock } = opts;Then update the dispatcher:
return await this.manualAllocate( request.eventType, request.eventId, request.slots, - request.idempotencyKey, - request.initialAllocation, - request.wideLock, + { + idempotencyKey: request.idempotencyKey, + initialAllocation: request.initialAllocation, + wideLock: request.wideLock, + }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/slotAllocation/SlotAllocationService.ts` around lines 827 - 834, Update SlotAllocationService.manualAllocate to replace the trailing idempotencyKey, initialAllocation, and wideLock positional parameters with a single options object containing those fields. Update its sole internal caller/dispatcher to pass the named options, preserving existing values and behavior.hooks/scheduling/useCalendarData.ts (1)
734-743: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGate
setLoading(false)with the same request-id guard.
fetchAvailabilitySlotsnow discards a stale response, but the caller's.finallystill runs for that stale promise. If the user navigates fast, the stale promise settles after the newer fetch started and clearsloadingwhile the newer request is still in flight. The spinner then reports "loaded" for a week whose data has not arrived.Expose the current request id (or a "latest settled" check) from the hook state and skip
setLoading(false)when the settled request is no longer current.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/scheduling/useCalendarData.ts` around lines 734 - 743, The fetch caller’s finally block unconditionally clears loading for stale requests. Update the request-id handling around fetchAvailabilitySlots and its .finally callback so setLoading(false) runs only when the settled request is still the current/latest request, while preserving loading for the newer in-flight request.lib/booking/reschedule-auto-confirm.ts (1)
74-121: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe failure mode described at lines 77-81 still exists: finalize is a separate transaction.
The comment states that the old shape broke because "the finalize step below ran in its own transaction, so failing it left a confirmed booking whose proposal never closed — openForAppointmentId still set, blocking every later reschedule of that appointment."
That is still the shape of this code.
SlotAllocationService.allocatecommits its own transaction at line 68. TheAUTO_ACCEPTEDtransition then runs in a second, independent transaction at line 107. If the second transaction fails for any reason other than a lost CAS race, the booking is confirmed at the new times while the request staysPENDING_REVIEWwithopenForAppointmentIdset. Every later reschedule of that appointment is then blocked by the nullable-unique.Two options:
- Add a recovery path that closes an
AUTO_ACCEPTED-pending request whose allocation already committed, for example an idempotent sweeper keyed on the request id.- Correct the comment so it does not claim the split-transaction hazard was removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/booking/reschedule-auto-confirm.ts` around lines 74 - 121, The split transaction between SlotAllocationService.allocate and transitionRescheduleRequest still leaves requests stuck when finalization fails. Add an idempotent recovery path keyed by the request id that completes the AUTO_ACCEPTED transition after allocation has committed, including non-CAS failures; otherwise revise the surrounding comments to accurately acknowledge the remaining hazard.app/api/slots/availability-with-allocation/[consultantId]/route.ts (1)
152-187: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not allow org admins to query another user’s calendar for allocate slots.
availability-with-allocationauthorizes an OWNER/MAINTAINER at line 172 but still rejectsconsulteeUserIdthere unless the caller is the consultee, the consultant, or PRIVILEGED. The allocate surface should not let the new org-admin calendar permission become a busy/free oracle forrequest.consulteeUserId, so this guard must remain narrower or pass the checked authorization through only for the authorized subject.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/slots/availability-with-allocation/`[consultantId]/route.ts around lines 152 - 187, Keep the requestedConsulteeUserId authorization in the availability-with-allocation route limited to the consultee, owning consultant, or privileged roles; do not reuse the broader maySeeCalendar org-admin authorization for this guard. Ensure org admins can view permitted calendar details only through the existing maySeeCalendar path and cannot query another user's calendar for allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/booking-algorithm/reschedule-withdraw.test.ts`:
- Around line 1-16: Add behavioral tests for withdrawRescheduleRequest using the
existing Prisma mocking pattern in this test directory. Cover NOT_INITIATOR,
PROPOSAL_NOT_OPEN for settled requests, restoring the slot even when
completionStatus is no longer RESCHEDULED, flipping consultation status from
PENDING to APPROVED while leaving subscriptions unchanged, and the lost-CAS
transaction path; retain the existing constant assertions.
In `@app/api/appointments/`[appointmentId]/reschedule/withdraw/route.ts:
- Around line 39-57: The non-initiator branch in the withdraw handler must
return the same 404 response as the no-open-request branch to avoid revealing an
existing reschedule request. Update the check around open.initiatedById and
preserve the existing 404 error shape and status.
In
`@app/dashboard/consultant/`[consultantId]/(features)/appointments/components/EventTimingsCalendar.tsx:
- Line 253: Update the SlotPickerSubject contract in slot-picker-policy.ts to
carry completed or remaining session information, then update
EventTimingsCalendar to pass the value derived from completedSessions,
groupTotalSessions, and totalSessions so the grid requests only the remaining
sessions, matching getDescriptionText.
In
`@app/dashboard/consultant/`[consultantId]/(features)/requests/[requestId]/allocate/page.tsx:
- Around line 42-46: Update the request guards after readAllocationRequest in
the allocation page to call notFound when request.status is not pending,
alongside the existing consultantProfileId ownership check. Preserve rendering
of the allocation grid only for pending requests.
In
`@app/dashboard/consultee/`[consulteeId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx:
- Around line 40-45: Update goBack in both RescheduleClient.tsx
files—app/dashboard/consultee/[consulteeId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx
lines 40-45 and
app/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx
lines 45-48—so appointments data is invalidated after the booking save via
revalidatePath/revalidateTag, or refreshed from the destination list route after
navigation; do not rely on router.refresh() immediately after
router.push(backHref).
In `@components/scheduling/DesktopOnlyNotice.tsx`:
- Around line 27-37: Update the isDesktop synchronization in DesktopOnlyNotice
so the first desktop match latches the state to true and later viewport changes
cannot reset it to false. Keep the initial media-query check and change
listener, while preserving the CSS-based hiding behavior for sub-1024px
viewports.
In `@components/scheduling/SessionReleasePicker.tsx`:
- Around line 102-107: Update the session button rendering to use the first
slot’s stable id as the React key instead of the array index, so reordered
sessions retain their identity. Remove the now-unused index parameter from the
corresponding sessions map callback while preserving the existing toggle and
disabled behavior.
- Around line 202-217: Update handleModeChange so the fallback session selected
when switching to "individual" is the first session outside the minLeadHours
lead window, rather than always sessions[0]. Preserve the alreadyPicked
selection when present, and fall back to an empty selection if no eligible
session exists.
In `@lib/booking/reschedule-auto-confirm.ts`:
- Around line 68-94: Normalize request.proposedSlots before passing them to
SlotAllocationService.allocate: expand each proposed row’s startsAt through
endsAt into consecutive 30-minute starts so manualAllocate receives one start
per allocated slot, or reject any unsupported duration. Preserve the existing
manual allocation options and ensure 60-minute proposals are not under-booked or
silently ignored.
In `@lib/booking/reschedule-withdraw.ts`:
- Around line 87-92: Replace the raw consultation update in the reschedule
withdrawal flow with transitionConsultationRequest, passing the consultation ID
and restricting fromIn to ["PENDING"] so only pending consultations transition
to APPROVED through the centralized guard.
- Around line 73-79: Update the slot restoration flow around the updateMany call
in reschedule-withdraw.ts to capture its affected-row count, compare it with
request.releasedSlotIds.length, and report any shortfall instead of proceeding
as a successful withdrawal. Preserve the existing restoration data and status
filters while ensuring partial restoration cannot return { withdrawn: true }
without signaling the mismatch.
- Around line 61-101: Update withdrawRescheduleRequest() to catch
IllegalTransitionError from transitionRescheduleRequest() before the generic
error handling, returning reason: "PROPOSAL_NOT_OPEN" for this expected lost-CAS
race. Preserve the existing reportSentryError and rethrow behavior for all other
errors, and mark the handled race as expected consistently with
reschedule-auto-confirm.ts.
---
Outside diff comments:
In `@app/api/slots/availability-with-allocation/`[consultantId]/route.ts:
- Around line 152-187: Keep the requestedConsulteeUserId authorization in the
availability-with-allocation route limited to the consultee, owning consultant,
or privileged roles; do not reuse the broader maySeeCalendar org-admin
authorization for this guard. Ensure org admins can view permitted calendar
details only through the existing maySeeCalendar path and cannot query another
user's calendar for allocation.
In `@hooks/scheduling/useCalendarData.ts`:
- Around line 734-743: The fetch caller’s finally block unconditionally clears
loading for stale requests. Update the request-id handling around
fetchAvailabilitySlots and its .finally callback so setLoading(false) runs only
when the settled request is still the current/latest request, while preserving
loading for the newer in-flight request.
In `@lib/booking/reschedule-auto-confirm.ts`:
- Around line 74-121: The split transaction between
SlotAllocationService.allocate and transitionRescheduleRequest still leaves
requests stuck when finalization fails. Add an idempotent recovery path keyed by
the request id that completes the AUTO_ACCEPTED transition after allocation has
committed, including non-CAS failures; otherwise revise the surrounding comments
to accurately acknowledge the remaining hazard.
In `@utils/slotAllocation/SlotAllocationService.ts`:
- Around line 827-834: Update SlotAllocationService.manualAllocate to replace
the trailing idempotencyKey, initialAllocation, and wideLock positional
parameters with a single options object containing those fields. Update its sole
internal caller/dispatcher to pass the named options, preserving existing values
and behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b139b3af-a4a6-4a7f-ae5e-28ecffaa4e90
📒 Files selected for processing (33)
__tests__/booking-algorithm/reschedule-proposals.test.ts__tests__/booking-algorithm/reschedule-withdraw.test.tsapp/api/appointments/[appointmentId]/reschedule/withdraw/route.tsapp/api/slots/availability-with-allocation/[consultantId]/route.tsapp/dashboard/consultant/[consultantId]/(features)/appointments/ConsultantAppointmentsAdapter.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/reschedule/page.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/components/EventTimingsCalendar.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/components/useConsultantEventActions.tsapp/dashboard/consultant/[consultantId]/(features)/requests/[appointmentId]/allocate/page.tsxapp/dashboard/consultant/[consultantId]/(features)/requests/[requestId]/allocate/AllocateClient.tsxapp/dashboard/consultant/[consultantId]/(features)/requests/[requestId]/allocate/page.tsxapp/dashboard/consultee/[consulteeId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsxapp/dashboard/consultee/[consulteeId]/(features)/appointments/[appointmentId]/reschedule/page.tsxcomponents/appointments/consultee/ConsulteeAppointmentsAdapter.tsxcomponents/appointments/consultee/RescheduleSessionsModal.tsxcomponents/appointments/consultee/useEventActions.tscomponents/dashboard/shared/requests/RequestSlotAllocationTab.tsxcomponents/scheduling/DesktopOnlyNotice.tsxcomponents/scheduling/SessionReleasePicker.tsxcomponents/scheduling/SlotPicker.tsxcomponents/scheduling/UnifiedCalendar.tsxcomponents/scheduling/slot-picker-policy.tshooks/scheduling/useCalendarData.tslib/booking/reschedule-auto-confirm.tslib/booking/reschedule-proposals.tslib/booking/reschedule-withdraw.tslib/booking/transitions.tslib/data/allocation-request.tslib/scheduling/slot-picker-subject.tsprisma/schema.prismautils/slotAllocation/SlotAllocationService.tsutils/slotAllocation/types.ts
💤 Files with no reviewable changes (2)
- components/appointments/consultee/RescheduleSessionsModal.tsx
- app/dashboard/consultant/[consultantId]/(features)/requests/[appointmentId]/allocate/page.tsx
"Forbidden: cannot access other user's calendar" on Allocate Slots. The org-admin arm was added to the appointment-details gate and not to the consulteeUserId gate immediately below it, so an org admin cleared the first check and was refused by the second — which made the allocate surface unusable for them, the exact case that arm was added to fix. The parameter belongs to them. It only marks cells BUSY, carrying no titles or names, so it is metadata rather than content and ADR 20 allows it. Allocation is wrong without it: the grid paints cells green that validation then rejects. The check is now resolved once and shared, rather than duplicated and drifting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This is why slot cells were ABSENT rather than faint, and why the first palette migration had to be reverted. Tailwind only emits a utility it has SEEN in a scanned file. `content` listed components/ and app/ but not lib/ or utils/ — so every class string defined in lib/scheduling/slot-status-tokens.ts produced NO CSS AT ALL unless the same class happened to appear under a scanned path too. Cells painted from those tokens had no fill and no border: not a faint cell, an invisible one. Past cells kept rendering because their classes were hardcoded inside the component, which was scanned. Two rounds of reasoning about opacity and border-colour precedence never found this, because the CSS was not losing a specificity contest — it did not exist. The calendar is not the only casualty. Seven files under lib/ and utils/ carry class strings: appointment status badges, org and session labels, document icons, auth provider buttons, support ticket UI. All were silently dropping whichever classes no scanned file happened to duplicate. Adding these paths means those previously-dead classes now emit, so expect those surfaces to change appearance — toward what they were always written to look like. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ble notes THE PALETTE. Cells and legend now render from SLOT_STATUS_TOKENS, so the legend finally describes the grid. The token shape changed to separate fill / border / text / hover fields rather than one className string, which makes the old failure structurally impossible: the base cell class carries `border` (width only) and exactly one token supplies the colour, so two border-colour utilities can never land on the same element again. `unavailable` goes back to slate-200. The reverted attempt used slate-100, which on a white card is close enough to the background that a sparse week read as an empty grid. `fullyBooked` moves to slate-300 so the two stay distinguishable now that unavailable is visible again — "nobody offered this" and "someone has this" are different answers. A test asserts the cell and the legend swatch resolve to the SAME utilities for every state, and scans the calendar source for the retired hardcoded classes. The existing test only asserted the legend COVERED every state, which is precisely how the two drifted apart unnoticed. PAGE IDENTITY. The three pages said only "Allocate slots" or "Reschedule", with no indication of which booking. They now show the offering title with the counterpart's name, and carry a generateMetadata tab title. The names come from data the pages already fetch — no extra query. REQUEST NOTES. The Note column clamps to three lines, which is right, but had no way to read the rest. A Read more toggle now appears ONLY when the text is actually clipped, measured against scrollHeight and re-measured on resize. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/dashboard/consultant/[consultantId]/(features)/requests/[requestId]/allocate/page.tsx (1)
68-72: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject a request that is not
PENDING.The page loads
request.statusfromreadAllocationRequestbut never checks it. Any status renders a live allocation grid. This URL survives a refresh, and a bookmarked link or the back button after a successful allocation reopens this page for a request that is already confirmed, rejected, or cancelled.Add the status check next to the ownership check.
🛡️ Proposed guard
const request = await loadRequest(requestId, eventType); if (!request) notFound(); // Binds the request to the URL's consultant; the guard above binds that // consultant to the session. if (request.consultantProfileId !== consultantId) notFound(); + // Only a pending request is allocatable. Without this, a bookmarked URL or + // the back button after a successful allocation reopens a live grid. + if (request.status !== "PENDING") notFound();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/consultant/`[consultantId]/(features)/requests/[requestId]/allocate/page.tsx around lines 68 - 72, Update the request guards after loadRequest in the allocation page to call notFound when request.status is not PENDING, alongside the existing consultantProfileId ownership check, so only pending requests render the allocation grid.app/api/slots/availability-with-allocation/[consultantId]/route.ts (1)
69-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting
isOrgAdminOfConsultantinto a shared auth helper.The comment states this membership shape mirrors logic used by
requireOrgAccess/catalog routes elsewhere. Duplicated authorization checks across files risk drifting out of sync as role semantics evolve. Moving this predicate intolib/auth-helpers.tsalongsideisPrivilegedkeeps the OWNER/MAINTAINER-over-EXPERT rule defined once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/slots/availability-with-allocation/`[consultantId]/route.ts around lines 69 - 100, Extract isOrgAdminOfConsultant into the shared auth helper module alongside isPrivileged, preserving its ACTIVE OWNER/MAINTAINER membership check against the consultant’s ACTIVE EXPERT organization membership. Update the availability route to import and reuse the shared helper, removing the local duplicate implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/dashboard/shared/requests/RequestSlotAllocationTab.tsx`:
- Line 211: Update the RequestNote props type to use Readonly<{ notes: string }>
instead of a mutable object type, preserving the existing notes field and
component behavior.
In `@lib/scheduling/slot-status-tokens.ts`:
- Around line 152-183: Update slotCellClassName to use cn from `@/utils/tailwind`
when combining SLOT_CELL_BASE_CLASS, the status token class, the optional faded
class, and options?.className. Preserve the existing argument order and
conditional class behavior while replacing the plain filter/join construction.
---
Outside diff comments:
In `@app/api/slots/availability-with-allocation/`[consultantId]/route.ts:
- Around line 69-100: Extract isOrgAdminOfConsultant into the shared auth helper
module alongside isPrivileged, preserving its ACTIVE OWNER/MAINTAINER membership
check against the consultant’s ACTIVE EXPERT organization membership. Update the
availability route to import and reuse the shared helper, removing the local
duplicate implementation.
In
`@app/dashboard/consultant/`[consultantId]/(features)/requests/[requestId]/allocate/page.tsx:
- Around line 68-72: Update the request guards after loadRequest in the
allocation page to call notFound when request.status is not PENDING, alongside
the existing consultantProfileId ownership check, so only pending requests
render the allocation grid.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 714a09b7-7e26-4d2b-9b36-2acd5a7b3aaa
📒 Files selected for processing (12)
__tests__/plans/offering-manifests.test.ts__tests__/schedule/slot-palette.test.tsapp/api/slots/availability-with-allocation/[consultantId]/route.tsapp/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/reschedule/page.tsxapp/dashboard/consultant/[consultantId]/(features)/requests/[requestId]/allocate/page.tsxapp/dashboard/consultee/[consulteeId]/(features)/appointments/[appointmentId]/reschedule/page.tsxcomponents/dashboard/shared/requests/RequestSlotAllocationTab.tsxcomponents/scheduling/SlotStatusLegend.tsxcomponents/scheduling/UnifiedCalendar.tsxlib/scheduling/slot-picker-subject.tslib/scheduling/slot-status-tokens.tstailwind.config.ts
…left The last cramped calendar. Its body already ran SlotPicker, but it kept a dialog wrapper, so the consultant scheduling their own webinar or class was still doing it at modal width while every other slot surface had a page. The route resolves two shapes of id, the same convention the appointments list already used client-side: a real Appointment id when the offering is scheduled, or the synthetic unscheduled-class-<id> / unscheduled-webinar-<id> when there is no Appointment row yet — which is exactly the case this page exists to fix. Deleting the dialog orphaned two files. utils/unscheduledAppointments.ts built its input; appointments/utils/appointmentTimingHelpers.ts exported two predicates nothing else called. Both are gone. Dead state after a deletion never lives in the file you deleted, it lives in what fed it. getEventDetails survives as lib/scheduling/manage-timings-subject.ts, now over a structural type rather than a client-only one, so a server page can call it. RequestedSlotsDialog was assessed and deliberately left alone. It mounts no calendar — it is a read-only validate-and-confirm summary of times the consultee already named, and its height cap exists because that list can be long, not because it wants a calendar's width. Genuinely dialog-shaped. ALSO IN THIS COMMIT — two fixes the user hit while testing, folded in rather than given a message of their own: An empty array is truthy, so a consultant with no org memberships still got the full switcher chrome in the sidebar — a chevron opening a menu containing nothing. The chip now becomes a dropdown only when there is at least one real destination; labels and separators are not somewhere to switch TO. Fixed in CollapsibleSidebar so consultee and anything added later inherit it. An affordance that opens an empty menu is worse than no affordance, which is why it hides rather than saying "no organizations". And the requests list no longer refetches on window focus. This went interval -> focus -> neither, and the last step mattered most: focus fires on every alt-tab, far MORE often than the timer it replaced for anyone actually working. It also reached the calendar, because that tab used to host it — a repaint mid-selection caused by data nobody asked for. Leaving it stale is safe, and that is the argument: the grid is a hint. Allocation re-validates server-side under a Redis lock against SlotValidationService and the btree_gist exclusion constraint, so a stale view cannot double-book — at worst a submit is rejected with a clear message. The Refresh button and the "Updated" label carry it instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
teetangh
force-pushed
the
feat/reschedule-pages
branch
from
July 31, 2026 19:59
a2fcb1f to
0584b36
Compare
The colour key sat below the calendar, so it was below the fold on a
laptop — a consultant saw a grid of colours with no way to learn what
they meant until after scrolling past them.
The header card cost the rest of that space. Each page rendered a
breadcrumb ending in a generic "allocate"/"reschedule"/"timings", then a
card repeating the offering title, then a back-link duplicating the
breadcrumb's own parent — ~200px restating what was already on screen,
and two destinations for one place (ADR 19).
The last crumb now carries the offering name via the existing
BreadcrumbOverrideProvider, which both dashboard layouts already mount
for precisely this ("a label replacing a stripped record-id crumb"). The
counterpart survives as a PanelHeader description, since it is the only
line saying who the booking is for.
Part of #1064
The consultee's menu checked that the booking had slots at all and that no reschedule was already in flight. The consultant's checked neither, so it offered Reschedule on an APPROVED booking with nothing allocated (no time to move, and no earliest released session to derive the proposal window from) and on one already awaiting a new time — where openForAppointmentId's nullable-unique guarantees a 409. Both now call slotsAllowReschedule. It lives in lib/appointments/slots, which exists for exactly this: predicates that were "duplicated with different windows and strictness" across the two sides. The status, role and route checks stay per-adapter — those differ for real reasons. Only the slot-derived half was ever meant to be the same. Part of #1064
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
app/dashboard/consultant/[consultantId]/(features)/requests/[requestId]/allocate/page.tsx (2)
53-72: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject a request that is not pending.
request.statusis loaded but never checked. This URL survives a refresh and can be linked from a notification, andAllocateClient.goBack()returns here after a successful allocation, so a bookmarked or revisited link reopens a live allocation grid for a request that is already confirmed, rejected, or cancelled. This code is unchanged from the prior commit where this was already flagged.🛡️ Proposed guard
const request = await loadRequest(requestId, eventType); if (!request) notFound(); if (request.consultantProfileId !== consultantId) notFound(); + if (request.status !== "PENDING") notFound();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/consultant/`[consultantId]/(features)/requests/[requestId]/allocate/page.tsx around lines 53 - 72, In AllocateSlotsPage, after loadRequest returns and before rendering the allocation grid, reject any request whose status is not pending by calling notFound(). Preserve the existing missing-request and consultantProfileId checks, and use the loaded request.status value for this guard.
37-51: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply the ownership guard before resolving the allocation request in
generateMetadata.
readAllocationRequest()resolves anyConsultation/Subscriptionby ID and returnsconsultantProfileId,title, andconsulteeName.generateMetadataresolves that request beforeAllocateSlotsPageruns itsrequirePersonalProfileAccess("consultant", consultantId)and request-ownership check, so an unowned URL can expose the consultee name and booking title in<title>before the page body returnsnotFound(). Run the same ownership check here and return the generic title on failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/consultant/`[consultantId]/(features)/requests/[requestId]/allocate/page.tsx around lines 37 - 51, Update generateMetadata to apply the same consultant ownership validation used by AllocateSlotsPage before exposing request-specific metadata. Validate access using the resolved request’s consultantProfileId and enforce the request-ownership check; on any validation failure, return the generic “Allocate slots” title, and only use request.title and request.consulteeName after authorization succeeds.app/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx (1)
48-51: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRefresh the appointments list from the destination, not with
router.refresh()afterrouter.push.Same unresolved pattern already flagged for this file in the prior review round:
router.refresh()targets the current route, which is unreliable right afterrouter.push(backHref).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/consultant/`[consultantId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx around lines 48 - 51, Update the goBack function to refresh the appointments data from the destination represented by backHref rather than calling router.refresh() after router.push(backHref). Preserve navigation to backHref while using the established destination-refresh pattern in RescheduleClient.app/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/reschedule/page.tsx (2)
59-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
planOwnerIdslogic — seelib/data/manage-timings-target.ts.This block re-implements the same plan-owner/collaborator flattening as
lib/data/manage-timings-target.ts(Lines 119-130), with the addition oftrialSession?.subscriptionPlan?.consultantProfile?.id. Extract one shared helper so the two ownership checks cannot drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/consultant/`[consultantId]/(features)/appointments/[appointmentId]/reschedule/page.tsx around lines 59 - 72, Extract the duplicated plan-owner and collaborator ID flattening from the page’s planOwnerIds block and lib/data/manage-timings-target.ts into one shared helper, preserving the trialSession subscription-plan owner. Update both ownership checks to call that helper, including the existing notFound behavior.
32-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMove the consultant access guard into
generateMetadata.
generateMetadataloads appointment detail only withappointmentIdand exposesresolved.consulteeNameandresolved.titlein the page title. Validate ownership in the metadata path withrequirePersonalProfileAccess("consultant", consultantId)plus theplanOwnerIdscheck before reading the appointment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/consultant/`[consultantId]/(features)/appointments/[appointmentId]/reschedule/page.tsx around lines 32 - 42, Update generateMetadata to extract consultantId from params and call requirePersonalProfileAccess("consultant", consultantId), then enforce the planOwnerIds ownership check before loadDetail or reading resolved appointment data. Preserve the existing fallback title and resolved title construction after authorization succeeds.app/dashboard/consultee/[consulteeId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx (1)
35-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace the empty
rawSlotswith the slot source used by the slot picker.
useEventActionsstill readsrawSlotsforgetJoinableSlotShared(), soRescheduleClientpassingrawSlots: []can show the “Meeting information is not available” toast or break join behavior. Pass the active subject/session slots instead of an empty placeholder; removeconsultantif any future UI does not need it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/consultee/`[consulteeId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx around lines 35 - 41, Update the useEventActions call in RescheduleClient so rawSlots uses the active subject/session slot source consumed by the slot picker instead of an empty array, preserving joinable-slot behavior; remove the consultant argument only if it is unused by the resulting UI.app/dashboard/consultee/[consulteeId]/(features)/appointments/[appointmentId]/reschedule/page.tsx (1)
32-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGate metadata generation before resolving the appointment subject.
generateMetadata()callsloadDetail(appointmentId)and formatsresolved.titleplusresolved.consultantNameinto the page title before validating the URL’sconsulteeIdagainst the session. Shared appointment detail includes counterpart names, so any caller with anappointmentIdcan read that booking title and consultant name through metadata even when the page body returnsnotFound(). Move the required check before resolving, or return a generic title until access is approved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/consultee/`[consulteeId]/(features)/appointments/[appointmentId]/reschedule/page.tsx around lines 32 - 42, Update generateMetadata to validate the URL consulteeId against the session before calling loadDetail or buildRescheduleSubject, and return the generic reschedule title when access is not approved. Ensure appointment-specific title and consultant name metadata is only produced after authorization succeeds.
♻️ Duplicate comments (1)
app/dashboard/consultee/[consulteeId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx (1)
43-48: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRefresh the appointments list from the destination, not with
router.refresh()afterrouter.push.
router.refresh()refreshes the current route. Calling it right afterrouter.push(backHref)targets whichever route is current at that moment, which is unreliable sincepushdoes not return a promise to sequence against. The comment on Line 44-45 states the intent is to invalidate the pre-release appointments list, but this call may not do that reliably. This is unchanged from the prior review round.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/consultee/`[consulteeId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx around lines 43 - 48, Update goBack so the destination appointments route is refreshed through its own navigation or revalidation mechanism rather than calling router.refresh() immediately after router.push(backHref). Ensure the appointments list at backHref is invalidated after returning, while preserving the existing back navigation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/dashboard/consultant/`[consultantId]/(features)/appointments/[appointmentId]/timings/ManageTimingsClient.tsx:
- Around line 30-33: Update goBack so the destination appointments list is
refreshed through the destination route’s established invalidation or refresh
mechanism instead of calling router.refresh() after router.push(backHref).
Follow the existing pattern in RescheduleClient.tsx for the corresponding
consultee and consultant flows, while preserving navigation to backHref.
In
`@app/dashboard/consultant/`[consultantId]/(features)/appointments/[appointmentId]/timings/page.tsx:
- Around line 36-45: Guard generateMetadata before calling
buildManageTimingsSubject by applying the same ownership validation used by
ManageTimingsPage, including requirePersonalProfileProfileAccess and the
target.planOwnerIds check. If access cannot be verified, return the generic
“Manage timings — Familiarise” title; only include the offering title for an
authorized consultant.
In `@components/dashboard/shared/requests/RequestSlotAllocationTab.tsx`:
- Around line 687-705: Update fetchData and its useEffect to remove error from
the callback dependencies, preventing failed requests from retriggering
automatically. Track whether the request succeeded locally so finally always
clears loading, while lastUpdated is updated only after a successful fetch;
preserve manual refresh behavior and the stable error view.
In `@lib/data/manage-timings-target.ts`:
- Around line 119-130: Extract the plan-owner ID aggregation into a shared
resolvePlanOwnerIds(appointment) helper, including consultation, subscription,
trialSession, webinar, and class plan consultants plus accepted collaborators.
Replace the local planOwnerIds computation in the current module and the
consultant reschedule route with calls to this helper, preserving the existing
ownerIds filtering behavior.
- Around line 140-164: Update the groupTotalSessions value in
readAppointmentDetail() to use the plan’s totalSessions rather than
scheduled.length, preserving the existing scheduled filtering only for
completedSessions and keeping the value available for subscription and class
plans.
---
Outside diff comments:
In
`@app/dashboard/consultant/`[consultantId]/(features)/appointments/[appointmentId]/reschedule/page.tsx:
- Around line 59-72: Extract the duplicated plan-owner and collaborator ID
flattening from the page’s planOwnerIds block and
lib/data/manage-timings-target.ts into one shared helper, preserving the
trialSession subscription-plan owner. Update both ownership checks to call that
helper, including the existing notFound behavior.
- Around line 32-42: Update generateMetadata to extract consultantId from params
and call requirePersonalProfileAccess("consultant", consultantId), then enforce
the planOwnerIds ownership check before loadDetail or reading resolved
appointment data. Preserve the existing fallback title and resolved title
construction after authorization succeeds.
In
`@app/dashboard/consultant/`[consultantId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx:
- Around line 48-51: Update the goBack function to refresh the appointments data
from the destination represented by backHref rather than calling
router.refresh() after router.push(backHref). Preserve navigation to backHref
while using the established destination-refresh pattern in RescheduleClient.
In
`@app/dashboard/consultant/`[consultantId]/(features)/requests/[requestId]/allocate/page.tsx:
- Around line 53-72: In AllocateSlotsPage, after loadRequest returns and before
rendering the allocation grid, reject any request whose status is not pending by
calling notFound(). Preserve the existing missing-request and
consultantProfileId checks, and use the loaded request.status value for this
guard.
- Around line 37-51: Update generateMetadata to apply the same consultant
ownership validation used by AllocateSlotsPage before exposing request-specific
metadata. Validate access using the resolved request’s consultantProfileId and
enforce the request-ownership check; on any validation failure, return the
generic “Allocate slots” title, and only use request.title and
request.consulteeName after authorization succeeds.
In
`@app/dashboard/consultee/`[consulteeId]/(features)/appointments/[appointmentId]/reschedule/page.tsx:
- Around line 32-42: Update generateMetadata to validate the URL consulteeId
against the session before calling loadDetail or buildRescheduleSubject, and
return the generic reschedule title when access is not approved. Ensure
appointment-specific title and consultant name metadata is only produced after
authorization succeeds.
In
`@app/dashboard/consultee/`[consulteeId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx:
- Around line 35-41: Update the useEventActions call in RescheduleClient so
rawSlots uses the active subject/session slot source consumed by the slot picker
instead of an empty array, preserving joinable-slot behavior; remove the
consultant argument only if it is unused by the resulting UI.
---
Duplicate comments:
In
`@app/dashboard/consultee/`[consulteeId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsx:
- Around line 43-48: Update goBack so the destination appointments route is
refreshed through its own navigation or revalidation mechanism rather than
calling router.refresh() immediately after router.push(backHref). Ensure the
appointments list at backHref is invalidated after returning, while preserving
the existing back navigation behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2c23f5b8-9e4a-472c-86b1-a138969f7d42
📒 Files selected for processing (20)
__tests__/booking-algorithm/reschedule-affordance.test.tsapp/dashboard/consultant/[consultantId]/(features)/appointments/ConsultantAppointmentsAdapter.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/reschedule/page.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/timings/ManageTimingsClient.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/timings/page.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/components/EventTimingsCalendar.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/utils/appointmentTimingHelpers.tsapp/dashboard/consultant/[consultantId]/(features)/appointments/utils/unscheduledAppointments.tsapp/dashboard/consultant/[consultantId]/(features)/requests/[requestId]/allocate/AllocateClient.tsxapp/dashboard/consultant/[consultantId]/(features)/requests/[requestId]/allocate/page.tsxapp/dashboard/consultee/[consulteeId]/(features)/appointments/[appointmentId]/reschedule/RescheduleClient.tsxapp/dashboard/consultee/[consulteeId]/(features)/appointments/[appointmentId]/reschedule/page.tsxcomponents/appointments/consultee/ConsulteeAppointmentsAdapter.tsxcomponents/dashboard/CollapsibleSidebar.tsxcomponents/dashboard/shared/requests/RequestSlotAllocationTab.tsxcomponents/scheduling/SafeUnifiedCalendar.tsxlib/appointments/slots.tslib/data/manage-timings-target.tslib/scheduling/manage-timings-subject.ts
💤 Files with no reviewable changes (3)
- app/dashboard/consultant/[consultantId]/(features)/appointments/components/EventTimingsCalendar.tsx
- app/dashboard/consultant/[consultantId]/(features)/appointments/utils/unscheduledAppointments.ts
- app/dashboard/consultant/[consultantId]/(features)/appointments/utils/appointmentTimingHelpers.ts
**A 60-minute proposal booked 30 minutes.** The proposal schema only asked that endsAt follow startsAt, but auto-confirm hands the allocator startsAt alone and manual mode reads each entry as one 30-minute start — endsAt is never consulted. The one-for-one count check still passed, which is what made it invisible: ask to move a 1-hour session, silently get half of one. Proposed rows are now exactly one atom (ADR B1), rejected at the edge. **The withdraw route was a membership oracle.** It never checks that the caller can see the appointment, so 404-vs-403 let any signed-in user walk appointment ids and learn which bookings had a live reschedule and whose. "No open request" and "someone else's" now answer identically. **A lost CAS was reported as a fault.** Another party answering mid-flight throws IllegalTransitionError, which reached apiError as a 500 and paged Sentry without expected:true. It is a modelled outcome and now returns PROPOSAL_NOT_OPEN, matching how auto-confirm treats the same race. **The allocate page ignored the status it loaded.** The URL is linkable from a notification and goBack() pushes, so the back button reopens it — on a request already confirmed, rejected or cancelled. Guarded with ALLOCATION_APPROVABLE_FROM rather than `=== PENDING`: a partial reschedule allocates from this same page, and a subscription is deliberately not flipped back to PENDING when a session is released (#448), so it arrives still APPROVED. Also: a partial slot restore no longer reports plain success, and the consultation flip goes through transitionConsultationRequest so the edge is owned centrally. 13 tests, including the behavioural coverage the withdraw module had none of. Part of #1064
**"One session" could select a session you cannot move.** Narrowing from "several" fell back to sessions[0], and the list is earliest-first — so it picked the session most likely to be inside the lead window. SessionList disables that row, leaving it ticked and untickable, while the submit stayed enabled (it counts ids, not eligibility) and the server rejected a release the user had no way to change. It now falls back to the first session outside the window. **A shrink below `lg` silently desynced the selection.** DesktopOnlyNotice unmounted the subtree on resize, destroying the calendar's own selection while SlotPicker kept its copy — the footer then claimed "N slots selected" over a grid showing none, and submitting sent times the user could no longer see. The mount gate latches; the CSS gate still hides the subtree, which is what the notice is actually for. Also: slotCellClassName merges through cn, since Tailwind resolves same-specificity conflicts by stylesheet position rather than concatenation order — the exact mechanism behind this PR's own border-transparent regression, and the open className extension point had no protection from it. Session rows key off their first slot id rather than an array index. Part of #1064
…fted owner check **A failing endpoint hammered the API and never settled.** fetchData both writes `error` and listed it as a dependency, and the effect calls fetchData — so a failure changed its identity, refired it, cleared the error, changed it again, and looped. The `finally` also read `error` from the render-time closure rather than the value just set, so a failed fetch still stamped "Updated" with a timestamp it had not earned. Success is now tracked locally and `error` is out of the deps. **generateMetadata ran before every guard on all four pages.** It loads and formats the offering title with no ownership check of its own, so the tab `<title>` named the offering — and on two pages the counterparty — for any appointment or request id a signed-in user cared to try. Each page's metadata now runs the same predicate its body does and falls back to the generic title. The consultee check is extracted so the two cannot diverge. **Two hand-written copies of an ownership check had already drifted.** manage-timings-target omitted `trialSession`, which the reschedule route includes — so a consultant opening the timings of their own trial was refused by one route and admitted by the other. Both now call resolvePlanOwnerIds. Collaborators stay webinar/class-only; widening that here would have granted access the other surfaces do not. **Program progress counted rows, not sessions.** groupTotalSessions read `scheduled?.length` over sibling Appointment rows, which are real appointments rather than session placeholders — so an unscheduled sibling was simply missing and the total shrank to whatever was already placed, folding completed sessions into "remaining". It reads the plan's totalSessions. Part of #1064
It was applied AFTER the segment loop, gated on a flag that the trailing task segment cleared on its way past: ["appointments", "<id>", "timings"] push Appointments flag=true, skip flag=false, push "timings" So it only ever worked when the record id was the last segment. On all four slot pages the id is followed by a task, which is why the trail still read "Appointments › timings" after the label was wired up — the hook was setting a value nothing consumed. The label now goes in the id's OWN position, which is where it belongs: that segment IS the record. Appointments › Basic Consultation › Timings Requests › Basic Consultation › Allocate Appointments › Basic Consultation › Reschedule It also keeps the id's href, so the offering name links to the detail page — a level these routes previously could not reach at all. PAGE_LABELS gains timings/allocate/reschedule, which is the other half of what was on screen: with no entry the crumb fell through to the raw lowercase URL segment. Also folds in three review items from the appointments filter bar: "All" leads the status tabs to match the "All types" chip; the Trials TAB is renamed "Trial requests" so it stops colliding with the Trials type chip one row below (same word, different result — the tab is the TrialSession queue, the chip filters the bucket); and the date inputs get room for Chrome's calendar-picker indicator, which was overflowing onto the border at w-140px with px-3. Part of #1064
|
4 tasks
This was referenced Aug 1, 2026
Closed
teetangh
added a commit
that referenced
this pull request
Aug 1, 2026
Two independent causes of cells reading as absent, both found while looking at the picker's opening position. The availability request was clamped to allowedEnd in allocate mode while the grid kept drawing a full seven columns, so every cell past the period had no server row — and because the route filters appointments by the same window, the BOOKED cells in that range vanished identically to the available ones, which is what ruled out a cap or a disabled state. A week further on, endDate fell before startDate, the server had no range to scan, and the entire grid blanked until the consultant pressed back. The comment above this argued the START must not be clamped, so a pre-period week shows real availability behind an "Outside Period" label rather than blanks; the same argument was never applied to the end. allowedStart/allowedEnd govern selectability — handleSlotClick's range guard and that label already enforce it — never visibility. Each allocate request is now a week rather than the rest of the period, which also removes the #997 cost this PR's description already flagged. Separately, SLOT_CELL_BASE_CLASS carried disabled:opacity-50. Every unavailable cell is disabled, so bg-slate-200 reached the screen at half strength over a white card — near enough to the slate-100 that #1064 records as reverted for making a sparse week read as an empty grid. The Tailwind `content` cause fixed there was real; this was a second cause cancelling the palette at render time. The legend swatch is not a disabled control, so the legend showed the true colour while the grid showed half of it. pointer-events-none stays, and the explicit opacity-50/60/70 renderTimeCell appends for PAST cells is untouched — that fade is asked for. Also on this hook: an unreadable response now sets an error instead of silently painting an empty grid, since an empty grid is a valid answer for a quiet week; and the spinner's finally is request-scoped like the success and error paths already were. Tests: the base class carries no disabled:opacity-*, which the existing swatch-equality test compares strings and structurally cannot see; and the allocate window ends at the end of the visible week, stays forward for a week past the period, and spans one week rather than a year. All four verified to fail against the pre-fix code. Part of #1073
teetangh
added a commit
that referenced
this pull request
Aug 1, 2026
) * feat(scheduling): open the slot picker on the session you clicked The picker opened on the current week scrolled to 00:00, so every visit began by hunting for the right day and scrolling past ten hours of empty night rows. Derive a target instant from the subject the page already carries, move the week to contain it and scroll the time axis to it. One resolver for all four surfaces: they differ only in which sessions their subject carries. A session awaiting a time outranks the next upcoming one, since filling gaps is the job on a partly-placed program; an offering with nothing scheduled falls back to its scheduling period, clamped out of the past and anchored on first availability. Fires once per open, held in a ref — re-aiming the grid under a reader is worse than the rows it replaces, and deps-driven focus is how this component reached React #185 before. Closes #1073 * fix(scheduling): make the focus effect fire, and stop it shipping attendees Review follow-ups on #1073. The effect keyed on values that settle BEFORE the week grid mounts, and the grid only renders once consultantDetails arrives. Whenever availability won that race the effect ran against a null container, returned, and — a ref cannot wake an effect — never ran again. The container is state now, via a callback ref, so the grid's own appearance is what triggers it. The comment claiming `loading` guarded this was wrong: it starts false, before anything is fetched. readAppointmentDetail's slots come from an `include`, so spreading them into the manage-timings subject shipped every attendee and every recording URL to the client — and on a class those rows are shared by the whole roster. Routed through one shared allowlist (#946's fix, reapplied), which reschedule now uses too. A past session is no longer the target while the scheduling period is still open: those programs have sessions LEFT to place, and pointing at a dead week also stretched the allocate-mode availability request from that week to the end of the period (#997). Released sessions whose time has passed, soft-deleted slots and inverted windows are handled for the same reason — never open where nothing can be chosen. Formatter pinned to hourCycle h23: `hour12: false` resolves to h24 under an en-US default, which writes midnight against the previous day. Tests: the effect (fires late, fires once), and both widened payloads. Both new suites verified to fail against the pre-fix code. Part of #1073 * fix(scheduling): stop the period clamping what the grid can show Two independent causes of cells reading as absent, both found while looking at the picker's opening position. The availability request was clamped to allowedEnd in allocate mode while the grid kept drawing a full seven columns, so every cell past the period had no server row — and because the route filters appointments by the same window, the BOOKED cells in that range vanished identically to the available ones, which is what ruled out a cap or a disabled state. A week further on, endDate fell before startDate, the server had no range to scan, and the entire grid blanked until the consultant pressed back. The comment above this argued the START must not be clamped, so a pre-period week shows real availability behind an "Outside Period" label rather than blanks; the same argument was never applied to the end. allowedStart/allowedEnd govern selectability — handleSlotClick's range guard and that label already enforce it — never visibility. Each allocate request is now a week rather than the rest of the period, which also removes the #997 cost this PR's description already flagged. Separately, SLOT_CELL_BASE_CLASS carried disabled:opacity-50. Every unavailable cell is disabled, so bg-slate-200 reached the screen at half strength over a white card — near enough to the slate-100 that #1064 records as reverted for making a sparse week read as an empty grid. The Tailwind `content` cause fixed there was real; this was a second cause cancelling the palette at render time. The legend swatch is not a disabled control, so the legend showed the true colour while the grid showed half of it. pointer-events-none stays, and the explicit opacity-50/60/70 renderTimeCell appends for PAST cells is untouched — that fade is asked for. Also on this hook: an unreadable response now sets an error instead of silently painting an empty grid, since an empty grid is a valid answer for a quiet week; and the spinner's finally is request-scoped like the success and error paths already were. Tests: the base class carries no disabled:opacity-*, which the existing swatch-equality test compares strings and structurally cannot see; and the allocate window ends at the end of the visible week, stays forward for a week past the period, and spans one week rather than a year. All four verified to fail against the pre-fix code. Part of #1073
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.
485 insertions, 1,179 deletions. The net is a removal, because most of this replaces machinery that existed to work around a shape that was wrong.
The live bug this fixes
A consultee's proposal could leave a booking permanently un-reschedulable, and nothing surfaced it.
Auto-confirm stamped the proposed times onto the released slot rows, ran the allocator in
requestedmode so it would read them back, and restored the originals from an in-memory snapshot if validation rejected them. Two ways that broke:PENDING_REVIEWwithopenForAppointmentIdstill set — and that nullable-unique then blocked every future reschedule of the appointment.Manual mode already accepts explicit times, and on a reschedule
deleteExistingAppointmentsremoves only tentative slots — exactly the released ones — so confirmed sessions elsewhere in the booking survive. Handing the allocator the times directly means nothing is written until it commits, so neither failure has anywhere to happen.That deletes the stamp transaction, the restore transaction, the snapshot and the one-to-one pairing. It also dissolves the atoms-vs-sessions problem rather than patching it: the allocator takes the times as a set, so two non-contiguous proposed cells now fail validation instead of passing as one moved session.
One trap found by checking rather than assuming. Manual mode shards its Redis lock by day (#860) so same-consultant allocations on different days run in parallel, with #440's GiST constraint backstopping overlap. But GiST sees overlaps, not counts — two sharded confirmations could each pass a per-week cap on the same stale read and take a 4-session week to 5. These times are not picked per-day by a human, so auto-confirm takes the consultant-wide lock via a new
wideLockflag. The consultant's own UI keeps its sharding.Withdrawing, and why it is its own status
There was no way to take back a reschedule you had asked for. The other party could Decline; the person who opened it could only wait for expiry.
Withdrawal is the initiator's alone, whichever side they are on. The recipient already has Decline — giving them this too would be a second Decline wearing a friendlier word.
The two differ in what happens to the released slots, which is exactly why
WITHDRAWNis not a reuse ofDECLINED:Collapsing them would leave the audit trail unable to say who ended it.
Restoring is cheap for one reason worth stating: a reschedule never rewrites
startsAt. The released rows still carry their original times, so this flips two flags rather than replaying a snapshot.WITHDRAWNis terminal, which is what releasesopenForAppointmentId— miss that and a withdrawn request holds the nullable-unique forever, blocking every later reschedule. There is a test for exactly that.Also adds
resolutionNote:reasonruns consultee → consultant, this is the return leg. The difference between "your sessions moved" and "moved to Thursdays, I've blocked Tuesdays from September".Both dialogs are deleted
Every defect found reviewing them traced back to width — labels overflowing their column, a legend that would not fit, a selection lost on reload. They were pages wearing modal costumes, the same shape the four planner dialogs had.
Three routes now run over one component:
The consultant reschedule route is a prerequisite, not an extra. That surface borrowed the consultee's modal, and the consultee route 403s a consultant — so deleting the dialogs without it would have left consultants unable to reschedule at all.
Differences are data, not flags. A policy carries the rules; a separate subject carries what is being placed. That split removed two booleans outright —
MANAGE_TIMINGSruns no counterpart-conflict check because it has no counterpart, not because a flag says so; and "is this a fresh allocation" is subject data.SlotPickernever branches on which surface it is.EventTimingsCalendaris the fourth caller and now runs the same component under its own policy rather than being a fifth calendar to keep in sync.Consultants gained a times picker they never had — previously a confirm-only dialog. Safe by construction: only a consultee proposal auto-confirms, so a consultant's is always an offer.
Mobile has two gates answering different questions. CSS decides what is visible, so there is no hydration flash and the page is not client-only.
matchMediadecides what is mounted, so belowlgthe calendar subtree never mounts and the availability fetch never fires — the CSS-only version hid the grid while still fetching for it.The segment is
[requestId], not[appointmentId]: it receives the consultation/subscription id, and theAppointmentrow is downstream of it — it does not exist for a request that was never scheduled.Two pre-existing bugs fixed on the way
Week-navigation flicker. Two causes of the three suspected.
weeklySlotCountwas a dependency of the very effect that fetches the data it derives from, so every navigation double-fetched. And nothing tracked which window a response belonged to, so a slow reply could repaint a week you had already left.The third suspected cause did not hold, which is worth recording: there was no clear-to-empty on navigation, so no keep-previous-data shim was needed.
Org-admin 403. The requests tab mounts the calendar in allocate mode, which asks for appointment details; the route authorised that on consultant ownership or
isPrivileged— platform ADMIN/STAFF, not org admins. An org admin allocating for a member consultant lost the entire calendar, because the route 403s rather than downgrading.They are now neither authorised nor refused. The detail payload carries plan titles and participant names, including for the consultant's personal bookings with unrelated consultees — ADR 20 gives an org metadata, not content. So an org OWNER or MAINTAINER gets the busy/free grid a buyer gets, and allocation works. Ordinary org members are deliberately excluded.
The counter-round is gone, and cost nothing
MAX_PROPOSAL_ROUNDSandmayCounterexisted and the transition map had the edge — but nothing anywhere ever wroteCOUNTERED. It was specified and never built, so this removes dead specification rather than a feature. The enum value stays: removing it needs a migration for no benefit, and leaving it documents a path considered and rejected rather than forgotten.Database
Two additive changes, already applied:
WITHDRAWNonRescheduleRequestStatus, andRescheduleRequest.resolutionNote.prisma migrate diffreports no difference.Verification
Cold
tscclean · 2,083 tests across 186 files (+4 tests, +1 suite) · lint clean apart from three pre-existingno-explicit-anywarnings on untouched lines.Not verified in a browser
None of the three pages has been opened. That is the acceptance gate, and given this PR deletes the dialogs, the routes are the only way to reschedule or allocate — so they need clicking before merge, on all three surfaces.
Deliberately not in this PR
AutoAllocationPreferencesis not wired. The preference must score candidates, never filter them — a filter reproduces the bug where auto-allocate reported "no slots available" with seven hours free.Sentry.init. 58 job files report into an uninitialised SDK, so none of the observability work reaches a scheduled job.Summary by CodeRabbit
New Features
Bug Fixes