Skip to content

Conversation

@sasamuku
Copy link
Member

@sasamuku sasamuku commented Mar 21, 2025

Issue

  • resolve:

Why is this change needed?

What would you like reviewers to focus on?

Testing Verification

What was done

🤖 Generated by PR Agent at a0fead8

  • Updated authentication callback URL to use VERCEL_BRANCH_URL.
  • Replaced fallback from VERCEL_URL to VERCEL_BRANCH_URL.
  • Ensured consistent URL formatting for callback construction.

Detailed Changes

Relevant files
Enhancement
actions.ts
Refactor callback URL to use `VERCEL_BRANCH_URL`                 

frontend/apps/app/app/(app)/app/login/actions.ts

  • Replaced VERCEL_URL with VERCEL_BRANCH_URL for callback URL.
  • Adjusted fallback logic for environment variable usage.
  • Maintained consistent URL formatting for callback construction.
  • +2/-2     

    Additional Notes


    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @changeset-bot
    Copy link

    changeset-bot bot commented Mar 21, 2025

    ⚠️ No Changeset found

    Latest commit: a0fead8

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    @vercel
    Copy link

    vercel bot commented Mar 21, 2025

    The latest updates on your projects. Learn more about Vercel for Git ↗︎

    Name Status Preview Comments Updated (UTC)
    liam-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2025 7:31am
    liam-erd-sample ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2025 7:31am
    4 Skipped Deployments
    Name Status Preview Comments Updated (UTC)
    liam-docs ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2025 7:31am
    test-liam-app ⬜️ Ignored (Inspect) Mar 21, 2025 7:31am
    test-liam-docs ⬜️ Ignored (Inspect) Mar 21, 2025 7:31am
    test-liam-erd-sample ⬜️ Ignored (Inspect) Mar 21, 2025 7:31am

    @supabase
    Copy link

    supabase bot commented Mar 21, 2025

    Updates to Preview Branch (refactor_callback_url_to_use_vercel_branch_url) ↗︎

    Deployments Status Updated
    Database Fri, 21 Mar 2025 07:04:42 UTC
    Services Fri, 21 Mar 2025 07:04:42 UTC
    APIs Fri, 21 Mar 2025 07:04:42 UTC

    Tasks are run on every commit but only new migration files are pushed.
    Close and reopen this PR if you want to apply changes from existing seed or migration files.

    Tasks Status Updated
    Configurations Fri, 21 Mar 2025 07:04:47 UTC
    Migrations Fri, 21 Mar 2025 07:04:47 UTC
    Seeding Fri, 21 Mar 2025 07:04:47 UTC
    Edge Functions Fri, 21 Mar 2025 07:04:47 UTC

    View logs for this Workflow Run ↗︎.
    Learn more about Supabase for Git ↗︎.

    @liam-migration-preview
    Copy link

    The schema changes you provided involve a modification to the authentication callback URL generation logic in the frontend/apps/app/app/(app)/app/login/actions.ts file. Specifically, the changes replace the environmental variable VERCEL_URL with VERCEL_BRANCH_URL in the getAuthCallbackUrl function. Here's a detailed review of this change:

    1. Purpose of Change: The primary purpose of this change seems to be to enhance the robustness of the URL generation logic by using VERCEL_BRANCH_URL. This adjustment is likely aimed at ensuring that the correct URL is used when the application is deployed on different branches or environments in Vercel.

    2. Environmental Variable Adjustment:

      • Previous Implementation: The previous implementation relied on VERCEL_URL, which typically points to the production environment URL. This could potentially lead to issues when deploying to preview branches, as the URL might not reflect the correct context.
      • New Implementation: By switching to VERCEL_BRANCH_URL, the function now dynamically adjusts to the specific branch context, which is beneficial for testing and staging environments. This enhances the flexibility and accuracy of redirect URLs during the OAuth process, ensuring users are directed back to the correct application state post-authentication.
    3. Impact on User Experience: The change is likely to improve user experience during the authentication process. Users logging in via GitHub are redirected to the appropriate branch URL, avoiding potential confusion or errors if they were redirected to a main production environment URL that does not correspond to their testing or feature branch.

    4. Code Quality Considerations:

      • The change is succinct and well-implemented, adhering to the existing code structure and style. The modification is clear and easy to understand, which is vital for future maintainability.
      • The use of template literals for URL construction is appropriate and helps to keep the code clean and readable.
    5. Error Handling: The error handling in the login function remains unchanged. It correctly redirects users to an error page if there is an issue with the authentication process. This is good practice, as it provides clear feedback to the user in case of failure.

    6. Next Steps:

      • It would be prudent to test the changes in a staging environment to ensure that the new URL generation logic behaves as expected across various deployment scenarios, particularly in branch and production contexts.
      • Consider documenting this change in your codebase to provide context for future developers regarding the rationale behind choosing VERCEL_BRANCH_URL over VERCEL_URL.
    7. Conclusion: Overall, the changes made to the schema are a positive enhancement that reflects a thoughtful approach to handling environment-specific configurations in the authentication flow. This adjustment should lead to a smoother and more reliable user experience, particularly in environments where multiple branches may be utilized for development and testing.

    Migration URL: https://liam-app-git-staging-route-06-core.vercel.app/app/projects/4/migrations/18

    ? `https://${process.env.SITE_URL}`
    : process.env.VERCEL_URL
    ? `https://${process.env.VERCEL_URL}`
    : process.env.VERCEL_BRANCH_URL
    Copy link
    Member Author

    Choose a reason for hiding this comment

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

    📝
    VERCEL_BRANCH_URL is used for Site URL by Supabase Branching.
    Use VERCEL_BRANCH_URL to match the Site URL to the redirect URL during GitHub authentication.

    貼り付けた画像_2025_03_21_16_32

    @sasamuku sasamuku marked this pull request as ready for review March 21, 2025 07:34
    @sasamuku sasamuku requested a review from a team as a code owner March 21, 2025 07:34
    @sasamuku sasamuku requested review from FunamaYukina, MH4GF, NoritakaIkeda, hoshinotsuyoshi and junkisai and removed request for a team March 21, 2025 07:34
    @qodo-free-for-open-source-projects
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Environment Variable Validation

    The code assumes VERCEL_BRANCH_URL will be properly formatted. Consider adding validation to ensure the URL is properly formed before using it in the callback URL construction.

    : process.env.VERCEL_BRANCH_URL
      ? `https://${process.env.VERCEL_BRANCH_URL}`

    @qodo-free-for-open-source-projects
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Learned
    best practice
    Add validation for environment variables before using them to construct URLs to prevent malformed URLs and potential security issues

    The code doesn't validate that the environment variables SITE_URL and
    VERCEL_BRANCH_URL are properly formatted before using them to construct URLs.
    This could lead to malformed URLs if these variables contain unexpected values.
    Add validation to ensure these environment variables are properly formatted
    before using them.

    frontend/apps/app/app/(app)/app/login/actions.ts [13-17]

    -let url = process.env.SITE_URL
    -  ? `https://${process.env.SITE_URL}`
    -  : process.env.VERCEL_BRANCH_URL
    -    ? `https://${process.env.VERCEL_BRANCH_URL}`
    -    : 'http://localhost:3001/'
    +// Helper function to validate URL components
    +const isValidUrlComponent = (url: string | undefined): boolean => {
    +  return !!url && !url.includes('://') && !url.includes('/');
    +};
     
    +let url;
    +if (process.env.SITE_URL && isValidUrlComponent(process.env.SITE_URL)) {
    +  url = `https://${process.env.SITE_URL}`;
    +} else if (process.env.VERCEL_BRANCH_URL && isValidUrlComponent(process.env.VERCEL_BRANCH_URL)) {
    +  url = `https://${process.env.VERCEL_BRANCH_URL}`;
    +} else {
    +  url = 'http://localhost:3001/';
    +}
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    Low
    General
    Standardize URL format

    The fallback URL 'http://localhost:3001/' has a trailing slash while the other
    URL constructions don't include it initially. This inconsistency is addressed
    later with the url.endsWith('/') check, but it would be cleaner to standardize
    the format across all URL sources.

    frontend/apps/app/app/(app)/app/login/actions.ts [15-17]

     : process.env.VERCEL_BRANCH_URL
       ? `https://${process.env.VERCEL_BRANCH_URL}`
    -  : 'http://localhost:3001/'
    +  : 'http://localhost:3001'
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly identifies an inconsistency in URL formatting where the localhost fallback includes a trailing slash while the other URLs don't. Removing the trailing slash standardizes the format and makes the subsequent endsWith check more meaningful. This is a minor improvement for code consistency.

    Low
    • More

    Copy link
    Contributor

    @MH4GF MH4GF left a comment

    Choose a reason for hiding this comment

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

    Thanks 👍🏻

    @MH4GF MH4GF added this pull request to the merge queue Mar 21, 2025
    Merged via the queue into main with commit e37afcb Mar 21, 2025
    25 checks passed
    @MH4GF MH4GF deleted the refactor_callback_url_to_use_vercel_branch_url branch March 21, 2025 07:41
    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.

    3 participants