バグの除去が完了したかも - #29
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 addresses bug fixes related to date handling, response schema cleanup, and UI improvements. The changes modify how dates are stored and validated, remove an unused response schema, and enhance the group detail page with invite URL functionality.
Changes:
- Changed
occurred_atfrom timestamp to date type in the database schema and made it required in the API - Removed unused
registerGroupDebtResponseSchemaand changed the register debt endpoint to return 204 No Content - Added invite URL copy functionality and additional group information fields to the group detail page
- Fixed error messages and removed display of user IDs in member lists
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| products/validator/src/response/group.ts | Added fields to group info response, added description and occurred_at to debt history, removed unused registerGroupDebtResponseSchema |
| products/validator/src/request/group.ts | Made occurred_at required with date format validation |
| products/validator/src/index.ts | Removed export of unused registerGroupDebtResponseSchema |
| products/frontend/src/routes/Root/index.tsx | Simplified member list display by removing user ID |
| products/frontend/src/routes/GroupDetail/index.tsx | Added invite URL copy functionality, changed date input from datetime-local to date, improved error messages |
| products/frontend/src/routes/GenerateGroup/index.tsx | Fixed success message grammar |
| products/frontend/src/api/openapi.d.ts | Updated type definitions to match API changes |
| products/frontend/openapi.json | Updated OpenAPI specification to match schema changes |
| products/backend/src/presentation/routes/info.ts | Removed updatedAt field update on soft delete |
| products/backend/src/presentation/routes/group.ts | Updated to use camelCase for inviteId, changed response to 204 No Content, removed timestamp assignments relying on database defaults |
| products/backend/src/db/pay-crew2-schema.ts | Changed occurredAt from timestamp to date type, renamed invite_id property to inviteId |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,5 @@ | |||
| // drizzle | |||
| import { pgTable, text, timestamp, index, uuid, uniqueIndex, integer } from 'drizzle-orm/pg-core'; | |||
| import { pgTable, text, timestamp, index, uuid, uniqueIndex, integer, date } from 'drizzle-orm/pg-core'; | |||
There was a problem hiding this comment.
The date import from 'drizzle-orm/pg-core' is added but should be verified that it's correctly used for the occurredAt field. Ensure that the date type correctly handles the YYYY-MM-DD format expected by the API validators and doesn't introduce timezone-related issues.
| amount: integer('amount').notNull(), | ||
| description: text('description'), | ||
| occurredAt: timestamp('occurred_at').defaultNow().notNull(), | ||
| occurredAt: date('occurred_at').notNull(), |
There was a problem hiding this comment.
Changing the column type from timestamp to date removes time information from debt records. This is a breaking change that could result in data loss if existing records contain time information. Consider using a migration strategy to handle existing data appropriately.
| occurredAt: date('occurred_at').notNull(), | |
| occurredAt: timestamp('occurred_at').notNull(), |
| setCopyStatus('success'); | ||
| } catch (e) { | ||
| console.error(e); | ||
| setCopyStatus('error'); |
There was a problem hiding this comment.
The inviteUrlHandler function does not reset the copy status after success. Users who copy the URL multiple times will see "コピー済み" (Copied) permanently after the first successful copy. Consider adding a timeout to reset the status back to 'idle' after a few seconds to provide better user feedback for subsequent copy operations.
| setCopyStatus('success'); | |
| } catch (e) { | |
| console.error(e); | |
| setCopyStatus('error'); | |
| setCopyStatus('success'); | |
| // 一定時間後にステータスをリセット | |
| setTimeout(() => { | |
| setCopyStatus('idle'); | |
| }, 2000); | |
| } catch (e) { | |
| console.error(e); | |
| setCopyStatus('error'); | |
| // エラー表示も一定時間後にリセット | |
| setTimeout(() => { | |
| setCopyStatus('idle'); | |
| }, 2000); |
| await db.insert(groupMembership).values({ | ||
| id: crypto.randomUUID(), | ||
| groupId: result[0].id, | ||
| userId: loginUser.id, |
There was a problem hiding this comment.
Removing the joinedAt field assignment means this field will rely solely on the database default. If the database schema doesn't have a proper defaultNow() or equivalent default configured for this column, it could result in null values. Verify that the database schema has the appropriate default configured.
| userId: loginUser.id, | |
| userId: loginUser.id, | |
| joinedAt: new Date(), |
| @@ -188,7 +177,6 @@ hono.openapi(joinGroupSchema, async (c) => { | |||
| id: crypto.randomUUID(), | |||
| groupId: groupData[0].id, | |||
| userId: user.id, | |||
There was a problem hiding this comment.
Removing the joinedAt field assignment means this field will rely solely on the database default. If the database schema doesn't have a proper defaultNow() or equivalent default configured for this column, it could result in null values. Verify that the database schema has the appropriate default configured.
| userId: user.id, | |
| userId: user.id, | |
| joinedAt: new Date(), |
| .set({ | ||
| deletedBy: loginUser.id, | ||
| deletedAt: now, |
There was a problem hiding this comment.
Removing the updatedAt field update on deletion is inconsistent with typical soft-delete patterns. When a record is soft-deleted (by setting deletedAt and deletedBy), it's common practice to also update the updatedAt timestamp to track when the deletion occurred. Consider whether this removal was intentional or if it could cause issues with audit trails or timestamp-based queries.
| <li key={index}> | ||
| {debt.debtor_name} さんが {debt.creditor_name} さんに {debt.amount} 円を借りています。 | ||
| {debt.debtor_name} さんが {debt.creditor_name} さんに {debt.amount} 円を借りています。 (詳細:{' '} | ||
| {debt.description}、発生日時: {debt.occurred_at} |
There was a problem hiding this comment.
The closing parenthesis ")" is missing at the end of line 238. The text starts with an opening parenthesis "(詳細:" on line 237 but doesn't have a corresponding closing parenthesis, which will result in malformed display text in the UI.
| {debt.description}、発生日時: {debt.occurred_at} | |
| {debt.description}、発生日時: {debt.occurred_at} ) |
| id: uuid('id').primaryKey(), | ||
| name: text('name').notNull(), | ||
| invite_id: text('invite_id').notNull().unique(), | ||
| inviteId: text('invite_id').notNull().unique(), |
There was a problem hiding this comment.
The column name is changed from invite_id (snake_case) to inviteId (camelCase) in the schema definition, but the actual database column name remains invite_id due to the second parameter in the text() function. This inconsistency between the schema property name and how it's referenced in the code could lead to confusion. Ensure all references in the codebase use the correct property name inviteId.
| inviteId: text('invite_id').notNull().unique(), | |
| invite_id: text('invite_id').notNull().unique(), |
| name: body.group_name, | ||
| invite_id: `${crypto.randomUUID()}-${crypto.randomUUID()}`, | ||
| inviteId: `${crypto.randomUUID()}-${crypto.randomUUID()}`, | ||
| createdBy: loginUser.id, |
There was a problem hiding this comment.
The removal of createdAt and updatedAt timestamp assignments means these fields will rely solely on database defaults. If the database default is not set or fails, this could result in null values. Verify that the database schema has proper defaultNow() or equivalent defaults configured for these columns.
| createdBy: loginUser.id, | |
| createdBy: loginUser.id, | |
| createdAt: new Date(), | |
| updatedAt: new Date(), |
| @@ -597,7 +566,6 @@ hono.openapi(deleteGroupDebtSchema, async (c) => { | |||
| .set({ | |||
| deletedBy: loginUser.id, | |||
| deletedAt: now, | |||
There was a problem hiding this comment.
Removing the updatedAt field update on deletion is inconsistent with typical soft-delete patterns. When a record is soft-deleted (by setting deletedAt and deletedBy), it's common practice to also update the updatedAt timestamp to track when the deletion occurred. Consider whether this removal was intentional or if it could cause issues with audit trails or timestamp-based queries.
| deletedAt: now, | |
| deletedAt: now, | |
| updatedAt: now, |
No description provided.