-
Notifications
You must be signed in to change notification settings - Fork 540
Eventlist hotfix #1338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eventlist hotfix #1338
Changes from all commits
48d24e7
539c65f
4ee6080
d03c581
57a63bc
3df15f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -37,7 +37,7 @@ export function CalendarView({ date, sessions, events, onNavigate }: CalendarVie | |||||||||||||||
| onNavigate({ date: today.toISOString() }); | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| const weekDays = ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"]; | ||||||||||||||||
| const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"]; | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainWeek starts on Monday: verify grid/body alignment and consider locale-aware labels Switching to Monday-first is fine, but please verify WorkspaceCalendar renders days in the same order to avoid header/body misalignment. Also consider deriving labels from locale instead of hard-coded English abbreviations. Run this to locate potential Sunday-first assumptions and duplicated weekday arrays: Optionally, replace the static array with a locale-aware, Monday-first calculation: - const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"];
+ const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"]; // TODO(why): Hard-coded for hotfix; consider locale-aware EEE with weekStartsOn=1.If you want the full locale-aware refactor, I can provide a minimal diff using date-fns format/startOfWeek. 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect WorkspaceCalendar implementation and any weekday-related logic
fd -t f 'workspace-calendar' | xargs -I{} sh -c 'echo "== {} =="; rg -n "week|weekday|startOfWeek|Sun|Mon|getDay" "{}" || true'
# Search for hard-coded weekday arrays across the repo
rg -n --glob '!node_modules' --glob '!*dist*' --glob '!*build*' --glob '!*target*' '"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"|weekDays'Length of output: 763 Fix weekday-order mismatch — calendar header is Monday-first but shared util is Sunday-first Found conflicting weekday arrays — this will risk header/body misalignment. Unify to a single source-of-truth and prefer locale-aware labels. Files to check
Actionable suggestions
Minimal inline replacement example (calendar-view.tsx) - const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"];
+ // locale-aware, Monday-first (example using Intl)
+ const weekDays = (() => {
+ const fmt = new Intl.DateTimeFormat(undefined, { weekday: "short" });
+ const monday = new Date(Date.UTC(2020, 0, 6)); // known Monday anchor
+ return Array.from({ length: 7 }).map((_, i) => fmt.format(new Date(monday.getTime() + i * 86400000)));
+ })();If you want, I can provide a PR-ready refactor that either (a) exports a locale-aware getter from packages/utils/src/datetime.ts or (b) uses date-fns (format/startOfWeek) to standardize weekStartsOn across the codebase. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
|
|
||||||||||||||||
| return ( | ||||||||||||||||
| <div className="flex flex-col h-full"> | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -226,6 +226,12 @@ impl CalendarSource for Handle { | |||||||||||||
| return None; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // experiment: check if the event is recurring | ||||||||||||||
| let is_recurring = unsafe { | ||||||||||||||
| let has_rules: Bool = msg_send![event, hasRecurrenceRules]; | ||||||||||||||
| has_rules.as_bool() | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
|
Comment on lines
+229
to
+234
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainRecurrence detection: confirm API availability and trim the comment (use “why”). Calling hasRecurrenceRules via msg_send is reasonable, but please confirm it’s available across the minimum iOS/macOS targets you support. If any target lacks this selector, consider a fallback check using recurrenceRules != nil. Also, the comment “experiment” doesn’t convey why we’re doing this. Replace with a short “why”. 🌐 Web query: 💡 Result: EKEvent.hasRecurrenceRules is not an officially documented property by Apple for EventKit; it does not appear in Apple’s EventKit class reference or official documentation, and therefore its cross-platform or minimum version availability cannot be confirmed from authoritative Apple sources. Official Apple Documentation
Availability and Platform Version
Context in the Ecosystem
Summary Table
References to recurrence rules in Apple documentation always use the recurrenceRules array and never a hasRecurrenceRules Boolean property.[4][7][5] To check for recurring events, use If you need confirmation from official Apple Developer documentation, see the Citations:
Recurrence detection: use documented EKEvent.hasRecurrenceRules is not an Apple-documented API and its availability cannot be relied on. Use the documented
Suggested replacement snippet: // why: use the documented `recurrenceRules` to determine if an event is recurring
let is_recurring = unsafe {
let rules: *mut objc::runtime::Object = msg_send![event, recurrenceRules];
if rules.is_null() {
false
} else {
let count: usize = msg_send![rules, count];
count > 0
}
};Apply this change (or at minimum check 🤖 Prompt for AI Agents |
||||||||||||||
| let participants = unsafe { event.attendees().unwrap_or_default() }; | ||||||||||||||
| let participant_list: Vec<Participant> = participants | ||||||||||||||
| .iter() | ||||||||||||||
|
|
@@ -247,6 +253,7 @@ impl CalendarSource for Handle { | |||||||||||||
| start_date: offset_date_time_from(start_date), | ||||||||||||||
| end_date: offset_date_time_from(end_date), | ||||||||||||||
| google_event_url: None, | ||||||||||||||
| is_recurring, | ||||||||||||||
| }) | ||||||||||||||
| }) | ||||||||||||||
| .sorted_by(|a, b| a.start_date.cmp(&b.start_date)) | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -45,6 +45,8 @@ pub struct Event { | |||||
| pub start_date: DateTime<Utc>, | ||||||
| pub end_date: DateTime<Utc>, | ||||||
| pub google_event_url: Option<String>, | ||||||
| #[serde(default)] | ||||||
| pub is_recurring: bool, | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding a new non-optional field to a serde-deserializable struct will break deserialization of existing data that doesn’t include this field. Provide a default or make it optional to maintain backward compatibility. Prompt for AI agents
Suggested change
|
||||||
| } | ||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,4 @@ | ||||||||||||||||||
| ALTER TABLE | ||||||||||||||||||
| events | ||||||||||||||||||
| ADD | ||||||||||||||||||
| COLUMN is_recurring BOOLEAN DEFAULT FALSE; | ||||||||||||||||||
|
Comment on lines
+1
to
+4
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Make the new column NOT NULL with a safe default (backward-compatible and type-safe) As written, existing rows may end up with NULLs depending on engine/version behavior. Given the Rust type is bool (non-Option), enforce NOT NULL and set a default. Recommended:
-ALTER TABLE
- events
-ADD
- COLUMN is_recurring BOOLEAN DEFAULT FALSE;
+ALTER TABLE
+ events
+ADD
+ COLUMN is_recurring BOOLEAN NOT NULL DEFAULT FALSE;
📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ user_common_derives! { | |
| pub end_date: DateTime<Utc>, | ||
| pub google_event_url: Option<String>, | ||
| pub participants: Option<String>, | ||
| pub is_recurring: bool, | ||
| } | ||
|
Comment on lines
+17
to
18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainNew non-optional bool requires DB non-null guarantee Adding a non-optional bool is good, but it must be backed by a NOT NULL column (with a default) to avoid deserialization failures when reading legacy rows.
- pub is_recurring: bool,
+ #[serde(default)]
+ pub is_recurring: bool,Note: Confirm user_common_derives! passes field attributes through as-is. 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Searching for occurrences of is_recurring ==="
rg --hidden -n "is_recurring" || true
echo
echo "=== Show file crates/db-user/src/events_types.rs (if exists) ==="
if [ -f "crates/db-user/src/events_types.rs" ]; then
sed -n '1,240p' crates/db-user/src/events_types.rs || true
else
echo "File not found: crates/db-user/src/events_types.rs"
fi
echo
echo "=== Search for migrations mentioning events or is_recurring ==="
rg --hidden -n "events_migration|events_migration_2.sql|events_migration_2|ALTER TABLE .*is_recurring|is_recurring.*NOT NULL|is_recurring.*DEFAULT" || true
echo
echo "=== Search for user_common_derives macro definition/usages ==="
rg --hidden -n "user_common_derives" || true
echo
echo "=== Try to show macro definition(s) for user_common_derives ==="
# Find files that contain the macro_rules or proc macro
rg --hidden -n "macro_rules! user_common_derives|proc_macro.*user_common_derives|fn user_common_derives" || true
# If found, print nearby context for each file
FILES=$(rg --hidden -n "user_common_derives" -l || true)
if [ -n "$FILES" ]; then
for f in $FILES; do
echo
echo "---- $f ----"
sed -n '1,240p' "$f" || true
done
else
echo "No files containing user_common_derives found."
fi
echo
echo "=== Search for serde(default) usages ==="
rg --hidden -n "#\\[serde\\(default\\)\\]|serde\\(default\\)" || trueLength of output: 31353 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== events_migration_2.sql ==="
if [ -f "crates/db-user/src/events_migration_2.sql" ]; then
sed -n '1,240p' crates/db-user/src/events_migration_2.sql
else
echo "NOT FOUND: crates/db-user/src/events_migration_2.sql"
fi
echo
echo "=== events_migration_1.sql ==="
if [ -f "crates/db-user/src/events_migration_1.sql" ]; then
sed -n '1,240p' crates/db-user/src/events_migration_1.sql
else
echo "NOT FOUND: crates/db-user/src/events_migration_1.sql"
fi
echo
echo "=== events_migration.sql ==="
if [ -f "crates/db-user/src/events_migration.sql" ]; then
sed -n '1,240p' crates/db-user/src/events_migration.sql
else
echo "NOT FOUND: crates/db-user/src/events_migration.sql"
fi
echo
echo "=== crates/db-user/src/events_ops.rs ==="
if [ -f "crates/db-user/src/events_ops.rs" ]; then
sed -n '1,240p' crates/db-user/src/events_ops.rs
else
echo "NOT FOUND: crates/db-user/src/events_ops.rs"
fi
echo
echo "=== crates/db-user/src/init.rs (relevant insert lines) ==="
if [ -f "crates/db-user/src/init.rs" ]; then
sed -n '1,240p' crates/db-user/src/init.rs | nl -ba | sed -n '1,240p'
else
echo "NOT FOUND: crates/db-user/src/init.rs"
fiLength of output: 10034 Fix DB + serde + ops for new non-optional Short: the repo already added the column with DEFAULT FALSE, but the migration, deserialization safety, and DB persistence need fixes so the non-optional bool can't cause runtime/serialization bugs. Files to change
Notes
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weekday labels are hard-coded in English and the week start is fixed to Monday, which ignores user locale. Use locale-aware formatting for day names and derive the first day of week from the active locale to ensure correct labels for non-English users and regions that start weeks on Sunday.
Prompt for AI agents