-
Notifications
You must be signed in to change notification settings - Fork 286
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
fyi #35
Comments
I think you're being quite harsh here. He doesn't need perfection to put up a free starter kit. There always going to be better ways at doing things. I'm sure sure which things you are referencing at being called multiple times but are they being cached? |
yeah maybe it came out that way didn't mean for it edited the comment to be less "harsh" but for example: that is all in one page.tsx I don't think it is used anywhere else the parent comp can just do all that and pass it down either through context or through props instead of calling the db in each comp especially the ones that get iterated over or map() is applied will individually call the db , instead maybe an alternative approach could be to call it one time in the parent for the whole group in one call and iterate over the users and append a canManage = user.id === userId or w.e so when it gets passed by the parent to the child you just unpack it. again sorry if it came out harsh |
I'm on my phone right now so hard to look at all the code. As for getCurrentUser. You can't really use middleware for auth as it runs on the edge. So this is probably the best approach. Also the function is cached so reduces the overhead. |
not so much the getCurrentUser that's not the one that's calling the db But just focused on the post/[postId] page.tsx A. export default async function PostPage...
B. async function ReplyAvatar...
C. async function RepliesList...
D. async function ReplyCard...
omitting getCurrentUser since its cached
B.
C.
D.
and the ones that get mapped .map() like the avatar etc run those things multiple times one at a time , instead of just running all of those once and checking the "access" / isAdminOrGroupOwner after you have all the data |
maybe something like this // src/use-cases/posts.ts
export async function getPostDataLoader(
postId: PostId,
userId: UserId // or Session or w.e getCurrentUser returns
) {
const db = getDB(); // I'm using Cloudflare D1 so this will be different
// Fetch post, group, and membership data in a single query
const postData = await db.query.postsTable.findFirst({
where: eq(postsTable.id, postId),
with: {
group: {
with: {
memberships: true,
},
},
replies: true, // Fetch replies with user IDs
},
});
if (!postData) {
throw new PublicError("Post not found");
}
const { group } = postData;
// Check if the group is visible to the user
const isGroupVisible = group.isPublic || group.memberships.some((membership) => membership.userId === userId);
if (!isGroupVisible) {
throw new AuthenticationError();
}
const isGroupOwner = group.userId === userId;
const isAdmin = group.memberships.some((membership) => membership.userId === userId && membership.role === "admin");
const isAdminOrOwner = isAdmin || isGroupOwner;
// Collect user IDs from replies
const userIds = postData.replies.map(reply => reply.userId);
// Fetch profiles for all users who have replied
const profiles = await db.query.profilesTable.findMany({
where: inArray(profilesTable.userId, userIds),
});
// Create a map of userId to profile for easy lookup
const profileMap = new Map(profiles.map(profile => [profile.userId, profile]));
// Map over replies to check if the user can edit each one, format the date, and attach the profile
const repliesWithAccess = postData.replies.map((reply) => {
const canEditReply = isAdminOrOwner || reply.userId === userId;
const profile = profileMap.get(reply.userId);
const profileImageUrl = profile ? getProfileImageFullUrl(profile) : "/profile.png";
return {
...reply,
canEditReply,
formattedDate: formatDate(reply.createdAt),
profile, // Attach the profile
profileImageUrl, // Attach the formatted profile image URL
};
});
return {
post: postData,
group,
isGroupOwner,
isMember: group.memberships.some((membership) => membership.userId === userId),
isAdminOrOwner,
replies: repliesWithAccess,
};
}
|
actually I just seen drizzle 0.34.0 came out and theres alot of things here that can improve things if your interested, things like this specifically |
I appreciate the feedback. The idea of the starter kit is I assume people will not use the existing code (because a group finding application is very niche). The important features include the authentication, project structure, rate limiting, etc. But yes, the most optimal way to do that page would be to do a top level query with a lot of joins instead of letting sub components query the db as they render. |
your [...posts] section is riddled with issues.
you call the same same DB fn in every component"" instead of just batching it or at least one call instead of multiple "use-cases" that are doing the same thing...
The text was updated successfully, but these errors were encountered: