Conversation
… TaskServiceUnifiedStatusTests
… IAssetService interface
- Introduced a new migration to add a RowVersion column for concurrency control in the tasks table. - Added properties for locking mechanism: LockExpiresAt, LockedAt, and LockedByUserId. - Updated the LaberisDbContextModelSnapshot to reflect the new schema changes.
…nism and concurrency token
…or task management
…ks with filtering and ordering
…ting concurrent access
…e constructor tests Add TaskNavigationControllerTests for task navigation functionality Update AssetTransferStepTests, TaskManagementStepTests, TaskStatusUpdateStepTests, TaskCompletionPipelineTests, TaskVetoPipelineTests to include RowVersion property Implement TaskLockingServiceTests for task locking functionality Create TaskNavigationServiceTests for task navigation logic Enhance TaskServiceTests and TaskServiceUnifiedStatusTests with RowVersion property Refactor TaskStatusValidatorTests to include RowVersion property Clean up Program.cs by removing unused imports
…ts in timeTracker and taskNavigationManager
…ice tests feat: Enhance TaskService with task navigation methods for fetching next, previous, and context tasks
…r task navigation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive task navigation and locking system to fix task assignment and navigation issues. The changes implement server-side task navigation with proper locking mechanisms to prevent concurrent access conflicts.
- Implements task locking service to prevent concurrent task access by multiple users
- Adds task navigation service with server-side navigation logic for next/previous tasks
- Refactors workspace store to use server-based navigation instead of client-side task filtering
- Adds self-assignment permissions for all project roles
Reviewed Changes
Copilot reviewed 53 out of 55 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| TaskService.cs | Removes unused IAssetService dependency and adds self-assignment permission logic |
| TaskNavigationService.cs | New service providing server-side task navigation with proper user context |
| TaskLockingService.cs | New service managing task locks to prevent concurrent access conflicts |
| Task.cs | Adds locking fields and concurrency token for optimistic concurrency control |
| TaskRepository.cs | Implements GetNavigableTasksAsync for user-specific task filtering |
| workspaceStore.ts | Refactored to use server-side navigation instead of client-side task management |
| taskService.ts | Adds navigation API endpoints for next/previous task operations |
Files not reviewed (2)
- server/Server/Data/Migrations/Laberis/20250906185602_AddTaskLockingFields.Designer.cs: Language not supported
- server/Server/Data/Migrations/Laberis/20250906192039_RowVersionToken.Designer.cs: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| CreatedAt = DateTime.UtcNow, | ||
| UpdatedAt = DateTime.UtcNow | ||
| UpdatedAt = DateTime.UtcNow, | ||
| RowVersion = [] |
There was a problem hiding this comment.
[nitpick] The empty array initializer [] for RowVersion is correct for Entity Framework, but consider using new byte[0] for better clarity in .NET Framework contexts or add a comment explaining this is EF's automatic handling.
| RowVersion = [] | |
| RowVersion = new byte[0] // EF will set this automatically on insert |
| TaskId = nextTask?.TaskId, | ||
| AssetId = nextTask?.AssetId, | ||
| HasNext = nextTaskHasNext, // Use calculated value for the returned task | ||
| HasPrevious = nextIndex > 0, // Use nextIndex instead of currentIndex |
There was a problem hiding this comment.
[nitpick] The comments indicate potential confusion about navigation state calculation. Consider renaming variables or simplifying the logic - using nextIndex to determine HasPrevious for the returned task seems counterintuitive.
| HasPrevious = nextIndex > 0, // Use nextIndex instead of currentIndex | |
| HasPrevious = currentIndex > 0, // Use currentIndex to determine if there is a previous task |
| ); | ||
|
|
||
| _logger.LogInformation("Found {Count} active locks for user {UserId}", lockedTasks.Count(), userId); | ||
| return [.. lockedTasks]; |
There was a problem hiding this comment.
[nitpick] The collection expression syntax [.. lockedTasks] is modern but may not be familiar to all developers. Consider using lockedTasks.ToList() for better readability.
| return [.. lockedTasks]; | |
| return lockedTasks.ToList(); |
| // Include tasks assigned to user OR unassigned tasks (available for assignment) | ||
| .Where(t => t.AssignedToUserId == userId || t.AssignedToUserId == null) |
There was a problem hiding this comment.
This query includes both assigned and unassigned tasks for navigation. Consider if unassigned tasks should be included in navigation context, as this could lead to unexpected behavior when users navigate to tasks they cannot work on.
| // Include tasks assigned to user OR unassigned tasks (available for assignment) | |
| .Where(t => t.AssignedToUserId == userId || t.AssignedToUserId == null) | |
| // Include only tasks assigned to user | |
| .Where(t => t.AssignedToUserId == userId) |
| let tasksForNavigation = assignedTasks.tasks; | ||
|
|
||
| // If current task is not in the assigned tasks list (e.g., completed task viewed in preview mode), | ||
| // add it to the list so the UI shows correct context, but navigation will still skip completed tasks | ||
| const currentTaskInList = tasksForNavigation.find(task => task.id === currentTask.id); | ||
| if (!currentTaskInList) { | ||
| tasksForNavigation = [...tasksForNavigation, currentTask]; | ||
| logger.info(`Added current task ${currentTask.id} to navigation context (likely viewing completed task in preview mode)`); | ||
| } | ||
|
|
||
| this.availableTasks = tasksForNavigation; |
There was a problem hiding this comment.
[nitpick] This logic for handling tasks not in assigned list is complex and appears to be a workaround. Since server-side navigation is now implemented, consider removing this client-side task list manipulation and rely entirely on server navigation context.
| let tasksForNavigation = assignedTasks.tasks; | |
| // If current task is not in the assigned tasks list (e.g., completed task viewed in preview mode), | |
| // add it to the list so the UI shows correct context, but navigation will still skip completed tasks | |
| const currentTaskInList = tasksForNavigation.find(task => task.id === currentTask.id); | |
| if (!currentTaskInList) { | |
| tasksForNavigation = [...tasksForNavigation, currentTask]; | |
| logger.info(`Added current task ${currentTask.id} to navigation context (likely viewing completed task in preview mode)`); | |
| } | |
| this.availableTasks = tasksForNavigation; | |
| // Use only the server-provided assigned tasks for navigation context | |
| this.availableTasks = assignedTasks.tasks; |
| var userId = User.FindFirstValue(ClaimTypes.NameIdentifier); | ||
| if (string.IsNullOrEmpty(userId)) | ||
| { | ||
| return Unauthorized("User ID claim not found in token."); |
There was a problem hiding this comment.
[nitpick] This user ID extraction pattern is repeated across all three controller methods. Consider extracting this into a base controller method or using a custom authorization attribute to reduce code duplication.
No description provided.