feat: show archived links on documents #1883
Conversation
WalkthroughThe PR removes an unused Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as /api/analytics (links)
participant DB as Database
Client->>API: GET analytics?type=links
API->>DB: Query links (no isArchived filter)
DB-->>API: Links (archived + active) + nested view data
API-->>Client: JSON response
Note over API,DB: Archived links are now included
sequenceDiagram
autonumber
actor Client
participant API as /api/teams/.../documents/.../links
participant Auth as Auth (authOptions)
participant DB as Database
Client->>API: GET document links
API->>Auth: Validate session (authOptions via alias)
Auth-->>API: Session OK
API->>DB: Fetch document with links (no isArchived filter), counts
DB-->>API: Document + all links (ordered)
API-->>Client: JSON response
Note over API,DB: Counts and lists now include archived links
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| _count: { | ||
| select: { | ||
| links: { where: { isArchived: false } }, | ||
| links: true, |
There was a problem hiding this comment.
The count query and actual data query are now inconsistent - the count checks all links but the early return logic expects only non-archived links, potentially causing incorrect empty responses.
View Details
📝 Patch Details
diff --git a/pages/api/teams/[teamId]/documents/[id]/links.ts b/pages/api/teams/[teamId]/documents/[id]/links.ts
index c13d0689..e2f7e1e3 100644
--- a/pages/api/teams/[teamId]/documents/[id]/links.ts
+++ b/pages/api/teams/[teamId]/documents/[id]/links.ts
@@ -42,7 +42,7 @@ export default async function handle(
ownerId: true,
_count: {
select: {
- links: true,
+ links: { where: { isArchived: false } },
},
},
},
@@ -64,6 +64,7 @@ export default async function handle(
id: true,
ownerId: true,
links: {
+ where: { isArchived: false },
orderBy: { createdAt: "desc" },
include: {
views: { orderBy: { viewedAt: "desc" } },
Analysis
Inconsistent link filtering returns archived links in /api/teams/[teamId]/documents/[id]/links
What fails: Document links API (pages/api/teams/[teamId]/documents/[id]/links.ts) returns archived links that should be filtered out, breaking consistency with other APIs
How to reproduce:
- Create a document with links
- Archive all links for that document
- Call
GET /api/teams/{teamId}/documents/{docId}/links
Result: Returns archived links in response array instead of empty array
Expected: Should return empty array, consistent with other APIs like /api/revalidate.ts (line 81) and /api/teams/[teamId]/documents/[id]/overview.ts (line 109) which filter isArchived: false
Root cause: Count query counts all links but main query also fetches all links, creating inconsistent behavior where archived links are returned when they should be hidden
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/links/links-table.tsx (1)
121-124: Bug: links.sort mutates props; copy before sorting.Sorting in place can cause subtle UI/state issues.
- const sortedLinks = links.sort( + const sortedLinks = [...links].sort( (a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime(), );
🧹 Nitpick comments (8)
components/links/links-table.tsx (6)
20-20: Prefer scoped mutate via useSWRConfig over global mutate.Using the context-scoped mutate avoids surprises in multi-provider setups and aligns with SWR best practices.
-import { mutate } from "swr"; +import { useSWRConfig } from "swr";Add inside the component (outside this range):
const { mutate } = useSWRConfig();
283-289: Use functional mutate to avoid stale updates.Current updates rely on the props snapshot; can drop changes if cache diverges.
- mutate( + mutate( `/api/teams/${currentTeamId}/${endpointTargetType}/${encodeURIComponent( link.documentId ?? link.dataroomId ?? "", )}/links`, - (links || []).concat(duplicatedLink), - false, + (current: LinkWithViews[] = []) => current.concat(duplicatedLink), + false, );- mutate( + mutate( `/api/teams/${currentTeamId}/${endpointTargetType}/${encodeURIComponent( duplicatedLink.documentId ?? duplicatedLink.dataroomId ?? "", )}/groups/${duplicatedLink.groupId}/links`, - groupLinks.concat(duplicatedLink), - false, + (current: LinkWithViews[] = []) => + (current.length ? current : groupLinks).concat(duplicatedLink), + false, );Also applies to: 295-301
521-527: Use functional mutate when archiving to ensure consistency.Prevents race conditions with concurrent cache updates.
- mutate( + mutate( `/api/teams/${currentTeamId}/${endpointTargetType}/${encodeURIComponent( targetId, )}/links`, - (links || []).map((link) => (link.id === linkId ? archivedLink : link)), - false, + (current: LinkWithViews[] = []) => + current.map((l) => (l.id === linkId ? archivedLink : l)), + false, );- mutate( + mutate( `/api/teams/${currentTeamId}/${endpointTargetType}/${encodeURIComponent( archivedLink.documentId ?? archivedLink.dataroomId ?? "", )}/groups/${groupId}/links`, - groupLinks.map((link) => (link.id === linkId ? archivedLink : link)), - false, + (current: LinkWithViews[] = []) => { + const base = current.length ? current : groupLinks; + return base.map((l) => (l.id === linkId ? archivedLink : l)); + }, + false, );Also applies to: 534-539
262-306: Prevent stuck loading state on duplicate errors.Wrap in try/catch/finally; currently errors leave isLoading=true.
const handleDuplicateLink = async (link: LinkWithViews) => { setIsLoading(true); - - const response = await fetch(`/api/links/${link.id}/duplicate`, { - method: "POST", - headers: { - "Content-Type": "application/json", - }, - body: JSON.stringify({ - teamId: currentTeamId, - }), - }); - - if (!response.ok) { - throw new Error(`HTTP error! status: ${response.status}`); - } - - const duplicatedLink = await response.json(); - const endpointTargetType = `${targetType.toLowerCase()}s`; // "documents" or "datarooms" - - // Update the duplicated link in the list of links - mutate( - `/api/teams/${currentTeamId}/${endpointTargetType}/${encodeURIComponent( - link.documentId ?? link.dataroomId ?? "", - )}/links`, - (links || []).concat(duplicatedLink), - false, - ); - - // Update the group-specific links cache if this is a group link - if (!!groupId) { - const groupLinks = - links?.filter((link) => link.groupId === groupId) || []; - mutate( - `/api/teams/${currentTeamId}/${endpointTargetType}/${encodeURIComponent( - duplicatedLink.documentId ?? duplicatedLink.dataroomId ?? "", - )}/groups/${duplicatedLink.groupId}/links`, - groupLinks.concat(duplicatedLink), - false, - ); - } - - toast.success("Link duplicated successfully"); - setIsLoading(false); + try { + const response = await fetch(`/api/links/${link.id}/duplicate`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ teamId: currentTeamId }), + }); + if (!response.ok) { + throw new Error(`HTTP error! status: ${response.status}`); + } + const duplicatedLink = await response.json(); + const endpointTargetType = `${targetType.toLowerCase()}s`; + mutate( + `/api/teams/${currentTeamId}/${endpointTargetType}/${encodeURIComponent( + link.documentId ?? link.dataroomId ?? "", + )}/links`, + (current: LinkWithViews[] = []) => current.concat(duplicatedLink), + false, + ); + if (!!groupId) { + const groupLinks = links?.filter((l) => l.groupId === groupId) || []; + mutate( + `/api/teams/${currentTeamId}/${endpointTargetType}/${encodeURIComponent( + duplicatedLink.documentId ?? duplicatedLink.dataroomId ?? "", + )}/groups/${duplicatedLink.groupId}/links`, + (current: LinkWithViews[] = []) => + (current.length ? current : groupLinks).concat(duplicatedLink), + false, + ); + } + toast.success("Link duplicated successfully"); + } catch (e) { + console.error(e); + toast.error("Failed to duplicate link"); + } finally { + setIsLoading(false); + } };
833-840: Param naming mismatch confuses logic.You pass checked (active) into a param named isArchived, then invert inside the API body. Rename for clarity to reduce future mistakes.
- const handleArchiveLink = async ( - linkId: string, - targetId: string, - isArchived: boolean, - ) => { + const handleArchiveLink = async ( + linkId: string, + targetId: string, + nextActive: boolean, + ) => { ... - body: JSON.stringify({ - isArchived: !isArchived, - }), + body: JSON.stringify({ isArchived: !nextActive }), ... - toast.success( - !isArchived ? "Link successfully archived" : "Link successfully reactivated", - ); + toast.success(nextActive ? "Link successfully reactivated" : "Link successfully archived");- onCheckedChange={(checked) => + onCheckedChange={(checked) => handleArchiveLink( link.id, link.documentId ?? link.dataroomId ?? "", - checked, + checked, ) }Also applies to: 495-500
106-107: Minor: avoid invalidating memo every render with now dependency.Either compute now inside the memo or make it stable (e.g., useRef) since there’s no timer-driven refresh.
Also applies to: 117-143
pages/api/teams/[teamId]/documents/[id]/links.ts (2)
91-121: N+1 queries for tags will scale poorly; batch-fetch tags.For many links (now including archived), this will be slow. Consider a single query for all linkIds and grouping on the app side.
Example approach (sketch):
const linkIds = docWithLinks!.links.map(l => l.id); const tags = await prisma.tag.findMany({ where: { items: { some: { linkId: { in: linkIds }, itemType: "LINK_TAG" } } }, select: { id: true, name: true, color: true, description: true, items: { select: { linkId: true } } }, }); const tagsByLinkId = new Map<string, typeof tags>(); for (const t of tags) { for (const it of t.items) { const arr = tagsByLinkId.get(it.linkId) ?? []; arr.push({ id: t.id, name: t.name, color: t.color, description: t.description }); tagsByLinkId.set(it.linkId, arr); } } links = links.map(link => ({ ...link, tags: tagsByLinkId.get(link.id) ?? [] }));
95-97: Reassess returning decrypted passwords to clients.Even with team membership checks, returning plaintext passwords increases risk. Prefer server-side-only comparisons or return a boolean that password is set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/links/links-table.tsx(1 hunks)pages/api/analytics/index.ts(0 hunks)pages/api/teams/[teamId]/documents/[id]/links.ts(2 hunks)
💤 Files with no reviewable changes (1)
- pages/api/analytics/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Vercel Agent Review
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
components/links/links-table.tsx (1)
26-26: LGTM: utils import.Looks correct; all imported symbols are used.
pages/api/teams/[teamId]/documents/[id]/links.ts (2)
45-46: Inclusion of archived links: align consumers and expectations.Count/select now include archived links. Confirm all consumers (UI, analytics, exports) expect archived records.
Also applies to: 66-85
3-3: Alias path verified — import resolves.
pages/api/auth/[...nextauth].ts exports authOptions (export const authOptions at line 38) and tsconfig.json maps "@/" → ["./"], so "@/pages/api/auth/[...nextauth]" resolves.
Summary by CodeRabbit