Conversation
Add team-level visitor groups that allow users to define named groups of emails/domains once, then apply them to document and data room links. New models: - VisitorGroup: team-scoped named group with emails/domains list - LinkVisitorGroup: many-to-many join table between Link and VisitorGroup Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
- GET/POST /api/teams/:teamId/visitor-groups - list & create - GET/PUT/DELETE /api/teams/:teamId/visitor-groups/:groupId - CRUD - Deletion protection: prevents deletion if group is used by active links - SWR hook for consuming visitor groups data in UI Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
- Add VisitorGroupsSection component with create/edit/delete UI - Add VisitorGroupModal for creating and editing groups - Add 'Visitor Groups' tab to /visitors page - Update AllowListSection with multi-group selector (popover with checkboxes) - Add visitorGroupIds to DEFAULT_LINK_TYPE and link sheet data flow - Update link create/update APIs to handle visitor group associations - Update document and dataroom link fetch APIs to include visitorGroups - Update LinkWithViews type to include visitorGroups - Update views and views-dataroom routes to merge group emails with allow list - Visitor group emails are additive: groups + individual emails combined Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
Ensure dataroom group links endpoint also returns visitor group associations for proper editing in the link sheet. Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
Incorporates fixes from PR #2069 (cursor/link-allow-list-email-update): - Replace useEffect-based sync with direct onChange handler updates for allow-list and deny-list, preventing stale data on quick saves - useEffect now only handles disabling when emailProtected is turned off - Add key props to AllowListSection and DenyListSection in link-options to force remount when switching between links (prevents stale state) - Visitor group clearing is also handled in the new useEffect pattern Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds visitor groups across DB, API, and UI: new Prisma models and migration, team-scoped visitor-group CRUD APIs, SWR hook and types, visitor-group UI and modal, link create/update flow and link edit UI extended with visitorGroupIds, and view authorization updated to combine link.allowList with visitor-group emails. Changes
Possibly related PRs
🚥 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. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/views/route.ts (1)
496-496:⚠️ Potential issue | 🟡 MinorRemove debug
console.log.This looks like a leftover debug statement that shouldn't ship to production.
- console.log("previewSession", previewSession);
🤖 Fix all issues with AI agents
In `@components/visitors/visitor-group-modal.tsx`:
- Around line 62-94: The finally block currently calls setOpen(false) which
closes the modal even when the API returns an error; change control flow so that
setOpen(false) and the
mutate(`/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`) call only run
on successful responses (after response.ok and toast.success), while keeping
setLoading(false) in finally to always clear loading; ensure the catch block
shows the error toast but does NOT call setOpen(false) or mutate so user input
is preserved when saving fails.
In `@pages/api/links/`[id]/index.ts:
- Around line 426-443: The update handler currently deletes and recreates
link-visitor group associations without verifying the provided visitorGroupIds
belong to the same team as the link; before calling
tx.linkVisitorGroup.deleteMany/createMany, fetch the existing link (to get its
teamId) and query tx.visitorGroup.findMany({ where: { id: { in:
linkData.visitorGroupIds }, teamId: link.teamId } }) and ensure the returned
count matches linkData.visitorGroupIds.length (or reject any ids not matching) —
if there are mismatches throw a validation error and abort the transaction; only
then proceed with tx.linkVisitorGroup.deleteMany and
tx.linkVisitorGroup.createMany.
- Around line 424-444: The response returns stale visitorGroups because
tx.link.update captures the old related data into the link variable before you
delete/create associations with tx.linkVisitorGroup.deleteMany and createMany;
fix by ensuring the returned link includes the post-update associations—either
move the visitor-group deletion/creation logic to run before calling
tx.link.update (so its include picks up the new associations) or, after
createMany, re-query the link's visitorGroups (e.g., refetch via
tx.link.findUnique with include: { visitorGroups: true }) and use that fresh
result for the response instead of the original link variable.
In `@pages/api/links/index.ts`:
- Around line 240-251: Validate that each ID in linkData.visitorGroupIds belongs
to the requesting user's team before passing them into the Prisma create/update
payload (the visitorGroups: { createMany: { data: ... } } block). Specifically,
in both the POST and PATCH handlers where linkData.visitorGroupIds is used,
fetch the visitor groups by the provided IDs (e.g., via a DB call filtering on
id IN visitorGroupIds and teamId === request.teamId) and ensure the returned
count matches the input count; if not, throw a 400/403 error. Only after this
check, map the validated visitorGroupIds into the visitorGroups createMany data;
keep the same variable names (linkData, visitorGroupIds, visitorGroups) so the
rest of the handler remains unchanged.
🧹 Nitpick comments (8)
pages/api/teams/[teamId]/visitor-groups/[groupId]/index.ts (1)
77-105: Emails are stored without validation or sanitization.The
emailsarray fromreq.bodyis persisted directly without verifying that entries are valid email addresses or domain patterns. Malformed entries could silently break downstream access-control checks in the views routes (whereisEmailMatchedis used against these stored values).Consider reusing the
sanitizeListutility (fromlib/utils.ts) or at minimum validating email/domain format server-side before storing.Proposed fix
+ import { sanitizeList } from "@/lib/utils"; + // PUT /api/teams/:teamId/visitor-groups/:groupId const { name, emails } = req.body as { name: string; emails: string[] }; if (!name || name.trim() === "") { return res.status(400).json({ error: "Group name is required." }); } + const sanitizedEmails = emails + ? sanitizeList(emails.join("\n")) + : []; + try { const visitorGroup = await prisma.visitorGroup.update({ where: { id: groupId, teamId, }, data: { name: name.trim(), - emails: emails || [], + emails: sanitizedEmails, },#!/bin/bash # Check if the same lack of validation exists in the POST handler rg -n 'emails' pages/api/teams/*/visitor-groups/index.ts fd 'index.ts' --path 'visitor-groups' --exec cat {}pages/api/teams/[teamId]/visitor-groups/index.ts (2)
14-51: Consider extracting auth/team-access check before method routing.The session retrieval and team access verification are duplicated between GET and POST branches. The sibling
[groupId]/index.tsroute already extracts this before method dispatch. Aligning here would reduce duplication.Sketch
export default async function handle(req, res) { + const session = await getServerSession(req, res, authOptions); + if (!session) return res.status(401).end("Unauthorized"); + const { teamId } = req.query as { teamId: string }; + const userId = (session.user as CustomUser).id; + const teamAccess = await prisma.userTeam.findUnique({ + where: { userId_teamId: { userId, teamId } }, + }); + if (!teamAccess) return res.status(401).end("Unauthorized"); + if (req.method === "GET") { - const session = ... - // ...team access check... // ...rest of GET logic } else if (req.method === "POST") { - const session = ... - // ...team access check... // ...rest of POST logic } }
81-92: Same email validation gap as in the[groupId]PUT handler.The
emailsarray fromreq.bodyis stored directly without validation. Apply the same sanitization recommended for the[groupId]route to maintain consistency.prisma/schema/link.prisma (1)
168-182: Consider adding a unique constraint on(teamId, name)to prevent duplicate group names within a team.Without this, users could create multiple visitor groups with the same name under the same team, leading to confusion in the UI (e.g., the multi-group selector in
AllowListSection).Suggested change
@@index([teamId]) + @@unique([teamId, name]) }components/visitors/visitor-group-modal.tsx (1)
39-47:useEffectwithexistingGroupobject reference may cause unnecessary re-runs.If the parent re-renders and creates a new
existingGroupobject reference (even with the same data), this effect will re-fire and reset the form mid-edit. Consider comparing byexistingGroup?.idandopeninstead, or memoizing the prop in the parent.Minimal fix
useEffect(() => { if (existingGroup) { setGroupName(existingGroup.name); setEmailsInput(existingGroup.emails.join("\n")); } else { setGroupName(""); setEmailsInput(""); } - }, [existingGroup, open]); + }, [existingGroup?.id, open]);components/visitors/visitor-groups-section.tsx (1)
48-74: CleareditingGroupafter deletion to prevent stale references.If the deleted group happens to be the one stored in
editingGroup, subsequent modal opens (e.g., via a rapid edit click before the list re-renders) could reference a deleted entity.Suggested fix
toast.success("Visitor group deleted successfully."); mutate(`/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`); + setEditingGroup((prev) => + prev?.id === group.id ? null : prev, + ); } catch (error) {pages/visitors/index.tsx (1)
26-31:useViewersfetches data even when the "groups" tab is active.Consider conditionally fetching viewers only when the "visitors" tab is active (e.g., pass
activeTab === "visitors"as anenabledflag), to avoid unnecessary API calls when the user is on the groups tab.components/links/link-sheet/allow-list-section.tsx (1)
73-91: When re-enabling the allow list,emailProtectedis forcedtruebutemailAuthenticatedis only set on enable, not cleared on disable.On Line 81, enabling sets
emailAuthenticated: true. But on Lines 84–89, disabling only clearsallowListandvisitorGroupIds— it does not resetemailAuthenticatedback. This is likely intentional (email authentication is a separate setting the user may want to keep), but worth confirming that this asymmetry matches the desired UX.
| try { | ||
| const url = existingGroup | ||
| ? `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups/${existingGroup.id}` | ||
| : `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`; | ||
|
|
||
| const response = await fetch(url, { | ||
| method: existingGroup ? "PUT" : "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ name: groupName.trim(), emails }), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const data = await response.json(); | ||
| toast.error(data.error || "Failed to save visitor group."); | ||
| setLoading(false); | ||
| return; | ||
| } | ||
|
|
||
| toast.success( | ||
| existingGroup | ||
| ? "Visitor group updated successfully!" | ||
| : "Visitor group created successfully!", | ||
| ); | ||
|
|
||
| mutate( | ||
| `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`, | ||
| ); | ||
| } catch (error) { | ||
| toast.error("Error saving visitor group. Please try again."); | ||
| } finally { | ||
| setLoading(false); | ||
| setOpen(false); | ||
| } |
There was a problem hiding this comment.
Bug: finally block closes the modal even on API errors, discarding user input.
In JavaScript, finally always executes — even after the early return on line 77. So when the API returns a non-OK response, setOpen(false) on line 93 still runs, closing the modal and losing the user's input despite the error toast.
Move setOpen(false) and mutate into the success path only:
Proposed fix
try {
const url = existingGroup
? `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups/${existingGroup.id}`
: `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`;
const response = await fetch(url, {
method: existingGroup ? "PUT" : "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ name: groupName.trim(), emails }),
});
if (!response.ok) {
const data = await response.json();
toast.error(data.error || "Failed to save visitor group.");
- setLoading(false);
return;
}
toast.success(
existingGroup
? "Visitor group updated successfully!"
: "Visitor group created successfully!",
);
mutate(
`/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`,
);
+ setOpen(false);
} catch (error) {
toast.error("Error saving visitor group. Please try again.");
} finally {
setLoading(false);
- setOpen(false);
}📝 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.
| try { | |
| const url = existingGroup | |
| ? `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups/${existingGroup.id}` | |
| : `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`; | |
| const response = await fetch(url, { | |
| method: existingGroup ? "PUT" : "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ name: groupName.trim(), emails }), | |
| }); | |
| if (!response.ok) { | |
| const data = await response.json(); | |
| toast.error(data.error || "Failed to save visitor group."); | |
| setLoading(false); | |
| return; | |
| } | |
| toast.success( | |
| existingGroup | |
| ? "Visitor group updated successfully!" | |
| : "Visitor group created successfully!", | |
| ); | |
| mutate( | |
| `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`, | |
| ); | |
| } catch (error) { | |
| toast.error("Error saving visitor group. Please try again."); | |
| } finally { | |
| setLoading(false); | |
| setOpen(false); | |
| } | |
| try { | |
| const url = existingGroup | |
| ? `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups/${existingGroup.id}` | |
| : `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`; | |
| const response = await fetch(url, { | |
| method: existingGroup ? "PUT" : "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ name: groupName.trim(), emails }), | |
| }); | |
| if (!response.ok) { | |
| const data = await response.json(); | |
| toast.error(data.error || "Failed to save visitor group."); | |
| return; | |
| } | |
| toast.success( | |
| existingGroup | |
| ? "Visitor group updated successfully!" | |
| : "Visitor group created successfully!", | |
| ); | |
| mutate( | |
| `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`, | |
| ); | |
| setOpen(false); | |
| } catch (error) { | |
| toast.error("Error saving visitor group. Please try again."); | |
| } finally { | |
| setLoading(false); | |
| } |
🤖 Prompt for AI Agents
In `@components/visitors/visitor-group-modal.tsx` around lines 62 - 94, The
finally block currently calls setOpen(false) which closes the modal even when
the API returns an error; change control flow so that setOpen(false) and the
mutate(`/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`) call only run
on successful responses (after response.ok and toast.success), while keeping
setLoading(false) in finally to always clear loading; ensure the catch block
shows the error toast but does NOT call setOpen(false) or mutate so user input
is preserved when saving fails.
|
|
||
| // Update visitor groups (replace all) | ||
| if (linkData.visitorGroupIds !== undefined) { | ||
| // Delete existing visitor group associations | ||
| await tx.linkVisitorGroup.deleteMany({ | ||
| where: { linkId: id }, | ||
| }); | ||
|
|
||
| // Create new associations | ||
| if (linkData.visitorGroupIds?.length > 0) { | ||
| await tx.linkVisitorGroup.createMany({ | ||
| data: linkData.visitorGroupIds.map( | ||
| (visitorGroupId: string) => ({ | ||
| linkId: id, | ||
| visitorGroupId, | ||
| }), | ||
| ), | ||
| skipDuplicates: true, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Response returns stale visitorGroups after update.
The tx.link.update on Line 326 includes visitorGroups and captures the result in link. Then Lines 428–443 delete and re-create the visitor group associations. But the link object (and thus the response at Line 489) still contains the old visitorGroups from before the delete+create.
The client will receive stale visitor group data until a re-fetch, causing potential UI inconsistencies (e.g., the link sheet may show the wrong groups after saving).
Re-query the visitor groups after creating the new associations, or move the association management before the link update's include.
🐛 Proposed fix — re-query visitor groups after update
await tx.linkVisitorGroup.createMany({
data: linkData.visitorGroupIds.map(
(visitorGroupId: string) => ({
linkId: id,
visitorGroupId,
}),
),
skipDuplicates: true,
});
}
+
+ // Re-query to get updated visitor groups
+ const updatedVisitorGroups = await tx.linkVisitorGroup.findMany({
+ where: { linkId: id },
+ select: { visitorGroupId: true },
+ });
+ link.visitorGroups = updatedVisitorGroups;
}🤖 Prompt for AI Agents
In `@pages/api/links/`[id]/index.ts around lines 424 - 444, The response returns
stale visitorGroups because tx.link.update captures the old related data into
the link variable before you delete/create associations with
tx.linkVisitorGroup.deleteMany and createMany; fix by ensuring the returned link
includes the post-update associations—either move the visitor-group
deletion/creation logic to run before calling tx.link.update (so its include
picks up the new associations) or, after createMany, re-query the link's
visitorGroups (e.g., refetch via tx.link.findUnique with include: {
visitorGroups: true }) and use that fresh result for the response instead of the
original link variable.
| if (linkData.visitorGroupIds !== undefined) { | ||
| // Delete existing visitor group associations | ||
| await tx.linkVisitorGroup.deleteMany({ | ||
| where: { linkId: id }, | ||
| }); | ||
|
|
||
| // Create new associations | ||
| if (linkData.visitorGroupIds?.length > 0) { | ||
| await tx.linkVisitorGroup.createMany({ | ||
| data: linkData.visitorGroupIds.map( | ||
| (visitorGroupId: string) => ({ | ||
| linkId: id, | ||
| visitorGroupId, | ||
| }), | ||
| ), | ||
| skipDuplicates: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Same cross-team validation concern as the POST endpoint.
No validation that visitorGroupIds belong to the same teamId as the link being updated. See the comment on pages/api/links/index.ts Lines 240–251 for the suggested fix.
🤖 Prompt for AI Agents
In `@pages/api/links/`[id]/index.ts around lines 426 - 443, The update handler
currently deletes and recreates link-visitor group associations without
verifying the provided visitorGroupIds belong to the same team as the link;
before calling tx.linkVisitorGroup.deleteMany/createMany, fetch the existing
link (to get its teamId) and query tx.visitorGroup.findMany({ where: { id: { in:
linkData.visitorGroupIds }, teamId: link.teamId } }) and ensure the returned
count matches linkData.visitorGroupIds.length (or reject any ids not matching) —
if there are mismatches throw a validation error and abort the transaction; only
then proceed with tx.linkVisitorGroup.deleteMany and
tx.linkVisitorGroup.createMany.
| // Connect visitor groups for allow-list | ||
| ...(linkData.visitorGroupIds?.length > 0 && { | ||
| visitorGroups: { | ||
| createMany: { | ||
| data: linkData.visitorGroupIds.map( | ||
| (visitorGroupId: string) => ({ | ||
| visitorGroupId, | ||
| }), | ||
| ), | ||
| }, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "\.ts$|\.prisma$" | head -20Repository: mfts/papermark
Length of output: 989
🏁 Script executed:
find . -name "*.prisma" -o -name "schema.prisma"Repository: mfts/papermark
Length of output: 375
🏁 Script executed:
ast-grep --pattern 'model VisitorGroup {
$$$
}'Repository: mfts/papermark
Length of output: 40
🏁 Script executed:
rg "model VisitorGroup" prisma/Repository: mfts/papermark
Length of output: 104
🏁 Script executed:
cat prisma/schema/link.prismaRepository: mfts/papermark
Length of output: 9827
🏁 Script executed:
cat -n pages/api/links/index.ts | sed -n '230,270p'Repository: mfts/papermark
Length of output: 1593
🏁 Script executed:
cat -n pages/api/links/index.ts | sed -n '1,50p'Repository: mfts/papermark
Length of output: 1749
🏁 Script executed:
cat -n pages/api/links/index.ts | sed -n '40,150p'Repository: mfts/papermark
Length of output: 3687
🏁 Script executed:
rg "visitorGroupIds" pages/api/links/index.ts -B 3 -A 3Repository: mfts/papermark
Length of output: 429
🏁 Script executed:
rg "visitorGroupIds" pages/api/links/ -B 5 -A 5Repository: mfts/papermark
Length of output: 2063
🏁 Script executed:
cat -n pages/api/links/index.ts | sed -n '150,240p'Repository: mfts/papermark
Length of output: 4424
🏁 Script executed:
cat -n pages/api/links/index.ts | sed -n '120,160p'Repository: mfts/papermark
Length of output: 1539
🏁 Script executed:
rg "agreementId|groupId" pages/api/links/index.ts -B 2 -A 2Repository: mfts/papermark
Length of output: 856
🏁 Script executed:
cat -n pages/api/links/\[id\]/index.ts | sed -n '1,50p'Repository: mfts/papermark
Length of output: 1859
🏁 Script executed:
rg -A 20 "Update visitor groups" pages/api/links/\[id\]/index.tsRepository: mfts/papermark
Length of output: 724
🏁 Script executed:
rg -B 50 "Update visitor groups" pages/api/links/\[id\]/index.ts | head -100Repository: mfts/papermark
Length of output: 1760
🏁 Script executed:
wc -l pages/api/links/\[id\]/index.tsRepository: mfts/papermark
Length of output: 91
🏁 Script executed:
cat -n pages/api/links/\[id\]/index.ts | sed -n '200,350p'Repository: mfts/papermark
Length of output: 5438
🏁 Script executed:
cat -n pages/api/links/\[id\]/index.ts | sed -n '350,430p'Repository: mfts/papermark
Length of output: 3548
🏁 Script executed:
cat -n pages/api/links/\[id\]/index.ts | sed -n '425,450p'Repository: mfts/papermark
Length of output: 1078
🏁 Script executed:
rg "visitorGroup" pages/api/links/ -A 3 -B 3 | grep -i "team\|validate\|where" | head -30Repository: mfts/papermark
Length of output: 120
🏁 Script executed:
rg "VisitorGroup" lib/ -lRepository: mfts/papermark
Length of output: 87
🏁 Script executed:
cat lib/swr/use-visitor-groups.tsRepository: mfts/papermark
Length of output: 741
🏁 Script executed:
rg "visitorGroup" lib/ -A 5 -B 5Repository: mfts/papermark
Length of output: 1584
🏁 Script executed:
rg "visitor-groups" pages/api/ -lRepository: mfts/papermark
Length of output: 165
🏁 Script executed:
rg "findMany.*VisitorGroup\|findUnique.*VisitorGroup" --type ts -A 10 -B 2Repository: mfts/papermark
Length of output: 40
🏁 Script executed:
cat pages/api/teams/\[teamId\]/visitor-groups/index.tsRepository: mfts/papermark
Length of output: 2728
🏁 Script executed:
rg "LinkVisitorGroup" prisma/schema/link.prisma -A 15Repository: mfts/papermark
Length of output: 1193
🏁 Script executed:
rg "findMany.*visitorGroup\|where.*visitorGroupId" pages/api/links/ -A 5Repository: mfts/papermark
Length of output: 40
Missing team-scoping validation for visitorGroupIds — cross-team data leak vulnerability.
The visitor group IDs from the request body are used directly without verifying they belong to the same teamId as the link being created or updated. A malicious user could associate visitor groups from another team with their link. While the FK constraint ensures the group exists, it doesn't enforce team ownership. This pattern appears in both POST (lines 240-251) and PATCH endpoints.
Add validation to confirm all provided visitorGroupIds belong to the requesting user's team before creating associations, consistent with how other entity IDs like agreementId and groupId are validated in this file.
🔒 Proposed fix
+ // Validate visitor groups belong to the same team
+ if (linkData.visitorGroupIds?.length > 0) {
+ const validGroups = await tx.visitorGroup.findMany({
+ where: {
+ id: { in: linkData.visitorGroupIds },
+ teamId: teamId,
+ },
+ select: { id: true },
+ });
+ if (validGroups.length !== linkData.visitorGroupIds.length) {
+ throw new Error("One or more visitor groups are invalid or do not belong to this team.");
+ }
+ }
// Connect visitor groups for allow-list
...(linkData.visitorGroupIds?.length > 0 && {
visitorGroups: {🤖 Prompt for AI Agents
In `@pages/api/links/index.ts` around lines 240 - 251, Validate that each ID in
linkData.visitorGroupIds belongs to the requesting user's team before passing
them into the Prisma create/update payload (the visitorGroups: { createMany: {
data: ... } } block). Specifically, in both the POST and PATCH handlers where
linkData.visitorGroupIds is used, fetch the visitor groups by the provided IDs
(e.g., via a DB call filtering on id IN visitorGroupIds and teamId ===
request.teamId) and ensure the returned count matches the input count; if not,
throw a 400/403 error. Only after this check, map the validated visitorGroupIds
into the visitorGroups createMany data; keep the same variable names (linkData,
visitorGroupIds, visitorGroups) so the rest of the handler remains unchanged.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (19)
.agents/skills/postgres/references/indexing.md-51-52 (1)
51-52:⚠️ Potential issue | 🟡 Minor
metadata_idxviolates the naming convention defined in the same file.Line 57 specifies
{table}_{column}_idx, but the example at line 52 usesmetadata_idx(missing the table prefixorder_). An AI agent applying this guide will produce an inconsistently named index.📝 Proposed fix
-CREATE INDEX metadata_idx ON order USING GIN (metadata); -- JSONB +CREATE INDEX order_metadata_idx ON order USING GIN (metadata); -- JSONB🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/indexing.md around lines 51 - 52, The index name metadata_idx does not follow the documented convention {table}_{column}_idx; update the CREATE INDEX statement to use the table-prefixed name (e.g., rename metadata_idx to order_metadata_idx) so the index on table order for column metadata matches the naming rule; locate the CREATE INDEX for metadata_idx and change its identifier to order_metadata_idx (ensure any references to metadata_idx elsewhere are updated accordingly)..agents/skills/postgres/references/index-optimization.md-45-45 (1)
45-45:⚠️ Potential issue | 🟡 MinorSection heading is incorrect — should be "Identify Duplicate Indexes".
The heading at line 45 repeats "Identify Unused Indexes," but the description ("Indexes with identical definitions … are duplicates") and the SQL query beneath it both target duplicate indexes, not unused ones. An AI agent processing this reference file will get a wrong signal about what the section covers.
📝 Proposed fix
-## Identify Unused Indexes +## Identify Duplicate Indexes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/index-optimization.md at line 45, The section heading currently reads "Identify Unused Indexes" but the content and SQL target duplicate indexes; update the heading text to "Identify Duplicate Indexes" (replace the string "Identify Unused Indexes" in this section) and verify any internal references or anchor links in the same file that point to this heading are updated to the new text so the heading and content match; leave the SQL and description unchanged..agents/skills/postgres/references/partitioning.md-47-56 (1)
47-56:⚠️ Potential issue | 🟡 Minor
orderis a reserved keyword — the SQL example will fail as written.
ORDERis reserved in both the SQL standard and PostgreSQL.CREATE TABLE order (...)without quoting the identifier produces a syntax error. Either quote it ("order") or, preferably, use a non-reserved name likeordersso the example works out-of-the-box for anyone who copies it.🐛 Proposed fix
-CREATE TABLE order ( +CREATE TABLE orders ( id BIGINT GENERATED ALWAYS AS IDENTITY, region TEXT NOT NULL, total NUMERIC(10,2), - PRIMARY KEY (id, region) -- Partition key MUST be part of PK -) PARTITION BY LIST (region); + PRIMARY KEY (id, region) -- Partition key MUST be part of PK +) PARTITION BY LIST (region); -CREATE TABLE order_us PARTITION OF order FOR VALUES IN ('us'); -CREATE TABLE order_eu PARTITION OF order FOR VALUES IN ('eu'); -CREATE TABLE order_default PARTITION OF order DEFAULT; -- catches unmatched values +CREATE TABLE orders_us PARTITION OF orders FOR VALUES IN ('us'); +CREATE TABLE orders_eu PARTITION OF orders FOR VALUES IN ('eu'); +CREATE TABLE orders_default PARTITION OF orders DEFAULT; -- catches unmatched values🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/partitioning.md around lines 47 - 56, The SQL example uses the reserved identifier order which will cause syntax errors; update the CREATE TABLE and partition declarations to use a non-reserved name (e.g., orders) or consistently quote the identifier ("order") across all references (PRIMARY KEY, PARTITION BY LIST, and partition tables order_us, order_eu, order_default) so the DDL runs as-is; ensure the partition table names and the parent table name match the chosen identifier..agents/skills/postgres/references/backup-recovery.md-24-30 (1)
24-30:⚠️ Potential issue | 🟡 MinorAdd a blank line before the table to satisfy MD058.
The markdownlint rule MD058 requires tables to be surrounded by blank lines. Line 23 (
## Tool Comparison) is immediately followed by the table header with no separating blank line.📝 Proposed fix
## Tool Comparison + | Tool | Use case | |------|----------|🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/backup-recovery.md around lines 24 - 30, Add a blank line between the "## Tool Comparison" heading and the table so the table is separated from the heading to satisfy markdownlint rule MD058; locate the heading text "## Tool Comparison" in .agents/skills/postgres/references/backup-recovery.md and insert a single empty line before the table starting with "| Tool | Use case |"..agents/skills/postgres/references/backup-recovery.md-15-15 (1)
15-15:⚠️ Potential issue | 🟡 MinorTwo inaccuracies in the physical backup section may cause operational failures.
Cross-architecture portability claim is misleading. PostgreSQL physical backups are not officially supported across different CPU architectures. The PostgreSQL docs require the same hardware architecture for a physical restore. Even though x86_64 and ARM64 (AArch64 in LE mode) are both little-endian, differences in OS page size, C struct alignment, and platform ABI can cause silent data corruption or a failed restore. Using
pg_dump/pg_restore(logical) is the supported path for cross-architecture migrations.
pg_backup_start()/pg_backup_stop()are PostgreSQL 15+ APIs. On PG ≤ 14 the functions are namedpg_start_backup()/pg_stop_backup(). An agent following this doc against an older cluster will generate invalid SQL.📝 Proposed fix
-Copies raw PGDATA; same major version and platform required; cross-architecture works if same endianness (e.g., x86_64 ↔ ARM64). Faster for large clusters; includes all databases. Flags: `-Ft -z -P` for compressed tar with progress. Manual alternative: `pg_backup_start()` → copy PGDATA → `pg_backup_stop()` (complex; must write returned `backup_label`). +Copies raw PGDATA; **same major version and same hardware architecture required** — cross-architecture restores are not supported. Faster for large clusters; includes all databases. Flags: `-Ft -z -P` for compressed tar with progress. Manual alternative (PG 15+): `pg_backup_start()` → copy PGDATA → `pg_backup_stop()` (PG ≤14: `pg_start_backup()` / `pg_stop_backup()`); must write returned `backup_label`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/backup-recovery.md at line 15, Update the physical backup note to remove the claim that cross-architecture restores are supported; state that PostgreSQL requires the same CPU architecture for physical restores (differences in page size, struct alignment, ABI can break restores) and recommend using logical backup (pg_dump/pg_restore) for cross-architecture migrations; also correct the API names: mention that pre-15 clusters use pg_start_backup() / pg_stop_backup() while PostgreSQL 15+ exposes pg_backup_start() / pg_backup_stop(), and advise the doc user to choose the correct function based on their server version and to ensure the returned backup_label is written when using the start/stop backup sequence..agents/skills/frontend-design/SKILL.md-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorRemove hardcoded "Claude" reference from line 42.
Line 42 explicitly names "Claude" in a model-agnostic skill file, which creates inconsistency. Other SKILL.md files in the repository avoid model-specific language. Rewrite this to be model-neutral (e.g., "Remember: AI can produce extraordinary creative work...").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/frontend-design/SKILL.md at line 4, Replace the hardcoded "Claude" reference on line 42 of SKILL.md with a model-neutral phrase; locate the string "Claude" in .agents/skills/frontend-design/SKILL.md and change it to a neutral wording such as "AI" or "an AI model" (for example: "Remember: AI can produce extraordinary creative work...") so the skill file remains model-agnostic and consistent with other SKILL.md files..agents/skills/postgres/references/ps-extensions.md-15-15 (1)
15-15:⚠️ Potential issue | 🟡 MinorWording slightly overstates restart frequency.
The phrase "This often requires a database restart" implies most extensions trigger a restart. PlanetScale's own documentation uses "may require a database restart" and indicates only the specific subset of extensions marked with ✅ in the "Restart required" column need it. The current wording could cause an AI agent to routinely over-communicate restart warnings to users.
✏️ Suggested wording correction
-Some extensions must first be **enabled in the PlanetScale Dashboard** (Clusters > Extensions) before they can be created in SQL. This often requires a database restart. +Some extensions must first be **enabled in the PlanetScale Dashboard** (Clusters > Extensions) before they can be created in SQL. Extensions marked as "Restart required" in the PlanetScale docs will require a database restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/ps-extensions.md at line 15, Change the sentence "This often requires a database restart" to a more accurate phrase: use "This may require a database restart" and add that only extensions marked with a "Restart required" indicator in the PlanetScale dashboard (Clusters > Extensions) require a restart; replace the original sentence in the ps-extensions.md content to avoid implying restarts are frequent for most extensions..agents/skills/postgres/references/replication.md-15-15 (1)
15-15:⚠️ Potential issue | 🟡 Minor
idle_replication_slot_timeoutwas introduced in PG 18, not PG 17.
idle_replication_slot_timeoutwas added in PostgreSQL 18. The PG 18 release notes confirm it as a new feature: "Allow inactive replication slots to be automatically invalidated using server variableidle_replication_slot_timeout." The(PG 17+)annotation should be corrected to(PG 18+)to avoid misleading guidance on PG 17 clusters.📝 Proposed fix
-Use `max_slot_wal_keep_size` to cap retained WAL per slot. Use `idle_replication_slot_timeout` (PG 17+) to auto-invalidate idle slots. +Use `max_slot_wal_keep_size` to cap retained WAL per slot. Use `idle_replication_slot_timeout` (PG 18+) to auto-invalidate idle slots.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/replication.md at line 15, The documentation incorrectly states that idle_replication_slot_timeout is available in PG 17; update the text to say the setting was introduced in PostgreSQL 18 by replacing the "(PG 17+)" annotation with "(PG 18+)" wherever idle_replication_slot_timeout is mentioned so readers aren't misled about availability; keep the surrounding guidance about slot behavior and WAL retention unchanged..agents/skills/postgres/references/monitoring.md-16-16 (1)
16-16:⚠️ Potential issue | 🟡 Minor
buffers_backenddescription is slightly imprecise.The parenthetical "high = backends bypassing bgwriter" understates the concern. Those
buffers_backendandbuffers_backend_fsynccolumns were removed frompg_stat_bgwriterin PG 17, and the information moved topg_stat_io. More precisely, a highbuffers_backendmeans backend processes are directly flushing dirty pages from shared buffers under memory pressure, bypassing both the bgwriter and the checkpointer — the real signal is a too-smallshared_buffers. The current wording could mislead a reader into tuning only bgwriter settings when the root fix is more shared memory.✏️ Suggested wording
-- **pg_stat_bgwriter**: `buffers_clean`, `maxwritten_clean`, `buffers_alloc`. Pre-PG 17 also had `buffers_checkpoint`, `buffers_backend` (high = backends bypassing bgwriter). PG 17+ moved checkpoint stats to `pg_stat_checkpointer`. +- **pg_stat_bgwriter**: `buffers_clean`, `maxwritten_clean`, `buffers_alloc`. Pre-PG 17 also had `buffers_checkpoint`, `buffers_backend` (high = backends writing dirty pages directly under shared buffer pressure, bypassing bgwriter/checkpointer; fix: increase `shared_buffers`). PG 17+ moved checkpoint stats to `pg_stat_checkpointer`; `buffers_backend` moved to `pg_stat_io`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/monitoring.md at line 16, Update the pg_stat_bgwriter row to clarify that the removed columns `buffers_backend` and `buffers_backend_fsync` (now exposed in `pg_stat_io`) indicate backend processes directly flushing dirty pages under memory pressure, which means shared_buffers is likely too small—not merely that the bgwriter is being bypassed; mention that PG17 moved checkpoint stats to `pg_stat_checkpointer` and recommend tuning `shared_buffers` (and investigating backend-driven flushes) rather than only adjusting bgwriter settings. Reference `pg_stat_bgwriter`, `buffers_backend`, `buffers_backend_fsync`, `pg_stat_io`, `pg_stat_checkpointer`, `shared_buffers`, bgwriter and checkpointer in the updated wording..agents/skills/postgres/references/schema-design.md-11-11 (1)
11-11:⚠️ Potential issue | 🟡 MinorUUIDv7 leaks record creation time — add a security note.
Using UUIDv7 is generally discouraged for security when the primary key is exposed to end users in external-facing APIs, because UUIDv7 incorporates a 48-bit Unix timestamp, meaning the identifier itself leaks the record's creation time. This is relevant for applications like Papermark where document/link IDs surface in URLs. The guide should call this out so consumers can make an informed choice.
📝 Suggested note to add
Prefer `BIGINT GENERATED ALWAYS AS IDENTITY`. Avoid random UUIDs (UUIDv4) as primary keys; use `uuidv7()` when you need UUIDs (requires PostgreSQL ≥ 18). + +> **Security note:** UUIDv7 embeds a millisecond-precision Unix timestamp. Avoid exposing UUIDv7 primary keys directly in public-facing URLs or APIs where leaking record creation time is unacceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/schema-design.md at line 11, Add a short security note to the schema guidance calling out that UUIDv7 embeds a 48-bit Unix timestamp and therefore leaks record creation time (relevant when IDs are exposed in external-facing APIs such as Papermark document/link URLs); advise consumers to avoid UUIDv7 for publicly visible primary keys and prefer BIGINT GENERATED ALWAYS AS IDENTITY for sequential IDs or use non-time-based UUIDs only when exposure is acceptable, and mention that UUIDv4 should also be avoided as the default primary key strategy in favor of BIGINT identity when possible..agents/skills/postgres/references/wal-operations.md-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor
synchronous_commitcaveat omitted — the fsync-on-commit guarantee is conditional."On COMMIT, PostgreSQL fsyncs WAL to disk and returns SUCCESS" is only true with the default
synchronous_commit = on. Withsynchronous_commit = offorlocal, WAL is not fsynced synchronously on commit, and the durability guarantee described here does not hold. Since this document is used as an operational reference, omitting this caveat could mislead an operator who needs to reason about durability settings.📝 Suggested addition
-On COMMIT, PostgreSQL fsyncs WAL to disk and returns SUCCESS — data files are updated lazily. +On COMMIT (with default `synchronous_commit=on`), PostgreSQL fsyncs WAL to disk and returns SUCCESS — data files are updated lazily. With `synchronous_commit=off/local`, the fsync is skipped for performance; durability is weakened accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/wal-operations.md at line 11, Update the WAL description to clarify that the fsync-on-commit behavior is conditional on the synchronous_commit setting: mention that "On COMMIT, PostgreSQL fsyncs WAL to disk and returns SUCCESS" only applies when synchronous_commit = on (the default); when synchronous_commit = off or local, WAL is not fsynced synchronously on commit and the durability guarantee does not hold; reference the synchronous_commit parameter and advise operators not to change it in production without understanding the durability trade-offs (and keep the existing warning about never disabling fsync)..agents/skills/postgres/references/wal-operations.md-38-38 (1)
38-38:⚠️ Potential issue | 🟡 Minor
pg_stat_ioversion attribution is inaccurate — it was introduced in PG16, not PG17.The surrounding context groups the
pg_stat_ionote under "PG17+", implying it arrived alongsidepg_stat_checkpointer. In reality,pg_stat_iowas introduced in PostgreSQL 16. An agent or operator relying on this document could incorrectly assume queryingpg_stat_iorequires PG17, or conversely apply the PG17 assumption to a PG16 instance.📝 Suggested clarification
-Backend-direct writes (formerly `buffers_backend` in `pg_stat_bgwriter`) are now tracked in `pg_stat_io`: `SELECT writes FROM pg_stat_io WHERE backend_type = 'client backend' AND object = 'relation';` +Backend-direct writes (formerly `buffers_backend` in `pg_stat_bgwriter`, removed in PG17) are tracked in `pg_stat_io` (available since PG16): `SELECT writes FROM pg_stat_io WHERE backend_type = 'client backend' AND object = 'relation';`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/wal-operations.md at line 38, Update the version note to attribute pg_stat_io to PostgreSQL 16 (not 17): change the grouping or text that places the pg_stat_io example (the SELECT writes FROM pg_stat_io WHERE backend_type = 'client backend' AND object = 'relation') under "PG17+" and instead mention that pg_stat_io was introduced in PG16 while pg_stat_checkpointer is PG17; ensure references to pg_stat_bgwriter and the example query remain but clarify the correct version for pg_stat_io to avoid confusion..agents/skills/postgres/references/memory-management-ops.md-14-14 (1)
14-14:⚠️ Potential issue | 🟡 Minor
hash_mem_multiplierdefault of2.0is only accurate for PostgreSQL 15+.The parameter was introduced in PostgreSQL 13 with a default of
1.0; it remained1.0in PostgreSQL 14 and was raised to2.0in PostgreSQL 15. The document doesn't specify a minimum version, which makes the stated default potentially misleading for teams running PostgreSQL 13 or 14. Consider adding a version qualifier or pinning the document to a minimum supported version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/memory-management-ops.md at line 14, The doc line about hash_mem_multiplier is inaccurate for older PostgreSQL versions; update the text around hash_mem_multiplier to either (a) state version-specific defaults (PostgreSQL 13–14 default 1.0; PostgreSQL 15+ default 2.0) or (b) add a clear minimum-supported-version note that pins the doc to PostgreSQL 15+, so readers on 13/14 aren't misled; reference and edit the `hash_mem_multiplier` entry to include the chosen version qualifier and the correct default values..agents/skills/postgres/references/memory-management-ops.md-1-39 (1)
1-39:⚠️ Potential issue | 🟡 MinorSpecify minimum PostgreSQL version for
hash_mem_multiplierdefault value.Line 14 states
hash_mem_multiplierdefault is 2.0, but this is only accurate for PostgreSQL 15+. PostgreSQL 13–14 have a default of 1.0. Either add a minimum version requirement to the document header or update the statement to include all version defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/memory-management-ops.md around lines 1 - 39, The doc incorrectly states hash_mem_multiplier defaults to 2.0 without version context; update the "Hash multiplier" bullet to mention version-specific defaults (hash_mem_multiplier = 1.0 for PG 13–14, 2.0 starting in PG15) or add a minimum PostgreSQL version in the header; reference the symbol hash_mem_multiplier and adjust the sentence(s) in the "Hash multiplier" section and/or the document header to clearly state the applicable PostgreSQL versions..agents/skills/postgres/references/query-patterns.md-34-38 (2)
34-38:⚠️ Potential issue | 🟡 MinorSARGable "Good" example contradicts the "SELECT specific columns" guideline
The "Good" variant fixes only the predicate but still uses
SELECT *. An AI agent reading this example in isolation learns thatSELECT *is acceptable in an optimized query, which directly undercuts the first guideline.✏️ Proposed fix
-- Bad: Full table scan -SELECT * FROM user WHERE date_trunc('day', created_at) = '2023-01-01'; +SELECT id, name, email FROM users WHERE date_trunc('day', created_at) = '2023-01-01'; -- Good: Index scan -SELECT * FROM user WHERE created_at >= '2023-01-01' AND created_at < '2023-01-02'; +SELECT id, name, email FROM users WHERE created_at >= '2023-01-01' AND created_at < '2023-01-02';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/query-patterns.md around lines 34 - 38, The "Good: Index scan" example still uses "SELECT *", contradicting the guideline to select specific columns; update the good example SQL (the line starting with "SELECT * FROM user WHERE created_at >= '2023-01-01'...") to list explicit columns (e.g., "SELECT id, created_at, <other_needed_columns> FROM user WHERE created_at >= '2023-01-01' AND created_at < '2023-01-02'") and add a brief note in the same example clarifying that choosing only required columns preserves the sargability benefit.
34-38:⚠️ Potential issue | 🟡 MinorSARGable "Good" example still uses
SELECT *, contradicting the first guidelineThe "Good" variant only fixes the predicate (range instead of
date_trunc), but retainsSELECT *. An AI agent reading this example in isolation will learn thatSELECT *is acceptable in an optimized query, undercutting the explicit rule stated in the first section.✏️ Proposed fix
-- Good: Index scan -SELECT * FROM user WHERE created_at >= '2023-01-01' AND created_at < '2023-01-02'; +SELECT id, name, email FROM users WHERE created_at >= '2023-01-01' AND created_at < '2023-01-02';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/query-patterns.md around lines 34 - 38, The "Good" SARGable example still shows SELECT * which contradicts the guideline against SELECT *; update the example query that currently uses "SELECT * FROM user WHERE created_at >= '2023-01-01' AND created_at < '2023-01-02';" to use an explicit column list (e.g., SELECT id, name, created_at ...) or a placeholder like SELECT column1, column2, created_at ... so the example demonstrates both a SARGable predicate (no date_trunc) and avoiding SELECT *; ensure the revised example keeps the improved WHERE clause and replaces SELECT * with explicit columns..agents/skills/postgres/references/query-patterns.md-12-25 (2)
12-25:⚠️ Potential issue | 🟡 Minor
userandorderare reserved PostgreSQL keywords — every example in these sections will produce a syntax error as writtenIn PostgreSQL, tokens labeled "reserved" are not allowed as column or table names. Both
userandorderfall in this category. Attempting to use an unquotedUseras a table name in PostgreSQL raisesSQLSTATE[42601]: Syntax error at or near "User", and an unquoted table namedorderlikewise requires quoting as"order"or renaming.Because this file is consumed by an AI coding agent as a canonical reference, the agent will reproduce these broken identifiers directly in generated SQL. The preferred fix is to rename to the conventional non-reserved plurals (
users,orders) so no quoting is needed anywhere.🛠️ Proposed fix — replace reserved identifiers throughout the file
-SELECT * FROM user WHERE status = 'active'; +SELECT * FROM users WHERE status = 'active'; -- Good: -SELECT id, name, email FROM user WHERE status = 'active'; +SELECT id, name, email FROM users WHERE status = 'active';-SELECT id, (SELECT COUNT(*) FROM order WHERE order.user_id = user.id) FROM user; +SELECT id, (SELECT COUNT(*) FROM orders WHERE orders.user_id = users.id) FROM users; -- Good -SELECT u.id, COUNT(o.id) FROM user u LEFT JOIN order o ON o.user_id = u.id GROUP BY u.id; +SELECT u.id, COUNT(o.id) FROM users u LEFT JOIN orders o ON o.user_id = u.id GROUP BY u.id;-SELECT * FROM user WHERE date_trunc('day', created_at) = '2023-01-01'; +SELECT id, name FROM users WHERE date_trunc('day', created_at) = '2023-01-01'; -- Good: Index scan -SELECT * FROM user WHERE created_at >= '2023-01-01' AND created_at < '2023-01-02'; +SELECT id, name FROM users WHERE created_at >= '2023-01-01' AND created_at < '2023-01-02';- cursor.execute("SELECT name FROM user WHERE id = %s", (uid,)) + cursor.execute("SELECT name FROM users WHERE id = %s", (uid,)) # Good (Postgres specific) -cursor.execute("SELECT id, name FROM user WHERE id = ANY(%s)", (list(user_ids),)) +cursor.execute("SELECT id, name FROM users WHERE id = ANY(%s)", (list(user_ids),)) # Good (Standard SQL) -# cursor.execute("SELECT id, name FROM user WHERE id IN %s", (tuple(user_ids),)) +# cursor.execute("SELECT id, name FROM users WHERE id IN %s", (tuple(user_ids),))-for user in User.query.all(): - print(user.posts) -users = User.query.options(joinedload(User.posts)).all() +for account in Account.query.all(): + print(account.posts) +accounts = Account.query.options(joinedload(Account.posts)).all()-SELECT id, name FROM user u -WHERE EXISTS (SELECT 1 FROM order o WHERE o.user_id = u.id AND o.total > 100); +SELECT id, name FROM users u +WHERE EXISTS (SELECT 1 FROM orders o WHERE o.user_id = u.id AND o.total > 100);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/query-patterns.md around lines 12 - 25, Replace every occurrence of the reserved identifiers user and order in the SQL examples with non-reserved plural names users and orders (e.g., change SELECT * FROM user to SELECT * FROM users and the correlated subquery referencing order.user_id to orders.user_id), updating all examples in the document (SELECTs, JOINs, aliases like u and o, GROUP BY examples) so they remain semantically identical but use users and orders; do not use quoted identifiers.
12-25:⚠️ Potential issue | 🟡 Minor
userandorderare PostgreSQL reserved keywords that require quoting or renamingBoth
USERandORDERare reserved keywords in PostgreSQL and cannot be used as unquoted table identifiers—any example copied verbatim will fail with a syntax error. Since this document serves as a reference for an AI coding agent, propagating broken SQL patterns here risks generating invalid code downstream.Either quote these identifiers every time (
"user","order") or rename tables to non-reserved names (users,orders). Renaming is the cleaner approach and avoids quote escaping overhead.Proposed fix — replace with non-reserved identifiers
-SELECT * FROM user WHERE status = 'active'; +SELECT * FROM users WHERE status = 'active';-SELECT id, (SELECT COUNT(*) FROM order WHERE order.user_id = user.id) FROM user; +SELECT id, (SELECT COUNT(*) FROM orders WHERE orders.user_id = users.id) FROM users; -- Good -SELECT u.id, COUNT(o.id) FROM user u LEFT JOIN order o ON o.user_id = u.id GROUP BY u.id; +SELECT u.id, COUNT(o.id) FROM users u LEFT JOIN orders o ON o.user_id = u.id GROUP BY u.id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/postgres/references/query-patterns.md around lines 12 - 25, The SQL examples use reserved identifiers "user" and "order" which will cause syntax errors; update all occurrences of the table names in this file (e.g., the examples showing SELECT * FROM user, SELECT id, (SELECT COUNT(*) FROM order ...), JOIN examples using user and order) to non-reserved identifiers such as users and orders (or consistently quote them) so the examples run as-is; ensure both the single-row subquery example and the JOIN/GROUP BY example use the new names (e.g., users and orders) throughout..agents/skills/postgres/references/ps-connections.md-19-21 (1)
19-21:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
The connection string block has no language tag, which triggers MD040. Using
textorplaintextresolves it without implying shell execution.📝 Proposed fix
-``` +```text postgresql://<user>:<password>@<host>.horizon.psdb.cloud:5432/<database>?sslmode=verify-full&sslrootcert=system&sslnegotiation=direct</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/postgres/references/ps-connections.md around lines 19 - 21,
Add a language identifier to the fenced code block containing the Postgres
connection string so the linter rule MD040 is satisfied; update the block that
currently contains
"postgresql://:@.horizon.psdb.cloud:5432/?sslmode=verify-full&sslrootcert=system&sslnegotiation=direct"
by changing the opening fence to include a plain language tag such as "text"
(e.g., ```text) to avoid implying shell execution.</details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
|
||
| ## Autovacuum Tuning | ||
|
|
||
| Triggers when dead tuples > `Min(autovacuum_vacuum_max_threshold, autovacuum_vacuum_threshold + autovacuum_vacuum_scale_factor * reltuples)`. `autovacuum_vacuum_max_threshold` defaults to 100M (PG 18+), capping the threshold for very large tables. Also triggers on inserts exceeding `autovacuum_vacuum_insert_threshold + autovacuum_vacuum_insert_scale_factor * reltuples * pct_not_frozen` (ensures insert-only tables get frozen; PG 13+). For large/hot tables, set per-table overrides: |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PostgreSQL 18 autovacuum_vacuum_max_threshold parameter documentation
💡 Result:
In PostgreSQL 18, autovacuum_vacuum_max_threshold is documented in “19.10. Vacuuming → 19.10.1 Automatic Vacuuming” as:
- Purpose: Sets the maximum number of updated/deleted tuples that can trigger an autovacuum
VACUUMfor a table—i.e., it caps the trigger value computed fromautovacuum_vacuum_threshold + autovacuum_vacuum_scale_factor * reltuples. - Default:
100000000tuples. - Disable cap: Set to
-1. - Scope: Set in
postgresql.conf/ server command line, and can be overridden per-table via table storage parameters. [1]
Per-table storage-parameter support is also listed in the CREATE TABLE / storage parameters documentation (usable via ALTER TABLE ... SET (autovacuum_vacuum_max_threshold = ...)). [2]
Sources: PostgreSQL 18 docs (Vacuuming / Automatic Vacuuming) [1], PostgreSQL docs (table storage parameters) [2].
[1] (postgresql.org)
[2] (postgresql.org)
Citations:
- 1: https://www.postgresql.org/docs/18/runtime-config-vacuum.html
- 2: https://www.postgresql.org/docs/current/static/sql-createtable.html?utm_source=openai
🌐 Web query:
PostgreSQL autovacuum_vacuum_insert_threshold pct_not_frozen parameter
💡 Result:
autovacuum_vacuum_insert_threshold / “insert-triggered” autovacuum
Since PostgreSQL 13, autovacuum can be triggered by inserts (useful for insert-only / append-only tables). Autovacuum runs when inserts since last vacuum exceed:
vacuum_insert_threshold =
autovacuum_vacuum_insert_threshold
+ autovacuum_vacuum_insert_scale_factor * reltuples
reltuples is the planner’s row estimate for the table. (postgresql.org)
Notes:
- Default values are commonly
autovacuum_vacuum_insert_threshold = 1000andautovacuum_vacuum_insert_scale_factor = 0.2(PG13+). (cybertec-postgresql.com) - You can disable insert-triggered vacuums per table by setting
autovacuum_vacuum_insert_threshold = -1as a storage parameter. (cybertec-postgresql.com)
What is pct_not_frozen?
pct_not_frozen is not a built-in PostgreSQL GUC/parameter. It’s typically a derived metric used by monitoring scripts/tools to express “how close are we to an anti-wraparound (aggressive) vacuum”, based on XID age.
A common way to compute something equivalent is using age(relfrozenxid) (or age(datfrozenxid) at the DB level) relative to autovacuum_freeze_max_age. The PostgreSQL docs explain relfrozenxid/datfrozenxid and how they’re used to track freezing progress. (postgresql.org)
Example (table-level “percent of freeze horizon consumed” — often what people mean by “% not frozen”):
SELECT
c.oid::regclass AS table_name,
age(c.relfrozenxid) AS xid_age,
round(100.0 * age(c.relfrozenxid)
/ current_setting('autovacuum_freeze_max_age')::numeric, 2) AS pct_of_freeze_horizon_consumed
FROM pg_class c
WHERE c.relkind IN ('r','m');(Variants of this “percent consumed” approach are also shown in vendor guidance for wraparound monitoring.) (cloud.google.com)
Citations:
- 1: https://www.postgresql.org/docs/17/routine-vacuuming.html?utm_source=openai
- 2: https://www.cybertec-postgresql.com/en/postgresql-autovacuum-insert-only-tables/?utm_source=openai
- 3: https://www.cybertec-postgresql.com/en/postgresql-autovacuum-insert-only-tables/?utm_source=openai
- 4: https://www.postgresql.org/docs/13/routine-vacuuming.html?utm_source=openai
- 5: https://cloud.google.com/blog/products/databases/how-to-accelerate-transaction-id-freezing-in-cloud-sql-for-postgresql?utm_source=openai
Remove pct_not_frozen from the insert-trigger formula—it is not a PostgreSQL parameter.
The insert-based autovacuum trigger formula in this line is incorrect. The actual formula per PostgreSQL 13+ documentation is:
autovacuum_vacuum_insert_threshold + autovacuum_vacuum_insert_scale_factor * reltuples
The line currently includes * pct_not_frozen at the end, which is neither a real PostgreSQL parameter nor part of the actual trigger condition. Remove it to avoid misleading agents that consume this skill file. (autovacuum_vacuum_max_threshold and its PG 18+ default of 100M are correct and do not need changes.)
🧰 Tools
🪛 LanguageTool
[style] ~19-~19: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...00M (PG 18+), capping the threshold for very large tables. Also triggers on inserts exceed...
(EN_WEAK_ADJECTIVE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/postgres/references/mvcc-vacuum.md at line 19, The
insert-trigger formula wrongly includes a non-existent parameter pct_not_frozen;
update the sentence that describes the autovacuum insert trigger to use the
correct PostgreSQL 13+ formula: "autovacuum_vacuum_insert_threshold +
autovacuum_vacuum_insert_scale_factor * reltuples" by removing "*
pct_not_frozen" and leave the surrounding text about
autovacuum_vacuum_max_threshold and its default unchanged; ensure you reference
autovacuum_vacuum_insert_threshold, autovacuum_vacuum_insert_scale_factor, and
reltuples in the corrected line.
|
|
||
| ## Primary Keys | ||
|
|
||
| Prefer `BIGINT GENERATED ALWAYS AS IDENTITY`. Avoid random UUIDs (UUIDv4) as primary keys; use `uuidv7()` when you need UUIDs. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When was uuidv7() function introduced in PostgreSQL? Which versions support it?
💡 Result:
uuidv7() was added as a built-in UUID generation function during the PostgreSQL 18 development cycle (merged on 2024-12-11). [1]
It is supported in PostgreSQL 18 and later (18+); it is not available as a built-in function in PostgreSQL 17 or earlier. [2][3]
Supported versions: PostgreSQL 18+. [2]
Sources: PostgreSQL 18 UUID functions documentation (shows uuidv7) [2]; pgsql-committers commit message adding uuidv7() dated 2024-12-11 [1]; background note that PostgreSQL 17 didn’t have native UUIDv7 generation [3].
🏁 Script executed:
# Read the schema-design.md file to verify its content
cat -n .agents/skills/postgres/references/schema-design.mdRepository: mfts/papermark
Length of output: 2469
🏁 Script executed:
# Verify PostgreSQL reserved keywords - user and order
# Using postgres documentation or language specs
python3 << 'EOF'
# PostgreSQL reserved keywords - checking documentation
# These are well-known reserved words in PostgreSQL
reserved_keywords = {
'user': True, # reserved in PostgreSQL
'order': True, # reserved in PostgreSQL
}
print("PostgreSQL Reserved Keywords (relevant to this file):")
print("'user' is a reserved keyword: TRUE")
print("'order' is a reserved keyword: TRUE")
print("\nUsage as table names without escaping will cause syntax errors.")
EOFRepository: mfts/papermark
Length of output: 247
Fix PostgreSQL version requirement for uuidv7() and remove reserved keywords from code examples.
The documentation contains two critical issues with the schema examples:
-
uuidv7()requires PostgreSQL 18+: This function was added in PostgreSQL 18 (December 2024) and is not available in PostgreSQL 13–17. An AI agent or developer following this guide on a pre-18 cluster will encounter "function uuidv7() does not exist" errors. The guide should specify the minimum version or provide a fallback (pg_uuidv7extension). -
Reserved keywords used as table names: The code examples use
userandorderas table names (lines 14, 34, 47), which are PostgreSQL reserved keywords. These DDL statements will fail with syntax errors. The guide's own naming convention section (line 56) recommendsuser_accountandorder_item—examples should follow that convention.
📝 Suggested fixes
-CREATE TABLE user (
+CREATE TABLE user_account (
id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
email TEXT NOT NULL UNIQUE
);-Prefer `BIGINT GENERATED ALWAYS AS IDENTITY`. Avoid random UUIDs (UUIDv4) as primary keys; use `uuidv7()` when you need UUIDs.
+Prefer `BIGINT GENERATED ALWAYS AS IDENTITY`. Avoid random UUIDs (UUIDv4) as primary keys; use `uuidv7()` when you need UUIDs (requires **PostgreSQL ≥ 18**; on older versions install the [`pg_uuidv7`](https://github.com/fboulnois/pg_uuidv7) extension).-CREATE TABLE order (
+CREATE TABLE order_item (📝 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.
| Prefer `BIGINT GENERATED ALWAYS AS IDENTITY`. Avoid random UUIDs (UUIDv4) as primary keys; use `uuidv7()` when you need UUIDs. | |
| Prefer `BIGINT GENERATED ALWAYS AS IDENTITY`. Avoid random UUIDs (UUIDv4) as primary keys; use `uuidv7()` when you need UUIDs (requires **PostgreSQL ≥ 18**; on older versions install the [`pg_uuidv7`](https://github.com/fboulnois/pg_uuidv7) extension). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/postgres/references/schema-design.md at line 11, Update the
schema docs to state that uuidv7() requires PostgreSQL 18+ (or instruct users to
install the pg_uuidv7 extension when on older servers) and change example DDL to
avoid reserved keywords: replace tables named user and order with user_account
and order_item (and update any references to those names in CREATE TABLE, FK
definitions, and examples). Also show the alternative pattern (BIGINT GENERATED
ALWAYS AS IDENTITY) as the preferred PK and note uuidv7() usage only when
PostgreSQL 18+ or with the pg_uuidv7 extension.
| ```sql | ||
| CREATE TABLE user ( | ||
| id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY, | ||
| email TEXT NOT NULL UNIQUE | ||
| ); | ||
| ``` |
There was a problem hiding this comment.
user and order are PostgreSQL reserved keywords — the SQL examples will fail as written.
user (line 14) and order (lines 34, 47) are reserved words in PostgreSQL. Executing CREATE TABLE user (...) or CREATE TABLE order (...) without double-quoting raises a syntax error. Because this guide is consumed by AI agents to generate real DDL, broken example SQL is a significant hazard.
Additionally, these names contradict the guide's own Naming Conventions section (line 56) which recommends compound snake_case identifiers like user_account and order_item.
Fix all three examples to use non-reserved, compound names that comply with the documented convention:
🐛 Proposed fix for all three SQL blocks
## Primary Keys
```sql
-CREATE TABLE user (
+CREATE TABLE user_account (
id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
email TEXT NOT NULL UNIQUE
);Data Types
-CREATE TABLE order (
+CREATE TABLE customer_order (
id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
status TEXT NOT NULL CHECK (status IN ('pending', 'shipped', 'delivered'))
);Foreign Keys
-CREATE TABLE order (
+CREATE TABLE customer_order (
id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
- customer_id BIGINT NOT NULL REFERENCES customer(id) ON DELETE CASCADE
+ customer_id BIGINT NOT NULL REFERENCES user_account(id) ON DELETE CASCADE
);
-CREATE INDEX order_customer_id_idx ON order (customer_id);
+CREATE INDEX customer_order_customer_id_idx ON customer_order (customer_id);Also applies to: 33-38, 46-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/postgres/references/schema-design.md around lines 13 - 18,
The SQL examples use reserved PostgreSQL identifiers "user" and "order" and
violate the guide's Naming Conventions; update all three SQL blocks to use
non-reserved, compound snake_case names (e.g., rename table "user" →
"user_account", rename "order" → "customer_order"), update any foreign key
references (e.g., REFERENCES user(id) → REFERENCES user_account(id)), and rename
related indexes (e.g., order_customer_id_idx → customer_order_customer_id_idx)
so the examples compile and follow the documented convention.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
components/visitors/visitor-groups-section.tsx (3)
21-22: Combine the twoswrimports into one.
mutateanduseSWRare imported via two separate statements from the same package.♻️ Proposed fix
-import { mutate } from "swr"; -import useSWR from "swr"; +import useSWR, { mutate } from "swr";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/visitors/visitor-groups-section.tsx` around lines 21 - 22, The file currently has two separate imports from the same package; combine them into a single import statement so both mutate and useSWR are imported together from "swr" (replace the two lines importing mutate and useSWR with one: import { mutate, useSWR } from "swr";), ensuring any other references to mutate or useSWR remain unchanged.
399-410: Use the already-extractedteamIdconstant instead of re-readingteamInfo?.currentTeam?.id.
teamIdis declared at line 376 buthandleDeletereadsteamInfo?.currentTeam?.idtwice. IfteamIdis undefined, thefetchwill produce the URL/api/teams/undefined/visitor-groups/…. Consider guarding early:♻️ Proposed fix
const handleDelete = async (group: VisitorGroupWithCount) => { + if (!teamId) return; if (!confirm(`Are you sure you want to delete "${group.name}"? ...`)) return; try { const response = await fetch( - `/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups/${group.id}`, + `/api/teams/${teamId}/visitor-groups/${group.id}`, { method: "DELETE" }, ); ... - mutate(`/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`); + mutate(`/api/teams/${teamId}/visitor-groups`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/visitors/visitor-groups-section.tsx` around lines 399 - 410, In handleDelete, stop re-reading teamInfo?.currentTeam?.id and use the already-extracted teamId constant for both the fetch URL and the mutate key; add an early guard that returns (and optionally shows an error) if teamId is falsy before calling fetch so you never construct /api/teams/undefined/..., and update the mutate call to mutate(`/api/teams/${teamId}/visitor-groups`) to match.
388-414: Replacewindow.confirm()with a proper confirmation dialog.
window.confirm()is a synchronous, blocking browser dialog that cannot be styled, is suppressed in some browser contexts (e.g., cross-origin iframes), and generally delivers poor UX. A shadcnAlertDialog(already available in the project's UI library) or similar component would be more consistent with the rest of the design system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/visitors/visitor-groups-section.tsx` around lines 388 - 414, The handler handleDelete currently uses a blocking window.confirm; replace that synchronous prompt with the project's shadcn AlertDialog component by moving the confirmation UI out of handleDelete into a non-blocking dialog flow: create dialog state (open/close) for selecting a VisitorGroupWithCount, render AlertDialog when a delete button is clicked (pass the group data to the dialog), and only call the existing delete logic (the fetch + toast + mutate flow inside handleDelete) after the user confirms in the AlertDialog; ensure the dialog provides cancel/confirm actions, handles loading/disabled state while the async delete runs, and closes on success or failure so the rest of handleDelete (including mutate(`/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`) and toast.* calls) can 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 `@components/visitors/visitor-groups-section.tsx`:
- Around line 282-299: The shared `content` currently always includes
`ExternalLinkIcon`, which makes the non-navigable fallback `<div>` (rendered
when `documentId`/`dataroomId` is null) appear clickable; change the render so
the icon is only shown for actual links: either split `content` into two
variants (e.g., `contentWithIcon` and `contentNoIcon`) or keep `content` but
conditionally render `ExternalLinkIcon` only inside the `Link` branch that uses
`groupLink.id` and `documentId`; apply the same conditional/icon-splitting fix
to the other similar block around the `317-336` range.
- Around line 118-128: The two interactive divs using role="button" (the main
header that calls toggle and the chevron button at the bottom) are missing
aria-expanded; update both elements to include aria-expanded={expanded} so
screen readers can detect the open/closed state, and add distinct aria-labels
(e.g., aria-label="Visitor group header" and aria-label="Toggle details") if
needed to differentiate them; ensure you update the header onClick/onKeyDown
block that references toggle and the chevron role="button" block (the other
occurrence around the chevron) to match.
- Around line 103-104: The header is shown during loading because hasLinkedItems
is true whenever loadingDetails is true; change the condition so the "Links"
section is gated by the group's known linkCount when available — e.g., compute
hasLinkedItems using group.linkCount > 0 OR (loadingDetails &&
(documentLinks.length > 0 || dataroomLinks.length > 0)) so you don't flash
"Links (0)" while details are still loading; update the code that uses
hasLinkedItems in visitor-groups-section.tsx (references: hasLinkedItems,
loadingDetails, documentLinks, dataroomLinks, group.linkCount).
---
Nitpick comments:
In `@components/visitors/visitor-groups-section.tsx`:
- Around line 21-22: The file currently has two separate imports from the same
package; combine them into a single import statement so both mutate and useSWR
are imported together from "swr" (replace the two lines importing mutate and
useSWR with one: import { mutate, useSWR } from "swr";), ensuring any other
references to mutate or useSWR remain unchanged.
- Around line 399-410: In handleDelete, stop re-reading
teamInfo?.currentTeam?.id and use the already-extracted teamId constant for both
the fetch URL and the mutate key; add an early guard that returns (and
optionally shows an error) if teamId is falsy before calling fetch so you never
construct /api/teams/undefined/..., and update the mutate call to
mutate(`/api/teams/${teamId}/visitor-groups`) to match.
- Around line 388-414: The handler handleDelete currently uses a blocking
window.confirm; replace that synchronous prompt with the project's shadcn
AlertDialog component by moving the confirmation UI out of handleDelete into a
non-blocking dialog flow: create dialog state (open/close) for selecting a
VisitorGroupWithCount, render AlertDialog when a delete button is clicked (pass
the group data to the dialog), and only call the existing delete logic (the
fetch + toast + mutate flow inside handleDelete) after the user confirms in the
AlertDialog; ensure the dialog provides cancel/confirm actions, handles
loading/disabled state while the async delete runs, and closes on success or
failure so the rest of handleDelete (including
mutate(`/api/teams/${teamInfo?.currentTeam?.id}/visitor-groups`) and toast.*
calls) can remain unchanged.
| const hasLinkedItems = | ||
| loadingDetails || documentLinks.length > 0 || dataroomLinks.length > 0; |
There was a problem hiding this comment.
"Links (0)" header is shown while details are still loading.
hasLinkedItems is true whenever loadingDetails is true, so the "Links" section with a count of (0) is rendered immediately on expansion before the data arrives. Consider incorporating the group's known linkCount to gate the section header, which avoids a confusing zero-count flash.
♻️ Proposed fix
-const hasLinkedItems =
- loadingDetails || documentLinks.length > 0 || dataroomLinks.length > 0;
+const hasLinkedItems =
+ (linkCount > 0 && loadingDetails) ||
+ documentLinks.length > 0 ||
+ dataroomLinks.length > 0;📝 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.
| const hasLinkedItems = | |
| loadingDetails || documentLinks.length > 0 || dataroomLinks.length > 0; | |
| const hasLinkedItems = | |
| (linkCount > 0 && loadingDetails) || | |
| documentLinks.length > 0 || | |
| dataroomLinks.length > 0; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/visitors/visitor-groups-section.tsx` around lines 103 - 104, The
header is shown during loading because hasLinkedItems is true whenever
loadingDetails is true; change the condition so the "Links" section is gated by
the group's known linkCount when available — e.g., compute hasLinkedItems using
group.linkCount > 0 OR (loadingDetails && (documentLinks.length > 0 ||
dataroomLinks.length > 0)) so you don't flash "Links (0)" while details are
still loading; update the code that uses hasLinkedItems in
visitor-groups-section.tsx (references: hasLinkedItems, loadingDetails,
documentLinks, dataroomLinks, group.linkCount).
| <div | ||
| role="button" | ||
| tabIndex={0} | ||
| onClick={toggle} | ||
| onKeyDown={(event) => { | ||
| if (event.key === "Enter" || event.key === " ") { | ||
| event.preventDefault(); | ||
| toggle(); | ||
| } | ||
| }} | ||
| className="cursor-pointer px-4 pt-3.5 sm:px-5 sm:pt-4" |
There was a problem hiding this comment.
Both toggle role="button" divs are missing aria-expanded.
Screen readers cannot determine the expanded/collapsed state without aria-expanded. Both interactive regions should reflect the current state, and optionally an aria-label to distinguish them from each other.
♻️ Proposed fix
- role="button"
- tabIndex={0}
- onClick={toggle}
+ role="button"
+ tabIndex={0}
+ aria-expanded={expanded}
+ aria-label={`${group.name} details`}
+ onClick={toggle}Apply the same aria-expanded={expanded} to the chevron role="button" at the bottom of the card.
Also applies to: 347-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/visitors/visitor-groups-section.tsx` around lines 118 - 128, The
two interactive divs using role="button" (the main header that calls toggle and
the chevron button at the bottom) are missing aria-expanded; update both
elements to include aria-expanded={expanded} so screen readers can detect the
open/closed state, and add distinct aria-labels (e.g., aria-label="Visitor group
header" and aria-label="Toggle details") if needed to differentiate them; ensure
you update the header onClick/onKeyDown block that references toggle and the
chevron role="button" block (the other occurrence around the chevron) to match.
| </> | ||
| ); | ||
| return documentId ? ( | ||
| <Link | ||
| key={groupLink.id} | ||
| href={`/documents/${documentId}`} | ||
| onClick={(e) => e.stopPropagation()} | ||
| className="group/link flex items-center justify-between rounded-md border border-gray-200 bg-white px-2.5 py-1.5 transition-colors hover:bg-gray-50 dark:border-gray-700 dark:bg-gray-800/50 dark:hover:bg-gray-800" | ||
| > | ||
| {content} | ||
| </Link> | ||
| ) : ( | ||
| <div | ||
| key={groupLink.id} | ||
| className="flex items-center justify-between rounded-md border border-gray-200 bg-white px-2.5 py-1.5 dark:border-gray-700 dark:bg-gray-800/50" | ||
| > | ||
| {content} | ||
| </div> |
There was a problem hiding this comment.
ExternalLinkIcon is misleading in the non-navigable fallback <div>.
When documentId (or dataroomId) is null, the shared content variable — which includes ExternalLinkIcon — is rendered inside a plain <div>. The icon visually implies the item is clickable/navigable, but it isn't.
♻️ Proposed fix
Omit the icon in the fallback render, for example by splitting content or conditionally rendering the icon only inside the <Link> branch:
- const content = (
- <>
- <div className="flex min-w-0 items-center gap-2">
- ...
- </div>
- <ExternalLinkIcon className="ml-2 h-3 w-3 ..." />
- </>
- );
+ const innerContent = (
+ <div className="flex min-w-0 items-center gap-2">
+ ...
+ </div>
+ );
return documentId ? (
<Link ...>
- {content}
+ {innerContent}
+ <ExternalLinkIcon className="ml-2 h-3 w-3 ..." />
</Link>
) : (
<div ...>
- {content}
+ {innerContent}
</div>
);Also applies to: 317-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/visitors/visitor-groups-section.tsx` around lines 282 - 299, The
shared `content` currently always includes `ExternalLinkIcon`, which makes the
non-navigable fallback `<div>` (rendered when `documentId`/`dataroomId` is null)
appear clickable; change the render so the icon is only shown for actual links:
either split `content` into two variants (e.g., `contentWithIcon` and
`contentNoIcon`) or keep `content` but conditionally render `ExternalLinkIcon`
only inside the `Link` branch that uses `groupLink.id` and `documentId`; apply
the same conditional/icon-splitting fix to the other similar block around the
`317-336` range.
2 participants
Add visitor groups for managing document and data room permissions and merge fixes for stale allow/deny list state in link settings.
The core problem addressed is the tedious and error-prone manual addition of individual emails/domains to multiple document/dataroom links. Visitor groups centralize viewer management, automatically updating access across all linked documents when a group is modified. The merged fixes from PR #2069 resolve issues where the allow/deny list sections in the link sheet could display stale data or behave inconsistently due to
useEffectsynchronization problems and lack of component remounting.Slack Thread
Summary by CodeRabbit
New Features
Behavior Changes
Documentation