Use branded types for identifiers everywhere #1149
Conversation
WalkthroughThe PR standardizes ID handling across web, backend, and domain layers by replacing plain strings with branded domain ID types (OrganisationId, SpaceIdOrOrganisationId, UserId, VideoId, CommentId). It updates function signatures, schemas, queries, and component props accordingly. It also adds SpaceIdOrOrganisationId, a small Option utility, and refines comment API flows. Changes
Sequence Diagram(s)sequenceDiagram
actor U as User
participant C as Client (page/app)
participant API as POST /api/video/comment
participant DB as Database
participant EM as Email Service
U->>C: Submit comment (type, content, videoId, [parentCommentId])
C->>API: Request with domain-wrapped IDs
API->>API: Validate required fields
alt invalid
API-->>C: 400 Bad Request
else valid
API->>DB: Insert comment (CommentId, VideoId, UserId)
opt type == "text" and user != anonymous
API-->>EM: Fire-and-forget notify new-comment
end
API-->>C: 200 Created (comment)
end
sequenceDiagram
actor U as User
participant C as Client
participant API as DELETE /api/video/comment
participant DB as Database
U->>C: Delete comment (commentId)
C->>API: Request with commentId
API->>API: Parse Option<CommentId>
alt missing
API-->>C: 400 Missing data
else present
API->>DB: Select comment by id and authorId
alt not found/unauthorized
API-->>C: 404 Not Found
else ok
API->>DB: Delete where id = commentId OR parentCommentId = commentId
API-->>C: 200 Deleted
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
apps/web/app/(org)/dashboard/settings/organization/components/OrgName.tsx (1)
16-32: Add null check for activeOrganization to prevent runtime error.The non-null assertion (
!) on line 22 assumesactiveOrganizationis always defined, but this is inconsistent with the optional chaining used elsewhere (lines 12, 63). IfactiveOrganizationisnullorundefined, a runtime error will occur when accessing.organization.id.Apply this diff to add a guard:
const handleOrgNameChange = async () => { try { - if (!orgName) return; + if (!orgName || !activeOrganization) return; setSaveLoading(true); await updateOrganizationDetails({ organizationName: orgName, organizationId: activeOrganization!.organization.id, });apps/web/app/api/desktop/[...route]/video.ts (1)
131-160: Type consistency violation between database IDs and branded types.Lines 131-160 mix plain string IDs from the database with branded
Organisation.OrganisationIdtypes, which undermines type safety:
- Line 131:
userOrgIdsis created from database results that return plain string IDs- Line 136, 141: Comparing branded
orgId/user.defaultOrgIdwithuserOrgIdsarray of plain strings usingincludes()- Lines 145, 154, 159: Assigning plain string IDs directly to
videoOrgIdwhich is typed asOrganisation.OrganisationIdThe database query at lines 110-130 returns plain strings from
organizations.id, but these are used without transformation to branded types.Transform database IDs to branded types immediately after retrieval:
const userOrganizations = await db() .select({ id: organizations.id, name: organizations.name, }) .from(organizations) .leftJoin( organizationMembers, eq(organizations.id, organizationMembers.organizationId), ) .where( or( eq(organizations.ownerId, user.id), eq(organizationMembers.userId, user.id), ), ) .groupBy(organizations.id, organizations.name) .orderBy(organizations.createdAt); -const userOrgIds = userOrganizations.map((org) => org.id); +const userOrgIds = userOrganizations.map((org) => Organisation.OrganisationId.make(org.id));Similarly, ensure
user.defaultOrgIdis transformed when retrieved from the database (verify this in the user query/schema).apps/web/app/(org)/dashboard/settings/organization/components/MembersCard.tsx (1)
115-131: Critical: Confirmation dialog shows hardcoded test data instead of actual member.The confirmation dialog (line 128) uses
pendingMemberTest(hardcoded on lines 115-119) instead of thependingMemberstate variable. This means users will always see "John Doe" in the confirmation message, regardless of which member they're actually removing.Apply this diff to use the correct state variable:
- const pendingMemberTest = { - id: "1", - name: "John Doe", - email: "john.doe@example.com", - }; - return ( <> <ConfirmationDialog open={confirmOpen} icon={<FontAwesomeIcon icon={faUser} />} title="Remove member" description={ - pendingMemberTest - ? `Are you sure you want to remove ${pendingMemberTest.name} + pendingMember + ? `Are you sure you want to remove ${pendingMember.name} from your organization? this action cannot be undone.` : "" }apps/web/actions/spaces/remove-videos.ts (2)
74-74: Replaceanywithunknownfor stricter type safety.As per coding guidelines, avoid
any. Useunknowninstead and perform type narrowing if needed.Apply this diff:
- } catch (error: any) { + } catch (error: unknown) { return { success: false, - message: error.message || "Failed to remove videos from space", + message: error instanceof Error ? error.message : "Failed to remove videos from space", };
25-25: Remove inline comments per coding guidelines.As per coding guidelines: "Do not add inline, block, or docstring comments in any language; code must be self-explanatory."
Apply this diff to remove the comments:
- // Only allow removing videos the user owns const userVideos = await db() .select({ id: videos.id }) .from(videos) .where(and(eq(videos.ownerId, user.id), inArray(videos.id, videoIds))); const validVideoIds = userVideos.map((v) => v.id); if (validVideoIds.length === 0) { throw new Error("No valid videos found"); } - // Remove from spaceVideos await db() .delete(spaceVideos) .where( and( eq(spaceVideos.spaceId, spaceId), inArray(spaceVideos.videoId, validVideoIds), ), ); - // Set folderId to null for any removed videos that are in folders belonging to this space - // Find all folder IDs in this space const folderRows = await db() .select({ id: folders.id }) .from(folders)Also applies to: 37-37, 47-48
apps/web/app/api/video/comment/delete/route.ts (1)
10-24: Route should use Effect HttpApi pattern per coding guidelines.The coding guidelines specify:
- "Construct API routes with @effect/platform HttpApi/HttpApiBuilder and export only the handler from apiToHandler(ApiLive)"
- "Use HttpAuthMiddleware for required auth and provideOptionalAuth for guest routes"
This route uses plain Next.js Response handlers and direct
getCurrentUser()calls instead of the Effect-based patterns. While the current PR focuses on branded IDs, consider tracking this architectural migration.As per coding guidelines.
apps/web/actions/organizations/get-organization-videos.ts (1)
30-33: Brand returned video IDs asVideo.VideoId[].
ImportVideofrom"@cap/web-domain"and cast in the return:import type { Organisation, Video } from "@cap/web-domain"; // … return { success: true, data: videoIds.map((v) => v.videoId as Video.VideoId), };packages/database/auth/drizzle-adapter.ts (1)
141-155: Wrong brand used for account IDs
accounts.idrepresents an account primary key, but we’re instantiating it withUser.UserId.make(...). That defeats the purpose of branded identifiers and risks leaking account IDs where a user ID is expected. Please keep this as the plainnanoId()(or introduce anAccountIdbrand) instead of reusing the user ID brand.apps/web/app/(org)/dashboard/spaces/[spaceId]/actions.ts (1)
180-184: Do not brandspaceMembers.idas aUserId.
spaceMembers.idis the primary key of the membership row, not a user identifier. Wrapping the generated value withUser.UserId.makemisbrands that column and undermines the guarantees you're adding elsewhere—callers could wrongly treat a membership ID as a user ID. Please leave this as a plain nanoid (or introduce a dedicatedSpaceMemberIdbrand if desired).apps/web/app/api/video/comment/route.ts (1)
50-71: Non-awaited fetch errors aren’t caught; remove inline comments
- try/catch won’t catch errors from an unawaited Promise.
- Inline comments violate the repo guideline to avoid comments in TS files.
Apply:
- // Trigger email notification for new comment - if (type === "text" && userId !== "anonymous") { - try { - // Don't await this to avoid blocking the response + if (type === "text" && !isAnonymous) { const absoluteUrl = new URL( "/api/email/new-comment", serverEnv().WEB_URL, ).toString(); - fetch(absoluteUrl, { + void fetch(absoluteUrl, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ commentId: id, }), - }); - } catch (error) { - console.error("Error triggering comment notification:", error); - // Don't fail the comment creation if notification fails - } + }).catch((error) => { + console.error("Error triggering comment notification:", error); + }); }As per coding guidelines
♻️ Duplicate comments (1)
apps/web/app/(org)/signup/form.tsx (1)
121-123: Verify input validation for branded type constructor.The raw
organizationIdstring from user input is wrapped withOrganisation.OrganisationId.make()without prior validation. If themake()constructor enforces format constraints or throws on invalid input, the existing try-catch may not provide user-friendly error messages.
🧹 Nitpick comments (13)
apps/web/actions/organizations/get-organization-videos.ts (2)
9-11: Consider adding an explicit return type annotation.While TypeScript infers the return type, explicitly defining it improves type safety, catches errors earlier, and enhances IDE support.
Apply this diff to add an explicit return type:
export async function getOrganizationVideoIds( organizationId: Organisation.OrganisationId, -) { +): Promise< + | { success: true; data: string[] } + | { success: false; error: string } +> {
19-21: Validation check is somewhat redundant with branded types.Since
organizationIdis now typed asOrganisation.OrganisationId, TypeScript enforces non-null/non-undefined values at compile time, making this truthiness check largely redundant. However, keeping it as defensive programming is acceptable.apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
521-525: Consider using brandedVideoIdtype for consistency.The
sendProgressUpdatefunction usesvideoId: stringinstead of a branded type, which is inconsistent with the PR objective of using branded types for identifiers everywhere. Since this function receives video identifiers (e.g.,uploadIdat line 439), consider updating it to useVideo.VideoIdfor type consistency.Apply this diff to use the branded type:
+import type { Video } from "@cap/web-domain"; + const sendProgressUpdate = async ( - videoId: string, + videoId: Video.VideoId, uploaded: number, total: number, ) => {apps/web/app/(org)/dashboard/settings/account/Settings.tsx (1)
127-134: Consider removing empty string fallback for type consistency.The Select's
valueprop chains multipleOrganisation.OrganisationIdvalues but falls back to an empty string""on line 133. This mixes branded types with a plain string, which could cause type mismatches. Consider usingundefinedinstead of""for the final fallback, or ensure all values in the chain are of the same type.Apply this diff to use a consistent type:
value={ defaultOrgId ?? user?.defaultOrgId ?? - organizationData?.[0]?.organization.id ?? - "" + organizationData?.[0]?.organization.id }apps/web/app/(org)/login/form.tsx (1)
4-4: Consider type-only import for Organisation.Since
Organisationis only used for accessing theOrganisationIdtype namespace, consider using a type-only import to clarify intent and potentially improve build performance.Apply this diff:
-import { Organisation } from "@cap/web-domain"; +import type { Organisation } from "@cap/web-domain";apps/web/app/(org)/signup/form.tsx (1)
4-4: Consider type-only import for Organisation.Since
Organisationis only used for accessing theOrganisationIdtype namespace, consider using a type-only import.Apply this diff:
-import { Organisation } from "@cap/web-domain"; +import type { Organisation } from "@cap/web-domain";packages/web-backend/src/Spaces/index.ts (1)
18-21: Branded ID adoption looks good; consider symmetric casting for the space queryYou correctly cast to
Organisation.OrganisationIdfor the org lookup. For consistency (and to keepDz.eqoperands narrowly typed), consider also casting the space lookup:
- Current:
where(Dz.eq(Db.spaces.id, spaceOrOrgId))- Suggested:
where(Dz.eq(Db.spaces.id, spaceOrOrgId as Space.SpaceId))This avoids passing a union into
eqand can prevent future type widening.Also applies to: 42-47
apps/web/app/s/[videoId]/_components/Toolbar.tsx (1)
40-47: Root comment sentinel: ensure empty CommentId is normalized server-sideUsing
Comment.CommentId.make("")to denote “no parent” is fine for UI, but ensurenewCommentmaps this tonull/undefinedbefore DB writes; storing""inparentCommentIdcan cause filtering/join issues later.If normalization isn’t already done, prefer omitting the field, passing
null, or introducing an explicit Option type.Also applies to: 59-62, 81-88, 100-101
apps/web/app/embed/[videoId]/page.tsx (1)
14-14: Verify: is sharedOrganization.organizationId branded before passing to EmbedContent?
EmbedContentexpectsOrganisation.OrganisationId, but the select mapssharedVideos.organizationId(likely a raw string). Brand it before passing (or during shaping) to satisfy the contract:Example (before render):
const branded = video.sharedOrganization ? { organizationId: Organisation.OrganisationId.make(video.sharedOrganization.organizationId) } : null; // pass: <EmbedContent video={{ ...video, sharedOrganization: branded }} ... />Alternatively, brand inside the select mapping if convenient.
Also applies to: 206-208
apps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsx (1)
88-95: Branded IDs applied correctly across comments/replies
- Good:
id/authorIdbranding and delete handler signatures.- Verify: Using
Comment.CommentId.make("")for roots — ensurenewCommentnormalizes this tonull/undefinedbefore persisting.If not normalized, prefer omitting
parentCommentIdor using an explicit Option to avoid storing empty strings.Also applies to: 105-112, 130-141, 176-178, 213-213
apps/web/app/api/video/comment/route.ts (3)
41-41: Branded videoId: good; add input validationBranding is correct. Consider validating the request body (type/content/videoId/parentCommentId/timestamp) with a shared schema (e.g., Zod/domain schema) before constructing branded types.
As per coding guidelines
92-95: Avoidas anyon POST; return a typed ResponsePrefer awaiting the middleware and returning a Response without
any.Apply:
export const POST = async (request: NextRequest) => { const headersList = await headers(); - return rateLimitMiddleware(10, handlePost(request), headersList) as any; + return await rateLimitMiddleware(10, handlePost(request), headersList); };If types still force a cast, consider refining rateLimitMiddleware’s return type instead of using
any.
1-99: Route style deviates from @effect/platform HttpApiThis route uses NextRequest directly. The repo guidelines for apps/web/app/api/**/route.ts require:
- Build APIs with @effect/platform HttpApi/HttpApiBuilder and export only the handler from apiToHandler(ApiLive).
- Map domain errors with HttpApiError.* exhaustively.
- Use HttpAuthMiddleware/provideOptionalAuth; avoid duplicate session lookups.
- Provide dependencies via Layer.provide.
Recommend migrating this route to the HttpApi pattern in a follow-up PR to keep this PR focused on branded IDs.
As per coding guidelines
|
@coderabbitai dismiss it all. |
|
@oscartbeaumont: I'll resolve all my review comments on this PR. ✅ Actions performedComments resolved. |
2 participants
This will allow better type-safety in the new Effect code, it will improve interop with that new code and will prevent identifiers being used in the wrong places.
Summary by CodeRabbit
New Features
Improvements
API
Refactor