Implement farm invitation system with pending workflow#470
Merged
Conversation
…to a farm, instead of having it automatically. This makes it also possible to invite non-registered users to get access to a farm after signing up.
🦋 Changeset detectedLatest commit: c370904 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces direct farm role grants with a resource‑agnostic invitation workflow (email or principal target) with expiry, rate limits, auto‑accept on email verification, email notifications, and UI/actions to list, accept, and decline pending invitations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Route
participant Core
participant DB
participant Email
User->>UI: Submit invite (email or username)
UI->>Route: POST invite
Route->>Core: grantRoleToFarm(farmId, target, role)
Core->>DB: createInvitation(resource, resource_id, inviter, target, role) (status: pending, expires +7d)
DB-->>Core: invitation created
Route->>Core: lookupPrincipal(target) (optional)
Core->>DB: query principal by email/username
DB-->>Core: principal data or none
Route->>Email: renderFarmInvitationEmail(...)
Email-->>Route: html/content
Route->>Email: sendEmail(targetEmail, content)
Email-->>Route: sent
Route-->>UI: success
sequenceDiagram
participant Invitee
participant Email
participant UI
participant Route
participant Core
participant DB
Invitee->>Email: Click Accept link (or visits dashboard)
Invitee->>UI: POST accept_farm_invitation(invitation_id)
UI->>Route: accept action
Route->>Core: acceptInvitation(invitation_id, user_id)
Core->>DB: fetch invitation
DB-->>Core: pending invitation
Core->>Core: validate target / emailVerified / org-admin
Core->>DB: grantRole(...) and update invitation status -> accepted
DB-->>Core: role recorded, invitation updated
Core-->>Route: success
Route-->>UI: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #470 +/- ##
===============================================
- Coverage 88.10% 87.87% -0.24%
===============================================
Files 93 94 +1
Lines 4675 4930 +255
Branches 1498 1582 +84
===============================================
+ Hits 4119 4332 +213
- Misses 556 598 +42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
coderabbitai
Bot
changed the title
coderabbitai
Bot
added
branch:development
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
fdm-app/app/components/custom/autocomplete.tsx (1)
158-181:⚠️ Potential issue | 🟡 MinorStale
openclosure may preventallowValuesOutsideListfrom committing typed values on blur
openis read inside thesetTimeoutcallback from the render-closure captured at the timehandleInputBlurwas called. When the input blurs (e.g., Tab or clicking outside),openis stilltrueat that render. Even if Radix firesonOpenChange(false)in the same event, React state updates are asynchronous — the closure never sees the new value. The!openguard evaluates tofalse, so theallowValuesOutsideListbranch is skipped, leavingform.usernameempty and causing a validation failure on submit.Fix by tracking
openthrough a ref so the timeout always reads the latest value:🐛 Proposed fix
+ const openRef = useRef(false) + // Keep ref in sync with open state + useEffect(() => { + openRef.current = open + }, [open]) const handleInputBlur = () => { setTimeout(() => { - if (!open) { + if (!openRef.current) { if (inputValue && !selectedValue) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/custom/autocomplete.tsx` around lines 158 - 181, The blur handler's setTimeout reads the stale render-closure value of open so the !open guard can be wrong; change it to read a mutable ref that always reflects the latest open state: create a ref (e.g., openRef = useRef(open)), update openRef.current inside the component's onOpenChange handler, and then in handleInputBlur's timeout check !openRef.current instead of !open; keep the existing logic that uses allowValuesOutsideList, selectedValue, onSelectedValueChange, form/name, setInputValue and selectedLabel but read open via the ref so typed values are committed correctly on blur.fdm-app/app/routes/farm.$b_id_farm.settings.access.tsx (1)
129-135:⚠️ Potential issue | 🟠 MajorUse
session.principal_idinstead ofsession.user.idforgrantRoleToFarm.This file passes
session.user.idtograntRoleToFarm(line 131), whereasfarm.create.$b_id_farm.$calendar.access.tsxpassessession.principal_id(line 175). The function parameter is the acting principal, which is used for thesharepermission check. If the session principal differs from the user ID (e.g., user acting on behalf of an organization), usingsession.user.idmay incorrectly deny or bypass the authorization check. Usesession.principal_idconsistently with the create route.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.settings.access.tsx around lines 129 - 135, The call to grantRoleToFarm is passing session.user.id but the function expects the acting principal for the `share` permission check; replace session.user.id with session.principal_id in the grantRoleToFarm call so the authorization uses the session principal (matching how farm.create.$b_id_farm.$calendar.access.tsx passes session.principal_id), ensuring correct permission enforcement around the `share` check.fdm-core/src/farm.ts (1)
317-321:⚠️ Potential issue | 🟡 Minor
principal_id: PrincipalIdis passed asinviter_id(expectsstring)
PrincipalIdcan bestring | string[](per the codebase convention — read operations usePrincipalId; write/insert operations usestring).inviter_idin thefarmInvitationschema expects a plainstring. If a caller passes an array, the insert silently stores an array reference instead of an ID.Per the project convention (noted in learnings), functions that insert records should declare
principal_id: stringrather thanPrincipalId. Changing the parameter type here would also propagate the correct constraint to callers.🛡️ Proposed fix
export async function grantRoleToFarm( fdm: FdmType, - principal_id: PrincipalId, + principal_id: string, target: string,Based on learnings: "functions that insert/create data intentionally use
principal_id: stringinstead ofPrincipalIdto enforce that only a single principal ID can be used for creation operations."Also applies to: 391-391, 404-404, 428-428, 441-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.ts` around lines 317 - 321, Change the parameter type for principal_id from PrincipalId to string for functions that perform inserts so inviter_id is stored as a plain string; update the function signature containing "principal_id: PrincipalId, target: string, b_id_farm: ..., role: ..." to "principal_id: string" and any other insert-related functions (the other occurrences noted at the same file around the blocks referenced at 391, 404, 428, 441) to enforce a single-string principal for farmInvitation/farm creation; ensure callers are adjusted if needed so they pass a string ID rather than a PrincipalId array or union type.fdm-core/src/farm.test.ts (1)
504-520:⚠️ Potential issue | 🔴 CriticalTest regression:
grantRoleToFarmnow creates pending invitations instead of active roles, breaking this error testBoth branches of
grantRoleToFarm(lines 315–445) create a pending farmInvitation rather than granting an active role. WhenrevokePrincipalFromFarmis then called withtarget_username, the target has no active role to revoke. SincerevokePrincipaluses a soft-delete (UPDATE deleted = now), it silently succeeds even when no matching role exists (0 rows affected, no exception). The owner count check then passes becauseprincipal_idremains an owner, and the function returns normally—causing therejects.toThrowErrorassertion to fail.Additionally,
fdmMock.revokePrincipalis mocked but never called:revokePrincipalFromFarmimportsrevokePrincipaldirectly from./authorization.ts(line 547), not from thefdmobject, so the mock is ineffective regardless.Fix by accepting the invitation before revoking (to restore an active role for the target), or remove the mock and let the actual behavior be tested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.test.ts` around lines 504 - 520, The test fails because grantRoleToFarm now creates a pending farmInvitation (not an active role), so revokePrincipalFromFarm finds no active role and the fdmMock.revokePrincipal mock is never invoked because revokePrincipal is imported directly from ./authorization.ts; fix by accepting the invitation before calling revokePrincipalFromFarm (e.g., call the function that transitions farmInvitation to an active role or simulate acceptance) so the target has an active role to revoke, or alternatively remove the ineffective fdmMock and instead mock/spy the revokePrincipal export from ./authorization.ts (or import the module and jest.mock that module) so the revokePrincipal call is observed; ensure you reference grantRoleToFarm, revokePrincipalFromFarm, revokePrincipal, farmInvitation and fdmMock when making the change.
🧹 Nitpick comments (11)
fdm-core/src/principal.ts (2)
197-197: Remove commented-out debug log- // console.log(principalDetails)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/principal.ts` at line 197, Remove the leftover commented-out debug log "// console.log(principalDetails)" from principal.ts; delete that line (and any similar commented console.log references in the same function or nearby) so the production code contains no stray debug prints, keeping the function that uses principalDetails unchanged.
159-208:identifyPrincipalreturn type is now redundant — simplify toPromise<Principal | undefined>
Principalalready declaresid: string(added in this PR), so{ id: string } & Principalis equivalent to justPrincipal. The explicitid: principal_id[0].idin the returned object literal (line 200) is also redundant because...principalDetailswill spread the same value.♻️ Proposed simplification
export async function identifyPrincipal( fdm: FdmType, identifier: string, -): Promise< - | ({ - id: string - } & Principal) - | undefined -> { +): Promise<Principal | undefined> {- return { - id: principal_id[0].id, - ...principalDetails, - } + return principalDetails🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/principal.ts` around lines 159 - 208, The identifyPrincipal function currently declares a redundant return type of Promise<{ id: string } & Principal | undefined> and constructs a redundant return object; update the signature to Promise<Principal | undefined>, and in the transaction return the principalDetails directly (or return principalDetails as-is) since Principal already contains id; ensure getPrincipal(tx, ...) still returns a Principal so no additional merging (the explicit id: principal_id[0].id and the object spread can be removed).fdm-core/src/db/schema-authz.ts (1)
59-82: No DB-level constraint enforces that at least one oftarget_email/target_principal_idis setApplication code in
grantRoleToFarmprevents both fields being NULL, but without a DB check constraint a direct insert could produce an orphaned invitation row with no target. Consider adding a constraint for defense-in-depth.♻️ Proposed addition
Add
checkto your imports fromdrizzle-orm/pg-core, then add to the constraints array:+import { ..., check } from "drizzle-orm/pg-core" export const farmInvitation = fdmAuthZSchema.table( "farm_invitation", { ... }, (table) => [ uniqueIndex("farm_invitation_unique_email_idx") .on(table.farm_id, table.target_email) .where(sql`${table.status} = 'pending'`), uniqueIndex("farm_invitation_unique_principal_idx") .on(table.farm_id, table.target_principal_id) .where(sql`${table.status} = 'pending'`), + check( + "farm_invitation_target_not_both_null", + sql`${table.target_email} IS NOT NULL OR ${table.target_principal_id} IS NOT NULL`, + ), ], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/db/schema-authz.ts` around lines 59 - 82, The farmInvitation table lacks a DB-level check that ensures at least one of target_email or target_principal_id is set; import check from drizzle-orm/pg-core and add a table constraint to the farmInvitation definition (in the constraints array passed as the third argument) that enforces (target_email IS NOT NULL OR target_principal_id IS NOT NULL), referencing the farmInvitation table and the target_email / target_principal_id column symbols so the DB prevents orphaned invitations.fdm-app/app/components/custom/autocomplete.tsx (1)
50-50: Unnecessaryascast onemptyMessagedefault
"No items."is astring, which is directly assignable tostring | ((inputValue: string) => string). The explicit cast is redundant.- emptyMessage = "No items." as string | ((inputValue: string) => string), + emptyMessage = "No items.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/custom/autocomplete.tsx` at line 50, The default value for the emptyMessage prop is using an unnecessary type assertion; replace the explicit cast ("No items." as string | ((inputValue: string) => string)) with the plain string literal "No items." and rely on the declared prop/type of emptyMessage to accept string | ((inputValue: string) => string); update the default in the autocomplete component where emptyMessage is defined so the value is simply "No items." without the as cast.fdm-app/app/components/blocks/email/farm-invitation.tsx (2)
16-27:invitationIdprop is declared but never used in the component.The
invitationIdis included inFarmInvitationEmailPropsand destructured (line 39), but it's never referenced in the rendered output. Either remove it from the interface and destructuring, or use it (e.g., as a reference ID in the email or as a query param in the CTA link).If it should be removed
interface FarmInvitationEmailProps { farmName: string inviterName: string targetEmail: string - invitationId: string role: string appName: string appBaseUrl?: string logoFileName?: string /** If true, renders a "create account" CTA for unregistered users */ isUnregistered?: boolean }export const FarmInvitationEmail = ({ farmName, inviterName, targetEmail, - invitationId, role, appName,Also applies to: 35-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/email/farm-invitation.tsx` around lines 16 - 27, The interface FarmInvitationEmailProps and the component destructuring include invitationId but it is never used; either remove invitationId from FarmInvitationEmailProps and from the component's destructuring, or incorporate it into the rendered output (for example append invitationId as a query param to the CTA link URL or render it as a reference ID in the email body). Locate the FarmInvitationEmailProps declaration and the component props destructure (where invitationId is currently pulled) and implement one of these changes consistently across the component (update the CTA link builder or remove the prop from the interface and destructure).
80-82: Greeting usestargetEmail— consider using the recipient's name when available.
"Hallo {targetEmail}"is functional, but for registered users whose name is known to the caller, passing and displaying a friendlier name would improve the email experience. Low priority.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/email/farm-invitation.tsx` around lines 80 - 82, The greeting currently renders the recipient's email via targetEmail; update the FarmInvitation component to prefer a recipient name when available by adding/using a prop like recipientName (or displayName) and change the greeting Text to render recipientName || targetEmail so it falls back to the email; ensure any callers that build this component pass the name when available and update prop typing for the component (e.g., props on FarmInvitation) to include the new optional recipientName.fdm-app/app/routes/farm._index.tsx (1)
331-434: ~100 lines of invitation card rendering duplicated within the same file.The invitation card UI (icon, role label, farm/org info, accept/decline forms) is identical between the "no farms" branch (lines 331–434) and the "has farms" branch (lines 559–661). Extract a shared
PendingInvitationCardcomponent (or aPendingInvitationsSection) to eliminate the duplication and keep both views in sync.Additionally, the role label mapping (
owner→Eigenaar,advisor→Adviseur, etc.) now appears in at least four places across the codebase (here twice, in the email component, and in the farm card badges). Consider extracting a sharedgetRoleLabel(role: string): stringutility.Also applies to: 559-661
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm._index.tsx` around lines 331 - 434, Extract the duplicated invitation card markup into a new React component named PendingInvitationCard (props: invitation) and replace both in-file repeated blocks (the branch at the current render and the other branch starting around the later block) with <PendingInvitationCard invitation={invitation} /> inside the .map; move the accept/decline Form logic, hidden inputs (intent and invitation_id), and buttons into that component and preserve Card, CardHeader, CardContent, CardFooter structure and keys (use invitation.invitation_id). Also create a small utility getRoleLabel(role: string): string that maps "owner"→"Eigenaar", "advisor"→"Adviseur", default→"Onderzoeker", import and use it in PendingInvitationCard (and update the other usages elsewhere such as the mail component and farm card badges) so role label logic is centralized.fdm-app/app/routes/farm.create.$b_id_farm.$calendar.access.tsx (1)
180-215: Duplicated email-invitation logic across two route actions.This entire block (normalize target → lookup principal → derive email → render & send) is nearly identical to
farm.$b_id_farm.settings.access.tsxlines 137–172. Consider extracting a shared helper (e.g.,sendFarmInvitationEmail(fdm, session, formValues, b_id_farm)) in~/lib/email.server.tsor a similar utility module to keep both routes in sync and reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.access.tsx around lines 180 - 215, Extract the duplicated invitation flow into a shared helper (e.g., sendFarmInvitationEmail) in ~/lib/email.server.ts: implement a function sendFarmInvitationEmail(fdm, session, formValues, b_id_farm) that performs normalization of formValues.username, determines isEmail, calls lookupPrincipal, picks targetPrincipal, derives targetEmail, calls renderFarmInvitationEmail with inviterName and isUnregistered, and calls sendEmail (returning success/failure or throwing on error); then replace the duplicated blocks in farm.create.$b_id_farm.$calendar.access.tsx and farm.$b_id_farm.settings.access.tsx with a single call to this helper and import it, preserving existing error handling at the call sites.fdm-core/src/farm.test.ts (1)
1100-1108: Strengthen the assertion for the "by principal_id" test caseThe test only verifies
Array.isArray(invitations), which provides no meaningful signal. At a minimum, assert the known state: iftarget_idhas pending invitations from earlier tests (e.g., from the unaccepted principal-targeted invitation created in thegrantRoleToFarmdescribe block), the array should be non-empty.♻️ Suggested improvement
it("should return pending invitations for a user by principal_id", async () => { const invitations = await listPendingInvitationsForUser(fdm, target_id) - // Target may have invitations from earlier tests (email-based or principal-based) - expect(Array.isArray(invitations)).toBe(true) + expect(Array.isArray(invitations)).toBe(true) + // target_id has at least one pending invitation from the unaccepted grantRoleToFarm call above + expect(invitations.length).toBeGreaterThanOrEqual(1) + expect(invitations.every(inv => inv.status === "pending")).toBe(true) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.test.ts` around lines 1100 - 1108, The test currently only checks Array.isArray(invitations) which is weak; update the "should return pending invitations for a user by principal_id" test that calls listPendingInvitationsForUser(fdm, target_id) to also assert that the returned invitations array is non-empty (e.g., expect(invitations.length).toBeGreaterThan(0)) since target_id is expected to have pending invitations from earlier tests—keep the array-type check if you want both assertions.fdm-core/src/farm.ts (2)
652-682: Initials calculation duplicatesgetPrincipaland has a null-unsafe initThis block is a verbatim copy of the logic in
getPrincipal(fdm-core/src/principal.tslines 32–131). Duplication here risks the two diverging silently.Additionally,
let initials = u.email(line 653) is typed asstring | null. Iffirstname/nameguards trigger,initials.toUpperCase()at line 681 throws at runtime. The same latent bug exists ingetPrincipal; extracting a shared helper fixes both at once.♻️ Suggested refactor: extract a shared helper in principal.ts
+// In principal.ts — add and export: +export function computeInitials(user: { + email: string | null + firstname?: string | null + surname?: string | null + name?: string | null +}): string { + let initials: string | null = user.email + if (user.firstname && user.surname) { + initials = user.firstname.charAt(0).toUpperCase() + const surnameParts = user.surname.split(/\s+/) + for (const part of surnameParts) { + if (part.length > 0) { + const char = part.charAt(0) + if (char === char.toUpperCase() && char.match(/[a-zA-Z]/)) { + initials += char.toUpperCase() + break + } + } + } + } else if (user.firstname) { + initials = user.firstname[0] + } else if (user.name) { + initials = user.name[0] + } + return (initials ?? "?").toUpperCase() +}Then replace the duplicated block in both
getPrincipalandlistPrincipalsForFarmwithcomputeInitials(u).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.ts` around lines 652 - 682, The initials logic in listPrincipalsForFarm duplicates getPrincipal and uses u.email (string | null) causing initials.toUpperCase() to throw; extract a shared helper computeInitials(principal: PrincipalLike) in principal.ts that returns a non-null string (use safe fallbacks like empty string and normalize casing), replace the duplicated initials computation in both getPrincipal and listPrincipalsForFarm to call computeInitials(u), and ensure principalsMap.set still stores initials: computeInitials(u). This removes duplication and fixes the null-unsafe email case.
359-365: Consider a library for email validation instead of regexWhile the current regex
/^[^\s@]+@[^\s@]+\.[^\s@]+$/functions correctly for basic email structure checks, using a dedicated library provides better RFC compliance and reduces maintenance burden. Considerisemail,email-validator, or an RFC 5322 parser likeemail-addressesfor more robust validation.The regex works safely in practice and doesn't exhibit the ReDoS behavior originally claimed. However, as a general best practice for email validation on user input, a library is preferable to custom regex.
📚 Library alternatives for TypeScript/Node.js
isemail(Hapi) – RFC-compliant with built-in TypeScript supportemail-validator– Simple package with included type definitionsemail-addresses– RFC 5322 parser-based approach (recommended for strict compliance)@sparser/email-address-parser– WASM-based RFC 5322/6532 parser with TS declarationsNote: Avoid the Segment
is-emailpackage in versions < 1.0.1 (has known ReDoS CVE-2021-36716).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.ts` around lines 359 - 365, The current inline regex (emailRegex) used to validate normalizedTarget before assigning targetEmail should be replaced with a well‑maintained email validation library (e.g., isemail or email-validator) for RFC compliance; update the code that currently references emailRegex.test(normalizedTarget) to call the library's validation function (for example isEmail(normalizedTarget) or validator.validate(normalizedTarget)), keep the same error behavior (throwing "Target not found and not a valid email address" when invalid), and add the proper import and TypeScript types for the chosen validator so normalizedTarget -> validation -> targetEmail assignment is robust and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/farm-invitation-app.md:
- Line 5: Fix the typo in the release note sentence "Add the possibilty for
users to accept or reject an invitation to a farm..." by replacing "possibilty"
with "possibility" so the sentence reads "Add the possibility for users to
accept or reject an invitation to a farm, instead of having it automatically."
In @.changeset/farm-invitation-system.md:
- Around line 1-12: The changeset currently declares a "minor" bump for
"@svenvw/fdm-core" but the change to grantRoleToFarm (now creating pending
invitations) is breaking; update the changeset header from "minor" to "major" so
the package version is bumped to a major release and consumers are properly
warned about the breaking API change (ensure the header string "major" replaces
"minor" and keep the rest of the changeset content intact).
In `@fdm-core/src/authentication.ts`:
- Around line 228-236: The call to autoAcceptInvitationsForNewUser inside the
after-hook can throw and block user creation; wrap the invocation in a try/catch
so failures are logged but do not propagate. Specifically, inside the after hook
where you check user.emailVerified and call autoAcceptInvitationsForNewUser(fdm,
user.email, user.id), catch any error, log it via the existing logger (or
process logger) with context (e.g., "autoAcceptInvitationsForNewUser failed for
user.id/user.email") and swallow the error so the hook returns successfully and
does not prevent user creation.
- Around line 239-249: The update hook currently calls
autoAcceptInvitationsForNewUser(user.email, user.id) directly which can throw
and runs on every profile edit; wrap that call in a try/catch to prevent
exceptions from bubbling out of the after hook, log a short debug/info message
before attempting auto-accept (including user.id or user.email) and log the
caught error (without rethrowing), and keep the existing guard that only runs
when user.emailVerified is truthy so work is only attempted when verified.
In `@fdm-core/src/farm.ts`:
- Around line 990-992: The declineFarmInvitation path currently only checks if
invitation.status !== "pending" (in declineFarmInvitation) and can wrongly allow
declining an already-expired invite; update declineFarmInvitation to mirror
acceptFarmInvitation by checking the invitation expiry before changing state —
if the invitation is expired, set invitation.status = "expired" (persist the
change) and throw an error instead of allowing a "declined" transition; ensure
you use the same expiry-detection logic used by acceptFarmInvitation and
persist/log the status change so audit records remain consistent.
- Around line 820-838: In acceptFarmInvitation, replace the separate SELECT +
application-level status check with a single atomic conditional UPDATE on
authZSchema.farmInvitation that sets status = 'accepted' (and
target_principal_id if required) WHERE invitation_id = invitation_id AND status
= 'pending' (and expires >= NOW()); check the UPDATE's affectedRows and throw
"Invitation not found" / "Invitation has expired" if zero rows were affected,
then call grantRole only after the successful conditional UPDATE, and remove the
later unconditional UPDATE that sets status = "accepted" (leave only any
target_principal_id update if not covered by the conditional UPDATE) to prevent
the TOCTOU race.
In `@fdm-core/src/invitation.test.ts`:
- Around line 186-228: The test currently passes the already-normalized
normalizedEmail into autoAcceptInvitationsForNewUser so it never verifies that
autoAcceptInvitationsForNewUser normalizes input; change the call to
autoAcceptInvitationsForNewUser(fdm, mixedCaseEmail, caseUser.user.id) so the
function receives the mixed-case address and exercises its normalization path
(leave grantRoleToFarm call and other assertions unchanged, referencing the
mixedCaseEmail variable and the autoAcceptInvitationsForNewUser function).
- Line 6: Remove the unused import causing TS6133: delete the import of
authNSchema from invitation.test.ts (the line importing "authNSchema" from
"./db/schema-authn") so the test file no longer imports an unused symbol;
alternatively, if the schema is actually needed, reference authNSchema in the
test (e.g., use a value from it) to avoid the unused-import error—prefer
removing the import if it's truly unused.
In `@fdm-core/src/invitation.ts`:
- Around line 79-84: The current catch block passes the raw email into
handleError (in the autoAcceptInvitationsForNewUser flow), which can leak PII to
logs; instead remove or obfuscate the email before adding it to the error
context—e.g., omit the email field or replace it with a hashed/masked value
(derived from email) and keep user_id and other safe metadata; update the call
site that invokes handleError(err, "Exception for
autoAcceptInvitationsForNewUser", { email, user_id }) to pass only non-PII or
masked_email and user_id.
---
Outside diff comments:
In `@fdm-app/app/components/custom/autocomplete.tsx`:
- Around line 158-181: The blur handler's setTimeout reads the stale
render-closure value of open so the !open guard can be wrong; change it to read
a mutable ref that always reflects the latest open state: create a ref (e.g.,
openRef = useRef(open)), update openRef.current inside the component's
onOpenChange handler, and then in handleInputBlur's timeout check
!openRef.current instead of !open; keep the existing logic that uses
allowValuesOutsideList, selectedValue, onSelectedValueChange, form/name,
setInputValue and selectedLabel but read open via the ref so typed values are
committed correctly on blur.
In `@fdm-app/app/routes/farm`.$b_id_farm.settings.access.tsx:
- Around line 129-135: The call to grantRoleToFarm is passing session.user.id
but the function expects the acting principal for the `share` permission check;
replace session.user.id with session.principal_id in the grantRoleToFarm call so
the authorization uses the session principal (matching how
farm.create.$b_id_farm.$calendar.access.tsx passes session.principal_id),
ensuring correct permission enforcement around the `share` check.
In `@fdm-core/src/farm.test.ts`:
- Around line 504-520: The test fails because grantRoleToFarm now creates a
pending farmInvitation (not an active role), so revokePrincipalFromFarm finds no
active role and the fdmMock.revokePrincipal mock is never invoked because
revokePrincipal is imported directly from ./authorization.ts; fix by accepting
the invitation before calling revokePrincipalFromFarm (e.g., call the function
that transitions farmInvitation to an active role or simulate acceptance) so the
target has an active role to revoke, or alternatively remove the ineffective
fdmMock and instead mock/spy the revokePrincipal export from ./authorization.ts
(or import the module and jest.mock that module) so the revokePrincipal call is
observed; ensure you reference grantRoleToFarm, revokePrincipalFromFarm,
revokePrincipal, farmInvitation and fdmMock when making the change.
In `@fdm-core/src/farm.ts`:
- Around line 317-321: Change the parameter type for principal_id from
PrincipalId to string for functions that perform inserts so inviter_id is stored
as a plain string; update the function signature containing "principal_id:
PrincipalId, target: string, b_id_farm: ..., role: ..." to "principal_id:
string" and any other insert-related functions (the other occurrences noted at
the same file around the blocks referenced at 391, 404, 428, 441) to enforce a
single-string principal for farmInvitation/farm creation; ensure callers are
adjusted if needed so they pass a string ID rather than a PrincipalId array or
union type.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/email/farm-invitation.tsx`:
- Around line 16-27: The interface FarmInvitationEmailProps and the component
destructuring include invitationId but it is never used; either remove
invitationId from FarmInvitationEmailProps and from the component's
destructuring, or incorporate it into the rendered output (for example append
invitationId as a query param to the CTA link URL or render it as a reference ID
in the email body). Locate the FarmInvitationEmailProps declaration and the
component props destructure (where invitationId is currently pulled) and
implement one of these changes consistently across the component (update the CTA
link builder or remove the prop from the interface and destructure).
- Around line 80-82: The greeting currently renders the recipient's email via
targetEmail; update the FarmInvitation component to prefer a recipient name when
available by adding/using a prop like recipientName (or displayName) and change
the greeting Text to render recipientName || targetEmail so it falls back to the
email; ensure any callers that build this component pass the name when available
and update prop typing for the component (e.g., props on FarmInvitation) to
include the new optional recipientName.
In `@fdm-app/app/components/custom/autocomplete.tsx`:
- Line 50: The default value for the emptyMessage prop is using an unnecessary
type assertion; replace the explicit cast ("No items." as string | ((inputValue:
string) => string)) with the plain string literal "No items." and rely on the
declared prop/type of emptyMessage to accept string | ((inputValue: string) =>
string); update the default in the autocomplete component where emptyMessage is
defined so the value is simply "No items." without the as cast.
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 331-434: Extract the duplicated invitation card markup into a new
React component named PendingInvitationCard (props: invitation) and replace both
in-file repeated blocks (the branch at the current render and the other branch
starting around the later block) with <PendingInvitationCard
invitation={invitation} /> inside the .map; move the accept/decline Form logic,
hidden inputs (intent and invitation_id), and buttons into that component and
preserve Card, CardHeader, CardContent, CardFooter structure and keys (use
invitation.invitation_id). Also create a small utility getRoleLabel(role:
string): string that maps "owner"→"Eigenaar", "advisor"→"Adviseur",
default→"Onderzoeker", import and use it in PendingInvitationCard (and update
the other usages elsewhere such as the mail component and farm card badges) so
role label logic is centralized.
In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.access.tsx:
- Around line 180-215: Extract the duplicated invitation flow into a shared
helper (e.g., sendFarmInvitationEmail) in ~/lib/email.server.ts: implement a
function sendFarmInvitationEmail(fdm, session, formValues, b_id_farm) that
performs normalization of formValues.username, determines isEmail, calls
lookupPrincipal, picks targetPrincipal, derives targetEmail, calls
renderFarmInvitationEmail with inviterName and isUnregistered, and calls
sendEmail (returning success/failure or throwing on error); then replace the
duplicated blocks in farm.create.$b_id_farm.$calendar.access.tsx and
farm.$b_id_farm.settings.access.tsx with a single call to this helper and import
it, preserving existing error handling at the call sites.
In `@fdm-core/src/db/schema-authz.ts`:
- Around line 59-82: The farmInvitation table lacks a DB-level check that
ensures at least one of target_email or target_principal_id is set; import check
from drizzle-orm/pg-core and add a table constraint to the farmInvitation
definition (in the constraints array passed as the third argument) that enforces
(target_email IS NOT NULL OR target_principal_id IS NOT NULL), referencing the
farmInvitation table and the target_email / target_principal_id column symbols
so the DB prevents orphaned invitations.
In `@fdm-core/src/farm.test.ts`:
- Around line 1100-1108: The test currently only checks
Array.isArray(invitations) which is weak; update the "should return pending
invitations for a user by principal_id" test that calls
listPendingInvitationsForUser(fdm, target_id) to also assert that the returned
invitations array is non-empty (e.g.,
expect(invitations.length).toBeGreaterThan(0)) since target_id is expected to
have pending invitations from earlier tests—keep the array-type check if you
want both assertions.
In `@fdm-core/src/farm.ts`:
- Around line 652-682: The initials logic in listPrincipalsForFarm duplicates
getPrincipal and uses u.email (string | null) causing initials.toUpperCase() to
throw; extract a shared helper computeInitials(principal: PrincipalLike) in
principal.ts that returns a non-null string (use safe fallbacks like empty
string and normalize casing), replace the duplicated initials computation in
both getPrincipal and listPrincipalsForFarm to call computeInitials(u), and
ensure principalsMap.set still stores initials: computeInitials(u). This removes
duplication and fixes the null-unsafe email case.
- Around line 359-365: The current inline regex (emailRegex) used to validate
normalizedTarget before assigning targetEmail should be replaced with a
well‑maintained email validation library (e.g., isemail or email-validator) for
RFC compliance; update the code that currently references
emailRegex.test(normalizedTarget) to call the library's validation function (for
example isEmail(normalizedTarget) or validator.validate(normalizedTarget)), keep
the same error behavior (throwing "Target not found and not a valid email
address" when invalid), and add the proper import and TypeScript types for the
chosen validator so normalizedTarget -> validation -> targetEmail assignment is
robust and maintainable.
In `@fdm-core/src/principal.ts`:
- Line 197: Remove the leftover commented-out debug log "//
console.log(principalDetails)" from principal.ts; delete that line (and any
similar commented console.log references in the same function or nearby) so the
production code contains no stray debug prints, keeping the function that uses
principalDetails unchanged.
- Around line 159-208: The identifyPrincipal function currently declares a
redundant return type of Promise<{ id: string } & Principal | undefined> and
constructs a redundant return object; update the signature to Promise<Principal
| undefined>, and in the transaction return the principalDetails directly (or
return principalDetails as-is) since Principal already contains id; ensure
getPrincipal(tx, ...) still returns a Principal so no additional merging (the
explicit id: principal_id[0].id and the object spread can be removed).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
fdm-app/app/components/custom/autocomplete.tsx (2)
68-73:⚠️ Potential issue | 🟠 Major
handleInputBlurclears the displayed input after focus+blur for committed free-form values whenallowValuesOutsideList=true.When a free-form value (e.g., an email address) is committed by the first blur,
selectedValue = "foo@bar.com"butselectedLabel = ""(the email is not initems). On a second focus + immediate blur without typing:
inputValue = "foo@bar.com"(truthy),!selectedValue = false→ the firstifon line 164 fails.else if (inputValue !== selectedLabel && selectedValue)→"foo@bar.com" !== "" && "foo@bar.com"→true.setInputValue(selectedLabel)=setInputValue("")— the input goes blank.
selectedValueand the form state still hold"foo@bar.com", so the form would submit the correct value, but the displayed input is now empty, creating a misleading UI state. TheuseEffectat lines 123–134 does not restore the input becauseselectedValue && selectedLabel=truthy && ""=false.The cleanest fix is to update
selectedLabelto fall back toselectedValuewhenallowValuesOutsideList=trueand the value is not found initems. This makes theelse ifcomparison self-consistent ("foo@bar.com" !== "foo@bar.com"→false) and also corrects theuseEffectrestore path for pre-populated free-form values:🐛 Proposed fix — make selectedLabel fall back to selectedValue for free-form entries
const selectedLabel = useMemo(() => { const selectedItem = items.find((item) => item.value === selectedValue) - return selectedItem?.label ?? "" + if (selectedItem) return selectedItem.label + // For free-form entries the value IS the display label + if (allowValuesOutsideList && selectedValue) return selectedValue as string + return "" -}, [selectedValue, items]) +}, [selectedValue, items, allowValuesOutsideList])Also applies to: 160-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/custom/autocomplete.tsx` around lines 68 - 73, The computed selectedLabel should fall back to the committed free-form value when the component allows values outside the list; update the useMemo that computes selectedLabel (the block referencing selectedValue and items) to return selectedItem?.label ?? (allowValuesOutsideList ? selectedValue ?? "" : "") so that when allowValuesOutsideList is true and the current selectedValue isn't in items the displayed label equals the selectedValue; this will also make the handleInputBlur comparisons and the useEffect restore logic behave consistently for pre-populated free-form values.
68-73:⚠️ Potential issue | 🟠 Major
handleInputBlur'selse ifbranch clears the displayed value after focus+blur on a committed free-form entry (allowValuesOutsideList=true).When a free-form value (e.g. an email address
"foo@bar.com") is committed by the first blur,selectedValue = "foo@bar.com"butselectedLabel = ""because the address is not initems. On a subsequent focus + immediate blur without typing anything:
- First
if (inputValue && !selectedValue)— fails (selectedValueis non-empty).else if ("foo@bar.com" !== "" && "foo@bar.com")— true →setInputValue("").The input goes blank while
selectedValueand the form state still hold"foo@bar.com". TheuseEffectat lines 123–134 does not restore the display becauseselectedValue && selectedLabel=truthy && ""= false.The cleanest fix is to have
selectedLabelfall back toselectedValuefor free-form entries, making every downstream comparison self-consistent:🐛 Proposed fix — fall back to selectedValue in selectedLabel for free-form entries
const selectedLabel = useMemo(() => { const selectedItem = items.find((item) => item.value === selectedValue) - return selectedItem?.label ?? "" -}, [selectedValue, items]) + if (selectedItem) return selectedItem.label + // For free-form values (allowValuesOutsideList) the value itself is the display label + if (allowValuesOutsideList && selectedValue) return selectedValue as string + return "" +}, [selectedValue, items, allowValuesOutsideList])This also corrects pre-populated free-form values on initial render, which currently show an empty input until a fetch fires.
Also applies to: 159-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/custom/autocomplete.tsx` around lines 68 - 73, selectedLabel falls back to empty string when selectedValue is a free-form entry, causing handleInputBlur's else-if to clear the input; change the useMemo that defines selectedLabel to return selectedItem?.label ?? selectedValue (i.e., use selectedValue as a fallback when no matching item exists) so free-form committed values display consistently (affects selectedLabel, selectedValue, and downstream logic in handleInputBlur and the related useEffect).fdm-core/src/principal.ts (1)
134-162:⚠️ Potential issue | 🟡 MinorStale JSDoc for
identifyPrincipalafter return-type change.The
@returnscomment still says "resolves to an array of LookupPrincipal objects … the array contains a single object … the array is empty" and the@examplecalls.lengthand[0]on the result — none of which is valid now that the return type isPromise<Principal | undefined>. This will mislead consumers of the public API.📝 Suggested JSDoc update
- * `@returns` A promise that resolves to an array of LookupPrincipal objects. If a principal is found, the array contains a single object with the principal's details. If no principal is found, the array is empty. + * `@returns` A promise that resolves to a {`@link` Principal} object if a matching principal is found, or `undefined` if none is found.Update the
@exampleaccordingly:- * const principalDetails = await identifyPrincipal(fdm, "john.doe@example.com"); - * if (principalDetails.length > 0) { - * console.log("Principal Details:", principalDetails[0]); - * } else { - * console.log("Principal not found."); - * } + * const principalDetails = await identifyPrincipal(fdm, "john.doe@example.com"); + * if (principalDetails) { + * console.log("Principal Details:", principalDetails); + * } else { + * console.log("Principal not found."); + * }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/principal.ts` around lines 134 - 162, Update the stale JSDoc for identifyPrincipal to match its actual return type Promise<Principal | undefined>: change the `@returns` description to state it resolves to a Principal or undefined (not an array), adjust the example to await identifyPrincipal and check for truthiness (e.g., const principal = await identifyPrincipal(...); if (principal) { /* use principal */ } else { /* not found */ }), and remove any array-specific references such as .length or [0]; keep the thrown-error and parameter docs as-is.
🧹 Nitpick comments (4)
fdm-core/src/db/schema-authz.ts (1)
69-69:statusas plaintext()has no DB-level constraint on valid values.Invalid status strings (
"revoked","cancelled", etc.) can be written directly to the database. Drizzle supports schema-scoped enums viapgSchema.enum(), which would also produce a proper PostgreSQLENUMtype and prevent invalid values at the DB level.♻️ Proposed refactor using a schema-scoped enum
+export const invitationStatusEnum = fdmAuthZSchema.enum("invitation_status", [ + "pending", + "accepted", + "declined", + "expired", +]) + export const invitation = fdmAuthZSchema.table( "invitation", { ... - status: text().notNull().default("pending"), + status: invitationStatusEnum().notNull().default("pending"), ... },Alternatively, a
CHECKconstraint (using the samecheck()import) keeps it astextwhile still enforcing valid values at the DB level:check( "invitation_status_check", sql`${table.status} IN ('pending', 'accepted', 'declined', 'expired')`, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/db/schema-authz.ts` at line 69, Change the plain text status column to a DB-enforced enum or add a CHECK constraint: replace the current status: text().notNull().default("pending") definition with a schema-scoped enum created via pgSchema.enum("invitation_status", ["pending","accepted","declined","expired"]) and then use that enum for the column (e.g., status: pgEnum("invitation_status").notNull().default("pending")), or keep text() but add a check() constraint (e.g., check("invitation_status_check", sql`${table.status} IN ('pending','accepted','declined','expired')`)); update any imports to include pgSchema/pgEnum or check/sql as needed and ensure the enum name and column identifier match (status) across the schema.fdm-app/app/routes/farm.create.$b_id_farm.$calendar.access.tsx (1)
182-215: Email invitation logic duplicated across two route files.The block at lines 182–215 is nearly identical to the same block in
fdm-app/app/routes/farm.$b_id_farm.settings.access.tsx(lines 138–172). Any bug fix or enhancement must be applied in both places; extract to a shared helper (e.g.,sendFarmInvitationEmailIfPossible) in~/lib/email.serveror a dedicated utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.access.tsx around lines 182 - 215, Extract the duplicated invitation-email block into a shared async helper (e.g., sendFarmInvitationEmailIfPossible) in ~/lib/email.server that takes the same inputs it needs (fdm, principalId, b_id_farm, formValues, session) and encapsulates the logic currently in the try block: call getFarm(b_id_farm), normalize formValues.username, detect isEmail via isEmail(), lookupPrincipal(), resolve targetPrincipal and targetEmail, renderFarmInvitationEmail(targetEmail, inviterName, farm.b_name_farm ?? b_id_farm, formValues.role, isUnregistered) and call sendEmail(email); preserve the try/catch and logging behavior (console.error) inside the helper and then replace both in-file blocks with a single await sendFarmInvitationEmailIfPossible(...) call so fixes are applied in one place.fdm-core/src/farm.ts (1)
1114-1118: Tighten invitation cleanup to the farm resource.This delete is keyed only by
resource_id. For clarity and future-proofing, consider also filtering onresource = "farm"so other resources with the same ID aren’t affected.♻️ Suggested tweak
- await tx - .delete(authZSchema.invitation) - .where(eq(authZSchema.invitation.resource_id, b_id_farm)) + await tx + .delete(authZSchema.invitation) + .where( + and( + eq(authZSchema.invitation.resource, "farm"), + eq(authZSchema.invitation.resource_id, b_id_farm), + ), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.ts` around lines 1114 - 1118, The invitation delete currently only filters by resource_id using tx.delete(authZSchema.invitation).where(eq(authZSchema.invitation.resource_id, b_id_farm)); tighten it to also filter by the resource type by adding a second condition (e.g., eq(authZSchema.invitation.resource, "farm")) combined with the existing predicate so only invitations for the farm resource with that ID are removed; update the where clause used in the tx.delete call to include both predicates (AND).fdm-app/app/routes/farm._index.tsx (1)
332-433: Deduplicate pending-invitation card markup.The invitation card UI is duplicated in both the “no farms” and “has farms” branches, which risks divergence. Consider extracting a shared
PendingInvitationscomponent.Also applies to: 559-661
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm._index.tsx` around lines 332 - 433, The pending-invitation card markup (rendering loaderData.pendingInvitations into Card elements using invitation.invitation_id, CardHeader/CardContent/CardFooter and the accept_farm_invitation / decline_farm_invitation Forms) is duplicated across the “no farms” and “has farms” branches; extract this into a reusable PendingInvitations component that accepts the invitations array and renders each Card (preserving keys, role mapping, org_name display, hidden inputs for intent and invitation_id, and Button variants) and replace both duplicated blocks with <PendingInvitations invitations={loaderData.pendingInvitations} /> to keep behavior identical and avoid future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/components/blocks/email/farm-invitation.tsx`:
- Around line 40-44: The default appBaseUrl is empty which makes logoPath =
`${appBaseUrl}${logoFileName}` produce a relative URL (e.g.,
`/fdm-high-resolution-logo-transparent.png`) that email clients cannot resolve;
update FarmInvitationEmailProps to require appBaseUrl (remove the
default/optional) or enforce a guard in the FarmInvitationEmail component:
validate/apply a fallback absolute URL before computing logoPath, and update any
callers (e.g., renderFarmInvitationEmail) or docs to always supply a full
absolute appBaseUrl so logoPath becomes an absolute URL.
- Around line 51-54: Remove the external Google Fonts <link> tag and the
adjacent <style> block from the farm-invitation.tsx email component (the <link
href="https://fonts.googleapis.com..."> element and any embedded <style> for
that font); email clients usually strip external Head content and this leaks
recipient info to Google, so rely on the existing fontFamily fallback stack
already used in the component's JSX/inline styles instead of loading web fonts.
In `@fdm-app/app/components/custom/autocomplete.tsx`:
- Line 59: The openRef created as const openRef = useRef(open) never reflects
programmatic opens via PopoverAnchor and so the guard in handleInputBlur is
ineffective; either remove openRef entirely and rely on the existing
e.preventDefault() guard on CommandItem.onMouseDown, or keep it and explicitly
update openRef.current whenever you call setOpen — update every site that
directly calls setOpen (handleSelectItem, onKeyDown, onMouseDown, onFocus) to
also set openRef.current to the same boolean, and ensure any Escape-key path
that calls setOpen(false) also sets openRef.current = false so blur logic
behaves correctly with both PopoverTrigger and PopoverAnchor usage.
- Line 59: openRef created via useRef(open) falls out of sync because setOpen is
called directly in several handlers; update openRef.current whenever you call
setOpen and also sync it inside the onOpenChange handler (and/or a useEffect
watching open) so the ref always matches the controlled open prop. Concretely:
wherever setOpen(true/false) is invoked (functions/handlers named onKeyDown,
onMouseDown, onFocus, handleSelectItem), immediately set openRef.current =
true/false to mirror it, and ensure onOpenChange (or a small useEffect that
watches open) also assigns openRef.current = open so Radix-driven updates stay
synced.
In `@fdm-app/app/routes/farm`.$b_id_farm.settings.access.tsx:
- Around line 138-172: The getFarm call inside the email-sending block is using
session.user.id which breaks org-principal flows; update the call to
getFarm(fdm, session.principal_id, b_id_farm) (same principal id used by
grantRoleToFarm) so permissions are correct and the invitation email will be
sent for organization principals; keep the rest of the logic unchanged and
ensure no other references to session.user.id are used for principal-scoped
operations in this block.
In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.access.tsx:
- Around line 183-215: The getFarm call uses the wrong identity so permission
checks fail when acting as an org principal; replace the incorrect
session.user.id (or any usage of session.user) passed into getFarm with the
existing principalId variable (session.principal_id) used by grantRoleToFarm so
getFarm(fdm, principalId, b_id_farm) is called; locate the getFarm invocation in
the try block around the email-sending flow and update the second argument to
principalId to ensure permission checks succeed and the invitation email is
sent.
In `@fdm-core/src/db/schema-authz.ts`:
- Around line 59-83: The invitation table currently allows both target_email and
target_principal_id to be NULL; add a DB-level CHECK constraint in the
fdmAuthZSchema.table callback (alongside the existing uniqueIndex entries for
invitation_unique_email_idx and invitation_unique_principal_idx) using
drizzle-orm's check() from drizzle-orm/pg-core that enforces (target_email IS
NOT NULL OR target_principal_id IS NOT NULL) so at least one target is set for
each invitation record.
In `@fdm-core/src/invitation.ts`:
- Around line 474-489: The decline path that checks invitation.target_email
currently only verifies the user's email string; update the same block in
invitation.ts (the else if handling invitation.target_email) to also select and
check authNSchema.user.email_verified from the tx query (alongside email),
require email_verified === true before allowing decline, and throw the same
verification-related error used in the accept flow when email is unverified;
keep the existing email lowercase/trim comparison and error flow otherwise.
In `@fdm-docs/docs/core-concepts/10-authorization.md`:
- Around line 41-48: The fenced code block showing the resource hierarchy
currently lacks a language identifier; update the triple-backtick fence around
the ASCII tree so it reads ```text (i.e., add the "text" language identifier to
the fenced block) to satisfy markdownlint MD040 and ensure the block is treated
as plain text.
---
Outside diff comments:
In `@fdm-app/app/components/custom/autocomplete.tsx`:
- Around line 68-73: The computed selectedLabel should fall back to the
committed free-form value when the component allows values outside the list;
update the useMemo that computes selectedLabel (the block referencing
selectedValue and items) to return selectedItem?.label ??
(allowValuesOutsideList ? selectedValue ?? "" : "") so that when
allowValuesOutsideList is true and the current selectedValue isn't in items the
displayed label equals the selectedValue; this will also make the
handleInputBlur comparisons and the useEffect restore logic behave consistently
for pre-populated free-form values.
- Around line 68-73: selectedLabel falls back to empty string when selectedValue
is a free-form entry, causing handleInputBlur's else-if to clear the input;
change the useMemo that defines selectedLabel to return selectedItem?.label ??
selectedValue (i.e., use selectedValue as a fallback when no matching item
exists) so free-form committed values display consistently (affects
selectedLabel, selectedValue, and downstream logic in handleInputBlur and the
related useEffect).
In `@fdm-core/src/principal.ts`:
- Around line 134-162: Update the stale JSDoc for identifyPrincipal to match its
actual return type Promise<Principal | undefined>: change the `@returns`
description to state it resolves to a Principal or undefined (not an array),
adjust the example to await identifyPrincipal and check for truthiness (e.g.,
const principal = await identifyPrincipal(...); if (principal) { /* use
principal */ } else { /* not found */ }), and remove any array-specific
references such as .length or [0]; keep the thrown-error and parameter docs
as-is.
---
Duplicate comments:
In `@fdm-core/src/authentication.ts`:
- Around line 229-244: No change required: the create hook now correctly guards
autoAcceptInvitationsForNewUser with a try/catch for user.emailVerified,
preventing throws from breaking signup; leave the block as-is
(autoAcceptInvitationsForNewUser(fdm, user.email, user.id) wrapped in try/catch
and console.warn) and no further modifications are necessary.
- Around line 247-264: Add a short-circuit so autoAcceptInvitationsForNewUser is
only invoked once per user: add a persistent flag on the user record (e.g.,
invitations_processed boolean or invitationsAcceptedAt timestamp) and check that
flag in the update.after hook before calling autoAcceptInvitationsForNewUser;
after a successful call set that flag on the user (update the User record) so
subsequent profile edits skip the transaction. Reference the update.after hook
where autoAcceptInvitationsForNewUser(fdm, user.email, user.id) is called and
ensure the flag is written only after successful completion to avoid
re-processing.
---
Nitpick comments:
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 332-433: The pending-invitation card markup (rendering
loaderData.pendingInvitations into Card elements using invitation.invitation_id,
CardHeader/CardContent/CardFooter and the accept_farm_invitation /
decline_farm_invitation Forms) is duplicated across the “no farms” and “has
farms” branches; extract this into a reusable PendingInvitations component that
accepts the invitations array and renders each Card (preserving keys, role
mapping, org_name display, hidden inputs for intent and invitation_id, and
Button variants) and replace both duplicated blocks with <PendingInvitations
invitations={loaderData.pendingInvitations} /> to keep behavior identical and
avoid future drift.
In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.access.tsx:
- Around line 182-215: Extract the duplicated invitation-email block into a
shared async helper (e.g., sendFarmInvitationEmailIfPossible) in
~/lib/email.server that takes the same inputs it needs (fdm, principalId,
b_id_farm, formValues, session) and encapsulates the logic currently in the try
block: call getFarm(b_id_farm), normalize formValues.username, detect isEmail
via isEmail(), lookupPrincipal(), resolve targetPrincipal and targetEmail,
renderFarmInvitationEmail(targetEmail, inviterName, farm.b_name_farm ??
b_id_farm, formValues.role, isUnregistered) and call sendEmail(email); preserve
the try/catch and logging behavior (console.error) inside the helper and then
replace both in-file blocks with a single await
sendFarmInvitationEmailIfPossible(...) call so fixes are applied in one place.
In `@fdm-core/src/db/schema-authz.ts`:
- Line 69: Change the plain text status column to a DB-enforced enum or add a
CHECK constraint: replace the current status:
text().notNull().default("pending") definition with a schema-scoped enum created
via pgSchema.enum("invitation_status",
["pending","accepted","declined","expired"]) and then use that enum for the
column (e.g., status: pgEnum("invitation_status").notNull().default("pending")),
or keep text() but add a check() constraint (e.g.,
check("invitation_status_check", sql`${table.status} IN
('pending','accepted','declined','expired')`)); update any imports to include
pgSchema/pgEnum or check/sql as needed and ensure the enum name and column
identifier match (status) across the schema.
In `@fdm-core/src/farm.ts`:
- Around line 1114-1118: The invitation delete currently only filters by
resource_id using
tx.delete(authZSchema.invitation).where(eq(authZSchema.invitation.resource_id,
b_id_farm)); tighten it to also filter by the resource type by adding a second
condition (e.g., eq(authZSchema.invitation.resource, "farm")) combined with the
existing predicate so only invitations for the farm resource with that ID are
removed; update the where clause used in the tx.delete call to include both
predicates (AND).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
fdm-docs/docs/core-concepts/10-authorization.md (1)
80-80: Minor: doc parameter nameprincipal_iddoesn't match the actual function signatureuser_id.The
listPendingInvitationsForPrincipalimplementation ininvitation.tsacceptsuser_id: string(only users, not arbitrary principals), but the API table exposesprincipal_id. The description "Lists all pending invitations for a user" already signals the correct intent — aligning the parameter name would prevent confusion.📘 Proposed fix
-| `listPendingInvitationsForPrincipal(fdm, principal_id)` | Lists all pending invitations for a user | +| `listPendingInvitationsForPrincipal(fdm, user_id)` | Lists all pending invitations for a user |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-docs/docs/core-concepts/10-authorization.md` at line 80, The docs use the parameter name `principal_id` for listPendingInvitationsForPrincipal but the actual implementation in invitation.ts accepts `user_id: string`; update the API table entry (the parameter column for listPendingInvitationsForPrincipal) to `user_id` to match the function signature and intent (only users), and scan for any other occurrences in the same doc to replace `principal_id` with `user_id` to keep names consistent with the invitation.ts function.fdm-core/src/invitation.ts (1)
300-301: Remove the redundant innerifcheck.Line 301 duplicates the outer guard on line 300 — both test
invitation.target_principal_id. The inner block should be dedented one level.♻️ Proposed fix
if (invitation.target_principal_id) { - if (invitation.target_principal_id) { - const orgMembership = await tx - .select() - .from(authNSchema.member) - ... - ... - } - + const orgMembership = await tx + .select() + .from(authNSchema.member) + ... + ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/invitation.ts` around lines 300 - 301, The code contains a duplicated guard: an outer if checking invitation.target_principal_id and an inner identical if (invitation.target_principal_id); remove the redundant inner if and dedent its body so the logic inside runs under the single outer guard (locate the block around invitation.target_principal_id in the invitation handling code and delete the inner conditional).fdm-core/src/db/schema-authz.ts (1)
60-91: LGTM —invitationtable schema is well-formed; the past CHECK-constraint concern is addressed.The partial unique indexes correctly scope duplicate-prevention to
status = 'pending', and theinvitation_target_checkconstraint enforces the at-least-one-target invariant at the DB level.One optional improvement: queries in
autoAcceptInvitationsForNewUserfilter solely bytarget_emailandstatus = 'pending'. The leading columns ofinvitation_unique_email_idxare(resource, resource_id, target_email), so the planner has to scan the full partial index for a by-email lookup. Consider a separate partial index on(target_email)WHEREstatus = 'pending'for that hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/db/schema-authz.ts` around lines 60 - 91, Queries in autoAcceptInvitationsForNewUser scan the partial unique index invitation_unique_email_idx whose leading columns are (resource, resource_id, target_email), causing inefficient lookups by target_email alone; add a new partial index (e.g. invitation_pending_target_email_idx) on table invitation for column target_email WHERE status = 'pending' (non-unique) to serve that hot path and speed up queries filtering by target_email and status; update the schema definition where invitation is declared to include this new index alongside invitation_unique_email_idx.fdm-core/src/farm.ts (3)
550-573: Initials computation is duplicated fromgetPrincipal.The surname-capital-letter logic (lines 550–573) is a near-exact copy of
getPrincipal(principal.ts lines 57–84). If the algorithm changes (e.g., handling diacritics), both locations must be updated in lockstep.Consider extracting a shared helper like
computeInitials(firstname, surname, email, name)and reusing it in bothgetPrincipalandlistPrincipalsForFarm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.ts` around lines 550 - 573, The initials computation in listPrincipalsForFarm duplicates the surname-capital-letter logic from getPrincipal; extract that logic into a shared helper (e.g., computeInitials(firstname, surname, email, name)) and replace the inlined code in both getPrincipal and listPrincipalsForFarm to call this helper; ensure the helper returns the same fallback order (email -> firstname+surname initials -> firstname[0] -> name[0]) and preserve the existing surname-splitting and uppercase-letter detection behavior so both locations remain consistent.
748-832:org_nameenrichment assumestarget_principal_idis always an organization — verify intent.Lines 792–813 collect all
target_principal_idvalues and look them up exclusively in theorganizationtable. If a pending invitation targets a user principal (not an org), the lookup simply yieldsnullfororg_name, which is benign. However, the variable names (orgTargetIds,orgNames) suggest the code expects only organization principals, while the data can contain user principals too.If the intention is to show the organization name only when the invitation target is an organization, this works correctly — but consider adding a brief comment clarifying the intent, since the variable naming is slightly misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.ts` around lines 748 - 832, The code in listPendingInvitationsForUser collects target_principal_id into orgTargetIds and looks them up only in authNSchema.organization (orgNames) which is ambiguous if invitations can target user principals; either clarify intent with a comment above the orgTargetIds/orgNames block stating that target_principal_id may be a user and org_name will be null for non-org targets, or if the intent is to resolve only organization principals, change the filter to only include invitations whose target principal type equals "organization" (or the appropriate enum) before building orgTargetIds and querying authNSchema.organization; update variable names (orgTargetIds/orgNames) or add the comment accordingly to remove ambiguity.
320-351:grantRoleToFarmno longer runs inside a transaction.
checkPermissionandcreateInvitationare two independent top-level calls againstfdm. There is a small TOCTOU window where the caller'ssharepermission could be revoked between the check (line 328) and the invitation creation (line 336). The practical risk is low because permission revocation is rare and the worst-case outcome is a single extra invitation, but it's worth noting.If
createInvitationalready opens its own transaction, wrapping both calls in one would require passing the transaction handle through. This is acceptable as-is.#!/bin/bash # Check if createInvitation opens its own transaction ast-grep --pattern 'async function createInvitation($$$) { $$$ fdm.transaction($$$) $$$ }'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.ts` around lines 320 - 351, The function grantRoleToFarm has a TOCTOU risk because checkPermission and createInvitation run outside a shared transaction; wrap both calls in a single fdm.transaction so the permission check and invitation creation are atomic. Modify grantRoleToFarm to call await fdm.transaction(async (trx) => { await checkPermission(fdm, "farm", "share", b_id_farm, principal_id, "grantRoleToFarm", trx); return await createInvitation(fdm, "farm", b_id_farm, principal_id, target, role, trx); }); and update checkPermission and createInvitation to accept an optional transaction parameter (trx) and use it for DB operations when provided; preserve the existing catch/handleError behavior.fdm-app/app/routes/farm._index.tsx (1)
332-456: Extract the invitation card into a shared component to eliminate ~120 lines of duplication.The invitation card rendering (icon, title, role label, description text, accept/decline forms) is duplicated nearly verbatim between the "no farms" view (lines 340–451) and the "has farms" view (lines 591–698). Extract it into a reusable
InvitationCardcomponent within this file or a separate component file.♻️ Sketch of extracted component
+function InvitationCard({ + invitation, +}: { + invitation: { + invitation_id: string + farm_name: string | null + resource_id: string + role: string + org_name: string | null + } +}) { + const roleLabel = + invitation.role === "owner" + ? "Eigenaar" + : invitation.role === "advisor" + ? "Adviseur" + : "Onderzoeker" + + return ( + <Card className="flex flex-col text-left"> + <CardHeader className="pb-2"> + <div className="flex items-center gap-3"> + <div className="flex h-10 w-10 items-center justify-center rounded-lg bg-amber-100 text-amber-600"> + <Bell className="h-5 w-5" /> + </div> + <div> + <CardTitle className="text-base"> + {invitation.farm_name ?? invitation.resource_id} + </CardTitle> + <CardDescription className="text-xs"> + Rol: {roleLabel} + </CardDescription> + </div> + </div> + </CardHeader> + <CardContent className="grow py-2 text-sm text-muted-foreground"> + Je hebt een uitnodiging ontvangen voor toegang tot bedrijf{" "} + {invitation.farm_name ?? invitation.resource_id} als {roleLabel}. + {invitation.org_name && ( + <span className="block text-xs text-muted-foreground"> + Deze uitnodiging ontvang je namens organisatie: {invitation.org_name} + </span> + )} + Je kunt deze uitnodiging accepteren of weigeren. + </CardContent> + <CardFooter className="flex gap-2 pt-2"> + <Form method="post" className="flex-1"> + <input type="hidden" name="intent" value="accept_farm_invitation" /> + <input type="hidden" name="invitation_id" value={invitation.invitation_id} /> + <Button type="submit" size="sm" className="w-full"> + <Check className="mr-1 h-3 w-3" /> + Accepteren + </Button> + </Form> + <Form method="post" className="flex-1"> + <input type="hidden" name="intent" value="decline_farm_invitation" /> + <input type="hidden" name="invitation_id" value={invitation.invitation_id} /> + <Button type="submit" size="sm" variant="outline" className="w-full"> + <X className="mr-1 h-3 w-3" /> + Weigeren + </Button> + </Form> + </CardFooter> + </Card> + ) +}Also applies to: 581-703
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm._index.tsx` around lines 332 - 456, The invitation card markup for loaderData.pendingInvitations is duplicated; extract it to a reusable InvitationCard component that accepts an invitation prop and renders the same structure (uses Card, CardHeader, CardTitle, CardDescription, CardContent, CardFooter, Bell, Check, X, and two Forms submitting intent + invitation_id). Replace both inline map renderings with loaderData.pendingInvitations.map(inv => <InvitationCard invitation={inv} />) (or equivalent), ensure the accept/decline forms keep their hidden inputs name="intent"/value ("accept_farm_invitation"/"decline_farm_invitation") and name="invitation_id"/value={invitation.invitation_id}, and export/import the component in this file (or a new file) so behavior and styling remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 358-364: The role label rendering for invitation.role currently
falls back to "Onderzoeker" and should match the farm card badge behavior (which
includes a fallback "Lid"); update the ternary that computes the label for
invitation.role so it maps "owner" => "Eigenaar", "advisor" => "Adviseur",
"researcher" => "Onderzoeker" and uses "Lid" as the default for any unknown
role; ideally extract this logic into a shared helper (e.g. getRoleLabel) and
use it both where invitation.role is rendered and in the farm card badge
rendering to keep labels consistent.
In `@fdm-core/src/farm.ts`:
- Around line 478-489: listPrincipalsForFarm currently returns duplicate
principals when a user has both an active role and a pending invitation; modify
listPrincipalsForFarm to deduplicate by principal_id: build a map keyed by
principal_id and for each entry prefer an "active" record over a "pending" one
(keep role from the chosen record, include invitation_id only for pending, and
set the status field to "active" or "pending"), then return the map values; this
ensures the returned (Principal & {role,status,invitation_id?})[] contains one
entry per principal and the UI will no longer see duplicates.
In `@fdm-core/src/invitation.ts`:
- Around line 630-666: Wrap the per-invitation grant+accept sequence in a
try/catch inside the pendingInvitations loop so a thrown error from
grantRole(tx, ...) (which uses tx.transaction/savepoints) does not abort the
outer fdm.transaction; specifically, around the call to grantRole(...) and the
following tx.update(...).set({ status: "accepted", ... }) for
authZSchema.invitation, catch the error, log or record the failure (e.g., set
status to "failed" or leave as "pending" per policy) and continue the loop; keep
the existing early expiry handling (the expires check and the update to status
"expired") unchanged.
---
Nitpick comments:
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 332-456: The invitation card markup for
loaderData.pendingInvitations is duplicated; extract it to a reusable
InvitationCard component that accepts an invitation prop and renders the same
structure (uses Card, CardHeader, CardTitle, CardDescription, CardContent,
CardFooter, Bell, Check, X, and two Forms submitting intent + invitation_id).
Replace both inline map renderings with loaderData.pendingInvitations.map(inv =>
<InvitationCard invitation={inv} />) (or equivalent), ensure the accept/decline
forms keep their hidden inputs name="intent"/value
("accept_farm_invitation"/"decline_farm_invitation") and
name="invitation_id"/value={invitation.invitation_id}, and export/import the
component in this file (or a new file) so behavior and styling remain identical.
In `@fdm-core/src/db/schema-authz.ts`:
- Around line 60-91: Queries in autoAcceptInvitationsForNewUser scan the partial
unique index invitation_unique_email_idx whose leading columns are (resource,
resource_id, target_email), causing inefficient lookups by target_email alone;
add a new partial index (e.g. invitation_pending_target_email_idx) on table
invitation for column target_email WHERE status = 'pending' (non-unique) to
serve that hot path and speed up queries filtering by target_email and status;
update the schema definition where invitation is declared to include this new
index alongside invitation_unique_email_idx.
In `@fdm-core/src/farm.ts`:
- Around line 550-573: The initials computation in listPrincipalsForFarm
duplicates the surname-capital-letter logic from getPrincipal; extract that
logic into a shared helper (e.g., computeInitials(firstname, surname, email,
name)) and replace the inlined code in both getPrincipal and
listPrincipalsForFarm to call this helper; ensure the helper returns the same
fallback order (email -> firstname+surname initials -> firstname[0] -> name[0])
and preserve the existing surname-splitting and uppercase-letter detection
behavior so both locations remain consistent.
- Around line 748-832: The code in listPendingInvitationsForUser collects
target_principal_id into orgTargetIds and looks them up only in
authNSchema.organization (orgNames) which is ambiguous if invitations can target
user principals; either clarify intent with a comment above the
orgTargetIds/orgNames block stating that target_principal_id may be a user and
org_name will be null for non-org targets, or if the intent is to resolve only
organization principals, change the filter to only include invitations whose
target principal type equals "organization" (or the appropriate enum) before
building orgTargetIds and querying authNSchema.organization; update variable
names (orgTargetIds/orgNames) or add the comment accordingly to remove
ambiguity.
- Around line 320-351: The function grantRoleToFarm has a TOCTOU risk because
checkPermission and createInvitation run outside a shared transaction; wrap both
calls in a single fdm.transaction so the permission check and invitation
creation are atomic. Modify grantRoleToFarm to call await fdm.transaction(async
(trx) => { await checkPermission(fdm, "farm", "share", b_id_farm, principal_id,
"grantRoleToFarm", trx); return await createInvitation(fdm, "farm", b_id_farm,
principal_id, target, role, trx); }); and update checkPermission and
createInvitation to accept an optional transaction parameter (trx) and use it
for DB operations when provided; preserve the existing catch/handleError
behavior.
In `@fdm-core/src/invitation.ts`:
- Around line 300-301: The code contains a duplicated guard: an outer if
checking invitation.target_principal_id and an inner identical if
(invitation.target_principal_id); remove the redundant inner if and dedent its
body so the logic inside runs under the single outer guard (locate the block
around invitation.target_principal_id in the invitation handling code and delete
the inner conditional).
In `@fdm-docs/docs/core-concepts/10-authorization.md`:
- Line 80: The docs use the parameter name `principal_id` for
listPendingInvitationsForPrincipal but the actual implementation in
invitation.ts accepts `user_id: string`; update the API table entry (the
parameter column for listPendingInvitationsForPrincipal) to `user_id` to match
the function signature and intent (only users), and scan for any other
occurrences in the same doc to replace `principal_id` with `user_id` to keep
names consistent with the invitation.ts function.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
fdm-app/app/components/custom/autocomplete.tsx (1)
59-59:openRefis written at everysetOpencall site but never read — safe to remove.The
handleInputBlurrewrite (lines 163–180) dropped the old!openRef.currentguard and now relies solely one.preventDefault()onCommandItem.onMouseDown. All fiveopenRef.current = …assignments remain but no code path ever readsopenRef.current. The ref is pure maintenance overhead.♻️ Proposed cleanup
- const openRef = useRef(open) ... const handleSelectItem = (itemValue: string) => { ... setOpen(false) - openRef.current = false } ... <Popover open={open} - onOpenChange={(value) => { - setOpen(value) - openRef.current = value - }} + onOpenChange={setOpen} > ... <CommandPrimitive.Input ... - onKeyDown={(e) => { - const next = e.key !== "Escape" - setOpen(next) - openRef.current = next - }} + onKeyDown={(e) => setOpen(e.key !== "Escape")} - onMouseDown={() => { - const next = !!inputValue || !open - setOpen(next) - openRef.current = next - }} + onMouseDown={() => setOpen(!!inputValue || !open)} - onFocus={() => { - setOpen(true) - openRef.current = true - }} + onFocus={() => setOpen(true)}Also applies to: 156-157, 186-189, 197-201, 202-206, 207-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/custom/autocomplete.tsx` at line 59, Remove the unused openRef ref and all assignments to it: delete the declaration const openRef = useRef(open) and remove every occurrence that sets openRef.current (the five assignments near the setOpen call sites). Update related code paths so they rely on the existing setOpen state and the new handleInputBlur behavior (do not reintroduce reads of openRef in handleInputBlur or CommandItem.onMouseDown); ensure no other code references openRef after the cleanup (search for openRef, openRef.current, and the specific assignment sites around setOpen, handleInputBlur, and CommandItem.onMouseDown and delete them).fdm-core/src/farm.ts (1)
682-692:as anycast in deduplication hides a type mismatch.The map is typed as
Map<string, (typeof principalsDetails)[number]>, butpendingDetailsentries don't match that type (they always carryinvitation_idwhileprincipalsDetailsentries do not). Theas anycast suppresses the error instead of aligning the types. Both arrays should be typed to the declared return element type before merging.♻️ Proposed fix
+type PrincipalWithRole = Principal & { + role: string + status: "active" | "pending" + invitation_id?: string +} -const deduped = new Map<string, (typeof principalsDetails)[number]>() +const deduped = new Map<string, PrincipalWithRole>() for (const entry of principalsDetails) { deduped.set(entry.id, entry) } for (const entry of pendingDetails) { if (!deduped.has(entry.id)) { - deduped.set(entry.id, entry as any) + deduped.set(entry.id, entry) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.ts` around lines 682 - 692, The deduplication uses Map<string, (typeof principalsDetails)[number]> but merges entries from principalsDetails and pendingDetails which have different shapes (pendingDetails entries include invitation_id), and the existing as any cast hides this mismatch; fix by defining a single element type (e.g., PrincipalDetailUnion or a common minimal interface used as the Map value) that both principalsDetails and pendingDetails are converted/mapped to before inserting into the Map, or explicitly map pendingDetails entries to the same shape as principalsDetails (dropping or optionalizing invitation_id) so you can remove the as any and then dedupe via the deduped Map (keep references to deduped, principalsDetails, pendingDetails to locate code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 155-158: The catch block in farm._index.tsx currently replaces all
errors with a generic message; update the catch in the action handler that calls
acceptInvitation/declineInvitation so it returns the original error message
instead (e.g., use error?.message || String(error) as the message passed to
dataWithError) and still log the full error to console; refer to the catch
surrounding the acceptInvitation/declineInvitation calls to locate and change
the return from dataWithError(null, "Er is iets misgegaan") to include the
actual error text.
- Around line 339-451: The pending-invitation Card JSX is duplicated in two
places (the loaderData.pendingInvitations.map blocks); extract that markup into
a reusable component (e.g., PendingInvitationCard) that accepts a single prop
invitation and encapsulates the Card, CardHeader/Content/Footer, the two Forms
with hidden inputs ("intent" and "invitation_id"), and the icons (Bell, Check,
X) and Dutch copy; then replace both duplicated map blocks with
loaderData.pendingInvitations.map(inv => <PendingInvitationCard
key={inv.invitation_id} invitation={inv} />) so layout, labels and form behavior
remain identical and updates are centralized.
In `@fdm-core/src/farm.ts`:
- Around line 550-579: The initials fallback currently sets initials = u.email
(full email) causing full address to be used; update the fallback logic in the
loop that builds principalsMap (the block using variables initials, u.firstname,
u.surname, u.name and calling principalsMap.set) so that when no
firstname/surname/name exist you set initials to the first character of u.email
(e.g., u.email.charAt(0) or equivalent) (and still call .toUpperCase()
downstream) — this mirrors the behavior of the previous getPrincipal helper and
ensures initials are a single letter.
- Around line 328-343: The permission check and invitation creation in
grantRoleToFarm are not atomic: checkPermission(...) and createInvitation(...)
run separately allowing a TOCTOU race; wrap both operations in a single
fdm.transaction so the share permission check and the call to
createInvitation("farm", b_id_farm, principal_id, target, role) execute in the
same transaction context (pass the transaction/context into checkPermission and
createInvitation if they accept it or call their transactional variants),
preserving the original atomic behavior.
In `@fdm-core/src/invitation.ts`:
- Around line 229-236: The error context for createInvitation is leaking PII by
passing the raw target (email) into handleError; remove target from the context
and instead include a non-PII identifier such as target_user_id (if present) or
a redacted indicator (e.g., "target_redacted": true) when calling handleError in
the createInvitation catch block so logs/error-tracking never contain the
plaintext email; update the call that currently references resource,
resource_id, target, role to use resource, resource_id, role and target_user_id
(or a redacted flag) instead.
In `@fdm-docs/docs/core-concepts/10-authorization.md`:
- Line 77: The API table entry for createInvitation omits the optional expires
parameter; update the table row for `createInvitation(fdm, resource,
resource_id, inviter_id, target, role)` to reflect the actual signature
`createInvitation(fdm, resource, resource_id, inviter_id, target, role,
expires?)` and note that `expires` is optional and defaults to 7 days so callers
know they can override the expiry when calling `createInvitation`.
---
Duplicate comments:
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 52-57: getRoleLabel currently maps known roles and returns "Lid"
as a fallback; ensure all call sites use this helper (e.g., where active farm
roles and pending invitation roles are rendered) by replacing any inline
role-to-label logic with calls to getRoleLabel(role) so both active roles and
pending invitations consistently display the same labels.
In `@fdm-docs/docs/core-concepts/10-authorization.md`:
- Around line 41-48: The markdown code block in 10-authorization.md now includes
the language identifier ```text which resolves the MD040 lint warning; no code
changes required—confirm the code block around the farm/field tree uses ```text
and leave it as-is to keep the markdownlint warning cleared.
---
Nitpick comments:
In `@fdm-app/app/components/custom/autocomplete.tsx`:
- Line 59: Remove the unused openRef ref and all assignments to it: delete the
declaration const openRef = useRef(open) and remove every occurrence that sets
openRef.current (the five assignments near the setOpen call sites). Update
related code paths so they rely on the existing setOpen state and the new
handleInputBlur behavior (do not reintroduce reads of openRef in handleInputBlur
or CommandItem.onMouseDown); ensure no other code references openRef after the
cleanup (search for openRef, openRef.current, and the specific assignment sites
around setOpen, handleInputBlur, and CommandItem.onMouseDown and delete them).
In `@fdm-core/src/farm.ts`:
- Around line 682-692: The deduplication uses Map<string, (typeof
principalsDetails)[number]> but merges entries from principalsDetails and
pendingDetails which have different shapes (pendingDetails entries include
invitation_id), and the existing as any cast hides this mismatch; fix by
defining a single element type (e.g., PrincipalDetailUnion or a common minimal
interface used as the Map value) that both principalsDetails and pendingDetails
are converted/mapped to before inserting into the Map, or explicitly map
pendingDetails entries to the same shape as principalsDetails (dropping or
optionalizing invitation_id) so you can remove the as any and then dedupe via
the deduped Map (keep references to deduped, principalsDetails, pendingDetails
to locate code).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.access.tsx:
- Around line 183-224: The email error handler currently calls
revokePrincipalFromFarm unconditionally and will throw "Target not found" for
email-only invitees; change the catch block to only call revokePrincipalFromFarm
when a registered principal was resolved (use the earlier
matchedPrincipals/targetPrincipal or check that targetPrincipal?.type === "user"
/ has an id) and skip revocation for email-only targets so the original
isInactiveRecipientError is preserved and pending invitations remain; ensure the
original emailError is returned via dataWithError when skipping revocation.
---
Duplicate comments:
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 155-159: The catch block currently swallows detailed invitation
errors and always returns a generic message; update the catch to inspect the
caught error (e.g., check error.message, error.code, or a custom InvitationError
predicate) and if it is an invitation-related error return dataWithError(null,
error.message) (or a mapped, user-friendly message), otherwise fall back to the
generic "Er is iets misgegaan"; ensure you reference the same catch handling and
the dataWithError helper so actionable invitation errors are surfaced to users.
In `@fdm-app/app/routes/farm`.$b_id_farm.settings.access.tsx:
- Around line 139-185: The catch block may call revokePrincipalFromFarm for an
invite that was sent to an email-only target (no registered principal), causing
a "Target not found" error; update the error-handling in the try/catch around
sendEmail in the farm.$b_id_farm.settings.access route to only call
revokePrincipalFromFarm when a principal was actually resolved (i.e., check that
targetPrincipal is truthy and not email-only) — use the existing local symbols
targetPrincipal and isEmailTarget to guard the revokePrincipalFromFarm(fdm,
session.principal_id, formValues.username, b_id_farm) call and otherwise skip
revocation (or return the same user-facing error) so we don’t attempt to revoke
a non-existent principal.
…to preserve error context
gerardhros
reviewed
Feb 23, 2026
gerardhros
reviewed
Feb 23, 2026
gerardhros
reviewed
Feb 23, 2026
gerardhros
reviewed
Feb 23, 2026
…tion from dropdown
gerardhros
reviewed
Feb 23, 2026
| * Enforced in {@link createInvitation} to prevent inbox flooding. | ||
| */ | ||
| export const MAX_INVITATIONS_PENDING_PER_TARGET = 10 | ||
|
|
Collaborator
There was a problem hiding this comment.
for now fine. on organization level this might be changed later.
Collaborator
|
check the automatic checks |
This was referenced Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Emails
Bug Fixes / Reliability
Tests
Closes #460