refactor: add changes to account for split 2 ticket ID 654#1132
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR consolidates helper functions into a centralized library module, establishes a foundation of reusable shared components (InfoTooltip, enhanced PageHeader), modernizes the dashboard and event attendance pages with these components, unifies tab styling in global CSS, and updates TypeScript path aliases. ChangesHelper function migration and library consolidation
Shared component foundations
Dashboard modernization with component patterns
Event attendance page redesign
UI styling consolidation and component adoption
TypeScript configuration updates
🚥 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. 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: 6
🤖 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/shared/InfoTooltip.tsx`:
- Around line 23-31: The icon-only button inside TooltipTrigger in
InfoTooltip.tsx lacks an accessible name; update the button element (the one
wrapping InformationCircleIcon) to include an appropriate accessible label
(e.g., aria-label or aria-labelledby) that describes its purpose, optionally
accepting a prop for the label or falling back to a sensible default, and ensure
the TooltipTrigger/InformationCircleIcon usage still renders the tooltip while
providing the screen-reader accessible name.
In `@frontend/src/lib/formatters.ts`:
- Around line 201-220: The three helpers getPreviousMonth, getNextMonth, and
formatMonthYear currently assume input is "YYYY-MM" and produce invalid dates on
malformed strings; add an input validation at the top of each function that
checks the incoming monthString/currentMonth against a strict /^\d{4}-\d{2}$/
pattern and verifies the parsed month is between 1 and 12, then on failure
either throw a clear Error (e.g. "Invalid month format, expected YYYY-MM") or
return a sentinel (empty string) depending on caller expectations — update
getPreviousMonth, getNextMonth, and formatMonthYear to perform this guard before
creating Date objects so callers never see "Invalid Date".
- Around line 6-13: The Timezones enum mixes standard/daylight abbreviations and
uses unnecessary quoted keys; update the Timezones enum so keys are unquoted and
use a region-based or neutral name for the Mountain entry (replace the 'MDT' key
with something like Mountain or MT mapped to 'America/Denver') while keeping
'MST' -> 'America/Phoenix' as-is; specifically modify the Timezones enum
(replace the 'MDT' key, remove quotes around all keys like
'CST','EST','AKST','PST','MST') so callers get a clear, non-misleading
Mountain-time mapping to 'America/Denver'.
- Around line 145-153: formatTimeHM may produce "24:00" at midnight in some
runtimes because hour12: false doesn't force the hour cycle; update formatTimeHM
to explicitly specify hourCycle: 'h23' in the toLocaleTimeString options so
midnight renders as "00:00" across environments (or alternatively replace the
locale formatter with manual padding using date.getHours() and date.getMinutes()
to build `${hh}:${mm}`). Ensure the change is applied inside the formatTimeHM
function.
- Around line 185-199: The parseRRule function currently calls rule.all()[0]
which can materialize infinite occurrences and return undefined; change the
implementation to declare rule as const inside the try, and use
rule.options.dtstart when startDtOnly is true or use rule.after(new Date(0),
true) to retrieve the first matching occurrence otherwise, guard against
undefined before passing to fromLocalDateToNumericDateFormat, and in the catch
block log the error instead of silently swallowing it (follow the rule.after() +
null-check pattern used in ClassHeader.tsx).
In `@frontend/src/pages/HelpCenter.tsx`:
- Around line 30-33: The if (close) branch renders the "Help Center" heading
with inline markup while the main return uses the PageHeader component; replace
the inline title markup in the if (close) branch with the PageHeader component
(using the same title "Help Center" and subtitle "Find answers to common
questions and get support.") so both branches render consistently; locate the
conditional that checks close and swap the inline JSX heading for <PageHeader
title="Help Center" subtitle="Find answers to common questions and get support."
/> ensuring any styling/props match the main return path.
🪄 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: cdd4d180-519a-4029-9902-846b4fa083a8
📒 Files selected for processing (20)
frontend/src/components/helperFunctions/formatting.tsxfrontend/src/components/helperFunctions/index.tsfrontend/src/components/history/formatHistoryEntry.tsfrontend/src/components/shared/InfoTooltip.tsxfrontend/src/components/shared/PageHeader.tsxfrontend/src/components/shared/index.tsfrontend/src/lib/formatters.tsfrontend/src/pages/Dashboard.tsxfrontend/src/pages/HelpCenter.tsxfrontend/src/pages/event-attendance/AttendanceRow.tsxfrontend/src/pages/event-attendance/constants.tsfrontend/src/pages/event-attendance/index.tsxfrontend/src/pages/insights/OperationalInsights.tsxfrontend/src/pages/program-detail/constants.tsfrontend/src/pages/programs/AddClassEnrollments.tsxfrontend/src/pages/programs/ProgramClassManagement.tsxfrontend/src/routes/program-routes.tsxfrontend/src/styles/globals.cssfrontend/tsconfig.app.jsonfrontend/tsconfig.node.json
💤 Files with no reviewable changes (2)
- frontend/src/components/helperFunctions/index.ts
- frontend/src/components/helperFunctions/formatting.tsx
CK-7vn
approved these changes
May 18, 2026
Member
CK-7vn
left a comment
CK-7vn
left a comment
There was a problem hiding this comment.
Just a couple comments, approving because it can go with or without the changes. Sweet job my friend.
| <div className="flex items-start justify-between"> | ||
| <div> | ||
| <h1 className="text-2xl font-bold text-foreground">{title}</h1> | ||
| <h1 className="text-[#203622] dark:text-white mb-2"> |
Member
There was a problem hiding this comment.
We're removing the dark mode, so I've just been trying to pull these dark tags out whenever I catch them, not a biggie though, we're not surfacing a way for users to use dark mode currently, so it probably really doesn't matter. Just wanted to note.
Contributor
Author
There was a problem hiding this comment.
okay i'll leave this one for now then for when CSS is worked on, definitely noted.
| {facilityName} | ||
| </p> | ||
| </div> | ||
| <DashboardHeader title="Facility Dashboard" subtitle={facilityName} /> |
Member
There was a problem hiding this comment.
Is there anyway we could use PageHeader here if we just add a className prop in PageHeader and remove the duplication in the bottom of this?
Contributor
Author
There was a problem hiding this comment.
i will make this change.
carddev81
force-pushed
the
carddev81/refactorcomponents
branch
from
433f8c7 to
f1f7a9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/pages/event-attendance/index.tsx (3)
119-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWait for
datesbefore freezing the initial row state.
initializedflips as soon as attendance data arrives, even ifscheduledTimesis still empty. If the dates request resolves later, untouched rows keepcheck_in_at: '', and a resident switched toPartialno longer inherits the scheduled start time. Gate this effect ondatesLoading === falsebefore settinginitialized.Suggested fix
useEffect(() => { - if (!data?.data || initialized) return; + if (!data?.data || initialized || datesLoading) return; const defaultCheckIn = scheduledTimes.check_in_at || ''; const mapped: RowState[] = data.data.map((item) => { const hasExisting = !!item.attendance_status; return { @@ setRows(mapped); setInitialized(true); -}, [data, initialized, scheduledTimes]); +}, [data, initialized, scheduledTimes, datesLoading]);🤖 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/event-attendance/index.tsx` around lines 119 - 141, The effect that builds initial rows (useEffect) sets initialized too early because it only waits on data and initialized but not on scheduledTimes/dates; change the guard to also require datesLoading === false (or !datesLoading) before mapping and calling setRows/setInitialized so scheduledTimes.check_in_at is available; update the effect dependencies to include datesLoading (and scheduledTimes if needed) and ensure initialized is only set after rows use scheduledTimes, referencing the useEffect, scheduledTimes, datesLoading, setRows, setInitialized and RowState symbols.
648-675:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t let
AttendanceReason.Otherhide its required note.When
requiredis true, the note can still start collapsed, andRemoveclears/collapses it again. That leavesOtherselected with no visible input while the save payload still sendsnote: ''.Suggested fix
- if (!expanded) { + if (!expanded && !required) { return ( <button onClick={onExpand} className="text-sm text-blue-600 hover:text-blue-700 hover:underline" > @@ - <button - onClick={() => { - onChange(''); - onCollapse(); - }} - className="text-xs text-gray-500 hover:text-gray-700" - > - Remove - </button> + {!required && ( + <button + onClick={() => { + onChange(''); + onCollapse(); + }} + className="text-xs text-gray-500 hover:text-gray-700" + > + Remove + </button> + )} @@ onValueChange={(v) => { const next = v as AttendanceReason; onUpdate(row.user_id, { reason: next, + showNoteField: + next === AttendanceReason.Other + ? true + : row.showNoteField, ...(next !== AttendanceReason.Other && row.reason === AttendanceReason.Other && { - note: '' + note: '', + showNoteField: false }) }); }}Also applies to: 711-746
🤖 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/event-attendance/index.tsx` around lines 648 - 675, The Note field can be hidden while AttendanceReason.Other is selected and required, so ensure the note is always visible in that case: when the reason switches to AttendanceReason.Other (in your reason change handler) call onExpand() so the input opens; when rendering, treat expanded as true if required && reason === AttendanceReason.Other so it never returns the collapsed "+ Add a note" button for that case; and change the Remove button’s onClick to only call onCollapse() when !(required && reason === AttendanceReason.Other) (otherwise just keep the note cleared via onChange('') but remain expanded). Use the existing symbols: AttendanceReason.Other, required, expanded, onExpand, onCollapse, onChange and the Remove button handler to implement these checks.
228-260:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
check_out_atwhenever a row becomesPresent.Both the single-row path and “Mark All Present” preserve an existing
check_out_at. APartial -> Presenttransition then submits a hidden clock-out time that the present UI no longer exposes.Suggested fix
const defaults = status === Attendance.Present ? { check_in_at: rows.find((r) => r.user_id === userId) ?.check_in_at ?? scheduledTimes.check_in_at ?? formatTimeHM(new Date()), - check_out_at: - rows.find((r) => r.user_id === userId) - ?.check_out_at ?? '' + check_out_at: '' } : {}; @@ status: Attendance.Present, reason: '', check_in_at: r.check_in_at || defaultCheckIn, + check_out_at: '', showNoteField: false, dirty: true })) );Also applies to: 263-275
🤖 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/event-attendance/index.tsx` around lines 228 - 260, handleStatusChange currently preserves an existing check_out_at when transitioning Partial -> Present, causing a hidden clock-out to be submitted; update the logic in handleStatusChange (and the similar "Mark All Present" code block referenced around the same area) so that when status === Attendance.Present you explicitly set check_out_at to an empty string instead of reusing defaults.check_out_at, i.e., ensure updateRow receives check_out_at: '' for Present transitions while still preserving check_in_at from defaults when appropriate.
🤖 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/lib/formatters.ts`:
- Around line 150-153: parseLocalDay currently constructs a Date which silently
normalizes invalid calendar dates (e.g. "2026-02-31") — update parseLocalDay to
validate the parsed year, month and day instead of relying on Date
normalization: parse the components from isoDate, ensure month is 1–12 and day
is within the correct range for that month (accounting for leap years), then
construct the Date and verify date.getFullYear(), date.getMonth()+1 and
date.getDate() match the parsed values; if validation fails, return a clear
failure (throw an Error or return null) rather than returning a normalized Date.
---
Outside diff comments:
In `@frontend/src/pages/event-attendance/index.tsx`:
- Around line 119-141: The effect that builds initial rows (useEffect) sets
initialized too early because it only waits on data and initialized but not on
scheduledTimes/dates; change the guard to also require datesLoading === false
(or !datesLoading) before mapping and calling setRows/setInitialized so
scheduledTimes.check_in_at is available; update the effect dependencies to
include datesLoading (and scheduledTimes if needed) and ensure initialized is
only set after rows use scheduledTimes, referencing the useEffect,
scheduledTimes, datesLoading, setRows, setInitialized and RowState symbols.
- Around line 648-675: The Note field can be hidden while AttendanceReason.Other
is selected and required, so ensure the note is always visible in that case:
when the reason switches to AttendanceReason.Other (in your reason change
handler) call onExpand() so the input opens; when rendering, treat expanded as
true if required && reason === AttendanceReason.Other so it never returns the
collapsed "+ Add a note" button for that case; and change the Remove button’s
onClick to only call onCollapse() when !(required && reason ===
AttendanceReason.Other) (otherwise just keep the note cleared via onChange('')
but remain expanded). Use the existing symbols: AttendanceReason.Other,
required, expanded, onExpand, onCollapse, onChange and the Remove button handler
to implement these checks.
- Around line 228-260: handleStatusChange currently preserves an existing
check_out_at when transitioning Partial -> Present, causing a hidden clock-out
to be submitted; update the logic in handleStatusChange (and the similar "Mark
All Present" code block referenced around the same area) so that when status ===
Attendance.Present you explicitly set check_out_at to an empty string instead of
reusing defaults.check_out_at, i.e., ensure updateRow receives check_out_at: ''
for Present transitions while still preserving check_in_at from defaults when
appropriate.
🪄 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: 2fcba03e-1312-4aa4-ae95-645b2ead9e02
📒 Files selected for processing (20)
frontend/src/components/helperFunctions/formatting.tsxfrontend/src/components/helperFunctions/index.tsfrontend/src/components/history/formatHistoryEntry.tsfrontend/src/components/shared/InfoTooltip.tsxfrontend/src/components/shared/PageHeader.tsxfrontend/src/components/shared/index.tsfrontend/src/lib/formatters.tsfrontend/src/pages/Dashboard.tsxfrontend/src/pages/HelpCenter.tsxfrontend/src/pages/event-attendance/AttendanceRow.tsxfrontend/src/pages/event-attendance/constants.tsfrontend/src/pages/event-attendance/index.tsxfrontend/src/pages/insights/OperationalInsights.tsxfrontend/src/pages/program-detail/constants.tsfrontend/src/pages/programs/AddClassEnrollments.tsxfrontend/src/pages/programs/ProgramClassManagement.tsxfrontend/src/routes/program-routes.tsxfrontend/src/styles/globals.cssfrontend/tsconfig.app.jsonfrontend/tsconfig.node.json
💤 Files with no reviewable changes (2)
- frontend/src/components/helperFunctions/formatting.tsx
- frontend/src/components/helperFunctions/index.ts
Comment on lines
+150
to
+153
| export function parseLocalDay(isoDate: string): Date { | ||
| const [year, month, day] = isoDate.split('-').map(Number); | ||
| return new Date(year, month - 1, day); | ||
| } |
There was a problem hiding this comment.
Reject impossible calendar dates instead of rolling them forward.
new Date(year, month - 1, day) normalizes invalid inputs like 2026-02-31 to March 3. This helper is now used for route dates, so a malformed URL can render a different day than the one in the path.
Suggested fix
export function parseLocalDay(isoDate: string): Date {
- const [year, month, day] = isoDate.split('-').map(Number);
- return new Date(year, month - 1, day);
+ const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(isoDate);
+ if (!match) throw new Error('Invalid date format');
+
+ const year = Number(match[1]);
+ const month = Number(match[2]);
+ const day = Number(match[3]);
+ const parsed = new Date(year, month - 1, day);
+
+ if (
+ parsed.getFullYear() !== year ||
+ parsed.getMonth() !== month - 1 ||
+ parsed.getDate() !== day
+ ) {
+ throw new Error('Invalid calendar date');
+ }
+
+ return parsed;
}🤖 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/lib/formatters.ts` around lines 150 - 153, parseLocalDay
currently constructs a Date which silently normalizes invalid calendar dates
(e.g. "2026-02-31") — update parseLocalDay to validate the parsed year, month
and day instead of relying on Date normalization: parse the components from
isoDate, ensure month is 1–12 and day is within the correct range for that month
(accounting for leap years), then construct the Date and verify
date.getFullYear(), date.getMonth()+1 and date.getDate() match the parsed
values; if validation fails, return a clear failure (throw an Error or return
null) rather than returning a normalized Date.
… component and then remove unused code
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
Made refactor changes per @CK-7vn 's split research. I also had to make sure the 'Take Attendance' page matched the figma because it did not. The following is what was accomplished in this ticket:
Split 2
components/helperFunctions/formatting.tsxintolib/formatters.ts; deleted the directory; pruned 13 dead exports.SummaryStatCardlocal component (4 tiles → 4 calls).<InfoTooltip>; replaces 3 sites on Dashboard.event-attendance/constants.tswithATTENDANCE_STATUSES; replacesSTATUS_BUTTONS(index.tsx) andATTENDANCE_OPTIONS(AttendanceRow.tsx).<CollapsibleNoteField>; replacesNoteOnly+ the duplicated note block insideReasonAndNote. Remove now always clears the note text.<SurfaceCard>wrapping thebg-white dark:bg-[#171717] rounded-lg border…chain; replaces 6 Dashboard sites.