-
Notifications
You must be signed in to change notification settings - Fork 939
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
Rewrite Person Detail Page to Server-side Component #609
Conversation
…shubham/for-903-rewrite-person-detail-page
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
apps/web/app/(app)/environments/[environmentId]/people/[personId]/PersonDetails.tsx
Outdated
Show resolved
Hide resolved
…for-903-rewrite-person-detail-page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
I just took a first look and continue the review next week. Here are a first few comments :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add
export const revalidate = REVALIDATION_INTERVAL;
import { REVALIDATION_INTERVAL } from "@formbricks/lib/constants";
on top of the file to make sure nextjs invalidates cache once in a while to show the new responses/displays/...
when not set the only option to get new data is to hard refresh (F5) the page.
displays = displays ?? []; | ||
|
||
const surveyIds = responses?.map((response) => response.surveyId) || []; | ||
const surveys = await Promise.all(surveyIds.map((surveyIdx) => getSurvey(surveyIdx))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for the database it could be faster to call getSurveys()
because it can list all surveys with just one call instead of making multiple database calls for the surveys needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, added a check to only fetch the surveys in the environment if the user atleast has one response, this would help reduce the extra DB calls to fetch the surveys even when a user has no responses!
packages/types/v1/responses.ts
Outdated
@@ -88,3 +89,17 @@ export const ZResponseUpdateInput = z.object({ | |||
}); | |||
|
|||
export type TResponseUpdateInput = z.infer<typeof ZResponseUpdateInput>; | |||
|
|||
export const ZResponseWithSurveyData = z.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to have a type for the response card. We can maybe wait with this until we have the redesigned response card (PR in the making by contributor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we can wait on this prolly haha 🤞🏼
import { TResponseWithSurveyData } from "@formbricks/types/v1/responses"; | ||
|
||
export default async function PersonPage({ params }) { | ||
let [personWithAttributes, displays, sessionsWithActions, responses] = await Promise.all([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to fetch all these in page.tsx. You can also fetch them in the server-component you need it. Next.js takes care of caching and deduping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! took some time for this, let me know what you think
…github.com/formbricks/formbricks into shubham/for-903-rewrite-person-detail-page
…for-903-rewrite-person-detail-page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShubhamPalriwala The overall structure is great but there were a few things I thought could be simplified or renamed to make it more consistent. For example I moved the whole types and services needed for the activityFeed into a custom prisma call inside the activity service to not make the other services more complicated for an activity feature that will probably need changing in the near future.
You can review the my commits to see what changed 😊
I will merge the changes into main now 💪
* feat: migration /[personId] page to server side * feat: decouple components in person page * fix: ZDisplaysWithSurveyName now extends the ZDisplay type * feat: drop custom service and use existing service for survey and response * run pnpm format * shift data fetching to component level but still server side * rename event to action * move special person services to activity service * remove activityFeedItem type in ActivityFeed * simplify TResponseWithSurvey --------- Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>
* feat: migration /[personId] page to server side * feat: decouple components in person page * fix: ZDisplaysWithSurveyName now extends the ZDisplay type * feat: drop custom service and use existing service for survey and response * run pnpm format * shift data fetching to component level but still server side * rename event to action * move special person services to activity service * remove activityFeedItem type in ActivityFeed * simplify TResponseWithSurvey --------- Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>
* feat: migration /[personId] page to server side * feat: decouple components in person page * fix: ZDisplaysWithSurveyName now extends the ZDisplay type * feat: drop custom service and use existing service for survey and response * run pnpm format * shift data fetching to component level but still server side * rename event to action * move special person services to activity service * remove activityFeedItem type in ActivityFeed * simplify TResponseWithSurvey --------- Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>
What does this PR do?
Migrates the
/environments/[environmentId]/people/[personId]/
page to server side component leveraging:Type of change
Checklist
pnpm build
console.logs
git pull origin main