Skip to content

♻️ Refactor project layouts by introducing isolated layout groups #1514

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

Merged
merged 1 commit into from
Apr 28, 2025

Conversation

junkisai
Copy link
Member

@junkisai junkisai commented Apr 25, 2025

Why is this change needed?

This PR addresses the issue mentioned in the comments of the previous PR.

ref. #1508 (comment)

This pull request refactors the layout handling for project-related routes by introducing isolated route groups using (with-project) and (with-project-and-branch) directories.

The goal is to decouple layout responsibilities per path depth and resolve issues caused by layout inheritance and central path parsing.

🐛 Context & Problem

Previously, app/layout.tsx was responsible for handling logic related to projectId and branchOrCommit paths.
This involved parsing the path manually via next/headers, introducing unnecessary coupling and runtime dependency on headers.

Moreover, using multiple layout.tsx files without isolation caused layout inheritance, which led to duplicated UI elements (e.g., multiple GlobalNav or AppBar components being rendered across nested layouts).

✅ Solution

To address the above:

What would you like reviewers to focus on?

Testing Verification

Before After
2025-04-25.17.19.41.mov
2025-04-25.20.17.43.mov

What was done

🤖 Generated by PR Agent at 55757c8

  • Refactored project and organization layouts using isolated route groups:
    • Introduced (with-project) and (with-project-and-branch) directories for layout isolation.
    • Removed path parsing logic from global layout; layouts now receive params directly.
    • Prevented unwanted layout inheritance and UI duplication.
  • Added and improved settings and project tab navigation:
    • Centralized tab constants and schemas for both project and settings tabs.
    • Implemented tabbed navigation with correct highlighting and routing.
  • Implemented and styled new organization settings pages:
    • Added general, members, billing, and projects settings pages with modular CSS.
    • Added organization deletion flow with confirmation modal and client-side feedback.
  • Added new project-level pages and utilities:
    • Implemented docs, schema, migrations, rule, and settings pages for projects.
    • Added server utilities for fetching project repositories and documentation file paths.
    • Added corresponding Vitest test files for new data utilities.
  • Cleaned up legacy code and improved modularity:
    • Removed obsolete path extraction utility.
    • Exported and modularized layout and header components.

Detailed Changes

Relevant files
Enhancement
56 files
index.ts
Export SettingsHeader component for modular usage               
[link]   
constants.ts
Add centralized settings tab constants and schema               
[link]   
getDocumentContent.ts
Add utility to fetch document content from repository       
[link]   
getProjectRepository.ts
Add utility to fetch project repository info                         
[link]   
getDocFilePaths.ts
Add utility to fetch documentation file paths                       
[link]   
extractProjectPathParts.ts
Remove obsolete project path extraction utility                   
+0/-34   
index.ts
Export ProjectHeader component for modular usage                 
[link]   
projectConstants.ts
Add centralized project tab constants and schema                 
[link]   
index.ts
Export ProjectLayout component for modular usage                 
+1/-0     
SettingsHeader.module.css
Add and style SettingsHeader component CSS                             
[link]   
DeleteConfirmationModal.module.css
Add and style DeleteConfirmationModal component CSS           
[link]   
DeleteOrganizationButtonClient.module.css
Add and style DeleteOrganizationButtonClient component CSS
[link]   
GeneralPage.module.css
Add and style GeneralPage component CSS                                   
[link]   
layout.module.css
Add and style organization settings layout CSS                     
[link]   
DocsDetailPage.module.css
Add and style DocsDetailPage component CSS                             
[link]   
ProjectHeader.module.css
Add and style ProjectHeader component CSS                               
[link]   
ProjectLayout.module.css
Add and style ProjectLayout component CSS                               
+11/-0   
page.tsx
Implement invitation token detail page with validation     
[link]   
layout.tsx
Refactor root layout to use CommonLayout directly               
[link]   
page.tsx
Implement new organization creation page                                 
[link]   
page.tsx
Implement organizations list page                                               
[link]   
page.tsx
Refactor root page to redirect based on user/org/project state
[link]   
page.tsx
Implement new project creation page with installations     
[link]   
page.tsx
Implement projects list page with organization context     
[link]   
page.tsx
Implement placeholder billing settings page                           
[link]   
SettingsHeader.tsx
Implement SettingsHeader tab navigation component               
[link]   
DeleteConfirmationModal.tsx
Implement DeleteConfirmationModal for organization deletion
[link]   
DeleteOrganizationButtonClient.tsx
Implement client-side delete organization button and modal
[link]   
GeneralPage.tsx
Implement server-side organization general settings page 
[link]   
GeneralPageClient.tsx
Implement client-side general settings form and deletion logic
[link]   
page.tsx
Implement general settings page with organization context
[link]   
layout.tsx
Implement tabbed organization settings layout with header
[link]   
page.tsx
Implement organization members settings page                         
[link]   
page.tsx
Implement placeholder projects settings page                         
[link]   
page.tsx
Implement project branches list page                                         
[link]   
layout.tsx
Implement isolated project layout for single-depth project routes
+24/-0   
page.tsx
Redirect project root to default branch detail page           
[link]   
RulePage.tsx
Implement placeholder project rule page                                   
[link]   
page.tsx
Implement project rule page with param validation               
[link]   
SettingsPage.tsx
Implement placeholder project settings page                           
[link]   
page.tsx
Implement project settings page with param validation       
[link]   
DocsListPage.tsx
Implement documentation files list page for project/branch
[link]   
DocsDetailPage.tsx
Implement documentation detail page with content fetch     
[link]   
page.tsx
Implement documentation detail page route with param validation
[link]   
page.tsx
Implement documentation list page route with param validation
[link]   
page.tsx
Implement knowledge suggestion detail page route                 
[link]   
page.tsx
Implement knowledge suggestions list page route                   
[link]   
layout.tsx
Implement isolated layout for project/branch routes           
+29/-0   
page.tsx
Implement migration detail page route with param validation
[link]   
page.tsx
Implement migrations list page route with param validation
[link]   
page.tsx
Implement branch detail page route with param validation 
[link]   
page.tsx
Implement schema file detail page route with param validation
[link]   
AppBar.tsx
Update AppBar props to accept optional project/branch params
+2/-2     
CommonLayout.tsx
Refactor CommonLayout to accept projectId/branchOrCommit as props
+7/-7     
ProjectHeader.tsx
Implement ProjectHeader with tab navigation and suggestion badge
[link]   
ProjectLayout.tsx
Refactor ProjectLayout for isolated usage and param-based navigation
+16/-32 
Tests
2 files
getProjectRepository.test.ts
Add tests for getProjectRepository utility                             
[link]   
getDocFilePaths.test.ts
Add tests for getDocFilePaths utility                                       
[link]   
Bug fix
1 files
getOriginalDocumentContent.ts
Update import path for getProjectRepository utility           
+1/-1     
Formatting
1 files
layout.module.css
Remove unused heading style from branch layout CSS             
+0/-7     

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.
  • Copy link

    changeset-bot bot commented Apr 25, 2025

    ⚠️ No Changeset found

    Latest commit: 55757c8

    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

    Copy link

    vercel bot commented Apr 25, 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 Apr 25, 2025 11:24am
    liam-erd-sample ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 25, 2025 11:24am
    1 Skipped Deployment
    Name Status Preview Comments Updated (UTC)
    liam-docs ⬜️ Ignored (Inspect) Apr 25, 2025 11:24am

    @junkisai junkisai marked this pull request as ready for review April 25, 2025 11:45
    @junkisai junkisai requested a review from a team as a code owner April 25, 2025 11:45
    @junkisai junkisai requested review from hoshinotsuyoshi, FunamaYukina, MH4GF and NoritakaIkeda and removed request for a team April 25, 2025 11:45
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    The error handling in getDocumentContent could be improved. It catches all errors and returns null, which might hide specific issues like network problems or permission errors. Consider adding more specific error handling or propagating certain errors.

    } catch (error) {
      console.error('Error fetching document content:', error)
      return null
    }
    Default Branch Handling

    The component uses 'main' as a hardcoded default branch value with a TODO comment. This might cause issues for repositories using different default branch names (like 'master' or 'develop'). Consider fetching the actual default branch from the repository data.

      branchOrCommit = 'main', // TODO: get default branch from API(using currentOrganization)
    }) => {
    
    State Management

    The component doesn't handle the case where the deletion is in progress but the user navigates away. Consider adding a mechanism to persist the deletion state or prevent navigation during deletion.

    const handleConfirmDelete = () => {
      // Check if confirmation text matches organization name
      if (confirmText === organizationName) {
        // Create FormData object
        const formData = new FormData()
        formData.append('organizationId', organizationId)
        formData.append('confirmText', confirmText)
    
        // Use startTransition to wrap the action call
        startTransition(() => {
          deleteAction(formData)
        })
    
        // Close the modal
        handleCloseModal()
      }
    }
    

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Check array before accessing element

    Check if project_repository_mappings array exists and has elements before
    accessing the first element. The current code assumes
    project_repository_mappings[0] exists, which could cause a runtime error if the
    array is empty.

    frontend/apps/app/app/(app)/app/(with-project-and-branch)/projects/[projectId]/ref/[branchOrCommit]/docs/[...docFilePath]/getProjectRepository.ts [34-43]

    -const repository =
    -  project?.project_repository_mappings[0]?.github_repositories
    +const mappings = project?.project_repository_mappings
    +if (!mappings || mappings.length === 0) {
    +  console.error('No repository mappings found for project')
    +  return null
    +}
    +
    +const repository = mappings[0]?.github_repositories
     if (
       !repository?.github_installation_identifier ||
       !repository.owner ||
       !repository.name
     ) {
       console.error('Repository information not found')
       return null
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: This suggestion addresses a potential runtime error by checking if the project_repository_mappings array exists and has elements before accessing the first element. This is a critical defensive programming practice that prevents null/undefined reference errors.

    Medium
    Add null check for API response

    Add error handling for the case where fileData or fileData.content is undefined.
    The current code assumes fileData.content always exists, which could lead to
    runtime errors if the API call fails or returns unexpected data.

    frontend/apps/app/app/(app)/app/(with-project-and-branch)/projects/[projectId]/ref/[branchOrCommit]/docs/[...docFilePath]/getDocumentContent.ts [26-33]

     const fileData = await getFileContent(
       repositoryFullName,
       filePath,
       branchOrCommit,
       repository.github_installation_identifier,
     )
     
    +if (!fileData || !fileData.content) {
    +  console.error('File content not found or empty')
    +  return null
    +}
    +
     return fileData.content
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: This suggestion adds important error handling for the case where the API response or its content property is undefined, preventing potential runtime errors. This is a good defensive programming practice that improves the robustness of the code.

    Medium
    General
    Ensure consistent parameter handling

    Add 'await' before 'params' to be consistent with other route handlers. All
    other similar files in this PR use 'await params' when parsing parameters.

    frontend/apps/app/app/(app)/app/(with-project-and-branch)/projects/[projectId]/ref/[branchOrCommit]/page.tsx [13]

    -const parsedParams = v.safeParse(paramsSchema, params)
    +const parsedParams = v.safeParse(paramsSchema, await params)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: This suggestion improves consistency across the codebase by adding 'await' before 'params'. All other similar route handlers in this PR use 'await params' when parsing parameters, making this change important for maintaining a consistent approach to parameter handling.

    Medium
    Fix inconsistent error handling

    Replace 'throw notFound()' with 'return notFound()' to be consistent with other
    route handlers. All other similar files in this PR use 'return notFound()'
    pattern.

    frontend/apps/app/app/(app)/app/(with-project-and-branch)/projects/[projectId]/ref/[branchOrCommit]/docs/[...docFilePath]/page.tsx [14]

    -if (!parsedParams.success) throw notFound()
    +if (!parsedParams.success) return notFound()
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: This suggestion improves consistency in error handling by replacing 'throw notFound()' with 'return notFound()'. All other similar files in this PR use the 'return notFound()' pattern, making this change important for maintaining consistent error handling throughout the codebase.

    Medium
    Learned
    best practice
    Validate function parameters

    Add input validation at the beginning of the function to check if all required
    parameters are provided and have the expected format. This will prevent
    potential runtime errors when trying to use undefined or malformed values.

    frontend/apps/app/app/(app)/app/(with-project-and-branch)/projects/[projectId]/ref/[branchOrCommit]/docs/[...docFilePath]/getDocumentContent.ts [10-37]

     export const getDocumentContent = async ({
       projectId,
       branchOrCommit,
       docFilePath,
     }: DocumentContentParams): Promise<string | null> => {
       try {
    +    if (!projectId || !branchOrCommit || !docFilePath || docFilePath.length === 0) {
    +      console.error('Missing required parameters for document content retrieval')
    +      return null
    +    }
    +    
         const projectRepo = await getProjectRepository(projectId)
         if (!projectRepo) {
           console.error('Repository information not found')
           return null
         }
     
         const { repository } = projectRepo
         const repositoryFullName = `${repository.owner}/${repository.name}`
         const filePath = docFilePath.join('/')
     
         const fileData = await getFileContent(
           repositoryFullName,
           filePath,
           branchOrCommit,
           repository.github_installation_identifier,
         )
     
         return fileData.content
       } catch (error) {
         console.error('Error fetching document content:', error)
         return null
       }
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 6
    Low
    • More

    Base automatically changed from feat/organize-project-id-page-path to main April 25, 2025 12:39
    Copy link
    Member

    @hoshinotsuyoshi hoshinotsuyoshi left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    import { getProjectRepository } from '../../../app/(app)/app/projects/[projectId]/ref/[branchOrCommit]/docs/[...docFilePath]/getProjectRepository'
    import { getProjectRepository } from '../../../app/(app)/app/(with-project-and-branch)/projects/[projectId]/ref/[branchOrCommit]/docs/[...docFilePath]/getProjectRepository'
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Not directly related to this PR, but the dependency direction here is a bit unusual in this project?

    @hoshinotsuyoshi hoshinotsuyoshi added this pull request to the merge queue Apr 28, 2025
    Merged via the queue into main with commit dd93e28 Apr 28, 2025
    18 of 19 checks passed
    @hoshinotsuyoshi hoshinotsuyoshi deleted the refactor/separate-project-layouts branch April 28, 2025 00:16
    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.

    2 participants