fix: add isDryRun behaviour for date overrides#24464
Conversation
keithwillcode
added
core
WalkthroughAn optional Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![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)
packages/features/schedules/components/DateOverrideInputDialog.tsx (1)
185-187: Toast notification misleads users in dry-run mode.The success toast is displayed regardless of
isDryRunstatus. When in dry-run mode, no actual changes are persisted, so showing a "success" message could mislead users into thinking their changes were saved.Apply this diff to conditionally show the toast:
<Button className="ml-2" color="primary" type="submit" onClick={() => { - showToast(t("date_successfully_added"), "success", 500); + if (!isDryRun) { + showToast(t("date_successfully_added"), "success", 500); + } }} disabled={selectedDates.length === 0} data-testid="add-override-submit-btn">
🧹 Nitpick comments (2)
packages/features/schedules/components/DateOverrideInputDialog.tsx (1)
241-241: Prefer named export over default export.As per coding guidelines, named exports provide better tree-shaking, easier refactoring, and clearer imports.
Apply this diff:
-export default DateOverrideInputDialog; +export { DateOverrideInputDialog };Then update imports in consuming files from:
import DateOverrideInputDialog from "./DateOverrideInputDialog";to:
import { DateOverrideInputDialog } from "./DateOverrideInputDialog";packages/features/schedules/components/DateOverrideList.tsx (1)
165-165: Prefer named export over default export.As per coding guidelines, named exports provide better tree-shaking, easier refactoring, and clearer imports.
Apply this diff:
-export default DateOverrideList; +export { DateOverrideList };Then update imports in consuming files from:
import DateOverrideList from "./DateOverrideList";to:
import { DateOverrideList } from "./DateOverrideList";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/features/schedules/components/DateOverrideInputDialog.tsx(5 hunks)packages/features/schedules/components/DateOverrideList.tsx(4 hunks)packages/platform/atoms/availability/AvailabilitySettings.tsx(8 hunks)packages/platform/atoms/availability/wrappers/AvailabilitySettingsPlatformWrapper.tsx(1 hunks)packages/platform/examples/base/src/pages/availability.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/platform/examples/base/src/pages/availability.tsxpackages/features/schedules/components/DateOverrideList.tsxpackages/features/schedules/components/DateOverrideInputDialog.tsxpackages/platform/atoms/availability/AvailabilitySettings.tsxpackages/platform/atoms/availability/wrappers/AvailabilitySettingsPlatformWrapper.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/platform/examples/base/src/pages/availability.tsxpackages/features/schedules/components/DateOverrideList.tsxpackages/features/schedules/components/DateOverrideInputDialog.tsxpackages/platform/atoms/availability/AvailabilitySettings.tsxpackages/platform/atoms/availability/wrappers/AvailabilitySettingsPlatformWrapper.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/platform/examples/base/src/pages/availability.tsxpackages/features/schedules/components/DateOverrideList.tsxpackages/features/schedules/components/DateOverrideInputDialog.tsxpackages/platform/atoms/availability/AvailabilitySettings.tsxpackages/platform/atoms/availability/wrappers/AvailabilitySettingsPlatformWrapper.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (4)
packages/features/schedules/components/DateOverrideInputDialog.tsx (1)
114-136: LGTM! Submission logic correctly gated.The logic correctly prevents
onChangefrom being called whenisDryRunis true, ensuring no side effects occur during dry-run mode.packages/platform/examples/base/src/pages/availability.tsx (1)
52-53: LGTM! Example demonstrates dry-run usage.The commented
isDryRunprop serves as helpful documentation for consumers of theAvailabilitySettingscomponent.packages/platform/atoms/availability/wrappers/AvailabilitySettingsPlatformWrapper.tsx (1)
36-36: LGTM! isDryRun correctly propagated.The wrapper properly threads the
isDryRunprop through to the nestedAvailabilitySettingscomponent with an appropriate default value.Also applies to: 59-59, 206-206
packages/platform/atoms/availability/AvailabilitySettings.tsx (1)
128-128: LGTM! isDryRun properly integrated throughout.The
isDryRunprop is correctly added to the component interface and consistently propagated to all nested date override components (DateOverrideList,DateOverrideInputDialog). The submission logic at lines 219-221 properly gateshandleSubmitto prevent updates during dry-run mode.Also applies to: 194-194, 208-208, 219-221, 248-248, 260-260, 318-318, 686-686
| if (selectedDates.length === 0) return; | ||
|
|
||
| if (datesUnavailable) { | ||
| if (!isDryRun && datesUnavailable) { |
There was a problem hiding this comment.
Instead of adding !isDryRun to current if and else & having duplicate onChange(datesInRanges); you could simply update line 112 to simplify code:
if (selectedDates.length === 0 || isDryRun) return;
There was a problem hiding this comment.
this won't work because we still need to set selected dates to an empty array at line 138, so I did this instead
if (selectedDates.length === 0) return;
if (isDryRun) {
setSelectedDates([]);
return;
}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/features/schedules/components/DateOverrideInputDialog.tsx (1)
114-117: Clarify the intent of clearing selectedDates during dry run.The past review suggested simplifying by combining the checks at lines 112 and 114-117 into
if (selectedDates.length === 0 || isDryRun) return;. However, your current implementation clearsselectedDatesbefore returning whenisDryRunis true (line 115).Is this clearing behavior intentional to reset the UI state and provide visual feedback that the action was acknowledged but not executed? If so, the current approach is correct. If not, the past review's suggestion would simplify the code.
🧹 Nitpick comments (1)
packages/features/schedules/components/DateOverrideInputDialog.tsx (1)
234-240: Note: isDryRun is forwarded implicitly via spread operator.The
isDryRunprop is correctly passed toDateOverrideFormvia{...passThroughProps}(line 237). While this works correctly, consider explicitly passingisDryRunfor better code clarity and easier traceability, similar to howexcludedDates,weekStart, anduserTimeFormatare explicitly passed.Apply this diff to make the prop forwarding more explicit:
<DateOverrideForm excludedDates={excludedDates} weekStart={weekStart} - {...passThroughProps} + workingHours={passThroughProps.workingHours} + onChange={passThroughProps.onChange} + value={passThroughProps.value} + isDryRun={passThroughProps.isDryRun} onClose={() => setOpen(false)} userTimeFormat={userTimeFormat} />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/schedules/components/DateOverrideInputDialog.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/schedules/components/DateOverrideInputDialog.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/schedules/components/DateOverrideInputDialog.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/schedules/components/DateOverrideInputDialog.tsx
🔇 Additional comments (2)
packages/features/schedules/components/DateOverrideInputDialog.tsx (2)
126-126: Good refactor: onChange calls are now explicit within each branch.Moving
onChange(datesInRanges)calls inside their respective branches (datesUnavailable true/false) makes the control flow clearer compared to having a single call after both branches.Also applies to: 140-140
29-29: LGTM: isDryRun prop additions are well-structured.The
isDryRunprop is correctly added with:
- A sensible default value of
false(line 29) for opt-in behavior- Proper TypeScript typing as optional (lines 38, 226)
- Consistent propagation through the component hierarchy
Also applies to: 38-38, 226-226
E2E results are ready! |
* chore: add `isDryRun` prop for date overrides * chore: add changesets * chore: implement PR feedback * fix: merge conflicts
3 participants
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
This can be tested in the platform examples app
/availabilitypage