fix: prevent desktop schedule service row overlap#283
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
anyulled
force-pushed
the
codex/desktop-schedule-layout-fix
branch
from
cccd321 to
1c870d5
Compare
📝 WalkthroughWalkthroughA new ChangesFavorite Session Notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces browser notifications for favorite sessions, scheduling reminders one minute before they start and grouping simultaneous sessions. It also refactors the ScheduleGrid to dynamically calculate row heights based on actual session times rather than fixed 30-minute intervals, and simplifies the SessionCard component. The feedback highlights several important improvements: preventing a potential integer overflow in setTimeout for far-future sessions, addressing browser compatibility issues with Array.prototype.at() and Notification.requestPermission(), resolving a React key warning by using flatMap instead of nested map calls, avoiding empty speaker containers that disrupt layout spacing, and moving a side effect out of a React state updater function.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
lib/session-notifications.ts (116-123)
JavaScript's setTimeout uses a 32-bit signed integer for the delay parameter. If a session is scheduled more than 24.8 days in the future, the delay (notification.notifyAt - now) will exceed 2147483647 ms, causing an integer overflow. This makes the timeout trigger immediately, spamming the user with notifications upon saving a session. It is highly recommended to filter out or cap notifications that are too far in the future (e.g., only schedule if the delay is within 24 hours or less than the 32-bit limit).
const timeoutIds = buildFavoriteSessionNotifications(schedule, savedSessionIds, now)
.filter((notification) => {
const delay = notification.notifyAt - now;
return delay > 0 && delay <= 2147483647; // Prevent setTimeout overflow
})
.map((notification) =>
window.setTimeout(() => {
new notificationApi(notification.title, {
body: notification.body,
tag: "devbcn-session-" + notification.id,
});
}, notification.notifyAt - now)
);components/schedule/ScheduleGrid.tsx (32)
Using Array.prototype.at() is an ES2022 feature that might not be supported in older browsers without appropriate polyfills. Since activeTab is guaranteed to be a valid non-negative index within the bounds of the schedule array, standard bracket notation schedule[activeTab] is safer, more compatible, and avoids unnecessary polyfill dependency.
const currentDay = hasSchedule ? (schedule[activeTab] ?? null) : null;
components/schedule/ScheduleGrid.tsx (118)
Using rooms.map here returns a nested array of React elements (JSX.Element[][]), which will trigger the React console warning: Warning: Each child in a list should have a unique "key" prop. because the outer arrays do not have keys. Using rooms.flatMap instead will flatten the nested arrays into a single flat array of elements, completely avoiding this warning.
return rooms.flatMap((room) => {
components/schedule/SessionCard.tsx (86-92)
If a session has no speakers (session.speakers is empty), this will render an empty <div className={styles.speakers}> container. Since .speakers has a margin-bottom: 12px in the SCSS, this empty container will introduce unnecessary vertical whitespace in the card layout. Wrapping the container in a length check prevents this.
{session.speakers.length > 0 && (
<div className={styles.speakers}>
{session.speakers.map((speaker) => (
<Link key={speaker.id} href={"/" + year + "/speakers/" + speaker.id} className={styles.speakerLink}>
{speaker.name}
</Link>
))}
</div>
)}
context/ScheduleContext.tsx (51-53)
In older browsers (such as Safari prior to version 11.1 and iOS Safari prior to 11.3), Notification.requestPermission() does not return a Promise (it uses a callback instead and returns void). Calling .catch() on undefined will cause a runtime TypeError and crash the application. It is safer to check if the returned value is a Promise before calling .catch().
const permissionResult = Notification.requestPermission();
if (permissionResult && typeof permissionResult.catch === "function") {
permissionResult.catch((error: unknown) => {
console.error("Failed to request notification permission", error);
});
}
context/ScheduleContext.tsx (58-65)
React state updater functions (the callback passed to setSavedSessionIds) must be pure and free of side effects. Calling requestNotificationPermission() inside the state updater is a side effect and can lead to unexpected behavior or multiple permission prompts during concurrent rendering. Move the side effect call outside of the state updater function.
setSavedSessionIds((prev) => {
if (prev.includes(sessionId)) {
return prev.filter((id) => id !== sessionId);
}
return [...prev, sessionId];
});
requestNotificationPermission();
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@context/ScheduleContext.tsx`:
- Around line 42-54: The useEffect that calls
scheduleFavoriteSessionNotifications at lines 42-44 does not have notification
permission state in its dependency array, so it never re-runs when the user
grants permission asynchronously through requestNotificationPermission(). Add a
state variable to track the current Notification.permission value and include
this state in the dependency array of the useEffect. Update the notification
permission state in the requestNotificationPermission callback after
Notification.requestPermission() resolves successfully, which will trigger the
useEffect to re-run and properly schedule notifications for sessions saved
before permission was granted.
In `@lib/session-notifications.ts`:
- Around line 42-57: The timeLabel property in the notificationsByNotifyAt.set
call uses the format function with only HH:mm format, which applies the host's
local timezone instead of explicitly using the session's timezone. Replace the
format(startsAt, "HH:mm") call with Intl.DateTimeFormat that explicitly
specifies the timeZone option set to "Europe/Madrid" to ensure the time is
formatted consistently in the correct timezone regardless of the host
environment where the code runs.
In `@styles/components/_index.scss`:
- Line 12: The `@forward` "live-schedule" statement on line 12 references a
stylesheet that Sass cannot locate, causing build failures. Verify that the
live-schedule stylesheet file exists in the expected location relative to the
_index.scss file (typically as _live-schedule.scss in the same directory). If
the file exists, correct the path in the `@forward` statement to match the actual
file location. If the file does not exist, remove the `@forward` statement
entirely. Ensure all forwarded stylesheets in the _index.scss file have
corresponding files that Sass can resolve.
🪄 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: CHILL
Plan: Pro
Run ID: ed1661cd-09d0-4a47-8d20-2a4a1894e063
📒 Files selected for processing (5)
__tests__/lib/session-notifications.test.tsapp/[year]/schedule/page.tsxcontext/ScheduleContext.tsxlib/session-notifications.tsstyles/components/_index.scss
| useEffect(() => { | ||
| return scheduleFavoriteSessionNotifications(schedule, savedSessionIds, typeof Notification === "undefined" ? undefined : Notification); | ||
| }, [schedule, savedSessionIds]); | ||
|
|
||
| const requestNotificationPermission = useCallback(() => { | ||
| if (typeof Notification === "undefined" || Notification.permission !== "default") { | ||
| return; | ||
| } | ||
|
|
||
| Notification.requestPermission().catch((error: unknown) => { | ||
| console.error("Failed to request notification permission", error); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ScheduleContext.tsx" -type fRepository: anyulled/devbcn-nextjs
Length of output: 96
🏁 Script executed:
cat -n context/ScheduleContext.tsx | head -70Repository: anyulled/devbcn-nextjs
Length of output: 2815
🏁 Script executed:
rg "scheduleFavoriteSessionNotifications" -A 10 -B 2 --type ts --type tsxRepository: anyulled/devbcn-nextjs
Length of output: 94
🏁 Script executed:
rg "scheduleFavoriteSessionNotifications" -A 10 -B 2Repository: anyulled/devbcn-nextjs
Length of output: 6300
🏁 Script executed:
cat -n lib/session-notifications.ts | head -100Repository: anyulled/devbcn-nextjs
Length of output: 4017
🏁 Script executed:
cat -n context/ScheduleContext.tsx | sed -n '56,68p'Repository: anyulled/devbcn-nextjs
Length of output: 503
🏁 Script executed:
cat -n lib/session-notifications.ts | sed -n '49,58p'Repository: anyulled/devbcn-nextjs
Length of output: 470
🏁 Script executed:
cat -n lib/session-notifications.ts | sed -n '35,48p'Repository: anyulled/devbcn-nextjs
Length of output: 543
🏁 Script executed:
cat -n lib/session-notifications.ts | sed -n '32,50p'Repository: anyulled/devbcn-nextjs
Length of output: 790
🏁 Script executed:
cat -n lib/session-notifications.ts | sed -n '105,130p'Repository: anyulled/devbcn-nextjs
Length of output: 968
Add notificationPermission state and dependency to reschedule when permission is granted.
When a user saves a session before granting notification permission, the effect at lines 42-44 runs with Notification.permission === "default", causing scheduleFavoriteSessionNotifications to return early without scheduling any notifications (line 112 check). The permission then transitions to "granted" asynchronously via requestNotificationPermission(), but because the effect has no dependency on permission state, it never re-runs to schedule those saved sessions.
Track permission changes in state and include it as an effect dependency so scheduling occurs after permission is granted.
💡 Proposed fix
export function ScheduleProvider({ children, schedule = [] }: ScheduleProviderProps) {
const [savedSessionIds, setSavedSessionIds] = useState<string[]>([]);
+ const [notificationPermission, setNotificationPermission] = useState<NotificationPermission>(
+ typeof Notification === "undefined" ? "denied" : Notification.permission
+ );
const [isLoaded, setIsLoaded] = useState(false);
useEffect(() => {
- return scheduleFavoriteSessionNotifications(schedule, savedSessionIds, typeof Notification === "undefined" ? undefined : Notification);
- }, [schedule, savedSessionIds]);
+ return scheduleFavoriteSessionNotifications(
+ schedule,
+ savedSessionIds,
+ typeof Notification === "undefined" || notificationPermission !== "granted" ? undefined : Notification
+ );
+ }, [schedule, savedSessionIds, notificationPermission]);
const requestNotificationPermission = useCallback(() => {
if (typeof Notification === "undefined" || Notification.permission !== "default") {
return;
}
- Notification.requestPermission().catch((error: unknown) => {
- console.error("Failed to request notification permission", error);
- });
+ Notification.requestPermission()
+ .then((permission) => setNotificationPermission(permission))
+ .catch((error: unknown) => {
+ console.error("Failed to request notification permission", error);
+ });
}, []);📝 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.
| useEffect(() => { | |
| return scheduleFavoriteSessionNotifications(schedule, savedSessionIds, typeof Notification === "undefined" ? undefined : Notification); | |
| }, [schedule, savedSessionIds]); | |
| const requestNotificationPermission = useCallback(() => { | |
| if (typeof Notification === "undefined" || Notification.permission !== "default") { | |
| return; | |
| } | |
| Notification.requestPermission().catch((error: unknown) => { | |
| console.error("Failed to request notification permission", error); | |
| }); | |
| }, []); | |
| const [savedSessionIds, setSavedSessionIds] = useState<string[]>([]); | |
| const [notificationPermission, setNotificationPermission] = useState<NotificationPermission>( | |
| typeof Notification === "undefined" ? "denied" : Notification.permission | |
| ); | |
| const [isLoaded, setIsLoaded] = useState(false); | |
| useEffect(() => { | |
| return scheduleFavoriteSessionNotifications( | |
| schedule, | |
| savedSessionIds, | |
| typeof Notification === "undefined" || notificationPermission !== "granted" ? undefined : Notification | |
| ); | |
| }, [schedule, savedSessionIds, notificationPermission]); | |
| const requestNotificationPermission = useCallback(() => { | |
| if (typeof Notification === "undefined" || Notification.permission !== "default") { | |
| return; | |
| } | |
| Notification.requestPermission() | |
| .then((permission) => setNotificationPermission(permission)) | |
| .catch((error: unknown) => { | |
| console.error("Failed to request notification permission", error); | |
| }); | |
| }, []); |
🤖 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 `@context/ScheduleContext.tsx` around lines 42 - 54, The useEffect that calls
scheduleFavoriteSessionNotifications at lines 42-44 does not have notification
permission state in its dependency array, so it never re-runs when the user
grants permission asynchronously through requestNotificationPermission(). Add a
state variable to track the current Notification.permission value and include
this state in the dependency array of the useEffect. Update the notification
permission state in the requestNotificationPermission callback after
Notification.requestPermission() resolves successfully, which will trigger the
useEffect to re-run and properly schedule notifications for sessions saved
before permission was granted.
| const startsAt = parseISO(session.startsAt); | ||
| const notifyAt = startsAt.getTime() - SESSION_REMINDER_LEAD_TIME_MS; | ||
|
|
||
| if (notifyAt <= now) { | ||
| return; | ||
| } | ||
|
|
||
| const existingNotification = notificationsByNotifyAt.get(notifyAt); | ||
| if (existingNotification === undefined) { | ||
| notificationsByNotifyAt.set(notifyAt, { | ||
| ids: [session.id], | ||
| sessionCount: 1, | ||
| titles: [session.title], | ||
| rooms: [session.room], | ||
| timeLabel: format(startsAt, "HH:mm"), | ||
| notifyAt, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the session-notifications.ts file
find . -name "session-notifications.ts" -type fRepository: anyulled/devbcn-nextjs
Length of output: 97
🏁 Script executed:
# Check the content around line 56
cat -n lib/session-notifications.ts | sed -n '40,70p'Repository: anyulled/devbcn-nextjs
Length of output: 1433
🏁 Script executed:
# Search for test files related to session-notifications
fd -t f ".*test.*" | grep -i "session.*notif" | head -20Repository: anyulled/devbcn-nextjs
Length of output: 110
🏁 Script executed:
# Look for any timezone configuration in the project
rg -i "timezone|timeZone|europe.*madrid" --type ts --type tsx --type js --type json -C 2Repository: anyulled/devbcn-nextjs
Length of output: 94
🏁 Script executed:
# Search for timezone configuration without specifying tsx
rg -i "timezone|timeZone|europe.*madrid" -C 2 | head -50Repository: anyulled/devbcn-nextjs
Length of output: 2792
🏁 Script executed:
# Examine the test file to see what the expected vs actual values are
cat -n __tests__/lib/session-notifications.test.ts | head -100Repository: anyulled/devbcn-nextjs
Length of output: 3532
🏁 Script executed:
# Look for any environment setup or timezone mocking in tests
rg -i "mock|jest|setup|timezone" __tests__/lib/session-notifications.test.ts -C 2Repository: anyulled/devbcn-nextjs
Length of output: 2174
🏁 Script executed:
# Check the imports and format usage in session-notifications.ts
cat -n lib/session-notifications.ts | head -30Repository: anyulled/devbcn-nextjs
Length of output: 1080
Make reminder time formatting timezone-explicit.
Line 56 formats the start time using format(startsAt, "HH:mm"), which applies the host's local timezone instead of the event's timezone. Sessions are stored in UTC (e.g., "2026-06-17T10:00:00.000Z"), but the event runs in Europe/Madrid (UTC+2 in June). The test expects "12:00" (Madrid time) but the current code produces "10:00" (UTC), breaking tests in CI environments.
Use Intl.DateTimeFormat with an explicit timeZone option set to "Europe/Madrid" to format times consistently regardless of the host environment.
🤖 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 `@lib/session-notifications.ts` around lines 42 - 57, The timeLabel property in
the notificationsByNotifyAt.set call uses the format function with only HH:mm
format, which applies the host's local timezone instead of explicitly using the
session's timezone. Replace the format(startsAt, "HH:mm") call with
Intl.DateTimeFormat that explicitly specifies the timeZone option set to
"Europe/Madrid" to ensure the time is formatted consistently in the correct
timezone regardless of the host environment where the code runs.
Source: Pipeline failures
| @forward "venue-wtc"; | ||
| @forward "convince-your-boss"; | ||
| @forward "features"; | ||
| @forward "live-schedule"; |
There was a problem hiding this comment.
Fix missing forwarded stylesheet before merge.
Line 12 forwards live-schedule, but CI shows Sass cannot resolve it, causing build failure across multiple E2E jobs.
🧰 Tools
🪛 GitHub Actions: E2E Tests / 0_cypress-run (home-pages).txt
[error] 12-12: Turbopack build failed: Can't find stylesheet to import. The @forward directive references 'live-schedule' which does not exist. Verify that the stylesheet file exists in the styles/components/ directory.
🪛 GitHub Actions: E2E Tests / 1_cypress-run (speakers).txt
[error] 12-12: Sass build failed: Can't find stylesheet to import for '@forward "live-schedule"'. (styles/components/_index.scss:12:1)
🪛 GitHub Actions: E2E Tests / 2_cypress-run (talks).txt
[error] 12-12: Sass/Turbopack build failed: Can't find stylesheet to import. Missing import in @forward "live-schedule".
🪛 GitHub Actions: E2E Tests / cypress-run (home-pages)
[error] 12-12: Sass build failed: Can't find stylesheet to import for '@forward "live-schedule"'.
🪛 GitHub Actions: E2E Tests / cypress-run (speakers)
[error] 12-12: Sass build failed while processing '@forward "live-schedule"'. Error: Can't find stylesheet to import.
🪛 GitHub Actions: E2E Tests / cypress-run (talks)
[error] 12-12: Sass build failed: Can't find stylesheet to import for @forward "live-schedule". Error location: styles/components/_index.scss:12:1 (imported from styles/main.scss:13:1).
🤖 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 `@styles/components/_index.scss` at line 12, The `@forward` "live-schedule"
statement on line 12 references a stylesheet that Sass cannot locate, causing
build failures. Verify that the live-schedule stylesheet file exists in the
expected location relative to the _index.scss file (typically as
_live-schedule.scss in the same directory). If the file exists, correct the path
in the `@forward` statement to match the actual file location. If the file does
not exist, remove the `@forward` statement entirely. Ensure all forwarded
stylesheets in the _index.scss file have corresponding files that Sass can
resolve.
Source: Pipeline failures
1 participant
Description
Please describe the changes in this PR. Include motivation, problem addressed, and details of the solution.
Checklist
npm run lint,npm run format:check)anytypes eradicatedTesting Performed
Please provide a brief output or description of the tests run to verify the change.
Summary by CodeRabbit
New Features
Tests