-
Notifications
You must be signed in to change notification settings - Fork 1
Enhance: Application 部分の重複する記述を関数として統一 #697
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@sahara2006 I've opened a new pull request, #698, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
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 refactors duplicate logic in the Application section by extracting common patterns into reusable composables. The main changes follow the established pattern from the partition feature, creating a unified approach for managing edit modes and update operations.
Key changes:
- Created
useApplicationInformationcomposable to centralize application editing logic - Renamed
EditModetoPartitionEditModefor clarity and consistency - Refactored
ApplicationContent.vueto use the new composable pattern with props and events
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/features/application/store.ts |
Adds helper function createDefaultApplicationSeed() and an incomplete store definition that needs to be removed |
src/components/partitionDetail/composables/usePartitionInformation.ts |
Renames EditMode type to PartitionEditMode for better clarity, but missing import for usePartitionStore |
src/components/partitionDetail/PartitionName.vue |
Updates type import to use renamed PartitionEditMode |
src/components/partitionDetail/PartitionGroup.vue |
Updates type import to use renamed PartitionEditMode |
src/components/partitionDetail/PartitionBudget.vue |
Updates type import to use renamed PartitionEditMode |
src/components/applicationDetail/composables/useApplicationInformation.ts |
New composable following the partition pattern, but contains incorrect error message referencing "partition" instead of "application" |
src/components/applicationDetail/ApplicationLogs.vue |
Integrates the new useApplicationInformation composable |
src/components/applicationDetail/ApplicationContent.vue |
Major refactor to use props/events pattern, but has duplicate imports and missing destructured functions from store |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/applicationDetail/composables/useApplicationInformation.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/applicationDetail/composables/useApplicationInformation.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/applicationDetail/composables/useApplicationInformation.ts
Show resolved
Hide resolved
src/components/applicationDetail/composables/useApplicationInformation.ts
Show resolved
Hide resolved
| <ApplicationContent | ||
| :is-edit-mode="editMode === 'content'" | ||
| @change-edit-mode="changeEditMode($event)" | ||
| @finish-editing="finishEditing" /> |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ApplicationContent no longer accepts the application prop from its parent, instead reading currentApplication from the store. However, the parent component (ApplicationLogs) still requires an application prop and validates it. This creates a potential inconsistency where the parent has a valid application but the child might render with a different (or null) application from the store. Consider either: 1) passing the application prop to ApplicationContent to ensure consistency, or 2) having ApplicationLogs also read from the store if the intent is to make the store the single source of truth.
重複する記述を削除 Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/applicationDetail/composables/useApplicationInformation.ts
Show resolved
Hide resolved
| (e: 'changeEditMode', value: ApplicationEditMode): void | ||
| (e: 'finishEditing'): void | ||
| }>() | ||
| const { currentApplication: application, editedValue } = useApplicationStore() |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The component now sources application from the store (line 26) rather than from props, but the parent component ApplicationLogs.vue still passes application as a prop and uses it to compute logs. This creates an inconsistency in the data flow pattern. Either both components should use the store, or both should use props. The current mixed approach could lead to confusion about the source of truth for the application data.
|
|
||
| const changeEditMode = (mode: ApplicationEditMode) => { | ||
| if (mode !== '') { | ||
| editMode.value = mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
後で切り出す
| } | ||
| isEditMode.value = false | ||
| } | ||
| const hasAuthority = computed(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auth 切り出し
sahara2006
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleUpdateTargetの検討
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { editMode, isSending, changeEditMode, finishEditing } = | ||
| useApplicationInformation() |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple components (ApplicationLogs, ApplicationHeader, and ApplicationSidebar) are each creating their own instance of useApplicationInformation(), which means each will have its own separate editMode and isSending state. This causes state synchronization issues where editing in one component won't properly update the edit mode in others. The composable should either be shared at a parent level or the state should be moved to the store.
| const { editMode, isSending, changeEditMode, finishEditing } = | ||
| useApplicationInformation() |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple components (ApplicationLogs, ApplicationHeader, and ApplicationSidebar) are each creating their own instance of useApplicationInformation(), which means each will have its own separate editMode and isSending state. This causes state synchronization issues where editing in one component won't properly update the edit mode in others. The composable should either be shared at a parent level or the state should be moved to the store.
| try { | ||
| await editApplication(props.application.id, { | ||
| ...props.application, | ||
| partition: props.application.partition.id, | ||
| targets: editedTargets.value, | ||
| }) | ||
| toast.success('更新しました') | ||
| editedValue.value.targets = editedTargets.value | ||
| emit('finishEditing') | ||
| } catch { | ||
| toast.error('更新に失敗しました') | ||
| } | ||
| isEditMode.value = false | ||
| } |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The try-catch block here is misleading. The code inside try block (lines 52-53) is a simple assignment that cannot throw an error, making the catch block unreachable. If error handling is needed, it should be around the finishEditing call that will be made by the parent component.
| if (mode !== '') { | ||
| editMode.value = mode | ||
| if (currentApplication.value) { | ||
| editedValue.value = { | ||
| createdBy: currentApplication.value.createdBy, | ||
| title: currentApplication.value.title, | ||
| content: currentApplication.value.content, | ||
| tags: [...currentApplication.value.tags], | ||
| partition: currentApplication.value.partition.id, | ||
| targets: [...currentApplication.value.targets], | ||
| } | ||
| } | ||
| } else { | ||
| editMode.value = '' | ||
| if (currentApplication.value) { | ||
| editedValue.value = { | ||
| createdBy: currentApplication.value.createdBy, | ||
| title: currentApplication.value.title, | ||
| content: currentApplication.value.content, | ||
| tags: [...currentApplication.value.tags], | ||
| partition: currentApplication.value.partition.id, | ||
| targets: [...currentApplication.value.targets], | ||
| } | ||
| } |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic in changeEditMode duplicates the same editedValue assignment in both branches (when mode is empty string and when it's not). This duplication is unnecessary since both branches reset editedValue to the current application values. Consider extracting this logic to avoid duplication.
| if (mode !== '') { | |
| editMode.value = mode | |
| if (currentApplication.value) { | |
| editedValue.value = { | |
| createdBy: currentApplication.value.createdBy, | |
| title: currentApplication.value.title, | |
| content: currentApplication.value.content, | |
| tags: [...currentApplication.value.tags], | |
| partition: currentApplication.value.partition.id, | |
| targets: [...currentApplication.value.targets], | |
| } | |
| } | |
| } else { | |
| editMode.value = '' | |
| if (currentApplication.value) { | |
| editedValue.value = { | |
| createdBy: currentApplication.value.createdBy, | |
| title: currentApplication.value.title, | |
| content: currentApplication.value.content, | |
| tags: [...currentApplication.value.tags], | |
| partition: currentApplication.value.partition.id, | |
| targets: [...currentApplication.value.targets], | |
| } | |
| } | |
| editMode.value = mode | |
| if (currentApplication.value) { | |
| editedValue.value = { | |
| createdBy: currentApplication.value.createdBy, | |
| title: currentApplication.value.title, | |
| content: currentApplication.value.content, | |
| tags: [...currentApplication.value.tags], | |
| partition: currentApplication.value.partition.id, | |
| targets: [...currentApplication.value.targets], | |
| } |
| const editedTargets = ref<ApplicationTargetDetail[]>( | ||
| props.application.targets.map(t => ({ ...t })) | ||
| application.value?.targets.map(t => ({ ...t })) ?? [] | ||
| ) |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The editedTargets ref is initialized once when the component is created, but it's not synchronized with the editedValue from the store or updated when edit mode is toggled. When the user clicks edit, changeEditMode updates editedValue.targets in the store, but editedTargets in this component remains with the initial values. This creates a state synchronization issue where changes made in editedTargets won't be reflected in editedValue until handleUpdateTargets is called, and vice versa.
| targets: editedTargets.value, | ||
| }) | ||
| toast.success('更新しました') | ||
| editedValue.value.targets = editedTargets.value |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a type mismatch between ApplicationTargetDetail (which includes id, paidAt, and createdAt fields) being assigned to editedValue.value.targets which expects ApplicationTarget[] (only amount and target fields). This will cause the API to receive unexpected fields, potentially causing issues on the backend.
| editedValue.value.targets = editedTargets.value | |
| editedValue.value.targets = editedTargets.value.map(({ amount, target }) => ({ | |
| amount, | |
| target | |
| })) |
| content: currentApplication.value.content, | ||
| tags: [...currentApplication.value.tags], | ||
| partition: currentApplication.value.partition.id, | ||
| targets: [...currentApplication.value.targets], |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assigning currentApplication.value.targets (ApplicationTargetDetail[]) to editedValue.value.targets (ApplicationTarget[]) creates a type mismatch. ApplicationTargetDetail includes additional fields (id, paidAt, createdAt) that should not be in ApplicationTarget. The targets should be mapped to extract only the amount and target fields needed for ApplicationSeed.
| const { isSending, editMode, changeEditMode, finishEditing } = | ||
| useApplicationInformation() |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple components (ApplicationLogs, ApplicationHeader, and ApplicationSidebar) are each creating their own instance of useApplicationInformation(), which means each will have its own separate editMode and isSending state. This causes state synchronization issues where editing in one component won't properly update the edit mode in others. The composable should either be shared at a parent level or the state should be moved to the store.
#474 送信などのロジックをusePartitionInformation.tsに分離