[WEB-5158] chore: calendar startofweek alignment fix and code refactoring #7978
Conversation
anmolsinghbhatia
requested review from
sriramveeraghanta and
vamsikrishnamathala
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
WalkthroughThe changes introduce configurable week-start day support to the calendar system. CalendarStore now accepts a root store reference, establishes a reactive watch on the user's Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Calendar as CalendarStore
participant RootStore as RootStore
participant Reaction as MobX Reaction
participant Utils as generateCalendarData
User->>Calendar: User changes start_of_the_week preference
activate Calendar
Calendar->>RootStore: Preference update propagates
activate RootStore
RootStore->>Reaction: start_of_the_week changed
deactivate RootStore
activate Reaction
Reaction->>Calendar: Trigger regenerateCalendar()
activate Calendar
Calendar->>Calendar: Clear cached calendar data
Calendar->>Utils: Call generateCalendarData(activeMonthDate, newStartOfWeek)
activate Utils
Utils->>Utils: Compute firstDayOfMonth with new alignment
Utils->>Utils: Generate weeks with new grouping
Utils-->>Calendar: Return updated ICalendarPayload
deactivate Utils
Calendar->>Calendar: Update calendar state
Calendar-->>User: Calendar UI refreshes
deactivate Calendar
deactivate Reaction
deactivate Calendar
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span three distinct files with cohesive logic focused on one feature. The CalendarStore modifications introduce reactive patterns and dependency injection that warrant careful review. The utility function extends calendar alignment logic with week-start computation. While not trivial, the changes follow a clear architectural pattern and avoid high-density complexity. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Pull Request Overview
Fix calendar start-of-week alignment and refactor calendar generation to respect user preference.
- Add startOfWeek support to calendar generation and adjust first-day offset calculations.
- Regenerate calendar on user preference changes; wire CalendarStore to IssueRootStore.
- Switch week keys to sequential indices within the month for consistency across start-of-week settings.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/utils/src/calendar.ts | Add startOfWeek param, adjust first-day offset, reset month data, and use sequential week keys. |
| apps/web/core/store/issue/root.store.ts | Pass IssueRootStore into CalendarStore constructor. |
| apps/web/core/store/issue/issue_calendar_view.store.ts | Inject root store, use startOfWeek from user profile, add reaction to regenerate calendar, and add regenerateCalendar method. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
apps/web/core/store/issue/issue_calendar_view.store.ts (1)
115-123: Potential bug: activeWeekNumber calculation may not align with sequential week keys.The
allDaysOfActiveWeekgetter usesthis.activeWeekNumber - 1to access the week data, but the week keys in the calendar payload are now sequential indices (w-0,w-1, etc.) rather than calculated week numbers. This could cause a mismatch.For example:
- If
activeWeekNumber(fromgetWeekNumberOfDate) is 42, the code tries to accessw-41- But the actual keys are
w-0,w-1,w-2, etc.To verify this issue, check how week keys are used elsewhere:
#!/bin/bash # Search for week key access patterns in the codebase ast-grep --pattern $'calendarPayload[$$$][`w-${$_}`]' # Also check getWeekNumberOfDate usage rg -nP --type=ts 'getWeekNumberOfDate' -A 2 -B 2The fix should align the week key access with the sequential indexing used in
generateCalendarData. Consider either:
- Calculating the week index from the date and active month start, or
- Storing a mapping from week numbers to sequential indices
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/core/store/issue/issue_calendar_view.store.ts(5 hunks)apps/web/core/store/issue/root.store.ts(1 hunks)packages/utils/src/calendar.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/core/store/issue/root.store.ts (1)
apps/web/core/store/issue/issue_calendar_view.store.ts (1)
CalendarStore(34-189)
apps/web/core/store/issue/issue_calendar_view.store.ts (2)
apps/web/core/store/issue/root.store.ts (1)
IIssueRootStore(48-114)packages/utils/src/calendar.ts (1)
generateCalendarData(13-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
packages/utils/src/calendar.ts (4)
13-17: LGTM! Function signature correctly extended.The addition of the
startOfWeekparameter with a sensible default value ofEStartOfTheWeek.SUNDAYmaintains backward compatibility while enabling the new functionality.
27-31: Verify the firstDayOfMonth calculation logic.The calculation
(firstDayOfMonthRaw - startOfWeek + 7) % 7determines the number of empty cells needed at the start of the calendar. This formula should correctly handle all combinations offirstDayOfMonthRaw(0-6) andstartOfWeek(0-6).Let me verify the calculation logic is correct for all possible inputs:
#!/usr/bin/env python3 # Verify the firstDayOfMonth calculation for all possible combinations print("Verifying firstDayOfMonth calculation:") print("Format: firstDayOfMonthRaw | startOfWeek | Result | Expected") print("-" * 60) for raw in range(7): # Sunday(0) to Saturday(6) for start in range(7): # All possible start days result = (raw - start + 7) % 7 # Expected: number of empty cells before the first day # If raw == start, we expect 0 empty cells # If raw > start, we expect raw - start empty cells # If raw < start, we expect (7 - start + raw) empty cells expected = (raw - start + 7) % 7 status = "✓" if result == expected else "✗" print(f"{status} {raw:19} | {start:11} | {result:6} | {expected:8}") print("\nTest scenarios:") # Sunday start, Sunday first day (no offset) print(f"Sunday(0) start, Sunday(0) first: {(0 - 0 + 7) % 7} empty cells (expect 0)") # Monday start, Sunday first day (6 offset) print(f"Monday(1) start, Sunday(0) first: {(0 - 1 + 7) % 7} empty cells (expect 6)") # Sunday start, Monday first day (1 offset) print(f"Sunday(0) start, Monday(1) first: {(1 - 0 + 7) % 7} empty cells (expect 1)")
34-35: Month data reset ensures clean regeneration.Resetting the month data on each call to
generateCalendarDataensures that any change instartOfWeekis properly reflected without stale data from previous calculations. This is the correct approach for the regeneration workflow.
63-65: Week key normalization improves consistency.Using sequential week indices (
w-${week}) instead of calculated week numbers (w-${weekNumber}) ensures consistent week grouping regardless of thestartOfWeekpreference. This is a good improvement that prevents potential key collisions or ordering issues.apps/web/core/store/issue/root.store.ts (1)
269-269: LGTM! Root store reference correctly passed.The change from
new CalendarStore()tonew CalendarStore(this)correctly provides the CalendarStore with access to the root store, enabling it to read user preferences likestart_of_the_week.apps/web/core/store/issue/issue_calendar_view.store.ts (6)
1-1: LGTM! Necessary imports added.The imports for
reaction,EStartOfTheWeek, andIIssueRootStoreare correctly added to support the new functionality.Also applies to: 6-6, 9-9
21-21: Interface correctly extended.The
ICalendarStoreinterface is properly updated to include the newregenerateCalendarmethod.
45-46: Root store wiring implemented correctly.The root store reference is properly stored and the constructor signature is updated to accept it. This enables access to user preferences throughout the calendar store.
Also applies to: 48-48, 68-68
158-158: Consistent null fallback for start_of_the_week.The code correctly falls back to
EStartOfTheWeek.SUNDAYwhen the user preference is not available. This ensures the calendar always has a valid start-of-week value.Also applies to: 166-166
161-161: LGTM! startOfWeek parameter correctly passed.The
startOfWeekvalue is correctly passed togenerateCalendarDatain bothupdateCalendarPayloadandinitCalendarmethods, ensuring calendar data respects user preferences.Also applies to: 167-167
174-188: Regeneration logic is correct.The
regenerateCalendarmethod correctly:
- Reads the current
startOfWeekpreference- Uses the current
activeMonthDateto maintain the user's view- Passes
nulltogenerateCalendarDatato force complete regeneration- Updates the calendar payload in a MobX action
3 participants
Description
This PR fixes calendar startofweek alignment and code refactoring.
Type of Change
Media
Summary by CodeRabbit
Bug Fixes
Refactor