feat(001): Implement USA Hockey readiness features#186
Conversation
…umbers, association IDs, and CSV roster export - Added jerseyNumber and usahMemberId fields to Player and TeamMember models. - Created quickstart, research, spec, and tasks documentation for the new features. - Developed validation schemas and server actions for adding and updating players and team officials. - Implemented CSV export functionality with proper encoding and formatting. - Updated UI components to support jersey number and USAH ID input, display, and export. - Added tests for validation, server actions, and CSV export functionality.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's readiness for USA Hockey-affiliated teams by integrating essential roster management features. It introduces the ability to track jersey numbers and USA Hockey Member IDs for both players and team officials, alongside a robust CSV export function for easy data sharing. These changes improve data integrity through comprehensive validation, streamline administrative tasks, and provide a stronger foundation for future hockey-specific functionalities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature for USA Hockey readiness, including jersey numbers, USA Hockey Member IDs, and a CSV roster export. The changes are comprehensive, touching the database schema, server actions, API routes, UI components, and type definitions. The addition of extensive documentation and thorough unit and integration tests is excellent. My review has identified a critical discrepancy between the CSV export implementation and the feature specification regarding the exported columns. I've also noted a minor documentation inconsistency and a potential UX improvement for displaying warnings. Overall, this is a well-executed feature addition that will be even stronger once these points are addressed.
| const headers = [ | ||
| "Type", | ||
| "Name", | ||
| "Email", | ||
| "Phone", | ||
| "Jersey #", | ||
| "USA Hockey Member ID", | ||
| "Emergency Contact", | ||
| "Emergency Phone", | ||
| ]; | ||
|
|
||
| const playerRows = players.map((p) => [ | ||
| "Player", | ||
| p.name, | ||
| p.email, | ||
| p.phone, | ||
| p.jerseyNumber, | ||
| p.usahMemberId, | ||
| p.emergencyContact, | ||
| p.emergencyPhone, | ||
| ]); | ||
|
|
||
| const officialRows = officials.map((o) => [ | ||
| "Team Official", | ||
| o.user.name || o.user.email, | ||
| o.user.email, | ||
| null, | ||
| null, | ||
| o.usahMemberId, | ||
| null, | ||
| null, | ||
| ]); |
There was a problem hiding this comment.
There's a significant discrepancy between the columns in the generated CSV and the requirements outlined in the feature specification (spec.md). The spec requires the export to contain Full Name, Role, Jersey Number, USA Hockey Member ID, Email. However, the current implementation exports additional sensitive fields like Phone and Emergency Contact, and uses Type instead of Role. Please update the implementation to match the specified columns to ensure correctness and avoid exposing unnecessary data.
const headers = [
"Full Name",
"Role",
"Jersey Number",
"USA Hockey Member ID",
"Email",
];
const playerRows = players.map((p) => [
p.name,
"Player",
p.jerseyNumber,
p.usahMemberId,
p.email,
]);
const officialRows = officials.map((o) => [
o.user.name || o.user.email,
"Team Official",
null,
o.usahMemberId,
o.user.email,
]);| if ("warning" in result && result.warning) { | ||
| showError(result.warning); | ||
| } |
There was a problem hiding this comment.
Displaying a success-with-warning message using showError can be misleading for the user, as it suggests the operation failed when it actually succeeded. It would be better to use a distinct visual treatment for warnings (e.g., a yellow toast). If your useToast component supports a 'warning' severity, please use it here. If not, this could be a good opportunity to enhance the useToast hook to include a showWarning function for better user experience.
| ``` | ||
| HTTP 200 | ||
| Content-Type: text/csv; charset=utf-8 | ||
| Content-Disposition: attachment; filename="roster-{teamName}-{YYYY-MM-DD}.csv" |
There was a problem hiding this comment.
The documentation for the Content-Disposition header specifies that the CSV filename should include the date (roster-{teamName}-{YYYY-MM-DD}.csv). However, the implementation in app/api/roster/export/route.ts generates a filename without the date ({teamSlug}-roster.csv). To ensure documentation and implementation are aligned, please update this line to reflect the actual filename format.
| Content-Disposition: attachment; filename="roster-{teamName}-{YYYY-MM-DD}.csv" | |
| Content-Disposition: attachment; filename="{teamSlug}-roster.csv" |
There was a problem hiding this comment.
Pull request overview
Implements “USA Hockey readiness” enhancements across the roster domain by adding jersey numbers + USA Hockey Member IDs to data models, surfacing them in the roster UI, and providing an admin-only CSV export endpoint, with accompanying docs/specs and tests.
Changes:
- Extend Prisma models + shared roster types to include
jerseyNumberandusahMemberId(players + team members). - Add validation + server actions for updating these fields, plus admin roster CSV export utilities/route and UI wiring.
- Add unit/integration tests for validation, roster actions, CSV utilities, and the export route handler.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
types/roster.ts |
Adds jerseyNumber/usahMemberId to Player and introduces TeamMemberWithUser type. |
prisma/schema.prisma |
Adds new nullable columns on Player and TeamMember. |
prisma/migrations/20260319100000_add_jersey_number_usah_member_id/migration.sql |
Migration to add the new columns. |
lib/utils/validation.ts |
Extends player schemas and adds updateTeamMemberUsahIdSchema. |
lib/utils/csv.ts |
Adds RFC 4180-compatible CSV escaping + BOM/CRLF utilities. |
lib/actions/roster.ts |
Persists new fields, adds duplicate-jersey warnings, and introduces updateTeamMemberUsahId. |
lib/actions/team-context.ts |
Expands roster page data fetch to include jersey number / USAH ID (admin-only) + team members list. |
lib/actions/league-context.ts |
Updates league roster typing to include new player fields. |
app/api/roster/export/route.ts |
New admin-only CSV export Route Handler. |
app/(dashboard)/roster/page.tsx |
Passes teamMembers into RosterList. |
components/features/roster/AddPlayerDialog.tsx |
Adds jersey number + USAH ID inputs and handles duplicate warnings. |
components/features/roster/PlayerCard.tsx |
Displays jersey number; shows USAH ID in admin-only section. |
components/features/roster/RosterList.tsx |
Adds CSV export button and a “Team Officials” section for admin management. |
components/features/roster/TeamOfficialCard.tsx |
New component for editing team member USAH IDs. |
__tests__/lib/utils/validation.test.ts |
Adds tests for jersey number + USAH ID validation. |
__tests__/lib/utils/csv.test.ts |
Adds tests for CSV escaping/content generation. |
__tests__/lib/actions/roster.test.ts |
Adds tests for roster actions (jersey warnings, USAH updates). |
__tests__/api/roster/export.test.ts |
Adds tests for export route auth + CSV output edge cases. |
package.json |
Adjusts @vitejs/plugin-react devDependency version. |
CLAUDE.md |
Updates repo structure/testing notes and documents known pre-existing test failures. |
specs/001-usah-jersey-roster-export/* |
Adds feature docs: spec/plan/research/tasks/quickstart/data-model/contracts/checklist. |
| prisma.player.findMany({ | ||
| where: { teamId }, | ||
| orderBy: [{ jerseyNumber: "asc" }, { name: "asc" }], | ||
| select: { | ||
| name: true, | ||
| email: true, | ||
| phone: true, | ||
| jerseyNumber: true, | ||
| usahMemberId: true, | ||
| emergencyContact: true, | ||
| emergencyPhone: true, | ||
| }, | ||
| }), | ||
| prisma.teamMember.findMany({ | ||
| where: { teamId, role: "ADMIN" }, | ||
| orderBy: { user: { name: "asc" } }, | ||
| select: { | ||
| role: true, | ||
| usahMemberId: true, | ||
| user: { select: { name: true, email: true } }, | ||
| }, | ||
| }), | ||
| ]); | ||
|
|
||
| const headers = [ | ||
| "Type", | ||
| "Name", | ||
| "Email", | ||
| "Phone", | ||
| "Jersey #", | ||
| "USA Hockey Member ID", | ||
| "Emergency Contact", | ||
| "Emergency Phone", | ||
| ]; |
There was a problem hiding this comment.
The roster export CSV includes phone and emergency contact fields (both in the DB select and in the CSV headers/rows). This goes beyond the PR description/spec’s stated export columns and increases the amount of sensitive PII in a file that’s likely to be shared externally. Consider limiting the export to the required columns (name/role/jerseyNumber/USAH ID/email), or explicitly updating the spec/requirements if these extra fields are intended.
| const teamSlug = team.name | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9]+/g, "-") | ||
| .replace(/^-|-$/g, ""); | ||
| const filename = `${teamSlug}-roster.csv`; | ||
|
|
There was a problem hiding this comment.
The export filename format doesn’t match the documented roster-{teamName}-{YYYY-MM-DD}.csv pattern in the spec/plan. If consumers rely on the documented naming (or to avoid overwriting downloads), consider adding the date and using the documented prefix, or update the docs/spec to reflect the actual behavior.
| prisma.teamMember.findMany({ | ||
| where: { teamId }, | ||
| select: { | ||
| id: true, | ||
| role: true, | ||
| // FR-008: USAH Member IDs are admin-only | ||
| usahMemberId: isAdmin, | ||
| user: { select: { id: true, name: true, email: true } }, | ||
| }, | ||
| orderBy: [{ role: "asc" }, { joinedAt: "asc" }], |
There was a problem hiding this comment.
getRosterData loads all teamMember records but the UI renders them under a “Team Officials” section. If this section is intended to manage only coaches/managers (ADMIN role), the query should filter where: { teamId, role: 'ADMIN' } (and ideally rename variables accordingly). Otherwise, the UI label/intent should be updated to avoid confusion.
| prisma.teamMember.findMany({ | ||
| where: { teamId }, | ||
| select: { | ||
| id: true, | ||
| role: true, | ||
| // FR-008: USAH Member IDs are admin-only | ||
| usahMemberId: isAdmin, | ||
| user: { select: { id: true, name: true, email: true } }, | ||
| }, | ||
| orderBy: [{ role: "asc" }, { joinedAt: "asc" }], | ||
| }), | ||
| isAdmin |
There was a problem hiding this comment.
getRosterData always queries and returns teamMembers, but RosterList only renders the “Team Officials” section when isAdmin is true. For non-admin users this extra query/serialization work is unnecessary (and still ships the teamMembers payload to the client). Consider only fetching/returning teamMembers when isAdmin, or returning an empty array for non-admin to reduce load and data exposure surface.
| prisma.teamMember.findMany({ | |
| where: { teamId }, | |
| select: { | |
| id: true, | |
| role: true, | |
| // FR-008: USAH Member IDs are admin-only | |
| usahMemberId: isAdmin, | |
| user: { select: { id: true, name: true, email: true } }, | |
| }, | |
| orderBy: [{ role: "asc" }, { joinedAt: "asc" }], | |
| }), | |
| isAdmin | |
| isAdmin | |
| ? prisma.teamMember.findMany({ | |
| where: { teamId }, | |
| select: { | |
| id: true, | |
| role: true, | |
| // FR-008: USAH Member IDs are admin-only | |
| usahMemberId: isAdmin, | |
| user: { select: { id: true, name: true, email: true } }, | |
| }, | |
| orderBy: [{ role: "asc" }, { joinedAt: "asc" }], | |
| }) | |
| : Promise.resolve([]), | |
| isAdmin |
|
Including jersey numbers, association IDs, and CSV roster export.
This pull request introduces comprehensive improvements to the codebase’s test coverage, validation, and documentation, particularly around roster management, CSV export, and schema organization. It adds new unit and integration tests for roster actions, CSV utilities, and input validation, while also expanding and clarifying the project’s file and folder structure in the documentation. These changes help ensure correctness, maintainability, and clarity for both developers and users of the system.
Testing improvements:
GET /api/roster/export), covering authentication, admin checks, CSV formatting (including edge cases for quoting and escaping), and correct header generation. (tests/api/roster/export.test.ts)addPlayer,updatePlayer,updateTeamMemberUsahId), verifying correct handling of jersey numbers, duplicate detection, and error cases. (tests/lib/actions/roster.test.ts)Documentation and project structure:
CLAUDE.mddocumentation to detail new and existing folders and files, including new sections for schedules, venues, league/team context, CSV utilities, shared types, and feature specifications. [1] [2] [3]These enhancements provide a stronger foundation for future development and improve reliability for roster-related features.