Modernize Organization UX using Subpages and Dedicated Sidebar#426
Merged
Conversation
🦋 Changeset detectedLatest commit: 959fbb4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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:
📝 WalkthroughWalkthroughAdds nested organization subpages and a dedicated organization sidebar; organization settings form and Zod schema; organization-scoped farms table with fuzzy search; new cards/components (OrganizationCard, FarmCard, pending-invitation cards, user avatar group); pending-invitation flows; and core role/authorization shape changed from string[] to structured role objects. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (User)
participant FormUI as OrganizationSettingsForm (Client)
participant RouteAction as Route Action (Server)
participant AuthAPI as Auth API (Service)
participant DB as Database (Persistence)
rect rgba(135,206,250,0.5)
Browser->>FormUI: fill form & submit
FormUI->>RouteAction: POST form (intent/update)
RouteAction->>RouteAction: validate via FormSchema (zod)
alt slug changed
RouteAction->>AuthAPI: checkOrganizationSlug(slug)
AuthAPI-->>RouteAction: available / SLUG_IS_TAKEN
end
RouteAction->>AuthAPI: updateOrganization(payload)
AuthAPI->>DB: persist changes
AuthAPI-->>RouteAction: success
RouteAction-->>FormUI: redirect + success info
FormUI-->>Browser: show toast and navigate
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #426 +/- ##
===============================================
+ Coverage 88.04% 88.06% +0.01%
===============================================
Files 91 91
Lines 4625 4632 +7
Branches 1494 1498 +4
===============================================
+ Hits 4072 4079 +7
Misses 553 553
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:
|
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 (1)
fdm-app/app/routes/organization.new.tsx (1)
146-148: Redirect newly created orgs to Members per UX objective.
The PR objectives state users should land on Members after creation, but the action redirects to/organization/:slug.🧭 Proposed fix
- return redirectWithSuccess(`/organization/${formValues.slug}`, { + return redirectWithSuccess(`/organization/${formValues.slug}/members`, { message: `Organisatie ${formValues.name} is aangemaakt! 🎉`, })
🤖 Fix all issues with AI agents
In `@fdm-app/app/components/blocks/farms/table.tsx`:
- Around line 61-65: The search target in the memoizedData mapping uses
owner?.name which is always undefined; update the mapping in the memoizedData
useMemo to include owner.displayUserName (e.g., use owner?.displayUserName)
instead of owner?.name so the owner name is searchable; change the mapping where
searchTarget is composed inside the memoizedData computation (refer to
memoizedData and FarmExtended.owner/displayUserName) and ensure you null-check
owner with owner?.displayUserName to avoid runtime errors.
- Around line 68-73: The fuzzyFilter implementation (FilterFn<TData>
fuzzyFilter) calls fuzzysort.go with filterValue and ends up excluding all rows
when filterValue is an empty string; update fuzzyFilter to first guard on an
empty/null/undefined filterValue (and trim it) and return true (i.e., no
filtering) in that case, otherwise call fuzzysort.go against (row.original as
any).searchTarget and return result.length > 0.
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 411-414: The Card component instance using key="atlas" is a
duplicate of the Atlas card key and will break reconciliation; locate the Card
element in farm._index.tsx that currently has key="atlas" (the one around the
Card declaration with className="transition-all hover:shadow-md") and change its
key to a unique value — either derive it from a unique identifier (e.g., an item
id or index) or give it a distinct literal (e.g., "atlas-<unique>" or
"atlas-secondary") so no two Card components share the same key.
In `@fdm-app/app/routes/organization`.$slug._index.tsx:
- Around line 82-88: Update the CardDescription strings in
organization.$slug._index.tsx to fix Dutch typos and stray punctuation: replace
"organizatie" with "organisatie" and remove the extra comma/period so the
sentence reads cleanly; specifically edit the CardDescription elements (the
block near CardTitle "Bedrijven" and the similar CardDescription at the other
occurrence) to use the corrected spelling and proper punctuation.
- Around line 18-36: The loader is calling getFarms(fdm, organization.id) but
getFarms expects a user-scoped principal_id; replace the second arg with the
authenticated session's principal_id (use session.principal_id) when calling
getFarms in the loader (and make the same change in organization.$slug.farms.tsx
where getFarms is called) so getFarms receives the correct PrincipalId instead
of organization.id.
In `@fdm-app/app/routes/organization`.$slug.farms.tsx:
- Around line 101-104: The description prop passed to the FarmTitle component is
incorrect (mentions "perceel"); update the description string in the FarmTitle
usage in organization.$slug.farms.tsx to reference farms instead (e.g., a phrase
like "Selecteer een bedrijf voor details of voeg een nieuw bedrijf toe."),
ensuring the change is only to the description prop while keeping the title prop
unchanged.
- Around line 50-77: In reduceFields(), cultivations and fertilizers are
initialized but never populated so they always return empty; inside the
fields.forEach loop (in reduceFields) extract the cultivation and fertilizer
info from each field (e.g., b_lu_name / b_lu_croprotation for cultivations and
p_name_nl / p_type for fertilizers or the actual property names present on the
field objects returned by getFields) and add entries into the cultivations and
fertilizers Records using a stable unique key (like the name or an id) to avoid
duplicates; leave the final return as Object.values(cultivations) and
Object.values(fertilizers) or remove those properties from the return if you
decide the table doesn't need them.
- Around line 31-48: getOwner currently returns raw accessor objects from
listPrincipalsForFarm which contain extra fields and a required type mismatch
for initials; update getOwner (or map its result before using it) to return an
object shaped for FarmExtended.owner by selecting only displayUserName, image,
and initials (converting initials to string | null as needed) and return null
when no owner found so the consumer can assign owner: ownerPrincipal ? {
displayUserName: ownerPrincipal.displayUserName, image: ownerPrincipal.image,
initials: ownerPrincipal.initials ?? null } : null.
In `@fdm-app/app/routes/organization.tsx`:
- Around line 63-65: Two organization route files are using the wrong session
field: replace all uses of session.user.id with session.principal_id so they
match the rest of the codebase (we already use session.principal_id in
organization.tsx). Locate the occurrences in the organization routes (e.g., the
settings and members route components) and update the variable references to
session.principal_id, then run the app/type checks to ensure no remaining
references to session.user.id remain.
🧹 Nitpick comments (7)
fdm-app/app/components/blocks/sidebar/platform.tsx (1)
97-104: LGTM with optional consideration.The hover-reveal external link icon pattern is well-implemented. Consider using
text-muted-foregroundinstead oftext-gray-500for consistency with other sidebar components that use semantic color tokens.fdm-app/app/components/blocks/organization/schema.ts (1)
3-16: Consider restricting leading/trailing hyphens in slugs.The current
isValidSlugallows slugs like-test-,--test, ortest--which may produce undesirable URLs. The form'sconvertToSlughelper trims these, but direct user input to the slug field could bypass that.♻️ Optional: stricter slug pattern
- // Slug should only contain lowercase letters, numbers, and hyphens - return /^[a-z0-9-]+$/.test(slug) + // Slug should only contain lowercase letters, numbers, and hyphens + // Must start and end with alphanumeric, no consecutive hyphens + return /^[a-z0-9]+(-[a-z0-9]+)*$/.test(slug)fdm-app/app/components/blocks/farms/columns.tsx (1)
42-74: Owner column sorting may not work as expected.The
owneraccessor returns an object, butenableSorting: trueis set. TanStack Table will convert the object to a string for comparison (usingtoString()), which will likely result in[object Object]comparisons. Consider providing a customsortingFnor disable sorting for this column.Additionally, the placeholder div at line 67 uses
h-8 w-8but the Avatar usesh-6 w-6, causing visual misalignment when owner is absent.♻️ Proposed fixes
{ accessorKey: "owner", - enableSorting: true, + enableSorting: false, header: ({ column }) => { return <DataTableColumnHeader column={column} title="Eigenaar" /> }, cell: ({ row }) => { const owner = row.original.owner return ( <div className="flex flex-row items-center text-muted-foreground gap-2"> {owner ? ( <> <Avatar className="h-6 w-6 rounded-lg"> <AvatarImage src={owner.image ?? undefined} alt={owner.displayUserName ?? undefined} /> <AvatarFallback> {owner.initials} </AvatarFallback> </Avatar> {owner.displayUserName} </> ) : ( <> - <div className="h-8 w-8 invisible" /> + <div className="h-6 w-6 invisible" /> Onbekend </> )} </div> ) }, },fdm-app/app/components/blocks/sidebar/organization.tsx (1)
129-143: Overly broadisActivematching may cause false highlights.Using
location.pathname.includes("/settings")will match any path containing/settings, including unrelated paths like/organization/test/settings-extraor even a different route like/user/settings. The same applies to/membersand/farmschecks.Consider using more precise matching:
♻️ Suggested improvement for settings check
<SidebarMenuItem> {organization ? ( <SidebarMenuButton asChild - isActive={location.pathname.includes( - "/settings", - )} + isActive={location.pathname === `/organization/${organization.slug}/settings`} > <NavLink to={`/organization/${organization.slug}/settings`} >Apply similar changes to the
/membersand/farmschecks.fdm-app/app/routes/organization.$slug.settings.tsx (2)
40-54: Computed permissions are not used.The
permissionsobject is computed in the loader (lines 43-49) and returned, but the component at line 79-81 doesn't pass it toOrganizationSettingsForm. If the form should disable fields based on permissions, pass them as props. Otherwise, consider removing the unused computation.♻️ Either use permissions or remove the computation
Option 1: Pass permissions to the form
<OrganizationSettingsForm organization={loaderData.organization} + disabled={!loaderData.permissions.canEdit} />Option 2: Remove unused code
- // Determine permissions - const currentUserMember = members.find((m) => m.userId === session.user.id) - const role = currentUserMember?.role || "viewer" - const permissions = { - canEdit: role === "owner" || role === "admin", - canDelete: role === "owner", - canInvite: role === "owner" || role === "admin", - canUpdateRoleUser: role === "owner" || role === "admin", - canRemoveUser: role === "owner" || role === "admin", - } return { organization: organization, - permissions: permissions, }
120-129: Unsafe type casting in error handling.Using
(e as any).body?.codeloses type safety. Consider creating a type guard or using a more structured error handling approach.♻️ Safer error handling
} catch (e) { - if ((e as any).body?.code === "SLUG_IS_TAKEN") { + const isSlugTakenError = + e && typeof e === "object" && + "body" in e && + (e as { body?: { code?: string } }).body?.code === "SLUG_IS_TAKEN" + if (isSlugTakenError) { return dataWithError( null, "Naam voor organisatie is niet meer beschikbaar. Kies een andere naam", ) } throw e }fdm-app/app/routes/organization.new.tsx (1)
1-73: Remove unused local form state;OrganizationSettingsFormalready owns the form.
AddOrganizationPagestill creates auseRemixForminstance and slug-sync effect, but the renderedOrganizationSettingsFormalready sets up its own form and slug logic (seefdm-app/app/components/blocks/organization/form.tsx, lines 13-118). The local form isn’t wired to any inputs and adds unnecessary subscriptions.♻️ Proposed cleanup
-import { zodResolver } from "@hookform/resolvers/zod" -import { useEffect } from "react" -import { useRemixForm } from "remix-hook-form" @@ export default function AddOrganizationPage() { - const form = useRemixForm({ - mode: "onTouched", - resolver: zodResolver(FormSchema), - defaultValues: { - name: undefined, - slug: undefined, - description: undefined, - }, - }) - - // Function to convert text to a slug - const convertToSlug = (text: string) => { - return text - .toLowerCase() - .replace(/[^a-z0-9-]+/g, "-") // Replace non-alphanumeric with - - .replace(/--+/g, "-") // Replace multiple - with single - - .replace(/^-|-$/g, "") // Trim - from start and end - } - - // Update slug when name changes - // biome-ignore lint/correctness/useExhaustiveDependencies: convertToSlug changes on every re-render and should not be used as a hook dependency - useEffect(() => { - const subscription = form.watch((value, { name }) => { - if (name === "name") { - const slug = convertToSlug(value.name ?? "") - form.setValue("slug", slug, { - shouldDirty: true, - shouldValidate: true, - }) - } - }) - return () => subscription.unsubscribe() - }, [form.watch, form.setValue]) - return ( <main className="container">
Comment on lines
+18
to
+36
| export async function loader({ params, request }: Route.LoaderArgs) { | ||
| try { | ||
| const organizations = await auth.api.listOrganizations({ | ||
| headers: request.headers, | ||
| }) | ||
|
|
||
| const organization = organizations.find( | ||
| (org) => org.slug === params.slug, | ||
| ) | ||
|
|
||
| if (!organization) { | ||
| throw data("Organisatie niet gevonden.", 404) | ||
| } | ||
|
|
||
| // Get a list of possible farms of the user | ||
| const farms = await getFarms(fdm, organization.id) | ||
|
|
||
| // Get latest available year | ||
| const calendar = getCalendarSelection()[0] ?? "all" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find getFarms definition and all its usages
echo "=== Searching for getFarms definition ==="
rg -n --type=ts 'export.*getFarms|function getFarms' -A 3
echo ""
echo "=== Searching for all getFarms calls ==="
rg -n 'getFarms\(' -B 2 -A 2 --type=ts
echo ""
echo "=== Checking fdm-core exports ==="
rg -n 'getFarms' fdm-core --type=ts -A 2Repository: SvenVw/fdm
Length of output: 14300
getFarms expects principal_id, not organization.id—this will fail or return empty results.
The getFarms function in fdm-core (farm.ts:165) is user-scoped and requires principal_id: PrincipalId as the second parameter. This loader passes organization.id instead, which is a different identifier type and will not work. All other routes correctly call getFarms(fdm, session.principal_id).
Update to:
const farms = await getFarms(fdm, session.principal_id)Note: organization.$slug.farms.tsx:26 has the same issue and should also be fixed.
🤖 Prompt for AI Agents
In `@fdm-app/app/routes/organization`.$slug._index.tsx around lines 18 - 36, The
loader is calling getFarms(fdm, organization.id) but getFarms expects a
user-scoped principal_id; replace the second arg with the authenticated
session's principal_id (use session.principal_id) when calling getFarms in the
loader (and make the same change in organization.$slug.farms.tsx where getFarms
is called) so getFarms receives the correct PrincipalId instead of
organization.id.
Collaborator
Author
There was a problem hiding this comment.
getFarms is called with the organization id in order to only get the organization farms. When this is done the database join with the organization members fails, and if the organization was given a role to a farm, its roles are listed with the principal type "user"
Maybe we could improve the listRolesOfPrincipalForResource function to return with principal type "organization" instead, but I don't find it to be necessary.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for getRolesOfPrincipalForResource implementation
echo "=== getRolesOfPrincipalForResource implementation ==="
ast-grep --pattern 'export async function getRolesOfPrincipalForResource($$$) {
$$$
}'
echo ""
echo "=== Find the full implementation with context ==="
rg -n 'getRolesOfPrincipalForResource' fdm-core/src/authorization.ts -A 60
echo ""
echo "=== Check how organization roles are joined in the query ==="
rg -n 'member_user_id|member_organization_id' fdm-core/src -A 3 -B 3
Length of output: 6681
🏁 Script executed:
#!/bin/bash
# Search for organization table schema
echo "=== Organization schema ==="
rg -n 'export const organization' fdm-core/src/db -A 10
echo ""
echo "=== Check how we could distinguish org vs user IDs ==="
rg -n 'better-auth.*organization|organizationId' fdm-core/src/db -B 2 -A 2
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Search more broadly for organization schema
echo "=== Find schema files ==="
fd -e ts schema fdm-core/src/db
echo ""
echo "=== Search for organization table ==="
rg -n 'organization.*=.*table|organization.*=.*pgTable' fdm-core/src -A 5
echo ""
echo "=== Check imports and usage of organization ==="
rg -n 'from.*schema.*organization|authNSchema' fdm-core/src/authorization.ts -B 2 -A 2
echo ""
echo "=== Look at the actual join logic more carefully ==="
rg -n 'leftJoin|member.organizationId' fdm-core/src/authorization.ts -B 3 -A 3
Length of output: 7157
@BoraIneviNMI, thank you for the clarification! I can now see the issue clearly.
Root cause: The logic at line 388-390 in getRolesOfPrincipalForResource determines principal_type based on whether member_user_id is present from the left join. When an organization ID is passed and that organization has no members (or the join doesn't match for any reason), member_user_id will be null, causing the code to incorrectly classify the organization's roles as principal_type: "user".
Proper fix (if you decide to address it later): Add a second left join to the organization table to definitively identify whether principal_id is an organization ID:
.leftJoin(
authNSchema.organization,
eq(authZSchema.role.principal_id, authNSchema.organization.id)
)Then update the principal_type logic to check the organization table:
const principal_type = item.organization_id ? "organization" : "user"This would work correctly regardless of member count.
For now: I respect your decision that this isn't critical to fix immediately. The functionality works even with the incorrect label, and you can address it in a future refactoring when needed.
.-. .-.
(o o) (o o)
| O \ / O |
\ ' /
` --- ' Noted for future!
✏️ Learnings added
Learnt from: BoraIneviNMI
Repo: SvenVw/fdm PR: 426
File: fdm-app/app/routes/organization.$slug._index.tsx:31-55
Timestamp: 2026-02-18T14:12:24.747Z
Learning: In fdm-core/src/authorization.ts, getRolesOfPrincipalForResource has a known limitation: when an organization ID is passed as principal_id and the organization has no members, the function incorrectly labels the principal_type as "user" instead of "organization" because the logic relies on member_user_id presence from a left join to the member table. This is a known issue that doesn't affect functionality but may cause confusion in debugging. The proper fix would be to add a left join to the organization table to definitively identify organization IDs.
<!-- [add_learning]
When calling getFarms with an organization.id in fdm-app routes (e.g., organization.$slug._index.tsx), the intent is to retrieve farms that the organization has access to. This is an intentional use case even though getFarms typically expects a user's principal_id. The resulting role data may have principal_type labeled incorrectly due to a known limitation in getRolesOfPrincipalForResource.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 42
File: fdm-app/app/routes/farm/_b_id_farm/layout.tsx:46-95
Timestamp: 2025-01-09T16:03:37.764Z
Learning: The `FarmLayout` component in `components/custom/farm-layout.tsx` provides a reusable layout structure for farm-related pages, with support for farm selection dropdown, customizable breadcrumb titles, and flexible content rendering through either children or Outlet components.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 151
File: fdm-app/app/routes/signin._index.tsx:101-101
Timestamp: 2025-06-02T10:31:27.097Z
Learning: In fdm-app/app/routes/signin._index.tsx, the redirect destinations are intentionally inconsistent by design: the component defaults new sign-ins to "/welcome" (line 101) while the loader redirects authenticated users to "/farm" (line 80) and the action uses "/farm" as fallback (line 434). This creates appropriate user flows where new users complete their profile via the welcome page, while existing authenticated users bypass it and go directly to the main application.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 274
File: fdm-app/app/routes/farm.$b_id_farm._index.tsx:151-204
Timestamp: 2025-09-23T12:27:07.391Z
Learning: In the FDM application, field overview functionality is implemented as a dedicated page accessible via `farm/{farmId}/{calendar}/field` rather than as a direct listing on the dashboard. The dashboard includes a "Perceelsoverzicht" quick action card that provides navigation to this comprehensive field management interface.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 42
File: fdm-app/app/routes/farm/_b_id_farm/layout.tsx:46-95
Timestamp: 2025-01-09T16:03:37.764Z
Learning: A shared layout component `FarmLayoutBase` has been created in `components/custom/farm-layout-base.tsx` to maintain consistency across farm-related pages. The component handles farm selection dropdown, breadcrumb navigation, and provides a common layout structure.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 42
File: fdm-app/app/routes/farm/_b_id_farm/layout.tsx:46-95
Timestamp: 2025-01-09T16:03:37.764Z
Learning: A comprehensive farm layout system has been created in `components/custom/farm-layouts/` with `BaseFarmLayout` and `FarmSidebarLayout` components. The system supports both simple and sidebar-based layouts while maintaining consistent header and farm selection functionality across all farm routes.
Learnt from: BoraIneviNMI
Repo: SvenVw/fdm PR: 272
File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.fertilizer.manage.new.$p_id.tsx:85-101
Timestamp: 2025-09-24T14:02:48.574Z
Learning: Both getFertilizer and getFertilizers functions in svenvw/fdm-core perform authorization checks using the user's principal_id to verify farm access before returning fertilizer data.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 124
File: fdm-core/src/organization.ts:300-339
Timestamp: 2025-04-18T14:51:48.033Z
Learning: The `getUsersInOrganization` function in fdm-core/src/organization.ts needs to include user ID and email fields to support role updates and user removal operations, which will be fixed in a future commit (b17fac16c9e5a0de56d0346e712b2ce966d305d5).
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 87
File: fdm-core/src/farm.ts:22-25
Timestamp: 2025-03-04T09:03:04.358Z
Learning: In the FDM codebase, functions that insert/create data (like `addFarm`) intentionally use `principal_id: string` instead of `PrincipalId` to enforce that only a single principal ID can be used for creation operations, while read operations use the more flexible `PrincipalId` type which supports both single IDs and arrays of IDs.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 124
File: fdm-core/src/organization.ts:300-339
Timestamp: 2025-04-18T14:51:48.033Z
Learning: The `getUsersInOrganization` function in fdm-core/src/organization.ts currently returns only firstname, surname, image, and role, but needs to include id and email fields to support downstream operations like role updates and user removal. This will be fixed in commit b17fac16c9e5a0de56d0346e712b2ce966d305d5.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 124
File: fdm-core/src/organization.ts:300-339
Timestamp: 2025-04-18T14:51:48.033Z
Learning: The `getUsersInOrganization` function in fdm-core/src/organization.ts needed to include id and email fields for users, which was fixed in commit b17fac16c9e5a0de56d0346e712b2ce966d305d5.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 124
File: fdm-core/src/organization.ts:300-339
Timestamp: 2025-04-18T14:51:48.033Z
Learning: The `getUsersInOrganization` function in fdm-core/src/organization.ts will be updated in commit b17fac16c9e5a0de56d0346e712b2ce966d305d5 to include id and email fields, which are necessary for subsequent role updates and user removal operations.
Learnt from: BoraIneviNMI
Repo: SvenVw/fdm PR: 272
File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.fertilizer.manage.new.$p_id.tsx:85-101
Timestamp: 2025-09-24T14:02:48.574Z
Learning: The getFertilizer function in svenvw/fdm-core does not perform authorization checks, unlike getFertilizers which includes a checkPermission call to verify farm access. This means getFertilizer(fdm, p_id) can potentially return fertilizer details for any fertilizer ID without validating user permissions.
Learnt from: BoraIneviNMI
Repo: SvenVw/fdm PR: 410
File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.fields.tsx:71-96
Timestamp: 2026-01-22T12:27:36.495Z
Learning: In fdm-core, functions like getFarm and getField call checkPermission before querying the database. The checkPermission function uses getResourceChain to verify resource existence; when a resource (farm, field, etc.) doesn't exist, getResourceChain returns an empty array, causing checkPermission to throw "Permission denied" exception rather than returning null or undefined. This means callers don't need null guards—they should rely on exception handling instead.
Learnt from: BoraIneviNMI
Repo: SvenVw/fdm PR: 272
File: fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fertilizers.$b_lu_catalogue.manage.$p_id.tsx:151-164
Timestamp: 2025-09-23T10:02:32.123Z
Learning: The getFertilizer function from svenvw/fdm-core throws an exception if the fertilizer doesn't exist, rather than returning null or undefined.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 124
File: fdm-core/src/db/schema-authn.ts:70-76
Timestamp: 2025-04-18T14:20:40.975Z
Learning: The organization schema in fdm-core/src/db/schema-authn.ts is managed by better-auth, and modifications to field constraints (like making the slug field non-nullable) should maintain compatibility with better-auth's expectations, even if application code assumes non-null values.
Comment on lines
+31
to
+48
| async function getOwner() { | ||
| const accessors = ( | ||
| await listPrincipalsForFarm( | ||
| fdm, | ||
| myOrganization.id, | ||
| farm.b_id_farm, | ||
| ) | ||
| ).filter((accessor) => accessor.type === "user") | ||
|
|
||
| return ( | ||
| accessors.find( | ||
| (accessor) => accessor.role === "owner", | ||
| ) ?? | ||
| accessors.find( | ||
| (accessor) => accessor.role === "advisor", | ||
| ) | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find listPrincipalsForFarm function definition and return type
rg "listPrincipalsForFarm" --type ts --type tsx -B 2 -A 15 | head -100Repository: SvenVw/fdm
Length of output: 81
🏁 Script executed:
# Find FarmExtended interface definition
rg "interface FarmExtended|type FarmExtended" --type ts --type tsx -A 10Repository: SvenVw/fdm
Length of output: 81
🏁 Script executed:
# Search for owner field usage in the context
rg "owner.*displayUserName|displayUserName.*owner" --type ts --type tsx -B 3 -A 3Repository: SvenVw/fdm
Length of output: 81
🏁 Script executed:
# Find listPrincipalsForFarm function definition
rg "listPrincipalsForFarm" -B 2 -A 15 | head -150Repository: SvenVw/fdm
Length of output: 8825
🏁 Script executed:
# Find FarmExtended interface definition
rg "interface FarmExtended|type FarmExtended" -A 10Repository: SvenVw/fdm
Length of output: 888
🏁 Script executed:
# Search for the specific file and examine its structure
rg "owner.*displayUserName|\.owner\s*=" -B 2 -A 2 | head -80Repository: SvenVw/fdm
Length of output: 1003
🏁 Script executed:
# Find Principal type definition
rg "type Principal|interface Principal" -A 10Repository: SvenVw/fdm
Length of output: 7411
🏁 Script executed:
# Check the getPrincipal function and what it returns
rg "export.*function getPrincipal" -B 2 -A 20 | head -80Repository: SvenVw/fdm
Length of output: 1583
🏁 Script executed:
# Look for the complete Principal object structure in the farm.ts implementation
rg "return {" fdm-core/src/farm.ts -B 5 -A 15 | grep -A 15 "principalsDetails\|Principal"Repository: SvenVw/fdm
Length of output: 133
🏁 Script executed:
# Get the full context of organization.$slug.farms.tsx
cat -n fdm-app/app/routes/organization.\$slug.farms.tsx | head -120Repository: SvenVw/fdm
Length of output: 4867
🏁 Script executed:
# Look for owner assignment and usage
rg "owner" fdm-app/app/routes/organization.\$slug.farms.tsx -B 3 -A 3Repository: SvenVw/fdm
Length of output: 584
🏁 Script executed:
# Get the complete implementation of listPrincipalsForFarm
rg "export async function listPrincipalsForFarm" fdm-core/src/farm.ts -A 50Repository: SvenVw/fdm
Length of output: 1683
🏁 Script executed:
# Also check what principalsDetails returns
rg "principalsDetails.*=" fdm-core/src/farm.ts -B 5 -A 25Repository: SvenVw/fdm
Length of output: 944
🏁 Script executed:
# Check if there's any type annotation for the returned data structure
rg "allFarms\|data" fdm-app/app/routes/organization.\$slug.farms.tsx -B 2 -A 5Repository: SvenVw/fdm
Length of output: 36
🏁 Script executed:
# Check if FarmExtended is actually used as the type for the returned data
rg "FarmExtended" fdm-app/app/routes/organization.\$slug.farms.tsx -B 3 -A 3Repository: SvenVw/fdm
Length of output: 36
🏁 Script executed:
# Check how data is passed to DataTable and if there's type casting
rg "DataTable.*columns.*data" fdm-app/app/routes/organization.\$slug.farms.tsx -B 5 -A 5Repository: SvenVw/fdm
Length of output: 458
Map the accessor object to match FarmExtended.owner shape.
The getOwner() function returns the raw accessor object from listPrincipalsForFarm, which includes extra fields (id, username, type, isVerified, role) not expected by FarmExtended.owner. Additionally, the accessor has initials as string, while FarmExtended.owner expects initials as string | null. Explicitly map the returned accessor to the expected shape:
const ownerPrincipal = await getOwner()
owner: ownerPrincipal ? {
displayUserName: ownerPrincipal.displayUserName,
image: ownerPrincipal.image,
initials: ownerPrincipal.initials,
} : null🤖 Prompt for AI Agents
In `@fdm-app/app/routes/organization`.$slug.farms.tsx around lines 31 - 48,
getOwner currently returns raw accessor objects from listPrincipalsForFarm which
contain extra fields and a required type mismatch for initials; update getOwner
(or map its result before using it) to return an object shaped for
FarmExtended.owner by selecting only displayUserName, image, and initials
(converting initials to string | null as needed) and return null when no owner
found so the consumer can assign owner: ownerPrincipal ? { displayUserName:
ownerPrincipal.displayUserName, image: ownerPrincipal.image, initials:
ownerPrincipal.initials ?? null } : null.
Comment on lines
+63
to
+65
| const member = membersListResponse.members.find( | ||
| (member) => member.userId === session.principal_id, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check user ID field usage in organization route files
echo "=== Checking organization route files for session ID usage ==="
rg -n "session\.(principal_id|user\.id)" fdm-app/app/routes/organization --type ts -A 2 -B 2
echo ""
echo "=== Checking if organization.$slug.settings.tsx exists and its content around line 41 ==="
if [ -f "fdm-app/app/routes/organization.\$slug.settings.tsx" ]; then
sed -n '35,50p' "fdm-app/app/routes/organization.\$slug.settings.tsx"
fi
echo ""
echo "=== List all files in organization routes directory ==="
ls -la fdm-app/app/routes/organization* 2>/dev/null | head -20Repository: SvenVw/fdm
Length of output: 1801
🏁 Script executed:
#!/bin/bash
# Check organization.tsx around line 64
echo "=== organization.tsx lines 60-70 ==="
sed -n '60,70p' fdm-app/app/routes/organization.tsx
echo ""
echo "=== organization.$slug.settings.tsx lines 38-48 ==="
sed -n '38,48p' fdm-app/app/routes/organization.\$slug.settings.tsx
echo ""
echo "=== Search for all session.user.id and session.principal_id usages ==="
echo "--- session.user.id occurrences ---"
rg "session\.user\.id" fdm-app/app/routes --type ts -n
echo ""
echo "--- session.principal_id occurrences ---"
rg "session\.principal_id" fdm-app/app/routes --type ts -nRepository: SvenVw/fdm
Length of output: 34805
Fix inconsistent user ID field in organization routes.
The code at organization.tsx:64 correctly uses session.principal_id, but organization.$slug.settings.tsx:41 and organization.$slug.members.tsx:58 incorrectly use session.user.id. Standardize on session.principal_id across all organization routes for consistency with the rest of the codebase.
🤖 Prompt for AI Agents
In `@fdm-app/app/routes/organization.tsx` around lines 63 - 65, Two organization
route files are using the wrong session field: replace all uses of
session.user.id with session.principal_id so they match the rest of the codebase
(we already use session.principal_id in organization.tsx). Locate the
occurrences in the organization routes (e.g., the settings and members route
components) and update the variable references to session.principal_id, then run
the app/type checks to ensure no remaining references to session.user.id remain.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@fdm-app/app/components/blocks/farms/table.tsx`:
- Around line 50-59: The initial columnVisibility state uses a mobile-specific
object ({ a_som_loi: false, b_soiltype_agr: false, b_area: false }) but the
useEffect resets visibility to a different mobile object ({ b_area: false }),
causing visible flash; consolidate by defining a single mobileVisibility
constant and use it in both the useState call that sets columnVisibility and in
the useEffect that calls setColumnVisibility (referencing columnVisibility,
setColumnVisibility, useState, useEffect, and isMobile) so the same set of
columns (a_som_loi, b_soiltype_agr, b_area) are consistently hidden on mobile.
In `@fdm-app/app/routes/organization`.$slug.settings.tsx:
- Around line 59-72: The file contains outdated/farm-specific copy and a typo:
update the top docstring to describe "organization settings" instead of "farm
properties" and fix the UI subtitle text passed to FarmTitle from "Werk de
gegevens bij van dit organisatie." to correct Dutch grammar (e.g., "Werk de
gegevens bij van deze organisatie."), and also review the title "Organisatie
instellingen" — consider "Organisatie-instellingen" for consistent hyphenation;
change the docstring and the string literals inside
FarmSettingsPropertiesBlock/FarmTitle accordingly.
- Around line 92-148: After locating currentOrganization in action, add a
server-side permission check: call getSession(request) result to obtain the user
id, call auth.api.listMembers({ headers: request.headers, query: {
organizationId: currentOrganization.id } }) (or the same service used in the
loader), find the member whose userId matches the session user id, and ensure
their role is "owner" or "admin"; if not, return a 403-style response (e.g.,
dataWithError or throw data(..., 403)) to block updates before calling
auth.api.updateOrganization in the action function.
♻️ Duplicate comments (4)
fdm-app/app/routes/organization.$slug._index.tsx (2)
1-33: Use the authenticated principal_id for getFarms.This loader passes
organization.id;getFarmsis user‑scoped. (Duplicate of earlier review.)🐛 Suggested fix
-import { auth } from "~/lib/auth.server" +import { auth, getSession } from "~/lib/auth.server" @@ - const organizations = await auth.api.listOrganizations({ + const session = await getSession(request) + const organizations = await auth.api.listOrganizations({ headers: request.headers, }) @@ - const farms = await getFarms(fdm, organization.id) + const farms = await getFarms(fdm, session.principal_id)
84-88: Remove the stray comma/period in the description. (Duplicate of earlier review.)✏️ Suggested fix
- Uitgebreide tabel met - bedrijven met toegang - tot deze organisatie, . + Uitgebreide tabel met + bedrijven met toegang + tot deze organisatie.fdm-app/app/routes/organization.$slug.farms.tsx (2)
31-48: Map the accessor to matchFarmExtended.ownershape.The
getOwner()function returns the raw accessor object which includes extra fields (id,username,type,isVerified,role) beyond whatFarmExtended.ownerexpects. Explicitly map to the expected shape to avoid passing unnecessary data to the client.
50-77:cultivationsandfertilizersare always empty.The
cultivationsandfertilizersRecords are initialized but never populated. Onlyb_areais accumulated in the loop. Either populate these records from field data or remove the unused properties from the return value.
🧹 Nitpick comments (4)
fdm-app/app/routes/organization.$slug.farms.tsx (1)
79-88: Consider extracting promises before the await to enable parallelism.The promises are correctly created before awaiting, enabling parallel execution. However, consider using
Promise.allfor cleaner semantics:♻️ Optional refactor
- const ownerPromise = getOwner() - const reduceFieldsPromise = reduceFields() - - return { - b_id_farm: farm.b_id_farm, - b_name_farm: farm.b_name_farm, - owner: await ownerPromise, - ...(await reduceFieldsPromise), - } + const [owner, fieldData] = await Promise.all([ + getOwner(), + reduceFields(), + ]) + + return { + b_id_farm: farm.b_id_farm, + b_name_farm: farm.b_name_farm, + owner, + ...fieldData, + }fdm-app/app/components/blocks/organization/form.tsx (1)
44-57: useEffect dependency array may cause stale closures.The dependency array includes
form.watchandform.setValue, but these are methods on the form object that change reference on re-renders. The biome-ignore comment addressesconvertToSlug, but the actual dependencies should be stable. Consider usingformdirectly or memoizing:♻️ Recommended fix
// Update slug when name changes - // biome-ignore lint/correctness/useExhaustiveDependencies: convertToSlug changes on every re-render and should not be used as a hook dependency useEffect(() => { const subscription = form.watch((value, { name }) => { if (!organization?.slug && name === "name") { const slug = convertToSlug(value.name ?? "") form.setValue("slug", slug, { shouldDirty: true, shouldValidate: true, }) } }) return () => subscription.unsubscribe() - }, [organization?.slug, form.watch, form.setValue]) + }, [organization?.slug, form])fdm-app/app/routes/organization.new.tsx (2)
28-34: Empty loader returns an empty object.The loader doesn't fetch any data but exists to satisfy the route convention. Consider adding a comment explaining its purpose or removing it if not required by the framework.
79-95: Improve type safety for error handling.The
(e as any).body?.codecast loses type safety. Consider defining an interface for the expected error shape or using a type guard:♻️ Recommended improvement
+ interface SlugCheckError { + body?: { code?: string } + } + + function isSlugCheckError(e: unknown): e is SlugCheckError { + return typeof e === 'object' && e !== null && 'body' in e + } + // Check if slug is available try { await auth.api.checkOrganizationSlug({ headers: request.headers, body: { slug: slug, }, }) } catch (e) { - if ((e as any).body?.code === "SLUG_IS_TAKEN") { + if (isSlugCheckError(e) && e.body?.code === "SLUG_IS_TAKEN") { return dataWithError( null, "Naam voor organisatie is niet meer beschikbaar. Kies een andere naam", ) } throw e }Note: The same pattern exists in
organization.$slug.settings.tsx. Consider extracting this error handling to a shared utility if used in multiple places.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@fdm-app/app/routes/organization._index.tsx`:
- Around line 24-27: The loader currently maps organizationsRaw into
organizations using JSON.parse on org.metadata which will throw on malformed
JSON and crash the loader; add a safeParseMetadata helper (e.g., function
safeParseMetadata(raw: string): Record<string, unknown>) and replace the inline
JSON.parse call in the organizations = organizationsRaw.map(...) transformation
with safeParseMetadata(org.metadata), where safeParseMetadata returns {} on
parse failure and logs the raw value (e.g., console.error or the app logger) so
the loader can continue instead of crashing.
In `@fdm-app/app/routes/organization`.$slug.settings.tsx:
- Around line 55-58: The code directly calls JSON.parse on
organizationRaw.metadata which will throw on malformed JSON and crash the
loader; update the loader in organization.$slug.settings.tsx to safely parse
metadata (e.g., wrap JSON.parse(organizationRaw.metadata) in a try/catch or use
a safeParse helper) so that on parse failure you fall back to an empty object
and optionally log the error; target the metadata assignment and any related
code that reads organizationRaw.metadata to ensure robust handling without
throwing.
🧹 Nitpick comments (1)
fdm-app/app/routes/organization.$slug.settings.tsx (1)
134-143: Improve type safety for error handling.The
(e as any).body?.codepattern loses type safety. Consider creating a type guard or using a more structured approach.♻️ Suggested improvement
} catch (e) { - if ((e as any).body?.code === "SLUG_IS_TAKEN") { + const errorBody = e && typeof e === "object" && "body" in e + ? (e as { body?: { code?: string } }).body + : undefined + if (errorBody?.code === "SLUG_IS_TAKEN") { return dataWithError( null, "Naam voor organisatie is niet meer beschikbaar. Kies een andere naam", ) } throw e }Alternatively, if
auth.api.checkOrganizationSlughas a typed error response, import and use that type.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/routes/organization.$slug.members.tsx (1)
278-287: Disable the "Verwijder" button during fetcher submission.The
Selectrespects thedisabledstate, but the "Verwijder" button remains clickable during role change submissions, potentially causing concurrent requests.Proposed fix
{permissions.canRemoveUser ? ( <Button variant="destructive" className="shrink-0" name="intent" value="remove_user" + disabled={disabled} > Verwijder </Button> ) : null}
🤖 Fix all issues with AI agents
In `@fdm-app/app/routes/organization`.$slug.members.tsx:
- Around line 245-255: The ref and submitRoleChange need null-safe handling and
a proper initial value for React 19: change the declaration const formRef =
useRef<HTMLFormElement | null>(null) and in submitRoleChange guard
formRef.current before using it (e.g., if (!formRef.current) { toast.error("Form
not mounted"); return } const formData = new FormData(formRef.current); then
append intent and call fetcher.submit as before). Ensure types reflect
HTMLFormElement | null and maintain the existing fetcher, disabled, and
submitRoleChange names.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/routes/organization.invitations.$invitation_id.respond.tsx (1)
169-180:⚠️ Potential issue | 🟠 MajorHandle
do_nothingbefore invitation lookup.Line 176 throws when the invitation is missing, so the
do_nothingbranch at Line 223 is unreachable. In the 404 boundary flow, the “Terug naar mijn uitnodigingen” submit can’t complete its intended redirect.💡 Proposed fix
export async function action({ request, params }: ActionFunctionArgs) { const formValues = await extractFormValuesFromRequest(request, FormSchema) await getSession(request) + + if (formValues.intent === "do_nothing") { + return redirect("/organization/invitations") + } const invitationId = params.invitation_id if (!invitationId) { throw data("Invitation not found", { status: 404, statusText: "Invitation not found", }) } @@ - if (formValues.intent === "do_nothing") { - return redirect("/organization/invitations") - } - throw new Error("invalid intent") }Also applies to: 223-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/organization.invitations`.$invitation_id.respond.tsx around lines 169 - 180, The code throws a 404 when invitation is missing before honoring the "do_nothing" action, making the do_nothing branch unreachable; update the request handling to read the form action (e.g., check for the "do_nothing" value from formData or request) before calling auth.api.getInvitation so that if action === "do_nothing" you skip the invitation lookup and return/redirect accordingly; ensure the early-return or redirect logic for the do_nothing branch is placed before the invocation of auth.api.getInvitation and remove or adjust the subsequent dataWithError("Invitation not found", ...) path so the do_nothing submit can complete its redirect even when the invitation is absent.
♻️ Duplicate comments (2)
fdm-app/app/routes/farm._index.tsx (2)
306-318:⚠️ Potential issue | 🟡 MinorMake farm partitions mutually exclusive to prevent duplicate cards.
Line 307 (
userRoles.length > 0) and Line 314 (farm.organization) overlap. Farms with both direct and organization access can render twice.♻️ Proposed fix
- const [userFarms, organizationFarms] = useMemo(() => { - const userFarms = loaderData.farms - .filter((farm) => farm.userRoles.length > 0) - .map((farm) => ({ - ...farm, - organization: undefined, - organizationRoles: undefined, - })) - const organizationFarms = loaderData.farms.filter( - (farm) => farm.organization, - ) + const [userFarms, organizationFarms] = useMemo(() => { + const organizationFarms = loaderData.farms.filter( + (farm) => farm.organization, + ) + const userFarms = loaderData.farms + .filter( + (farm) => farm.userRoles.length > 0 && !farm.organization, + ) + .map((farm) => ({ + ...farm, + organization: undefined, + organizationRoles: undefined, + })) return [userFarms, organizationFarms] }, [loaderData])🤖 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 306 - 318, The current useMemo partition creates overlapping arrays because userFarms filters only on userRoles.length > 0 while organizationFarms filters on farm.organization, causing farms with both to appear in both lists; update the userFarms filter to exclude farms that belong to an organization (e.g., filter for farm.userRoles.length > 0 && !farm.organization) so that userFarms and organizationFarms are mutually exclusive while preserving the existing map that strips organization and organizationRoles.
115-117:⚠️ Potential issue | 🟠 MajorMatch organization memberships by
session.user.id, not active principal.Line 116 compares
member.userIdagainstsession.principal_id. When the active principal is an organization, this can yield empty role matches for the signed-in user and incorrect organization role display.🛠️ Proposed fix
- const userRoles = membersListResponse.members - .filter((member) => member.userId === session.principal_id) - .map((member) => member.role) + const userRoles = membersListResponse.members + .filter((member) => member.userId === session.user.id) + .map((member) => member.role)🤖 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 115 - 117, The role-matching logic uses session.principal_id which can be an organization ID; change the comparison to match the signed-in user ID instead by comparing member.userId to session.user.id (i.e., replace uses of session.principal_id in the userRoles computation with session.user.id) so membersListResponse.members.filter(...).map(...) correctly finds the current user's roles; ensure any other places that derive roles for display use session.user.id as well.
🧹 Nitpick comments (6)
fdm-app/app/components/custom/expandable.tsx (1)
54-68: UnusedmyPropsonClick in non-asChild branch.The
myPropsobject defines anonClickhandler (line 55-57), but whenasChildis false, line 68 defines a separate inlineonClick, makingmyPropsonly used for theasChildbranch. Consider consolidating to avoid confusion.♻️ Proposed refactor to consolidate onClick handling
export function ExpandableTrigger(props: ExpandableTriggerProps) { const { expanded, setExpanded, isOverflowing } = useContext(ctx) - const myProps = { - onClick() { - setExpanded(!expanded) - }, - } + const handleClick = () => setExpanded((prev) => !prev) + return props.asChild ? ( - <Slot {...props} {...myProps} /> + <Slot {...props} onClick={handleClick} /> ) : ( <Button variant="link" className={cn( "-ms-4 transition-opacity duration-300 text-muted-foreground", !isOverflowing && "opacity-0", )} - onClick={() => setExpanded((prev) => !prev)} + onClick={handleClick} > {expanded ? "Toon minder" : "Lees verder"} </Button> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/custom/expandable.tsx` around lines 54 - 68, The myProps object (with onClick calling setExpanded(!expanded)) is only applied to the Slot branch while the Button branch declares its own inline onClick, causing duplication and confusion; consolidate by moving the onClick logic into a single handler (e.g., a toggle function or reuse myProps.onClick) and apply it to both branches—replace the Button's inline onClick with the shared handler (or spread myProps) and ensure the Slot branch still spreads {...props} {...myProps} so toggle behavior is consistent for props.asChild true or false, referencing myProps, setExpanded, expanded, Slot, Button, and props.asChild.fdm-app/app/routes/organization.new.tsx (1)
59-76: Extract slug-availability handling into a shared helper.This block now duplicates the same check/error-mapping used in
fdm-app/app/routes/organization.$slug.settings.tsx(Lines 120-146 in the provided snippet). A shared server helper would prevent drift in behavior/messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/organization.new.tsx` around lines 59 - 76, Extract the slug-availability logic into a shared server helper (e.g., checkOrganizationSlugAvailability) that calls auth.api.checkOrganizationSlug and maps the SLUG_IS_TAKEN error to the same dataWithError message; replace the in-place try/catch in organization.new route (the block using auth.api.checkOrganizationSlug and dataWithError) and the similar block in organization.$slug.settings route with a single call to that helper so both routes reuse identical logic and error mapping.fdm-app/app/routes/organization._index.tsx (1)
44-62: Loader can fan out heavily onlistMemberscalls.This does one members API call per organization. For users in many organizations, response time and backend load can degrade quickly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/organization._index.tsx` around lines 44 - 62, The loader currently calls auth.api.listMembers once per organization (inside the organizationsRaw.map async block), which can fan out; instead fetch memberships in a single batched call or a single list of the current user's memberships, then map those results into each organization to compute userRoles and orderedUserRoles; locate the organizationsRaw processing and replace the per-org auth.api.listMembers call with a pre-fetched membership collection (keyed by organizationId) and reuse parseOrganizationMetadata and session.principal_id to build each organization's userRoles and metadata.fdm-app/app/lib/organization-helpers.ts (1)
21-27: Treat empty metadata as empty metadata, not a parse failure.Line 26 parses unconditionally, so organizations without metadata are returned as
{ error }instead of a normal empty metadata shape.♻️ Proposed adjustment
export function parseOrganizationMetadata(organization: Organization): { data?: OrganizationMetadata error?: Error } { + if (!organization.metadata) { + return { data: {} } + } try { - let parsedMetadata = JSON.parse(organization.metadata) + const parsedMetadata = JSON.parse(organization.metadata) try { parsedMetadata.description = JSON.parse( `"${parsedMetadata.description}"`, ) } catch (_triedAndDidNotWork) {}Also applies to: 35-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/lib/organization-helpers.ts` around lines 21 - 27, The parseOrganizationMetadata function currently JSON.parse's organization.metadata unconditionally which treats empty or missing metadata as a parse error; update parseOrganizationMetadata to first check if organization.metadata is falsy or an empty string and in that case return { data: {} } (or the appropriate empty OrganizationMetadata shape) instead of attempting JSON.parse, and apply the same guard to the similar parsing block around lines 35-42 so both parsing flows bail out early on empty metadata and only call JSON.parse when metadata is a non-empty string, returning { error } only for actual parse exceptions.fdm-app/app/routes/organization.$slug.members.tsx (1)
228-234: Deduplicate permissions prop typing for member components.The same inline permissions shape appears twice. A shared type keeps these props in sync and reduces drift risk.
♻️ Proposed cleanup
+type OrganizationMemberPermissions = { + canEdit: boolean + canDelete: boolean + canInvite: boolean + canUpdateRoleUser: boolean + canRemoveUser: boolean +} + const MemberRow = ({ member, permissions, }: { member: MemberWithUser - permissions: { - canEdit: boolean - canDelete: boolean - canInvite: boolean - canUpdateRoleUser: boolean - canRemoveUser: boolean - } + permissions: OrganizationMemberPermissions }) => { @@ const MemberAction = ({ member, permissions, }: { member: MemberWithUser - permissions: { - canEdit: boolean - canDelete: boolean - canInvite: boolean - canUpdateRoleUser: boolean - canRemoveUser: boolean - } + permissions: OrganizationMemberPermissions }) => {Also applies to: 267-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/organization`.$slug.members.tsx around lines 228 - 234, Replace the duplicated inline permissions object type used for member components with a single shared type alias or interface (e.g., "MemberPermissions") and use that alias wherever the inline shape appears (the duplicated blocks at the permissions prop declaration and the second occurrence around lines referenced in the review). Specifically, define: type MemberPermissions = { canEdit: boolean; canDelete: boolean; canInvite: boolean; canUpdateRoleUser: boolean; canRemoveUser: boolean } (export it if used across modules), then update the component prop typings to use MemberPermissions instead of repeating the inline object so both member-related props stay in sync.fdm-app/app/routes/organization.$slug._index.tsx (1)
122-123: Consider removing or using loadercalendarto avoid split source-of-truth.
calendaris returned by the loader (Line 158) but the component reads year state fromuseCalendarStore(Line 179) instead. Keeping one source simplifies maintenance.Also applies to: 155-159, 179-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/organization`.$slug._index.tsx around lines 122 - 123, The component creates a local const calendar via getCalendarSelection() while the loader also returns calendar and the component uses useCalendarStore for calendar state; pick one source-of-truth to eliminate duplication. Either remove the local getCalendarSelection() usage and read calendar exclusively from the loader-provided value (prop name calendar) or initialize/rehydrate useCalendarStore with the loader's calendar value (e.g., call the store's setter in the component or during hydration) and remove direct reads of getCalendarSelection(); update references to the calendar variable and the useCalendarStore getter/setter so only the loader or the store is authoritative (ensure getCalendarSelection(), calendar variable, and useCalendarStore usage are all consistent).
🤖 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/invitation.tsx`:
- Around line 85-87: The Link in the invitation component currently points to
`${baseUrl}/organization` but its label says “bekijk je uitnodigingen”, which is
misleading; update the Link in the Invitation component (the Link element using
`${baseUrl}/organization`) so the destination and CTA match — either change the
href to the invitations route (e.g., `${baseUrl}/organization/invitations` or
similar invitations path used elsewhere) or change the visible text to reflect
the broader organization page; ensure the Link element and its label are
consistent.
In `@fdm-app/app/components/custom/expandable.tsx`:
- Around line 79-80: The ExpandableContent component is spreading custom props
(collapsedClassName, expandedClassName) onto the div which causes invalid DOM
attribute warnings; inside ExpandableContent destructure collapsedClassName and
expandedClassName out of props (e.g., const { collapsedClassName,
expandedClassName, ...rest } = props) and then compute the className based on
expanded state (merge collapsedClassName or expandedClassName with any
rest.className) and pass only the cleaned rest props plus the computed className
to the div instead of spreading collapsedClassName/expandedClassName.
- Line 70: The UI string for the collapsed state in the Expandable component
contains a typo; update the ternary text where the component renders {expanded ?
"Toon minder" : "Lees werder"} to use the correct Dutch phrase "Lees verder".
Locate the JSX in the Expandable component (the expression referencing the
expanded boolean) and replace "Lees werder" with "Lees verder" so the collapsed
label reads correctly.
In `@fdm-app/app/routes/organization`.$slug.settings.tsx:
- Around line 122-130: The pre-check using auth.api.checkOrganizationSlug is
race-prone; update the route handler to also catch and handle slug conflict
errors from the actual update call (the call to updateOrganization via
auth.api.updateOrganization or whatever method is used) so the user sees the
same "slug taken" message if another request races and claims it between the
check and the update. Specifically, wrap the updateOrganization invocation in a
try/catch, detect the SLUG_IS_TAKEN error code (or error.message) returned by
the API, and return the same validation/response flow you use for the pre-check
(same error text/status) instead of bubbling a generic failure. Ensure you
reference currentOrganization.slug and the existing checkOrganizationSlug logic
so behavior is consistent.
In `@fdm-app/app/routes/organization.new.tsx`:
- Around line 67-69: The catch block that checks for (e as any).body?.code ===
"SLUG_IS_TAKEN" must first ensure e is a non-null object to avoid crashing on
primitive/null throws; update the guard in the catch in organization.new.tsx so
it only accesses body when typeof e === "object" && e !== null (or use (e as
any)?.body?.code with an initial null/object check), and keep the existing
branch that returns dataWithError("SLUG_IS_TAKEN") while rethrowing or handling
other throw types normally.
---
Outside diff comments:
In `@fdm-app/app/routes/organization.invitations`.$invitation_id.respond.tsx:
- Around line 169-180: The code throws a 404 when invitation is missing before
honoring the "do_nothing" action, making the do_nothing branch unreachable;
update the request handling to read the form action (e.g., check for the
"do_nothing" value from formData or request) before calling
auth.api.getInvitation so that if action === "do_nothing" you skip the
invitation lookup and return/redirect accordingly; ensure the early-return or
redirect logic for the do_nothing branch is placed before the invocation of
auth.api.getInvitation and remove or adjust the subsequent
dataWithError("Invitation not found", ...) path so the do_nothing submit can
complete its redirect even when the invitation is absent.
---
Duplicate comments:
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 306-318: The current useMemo partition creates overlapping arrays
because userFarms filters only on userRoles.length > 0 while organizationFarms
filters on farm.organization, causing farms with both to appear in both lists;
update the userFarms filter to exclude farms that belong to an organization
(e.g., filter for farm.userRoles.length > 0 && !farm.organization) so that
userFarms and organizationFarms are mutually exclusive while preserving the
existing map that strips organization and organizationRoles.
- Around line 115-117: The role-matching logic uses session.principal_id which
can be an organization ID; change the comparison to match the signed-in user ID
instead by comparing member.userId to session.user.id (i.e., replace uses of
session.principal_id in the userRoles computation with session.user.id) so
membersListResponse.members.filter(...).map(...) correctly finds the current
user's roles; ensure any other places that derive roles for display use
session.user.id as well.
---
Nitpick comments:
In `@fdm-app/app/components/custom/expandable.tsx`:
- Around line 54-68: The myProps object (with onClick calling
setExpanded(!expanded)) is only applied to the Slot branch while the Button
branch declares its own inline onClick, causing duplication and confusion;
consolidate by moving the onClick logic into a single handler (e.g., a toggle
function or reuse myProps.onClick) and apply it to both branches—replace the
Button's inline onClick with the shared handler (or spread myProps) and ensure
the Slot branch still spreads {...props} {...myProps} so toggle behavior is
consistent for props.asChild true or false, referencing myProps, setExpanded,
expanded, Slot, Button, and props.asChild.
In `@fdm-app/app/lib/organization-helpers.ts`:
- Around line 21-27: The parseOrganizationMetadata function currently
JSON.parse's organization.metadata unconditionally which treats empty or missing
metadata as a parse error; update parseOrganizationMetadata to first check if
organization.metadata is falsy or an empty string and in that case return {
data: {} } (or the appropriate empty OrganizationMetadata shape) instead of
attempting JSON.parse, and apply the same guard to the similar parsing block
around lines 35-42 so both parsing flows bail out early on empty metadata and
only call JSON.parse when metadata is a non-empty string, returning { error }
only for actual parse exceptions.
In `@fdm-app/app/routes/organization._index.tsx`:
- Around line 44-62: The loader currently calls auth.api.listMembers once per
organization (inside the organizationsRaw.map async block), which can fan out;
instead fetch memberships in a single batched call or a single list of the
current user's memberships, then map those results into each organization to
compute userRoles and orderedUserRoles; locate the organizationsRaw processing
and replace the per-org auth.api.listMembers call with a pre-fetched membership
collection (keyed by organizationId) and reuse parseOrganizationMetadata and
session.principal_id to build each organization's userRoles and metadata.
In `@fdm-app/app/routes/organization`.$slug._index.tsx:
- Around line 122-123: The component creates a local const calendar via
getCalendarSelection() while the loader also returns calendar and the component
uses useCalendarStore for calendar state; pick one source-of-truth to eliminate
duplication. Either remove the local getCalendarSelection() usage and read
calendar exclusively from the loader-provided value (prop name calendar) or
initialize/rehydrate useCalendarStore with the loader's calendar value (e.g.,
call the store's setter in the component or during hydration) and remove direct
reads of getCalendarSelection(); update references to the calendar variable and
the useCalendarStore getter/setter so only the loader or the store is
authoritative (ensure getCalendarSelection(), calendar variable, and
useCalendarStore usage are all consistent).
In `@fdm-app/app/routes/organization`.$slug.members.tsx:
- Around line 228-234: Replace the duplicated inline permissions object type
used for member components with a single shared type alias or interface (e.g.,
"MemberPermissions") and use that alias wherever the inline shape appears (the
duplicated blocks at the permissions prop declaration and the second occurrence
around lines referenced in the review). Specifically, define: type
MemberPermissions = { canEdit: boolean; canDelete: boolean; canInvite: boolean;
canUpdateRoleUser: boolean; canRemoveUser: boolean } (export it if used across
modules), then update the component prop typings to use MemberPermissions
instead of repeating the inline object so both member-related props stay in
sync.
In `@fdm-app/app/routes/organization.new.tsx`:
- Around line 59-76: Extract the slug-availability logic into a shared server
helper (e.g., checkOrganizationSlugAvailability) that calls
auth.api.checkOrganizationSlug and maps the SLUG_IS_TAKEN error to the same
dataWithError message; replace the in-place try/catch in organization.new route
(the block using auth.api.checkOrganizationSlug and dataWithError) and the
similar block in organization.$slug.settings route with a single call to that
helper so both routes reuse identical logic and error mapping.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
fdm-app/app/components/blocks/email/invitation.tsxfdm-app/app/components/blocks/organization/form.tsxfdm-app/app/components/blocks/organization/organization-card.tsxfdm-app/app/components/blocks/organization/pending-organization-invitation.tsxfdm-app/app/components/custom/expandable.tsxfdm-app/app/lib/organization-helpers.tsfdm-app/app/routes/farm._index.tsxfdm-app/app/routes/organization.$slug._index.tsxfdm-app/app/routes/organization.$slug.members.tsxfdm-app/app/routes/organization.$slug.settings.tsxfdm-app/app/routes/organization._index.tsxfdm-app/app/routes/organization.invitations.$invitation_id.respond.tsxfdm-app/app/routes/organization.new.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- fdm-app/app/components/blocks/organization/organization-card.tsx
- fdm-app/app/components/blocks/organization/pending-organization-invitation.tsx
- fdm-app/app/components/blocks/organization/form.tsx
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
fdm-app/app/components/blocks/organization/form.tsx (1)
63-72:⚠️ Potential issue | 🟠 MajorSlug auto-sync is still non-reactive due snapshot reads.
At Line 63,
form.getValues("name")is a snapshot, so the effect may not re-run when the user edits the name. Use a subscribed value (watch/useWatch) for this effect trigger.🛠️ Proposed fix
- const organizationName = form.getValues("name") + const organizationName = form.watch("name") @@ - }, [organizationName, form.getValues, form.setValue]) + }, [organizationName, form.setValue])For react-hook-form v7, does getValues("field") subscribe to changes, or should watch/useWatch be used for reactive updates in useEffect?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/organization/form.tsx` around lines 63 - 72, The effect uses a snapshot via form.getValues("name") so it won't re-run as the user types; replace that with a subscribed value using react-hook-form's watch or useWatch (e.g., const organizationName = watch("name") or const organizationName = useWatch({ control: form.control, name: "name" })) and then update the useEffect to depend on organizationName and form.setValue (remove form.getValues from deps); keep the same slug logic (convertToSlug and form.setValue("slug", ...)) but trigger from the watched value so the slug auto-sync is reactive.fdm-app/app/routes/farm._index.tsx (3)
661-661:⚠️ Potential issue | 🟡 MinorDutch grammar regression in organization description text.
“in een gemakkelijke manier” should be “op een gemakkelijke manier”.
Also applies to: 727-727
🤖 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` at line 661, Update the Dutch description string used in the JSX prop named description in farm._index.tsx: replace "in een gemakkelijke manier" with the correct phrase "op een gemakkelijke manier" at the two occurrences (the description prop around the existing lines shown and the duplicate occurrence later). Ensure both string literals are updated so the organization description reads "Werk samen met andere gebruikers op een gemakkelijke manier."
115-117:⚠️ Potential issue | 🟠 MajorMatch organization member roles against the signed-in user ID.
At Line 116, comparing
member.userIdtosession.principal_idcan produce empty/incorrectuserRoleswhen the active principal is not the user ID. Compare tosession.user.idfor member-role matching.🛠️ Proposed fix
- const userRoles = membersListResponse.members - .filter((member) => member.userId === session.principal_id) - .map((member) => member.role) + const userRoles = membersListResponse.members + .filter((member) => member.userId === session.user.id) + .map((member) => member.role)#!/bin/bash # Verify how member identity is represented and where it is compared. rg -n -C3 'auth\.api\.listMembers|member\.userId|session\.(principal_id|user\.id)' --type ts🤖 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 115 - 117, userRoles is being computed by matching membersListResponse.members' member.userId against session.principal_id which can be the wrong identity; update the filter to compare member.userId to session.user.id instead so roles are matched to the signed-in user, i.e. change the predicate in the userRoles calculation (the filter on membersListResponse.members) from member.userId === session.principal_id to member.userId === session.user.id and keep the subsequent .map((member) => member.role) unchanged.
306-318:⚠️ Potential issue | 🟡 Minor
userFarmsandorganizationFarmsare not mutually exclusive.A farm with both direct user access and org access will render twice. Make partitions exclusive (prefer org bucket when
farm.organizationexists).♻️ Proposed fix
- const userFarms = loaderData.farms - .filter((farm) => farm.userRoles.length > 0) - .map((farm) => ({ - ...farm, - organization: undefined, - organizationRoles: undefined, - })) const organizationFarms = loaderData.farms.filter( (farm) => farm.organization, ) + const userFarms = loaderData.farms + .filter((farm) => farm.userRoles.length > 0 && !farm.organization) + .map((farm) => ({ + ...farm, + organization: undefined, + organizationRoles: undefined, + }))🤖 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 306 - 318, The current useMemo splits loaderData.farms into userFarms and organizationFarms but they overlap; change the partition so farms with an organization go only into organizationFarms and are excluded from userFarms. Update the useMemo that defines userFarms and organizationFarms: compute organizationFarms = loaderData.farms.filter(f => f.organization) and userFarms = loaderData.farms.filter(f => f.userRoles.length > 0 && !f.organization). Keep the existing mapping that strips organization/organizationRoles for userFarms (the spread and setting organization: undefined, organizationRoles: undefined) and return [userFarms, organizationFarms] from the same useMemo.
🧹 Nitpick comments (1)
fdm-app/app/routes/organization.$slug._index.tsx (1)
125-147: Batch field loading to reduce loader fan-out.Line 127 executes one
getFieldscall per farm. This can become slow for organizations with many farms. Consider a batched core method (fields-by-farmIds) and group results in-memory to keep loader latency predictable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/organization`.$slug._index.tsx around lines 125 - 147, The current farms.map loop calls getFields once per farm causing N requests; replace this with a batched fetch: add or call a fields-by-farmIds method (e.g., getFieldsByFarmIds) with the array of farm b_id_farm values, then group the returned fields by b_id_farm in-memory and compute b_area_farm for each farm from that grouped result; update the mapping that currently uses farms.map and getFields to instead reference the grouped fields lookup when computing farmArea and userRoles and remove per-farm getFields calls.
🤖 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/custom/expandable.tsx`:
- Around line 102-113: The measurement temporarily forces the "line-clamp-3"
class but always removes it afterwards, which can clear a clamp that was already
present; change the measurement to detect whether the clamp was originally
present (e.g., const hadClamp = el.classList.contains("line-clamp-3")), only add
the class if missing before measuring, then after measuring only remove it if
you added it (or otherwise restore the original state) so that
setCollapsedHeight, setFullHeight, and setIsOverflowing use the measurement
without mutating existing DOM state for children.
- Around line 123-130: The initial-render branch for the ExpandableContent
component omits restProps when fullHeight === null, so id/aria-/data- attributes
passed to ExpandableContent are dropped; update the early return in the
component (the block that checks fullHeight === null and renders the <div
className="w-full"> / inner div with ref={textRef} and
className={computedClassName}) to spread restProps onto the element that should
receive passthrough attributes (either the outer wrapper or the inner content
div as used elsewhere), ensuring restProps is applied alongside ref={textRef},
computedClassName, and children so attributes are present on initial render.
- Around line 64-67: The trigger is only visually hidden via "opacity-0" but
remains interactive; when !isOverflowing update the trigger element (the
component that uses className={cn(...)} and the isOverflowing flag) to also
remove it from interaction and the accessibility tree: add pointer-events-none
(or similar) to the className via cn when !isOverflowing, and set
aria-hidden="true" and tabIndex={-1} on the trigger (or conditionally avoid
rendering it) so it is not focusable or clickable by keyboard/screen-readers.
- Around line 49-71: ExpandableTrigger currently ignores incoming
ButtonHTMLAttributes and leaks the control prop `asChild` into Slot; fix by
destructuring props to pull out `asChild`, `children`, and any event handlers
you need (e.g., `onClick`), then spread the remaining buttonProps into the
non-`asChild` Button so attributes like `disabled`, `id`, `data-*`, `aria-*` are
preserved; when composing onClick, call the user-provided handler (if any) then
toggle setExpanded (or vice versa) to maintain expected behavior; for the
`asChild` branch, spread the same sanitized buttonProps (without `asChild`) into
Slot so `children` and other attributes are forwarded but `asChild` is not.
In `@fdm-app/app/lib/organization-helpers.ts`:
- Around line 28-31: The code currently wraps parsedMetadata.description in
JSON.parse(`"${parsedMetadata.description}"`) which turns undefined into the
literal string "undefined"; change the logic in organization-helpers.ts to only
run the normalization when parsedMetadata.description is a real string (e.g.,
typeof parsedMetadata.description === 'string' && parsedMetadata.description !==
undefined && parsedMetadata.description !== null) and skip JSON.parse otherwise
so missing descriptions are not coerced into the string "undefined".
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 656-753: The organization UI is nested inside the farms-length
gating logic so users with zero farms never see organizations; move the whole
organizations JSX block that renders FarmTitle plus the organizations list and
pendingOrganizationInvitations (the code that uses loaderData.organizations and
loaderData.pendingOrganizationInvitations and components OrganizationCard,
PendingOrganizationInvitationCard, and the "Nieuwe organisatie" Card/NavLink)
out of the farms-length conditional (the branch gated by loaderData.farms.length
!== 0) so it renders independently whenever loaderData.organizations.length > 0
or loaderData.pendingOrganizationInvitations.length > 0 (or show the empty-state
when both are zero), ensuring organization UI is no longer dependent on
loaderData.farms.
In `@fdm-app/app/routes/organization.new.tsx`:
- Around line 72-74: The redirect after creating an organization currently uses
redirectWithSuccess(`/organization/${formValues.slug}`, ...) which sends users
to the org dashboard; change the target route to the Members page (e.g.
`/organization/${formValues.slug}/members`) so new creators land on Members.
Update the call to redirectWithSuccess in organization.new.tsx to use the
members path while keeping the same success message payload (message:
`Organisatie ${formValues.name} is aangemaakt! 🎉`) and any existing behavior
around formValues.slug.
---
Duplicate comments:
In `@fdm-app/app/components/blocks/organization/form.tsx`:
- Around line 63-72: The effect uses a snapshot via form.getValues("name") so it
won't re-run as the user types; replace that with a subscribed value using
react-hook-form's watch or useWatch (e.g., const organizationName =
watch("name") or const organizationName = useWatch({ control: form.control,
name: "name" })) and then update the useEffect to depend on organizationName and
form.setValue (remove form.getValues from deps); keep the same slug logic
(convertToSlug and form.setValue("slug", ...)) but trigger from the watched
value so the slug auto-sync is reactive.
In `@fdm-app/app/routes/farm._index.tsx`:
- Line 661: Update the Dutch description string used in the JSX prop named
description in farm._index.tsx: replace "in een gemakkelijke manier" with the
correct phrase "op een gemakkelijke manier" at the two occurrences (the
description prop around the existing lines shown and the duplicate occurrence
later). Ensure both string literals are updated so the organization description
reads "Werk samen met andere gebruikers op een gemakkelijke manier."
- Around line 115-117: userRoles is being computed by matching
membersListResponse.members' member.userId against session.principal_id which
can be the wrong identity; update the filter to compare member.userId to
session.user.id instead so roles are matched to the signed-in user, i.e. change
the predicate in the userRoles calculation (the filter on
membersListResponse.members) from member.userId === session.principal_id to
member.userId === session.user.id and keep the subsequent .map((member) =>
member.role) unchanged.
- Around line 306-318: The current useMemo splits loaderData.farms into
userFarms and organizationFarms but they overlap; change the partition so farms
with an organization go only into organizationFarms and are excluded from
userFarms. Update the useMemo that defines userFarms and organizationFarms:
compute organizationFarms = loaderData.farms.filter(f => f.organization) and
userFarms = loaderData.farms.filter(f => f.userRoles.length > 0 &&
!f.organization). Keep the existing mapping that strips
organization/organizationRoles for userFarms (the spread and setting
organization: undefined, organizationRoles: undefined) and return [userFarms,
organizationFarms] from the same useMemo.
---
Nitpick comments:
In `@fdm-app/app/routes/organization`.$slug._index.tsx:
- Around line 125-147: The current farms.map loop calls getFields once per farm
causing N requests; replace this with a batched fetch: add or call a
fields-by-farmIds method (e.g., getFieldsByFarmIds) with the array of farm
b_id_farm values, then group the returned fields by b_id_farm in-memory and
compute b_area_farm for each farm from that grouped result; update the mapping
that currently uses farms.map and getFields to instead reference the grouped
fields lookup when computing farmArea and userRoles and remove per-farm
getFields calls.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
fdm-app/app/components/blocks/organization/form.tsxfdm-app/app/components/custom/expandable.tsxfdm-app/app/lib/organization-helpers.tsfdm-app/app/routes/farm.$b_id_farm.settings.delete.tsxfdm-app/app/routes/farm._index.tsxfdm-app/app/routes/organization.$slug._index.tsxfdm-app/app/routes/organization.$slug.members.tsxfdm-app/app/routes/organization.$slug.settings.tsxfdm-app/app/routes/organization._index.tsxfdm-app/app/routes/organization.new.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/routes/farm.$b_id_farm.settings.delete.tsx
Comment on lines
+49
to
+71
| type ExpandableTriggerProps = | ||
| | ({ asChild?: false } & ButtonHTMLAttributes<HTMLButtonElement>) | ||
| | { asChild: true; children: ReactNode } | ||
| export function ExpandableTrigger(props: ExpandableTriggerProps) { | ||
| const { expanded, setExpanded, isOverflowing } = useContext(ctx) | ||
| const myProps = { | ||
| onClick() { | ||
| setExpanded(!expanded) | ||
| }, | ||
| } | ||
| return props.asChild ? ( | ||
| <Slot {...props} {...myProps} /> | ||
| ) : ( | ||
| <Button | ||
| variant="link" | ||
| className={cn( | ||
| "-ms-4 transition-opacity duration-300 text-muted-foreground", | ||
| !isOverflowing && "opacity-0", | ||
| )} | ||
| onClick={() => setExpanded((prev) => !prev)} | ||
| > | ||
| {expanded ? "Toon minder" : "Lees verder"} | ||
| </Button> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n fdm-app/app/components/custom/expandable.tsxRepository: nmi-agro/fdm
Length of output: 5697
🏁 Script executed:
rg -n "ExpandableTrigger" --type=tsxRepository: nmi-agro/fdm
Length of output: 83
🏁 Script executed:
# Run the verification script from the review comment to find callsites with props
rg -nP --type=tsx -C2 '<ExpandableTrigger\b[^>]*(disabled|id=|data-|aria-|onClick=|className=|type=)'Repository: nmi-agro/fdm
Length of output: 83
🏁 Script executed:
rg -n "ExpandableTrigger" --type-list | head -20Repository: nmi-agro/fdm
Length of output: 445
🏁 Script executed:
# Search for ExpandableTrigger usage across TypeScript/TSX files
fd -e tsx -e ts | xargs rg -l "ExpandableTrigger"Repository: nmi-agro/fdm
Length of output: 149
🏁 Script executed:
# Search for ExpandableTrigger with context to see how it's being used
rg -n -A3 -B1 "ExpandableTrigger"Repository: nmi-agro/fdm
Length of output: 2512
🏁 Script executed:
# Get full context around ExpandableTrigger usage
rg -n -B5 -A5 "ExpandableTrigger" fdm-app/app/routes/organization.$slug._index.tsxRepository: nmi-agro/fdm
Length of output: 139
🏁 Script executed:
rg -n -B5 -A5 "ExpandableTrigger" "fdm-app/app/routes/organization.\$slug._index.tsx"Repository: nmi-agro/fdm
Length of output: 1273
🏁 Script executed:
# Check if there are any other ExpandableTrigger usages we might have missed
rg "ExpandableTrigger" --count-matchesRepository: nmi-agro/fdm
Length of output: 153
🏁 Script executed:
# Verify the Button component accepts the standard ButtonHTMLAttributes
rg -n -A10 "export.*Button" fdm-app/app/components/ui/button.tsx | head -30Repository: nmi-agro/fdm
Length of output: 92
ExpandableTrigger violates its type contract by ignoring ButtonHTMLAttributes in the non-asChild branch.
The type at lines 49–51 declares that ButtonHTMLAttributes<HTMLButtonElement> are accepted when asChild?: false, but the implementation at lines 62–71 ignores all incoming props and only spreads variant, className, and a hardcoded onClick. This breaks the API contract; callers cannot pass disabled, id, data-*, aria-*, or custom onClick handlers.
Additionally, line 60 spreads {...props} into Slot, which includes the asChild control prop—this should be destructured out and omitted.
🛠️ Proposed fix
export function ExpandableTrigger(props: ExpandableTriggerProps) {
- const { expanded, setExpanded, isOverflowing } = useContext(ctx)
- const myProps = {
- onClick() {
- setExpanded(!expanded)
- },
- }
- return props.asChild ? (
- <Slot {...props} {...myProps} />
- ) : (
+ const { expanded, setExpanded, isOverflowing } = useContext(ctx)
+ const toggle = () => setExpanded((prev) => !prev)
+
+ if (props.asChild) {
+ const { asChild: _asChild, children } = props
+ return <Slot onClick={toggle}>{children}</Slot>
+ }
+
+ const { asChild: _asChild, type, onClick, ...buttonProps } = props
+ return (
<Button
+ {...buttonProps}
+ type={type ?? "button"}
variant="link"
className={cn(
"-ms-4 transition-opacity duration-300 text-muted-foreground",
!isOverflowing && "opacity-0",
)}
- onClick={() => setExpanded((prev) => !prev)}
+ onClick={(e) => {
+ onClick?.(e)
+ if (!e.defaultPrevented) toggle()
+ }}
>
{expanded ? "Toon minder" : "Lees verder"}
</Button>
- )
+ )
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/components/custom/expandable.tsx` around lines 49 - 71,
ExpandableTrigger currently ignores incoming ButtonHTMLAttributes and leaks the
control prop `asChild` into Slot; fix by destructuring props to pull out
`asChild`, `children`, and any event handlers you need (e.g., `onClick`), then
spread the remaining buttonProps into the non-`asChild` Button so attributes
like `disabled`, `id`, `data-*`, `aria-*` are preserved; when composing onClick,
call the user-provided handler (if any) then toggle setExpanded (or vice versa)
to maintain expected behavior; for the `asChild` branch, spread the same
sanitized buttonProps (without `asChild`) into Slot so `children` and other
attributes are forwarded but `asChild` is not.
Comment on lines
+64
to
+67
| className={cn( | ||
| "-ms-4 transition-opacity duration-300 text-muted-foreground", | ||
| !isOverflowing && "opacity-0", | ||
| )} |
Contributor
There was a problem hiding this comment.
Invisible trigger remains interactive for keyboard/screen-reader users.
Line 66 hides the control visually with opacity-0, but it remains focusable/clickable. This creates an accessibility trap when content is not overflowing.
♿ Proposed fix
<Button
variant="link"
className={cn(
"-ms-4 transition-opacity duration-300 text-muted-foreground",
- !isOverflowing && "opacity-0",
+ !isOverflowing && "opacity-0 pointer-events-none",
)}
+ aria-hidden={!isOverflowing}
+ tabIndex={isOverflowing ? 0 : -1}
+ disabled={!isOverflowing}
onClick={() => setExpanded((prev) => !prev)}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/components/custom/expandable.tsx` around lines 64 - 67, The
trigger is only visually hidden via "opacity-0" but remains interactive; when
!isOverflowing update the trigger element (the component that uses
className={cn(...)} and the isOverflowing flag) to also remove it from
interaction and the accessibility tree: add pointer-events-none (or similar) to
the className via cn when !isOverflowing, and set aria-hidden="true" and
tabIndex={-1} on the trigger (or conditionally avoid rendering it) so it is not
focusable or clickable by keyboard/screen-readers.
Comment on lines
+102
to
+113
| el.classList.add("line-clamp-3") | ||
| const collapsed = el.clientHeight | ||
| const scroll = el.scrollHeight | ||
|
|
||
| setCollapsedHeight(collapsed) | ||
| setFullHeight(scroll) | ||
|
|
||
| // Overflow only depends on collapsed state | ||
| setIsOverflowing(scroll > collapsed + 1) | ||
|
|
||
| el.classList.remove("line-clamp-3") | ||
| }, [children]) |
Contributor
There was a problem hiding this comment.
Measurement step can accidentally clear an existing clamp class.
At Line 102-Line 113, line-clamp-3 is always removed after measurement. If it was already present before measurement, this can leave the DOM in the wrong visual state until another render.
🧪 Proposed fix
- el.classList.add("line-clamp-3")
+ const hadClamp = el.classList.contains("line-clamp-3")
+ if (!hadClamp) el.classList.add("line-clamp-3")
const collapsed = el.clientHeight
const scroll = el.scrollHeight
@@
- el.classList.remove("line-clamp-3")
+ if (!hadClamp) el.classList.remove("line-clamp-3")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/components/custom/expandable.tsx` around lines 102 - 113, The
measurement temporarily forces the "line-clamp-3" class but always removes it
afterwards, which can clear a clamp that was already present; change the
measurement to detect whether the clamp was originally present (e.g., const
hadClamp = el.classList.contains("line-clamp-3")), only add the class if missing
before measuring, then after measuring only remove it if you added it (or
otherwise restore the original state) so that setCollapsedHeight, setFullHeight,
and setIsOverflowing use the measurement without mutating existing DOM state for
children.
Comment on lines
+123
to
+130
| if (fullHeight === null) { | ||
| return ( | ||
| <div className="w-full"> | ||
| <div ref={textRef} className={computedClassName}> | ||
| {children} | ||
| </div> | ||
| </div> | ||
| ) |
Contributor
There was a problem hiding this comment.
Initial render branch drops passthrough attributes.
When fullHeight === null (Line 123), restProps are not applied, so id/aria-*/data-* passed to ExpandableContent are temporarily missing.
🧩 Proposed fix
if (fullHeight === null) {
return (
<div className="w-full">
- <div ref={textRef} className={computedClassName}>
+ <div {...restProps} ref={textRef} className={computedClassName}>
{children}
</div>
</div>
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (fullHeight === null) { | |
| return ( | |
| <div className="w-full"> | |
| <div ref={textRef} className={computedClassName}> | |
| {children} | |
| </div> | |
| </div> | |
| ) | |
| if (fullHeight === null) { | |
| return ( | |
| <div className="w-full"> | |
| <div {...restProps} ref={textRef} className={computedClassName}> | |
| {children} | |
| </div> | |
| </div> | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/components/custom/expandable.tsx` around lines 123 - 130, The
initial-render branch for the ExpandableContent component omits restProps when
fullHeight === null, so id/aria-/data- attributes passed to ExpandableContent
are dropped; update the early return in the component (the block that checks
fullHeight === null and renders the <div className="w-full"> / inner div with
ref={textRef} and className={computedClassName}) to spread restProps onto the
element that should receive passthrough attributes (either the outer wrapper or
the inner content div as used elsewhere), ensuring restProps is applied
alongside ref={textRef}, computedClassName, and children so attributes are
present on initial render.
Comment on lines
+28
to
+31
| try { | ||
| parsedMetadata.description = JSON.parse( | ||
| `"${parsedMetadata.description}"`, | ||
| ) |
Contributor
There was a problem hiding this comment.
Description normalization can inject literal "undefined" into metadata.
At Lines 28-31, missing descriptions are coerced into the string "undefined", which can surface in UI. Only normalize when description is actually a string.
🛠️ Proposed fix
- try {
- parsedMetadata.description = JSON.parse(
- `"${parsedMetadata.description}"`,
- )
- } catch (_triedAndDidNotWork) {}
+ if (
+ parsedMetadata &&
+ typeof parsedMetadata === "object" &&
+ typeof (parsedMetadata as OrganizationMetadata).description ===
+ "string"
+ ) {
+ try {
+ ;(parsedMetadata as OrganizationMetadata).description =
+ JSON.parse(
+ JSON.stringify(
+ (parsedMetadata as OrganizationMetadata).description,
+ ),
+ )
+ } catch (_triedAndDidNotWork) {}
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/lib/organization-helpers.ts` around lines 28 - 31, The code
currently wraps parsedMetadata.description in
JSON.parse(`"${parsedMetadata.description}"`) which turns undefined into the
literal string "undefined"; change the logic in organization-helpers.ts to only
run the normalization when parsedMetadata.description is a real string (e.g.,
typeof parsedMetadata.description === 'string' && parsedMetadata.description !==
undefined && parsedMetadata.description !== null) and skip JSON.parse otherwise
so missing descriptions are not coerced into the string "undefined".
Comment on lines
+656
to
+753
| {loaderData.organizations.length > 0 || | ||
| loaderData.pendingOrganizationInvitations.length > 0 ? ( | ||
| <> | ||
| <FarmTitle | ||
| title="Organisaties" | ||
| description="Werk samen met andere gebruikers op bedrijven in een gemakkelijke manier." | ||
| action={{ | ||
| label: "Naar organisaties", | ||
| to: "/organization", | ||
| }} | ||
| /> | ||
| {loaderData.organizations.length > 0 && ( | ||
| <div className="grid gap-6 p-6 md:p-10 md:pt-0 lg:grid-cols-2 xl:grid-cols-3"> | ||
| {loaderData.organizations.map( | ||
| (organization) => ( | ||
| <OrganizationCard | ||
| key={organization.id} | ||
| organization={organization} | ||
| /> | ||
| ), | ||
| )} | ||
|
|
||
| <Card className="flex flex-col border-dashed transition-all hover:border-primary/50 hover:bg-muted/50"> | ||
| <NavLink | ||
| to="/organization/new" | ||
| className="flex h-full flex-col" | ||
| > | ||
| <CardHeader className="grow items-center justify-center text-center"> | ||
| <div className="mb-4 flex h-12 w-12 items-center justify-center rounded-full bg-muted text-muted-foreground"> | ||
| <Plus className="h-6 w-6" /> | ||
| </div> | ||
| <CardTitle> | ||
| Nieuwe organisatie | ||
| </CardTitle> | ||
| <CardDescription> | ||
| Voeg een extra | ||
| organisatie toe aan uw | ||
| account. | ||
| </CardDescription> | ||
| </CardHeader> | ||
| </NavLink> | ||
| </Card> | ||
| </div> | ||
| )} | ||
| {loaderData.pendingOrganizationInvitations | ||
| .length > 0 && ( | ||
| <> | ||
| {loaderData.organizations.length > | ||
| 0 && ( | ||
| <FarmTitle | ||
| title="Openstaande uitnodigingen naar organisaties" | ||
| description="Je hebt uitnodigingen ontvangen voor toegang tot de volgende organisaties." | ||
| /> | ||
| )} | ||
| <div className="grid gap-6 p-6 md:p-10 md:pt-0 lg:grid-cols-2 xl:grid-cols-3"> | ||
| {loaderData.pendingOrganizationInvitations.map( | ||
| (invitation) => ( | ||
| <PendingOrganizationInvitationCard | ||
| key={invitation.id} | ||
| invitation={invitation} | ||
| /> | ||
| ), | ||
| )} | ||
| </div> | ||
| </> | ||
| )} | ||
| </> | ||
| ) : ( | ||
| <> | ||
| <FarmTitle | ||
| title="Organisaties" | ||
| description="Werk samen met andere gebruikers op bedrijven in een gemakkelijke manier." | ||
| action={{ | ||
| label: "Naar organisaties", | ||
| to: "/organization", | ||
| }} | ||
| /> | ||
| <div className="mx-auto flex items-center flex-col justify-center space-y-6 sm:w-87.5 mb-6"> | ||
| <div className="flex flex-col space-y-2 text-center"> | ||
| <h1 className="text-2xl font-semibold tracking-tight"> | ||
| Het lijkt erop dat je nog geen | ||
| organisatie hebt. | ||
| </h1> | ||
| </div> | ||
| <div className="flex flex-col items-center relative"> | ||
| <Button asChild> | ||
| <NavLink to="/organization/new"> | ||
| Maak een organisatie | ||
| </NavLink> | ||
| </Button> | ||
| </div> | ||
| <p className="text-center text-sm text-muted-foreground"> | ||
| of kunt u organisaties vragen om u uit | ||
| te nodigen. | ||
| </p> | ||
| </div> | ||
| </> | ||
| )} |
Contributor
There was a problem hiding this comment.
Organization content is unreachable for users with zero farms.
These organization sections only render inside the loaderData.farms.length !== 0 branch (Line 329 gate). Users with no farms but with organization memberships/invitations won’t see this UI.
🤖 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 656 - 753, The organization
UI is nested inside the farms-length gating logic so users with zero farms never
see organizations; move the whole organizations JSX block that renders FarmTitle
plus the organizations list and pendingOrganizationInvitations (the code that
uses loaderData.organizations and loaderData.pendingOrganizationInvitations and
components OrganizationCard, PendingOrganizationInvitationCard, and the "Nieuwe
organisatie" Card/NavLink) out of the farms-length conditional (the branch gated
by loaderData.farms.length !== 0) so it renders independently whenever
loaderData.organizations.length > 0 or
loaderData.pendingOrganizationInvitations.length > 0 (or show the empty-state
when both are zero), ensuring organization UI is no longer dependent on
loaderData.farms.
Comment on lines
+72
to
+74
| return redirectWithSuccess(`/organization/${formValues.slug}`, { | ||
| message: `Organisatie ${formValues.name} is aangemaakt! 🎉`, | ||
| }) |
Contributor
There was a problem hiding this comment.
Creation redirect does not match the stated UX goal.
After create, users are sent to the org dashboard, but the objective for this PR is to route new creators to Members.
🛠️ Proposed fix
- return redirectWithSuccess(`/organization/${formValues.slug}`, {
+ return redirectWithSuccess(`/organization/${formValues.slug}/members`, {
message: `Organisatie ${formValues.name} is aangemaakt! 🎉`,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return redirectWithSuccess(`/organization/${formValues.slug}`, { | |
| message: `Organisatie ${formValues.name} is aangemaakt! 🎉`, | |
| }) | |
| return redirectWithSuccess(`/organization/${formValues.slug}/members`, { | |
| message: `Organisatie ${formValues.name} is aangemaakt! 🎉`, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/routes/organization.new.tsx` around lines 72 - 74, The redirect
after creating an organization currently uses
redirectWithSuccess(`/organization/${formValues.slug}`, ...) which sends users
to the org dashboard; change the target route to the Members page (e.g.
`/organization/${formValues.slug}/members`) so new creators land on Members.
Update the call to redirectWithSuccess in organization.new.tsx to use the
members path while keeping the same success message payload (message:
`Organisatie ${formValues.name} is aangemaakt! 🎉`) and any existing behavior
around formValues.slug.
SvenVw
reviewed
Mar 4, 2026
| )} | ||
| > | ||
| <NavLink to={"/organization/invitations"}> | ||
| <NavLink |
Collaborator
There was a problem hiding this comment.
This link would result in a 404?
SvenVw
approved these changes
Mar 4, 2026
Collaborator
SvenVw
left a comment
SvenVw
left a comment
There was a problem hiding this comment.
Thanks for improving the organization functions!
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.
New Features
Closes #401
Summary by CodeRabbit
New Features
Improvements
Chores