From f48ed13b00fe1d29babbd5bfabbddb5772f2e557 Mon Sep 17 00:00:00 2001 From: Riyadh Date: Sat, 9 May 2026 11:13:56 +0000 Subject: [PATCH] notes: address review feedback for folder sharing (Task #445) Re-applied review fixes on top of the live folder-sharing implementation: - PATCH/DELETE /notes/:id now do an id-only lookup and return an explicit 403 when the caller is not the owner (previously returned an ambiguous 404). This makes the read-only contract for shared-folder recipients precise instead of silently masquerading as "not found". - PUT /note-folders/:id/shares rejects self-share with 400 "Cannot share folder with yourself" instead of silently filtering the caller's id out of the recipient set. - GET /note-folders/shared-with-me now includes a `noteCount` (live count of the owner's non-archived notes in the folder) computed via SQL subquery; SharedFolder TS type updated; folders rail renders the count next to each shared folder. - SharedFolderView accepts the page's `search` value and filters the read-only list client-side over title + content + checklist item text, matching the owner's search experience. Empty state distinguishes "no notes" from "no matches". - Added GET /notes?sharedFolderId=:id as a spec-aligned alias that returns the owner's live notes inside a folder shared with the caller (gated by an active row in note_folder_shares). - Updated the stale comment block above the share routes that claimed recipient writes "just 404"; documents the new 403 contract. Deviations / out of scope: - Project uses `drizzle-kit push` (no migrations directory); no SQL migration file was added. - Pre-existing TS errors in artifacts/api-server/src/routes/executive-meetings.ts remain; unrelated to Task #445. - "Strict reject for nonexistent recipient ids in share PUT" left as optional follow-up (currently silently dropped per existing behavior). --- artifacts/api-server/src/routes/notes.ts | 94 +++++++++++++++++-- .../src/components/notes/folders-rail.tsx | 8 ++ artifacts/tx-os/src/lib/notes-api.ts | 3 + artifacts/tx-os/src/pages/notes.tsx | 30 +++++- 4 files changed, 122 insertions(+), 13 deletions(-) diff --git a/artifacts/api-server/src/routes/notes.ts b/artifacts/api-server/src/routes/notes.ts index 2396faf7..2d9f99e4 100644 --- a/artifacts/api-server/src/routes/notes.ts +++ b/artifacts/api-server/src/routes/notes.ts @@ -303,6 +303,45 @@ async function handleListMyNotes( res: import("express").Response, ): Promise { const userId = req.session.userId!; + // ?sharedFolderId=:id — read-only access to the live notes inside a + // folder shared WITH the caller. Spec alias for the same data the + // dedicated `/note-folders/:id/shared-notes` endpoint returns; kept + // here so clients that prefer the unified `/notes?…` shape work too. + const sharedFolderIdRaw = req.query.sharedFolderId; + if (typeof sharedFolderIdRaw === "string" && sharedFolderIdRaw.length > 0) { + const parsed = Number(sharedFolderIdRaw); + if (!Number.isInteger(parsed) || parsed <= 0) { + res.status(400).json({ error: "Invalid sharedFolderId" }); + return; + } + const [share] = await db + .select({ folderId: noteFolderSharesTable.folderId }) + .from(noteFolderSharesTable) + .where( + and( + eq(noteFolderSharesTable.folderId, parsed), + eq(noteFolderSharesTable.recipientUserId, userId), + ), + ); + if (!share) { + res.status(403).json({ error: "Folder is not shared with you" }); + return; + } + const [folder] = await db + .select({ ownerId: noteFoldersTable.userId }) + .from(noteFoldersTable) + .where(eq(noteFoldersTable.id, parsed)); + if (!folder) { + res.status(404).json({ error: "Folder not found" }); + return; + } + const ownerNotes = await loadNotesWithLabels(folder.ownerId, false); + const filtered = ownerNotes + .filter((n) => n.folderId === parsed) + .map((n) => ({ ...n, readOnly: true as const })); + res.json(filtered); + return; + } const archived = req.query.archived === "true"; const notes = await loadNotesWithLabels(userId, archived); res.json(notes); @@ -357,14 +396,21 @@ router.patch("/notes/:id", requireAuth, async (req, res): Promise => { return; } const { labelIds, ...data } = parsed.data; + // Two-step lookup so non-owners get a precise 403 (instead of an + // ambiguous 404). Recipients of a shared folder must never be able to + // mutate the owner's notes — see Task #445 acceptance criteria. const [existing] = await db .select() .from(notesTable) - .where(and(eq(notesTable.id, id.id), eq(notesTable.userId, userId))); + .where(eq(notesTable.id, id.id)); if (!existing) { res.status(404).json({ error: "Note not found" }); return; } + if (existing.userId !== userId) { + res.status(403).json({ error: "Forbidden" }); + return; + } if (data.folderId != null && !(await userOwnsFolder(userId, data.folderId))) { res.status(400).json({ error: "Invalid folderId" }); return; @@ -395,14 +441,21 @@ router.delete("/notes/:id", requireAuth, async (req, res): Promise => { res.status(400).json({ error: id.error }); return; } - const result = await db - .delete(notesTable) - .where(and(eq(notesTable.id, id.id), eq(notesTable.userId, userId))) - .returning(); - if (result.length === 0) { + // Two-step lookup so non-owners (e.g. shared-folder recipients) get + // a precise 403 instead of a 404 — see Task #445 acceptance criteria. + const [existing] = await db + .select({ userId: notesTable.userId }) + .from(notesTable) + .where(eq(notesTable.id, id.id)); + if (!existing) { res.status(404).json({ error: "Note not found" }); return; } + if (existing.userId !== userId) { + res.status(403).json({ error: "Forbidden" }); + return; + } + await db.delete(notesTable).where(eq(notesTable.id, id.id)); res.json({ success: true }); }); @@ -1278,9 +1331,11 @@ router.delete("/note-folders/:id", requireAuth, async (req, res): Promise // // A folder owner can share a folder read-only with other users. Recipients see // the owner's CURRENT notes inside the folder (live, not snapshot). They cannot -// edit, add, move, or delete anything; the existing per-note routes already -// filter by `notesTable.userId = req.session.userId`, so any write attempt by a -// recipient just 404s — no extra gating required there. +// edit, add, move, or delete anything; the per-note write routes +// (`PATCH/DELETE /notes/:id`, archive, checklist toggle for non-recipients) +// look up the row by id and explicitly return 403 when the caller is not +// the owner, so a shared-folder recipient that probes a write endpoint +// gets a clear authorization error instead of an ambiguous 404. router.get( "/note-folders/shared-with-me", @@ -1296,6 +1351,15 @@ router.get( ownerUsername: usersTable.username, ownerDisplayNameAr: usersTable.displayNameAr, ownerDisplayNameEn: usersTable.displayNameEn, + // Live count of the owner's non-archived notes in this folder so + // the recipient's rail can show "Folder name (12)" without a + // second roundtrip per row. + noteCount: sql`( + SELECT COUNT(*)::int FROM ${notesTable} + WHERE ${notesTable.folderId} = ${noteFoldersTable.id} + AND ${notesTable.userId} = ${noteFoldersTable.userId} + AND ${notesTable.isArchived} = false + )`.as("note_count"), }) .from(noteFolderSharesTable) .innerJoin( @@ -1361,7 +1425,17 @@ router.put( const desired = new Set(); for (const x of body.recipientUserIds) { const n = Number(x); - if (Number.isInteger(n) && n > 0 && n !== userId) desired.add(n); + if (!Number.isInteger(n) || n <= 0) { + res.status(400).json({ error: "Invalid recipientUserIds" }); + return; + } + // Self-share is meaningless and the spec calls for an explicit + // 400 rather than silently dropping the id. + if (n === userId) { + res.status(400).json({ error: "Cannot share folder with yourself" }); + return; + } + desired.add(n); } // Validate that every desired recipient actually exists. if (desired.size > 0) { diff --git a/artifacts/tx-os/src/components/notes/folders-rail.tsx b/artifacts/tx-os/src/components/notes/folders-rail.tsx index aeb71a02..7c4f9762 100644 --- a/artifacts/tx-os/src/components/notes/folders-rail.tsx +++ b/artifacts/tx-os/src/components/notes/folders-rail.tsx @@ -583,6 +583,14 @@ export function FoldersRail({ })} + + {sf.noteCount} + ); diff --git a/artifacts/tx-os/src/lib/notes-api.ts b/artifacts/tx-os/src/lib/notes-api.ts index 2c49e9c9..a7527997 100644 --- a/artifacts/tx-os/src/lib/notes-api.ts +++ b/artifacts/tx-os/src/lib/notes-api.ts @@ -58,6 +58,9 @@ export interface SharedFolder { ownerUsername: string; ownerDisplayNameAr: string | null; ownerDisplayNameEn: string | null; + // Live count of the owner's non-archived notes inside this folder. Lets + // the rail render "Folder name (12)" without a per-row roundtrip. + noteCount: number; } // One row in a folder's share list (people the owner has shared TO). diff --git a/artifacts/tx-os/src/pages/notes.tsx b/artifacts/tx-os/src/pages/notes.tsx index e5235318..22162ba4 100644 --- a/artifacts/tx-os/src/pages/notes.tsx +++ b/artifacts/tx-os/src/pages/notes.tsx @@ -478,6 +478,7 @@ export default function NotesPage() { setFolderSelection({ kind: "all" })} /> )} @@ -2964,16 +2965,35 @@ function LabelsDialog({ function SharedFolderView({ folderId, labels: _labels, + search, onBack, }: { folderId: number; labels: NoteLabel[]; + search: string; onBack: () => void; }) { const { t, i18n } = useTranslation(); const lang = i18n.language; const { data, isLoading, error } = useSharedFolderNotes(folderId); + // Recipients use the same top-bar search input as their personal notes; + // filter the read-only list client-side over title + content + checklist + // item text so the experience matches the owner's view. + const visibleNotes = useMemo(() => { + if (!data) return []; + const q = search.trim().toLowerCase(); + if (!q) return data.notes; + return data.notes.filter((n) => { + if (n.title?.toLowerCase().includes(q)) return true; + if (n.content?.toLowerCase().includes(q)) return true; + if (n.kind === "checklist" && Array.isArray(n.items)) { + return n.items.some((it) => it.text?.toLowerCase().includes(q)); + } + return false; + }); + }, [data, search]); + if (isLoading) return ; // 403/404 — the owner unshared the folder while we were viewing it. Show a @@ -3030,10 +3050,14 @@ function SharedFolderView({ {t("notes.folders.readOnly", "Read-only")} - {data.notes.length === 0 ? ( + {visibleNotes.length === 0 ? ( } - text={t("notes.folders.sharedEmpty", "This folder has no notes yet")} + text={ + search.trim() + ? t("notes.noMatches", "No notes match your search") + : t("notes.folders.sharedEmpty", "This folder has no notes yet") + } /> ) : (
- {data.notes.map((n) => ( + {visibleNotes.map((n) => (