Skip to content

♻️ Refactor AppBar: Convert to Server Components and remove use client directives #1380

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

♻️ Refactor AppBar: Convert to Server Components and remove use client directives #1380

wants to merge 9 commits into from

Conversation

devin-ai-integration[bot]
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Apr 17, 2025 •
edited
Loading

What changes does this PR introduce?

Refactoring:

  • Remove unnecessary 'use client' directives from Avatar/UserAvatarIcon
  • Simplify AppBar architecture by removing wrapper components and handling data fetching directly
  • Define custom getProject function in AppBar component

PR Checklist

  • Added tests related to the changes
  • Updated documentation (if necessary)
  • Added Changeset for incompatible changes
  • Verified that CI passes

Screenshots

None

Additional Information

Refactoring based on PR #1361:

Link to Devin run: https://app.devin.ai/sessions/b9ad63baf00b444eb57ebad3c4e6244d
Requested by: hirotaka.miyagi@route06.co.jp

@devin-ai-integration @MH4GF
♻️ Refactor AppBar: Remove use client directives and convert to Serve…
7cde4a4 
…r Components

Co-Authored-By: hirotaka.miyagi@route06.co.jp <hirotaka.miyagi@route06.co.jp>
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Apr 17, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: 45ad665

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 Vercel
Copy link

vercel bot commented Apr 17, 2025 •
edited
Loading

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

@devin-ai-integration Devin.ai Integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@supabase Supabase
Copy link

supabase bot commented Apr 17, 2025 •
edited
Loading

Updates to Preview Branch (devin/1744864520-appbar-refactoring) ↗︎

Deployments Status Updated
Database ✅ Thu, 17 Apr 2025 07:10:02 UTC
Services ✅ Thu, 17 Apr 2025 07:10:02 UTC
APIs ✅ Thu, 17 Apr 2025 07:10:02 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 ✅ Thu, 17 Apr 2025 07:10:02 UTC
Migrations ✅ Thu, 17 Apr 2025 07:10:02 UTC
Seeding ✅ Thu, 17 Apr 2025 07:10:02 UTC
Edge Functions ✅ Thu, 17 Apr 2025 07:10:02 UTC

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

@devin-ai-integration devin-ai-integration bot marked this pull request as ready for review April 17, 2025 04:55
@devin-ai-integration devin-ai-integration bot requested a review from a team as a code owner April 17, 2025 04:55
@devin-ai-integration devin-ai-integration bot requested review from hoshinotsuyoshi, FunamaYukina, junkisai, MH4GF and NoritakaIkeda and removed request for a team April 17, 2025 04:55
@MH4GF
Copy link
Member

MH4GF commented Apr 17, 2025

Please write description in English

@MH4GF
Copy link
Member

MH4GF commented Apr 17, 2025

Please convert to draft

@devin-ai-integration devin-ai-integration bot marked this pull request as draft April 17, 2025 05:20
@devin-ai-integration @MH4GF
♻️ Simplify AppBar: Remove wrapper components and handle data fetchin…
ed4863f 
…g directly

Co-Authored-By: hirotaka.miyagi@route06.co.jp <hirotaka.miyagi@route06.co.jp>
@devin-ai-integration @MH4GF
🔧 Fix linting issues in AppBar and CommonLayout
5607929 
Co-Authored-By: hirotaka.miyagi@route06.co.jp <hirotaka.miyagi@route06.co.jp>
MH4GF
MH4GF reviewed Apr 17, 2025
View reviewed changes
frontend/apps/app/components/AppBar/AppBar.tsx Outdated
Comment on lines 43 to 46
let project: Tables<'Project'> | null = null
if (projectId) {
project = await getProject(projectId)
}
Copy link
Member

Choose a reason for hiding this comment

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

null is not necessary

@devin-ai-integration @MH4GF
🔧 Fix type in AppBar to handle null return from getProject
b16d009 
Co-Authored-By: hirotaka.miyagi@route06.co.jp <hirotaka.miyagi@route06.co.jp>
@devin-ai-integration devin-ai-integration bot marked this pull request as ready for review April 17, 2025 06:16
MH4GF
MH4GF reviewed Apr 17, 2025
View reviewed changes
frontend/apps/app/components/AppBar/AppBar.tsx Outdated
Comment on lines 43 to 46
let project: Tables<'Project'> | null | undefined
if (projectId) {
project = await getProject(projectId)
}
Copy link
Member

Choose a reason for hiding this comment

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

Please delete null

MH4GF
MH4GF reviewed Apr 17, 2025
View reviewed changes
frontend/apps/app/components/AppBar/AppBar.tsx Outdated
@@ -40,8 +37,14 @@ export const AppBar = ({
avatarInitial = 'L',
avatarColor = 'var(--avatar-background)',
minimal = false,
children,
Copy link
Member

Choose a reason for hiding this comment

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

children is not passed. please delete it

MH4GF
MH4GF reviewed Apr 17, 2025
View reviewed changes
frontend/apps/app/components/AppBar/AppBar.tsx Outdated
@@ -1,5 +1,4 @@
'use client'

import { getProject } from '@/features/projects/pages/ProjectDetailPage/getProject'
Copy link
Member

Choose a reason for hiding this comment

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

Since the columns to select are different, the getProject function for this component should be defined here

@devin-ai-integration @MH4GF
♻️ Define custom getProject function in AppBar and remove children props
10a56e7 
Co-Authored-By: hirotaka.miyagi@route06.co.jp <hirotaka.miyagi@route06.co.jp>
@MH4GF MH4GF marked this pull request as draft April 17, 2025 06:46
@devin-ai-integration @MH4GF
✨ Implement pathname extraction in server components using middleware
51d227a 
Co-Authored-By: hirotaka.miyagi@route06.co.jp <hirotaka.miyagi@route06.co.jp>
@devin-ai-integration @MH4GF
🐛 Fix TypeScript errors with headers() function in server components
45ad665 
Co-Authored-By: hirotaka.miyagi@route06.co.jp <hirotaka.miyagi@route06.co.jp>
@devin-ai-integration devin-ai-integration bot marked this pull request as ready for review April 17, 2025 06:59
MH4GF
MH4GF reviewed Apr 17, 2025
View reviewed changes
frontend/apps/app/components/AppBar/AppBar.tsx Outdated
Comment on lines 63 to 70
try {
const headersList = headers() as unknown as {
get(name: string): string | null
}
pathname = headersList.get('x-pathname') || ''
} catch (error) {
console.error('Error getting pathname from headers:', error)
}
Copy link
Member

Choose a reason for hiding this comment

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

Please use getPathname

MH4GF
MH4GF reviewed Apr 17, 2025
View reviewed changes
frontend/apps/app/components/AppBar/AppBar.tsx Outdated
console.error('Error getting pathname from headers:', error)
}

let extractedProjectId = projectId
Copy link
Member

Choose a reason for hiding this comment

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

projectId props doesn't need

@devin-ai-integration @MH4GF
♻️ Remove try-catch and type assertions from headers() usage
a8181e8 
Co-Authored-By: hirotaka.miyagi@route06.co.jp <hirotaka.miyagi@route06.co.jp>
@devin-ai-integration @MH4GF
🔧 Add generated CSS module type definition
8e8ba4e 
Co-Authored-By: hirotaka.miyagi@route06.co.jp <hirotaka.miyagi@route06.co.jp>
@MH4GF MH4GF marked this pull request as draft April 18, 2025 04:07
@MH4GF MH4GF closed this Apr 24, 2025
@MH4GF MH4GF deleted the devin/1744864520-appbar-refactoring branch April 24, 2025 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MH4GF MH4GF MH4GF left review comments

@hoshinotsuyoshi hoshinotsuyoshi Awaiting requested review from hoshinotsuyoshi hoshinotsuyoshi is a code owner automatically assigned from liam-hq/liam-dev

@FunamaYukina FunamaYukina Awaiting requested review from FunamaYukina FunamaYukina is a code owner automatically assigned from liam-hq/liam-dev

@junkisai junkisai Awaiting requested review from junkisai junkisai is a code owner automatically assigned from liam-hq/liam-dev

@NoritakaIkeda NoritakaIkeda Awaiting requested review from NoritakaIkeda NoritakaIkeda is a code owner automatically assigned from liam-hq/liam-dev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@MH4GF