feat: update roster to add shifts in holidays and leaves#4123
feat: update roster to add shifts in holidays and leaves#4123sarathibalamurugan wants to merge 1 commit intofrappe:developfrom
Conversation
sarathibalamurugan
requested review from
asmitahase and
ruchamahabal
as code owners
|
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:
WalkthroughMonthViewTable.vue was refactored to use per-date DateEvents objects 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
roster/src/components/MonthViewTable.vue (3)
373-380:⚠️ Potential issue | 🔴 CriticalBug:
hasSameShiftchecks the wrong data path after the refactor.After the restructure,
events.data[employee][day]is an object{ holiday?, leave?, shift? }, not an array.Array.isArray(...)will always returnfalse, sohasSameShiftwill never detect a duplicate shift — meaning drag-and-drop onto a cell with an identical shift will no longer be blocked.🐛 Proposed fix
const hasSameShift = (employee: string, day: string) => - Array.isArray(events.data?.[employee]?.[day]) && - events.data?.[employee]?.[day].some( + Array.isArray(events.data?.[employee]?.[day]?.shift) && + events.data?.[employee]?.[day].shift.some( (shift: Shift) => shift.shift_type === hoveredCell.value.shift_type && shift.shift_location === hoveredCell.value.shift_location && shift.status === hoveredCell.value.shift_status, );
502-507:⚠️ Potential issue | 🔴 CriticalBug:
sortShiftsByStartTimeoperates on the date entry object instead of the.shiftarray.Same root cause as
hasSameShift: after the refactor,mappedEvents[employee][key]is{ holiday?, leave?, shift? }. TheArray.isArraycheck always fails, so shifts are never sorted.🐛 Proposed fix
const sortShiftsByStartTime = (mappedEvents: MappedEvents, employee: string, key: string) => { - if (Array.isArray(mappedEvents[employee][key])) - mappedEvents[employee][key].sort((a: Shift, b: Shift) => + const entry = mappedEvents[employee][key]; + if (entry && typeof entry === "object" && "shift" in entry && Array.isArray(entry.shift)) + entry.shift.sort((a: Shift, b: Shift) => a.start_time.localeCompare(b.start_time), ); };
316-316: 🛠️ Refactor suggestion | 🟠 Major
MappedEventstype no longer matches the actual runtime structure.The type is
Record<string, Record<string, Holiday | Leave | Shift[]>>, but after the refactor each date entry is an object with optionalholiday,leave, andshiftkeys. This type mismatch suppresses useful compiler checks and is the likely reason the two bugs above weren't caught.♻️ Suggested type update
-type MappedEvents = Record<string, Record<string, Holiday | Leave | Shift[]>>; +interface DateEvents { + holiday?: Holiday; + leave?: Leave; + shift?: Shift[]; +} +type MappedEvents = Record<string, Record<string, DateEvents>>;
sarathibalamurugan
force-pushed
the
feat/issue-3615
branch
from
81646d6 to
7f03fa4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@roster/src/components/MonthViewTable.vue`:
- Around line 108-115: The template uses v-html to render
events.data[employee.name][day.date].holiday.description which opens an XSS
vector; replace the v-html usage in MonthViewTable.vue with safe text
interpolation and conditional rendering: remove v-html and render
holiday.description inside {{ }} (or a sanitized text helper) and render the
weekly_off marker as a separate conditional element (e.g., a <strong>WO</strong>
shown via v-if/v-else) so no raw HTML from holiday.description is injected;
ensure you reference the same keys (events.data, employee.name, day.date,
holiday.description, weekly_off) when updating the template and, if HTML must be
allowed, run the description through a sanitizer utility before rendering.
🧹 Nitpick comments (1)
roster/src/components/MonthViewTable.vue (1)
118-124: Leave indicator is suppressed when a holiday is also present on the same date.Using
v-else-ifmeans a date with both a holiday and a leave only shows the holiday badge. If this is intentional (holiday takes precedence), it's fine — just calling it out for confirmation.
sarathibalamurugan
force-pushed
the
feat/issue-3615
branch
from
7f03fa4 to
c6a3fc3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
roster/src/components/MonthViewTable.vue (1)
507-511: Overly defensive guards insortShiftsByStartTime.Given that
mappedEvents[employee][key]is always initialized as{}on line 443, theentry && typeof entry === "object"checks are redundant. TheArray.isArray(entry.shift)check alone suffices. This is a minor nit.Simplified guard
const sortShiftsByStartTime = (mappedEvents: MappedEvents, employee: string, key: string) => { - const entry = mappedEvents[employee][key]; - if (entry && typeof entry === "object" && "shift" in entry && Array.isArray(entry.shift)) - entry.shift.sort((a: Shift, b: Shift) => a.start_time.localeCompare(b.start_time)); + const shifts = mappedEvents[employee][key]?.shift; + if (Array.isArray(shifts)) + shifts.sort((a: Shift, b: Shift) => a.start_time.localeCompare(b.start_time)); };
sarathibalamurugan
force-pushed
the
feat/issue-3615
branch
from
c6a3fc3 to
d57d922
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@roster/src/components/MonthViewTable.vue`:
- Around line 93-99: The swap_shift handler currently allows assigning shifts on
dates that may be holidays or in an employee's approved leave; update swap_shift
to validate against those business rules before performing any operation and to
return a clear error when conflicts exist. Specifically, before calling
break_shift or insert_shift, query the employee's holiday calendar and any
approved leave/applications for the target date(s) and if any match, abort and
return a 4xx error (with a message like "cannot assign shift during
holiday/approved leave"); ensure break_shift and insert_shift are not executed
when a conflict is detected and add corresponding unit/error-path handling so
callers receive the rejection.
🧹 Nitpick comments (2)
roster/src/components/MonthViewTable.vue (2)
378-385: Inconsistent optional chaining inhasSameShift.Line 379 uses
?.[day]?.shiftbut Line 380 drops to?.[day].shift.some(...). Safe due to&&short-circuiting, but inconsistent and fragile if the expression is ever refactored (e.g., extracted or reordered).Suggested fix
const hasSameShift = (employee: string, day: string) => Array.isArray(events.data?.[employee]?.[day]?.shift) && - events.data?.[employee]?.[day].shift.some( + events.data?.[employee]?.[day]?.shift?.some( (shift: Shift) => shift.shift_type === hoveredCell.value.shift_type && shift.shift_location === hoveredCell.value.shift_location && shift.status === hoveredCell.value.shift_status, );
507-511: Overly defensive guard insortShiftsByStartTime.Given
mappedEvents[employee][key]is always initialized as{}on Line 443 and.shiftis only ever set as an array, thetypeof entry === "object"and"shift" in entrychecks are redundant. TheArray.isArraycheck alone suffices.Simplified version
const sortShiftsByStartTime = (mappedEvents: MappedEvents, employee: string, key: string) => { - const entry = mappedEvents[employee][key]; - if (entry && typeof entry === "object" && "shift" in entry && Array.isArray(entry.shift)) - entry.shift.sort((a: Shift, b: Shift) => a.start_time.localeCompare(b.start_time)); + const shifts = mappedEvents[employee][key]?.shift; + if (Array.isArray(shifts)) + shifts.sort((a: Shift, b: Shift) => a.start_time.localeCompare(b.start_time)); };
sarathibalamurugan
force-pushed
the
feat/issue-3615
branch
from
d57d922 to
0b3f06a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
roster/src/components/MonthViewTable.vue (1)
444-444:Object.values()called on an array — use direct iteration instead.
data[employee]is typed as(HolidayWithDate | LeaveApplication | ShiftAssignment)[]. Wrapping it inObject.values()works (returns the same elements for dense arrays) but is unidiomatic, adds a needless allocation, and loses direct array-type inference.♻️ Proposed refactor
- for (const event of Object.values(data[employee])) { + for (const event of data[employee]) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roster/src/components/MonthViewTable.vue` at line 444, The loop currently iterates with Object.values(data[employee]) which is unnecessary because data[employee] is already an array; change the iteration to iterate directly over the array (use data[employee] in the for...of) so you avoid the extra allocation and preserve array-type inference for types like HolidayWithDate | LeaveApplication | ShiftAssignment; update the loop in MonthViewTable.vue where the code uses Object.values(data[employee]) to use direct iteration (for (const event of data[employee])) and ensure any downstream code relying on the previous Object.values result still works with an array element (event) as typed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@roster/src/components/MonthViewTable.vue`:
- Around line 119-124: The template currently uses v-else-if on the leave block
so a leave is hidden when a holiday is present; update the template to render
both indicators by removing the exclusive v-else-if and using independent
conditionals (e.g., change
v-else-if="events.data?.[employee.name]?.[day.date]?.leave" to v-if or a
separate element) so that events.data[employee.name][day.date].holiday and
events.data[employee.name][day.date].leave can both display; ensure any CSS for
.blocked-cell or the new element keeps the layout readable.
---
Duplicate comments:
In `@roster/src/components/MonthViewTable.vue`:
- Around line 108-115: The template currently uses v-html with
events.data[employee.name][day.date].holiday.description which is an XSS risk;
replace the v-html binding with a safe text binding (mustache interpolation or
v-text) so the description is rendered as escaped plain text, e.g. bind to
events.data[employee.name][day.date].holiday.description via {{ ... }} or
v-text; if HTML rendering is required, instead pass the description through a
sanitizer utility (e.g., DOMPurify) in a computed or method like
sanitizeHolidayDescription(description) and bind the sanitized result,
referencing the same events.data[...]holiday.description symbol.
- Around line 93-99: The swap_shift flow removed the client-side
isHolidayOrLeave guard but the swap_shift endpoint still lacks server-side
validation, so add a validation in the swap_shift request handler to reject
swaps that would place a shift on an employee's approved leave or a holiday:
call the existing isHolidayOrLeave (or the domain function that checks
leaves/holidays) for the target employee/date inside the swap_shift handler
before applying changes, return a 4xx error with a clear message when it
conflicts, and ensure any callers (e.g., the frontend swapShift.submit() path)
surface that error to the user; also keep hasSameShift logic as a short-circuit
to avoid unnecessary validation if no change is happening.
---
Nitpick comments:
In `@roster/src/components/MonthViewTable.vue`:
- Line 444: The loop currently iterates with Object.values(data[employee]) which
is unnecessary because data[employee] is already an array; change the iteration
to iterate directly over the array (use data[employee] in the for...of) so you
avoid the extra allocation and preserve array-type inference for types like
HolidayWithDate | LeaveApplication | ShiftAssignment; update the loop in
MonthViewTable.vue where the code uses Object.values(data[employee]) to use
direct iteration (for (const event of data[employee])) and ensure any downstream
code relying on the previous Object.values result still works with an array
element (event) as typed.
56b58f8 to
bdb79bd
Compare
sarathibalamurugan
force-pushed
the
feat/issue-3615
branch
2 times, most recently
from
4914973 to
3b6e0e6
Compare
sarathibalamurugan
force-pushed
the
feat/issue-3615
branch
2 times, most recently
from
d865a76 to
dcf2362
Compare
sarathibalamurugan
force-pushed
the
feat/issue-3615
branch
from
dcf2362 to
f473346
Compare
1 participant
Description:
Currently in Roster we can't assign shifts in holidays and leave. Now we can assign shifts in holidays and leaves seamlessly.
ref: 49986
Before:
RosterBefore.webm
After:
RosterAfter.webm
Backport needed for: v15, v16
Summary by CodeRabbit
Bug Fixes
Refactor