Fix/delete selected calendar error propagation#29487
Conversation
|
Welcome to Cal.diy, @SahilArate! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR hardens the security of parent-child iframe communication in the embed SDK by implementing origin validation, refactors embed state management hooks to prevent unintended re-renders, filters reserved parameters from query forwarding in modal routing, and improves error handling in the calendar service. The changes reorganize code structure across the embed iframe and embed client files while maintaining existing functionality, with security-critical updates to message validation in both directions (iframe-to-parent and parent-to-iframe postMessage calls). 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
looks good to me |
2 participants
What does this PR do?
Fixes #29456
The catch block in deleteSelectedCalendar correctly handled two known errors (NO_SELECTED_CALENDAR_FOUND, MULTIPLE_SELECTED_CALENDARS_FOUND) but silently swallowed all other unexpected errors, causing the function to implicitly return undefined on database failures or runtime errors. Callers had no way to detect the deletion had failed.
Fixed by throwing InternalServerErrorException for any unrecognized error, consistent with the NestJS HTTP exception pattern already used in this service (NotFoundException, BadRequestException). Also handles the edge case where the thrown value is not an Error instance.
Visual Demo
Image Demo:
Before (Buggy):
DB crash → catch block → no matching error message → silent undefined returned ❌
Caller has no idea the deletion failed
After (Fixed):
DB crash → catch block → no matching error message → InternalServerErrorException thrown ✅
Caller receives HTTP 500 with a clear error message
Code change:
typescript// BEFORE — falls off silently
} catch (error) {
if (error instanceof Error) {
if (error.message === NO_SELECTED_CALENDAR_FOUND) {
throw new NotFoundException(NO_SELECTED_CALENDAR_FOUND);
} else if (error.message === MULTIPLE_SELECTED_CALENDARS_FOUND) {
throw new BadRequestException(MULTIPLE_SELECTED_CALENDARS_FOUND);
}
}
// ❌ returns undefined silently here
}
// AFTER — unexpected errors propagate correctly
} catch (error) {
if (error instanceof Error) {
if (error.message === NO_SELECTED_CALENDAR_FOUND) {
throw new NotFoundException(NO_SELECTED_CALENDAR_FOUND);
} else if (error.message === MULTIPLE_SELECTED_CALENDARS_FOUND) {
throw new BadRequestException(MULTIPLE_SELECTED_CALENDARS_FOUND);
}
}
// ✅ Re-throw unexpected errors so callers are aware the deletion failed
throw new InternalServerErrorException(
error instanceof Error
? error.message
: "An unexpected error occurred while deleting the selected calendar"
);
}
Mandatory Tasks (DO NOT REMOVE)
I have self-reviewed the code.
N/A — no documentation change required for this bug fix.
I confirm automated tests are in place that prove my fix is effective.
How should this be tested?
No special environment variables needed
Need a valid user with at least one connected calendar credential
To reproduce the bug: mock removeUserSelectedCalendar to throw new Error("DB_CONNECTION_FAILED") — before fix it returns undefined silently, after fix it throws 500 InternalServerErrorException
Happy path (unchanged): calendar not found → 404, multiple found → 400, success → returns removed calendar entry
Checklist
I have read the contributing guide
My code follows the style guidelines of this project
I have commented my code where relevant
My changes generate no new warnings
My PR is small and focused — 1 file changed, 2 lines added