♻️(caldav) remove tsdav library and refactor network path#58
Merged
Conversation
We were building too much workarounds around the core tsdav library, so we switch to a partial reimplementation for what actually matters for our more narrow usecase. Also cleanup various other deps and unify the network call path for all caldav requests
|
Warning Review limit reached
More reviews will be available in 15 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughRefactors frontend CalDAV integration: replace tsdav helpers with a unified DavClient/davRequest layer (XML building/parsing, error handling), simplify CalDAV types, rewrite CalDavService to use davRequest for PROPFIND/REPORT/MKCALENDAR/PROPPATCH/PUT/DELETE/MOVE with explicit ETag handling, add tests, and update integration points and Docker/npm config. ChangesCalDAV Client Refactoring
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/frontend/apps/calendars/src/features/calendar/services/dav/CalDavService.ts (1)
40-58: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRemove unused import
parseSharePrivilege.Static analysis indicates
parseSharePrivilegeis imported but never used in this file after the refactor.🧹 Proposed fix
import { buildProppatchXml, buildShareRequestXml, buildUnshareRequestXml, buildMkCalendarXml, buildCalendarQueryXml, escapeXml, CALENDAR_PROPS, NS, parseCalendarComponents, - parseSharePrivilege, parseInviteSharees, parseInviteOrganizerEmail, parseCalendarOrder, getCalendarUrlFromEventUrl, asResult, type ShareeXmlParams, type CalendarProps, } from './caldav-helpers'🤖 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 `@src/frontend/apps/calendars/src/features/calendar/services/dav/CalDavService.ts` around lines 40 - 58, Remove the unused import parseSharePrivilege from the import list at the top of CalDavService.ts (the import that pulls from './caldav-helpers'); update the import statement that currently includes parseSharePrivilege alongside buildProppatchXml, buildShareRequestXml, etc., to no longer reference parseSharePrivilege and run the linter/TypeScript compile to confirm no other references to parseSharePrivilege remain.src/frontend/apps/calendars/src/features/api/fetchApi.ts (1)
32-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent permanent redirect lock if sessionStorage write fails.
On Line 33,
redirectInFlightis set beforesessionStorage.setItem(Line 34). If storage throws, the function exits and future 401s are no-ops, so users may never be redirected to login in that runtime.Suggested fix
export function redirectToLogin() { if (redirectInFlight) return; - redirectInFlight = true; - sessionStorage.setItem( - SESSION_STORAGE_REDIRECT_AFTER_LOGIN_URL, - window.location.href, - ); + redirectInFlight = true; + try { + sessionStorage.setItem( + SESSION_STORAGE_REDIRECT_AFTER_LOGIN_URL, + window.location.href, + ); + } catch { + // ignore storage failures; redirect should still proceed + } window.location.replace(new URL("authenticate/", baseApiUrl()).href); }🤖 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 `@src/frontend/apps/calendars/src/features/api/fetchApi.ts` around lines 32 - 38, The code sets the module-level flag redirectInFlight before writing to sessionStorage which can throw and leave redirectInFlight true permanently; change the logic in the 401 handling around redirectInFlight/SESSION_STORAGE_REDIRECT_AFTER_LOGIN_URL so that sessionStorage.setItem is attempted inside a try/catch and redirectInFlight is only set after a successful write (or set regardless if the write fails), and always call window.location.replace(new URL("authenticate/", baseApiUrl()).href) in the finally/error path; specifically update the block that references redirectInFlight, sessionStorage.setItem, SESSION_STORAGE_REDIRECT_AFTER_LOGIN_URL, and window.location.replace/baseApiUrl() so a thrown storage error does not prevent future redirects.
🤖 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 `@src/frontend/apps/calendars/package.json`:
- Line 35: Update the dependency entry for xml-js in package.json to pin it to
an exact version instead of a caret range: replace the semver spec "^1.6.11" for
"xml-js" with the exact version "1.6.11" so installs are reproducible and
consistent with the rest of the file.
- Line 35: The dependency "xml-js": "^1.6.11" in package.json is old (last
published 2019) so confirm its acceptability for CalDAV parsing by: 1) running
Snyk/OSV/npm audit to verify no advisories for xml-js@1.6.11, 2) documenting the
outcome (keep or replace) in the repo (e.g., dependency decision note), and 3)
if you choose to replace, swap xml-js with a maintained XML library (e.g.,
fast-xml-parser) and update all CalDAV parsing code that imports/uses xml-js to
the new parser APIs; reference the "xml-js" entry in package.json and the CalDAV
parsing modules when making changes.
In
`@src/frontend/apps/calendars/src/features/calendar/services/dav/CalDavService.ts`:
- Around line 1100-1103: Remove the now-unused sourceCalendar variable and its
no-op suppression from CalDavService: delete the sourceCalendar variable
declaration/assignment (the earlier assignment that was only used for fallback
semantics) and remove the `void sourceCalendar` line where it was used to
silence warnings; ensure any logic relying on davRequest and shared session auth
remains intact (refer to symbols sourceCalendar, davRequest and class
CalDavService to locate the affected code).
---
Outside diff comments:
In `@src/frontend/apps/calendars/src/features/api/fetchApi.ts`:
- Around line 32-38: The code sets the module-level flag redirectInFlight before
writing to sessionStorage which can throw and leave redirectInFlight true
permanently; change the logic in the 401 handling around
redirectInFlight/SESSION_STORAGE_REDIRECT_AFTER_LOGIN_URL so that
sessionStorage.setItem is attempted inside a try/catch and redirectInFlight is
only set after a successful write (or set regardless if the write fails), and
always call window.location.replace(new URL("authenticate/", baseApiUrl()).href)
in the finally/error path; specifically update the block that references
redirectInFlight, sessionStorage.setItem,
SESSION_STORAGE_REDIRECT_AFTER_LOGIN_URL, and
window.location.replace/baseApiUrl() so a thrown storage error does not prevent
future redirects.
In
`@src/frontend/apps/calendars/src/features/calendar/services/dav/CalDavService.ts`:
- Around line 40-58: Remove the unused import parseSharePrivilege from the
import list at the top of CalDavService.ts (the import that pulls from
'./caldav-helpers'); update the import statement that currently includes
parseSharePrivilege alongside buildProppatchXml, buildShareRequestXml, etc., to
no longer reference parseSharePrivilege and run the linter/TypeScript compile to
confirm no other references to parseSharePrivilege remain.
🪄 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
Run ID: 9c919bd6-a3c0-4808-9472-cdd73d4f8a07
⛔ Files ignored due to path filters (1)
src/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
.gitignoresrc/frontend/Dockerfilesrc/frontend/apps/calendars/package.jsonsrc/frontend/apps/calendars/src/features/api/fetchApi.tssrc/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsxsrc/frontend/apps/calendars/src/features/calendar/services/dav/CalDavService.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/VCardComponent.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/__tests__/CalDavService.connect.test.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/__tests__/CalDavService.moveEvent.test.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/__tests__/caldav-helpers.test.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/__tests__/event-calendar-helper.test.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/__tests__/ics-helper.test.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/__tests__/timezone-conversion.test.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/caldav-helpers.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/helpers/dav-helper.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/helpers/event-calendar-helper.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/helpers/ics-helper.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/helpers/index.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/helpers/types-helper.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/types/addressbook.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/types/caldav-service.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/types/calendar.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/types/options.tssrc/frontend/apps/calendars/src/features/calendar/utils/DavClient.tssrc/frontend/apps/calendars/src/features/calendar/utils/__tests__/DavClient.test.tssrc/frontend/apps/calendars/src/features/resources/api/useResourcePrincipals.tssrc/frontend/apps/calendars/src/features/settings/api/useWorkingHours.tssrc/frontend/apps/calendars/src/styles/globals.scss
💤 Files with no reviewable changes (13)
- src/frontend/apps/calendars/src/features/calendar/services/dav/tests/event-calendar-helper.test.ts
- src/frontend/apps/calendars/src/features/calendar/services/dav/types/calendar.ts
- src/frontend/apps/calendars/src/features/calendar/services/dav/tests/ics-helper.test.ts
- src/frontend/apps/calendars/src/features/calendar/services/dav/types/addressbook.ts
- src/frontend/apps/calendars/src/features/calendar/services/dav/tests/timezone-conversion.test.ts
- src/frontend/apps/calendars/src/features/calendar/services/dav/helpers/dav-helper.ts
- src/frontend/apps/calendars/src/features/calendar/services/dav/helpers/ics-helper.ts
- src/frontend/apps/calendars/src/features/calendar/services/dav/helpers/index.ts
- src/frontend/apps/calendars/src/features/calendar/services/dav/VCardComponent.ts
- src/frontend/apps/calendars/src/styles/globals.scss
- src/frontend/apps/calendars/src/features/calendar/services/dav/helpers/types-helper.ts
- src/frontend/apps/calendars/src/features/calendar/services/dav/types/options.ts
- src/frontend/apps/calendars/src/features/calendar/services/dav/helpers/event-calendar-helper.ts
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
1 participant
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.
We were building too much workarounds around the core tsdav library, so we switch to a partial reimplementation for what actually matters for our more narrow usecase. Also cleanup various other deps and unify the network call path for all caldav requests
Summary by CodeRabbit
Bug Fixes
Infrastructure
Refactoring
Chores