feat: provide collection functionality - #128
Conversation
Review Summary by QodoAdd collection management system with public sharing support
WalkthroughsDescription• Add complete collection management system with CRUD operations • Implement public/private collection sharing with Firestore indexing • Create collection UI components for browsing and managing collections • Integrate collection buttons into content pages alongside favorites • Add Firestore security rules for collection access control Diagramflowchart LR
A["User Content<br/>Sections/Chapters"] -->|"Add to Collection"| B["Collection<br/>Management"]
B -->|"Create/Update/Delete"| C["Firestore<br/>Collections"]
C -->|"Sync Public"| D["Public Collections<br/>Index"]
B -->|"Browse"| E["My Collections<br/>Page"]
D -->|"View"| F["Public Collections<br/>Page"]
G["Security Rules"] -->|"Enforce Access"| C
G -->|"Allow Read"| D
File Changes1. src/lib/collections.ts
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a full collections feature: Firestore security rules for collections and publicCollections, new collection types and Firestore-backed library, multiple client UI components and pages for creating/managing/public-browsing collections, and integration buttons in existing content views. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant UI as Browser UI
participant API as Collections API
participant DB as Firestore
User->>UI: Click "Add to Collection"
UI->>API: getCollections(userId) & getCollectionsForContent(userId, contentId)
API->>DB: Read user collections & membership
DB-->>API: Collections data
API-->>UI: Return lists (collections, included-in)
User->>UI: Toggle membership or create collection
UI->>API: addContentToCollection / removeContentFromCollection / createCollection
API->>DB: Transactional add/remove, update counts, sync public index
DB-->>API: Transaction result
API-->>UI: Success/Failure
UI->>User: Update UI and dispatch "collections-changed"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Visit the preview URL for this PR (updated for commit d029015): https://izuminokami-kanesada--pr128-issue-85-collection-9aqwvk12.web.app (expires Wed, 18 Feb 2026 20:55:30 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 4c4412227845b968bcb4c8b6996048cdd07fd6de |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ContentPageClient.tsx (1)
13-17:⚠️ Potential issue | 🟡 MinorStale JSDoc — now also displays the collection button.
The comment says "Displaying favorite button" but the component now renders both
FavoriteButtonandAddToCollectionButton.📝 Proposed fix
/** * Client component for content page that handles: * - Recording access history - * - Displaying favorite button + * - Displaying favorite button and collection button */firestore.rules (1)
26-56:⚠️ Potential issue | 🔴 CriticalCatch-all rule makes private collections publicly readable.
Firestore rules are additive (OR semantics). The
match /{document=**}block at line 50 withallow read: if trueapplies to every document path, including/collections/{userId}/items/{collectionId}/contents/{contentId}. When multiple rules match a path, access is granted if ANY rule allows it. The catch-all unconditionally allows read on all paths, bypassing the owner-only read restrictions defined in the/collections/{userId}rules (lines 28-40). Any unauthenticated user can read another user's private collections.This is a critical security issue for the new private collection feature. Additionally, the
publicCollectionsupdate rule (line 46) only validates that the current owner matches the requestor but does not prevent mutation of theuserIdfield itself—an owner could change this field to another user's ID in a single update.Consider restructuring: either scope the catch-all to only genuinely public content paths (e.g., book/chapter reference data), or remove the wildcard and enumerate all public-facing collections explicitly.
🤖 Fix all issues with AI agents
In `@firestore.rules`:
- Around line 42-47: The update rule for match /publicCollections/{collectionId}
currently allows owners to update but doesn't prevent changing the userId;
modify the allow update condition to require request.auth != null,
resource.data.userId == request.auth.uid, and additionally enforce
request.resource.data.userId == resource.data.userId so the owner cannot mutate
the userId field (keep the existing delete rule unchanged).
In `@src/app/collections/public/page.tsx`:
- Around line 45-53: The breadcrumb currently links to the auth-gated route via
Link href="/collections" inside the page component's nav; change this so public
viewers aren't sent to a protected page—either replace the href="/collections"
with href="/" (top page) or conditionally render that Link only when the user is
authenticated (e.g., check your auth helper like getCurrentUser/useSession
inside the page component and render the "マイコレクション" Link only if authenticated).
Update the Link element and any surrounding nav markup accordingly so the
breadcrumb never points unauthenticated users to the protected /collections
route.
In `@src/components/AddToCollectionModal.tsx`:
- Around line 60-95: handleToggle currently swallows errors by only logging them
to console; update it to surface failures to the user by showing a brief toast
or inline error when addContentToCollection or removeContentFromCollection
rejects. Inside the catch block of handleToggle, call your existing
toast/notification helper (or set a local error state like setToggleError with
collectionId) to display a concise message (e.g., "Failed to add to collection"
/ "Failed to remove from collection") and ensure the UI rollbacks any optimistic
state change if you applied one; keep setTogglingId(null) in finally and still
emit window.dispatchEvent('collections-changed') only on success (move that line
into the try after the await calls) so events aren’t fired when the operation
fails.
In `@src/components/CollectionDetailModal.tsx`:
- Around line 14-51: Extract the duplicated helper functions getPreviewText and
getContentDisplayInfo into a new shared module (e.g., create
src/lib/contentDisplay.ts), export both functions with their current signatures
and any used types, and replace the copies in CollectionDetailModal.tsx and
PublicCollectionDetailModal.tsx with imports from that new module; ensure you
preserve calls to getContentById, getBookById, getSectionById and default
parameter (maxLength = 30), keep the returned shape (type/preview/title/href),
and update the component files to import { getPreviewText, getContentDisplayInfo
} from 'src/lib/contentDisplay' (or the correct relative path) removing the
local definitions.
In `@src/lib/collections.ts`:
- Around line 80-135: deleteCollection currently uses getDocs (contentsRef /
contentsSnapshot) outside the transaction so new content added after that read
but before commit can be orphaned; update deleteCollection to ensure eventual
consistency by (a) performing the transactional delete of the collection, parent
count update, and publicCollections delete as-is using runTransaction, and then
(b) after the transaction completes, perform a second cleanup pass that queries
contentsRef again and deletes any remaining documents (paginating/batching as
needed) or schedule a Cloud Function trigger to remove leftover contents;
reference symbols: deleteCollection, getDocs, contentsRef, contentsSnapshot,
runTransaction.
- Around line 274-276: The sort comparator calls .toMillis() directly on
Timestamp fields which can be null; change the comparator to safely handle null
by using optional chaining and a numeric fallback (e.g. use
(a.updatedAt?.toMillis() ?? 0) and (b.updatedAt?.toMillis() ?? 0)) or create a
small helper like safeMillis(ts) that returns ts?.toMillis() ?? 0, then use it
in the sort for summaries, the addedAt sort in getCollectionWithContents, and
the updatedAt sort in getPublicCollections so null timestamps won't throw.
- Around line 534-555: The function getPublicCollectionWithContents currently
trusts the publicCollections index; change it to verify the collection's actual
isPublic flag before returning: after obtaining userId (from publicDoc.data()),
fetch the real collection (via getCollectionWithContents or by reading the
collection doc directly), check that the returned CollectionWithContents (or
collection doc) has isPublic === true, and only return it when that guard
passes; if isPublic is false or missing, return null and log appropriately.
Ensure you reference getPublicCollectionWithContents and
getCollectionWithContents when locating where to add this verification.
- Around line 419-455: getCollectionsForContent is doing sequential
per-collection getDoc calls (after calling getCollections), causing O(N)
sequential reads; change it to build all content document references using
encodeContentId and doc for each collection and then parallelize the reads with
Promise.all over getDoc promises (or alternatively use batched getDocs if
available) and then iterate the results to push matching ContentCollectionInfo
objects; keep the existing try/catch and error logging but replace the for-await
loop with a Promise.all map that returns the same result shape referencing
getCollections, getDoc, encodeContentId, and ContentCollectionInfo.
- Around line 238-281: The N+1 read issue in getCollections: stop fetching each
contents subcollection to compute contentCount; instead add and maintain a
numeric contentCount field on the collection document and read that field in
getCollections. Update addContentToCollection and removeContentFromCollection to
atomically increment/decrement contentCount (use Firestore
FieldValue.increment(1) / increment(-1)) when a content is added/removed and
update updatedAt there as well; then change getCollections to read
data.contentCount (defaulting to 0 if missing) instead of issuing getDocs for
the contents subcollection. Ensure types (CollectionSummary.contentCount) accept
the number and handle missing/undefined values safely.
🧹 Nitpick comments (9)
src/components/AddToCollectionModal.tsx (2)
97-127: UnguardedgetCollectionscall outside try/catch.Line 102 calls
getCollectionsoutside the try/catch block. AlthoughgetCollectionsinternally catches errors and returns[], if the implementation ever changes to throw, this would result in an unhandled promise rejection. Wrap the full sequence for consistency:🛡️ Proposed fix
const handleCollectionCreated = async (newCollectionId: string) => { setIsCreating(false); if (!user) return; - // コレクション一覧を再読み込み - const cols = await getCollections(user.uid); - setCollections(cols); - - // 新しいコレクションにコンテンツを追加 try { + // コレクション一覧を再読み込み + const cols = await getCollections(user.uid); + setCollections(cols); + + // 新しいコレクションにコンテンツを追加 await addContentToCollection(
132-227: Modal lacks Escape key and backdrop-click dismissal.The modal overlay (
bg-black/50) doesn't respond to clicks, and there's no keyboard listener for the Escape key. Other modals in this codebase (e.g.,AuthButton) support Escape to close. Consider adding anonKeyDownhandler for Escape and anonClickon the backdrop for parity and accessibility.src/app/collections/public/page.tsx (1)
1-1: Public page is fully client-rendered — no metadata for SEO.This page is marked
'use client', so it cannot exportgenerateMetadata. Since public collections are meant to be discoverable, consider making this a server component (or using a server component wrapper) to provide metadata such as<title>and<meta name="description">for search engines and social sharing.src/app/collections/page.tsx (3)
40-59: Global custom events for state synchronization is fragile.Using
window.dispatchEvent(new CustomEvent('collections-changed'))as a cross-component communication mechanism bypasses React's data flow. This works but is harder to debug, test, and maintain than alternatives like a shared context/store or lifting state up with callback props.Not a blocker for this PR, but consider migrating to a lightweight state management approach (e.g., a React context with a refresh trigger, or a simple Zustand store) to improve testability and traceability.
101-109: Modal overlay does not close on backdrop click or Escape key.Both
CreateCollectionModalandCollectionDetailModalrender a full-screen overlay (.fixed.inset-0) but there's no handler to close them when clicking outside the dialog or pressing Escape. This is a common accessibility and UX expectation for modal dialogs.Consider adding
onClick={onClose}on the backdrop and stopping propagation on the inner dialog, plus anonKeyDownEscape handler.
261-266:onCreatedcallback discards thecollectionId.The
CreateCollectionModalpassescollectionIdtoonCreated, but the handler() => setIsCreating(false)ignores it. This is not a bug, but if you later want to auto-navigate to the newly created collection (e.g., open the detail modal), you'd need it.src/lib/collections.ts (3)
336-375:addContentToCollectionandremoveContentFromCollectionare not transactional.The
setDoc/deleteDocfor the content document, theupdateDocfor the collection'supdatedAt, and thesyncPublicCollectionIndexcall are three separate non-atomic operations. If the process fails midway (e.g., after writing the content doc but before updatingupdatedAt), the collection metadata becomes stale.Consider wrapping the content write and collection update in a transaction, especially if you adopt the
contentCountfield suggested above — an inconsistent count would be more visible to users than a staleupdatedAt.
21-21:encodeContentId/decodeContentIdimported fromfavorites.ts.These are general-purpose utilities that happen to live in a feature-specific module. Consider extracting them to a shared utility file (e.g.,
src/lib/utils.tsorsrc/lib/contentId.ts) so the dependency between collections and favorites is removed.
140-187:updateCollectionread-then-write is not atomic.The
getDoc+updateDoc+ conditional public index sync are separate operations. A concurrentupdateCollectioncall could interleave, causing the public index to become inconsistent (e.g., two toggle-public calls could both readisPublic=falseand both create the public index). The impact is limited since the same user is unlikely to trigger concurrent updates, but wrapping the read and write in a transaction would be more robust.
| function getPreviewText(contentId: string, maxLength = 30): string { | ||
| const content = getContentById(contentId); | ||
| if (!content || content.segments.length === 0) { | ||
| return ''; | ||
| } | ||
| const text = content.segments.map((s) => s.text.original).join(''); | ||
| if (text.length <= maxLength) { | ||
| return text; | ||
| } | ||
| return `${text.slice(0, maxLength)}…`; | ||
| } | ||
|
|
||
| function getContentDisplayInfo(contentId: string) { | ||
| const parts = contentId.split('/'); | ||
| const bookId = parts[0]; | ||
| const sectionId = parts[1]; | ||
| const chapterId = parts[2]; | ||
|
|
||
| const book = getBookById(bookId); | ||
| const section = getSectionById(bookId, sectionId); | ||
|
|
||
| if (chapterId) { | ||
| const content = getContentById(contentId); | ||
| return { | ||
| type: 'chapter' as const, | ||
| title: `${book?.name || bookId} ${section?.name || sectionId} ${content?.chapter || chapterId}`, | ||
| preview: getPreviewText(contentId), | ||
| href: `/books/${contentId}`, | ||
| }; | ||
| } else { | ||
| return { | ||
| type: 'section' as const, | ||
| title: `${book?.name || bookId} ${section?.name || sectionId}`, | ||
| preview: section ? `${section.totalChapters}章` : '', | ||
| href: `/books/${bookId}/${sectionId}`, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
getPreviewText and getContentDisplayInfo are duplicated across modals.
These two helper functions are identical in both CollectionDetailModal.tsx and PublicCollectionDetailModal.tsx. Extract them into a shared utility module (e.g., src/lib/contentDisplay.ts) to avoid divergence and reduce maintenance burden.
#!/bin/bash
# Verify the duplication between the two files
echo "=== CollectionDetailModal helpers ==="
rg -n 'function getPreviewText|function getContentDisplayInfo' --type=ts
echo ""
echo "=== Diff of the two helper blocks ==="
# Extract helper functions from both files and compare
fd 'CollectionDetailModal.tsx' --type f --exec head -51 {} \;
echo "---"
fd 'PublicCollectionDetailModal.tsx' --type f --exec head -47 {} \;🤖 Prompt for AI Agents
In `@src/components/CollectionDetailModal.tsx` around lines 14 - 51, Extract the
duplicated helper functions getPreviewText and getContentDisplayInfo into a new
shared module (e.g., create src/lib/contentDisplay.ts), export both functions
with their current signatures and any used types, and replace the copies in
CollectionDetailModal.tsx and PublicCollectionDetailModal.tsx with imports from
that new module; ensure you preserve calls to getContentById, getBookById,
getSectionById and default parameter (maxLength = 30), keep the returned shape
(type/preview/title/href), and update the component files to import {
getPreviewText, getContentDisplayInfo } from 'src/lib/contentDisplay' (or the
correct relative path) removing the local definitions.
Code Review by Qodo
1. No collection audit logs
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| export async function createCollection( | ||
| userId: string, | ||
| name: string, | ||
| description?: string, | ||
| isPublic = false, | ||
| ): Promise<string> { | ||
| if (!db) throw new Error('Firestore is not initialized'); | ||
|
|
||
| const firestore = db; | ||
|
|
||
| return runTransaction(firestore, async (transaction) => { | ||
| const parentRef = doc(firestore, 'collections', userId); | ||
| const parentDoc = await transaction.get(parentRef); | ||
|
|
||
| const currentCount = parentDoc.exists() | ||
| ? parentDoc.data().collectionCount || 0 | ||
| : 0; | ||
|
|
||
| if (currentCount >= MAX_COLLECTIONS) { | ||
| throw new Error(`コレクションは最大${MAX_COLLECTIONS}個までです`); | ||
| } | ||
|
|
||
| const collectionRef = doc( | ||
| collection(firestore, 'collections', userId, 'items'), | ||
| ); | ||
|
|
||
| transaction.set( | ||
| parentRef, | ||
| { | ||
| userId, | ||
| collectionCount: increment(1), | ||
| }, | ||
| { merge: true }, | ||
| ); | ||
|
|
||
| transaction.set(collectionRef, { | ||
| name, | ||
| description: description || null, | ||
| isPublic, | ||
| createdAt: serverTimestamp(), | ||
| updatedAt: serverTimestamp(), | ||
| }); | ||
|
|
||
| return collectionRef.id; | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * コレクション削除 | ||
| */ | ||
| export async function deleteCollection( | ||
| userId: string, | ||
| collectionId: string, | ||
| ): Promise<void> { | ||
| if (!db) throw new Error('Firestore is not initialized'); | ||
|
|
||
| const firestore = db; | ||
|
|
||
| // まずコレクションが公開かどうか確認 | ||
| const collectionRef = doc( | ||
| firestore, | ||
| 'collections', | ||
| userId, | ||
| 'items', | ||
| collectionId, | ||
| ); | ||
| const collectionDoc = await getDoc(collectionRef); | ||
|
|
||
| if (!collectionDoc.exists()) { | ||
| throw new Error('コレクションが見つかりません'); | ||
| } | ||
|
|
||
| const wasPublic = collectionDoc.data().isPublic; | ||
|
|
||
| return runTransaction(firestore, async (transaction) => { | ||
| // コンテンツを全て削除 | ||
| const contentsRef = collection( | ||
| firestore, | ||
| 'collections', | ||
| userId, | ||
| 'items', | ||
| collectionId, | ||
| 'contents', | ||
| ); | ||
| const contentsSnapshot = await getDocs(contentsRef); | ||
|
|
||
| for (const contentDoc of contentsSnapshot.docs) { | ||
| transaction.delete(contentDoc.ref); | ||
| } | ||
|
|
||
| // コレクションを削除 | ||
| transaction.delete(collectionRef); | ||
|
|
||
| // カウントをデクリメント | ||
| const parentRef = doc(firestore, 'collections', userId); | ||
| transaction.update(parentRef, { | ||
| collectionCount: increment(-1), | ||
| }); | ||
|
|
||
| // 公開コレクションだった場合、インデックスも削除 | ||
| if (wasPublic) { | ||
| const publicRef = doc(firestore, 'publicCollections', collectionId); | ||
| transaction.delete(publicRef); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * コレクション更新 | ||
| */ | ||
| export async function updateCollection( | ||
| userId: string, | ||
| collectionId: string, | ||
| updates: { | ||
| name?: string; | ||
| description?: string; | ||
| isPublic?: boolean; | ||
| }, | ||
| ): Promise<void> { | ||
| if (!db) throw new Error('Firestore is not initialized'); | ||
|
|
||
| const firestore = db; | ||
|
|
||
| const collectionRef = doc( | ||
| firestore, | ||
| 'collections', | ||
| userId, | ||
| 'items', | ||
| collectionId, | ||
| ); | ||
| const collectionDoc = await getDoc(collectionRef); | ||
|
|
||
| if (!collectionDoc.exists()) { | ||
| throw new Error('コレクションが見つかりません'); | ||
| } | ||
|
|
||
| const currentData = collectionDoc.data(); | ||
| const wasPublic = currentData.isPublic; | ||
| const willBePublic = updates.isPublic ?? wasPublic; | ||
|
|
||
| await updateDoc(collectionRef, { | ||
| ...updates, | ||
| updatedAt: serverTimestamp(), | ||
| }); | ||
|
|
||
| // 公開状態の変更に応じてインデックスを更新 | ||
| if (willBePublic && !wasPublic) { | ||
| // 非公開→公開: インデックスに追加 | ||
| await syncPublicCollectionIndex(userId, collectionId); | ||
| } else if (!willBePublic && wasPublic) { | ||
| // 公開→非公開: インデックスから削除 | ||
| const publicRef = doc(firestore, 'publicCollections', collectionId); | ||
| await deleteDoc(publicRef); | ||
| } else if (willBePublic) { | ||
| // 公開のまま更新: インデックスも更新 | ||
| await syncPublicCollectionIndex(userId, collectionId); | ||
| } | ||
| } |
There was a problem hiding this comment.
1. No collection audit logs 📘 Rule violation ✧ Quality
• Collection create/update/delete are critical write/delete actions but the new implementation does not record an audit trail (who did what, when, and the outcome). • Without structured audit events, it’s not possible to reliably reconstruct user actions for security analysis or compliance investigations.
Agent Prompt
## Issue description
Collection mutations (create/update/delete collections and add/remove items) do not emit audit trail events with required context (user, timestamp, action, outcome).
## Issue Context
Compliance requires reconstructable audit trails for critical actions. Current implementation performs writes/deletes without emitting audit records.
## Fix Focus Areas
- src/lib/collections.ts[30-187]
- src/lib/collections.ts[336-414]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } catch (err) { | ||
| console.error('Failed to create collection:', err); | ||
| setError( | ||
| err instanceof Error ? err.message : 'コレクションの作成に失敗しました', | ||
| ); |
There was a problem hiding this comment.
2. err.message shown to users 📘 Rule violation ⛨ Security
• The create-collection UI displays err.message directly to the user, which can expose internal implementation details (e.g., initialization/config errors or backend messages). • This violates secure error handling expectations for user-facing messages and can leak sensitive system context.
Agent Prompt
## Issue description
The UI exposes raw exception messages via `err.message`.
## Issue Context
User-facing errors must be generic to avoid leaking internal details; detailed diagnostics should be logged securely.
## Fix Focus Areas
- src/components/CreateCollectionModal.tsx[37-41]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| export async function getPublicCollectionWithContents( | ||
| collectionId: string, | ||
| ): Promise<CollectionWithContents | null> { | ||
| if (!db) return null; | ||
|
|
||
| try { | ||
| // まず公開インデックスから情報を取得 | ||
| const publicRef = doc(db, 'publicCollections', collectionId); | ||
| const publicDoc = await getDoc(publicRef); | ||
|
|
||
| if (!publicDoc.exists()) return null; | ||
|
|
||
| const publicData = publicDoc.data(); | ||
| const userId = publicData.userId; | ||
|
|
||
| // 実際のコレクションデータを取得 | ||
| return getCollectionWithContents(userId, collectionId); | ||
| } catch (error) { |
There was a problem hiding this comment.
3. Public collections not viewable 📎 Requirement gap ✓ Correctness
• Public collection detail loading reads from /collections/{userId}/items/{collectionId}, but
Firestore rules allow reads there only for the owner (request.auth.uid == userId).
• As a result, other users (and anonymous users) cannot actually view the contents of a public
collection, so the public/private setting is not reflected in access.
Agent Prompt
## Issue description
Public collection detail uses a path that is forbidden to non-owners by Firestore rules, so public collections cannot actually be viewed.
## Issue Context
The public index is readable, but the underlying collection contents remain private under `/collections/{userId}/...`.
## Fix Focus Areas
- firestore.rules[27-47]
- src/lib/collections.ts[534-551]
- src/components/PublicCollectionDetailModal.tsx[60-77]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| transaction.set( | ||
| parentRef, | ||
| { | ||
| userId, | ||
| collectionCount: increment(1), | ||
| }, | ||
| { merge: true }, | ||
| ); | ||
|
|
||
| transaction.set(collectionRef, { | ||
| name, | ||
| description: description || null, | ||
| isPublic, | ||
| createdAt: serverTimestamp(), | ||
| updatedAt: serverTimestamp(), | ||
| }); |
There was a problem hiding this comment.
4. Missing userid/collectionid fields 📎 Requirement gap ✓ Correctness
• The persisted Firestore documents do not store fields required by the spec: collection docs omit userId, and collection item docs omit collectionId. • Even if these can be inferred from the document path, the compliance requirement explicitly calls out these fields as part of the data model, and missing fields can break integrations/queries that rely on them.
Agent Prompt
## Issue description
Firestore persistence does not include required spec fields (`userId` on Collection docs and `collectionId` on CollectionItem docs).
## Issue Context
The compliance spec requires these fields to be held by the data model, not only inferred from paths.
## Fix Focus Areas
- src/lib/collections.ts[40-71]
- src/lib/collections.ts[355-360]
- src/types/collection.ts[17-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| export async function addContentToCollection( | ||
| userId: string, | ||
| collectionId: string, | ||
| contentId: string, | ||
| contentType: ContentType, | ||
| ): Promise<void> { | ||
| if (!db) throw new Error('Firestore is not initialized'); | ||
|
|
||
| const encodedId = encodeContentId(contentId); | ||
| const contentRef = doc( | ||
| db, | ||
| 'collections', | ||
| userId, | ||
| 'items', | ||
| collectionId, | ||
| 'contents', | ||
| encodedId, | ||
| ); | ||
|
|
||
| await setDoc(contentRef, { | ||
| contentId, | ||
| contentType, | ||
| addedAt: serverTimestamp(), | ||
| }); | ||
|
|
||
| // コレクションの更新日時を更新 | ||
| const collectionRef = doc(db, 'collections', userId, 'items', collectionId); | ||
| const collectionDoc = await getDoc(collectionRef); | ||
|
|
||
| if (collectionDoc.exists()) { | ||
| await updateDoc(collectionRef, { | ||
| updatedAt: serverTimestamp(), | ||
| }); | ||
|
|
There was a problem hiding this comment.
5. Orphan content docs possible 📘 Rule violation ⛯ Reliability
• addContentToCollection() writes the content document before verifying that the target collection exists. • If collectionId is invalid/deleted, this can create orphaned subcollection documents and inconsistent state, which is an unhandled edge case.
Agent Prompt
## Issue description
`addContentToCollection()` can create orphan `contents` documents when the parent collection does not exist.
## Issue Context
Edge cases like invalid/deleted collection IDs must be explicitly handled to avoid inconsistent state.
## Fix Focus Areas
- src/lib/collections.ts[336-374]
- src/lib/collections.ts[380-414]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Collections: users can only read/write their own collections | ||
| match /collections/{userId} { | ||
| allow read, write: if request.auth != null && request.auth.uid == userId; | ||
|
|
||
| // Collection items subcollection | ||
| match /items/{collectionId} { | ||
| allow read, write: if request.auth != null && request.auth.uid == userId; | ||
|
|
||
| // Contents within a collection | ||
| match /contents/{contentId} { | ||
| allow read, write: if request.auth != null && request.auth.uid == userId; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Public collections: anyone can read, only owner can write | ||
| match /publicCollections/{collectionId} { | ||
| allow read: if true; | ||
| allow create: if request.auth != null && request.resource.data.userId == request.auth.uid; | ||
| allow update, delete: if request.auth != null && resource.data.userId == request.auth.uid; | ||
| } | ||
|
|
||
| // Legacy admin-only rules for other collections | ||
| match /{document=**} { | ||
| // Anyone can read (public content) |
There was a problem hiding this comment.
6. Catch-all read bypass 🐞 Bug ⛨ Security
• The new /collections/{userId} rules intend to restrict reads/writes to the owning user, but the
existing catch-all match /{document=**} still grants allow read: if true.
• Firestore allows are effectively OR’ed across matching rules, so this makes collections (and other
user data) publicly readable despite the new restrictions.
• This is a critical privacy/security issue and should be fixed before shipping collections.
Agent Prompt
## Issue description
The Firestore rules contain a global wildcard rule that allows any read (`allow read: if true`). This defeats the intent of the new `/collections/{userId}` owner-only rules, making private collections publicly readable.
## Issue Context
Firestore rules are not “first match wins”; permissive rules on any matching `match` path can grant access. A wildcard read rule at `/{document=**}` will match every document and can therefore bypass more restrictive rules.
## Fix Focus Areas
- firestore.rules[27-56]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| transaction.set(collectionRef, { | ||
| name, | ||
| description: description || null, | ||
| isPublic, | ||
| createdAt: serverTimestamp(), | ||
| updatedAt: serverTimestamp(), | ||
| }); | ||
|
|
||
| return collectionRef.id; | ||
| }); |
There was a problem hiding this comment.
🔴 createCollection with isPublic=true never creates the public index document
When a user creates a collection with the "公開する" checkbox enabled, the createCollection function stores isPublic: true in the collection document but never calls syncPublicCollectionIndex to create the corresponding publicCollections/{collectionId} document.
Root Cause and Impact
The createCollection function (src/lib/collections.ts:30-75) accepts an isPublic parameter and writes it to the Firestore document at line 68, but has no logic to create a public index entry. Compare this to updateCollection (src/lib/collections.ts:176-186) which correctly handles public index sync:
// updateCollection handles it:
if (willBePublic && !wasPublic) {
await syncPublicCollectionIndex(userId, collectionId);
}But createCollection has no such call after the transaction.
The CreateCollectionModal component (src/components/CreateCollectionModal.tsx:29-34) passes isPublic directly to createCollection. As a result:
- The collection appears with a "公開" badge in the owner's list (since
isPublic: trueis stored). - But
getPublicCollections()(src/lib/collections.ts:507-528) queries thepublicCollectionsFirestore collection, which has no entry for this collection. - The collection never appears on the public collections page for other users.
Impact: Creating a public collection is silently broken. The owner sees it marked as public, but it's invisible to everyone else. The only workaround is toggling public off then on again via updateCollection.
Prompt for agents
In src/lib/collections.ts, after the runTransaction completes in createCollection (around line 74), add logic to sync the public collection index if isPublic is true. Since syncPublicCollectionIndex needs to read the just-created document, add the call after the transaction returns:
After line 74 (the closing of runTransaction), before the function's closing brace, add:
const newId = await runTransaction(...);
if (isPublic) {
await syncPublicCollectionIndex(userId, newId);
}
return newId;
This requires restructuring the return slightly so the transaction result is captured in a variable first, then the sync is performed, then the ID is returned.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/AddToCollectionModal.tsx`:
- Around line 104-136: handleCollectionCreated currently calls getCollections
outside the try/catch so a rejection can crash the component; move the
getCollections(user.uid) call into the existing try block (alongside
addContentToCollection) or wrap it in its own try/catch, ensure errors set
setToggleError(...) and reset setIsCreating(false), and keep updating
setCollections(cols) only after a successful fetch; reference
handleCollectionCreated, getCollections, addContentToCollection, setCollections,
setToggleError, and setIsCreating when making the change.
In `@src/lib/collections.ts`:
- Around line 252-272: The code in syncPublicCollectionIndex is doing an
expensive getDocs on the contents subcollection to compute contentCount; instead
read the already-denormalized count from the collection document (the value
maintained by addContentToCollection and removeContentFromCollection) and use
that for contentCount when writing to the publicCollections doc; update the
logic in syncPublicCollectionIndex to reference data.contentCount (with a safe
fallback like 0 if missing) and remove the getDocs(contentsRef) call to
eliminate the subcollection read.
🧹 Nitpick comments (9)
firestore.rules (1)
50-57: Pre-existing catch-all rule grants public read access to private collections.The
match /{document=**}withallow read: if truematches all documents including/collections/{userId}/items/…. In Firestore, if any matchingallowevaluates totrue, the request is granted — so the owner-only restriction on collections (and on users/favorites) is effectively a no-op for reads.This is not introduced by this PR (the pattern pre-dates it), but now that private collections with user-curated content are being stored, the exposure surface is larger. Consider scoping the catch-all to specific top-level collections that are truly public content, or removing the recursive wildcard.
src/lib/contentDisplay.ts (1)
29-53: Consider guarding against malformedcontentIdvalues.If
contentIdhas fewer than two/-separated parts,sectionIdwill beundefined, andgetSectionById(bookId, undefined)may behave unexpectedly. A defensive early return or validation would improve robustness.🛡️ Suggested guard
export function getContentDisplayInfo(contentId: string): ContentDisplayInfo { const parts = contentId.split('/'); const bookId = parts[0]; const sectionId = parts[1]; const chapterId = parts[2]; + + if (!bookId || !sectionId) { + return { + type: 'section', + title: contentId, + preview: '', + href: `/books/${contentId}`, + }; + } const book = getBookById(bookId);src/components/AddToCollectionModal.tsx (1)
141-241: Modal does not close on Escape key or backdrop click.The overlay (
bg-black/50) doesn't have anonClickhandler for closing on backdrop click, and there's nokeydownlistener for the Escape key. This is a common UX expectation for modals.src/lib/collections.ts (4)
384-404:addContentToCollectionis not atomic —setDocandupdateDocare independent operations.If
setDocat line 384 succeeds butupdateDocat line 395 fails (or vice versa),contentCountdrifts out of sync with the actual contents. Similarly,syncPublicCollectionIndexcan fail independently. Consider wrapping at least the content write and collection update in a transaction or a batched write.
428-444: Same non-atomicity concern applies toremoveContentFromCollection.
deleteDocandupdateDoc(increment(-1))are not atomic. A failure after delete but before decrement leavescontentCountoff by one.
85-140: TOCTOU:wasPublicis read outside the transaction, potentially stale by commit time.The
isPublicflag is read at line 107 before the transaction starts at line 110. If a concurrentupdateCollectiontogglesisPublicbetween the read and the transaction commit, the public index may not be correctly updated (e.g., the collection becomes public but the public index entry gets deleted, or vice versa).Moving the initial read into the transaction would close this window:
♻️ Sketch
- const collectionDoc = await getDoc(collectionRef); - - if (!collectionDoc.exists()) { - throw new Error('コレクションが見つかりません'); - } - - const wasPublic = collectionDoc.data().isPublic; - await runTransaction(firestore, async (transaction) => { + const collectionDoc = await transaction.get(collectionRef); + if (!collectionDoc.exists()) { + throw new Error('コレクションが見つかりません'); + } + const wasPublic = collectionDoc.data().isPublic; + // コンテンツを全て削除
540-565:getPublicCollectionsfetches all documents without pagination.If the number of public collections grows, this will hit Firestore read limits and increase latency and cost. Consider adding pagination (e.g.,
limit+startAftercursor) or at minimum an upper-boundlimit.src/components/CollectionDetailModal.tsx (2)
45-59:alert()for error notification is functional but inconsistent withAddToCollectionModal.
AddToCollectionModaluses inline error state (toggleError) for a better UX, while this component usesalert()(line 55). Consider aligning the error handling pattern across modals for consistency.
61-149: Same asAddToCollectionModal: modal lacks Escape key and backdrop-click dismiss behavior.Both modals share this gap. If you address it, consider extracting a shared
ModalWrappercomponent to avoid duplicating the event listener logic.
9a878ec to
c96cc60
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
firestore.rules (1)
26-57:⚠️ Potential issue | 🔴 CriticalCritical: Catch-all rule allows public read to all collections, including private user data.
Firestore evaluates overlapping
matchpaths with OR semantics—if any matching rule grants access, the operation is allowed. The catch-all rule at line 51 (match /{document=**}withallow read: if true) will match every document path, including:
/users/{userId}/{document=**}(intended owner-only)/favorites/{userId}/items/{contentId}(intended owner-only)/collections/{userId}/items/{collectionId}/contents/{contentId}(intended owner-only)This bypasses all three owner-only read restrictions—any user can read any other user's private data (users, favorites, and collections). Only
/commentsand/publicCollectionsare unaffected since they're already publicly readable.The issue is structural: you cannot make a broad catch-all "public" and then restrict it elsewhere with narrower rules. Either remove the catch-all entirely (if no truly legacy collections exist), or explicitly list which top-level collections should have public read + admin write access, like:
- // Legacy admin-only rules for other collections - match /{document=**} { - allow read: if true; - allow write: if request.auth != null && request.auth.token.admin == true; - } + // Legacy collections: public read + admin write only + match /announcements/{document=**} { + allow read: if true; + allow write: if request.auth != null && request.auth.token.admin == true; + }(Replace
announcementswith actual legacy collection names if they exist.)
🤖 Fix all issues with AI agents
In `@src/lib/collections.ts`:
- Around line 354-394: The content count can drift because
addContentToCollection uses setDoc (an upsert) and then unconditionally
increments contentCount, and removeContentFromCollection deletes and
unconditionally decrements; change both functions to perform the content
write/delete and the collection counter update atomically by using a Firestore
transaction (runTransaction) on contentRef and collectionRef: read the content
document inside the transaction, if adding only call set if it doesn't exist and
increment(1) only when it was absent; if removing only call delete and
increment(-1) when the document existed; keep references to
encodedId/contentRef/collectionRef and preserve updatedAt/serverTimestamp()
updates inside the same transaction and still call
syncPublicCollectionIndex(userId, collectionId) after a successful transaction
when the collection is public.
🧹 Nitpick comments (6)
src/components/AddToCollectionModal.tsx (3)
37-59: No user-facing feedback when initial data load fails.If
getCollectionsorgetCollectionsForContentthrows, the error is logged to the console (Line 52) but the user sees either an empty list or no indication that something went wrong. Consider setting an error state here so the UI can display a retry prompt or error message.💡 Suggested change
+ const [loadError, setLoadError] = useState(false); + useEffect(() => { if (!user) return; const currentUser = user; async function loadData() { setIsLoading(true); + setLoadError(false); try { const [cols, included] = await Promise.all([ getCollections(currentUser.uid), getCollectionsForContent(currentUser.uid, contentId), ]); setCollections(cols); setIncludedIn(included); } catch (error) { console.error('Failed to load collections:', error); + setLoadError(true); } finally { setIsLoading(false); } } loadData(); }, [user, contentId]);Then render an error/retry UI when
loadErroris true.
145-244: Modal lacks Escape-key dismissal and click-outside-to-close.Neither this modal nor
CreateCollectionModalhandleEscapekey or backdrop clicks to close. This is a common accessibility expectation for modals (WAI-ARIA dialog pattern). A single shared handler or a reusable modal wrapper would address both.
132-139: RedundantsetIsCreating(false)in catch block.
setIsCreating(false)is already called at Line 105 before thetryblock, so the call at Line 138 is a no-op.src/lib/collections.ts (3)
85-152: Non-transactional pre-read introduces a TOCTOU window indeleteCollection.The collection document is read at Line 101 (outside the transaction) to capture
wasPublic, then the transaction at Line 110 proceeds assuming that value is still valid. A concurrentupdateCollectiontogglingisPublicbetween these two points could leave a stalepublicCollectionsindex document (or skip deleting one that should be removed). Moving the read inside the transaction (usingtransaction.get) would close this gap.This is unlikely in a single-user-owned collection flow, but worth noting.
180-227:updateCollectionread-then-write is not transactional — public index sync could be inconsistent.The
getDocat Line 200 andupdateDocat Line 210 are separate operations. A concurrentupdateCollectioncall (e.g., from two browser tabs) could produce conflicting public-index state since both reads see the samewasPublicand both attempt to sync/delete the index independently. Wrapping the read + update + index sync in a transaction would prevent this.Low risk in a single-user flow, but flagging for awareness.
304-349:contentCountis omitted from the returnedCollectionWithContentsobject.The
Collectioninterface includes an optionalcontentCountfield, anddata.contentCountis available on the Firestore document, but it's not included in the returned object at Lines 333-344. Consumers ofgetCollectionWithContentswon't seecontentCounteven though they can access it on the type.💡 Suggested fix
return { id: collectionDoc.id, userId, name: data.name, description: data.description || undefined, isPublic: data.isPublic || false, + contentCount: data.contentCount ?? 0, createdAt: data.createdAt, updatedAt: data.updatedAt, contents: contents.sort(
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/collections.ts`:
- Around line 180-227: The updateCollection flow has a TOCTOU on isPublic; wrap
the read+write in a Firestore transaction (use runTransaction inside
updateCollection) so you read the document with transaction.get(collectionRef),
compute wasPublic and willBePublic inside the transaction, and perform the
update via transaction.update(collectionRef, { ...updates, updatedAt:
serverTimestamp() }); then perform the corresponding public-index mutation
inside the same transaction (use transaction.set on the publicCollections doc to
add/update or transaction.delete to remove) instead of calling
syncPublicCollectionIndex after the transactional update; alternatively, change
syncPublicCollectionIndex to perform its work within the same transaction and
call it from inside the transaction using the transactional reference
(collectionRef/publicRef) to ensure atomicity.
🧹 Nitpick comments (2)
src/lib/collections.ts (2)
154-175: Non-null assertion onfirestoreparameter that may benullby type.
typeof dbincludesnull(sincedbcan benullbefore initialization). Thefirestore!on Line 164 is safe at runtime because the caller already guardsdb, but the type doesn't encode that guarantee.♻️ Tighten the parameter type
-async function cleanupOrphanedContents( - firestore: typeof db, +async function cleanupOrphanedContents( + firestore: NonNullable<typeof db>, contentsRef: ReturnType<typeof collection>, ): Promise<void> { let queryObj = query(contentsRef, limit(100)); while (true) { const snapshot = await getDocs(queryObj); if (snapshot.empty) break; - const batch = writeBatch(firestore!); + const batch = writeBatch(firestore);
560-573: Minor: inconsistent nullish handling forcontentCount.Line 566 uses
|| 0forcontentCountwhile Lines 258 and 286 use?? 0. With||, a contentCount of0would also fall through (though to the same0result, so it's functionally identical). Use?? 0consistently to match the rest of the file and avoid the semantic ambiguity.♻️ Consistent nullish coalescing
- contentCount: d.data().contentCount || 0, + contentCount: d.data().contentCount ?? 0,
… using transactions
1 participant
Summary by CodeRabbit
New Features
Chores