feat: tasks management with periodic schedule #1781
Conversation
WalkthroughThis update introduces a comprehensive task management system to the application. It adds new backend models ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant DB
User->>Frontend: Navigates to /calendar/{year}/{month}
Frontend->>API: GET applied-controls, risk-acceptances, user tasks (for month)
API->>DB: Fetch applied controls, risk acceptances, task nodes/templates
DB-->>API: Return data
API-->>Frontend: Return controls, acceptances, tasks
Frontend->>User: Renders calendar with all events
sequenceDiagram
participant Admin
participant Frontend
participant API
participant DB
Admin->>Frontend: Creates/updates TaskTemplate via form
Frontend->>API: POST/PUT TaskTemplate
API->>DB: Save TaskTemplate
API->>API: Sync TaskNodes for template (generate occurrences)
API->>DB: Create/update/delete TaskNode records as needed
DB-->>API: Confirmation
API-->>Frontend: Response with updated TaskTemplate/TaskNodes
Frontend->>Admin: Shows updated template and instances
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🪧 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
CodeRabbit Configuration File (
|
ab-smith
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
backend/core/views.py (4)
5173-5179: Update comment to match implementation.The comment mentions "next 10 upcoming tasks" but the code doesn't limit to 10 tasks - it processes all tasks.
Update the comment to reflect the actual implementation:
- # Process all past tasks and next 10 upcoming tasks + # Process all past tasks and all upcoming tasks
5302-5304: Fix parameter name typo in status method.There's a typo in the parameter name:
srlfinstead ofself.- def status(srlf, request): + def status(self, request):🧰 Tools
🪛 Ruff (0.8.2)
5304-5304:
TaskNodemay be undefined, or defined from star imports(F405)
5305-5310: Don't call save() redundantly in perform_create.The
perform_createmethod callssave()on the instance, but theserializer.save()already does this. This is redundant and could potentially cause issues.def perform_create(self, serializer): instance: TaskNode = serializer.save() - instance.save() return super().perform_create(serializer)🧰 Tools
🪛 Ruff (0.8.2)
5307-5307:
TaskNodemay be undefined, or defined from star imports(F405)
5264-5282: Implement pagination for the calendar endpoint to handle large volumes of tasks.The calendar endpoint retrieves all enabled task templates without pagination, which could lead to performance issues with a large number of templates.
Consider adding pagination support or limiting the number of tasks returned:
@action( detail=False, name="Get tasks for the calendar", url_path="calendar/(?P<start>.+)/(?P<end>.+)", ) def calendar( self, request, start=None, end=None, ): if start is None: start = timezone.now().date() if end is None: end = timezone.now().date() + relativedelta.relativedelta(months=1) + + # Get limit from query params or use default + limit = request.query_params.get('limit', None) + if limit: + try: + limit = int(limit) + except ValueError: + return Response({"error": "Invalid limit parameter"}, status=400) + + # Get all tasks + tasks = self.task_calendar( + task_templates=TaskTemplate.objects.filter(enabled=True), + start=start, + end=end, + ) + + # Apply limit if specified + if limit: + tasks = tasks[:limit] - return Response( - self.task_calendar( - task_templates=TaskTemplate.objects.filter(enabled=True), - start=start, - end=end, - ) - ) + return Response(tasks)🧰 Tools
🪛 Ruff (0.8.2)
5277-5277:
TaskTemplatemay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/views.py
2216-2216: TaskTemplate may be undefined, or defined from star imports
(F405)
5140-5140: TaskTemplate may be undefined, or defined from star imports
(F405)
5201-5201: TaskTemplate may be undefined, or defined from star imports
(F405)
5202-5202: TaskNode may be undefined, or defined from star imports
(F405)
5213-5213: TaskNodeReadSerializer may be undefined, or defined from star imports
(F405)
5217-5217: TaskTemplate may be undefined, or defined from star imports
(F405)
5219-5219: TaskNode may be undefined, or defined from star imports
(F405)
5245-5245: TaskTemplate may be undefined, or defined from star imports
(F405)
5251-5251: TaskNode may be undefined, or defined from star imports
(F405)
5258-5258: TaskNode may be undefined, or defined from star imports
(F405)
5277-5277: TaskTemplate may be undefined, or defined from star imports
(F405)
5299-5299: TaskNode may be undefined, or defined from star imports
(F405)
5304-5304: TaskNode may be undefined, or defined from star imports
(F405)
5307-5307: TaskNode may be undefined, or defined from star imports
(F405)
🪛 GitHub Actions: Backend Linters
backend/core/views.py
[error] 1-1: Ruff formatting check failed. File would be reformatted. Run 'ruff format' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/core/views.py (1)
1989-1995: Good enhancement for time-based filtering.Adding 'month' and 'year' filters to the expiry_date field in RiskAcceptanceFilterSet enhances the API's filtering capabilities for calendar-based views.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/core/views.py (2)
5149-5156: Ensure logic aligns with code comments regarding "next 10 tasks."The comment mentions "next 10 upcoming tasks," but the code doesn't actually limit to 10 tasks. This mismatch might cause confusion for maintainers.
5192-5235: 🛠️ Refactor suggestionConsider more targeted updates to prevent data loss.
The current implementation marks all task nodes for deletion upfront and then conditionally recreates them. If an error occurs during processing, all task nodes might be deleted without proper replacements.
Use a transaction to ensure atomicity and handle cleanup properly:
def _sync_task_nodes(self, task_template: TaskTemplate): + with transaction.atomic(): # Soft-delete all existing TaskNode instances associated with this TaskTemplate TaskNode.objects.filter(task_template=task_template).update(to_delete=True) # Determine the end date based on the frequency if task_template.is_recurrent: # ... existing code ... else: TaskNode.objects.create( due_date=task_template.task_date, status="pending", task_template=task_template, folder=task_template.folder, ) # garbage-collect TaskNode.objects.filter(to_delete=True).delete()🧰 Tools
🪛 Ruff (0.8.2)
5192-5192:
TaskTemplatemay be undefined, or defined from star imports(F405)
5195-5195:
TaskNodemay be undefined, or defined from star imports(F405)
5221-5221:
TaskTemplatemay be undefined, or defined from star imports(F405)
5227-5227:
TaskNodemay be undefined, or defined from star imports(F405)
5234-5234:
TaskNodemay be undefined, or defined from star imports(F405)
🧹 Nitpick comments (4)
backend/core/views.py (4)
11-11: Redundant import found.You have two imports for
relativedeltafromdateutilat lines 11 and 97 - one as the default name and one aliased asrd. This could cause confusion for developers.-from dateutil import relativedelta +from dateutil import relativedelta # ... other imports ... -from dateutil import relativedelta as rd +# Use the already imported relativedeltaAlso applies to: 97-97
5115-5115: Make imports explicit instead of using star imports.The code refers to models and serializers (
TaskTemplate,TaskNode,TaskNodeReadSerializer) imported from star imports (line 110:from .models import *and line 111:from .serializers import *). This makes it harder to understand dependencies and can lead to name collisions.Import these classes explicitly:
-from .models import * +from .models import ( + AppliedControl, + # ... other existing models ... + TaskTemplate, + TaskNode, +) -from .serializers import * +from .serializers import ( + AppliedControlReadSerializer, + # ... other existing serializers ... + TaskNodeReadSerializer, +)Also applies to: 5176-5176, 5177-5177, 5188-5188, 5195-5195, 5221-5221, 5227-5227, 5234-5234, 5253-5253, 5277-5277, 5282-5282, 5285-5285
🧰 Tools
🪛 Ruff (0.8.2)
5115-5115:
TaskTemplatemay be undefined, or defined from star imports(F405)
5157-5191: Avoid in-place modification of tasks_list.The code modifies
tasks_listin-place while iterating through it, which could lead to unexpected behavior if the list is used elsewhere or if the iteration pattern changes.Instead of modifying the original list, consider creating a new list with the processed tasks:
-# Directly modify tasks in the original tasks_list -for i in range(len(tasks_list)): - task = tasks_list[i] +processed_tasks = [] +for task in tasks_list: task_date = task["due_date"] task_template_id = task["task_template"] # Create a unique identifier for this task to avoid duplication task_identifier = (task_template_id, task_date) # Skip if we've already processed this task if task_identifier in processed_tasks_identifiers: - continue + processed_tasks.append(task) + continue # Check if this task should be processed (is in past or next 10) if task in tasks_to_process: processed_tasks_identifiers.add(task_identifier) # Get or create the TaskNode task_template = TaskTemplate.objects.get(id=task_template_id) task_node, created = TaskNode.objects.get_or_create( task_template=task_template, due_date=task_date, defaults={ "status": "pending", "folder": task_template.folder, }, ) task_node.to_delete = False task_node.save(update_fields=["to_delete"]) # Replace the task dictionary with the actual TaskNode in the original list - tasks_list[i] = TaskNodeReadSerializer(task_node).data + processed_tasks.append(TaskNodeReadSerializer(task_node).data) + else: + processed_tasks.append(task) -return tasks_list +return processed_tasks🧰 Tools
🪛 Ruff (0.8.2)
5176-5176:
TaskTemplatemay be undefined, or defined from star imports(F405)
5177-5177:
TaskNodemay be undefined, or defined from star imports(F405)
5188-5188:
TaskNodeReadSerializermay be undefined, or defined from star imports(F405)
5281-5282: Fix typo in method parameter name.There's a typo in the parameter name (
srlfinstead ofself).-def status(srlf, request): +def status(self, request): return Response(dict(TaskNode.TASK_STATUS_CHOICES))🧰 Tools
🪛 Ruff (0.8.2)
5282-5282:
TaskNodemay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/core/models.py(1 hunks)backend/core/serializers.py(1 hunks)backend/core/views.py(5 hunks)frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/Calendar/Calendar.svelte(4 hunks)frontend/src/lib/utils/stores.ts(1 hunks)frontend/src/routes/(app)/(internal)/calendar/[year]/[month]/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/calendar/[year]/[month]/+page.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/src/lib/utils/stores.ts
- frontend/messages/fr.json
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/routes/(app)/(internal)/calendar/[year]/[month]/+page.server.ts
- frontend/src/routes/(app)/(internal)/calendar/[year]/[month]/+page.svelte
- frontend/messages/en.json
- backend/core/models.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/core/serializers.py (2)
backend/core/serializer_fields.py (1)
FieldsRelatedField(24-86)backend/core/models.py (14)
risk_assessments(934-935)risk_assessments(2158-2159)assigned_to(4333-4334)next_occurrence(4259-4264)last_occurrence_status(4267-4279)Meta(124-125)Meta(179-180)Meta(207-208)Meta(212-214)Meta(811-813)Meta(877-879)Meta(1008-1010)TaskTemplate(4147-4298)TaskNode(4301-4338)
🪛 Ruff (0.8.2)
backend/core/views.py
5115-5115: TaskTemplate may be undefined, or defined from star imports
(F405)
5176-5176: TaskTemplate may be undefined, or defined from star imports
(F405)
5177-5177: TaskNode may be undefined, or defined from star imports
(F405)
5188-5188: TaskNodeReadSerializer may be undefined, or defined from star imports
(F405)
5192-5192: TaskTemplate may be undefined, or defined from star imports
(F405)
5195-5195: TaskNode may be undefined, or defined from star imports
(F405)
5221-5221: TaskTemplate may be undefined, or defined from star imports
(F405)
5227-5227: TaskNode may be undefined, or defined from star imports
(F405)
5234-5234: TaskNode may be undefined, or defined from star imports
(F405)
5253-5253: TaskTemplate may be undefined, or defined from star imports
(F405)
5277-5277: TaskNode may be undefined, or defined from star imports
(F405)
5282-5282: TaskNode may be undefined, or defined from star imports
(F405)
5285-5285: TaskNode may be undefined, or defined from star imports
(F405)
backend/core/serializers.py
1419-1419: TaskTemplate may be undefined, or defined from star imports
(F405)
1425-1425: TaskTemplate may be undefined, or defined from star imports
(F405)
1439-1439: TaskNode may be undefined, or defined from star imports
(F405)
1445-1445: TaskNode may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
frontend/src/lib/components/Calendar/Calendar.svelte (9)
4-6: Imports look good.The newly introduced imports for SlideToggle, page store, and showAllTasks are correctly declared without any apparent issues.
12-16: Validated date calculations.Using
new Date(year, month, 0)to get the last day of the month andnew Date(year, month - 1, 1).getDay()for the first day is a well-known pattern and appears logically correct with the 1-based month approach.
42-44: Simple function for current month navigation.This function correctly generates a path to the current month and year. Confirm that the route
/calendar/<year>/<month>is valid in the rest of the application.
46-51: Month navigation logic (nextMonth).The logic properly handles transitioning from December to January of the next year.
54-60: Month navigation logic (prevMonth).The logic correctly handles transitioning from January to December of the previous year.
62-71: Reactive filtering of tasks.The conditional logic ensures that if showAllTasks is false, tasks are filtered to those assigned to the current user. Consider verifying that each event's
usersfield always exists to prevent potential undefined property access.
80-90: Anchor-based navigation elements.Replacing button-based navigation with anchor links is straightforward. The references to
prevMonth(year, month)andnextMonth(year, month)meet the desired routing approach.
111-113: Key block usage for filtered events.Wrapping
<Day>components with{#key filteredInfo}ensures reactivity when the filtered list changes. This is a solid approach.
117-127: SlideToggle integration for task visibility.Binding
$showAllTasksto the SlideToggle is a concise way to manage state. The link tocurrentMonth()is also correctly implemented.backend/core/serializers.py (4)
1408-1421: Introducing TaskTemplateReadSerializer.This serializer exposes relationship fields and computed properties (e.g., next_occurrence, last_occurrence_status) while excluding
schedule. Ensure all needed fields are included or excluded as intended.🧰 Tools
🪛 Ruff (0.8.2)
1419-1419:
TaskTemplatemay be undefined, or defined from star imports(F405)
1423-1427: Write serializer includes schedule?
TaskTemplateWriteSerializerusesfields = "__all__", which implicitly includes theschedulefield. This may be intentional, but confirm that it aligns with your plan to excludeschedulefrom reads while allowing it on writes.🧰 Tools
🪛 Ruff (0.8.2)
1425-1425:
TaskTemplatemay be undefined, or defined from star imports(F405)
1429-1441: TaskNodeReadSerializer logic.Exposing
task_template,folder, and the dynamicnamefield referencing the template looks consistent with the model. Also note that you excludeto_deletefrom output.🧰 Tools
🪛 Ruff (0.8.2)
1439-1439:
TaskNodemay be undefined, or defined from star imports(F405)
1443-1447: TaskNodeWriteSerializer excludes task_template.Hiding the
task_templatefield on writes suggests it’s set automatically or managed in code. Verify that you don’t need to capture user input for task_template in create/update operations.🧰 Tools
🪛 Ruff (0.8.2)
1445-1445:
TaskNodemay be undefined, or defined from star imports(F405)
backend/core/views.py (2)
1316-1316: Good addition of month and year filtering.Adding month and year filtering options to date fields in
AppliedControlFilterSetandRiskAcceptanceFilterSetis a valuable enhancement for calendar-based filtering.Also applies to: 1994-1994
5269-5273: Good transaction handling in object method.The override of the
objectaction includes a call to_sync_task_nodesto ensure task nodes are synchronized when fetching a task template, which maintains data consistency.
|
|
||
|
|
||
| class TaskTemplateReadSerializer(BaseModelSerializer): | ||
| folder = FieldsRelatedField() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace wildcard import with explicit imports.
Static analysis flags TaskTemplate and TaskNode as potentially undefined due to from core.models import *. To resolve these warnings and follow best practices, replace the star import with explicit imports:
- from core.models import *
+ from core.models import TaskTemplate, TaskNodeAlso applies to: 1419-1419, 1425-1425, 1439-1439, 1445-1445
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/src/lib/components/Calendar/Day.svelte (3)
8-10: Add type definitions for exported storesThe exported stores
selectedDayandshowSidePanellack type definitions, which could lead to type safety issues. Consider adding proper TypeScript interfaces to improve type safety and code maintainability.-export let info: any[]; -export let selectedDay; -export let showSidePanel; +export let info: any[]; // Consider defining a more specific type for calendar items +export let selectedDay: import('svelte/store').Writable<{day: number, month: number, year: number} | null>; +export let showSidePanel: import('svelte/store').Writable<boolean>;
23-28: Consider performance optimization for filteringThe day info filtering is performed independently in each Day component, which could become inefficient if there are many events or calendar days. Consider moving this filtering logic up to the parent Calendar component and passing down only the relevant items for each day.
-$: dayInfo = info.filter( - (item) => - item.date.getDate() === day && - item.date.getMonth() + 1 === month && - item.date.getFullYear() === year -); +export let dayInfo: any[]; // Receive pre-filtered info from parent
39-44: Consider extracting utility functionThe
truncateLabelfunction is a good utility that might be useful in other components. Consider moving it to a shared utilities file if it's used elsewhere in the application.frontend/src/lib/components/Calendar/Calendar.svelte (2)
20-27: Add type definitions for side panel storesSimilar to the Day component, the writable stores lack type definitions. Consider adding proper TypeScript interfaces:
-export const selectedDay = writable(null); -export const showSidePanel = writable(false); +export const selectedDay = writable<{day: number, month: number, year: number} | null>(null); +export const showSidePanel = writable<boolean>(false);
136-138: Consider performance optimization with memoized filteringSimilar to the comment for the Day component, consider pre-filtering the info for each day at the Calendar level rather than passing the entire info array to each Day component. This could improve performance, especially with many events.
-<Day {day} {month} {year} info={filteredInfo} {selectedDay} {showSidePanel} /> +{#key day} + {@const dayInfo = filteredInfo.filter(item => + item.date.getDate() === day && + item.date.getMonth() + 1 === month && + item.date.getFullYear() === year + )} + <Day {day} {month} {year} dayInfo={dayInfo} {selectedDay} {showSidePanel} /> +{/key}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/Calendar/Calendar.svelte(3 hunks)frontend/src/lib/components/Calendar/Day.svelte(1 hunks)frontend/src/lib/components/Calendar/Notifications.svelte(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/lib/components/Calendar/Notifications.svelte
✅ Files skipped from review due to trivial changes (2)
- frontend/messages/fr.json
- frontend/messages/en.json
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
frontend/src/lib/components/Calendar/Day.svelte (6)
12-13: LGTM: Clear constant definitionsThe constants for today's date and the maximum number of items to display are well-defined.
15-21: LGTM: Well-implemented date comparisonsThe reactive declarations for
isTodayandisPastare cleanly implemented and provide good semantics for styling and interaction logic.
30-32: LGTM: Efficient calculation of visible and extra itemsThe reactive declarations for calculating visible items and extra items count are well-implemented.
34-37: LGTM: Clean side panel managementThe
openSidePanelfunction appropriately updates both stores to show the selected day's details.
48-60: LGTM: Clean button implementation with dynamic stylingThe day button element has well-structured conditional styling that reflects the day's state (current day, past, etc.) and proper event handling.
62-89: LGTM: Well-organized display of day informationThe conditional rendering of day information with a limit on visible items and a "+N" button for additional items is well-implemented and provides a clean user experience.
frontend/src/lib/components/Calendar/Calendar.svelte (8)
4-8: LGTM: Appropriate imports for enhanced functionalityThe new imports support the enhanced calendar functionality well, particularly the SlideToggle for task filtering and the stores for state management.
14-19: LGTM: Well-structured calendar date managementThe export of month and year variables with defaults to the current date and the reactive calculations for calendar structure are clean and effective.
29-37: LGTM: Efficient filtering of selected day itemsThe reactive declaration for filtering items for the selected day is well-implemented.
63-81: LGTM: Improved navigation with URL-based approachThe navigation functions now return URL strings instead of mutating state, which is a better approach for maintainability and bookmarkable routes.
95-116: LGTM: Responsive layout with dynamic width adjustmentThe flex layout with dynamic width adjustment based on side panel visibility provides a good user experience, and the month navigation links are clean.
141-156: LGTM: Well-designed control sectionThe calendar controls section with today button and task filtering toggle is well-designed and provides good functionality.
158-195: LGTM: Excellent side panel implementationThe side panel implementation with smooth transitions and detailed day information provides a great user experience. The conditional rendering and styling are well done.
83-92:❓ Verification inconclusive
Verify user property access
The filtering logic assumes
user.idis available. Ensure you have proper null checks for cases where user data might not be loaded yet:-filteredInfo = info.filter((event) => event.users.some((userObj) => userObj.id === user.id)); +filteredInfo = info.filter((event) => user && event.users.some((userObj) => userObj.id === user.id));
🏁 Script executed:
#!/bin/bash # Checking how user data is handled elsewhere in the codebase rg -A 2 "page.data.user" --type svelteLength of output: 69
Below is an updated script to explicitly search for the user data usage in Svelte files. This will help verify how the user object is accessed in the codebase:
Once this script is executed and you have the output, we can better confirm whether a null-check is needed around
userin the filtering logic. Please run this script for manual verification of the user property usage.
🏁 Script executed:
#!/bin/bash # Re-run rg search to correctly identify occurrences of "$page.data.user" in .svelte files rg -A 4 "\$page\.data\.user" -g "*.svelte"Length of output: 165
Null Check on
userRequired:
The filtering logic infrontend/src/lib/components/Calendar/Calendar.svelteassumes that theuserobject (sourced from$page.data.user) is always available. To guard against runtime errors in scenarios where user data might not have loaded yet, update the filter condition as follows:- filteredInfo = info.filter((event) => event.users.some((userObj) => userObj.id === user.id)); + filteredInfo = info.filter((event) => user && event.users.some((userObj) => userObj.id === user.id));Please verify that this change aligns with how user data is managed across the app, ensuring that a null or undefined user does not cause unintended failures in event filtering.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/routes/(app)/(internal)/calendar/[year]/[month]/+page.svelte (3)
11-40: Consider enhancing type safety with more specific interfaces.The
createCalendarEventsfunction usesRecord<string, string>for all input parameters, but the actual objects likely have more specific structures. Using dedicated interfaces would improve type safety and code readability.+interface AppliedControl { + id: string; + name: string; + eta: string; + owner: string[]; +} + +interface RiskAcceptance { + id: string; + name: string; + expiry_date: string; + approver?: string; +} + +interface Task { + id: string; + name: string; + due_date: string; + assigned_to: string[]; + virtual: boolean; +} + function createCalendarEvents( - appliedControls: Record<string, string>[], - riskAcceptances: Record<string, string>[], - tasks: Record<string, string>[] + appliedControls: AppliedControl[], + riskAcceptances: RiskAcceptance[], + tasks: Task[] ): Array<{ label: string; date: Date; link: string; users: string[]; color: string }> {
34-34: Add a comment explaining thevirtualproperty.The code checks for
task.virtualto determine the link URL, but it's not immediately clear what this property represents. A brief comment would improve code readability.- link: task.virtual ? `/task-templates/${task.id}` : `/task-nodes/${task.id}`, + // Virtual tasks are templates, non-virtual tasks are individual task nodes + link: task.virtual ? `/task-templates/${task.id}` : `/task-nodes/${task.id}`,
11-42: Consider moving thecreateCalendarEventsfunction to a utility file.The
createCalendarEventsfunction is a good candidate for extraction to a separate utility file, especially if it might be reused elsewhere in the application. This would improve code organization and maintainability.Consider creating a new file like
$lib/utils/calendar.tsand moving this function there. Then import it in this component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/lib/components/Calendar/Calendar.svelte(3 hunks)frontend/src/lib/components/Calendar/Day.svelte(1 hunks)frontend/src/routes/(app)/(internal)/calendar/[year]/[month]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/lib/components/Calendar/Day.svelte
- frontend/src/lib/components/Calendar/Calendar.svelte
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/src/routes/(app)/(internal)/calendar/[year]/[month]/+page.svelte (1)
1-46: Good integration of tasks into the calendar view.The component effectively integrates tasks alongside existing applied controls and risk acceptances in the calendar view. The consistent color coding and labeling will help users distinguish between different types of events.
| date: new Date(control.eta), | ||
| link: `/applied-controls/${control.id}`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for date parsing and missing fields.
The code creates Date objects directly from strings without validation. If a date string is invalid or missing, this could lead to unexpected behavior. Consider adding error handling or validation.
...appliedControls.map((control: Record<string, string>) => ({
label: `AC: ${control.name}`,
- date: new Date(control.eta),
+ date: control.eta ? new Date(control.eta) : new Date(),
link: `/applied-controls/${control.id}`,
users: control.owner,
color: 'tertiary'
})),
...riskAcceptances.map((ra: Record<string, string>) => ({
label: `RA: ${ra.name}`,
- date: new Date(ra.expiry_date),
+ date: ra.expiry_date ? new Date(ra.expiry_date) : new Date(),
link: `/risk-acceptances/${ra.id}`,
users: ra.approver ? [ra.approver] : [],
color: 'secondary'
})),
...tasks.map((task: Record<string, string>) => ({
label: `TA: ${task.name}`,
- date: new Date(task.due_date),
+ date: task.due_date ? new Date(task.due_date) : new Date(),
link: task.virtual ? `/task-templates/${task.id}` : `/task-nodes/${task.id}`,Also applies to: 26-27, 33-34
| appliedControls: Record<string, string>[], | ||
| riskAcceptances: Record<string, string>[], | ||
| tasks: Record<string, string>[] | ||
| ): Array<{ label: string; date: Date; link: string }> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return type should include all properties.
The function's return type only lists some of the properties (label, date, link) but the actual return includes additional properties (users, color). Update the return type to match the actual implementation.
-): Array<{ label: string; date: Date; link: string }> {
+): Array<{ label: string; date: Date; link: string; users: string[] | []; color: string }> {📝 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.
| ): Array<{ label: string; date: Date; link: string }> { | |
| ): Array<{ label: string; date: Date; link: string; users: string[] | []; color: string }> { |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores