refactor: multi-tenant SaaS with slug-based routing, sidebar layout, and security hardening#108
Merged
refactor: multi-tenant SaaS with slug-based routing, sidebar layout, and security hardening#108
Conversation
…ysis Experimental sandbox for testing hypotheses about code review bottlenecks. Includes data fetching from GitHub API, PR size classification, queue visualization, throughput correlation, and auto-merge simulation experiments. Sensitive data (JSON outputs, notes, raw data) is gitignored. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add Gemini Flash Lite classification to compare with rule-based sizing. Key finding: XS+S auto-merge reduces review queue by 50% with LLM classification vs 19% with rule-based, at $0.06 for 971 PRs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…inks - Add owner to PRSizeInfo type and fetchPRSizes output - Backfill owner from DB when merging cached per-repo files - Include owner in experiment 004/005 output data - Add LLM automerge simulation visualization HTML with PR links Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ion report - Add lab/lib/report.ts: builder API for self-contained HTML reports (inline data, Canvas 2D charts, GitHub Dark theme, no server needed) - Rewrite 005 report: "AI auto-merge simulation" focused on impact (removed rule-based comparison, plain language, annotated statistics) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add organizations.slug as NOT NULL with unique index - Add auth helpers: requireOrgMember, requireOrgAdmin, getUserOrganizations, getFirstOrganization, isReservedSlug - Move dashboard routes from _dashboard+/$organization to $orgSlug/ - Move org settings from admin+/$organization.* to $orgSlug/settings/ - All org-scoped routes now guarded by membership checks - Org switcher uses slug-based URLs instead of IDs - Add root index redirect (/ → first org or /no-org) - Add /no-org page for users without organization membership - Shrink /admin to superadmin-only (org list + create) - Fix listUserOrganizations to filter by membership - Update seed to use nanoid for org ID + separate slug Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…itHub Users Replace Card+Tabs layout with SidebarProvider shell (org switcher, nav groups, user menu). Add full data tables with server-side pagination, sorting, and search for Members, Repositories, and GitHub Users. Implement inline CRUD actions (role change, delete, edit) using ConfirmDialog + fetcher pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Settings page now creates a default organizationSetting row when one doesn't exist, instead of throwing a 404. Also add a guard in db.server.ts to fail fast if DATABASE_URL is not set, preventing stray .db files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add organizationSettings and exportSettings to seed data - Clear existing data before seeding to allow re-runs - Use `import 'dotenv/config'` to load env before db.server.ts - Rename `setup` to `db:setup` to avoid pnpm builtin conflict Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SQLite accepts '==' but '=' is the standard SQL equality operator. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…bases Adds idempotent data migration in start.sh that inserts admin users as org owners when no members exist, ensuring smooth transition to the new membership-based auth system on Fly.io deploy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds multi-tenant org-scoped routing and auth helpers, enforces non-null organization slugs in the DB, introduces an OrganizationScopePlugin, refactors routes and loaders to use orgSlug-based context, adds many layout/UI components (sidebar, header, data tables, dialogs, hooks), adds lab analytics tools (LLM classification, experiments, reports), and updates tooling/config (routing, scripts, vitest). Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/routes/admin+/create/mutations.server.ts (1)
16-28:⚠️ Potential issue | 🟠 MajorNo membership is created for the new organization.
The
createOrganizationmutation only inserts into theorganizationstable and returns without creating any member record. SincerequireOrgMemberqueries thememberstable and redirects to/no-orgif no membership exists, the admin who created the organization cannot access it. Either the creating admin should automatically become an owner when the organization is created, or this membership must be manually added before the organization is usable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/admin`+/create/mutations.server.ts around lines 16 - 28, The transaction that creates the organization only inserts into 'organizations' and must also create a membership for the creating admin so they can access the org; after the tsx.insertInto('organizations') call, add a tsx.insertInto('members') within the same transaction to create a member row (use nanoid() for id, set organization_id to the newly created organization.id, user_id to the current user's id from the request/session, and role to 'owner' or equivalent), return or include that member with the response, and ensure this uses the same transaction (tsx) so requireOrgMember will find the membership immediately.app/routes/$orgSlug/settings/_index/forms/export-settings.action.server.ts (1)
9-9:⚠️ Potential issue | 🟡 MinorMissing
awaitonparseWithZod— inconsistent with all sibling action files.Every other settings action in this directory (
integration-settings,organization-settings) awaitsparseWithZod. IfexportSettingsSchemagains async refinements in the future,submissionbecomes aPromiseand the early-return guard onsubmission.statussilently stops working, allowing auth and mutations to proceed on invalid input.📝 Proposed fix
- const submission = parseWithZod(await request.formData(), { schema }) + const submission = await parseWithZod(await request.formData(), { schema })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/forms/export-settings.action.server.ts at line 9, The code calls parseWithZod without awaiting it, so change the assignment in export-settings.action.server.ts to await parseWithZod(...) so submission is the resolved result (not a Promise); locate the line with "const submission = parseWithZod(await request.formData(), { schema })" and add the missing await, ensuring the subsequent guard that checks submission.status operates on the parsed object synchronously (consistent with other action files like integration-settings and organization-settings).app/routes/$orgSlug/settings/_index/forms/integration-settings.action.server.ts (1)
34-42:⚠️ Potential issue | 🟡 MinorWrong success toast message — copy-paste from
export-settings.action.server.ts.
'Update export settings successfully'is displayed for an integration settings update.📝 Proposed fix
- message: 'Update export settings successfully', + message: 'Update integration settings successfully',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/forms/integration-settings.action.server.ts around lines 34 - 42, Update the success message returned by dataWithSuccess in the integration settings action so it reflects integration settings (not export settings); locate the return that sets intent: INTENTS.integrationSettings and lastResult: submission.reply() in integration-settings.action.server.ts and change the message from 'Update export settings successfully' to a correct string like 'Update integration settings successfully' (or the project's canonical phrasing for integration updates).
♻️ Duplicate comments (6)
app/routes/$orgSlug/settings/repositories._index/+hooks/use-data-table-state.ts (1)
9-43: Same schema crash risk on invalid URL params as the github-users hook.Invalid URL values for
sort_orderorpagewill throw aZodError, crashing the component. Add.catch()fallbacks as noted in the github-users review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories._index/+hooks/use-data-table-state.ts around lines 9 - 43, Query parsing currently can throw ZodError for invalid URL params (sort_order, page) via QuerySchema/SortSchema/PaginationSchema; change the code that parses URL params to use safe parsing and fall back to defaults instead of letting Zod throw. Specifically, when reading params for QuerySchema, SortSchema, and PaginationSchema, replace direct parse/parseAsync calls with safeParse (or wrap in try/catch) and on failure return the schema's defaults (e.g., repo: '', sort_order: 'asc', page: 1, per_page: PAGINATION_PER_PAGE_DEFAULT) so the component doesn't crash on malformed URL values.app/routes/$orgSlug/settings/members/+components/data-table-column-header.tsx (1)
1-83: Duplicate of the repositories variant — see comment on the repositories file.Same component duplicated verbatim. The duplication comment on the repositories version applies here as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/members/+components/data-table-column-header.tsx around lines 1 - 83, This component DataTableColumnHeader is a verbatim duplicate of the one in the repositories area; refactor by removing the duplicate and extracting a single shared component used by both places: create a shared DataTableColumnHeader component (preserving the props interface DataTableColumnHeaderProps and the function name DataTableColumnHeader) that imports useDataTableState and the same UI primitives, then update the callers to import this single component instead of their local copies; ensure exported names and props match exactly (column, title, className) so consumers need minimal changes and preserve behavior for updateSort, column.getCanSort(), and column.toggleVisibility().app/routes/$orgSlug/settings/repositories._index/+components/repo-table.tsx (1)
25-29: DuplicateColumnMetamodule augmentation — same as members-table and github-users-table.Already flagged in
members-table.tsx. This is the same augmentation repeated across all three table files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories._index/+components/repo-table.tsx around lines 25 - 29, The duplicate module augmentation for ColumnMeta (interface ColumnMeta<TData extends RowData, TValue> { className: string }) should be removed from this file and other table files (members-table.tsx and github-users-table.tsx); instead create a single shared declaration (e.g., a global/react-table.d.ts or shared types file) that augments '@tanstack/react-table' once with ColumnMeta and import/include it globally so all tables use that single augmentation.app/routes/$orgSlug/settings/github-users._index/+components/github-users-table.tsx (1)
25-29: DuplicateColumnMetamodule augmentation — same as members-table and repo-table.Already flagged in
members-table.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/github-users._index/+components/github-users-table.tsx around lines 25 - 29, Duplicate ambient augmentation of ColumnMeta is present in multiple components; remove the duplicate declare module '@tanstack/react-table' blocks from github-users-table.tsx (and the other components like members-table.tsx and repo-table.tsx) and consolidate the augmentation into a single shared ambient declaration file that TypeScript picks up (e.g., create a new react-table-augmentations.d.ts with the declare module '@tanstack/react-table' { interface ColumnMeta<TData extends RowData, TValue> { className: string } }), then ensure that file is included in tsconfig so the single ColumnMeta augmentation is used project-wide.app/routes/$orgSlug/settings/repositories._index/+components/data-table-pagination.tsx (1)
89-108: Same zero-items pagination bug as noted in the github-users pagination component.The
disabledconditions on next/last buttons don't handletotalPages = 0correctly. See the fix proposed in the github-usersdata-table-pagination.tsxreview.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories._index/+components/data-table-pagination.tsx around lines 89 - 108, The next/last Button disabled logic fails when totalPages is 0 (zero-items case) causing incorrect enabled state; update the disabled expressions for the ChevronRightIcon and ChevronsRightIcon buttons (the Button components using onClick={() => updatePagination({ page: currentPage + 1 })} and onClick={() => updatePagination({ page: totalPages })}) to explicitly handle totalPages <= 1 or totalPages === 0 (e.g., use currentPage >= totalPages || totalPages <= 1 or totalPages === 0) so the buttons are disabled when there are no pages—apply the same guard used in the github-users data-table-pagination fix to both next and last buttons.app/routes/$orgSlug/settings/members/+components/data-table-pagination.tsx (1)
89-108: Same zero-items pagination bug as noted in the github-users pagination component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/members/+components/data-table-pagination.tsx around lines 89 - 108, The pagination buttons incorrectly handle the zero-items case (totalPages === 0); update the disabled logic on the next/last Buttons to use totalPages <= 1 instead of totalPages === 1 and ensure updatePagination calls (used in the onClick handlers) clamp the target page between 1 and totalPages (e.g., when calling updatePagination({ page: currentPage + 1 }) or updatePagination({ page: totalPages })) so clicking cannot set page 0 or > totalPages; reference the Button onClick handlers, updatePagination, currentPage and totalPages when making the change.
🟠 Major comments (16)
app/components/layout/main.tsx-8-18 (1)
8-18:⚠️ Potential issue | 🟠 Major
classNamefrom{...props}silently overrides base classes — destructure and merge itIn JSX, a later prop wins over an earlier one with the same key. Because
{...props}is spread after the explicitclassNameattribute, anyclassNamepassed by a consumer (e.g.<Main className="mt-8">) replaces the entirecn(...)result instead of merging with it. This makes it impossible for callers to augment the base styles.🐛 Proposed fix
-export const Main = ({ fixed, ...props }: MainProps) => { +export const Main = ({ fixed, className, ...props }: MainProps) => { return ( <main className={cn( 'px-4 py-6', fixed && 'fixed-main flex grow flex-col overflow-hidden', + className, )} {...props} /> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/layout/main.tsx` around lines 8 - 18, The Main component currently spreads {...props} after setting className so any incoming className silently overrides the computed classes; destructure className from props (e.g., in Main's parameter or inside the function), merge it with the computed classes via cn including the fixed conditional (the 'fixed-main flex grow flex-col overflow-hidden' expression), then spread the remaining props (rest) after using the merged className so callers can augment rather than replace the base classes; update references to Main and the fixed conditional accordingly.app/hooks/use-debounce.ts-5-19 (1)
5-19:⚠️ Potential issue | 🟠 MajorMissing cleanup on unmount — pending timer fires after component is gone
timer.currentis never cleared when the host component unmounts. If the user navigates away while a debounce is in flight, thefncallback (typically asetSearchParamsor URL push) executes in the context of the old route, potentially causing stale navigation or React warnings.Add a
useEffectcleanup:🛡️ Proposed fix
-import { useCallback, useRef } from 'react' +import { useCallback, useEffect, useRef } from 'react' type Debounce = (fn: () => void) => void export const useDebounce = (timeout = 200): Debounce => { const timer = useRef<ReturnType<typeof setTimeout> | null>(null) + + useEffect(() => { + return () => { + if (timer.current) { + clearTimeout(timer.current) + } + } + }, []) + const debounce: Debounce = useCallback( (fn) => { if (timer.current) { clearTimeout(timer.current) } timer.current = setTimeout(() => { fn() }, timeout) }, [timeout], ) return debounce }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/hooks/use-debounce.ts` around lines 5 - 19, The useDebounce hook leaves timer.current active on unmount causing the debounced fn to run after the component is gone; add a cleanup effect in useDebounce that clears timer.current on unmount (useEffect with return that calls clearTimeout(timer.current) and sets timer.current = null) so pending timeouts are cancelled; update the hook surrounding the existing debounce (function useDebounce, const timer) to include this useEffect cleanup to prevent stale callbacks (e.g., setSearchParams) from firing.app/routes/$orgSlug/settings/_index/functions/queries.server.ts-23-38 (1)
23-38:⚠️ Potential issue | 🟠 MajorConcurrent inserts are not idempotent — add conflict handling
If the settings page is loaded simultaneously in two tabs (or during a retry), both requests will find no row and each will call
createDefaultOrganizationSetting. On aUNIQUEconstraint onorganizationId, the second insert throws a DB error; without one, duplicate rows are silently created and futureexecuteTakeFirst()calls return an arbitrary row.Add
.onConflicthandling to make the operation idempotent:🛡️ Proposed fix
await db .insertInto('organizationSettings') .values({ id, organizationId, updatedAt: new Date().toISOString(), }) + .onConflict((oc) => oc.doNothing()) .execute()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/functions/queries.server.ts around lines 23 - 38, The createDefaultOrganizationSetting function can race and cause duplicate/failed inserts; modify the db.insertInto('organizationSettings') call inside createDefaultOrganizationSetting to add onConflict handling (e.g., .onConflict(conflict => conflict.column('organizationId').doNothing()) or an appropriate .doUpdate to set updatedAt) so the insert becomes idempotent, then continue to call getOrganizationSetting(organizationId) and return that row; ensure the conflict target is organizationId and that the function still throws if getOrganizationSetting returns no row.app/routes/$orgSlug/settings/members/mutations.server.ts-11-13 (1)
11-13:⚠️ Potential issue | 🟠 MajorAdd guard against removing the organization's sole owner
The
removeMembercall at line 77 lacks validation that the member is not the organization's only owner. Deleting the sole owner would render the organization unmanageable. Before invokingremoveMember, check that the member being removed is not the onlyownerin the organization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/members/mutations.server.ts around lines 11 - 13, The removeMember function must guard against deleting the organization's sole owner: first SELECT the member row by id (use removeMember's memberId to query the members table and read org_id/orgId and role), and if the member.role === 'owner' then COUNT members WHERE org_id = that org and role = 'owner'; if that count is <= 1 throw an error (or return a validation failure) instead of executing the DELETE; only call db.deleteFrom('members').where('id','=',memberId).execute() after this check.app/routes/resources+/organization/functions/queries.ts-1-11 (1)
1-11: 🛠️ Refactor suggestion | 🟠 MajorFile should use
.server.tssuffix per coding guidelines.This file imports
dbfrom~/app/services/db.server, making it server-only code. However, the filename isqueries.tsinstead ofqueries.server.ts. If this module is ever transitively imported from a client component, it will break the build or leak server internals.Other similar files in this PR (e.g.,
members/queries.server.ts,github-users._index/queries.server.ts) correctly use the.server.tssuffix.As per coding guidelines, "Use .server.ts suffix for server-only code that should not be bundled for the client (queries.server.ts, mutations.server.ts, functions.server.ts, *.action.server.ts)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/resources`+/organization/functions/queries.ts around lines 1 - 11, This module imports server-only db and must be server-only: rename the file from queries.ts to queries.server.ts and update any imports that reference this module to the new filename; ensure the exported function listUserOrganizations and its import of db from '~/app/services/db.server' remain unchanged so the module stays server-only and won't be bundled into client code.lab/lib/classify.ts-129-137 (1)
129-137:⚠️ Potential issue | 🟠 MajorXL rule on line 136 is dead code — shadowed by L rules above.
The L-classification at line 130 (
hasAuthChange → return 'L') will always fire before line 136 (hasDBChange && hasAPIChange && hasAuthChange → return 'XL') can be reached, becausehasAuthChangealone is sufficient for L.Similarly,
hasDBChange && hasAPIChangereturns L at line 131, so any PR matching the XL condition on line 136 is already captured as L.If the intent is that the triple-risk combination should be XL, move the XL checks above the L checks.
🔧 Proposed fix: reorder XL before L
+ // XL: System-wide (check before L to avoid shadowing) + if (totalLines > 1000 && changedFiles > 30) return 'XL' + if (hasDBChange && hasAPIChange && hasAuthChange) return 'XL' + if (componentDirs.size > 10) return 'XL' + // L: Wide impact or critical domain if (hasPaymentChange || hasAuthChange) return 'L' if (hasDBChange && hasAPIChange) return 'L' if (totalLines > 500 && changedFiles > 15) return 'L' - // XL: System-wide - if (totalLines > 1000 && changedFiles > 30) return 'XL' - if (hasDBChange && hasAPIChange && hasAuthChange) return 'XL' - if (componentDirs.size > 10) return 'XL'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lab/lib/classify.ts` around lines 129 - 137, The XL rule (return 'XL') that checks hasDBChange && hasAPIChange && hasAuthChange is dead because earlier L rules (checks using hasAuthChange and hasDBChange && hasAPIChange) short-circuit; to fix, move the XL-level checks above the L-level checks in lab/lib/classify.ts (or explicitly test the triple-risk condition before the single/double-risk L conditions) so that the function evaluates the higher-severity condition (hasDBChange && hasAPIChange && hasAuthChange, componentDirs.size > 10, totalLines > 1000 && changedFiles > 30) first and only falls back to the L rules (hasAuthChange, hasDBChange && hasAPIChange, totalLines > 500 && changedFiles > 15) afterwards.app/routes/$orgSlug/settings/repositories._index/+hooks/use-data-table-state.ts-1-148 (1)
1-148: 🛠️ Refactor suggestion | 🟠 MajorNear-identical duplication with the github-users and members
use-data-table-statehooks.This file shares ~95% of its code with
app/routes/$orgSlug/settings/github-users._index/+hooks/use-data-table-state.ts— only theQuerySchemafield differs (repovssearch). TheSortSchema,PaginationSchema, constants, types, and all update/reset functions are identical.Extract a shared, generic
useDataTableState<TQuerySchema>into a common location (e.g.,~/app/hooks/use-data-table-state.ts) that accepts the query schema as a parameter:// ~/app/hooks/use-data-table-state.ts export function useDataTableState<T extends z.ZodObject<any>>(querySchema: T) { // shared logic, parameterized by querySchema }Each route-specific hook would then simply define its
QuerySchemaand call the shared hook.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories._index/+hooks/use-data-table-state.ts around lines 1 - 148, Extract the duplicated table-state logic into a shared generic hook named useDataTableState<T extends z.ZodObject<any>>(querySchema) that lives in a common module and replace the route-local implementations with thin wrappers that only define their QuerySchema and call the shared hook. Move the constants PAGINATION_PER_PAGE_DEFAULT and PAGINATION_PER_PAGE_ITEMS, the SortSchema, PaginationSchema, the Pagination/Sort/Queries types, and the logic that uses useSearchParams, useDebounce, and setSearchParams (including updateQueries, updateSort, updatePagination, isFiltered, resetFilters) into the shared hook; ensure the shared hook parses query values using the provided querySchema (instead of the local QuerySchema) and returns the same API the routes expect. Ensure type inference for the query shape uses the generic T so route wrappers can call useDataTableState(QuerySchema) and keep all existing function names (updateQueries, updateSort, updatePagination, resetFilters) and behaviors unchanged.lab/fetch.ts-27-28 (1)
27-28:⚠️ Potential issue | 🟠 Major
--max-pageswithout a value silently producesNaN, causing zero pages to be fetched.
Number(undefined)evaluates toNaN. SinceNaNis notnull/undefined, theopts.maxPages ?? 8fallback ingithub.tswon't activate, andpage < NaNis alwaysfalse, so the loop never executes — no data is fetched with no error message.Proposed fix — validate the parsed number
const maxPagesIdx = args.indexOf('--max-pages') -const maxPages = maxPagesIdx >= 0 ? Number(args[maxPagesIdx + 1]) : 8 +const maxPages = + maxPagesIdx >= 0 ? (Number(args[maxPagesIdx + 1]) || 8) : 8🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lab/fetch.ts` around lines 27 - 28, The parsing of --max-pages (vars maxPagesIdx and maxPages) can yield NaN when no value is provided; change the logic to explicitly validate the next arg exists and is a finite integer before using it (e.g., check args[maxPagesIdx + 1] !== undefined and Number.isFinite(Number(...)) or parseInt result is a valid number) and otherwise set maxPages to the default (8) or leave undefined so github.ts's opts.maxPages fallback works; ensure the code uses a validated numeric value for maxPages to avoid passing NaN into the page loop (refer to maxPagesIdx, maxPages, and opts.maxPages).app/routes/$orgSlug/settings/github-users._index/+hooks/use-data-table-state.ts-9-43 (1)
9-43:⚠️ Potential issue | 🟠 MajorSchemas throw on invalid URL params (e.g.,
?sort_order=fooor?page=abc), crashing the component.
SortSchema'sz.union([z.literal('asc'), z.literal('desc')])will throw aZodErrorif the URL contains an unexpected value. Similarly,PaginationSchemawill happily transformpage=abctoNaNviaNumber(). Consider adding.catch()to each schema field to gracefully fall back to defaults on invalid input:Example for sort_order
sort_order: z.preprocess( (val) => (val === null ? undefined : val), z .union([z.literal('asc'), z.literal('desc')]) .optional() .default('asc'), - ), + ).catch('asc'),Similarly for
page— consider validating the transformed number is a positive integer, or use.catch(1).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/github-users._index/+hooks/use-data-table-state.ts around lines 9 - 43, The current QuerySchema/SortSchema/PaginationSchema can throw on invalid URL params (e.g., sort_order or page) — update SortSchema.sort_order and PaginationSchema.page (and per_page) to gracefully handle bad input by adding safe fallbacks: keep the existing preprocess/transform but append .catch(...) on the field schemas (or replace the transform with an explicit number coercion + .refine(...).catch(...)) so invalid values fall back to defaults (e.g., 'asc' for sort_order, 1 for page, PAGINATION_PER_PAGE_DEFAULT for per_page) and ensure page is validated as a positive integer (use .refine or z.coerce.number().int().positive() with .catch(1)). Target the SortSchema (symbol sort_order) and PaginationSchema (symbols page, per_page) for these changes.app/components/layout/nav-group.tsx-172-181 (1)
172-181:⚠️ Potential issue | 🟠 MajorThe mainNav check has a latent bug in the path-matching logic, but this doesn't currently manifest because no collapsible nav items with subitems exist yet.
Currently, all items in
nav-config.tsare flat—they have no.itemsproperty. However, the logic incheckIsActive(lines 172–181) would incorrectly match all items within the same org if collapsible items were added in the future.When
mainNav=true, comparing onlyhref.split('/')[1](the orgSlug) againstitem.url.split('/')[1]would incorrectly returntruefor any nav item in the same organization, regardless of the deeper path. For example, navigating to/{orgSlug}/settings/memberswould incorrectly mark a collapsible item at/{orgSlug}/ongoingas active, causing all sections to open.Update the mainNav comparison to account for deeper path segments:
Proposed fix
function checkIsActive(href: string, item: NavItem, mainNav = false) { return ( href === item.url || href.split('?')[0] === item.url || !!item?.items?.filter((i) => i.url === href).length || (mainNav && - href.split('/')[1] !== '' && - href.split('/')[1] === item?.url?.split('/')[1]) + item.url != null && + href.startsWith(item.url)) ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/layout/nav-group.tsx` around lines 172 - 181, The mainNav branch in checkIsActive incorrectly matches only the org slug (href.split('/')[1]) which would mark all items in the same org active; change that comparison to match the first two non-empty path segments (e.g. compare href.split('/').filter(Boolean).slice(0,2).join('/') to item.url.split('/').filter(Boolean).slice(0,2).join('/')) and ensure item.url is defined before splitting so collapsible nav items only become active when org+section match rather than every item in the org.app/routes/$orgSlug/settings/repositories.$repository.delete/route.tsx-22-43 (1)
22-43:⚠️ Potential issue | 🟠 MajorSecurity: Missing org-ownership check on
repositoryId(IDOR).
requireOrgAdminverifies the user is admin oforgSlug, butrepositoryIdis taken directly from URL params and used without verifying thatrepository.organizationId === organization.id. An admin of org A can craft/:orgA/settings/repositories/:repoBId/deleteto read (loader) or permanently delete (action) a repository belonging to org B.🔒 Proposed fix — add ownership guard in both loader and action
export const loader = async ({ request, params }: Route.LoaderArgs) => { const { organization } = await requireOrgAdmin(request, params.orgSlug) const { repository: repositoryId } = zx.parseParams(params, { repository: z.string(), }) const repository = await getRepository(repositoryId) - if (!repository) { - throw new Error('repository not found') - } + if (!repository || repository.organizationId !== organization.id) { + throw new Response('repository not found', { status: 404 }) + } return { organization, repositoryId, repository } } export const action = async ({ request, params }: Route.ActionArgs) => { const { organization } = await requireOrgAdmin(request, params.orgSlug) const { repository: repositoryId } = zx.parseParams(params, { repository: z.string(), }) + const repository = await getRepository(repositoryId) + if (!repository || repository.organizationId !== organization.id) { + throw new Response('repository not found', { status: 404 }) + } + await deleteRepository(repositoryId) return redirect(`/${organization.slug}/settings/repositories`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.$repository.delete/route.tsx around lines 22 - 43, The loader and action take repositoryId from params and call getRepository/deleteRepository without verifying the repository belongs to the org returned by requireOrgAdmin, enabling IDOR; update both loader and action to fetch the repository via getRepository(repositoryId) (or reuse the loader result in action), check repository.organizationId === organization.id, and if mismatched or repository not found throw a 404 or 403 (e.g., throw new Response(null, { status: 404 }) or throw new Error) before proceeding to return the data or call deleteRepository; reference the loader, action, requireOrgAdmin, getRepository, deleteRepository, repository.organizationId and organization.id symbols when making the checks.app/routes/$orgSlug/settings/members/+components/data-table-toolbar.tsx-1-11 (1)
1-11: 🛠️ Refactor suggestion | 🟠 MajorRemove unused
tableprop,DataTableToolbarProps, andTableimport.The component ignores
_propsentirely and relies solely onuseDataTableState()for its state. TheTable<TData>prop, the generic<TData>type parameter, and theTableimport from@tanstack/react-tableare all dead code — the other two toolbars in this PR (repositories, github-users) correctly omit this prop.♻️ Proposed fix
-import type { Table } from '@tanstack/react-table' import { XIcon } from 'lucide-react' import { SearchInput } from '~/app/components/search-input' import { Button } from '~/app/components/ui/button' import { useDataTableState } from '../+hooks/use-data-table-state' -interface DataTableToolbarProps<TData> { - table: Table<TData> -} - -export function DataTableToolbar<TData>(_props: DataTableToolbarProps<TData>) { +export function DataTableToolbar() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/members/+components/data-table-toolbar.tsx around lines 1 - 11, Remove the unused Table import, the DataTableToolbarProps interface, the generic parameter <TData> and the unused _props parameter from the DataTableToolbar component; update the component signature to a plain function DataTableToolbar() that uses useDataTableState() internally (referencing DataTableToolbar, DataTableToolbarProps, Table, and _props to locate the dead code) and delete the import of Table from '@tanstack/react-table'.app/routes/$orgSlug/settings/github-users._index/route.tsx-61-98 (1)
61-98:⚠️ Potential issue | 🟠 MajorUnsafe
as stringcasts on form data — missing input validation.All
formData.get(...)calls are cast withas stringwithout validation.FormData.get()returnsFormDataEntryValue | null, so if any field is absent the cast silently passesnullto mutation functions, risking DB constraint violations or corrupted data.Also, the coding guidelines specify using ts-pattern for intent-based dispatch and Conform with Zod for form validation, but this action uses raw if/else chains and unvalidated casts.
Proposed fix using Zod validation and ts-pattern
+import { z } from 'zod' +import { match } from 'ts-pattern' export const action = async ({ request, params }: Route.ActionArgs) => { const { organization } = await requireOrgAdmin(request, params.orgSlug) const formData = await request.formData() - const intent = formData.get('intent') - - if (intent === 'add') { - const login = formData.get('login') as string - const displayName = formData.get('displayName') as string - await addGithubUser({ - login, - displayName, - organizationId: organization.id, - }) - return data({ ok: true }) - } - - if (intent === 'update') { - const login = formData.get('login') as string - const displayName = formData.get('displayName') as string - const name = (formData.get('name') as string) || null - const email = (formData.get('email') as string) || null - await updateGithubUser({ - login, - organizationId: organization.id, - displayName, - name, - email, - }) - return data({ ok: true }) - } - - if (intent === 'delete') { - const login = formData.get('login') as string - await deleteGithubUser(login, organization.id) - return data({ ok: true }) - } - - return data({ error: 'Invalid intent' }, { status: 400 }) + const intent = formData.get('intent') as string + + return match(intent) + .with('add', async () => { + const { login, displayName } = z + .object({ login: z.string().min(1), displayName: z.string().min(1) }) + .parse({ login: formData.get('login'), displayName: formData.get('displayName') }) + await addGithubUser({ login, displayName, organizationId: organization.id }) + return data({ ok: true }) + }) + .with('update', async () => { + const parsed = z + .object({ + login: z.string().min(1), + displayName: z.string().min(1), + name: z.string().nullable().default(null), + email: z.string().nullable().default(null), + }) + .parse({ + login: formData.get('login'), + displayName: formData.get('displayName'), + name: formData.get('name') || null, + email: formData.get('email') || null, + }) + await updateGithubUser({ ...parsed, organizationId: organization.id }) + return data({ ok: true }) + }) + .with('delete', async () => { + const { login } = z + .object({ login: z.string().min(1) }) + .parse({ login: formData.get('login') }) + await deleteGithubUser(login, organization.id) + return data({ ok: true }) + }) + .otherwise(() => data({ error: 'Invalid intent' }, { status: 400 })) }As per coding guidelines,
app/routes/**/*.{tsx,ts}: "Use intent-based dispatch with ts-pattern for routes with multiple form actions" and "Use Conform with Zod for type-safe form validation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/github-users._index/route.tsx around lines 61 - 98, The action handler currently casts FormData values unsafely and uses raw if/else intent checks; replace the casts with proper Conform+Zod validation and switch intent handling to ts-pattern: define Zod schemas for each intent payload (e.g., AddGithubUserSchema, UpdateGithubUserSchema, DeleteGithubUserSchema) and parse formData into a typed object, handling validation errors by returning a 400 with errors; then use ts-pattern on the parsed intent value to dispatch to addGithubUser, updateGithubUser, or deleteGithubUser, passing only validated fields (e.g., login, displayName, name, email) and avoid null/undefined casts; keep requireOrgAdmin call as-is and return data({ ok: true }) on success.app/routes/$orgSlug/settings/members/route.tsx-68-79 (1)
68-79: 🛠️ Refactor suggestion | 🟠 MajorUse ts-pattern for intent dispatch and validate form inputs.
Two issues here:
Guideline violation: The coding guidelines require
ts-pattern(match/with/exhaustive) for routes with multiple form actions. The settings_index/route.tsxin this PR correctly uses this pattern — this action should follow suit.Unsafe casts:
formData.get('memberId') as stringwill benullcast tostringif the field is missing, silently passing invalid data to mutations.Proposed refactor using ts-pattern + validation
export const action = async ({ request, params }: Route.ActionArgs) => { - await requireOrgAdmin(request, params.orgSlug) + const { organization } = await requireOrgAdmin(request, params.orgSlug) const formData = await request.formData() - const intent = formData.get('intent') - - if (intent === 'changeRole') { - const memberId = formData.get('memberId') as string - const role = formData.get('role') as string - await changeMemberRole(memberId, role) - return data({ ok: true }) - } - - if (intent === 'removeMember') { - const memberId = formData.get('memberId') as string - await removeMember(memberId) - return data({ ok: true }) - } + const intent = String(formData.get('intent')) - return data({ error: 'Invalid intent' }, { status: 400 }) + return match(intent) + .with('changeRole', async () => { + const memberId = formData.get('memberId') + const role = formData.get('role') + if (typeof memberId !== 'string' || typeof role !== 'string') { + return data({ error: 'Invalid form data' }, { status: 400 }) + } + await changeMemberRole(memberId, role) + return data({ ok: true }) + }) + .with('removeMember', async () => { + const memberId = formData.get('memberId') + if (typeof memberId !== 'string') { + return data({ error: 'Invalid form data' }, { status: 400 }) + } + await removeMember(memberId) + return data({ ok: true }) + }) + .otherwise(() => data({ error: 'Invalid intent' }, { status: 400 })) }As per coding guidelines: "Use intent-based dispatch with ts-pattern for routes with multiple form actions, parsing intent from form data and using match/with/exhaustive pattern matching."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/members/route.tsx around lines 68 - 79, Replace the if-chain dispatch with ts-pattern's match/with/exhaustive on the intent value and validate form inputs before calling mutations; specifically, read intent from formData and use match(intent).with('changeRole', ...) to extract and validate memberId and role (ensure formData.get('memberId') and formData.get('role') are non-null strings) before calling changeMemberRole(memberId, role), and use .with('removeMember', ...) to validate memberId before calling removeMember(memberId); on validation failure return a proper bad-request response (or throw) instead of casting null to string, and include exhaustive() to satisfy the matcher requirement.app/routes/$orgSlug/settings/members/route.tsx-63-82 (1)
63-82:⚠️ Potential issue | 🟠 MajorAuthorization gap:
memberIdoperations are not scoped to the current organization.The action validates that the requester is an org admin via
requireOrgAdmin, but does not destructure the organization context. The mutationschangeMemberRoleandremoveMemberoperate on the members table using only thememberIdin their WHERE clause, with noorganization_idfiltering. Since the members table has anorganization_idcolumn, a malicious admin of Org A could craft a request with amemberIdfrom Org B to manipulate their roles or remove them.Additionally, the action uses an if/else chain for intent dispatch instead of the required ts-pattern match/with/exhaustive pattern (see the settings _index route for the correct approach), and extracts form values with unsafe
as stringcasts without validation.Proposed fixes
- Scope mutations to the organization by adding an
organization_idparameter:export const changeMemberRole = async ( memberId: string, + organizationId: string, role: string ) => { await db .updateTable('members') .set({ role }) .where('id', '=', memberId) + .where('organization_id', '=', organizationId) .execute() } export const removeMember = async ( memberId: string, + organizationId: string ) => { await db .deleteFrom('members') .where('id', '=', memberId) + .where('organization_id', '=', organizationId) .execute() }
- Update the action to pass the organization context:
export const action = async ({ request, params }: Route.ActionArgs) => { - await requireOrgAdmin(request, params.orgSlug) + const { organization } = await requireOrgAdmin(request, params.orgSlug) const formData = await request.formData() const intent = formData.get('intent') if (intent === 'changeRole') { const memberId = formData.get('memberId') as string const role = formData.get('role') as string - await changeMemberRole(memberId, role) + await changeMemberRole(memberId, organization.id, role) return data({ ok: true }) } if (intent === 'removeMember') { const memberId = formData.get('memberId') as string - await removeMember(memberId) + await removeMember(memberId, organization.id) return data({ ok: true }) } return data({ error: 'Invalid intent' }, { status: 400 }) }
- Refactor to use ts-pattern for intent dispatch and parse/validate form data with Conform/Zod.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/members/route.tsx around lines 63 - 82, The action currently calls changeMemberRole and removeMember with only memberId (and uses unsafe "as string" casts and if/else intent checks), allowing cross-organization changes; update the action to obtain the organization context from requireOrgAdmin (or resolve organization_id from params.orgSlug) and pass organization_id into changeMemberRole and removeMember so their WHERE clauses include organization_id, replace the if/else intent handling with ts-pattern's match(...).with(...).exhaustive pattern, and validate/parses form values (memberId, role, intent) using Conform/Zod instead of raw "as string" casts before calling the mutation functions.app/routes/$orgSlug/settings/repositories.$repository._index/route.tsx-33-46 (1)
33-46:⚠️ Potential issue | 🟠 MajorRepository lookup does not verify ownership by organization — potential cross-org data leak.
getRepository(repositoryId)fetches the repository solely by ID without verifying it belongs toorganization.id. An admin of Org A could view repositories belonging to Org B by manipulating the$repositoryURL param. The database schema includesorganizationIdon the repositories table, but all four route implementations ofgetRepository()(in._index,.settings,.delete, and.$pull) omit this filter.🔒 Proposed fix: add organization ownership check
const repository = await getRepository(repositoryId) - if (!repository) { + if (!repository || repository.organizationId !== organization.id) { throw new Response('repository not found', { status: 404 }) }Apply the same check in
repositories.$repository.settings/route.tsxloader and action, along with the other affected routes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.$repository._index/route.tsx around lines 33 - 46, The loader uses getRepository(repositoryId) without checking organization ownership, allowing cross-org access; update the loader in this route (and the other affected routes: repositories.$repository.settings/route.tsx, .delete, and .$pull) to fetch or validate the repository by organization.id — either call a new/updated getRepositoryByOrg(repositoryId, organization.id) or after getRepository(repositoryId) assert repository.organizationId === organization.id and throw a 404 (or unauthorized) if not, then proceed to call listPullRequests(repositoryId) only for the validated repository.
app/routes/$orgSlug/settings/github-users._index/+components/github-user-row-actions.tsx
Show resolved
Hide resolved
app/routes/$orgSlug/settings/repositories.$repository.settings/_layout.tsx
Show resolved
Hide resolved
Prepare for react-router-auto-routes migration by moving the flat repositories.tsx parent route into repositories/_layout.tsx and adding an Outlet component for proper child route nesting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace remix-flat-routes + @react-router/remix-routes-option-adapter with react-router-auto-routes for simpler, native React Router v7 routing - Rename route group folders: admin+/ → admin/, _auth+/ → _auth/, resources+/ → resources/ - Rename route.tsx → _layout.tsx (auto-routes convention) - Add + prefix to colocated helper directories (functions/, forms/, components/) to prevent them from being treated as routes - Rename types.ts → +schema.ts to avoid +types/ namespace conflict - Exclude opensrc/ from tsconfig to avoid unrelated type errors - Update all affected import paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move ConfirmDialog children inside Form so hidden inputs are submitted - Add organizationId validation to member mutations to prevent IDOR - Add server-side Zod validation for repository settings action Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
app/routes/$orgSlug/settings/_index/+forms/organization-settings.action.server.ts (1)
12-20:⚠️ Potential issue | 🟠 MajorAuth guard must be called before form parsing / early-return.
requireOrgAdminis invoked on line 20, but by that point:
request.formData()has already been consumed (line 12).- An unauthenticated/unauthorized user who submits an invalid payload hits the early return (lines 13–18) and receives Conform validation error messages — the auth guard is never reached.
This violates the principle of authenticate-before-process: validation feedback (field names, type constraints, enum values, etc.) is surfaced to callers who have not been authorized to use this endpoint.
The fix is to run
requireOrgAdminfirst, then parse form data:🔒 Proposed fix — authenticate before processing
export const action = async ({ request, params }: Route.ActionArgs) => { + const { organization } = await requireOrgAdmin(request, params.orgSlug) + const submission = await parseWithZod(await request.formData(), { schema }) if (submission.status !== 'success') { return { intent: INTENTS.organizationSettings, lastResult: submission.reply(), } } - const { organization } = await requireOrgAdmin(request, params.orgSlug) - const {Note:
requireOrgAdminuses cookies/session headers, so calling it beforerequest.formData()does not affect body consumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/+forms/organization-settings.action.server.ts around lines 12 - 20, Move the auth guard before form parsing: call requireOrgAdmin(request, params.orgSlug) first (use its returned { organization }) and only then call parseWithZod(await request.formData(), { schema }); ensure you do not call request.formData() prior to requireOrgAdmin so unauthenticated/unauthorized requests never reach the Conform validation early-return, and keep the existing early-return behavior that returns intent INTENTS.organizationSettings and lastResult: submission.reply() after parsing.app/routes/$orgSlug/ongoing/_layout.tsx (1)
46-50:⚠️ Potential issue | 🟡 MinorUnhandled clipboard Promise produces a false-positive success toast
navigator.clipboard.writeTextreturns a Promise. The current code firestoast.infounconditionally before the Promise settles, so the user sees "Copied N rows" even when clipboard access is denied (permission denied, non-HTTPS context, etc.), and an unhandled rejection is logged to the browser console.🐛 Proposed fix — await the write before toasting
- onClick={() => { - // markdown 表形式でコピー - navigator.clipboard.writeText(generateMarkdown(pullRequests)) - toast.info(`Copied ${pullRequests.length} rows`) - }} + onClick={() => { + // markdown 表形式でコピー + navigator.clipboard + .writeText(generateMarkdown(pullRequests)) + .then(() => toast.info(`Copied ${pullRequests.length} rows`)) + .catch(() => toast.error('Failed to copy to clipboard')) + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/ongoing/_layout.tsx around lines 46 - 50, The onClick handler currently calls navigator.clipboard.writeText(generateMarkdown(pullRequests)) and immediately calls toast.info, causing a false-positive success and an unhandled rejection; update the handler in the same onClick to await the Promise from navigator.clipboard.writeText(generateMarkdown(pullRequests)) (or chain .then/.catch) and only call toast.info(`Copied ${pullRequests.length} rows`) after the Promise resolves, and handle failures by catching the error to call toast.error with a useful message and optionally console.error the error so clipboard permission/HTTPS failures are reported instead of producing unhandled rejections.app/routes/$orgSlug/settings/_index/+forms/organization-settings.tsx (2)
48-53:⚠️ Potential issue | 🟠 Major
isActiveis missing fromdefaultValue, so the Active toggle always renders unchecked.
fields.isActiveis used in the Switch at line 116 (defaultChecked={fields.isActive.initialValue === '1'}), butisActiveis not present indefaultValue(lines 48–53). Because Conform derivesinitialValuefromdefaultValue, the property resolves toundefinedon every render, causing the switch to ignore whatever value is persisted in the database.🐛 Proposed fix — include `isActive` in `defaultValue`
The field should come from whichever model owns it (
organizationororganizationSetting):defaultValue: { name: organization.name, releaseDetectionMethod: organizationSetting?.releaseDetectionMethod, releaseDetectionKey: organizationSetting?.releaseDetectionKey, excludedUsers: organizationSetting?.excludedUsers, + isActive: organizationSetting?.isActive ? '1' : undefined, },Adjust the source (
organizationvsorganizationSetting) to match whichever model holds theisActivecolumn.Also applies to: 112-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/+forms/organization-settings.tsx around lines 48 - 53, defaultValue for the Conform form is missing the isActive property, so fields.isActive.initialValue is undefined and the Active Switch renders unchecked; update the defaultValue object (the one that currently sets name, releaseDetectionMethod, releaseDetectionKey, excludedUsers) to also include isActive sourced from the correct model (organization or organizationSetting) so fields.isActive.initialValue is derived correctly and the Switch defaultChecked={fields.isActive.initialValue === '1'} reflects the persisted value.
141-141:⚠️ Potential issue | 🟡 MinorMixed-language UI copy —
システムエラーshould be English.The error alert title is in Japanese while every other user-facing string in this file is in English. Replace it with a consistent English string.
✏️ Proposed fix
- <AlertTitle>システムエラー</AlertTitle> + <AlertTitle>System Error</AlertTitle>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/+forms/organization-settings.tsx at line 141, Replace the mixed-language AlertTitle string "システムエラー" with an English equivalent to match the rest of the UI (e.g., "System Error") in the AlertTitle component usage in this file; locate the AlertTitle element inside the organization settings form (the Alert/AlertTitle JSX block) and update its text content to the chosen English phrase so all user-facing strings remain consistent.app/routes/$orgSlug/settings/repositories.$repository.$pull/_layout.tsx (1)
21-28:⚠️ Potential issue | 🔴 CriticalAdd organizationId filter to prevent unauthorized repository access.
The
getRepositorycall on line 28 only filters byrepositoryId, but therepositoriestable has anorganizationIdcolumn. An attacker could exploit this by accessing a repository from a different organization if they know its ID, since the query bypasses the organization scope authenticated viarequireOrgAdmin.Pass
organization.idtogetRepositoryand add awhere('organizationId', '=', organizationId)filter, consistent with other repository queries in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.$repository.$pull/_layout.tsx around lines 21 - 28, The loader currently calls getRepository(repositoryId) without scoping to the organization from requireOrgAdmin; update the loader to pass organization.id (from the requireOrgAdmin result) into getRepository or call getRepository with both repositoryId and organizationId, and ensure the underlying query adds a where('organizationId', '=', organizationId) filter (consistent with other repo queries) so repository lookups in function getRepository/loader are limited to the authenticated organization.app/routes/$orgSlug/settings/_index/+forms/integration-settings.action.server.ts (1)
39-41:⚠️ Potential issue | 🟡 MinorIncorrect success message — copy-paste from export settings.
The message says
'Update export settings successfully'but this is the integration settings action.✏️ Proposed fix
- message: 'Update export settings successfully', + message: 'Update integration settings successfully',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/+forms/integration-settings.action.server.ts around lines 39 - 41, The success message in the integration settings action is wrong (copy-pasted from export settings); update the response object in integration-settings.action.server.ts where the message is set (the object containing message: 'Update export settings successfully') to use a correct string like 'Update integration settings successfully' so the integration settings action returns an accurate success message.app/routes/admin/+create/mutations.server.ts (1)
16-28:⚠️ Potential issue | 🟠 MajorCreate organizationSettings in the transaction; exportSettings can remain optional.
The transaction only creates the organization row but
organizationSettingsis required by downstream code. The$orgSlug/settings/repositories.addroute callsaddRepository(), which throws iforganizationSettingsis missing (.executeTakeFirstOrThrow()). Although the settings page createsorganizationSettingson-demand when first accessed, a user who navigates directly to/repositories/addbefore the settings page will hit a failure.
exportSettingsis safe to leave optional—the UI and batch queries already handle null values gracefully (.executeTakeFirst()+ nullable component prop).🔧 Add organizationSettings initialization
return await db.transaction().execute(async (tsx) => { const organization = await tsx .insertInto('organizations') .values({ id: nanoid(), name: organizationName, slug: organizationSlug, }) .returningAll() .executeTakeFirstOrThrow() + await tsx + .insertInto('organizationSettings') + .values({ + id: crypto.randomUUID(), + organizationId: organization.id, + // …add default values for other columns + }) + .execute() + return { organization } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/admin/`+create/mutations.server.ts around lines 16 - 28, The transaction currently only inserts into 'organizations' which leaves organizationSettings missing and causes addRepository() to throw; update the same db.transaction().execute(async (tsx) => { ... }) block to also insert a corresponding organizationSettings row (using tsx.insertInto('organization_settings') or the project's settings table name) referencing the newly created organization.id (or organization.id from the returned organization), provide sensible defaults for required fields and leave exportSettings null/optional, and return the created organization and settings together so downstream code that calls .executeTakeFirstOrThrow() will find the settings.
♻️ Duplicate comments (2)
app/routes/$orgSlug/settings/members/mutations.server.ts (1)
3-14: IDOR fix confirmed —organizationIdscoping is correctly applied.Both mutations now include
.where('organizationId', '=', organizationId), resolving the previously flagged cross-org mutation vulnerability.The
role: stringtype (line 6) is still unnarrowed. UsingDB.Members['role']would prevent passing invalid role strings at compile time:💡 Narrow the role type
+import type { DB } from '~/app/services/db.server' + export const changeMemberRole = async ( memberId: string, organizationId: string, - role: string, + role: DB.Members['role'], ) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/members/mutations.server.ts around lines 3 - 14, The changeMemberRole mutation correctly scopes by organizationId but its role parameter is too permissive; update the function signature of changeMemberRole to use the narrow DB typing (e.g., DB.Members['role']) instead of plain string so invalid role values are caught at compile time, and adjust any callers to pass the typed role; locate the changeMemberRole function and replace the role: string parameter with the appropriate DB.Members['role'] type.app/routes/$orgSlug/settings/repositories.$repository.settings/_layout.tsx (1)
70-84: Previous server-side validation issue is now addressed — looks good.The action now uses
parseWithZod(formData, { schema: githubSchema })and only passessubmission.valuetoupdateRepository, properly preventing arbitrary field injection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.$repository.settings/_layout.tsx around lines 70 - 84, The server-side validation issue was fixed by parsing formData with parseWithZod using githubSchema and only passing the validated payload to updateRepository; ensure the action function continues to: call requireOrgAdmin to authorize, extract repositoryId via zx.parseParams, parse formData with parseWithZod({ schema: githubSchema }), check submission.status === 'success' and return a 400 with submission.reply() on failure, and call updateRepository(repositoryId, submission.value) before redirecting — preserve these exact checks and use of parseWithZod, githubSchema, updateRepository, and requireOrgAdmin to prevent arbitrary field injection.
🧹 Nitpick comments (12)
app/routes/$orgSlug/ongoing/_layout.tsx (1)
32-32:fromandtoare serialised to the client but never consumedThe component only destructures
pullRequestsfromloaderData;from(alwaysnull) andtoare dead payload on every response.♻️ Proposed cleanup
- return { pullRequests, from, to } + return { pullRequests }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/ongoing/_layout.tsx at line 32, The loader is returning unused serialized values `from` and `to` (always `from` === null) which bloat responses; update the loader to only return `{ pullRequests }` (remove `from` and `to` from the returned object and any related local vars), or alternatively consume `from`/`to` in the component that reads `loaderData`; target the code around the `loader` that returns `return { pullRequests, from, to }` and the component using `loaderData` (currently only destructuring `pullRequests`) and either strip the unused `from`/`to` or use them consistently.app/routes.ts (1)
1-4: Clean migration — update project documentation to reflect the new routing library.The
autoRoutes()no-argument call is the canonical API, theimport typeimport is correct, and thesatisfies RouteConfigassertion gives compile-time type safety without type widening. This is a well-structured replacement for the previousremixRoutesOptionAdapter(flatRoutes(...))setup.The library is designed specifically for this usage pattern (
export default autoRoutes()) and supports all the file-based routing conventions already in use (flat routes,_layout.tsx,$param, etc.).The retrieved learning from
CLAUDE.mdstill references the old convention: "Use file-based routing with remix-flat-routes convention". Sinceremix-flat-routeshas been replaced,CLAUDE.mdshould be updated to referencereact-router-auto-routesto keep developer guidance accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes.ts` around lines 1 - 4, Update project docs to reflect the new routing library: replace references to the old "remix-flat-routes" convention with "react-router-auto-routes" and document the canonical usage pattern export default autoRoutes() satisfies RouteConfig (showing no-argument call and the satisfies RouteConfig assertion). In CLAUDE.md (and any other docs referencing the old adapter) update examples and guidance to mention that file-based routing conventions like flat routes, _layout.tsx, and $param are supported by react-router-auto-routes and remove or mark obsolete the remixRoutesOptionAdapter(flatRoutes(...)) example. Also add a brief note about the type-safety benefit of using satisfies RouteConfig for compile-time checks.package.json (1)
28-29:lab:servehas an implicit Python 3 system dependency.
python3 -m http.serverrequires Python 3 inPATH, which won't exist on all dev machines and isn't declared in the project manifest. Consider replacing with a Node.js-native server to stay self-contained:🔧 Node.js alternative
- "lab:serve": "python3 -m http.server 8787 -d lab/output" + "lab:serve": "npx serve -l 8787 lab/output"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 28 - 29, The "lab:serve" script currently depends on a system Python3 binary; replace it with a Node-native devDependency and update the package.json script to use that tool so the project is self-contained. Add a lightweight devDependency such as "serve" or "http-server" to devDependencies, update the "lab:serve" script to run that package (binding to port 8787 and serving lab/output), and remove reliance on "python3 -m http.server" so contributors don't need Python in PATH.biome.json (1)
2-2:2.3.13is valid but outdated — consider upgrading to the latest2.4.xrelease.Biome
2.3.13is a confirmed release, so the$schemaURL is valid. However, the latest available release is at least2.4.4, meaning this config is pinning to a version several minor/patch releases behind. Since Biome2.xhas been evolving quickly, consider upgrading to keep schema validation, lint rules, and tooling in sync with the installed package version.♻️ Suggested update
- "$schema": "https://biomejs.dev/schemas/2.3.13/schema.json", + "$schema": "https://biomejs.dev/schemas/2.4.4/schema.json",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@biome.json` at line 2, Update the Biome schema version pinned in the "$schema" field so it points to the current 2.4.x release instead of "https://biomejs.dev/schemas/2.3.13/schema.json"; locate the "$schema" entry in biome.json and change the URL to the latest 2.4 release (for example replace "2.3.13" with "2.4.4" or the specific 2.4.x version you are using) to keep schema validation and tooling in sync with the installed Biome version.app/routes/admin/_index/queries.server.ts (1)
3-8: Consider adding anorderByfor deterministic row ordering.Without it the DB is free to return rows in heap order, which can change after vacuums or writes, causing the admin list to re-sort unexpectedly between page loads.
♻️ Suggested change
export const listOrganizations = async () => { return await db .selectFrom('organizations') .select(['id', 'name', 'slug']) + .orderBy('name', 'asc') .execute() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/admin/_index/queries.server.ts` around lines 3 - 8, The listOrganizations query can return rows in non-deterministic heap order; update the listOrganizations function to add a deterministic ORDER BY (e.g., .orderBy('name', 'asc') or .orderBy('id', 'asc')) to the Kysely/DB query so results are consistently sorted across requests; locate the export const listOrganizations function and append the orderBy clause before .execute() to enforce stable ordering.app/routes/admin/_index/_layout.tsx (1)
49-51: Use thehrefhelper consistently for the Settings link.Line 62 uses
href('/admin/create')for type-safe route resolution, but line 49 falls back to a plain template literal. React Router v7'shrefhelper also accepts dynamic-segment routes with a params object, which would give the same compile-time route validation here.♻️ Suggested change
- <Link to={`/${organization.slug}/settings`}> + <Link to={href('/:orgSlug/settings', { orgSlug: organization.slug })}>Please verify that React Router v7's
hrefhelper accepts a dynamic-segment pattern and a params object with the syntax shown above:React Router v7 href function dynamic segment params syntax framework mode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/admin/_index/_layout.tsx` around lines 49 - 51, Replace the plain template literal Link to the settings page with the type-safe href helper: update the Link usage (component named Link) that currently uses `to={`/${organization.slug}/settings`}` to instead call `href('/admin/:organization/settings', { organization: organization.slug })` (using the href helper with the dynamic-segment pattern and params object) so React Router v7 validates the route at compile time and mirrors the existing usage of href('/admin/create') elsewhere.tsconfig.json (1)
3-3: Addnode_modulesto theexcludearray for best practice.When you explicitly set
exclude, TypeScript no longer applies its default exclusions (node_modules,bower_components,jspm_packages,outDir). Whilenode_modulesdoesn't currently exist in the repository, it will be created when dependencies are installed. The broad**/*.tsand**/*.tsxinclude patterns combined withallowJs: truecould then inadvertently pull in source files fromnode_modules. TypeScript documentation recommends keepingnode_modulesin theexcludearray when defining custom exclusions.🔧 Proposed fix
- "exclude": ["opensrc"], + "exclude": ["node_modules", "opensrc"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsconfig.json` at line 3, Add "node_modules" to the tsconfig.json "exclude" array to prevent TypeScript from pulling in installed packages when custom exclusions are used; update the existing exclude (currently ["opensrc"]) to include "node_modules" so the array becomes something like ["opensrc", "node_modules"] and ensure this change is applied in the tsconfig.json file.app/routes/resources/organization.tsx (1)
14-15: Relative imports for app-internal paths should use~prefix.Lines 14 and 15 use relative
./paths pointing into theapp/directory. Per coding guidelines, all imports from theapp/directory should use the~alias.♻️ Proposed refactor
-import { listUserOrganizations } from './+organization/functions.server' -import { useCurrentOrganization } from './+organization/hooks/useCurrentOrganization' +import { listUserOrganizations } from '~/app/routes/resources/+organization/functions.server' +import { useCurrentOrganization } from '~/app/routes/resources/+organization/hooks/useCurrentOrganization'As per coding guidelines: "Use
~prefix for imports fromapp/directory (e.g.,import { db } from '~/app/services/db.server')."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/resources/organization.tsx` around lines 14 - 15, Update the two imports to use the app-root alias instead of relative paths: replace the import of listUserOrganizations from './+organization/functions.server' and the import of useCurrentOrganization from './+organization/hooks/useCurrentOrganization' with their '~' aliased counterparts (e.g., import from '~/app/routes/resources/+organization/functions.server' style), ensuring the unique symbols listUserOrganizations and useCurrentOrganization remain the same; this aligns the file with the guideline to use '~' for all app/ internal imports.app/routes/resources/+organization/hooks/useCurrentOrganization.ts (1)
3-13:RESERVED_PREFIXESduplicatesisReservedSluglogic inauth.server.ts— divergence risk.Both this set and the server-side
isReservedSlugfunction must be kept in sync manually. Adding a new reserved route on the server without updating this hook (or vice versa) causes the client to return a non-null slug for reserved paths, potentially triggering spurious org-context fetches.Consider extracting the reserved slugs list into a shared constant file (e.g.,
app/libs/reserved-slugs.ts) that bothauth.server.tsand this hook import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/resources/`+organization/hooks/useCurrentOrganization.ts around lines 3 - 13, The client-side RESERVED_PREFIXES constant in useCurrentOrganization conflicts with server-side isReservedSlug and must be unified: extract the reserved slugs array into a shared module (e.g., exported constant reservedSlugs in a new libs file) and update the useCurrentOrganization hook to import that constant instead of defining RESERVED_PREFIXES, and update auth.server.ts to import and use the same exported reservedSlugs (or derive isReservedSlug from it); ensure names match (reservedSlugs) and replace usages of RESERVED_PREFIXES and any hardcoded lists so both the client hook and isReservedSlug use the single shared source.app/routes/$orgSlug/settings/repositories._index/_layout.tsx (1)
13-17: Consider co-locating shared schemas outside the+hooksdirectory.
PaginationSchema,QuerySchema, andSortSchemaare imported from./+hooks/use-data-table-stateinto the server-side loader. While this works (the schemas are plain Zod objects), importing from a hooks module in a loader blurs the client/server boundary. Consider extracting the schemas into a shared+schemaor+constantsmodule, keeping the hook file focused on client-side state management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories._index/_layout.tsx around lines 13 - 17, The loader currently imports PaginationSchema, QuerySchema, and SortSchema from the client hooks module use-data-table-state which blurs client/server boundaries; extract these Zod schemas into a new shared module (e.g., +schema or +constants) and update both use-data-table-state and this layout to import the schemas from that shared module instead of from ./+hooks/use-data-table-state so the hook stays client-focused and the loader imports only server-safe shared code.app/routes/$orgSlug/settings/_index/+forms/export-settings.action.server.ts (1)
8-17: Auth check after body consumption is safe but consider ordering.
requireOrgAdminis called afterrequest.formData()(line 9). This works becauserequireOrgAdminreads cookies/headers, not the body. However, placing the auth guard before form parsing is the more conventional pattern — it short-circuits earlier for unauthenticated/unauthorized users and avoids processing untrusted input unnecessarily.This is a minor observation and consistent across the other action files in this PR, so not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/+forms/export-settings.action.server.ts around lines 8 - 17, Move the authorization guard before consuming the request body: in the action function, call requireOrgAdmin(request, params.orgSlug) first (using the existing requireOrgAdmin symbol) and only after it returns successfully call parseWithZod(await request.formData(), { schema }) and proceed with the existing INTENTS.exportSettings handling; this short-circuits unauthorized requests and avoids parsing untrusted input unnecessarily while keeping the rest of the flow unchanged.app/routes/admin/create.tsx (1)
84-91:JSON.stringify(form.errors)renders raw array notation in the UI.Users will see
["Failed to create organization: …"]instead of the plain message.♻️ Proposed fix
- {JSON.stringify(form.errors)} + {form.errors?.join(', ')}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/admin/create.tsx` around lines 84 - 91, The UI currently displays raw JSON from form.errors (JSON.stringify(form.errors)) inside AlertDescription, producing array notation; update the rendering to show a human-readable message by extracting/formatting the error(s) instead of stringifying: inside the AlertDescription replace JSON.stringify(form.errors) with logic that checks Array.isArray(form.errors) and either joins the messages (e.g., form.errors.join(', ') or '\n') or displays the first message, otherwise fall back to String(form.errors); keep this change localized to the Alert/AlertDescription rendering in create.tsx so AlertTitle and surrounding logic remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/AppHeader.tsx`:
- Line 20: Replace the relative import for DropdownMenuLogout with the project
alias: change the import of DropdownMenuLogout (currently from
'../routes/_auth/logout') to use the '~' prefix pointing into the app root
(e.g., import from '~/routes/_auth/logout') so it matches the existing
'~/app/routes/resources/organization' style; update the import statement that
references DropdownMenuLogout in AppHeader.tsx accordingly.
In `@app/components/layout/main.tsx`:
- Around line 8-15: The Main component's consumer className is being overwritten
because {...props} is spread after className={cn(...)}; update the component
signature to extract className from props (e.g., const { className, fixed,
...props } = props) and pass that consumer className into the cn call
(className={cn('px-4 py-6', fixed && 'flex grow flex-col overflow-hidden',
className)}), then spread the remaining props after—this ensures the cn utility
merges base/fixed classes with any consumer-provided classes.
In
`@app/routes/`$orgSlug/settings/_index/+forms/delete-organization.action.server.ts:
- Around line 17-18: The current flow uses requireOrgAdmin to authorize
deletion, allowing both admin and owner roles; change it to require an
owner-only guard by replacing or supplementing requireOrgAdmin with a call to
requireOrgOwner (or add an explicit role check on the returned organization
role) before calling deleteOrganization(organization.id). Locate the
authorization call around where requireOrgAdmin(request, params.orgSlug) is used
and either call requireOrgOwner(request, params.orgSlug) or inspect the returned
user role and throw/redirect unless role === 'owner', then proceed to await
deleteOrganization(organization.id).
In `@app/routes/`$orgSlug/settings/_index/+functions/queries.server.ts:
- Around line 23-38: createDefaultOrganizationSetting currently inserts blindly
and will throw a raw DB error on unique constraint violations; update it to be
idempotent by either (A) querying getOrganizationSetting(organizationId) first
and returning early if a row exists, or (B) using Kysely's upsert/onConflict on
db.insertInto('organizationSettings') to do nothing on conflict and then call
getOrganizationSetting(organizationId) to return the existing or newly created
row; ensure errors are handled and the function always returns a valid setting
or throws a clear application error.
In `@app/routes/`$orgSlug/settings/github-users._index/_layout.tsx:
- Around line 61-99: The action handler should validate form input with Zod and
dispatch intents using ts-pattern instead of raw if/else and unsafe casts;
create Zod schemas (e.g., AddGithubUserSchema, UpdateGithubUserSchema,
DeleteGithubUserSchema) that parse the required fields from the incoming
FormData (use Conform or call schema.parse on an object built from
formData.get(...)) and replace all "formData.get(...) as string" casts with
validated values, then refactor the intent branch inside export const action to
use match(intent).with(...) / .exhaustive() from ts-pattern to call
addGithubUser, updateGithubUser, or deleteGithubUser with the validated payload
and return appropriate data() or error responses.
In `@app/routes/`$orgSlug/settings/members/_layout.tsx:
- Around line 63-82: The action handler currently reads formData and casts
intent/role without validation; update the action to use ts-pattern intent
dispatch (match/when) instead of ifs and validate inputs with Zod/Conform:
create a Zod schema for the form that requires memberId (string) and validates
role using z.enum(['admin','member']), parse formData with that schema, and then
dispatch on intent to call changeMemberRole( memberId, organization.id, role )
or removeMember( memberId, organization.id ); on validation or unknown intent
return data errors with appropriate status codes; keep requireOrgAdmin usage
as-is.
In `@app/routes/`$orgSlug/settings/repositories.$repository._index/_layout.tsx:
- Around line 33-45: The loader exposes an IDOR because
getRepository(repositoryId) and listPullRequests(repositoryId) do not verify the
repository's organization; update the loader to enforce ownership by passing
organization.id into the data calls (or performing a post-fetch check) — call
getRepository with both repositoryId and organization.id (or check returned
repository.owner/organization_id against organization.id) and call
listPullRequests scoped to repositoryId and organization.id (or reject if
ownership mismatch) so only repositories belonging to the current organization
(from requireOrgAdmin) are returned.
In `@app/routes/`$orgSlug/settings/repositories.$repository.delete/_layout.tsx:
- Around line 22-42: The loader and action use repositoryId without verifying it
belongs to the current organization; after calling getRepository(repositoryId)
in loader and before deleteRepository(repositoryId) in action, validate
repository.organizationId (or repository.orgId) matches organization.id and if
not throw a 404/unauthorized (in loader) or return a proper redirect/error (in
action); update the loader (function loader) and action (function action) to
perform this ownership check using the fetched repository object and fail early
if the IDs don't match to prevent cross-org access.
In `@app/routes/`$orgSlug/settings/repositories.$repository.settings/_layout.tsx:
- Around line 48-68: The loader uses repositoryId from params without verifying
ownership: after fetching repository with getRepository(repositoryId) in loader
(and likewise before calling updateRepository in the action), confirm
repository.organizationId (or repository.ownerId) === organization.id
(organization came from requireOrgAdmin) and if not, throw a 404 or 403
Response; add the same ownership check in the action before invoking
updateRepository to prevent IDORs (refer to loader, requireOrgAdmin,
getRepository, organization.id, action, updateRepository).
- Around line 58-60: The thrown Error message is misleading when the
`integration` check fails; update the throw in the block that checks `if
(!integration)` (the Error created there) to use a correct, clear message such
as "organization integration not set up" or "organization integration not found"
so it accurately reflects the missing integration when `integration` is falsy.
In `@app/routes/`$orgSlug/settings/repositories.add/_layout.tsx:
- Line 99: The exported action function is incorrectly typed with
Route.LoaderArgs; update its signature to use Route.ActionArgs instead: locate
the export named action and replace the parameter type annotation from
Route.LoaderArgs to Route.ActionArgs so the action handler uses the correct
Remix semantics (keep the same destructured { request, params } parameters and
return type).
In `@app/routes/admin/create.tsx`:
- Around line 26-29: The organizationSlug schema currently uses a permissive
regex that allows leading/trailing and consecutive hyphens; update the
z.string().regex validator for organizationSlug to enforce that the slug starts
and ends with a lowercase alphanumeric character and only allows single internal
hyphens (for example: use the pattern ^[a-z0-9]+(?:-[a-z0-9]+)*$), so change the
regex on the organizationSlug declaration in create.tsx accordingly and keep the
same error message text or adjust it to reflect the stricter rule.
In `@package.json`:
- Line 15: The package.json script "setup" was renamed to "db:setup", which will
break any callers still invoking "pnpm setup" or "pnpm run setup"; search for
and update all references to the old "setup" invocation in CI/CD configs, Fly.io
release commands, README.md quickstart, onboarding docs, and any pipeline
scripts to use "pnpm run db:setup" (or add a compatibility alias script "setup":
"pnpm run db:setup" in package.json if you want to preserve backward
compatibility). Ensure the unique symbol "db:setup" is used consistently or add
the "setup" script entry to package.json to avoid silent failures.
---
Outside diff comments:
In `@app/routes/`$orgSlug/ongoing/_layout.tsx:
- Around line 46-50: The onClick handler currently calls
navigator.clipboard.writeText(generateMarkdown(pullRequests)) and immediately
calls toast.info, causing a false-positive success and an unhandled rejection;
update the handler in the same onClick to await the Promise from
navigator.clipboard.writeText(generateMarkdown(pullRequests)) (or chain
.then/.catch) and only call toast.info(`Copied ${pullRequests.length} rows`)
after the Promise resolves, and handle failures by catching the error to call
toast.error with a useful message and optionally console.error the error so
clipboard permission/HTTPS failures are reported instead of producing unhandled
rejections.
In
`@app/routes/`$orgSlug/settings/_index/+forms/integration-settings.action.server.ts:
- Around line 39-41: The success message in the integration settings action is
wrong (copy-pasted from export settings); update the response object in
integration-settings.action.server.ts where the message is set (the object
containing message: 'Update export settings successfully') to use a correct
string like 'Update integration settings successfully' so the integration
settings action returns an accurate success message.
In
`@app/routes/`$orgSlug/settings/_index/+forms/organization-settings.action.server.ts:
- Around line 12-20: Move the auth guard before form parsing: call
requireOrgAdmin(request, params.orgSlug) first (use its returned { organization
}) and only then call parseWithZod(await request.formData(), { schema }); ensure
you do not call request.formData() prior to requireOrgAdmin so
unauthenticated/unauthorized requests never reach the Conform validation
early-return, and keep the existing early-return behavior that returns intent
INTENTS.organizationSettings and lastResult: submission.reply() after parsing.
In `@app/routes/`$orgSlug/settings/_index/+forms/organization-settings.tsx:
- Around line 48-53: defaultValue for the Conform form is missing the isActive
property, so fields.isActive.initialValue is undefined and the Active Switch
renders unchecked; update the defaultValue object (the one that currently sets
name, releaseDetectionMethod, releaseDetectionKey, excludedUsers) to also
include isActive sourced from the correct model (organization or
organizationSetting) so fields.isActive.initialValue is derived correctly and
the Switch defaultChecked={fields.isActive.initialValue === '1'} reflects the
persisted value.
- Line 141: Replace the mixed-language AlertTitle string "システムエラー" with an
English equivalent to match the rest of the UI (e.g., "System Error") in the
AlertTitle component usage in this file; locate the AlertTitle element inside
the organization settings form (the Alert/AlertTitle JSX block) and update its
text content to the chosen English phrase so all user-facing strings remain
consistent.
In `@app/routes/`$orgSlug/settings/repositories.$repository.$pull/_layout.tsx:
- Around line 21-28: The loader currently calls getRepository(repositoryId)
without scoping to the organization from requireOrgAdmin; update the loader to
pass organization.id (from the requireOrgAdmin result) into getRepository or
call getRepository with both repositoryId and organizationId, and ensure the
underlying query adds a where('organizationId', '=', organizationId) filter
(consistent with other repo queries) so repository lookups in function
getRepository/loader are limited to the authenticated organization.
In `@app/routes/admin/`+create/mutations.server.ts:
- Around line 16-28: The transaction currently only inserts into 'organizations'
which leaves organizationSettings missing and causes addRepository() to throw;
update the same db.transaction().execute(async (tsx) => { ... }) block to also
insert a corresponding organizationSettings row (using
tsx.insertInto('organization_settings') or the project's settings table name)
referencing the newly created organization.id (or organization.id from the
returned organization), provide sensible defaults for required fields and leave
exportSettings null/optional, and return the created organization and settings
together so downstream code that calls .executeTakeFirstOrThrow() will find the
settings.
---
Duplicate comments:
In `@app/routes/`$orgSlug/settings/members/mutations.server.ts:
- Around line 3-14: The changeMemberRole mutation correctly scopes by
organizationId but its role parameter is too permissive; update the function
signature of changeMemberRole to use the narrow DB typing (e.g.,
DB.Members['role']) instead of plain string so invalid role values are caught at
compile time, and adjust any callers to pass the typed role; locate the
changeMemberRole function and replace the role: string parameter with the
appropriate DB.Members['role'] type.
In `@app/routes/`$orgSlug/settings/repositories.$repository.settings/_layout.tsx:
- Around line 70-84: The server-side validation issue was fixed by parsing
formData with parseWithZod using githubSchema and only passing the validated
payload to updateRepository; ensure the action function continues to: call
requireOrgAdmin to authorize, extract repositoryId via zx.parseParams, parse
formData with parseWithZod({ schema: githubSchema }), check submission.status
=== 'success' and return a 400 with submission.reply() on failure, and call
updateRepository(repositoryId, submission.value) before redirecting — preserve
these exact checks and use of parseWithZod, githubSchema, updateRepository, and
requireOrgAdmin to prevent arbitrary field injection.
---
Nitpick comments:
In `@app/routes.ts`:
- Around line 1-4: Update project docs to reflect the new routing library:
replace references to the old "remix-flat-routes" convention with
"react-router-auto-routes" and document the canonical usage pattern export
default autoRoutes() satisfies RouteConfig (showing no-argument call and the
satisfies RouteConfig assertion). In CLAUDE.md (and any other docs referencing
the old adapter) update examples and guidance to mention that file-based routing
conventions like flat routes, _layout.tsx, and $param are supported by
react-router-auto-routes and remove or mark obsolete the
remixRoutesOptionAdapter(flatRoutes(...)) example. Also add a brief note about
the type-safety benefit of using satisfies RouteConfig for compile-time checks.
In `@app/routes/`$orgSlug/ongoing/_layout.tsx:
- Line 32: The loader is returning unused serialized values `from` and `to`
(always `from` === null) which bloat responses; update the loader to only return
`{ pullRequests }` (remove `from` and `to` from the returned object and any
related local vars), or alternatively consume `from`/`to` in the component that
reads `loaderData`; target the code around the `loader` that returns `return {
pullRequests, from, to }` and the component using `loaderData` (currently only
destructuring `pullRequests`) and either strip the unused `from`/`to` or use
them consistently.
In `@app/routes/`$orgSlug/settings/_index/+forms/export-settings.action.server.ts:
- Around line 8-17: Move the authorization guard before consuming the request
body: in the action function, call requireOrgAdmin(request, params.orgSlug)
first (using the existing requireOrgAdmin symbol) and only after it returns
successfully call parseWithZod(await request.formData(), { schema }) and proceed
with the existing INTENTS.exportSettings handling; this short-circuits
unauthorized requests and avoids parsing untrusted input unnecessarily while
keeping the rest of the flow unchanged.
In `@app/routes/`$orgSlug/settings/repositories._index/_layout.tsx:
- Around line 13-17: The loader currently imports PaginationSchema, QuerySchema,
and SortSchema from the client hooks module use-data-table-state which blurs
client/server boundaries; extract these Zod schemas into a new shared module
(e.g., +schema or +constants) and update both use-data-table-state and this
layout to import the schemas from that shared module instead of from
./+hooks/use-data-table-state so the hook stays client-focused and the loader
imports only server-safe shared code.
In `@app/routes/admin/_index/_layout.tsx`:
- Around line 49-51: Replace the plain template literal Link to the settings
page with the type-safe href helper: update the Link usage (component named
Link) that currently uses `to={`/${organization.slug}/settings`}` to instead
call `href('/admin/:organization/settings', { organization: organization.slug
})` (using the href helper with the dynamic-segment pattern and params object)
so React Router v7 validates the route at compile time and mirrors the existing
usage of href('/admin/create') elsewhere.
In `@app/routes/admin/_index/queries.server.ts`:
- Around line 3-8: The listOrganizations query can return rows in
non-deterministic heap order; update the listOrganizations function to add a
deterministic ORDER BY (e.g., .orderBy('name', 'asc') or .orderBy('id', 'asc'))
to the Kysely/DB query so results are consistently sorted across requests;
locate the export const listOrganizations function and append the orderBy clause
before .execute() to enforce stable ordering.
In `@app/routes/admin/create.tsx`:
- Around line 84-91: The UI currently displays raw JSON from form.errors
(JSON.stringify(form.errors)) inside AlertDescription, producing array notation;
update the rendering to show a human-readable message by extracting/formatting
the error(s) instead of stringifying: inside the AlertDescription replace
JSON.stringify(form.errors) with logic that checks Array.isArray(form.errors)
and either joins the messages (e.g., form.errors.join(', ') or '\n') or displays
the first message, otherwise fall back to String(form.errors); keep this change
localized to the Alert/AlertDescription rendering in create.tsx so AlertTitle
and surrounding logic remain unchanged.
In `@app/routes/resources/`+organization/hooks/useCurrentOrganization.ts:
- Around line 3-13: The client-side RESERVED_PREFIXES constant in
useCurrentOrganization conflicts with server-side isReservedSlug and must be
unified: extract the reserved slugs array into a shared module (e.g., exported
constant reservedSlugs in a new libs file) and update the useCurrentOrganization
hook to import that constant instead of defining RESERVED_PREFIXES, and update
auth.server.ts to import and use the same exported reservedSlugs (or derive
isReservedSlug from it); ensure names match (reservedSlugs) and replace usages
of RESERVED_PREFIXES and any hardcoded lists so both the client hook and
isReservedSlug use the single shared source.
In `@app/routes/resources/organization.tsx`:
- Around line 14-15: Update the two imports to use the app-root alias instead of
relative paths: replace the import of listUserOrganizations from
'./+organization/functions.server' and the import of useCurrentOrganization from
'./+organization/hooks/useCurrentOrganization' with their '~' aliased
counterparts (e.g., import from
'~/app/routes/resources/+organization/functions.server' style), ensuring the
unique symbols listUserOrganizations and useCurrentOrganization remain the same;
this aligns the file with the guideline to use '~' for all app/ internal
imports.
In `@biome.json`:
- Line 2: Update the Biome schema version pinned in the "$schema" field so it
points to the current 2.4.x release instead of
"https://biomejs.dev/schemas/2.3.13/schema.json"; locate the "$schema" entry in
biome.json and change the URL to the latest 2.4 release (for example replace
"2.3.13" with "2.4.4" or the specific 2.4.x version you are using) to keep
schema validation and tooling in sync with the installed Biome version.
In `@package.json`:
- Around line 28-29: The "lab:serve" script currently depends on a system
Python3 binary; replace it with a Node-native devDependency and update the
package.json script to use that tool so the project is self-contained. Add a
lightweight devDependency such as "serve" or "http-server" to devDependencies,
update the "lab:serve" script to run that package (binding to port 8787 and
serving lab/output), and remove reliance on "python3 -m http.server" so
contributors don't need Python in PATH.
In `@tsconfig.json`:
- Line 3: Add "node_modules" to the tsconfig.json "exclude" array to prevent
TypeScript from pulling in installed packages when custom exclusions are used;
update the existing exclude (currently ["opensrc"]) to include "node_modules" so
the array becomes something like ["opensrc", "node_modules"] and ensure this
change is applied in the tsconfig.json file.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (71)
app/components/AppHeader.tsxapp/components/confirm-dialog.tsxapp/components/layout/main.tsxapp/routes.tsapp/routes/$orgSlug/_index/+columns.tsxapp/routes/$orgSlug/_index/+functions/generate-markdown.tsapp/routes/$orgSlug/_index/+functions/queries.tsapp/routes/$orgSlug/_index/+functions/utils.tsapp/routes/$orgSlug/_index/_layout.tsxapp/routes/$orgSlug/_index/functions.server.tsapp/routes/$orgSlug/ongoing/+columns.tsxapp/routes/$orgSlug/ongoing/+functions/generate-markdown.tsapp/routes/$orgSlug/ongoing/+functions/queries.tsapp/routes/$orgSlug/ongoing/+functions/utils.tsapp/routes/$orgSlug/ongoing/_layout.tsxapp/routes/$orgSlug/ongoing/functions.server.tsapp/routes/$orgSlug/settings/_index/+forms/actions.server.tsapp/routes/$orgSlug/settings/_index/+forms/delete-organization.action.server.tsapp/routes/$orgSlug/settings/_index/+forms/delete-organization.tsxapp/routes/$orgSlug/settings/_index/+forms/export-settings.action.server.tsapp/routes/$orgSlug/settings/_index/+forms/export-settings.tsxapp/routes/$orgSlug/settings/_index/+forms/index.tsapp/routes/$orgSlug/settings/_index/+forms/integration-settings.action.server.tsapp/routes/$orgSlug/settings/_index/+forms/integration-settings.tsxapp/routes/$orgSlug/settings/_index/+forms/organization-settings.action.server.tsapp/routes/$orgSlug/settings/_index/+forms/organization-settings.tsxapp/routes/$orgSlug/settings/_index/+forms/recalculate.tsxapp/routes/$orgSlug/settings/_index/+functions/mutations.server.tsapp/routes/$orgSlug/settings/_index/+functions/queries.server.tsapp/routes/$orgSlug/settings/_index/+schema.tsapp/routes/$orgSlug/settings/_index/_layout.tsxapp/routes/$orgSlug/settings/_index/functions.server.tsapp/routes/$orgSlug/settings/github-users._index/_layout.tsxapp/routes/$orgSlug/settings/members/_layout.tsxapp/routes/$orgSlug/settings/members/mutations.server.tsapp/routes/$orgSlug/settings/repositories.$repository.$pull/_layout.tsxapp/routes/$orgSlug/settings/repositories.$repository._index/_layout.tsxapp/routes/$orgSlug/settings/repositories.$repository.delete/+functions/mutations.tsapp/routes/$orgSlug/settings/repositories.$repository.delete/+functions/queries.tsapp/routes/$orgSlug/settings/repositories.$repository.delete/_layout.tsxapp/routes/$orgSlug/settings/repositories.$repository.delete/functions.server.tsapp/routes/$orgSlug/settings/repositories.$repository.settings/+functions/mutations.tsapp/routes/$orgSlug/settings/repositories.$repository.settings/+functions/queries.tsapp/routes/$orgSlug/settings/repositories.$repository.settings/_layout.tsxapp/routes/$orgSlug/settings/repositories.$repository.settings/functions.server.tsapp/routes/$orgSlug/settings/repositories._index/_layout.tsxapp/routes/$orgSlug/settings/repositories.add/+components/index.tsapp/routes/$orgSlug/settings/repositories.add/+components/repository-item.tsxapp/routes/$orgSlug/settings/repositories.add/+components/repository-list.tsxapp/routes/$orgSlug/settings/repositories.add/+functions/get-repositories-by-owner-and-keyword.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-unique-owners.tsapp/routes/$orgSlug/settings/repositories.add/+functions/mutations.tsapp/routes/$orgSlug/settings/repositories.add/+functions/queries.tsapp/routes/$orgSlug/settings/repositories.add/_layout.tsxapp/routes/$orgSlug/settings/repositories.add/functions.server.tsapp/routes/$orgSlug/settings/repositories/_layout.tsxapp/routes/_auth/login.tsxapp/routes/_auth/logout.tsxapp/routes/admin/+create/mutations.server.tsapp/routes/admin/_index/_layout.tsxapp/routes/admin/_index/queries.server.tsapp/routes/admin/_layout.tsxapp/routes/admin/create.tsxapp/routes/api.auth.$.tsapp/routes/resources/+organization/functions.server.tsapp/routes/resources/+organization/functions/queries.tsapp/routes/resources/+organization/hooks/useCurrentOrganization.tsapp/routes/resources/organization.tsxbiome.jsonpackage.jsontsconfig.json
✅ Files skipped from review due to trivial changes (1)
- app/routes/$orgSlug/settings/repositories.add/functions.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/confirm-dialog.tsx
app/routes/$orgSlug/settings/_index/+forms/delete-organization.action.server.ts
Outdated
Show resolved
Hide resolved
app/routes/$orgSlug/settings/repositories.$repository.settings/_layout.tsx
Show resolved
Hide resolved
app/routes/$orgSlug/settings/repositories.$repository.settings/_layout.tsx
Show resolved
Hide resolved
Security (IDOR): - Add organizationId ownership check to repository delete, index, settings, pull routes - Add org ownership check in repository settings action Bugs: - Add useEffect cleanup in useDebounce to clear timer on unmount - Add onConflict(doNothing) to createDefaultOrganizationSetting for idempotency - Auto-create owner membership when creating organization - Guard against removing sole owner of organization - Add await to parseWithZod in export-settings action - Fix integration-settings success toast message (was copy-pasted from export) Code quality: - Rename resources/organization/queries.ts to .server.ts - Replace unsafe `as string` casts with Zod validation in github-users action - Replace if/else intent dispatch with ts-pattern in members and github-users actions - Remove unused table prop from members DataTableToolbar - Fix nav-group checkIsActive to use startsWith instead of slug-only comparison - Reorder XL classification rules before L in lab/classify.ts to fix dead code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Auth guard before form parsing in all settings action files - Org deletion requires owner role, not just admin - Create organizationSettings in create-org transaction - Fix isActive missing from org-settings defaultValue - Fix Route.LoaderArgs -> ActionArgs in repositories.add action - Fix error message typo in repo settings loader - Stricter slug regex (no leading/trailing/consecutive hyphens) - Handle clipboard promise rejection in ongoing page - Fix mixed-language string and JSON.stringify for form errors - Remove unused from/to from ongoing loader - Main component merges className instead of overriding - Extract RESERVED_SLUGS to shared module - Use ~ alias for imports, add node_modules to tsconfig exclude - Add orderBy to listOrganizations, update CLAUDE.md routing docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pnpm setup was silently running pnpm's built-in setup command (PATH configuration) instead of the project's DB setup script. Now that it's correctly named db:setup, it actually runs but fails because the data/ directory doesn't exist in CI (it's gitignored). Adding mkdir -p data ensures the directory is created before atlas tries to open the DB. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
opensrc/ contains third-party source code with their own tests that depend on packages not installed in this project (e.g. picomatch). Separating vitest config from vite.config.ts keeps concerns clean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLAUDE.md (1)
62-68:⚠️ Potential issue | 🟡 MinorStale "Project Structure" after the routing convention change.
Line 64 still says
File-based routing (remix-flat-routes convention)and the listed paths (_dashboard+/,admin+/,_auth+/,api.auth.$/) reflect the old flat-routes structure, not the new$orgSlug/-based layout introduced in this PR. The Routing Convention section (line 92) was updated but this block was not.📝 Suggested update
-├── routes/ # File-based routing (remix-flat-routes convention) -│ ├── _dashboard+/ # Dashboard views (authenticated) -│ ├── admin+/ # Admin/settings views -│ ├── _auth+/ # Authentication routes -│ └── api.auth.$/ # Auth API endpoints +├── routes/ # File-based routing (react-router-auto-routes convention) +│ ├── $orgSlug/ # Org-scoped views (authenticated members) +│ ├── _auth+/ # Authentication routes +│ └── api.auth.$/ # Auth API endpoints🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 62 - 68, Update the stale "Project Structure" block under the app/ routes/ listing to reflect the new $orgSlug-based routing convention: replace the phrase "File-based routing (remix-flat-routes convention)" and the old path examples `_dashboard+/`, `admin+/`, `_auth+/`, `api.auth.$/` with the new routing description (mentioning $orgSlug/) and example directories/files that match the PR's routing layout (e.g. `$orgSlug/` and any org-scoped subroutes); ensure the Routing Convention wording now matches the updated section further down the file.app/routes/$orgSlug/settings/repositories.add/_layout.tsx (1)
148-157:⚠️ Potential issue | 🟡 MinorMinor: hidden inputs with duplicate search param keys will produce React key warnings.
searchParams.entries()can contain duplicate keys (e.g., multiplerolevalues). Usingkey={key}will cause React to render only the last entry for each duplicate key, silently dropping values.Proposed fix: use index-based keys
- {[...searchParams.entries()].map(([key, value]) => ( - <input key={key} type="hidden" name={key} value={value} /> + {[...searchParams.entries()].map(([key, value], index) => ( + <input key={`${key}-${index}`} type="hidden" name={key} value={value} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.add/_layout.tsx around lines 148 - 157, The hidden inputs mapping uses [...searchParams.entries()].map(([key, value]) => ...) with key={key}, which collides for duplicate query parameter names; update the mapping in the Form that renders searchParams so each element gets a unique React key (e.g., include the index or the value in the key like `${key}-${i}` or `${key}-${value}-${i}`) while keeping name={key} and value={value} intact so duplicate keys are preserved in the submitted form; change the map callback signature to receive the index (map(([key, value], i) => ...) and use that index in the key.
♻️ Duplicate comments (2)
app/routes/$orgSlug/settings/_index/+functions/queries.server.ts (1)
23-39: LGTM — idempotency concern is properly addressed.The
.onConflict((oc) => oc.doNothing())guard correctly handles concurrent or repeated calls without risking a raw constraint error. The subsequent read-back of the row viagetOrganizationSettingensures the function always returns a valid setting whether the insert was new or skipped, and the guard on line 37 provides a clean failure path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/+functions/queries.server.ts around lines 23 - 39, The createDefaultOrganizationSetting function is already idempotent and safe for concurrent calls: keep the .onConflict((oc) => oc.doNothing()) on the insert and the subsequent getOrganizationSetting(organizationId) read-back and the throw-on-missing row behavior as implemented; no code changes required to address the concurrency/idempotency concern in createDefaultOrganizationSetting.app/routes/$orgSlug/settings/members/mutations.server.ts (1)
3-14:role: stringcould be narrowed for type safety.As noted in a previous review, the
roleparameter accepts arbitrary strings. Narrowing toDB.Members['role']would catch invalid values at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/members/mutations.server.ts around lines 3 - 14, The changeMemberRole function currently accepts a plain string for the role parameter which allows invalid values; update the function signature to type the role as DB.Members['role'] (or the equivalent union type used by your DB types) so TypeScript enforces valid member roles, then adjust any call-sites if necessary to satisfy the narrower type; locate the changeMemberRole declaration and the .set({ role }) usage to apply the type change.
🧹 Nitpick comments (11)
app/components/layout/main.tsx (1)
4-6:MainPropsis not exportedThe AI summary lists
MainPropsas part of the public API, but the interface is not exported. Consumers who want to type-annotate a variable holding component props (e.g., when building wrappers or stories) cannot reference it.♻️ Proposed change
-interface MainProps extends React.ComponentPropsWithRef<'main'> { +export interface MainProps extends React.ComponentPropsWithRef<'main'> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/layout/main.tsx` around lines 4 - 6, The interface MainProps is currently declared but not exported, preventing consumers from importing it for typing; update the declaration of MainProps so it is exported (e.g., export interface MainProps extends React.ComponentPropsWithRef<'main'> { fixed?: boolean }) and ensure any related named export for the Main component remains consistent so external modules can import both Main and MainProps; locate the interface named MainProps in main.tsx and change its declaration to an exported interface.app/libs/auth.server.ts (3)
177-177: Mid-file import.The
RESERVED_SLUGSimport is placed in the middle of the file rather than at the top with other imports. While this works, it diverges from the standard convention and may be overlooked by linters or developers scanning the import block.Move import to the top of the file
import { betterAuth } from 'better-auth' import { admin } from 'better-auth/plugins/admin' import { organization } from 'better-auth/plugins/organization' import { nanoid } from 'nanoid' import { href, redirect } from 'react-router' import { db, dialect } from '~/app/services/db.server' +import { RESERVED_SLUGS } from './reserved-slugs'Then remove Line 177.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/libs/auth.server.ts` at line 177, Move the mid-file import of RESERVED_SLUGS into the main import block at the top of app/libs/auth.server.ts alongside the other imports and remove the existing import statement at line 177; ensure any references to RESERVED_SLUGS remain unchanged and that there are no duplicate imports after relocating it.
189-228: Inconsistent redirect style:href()vs raw string.
requireOrgMemberuseshref('/login')on Line 195 but a raw string'/no-org'on Line 213. Similarly,requireOrgAdminuses a template literal on Line 239. The rest of the file (Lines 159, 167, 170) consistently useshref(). Preferhref()throughout for type-safe route references.Proposed fix
if (!result) { - throw redirect('/no-org') + throw redirect(href('/no-org')) }And in
requireOrgAdmin:- throw redirect(`/${orgSlug}`) + throw redirect(href('/:orgSlug', { orgSlug }))(Adjust the
hrefcall to match the actual route pattern if different.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/libs/auth.server.ts` around lines 189 - 228, Replace raw-string and template-literal redirects with the type-safe href() helper so redirect usages are consistent: update the '/no-org' redirect in requireOrgMember to redirect(href('/no-org')) and update the template-literal redirect in requireOrgAdmin to redirect(href('/your-org-admin-route')) (use the actual route string used elsewhere). Ensure all redirect(...) calls in requireOrgMember and requireOrgAdmin use href(...) for consistency with other functions like getSession and the earlier redirects.
183-187:membership.roleis typed asstring.The
OrgContextinterface typesroleasstring, which is consistent with how it's used across mutations and route handlers. For stronger type safety across the codebase, consider narrowing this to a union type (e.g.,'owner' | 'admin' | 'member') or referencing the DB type. This would benefit all consumers ofOrgContext, including the role checks inrequireOrgAdminand the delete-organization action.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/libs/auth.server.ts` around lines 183 - 187, The OrgContext interface currently types membership.role as a plain string; narrow it to a stricter union or DB-derived type to improve type safety. Update OrgContext (membership.role) to use a union like 'owner'|'admin'|'member' or import the role enum/type from your DB schema, then update any callers (e.g., requireOrgAdmin and the delete-organization action) to rely on the narrowed type so role checks are type-safe across the codebase.app/libs/reserved-slugs.ts (1)
1-12: Looks good — consider adding a maintenance note.The reserved slugs set correctly covers the current top-level routes. One thing to keep in mind: this list must be kept in sync with any new top-level route segments added in the future. A brief inline comment reminding future contributors to update this set when adding top-level routes would help prevent accidental collisions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/libs/reserved-slugs.ts` around lines 1 - 12, Add a brief inline comment immediately above the RESERVED_SLUGS declaration reminding contributors to keep this Set in sync with any new top-level route segments (e.g., when adding new pages or API roots), and optionally point to where top-level routes are defined so they remember to update RESERVED_SLUGS whenever they add or remove a top-level route; reference the RESERVED_SLUGS symbol when placing the comment.app/routes/resources/+organization/functions/queries.server.ts (1)
3-11: Remove duplication betweenlistUserOrganizationsandgetUserOrganizations.This function duplicates the query logic in
getUserOrganizationsatapp/libs/auth.server.ts:244-257. Both use identical Kysely queries to fetch organizations by userId with the same joins and ordering. The only difference isgetUserOrganizationsalso selectsmembers.role. Extract a shared query function (or overload parameters) to prevent maintenance drift between the two implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/resources/`+organization/functions/queries.server.ts around lines 3 - 11, listUserOrganizations duplicates the Kysely query in getUserOrganizations; extract the shared query into a single helper (e.g., buildUserOrganizationsQuery or getUserOrganizationsQueryWithOptions) that accepts userId and an option to include members.role, move the .select/.innerJoin/.where/.orderBy logic into that helper, and update both listUserOrganizations and getUserOrganizations to call the helper (one requesting role, the other not) so joins, ordering and filters remain identical and maintenance is centralized.app/routes/admin/+create/mutations.server.ts (1)
14-27: Add graceful handling for duplicate slug errors to improve user experience.The
organizationstable has aUNIQUEconstraint onslug, so attempting to create a duplicate will raise a database error. While the route handler does catch this error (line 50), the user sees the raw database message "UNIQUE constraint failed: organizations.slug" rather than a friendly, domain-specific error.Consider pre-checking slug availability before the transaction or catching the constraint violation specifically to provide a clearer error message like "This slug is already in use."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/admin/`+create/mutations.server.ts around lines 14 - 27, The organization creation path currently throws raw DB constraint errors when inserting into organizations via db.transaction().execute and tsx.insertInto('organizations'); adjust this to return a user-friendly message for duplicate slugs by either (a) pre-checking slug existence with a quick select on organizations where slug = organizationSlug before performing the insert, or (b) catching the specific unique-constraint/database error from the insert inside the transaction (around the tsx.insertInto('organizations').returningAll().executeTakeFirstOrThrow call) and rethrowing a domain-specific Error like "This slug is already in use." Ensure you still keep the existing isReservedSlug check and only convert the DB unique-constraint case to the friendly message.app/routes/$orgSlug/settings/github-users._index/_layout.tsx (1)
84-108: Action mutation calls lack try/catch — errors will surface as 500s.Unlike the members action (which wraps
removeMemberin try/catch) and the export-settings action, none of theadd/update/deletearms here handle mutation errors. A DB constraint violation (e.g., duplicatelogin) will produce an unhandled 500 instead of a user-friendly error response.Consider wrapping mutation calls with try/catch to return structured errors, similar to the pattern in
export-settings.action.server.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/github-users._index/_layout.tsx around lines 84 - 108, The add/update/delete branches in the match(intent) handler (the async arms that call addGithubUser, updateGithubUser, and deleteGithubUser) do not catch errors — wrap each mutation call in a try/catch (or a single try around the parsed + mutation per arm) and on failure return a structured error response (e.g., data({ error: error.message || '...' }, { status: 400 or 422 })) instead of letting exceptions bubble to a 500; ensure you reference the same parsed inputs (addSchema/updateSchema/deleteSchema) and return the same success shape (data({ ok: true })) on success.app/routes/$orgSlug/settings/repositories.$repository.$pull/_layout.tsx (1)
46-62: Consider parallelizing sequential async I/O calls.Store reads (lines 48-51) and fetch calls (lines 59-62) are each awaited sequentially. Since they're independent, wrapping each group in
Promise.allwould reduce loader latency.Suggested optimization
- const storeData = { - commits: await store.loader.commits(pullId), - comments: await store.loader.discussions(pullId), - reviews: await store.loader.reviews(pullId), - } + const [storeCommits, storeComments, storeReviews] = await Promise.all([ + store.loader.commits(pullId), + store.loader.discussions(pullId), + store.loader.reviews(pullId), + ]) + const storeData = { commits: storeCommits, comments: storeComments, reviews: storeReviews } - const fetchData = { - commits: await fetcher.commits(pullId), - comments: await fetcher.comments(pullId), - reviews: await fetcher.reviews(pullId), - } + const [fetchCommits, fetchComments, fetchReviews] = await Promise.all([ + fetcher.commits(pullId), + fetcher.comments(pullId), + fetcher.reviews(pullId), + ]) + const fetchData = { commits: fetchCommits, comments: fetchComments, reviews: fetchReviews }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.$repository.$pull/_layout.tsx around lines 46 - 62, The three sequential awaits for store data (store.loader.commits, store.loader.discussions, store.loader.reviews called to build storeData) and the three sequential awaits for fetcher data (fetcher.commits, fetcher.comments, fetcher.reviews called to build fetchData) should be run in parallel; replace the sequential awaits with Promise.all for each group so createStore(...)/createFetcher(...) calls remain the same but construct storeData and fetchData by awaiting Promise.all on the trio of loader/fetcher promises (refer to createStore, store.loader.commits/discussions/reviews, createFetcher, fetcher.commits/comments/reviews, and the storeData/fetchData variables).app/routes/$orgSlug/settings/repositories.$repository.delete/_layout.tsx (1)
60-66:readOnlyis redundant alongsidedisabled.The
disabledattribute already prevents user interaction and editing. Having bothreadOnlyanddisabledis redundant.Proposed fix
<Input - readOnly disabled defaultValue={`${repository.owner}/${repository.repo}`} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.$repository.delete/_layout.tsx around lines 60 - 66, The Input element inside the Form (using getFormProps(form)) currently sets both readOnly and disabled on the same field; remove the redundant readOnly prop from the Input that renders `${repository.owner}/${repository.repo}` so the field remains non-interactive via disabled only and avoid duplicate attributes (keep Input, Label, Form, getFormProps, repository as-is).app/routes/$orgSlug/settings/repositories.$repository._index/_layout.tsx (1)
82-88: Consider using Tailwind utility classes instead of inline styles for text wrapping.The inline
styleobject for text wrapping could be replaced with Tailwind classes for consistency with the rest of the UI.Proposed change
<TableCell - style={{ - lineBreak: 'strict', - wordBreak: 'normal', - overflowWrap: 'anywhere', - }} + className="break-words" >Note: Verify
break-words(overflow-wrap: anywhere) provides the desired wrapping behavior. Tailwind doesn't have a directline-break: strictutility, so if that specific behavior is needed, the inline style is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.$repository._index/_layout.tsx around lines 82 - 88, Replace the inline style object on the TableCell component with Tailwind utility classes: remove the style prop and add a className that captures the wrapping behavior (e.g., "break-words break-normal" or "break-words whitespace-normal" to approximate overflow-wrap: anywhere and normal word-break). If you still require the specific lineBreak: 'strict' behavior, keep only that single inline style and move the rest to Tailwind classes on the same TableCell; update the TableCell instance accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/routes/`$orgSlug/settings/_index/+forms/delete-organization.action.server.ts:
- Line 29: The current post-delete redirect uses redirect('/admin'), which is
only valid for super-admins (requireSuperAdmin) and causes cascading redirects
for organization owners validated via requireOrgAdmin; update the action that
throws redirect('/admin') to instead throw redirect('/') so non-super-admin org
owners are sent to the generic root flow (which will route them to their next
org or /no-org) after deleting an organization.
In `@app/routes/`$orgSlug/settings/members/_layout.tsx:
- Around line 65-68: The changeRoleSchema currently allows any non-empty string
for role, enabling privilege escalation; update changeRoleSchema to validate
role against an explicit allowlist (use z.enum with the exact roles admins are
permitted to assign, e.g., "admin" and "member" or whatever your DB/ACL permits)
and update the changeMemberRole mutation's parameter types/signature to accept
that same constrained role type so both the request parsing and downstream
mutation enforce the same enum.
In `@app/routes/resources/`+organization/hooks/useCurrentOrganization.ts:
- Around line 9-16: The hook useCurrentOrganization reads the raw first URL
segment (via useLocation -> location.pathname -> segments[0]) and compares it to
RESERVED_SLUGS directly, creating a case-sensitivity mismatch with server helper
isReservedSlug which lowercases before checking; change the hook to normalize
the segment (e.g., const firstSegment = segments[0]?.toLowerCase() or similar)
before testing RESERVED_SLUGS.has(...) and return the normalized slug so client
and server use the same lowercase canonical form.
In `@vite.config.ts`:
- Around line 7-9: The devtoolsJson() plugin is currently included
unconditionally in defineConfig which exposes a server-side filesystem path in
production; change the export to use the function form of
defineConfig(({command, mode}) => ...) and only add devtoolsJson() to the
plugins array when in dev (e.g., command === 'serve' or mode === 'development'),
leaving tailwindcss(), reactRouter(), and tsconfigPaths() always enabled so the
plugin is gated to development only.
In `@vitest.config.ts`:
- Around line 6-8: The test.exclude override drops Vitest's default exclude
patterns and uses non-glob strings; import configDefaults from 'vitest/config'
and merge its exclude with your additional patterns (e.g., include a glob like
'**/opensrc/**' if needed) when setting test.exclude so you extend instead of
replace defaults; adjust the pattern strings to proper picomatch globs (use
'**/opensrc/**' not 'opensrc') and avoid re-adding '**/node_modules/**' since it
is already present in configDefaults.exclude.
- Around line 1-5: vitest.config.ts currently overrides vite.config.ts and drops
essential Vite plugins (tailwindcss, reactRouter, devtoolsJson); instead import
mergeConfig from 'vite' and the base config from vite.config.ts, then export a
merged config (use mergeConfig(baseConfig, defineConfig({ test: {...} }))) so
tests inherit plugins; remove the duplicate tsconfigPaths() plugin from
vitest.config.ts since it is already in vite.config.ts and ensure only
test-specific settings remain in the defineConfig call you merge.
---
Outside diff comments:
In `@app/routes/`$orgSlug/settings/repositories.add/_layout.tsx:
- Around line 148-157: The hidden inputs mapping uses
[...searchParams.entries()].map(([key, value]) => ...) with key={key}, which
collides for duplicate query parameter names; update the mapping in the Form
that renders searchParams so each element gets a unique React key (e.g., include
the index or the value in the key like `${key}-${i}` or `${key}-${value}-${i}`)
while keeping name={key} and value={value} intact so duplicate keys are
preserved in the submitted form; change the map callback signature to receive
the index (map(([key, value], i) => ...) and use that index in the key.
In `@CLAUDE.md`:
- Around line 62-68: Update the stale "Project Structure" block under the app/
routes/ listing to reflect the new $orgSlug-based routing convention: replace
the phrase "File-based routing (remix-flat-routes convention)" and the old path
examples `_dashboard+/`, `admin+/`, `_auth+/`, `api.auth.$/` with the new
routing description (mentioning $orgSlug/) and example directories/files that
match the PR's routing layout (e.g. `$orgSlug/` and any org-scoped subroutes);
ensure the Routing Convention wording now matches the updated section further
down the file.
---
Duplicate comments:
In `@app/routes/`$orgSlug/settings/_index/+functions/queries.server.ts:
- Around line 23-39: The createDefaultOrganizationSetting function is already
idempotent and safe for concurrent calls: keep the .onConflict((oc) =>
oc.doNothing()) on the insert and the subsequent
getOrganizationSetting(organizationId) read-back and the throw-on-missing row
behavior as implemented; no code changes required to address the
concurrency/idempotency concern in createDefaultOrganizationSetting.
In `@app/routes/`$orgSlug/settings/members/mutations.server.ts:
- Around line 3-14: The changeMemberRole function currently accepts a plain
string for the role parameter which allows invalid values; update the function
signature to type the role as DB.Members['role'] (or the equivalent union type
used by your DB types) so TypeScript enforces valid member roles, then adjust
any call-sites if necessary to satisfy the narrower type; locate the
changeMemberRole declaration and the .set({ role }) usage to apply the type
change.
---
Nitpick comments:
In `@app/components/layout/main.tsx`:
- Around line 4-6: The interface MainProps is currently declared but not
exported, preventing consumers from importing it for typing; update the
declaration of MainProps so it is exported (e.g., export interface MainProps
extends React.ComponentPropsWithRef<'main'> { fixed?: boolean }) and ensure any
related named export for the Main component remains consistent so external
modules can import both Main and MainProps; locate the interface named MainProps
in main.tsx and change its declaration to an exported interface.
In `@app/libs/auth.server.ts`:
- Line 177: Move the mid-file import of RESERVED_SLUGS into the main import
block at the top of app/libs/auth.server.ts alongside the other imports and
remove the existing import statement at line 177; ensure any references to
RESERVED_SLUGS remain unchanged and that there are no duplicate imports after
relocating it.
- Around line 189-228: Replace raw-string and template-literal redirects with
the type-safe href() helper so redirect usages are consistent: update the
'/no-org' redirect in requireOrgMember to redirect(href('/no-org')) and update
the template-literal redirect in requireOrgAdmin to
redirect(href('/your-org-admin-route')) (use the actual route string used
elsewhere). Ensure all redirect(...) calls in requireOrgMember and
requireOrgAdmin use href(...) for consistency with other functions like
getSession and the earlier redirects.
- Around line 183-187: The OrgContext interface currently types membership.role
as a plain string; narrow it to a stricter union or DB-derived type to improve
type safety. Update OrgContext (membership.role) to use a union like
'owner'|'admin'|'member' or import the role enum/type from your DB schema, then
update any callers (e.g., requireOrgAdmin and the delete-organization action) to
rely on the narrowed type so role checks are type-safe across the codebase.
In `@app/libs/reserved-slugs.ts`:
- Around line 1-12: Add a brief inline comment immediately above the
RESERVED_SLUGS declaration reminding contributors to keep this Set in sync with
any new top-level route segments (e.g., when adding new pages or API roots), and
optionally point to where top-level routes are defined so they remember to
update RESERVED_SLUGS whenever they add or remove a top-level route; reference
the RESERVED_SLUGS symbol when placing the comment.
In `@app/routes/`$orgSlug/settings/github-users._index/_layout.tsx:
- Around line 84-108: The add/update/delete branches in the match(intent)
handler (the async arms that call addGithubUser, updateGithubUser, and
deleteGithubUser) do not catch errors — wrap each mutation call in a try/catch
(or a single try around the parsed + mutation per arm) and on failure return a
structured error response (e.g., data({ error: error.message || '...' }, {
status: 400 or 422 })) instead of letting exceptions bubble to a 500; ensure you
reference the same parsed inputs (addSchema/updateSchema/deleteSchema) and
return the same success shape (data({ ok: true })) on success.
In `@app/routes/`$orgSlug/settings/repositories.$repository._index/_layout.tsx:
- Around line 82-88: Replace the inline style object on the TableCell component
with Tailwind utility classes: remove the style prop and add a className that
captures the wrapping behavior (e.g., "break-words break-normal" or "break-words
whitespace-normal" to approximate overflow-wrap: anywhere and normal
word-break). If you still require the specific lineBreak: 'strict' behavior,
keep only that single inline style and move the rest to Tailwind classes on the
same TableCell; update the TableCell instance accordingly.
In `@app/routes/`$orgSlug/settings/repositories.$repository.$pull/_layout.tsx:
- Around line 46-62: The three sequential awaits for store data
(store.loader.commits, store.loader.discussions, store.loader.reviews called to
build storeData) and the three sequential awaits for fetcher data
(fetcher.commits, fetcher.comments, fetcher.reviews called to build fetchData)
should be run in parallel; replace the sequential awaits with Promise.all for
each group so createStore(...)/createFetcher(...) calls remain the same but
construct storeData and fetchData by awaiting Promise.all on the trio of
loader/fetcher promises (refer to createStore,
store.loader.commits/discussions/reviews, createFetcher,
fetcher.commits/comments/reviews, and the storeData/fetchData variables).
In `@app/routes/`$orgSlug/settings/repositories.$repository.delete/_layout.tsx:
- Around line 60-66: The Input element inside the Form (using
getFormProps(form)) currently sets both readOnly and disabled on the same field;
remove the redundant readOnly prop from the Input that renders
`${repository.owner}/${repository.repo}` so the field remains non-interactive
via disabled only and avoid duplicate attributes (keep Input, Label, Form,
getFormProps, repository as-is).
In `@app/routes/admin/`+create/mutations.server.ts:
- Around line 14-27: The organization creation path currently throws raw DB
constraint errors when inserting into organizations via db.transaction().execute
and tsx.insertInto('organizations'); adjust this to return a user-friendly
message for duplicate slugs by either (a) pre-checking slug existence with a
quick select on organizations where slug = organizationSlug before performing
the insert, or (b) catching the specific unique-constraint/database error from
the insert inside the transaction (around the
tsx.insertInto('organizations').returningAll().executeTakeFirstOrThrow call) and
rethrowing a domain-specific Error like "This slug is already in use." Ensure
you still keep the existing isReservedSlug check and only convert the DB
unique-constraint case to the friendly message.
In `@app/routes/resources/`+organization/functions/queries.server.ts:
- Around line 3-11: listUserOrganizations duplicates the Kysely query in
getUserOrganizations; extract the shared query into a single helper (e.g.,
buildUserOrganizationsQuery or getUserOrganizationsQueryWithOptions) that
accepts userId and an option to include members.role, move the
.select/.innerJoin/.where/.orderBy logic into that helper, and update both
listUserOrganizations and getUserOrganizations to call the helper (one
requesting role, the other not) so joins, ordering and filters remain identical
and maintenance is centralized.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
.github/workflows/deploy.ymlCLAUDE.mdapp/components/AppHeader.tsxapp/components/layout/main.tsxapp/components/layout/nav-group.tsxapp/components/ui/sidebar.tsxapp/hooks/use-debounce.tsapp/libs/auth.server.tsapp/libs/reserved-slugs.tsapp/routes/$orgSlug/ongoing/_layout.tsxapp/routes/$orgSlug/settings/_index/+forms/delete-organization.action.server.tsapp/routes/$orgSlug/settings/_index/+forms/export-settings.action.server.tsapp/routes/$orgSlug/settings/_index/+forms/integration-settings.action.server.tsapp/routes/$orgSlug/settings/_index/+forms/organization-settings.action.server.tsapp/routes/$orgSlug/settings/_index/+forms/organization-settings.tsxapp/routes/$orgSlug/settings/_index/+functions/queries.server.tsapp/routes/$orgSlug/settings/github-users._index/_layout.tsxapp/routes/$orgSlug/settings/members/+components/data-table-toolbar.tsxapp/routes/$orgSlug/settings/members/+components/members-table.tsxapp/routes/$orgSlug/settings/members/_layout.tsxapp/routes/$orgSlug/settings/members/mutations.server.tsapp/routes/$orgSlug/settings/repositories.$repository.$pull/_layout.tsxapp/routes/$orgSlug/settings/repositories.$repository.$pull/queries.server.tsapp/routes/$orgSlug/settings/repositories.$repository._index/_layout.tsxapp/routes/$orgSlug/settings/repositories.$repository.delete/_layout.tsxapp/routes/$orgSlug/settings/repositories.$repository.settings/_layout.tsxapp/routes/$orgSlug/settings/repositories.add/_layout.tsxapp/routes/admin/+create/mutations.server.tsapp/routes/admin/_index/queries.server.tsapp/routes/admin/create.tsxapp/routes/resources/+organization/functions.server.tsapp/routes/resources/+organization/functions/queries.server.tsapp/routes/resources/+organization/hooks/useCurrentOrganization.tsapp/routes/resources/organization.tsxlab/lib/classify.tspackage.jsontsconfig.jsonvite.config.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- app/components/ui/sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- app/hooks/use-debounce.ts
- app/routes/$orgSlug/settings/members/+components/data-table-toolbar.tsx
- app/components/layout/nav-group.tsx
- app/routes/$orgSlug/settings/members/+components/members-table.tsx
- app/routes/$orgSlug/settings/repositories.$repository.$pull/queries.server.ts
- app/routes/admin/_index/queries.server.ts
- tsconfig.json
- app/routes/$orgSlug/settings/_index/+forms/organization-settings.action.server.ts
- lab/lib/classify.ts
- app/components/AppHeader.tsx
- app/routes/$orgSlug/settings/_index/+forms/integration-settings.action.server.ts
app/routes/$orgSlug/settings/_index/+forms/delete-organization.action.server.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+65
to
+68
| const changeRoleSchema = z.object({ | ||
| memberId: z.string().min(1), | ||
| role: z.string().min(1), | ||
| }) |
There was a problem hiding this comment.
role field should be validated against an allowlist to prevent privilege escalation.
z.string().min(1) allows any value, including owner. An org admin could craft a request with role=owner to escalate a member's privileges. The downstream mutation (changeMemberRole) also accepts an unconstrained string. Restrict to allowed values.
Proposed fix
const changeRoleSchema = z.object({
memberId: z.string().min(1),
- role: z.string().min(1),
+ role: z.enum(['admin', 'member']),
})Adjust the enum values to match the roles an admin is permitted to assign.
#!/bin/bash
# Check if there's a DB-level enum or constraint on the 'role' column in the members table,
# and verify what role values are used across the codebase.
echo "=== Searching for role enum/constraint definitions ==="
rg -n -C3 'role.*enum\|role.*check\|members.*role' --type=sql 2>/dev/null || echo "No SQL files found"
echo ""
echo "=== Searching for member role values used in the codebase ==="
rg -nP "role.*['\"](?:owner|admin|member)['\"]" --type=ts -g '!node_modules/**' -C1
echo ""
echo "=== Checking the changeMemberRole mutation ==="
ast-grep --pattern 'export const changeMemberRole = async ($$$) {
$$$
}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/`$orgSlug/settings/members/_layout.tsx around lines 65 - 68, The
changeRoleSchema currently allows any non-empty string for role, enabling
privilege escalation; update changeRoleSchema to validate role against an
explicit allowlist (use z.enum with the exact roles admins are permitted to
assign, e.g., "admin" and "member" or whatever your DB/ACL permits) and update
the changeMemberRole mutation's parameter types/signature to accept that same
constrained role type so both the request parsing and downstream mutation
enforce the same enum.
Comment on lines
+9
to
+16
| export const useCurrentOrganization = () => { | ||
| const location = useLocation() | ||
| const segments = location.pathname.split('/').filter(Boolean) | ||
| const firstSegment = segments[0] | ||
| if (!firstSegment || RESERVED_SLUGS.has(firstSegment)) { | ||
| return null | ||
| } | ||
| return firstSegment |
There was a problem hiding this comment.
Case-sensitivity mismatch with isReservedSlug.
isReservedSlug in app/libs/auth.server.ts (Line 180) lowercases the slug before checking against RESERVED_SLUGS, but this hook compares the raw URL segment directly. If a path like /Admin/... is somehow reached, the hook would return 'Admin' as a valid org slug while the server-side helper would correctly reject it.
Consider lowercasing for consistency:
Proposed fix
export const useCurrentOrganization = () => {
const location = useLocation()
const segments = location.pathname.split('/').filter(Boolean)
const firstSegment = segments[0]
- if (!firstSegment || RESERVED_SLUGS.has(firstSegment)) {
+ if (!firstSegment || RESERVED_SLUGS.has(firstSegment.toLowerCase())) {
return null
}
return firstSegment
}📝 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
| export const useCurrentOrganization = () => { | |
| const location = useLocation() | |
| const segments = location.pathname.split('/').filter(Boolean) | |
| const firstSegment = segments[0] | |
| if (!firstSegment || RESERVED_SLUGS.has(firstSegment)) { | |
| return null | |
| } | |
| return firstSegment | |
| export const useCurrentOrganization = () => { | |
| const location = useLocation() | |
| const segments = location.pathname.split('/').filter(Boolean) | |
| const firstSegment = segments[0] | |
| if (!firstSegment || RESERVED_SLUGS.has(firstSegment.toLowerCase())) { | |
| return null | |
| } | |
| return firstSegment | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/resources/`+organization/hooks/useCurrentOrganization.ts around
lines 9 - 16, The hook useCurrentOrganization reads the raw first URL segment
(via useLocation -> location.pathname -> segments[0]) and compares it to
RESERVED_SLUGS directly, creating a case-sensitivity mismatch with server helper
isReservedSlug which lowercases before checking; change the hook to normalize
the segment (e.g., const firstSegment = segments[0]?.toLowerCase() or similar)
before testing RESERVED_SLUGS.has(...) and return the normalized slug so client
and server use the same lowercase canonical form.
Change onConflict key from user-controlled `id` to server-derived `organizationId` in upsertIntegration and upsertExportSetting, closing a cross-org data modification vulnerability. Remove hidden `id` inputs from forms and `id` fields from Zod schemas. Add OrganizationScopePlugin — a Kysely plugin that auto-injects `WHERE organization_id = ?` into SELECT/UPDATE/DELETE queries on org-scoped tables, for defense-in-depth. Includes 8 tests. Update CLAUDE.md with multi-tenant security rules and project structure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Redirect to / after org deletion (not /admin which requires super-admin) - Validate role with z.enum(['owner','admin','member']) to prevent escalation - Case-insensitive reserved slug check in useCurrentOrganization - Gate devtoolsJson() plugin to non-production mode - Use mergeConfig in vitest.config.ts to inherit vite plugins and defaults Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
coji
changed the title
This was referenced Feb 24, 2026
coji
added a commit
that referenced
this pull request
Feb 26, 2026
This was referenced Feb 27, 2026
5 tasks
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Single-tenant dashboard → multi-tenant SaaS with organization-scoped routing, membership-based access control, and a modern sidebar layout.
Core Changes
/:orgSlug/...) replace old flat/_dashboard+/$organizationpathsrequireOrgMember/requireOrgAdminon all org-scoped routesWHERE organization_id = ?on SELECT/UPDATE/DELETE for defense-in-depth tenant isolationSecurity
onConflict(organizationId)instead of user-controlledidz.enum(['owner', 'admin', 'member'])to prevent privilege escalationorganizationIdownership before accessdevtoolsJson()gated to dev mode to prevent filesystem path disclosure in productionInfrastructure
react-router-auto-routesreplacesremix-flat-routesfor simpler file-based routingvitest.config.tsusesmergeConfigto inherit Vite plugins properlyDATABASE_URLvalidation guard indb.server.tspnpm setup→pnpm db:setup(avoids pnpm builtin conflict)start.shfor production deployLab (experimental)
Test plan
pnpm typecheckpassespnpm lintpassespnpm testpasses (34 tests including OrganizationScopePlugin)pnpm buildsucceedsstart.shflow🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores