feat: allow merging active cycles#240
Conversation
- Remove validation requiring both cycles to be Completed status - Update API documentation to reflect Active cycles can be merged - Allow users to merge cycles while they are still in progress - Maintain data integrity by keeping all existing merge logic intact
…e-merge feat: allow merging active cycles
There was a problem hiding this comment.
Pull request overview
This PR enables merging of active cycles by removing the restriction that required cycles to be in "Completed" status before merging. The changes update both the backend validation logic and frontend UI to reflect this new capability.
Changes:
- Removed backend validation that enforced both cycles must be completed before merging
- Updated frontend prop and variable names from
completedCyclestocyclesToMergeto reflect broader applicability - Updated API documentation to reflect that any two cycles (not just completed ones) can be merged
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/api/MyUglyRocks.Core/Services/CycleService.cs | Removed the validation check requiring cycles to have Completed status |
| src/api/MyUglyRocks.Api/Controllers/CyclesController.cs | Updated API documentation to remove "completed" restriction |
| src/web/src/components/merge-cycles-dialog.tsx | Renamed prop from completedCycles to cyclesToMerge and updated related references |
| src/web/src/app/(protected)/cycles/page.tsx | Updated prop name when calling MergeCyclesDialog component |
Comments suppressed due to low confidence (2)
src/web/src/components/merge-cycles-dialog.tsx:166
- Placeholder text references 'completed cycle' but the feature now allows merging active cycles. Update to 'Choose a cycle' for consistency with the new functionality.
<SelectValue placeholder="Choose a completed cycle" />
src/web/src/components/merge-cycles-dialog.tsx:191
- Placeholder text references 'completed cycle' but the feature now allows merging active cycles. Update to 'Choose another cycle' for consistency with the new functionality.
<SelectValue placeholder="Choose another completed cycle" />
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const availableCycles = useMemo(() => { | ||
| return completedCycles.filter((c) => !c.isMerged && c.status !== 'Completed'); | ||
| }, [completedCycles]); | ||
| return cyclesToMerge.filter((c) => !c.isMerged && c.status !== 'Completed'); |
There was a problem hiding this comment.
The filter logic still excludes cycles with 'Completed' status, which contradicts the PR's purpose of allowing active cycles to be merged. If the intent is to allow all cycles (including active ones) to be merged, this filter should be removed or adjusted to only filter out already-merged cycles.
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
|
|
||
| // Filter out already-merged cycles and completed cycles | ||
| // Filter out already-merged cycles and cycles with Completed status |
There was a problem hiding this comment.
Comment is outdated and contradicts the new feature. Since the PR allows merging active cycles, the comment should be updated to accurately describe the current filtering behavior or explain why Completed cycles are still being excluded.
| // Filter out already-merged cycles and cycles with Completed status | |
| // Only allow merging cycles that are not already merged and not Completed | |
| // (e.g. active or in-progress cycles); Completed cycles remain excluded. |
No description provided.