Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This pull request implements a major refactoring of the Pay Crew2 application, consolidating the user profile functionality and improving the overall code structure. The changes include merging the user_profile table into the user table, updating API endpoints to use proper HTTP methods (PATCH instead of PUT for updates, 204 for deletions), adding new environment variables, and enhancing the frontend with better UI/UX features.
Changes:
- Consolidated database schema by merging
user_profiletable intousertable - Updated API endpoints to use semantically correct HTTP methods and status codes (PATCH for updates, 204 No Content for deletions)
- Added VITE_CLIENT_URL environment variable and invite URL copying functionality in the frontend
- Refactored backend code to improve consistency and remove unnecessary validation logic
Reviewed changes
Copilot reviewed 34 out of 38 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| setup/src/types.ts | Added VITE_CLIENT_URL to frontend configuration |
| setup/src/index.ts | Integrated VITE_CLIENT_URL into environment setup |
| setup/src/functions.ts | Added VITE_CLIENT_URL loading and logging |
| products/validator/src/response/userProfile.ts | Removed unused updateUserProfile response schema |
| products/validator/src/response/sample.ts | Deleted sample schema file (cleanup) |
| products/validator/src/response/info.ts | Updated group schema with separate creator ID and name fields |
| products/validator/src/response/group.ts | Removed unused deleteGroupDebt response schema |
| products/validator/src/request/group.ts | Renamed field from 'name' to 'group_name' for clarity |
| products/validator/src/index.ts | Updated exports to remove deleted schemas |
| products/validator/src/form/group.ts | Added new form schema for group debt registration |
| products/frontend/src/routes/Root/index.tsx | Enhanced UI with Japanese labels and improved loading states |
| products/frontend/src/routes/Profile/index.tsx | Refactored to use PATCH method and simplified state management |
| products/frontend/src/routes/Invite/index.tsx | Simplified error handling and state management |
| products/frontend/src/routes/GroupDetail/index.tsx | Added member dropdowns and prevention of selecting same user for debtor/creditor |
| products/frontend/src/routes/GenerateGroup/index.tsx | Added invite URL copying functionality with VITE_CLIENT_URL |
| products/frontend/src/api/openapi.d.ts | Updated TypeScript types to match API changes |
| products/frontend/openapi.json | Updated OpenAPI specification with new endpoints and status codes |
| products/frontend/.env.example | Added VITE_CLIENT_URL variable |
| products/backend/src/presentation/utils/types.ts | Added TransactionType for transaction aggregation |
| products/backend/src/presentation/routes/userProfile.ts | Refactored to query user table directly, use PATCH method and 204 response |
| products/backend/src/presentation/routes/info.ts | Updated to use user table, improved query efficiency |
| products/backend/src/presentation/routes/group.ts | Comprehensive refactoring with improved validation and consistency |
| products/backend/src/presentation/factory/hono.ts | Changed validation error message to generic "Bad Request" |
| products/backend/src/index.ts | Added PATCH to allowed CORS methods |
| products/backend/src/db/schema.ts | Updated exports to include auth and pay-crew2 schemas |
| products/backend/src/db/relation.ts | Removed userProfile relations after table consolidation |
| products/backend/src/db/auth-schema.ts | Merged userProfile fields into user table |
| products/backend/drizzle.config.ts | Updated schema paths for better organization |
| products/backend/README.md | Added documentation for schema structure and better-auth |
| products/backend/.env.example | Added Google OAuth credentials |
| pnpm-workspace.yaml | Updated dotenv-caster dependency version |
| pnpm-lock.yaml | Lock file updates for dependency changes |
| flake.lock | Updated nixpkgs version |
| assets/er.mmd | Updated ER diagram to reflect schema consolidation |
| .github/workflows/preview-frontend.yaml | Added VITE_CLIENT_URL to build environment |
| .github/workflows/deploy-frontend.yaml | Added VITE_CLIENT_URL to deployment environment |
| .env.example | Added Google OAuth credentials (missing VITE_CLIENT_URL) |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| path: '/api/profile', | ||
| method: 'get', | ||
| description: 'get user profile', | ||
| description: 'ログインユーザーのプロフィール情報を取得するエンドポイント', |
There was a problem hiding this comment.
The description for this endpoint has been changed to Japanese. For consistency with the rest of the OpenAPI specification, consider keeping endpoint descriptions in English, or standardize all descriptions to Japanese throughout the API.
|
|
||
| // add user to the group | ||
| const groupId = groupData[0].id; | ||
| // すでに参加している場合は即座に返スす |
There was a problem hiding this comment.
Typo in comment. The Japanese text reads "即座に返スす" which contains an erroneous katakana "ス" in the middle of the word. It should be "即座に返す" (return immediately).
| // すでに参加している場合は即座に返スす | |
| // すでに参加している場合は即座に返す |
| const DebtorNameInfo = await db | ||
| .select({ | ||
| name: user.name, | ||
| displayName: user.displayName, | ||
| }) | ||
| .from(user) | ||
| .where(eq(user.id, debtEntry.debtorId)) | ||
| .limit(1); | ||
| if ( | ||
| debtorDisplayNameInfo.length > 0 && | ||
| typeof debtorDisplayNameInfo[0].displayName === 'string' && | ||
| debtorDisplayNameInfo[0].displayName.length > 0 | ||
| ) { | ||
| debtorName = debtorDisplayNameInfo[0].displayName; | ||
| } | ||
|
|
||
| // get creditor name from user table | ||
| const creditorInfo = await db.select().from(user).where(eq(user.id, debtEntry.creditorId)).limit(1); | ||
| if (creditorInfo.length === 0) { | ||
| throw new HTTPException(500, { message: 'A creditor user was not found' }); | ||
| } | ||
| let creditorName = creditorInfo[0].name; | ||
| // get display name from user_profile table | ||
| const creditorDisplayNameInfo = await db | ||
| .select() | ||
| .from(userProfile) | ||
| .where(eq(userProfile.userId, debtEntry.creditorId)) | ||
|
|
||
| // creditor_name を取得 | ||
| // NOTE: 共通化できそう | ||
| // ユーザ名を取得 (user table) | ||
| const CreditorNameInfo = await db |
There was a problem hiding this comment.
Inconsistent variable naming. In this file, some variables use PascalCase like DebtorNameInfo and CreditorNameInfo, while similar variables elsewhere use camelCase like createdByUserNameInfo and memberUserNameInfo. Consider using consistent camelCase naming convention throughout the codebase.
| @@ -1,5 +1,6 @@ | |||
| # frontend | |||
| VITE_API_URL= | |||
There was a problem hiding this comment.
Missing environment variable VITE_CLIENT_URL. The diff shows VITE_CLIENT_URL being added to various configuration files (setup/src/types.ts, products/frontend/.env.example, GitHub workflows), but it's missing from the root .env.example file. This variable should be added after VITE_API_URL for consistency.
| VITE_API_URL= | |
| VITE_API_URL= | |
| VITE_CLIENT_URL= |
| console.error(result.error); | ||
| throw new HTTPException(400, { | ||
| message: 'Zod Validation Error', | ||
| message: 'Bad Request', |
There was a problem hiding this comment.
The error message has been changed from "Zod Validation Error" to the generic "Bad Request". While this may be intentional for security reasons to avoid exposing internal validation details, it makes debugging more difficult. Consider logging the specific validation error internally while returning a generic message to the client, or provide more specific error messages in a development environment.
| 201: { | ||
| description: 'Created', |
There was a problem hiding this comment.
Inconsistent HTTP status code for POST endpoint. This POST endpoint for retrieving debt history returns 201 (Created), but it's not creating a resource - it's retrieving existing data. This should use 200 (OK) status code instead, or the endpoint should be changed to use GET method if it's purely a read operation.
| 201: { | |
| description: 'Created', | |
| 200: { | |
| description: 'OK', |
| setResult({ | ||
| group_id: data?.group_id, | ||
| invite_id: data?.invite_id, | ||
| }); | ||
| setInviteUrl(`${import.meta.env.VITE_CLIENT_URL}/invite/${data?.invite_id}`); |
There was a problem hiding this comment.
Potential null/undefined handling issue. The code checks data?.group_id and data?.invite_id in the onSuccess callback, but if either is falsy (including empty string), the result will still be set with those potentially invalid values. Consider validating that these values are non-empty strings before setting the result.
| setResult({ | |
| group_id: data?.group_id, | |
| invite_id: data?.invite_id, | |
| }); | |
| setInviteUrl(`${import.meta.env.VITE_CLIENT_URL}/invite/${data?.invite_id}`); | |
| const groupId = | |
| typeof data?.group_id === 'string' && data.group_id.trim() !== '' ? data.group_id : null; | |
| const inviteId = | |
| typeof data?.invite_id === 'string' && data.invite_id.trim() !== '' ? data.invite_id : null; | |
| if (groupId && inviteId) { | |
| setResult({ | |
| group_id: groupId, | |
| invite_id: inviteId, | |
| }); | |
| setInviteUrl(`${import.meta.env.VITE_CLIENT_URL}/invite/${inviteId}`); | |
| } else { | |
| setResult(null); | |
| setInviteUrl(null); | |
| } |
| 201: { | ||
| description: 'Created', |
There was a problem hiding this comment.
Inconsistent HTTP status code. The endpoint description says "Created" but returns status code 201, which is correct for resource creation. However, the GET endpoints for group info and debt history also return 201 (Created) instead of 200 (OK), which is semantically incorrect. GET requests retrieve existing resources and should return 200, not 201.
| 201: { | |
| description: 'Created', | |
| 200: { | |
| description: 'OK', |
| display_name: userData[0].display_name === null ? '' : userData[0].display_name, | ||
| avatar_url: userData[0].avatar_url === null ? '' : userData[0].avatar_url, | ||
| bio: userData[0].bio === null ? '' : userData[0].bio, |
There was a problem hiding this comment.
Missing null check before array access. The code accesses userData[0], createdByUserNameInfo[0], and userNameInfo[0] without checking if the array has any elements. If the database query returns an empty array, this will cause a runtime error. Consider adding validation or using optional chaining.
| const createdByUserNameInfo = await db | ||
| .select({ | ||
| name: user.name, | ||
| displayName: user.displayName, | ||
| }) | ||
| .from(user) | ||
| .where(eq(user.id, groupData.createdBy)) | ||
| .limit(1); | ||
|
|
||
| // NOTE: --- 共通化開始 --- | ||
| //* groupData.id のメンバー情報を取得 *// | ||
| // グループメンバーのユーザーIDを取得 (group_membership table) | ||
| const memberUserIds = await db | ||
| .select({ | ||
| userId: groupMembership.userId, | ||
| }) | ||
| .from(groupMembership) | ||
| .where(eq(groupMembership.groupId, groupData.id)); | ||
|
|
||
| // メンバー情報を格納する配列 | ||
| const members: InfoAboutGroupsTheUserBelongsToResponseMemberElementSchemaType[] = []; | ||
|
|
||
| for (const memberUserId of memberUserIds) { | ||
| // initialize user name | ||
| let userName = ''; | ||
|
|
||
| // get user name from user table | ||
| const userNameInfo = await db.select().from(user).where(eq(user.id, memberUserId)).limit(1); | ||
| if (userNameInfo.length === 0) { | ||
| throw new HTTPException(500, { message: 'A group member was not found' }); | ||
| } | ||
| //set user name | ||
| userName = userNameInfo[0].name; | ||
|
|
||
| // get display name from user_profile table | ||
| const displayNameInfo = await db.select().from(userProfile).where(eq(userProfile.userId, memberUserId)).limit(1); | ||
| if ( | ||
| displayNameInfo.length > 0 && | ||
| typeof displayNameInfo[0].displayName === 'string' && | ||
| displayNameInfo[0].displayName.length > 0 | ||
| ) { | ||
| userName = displayNameInfo[0].displayName; | ||
| } | ||
|
|
||
| // push to members array | ||
| // NOTE: 共通化できそう | ||
| // ユーザ名を取得 (user table) | ||
| const userNameInfo = await db | ||
| .select({ | ||
| name: user.name, | ||
| displayName: user.displayName, | ||
| }) | ||
| .from(user) | ||
| .where(eq(user.id, memberUserId.userId)) | ||
| .limit(1); | ||
|
|
||
| // メンバー情報を配列に追加 | ||
| members.push({ | ||
| user_id: memberUserId, | ||
| user_name: userName, | ||
| user_id: memberUserId.userId, | ||
| user_name: | ||
| userNameInfo[0].displayName !== null && userNameInfo[0].displayName.length > 0 | ||
| ? userNameInfo[0].displayName | ||
| : userNameInfo[0].name, | ||
| }); | ||
| } | ||
| // push to groupInfo array | ||
| // NOTE: --- 共通化終了 --- | ||
|
|
||
| // グループ情報を配列に追加 | ||
| groupInfo.push({ | ||
| group_id: groupData.id, | ||
| group_name: groupData.name, | ||
| created_by: groupData.createdBy, | ||
| created_by_id: groupData.createdBy, | ||
| created_by_name: | ||
| createdByUserNameInfo[0].displayName !== null && createdByUserNameInfo[0].displayName.length > 0 | ||
| ? createdByUserNameInfo[0].displayName | ||
| : createdByUserNameInfo[0].name, |
There was a problem hiding this comment.
Potential array access error. Multiple array accesses without checking array length: groupData[0], createdByUserNameInfo[0], memberUserNameInfo[0], etc. Throughout this file, database query results are accessed at index 0 without verifying the array is non-empty, which could cause runtime errors.
No description provided.