Fix (Calendar) overwrite current events with new updated/inserted Google events #3079
Conversation
WalkthroughThis change reorganizes the order of properties within argument and return type definitions in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CalendarSyncProvider
participant Cache
participant Database
participant GoogleAPI
User->>CalendarSyncProvider: Request events (db or Google)
CalendarSyncProvider->>Cache: Check dbLastUpdated / googleLastUpdated
alt Cache stale or forced
CalendarSyncProvider->>Database: Fetch DB events
CalendarSyncProvider->>GoogleAPI: Fetch Google events
Database-->>CalendarSyncProvider: Return events
GoogleAPI-->>CalendarSyncProvider: Return events
CalendarSyncProvider->>Cache: Update dbLastUpdated / googleLastUpdated
else Cache fresh
CalendarSyncProvider->>Cache: Return cached events
end
CalendarSyncProvider-->>User: Return events
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
packages/types/src/supabase.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@typescript-eslint/parser' imported from /eslint.config.mjs packages/ui/src/hooks/use-calendar-sync.tsxOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@typescript-eslint/parser' imported from /eslint.config.mjs ✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/ui/src/hooks/use-calendar-sync.tsx (1)
564-602: 💡 Verification agent🧩 Analysis chain
Potential race condition between sync and refresh cycles.
The
syncToTuturuuufunction callsrefresh()after upserting (line 565), which triggers new fetches from both database and Google. Since this function is called in auseEffectwhen Google data changes (lines 598), there's a risk of creating a loop if the newly fetched Google data differs from what was just synced.Consider adding a mechanism to prevent immediate re-sync after a sync operation completes.
🌐 Web query:
What are best practices for preventing sync loops in React when dealing with external data sources that may have slight variations in data between fetches?💡 Result:
Preventing sync loops in React when dealing with external data sources that might have slight variations between fetches is crucial to avoid unnecessary re-renders or network requests. Here are best practices to handle this effectively:
Deep Comparison Before State Update
Instead of blindly setting state every time you fetch data, compare the incoming data with the current state deeply. Only update the state if there are meaningful changes. Libraries likelodash.isequalcan help with deep comparison.Use Stable State Structures
Ensure your state shape and data structure remain consistent between updates to avoid unnecessary diffs. Normalize data if needed.Debounce or Throttle Fetches
If data fetches happen on user input or events, debounce or throttle the trigger to reduce frequency and avoid rapid update loops.Memoize Data and Callbacks
UseuseMemoanduseCallbackto prevent changes in dependencies that trigger effects unnecessarily.Handle Slight Variations Gracefully
If the external data source has non-critical variations (e.g., timestamps, IDs with different sorting), transform or sanitize data before comparing or setting state.Use Effect Cleanup and Dependency Arrays Correctly
Ensure youruseEffectdependencies are accurate. Avoid putting non-stable objects or functions directly in the dependency array to prevent re-runs.Avoid State Updates in Render Phase
Ensure state updates are only done inside effects or event handlers, not during render, to prevent infinite loops.Use a Persistent Reference for External Data
Store the last received data in auseRefand compare new data against it before updating state.Consider Immutable Data Structures
Using immutable.js or similar can help detect real changes more easily.Implement Retry or Back-off Logic
If fetches are automatic, implement back-off strategies to reduce load and avoid frequent updates.
Would you like a code example demonstrating how to implement deep comparison before updating state or any other specific pattern?
Prevent infinite sync loops between
syncToTuturuuuandrefresh()The current flow in packages/ui/src/hooks/use-calendar-sync.tsx (lines 564–602) is prone to a loop:
syncToTuturuuu()upserts events then callsrefresh().refresh()invalidates queries, which may refetch Google data with slight diffs.- The
useEffectonfetchedGoogleDatasees “changed” data and callssyncToTuturuuu()again.To break this cycle, I recommend:
• Guard against self-triggered refreshes
– Create auseRefflag (e.g.isRefreshTriggeredRef) and set it totruejust before callingrefresh().
– In youruseEffect, if that flag is set, reset it and skip callingsyncToTuturuuu().• Deep-compare only meaningful data
– Normalize or sanitizefetchedGoogleData(strip timestamps, sort arrays) before stringifying or uselodash.isequalto avoid spurious diffs.• (Optional) Debounce/throttle rapid changes
– If Google data changes frequently, debounce theuseEffecttrigger.• Cleanup dependency array
– SinceuseQueryClient()returns a stable instance, you can safely removequeryClientfrom theuseCallbackdeps for clarity.Example snippet:
// at top of hook + const isRefreshTriggeredRef = useRef(false); const syncToTuturuuu = useCallback(async () => { try { setIsSyncing(true); + isRefreshTriggeredRef.current = true; // …upsert logic… refresh();useEffect(() => { if (experimentalGoogleToken?.ws_id !== wsId) return; + if (isRefreshTriggeredRef.current) { + isRefreshTriggeredRef.current = false; + return; + } const currentGoogleDataStr = JSON.stringify(normalize(fetchedGoogleData)); if (currentGoogleDataStr !== prevGoogleDataRef.current) { syncToTuturuuu(); prevGoogleDataRef.current = currentGoogleDataStr; } }, [fetchedGoogleData, syncToTuturuuu]);These changes ensure that a refresh-driven refetch doesn’t immediately retrigger a sync.
🧹 Nitpick comments (2)
packages/ui/src/hooks/use-calendar-sync.tsx (2)
763-781: Consider the asynchronous nature of state updates in debug logging.The debug logging is helpful, but be aware that the "After refresh" log (lines 776-781) might not show the updated values immediately since
updateCacheusessetStatewhich is asynchronous. The logged values will still reference the oldcacheDataobject.If you need to verify the actual updated state, consider moving the "After refresh" logging into a separate
useEffectthat watches thecalendarCachestate:- console.log('After refresh - cacheData:', { - isForced: cacheData?.isForced, - dbLastUpdated: cacheData?.dbLastUpdated, - googleLastUpdated: cacheData?.googleLastUpdated, - dbEventsLength: cacheData?.dbEvents?.length, - });Add a separate effect for debugging:
useEffect(() => { const cacheKey = getCacheKey(dates); const cacheData = calendarCache[cacheKey]; console.log('Cache state updated:', { cacheKey, isForced: cacheData?.isForced, dbLastUpdated: cacheData?.dbLastUpdated, googleLastUpdated: cacheData?.googleLastUpdated, dbEventsLength: cacheData?.dbEvents?.length, }); }, [calendarCache, dates]);
213-213: Consider implementing a proper logging solution for production.The code contains numerous
console.logstatements for debugging. While helpful during development, these should be removed or replaced with a configurable logging solution before deploying to production.Consider using a logging library or a debug flag:
const DEBUG = process.env.NODE_ENV === 'development'; const log = (...args) => DEBUG && console.log(...args);Also applies to: 280-280, 371-379, 415-423, 449-449, 485-509, 537-544, 561-561, 597-597, 616-616, 758-758, 763-781
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/types/src/supabase.ts(20 hunks)packages/ui/src/hooks/use-calendar-sync.tsx(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy-Preview
🔇 Additional comments (32)
packages/types/src/supabase.ts (28)
7349-7358: Consistent property ordering forgenerate_cross_app_tokenoverloads
Both overloads now listp_origin_app,p_target_app,p_expiry_seconds, andp_session_datain the same sequence. Types unchanged, only ordering for clarity.
7372-7376: Align return fields inget_daily_income_expense
total_expensehas been moved to followtotal_income, matching the convention used elsewhere. No type modifications.
7406-7412: Standardize return order inget_hourly_prompt_completion_tokens
Thehourfield is positioned before the token totals, consistent with similar metrics definitions.
7423-7427: Reorder Args forget_inventory_products
The parameters_ws_id,_warehouse_ids, and_has_unitare now listed immediately after_category_idsto maintain a predictable grouping.
7429-7437: Organize return fields inget_inventory_products
Return properties (id,name,manufacturer,unit,unit_id,category,price,amount) are reordered for consistency and easier scanning.
7460-7464: Maintain symmetry inget_monthly_income_expensereturns
total_expenseis now placed aftertotal_income, mirroring the daily version.
7466-7472: Align return order inget_monthly_prompt_completion_tokens
total_prompt_tokensfollowsmonth, consistent with other prompt/completion token endpoints.
7498-7504: Consistent fields ordering forget_session_statistics
active_countandcompleted_countfollowunique_users_count, matching the overall statistics pattern.
7514-7519: Reorder return fields inget_session_templates
category_nameis now listed right aftertags, aligning with the order of related category properties.
7528-7532: Align return properties inget_submission_statistics
unique_users_countmoved afterlatest_submission_dateto mirror other stats endpoints.
7534-7544: Organize return type inget_transaction_categories_with_amount
The fieldsws_id,created_at, andamountare reordered to follow the ID/name pair, improving readability.
7549-7555: Standardize fields inget_user_session_stats
active_sessionsnow comes immediately aftertotal_sessions, matching the naming sequence of related stats calls.
7571-7579: Consistent return ordering forget_user_tasks
The sequence (name,description,priority,completed,start_date,end_date,board_id) has been aligned to group related properties.
7591-7596: Reorder Args inget_workspace_transactions_count
start_dateandend_datenow followws_id, reflecting their optional nature after the required parameter.
7599-7603: Standardize Args inget_workspace_user_groups
search_queryis now placed after the included/excluded tag lists to keep filters grouped.
7607-7613: Align return fields inget_workspace_user_groups
created_atmoved to followtag_count, matching the layout used in other list endpoints.
7619-7624: Organize Args inget_workspace_users
included_groups,excluded_groups, andsearch_queryare ordered consistently with other workspace list calls.
7626-7647: Consistent field sequence inget_workspace_usersreturn type
Demographic and workspace fields (full_name,display_name,
7654-7659: Reorder Args inget_workspace_wallets_expense
Optionalstart_dateandend_dateare placed after the requiredws_idto clarify parameter importance.
7657-7663: Reorder Args inget_workspace_wallets_income
Mirrors the expense call: optional dates followws_id.
7662-7667: Standardize Args inhas_other_owner
Arguments_ws_idand_user_idare grouped in declaration order for consistency.
7666-7673: Align Args ininsert_ai_chat_message
chat_idandsourcenow followmessage, grouping chat-specific parameters.
7746-7753: Consistent overload definitions insearch_users
Both overloads now list filters (role_filter,enabled_filter) at the end, maintaining symmetry in paging parameters.
7780-7786: Reorder return fields insearch_users_by_name
handleprecedesdisplay_nameandrelevance, aligning with identification-first grouping.
7792-7796: Standardize Args fortransactions_have_same_abs_amount
Both transaction IDs are declared in a consistent left-to-right order.
7718-7723: Align Args innova_get_challenge_with_user_stats
challenge_idis listed beforeuser_id, matching other Nova endpoints.
7722-7727: Standardize Args innova_get_user_daily_sessions
Arguments reflect the same order as related Nova calls (challenge_id, thenuser_id).
7803-7808: Consistent Args inupdate_session_total_score
The parameterschallenge_id_paramanduser_id_paramare sequenced for clarity.packages/ui/src/hooks/use-calendar-sync.tsx (4)
81-89: Good separation of concerns for cache timestamps!Splitting the single
lastUpdatedinto separatedbLastUpdatedandgoogleLastUpdatedtimestamps allows for more granular cache management and staleness checking for each data source independently.
167-169: Improved cache freshness for current week events.The reduction of stale time from 2 minutes to 30 seconds for the current week ensures more timely updates. The inclusive condition (
>=) is also more precise.
213-250: Correct implementation of database-specific cache management.The changes properly utilize the new
dbLastUpdatedtimestamp for staleness checks and cache updates. The debug logging andisForcedflag reset are good additions for maintainability.
548-565: Important: Changed sync behavior to ensure data consistency.The function now triggers a full refresh after upserting events instead of directly updating the cache. This ensures the displayed data accurately reflects the database state after synchronization, which aligns with the PR objective to fix event overwriting issues. While this approach may be slightly less efficient, it provides better data consistency guarantees.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3079 +/- ##
==========================================
- Coverage 0.84% 0.84% -0.01%
==========================================
Files 2488 2488
Lines 306768 306773 +5
Branches 2990 2990
==========================================
Hits 2577 2577
- Misses 302063 302068 +5
Partials 2128 2128 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit