fix: show schedule exception reasons in the slot tooltip#16426
fix: show schedule exception reasons in the slot tooltip#16426Valyrian-Code wants to merge 7 commits into
Conversation
computeAppointmentSlots dropped conflicting slots and always returned isAvailable=true / exceptions=[], so ScheduleHome's hasExceptions was always false and the slot tooltip rendered empty. Attach overlapping exceptions to each slot and mark conflicting ones unavailable; the available-slot count is unchanged (filter(isAvailable)) and the tooltip now shows the exception reasons. See issue ohcnetwork#16415.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughcomputeAppointmentSlots now pre-parses exceptions, attaches overlapping exceptions to each generated slot (setting isAvailable accordingly), and accepts includeUnavailableSlots to optionally return unavailable slots; ScheduleHome requests unavailable slots for tooltip rendering. ChangesSlot Exception Filtering and Attachment
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates appointment slot computation to attach overlapping exceptions to each slot and mark slots as unavailable instead of skipping them.
Changes:
- Replace per-slot conflict detection loop with an
exceptions.filter(...)to gather overlapping exceptions. - Always push slots, using
isAvailablederived from whether overlaps exist. - Populate each slot’s
exceptionsarray with the overlaps found.
Greptile SummaryThis PR fixes a bug where schedule exception reasons were never shown in the slot tooltip on the Availability view. The root cause was that
Confidence Score: 5/5Safe to merge — the change is a small, self-contained fix with no side-effects on the available-slot count or other callers. The diff is minimal and the logic is correct: every slot is now emitted with the right No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "fix: surface schedule exception reasons ..." | Re-trigger Greptile |
🚀 Preview Deployment Ready!🔗 Preview URL: https://pr-16426.care-preview-a7w.pages.dev 📱 Mobile Access: This preview will be automatically updated when you push new commits to this PR. |
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #9486 |
Addresses review feedback on computeAppointmentSlots: - Behavior compatibility: returning unavailable slots is now opt-in via `includeUnavailableSlots` (default false = previous "available slots only" behavior). ScheduleHome passes true so the tooltip can read the overlapping exceptions; any other caller is unaffected. - Performance: parse each exception's time window once outside the loop, determine availability with some() (early-exit on first overlap), and only build the overlapping-exception list for unavailable slots that are actually returned.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/components/Schedule/ScheduleHome.tsx (1)
447-449: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueOptional: dedupe exceptions by
idrather than object identity.
new Set(computedSlots.flatMap((slot) => slot.exceptions))currently dedupes correctly only becausecomputeAppointmentSlotsre-emits the sameScheduleExceptionreferences across slots. This couples this component to an implementation detail of the util; if it ever returns cloned objects, duplicates would slip through. Deduping byidis more robust and also collapses entries that genuinely share an id.♻️ Proposed change
- const exceptions = [ - ...new Set(computedSlots.flatMap((slot) => slot.exceptions)), - ]; + const exceptions = [ + ...new Map( + computedSlots.flatMap((slot) => slot.exceptions).map((e) => [e.id, e]), + ).values(), + ];🤖 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 `@src/components/Schedule/ScheduleHome.tsx` around lines 447 - 449, The current dedupe for exceptions uses object identity which can fail if computeAppointmentSlots returns cloned ScheduleException objects; update the exceptions construction to dedupe by exception.id instead: iterate computedSlots.flatMap(slot => slot.exceptions) and collapse into a Map keyed by exception.id (or use a reducer keyed by id) to keep the first occurrence for each id, then take the Map values as the exceptions array; reference the computedSlots variable and the exceptions constant to locate where to replace the Set-based dedupe.
🤖 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 `@src/pages/Scheduling/utils.ts`:
- Around line 91-108: Extract the overlap predicate into a single reusable
function/closure (e.g., overlaps = ({start, end}) => start < slotEndTime && end
> time) and use it both to compute isAvailable (isAvailable =
!parsedExceptions.some(overlaps)) and to build the exceptions list
(parsedExceptions.filter(overlaps).map(({ exception }) => exception)) so the
availability check and attached exceptions cannot diverge; keep references to
parsedExceptions, isAvailable, includeUnavailableSlots, slots, start_time and
end_time when making the change.
- Around line 79-83: The parsedExceptions mapping in utils.ts currently always
parses exception.start_time/end_time with date-fns parse using "HH:mm:ss", which
yields Invalid Date for inputs like "HH:mm"; update the logic in the
exceptions.map block that builds parsedExceptions to detect whether each time
string includes seconds (e.g., contains two colons) or to normalize the string
by appending ":00" when seconds are absent, then call parse with the matching
format ("HH:mm" or "HH:mm:ss") (or always parse the normalized "HH:mm:ss") and
validate the resulting Date objects (reference the parsedExceptions variable,
exception.start_time, exception.end_time, parse, and referenceDate) so overlap
checks aren’t broken by Invalid Date values.
---
Outside diff comments:
In `@src/components/Schedule/ScheduleHome.tsx`:
- Around line 447-449: The current dedupe for exceptions uses object identity
which can fail if computeAppointmentSlots returns cloned ScheduleException
objects; update the exceptions construction to dedupe by exception.id instead:
iterate computedSlots.flatMap(slot => slot.exceptions) and collapse into a Map
keyed by exception.id (or use a reducer keyed by id) to keep the first
occurrence for each id, then take the Map values as the exceptions array;
reference the computedSlots variable and the exceptions constant to locate where
to replace the Set-based dedupe.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0d5674f-4150-4319-8aac-927a14e5eead
📒 Files selected for processing (2)
src/components/Schedule/ScheduleHome.tsxsrc/pages/Scheduling/utils.ts
Address review feedback: - Compute each slot's overlapping exceptions in a single filter pass and derive isAvailable from its length, instead of a some() check followed by a separate filter for unavailable slots. - Extract a named ComputeAppointmentSlotsOptions interface for the options parameter instead of an inline anonymous type.
The exception time inputs produce HH:mm values (e.g. "00:00"/"23:59"), so hardcoding parse(..., "HH:mm:ss") would yield Invalid Date and break the overlap checks. Add a parseScheduleTime helper that selects the format by length, and use it for both availability and exception times. (The single-pass overlap from the previous commit already keeps isAvailable and the attached exceptions derived from one computation.)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/pages/Scheduling/utils.ts:1
- When
includeUnavailableSlotsis false (the default), this still buildsslotExceptionsby filtering/mapping the full exception list for every slot. Previously the code could short-circuit on the first conflict. Consider splitting the logic: useparsedExceptions.some(...)(early-exit) to computeisAvailablewhen unavailable slots are not needed, and only build the fullslotExceptionsarray whenincludeUnavailableSlotsis true. This preserves the new behavior while avoiding a potential per-slot performance regression.
import {
Use value.trim().split(":").length to choose HH:mm vs HH:mm:ss so format
selection is structural (and tolerant of surrounding whitespace) rather
than relying on the brittle string-length heuristic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/pages/Scheduling/utils.ts`:
- Around line 55-63: parseScheduleTime currently returns the result of
parse(...) without validating it, allowing "Invalid Date" to propagate; update
parseScheduleTime to validate the parsed Date (similar to getDurationInMinutes)
and throw a clear error or return a controlled failure when parse yields an
invalid date. Locate parseScheduleTime and ensure after calling parse(trimmed,
pattern, referenceDate) you check for Invalid Date (or use isValid(parsed)) and
handle it (throw new Error with the original value and pattern or return a
defined sentinel) so callers won't silently proceed with invalid dates.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 075a2b29-6064-4a85-b807-cd776b95aa5b
📒 Files selected for processing (1)
src/pages/Scheduling/utils.ts
nihal467
left a comment
nihal467
left a comment
There was a problem hiding this comment.
Add the screenshot of the changes made in the PR
ohc-project-manager
Bot
added
Changes Required
and removed
needs review
needs testing
labels
Fixes #16415
Proposed Changes
On the user Availability / Schedule view, when a schedule exception reduces a session's available slots, the slot line is shown struck-through with a tooltip trigger — but hovering shows an empty tooltip; the exception reason is never displayed.
Root cause —
computeAppointmentSlots(src/pages/Scheduling/utils.ts) drops conflicting slots and always returnsisAvailable: true, exceptions: []:So every slot has an empty
exceptions, andScheduleHomecomputeshasExceptionsfromcomputedSlots.flatMap((s) => s.exceptions)— which is therefore alwaysfalse, so theTooltipContentwith the reasons never renders. TheVirtualSlot.isAvailable/.exceptionsfields the consumer relies on were effectively dead.Fix — attach the overlapping exception(s) to each slot and mark conflicting slots unavailable instead of dropping them:
ScheduleHomeis the only consumer; it readsslot.isAvailable(filtered for the count) andslot.exceptions(for the tooltip). The available-slot count is unchanged (filter(isAvailable)), and the tooltip now shows the reason(s).Reproduce
Exceptions: <reason>(previously empty).Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
userScheduleCreationspec still passes (slot count unchanged). Happy to add an E2E if you can advise on a non-flaky approach for the tooltip (see Schedule exception reasons never shown: computeAppointmentSlots leaves tooltip empty #16415).exceptionskey + exception reasons.)Summary by CodeRabbit
New Features
Bug Fixes