検索結果画面で2回目以降の検索が反映されるように修正 #195

Futadaruma · 2024-07-08T06:38:32Z

User description

close #191

PR Type

Bug fix

Description

src/components/Search/UserList.vueで、queryプロパティを追加し、APIからユーザーを動的に取得するfetchUsers関数を実装。
src/pages/SearchPage.vueで、user-listコンポーネントにqueryプロパティを渡すように変更し、ユーザーの取得ロジックを削除。

Changes walkthrough 📝

Relevant files

Bug fix

UserList.vue `ユーザーリストの動的更新を実装` src/components/Search/UserList.vue `ref`と`watch`をインポート `query`プロパティを追加 `fetchUsers`関数を追加し、APIからユーザーを取得 `watch`で`query`の変更を監視し、`fetchUsers`を呼び出す	+10/-2
SearchPage.vue `検索クエリをユーザーリストに渡す処理を修正` src/pages/SearchPage.vue `users`の取得ロジックを削除 `user-list`コンポーネントに`query`プロパティを渡すように変更	+1/-3

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions · 2024-07-08T06:39:38Z

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review データ取得のエラーハンドリング: `fetchUsers`関数でAPIからデータを取得する際、エラーハンドリングが実装されていません。API呼び出しに失敗した場合のユーザー体験を考慮し、適切なエラーハンドリングを追加することをお勧めします。パフォーマンスの懸念: `watch`関数が`immediate: true`で設定されているため、コンポーネントの初期化時にも`fetchUsers`が呼び出されます。これにより、不要なAPI呼び出しが発生する可能性があります。`query`の初期値が空でない場合のみAPIを呼び出すように検討してください。

github-actions · 2024-07-08T06:40:32Z

PR Code Suggestions ✨

Latest suggestions up to 877643f

Category	Suggestion	Score
Possible issue	非同期処理をライフサイクルフック内で行うように変更する `await`式は`setup()`関数内で直接使用することはできません。非同期データの取得は`onMounted`や`watch`などのライフサイクルフックを使用して行うべきです。これにより、コンポーネントの初期化が完了する前にデータが利用可能になることを保証できます。 src/components/Search/UserList.vue [10] -const users = (await apis.getUsers(undefined, props.search)).data +let users = ref([]) +onMounted(async () => { + users.value = (await apis.getUsers(undefined, props.search)).data +}) Suggestion importance[1-10]: 9 Why: The suggestion correctly identifies that `await` should not be used directly in the setup function and provides a proper solution using `onMounted`. This improves the reliability and correctness of the component initialization.	9
Best practice	プロパティの型を明確にする `props`の定義において、`search`プロパティの型を明示的に指定することで、より型安全なコードにすることができます。TypeScriptを使用しているため、この機会に型の利点を最大限に活用しましょう。 src/components/Search/UserList.vue [6] -search: string +search: string \| null Suggestion importance[1-10]: 8 Why: Explicitly defining the type of the `search` property increases type safety and leverages TypeScript's strengths, which is a best practice.	8
Enhancement	ローディングインジケーターを詳細にするタグの`fallback`スロットには、ローディング中のユーザー体験を向上させるために、より詳細なローディングインジケーターを提供することをお勧めします。例えば、スピナーやプログレスバーを使用することが考えられます。 src/pages/SearchPage.vue [21] -<template #fallback>loading...</template> +<template #fallback> + <div class="loading-container"> + <progress-spinner></progress-spinner> + <span>Loading...</span> + </div> +</template> Suggestion importance[1-10]: 7 Why: Enhancing the loading indicator improves user experience, but it is a minor enhancement and not critical to the functionality of the application.	7
Maintainability	プロップの命名を一貫性のあるものに変更するコンポーネントに`query`プロップを渡す代わりに、`search`プロップを直接使用することで、親コンポーネントと子コンポーネント間のプロップの命名を一貫性を持たせることができます。これにより、コードの可読性が向上します。 src/pages/SearchPage.vue [20] -<user-list :query="query ?? ''" :search="query ?? ' '" /> +<user-list :search="query ?? ''" /> Suggestion importance[1-10]: 6 Why: Ensuring consistent naming of props between parent and child components improves code readability and maintainability, but it is a minor improvement.	6

Previous suggestions

Suggestions up to commit 3e72cd8

Category	Suggestion	Score
Possible bug	API 呼び出しの失敗を処理するために例外処理を追加します。 `fetchUsers` 関数内で例外処理を追加してください。API 呼び出しは失敗する可能性があり、その場合にアプリケーションがクラッシュすることを防ぐためです。 src/components/Search/UserList.vue [13-16] const fetchUsers = async (query: string) => { - const response = await apis.getUsers(undefined, query) - members.value = response.data + try { + const response = await apis.getUsers(undefined, query) + members.value = response.data + } catch (error) { + console.error('Failed to fetch users:', error) + } } Suggestion importance[1-10]: 9 Why: Adding error handling to the `fetchUsers` function is crucial for preventing the application from crashing due to failed API calls. This is a significant improvement for robustness and reliability.	9
Best practice	型安全性を向上させるために、プロパティの型を明示的に指定します。 `props` の定義を `defineProps` で行う際に、`query` プロパティの型を明示的に `string` としてください。これにより、型安全性が向上します。 src/components/Search/UserList.vue [10] -const props = defineProps<Props>() +const props = defineProps<{ query: string }>() Suggestion importance[1-10]: 8 Why: Explicitly defining the type of `query` in `defineProps` enhances type safety and code clarity, which is a good practice in TypeScript.	8
Maintainability	コンポーネントの再利用性を向上させるために、デフォルト値の設定をコンポーネント側で行います。 `search` 変数が `null` または `undefined` の場合に空文字列を渡すのではなく、コンポーネント側でデフォルト値を設定するように変更してください。これにより、コンポーネントの再利用性が向上します。 src/pages/SearchPage.vue [19] -<user-list :query="search ?? ''" /> +<user-list :query="search" /> Suggestion importance[1-10]: 7 Why: Moving the default value handling to the component improves reusability and separation of concerns. However, it is a minor enhancement and not critical.	7

mehm8128 · 2024-07-08T06:49:54Z

src/pages/SearchPage.vue

@@ -18,7 +16,7 @@ onMounted(() => {
  <page-container>
    <div :class="$style.container">
      <page-title>検索結果: {{ search }}</page-title>
-      <user-list :members="users" />
+      <user-list :query="search ?? ''" />


ここで:key="search"すれば、UserList.vueでwatchを使わなくてもsearchが変化したときに再取得できるはずです
https://arc.net/l/quote/cnfkhffg

Pugma · 2024-07-08T12:20:17Z

@mehm8128
僕が一旦 SearchPage.vueで以下の実装をやってみたらしっかり更新時にクエリが飛んでそうだったんですが、これでよさそうじゃないですか…？

import { onMounted, watch, ref } from 'vue'
import { User } from '/@/lib/apis'

const search = useQuery('q')
const users = ref<User[]>((await apis.getUsers(undefined, search.value)).data)

watch(search, async () => {
  users.value = (await apis.getUsers(undefined, search.value)).data
})

mehm8128 · 2024-07-08T12:49:13Z

普段あんまり使わなくて普通にwatchの存在忘れてたので元issueではkey使う方法を提案してました(watchをそんなに知らないのでなんとも言えないけどコードが分かりづらくなりそうなイメージがある)
また、↓みたいにUserListをSuspenseで囲うと、再fetchする度にloadingが表示されるようになるのでそこらへんちゃんと書くならkeyを使って子コンポーネントで取得するようにする方がよさそうな気がします(多分提案してくれた書き方だと再fetch時にtop level awaitしてないのでApp.vueのSuspenseも効かなさそう？)

<suspense :key="search">
        <user-list :query="search ?? ''" :users="users" />
        <template #fallback>loading...</template>
      </suspense>
    </div>

僕はVueそんなに詳しいわけではないのでお任せします

Futadaruma · 2024-07-08T13:07:48Z

複数のコードを比較してどちらが良いか判断できるほどの知識と技術がないので、どれにするか決めるのはお任せしたいです
keyの方法を提案してもらっていたのにwatchで実装したことに深い理由はなくて（そこまで考えれてなくて)、実装方法を色々と調べていたらwatchの記法を見つけて、飛びついてしまったような形です

Pugma · 2024-07-08T14:46:47Z

あ〜、Suspenceが効かないのはそうですね…
色々再描画の手法も調べてみたんですが、新しめの情報があまり出てこなかったのであまり現実的な実装じゃないかもです

そうなるとめふもさんが提案してくださった実装のほうがよさそうですね

個人的には先に挙げたやり方で行けると思っていたので実装が軽いと想定しちゃってました

mehm8128 · 2024-07-10T07:52:49Z

src/pages/SearchPage.vue

@@ -18,7 +18,10 @@ onMounted(() => {
  <page-container>
    <div :class="$style.container">
      <page-title>検索結果: {{ search }}</page-title>
-      <user-list :members="users" />
+      <suspense :key="search">
+        <user-list :query="search ?? ''" :members="users" />


すみません僕が前に提示したコードが間違えてて、usersの取得をUserList.vue側で行うようにしてほしいです:pray:(明日ぷぐまくんが教えてくれるっぽいですが、僕のミスなので...)

Pugma · 2024-07-11T06:04:23Z

/improve

Pugma

僕もペアプロのときにチェック漏れが発生していてお手数をかけて申し訳ないんですが、この変更をお願いします…！

Pugma · 2024-07-11T09:00:37Z

src/pages/SearchPage.vue

-      <user-list :members="users" />
+      <page-title>検索結果: {{ query }}</page-title>
+      <suspense :key="query">
+        <user-list :query="query ?? ''" :search="query ?? ' '" />


不必要なqueryを消してほしいということを伝え忘れていたんですが、これもお願いします！

Suggested change

<user-list :query="query ?? ''" :search="query ?? ' '" />

<user-list :query="query ?? ''" />

Pugma · 2024-07-11T09:01:05Z

src/components/Search/UserList.vue


 interface Props {
-  members: User[]
+  search: string


あわせてこちらの名称も変更をお願いします

Suggested change

search: string

query: string

mehm8128

よさそうです、ありがとうございます:kansya_kansya:

Pugma

修正依頼にも応えてくれてありがとうございます！
お疲れさまでした

検索結果画面で2回目以降の検索が反映されるように修正

3e72cd8

Futadaruma requested a review from Pugma July 8, 2024 06:38

github-actions bot added the Review effort [1-5]: 2 label Jul 8, 2024

format

206f15f

mehm8128 reviewed Jul 8, 2024

View reviewed changes

手詰まり

a2bdfbe

mehm8128 reviewed Jul 10, 2024

View reviewed changes

2回目以降の検索が反映されるように修正

ceb747e

Futadaruma requested a review from mehm8128 July 11, 2024 05:29

変数名の変更search→query

877643f

Pugma requested changes Jul 11, 2024

View reviewed changes

変数名の変更と不要部分の削除

fcc5c05

mehm8128 approved these changes Jul 11, 2024

View reviewed changes

Pugma approved these changes Jul 11, 2024

View reviewed changes

Futadaruma merged commit 4f67e51 into master Jul 11, 2024
8 checks passed

Futadaruma deleted the fix/SearchPage-userlist branch July 11, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

検索結果画面で2回目以降の検索が反映されるように修正 #195

検索結果画面で2回目以降の検索が反映されるように修正 #195

Futadaruma commented Jul 8, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Jul 8, 2024

github-actions bot commented Jul 8, 2024 •

edited

Loading

mehm8128 Jul 8, 2024

Pugma commented Jul 8, 2024

mehm8128 commented Jul 8, 2024

Futadaruma commented Jul 8, 2024

Pugma commented Jul 8, 2024

mehm8128 Jul 10, 2024

Pugma commented Jul 11, 2024

Pugma left a comment

Pugma Jul 11, 2024 •

edited

Loading

Pugma Jul 11, 2024

mehm8128 left a comment

Pugma left a comment

	<user-list :query="query ?? ''" :search="query ?? ' '" />
	<user-list :query="query ?? ''" />

検索結果画面で2回目以降の検索が反映されるように修正 #195

検索結果画面で2回目以降の検索が反映されるように修正 #195

Conversation

Futadaruma commented Jul 8, 2024 • edited by github-actions bot Loading

User description

PR Type

Description

Changes walkthrough 📝

github-actions bot commented Jul 8, 2024

PR Reviewer Guide 🔍

github-actions bot commented Jul 8, 2024 • edited Loading

PR Code Suggestions ✨

Previous suggestions

mehm8128 Jul 8, 2024

Choose a reason for hiding this comment

Pugma commented Jul 8, 2024

mehm8128 commented Jul 8, 2024

Futadaruma commented Jul 8, 2024

Pugma commented Jul 8, 2024

mehm8128 Jul 10, 2024

Choose a reason for hiding this comment

Pugma commented Jul 11, 2024

Pugma left a comment

Choose a reason for hiding this comment

Pugma Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Pugma Jul 11, 2024

Choose a reason for hiding this comment

mehm8128 left a comment

Choose a reason for hiding this comment

Pugma left a comment

Choose a reason for hiding this comment

Futadaruma commented Jul 8, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Jul 8, 2024 •

edited

Loading

Pugma Jul 11, 2024 •

edited

Loading