Skip to content

Admin Bugbash Fixes - #209

Open
Yurika-Kan wants to merge 18 commits into
mainfrom
yk/admin-bugbash
Open

Admin Bugbash Fixes#209
Yurika-Kan wants to merge 18 commits into
mainfrom
yk/admin-bugbash

Conversation

@Yurika-Kan

Copy link
Copy Markdown
Collaborator

📝 Description

  • Navbar clears the user only on unauthenticated, killing the load/refresh flicker; phone validation messages reworded.
  • Invalid ?donationId / ?orderId deep links show a "not found" error.
  • Donation Statistics totals fetch is independent of pagination, so "All Pantries" renders on every page.
  • Failed application fetches render an "Unable to Load Applications" error state.
  • Application Review "View Details" uses navigate() instead of href (SPA transition).
  • Cognito errors include the AWS reason and re-throw existing HttpExceptions.
  • Approve Food Manufacturers pagination arrows disable at first/last page.
  • Pantry assignee column checks for active volunteers before falling back to "No Volunteer".
  • User Management shows an "Admin" badge on admin rows.
  • Donation / Pantry / Manufacturer / Order Management show a "no results match filter" state.
  • Order Management table wrapped in overflowX="auto".
  • Removed dev-only "All Pages" nav link.
  • Donation details and delete modals hold the last donation in a ref while closing, fixing focus-trap errors.
  • Admin order table now refreshes auto after allocation edits.

Copilot AI lite review requested due to automatic review settings August 12, 2026 04:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR bundles a set of admin-facing UI/UX bug fixes across the frontend (empty/error states, pagination behavior, SPA navigation, table overflow, and modal stability) plus backend improvements to validation messages and Cognito error propagation, along with an auth endpoint audit document.

Changes:

  • Improve admin list pages: add “no matching results” / error states, fix pagination behaviors, and enhance navigation/labels.
  • Stabilize donation modals during close (avoid focus-trap/body-lock issues) and refresh admin orders after allocation edits.
  • Update backend phone validation messages and include underlying Cognito/AWS failure reasons in auth errors; add backend auth posture audit doc.

Reviewed changes

Copilot reviewed 19 out of 21 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
apps/frontend/src/containers/userManagement.tsx Adds an “Admin” badge for admin rows in the volunteer table.
apps/frontend/src/containers/approvePantries.tsx Adds fetch error state UI and switches “View Details” to SPA navigation.
apps/frontend/src/containers/approveFoodManufacturers.tsx Adds fetch error UI, “no matching applications” empty state, SPA navigation, and disables pagination arrows at bounds.
apps/frontend/src/containers/adminPantryManagement.tsx Adds “no pantries / no matching pantries” empty state and refines assignee display to only consider active volunteers.
apps/frontend/src/containers/adminOrderManagement.tsx Refactors order fetch into a reusable callback, adds deep-link “not found” error, refreshes after edits, and adds horizontal overflow for the table.
apps/frontend/src/containers/adminDonationStats.tsx Fetches total stats independent of pagination so “All Pantries” totals render on every page.
apps/frontend/src/containers/adminDonation.tsx Adds deep-link “donation not found” error, adds filtered empty state, and renders details modal in a way that supports stable closing.
apps/frontend/src/components/Navbar.tsx Avoids clearing current user during non-unauthenticated transitions; removes dev-only nav link; adjusts label text.
apps/frontend/src/components/forms/fmDeleteDonationModal.tsx Holds the last donation in a ref during close to prevent modal lock/focus issues.
apps/frontend/src/components/forms/donationDetailsModal.tsx Holds the last donation in a ref during close; updates header/details rendering accordingly.
apps/backend/src/users/dtos/userSchema.dto.ts Updates phone validation message wording.
apps/backend/src/users/dtos/update-user-info.dto.ts Updates phone validation message wording.
apps/backend/src/pantries/dtos/update-pantry-application.dto.ts Updates secondary contact phone validation message wording.
apps/backend/src/pantries/dtos/pantry-application.dto.ts Updates phone and secondary contact phone validation message wording.
apps/backend/src/foodManufacturers/dtos/update-manufacturer-application.dto.ts Updates secondary contact phone validation message wording.
apps/backend/src/foodManufacturers/dtos/manufacturer-application.dto.ts Updates phone and secondary contact phone validation message wording.
apps/backend/src/auth/dtos/sign-up.dto.ts Updates phone validation message wording.
apps/backend/src/auth/auth.service.ts Re-throws existing HttpExceptions and appends underlying error “reason” to internal server errors.
apps/backend/src/auth/AUTH_AUDIT.md Adds a living inventory of backend routes and their auth/guard posture.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

if (donation) donationRef.current = donation;
const displayDonation = donation ?? donationRef.current;

const donationId = donation?.donationId;
Comment on lines 336 to 340
<Link
color="neutral.700"
fontWeight={400}
textStyle="p2"
variant="underline"
Comment on lines +373 to +387
<Link
color="neutral.700"
fontWeight={400}
textStyle="p2"
variant="underline"
textDecorationColor="neutral.700"
onClick={() =>
navigate(
ROUTES.FOOD_MANUFACTURER_APPLICATION_DETAILS.replace(
':applicationId',
String(foodManufacturer.foodManufacturerId),
),
)
}
>
);
})()
) : (
<Box color="neutral.600" fontStyle="p2">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also a valid comment.

Comment on lines 46 to 49
@IsPhoneNumber('US', {
message:
'Secondary contact phone must be a valid phone number (make sure all the digits are correct)',
'Secondary phone contact must be a valid phone number. Make sure all the digits are correct.',
})
Comment on lines 76 to 79
@IsPhoneNumber('US', {
message:
'secondaryContactPhone must be a valid phone number (make sure all the digits are correct)',
'Secondary phone contact must be a valid phone number. Make sure all the digits are correct.',
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is a valid comment that we should implement

Comment on lines 39 to 42
@IsPhoneNumber('US', {
message:
'secondaryContactPhone must be a valid phone number (make sure all the digits are correct)',
'Secondary phone contact must be a valid phone number. Make sure all the digits are correct.',
})
Comment on lines 69 to 72
@IsPhoneNumber('US', {
message:
'secondaryContactPhone must be a valid phone number (make sure all the digits are correct)',
'Secondary phone contact must be a valid phone number. Make sure all the digits are correct.',
})

@dburkhart07 dburkhart07 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one other thing: is the test.ico actually needed here?

Comment thread apps/backend/src/auth/AUTH_AUDIT.md Outdated
@@ -0,0 +1,195 @@
# Backend Auth Audit

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file meant to be here?

@IsPhoneNumber('US', {
message:
'phone must be a valid phone number (make sure all the digits are correct)',
'Phone must be a valid phone number. Make sure all the digits are correct.',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency and to avoid confusion, I think we should make all of these "Phone must be a valid US phone number"

Comment on lines 76 to 79
@IsPhoneNumber('US', {
message:
'secondaryContactPhone must be a valid phone number (make sure all the digits are correct)',
'Secondary phone contact must be a valid phone number. Make sure all the digits are correct.',
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is a valid comment that we should implement

</RouterLink>

<VStack align="stretch" gap={2} flex={1} overflowY="auto">
<NavLink

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are at it, we should delete the HOME route here I think

}
}, [setAlertMessage]);

useEffect(() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this a dependency issue and thats why it needed to be a callback? I thought we have a lot of these dependency issues throughout the code. Why are we just cleaning up this one?

);
})()
) : (
<Box color="neutral.600" fontStyle="p2">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also a valid comment.

)}

{totalPages > 1 && (
<Pagination.Root

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our current Pagination uses 2 different patterns it seems.

Pattern 1:

<Pagination.PrevTrigger
              color="neutral.800"
              variant="outline"
              disabled={currentPage === 1}
              _hover={{ color: 'black', cursor: 'pointer' }}
            >
              <ChevronLeft size={16} />
            </Pagination.PrevTrigger>

Pattern 2:

<Pagination.PrevTrigger asChild>
              <IconButton
                variant="ghost"
                disabled={currentPage === 1}
                onClick={() => setCurrentPage((prev) => Math.max(prev - 1, 1))}
              >
                <ChevronLeft />
              </IconButton>
            </Pagination.PrevTrigger>

We should choose just one of these (I think the second one is better), apply the same change for the Pagination next trigger, put it into a separate UI component, and then use those 2 components throughout the entire code, for consistency

{getInitials(volunteer.firstName, volunteer.lastName)}
</Box>
{volunteer.firstName} {volunteer.lastName}
{volunteer.role === Role.ADMIN && (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only admins can access this page anyways i thought. why do we need this?

Prevents "Donation #undefined" header flash during modal close animation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants