chore: fix dom struct - #54
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to cancel deleted group debts (restore them to active status) and refactors the warning message display into a reusable component. The changes support soft-delete/restore patterns for debt entries within groups.
Changes:
- Added new API endpoint and frontend functionality to cancel (restore) deleted group debts
- Refactored repeated warning message text into a reusable
WarningMessagecomponent - Extended debt history to include and display deleted debts with deletion metadata (deleted_at, deleted_by_id, deleted_by_name)
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| products/validator/src/response/group.ts | Adds deleted_at, deleted_by_id, and deleted_by_name fields to debt history response schema |
| products/validator/src/request/group.ts | Adds cancelGroupDebtRequestSchema for the cancel debt endpoint |
| products/validator/src/index.ts | Exports the new cancel debt request schema |
| products/frontend/src/share/WarningMessage/index.tsx | Creates reusable WarningMessage component |
| products/frontend/src/share/WarningMessage/index.module.css | Adds styling for WarningMessage component |
| products/frontend/src/share/index.ts | Exports WarningMessage component |
| products/frontend/src/routes/Root/index.tsx | Replaces inline Error component with WarningMessage |
| products/frontend/src/routes/Login/index.tsx | Replaces inline Error component with WarningMessage |
| products/frontend/src/routes/Profile/index.tsx | Improves error message specificity for profile operations |
| products/frontend/src/routes/GroupDetail/index.tsx | Adds cancel debt mutation and handler |
| products/frontend/src/routes/GroupDetail/components/History/index.tsx | Implements UI to display completed debts and cancel deletion button |
| products/frontend/src/routes/GroupDetail/components/History/index.module.css | Adds styling for completed debt entries |
| products/frontend/src/routes/GroupDetail/components/Member/index.module.css | Adds width: fit-content for better layout |
| products/frontend/src/index.css | Updates disabled color to lighter gray (#a3a3a3) |
| products/frontend/src/api/openapi.d.ts | Updates TypeScript definitions for new API endpoint and fields |
| products/frontend/openapi.json | Adds OpenAPI specification for cancel debt endpoint |
| products/backend/src/presentation/routes/info.ts | Adds clarifying comment about UTC time |
| products/backend/src/presentation/routes/group.ts | Implements cancel debt endpoint, updates debt history query to include deleted entries, fixes naming convention |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //* body.debt_id の貸し借りの履歴の削除の取り消し (論理削除の取り消し) *// | ||
| await db | ||
| .update(debt) | ||
| .set({ | ||
| deletedBy: null, | ||
| deletedAt: null, | ||
| }) | ||
| .where(and(eq(debt.id, body.debt_id), eq(debt.groupId, body.group_id), isNotNull(debt.deletedAt))); |
There was a problem hiding this comment.
The cancel endpoint should verify that the debt entry exists and belongs to the specified group before attempting to update it. Currently, if a non-existent debt_id is provided, the update operation will silently succeed without affecting any rows. Consider adding validation to check if the debt exists and return an appropriate error if it doesn't.
| {props.debtHistoryResult.debts.map((debt) => | ||
| debt.deleted_at === null ? ( | ||
| <li className={styles.li} key={debt.debt_id}> | ||
| <div className={styles.info}> | ||
| {/* section 1 */} | ||
| <p className={styles.summary}> | ||
| {debt.debtor_name} さんが {debt.creditor_name} さんに {debt.amount} 円を借りています。 | ||
| </p> | ||
| {/* section 2 */} | ||
| <p> | ||
| <span className={styles.label}>[発生日] </span> | ||
| <span> {debt.occurred_at}</span> | ||
| </p> | ||
| {/* section 3 */} | ||
| {!detail.has(debt.debt_id) && ( | ||
| <button className={styles.button} type="button" onClick={() => detailExpandHandler(debt.debt_id)}> | ||
| さらに表示 | ||
| </button> | ||
| )} | ||
| {detail.has(debt.debt_id) && ( | ||
| <> | ||
| <p className={styles.detail}>{debt.description || '詳細情報は、未記入のようです。'}</p> | ||
| <button | ||
| className={styles.button} | ||
| type="button" | ||
| onClick={() => detailShrinkHandler(debt.debt_id)} | ||
| > | ||
| 閉じる | ||
| </button> | ||
| </> | ||
| )} | ||
| </div> | ||
| <FullPaymentButton | ||
| onClick={() => props.deleteGroupDebtHandler(debt.debt_id)} | ||
| disabled={props.fullPaymentButtonDisabled} | ||
| /> | ||
| </li> | ||
| ) : ( | ||
| showCompleted && ( | ||
| <li className={styles.completedLi} key={debt.debt_id}> | ||
| <div className={styles.info}> | ||
| {/* section 1 */} | ||
| <p className={styles.completedSummary}> | ||
| {debt.debtor_name} さんが {debt.creditor_name} さんに {debt.amount} 円を借りていました。 | ||
| </p> | ||
| {/* section 2 */} | ||
| <p> | ||
| <span className={styles.completedLabel}>[削除日] </span> | ||
| <span className={styles.completedValue}> {debt.deleted_at}</span> | ||
| </p> | ||
| {/* section 3 */} | ||
| <div className={styles.completedButtonWrapper}> | ||
| {!detail.has(debt.debt_id) ? ( | ||
| <button | ||
| className={styles.button} | ||
| type="button" | ||
| onClick={() => detailExpandHandler(debt.debt_id)} | ||
| > | ||
| さらに表示 | ||
| </button> | ||
| ) : ( | ||
| <div>{/* 完済取り消しボタンの一を固定するための、ダミーのDOM */}</div> | ||
| )} | ||
| <button | ||
| className={styles.button} | ||
| type="button" | ||
| onClick={() => props.cancelGroupDebtHandler(debt.debt_id)} | ||
| disabled={props.cancelButtonDisabled} | ||
| > | ||
| 完済取り消し | ||
| </button> | ||
| </div> | ||
| {detail.has(debt.debt_id) && ( | ||
| <div className={styles.completedDetailWrapper}> | ||
| <p> | ||
| <span className={styles.completedLabel}>deleted by : </span> | ||
| <span className={styles.completedValue}> {debt.deleted_by_name}</span> | ||
| </p> | ||
| <p> | ||
| <span className={styles.completedLabel}>[発生日] </span> | ||
| <span className={styles.completedValue}> {debt.occurred_at}</span> | ||
| </p> | ||
| <p className={styles.completedDetail}> | ||
| {debt.description || '詳細情報は、未記入のようです。'} | ||
| </p> | ||
| <button | ||
| className={styles.button} | ||
| type="button" | ||
| onClick={() => detailShrinkHandler(debt.debt_id)} | ||
| > | ||
| 閉じる | ||
| </button> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </li> | ||
| ) | ||
| ) | ||
| )} |
There was a problem hiding this comment.
The map function returns either a React element or a boolean (false) when showCompleted is false and the debt is deleted. While React will skip rendering false values, this is not a clean pattern. Consider filtering the debts array before mapping, or using a null return value, or restructuring the logic to separate active and completed debts for clearer code structure.
| deleted_at: z | ||
| .string() | ||
| .regex(/^\d{4}-\d{2}-\d{2}$/) | ||
| .nullable(), | ||
| deleted_by_id: z.string().min(1).nullable(), | ||
| deleted_by_name: z.string().min(1).nullable(), |
There was a problem hiding this comment.
The validation schema for deleted_by_id and deleted_by_name allows nullable values but also requires min(1) when the value is a string. However, there's a logical inconsistency: if deleted_at is null (not deleted), then deleted_by_id and deleted_by_name should also be null. If deleted_at is not null (deleted), then deleted_by_id and deleted_by_name should be non-null strings. Consider adding a refinement to ensure these fields are consistent with each other - all three should either be null together or all have values together.
| さらに表示 | ||
| </button> | ||
| ) : ( | ||
| <div>{/* 完済取り消しボタンの一を固定するための、ダミーのDOM */}</div> |
There was a problem hiding this comment.
Typo in the comment: "一" should be "位置" (position). The current character means "one" instead of "position".
| <div>{/* 完済取り消しボタンの一を固定するための、ダミーのDOM */}</div> | |
| <div>{/* 完済取り消しボタンの位置を固定するための、ダミーのDOM */}</div> |
No description provided.