🗿 Sculptor: Refactor TimetableFilters to Compound Components#247
🗿 Sculptor: Refactor TimetableFilters to Compound Components#247ldsgroups225 wants to merge 1 commit intomasterfrom
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe pull request refactors the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/school/src/components/timetables/timetable-filters.tsx`:
- Around line 4-5: The FilterSelect component currently imports and uses
motion.div directly; change the import to use LazyMotion and domAnimation and
swap to the lightweight m element so animations load via the lazy feature
bundle: import LazyMotion and domAnimation and the m factory (symbols:
LazyMotion, domAnimation, m), wrap the component's animation root with
<LazyMotion features={domAnimation}>...</LazyMotion>, and replace motion.div
usages inside FilterSelect with m.div (or corresponding m.* elements) to ensure
the lazy feature bundle pattern is applied.
In `@apps/school/src/routes/_auth/schedules.tsx`:
- Around line 211-238: The teacher and classroom FilterSelects should be
disabled when there is no resolved school year to prevent a dead-end UI; update
the two TimetableFilters.FilterSelect components (the ones rendered when
viewMode === 'teacher' and viewMode === 'classroom') to pass disabled={true} (or
disabled={!effectiveSchoolYear}) and optionally isLoading={true} when the
resolved/effective school year context (e.g., effectiveSchoolYear / resolvedYear
from your year context) is missing; keep the existing value/onChange handlers
(selectedTeacherId/setSelectedTeacherId and
selectedClassroomId/setSelectedClassroomId) but ensure the selector UI reflects
the disabled state so users cannot interact with them when year context is
unavailable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c6d1f77-dce6-4738-8a02-83d99d1f5e06
📒 Files selected for processing (3)
.jules/sculptor.mdapps/school/src/components/timetables/timetable-filters.tsxapps/school/src/routes/_auth/schedules.tsx
| import { motion } from 'motion/react' | ||
| import * as React from 'react' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/school/src/components/timetables/timetable-filters.tsxRepository: ldsgroups225/Yeko
Length of output: 4289
Use LazyMotion with domAnimation for motion components.
The FilterSelect component uses motion.div directly without the required lazy feature bundle pattern. Update the imports and wrap the animation container accordingly.
Suggested fix
-import { motion } from 'motion/react'
+import { LazyMotion, domAnimation, m } from 'motion/react'
import * as React from 'react'
@@
- <motion.div
- initial={{ opacity: 0, scale: 0.95 }}
- animate={{ opacity: 1, scale: 1 }}
- className="
- w-full space-y-1.5
- sm:w-[240px]
- "
- >
+ <LazyMotion features={domAnimation}>
+ <m.div
+ initial={{ opacity: 0, scale: 0.95 }}
+ animate={{ opacity: 1, scale: 1 }}
+ className="
+ w-full space-y-1.5
+ sm:w-[240px]
+ "
+ >
@@
- </motion.div>
+ </m.div>
+ </LazyMotion>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/school/src/components/timetables/timetable-filters.tsx` around lines 4 -
5, The FilterSelect component currently imports and uses motion.div directly;
change the import to use LazyMotion and domAnimation and swap to the lightweight
m element so animations load via the lazy feature bundle: import LazyMotion and
domAnimation and the m factory (symbols: LazyMotion, domAnimation, m), wrap the
component's animation root with <LazyMotion
features={domAnimation}>...</LazyMotion>, and replace motion.div usages inside
FilterSelect with m.div (or corresponding m.* elements) to ensure the lazy
feature bundle pattern is applied.
| {viewMode === 'teacher' && ( | ||
| <TimetableFilters.FilterSelect | ||
| label={t.teachers.title()} | ||
| placeholder={t.teachers.select()} | ||
| value={selectedTeacherId} | ||
| onChange={setSelectedTeacherId} | ||
| isLoading={false} | ||
| options={(teachers || []).map((teacher: TeacherListItem) => ({ | ||
| value: teacher.id, | ||
| label: teacher.user?.name || '', | ||
| icon: <div className="bg-success size-2 rounded-full" /> | ||
| }))} | ||
| /> | ||
| )} | ||
|
|
||
| {viewMode === 'classroom' && ( | ||
| <TimetableFilters.FilterSelect | ||
| label={t.spaces.title()} | ||
| placeholder={t.spaces.availability.title()} | ||
| value={selectedClassroomId} | ||
| onChange={setSelectedClassroomId} | ||
| options={(classrooms || []).map(c => ({ | ||
| value: c.id, | ||
| label: c.name, | ||
| icon: <div className="bg-primary size-2 rounded-full" /> | ||
| }))} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Disable teacher/classroom selectors when year context is unavailable.
These selectors stay interactive even when no effective school year is resolved, but timetable rendering is still blocked by the year gate. That creates a dead-end UI state.
Suggested fix
{viewMode === 'teacher' && (
<TimetableFilters.FilterSelect
label={t.teachers.title()}
placeholder={t.teachers.select()}
value={selectedTeacherId}
onChange={setSelectedTeacherId}
isLoading={false}
+ disabled={!effectiveYearId || contextPending}
options={(teachers || []).map((teacher: TeacherListItem) => ({
value: teacher.id,
label: teacher.user?.name || '',
icon: <div className="bg-success size-2 rounded-full" />
}))}
/>
)}
{viewMode === 'classroom' && (
<TimetableFilters.FilterSelect
label={t.spaces.title()}
placeholder={t.spaces.availability.title()}
value={selectedClassroomId}
onChange={setSelectedClassroomId}
+ disabled={!effectiveYearId || contextPending}
options={(classrooms || []).map(c => ({
value: c.id,
label: c.name,
icon: <div className="bg-primary size-2 rounded-full" />
}))}
/>
)}📝 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.
| {viewMode === 'teacher' && ( | |
| <TimetableFilters.FilterSelect | |
| label={t.teachers.title()} | |
| placeholder={t.teachers.select()} | |
| value={selectedTeacherId} | |
| onChange={setSelectedTeacherId} | |
| isLoading={false} | |
| options={(teachers || []).map((teacher: TeacherListItem) => ({ | |
| value: teacher.id, | |
| label: teacher.user?.name || '', | |
| icon: <div className="bg-success size-2 rounded-full" /> | |
| }))} | |
| /> | |
| )} | |
| {viewMode === 'classroom' && ( | |
| <TimetableFilters.FilterSelect | |
| label={t.spaces.title()} | |
| placeholder={t.spaces.availability.title()} | |
| value={selectedClassroomId} | |
| onChange={setSelectedClassroomId} | |
| options={(classrooms || []).map(c => ({ | |
| value: c.id, | |
| label: c.name, | |
| icon: <div className="bg-primary size-2 rounded-full" /> | |
| }))} | |
| /> | |
| )} | |
| {viewMode === 'teacher' && ( | |
| <TimetableFilters.FilterSelect | |
| label={t.teachers.title()} | |
| placeholder={t.teachers.select()} | |
| value={selectedTeacherId} | |
| onChange={setSelectedTeacherId} | |
| isLoading={false} | |
| disabled={!effectiveYearId || contextPending} | |
| options={(teachers || []).map((teacher: TeacherListItem) => ({ | |
| value: teacher.id, | |
| label: teacher.user?.name || '', | |
| icon: <div className="bg-success size-2 rounded-full" /> | |
| }))} | |
| /> | |
| )} | |
| {viewMode === 'classroom' && ( | |
| <TimetableFilters.FilterSelect | |
| label={t.spaces.title()} | |
| placeholder={t.spaces.availability.title()} | |
| value={selectedClassroomId} | |
| onChange={setSelectedClassroomId} | |
| disabled={!effectiveYearId || contextPending} | |
| options={(classrooms || []).map(c => ({ | |
| value: c.id, | |
| label: c.name, | |
| icon: <div className="bg-primary size-2 rounded-full" /> | |
| }))} | |
| /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/school/src/routes/_auth/schedules.tsx` around lines 211 - 238, The
teacher and classroom FilterSelects should be disabled when there is no resolved
school year to prevent a dead-end UI; update the two
TimetableFilters.FilterSelect components (the ones rendered when viewMode ===
'teacher' and viewMode === 'classroom') to pass disabled={true} (or
disabled={!effectiveSchoolYear}) and optionally isLoading={true} when the
resolved/effective school year context (e.g., effectiveSchoolYear / resolvedYear
from your year context) is missing; keep the existing value/onChange handlers
(selectedTeacherId/setSelectedTeacherId and
selectedClassroomId/setSelectedClassroomId) but ensure the selector UI reflects
the disabled state so users cannot interact with them when year context is
unavailable.
1 participant
What
Refactored the
TimetableFilterscomponent to use the Compound Components pattern, eliminating prop-drilling and a heavy 15-prop monolithic signature.Why
The original component suffered from severe boolean prop proliferation (
classesPending,teachersPending,contextPending) and rigid internal structure. By delegating the rendering logic to the parent viaTimetableFilters.FilterSelect, the consumer (schedules.tsx) gains direct control over its layout and specific loading states without polluting the child component.Patterns Used
TimetableFilters(Container) +TimetableFilters.ViewSwitcher+TimetableFilters.FiltersContainer+TimetableFilters.FilterSelect.FilterSelectabstraction.PR created automatically by Jules for task 10317478516982637236 started by @ldsgroups225
Summary by CodeRabbit