fix:#681 Added cancel buttons on Edit Group page#702
fix:#681 Added cancel buttons on Edit Group page#702
Conversation
tonyb650
requested review from
bethanyann,
nickytonline and
shashilo
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe Edit Group page now includes a close button (X icon) in the header and a cancel button in the footer, both navigating to the dashboard. Corresponding test suite refactored from create group tests to edit group tests with new mocks and reorganized test structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditGroupPage as Edit Group Page
participant Router
User->>EditGroupPage: View page
alt Close/Cancel Interaction
User->>EditGroupPage: Click X button or Cancel button
EditGroupPage->>Router: Navigate to /dashboard
Router->>User: Redirect to dashboard
else Save/Delete Actions
User->>EditGroupPage: Click Save Changes or Delete Group
EditGroupPage->>EditGroupPage: Handle group update/deletion
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
shashilo
requested review from
a team and
shashilo
and removed request for
a team,
bethanyann,
nickytonline and
shashilo
![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 (2)
app/gift-exchanges/[id]/edit/page.tsx (1)
413-432: Simplify spacing utilities to avoid conflicts.The combination of
space-x-2 space-y-2alongsidesm:gap-3withsm:space-x-0 sm:space-y-0creates redundant and potentially conflicting spacing rules. At the small breakpoint,gap-3is applied while space utilities are reset to 0, but at smaller screens both sets of utilities are active.Consider simplifying to:
-<div className="sm:flex sm:justify-center md:justify-end sm:gap-3 mx-3 space-x-2 space-y-2 sm:space-x-0 sm:space-y-0"> +<div className="flex flex-col sm:flex-row sm:justify-center md:justify-end gap-2 sm:gap-3 mx-3">This approach uses flex-col for mobile (stacked buttons) and flex-row at sm+ breakpoints, with gap handling all spacing consistently.
app/gift-exchanges/[id]/edit/page.test.tsx (1)
66-98: Consider testing Calendar integration within EditGroupPage.The current test renders the
Calendarcomponent in isolation rather than as part ofEditGroupPage. This verifies the Calendar's behavior but doesn't test its integration with the form (e.g., the exchange date calendar's dependency ongiftDrawingDate).Consider adding an integration test:
it('disables past dates for drawing date calendar in EditGroupPage', async () => { render(<EditGroupPage />); // Wait for component to load await screen.findByText('Edit Secret Santa Page'); // Open drawing date calendar const drawingDateButton = screen.getByRole('button', { name: /gift drawing date/i }); fireEvent.click(drawingDateButton); // Verify past dates are disabled const pastDate = screen.getByText('13'); expect(pastDate).toBeDisabled(); });Alternatively, if this test is meant to verify the Calendar component itself, consider moving it to the Calendar component's test file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/gift-exchanges/[id]/edit/page.test.tsx(1 hunks)app/gift-exchanges/[id]/edit/page.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/gift-exchanges/[id]/edit/page.test.tsx (1)
app/gift-exchanges/[id]/edit/page.tsx (1)
EditGroupPage(85-439)
🔇 Additional comments (5)
app/gift-exchanges/[id]/edit/page.tsx (3)
19-19: LGTM!The X icon import is correctly added and used in the header close button.
76-78: LGTM!Formatting adjustment aligns with code style conventions.
195-201: LGTM!The X button implementation correctly uses the
asChildpattern with Next.js Link for client-side navigation to the dashboard. Thedata-testidattribute enables proper test coverage.app/gift-exchanges/[id]/edit/page.test.tsx (2)
44-53: LGTM!The test correctly verifies that the X button navigates to the dashboard. Once the Supabase auth mock is added (as noted in the previous comment), this test will properly validate the full component rendering.
55-64: LGTM!The test correctly verifies the Cancel button's navigation behavior and uses accessible queries with
getByRole.
| jest.mock('next/navigation', () => ({ | ||
| useRouter: () => ({ | ||
| push: jest.fn(), | ||
| refresh: jest.fn(), | ||
| }), | ||
| useParams: () => ({ | ||
| id: 'mock-group-id', | ||
| }), | ||
| })); | ||
|
|
||
| beforeEach(() => { | ||
| jest.useFakeTimers(); | ||
| jest.setSystemTime(currentDate); | ||
| class MockResizeObserver { | ||
| observe() {} | ||
| unobserve() {} | ||
| disconnect() {} | ||
| } | ||
|
|
||
| global.ResizeObserver = MockResizeObserver; | ||
| global.fetch = jest | ||
| .fn() | ||
| .mockResolvedValue({ | ||
| ok: true, | ||
| json: () => Promise.resolve({ owner_id: '1', name: '' }), | ||
| }); |
There was a problem hiding this comment.
Add Supabase auth mock and complete the fetch mock data.
The component uses supabase.auth.getUser() to verify ownership (lines 114-119 in page.tsx), but there's no mock for the Supabase client. Additionally, the fetch mock returns incomplete data that doesn't match the form's expected structure.
Add the following mocks:
+jest.mock('@/lib/supabase/client', () => ({
+ createClient: () => ({
+ auth: {
+ getUser: jest.fn().mockResolvedValue({
+ data: { user: { id: '1' } },
+ error: null,
+ }),
+ },
+ }),
+}));
+
global.ResizeObserver = MockResizeObserver;
global.fetch = jest
.fn()
.mockResolvedValue({
ok: true,
- json: () => Promise.resolve({ owner_id: '1', name: '' }),
+ json: () => Promise.resolve({
+ id: 'mock-group-id',
+ owner_id: '1',
+ name: 'Test Group',
+ description: 'Test Description',
+ drawing_date: '2025-12-20',
+ exchange_date: '2025-12-25',
+ budget: '10-20',
+ group_image: '/images/group-1.png',
+ }),
});📝 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.
| jest.mock('next/navigation', () => ({ | |
| useRouter: () => ({ | |
| push: jest.fn(), | |
| refresh: jest.fn(), | |
| }), | |
| useParams: () => ({ | |
| id: 'mock-group-id', | |
| }), | |
| })); | |
| beforeEach(() => { | |
| jest.useFakeTimers(); | |
| jest.setSystemTime(currentDate); | |
| class MockResizeObserver { | |
| observe() {} | |
| unobserve() {} | |
| disconnect() {} | |
| } | |
| global.ResizeObserver = MockResizeObserver; | |
| global.fetch = jest | |
| .fn() | |
| .mockResolvedValue({ | |
| ok: true, | |
| json: () => Promise.resolve({ owner_id: '1', name: '' }), | |
| }); | |
| jest.mock('next/navigation', () => ({ | |
| useRouter: () => ({ | |
| push: jest.fn(), | |
| refresh: jest.fn(), | |
| }), | |
| useParams: () => ({ | |
| id: 'mock-group-id', | |
| }), | |
| })); | |
| jest.mock('@/lib/supabase/client', () => ({ | |
| createClient: () => ({ | |
| auth: { | |
| getUser: jest.fn().mockResolvedValue({ | |
| data: { user: { id: '1' } }, | |
| error: null, | |
| }), | |
| }, | |
| }), | |
| })); | |
| class MockResizeObserver { | |
| observe() {} | |
| unobserve() {} | |
| disconnect() {} | |
| } | |
| global.ResizeObserver = MockResizeObserver; | |
| global.fetch = jest | |
| .fn() | |
| .mockResolvedValue({ | |
| ok: true, | |
| json: () => Promise.resolve({ | |
| id: 'mock-group-id', | |
| owner_id: '1', | |
| name: 'Test Group', | |
| description: 'Test Description', | |
| drawing_date: '2025-12-20', | |
| exchange_date: '2025-12-25', | |
| budget: '10-20', | |
| group_image: '/images/group-1.png', | |
| }), | |
| }); |
There was a problem hiding this comment.
Pull request overview
This PR adds cancel buttons to the Edit Group page, matching the UX improvements previously implemented for the Create Group page in PR #633. The changes provide users with more intuitive navigation options beyond the "Back to Dashboard" link.
Key Changes:
- Added an X button in the upper-right corner of the Edit Group form
- Added a Cancel button in the footer alongside the existing Save Changes and Delete Group buttons
- Updated styling and layout for better responsive behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/gift-exchanges/[id]/edit/page.tsx | Adds X button to header and Cancel button to footer, updates responsive styling for button layout |
| app/gift-exchanges/[id]/edit/page.test.tsx | Adds test coverage for new X and Cancel buttons, reorganizes existing Calendar tests into better structure |
| /> | ||
| <div className="flex md:justify-start justify-center md:m-5 m-0 w-full"> | ||
| <Button className="m-2" type="submit"> | ||
| <div className="sm:flex sm:justify-center md:justify-end sm:gap-3 mx-3 space-x-2 space-y-2 sm:space-x-0 sm:space-y-0"> |
There was a problem hiding this comment.
The container div uses conflicting spacing utilities: space-x-2 space-y-2 are applied at the base level but then overridden with sm:space-x-0 sm:space-y-0 on small screens. This creates inconsistent spacing behavior.
On screens smaller than sm breakpoint, both space-x-2 and space-y-2 are applied, which will add spacing in both directions. This can cause layout issues since the buttons will stack vertically but also have horizontal spacing.
Consider using a consistent approach like the Create Group page (flex gap-2 justify-center md:justify-end p-3) or conditionally applying spacing based on flex direction.
| <div className="sm:flex sm:justify-center md:justify-end sm:gap-3 mx-3 space-x-2 space-y-2 sm:space-x-0 sm:space-y-0"> | |
| <div className="flex flex-col sm:flex-row gap-2 justify-center md:justify-end p-3"> |
| @@ -1,36 +1,99 @@ | |||
| import { Calendar } from '@/components/Calendar/calendar'; | |||
There was a problem hiding this comment.
Missing copyright header. Test files in this codebase should include:
// Copyright (c) Gridiron Survivor.
// Licensed under the MIT License.This can be seen in other test files like app/create-group-page/page.test.tsx.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| import { Check, ChevronsUpDown, ChevronLeft } from 'lucide-react'; | ||
| import { Input } from '@/components/Input/Input'; | ||
| import { Button } from '@/components/Button/button'; | ||
| import { CalendarIcon } from 'lucide-react'; | ||
| import { CalendarIcon, X } from 'lucide-react'; |
There was a problem hiding this comment.
The CalendarIcon and X imports from lucide-react on line 19 should be combined with the other lucide-react imports on line 16.
Currently:
import { Check, ChevronsUpDown, ChevronLeft } from 'lucide-react';
...
import { CalendarIcon, X } from 'lucide-react';Should be:
import { CalendarIcon, Check, ChevronsUpDown, ChevronLeft, X } from 'lucide-react';This follows the pattern used in app/create-group-page/page.tsx line 16.
| <Button variant="ghost" asChild data-testid="x-button"> | ||
| <Link href="/dashboard"> | ||
| <X className="text-black" /> | ||
| </Link> | ||
| </Button> |
There was a problem hiding this comment.
The X button lacks an accessible label for screen readers. Users relying on assistive technologies won't know what this button does.
Add an aria-label to the Button component:
<Button variant="ghost" asChild data-testid="x-button" aria-label="Close and return to dashboard">
<Link href="/dashboard">
<X className="text-black" />
</Link>
</Button>The same issue should be addressed in the Create Group page if not already fixed.
Description
Before:
Navigating away from the "Edit Group" page was unintuitive, with only the "< Back to Dashboard" link and no "Cancel" button.
After:
There are now 2 additional, more conventional ways to navigate back to the dashboard page: an "X" button in the upper-right corner and a "Cancel" button at the bottom of the page.
Closes #681
Additional information
This is a follow up to PR #633 which has been merged in and addressed the exact same issue for the "Create Group" page.
Screenshots
Pre-submission checklist
test #001: created unit test for __ component)Peer Code ReviewersandSenior+ Code Reviewersgroupsgis-code-questionsSummary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.