fix: Init deny calendar ui v2 #456
Conversation
WalkthroughThis pull request introduces several enhancements and restructuring across multiple packages. New dependencies are added to support a calendar component and its TypeScript types, along with an in-workspace rule engine dependency. In the API package, a dedicated deny window router has been implemented—featuring endpoints for list, create (and createInCalendar), resize, drag, update, and delete—while older endpoints have been removed. Additionally, a new method has been added to the rule engine to retrieve denied time windows. UI components have been updated with extra styling options, and new deny window components with context support have been introduced in the webservice. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as "DenyWindow UI"
participant API as "denyWindowRouter"
participant DB as "Database"
U->>UI: Initiate deny window creation
UI->>API: Call create/modify deny window endpoint
API->>DB: Execute database operation
DB-->>API: Return operation result
API-->>UI: Send updated deny window data
UI->>U: Render updated calendar state
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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
🧹 Nitpick comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (4)
57-58: Remove console.log statements before production.There's a console.log statement that should be removed before deploying to production.
- onClick={(e) => { - e.stopPropagation(); - console.log("clicked!", event); - }} + onClick={(e) => { + e.stopPropagation(); + }}
43-72: Add proper typing for the event prop in EventComponent.Using
anytype for the event prop reduces type safety. Consider creating a proper type definition.-const EventComponent: React.FC<{ - event: any; - creatingDenyWindow: boolean; -}> = ({ event, creatingDenyWindow }) => { +const EventComponent: React.FC<{ + event: Event & { + title: string; + start: Date; + end: Date; + }; + creatingDenyWindow: boolean; +}> = ({ event, creatingDenyWindow }) => {
130-138: Extract the UUID extraction logic to a reusable function.The UUID regex and extraction logic is duplicated in both
handleEventResizeandhandleEventDrag. Extract it to a reusable function.+ const extractDenyWindowId = (eventId: string) => { + const uuidRegex = + /^([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})-\d+$/i; + const match = uuidRegex.exec(eventId); + const denyWindowId = match ? match[1] : null; + return { + denyWindowId, + denyWindow: denyWindowId + ? denyWindows.find(window => window.id === denyWindowId) + : null + }; + }; const handleEventResize = (event: EventChange) => { const { start, end } = event; const e = event.event as { end: Date; start: Date; id: string; }; - const uuidRegex = - /^([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})-\d+$/i; - const match = uuidRegex.exec(e.id); - const denyWindowId = match ? match[1] : null; - const denyWindow = denyWindows.find( - (denyWindow) => denyWindow.id === denyWindowId, - ); + const { denyWindowId, denyWindow } = extractDenyWindowId(e.id);
196-240: Consider adding loading and error states in the UI.The component doesn't handle loading or error states from the various API calls. Add visual feedback to improve user experience.
return ( <div onClick={() => setCreatingDenyWindow(false)}> + {denyWindowsQ.isLoading && ( + <div className="flex justify-center items-center h-10 mb-2"> + <div className="text-sm">Loading calendar data...</div> + </div> + )} + + {denyWindowsQ.isError && ( + <div className="flex justify-center items-center h-10 mb-2 text-destructive"> + <div className="text-sm">Error loading calendar data. Please try again.</div> + </div> + )} + <DnDCalendarpackages/rule-engine/src/rules/deployment-deny-rule.ts (1)
226-236: Consider more robust timezone offset extraction.The current implementation for extracting timezone offset relies on string parsing of formatted dates, which could be fragile. Consider using a more robust approach.
- const formatter = new Intl.DateTimeFormat("en-US", { - timeZone: this.timezone, - timeZoneName: "longOffset", - }); - - const offsetStr = formatter.format(windowStart).split("GMT")[1]; - const offsetHours = parseInt(offsetStr?.split(":")[0] ?? "0", 10); - - const realStartUTC = subHours(windowStart, offsetHours); - const realEndUTC = subHours(windowEnd, offsetHours); + // Get the timezone offset in minutes at the specific date + const localDate = new Date(windowStart); + const utcDate = new Date(Date.UTC( + localDate.getUTCFullYear(), + localDate.getUTCMonth(), + localDate.getUTCDate(), + localDate.getUTCHours(), + localDate.getUTCMinutes(), + localDate.getUTCSeconds() + )); + + // Calculate offset between UTC and the timezone + const tzDate = this.castTimezone(utcDate, this.timezone); + const offsetMinutes = (tzDate.getTime() - utcDate.getTime()) / (1000 * 60); + const offsetHours = offsetMinutes / 60; + + const realStartUTC = subHours(windowStart, offsetHours); + const realEndUTC = subHours(windowEnd, offsetHours);packages/api/src/router/policy/deny-window.ts (3)
5-26: Consider using enums or constants for weekdays.Your
weekdayMapobject works fine, but defining a TypeScript enum might provide more clarity and reduce possible mistakes. This is optional.
28-74: Potential pagination needed.While the logic for retrieving and transforming deny windows is correct, returning all results at once may pose scalability issues for large datasets. Consider adding pagination or filters if this route could return many records.
92-112: Incomplete mutation handler.
createInCalendarlogs the input but doesn't persist data to the database. If this is intentional, consider leaving a TODO comment or an explanatory note.Do you want me to propose a complete implementation for this procedure?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/webservice/package.json(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.css(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/page.tsx(4 hunks)packages/api/package.json(1 hunks)packages/api/src/root.ts(1 hunks)packages/api/src/router/policy/deny-window.ts(1 hunks)packages/api/src/router/policy/router.ts(2 hunks)packages/db/src/schema/policy.ts(1 hunks)packages/rule-engine/src/rules/deployment-deny-rule.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/schema/policy.tspackages/api/src/root.tspackages/api/src/router/policy/router.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsxpackages/rule-engine/src/rules/deployment-deny-rule.tspackages/api/src/router/policy/deny-window.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/page.tsx
🧬 Code Definitions (2)
packages/api/src/router/policy/router.ts (1)
packages/api/src/router/policy/deny-window.ts (1)
denyWindowRouter(28-258)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/page.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (1)
CreateDenyRuleDialog(85-241)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Test
🔇 Additional comments (24)
packages/api/package.json (1)
32-32: New dependency looks good.Adding the rule-engine workspace package will enable deny window functionality required for the calendar UI implementation. This is consistent with the PR's objective of building the deny calendar UI.
packages/api/src/root.ts (1)
6-6: More specific import path looks good.This change makes the import path more precise by referencing the router file directly. This clarifies the module structure and follows good import practices.
apps/webservice/package.json (2)
78-78: New calendar dependency looks good.The addition of
react-big-calendaris appropriate for implementing the calendar UI for deny windows as described in the PR objectives.
107-107: Type definitions for the calendar library look good.Adding the corresponding TypeScript type definitions as a dev dependency follows best practices and ensures proper type checking for the new calendar component.
packages/db/src/schema/policy.ts (1)
209-218: Exporting the schema is appropriate.Making the deny window schema available for import from other modules is necessary for the deny window functionality being implemented. This follows good modular design practices by exposing only what's needed by other components.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.css (3)
1-880: The CSS file appears to be using react-big-calendar's default styles with adaptations for the design system.The file includes comprehensive styling for the calendar components, properly using CSS variables with the format
hsl(var(--variable-name))for theming consistency.
812-834: Well-implemented cross-browser scrollbar compatibility.The custom scrollbar implementation includes styling for both WebKit browsers (Chrome, Safari) and Firefox, ensuring a consistent user experience across different browsers.
882-947: Smooth drag and drop animations for better UX.The transition effect on events during drag operations (line 895) and the hover effects for resize anchors improve user interaction feedback.
packages/api/src/router/policy/router.ts (2)
15-16: Good modularization of the deny window functionality.Importing the
denyWindowRouterfrom a separate file improves code organization and separation of concerns.
152-152: Refactored deny window endpoints into a dedicated router.The integration of
denyWindowRouterreplaces the previous direct endpoint implementations (createDenyWindow,updateDenyWindow,deleteDenyWindow), making the code more maintainable and following the single responsibility principle.apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (1)
182-193:❓ Verification inconclusive
Implement or remove commented code in handleEventCreate.
The
handleEventCreatefunction contains commented-out code that appears to be incomplete functionality. Either implement it properly or remove the commented code.- const handleEventCreate = (event: EventCreate) => { - console.log("creating deny window", event); - const { start, end } = event; - // console.log("creating deny window", start, end); - // createDenyWindow.mutate({ - // policyId: "123", - // start, - // end, - // timeZone, - // }); - // setEvents((prev) => [...prev, { id: "temp", start, end, title: "" }]); - }; + const handleEventCreate = (event: EventCreate) => { + const { start, end } = event; + // This implementation needs a policy ID + // You might want to: + // 1. Get it from props or context + // 2. Show a dialog to select a policy + // 3. Create a default policy if none exists + setCreatingDenyWindow(true); + + // Show a temporary event until the backend confirms creation + const tempId = `temp-${Date.now()}`; + setEvents((prev) => [...prev, { + id: tempId, + start, + end, + title: "New Deny Window" + }]); + + // TODO: Get the actual policy ID before uncommenting + // createDenyWindow.mutate({ + // policyId: "[POLICY_ID]", + // start, + // end, + // timeZone, + // }, { + // onSuccess: () => { + // denyWindowsQ.refetch(); + // setCreatingDenyWindow(false); + // }, + // onError: (error) => { + // // Remove the temporary event on error + // setEvents((prev) => prev.filter(e => e.id !== tempId)); + // setCreatingDenyWindow(false); + // // Handle error (e.g., show an error toast) + // } + // }); + };
🏁 Script executed:
#!/bin/bash # Check if there's any context about how policy IDs are obtained in other components echo "Searching for uses of policy IDs in deny window context:" rg "policyId.*denyWindow" --type=ts echo "Searching for any policy selection components:" rg "PolicySelect|SelectPolicy" --type=tsxLength of output: 335
Below is an additional shell script that uses a broader search (avoiding the unrecognized tsx file type issue) to help verify whether there’s any established mechanism for obtaining a policy ID (or a policy selection component) elsewhere in the deny-windows context:
Please run the above script and share its output.
Based on its results we can confirm if there’s an existing approach for resolving the policy ID. If no references are found, you may consider either:
- Removing the commented code until a clear solution for obtaining a policy ID is implemented, or
- Implementing the new approach (e.g. fetching the policy ID from props, context, or prompting the user) as suggested in the diff.
Once you verify this context manually, please update or finalize the implementation accordingly.
🏁 Script executed:
#!/bin/bash # Searching for any occurrence of "policyId" in files under the deny-windows folder echo "Searching for 'policyId' usage in deny-windows components:" rg "policyId" apps/webservice/src/app/\[workspaceSlug\]/\(app\)/policies/deny-windows/ # Searching for potential policy selection components by looking for "PolicySelect" or "SelectPolicy" in TSX or TS files echo "Searching for PolicySelect or SelectPolicy keywords in TS/TSX files:" find . -type f \( -name "*.tsx" -or -name "*.ts" \) -exec grep -EH "PolicySelect|SelectPolicy" {} \;Length of output: 719
Action: Address Incomplete Policy ID Handling in Deny Window Creation
The current implementation of
handleEventCreatein
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
still contains commented-out code for invoking the mutation with a hardcoded policy ID ("123"). Our search did not reveal an existing mechanism or component (such as a "PolicySelect" or "SelectPolicy") for obtaining a valid policy ID. As a result, the placeholder code remains ambiguous and could be confusing to future maintainers.Please choose one of the following approaches:
Implement a Complete Solution:
• Retrieve the policy ID from a reliable source (e.g., via props, React context, or by presenting a policy selection UI) before calling the mutation.
• Uncomment and adapt the mutation call accordingly, ensuring that the actual policy ID is used.Remove the Placeholder Code:
• If the proper implementation is not ready, remove the commented-out code to clean up the file and avoid any misconceptions.
• Add a clear TODO or FIXME comment to document that policy selection/ID retrieval needs to be addressed later.The diff snippet suggestion remains useful as it outlines a temporary approach (e.g., using a temporary ID for UI feedback) with a placeholder for the policy ID. However, without an established pattern in the codebase, it's important to either finalize this mechanism or eliminate the placeholder.
Please update the code accordingly and ensure that the component behavior is clear and maintainable.
packages/rule-engine/src/rules/deployment-deny-rule.ts (2)
9-9: Import style change from default to namespace import.The import statement for rrule has been changed from what appears to have been a default import to a namespace import, which is a good practice for modules with multiple exports.
162-241: Well-implemented date range functionality with detailed timezone handling.The new
getWindowsInRangemethod provides a robust way to calculate deny windows within a date range. The code includes thorough comments explaining the complex timezone handling logic.apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/page.tsx (5)
1-1: Looks good.Importing
notFoundfromnext/navigationis appropriate for returning a 404 when the resource is unavailable.
24-26: No concerns.Import statements from
~trpc/serverand your localCreateDenyRulecomponent look properly scoped and consistent with the rest of the file.
33-35: Ensure param resolution is maintained.Using
await paramsto destructure{ workspaceSlug }and validatingworkspacewithnotFound()is logically correct. Confirm that calling code always provides a valid promise forparams.
63-63: Minor UI improvement.Adding the
mb-6class to the<Card>for spacing is a clear enhancement of the page layout.
85-85: Good integration of the deny window dialog.Passing
workspace.idensures the dialog has the context needed to manage deny windows. The null check above prevents runtime issues.packages/api/src/router/policy/deny-window.ts (6)
1-4: Imports are well-chosen.Using types and utilities from
rrule,date-fns, andzodis appropriate for date manipulation and input validation.
76-90: Creation logic is straightforward.The insert operation with proper permission checks looks good. No major concerns here.
113-159: Resize functionality seems correct.Your approach to calculating new start/end times and updating
dtstartanddtendin the database is clear. Make sure to test boundary cases (e.g., negative offsets, extremely large offsets) to ensure stability.
161-209: Drag logic is well-structured.Mapping the new weekday and adjusting offsets looks correct. Suggest verifying that multi-day drags behave as expected within the recurrence rules.
210-235: Update procedure is straightforward.The usage of
updatePolicyRuleDenyWindowprovides a clean approach for partial updates. The permission check is also consistent with the rest of the file.
236-258: Deletion logic appears safe.Ensures the user has
PolicyDeletepermission, then removes the record from the database. Everything is consistent and aligned with best practices.
...ebservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (4)
44-44: Use a more specific type thananyfor event.Currently,
eventis declared asany, which can mask potential type mismatches or usage issues. Consider introducing an explicit interface or a provided type (e.g., from the calendar library) to ensure clarity and type safety.
45-47: Remove or utilize the unusedcreatingDenyWindowprop.The
creatingDenyWindowprop is declared but never referenced in the component’s logic, leading to dead code. Either safely remove it, or consider leveraging it to manage UI state or behavior relevant to creating deny windows.
105-110: Consider adding explicit error or loading states.While fetching deny windows, the UI does not appear to handle loading or error conditions. Providing feedback for these states would help users understand the app’s status and avoid confusion.
193-198: Clean up commented-out code.The creation logic for deny windows is partially commented out. This can lead to confusion or unmaintained code. Remove or reintroduce it with a finalized approach for creating deny windows in the calendar.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (1)
74-76: Replace placeholder content in PopoverContent with meaningful details.The placeholder "TEST TEXT" is still present. Consider rendering actual event details or helpful information regarding the deny window to enhance the user experience.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (7)
44-44: Replaceanytype with a more specific type.Using
anytype reduces TypeScript's ability to catch errors at compile time. Consider replacing it with a more specific type that matches the event structure from react-big-calendar.- event: any; + event: Event;
59-59: Remove console.log statement before production.Console logs should be removed before merging to production. They can expose sensitive information and create noise in the browser console.
- console.log("clicked!", event);
77-77: Replace genericobjecttype with a more specific type.Using
objecttype is too generic and doesn't provide proper type checking. Consider using a more specific type that matches the event structure.- event: object; + event: { + end: Date; + start: Date; + id: string; + title: string; + };
111-113: Initialize state with an empty array instead of mapping over potentially undefined data.The current approach might cause issues if
denyWindowsQ.datais undefined on the initial render. It's better to initialize with an empty array and update state when data becomes available.- const [events, setEvents] = useState<Event[]>( - denyWindows.flatMap((denyWindow) => denyWindow.events), - ); + const [events, setEvents] = useState<Event[]>([]);
132-135: Extract UUID regex to a constant or utility function.The UUID regex is defined inline within the event handlers. Since it's used in multiple places, consider extracting it to a constant outside the component or to a utility function for better reusability.
+ const DENY_WINDOW_ID_REGEX = /^([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})-\d+$/i; + + const extractDenyWindowId = (eventId: string): string | null => { + const match = DENY_WINDOW_ID_REGEX.exec(eventId); + return match ? match[1] : null; + }; // Then in the handlers: - const uuidRegex = - /^([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})-\d+$/i; - const match = uuidRegex.exec(e.id); - const denyWindowId = match ? match[1] : null; + const denyWindowId = extractDenyWindowId(e.id);
162-165: Duplicate UUID regex pattern - extract to a shared function.The same UUID regex pattern is duplicated in handleEventResize and handleEventDrag. Extract it to a shared function to eliminate redundancy.
// (Same suggestion as above, but for the second occurrence) - const uuidRegex = - /^([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})-\d+$/i; - const match = uuidRegex.exec(e.id); - const denyWindowId = match ? match[1] : null; + const denyWindowId = extractDenyWindowId(e.id);
223-224: Consider making drag and resize configurable.Currently, all events are draggable and resizable. Consider making this configurable based on event properties or user permissions.
- resizableAccessor={() => true} - draggableAccessor={() => true} + resizableAccessor={(event) => event.id !== "new"} + draggableAccessor={(event) => event.id !== "new"}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: Test
🔇 Additional comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (4)
69-71: Replace placeholder content in PopoverContent.The "TEST TEXT" placeholder should be replaced with meaningful content about the event.
- <PopoverContent side="right" align="center" className="p-2"> - <div>TEST TEXT</div> - </PopoverContent> + <PopoverContent side="right" align="center" className="p-2"> + <div className="space-y-2"> + <div className="font-medium">{event.title}</div> + <div className="text-xs"> + <span>Time: </span> + <span>{start} - {end}</span> + </div> + </div> + </PopoverContent>
90-92: LGTM: Good use of browser's timezone.Using the browser's timezone through
Intl.DateTimeFormat().resolvedOptions()is a good approach to ensure the calendar displays in the user's local timezone.
95-99: LGTM: Well-structured date range initialization.The current range state is well-structured with proper default values using the current week. Good job!
200-219: LGTM: Thorough range change handling.The range change handler properly handles both array and object range formats, ensuring the calendar responds correctly to user navigation.
...ebservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
Outdated
Show resolved
Hide resolved
| console.log("creating deny window", event); | ||
| const { start, end } = event; | ||
| // // console.log("creating deny window", start, end); | ||
| // // createDenyWindow.mutate({ | ||
| // // policyId: "123", | ||
| // // start, | ||
| // end, | ||
| // timeZone, | ||
| // }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up commented code and implement the actual window creation.
There's commented code and a console.log that should be removed or implemented properly. The current implementation doesn't actually create a deny window in the backend.
- console.log("creating deny window", event);
- const { start, end } = event;
- // // console.log("creating deny window", start, end);
- // // createDenyWindow.mutate({
- // // policyId: "123",
- // // start,
- // end,
- // timeZone,
- // });
+ const { start, end } = event;
+ // Set local state first for immediate UI feedback
+ setCreatingDenyWindow(true);
+ setEvents((prev) => [...prev, { id: "new", start, end, title: "New Deny Window" }]);
+
+ // Uncomment and implement when ready to connect to backend
+ /*
+ createDenyWindow.mutate({
+ workspaceId,
+ start,
+ end,
+ timeZone,
+ });
+ */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("creating deny window", event); | |
| const { start, end } = event; | |
| // // console.log("creating deny window", start, end); | |
| // // createDenyWindow.mutate({ | |
| // // policyId: "123", | |
| // // start, | |
| // end, | |
| // timeZone, | |
| // }); | |
| const { start, end } = event; | |
| // Set local state first for immediate UI feedback | |
| setCreatingDenyWindow(true); | |
| setEvents((prev) => [...prev, { id: "new", start, end, title: "New Deny Window" }]); | |
| // Uncomment and implement when ready to connect to backend | |
| /* | |
| createDenyWindow.mutate({ | |
| workspaceId, | |
| start, | |
| end, | |
| timeZone, | |
| }); | |
| */ |
| useEffect( | ||
| () => setEvents(denyWindows.flatMap((denyWindow) => denyWindow.events)), | ||
| [denyWindows], | ||
| ); |
There was a problem hiding this comment.
If we can remove this use effect we should
| id: string; | ||
| }; | ||
|
|
||
| const uuidRegex = |
| id: string; | ||
| }; | ||
|
|
||
| const uuidRegex = |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/DenyWindowContext.tsx (1)
8-10: Optional: Provide default context value.Currently, the context is initialized as
undefined, which is handled by the custom hook. For additional clarity, consider providing a default context value (e.g.,nullforopenEventIdand a no-op updater) during development or testing.apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (3)
124-171: Optional: Provide a fallback or visual cue ifdenyWindowis null.Currently, the
EventComponentalways renders a popover. Consider gracefully handling the case where a correspondingdenyWindowis unavailable or not loaded. This is handled indirectly by returning null ifdenyWindow == nulldownstream, but you might add a quick check earlier or render a fallback UI.
173-297: Handle server-side operation failures gracefully.In the drag, resize, and create handlers, the code updates state optimistically but does not handle potential failures. Consider adding error handling to revert changes or inform users if a backend operation fails.
+ resizeDenyWindow.mutate( + { windowId: id, dtstartOffset, dtendOffset }, + { + onError: (err) => { + console.error("Resize failed", err); + // Possibly revert UI changes or show notification + }, + }, + );
285-295: Remove or clarify commented code for deny window creation.The creation logic is commented out. If you plan to integrate it soon, consider adding a TODO. Otherwise, remove the commented code to avoid confusion.
packages/api/src/router/policy/deny-window.ts (5)
28-74: Watch out for large result sets.The
listprocedure usesflatMapto generate potentially large event arrays. For extremely large date ranges, consider adding pagination or a limit to prevent excessive data fetching.
52-73: Consider partial or lazy evaluation ofgetWindowsInRange.If the generated intervals can be large, you might want to fetch or compute them in chunks. This will help avoid performance bottlenecks if users query significantly large ranges.
176-224: Allow multi-weekday recurrences.At line 213, you overwrite
byweekdaywith[input.day]. If the domain requires multi-day recurrences, consider merging or appending to an existing array instead.- byweekday: [input.day as rrule.ByWeekday], + const updatedByWeekday = Array.isArray(denyWindow.rrule.byweekday) + ? [...denyWindow.rrule.byweekday, input.day as rrule.ByWeekday] + : [input.day as rrule.ByWeekday]; + byweekday: updatedByWeekday,
225-249: Checks for nonexistent records.The
updateprocedure relies ontakeFirst. If no record is found, the code does not handle that scenario explicitly. Consider throwing an error or returning a friendly message whendenyWindowis not in the DB.
251-273: Ensure a consistent user experience after deletion.The
deleteprocedure similarly does not indicate if no record is found. If needed, handle the case where the record is already deleted or never existed to keep the UI or calling code consistent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/DenyWindowContext.tsx(1 hunks)packages/api/src/router/policy/deny-window.ts(1 hunks)packages/db/src/schema/policy.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/schema/policy.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsxpackages/api/src/router/policy/deny-window.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/DenyWindowContext.tsx
🧬 Code Definitions (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (2)
packages/db/src/schema/policy.ts (2)
PolicyRuleDenyWindow(254-256)Policy(252-252)apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/DenyWindowContext.tsx (2)
useDenyWindow(24-29)DenyWindowProvider(12-22)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/DenyWindowContext.tsx (3)
1-10: Great use of typed context.Defining
DenyWindowContextTypeand passing it tocreateContextensures a strong contract, preventing accidental misuse of the context properties.
12-22: Provider usage looks good.The
DenyWindowProvidercorrectly managesopenEventIdandsetOpenEventId, and exposes them to the subtree. This pattern is straightforward and encourages a clean separation of concerns.
24-29: Clear custom hook.
useDenyWindowprovides an excellent developer experience and enforces usage within the provider with a descriptive error. Good job ensuring the context is never accessed unexpectedly.apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (2)
86-122: Good separation of read vs. edit states.
DenyWindowInfoandEditDenyWindoware logically separated, improving readability and maintainability by isolating presentation from editing concerns.
298-352: Calendar usage is well integrated.The overall approach to managing the calendar with
react-big-calendarandwithDragAndDropis clear. The arrangement ofonRangeChange,onSelectSlot, andonEventDrophelps keep logic centralized, improving code clarity.packages/api/src/router/policy/deny-window.ts (2)
76-105: Good usage of typed schema.This
createprocedure properly referencescreatePolicyRuleDenyWindowfor validation, helping ensure data correctness at the boundary layer.
128-174: Consider negative offset validation.In the
resizeprocedure, ensure negative offsets (which effectively shrink windows or move them before dtstart) are acceptable or handle them gracefully. Maintaining a minimum length or limiting the shift might be necessary in some use cases.Would you like to confirm we have a max/min offset requirement in your domain rules? If so, we can add a zod refinement or a check before updating the window.
...ebservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
Outdated
Show resolved
Hide resolved
| createInCalendar: protectedProcedure | ||
| .meta({ | ||
| authorizationCheck: ({ canUser, input }) => | ||
| canUser | ||
| .perform(Permission.PolicyCreate) | ||
| .on({ type: "policy", id: input.policyId }), | ||
| }) | ||
| .input( | ||
| z.object({ | ||
| policyId: z.string().uuid(), | ||
| start: z.date(), | ||
| end: z.date(), | ||
| timeZone: z.string(), | ||
| }), | ||
| ) | ||
| .mutation(({ ctx, input }) => { | ||
| console.log(input); | ||
|
|
||
| return; | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complete the calendar creation logic.
The createInCalendar mutation currently only logs input. Finalize its implementation or remove it if it remains unused.
- console.log(input);
- return;
+ // Example:
+ return ctx.db.insert(policyRuleDenyWindow)
+ .values({
+ policyId: input.policyId,
+ name: "Calendar-based window",
+ // Additional fields...
+ })
+ .returning()
+ .then(takeFirst);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| createInCalendar: protectedProcedure | |
| .meta({ | |
| authorizationCheck: ({ canUser, input }) => | |
| canUser | |
| .perform(Permission.PolicyCreate) | |
| .on({ type: "policy", id: input.policyId }), | |
| }) | |
| .input( | |
| z.object({ | |
| policyId: z.string().uuid(), | |
| start: z.date(), | |
| end: z.date(), | |
| timeZone: z.string(), | |
| }), | |
| ) | |
| .mutation(({ ctx, input }) => { | |
| console.log(input); | |
| return; | |
| }), | |
| createInCalendar: protectedProcedure | |
| .meta({ | |
| authorizationCheck: ({ canUser, input }) => | |
| canUser | |
| .perform(Permission.PolicyCreate) | |
| .on({ type: "policy", id: input.policyId }), | |
| }) | |
| .input( | |
| z.object({ | |
| policyId: z.string().uuid(), | |
| start: z.date(), | |
| end: z.date(), | |
| timeZone: z.string(), | |
| }), | |
| ) | |
| .mutation(({ ctx, input }) => { | |
| // Example: | |
| return ctx.db.insert(policyRuleDenyWindow) | |
| .values({ | |
| policyId: input.policyId, | |
| name: "Calendar-based window", | |
| // Additional fields... | |
| }) | |
| .returning() | |
| .then(takeFirst); | |
| }), |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/api/src/router/policy/deny-window.ts (1)
125-128: 🛠️ Refactor suggestionImplement or remove the placeholder
createInCalendarmutation.The
createInCalendarmutation logs the input but doesn't persist data. If you intend to allow calendar-based creation, finalize it with a proper insert statement; otherwise, remove the placeholder to avoid confusion.console.log(input); return; +// Example implementation, adjust as needed: +return ctx.db.insert(policyRuleDenyWindow) + .values({ + policyId: input.policyId, + // Additional fields referencing input.start, input.end, etc. + }) + .returning() + .then(takeFirst);
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (3)
63-69: Remove or finalize the commented-out update logic.The update code is currently commented out, which could be confusing for future maintainers. If you intend to implement this logic soon, consider adding a TODO note or finalize the mutation call. Otherwise, remove it to keep the code clean.
console.log("data", data); - // updateDenyWindow.mutate({ - // id: denyWindow.id, - // data, - // }); - // setEditing(); + updateDenyWindow.mutate({ + id: denyWindow.id, + data, + }); + setEditing();
230-255: Add error handling when resizing a deny window.When the resizeDenyWindow mutation fails, there is no feedback for the user, and the UI might become out of sync. Consider adding try/catch or using the mutation’s onError callback to revert local state.
- resizeDenyWindow.mutate({ windowId: id, dtstartOffset, dtendOffset }); + resizeDenyWindow.mutate( + { windowId: id, dtstartOffset, dtendOffset }, + { + onError: (error) => { + console.error("Failed to resize event:", error); + // Optionally revert local state changes or show a notification. + }, + } + );
326-332: Review slot selection logic to avoid unintentional creation.Currently, clicking a slot when an event popover is open discards the “new” event, but clicking when no popover is open creates a new event immediately. This might surprise users or lead to accidental creations. Consider prompting for confirmation or at least providing a form for basic input before finalizing creation.
packages/api/src/router/policy/deny-window.ts (1)
178-219: Confirm correct weekday mapping on drag.The code sets the
byweekdaybased on the day index. Ensure that the user’s time zone or start-of-week preferences do not lead to unexpected results. If day boundaries cross midnight, partial offsets might shift the day incorrectly from the user’s perspective.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx(1 hunks)packages/api/src/router/policy/deny-window.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/policy/deny-window.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
packages/api/src/router/policy/deny-window.ts (1)
130-155:❓ Verification inconclusive
Consider validating window boundaries when resizing.
Resizing a deny window might cause it to overlap with other blocks or go out of permissible ranges. If relevant, add validations or user feedback to ensure a consistent UX.
Are there rules in the codebase that prevent overlapping windows or limit resizing? If so, verify them with a script:
🏁 Script executed:
#!/bin/bash # Search for existing constraints or checks around policyRuleDenyWindow. rg -A 6 'constraint.*policyRuleDenyWindow'Length of output: 44
ACTION: Verify and Add Boundary Validations for Deny Window Resizing
The search for constraints enforcing non-overlapping or boundary limits on deny windows did not yield any evidence of such validations. This might imply that resizing currently lacks these checks, which could lead to overlaps or out-of-range configurations. Please verify manually whether:
- There are additional validations elsewhere in the codebase (or in a shared utility) that guard against overlapping windows or enforce allowed ranges.
- Resizing operations should incorporate boundary validations and user feedback to maintain a consistent UX.
If no such validations exist, consider adding explicit checks in the logic around
packages/api/src/router/policy/deny-window.ts(lines 130–155).
...ebservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
Outdated
Show resolved
Hide resolved
...ebservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (4)
229-234: Avoid usinganytype for the event propA past comment addressed this. Using the
anytype dilutes type safety. Consider defining or inferring an exact event type for improved readability and maintainability.
328-331: Consider removing thisuseEffectA past comment suggested removing this if not strictly required. Evaluate whether you can derive the same outcome without an additional
useEffectto simplify the component’s lifecycle.
333-389: Add error handling for resize and drag mutationsA past review already flagged the lack of error handling. If these mutations fail (e.g., network error), you should handle the error and revert the UI or display a notification. This prevents silent failures.
Also applies to: 441-441
🧰 Tools
🪛 GitHub Check: Lint
[failure] 335-335:
'createDenyWindow' is assigned a value but never used. Allowed unused vars must match /^_/u
391-401: Remove or finalize commented-outcreateDenyWindowcallThis block was flagged in a previous review. If you plan to use
createDenyWindow, implement and uncomment the mutation. Otherwise, remove this commented code to keep the codebase clean.
🧹 Nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (2)
65-66: UnusedupdateDenyWindowMutationThis mutation is declared but never used. If it's intended for future use, consider prefixing it with an underscore (e.g.,
_updateDenyWindow) or removing it to comply with lint rules.Also applies to: 88-88
335-335: UnusedcreateDenyWindowMutationThis mutation is declared but never used. If it is reserved for future changes, consider prefixing it with an underscore or remove it to adhere to lint rules.
🧰 Tools
🪛 GitHub Check: Lint
[failure] 335-335:
'createDenyWindow' is assigned a value but never used. Allowed unused vars must match /^_/u
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx(1 hunks)packages/ui/src/datetime-picker.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/ui/src/datetime-picker.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
🧬 Code Graph Analysis (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx (7)
packages/ui/src/calendar.tsx (1)
Calendar(73-73)packages/db/src/schema/rules/deny-window.ts (1)
PolicyRuleDenyWindow(142-144)packages/db/src/schema/policy.ts (1)
Policy(104-104)packages/ui/src/button.tsx (1)
Button(80-80)packages/ui/src/datetime-picker.tsx (1)
TimePicker(870-870)apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/DenyWindowContext.tsx (2)
useDenyWindow(24-29)DenyWindowProvider(12-22)packages/ui/src/popover.tsx (3)
Popover(35-35)PopoverTrigger(35-35)PopoverContent(35-35)
🪛 GitHub Check: Lint
apps/webservice/src/app/[workspaceSlug]/(app)/policies/deny-windows/_components/CreateDenyRule.tsx
[failure] 335-335:
'createDenyWindow' is assigned a value but never used. Allowed unused vars must match /^_/u
[failure] 88-88:
'updateDenyWindow' is assigned a value but never used. Allowed unused vars must match /^_/u
🔇 Additional comments (3)
packages/ui/src/datetime-picker.tsx (3)
379-395: Great addition for flexible stylingThe new
classNameprop inPeriodSelectorPropsprovides more granular control over styling. The overall approach of passing combined classes viacnis consistent with the rest of the codebase.
579-582: New style-related props enhance customizationThe additional optional props (
showIcon,className,inputClassName,timePeriodPickerClassName) expand customization capabilities for theTimePicker, making it more versatile.
592-602: Conditional icon rendering and class handling looks goodThe logic to conditionally render the clock icon and apply custom classes (
className,inputClassName,timePeriodPickerClassName) is well-structured and maintains clarity.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/rule-engine/src/rules/deployment-deny-rule.ts (2)
174-246: Well-implemented date range functionality with comprehensive documentationThe
getWindowsInRangemethod is well-documented and handles the complex task of generating denied time windows within a date range. The timezone handling is particularly thorough.However, the comment block on lines 224-230 about timezone conversion could be clearer. Consider revising it to more explicitly explain the conversion process.
- /** - * Window start and end are in the rrule's internal pretend UTC timezone - * Since we know the rule's timezone, we first figure out the offset of this timezone - * to the actual UTC time. Then we convert the window start and end to the actual UTC time - * by subtracting the offset. We end up not needing to know the requester's timezone at all - * because timezone in parts will - */ + /** + * Window start and end are in the rrule's internal timezone representation. + * To convert to actual UTC: + * 1. We determine the offset between the rule's timezone and UTC + * 2. We adjust the timestamps by subtracting this offset + * 3. This gives us the correct UTC time regardless of the requester's timezone + */
231-245: Time zone offset extraction could be more robustThe current implementation extracts timezone offset from a formatted string, which works but could be fragile if the format changes. Consider using a more direct method to calculate timezone offsets if available in your date library.
- const formatter = new Intl.DateTimeFormat("en-US", { - timeZone: this.timezone, - timeZoneName: "longOffset", - }); - - const offsetStr = formatter.format(windowStart).split("GMT")[1]; - const offsetHours = parseInt(offsetStr?.split(":")[0] ?? "0", 10); - - const realStartUTC = subHours(windowStart, offsetHours); - const realEndUTC = subHours(windowEnd, offsetHours); + // Get the timezone offset directly + const startInTz = this.castTimezone(windowStart, this.timezone); + const offsetMinutes = startInTz.getTimezoneOffset(); + const offsetHours = offsetMinutes / 60; + + const realStartUTC = subHours(windowStart, offsetHours); + const realEndUTC = subHours(windowEnd, offsetHours);Note: The above is just a suggestion. If your current approach works reliably with your date library setup, it may be preferable to maintain consistency with the existing codebase.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/CreateDenyRule.tsx (3)
229-277: Consider explicit timezone handling inEventComponent.While formatting times for events, you might face time zone discrepancies if multiple users differ in locale/time zones. To ensure consistency, pass the relevant time zone or unify display logic to avoid confusion about when an event starts or ends.
324-332: Be cautious about local state synchronization forevents.Since you maintain your own
eventsstate while also pulling data from the backend, there’s a risk of mismatches if the server updates or if multiple clients modify data. Consider a single source of truth or re-synchronizing state when new data loads.
333-389: Add error handling for resize and drag mutations.Right now, the UI is updated optimistically, but if
resizeDenyWindowordragDenyWindowfails, there's no rollback or user feedback. Consider reverting the UI changes or notifying the user of the failure to keep the UI consistent with server data.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/webservice/package.json(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/CreateDenyRule.css(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/CreateDenyRule.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/DenyWindowContext.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/page.tsx(4 hunks)packages/rule-engine/src/rules/deployment-deny-rule.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/package.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/DenyWindowContext.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/CreateDenyRule.tsxpackages/rule-engine/src/rules/deployment-deny-rule.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/page.tsx
🧬 Code Graph Analysis (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/CreateDenyRule.tsx (3)
packages/db/src/schema/rules/deny-window.ts (1)
PolicyRuleDenyWindow(142-144)packages/db/src/schema/event.ts (1)
event(17-24)apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/DenyWindowContext.tsx (2)
useDenyWindow(24-29)DenyWindowProvider(12-22)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (12)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/page.tsx (3)
33-35: Good implementation of 404 handlingThe code correctly fetches the workspace by slug and shows a 404 page when the workspace doesn't exist, which is a good pattern for handling invalid routes.
63-63: UI improvement with marginAdding margin to the Card component improves the layout spacing, creating better visual separation between elements.
85-85: Well-implemented component with proper prop passingThe CreateDenyRuleDialog is correctly integrated with the workspace ID passed as a prop, ensuring the deny rules are associated with the correct workspace.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/CreateDenyRule.css (2)
1-880: Well-structured CSS using theme variablesThe CSS is properly organized and utilizes CSS variables with the
hsl(var(--variable))pattern for theming, which is a good practice for maintaining consistency across the UI.
881-947: Well-implemented drag and resize functionalityThe CSS for the drag-and-drop and resize functionality is well-implemented, providing good visual feedback for users when interacting with calendar events.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/DenyWindowContext.tsx (3)
3-6: Well-defined context typeThe context type definition is clear and concise, providing good type safety for the context values.
12-22: Clean implementation of context providerThe DenyWindowProvider component follows React best practices by using useState for state management and providing the context values to children.
24-29: Good error handling in custom hookThe useDenyWindow hook includes proper error handling to ensure it's only used within a DenyWindowProvider, which helps prevent runtime errors.
packages/rule-engine/src/rules/deployment-deny-rule.ts (1)
9-9: Updated import style for rruleThe change from default import to namespace import is appropriate and ensures proper access to all rrule exports.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/deny-windows/_components/CreateDenyRule.tsx (3)
79-85: Double-check the frequency mapping logic.Is it intentional that
freq=1corresponds to "Monthly,"freq=2to "Weekly," and the fallback to "Daily"? Conventionally, one might expectfreq=1→ "Daily,"freq=2→ "Weekly,"freq=3→ "Monthly." Please verify that your current setup is correct for your needs.
431-439: Verify the slot selection flow.The slot handler calls
handleEventCreatewhen no event is open, but if one is open, it removes the "new" placeholder event and resetsopenEventId. Confirm whether this interaction aligns with users' expectations for creating multiple new deny windows or continuing to edit the current one.
445-458: Good usage of the custom event component.Your approach of mapping each calendar event to the matching
DenyWindowand rendering theEventComponentis well-structured. This keeps the UI consistent and modular. Nice job integrating drag-and-drop and resize logic in a clean, composable way!
| const onSubmit = form.handleSubmit((data) => { | ||
| console.log("data", data); | ||
| // updateDenyWindow.mutate({ | ||
| // id: denyWindow.id, | ||
| // data, | ||
| // }); | ||
| // setEditing(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing or re-implementing the commented-out update logic.
These lines for updating the deny window are commented out and might create confusion. If you still plan on implementing the update functionality, go ahead and finalize it. Otherwise, remove this code to keep the file tidy.
| const handleEventCreate = (event: EventCreate) => { | ||
| console.log("creating deny window", event); | ||
| const { start, end } = event; | ||
| // // console.log("creating deny window", start, end); | ||
| // // createDenyWindow.mutate({ | ||
| // // policyId: "123", | ||
| // // start, | ||
| // end, | ||
| // timeZone, | ||
| // }); | ||
| setEvents((prev) => [...prev, { id: "new", start, end, title: "" }]); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complete or remove the creation logic.
Currently, the create mutation code is commented out, and a placeholder event with id: "new" is added locally. This can lead to data mismatch if not eventually persisted. Decide whether to fully implement the create flow or remove the placeholder event logic.
Summary by CodeRabbit
New Features
Chores
Style