refactor: add and remove logic to consolidate schedule popout to remove duplicated code#1128
Conversation
…ve duplicated code
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughSessionDetailSheet gains optional admin/calendar props and callbacks; Schedule.tsx adds event→SessionDisplay mapping and centralizes session UI into the enhanced SessionDetailSheet, replacing the previous inline sheet and trimming modals to reschedule and restore. Two class-detail tabs update imports to the shared component. ChangesSession Detail Sheet Consolidation & Admin Integration
🚥 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 docstrings
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/schedule/SessionDetailSheet.tsx (1)
200-205: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider renaming
disableModifyActionsto reduce double-negative.The prop
disableModifyActionsrequires!disableModifyActionsin the condition, creating a double-negative that can reduce readability. Consider renaming toallowModifyActionsorcanModifyActionsfor clearer intent.♻️ Proposed refactor
In the interface (lines 55-75):
- /** Force-disable the modify actions block (Reschedule/Cancel/Change Instructor/Change Room) regardless of session flags. */ - disableModifyActions?: boolean; + /** Enable the modify actions block (Reschedule/Cancel/Change Instructor/Change Room) when true. */ + allowModifyActions?: boolean;In the destructuring and logic (lines 138-205):
- disableModifyActions = false + allowModifyActions = true }: SessionDetailSheetProps) { ... const canModify = - !disableModifyActions && + allowModifyActions && !hasAttendance &&🤖 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/schedule/SessionDetailSheet.tsx` around lines 200 - 205, The condition computing canModify is hard to read due to the prop name disableModifyActions causing a double-negative; rename the prop to allowModifyActions (or canModifyActions) in the component's props/interface, update every destructuring site that reads disableModifyActions (e.g., the props declaration and the destructuring around where canModify is computed), and replace the negation in the canModify expression with the positive check (use allowModifyActions instead of !disableModifyActions) so the computed const canModify = allowModifyActions && !hasAttendance && !isCancelled && !isRescheduledFrom && !isCancelledReschedule remains clear and correct; ensure any parent component usages and type definitions are updated to the new prop name to keep types consistent.
🤖 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/schedule/SessionDetailSheet.tsx`:
- Around line 266-275: The Active badge currently shares the same Tailwind
classes as the Completed badge, causing confusion; update the Badge rendered
when showActiveBadge is true (the Badge in the showActiveBadge branch) to use a
distinct color scheme (e.g., a blue or amber palette) by replacing the className
"bg-green-50 text-[`#556830`] border-green-200" with a new set of classes that
visually distinguish Active from Completed (choose consistent bg, text and
border classes such as "bg-blue-50 text-[`#0f62fe`] border-blue-200" or similar)
so the Active Badge is clearly different from the Completed Badge.
- Around line 24-28: SessionDetailSheet imports page-level artifacts which
breaks component boundaries: move the shared types and helpers (session-utils
exports SessionDisplay and findActiveOverride) into a dedicated shared utilities
directory and update imports in SessionDetailSheet and any other consumers to
reference that shared module; then resolve the duplicate RescheduleSessionModal
implementations by either consolidating them into a single shared modal
implementation (pick the desired behavior—FormModal with room conflict handling
or Dialog with SWR data fetching) and placing it under components/shared or
components/schedule, or split responsibilities clearly (e.g., keep data-fetching
wrapper in pages/class-detail that uses a shared presentational
RescheduleSessionModal from components) and update SessionDetailSheet to import
the correct consolidated/shared RescheduleSessionModal.
---
Outside diff comments:
In `@frontend/src/components/schedule/SessionDetailSheet.tsx`:
- Around line 200-205: The condition computing canModify is hard to read due to
the prop name disableModifyActions causing a double-negative; rename the prop to
allowModifyActions (or canModifyActions) in the component's props/interface,
update every destructuring site that reads disableModifyActions (e.g., the props
declaration and the destructuring around where canModify is computed), and
replace the negation in the canModify expression with the positive check (use
allowModifyActions instead of !disableModifyActions) so the computed const
canModify = allowModifyActions && !hasAttendance && !isCancelled &&
!isRescheduledFrom && !isCancelledReschedule remains clear and correct; ensure
any parent component usages and type definitions are updated to the new prop
name to keep types consistent.
🪄 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: 3af9ee74-2194-4a3b-b640-0d9ee2ec0e4c
📒 Files selected for processing (4)
frontend/src/components/schedule/SessionDetailSheet.tsxfrontend/src/pages/Schedule.tsxfrontend/src/pages/class-detail/ScheduleTab.tsxfrontend/src/pages/class-detail/SessionsTab.tsx
| import { RescheduleSessionModal } from '@/pages/class-detail/RescheduleSessionModal'; | ||
| import { | ||
| findActiveOverride, | ||
| type SessionDisplay | ||
| } from '@/pages/class-detail/session-utils'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if RescheduleSessionModal and session-utils are used in multiple locations
rg -l 'RescheduleSessionModal' --type=tsx --type=ts
rg -l 'session-utils|SessionDisplay' --type=tsx --type=tsRepository: UnlockedLabs/UnlockEdv2
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Search for files containing RescheduleSessionModal (without --type flags)
echo "=== Files containing RescheduleSessionModal ==="
rg 'RescheduleSessionModal' -l --glob='*.ts' --glob='*.tsx' frontend/src/
echo ""
echo "=== Files containing session-utils or SessionDisplay ==="
rg 'session-utils|SessionDisplay' -l --glob='*.ts' --glob='*.tsx' frontend/src/
echo ""
echo "=== Check directory structure of pages and components ==="
find frontend/src/pages/class-detail -type f \( -name '*.ts' -o -name '*.tsx' \) 2>/dev/null | head -20
echo ""
echo "=== Check SessionDetailSheet location and imports ==="
cat -n frontend/src/components/schedule/SessionDetailSheet.tsx | head -35Repository: UnlockedLabs/UnlockEdv2
Length of output: 3063
🏁 Script executed:
#!/bin/bash
echo "=== RescheduleSessionModal in components/schedule ==="
head -30 frontend/src/components/schedule/RescheduleSessionModal.tsx
echo ""
echo "=== RescheduleSessionModal in pages/class-detail ==="
head -30 frontend/src/pages/class-detail/RescheduleSessionModal.tsx
echo ""
echo "=== Check what SessionDetailSheet is actually using ==="
rg 'RescheduleSessionModal' -A 3 -B 3 frontend/src/components/schedule/SessionDetailSheet.tsx | head -40
echo ""
echo "=== Check if session-utils should be in shared location ==="
wc -l frontend/src/pages/class-detail/session-utils.ts
head -20 frontend/src/pages/class-detail/session-utils.tsRepository: UnlockedLabs/UnlockEdv2
Length of output: 3336
Move shared utilities to a dedicated shared directory; consolidate RescheduleSessionModal implementations.
SessionDetailSheet (in components/schedule/) imports RescheduleSessionModal from @/pages/class-detail/ and depends on session-utils (also from pages), creating an architectural issue where a shared component depends on page-specific code.
Additionally, RescheduleSessionModal has two distinct implementations:
components/schedule/RescheduleSessionModal.tsx(FormModal-based with room conflict handling)pages/class-detail/RescheduleSessionModal.tsx(Dialog-based with SWR data fetching)
The current usage in SessionDetailSheet imports the pages version, but this inconsistency suggests an incomplete refactoring. Move session-utils and its exports (SessionDisplay, findActiveOverride) to a shared utilities directory, then consolidate the RescheduleSessionModal implementations or clearly separate their concerns and place them in the correct architectural layer.
🤖 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/schedule/SessionDetailSheet.tsx` around lines 24 -
28, SessionDetailSheet imports page-level artifacts which breaks component
boundaries: move the shared types and helpers (session-utils exports
SessionDisplay and findActiveOverride) into a dedicated shared utilities
directory and update imports in SessionDetailSheet and any other consumers to
reference that shared module; then resolve the duplicate RescheduleSessionModal
implementations by either consolidating them into a single shared modal
implementation (pick the desired behavior—FormModal with room conflict handling
or Dialog with SWR data fetching) and placing it under components/shared or
components/schedule, or split responsibilities clearly (e.g., keep data-fetching
wrapper in pages/class-detail that uses a shared presentational
RescheduleSessionModal from components) and update SessionDetailSheet to import
the correct consolidated/shared RescheduleSessionModal.
There was a problem hiding this comment.
This will be consolidated in another PR.
There was a problem hiding this comment.
@carddev81, understood — thanks for the context! Would you like me to open a follow-up GitHub issue to track the consolidation of session-utils / SessionDisplay / findActiveOverride into a shared utilities directory and the resolution of the duplicate RescheduleSessionModal implementations? That way it won't get lost before the next PR.
| if (showActiveBadge) { | ||
| return ( | ||
| <Badge | ||
| variant="outline" | ||
| className="bg-green-50 text-[#556830] border-green-200" | ||
| > | ||
| Active | ||
| </Badge> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Active and Completed badges use identical styling.
The "Active" badge (lines 266-275) uses the same colors as the "Completed" badge (lines 256-265): bg-green-50 text-[#556830] border-green-200. This may cause users to confuse active sessions with completed ones.
Consider using distinct colors for the "Active" badge to improve visual differentiation.
🎨 Proposed fix with distinct styling
if (showActiveBadge) {
return (
<Badge
variant="outline"
- className="bg-green-50 text-[`#556830`] border-green-200"
+ className="bg-emerald-50 text-emerald-700 border-emerald-300"
>
Active
</Badge>
);
}📝 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.
| if (showActiveBadge) { | |
| return ( | |
| <Badge | |
| variant="outline" | |
| className="bg-green-50 text-[#556830] border-green-200" | |
| > | |
| Active | |
| </Badge> | |
| ); | |
| } | |
| if (showActiveBadge) { | |
| return ( | |
| <Badge | |
| variant="outline" | |
| className="bg-emerald-50 text-emerald-700 border-emerald-300" | |
| > | |
| Active | |
| </Badge> | |
| ); | |
| } |
🤖 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/schedule/SessionDetailSheet.tsx` around lines 266 -
275, The Active badge currently shares the same Tailwind classes as the
Completed badge, causing confusion; update the Badge rendered when
showActiveBadge is true (the Badge in the showActiveBadge branch) to use a
distinct color scheme (e.g., a blue or amber palette) by replacing the className
"bg-green-50 text-[`#556830`] border-green-200" with a new set of classes that
visually distinguish Active from Completed (choose consistent bg, text and
border classes such as "bg-blue-50 text-[`#0f62fe`] border-blue-200" or similar)
so the Active Badge is clearly different from the Completed Badge.
There was a problem hiding this comment.
CSS will be fixed in another PR
There was a problem hiding this comment.
@carddev81 Understood! Would you like me to open a follow-up GitHub issue to track the badge styling fix so it doesn't get lost?
CK-7vn
left a comment
CK-7vn
left a comment
There was a problem hiding this comment.
Just one tiny thing, that's it.
| !hasAttendance && | ||
| !isCancelled && | ||
| !isRescheduledFrom && | ||
| !isCancelledReschedule; |
There was a problem hiding this comment.
I think we need && !isRescheduledTo here, but thats it my friend!
2 participants
Pre-Submission PR Checklist
Description of the change
Schedule popout logic was consolidated between the Schedule page and the Classes Detail page. Removes duplicated logic and now the code is shared between both pages.
NOTE: Didn't touch any modals here, left as is.
Screenshot(s)
Schedule Page:
Class Details Page:
