Add pending invitation cancellation and role update operations#477
Merged
Conversation
…lear the user is invited by showing the expiring date
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:
📝 WalkthroughWalkthroughAdds two invitation operations to fdm-core (cancelInvitationForFarm, updateRoleOfInvitationForFarm), surfaces invitation metadata through app UI, adds email templates and renderers for cancellation/role-update, and updates route handlers to branch on invitation_id for invitation-specific flows. Changes
Sequence DiagramsequenceDiagram
participant User as User (Client)
participant Route as Route Handler
participant FdmCore as FdmCore
participant DB as Database
participant Email as Email Renderer/Service
User->>Route: Submit role update or remove (may include invitation_id)
Route->>Route: Detect invitation_id in form data
Route->>FdmCore: updateRoleOfInvitationForFarm(...) / cancelInvitationForFarm(...)
FdmCore->>DB: Fetch & validate pending invitation
DB-->>FdmCore: Invitation record
FdmCore->>DB: Update invitation (role or status)
DB-->>FdmCore: Success
FdmCore-->>Route: Operation complete
Route->>Email: renderFarmInvitationRoleUpdatedEmail(...) / renderFarmInvitationCancelledEmail(...)
Email-->>Route: Email object
Route->>Email: Send email (log failures, non-fatal)
Email-->>Route: Send result
Route->>User: Return success response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
coderabbitai
Bot
changed the title
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-app/app/components/blocks/farm/pending-invitation.tsx (1)
15-40:⚠️ Potential issue | 🟠 MajorFix expiry calculation — it’s currently hard-coded.
The expiry text always uses a static timestamp, so every invitation shows the same countdown. Use the invitation’s actual expiry value (and normalize string/Date).🐛 Suggested fix
type PendingInvitation = { invitation_id: string resource_id: string role: string farm_name: string | null org_name: string | null - expires: Date + expires: Date | string } export function PendingInvitationCard({ invitation }: Props) { const farmLabel = invitation.farm_name ?? invitation.resource_id - const expiresText = formatDistanceToNow(new Date("2026-02-24 12:30:00"), { + const expiresText = formatDistanceToNow(new Date(invitation.expires), { addSuffix: true, locale: nl, })Also applies to: 67-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/farm/pending-invitation.tsx` around lines 15 - 40, The expiry text is using a hard-coded timestamp; update the calculation in PendingInvitationCard (the expiresText variable) to use the invitation.expires value instead: normalize invitation.expires to a Date (e.g., if it's a string, wrap with new Date(invitation.expires) or check instanceof Date) and pass that Date into formatDistanceToNow(...). Replace any other occurrences doing the same hard-coded Date (see the other expiresText usage) so all invitation expiry displays use the actual invitation.expires value.
🧹 Nitpick comments (2)
fdm-core/src/farm.ts (1)
893-947: Significant duplication betweencancelInvitationForFarmandupdateRoleOfInvitationForFarm.Both functions share identical invitation lookup + status validation + permission check logic (lines 900–933 and 967–1001). Consider extracting a shared helper like
getAndValidatePendingFarmInvitation(tx, invitation_id, principal_id)that returns the validated invitation row.♻️ Proposed helper extraction
+/** Fetches a pending farm invitation, validates its status, and checks share permission. */ +async function getValidatedPendingFarmInvitation( + tx: FdmType, + invitation_id: string, + principal_id: PrincipalId, + callerName: string, +): Promise<authZSchema.invitationTypeSelect> { + const invitations = await tx + .select() + .from(authZSchema.invitation) + .where( + and( + eq(authZSchema.invitation.invitation_id, invitation_id), + eq(authZSchema.invitation.resource, "farm"), + ), + ) + .limit(1) + + if (invitations.length === 0) { + throw new Error("Invitation not found") + } + + if (invitations[0].status !== "pending") { + throw new Error(`Invitation is already ${invitations[0].status}`) + } + + await checkPermission( + tx, + "farm", + "share", + invitations[0].resource_id, + principal_id, + callerName, + ) + + return invitations[0] +}Also applies to: 960-1016
🤖 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 893 - 947, Extract a shared helper getAndValidatePendingFarmInvitation(tx, invitation_id, principal_id) that performs the invitation lookup (select from authZSchema.invitation by invitation_id and resource "farm"), throws the same errors if not found or not "pending", runs checkPermission(tx, "farm", "share", resource_id, principal_id, "getAndValidatePendingFarmInvitation") and returns the validated invitation row; then replace the duplicated lookup/validation/permission blocks inside cancelInvitationForFarm and updateRoleOfInvitationForFarm to call this helper and use its returned invitation (b_id_farm/resource_id) before proceeding with the update logic, preserving existing error handling via handleError.fdm-app/app/routes/farm.$b_id_farm.settings.access.tsx (1)
236-283: Significant duplication of target email resolution logic across intents and route files.The pattern of normalizing the target, checking
isEmail, callinglookupPrincipal, finding a match, and resolvingtargetEmailis repeated verbatim ininvite_user(lines 148-163),update_role(lines 247-259), andremove_user(lines 300-312) — and again infarm.create.$b_id_farm.$calendar.access.tsx. Consider extracting a helper likeresolveTargetEmail(fdm, username)to reduce duplication and make future changes less error-prone.♻️ Example helper
// e.g. in ~/lib/principal-email.server.ts import { lookupPrincipal } from "@nmi-agro/fdm-core" import isEmail from "validator/lib/isEmail" export async function resolveTargetEmail( fdm: any, username: string, ): Promise<{ email: string | null; principal: any | null }> { const normalizedTarget = username.toLowerCase().trim() const isEmailTarget = isEmail(normalizedTarget) const matchedPrincipals = await lookupPrincipal(fdm, normalizedTarget) const targetPrincipal = matchedPrincipals.find( (p) => p.username.toLowerCase() === normalizedTarget || (isEmailTarget && p.email?.toLowerCase() === normalizedTarget), ) const email = isEmailTarget ? normalizedTarget : targetPrincipal?.type === "user" ? targetPrincipal.email : null return { email, principal: targetPrincipal } }🤖 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 236 - 283, The code duplicates the "normalize target → isEmail → lookupPrincipal → find match → resolve targetEmail" logic across invite_user, update_role, remove_user intents and this route; extract a single helper (e.g. resolveTargetEmail(fdm, username): Promise<{ email: string|null; principal:any|null }>) that performs normalization (toLowerCase().trim()), uses validator's isEmail, calls lookupPrincipal, finds the matching principal, and returns email/principal; import and call resolveTargetEmail wherever the duplicated block appears (including before calling renderFarmInvitationRoleUpdatedEmail/sendEmail in this route after updateRoleOfInvitationForFarm and in code paths that send email), and keep the existing guard that only renders/sends email when email is non-null.
🤖 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/modern-years-win.md:
- Line 5: The changeset description contains a grammar error ("to enabling");
update the sentence to read clearly, e.g. "Add functions cancelInvitationForFarm
and updateRoleOfInvitationForFarm to enable managing invitations" (or "…for
managing invitations") so that the function names cancelInvitationForFarm and
updateRoleOfInvitationForFarm are referenced with correct verb form.
In @.changeset/thirty-chicken-tease.md:
- Line 5: Fix the grammar in the changeset description by replacing the phrase
"an user" with "a user" and rephrase for clarity so the sentence reads something
like "In the access list of farms, make it clear that if a user is invited they
can enable managing invitations" (edit the changeset description in the
.changeset entry to this clearer wording).
In `@fdm-core/src/farm.ts`:
- Around line 914-922: Both cancelInvitationForFarm and
updateRoleOfInvitationForFarm currently only check invitations[0].status ===
"pending" but do not verify the invitation expiry; add a guard that verifies the
invitation's expires timestamp is in the future (e.g., expires > now) for the
selected invitation (invitations[0]) and throw an error like "Invitation
expired" if it is not, ensuring behavior matches
listPrincipalsForFarm/listPendingInvitationsForFarm which filter by expiry;
update the same check location that currently throws "Invitation not found" /
"Invitation is already ..." to include this expiry check before allowing cancel
or role update.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/farm/pending-invitation.tsx`:
- Around line 15-40: The expiry text is using a hard-coded timestamp; update the
calculation in PendingInvitationCard (the expiresText variable) to use the
invitation.expires value instead: normalize invitation.expires to a Date (e.g.,
if it's a string, wrap with new Date(invitation.expires) or check instanceof
Date) and pass that Date into formatDistanceToNow(...). Replace any other
occurrences doing the same hard-coded Date (see the other expiresText usage) so
all invitation expiry displays use the actual invitation.expires value.
---
Nitpick comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.settings.access.tsx:
- Around line 236-283: The code duplicates the "normalize target → isEmail →
lookupPrincipal → find match → resolve targetEmail" logic across invite_user,
update_role, remove_user intents and this route; extract a single helper (e.g.
resolveTargetEmail(fdm, username): Promise<{ email: string|null;
principal:any|null }>) that performs normalization (toLowerCase().trim()), uses
validator's isEmail, calls lookupPrincipal, finds the matching principal, and
returns email/principal; import and call resolveTargetEmail wherever the
duplicated block appears (including before calling
renderFarmInvitationRoleUpdatedEmail/sendEmail in this route after
updateRoleOfInvitationForFarm and in code paths that send email), and keep the
existing guard that only renders/sends email when email is non-null.
In `@fdm-core/src/farm.ts`:
- Around line 893-947: Extract a shared helper
getAndValidatePendingFarmInvitation(tx, invitation_id, principal_id) that
performs the invitation lookup (select from authZSchema.invitation by
invitation_id and resource "farm"), throws the same errors if not found or not
"pending", runs checkPermission(tx, "farm", "share", resource_id, principal_id,
"getAndValidatePendingFarmInvitation") and returns the validated invitation row;
then replace the duplicated lookup/validation/permission blocks inside
cancelInvitationForFarm and updateRoleOfInvitationForFarm to call this helper
and use its returned invitation (b_id_farm/resource_id) before proceeding with
the update logic, preserving existing error handling via handleError.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.changeset/modern-years-win.md.changeset/thirty-chicken-tease.mdfdm-app/app/components/blocks/access/access-management-card.tsxfdm-app/app/components/blocks/access/invitation-form.tsxfdm-app/app/components/blocks/access/principal-row.tsxfdm-app/app/components/blocks/email/farm-invitation-cancelled.tsxfdm-app/app/components/blocks/email/farm-invitation-role-updated.tsxfdm-app/app/components/blocks/farm/pending-invitation.tsxfdm-app/app/components/custom/autocomplete.tsxfdm-app/app/lib/email.server.tsfdm-app/app/routes/farm.$b_id_farm.settings.access.tsxfdm-app/app/routes/farm._index.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.access.tsxfdm-core/src/farm.test.tsfdm-core/src/farm.tsfdm-core/src/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-core/src/farm.ts`:
- Around line 917-919: The expiry boundary is inconsistent with
listPendingInvitationsForFarm (which uses expires > now), so change the guard in
the mutation helper from “if (invitation.expires < new Date())” to block when
expires is less-than-or-equal-to now; update the check to “if
(invitation.expires <= now)” (or compare getTime() values) so invitations with
expires === now cannot be mutated, and use the same now reference/precision as
listPendingInvitationsForFarm to avoid off-by-millisecond mismatches.
- Around line 951-964: The current cancelInvitationForFarm (and similarly
updateRoleOfInvitationForFarm) validates with
getAndValidatePendingFarmInvitation() but then blindly runs
tx.update(authZSchema.invitation).set(...).where(eq(authZSchema.invitation.invitation_id,
invitation_id)), which can overwrite concurrent changes; modify the UPDATE to
include a status guard (e.g., AND status = 'pending') in the WHERE so it only
updates if still pending, then check the update result to ensure a row was
affected (throw a concurrency/error if 0 rows updated). Update both
cancelInvitationForFarm and updateRoleOfInvitationForFarm and reference the same
authZSchema.invitation update call and getAndValidatePendingFarmInvitation()
validation to implement this guard-and-verify behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/modern-years-win.md.changeset/thirty-chicken-tease.mdfdm-app/app/components/blocks/farm/pending-invitation.tsxfdm-core/src/farm.test.tsfdm-core/src/farm.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- fdm-app/app/components/blocks/farm/pending-invitation.tsx
- .changeset/thirty-chicken-tease.md
- .changeset/modern-years-win.md
Collaborator
BoraIneviNMI
left a comment
BoraIneviNMI
left a comment
There was a problem hiding this comment.
I tried to test it to the best of my ability. I found a few bugs possibly more related to #470 than this. I also made a couple suggestions.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fdm-app/app/components/blocks/access/principal-row.tsx (1)
29-31: Consider a discriminated union to enforceinvitation_idis required whenstatus === "pending".As-is, a caller can pass
status: "pending"withoutinvitation_id. In that case, bothhandleRemoveandhandleSelectChangeomitinvitation_idfrom the fetcher payload, causing the route handler to fall through to the regular user path — silently executing the wrong operation on a pending invitation. A discriminated union eliminates this at compile time:♻️ Proposed type refactor
-type PrincipalRowProps = { - username: string - displayUserName: string | null - image?: string | null - initials: string - role: "owner" | "advisor" | "researcher" - type: "user" | "organization" - status: "active" | "pending" - invitation_id?: string - invitation_expires_at?: Date | string - hasSharePermission: boolean -} +type ActivePrincipalRowProps = { + username: string + displayUserName: string | null + image?: string | null + initials: string + role: "owner" | "advisor" | "researcher" + type: "user" | "organization" + status: "active" + hasSharePermission: boolean +} + +type PendingPrincipalRowProps = { + username: string + displayUserName: string | null + image?: string | null + initials: string + role: "owner" | "advisor" | "researcher" + type: "user" | "organization" + status: "pending" + invitation_id: string // required for pending + invitation_expires_at?: Date | string + hasSharePermission: boolean +} + +type PrincipalRowProps = ActivePrincipalRowProps | PendingPrincipalRowProps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/access/principal-row.tsx` around lines 29 - 31, Change the props/type that currently declare "status: 'active' | 'pending'; invitation_id?: string; invitation_expires_at?: Date | string" into a discriminated union so that when status === "pending" invitation_id is required (e.g. type Principal = { status: "active"; invitation_id?: never; invitation_expires_at?: Date | string } | { status: "pending"; invitation_id: string; invitation_expires_at?: Date | string }). Update the component/prop annotation in principal-row.tsx to use this new union and adjust any call sites accordingly, and then ensure handleRemove and handleSelectChange always include invitation_id in the fetcher payload when status === "pending" (use the narrowed type to access invitation_id without optional checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-app/app/components/blocks/access/principal-row.tsx`:
- Around line 29-31: Change the props/type that currently declare "status:
'active' | 'pending'; invitation_id?: string; invitation_expires_at?: Date |
string" into a discriminated union so that when status === "pending"
invitation_id is required (e.g. type Principal = { status: "active";
invitation_id?: never; invitation_expires_at?: Date | string } | { status:
"pending"; invitation_id: string; invitation_expires_at?: Date | string }).
Update the component/prop annotation in principal-row.tsx to use this new union
and adjust any call sites accordingly, and then ensure handleRemove and
handleSelectChange always include invitation_id in the fetcher payload when
status === "pending" (use the narrowed type to access invitation_id without
optional checks).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fdm-app/app/components/blocks/access/invitation-form.tsxfdm-app/app/components/blocks/access/principal-row.tsxfdm-app/app/components/custom/autocomplete.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-app/app/components/custom/autocomplete.tsx
- fdm-app/app/components/blocks/access/invitation-form.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
fdm-app/app/components/custom/autocomplete.tsx (1)
185-224: Consider guardingPopover'sopenstate againstdisabled.The
disabledprop is correctly forwarded to the DOMInput, so user-driven interactions (focus, keyboard, mouse) are naturally blocked. However, thePopover'sopenstate has no explicit guard. In a disabled+pre-populated scenario, the sync effect (lines 124–135) can still updateinputValue, which then flows into the debounce effect and could setopenindirectly if a caller ever callssetOpen(true). Being explicit is more defensive:♻️ Proposed guard on `open` prop
- <Popover - open={open} - onOpenChange={(value) => { - setOpen(value) - openRef.current = value - }} - > + <Popover + open={!disabled && open} + onOpenChange={(value) => { + if (disabled) return + setOpen(value) + openRef.current = value + }} + >🤖 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 185 - 224, The Popover's open state must be guarded by the disabled prop: compute an effectiveOpen (e.g., const effectiveOpen = disabled ? false : open) and pass that to <Popover open={...}> and update the onOpenChange handler to ignore attempts to open when disabled (only allow closing). Also ensure all places that call setOpen(true) or update openRef (onKeyDown, onMouseDown, onFocus, and any effects that call setOpen) check disabled first and avoid setting true when disabled so the component never opens while disabled; keep closing behavior allowed.fdm-app/app/routes/farm.$b_id_farm.settings.access.tsx (1)
260-272: Consider extracting the email-target resolution logic into a shared helper.The block that resolves
normalizedTarget → lookupPrincipal → targetEmail(lines 260–272) is an exact copy of lines 146–161 in theinvite_userbranch. A small shared utility (e.g.,resolveEmailForPrincipal(fdm, username)) would eliminate the duplication and make both call-sites easier to maintain.♻️ Suggested helper extraction
+// e.g. in ~/lib/farm.server.ts or at the top of this file +async function resolveEmailForPrincipal( + fdm: FdmType, + username: string, +): Promise<{ targetEmail: string | null; targetPrincipal: any | null }> { + const normalized = username.toLowerCase().trim() + const isEmailTarget = isEmail(normalized) + const matched = await lookupPrincipal(fdm, normalized) + const principal = matched.find( + (p) => + p.username.toLowerCase() === normalized || + (isEmailTarget && p.email?.toLowerCase() === normalized), + ) + const targetEmail = isEmailTarget + ? normalized + : principal?.type === "user" + ? principal.email + : null + return { targetEmail, targetPrincipal: principal ?? null } +}Then both branches become:
-const normalizedTarget = formValues.username.toLowerCase().trim() -const isEmailTarget = isEmail(normalizedTarget) -const matchedPrincipals = await lookupPrincipal(fdm, normalizedTarget) -const targetPrincipal = matchedPrincipals.find(...) -const targetEmail = isEmailTarget - ? normalizedTarget - : targetPrincipal?.type === "user" - ? targetPrincipal.email - : null +const { targetEmail, targetPrincipal } = await resolveEmailForPrincipal(fdm, formValues.username)🤖 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 260 - 272, Extract the duplicated resolution logic (normalizing formValues.username, checking isEmail, calling lookupPrincipal, finding targetPrincipal and deriving targetEmail) into a shared helper like resolveEmailForPrincipal(fdm, username) that returns { targetEmail, targetPrincipal } (or at least targetEmail); replace the inline block using normalizedTarget, isEmailTarget, lookupPrincipal, targetPrincipal and targetEmail in this file and the invite_user branch with a call to resolveEmailForPrincipal(fdm, formValues.username) to remove duplication and centralize behavior.fdm-app/app/components/blocks/access/principal-row.tsx (2)
135-143: Duplicate role-label mapping — extract a helper.The role→Dutch label mapping (
owner→"Eigenaar", etc.) appears three times in this file (here on Line 137, thehasSharePermission=falsebadge on Line 188, and implicitly mirrored in theSelectItemlabels). A small helper eliminates all copies:♻️ Proposed refactor
+ const roleLabel = (r: "owner" | "advisor" | "researcher" | string) => { + if (r === "owner") return "Eigenaar" + if (r === "advisor") return "Adviseur" + if (r === "researcher") return "Onderzoeker" + return "Onbekend" + }Then replace all three call sites:
- {role === "owner" - ? "Eigenaar" - : role === "advisor" - ? "Adviseur" - : role === "researcher" - ? "Onderzoeker" - : "Onbekend"} + {roleLabel(role)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/access/principal-row.tsx` around lines 135 - 143, Extract the repeated role→Dutch label mapping into a single helper (e.g., mapRoleToDutchLabel or getRoleLabel) inside the principal-row component file and use it everywhere: replace the inline ternary at the isPending Badge (current snippet), the Badge when hasSharePermission is false, and the SelectItem label definitions so all three sites call the new helper (accepting the role string and returning "Eigenaar"/"Adviseur"/"Onderzoeker"/"Onbekend"). Ensure the helper is exported/visible to the SelectItem usage if it's outside the component scope and update all call sites to pass the same role value.
60-60: Remove unnecessaryasyncfrom both handlers.Neither
handleRemovenorhandleSelectChangeusesawait, so theasyncmodifier is noise.♻️ Proposed fix
- const handleRemove = async (event: React.MouseEvent<HTMLButtonElement>) => { + const handleRemove = (event: React.MouseEvent<HTMLButtonElement>) => {- const handleSelectChange = async (value: string) => { + const handleSelectChange = (value: string) => {Also applies to: 73-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/access/principal-row.tsx` at line 60, Remove the unnecessary async modifiers from the event handlers that don't use await: update the function declarations for handleRemove and handleSelectChange in principal-row.tsx to be regular synchronous functions (remove "async" from "const handleRemove = async (event: ...)" and from "const handleSelectChange = async (event: ...)" ), leaving their bodies and signatures otherwise unchanged so behavior and types remain the same.
🤖 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`.$b_id_farm.settings.access.tsx:
- Around line 292-294: The success message currently uses dataWithSuccess(..., {
message: `Gebruiker ${formValues.username} is verwijderd` }) for both cancelling
invitations and revoking full members; change the handler that calls
dataWithSuccess to choose the message based on whether the target was a pending
invitation or an actual member (e.g., inspect a flag from formValues or the
fetched target like formValues.isInvitation or target.isPendingInvite) and call
dataWithSuccess with either `Uitnodiging voor ${formValues.username} is
geannuleerd` (cancellation) or `Gebruiker ${formValues.username} is verwijderd`
(revocation) so the user-facing text correctly distinguishes the two paths.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/access/principal-row.tsx`:
- Around line 135-143: Extract the repeated role→Dutch label mapping into a
single helper (e.g., mapRoleToDutchLabel or getRoleLabel) inside the
principal-row component file and use it everywhere: replace the inline ternary
at the isPending Badge (current snippet), the Badge when hasSharePermission is
false, and the SelectItem label definitions so all three sites call the new
helper (accepting the role string and returning
"Eigenaar"/"Adviseur"/"Onderzoeker"/"Onbekend"). Ensure the helper is
exported/visible to the SelectItem usage if it's outside the component scope and
update all call sites to pass the same role value.
- Line 60: Remove the unnecessary async modifiers from the event handlers that
don't use await: update the function declarations for handleRemove and
handleSelectChange in principal-row.tsx to be regular synchronous functions
(remove "async" from "const handleRemove = async (event: ...)" and from "const
handleSelectChange = async (event: ...)" ), leaving their bodies and signatures
otherwise unchanged so behavior and types remain the same.
In `@fdm-app/app/components/custom/autocomplete.tsx`:
- Around line 185-224: The Popover's open state must be guarded by the disabled
prop: compute an effectiveOpen (e.g., const effectiveOpen = disabled ? false :
open) and pass that to <Popover open={...}> and update the onOpenChange handler
to ignore attempts to open when disabled (only allow closing). Also ensure all
places that call setOpen(true) or update openRef (onKeyDown, onMouseDown,
onFocus, and any effects that call setOpen) check disabled first and avoid
setting true when disabled so the component never opens while disabled; keep
closing behavior allowed.
In `@fdm-app/app/routes/farm`.$b_id_farm.settings.access.tsx:
- Around line 260-272: Extract the duplicated resolution logic (normalizing
formValues.username, checking isEmail, calling lookupPrincipal, finding
targetPrincipal and deriving targetEmail) into a shared helper like
resolveEmailForPrincipal(fdm, username) that returns { targetEmail,
targetPrincipal } (or at least targetEmail); replace the inline block using
normalizedTarget, isEmailTarget, lookupPrincipal, targetPrincipal and
targetEmail in this file and the invite_user branch with a call to
resolveEmailForPrincipal(fdm, formValues.username) to remove duplication and
centralize behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
fdm-app/app/components/blocks/access/principal-row.tsxfdm-app/app/components/custom/autocomplete.tsxfdm-app/app/routes/farm.$b_id_farm.settings.access.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.access.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/routes/farm.create.$b_id_farm.$calendar.access.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
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
2 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