Skip to content
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

router-linkのto属性でオブジェクトを使用するように #197

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ogu-kazemiya
Copy link

@ogu-kazemiya ogu-kazemiya commented Jul 10, 2024

User description

close #172


PR Type

enhancement


Description

  • router-linkto属性を文字列からオブジェクト形式に変更し、各コンポーネントで適切なparamsを設定。
  • paramsのキーをIdに統一。

Changes walkthrough 📝

Relevant files
Enhancement
14 files
ContestTeamListItem.vue
`router-link`の`to`属性をオブジェクト形式に変更                                                 

src/components/Contest/ContestTeamListItem.vue

  • router-linkto属性をオブジェクト形式に変更
  • paramscontestIdteamIdを設定
  • +4/-1     
    MemberListItem.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/ContestTeam/MemberListItem.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramsuserIdを設定
    +4/-1     
    ContestListItem.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Contests/ContestListItem.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramscontestIdを設定
    +4/-1     
    HostnameListItem.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Event/HostnameListItem.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramsuserIdを設定
    +4/-1     
    EventListItem.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Events/EventListItem.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramseventIdを設定
    +4/-1     
    AdminListItem.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Group/AdminListItem.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramsuserIdを設定
    +4/-1     
    MemberListItem.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Group/MemberListItem.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramsuserIdを設定
    +4/-1     
    GroupListItem.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Groups/GroupListItem.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramsgroupIdを設定
    +4/-1     
    GroupList.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Index/GroupList.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramsgroupIdを設定
    +1/-1     
    PageHeader.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Layout/PageHeader.vue

    • router-linkto属性をオブジェクト形式に変更
    • nameIndexを設定
    +1/-1     
    MemberListItem.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Project/MemberListItem.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramsuserIdを設定
    +4/-1     
    ProjectListItem.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Projects/ProjectListItem.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramsprojectIdを設定
    +4/-1     
    UserListItem.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/Search/UserListItem.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramsuserIdを設定
    +4/-1     
    ContestsContainer.vue
    `router-link`の`to`属性をオブジェクト形式に変更                                                 

    src/components/User/ContestsContainer.vue

    • router-linkto属性をオブジェクト形式に変更
    • paramscontestIdを設定
    +4/-1     

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    パラメータの統一性:
    paramsのキーがIdに統一されているかどうかを確認する必要があります。例えば、userIdcontestIdなどが正しく設定されているかレビューすることが重要です。

    リンクの動作確認:
    router-linkto属性がオブジェクト形式に変更されたため、これらのリンクが正しく機能するかどうかを確認する必要があります。特に、新しいパラメータ形式が適切にルーティングされているかテストすることが推奨されます。

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    存在しないイベントIDを参照するリスクを避けるために条件付きレンダリングを使用する。


    :to属性のオブジェクトでeventIdを使用していますが、このIDが実際に存在するかどうかの検証が必要です。存在しないIDを参照すると、ユーザーがエラーページにリダイレクトされる可能性があります。

    src/components/Events/EventListItem.vue [19]

    -:to="{ name: 'Event', params: { eventId: event.id } }"
    +:to="event.id ? { name: 'Event', params: { eventId: event.id } } : { name: 'NotFound' }"
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug by ensuring that the code does not attempt to navigate to a non-existent event, which would improve user experience and prevent errors.

    9
    Possible issue
    teamIdの値がundefinedになる可能性があるため、オプショナルチェイニングを使用して安全にアクセスする。


    :to属性のオブジェクトでcontestIdteamIdを使用していますが、teamIdの値がcontestTeam.idとして正しく設定されているか確認してください。もしcontestTeamが未定義の場合やidプロパティが存在しない場合、リンクが正しく機能しない可能性があります。

    src/components/Contest/ContestTeamListItem.vue [16-17]

     :to="{
       name: 'ContestTeam',
    -  params: { contestId: contestId, teamId: contestTeam.id }
    +  params: { contestId: contestId, teamId: contestTeam?.id }
     }"
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves the robustness of the code by using optional chaining to prevent potential errors if contestTeam or its id property is undefined. This is a significant improvement for preventing runtime errors.

    8
    Enhancement
    ユーザーIDとして名前ではなく、一意の識別子を使用する。


    :to属性でuserIdmember.nameに設定していますが、ユーザーIDとして名前を使用することは一般的ではありません。通常、ユーザーIDは数値または一意の識別子であるべきです。適切なユーザーIDフィールドを使用してください。

    src/components/ContestTeam/MemberListItem.vue [14]

    -:to="{ name: 'User', params: { userId: member.name } }"
    +:to="{ name: 'User', params: { userId: member.id } }"
     
    Suggestion importance[1-10]: 7

    Why: Using a unique identifier instead of a name for user IDs is a good practice for ensuring data integrity and avoiding potential conflicts. This suggestion enhances the code quality and maintainability.

    7
    Best practice
    ユーザー識別には名前ではなく、一意のIDを使用することを推奨します。


    :to属性でuserIdmember.nameに設定していますが、このフィールドがユーザーの一意の識別子であるか確認してください。もしmemberオブジェクトにidプロパティがある場合は、それを使用することをお勧めします。

    src/components/Group/MemberListItem.vue [15]

    -:to="{ name: 'User', params: { userId: member.name } }"
    +:to="{ name: 'User', params: { userId: member.id || member.name } }"
     
    Suggestion importance[1-10]: 7

    Why: This suggestion promotes best practices by recommending the use of a unique identifier for user IDs, which enhances the reliability and clarity of the code.

    7

    @Pugma
    Copy link
    Collaborator

    Pugma commented Jul 10, 2024

    @ogu-kazemiya パンくずリストのPropsについても部分的に編集してあるので、残りの部分をお願いします!

    Copy link
    Collaborator

    @Pugma Pugma left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    いくつか修正が必要な箇所を挙げているので対応をお願いします:pray:

    あと、コンテストの詳細ページ -> コンテスト一覧 (コンポーネントでいうと ProjectsPage -> ProjectPage) の遷移時に正常に進行できなくなる不具合を発見したんだけど、これに関しては一旦↓のように書いておくと応急処置ができるのでこれも同時にやっておいてほしいです

    <contest-team-list
        :contest-teams="contestDetail.teams"
        :contest-id="contestId ?? 'undefined'"
    />

    <contest-team-list
    :contest-teams="contestDetail.teams"
    :contest-id="contestId"
    />

    src/pages/UserPage.vue Outdated Show resolved Hide resolved
    src/pages/ContestPage.vue Outdated Show resolved Hide resolved
    @Pugma
    Copy link
    Collaborator

    Pugma commented Jul 16, 2024

    返信遅れた & 再度の修正依頼で超絶申し訳ないんですが、

    const useParam = (paramName: string): ComputedRef<string> => {
    const route = useRoute()
    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
    return computed(() => toStringIfArray(route.params[paramName])!)
    }

    の部分を

    const useParam = (paramName: string): ComputedRef<string> => {
      const route = useRoute()
      return computed(() => toStringIfArray(route.params[paramName]) ?? 'undefined')
    }
    

    みたいな感じで ?? 'undefined' をつけておくと、前回つけた ?? 'undefined' の部分を消しても問題なく動く、かつ他の僕が見落としていた箇所も動くようになっていそうです

    @ogu-kazemiya ogu-kazemiya requested a review from Pugma July 20, 2024 16:18
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    router-linkのパラメータを第二引数で指定するようにする
    2 participants