Ck 7vn/split3#1133
Merged
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughDecomposes SessionDetailSheet into reusable sub-components, refactors SessionsTab to delegate filtering and modal UI to new components, and adds infrastructure for session row rendering and filtering. Supporting changes improve content navigation, update type contracts, and enhance formatters. ChangesSession Detail UI Decomposition and Sessions Tab Refactoring
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/pages/class-detail/index.tsx (1)
88-97: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReuse
DeleteBlockersin the SWR response type to avoid drift.
blockersis re-declared inline here even thoughDeleteBlockerswas introduced above. Reusing the interface keeps one source of truth.Suggested fix
const { data: deleteCheckResp, mutate: mutateDeleteCheck } = useSWR< ServerResponseOne<{ can_delete: boolean; - blockers: { - enrollments?: number; - completions?: number; - attendance_flags?: number; - non_deletable_status?: string; - }; + blockers: DeleteBlockers; }> >(class_id ? `/api/program-classes/${class_id}/delete-check` : null);🤖 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 `@frontend/src/pages/class-detail/index.tsx` around lines 88 - 97, The SWR response type re-declares the blockers shape instead of reusing the existing DeleteBlockers type; update the generic passed to useSWR in the delete-check hook so ServerResponseOne references DeleteBlockers (e.g., useSWR<ServerResponseOne<DeleteBlockers>> or ServerResponseOne<{ can_delete: boolean; blockers: DeleteBlockers }>) so the deleteCheckResp / mutateDeleteCheck types derive from the single DeleteBlockers definition used elsewhere.frontend/src/pages/knowledge-center/VideoViewer.tsx (1)
28-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle rejected API calls so loading state always resolves.
If
API.getthrows,isLoadingnever flips tofalse, leaving the page stuck in skeleton state. Addtry/catch/finallyaround Line 28.Suggested fix
const fetchVideoData = async () => { - const resp = await API.get<Video>(`videos/${videoId}`); - if (resp.success && resp.type === 'one') { - setIsLoading(false); - setVideo(resp.data); - } else { - setError(resp.message); - setIsLoading(false); - } + try { + const resp = await API.get<Video>(`videos/${videoId}`); + if (resp.success && resp.type === 'one') { + setVideo(resp.data); + setError(null); + } else { + setError(resp.message || 'Video currently unavailable'); + } + } catch { + setError('Video currently unavailable or unauthorized to view'); + } finally { + setIsLoading(false); + } };🤖 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 `@frontend/src/pages/knowledge-center/VideoViewer.tsx` around lines 28 - 35, The API.get call in the VideoViewer component can throw and leave isLoading true; wrap the call to API.get<Video>(`videos/${videoId}`) inside a try/catch/finally block (within the async effect or handler where this code lives) so that on success you call setVideo(resp.data) and on failure you call setError(err?.message || resp?.message) in the catch, and always call setIsLoading(false) in the finally; reference the VideoViewer component and the state setters setIsLoading, setVideo, setError and the API.get invocation when making the change.
🤖 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 `@frontend/src/components/dashboard/TopContentList.tsx`:
- Around line 25-27: Replace the interactive <div> wrapper in the TopContentList
component with a semantic <button type="button"> to restore keyboard focus and
assistive tech support: change the element that currently uses
onClick={handleClick} and the className "flex items-center gap-3 p-3 rounded-lg
border border-border bg-card hover:shadow-md transition-shadow cursor-pointer"
to a <button type="button"> keeping the same className and onClick handler
(ensure it is not nested inside another button), and remove any non-button-only
attributes if present so the element behaves like a proper button for keyboard
users.
In `@frontend/src/pages/class-detail/ClassNotFoundCard.tsx`:
- Line 9: The flex container in ClassNotFoundCard (the div with className
"bg-[`#E2E7EA`] flex items-center justify-center") lacks a defined height so
items-center has no effect; update that container to include an appropriate
height utility (e.g., h-full, min-h-screen, or a fixed height like
h-96/min-h-[200px] depending on the layout) so the content will actually be
vertically centered.
In `@frontend/src/pages/class-detail/LoadingSkeleton.tsx`:
- Around line 11-21: The skeleton grids use fixed column counts causing overflow
on small screens; update the container div classNames in LoadingSkeleton.tsx to
use responsive Tailwind grid classes (e.g., replace "grid grid-cols-5 gap-4"
with something like "grid grid-cols-1 sm:grid-cols-2 md:grid-cols-3
lg:grid-cols-5 gap-4" for the five-item row, and replace "grid grid-cols-3 gap-6
mb-6" with "grid grid-cols-1 sm:grid-cols-2 md:grid-cols-3 gap-6 mb-6" for the
three-item row) so the Skeleton components resize and wrap appropriately.
In `@frontend/src/pages/class-detail/session-utils.ts`:
- Around line 517-533: The current lookup uses findActiveOverride(classEvents,
session.instance.date) which matches by date only and can pick the wrong
override for sessions on the same day; update the call and matching logic to
consider the session start time as well: parse the session.instance.class_time
start (parts[0]) into a normalized time string or minutes and pass it into
findActiveOverride (e.g., findActiveOverride(classEvents, session.instance.date,
startTime)), then change findActiveOverride to require both date and start-time
equality when selecting an active override so room_id/instructor_id/override_id
resolution uses date+start-time instead of date-only.
In `@frontend/src/pages/class-detail/SessionRow.tsx`:
- Around line 107-110: The clickable row in SessionRow.tsx currently only uses
onClick and cursor-pointer, making it inaccessible to keyboard users; update the
outer div (the element with onClick and className that includes
getBorderClass()) to be focusable and announced as a button by adding
role="button" and tabIndex={0}, and add an onKeyDown handler that invokes the
same onClick callback when Enter or Space is pressed (ensure it prevents default
for Space so the page doesn't scroll). If the row can be disabled, mirror that
state with aria-disabled accordingly.
In `@frontend/src/pages/class-detail/useSessionFilters.ts`:
- Around line 80-84: The block handling statusFilter === 'upcoming' performs an
unnecessary reversal (result = [...result].reverse()) that is later undone by
the chronological sort in the upcomingSessions memo; remove that reverse line so
the filtered array remains in its natural order (keep the filter using
isUpcoming, isCancelled, and isRescheduledFrom) and ensure any length checks
that use filtered continue to work unchanged.
In `@frontend/src/pages/student/ResidentHome.tsx`:
- Around line 101-104: Replace the non-semantic, non-keyboard-operable div
wrapper with a semantic <button type="button"> that preserves the existing
onClick={handleClick} and className (so existing styles and handlers on the
element remain intact); ensure you remove any redundant role or tabIndex
previously added to the div, confirm there are no nested interactive controls
inside the button (or move them out) and if needed add an aria-label prop for
accessibility—locate the wrapper around the favorite item (the element using
handleClick in ResidentHome.tsx / FavoriteItem) and perform this replacement.
---
Outside diff comments:
In `@frontend/src/pages/class-detail/index.tsx`:
- Around line 88-97: The SWR response type re-declares the blockers shape
instead of reusing the existing DeleteBlockers type; update the generic passed
to useSWR in the delete-check hook so ServerResponseOne references
DeleteBlockers (e.g., useSWR<ServerResponseOne<DeleteBlockers>> or
ServerResponseOne<{ can_delete: boolean; blockers: DeleteBlockers }>) so the
deleteCheckResp / mutateDeleteCheck types derive from the single DeleteBlockers
definition used elsewhere.
In `@frontend/src/pages/knowledge-center/VideoViewer.tsx`:
- Around line 28-35: The API.get call in the VideoViewer component can throw and
leave isLoading true; wrap the call to API.get<Video>(`videos/${videoId}`)
inside a try/catch/finally block (within the async effect or handler where this
code lives) so that on success you call setVideo(resp.data) and on failure you
call setError(err?.message || resp?.message) in the catch, and always call
setIsLoading(false) in the finally; reference the VideoViewer component and the
state setters setIsLoading, setVideo, setError and the API.get invocation when
making the change.
🪄 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: c561dcac-acf6-4653-b9fc-fd70ba888551
📒 Files selected for processing (25)
frontend/src/components/dashboard/TopContentList.tsxfrontend/src/components/schedule/RescheduleSessionModal.tsxfrontend/src/components/schedule/SessionDetailActions.tsxfrontend/src/components/schedule/SessionDetailClassDetails.tsxfrontend/src/components/schedule/SessionDetailHeader.tsxfrontend/src/components/schedule/SessionDetailSheet.tsxfrontend/src/components/schedule/SessionDetailStatusSection.tsxfrontend/src/components/shared/StatusBadge.tsxfrontend/src/layouts/AuthenticatedLayout.tsxfrontend/src/lib/formatters.tsfrontend/src/pages/class-detail/ClassNotFoundCard.tsxfrontend/src/pages/class-detail/EnrollResidentsModal.tsxfrontend/src/pages/class-detail/LoadingSkeleton.tsxfrontend/src/pages/class-detail/SessionRow.tsxfrontend/src/pages/class-detail/SessionsTab.tsxfrontend/src/pages/class-detail/SessionsTabBulkActions.tsxfrontend/src/pages/class-detail/SessionsTabFilterBar.tsxfrontend/src/pages/class-detail/SessionsTabModals.tsxfrontend/src/pages/class-detail/index.tsxfrontend/src/pages/class-detail/session-utils.tsfrontend/src/pages/class-detail/useSessionFilters.tsfrontend/src/pages/knowledge-center/VideoViewer.tsxfrontend/src/pages/programs/ClassEvents.tsxfrontend/src/pages/student/ResidentHome.tsxfrontend/src/types/events.ts
Comment on lines
+25
to
+27
| <div | ||
| onClick={handleClick} | ||
| className="flex items-center gap-3 p-3 rounded-lg border border-border bg-card hover:shadow-md transition-shadow cursor-pointer" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find frontend -name "TopContentList.tsx" -type fRepository: UnlockedLabs/UnlockEdv2
Length of output: 120
🏁 Script executed:
cat -n frontend/src/components/dashboard/TopContentList.tsxRepository: UnlockedLabs/UnlockEdv2
Length of output: 3262
🏁 Script executed:
rg "onClick.*handleClick" frontend/src/components/dashboard/ -A 3 -B 1Repository: UnlockedLabs/UnlockEdv2
Length of output: 566
🏁 Script executed:
rg "<div[^>]*onClick" frontend/src/components/ --type tsx --type ts -cRepository: UnlockedLabs/UnlockEdv2
Length of output: 95
🏁 Script executed:
rg "<div[^>]*onClick" frontend/src/components/ -cRepository: UnlockedLabs/UnlockEdv2
Length of output: 49
🏁 Script executed:
rg "onKeyDown|onKeyPress|onKeyUp" frontend/src/components/dashboard/TopContentList.tsxRepository: UnlockedLabs/UnlockEdv2
Length of output: 49
Replace interactive div with semantic button element for keyboard accessibility.
Line 25 uses a non-semantic <div> for interactive content, which is not keyboard-focusable by default and blocks keyboard users from accessing this core navigation feature. Since the component already uses semantic buttons elsewhere (line 71), replace this div with a <button type="button"> element to provide proper keyboard and assistive technology support.
Suggested fix
- <div
+ <button
+ type="button"
onClick={handleClick}
- className="flex items-center gap-3 p-3 rounded-lg border border-border bg-card hover:shadow-md transition-shadow cursor-pointer"
+ className="w-full text-left flex items-center gap-3 p-3 rounded-lg border border-border bg-card hover:shadow-md transition-shadow cursor-pointer"
>
@@
- </div>
+ </button>🤖 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 `@frontend/src/components/dashboard/TopContentList.tsx` around lines 25 - 27,
Replace the interactive <div> wrapper in the TopContentList component with a
semantic <button type="button"> to restore keyboard focus and assistive tech
support: change the element that currently uses onClick={handleClick} and the
className "flex items-center gap-3 p-3 rounded-lg border border-border bg-card
hover:shadow-md transition-shadow cursor-pointer" to a <button type="button">
keeping the same className and onClick handler (ensure it is not nested inside
another button), and remove any non-button-only attributes if present so the
element behaves like a proper button for keyboard users.
|
|
||
| export function ClassNotFoundCard({ onBack }: ClassNotFoundCardProps) { | ||
| return ( | ||
| <div className="bg-[#E2E7EA] flex items-center justify-center"> |
There was a problem hiding this comment.
Add container height for actual vertical centering.
items-center won’t vertically center here without a defined height on the flex container, so the card will sit at the top instead of centered.
Suggested fix
- <div className="bg-[`#E2E7EA`] flex items-center justify-center">
+ <div className="bg-[`#E2E7EA`] min-h-screen flex items-center justify-center">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div className="bg-[#E2E7EA] flex items-center justify-center"> | |
| <div className="bg-[`#E2E7EA`] min-h-screen flex items-center justify-center"> |
🤖 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 `@frontend/src/pages/class-detail/ClassNotFoundCard.tsx` at line 9, The flex
container in ClassNotFoundCard (the div with className "bg-[`#E2E7EA`] flex
items-center justify-center") lacks a defined height so items-center has no
effect; update that container to include an appropriate height utility (e.g.,
h-full, min-h-screen, or a fixed height like h-96/min-h-[200px] depending on the
layout) so the content will actually be vertically centered.
Comment on lines
+11
to
+21
| <div className="grid grid-cols-5 gap-4"> | ||
| {Array.from({ length: 5 }).map((_, i) => ( | ||
| <Skeleton key={i} className="h-20 rounded-lg" /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div className="max-w-7xl mx-auto px-6 py-6"> | ||
| <div className="grid grid-cols-3 gap-6 mb-6"> | ||
| {Array.from({ length: 3 }).map((_, i) => ( | ||
| <Skeleton key={i} className="h-40 rounded-lg" /> |
There was a problem hiding this comment.
Use responsive grid columns for small screens.
The fixed 5/3-column skeleton grids can break on narrow widths. Making these responsive avoids layout overflow in loading state.
Suggested fix
- <div className="grid grid-cols-5 gap-4">
+ <div className="grid grid-cols-1 sm:grid-cols-2 lg:grid-cols-5 gap-4">
...
- <div className="grid grid-cols-3 gap-6 mb-6">
+ <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-6 mb-6">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div className="grid grid-cols-5 gap-4"> | |
| {Array.from({ length: 5 }).map((_, i) => ( | |
| <Skeleton key={i} className="h-20 rounded-lg" /> | |
| ))} | |
| </div> | |
| </div> | |
| </div> | |
| <div className="max-w-7xl mx-auto px-6 py-6"> | |
| <div className="grid grid-cols-3 gap-6 mb-6"> | |
| {Array.from({ length: 3 }).map((_, i) => ( | |
| <Skeleton key={i} className="h-40 rounded-lg" /> | |
| <div className="grid grid-cols-1 sm:grid-cols-2 lg:grid-cols-5 gap-4"> | |
| {Array.from({ length: 5 }).map((_, i) => ( | |
| <Skeleton key={i} className="h-20 rounded-lg" /> | |
| ))} | |
| </div> | |
| </div> | |
| </div> | |
| <div className="max-w-7xl mx-auto px-6 py-6"> | |
| <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-6 mb-6"> | |
| {Array.from({ length: 3 }).map((_, i) => ( | |
| <Skeleton key={i} className="h-40 rounded-lg" /> |
🤖 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 `@frontend/src/pages/class-detail/LoadingSkeleton.tsx` around lines 11 - 21,
The skeleton grids use fixed column counts causing overflow on small screens;
update the container div classNames in LoadingSkeleton.tsx to use responsive
Tailwind grid classes (e.g., replace "grid grid-cols-5 gap-4" with something
like "grid grid-cols-1 sm:grid-cols-2 md:grid-cols-3 lg:grid-cols-5 gap-4" for
the five-item row, and replace "grid grid-cols-3 gap-6 mb-6" with "grid
grid-cols-1 sm:grid-cols-2 md:grid-cols-3 gap-6 mb-6" for the three-item row) so
the Skeleton components resize and wrap appropriately.
Comment on lines
+517
to
+533
| const activeOverride = findActiveOverride(classEvents, session.instance.date); | ||
| const parts = session.instance.class_time.split('-'); | ||
| const [sh = 0, sm = 0] = (parts[0] ?? '').split(':').map(Number); | ||
| const [eh = 0, em = 0] = (parts[1] ?? '').split(':').map(Number); | ||
| const start = new Date(session.dateObj); | ||
| start.setHours(sh, sm, 0, 0); | ||
| const end = new Date(session.dateObj); | ||
| end.setHours(eh, em, 0, 0); | ||
| return { | ||
| id: eventId, | ||
| class_id: classId, | ||
| duration: backingEvent?.duration ?? '', | ||
| room_id: activeOverride?.room_id ?? backingEvent?.room_id ?? 0, | ||
| recurrence_rule: backingEvent?.recurrence_rule ?? '', | ||
| is_cancelled: session.instance.is_cancelled, | ||
| instructor_id: activeOverride?.instructor_id ?? backingEvent?.instructor_id ?? null, | ||
| overrides: backingEvent?.overrides ?? [], |
There was a problem hiding this comment.
Match overrides by date and start time when building the facility event.
Line 517 uses a date-only lookup, so sessions sharing the same date can resolve to the wrong override (room/instructor/override_id). This can drive incorrect modal payloads for cancel/change actions.
Suggested fix
- const activeOverride = findActiveOverride(classEvents, session.instance.date);
+ const sessionStart = (session.instance.class_time.split('-')[0] ?? '').trim();
+ const activeOverride = classEvents
+ .flatMap((e) => e.overrides ?? [])
+ .find((ov) => {
+ if (ov.is_cancelled) return false;
+ const ovDate = parseOverrideDate(ov.override_rrule);
+ if (ovDate !== session.instance.date) return false;
+ const ovStart = parseOverrideStartTime(ov.override_rrule);
+ return !ovStart || ovStart === sessionStart;
+ });🤖 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 `@frontend/src/pages/class-detail/session-utils.ts` around lines 517 - 533, The
current lookup uses findActiveOverride(classEvents, session.instance.date) which
matches by date only and can pick the wrong override for sessions on the same
day; update the call and matching logic to consider the session start time as
well: parse the session.instance.class_time start (parts[0]) into a normalized
time string or minutes and pass it into findActiveOverride (e.g.,
findActiveOverride(classEvents, session.instance.date, startTime)), then change
findActiveOverride to require both date and start-time equality when selecting
an active override so room_id/instructor_id/override_id resolution uses
date+start-time instead of date-only.
Comment on lines
+107
to
+110
| <div | ||
| onClick={onClick} | ||
| className={`flex items-center justify-between p-4 rounded-lg border transition-colors cursor-pointer ${getBorderClass()}`} | ||
| > |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider improving keyboard accessibility for the clickable row.
The row div uses onClick and cursor-pointer but lacks a role="button" attribute and keyboard event handler (onKeyDown). While the row contains interactive child elements, keyboard-only users cannot trigger the row's onClick action directly.
♿ Proposed accessibility enhancement
<div
onClick={onClick}
+ role="button"
+ tabIndex={0}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ onClick();
+ }
+ }}
className={`flex items-center justify-between p-4 rounded-lg border transition-colors cursor-pointer ${getBorderClass()}`}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div | |
| onClick={onClick} | |
| className={`flex items-center justify-between p-4 rounded-lg border transition-colors cursor-pointer ${getBorderClass()}`} | |
| > | |
| <div | |
| onClick={onClick} | |
| role="button" | |
| tabIndex={0} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| onClick(); | |
| } | |
| }} | |
| className={`flex items-center justify-between p-4 rounded-lg border transition-colors cursor-pointer ${getBorderClass()}`} | |
| > |
🤖 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 `@frontend/src/pages/class-detail/SessionRow.tsx` around lines 107 - 110, The
clickable row in SessionRow.tsx currently only uses onClick and cursor-pointer,
making it inaccessible to keyboard users; update the outer div (the element with
onClick and className that includes getBorderClass()) to be focusable and
announced as a button by adding role="button" and tabIndex={0}, and add an
onKeyDown handler that invokes the same onClick callback when Enter or Space is
pressed (ensure it prevents default for Space so the page doesn't scroll). If
the row can be disabled, mirror that state with aria-disabled accordingly.
Comment on lines
+80
to
+84
| } else if (statusFilter === 'upcoming') { | ||
| result = result.filter( | ||
| (s) => s.isUpcoming && !s.isCancelled && !s.isRescheduledFrom | ||
| ); | ||
| result = [...result].reverse(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Redundant reverse operation.
Line 84 reverses the upcoming filter results, but line 106 later sorts upcomingSessions chronologically, effectively undoing the reverse. Since filtered is only used for length checks (not direct iteration), the reverse appears unnecessary.
🔧 Simplification suggestion
} else if (statusFilter === 'upcoming') {
result = result.filter(
(s) => s.isUpcoming && !s.isCancelled && !s.isRescheduledFrom
);
- result = [...result].reverse();
} else if (statusFilter === 'cancelled') {The upcomingSessions memo already handles sorting, so this reverse is redundant.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (statusFilter === 'upcoming') { | |
| result = result.filter( | |
| (s) => s.isUpcoming && !s.isCancelled && !s.isRescheduledFrom | |
| ); | |
| result = [...result].reverse(); | |
| } else if (statusFilter === 'upcoming') { | |
| result = result.filter( | |
| (s) => s.isUpcoming && !s.isCancelled && !s.isRescheduledFrom | |
| ); |
🤖 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 `@frontend/src/pages/class-detail/useSessionFilters.ts` around lines 80 - 84,
The block handling statusFilter === 'upcoming' performs an unnecessary reversal
(result = [...result].reverse()) that is later undone by the chronological sort
in the upcomingSessions memo; remove that reverse line so the filtered array
remains in its natural order (keep the filter using isUpcoming, isCancelled, and
isRescheduledFrom) and ensure any length checks that use filtered continue to
work unchanged.
Comment on lines
+101
to
104
| <div | ||
| onClick={handleClick} | ||
| className="flex items-center gap-3 p-3 rounded-lg border border-border hover:shadow-md transition-shadow cursor-pointer" | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get an overview of its structure
cd frontend/src/pages/student && wc -l ResidentHome.tsxRepository: UnlockedLabs/UnlockEdv2
Length of output: 88
🏁 Script executed:
# Read the specific lines mentioned (101-104) with context
sed -n '85,120p' frontend/src/pages/student/ResidentHome.tsxRepository: UnlockedLabs/UnlockEdv2
Length of output: 1240
🏁 Script executed:
# Look for the complete component structure and any accessibility attributes
sed -n '1,150p' frontend/src/pages/student/ResidentHome.tsx | grep -n -i "div\|button\|aria\|role\|onClick" | head -20Repository: UnlockedLabs/UnlockEdv2
Length of output: 615
Replace clickable <div> with <button> for keyboard accessibility.
The FavoriteItem component uses a <div> with onClick handler that is not keyboard-operable. Users cannot access this favorite item using Tab and Enter/Space keys, which violates WCAG accessibility standards. Replace the <div> with a semantic <button type="button"> element.
Suggested fix
- <div
+ <button
+ type="button"
onClick={handleClick}
- className="flex items-center gap-3 p-3 rounded-lg border border-border hover:shadow-md transition-shadow cursor-pointer"
+ className="w-full text-left flex items-center gap-3 p-3 rounded-lg border border-border hover:shadow-md transition-shadow cursor-pointer"
>
@@
- </div>
+ </button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div | |
| onClick={handleClick} | |
| className="flex items-center gap-3 p-3 rounded-lg border border-border hover:shadow-md transition-shadow cursor-pointer" | |
| > | |
| <button | |
| type="button" | |
| onClick={handleClick} | |
| className="w-full text-left flex items-center gap-3 p-3 rounded-lg border border-border hover:shadow-md transition-shadow cursor-pointer" | |
| > |
🤖 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 `@frontend/src/pages/student/ResidentHome.tsx` around lines 101 - 104, Replace
the non-semantic, non-keyboard-operable div wrapper with a semantic <button
type="button"> that preserves the existing onClick={handleClick} and className
(so existing styles and handlers on the element remain intact); ensure you
remove any redundant role or tabIndex previously added to the div, confirm there
are no nested interactive controls inside the button (or move them out) and if
needed add an aria-label prop for accessibility—locate the wrapper around the
favorite item (the element using handleClick in ResidentHome.tsx / FavoriteItem)
and perform this replacement.
Member
Author
There was a problem hiding this comment.
:'( Oh no..this is one of the instances I've been talking about for weeks!
There was a problem hiding this comment.
@CK-7vn Sorry for piling on with a known pain point! 😅 It sounds like the <div onClick> → <button> accessibility refactor is something that's been on the radar for a while and spans multiple places in the codebase.
Would you like me to open a GitHub issue to formally track this pattern? I can scan the codebase for all similar <div onClick> / non-semantic clickable elements and capture them in one place so it's easier to address holistically in a dedicated PR rather than as a series of one-off review comments.
Just say the word and I'll create the issue and assign it to you!
Member
Author
|
I've went over all of code-rabbits findings, and although correct, I would argue against implementing them in this PR as this is simply a byte to byte refactoring and not trying to fix issues like what was noted. Worth making tickets for the majority of them though. |
Contributor
@CK-7vn Sounds good. Could you please make tickets for the ones you think we should correct. I will proceed to with reviewing changes and testing functionality. |
Contributor
|
@CK-7vn no telling what caused this, but the videoviewer expands off of the page now, should be an easy fix: 
|
Contributor
|
@CK-7vn There are no scrollbars on the Knowledge Center page (Residents View) 
|
carddev81
approved these changes
May 18, 2026
Contributor
carddev81
left a comment
carddev81
left a comment
There was a problem hiding this comment.
Left some functionality issues. Looks great. Good job
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 (1)
frontend/src/pages/class-detail/EnrollResidentsModal.tsx (1)
64-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
conflictMapwhen conflict fetch is not a validoneresponse.As written, Line 64 only updates state on a successful
onepayload. On failure/non-one, old conflicts can remain visible from a prior fetch in the same modal session.Suggested fix
}).then((resp) => { if (resp.success && resp.type === 'one') { const map = new Map<number, ConflictDetail>(); for (const c of resp.data.conflicts ?? []) { map.set(c.user_id, c); } setConflictMap(map); + } else { + setConflictMap(new Map()); } });🤖 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 `@frontend/src/pages/class-detail/EnrollResidentsModal.tsx` around lines 64 - 70, When handling the fetch response in EnrollResidentsModal (the resp handling block that currently only updates on resp.success && resp.type === 'one'), ensure you clear stale conflicts when the response is not a valid "one" payload by calling setConflictMap(new Map()) (or an empty value consistent with the ConflictMap type) in the else path; i.e., after the existing for-loop that builds the Map for resp.data.conflicts, add an else branch that calls setConflictMap(new Map()) so old conflicts are removed when resp.success is false or resp.type !== 'one'.
🤖 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 `@frontend/src/layouts/AuthenticatedLayout.tsx`:
- Line 106: Remove the now-unused local variable isResidentKnowledgeCenter from
AuthenticatedLayout (the dead declaration added earlier) to fix the TS6133
error; locate the declaration in AuthenticatedLayout.tsx (near where
contentClass is defined) and delete it, and then run a quick search for any
remaining references to isResidentKnowledgeCenter to ensure nothing else depends
on it before committing.
In `@frontend/src/lib/formatters.ts`:
- Around line 242-247: In formatDurationStr, guard against malformed time inputs
by validating the numeric results from timeToMinutes before computing duration:
call timeToMinutes(startTime) and timeToMinutes(endTime), ensure both are finite
numbers (e.g., using Number.isFinite), and if either is not finite or end <=
start return '0h0m0s'; otherwise compute totalMin, hours, minutes and format as
before. Reference: formatDurationStr and timeToMinutes.
In `@frontend/src/pages/class-detail/SessionRow.tsx`:
- Around line 58-60: The same-date reschedule flag is computed
(isSameDateReschedule) but not honored because later conditional checks still
prioritize isRescheduledFrom/To; update the rendering and state precedence in
SessionRow so any conditional branches that check isRescheduledFrom or
isRescheduledTo (including the blocks around the occurrences you noted: the
conditionals at lines handling 63-70, 79-87, 104, 140-148, 173-179) first test
isSameDateReschedule and treat it as a "to" row (apply the blue/to-row styling,
single undo control, and avoid from-row dashed/strike-through UI) before falling
back to isRescheduledFrom/isRescheduledTo; ensure the same precedence is applied
wherever isRescheduledFrom/isRescheduledTo affect className, displayed text, or
action buttons so same-date reschedules consistently render as the "to" state.
In `@frontend/src/pages/class-detail/SessionsTab.tsx`:
- Around line 443-445: SelectedCount currently uses selectedDates.size but bulk
actions operate on selectedUpcomingSessions, causing the toolbar/count to be
wrong after mutations; fix by computing an actionable selection count (e.g.,
const actionableSelectionCount = selectedUpcomingSessions instanceof Set ?
selectedUpcomingSessions.size : new Set([...selectedDates].filter(id =>
selectedUpcomingSessions.has ? selectedUpcomingSessions.has(id) :
selectedUpcomingSessions.includes(id))).size) and pass actionableSelectionCount
to SessionsTabBulkActions as selectedCount, and keep onClearSelection clearing
selection state via setSelectedDates(new Set()) (and any other selection state)
so the UI and actionable set stay in sync; reference SessionsTabBulkActions,
selectedDates, selectedUpcomingSessions, and setSelectedDates.
In `@frontend/src/pages/knowledge-center/ResidentKnowledgeCenter.tsx`:
- Around line 337-338: The new wrapper for ResidentKnowledgeCenter removed
explicit scrolling which caused the missing scrollbar; update the element with
id "knowledge-center-landing" (or the surrounding wrapper used in the
ResidentKnowledgeCenter component) to restore vertical scrolling by adding a
vertical scroll container (e.g., add className/props such as "overflow-y-auto"
and a constrained height like "h-full" or style={{overflowY: 'auto', height:
'100%'}}) so the page regains scrollbars; apply the same change to the other
wrapper instance referenced in the file.
---
Outside diff comments:
In `@frontend/src/pages/class-detail/EnrollResidentsModal.tsx`:
- Around line 64-70: When handling the fetch response in EnrollResidentsModal
(the resp handling block that currently only updates on resp.success &&
resp.type === 'one'), ensure you clear stale conflicts when the response is not
a valid "one" payload by calling setConflictMap(new Map()) (or an empty value
consistent with the ConflictMap type) in the else path; i.e., after the existing
for-loop that builds the Map for resp.data.conflicts, add an else branch that
calls setConflictMap(new Map()) so old conflicts are removed when resp.success
is false or resp.type !== 'one'.
🪄 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: fe9f9bad-a5a9-42c1-8e92-5b8c270aaf7f
📒 Files selected for processing (26)
frontend/src/components/dashboard/TopContentList.tsxfrontend/src/components/schedule/RescheduleSessionModal.tsxfrontend/src/components/schedule/SessionDetailActions.tsxfrontend/src/components/schedule/SessionDetailClassDetails.tsxfrontend/src/components/schedule/SessionDetailHeader.tsxfrontend/src/components/schedule/SessionDetailSheet.tsxfrontend/src/components/schedule/SessionDetailStatusSection.tsxfrontend/src/components/shared/StatusBadge.tsxfrontend/src/layouts/AuthenticatedLayout.tsxfrontend/src/lib/formatters.tsfrontend/src/pages/class-detail/ClassNotFoundCard.tsxfrontend/src/pages/class-detail/EnrollResidentsModal.tsxfrontend/src/pages/class-detail/LoadingSkeleton.tsxfrontend/src/pages/class-detail/SessionRow.tsxfrontend/src/pages/class-detail/SessionsTab.tsxfrontend/src/pages/class-detail/SessionsTabBulkActions.tsxfrontend/src/pages/class-detail/SessionsTabFilterBar.tsxfrontend/src/pages/class-detail/SessionsTabModals.tsxfrontend/src/pages/class-detail/index.tsxfrontend/src/pages/class-detail/session-utils.tsfrontend/src/pages/class-detail/useSessionFilters.tsfrontend/src/pages/knowledge-center/ResidentKnowledgeCenter.tsxfrontend/src/pages/knowledge-center/VideoViewer.tsxfrontend/src/pages/programs/ClassEvents.tsxfrontend/src/pages/student/ResidentHome.tsxfrontend/src/types/events.ts
Comment on lines
+337
to
+338
| <div id="knowledge-center-landing"> | ||
| <div className="max-w-7xl mx-auto px-6 py-8"> |
There was a problem hiding this comment.
Scroll container behavior regressed in resident knowledge center layout.
The wrapper replacement dropped explicit scroll behavior; this aligns with the reported “no scrollbars” regression for residents. Add a vertical scroll container on the new wrapper (or restore the previous scroll wrapper behavior).
Proposed fix
- <div id="knowledge-center-landing">
- <div className="max-w-7xl mx-auto px-6 py-8">
+ <div id="knowledge-center-landing" className="h-full overflow-y-auto">
+ <div className="max-w-7xl mx-auto px-6 py-8">Also applies to: 478-479
🤖 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 `@frontend/src/pages/knowledge-center/ResidentKnowledgeCenter.tsx` around lines
337 - 338, The new wrapper for ResidentKnowledgeCenter removed explicit
scrolling which caused the missing scrollbar; update the element with id
"knowledge-center-landing" (or the surrounding wrapper used in the
ResidentKnowledgeCenter component) to restore vertical scrolling by adding a
vertical scroll container (e.g., add className/props such as "overflow-y-auto"
and a constrained height like "h-full" or style={{overflowY: 'auto', height:
'100%'}}) so the page regains scrollbars; apply the same change to the other
wrapper instance referenced in the file.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
frontend/src/pages/class-detail/SessionRow.tsx (2)
245-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply same-date reschedule precedence to action controls.
isSameDateRescheduleis computed, but actions still branch on raw reschedule flags, so same-date rows can show the “from” undo path.Proposed fix
- {isRescheduledFrom && isUpcoming && ( + {treatAsFrom && isUpcoming && ( <Button variant="outline" size="sm" onClick={() => onUndo()} className="border-amber-300 hover:bg-amber-50" > <Undo2 className="size-4 mr-1.5" /> Undo </Button> )} + {isSameDateReschedule && isUpcoming && ( + <Button + variant="outline" + size="sm" + onClick={() => onUndo()} + className="border-gray-300 hover:bg-gray-50" + title="Undo reschedule" + > + <Undo2 className="size-4 mr-1.5" /> + Undo + </Button> + )} - {isRescheduledTo && isUpcoming && !isSameDateReschedule && ( + {isRescheduledTo && isUpcoming && !isSameDateReschedule && ( <> <Button variant="outline" size="sm" onClick={() => onReschedule()}🤖 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 `@frontend/src/pages/class-detail/SessionRow.tsx` around lines 245 - 267, The action buttons currently check raw flags (isRescheduledFrom, isCancelledReschedule, isRescheduledTo) and ignore isSameDateReschedule, so same-date rescheduled rows still show the "from" undo path; update the JSX conditional logic to give isSameDateReschedule precedence by adding !isSameDateReschedule to the relevant branches (wrap the isRescheduledFrom and isCancelledReschedule conditions with isUpcoming && !isSameDateReschedule) and ensure any other action branches that should be suppressed for same-date reschedules also include that check; locate these conditions in SessionRow.tsx around the Button rendering (symbols: isSameDateReschedule, isRescheduledFrom, isCancelledReschedule, isRescheduledTo, isUpcoming, onUndo, onUndoCancel) and modify them accordingly.
108-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the clickable row keyboard-accessible.
The row click target is mouse-only right now, so keyboard users can’t open session details from it.
Proposed fix
<div onClick={onClick} + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onClick(); + } + }} className={`flex items-center justify-between p-4 rounded-lg border transition-colors cursor-pointer ${getBorderClass()}`} >🤖 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 `@frontend/src/pages/class-detail/SessionRow.tsx` around lines 108 - 111, The div used in the SessionRow component is only clickable by mouse; make it keyboard-accessible by adding role="button" and tabIndex={0} to the outer div, and implement an onKeyDown handler (in the same component) that calls the existing onClick when the user presses Enter or Space; keep using getBorderClass() for styling and ensure any aria-label or aria-expanded is set elsewhere if needed.frontend/src/pages/knowledge-center/ResidentKnowledgeCenter.tsx (1)
419-420:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore vertical scrolling on the landing container.
Line 419 still drops explicit scroll behavior, which matches the reported “no scrollbars” regression in Residents view.
Proposed fix
- <div id="knowledge-center-landing"> + <div id="knowledge-center-landing" className="h-full overflow-y-auto"> <div className="max-w-7xl mx-auto px-6 py-8">🤖 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 `@frontend/src/pages/knowledge-center/ResidentKnowledgeCenter.tsx` around lines 419 - 420, The landing container with id "knowledge-center-landing" lost its vertical scrolling; restore scroll behavior by adding an explicit vertical overflow (e.g., apply CSS overflow-y: auto or Tailwind class "overflow-y-auto") to the outer div (the element with id "knowledge-center-landing") so the Residents view shows scrollbars again; update either the JSX for that div or the associated CSS rule used by that id.
🤖 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.
Duplicate comments:
In `@frontend/src/pages/class-detail/SessionRow.tsx`:
- Around line 245-267: The action buttons currently check raw flags
(isRescheduledFrom, isCancelledReschedule, isRescheduledTo) and ignore
isSameDateReschedule, so same-date rescheduled rows still show the "from" undo
path; update the JSX conditional logic to give isSameDateReschedule precedence
by adding !isSameDateReschedule to the relevant branches (wrap the
isRescheduledFrom and isCancelledReschedule conditions with isUpcoming &&
!isSameDateReschedule) and ensure any other action branches that should be
suppressed for same-date reschedules also include that check; locate these
conditions in SessionRow.tsx around the Button rendering (symbols:
isSameDateReschedule, isRescheduledFrom, isCancelledReschedule, isRescheduledTo,
isUpcoming, onUndo, onUndoCancel) and modify them accordingly.
- Around line 108-111: The div used in the SessionRow component is only
clickable by mouse; make it keyboard-accessible by adding role="button" and
tabIndex={0} to the outer div, and implement an onKeyDown handler (in the same
component) that calls the existing onClick when the user presses Enter or Space;
keep using getBorderClass() for styling and ensure any aria-label or
aria-expanded is set elsewhere if needed.
In `@frontend/src/pages/knowledge-center/ResidentKnowledgeCenter.tsx`:
- Around line 419-420: The landing container with id "knowledge-center-landing"
lost its vertical scrolling; restore scroll behavior by adding an explicit
vertical overflow (e.g., apply CSS overflow-y: auto or Tailwind class
"overflow-y-auto") to the outer div (the element with id
"knowledge-center-landing") so the Residents view shows scrollbars again; update
either the JSX for that div or the associated CSS rule used by that id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cf08f0af-b748-40c0-96ac-c9bae957c736
📒 Files selected for processing (7)
frontend/src/hooks/useUrlPagination.tsfrontend/src/layouts/AuthenticatedLayout.tsxfrontend/src/lib/formatters.tsfrontend/src/pages/class-detail/SessionRow.tsxfrontend/src/pages/class-detail/SessionsTab.tsxfrontend/src/pages/knowledge-center/ResidentKnowledgeCenter.tsxfrontend/src/pages/programs/ClassEvents.tsx
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
2 participants
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.
Pre-Submission PR Checklist
Description of the change
This is split 3 of the refactor work, primarily focusing on Class Details. Every TSX block is byte-identical, the only things that have been changed is file/function boundaries, 3 type/bug findings, and the decomposition of 2 mega-files (SessionsTab.tsx and SessionDetailSheet.tsx) into sub-components behind unchanged parents.
Shipped in 6 commits, sequenced for review:
9b4511c3+821fe8ceparseLocalDate,formatShortDate,buildSessionPayloadsession-utils.ts;toDateInput/toTimeInput+ guardedformatDurationStradded tolib/formatters.ts;ClassEvents.tsxconsumes existing month helpers fromhelperFunctions/formatting;StatusBadge.getStyleForStatusDRY'd.fa78affalinked_override_eventtyped| null(was lied about withas unknown asVideoViewer.tsxswitched to typedAPI.get<Video>with discriminated-unioneslint-disable-next-linedocumenting thatResidentHome.tsxuseEffect's narrow deps are intentionald58ddf3fSessionDetailSheet.tsx758 → 361 LOC. Extracted 4 sub-components:SessionDetailHeader,SessionDetailClassDetails,SessionDetailStatusSection,SessionDetailActions.getStatusBadgemoved into theaf161f06SessionsTab.tsx1,200 → 470 LOC. ExtractedSessionRow,SessionsTabFilterBar(absorbsStatButton+FilterButton),SessionsTabModals,SessionsTabBulkActions, and auseSessionFiltershook.03457f09class-detail/index.tsxcleanup: extractedLoadingSkeletonandClassNotFoundCardto siblingdeleteBlockerReasonIIFE into a module-levelgetDeleteBlockerReason(blockers)helperDeleteBlockersinterface.6ba13b47buildFacilityEventwas duplicated betweenSessionDetailSheet.tsxandSessionsTabModals.tsx— moved tosession-utils.tsand imported in both;EnrollResidentsModal:65`as unknowndouble cast replaced withresp.type === 'one'` narrowing (same pattern as the VideoViewer fix).a09cb4a5("fix: routes for top content and resident dashboard") is a separate routing fix I made kind of in the spur of the moment to take screenshots for Carolina.TopContentList.tsx,AuthenticatedLayout.tsx, and the FavoriteItem insideResidentHome.tsx.Not part of the Split 3 batches.