feat(scheduling): open the slot picker on the session you clicked - #1075
Conversation
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
|
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. |
|
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 (17)
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 |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
…endees 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
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
|
1 participant
Opening Manage Timings, Allocate or Reschedule today drops the consultant into the current week, scrolled to the top of a grid that starts at 00:00. If the booking they came to edit is on Saturday at 10:30, the first thing they do on every single visit is hunt for Saturday and then scroll down past ten hours of empty night-time rows before the session they clicked is even on screen. For a session next month they navigate week by week to get there. None of that is a decision the picker was asking them to make; it is simply where the viewport happened to start.
This change makes the picker open already looking at the thing that was clicked. It picks a target instant from the subject the page already has, moves the visible week to contain it, and scrolls the time axis so the target sits a couple of rows down rather than flush against the top edge. Nothing about what is selectable, the palette, or the allocation rules changes — this is purely the starting position of the viewport.
Working on the opening position put me in front of the grid for long enough to find two defects that have nothing to do with focus, and both of them make cells read as absent. They are described in their own section below, after the focus work.
What each surface opens on
The four surfaces share one component, so they share one resolver. They differ only in which sessions their
SlotPickerSubjectcarries, and the ordering below answers all of them without any surface growing its own private notion of "the thing you clicked".unscheduled-class-<id>,unscheduled-webinar-<id>, or a request with no requested times)Why the multi-session case needed a decision
A subscription is N sessions spread over weeks or months, each on its own
Appointmentrow, and the picker shows one week. "The booking" is therefore not a single point on the grid, and the choice between candidate targets is a product decision rather than something an implementation should guess at.The rule adopted here is that a session still awaiting a time outranks everything else. On a partly-placed program the consultant's job is filling the gaps, not admiring the sessions that are already booked, so the released session is the one they came for even when a confirmed session falls earlier in the calendar. Only when nothing is awaiting a time does the next upcoming session win, because that is what a consultant ordinarily means by "this booking" — the thing about to affect their week. The most recent past session is the last resort, and it applies only once the scheduling period has closed as well. A program whose placed sessions have all run but whose period still has months left has sessions left to place, and they can only go in the future, so opening on a dead week would be the wrong answer twice over.
An earlier revision of this description added a second reason for that last rule: opening on a dead week would also stretch the allocate-mode availability request from that week to the end of the period, on the endpoint #997 measured in tens of seconds. The mechanism in that note was right and the consequence was wrong. The stretch was not merely slow — it is the source of the blanking described below, and the first defect in the next section removes it outright. The allocate request is now one week wide wherever the consultant is standing, so the ordering rule above no longer carries any performance argument at all; it stands on the product reasoning alone.
Two other questions the issue left open are settled the same way. Far-future targets move the week as well as the scroll: a consultant who opens Manage Timings for a session in September wants to see September, and silently staying on this week while claiming to have focused would be worse than jumping. And focus fires once, on first open only. It is deliberately not keyed to the release selection on a Reschedule page, because yanking the grid back while somebody is reading it is worse than the empty night rows this replaces.
The parts that were fiddly
The target is an instant, but the grid is drawn in wall-clock time, so the row has to be read in the same timezone the columns were built in or the focus lands off by the UTC offset — the same class of defect as the
datetime-localbug fixed in #1064.focusGridPositiontherefore takes the timezone explicitly and the calendar passes the zone it actually draws in. Worth noting for review: the week grid builds its cells withsetHourson a localDateand the footer prints the browser's zone, so that zone — not the event'sschedulingTimezone, which buckets day and week limits under ADR B9 — is the one the columns exist in. Choosing the row in the scheduling zone would have introduced exactly the offset the issue warns about whenever the two disagree.Idempotence is held in a ref rather than in effect dependencies. This component has already produced a React #185 "Maximum update depth exceeded" loop from callback identity in an effect's deps, and the fix there was a ref; a deps-driven focus would become the same thing. The scroll also runs only once availability has settled, measures the row rather than re-deriving a Tailwind row height, and backs off entirely if the user has already scrolled — it never fights somebody who got there first.
The vertical anchor for an offering with nothing scheduled is the consultant's earliest published availability rather than 00:00, since a scheduling-window bound carries no meaningful time of day and their working day is where the grid has anything to offer at all.
Two defects that make cells read as absent
Neither of these is caused by the focus work, and neither is fixed by it. They are here because opening the picker on the session you clicked is what put a real Manage Timings grid on screen often enough for the symptoms to become obvious.
The fetch window was clamped to the scheduling period; the grid never was
On Manage Timings for a subscription or a class, walking forward through the weeks made cells disappear — and crucially the already-booked ones disappeared too, which is what rules out a per-week cap or a disabled state as the explanation. One week past the end of the scheduling period, the whole grid went blank. Pressing
‹brought it all back.The grid always renders seven columns. The availability request, in allocate mode, stopped at
allowedEnd. Every cell past that end therefore had no server row to render from, and because the route filters appointments through the same window, the appointments in that range were dropped as well — which is exactly why a booked cell vanished in the same way an available one did. Navigate one week further andendDatefalls beforestartDate; the server has no range to scan, the route returns an empty payload, and every cell on the screen blanks at once.The fix is to delete the clamp, so the window is always the visible week or month. The comment sitting directly above it already argued that the START must not be clamped, precisely so a pre-period week shows the consultant's real availability behind an "Outside Period" label rather than a row of blanks. That argument was always true of the end as well and had simply never been applied there.
allowedStartandallowedEndgovern what is SELECTABLE — the range guard inhandleSlotClickand the "Outside Period" label already enforce that, and they are untouched — and must not govern what is VISIBLE. As a side effect this caps an allocate request at one week instead of up to a year, which is the perf note discussed above, now moot.disabled:opacity-50was cancelling the palette at render timeSLOT_CELL_BASE_CLASSended withdisabled:pointer-events-none disabled:opacity-50. Everyunavailablecell is disabled, sobg-slate-200reached the screen at half strength over a white card — near enough toslate-100to reproduce the exact near-white this file's own comment records as REVERTED for making a sparse week read as an empty grid rather than a full one with little availability.This is the second symptom the #1064 palette work never explained. The Tailwind
contentcause identified there was real and is fixed; this is a separate cause, downstream of the tokens, undoing them at paint time. It is also why the legend and the grid disagreed even after the tokens were unified: a legend swatch is a 12px block, not a disabled control, so the legend showed the trueslate-200while the grid showed half of it.disabled:opacity-50is gone anddisabled:pointer-events-nonestays — not being clickable was the part that was actually wanted. Where a fade is genuinely intended,renderTimeCelland thefadedoption append an explicitopacity-50,opacity-60oropacity-70for past cells; those are deliberate, they were checked first, and they are unchanged.Two smaller things in the same hook
An availability response of an unreadable shape used to
console.warnand set the slots to empty. An empty grid is a perfectly valid answer for a quiet week, so that is indistinguishable from success to the consultant looking at it — the same "a blank grid reads as a real answer" failure as the defect above. It sets an error now. Separately, the loading spinner'sfinallywas not request-scoped, so a stale reply could clear the spinner a newer request was still waiting on; the success and error paths were already guarded by the request id, and thefinallynow is too.Data
No schema changes, no new queries. Manage Timings and Allocate now carry their existing session rows through to the subject: the allocate read already selected
slotsOfAppointmentto derivehasReleasedSlotsand simply selects a few more columns off the same rows, and the manage-timings read already had the appointment and its siblings in hand.Verification
Type check, lint and the full Jest suite pass:
tsc --noEmitclean,eslintclean on every changed file, and 196 suites / 2208 tests green, of which 4 suites / 43 tests are new.The resolver tests cover a one-session consultation, a partly-scheduled subscription where the released session beats an earlier upcoming one, a fully-scheduled program, an unscheduled class carrying only a scheduling period, and the timezone case proving the row and the calendar date are both read in the zone passed in rather than the viewer's. A second suite covers the focus effect itself — that it fires when the grid appears late, and that it never re-aims a grid the consultant has already scrolled. A third pins both widened reads to their five-field allowlist, including a whole-payload assertion that no attendee name or recording URL crosses the RSC boundary.
The two defects get assertions of their own. A new suite drives the calendar hook and asserts on the ARGUMENTS of the availability fetch, because nothing about a rendered cell could ever have caught a bug in the window that was requested: the allocate window must end at the end of the visible week rather than at
allowedEnd, a week starting after the period closes must still request a forward range rather than an inverted one, and the request must span one week rather than the remainder of the period. The palette suite gains an assertion that the base cell class carries nodisabled:opacity-*at all, with a comment recording why the existing swatch-equality tests could never have seen it — they compare token strings, and a variant that fades the grid but not the legend is invisible to a string comparison. All four of these were verified to fail against the pre-fix code, as were the two review-follow-up suites.I have not verified this in a browser. The scroll is asserted against a rendered grid in jsdom with the row heights stubbed, so the arithmetic and the once-only behaviour are covered, but nobody has watched the real grid land where it should, and nobody has watched a booked cell survive a walk past the end of a scheduling period.
Closes #1073