feat(graphql): implement cursor-based pagination for events query (#1354) - #1594
Conversation
github-actions
Bot
added
backend
ECSoC26
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds cursor-paginated GraphQL events, TipTap club and event embeds, realtime comment subscriptions, storage cleanup, attendance reporting, immutable audit logs, private-group RLS, and related tests and migrations. ChangesGraphQL events and pagination
TipTap editor and embedded content
Realtime comment delivery
Supabase operations and policies
Link preview formatting
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQLYoga
participant EventsResolver
participant Supabase
Client->>GraphQLYoga: Request events(first, after)
GraphQLYoga->>EventsResolver: Resolve EventConnection
EventsResolver->>Supabase: Apply cursor filter and fetch rows
Supabase-->>EventsResolver: Ordered event records
EventsResolver-->>GraphQLYoga: Build edges and pageInfo
GraphQLYoga-->>Client: Return paginated events
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/feed.tsx (1)
415-437: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winComment counts on collapsed posts no longer update live.
The previous blanket
commentslistener that calledrefetchPosts()on any comment change was removed, and the new targeted effect (Lines 439-511) only subscribes forpostIds inexpandedPostIds. A collapsed post'sComments ({postComments.length})count (Line 1320) is now only as fresh as the last unrelatedrefetchPosts()— a new comment on a collapsed post won't be reflected until something else triggers a full refetch.If live counts on collapsed posts matter, consider a lightweight listener that only increments a per-post counter (no profile fetch) instead of restoring the old blanket refetch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/feed.tsx` around lines 415 - 437, Update the realtime subscriptions in the feed effect around supabase.channel("realtime_feed") to handle comment INSERT events for collapsed posts as well as expanded posts. Add a lightweight per-post comment-count update that increments the affected post’s count without fetching profiles or triggering a blanket refetchPosts(); preserve the existing expanded-post comment subscription behavior and avoid double-counting events.
🟡 Minor comments (6)
supabase/functions/cleanup-orphaned-storage/index.ts-137-155 (1)
137-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not report failed deletions as deleted.
deletedFilesLogis populated beforeremove(). A failed batch still returns those names underdeletedFilesand a top-levelsuccess: true. Append only after a successful removal and surface partial failures explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/cleanup-orphaned-storage/index.ts` around lines 137 - 155, The cleanup flow currently records files in deletedFilesLog before confirming removal, so failed batches are reported as deleted. Move the deletedFilesLog updates from the orphan-detection loop into the successful branch after supabase.storage.remove completes, and ensure partial batch failures are explicitly surfaced rather than returning those files as deleted.graphql/resolvers/index.ts-294-302 (1)
294-302: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
hasPreviousPageistrueeven when the cursor was invalid and silently ignored.If
afteris provided butdecodeCursor(after)fails (malformed/forged cursor), the code skips applying the.or()filter and effectively returns page 1 unpaginated — yetpageInfo.hasPreviousPage: !!afterstill reportstrue. Clients following this hint would believe there's a prior page when the response is actually the first page. Consider tracking whether the filter was actually applied (e.g.,hasPreviousPage: !!after && !!decoded), or throwing a GraphQL error for invalid cursors instead of silently falling back to page 1.Also applies to: 325-336
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@graphql/resolvers/index.ts` around lines 294 - 302, Update the pagination flow around decodeCursor and pageInfo.hasPreviousPage so an invalid after cursor does not report a previous page when its filter was skipped. Track whether decodeCursor(after) succeeded and use that state when computing hasPreviousPage, while preserving the existing keyset filter for valid cursors; alternatively, reject invalid cursors with the established GraphQL error mechanism.src/hooks/useCursorEventsQuery.ts-96-113 (1)
96-113: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
fetchGraphQLdoesn't check HTTP status before parsing the response.If the endpoint returns a non-2xx response without a GraphQL-shaped JSON body (e.g., an auth gateway error page or plain-text 500),
res.json()can throw an unhelpful parse error, orjson.datacan be silentlyundefined, instead of a clear error surfaced to the caller.🛡️ Proposed fix
const res = await fetch("/api/graphql", { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ query, variables }), }); + if (!res.ok) { + throw new Error(`GraphQL request failed with status ${res.status}`); + } const json = await res.json();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useCursorEventsQuery.ts` around lines 96 - 113, Update fetchGraphQL to validate the fetch response’s HTTP status before parsing or returning data. For non-2xx responses, throw a clear error that includes the response status, and preserve the existing GraphQL error handling and successful json.data return path for valid responses.src/components/Editor/TiptapRichTextEditor.tsx-90-94 (1)
90-94: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDrop the
id.ilikefallback.
events.idis defined as UUID, soPostgRESTBuilder.or(\id.eq.${eventId},id.ilike.${eventId}`)includes an invalid UUID ILIKE branch; valid UUID pastes should match withid.eq.${eventId}` only, while non-UUID input should be rejected rather than appended into the PostgREST filter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Editor/TiptapRichTextEditor.tsx` around lines 90 - 94, Update the events query in the TiptapRichTextEditor flow to use only the id.eq filter, removing the id.ilike fallback. Validate eventId as a UUID before constructing the PostgREST filter and reject non-UUID input instead of issuing a query with an invalid identifier.src/components/Editor/extensions/ClubMentionExtension.tsx-18-23 (1)
18-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid full-page reload on club mention clicks.
The app uses React Router with
/clubs/:slugrouting, but this link forces a browser reload viawindow.location.href. Node views rendered byReactNodeViewRendererdon’t inherit the app’s router context, so pass a navigation callback intoClubMentionView/the extension options instead of settingwindow.locationdirectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Editor/extensions/ClubMentionExtension.tsx` around lines 18 - 23, Update handleClick in ClubMentionView and the ClubMentionExtension options to accept and invoke a navigation callback for `/clubs/${clubSlug}` instead of assigning window.location.href. Thread this callback from the React Router-aware parent through the extension and ClubMentionView, preserving preventDefault and the existing clubSlug guard.src/components/Editor/extensions/EventCardExtension.tsx-108-118 (1)
108-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMark EventCard attributes as internal before re-merging them.
Tiptap renders declared attributes as HTML attributes by default, so
renderHTMLhere adds the intendeddata-*attrs and thenHTMLAttributesalso outputs raw attrs likeeventid,title,bannerurl, andlink. That also adds a nativetitletooltip for the card content. Addrendered: falseto these attributes, or build the final props without merging the raw attrs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Editor/extensions/EventCardExtension.tsx` around lines 108 - 118, Update EventCardExtension’s addAttributes() so all internal event-card attributes are marked rendered: false before renderHTML re-merges them into data-* props. Ensure raw attributes such as eventId, title, bannerUrl, and url are not emitted or treated as native HTML attributes, while preserving the intended data-* output.
🧹 Nitpick comments (5)
supabase/functions/cleanup-orphaned-storage/index.test.ts (1)
5-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover the destructive cleanup path.
Add mocked-client tests for failed reference queries, exact bucket/path matching, nested/paginated listings, grace-period handling, and failed deletes. The current tests cannot prevent regressions that delete active files or misreport removals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/cleanup-orphaned-storage/index.test.ts` around lines 5 - 44, Add mocked Supabase-client tests for the destructive cleanup flow exercised by the handler, covering failed reference queries, exact bucket/path matching, nested and paginated storage listings, grace-period filtering, and failed delete operations. Assert that active or recently modified files are preserved and that removal results and errors are reported accurately.src/graphql/server.test.ts (1)
82-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for second-page (
after) pagination or resolver-level invalid-cursor handling.Given this PR's core objective is keyset pagination robustness, only the first page (
first: 2, noafter) is exercised end-to-end. Consider adding a case that requests a second page using theendCursorfrom page 1 and asserts the.or()filter narrows results correctly, plus a case passing a malformedaftervalue to verify resolver-level behavior (see related comment on thehasPreviousPage/invalid-cursor handling ingraphql/resolvers/index.ts).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphql/server.test.ts` around lines 82 - 154, Extend the GraphQL pagination tests in “GraphQL Cursor-Based Events Pagination” with a second-page request using the first response’s pageInfo.endCursor as after, asserting the returned events reflect the narrowed keyset results. Add a resolver-level test that sends a malformed after cursor and verifies the expected invalid-cursor behavior, including the related hasPreviousPage handling in the events resolver.supabase/migrations/20260726030000_admin_audit_logs_immutability.sql (1)
24-27: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdding FK constraint in-place locks
audit_logsandprofiles.Static analysis flags this: adding
admin_id UUID REFERENCES public.profiles(id)directly requires a table scan andSHARE ROW EXCLUSIVElock on both tables, blocking writes to each while validating. Ifaudit_logsalready has rows, split intoNOT VALID+ a separateVALIDATE CONSTRAINTtransaction.♻️ Proposed fix
ALTER TABLE public.audit_logs - ADD COLUMN IF NOT EXISTS admin_id UUID REFERENCES public.profiles(id) ON DELETE SET NULL, + ADD COLUMN IF NOT EXISTS admin_id UUID, ADD COLUMN IF NOT EXISTS old_value JSONB, ADD COLUMN IF NOT EXISTS new_value JSONB; + +ALTER TABLE public.audit_logs + ADD CONSTRAINT audit_logs_admin_id_fkey FOREIGN KEY (admin_id) + REFERENCES public.profiles(id) ON DELETE SET NULL NOT VALID; +-- run in a follow-up migration/transaction: +-- ALTER TABLE public.audit_logs VALIDATE CONSTRAINT audit_logs_admin_id_fkey;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260726030000_admin_audit_logs_immutability.sql` around lines 24 - 27, Update the audit_logs migration’s admin_id foreign-key creation to add the constraint as NOT VALID, avoiding an immediate existing-row scan and long write-blocking lock. Add a separate validation step using the named foreign-key constraint, keeping the old_value and new_value column additions unchanged.Source: Linters/SAST tools
src/hooks/useRealtimeComments.ts (1)
96-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo status handling on
.subscribe().If the channel fails to join (
CHANNEL_ERROR/TIMED_OUT), there's no visibility into the failure — comments simply stop arriving in real time with no signal to diagnose why.🔧 Suggested fix
- .subscribe(); + .subscribe((status) => { + if (status === "CHANNEL_ERROR" || status === "TIMED_OUT") { + console.error(`Realtime comments subscription failed for post ${postId}:`, status); + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useRealtimeComments.ts` at line 96, Update the realtime channel subscription in useRealtimeComments so the .subscribe() callback receives and handles the subscription status, explicitly logging or reporting CHANNEL_ERROR and TIMED_OUT failures while preserving the existing successful subscription behavior.src/hooks/useRealtimeComments.test.ts (1)
34-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage gap: async insert-handling path and
enabled: falsecase are untested.Only the channel-name-on-subscribe path is covered. The core logic — the
on("postgres_changes", ...)callback that fetches the author profile, buildsformattedComment, and callsonNewComment— along with theenabled: falsebranch and cleanup (removeChannelon unmount) are not exercised.Consider capturing the handler passed to
mockOnand invoking it with a mock payload to assertonNewCommentreceives the expected shape, plus a dedicated test forenabled: false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useRealtimeComments.test.ts` around lines 34 - 63, Extend the useRealtimeComments tests to cover the postgres_changes handler: capture the callback passed through mockOn, invoke it with a mock insert payload, mock the author-profile fetch, and assert onNewComment receives the expected formatted comment. Add a dedicated enabled: false test asserting no subscription occurs, and verify unmount cleanup calls removeChannel.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@graphql/resolvers/index.ts`:
- Around line 290-292: Update the events resolver to compute totalCount with a
separate count-only Supabase query using head: true, applying the base event
filters but excluding the after cursor condition. Keep the existing paginated
query for fetching nodes, and return the full matching event count consistently
across pages.
- Around line 7-24: Update SimpleDataLoader.load to coalesce concurrent cache
misses within the same microtask into one batchFn call, rather than invoking
batchFn([key]) per key. Queue pending keys and their resolvers, schedule a
single flush, then map each returned value back to the corresponding key while
preserving caching and null results.
- Around line 290-303: Validate the result of decodeCursor in the events
resolver before using decoded.createdAt or decoded.id in query.or. Require a
valid ISO date and a safe, expected ID format/shape, returning null or ignoring
the cursor when validation fails; only construct the keyset filter from
validated values and preserve the existing pagination behavior for valid
cursors.
- Around line 337-342: Update the event resolver to use Supabase’s maybeSingle
query method instead of single, while retaining the existing error handling.
Ensure missing or deleted IDs return null through the nullable event field,
while genuine query errors still throw.
In `@src/hooks/useRealtimeComments.ts`:
- Around line 44-101: The useRealtimeComments subscription effect depends on the
unstable onNewComment callback, causing channel teardown and recreation on each
render. In src/hooks/useRealtimeComments.ts lines 44-101, store onNewComment in
a ref, update that ref in a separate effect, and have the subscription callback
invoke the current ref while removing onNewComment from the subscription effect
dependencies; retain postId and enabled as subscription dependencies. In
src/components/Feed/CommentSection.tsx lines 28-37, make no direct change
because the inline callback is safe once the hook is fixed.
In `@src/routes/feed.tsx`:
- Around line 439-511: Replace the manual realtime subscription useEffect in the
feed component with a per-post PostCommentsRealtimeSubscriber that delegates to
useRealtimeComments. Render one keyed subscriber for each expandedPostIds entry,
forwarding new comments into the existing setLazyComments update and preserving
duplicate prevention; remove the duplicated author enrichment, formatting,
channel array, and teardown logic.
- Around line 439-511: Prevent the realtime INSERT handler in the expanded-post
subscription from replacing a cleared comment cache with only the newly inserted
comment. Update the related commentMutation success flow to stop deleting
lazyComments[postId] and let the subscription append the insert to the existing
thread; if the cache-bust is retained, fetch the complete thread when the entry
is missing instead of initializing it from the realtime payload.
In `@supabase/functions/cleanup-orphaned-storage/index.ts`:
- Around line 93-135: Replace the substring-based check in the orphan detection
loop with exact object matching: parse active database storage URLs into
canonical bucket-and-path keys, decode URL-encoded object names, and compare
each listed object’s full path together with bucket.name. Update the active URL
key construction and the isReferenced logic while preserving the existing
grace-period and deletion flow.
- Around line 108-111: The bucket scan around the storage list call must
paginate through all root entries and recursively traverse folder-like prefixes,
rather than processing only the first 1,000 results. Update the cleanup flow
using the existing bucket iteration and list/delete logic to maintain a
prefix/page queue, continue until each listing is exhausted, and evaluate nested
files for orphan removal.
- Around line 48-94: Update the mandatory Supabase selects in the
active-reference loading flow for profiles, clubs, events, and certificates to
inspect each returned error and abort cleanup immediately when any query fails,
before processing incomplete data. Preserve the existing event_photos try/catch
as the only failure path treated as an unavailable optional table, and ensure
cleanup does not proceed after a mandatory query failure.
In `@supabase/migrations/20260726010000_get_user_attendance_rate.sql`:
- Around line 10-38: Update public.get_user_attendance_rate to authorize access
before querying attendance data: allow requests for auth.uid() and permitted
admin users, and reject all other target_user_id values. Preserve the existing
attendance calculation for authorized callers, using the project’s established
admin-role check and an appropriate authorization error for unauthorized
requests.
In `@supabase/migrations/20260726020000_cron_cleanup_orphaned_storage.sql`:
- Around line 25-30: Update the net.http_post call in the scheduled cleanup job
to read SUPABASE_URL and SUPABASE_SERVICE_ROLE_KEY from the provisioned
vault.decrypted_secrets view instead of secrets.decrypted_secrets, preserving
the existing secret-name filters and request construction.
In `@supabase/migrations/20260726030000_admin_audit_logs_immutability.sql`:
- Around line 32-43: The "System admins can view audit logs" policy currently
grants all club_admin users access; update its profiles role condition to
require system_admin instead. Preserve the policy’s authenticated SELECT
behavior and existing auth.uid() profile matching.
In `@supabase/migrations/20260726050000_private_groups_rls.sql`:
- Around line 21-29: The group_members INSERT policy must prevent self-joining
private groups as approved. Update the policy governing group_members inserts to
require public groups for self-service membership and restrict user-supplied
status so private-group requests cannot bypass approval; preserve the
pending/approved workflow for administrator-approved joins. Add a pgTAP
regression case in groups_rls.test.sql that verifies an authenticated user
cannot obtain approved membership by inserting into a private group.
- Around line 166-180: Add a WITH CHECK clause to the "Authors or group admins
can update posts" policy requiring the updated row’s group_id to belong to a
group where auth.uid() is a member, while preserving author/admin authorization
in the existing USING clause. Ensure post authors cannot reassign posts to
groups they do not belong to.
---
Outside diff comments:
In `@src/routes/feed.tsx`:
- Around line 415-437: Update the realtime subscriptions in the feed effect
around supabase.channel("realtime_feed") to handle comment INSERT events for
collapsed posts as well as expanded posts. Add a lightweight per-post
comment-count update that increments the affected post’s count without fetching
profiles or triggering a blanket refetchPosts(); preserve the existing
expanded-post comment subscription behavior and avoid double-counting events.
---
Minor comments:
In `@graphql/resolvers/index.ts`:
- Around line 294-302: Update the pagination flow around decodeCursor and
pageInfo.hasPreviousPage so an invalid after cursor does not report a previous
page when its filter was skipped. Track whether decodeCursor(after) succeeded
and use that state when computing hasPreviousPage, while preserving the existing
keyset filter for valid cursors; alternatively, reject invalid cursors with the
established GraphQL error mechanism.
In `@src/components/Editor/extensions/ClubMentionExtension.tsx`:
- Around line 18-23: Update handleClick in ClubMentionView and the
ClubMentionExtension options to accept and invoke a navigation callback for
`/clubs/${clubSlug}` instead of assigning window.location.href. Thread this
callback from the React Router-aware parent through the extension and
ClubMentionView, preserving preventDefault and the existing clubSlug guard.
In `@src/components/Editor/extensions/EventCardExtension.tsx`:
- Around line 108-118: Update EventCardExtension’s addAttributes() so all
internal event-card attributes are marked rendered: false before renderHTML
re-merges them into data-* props. Ensure raw attributes such as eventId, title,
bannerUrl, and url are not emitted or treated as native HTML attributes, while
preserving the intended data-* output.
In `@src/components/Editor/TiptapRichTextEditor.tsx`:
- Around line 90-94: Update the events query in the TiptapRichTextEditor flow to
use only the id.eq filter, removing the id.ilike fallback. Validate eventId as a
UUID before constructing the PostgREST filter and reject non-UUID input instead
of issuing a query with an invalid identifier.
In `@src/hooks/useCursorEventsQuery.ts`:
- Around line 96-113: Update fetchGraphQL to validate the fetch response’s HTTP
status before parsing or returning data. For non-2xx responses, throw a clear
error that includes the response status, and preserve the existing GraphQL error
handling and successful json.data return path for valid responses.
In `@supabase/functions/cleanup-orphaned-storage/index.ts`:
- Around line 137-155: The cleanup flow currently records files in
deletedFilesLog before confirming removal, so failed batches are reported as
deleted. Move the deletedFilesLog updates from the orphan-detection loop into
the successful branch after supabase.storage.remove completes, and ensure
partial batch failures are explicitly surfaced rather than returning those files
as deleted.
---
Nitpick comments:
In `@src/graphql/server.test.ts`:
- Around line 82-154: Extend the GraphQL pagination tests in “GraphQL
Cursor-Based Events Pagination” with a second-page request using the first
response’s pageInfo.endCursor as after, asserting the returned events reflect
the narrowed keyset results. Add a resolver-level test that sends a malformed
after cursor and verifies the expected invalid-cursor behavior, including the
related hasPreviousPage handling in the events resolver.
In `@src/hooks/useRealtimeComments.test.ts`:
- Around line 34-63: Extend the useRealtimeComments tests to cover the
postgres_changes handler: capture the callback passed through mockOn, invoke it
with a mock insert payload, mock the author-profile fetch, and assert
onNewComment receives the expected formatted comment. Add a dedicated enabled:
false test asserting no subscription occurs, and verify unmount cleanup calls
removeChannel.
In `@src/hooks/useRealtimeComments.ts`:
- Line 96: Update the realtime channel subscription in useRealtimeComments so
the .subscribe() callback receives and handles the subscription status,
explicitly logging or reporting CHANNEL_ERROR and TIMED_OUT failures while
preserving the existing successful subscription behavior.
In `@supabase/functions/cleanup-orphaned-storage/index.test.ts`:
- Around line 5-44: Add mocked Supabase-client tests for the destructive cleanup
flow exercised by the handler, covering failed reference queries, exact
bucket/path matching, nested and paginated storage listings, grace-period
filtering, and failed delete operations. Assert that active or recently modified
files are preserved and that removal results and errors are reported accurately.
In `@supabase/migrations/20260726030000_admin_audit_logs_immutability.sql`:
- Around line 24-27: Update the audit_logs migration’s admin_id foreign-key
creation to add the constraint as NOT VALID, avoiding an immediate existing-row
scan and long write-blocking lock. Add a separate validation step using the
named foreign-key constraint, keeping the old_value and new_value column
additions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01c57ee8-cf5b-46c2-970b-06ea006c50c3
📒 Files selected for processing (26)
graphql/resolvers/index.tssrc/components/Editor/TiptapExtensions.test.tsxsrc/components/Editor/TiptapReadOnlyViewer.tsxsrc/components/Editor/TiptapRichTextEditor.tsxsrc/components/Editor/extensions/ClubMentionExtension.tsxsrc/components/Editor/extensions/EventCardExtension.tsxsrc/components/EventCard.tsxsrc/components/Feed/CommentSection.tsxsrc/components/ui/EventCard/EventCardActions.tsxsrc/graphql/server.test.tssrc/hooks/useCursorEventsQuery.test.tssrc/hooks/useCursorEventsQuery.tssrc/hooks/useRealtimeComments.test.tssrc/hooks/useRealtimeComments.tssrc/routes/feed.tsxsupabase/functions/cleanup-orphaned-storage/index.test.tssupabase/functions/cleanup-orphaned-storage/index.tssupabase/functions/link-preview/index.tssupabase/migrations/20260726010000_get_user_attendance_rate.sqlsupabase/migrations/20260726020000_cron_cleanup_orphaned_storage.sqlsupabase/migrations/20260726030000_admin_audit_logs_immutability.sqlsupabase/migrations/20260726040000_realtime_comments_broadcast.sqlsupabase/migrations/20260726050000_private_groups_rls.sqlsupabase/tests/audit_logs.test.sqlsupabase/tests/get_user_attendance_rate.test.sqlsupabase/tests/groups_rls.test.sql
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/feed.tsx (1)
415-437: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winComment counts on collapsed posts no longer update live.
The previous blanket
commentslistener that calledrefetchPosts()on any comment change was removed, and the new targeted effect (Lines 439-511) only subscribes forpostIds inexpandedPostIds. A collapsed post'sComments ({postComments.length})count (Line 1320) is now only as fresh as the last unrelatedrefetchPosts()— a new comment on a collapsed post won't be reflected until something else triggers a full refetch.If live counts on collapsed posts matter, consider a lightweight listener that only increments a per-post counter (no profile fetch) instead of restoring the old blanket refetch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/feed.tsx` around lines 415 - 437, Update the realtime subscriptions in the feed effect around supabase.channel("realtime_feed") to handle comment INSERT events for collapsed posts as well as expanded posts. Add a lightweight per-post comment-count update that increments the affected post’s count without fetching profiles or triggering a blanket refetchPosts(); preserve the existing expanded-post comment subscription behavior and avoid double-counting events.
🟡 Minor comments (6)
supabase/functions/cleanup-orphaned-storage/index.ts-137-155 (1)
137-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not report failed deletions as deleted.
deletedFilesLogis populated beforeremove(). A failed batch still returns those names underdeletedFilesand a top-levelsuccess: true. Append only after a successful removal and surface partial failures explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/cleanup-orphaned-storage/index.ts` around lines 137 - 155, The cleanup flow currently records files in deletedFilesLog before confirming removal, so failed batches are reported as deleted. Move the deletedFilesLog updates from the orphan-detection loop into the successful branch after supabase.storage.remove completes, and ensure partial batch failures are explicitly surfaced rather than returning those files as deleted.graphql/resolvers/index.ts-294-302 (1)
294-302: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
hasPreviousPageistrueeven when the cursor was invalid and silently ignored.If
afteris provided butdecodeCursor(after)fails (malformed/forged cursor), the code skips applying the.or()filter and effectively returns page 1 unpaginated — yetpageInfo.hasPreviousPage: !!afterstill reportstrue. Clients following this hint would believe there's a prior page when the response is actually the first page. Consider tracking whether the filter was actually applied (e.g.,hasPreviousPage: !!after && !!decoded), or throwing a GraphQL error for invalid cursors instead of silently falling back to page 1.Also applies to: 325-336
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@graphql/resolvers/index.ts` around lines 294 - 302, Update the pagination flow around decodeCursor and pageInfo.hasPreviousPage so an invalid after cursor does not report a previous page when its filter was skipped. Track whether decodeCursor(after) succeeded and use that state when computing hasPreviousPage, while preserving the existing keyset filter for valid cursors; alternatively, reject invalid cursors with the established GraphQL error mechanism.src/hooks/useCursorEventsQuery.ts-96-113 (1)
96-113: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
fetchGraphQLdoesn't check HTTP status before parsing the response.If the endpoint returns a non-2xx response without a GraphQL-shaped JSON body (e.g., an auth gateway error page or plain-text 500),
res.json()can throw an unhelpful parse error, orjson.datacan be silentlyundefined, instead of a clear error surfaced to the caller.🛡️ Proposed fix
const res = await fetch("/api/graphql", { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ query, variables }), }); + if (!res.ok) { + throw new Error(`GraphQL request failed with status ${res.status}`); + } const json = await res.json();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useCursorEventsQuery.ts` around lines 96 - 113, Update fetchGraphQL to validate the fetch response’s HTTP status before parsing or returning data. For non-2xx responses, throw a clear error that includes the response status, and preserve the existing GraphQL error handling and successful json.data return path for valid responses.src/components/Editor/TiptapRichTextEditor.tsx-90-94 (1)
90-94: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDrop the
id.ilikefallback.
events.idis defined as UUID, soPostgRESTBuilder.or(\id.eq.${eventId},id.ilike.${eventId}`)includes an invalid UUID ILIKE branch; valid UUID pastes should match withid.eq.${eventId}` only, while non-UUID input should be rejected rather than appended into the PostgREST filter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Editor/TiptapRichTextEditor.tsx` around lines 90 - 94, Update the events query in the TiptapRichTextEditor flow to use only the id.eq filter, removing the id.ilike fallback. Validate eventId as a UUID before constructing the PostgREST filter and reject non-UUID input instead of issuing a query with an invalid identifier.src/components/Editor/extensions/ClubMentionExtension.tsx-18-23 (1)
18-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid full-page reload on club mention clicks.
The app uses React Router with
/clubs/:slugrouting, but this link forces a browser reload viawindow.location.href. Node views rendered byReactNodeViewRendererdon’t inherit the app’s router context, so pass a navigation callback intoClubMentionView/the extension options instead of settingwindow.locationdirectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Editor/extensions/ClubMentionExtension.tsx` around lines 18 - 23, Update handleClick in ClubMentionView and the ClubMentionExtension options to accept and invoke a navigation callback for `/clubs/${clubSlug}` instead of assigning window.location.href. Thread this callback from the React Router-aware parent through the extension and ClubMentionView, preserving preventDefault and the existing clubSlug guard.src/components/Editor/extensions/EventCardExtension.tsx-108-118 (1)
108-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMark EventCard attributes as internal before re-merging them.
Tiptap renders declared attributes as HTML attributes by default, so
renderHTMLhere adds the intendeddata-*attrs and thenHTMLAttributesalso outputs raw attrs likeeventid,title,bannerurl, andlink. That also adds a nativetitletooltip for the card content. Addrendered: falseto these attributes, or build the final props without merging the raw attrs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Editor/extensions/EventCardExtension.tsx` around lines 108 - 118, Update EventCardExtension’s addAttributes() so all internal event-card attributes are marked rendered: false before renderHTML re-merges them into data-* props. Ensure raw attributes such as eventId, title, bannerUrl, and url are not emitted or treated as native HTML attributes, while preserving the intended data-* output.
🧹 Nitpick comments (5)
supabase/functions/cleanup-orphaned-storage/index.test.ts (1)
5-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover the destructive cleanup path.
Add mocked-client tests for failed reference queries, exact bucket/path matching, nested/paginated listings, grace-period handling, and failed deletes. The current tests cannot prevent regressions that delete active files or misreport removals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/cleanup-orphaned-storage/index.test.ts` around lines 5 - 44, Add mocked Supabase-client tests for the destructive cleanup flow exercised by the handler, covering failed reference queries, exact bucket/path matching, nested and paginated storage listings, grace-period filtering, and failed delete operations. Assert that active or recently modified files are preserved and that removal results and errors are reported accurately.src/graphql/server.test.ts (1)
82-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for second-page (
after) pagination or resolver-level invalid-cursor handling.Given this PR's core objective is keyset pagination robustness, only the first page (
first: 2, noafter) is exercised end-to-end. Consider adding a case that requests a second page using theendCursorfrom page 1 and asserts the.or()filter narrows results correctly, plus a case passing a malformedaftervalue to verify resolver-level behavior (see related comment on thehasPreviousPage/invalid-cursor handling ingraphql/resolvers/index.ts).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphql/server.test.ts` around lines 82 - 154, Extend the GraphQL pagination tests in “GraphQL Cursor-Based Events Pagination” with a second-page request using the first response’s pageInfo.endCursor as after, asserting the returned events reflect the narrowed keyset results. Add a resolver-level test that sends a malformed after cursor and verifies the expected invalid-cursor behavior, including the related hasPreviousPage handling in the events resolver.supabase/migrations/20260726030000_admin_audit_logs_immutability.sql (1)
24-27: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdding FK constraint in-place locks
audit_logsandprofiles.Static analysis flags this: adding
admin_id UUID REFERENCES public.profiles(id)directly requires a table scan andSHARE ROW EXCLUSIVElock on both tables, blocking writes to each while validating. Ifaudit_logsalready has rows, split intoNOT VALID+ a separateVALIDATE CONSTRAINTtransaction.♻️ Proposed fix
ALTER TABLE public.audit_logs - ADD COLUMN IF NOT EXISTS admin_id UUID REFERENCES public.profiles(id) ON DELETE SET NULL, + ADD COLUMN IF NOT EXISTS admin_id UUID, ADD COLUMN IF NOT EXISTS old_value JSONB, ADD COLUMN IF NOT EXISTS new_value JSONB; + +ALTER TABLE public.audit_logs + ADD CONSTRAINT audit_logs_admin_id_fkey FOREIGN KEY (admin_id) + REFERENCES public.profiles(id) ON DELETE SET NULL NOT VALID; +-- run in a follow-up migration/transaction: +-- ALTER TABLE public.audit_logs VALIDATE CONSTRAINT audit_logs_admin_id_fkey;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260726030000_admin_audit_logs_immutability.sql` around lines 24 - 27, Update the audit_logs migration’s admin_id foreign-key creation to add the constraint as NOT VALID, avoiding an immediate existing-row scan and long write-blocking lock. Add a separate validation step using the named foreign-key constraint, keeping the old_value and new_value column additions unchanged.Source: Linters/SAST tools
src/hooks/useRealtimeComments.ts (1)
96-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo status handling on
.subscribe().If the channel fails to join (
CHANNEL_ERROR/TIMED_OUT), there's no visibility into the failure — comments simply stop arriving in real time with no signal to diagnose why.🔧 Suggested fix
- .subscribe(); + .subscribe((status) => { + if (status === "CHANNEL_ERROR" || status === "TIMED_OUT") { + console.error(`Realtime comments subscription failed for post ${postId}:`, status); + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useRealtimeComments.ts` at line 96, Update the realtime channel subscription in useRealtimeComments so the .subscribe() callback receives and handles the subscription status, explicitly logging or reporting CHANNEL_ERROR and TIMED_OUT failures while preserving the existing successful subscription behavior.src/hooks/useRealtimeComments.test.ts (1)
34-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage gap: async insert-handling path and
enabled: falsecase are untested.Only the channel-name-on-subscribe path is covered. The core logic — the
on("postgres_changes", ...)callback that fetches the author profile, buildsformattedComment, and callsonNewComment— along with theenabled: falsebranch and cleanup (removeChannelon unmount) are not exercised.Consider capturing the handler passed to
mockOnand invoking it with a mock payload to assertonNewCommentreceives the expected shape, plus a dedicated test forenabled: false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useRealtimeComments.test.ts` around lines 34 - 63, Extend the useRealtimeComments tests to cover the postgres_changes handler: capture the callback passed through mockOn, invoke it with a mock insert payload, mock the author-profile fetch, and assert onNewComment receives the expected formatted comment. Add a dedicated enabled: false test asserting no subscription occurs, and verify unmount cleanup calls removeChannel.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@graphql/resolvers/index.ts`:
- Around line 290-292: Update the events resolver to compute totalCount with a
separate count-only Supabase query using head: true, applying the base event
filters but excluding the after cursor condition. Keep the existing paginated
query for fetching nodes, and return the full matching event count consistently
across pages.
- Around line 7-24: Update SimpleDataLoader.load to coalesce concurrent cache
misses within the same microtask into one batchFn call, rather than invoking
batchFn([key]) per key. Queue pending keys and their resolvers, schedule a
single flush, then map each returned value back to the corresponding key while
preserving caching and null results.
- Around line 290-303: Validate the result of decodeCursor in the events
resolver before using decoded.createdAt or decoded.id in query.or. Require a
valid ISO date and a safe, expected ID format/shape, returning null or ignoring
the cursor when validation fails; only construct the keyset filter from
validated values and preserve the existing pagination behavior for valid
cursors.
- Around line 337-342: Update the event resolver to use Supabase’s maybeSingle
query method instead of single, while retaining the existing error handling.
Ensure missing or deleted IDs return null through the nullable event field,
while genuine query errors still throw.
In `@src/hooks/useRealtimeComments.ts`:
- Around line 44-101: The useRealtimeComments subscription effect depends on the
unstable onNewComment callback, causing channel teardown and recreation on each
render. In src/hooks/useRealtimeComments.ts lines 44-101, store onNewComment in
a ref, update that ref in a separate effect, and have the subscription callback
invoke the current ref while removing onNewComment from the subscription effect
dependencies; retain postId and enabled as subscription dependencies. In
src/components/Feed/CommentSection.tsx lines 28-37, make no direct change
because the inline callback is safe once the hook is fixed.
In `@src/routes/feed.tsx`:
- Around line 439-511: Replace the manual realtime subscription useEffect in the
feed component with a per-post PostCommentsRealtimeSubscriber that delegates to
useRealtimeComments. Render one keyed subscriber for each expandedPostIds entry,
forwarding new comments into the existing setLazyComments update and preserving
duplicate prevention; remove the duplicated author enrichment, formatting,
channel array, and teardown logic.
- Around line 439-511: Prevent the realtime INSERT handler in the expanded-post
subscription from replacing a cleared comment cache with only the newly inserted
comment. Update the related commentMutation success flow to stop deleting
lazyComments[postId] and let the subscription append the insert to the existing
thread; if the cache-bust is retained, fetch the complete thread when the entry
is missing instead of initializing it from the realtime payload.
In `@supabase/functions/cleanup-orphaned-storage/index.ts`:
- Around line 93-135: Replace the substring-based check in the orphan detection
loop with exact object matching: parse active database storage URLs into
canonical bucket-and-path keys, decode URL-encoded object names, and compare
each listed object’s full path together with bucket.name. Update the active URL
key construction and the isReferenced logic while preserving the existing
grace-period and deletion flow.
- Around line 108-111: The bucket scan around the storage list call must
paginate through all root entries and recursively traverse folder-like prefixes,
rather than processing only the first 1,000 results. Update the cleanup flow
using the existing bucket iteration and list/delete logic to maintain a
prefix/page queue, continue until each listing is exhausted, and evaluate nested
files for orphan removal.
- Around line 48-94: Update the mandatory Supabase selects in the
active-reference loading flow for profiles, clubs, events, and certificates to
inspect each returned error and abort cleanup immediately when any query fails,
before processing incomplete data. Preserve the existing event_photos try/catch
as the only failure path treated as an unavailable optional table, and ensure
cleanup does not proceed after a mandatory query failure.
In `@supabase/migrations/20260726010000_get_user_attendance_rate.sql`:
- Around line 10-38: Update public.get_user_attendance_rate to authorize access
before querying attendance data: allow requests for auth.uid() and permitted
admin users, and reject all other target_user_id values. Preserve the existing
attendance calculation for authorized callers, using the project’s established
admin-role check and an appropriate authorization error for unauthorized
requests.
In `@supabase/migrations/20260726020000_cron_cleanup_orphaned_storage.sql`:
- Around line 25-30: Update the net.http_post call in the scheduled cleanup job
to read SUPABASE_URL and SUPABASE_SERVICE_ROLE_KEY from the provisioned
vault.decrypted_secrets view instead of secrets.decrypted_secrets, preserving
the existing secret-name filters and request construction.
In `@supabase/migrations/20260726030000_admin_audit_logs_immutability.sql`:
- Around line 32-43: The "System admins can view audit logs" policy currently
grants all club_admin users access; update its profiles role condition to
require system_admin instead. Preserve the policy’s authenticated SELECT
behavior and existing auth.uid() profile matching.
In `@supabase/migrations/20260726050000_private_groups_rls.sql`:
- Around line 21-29: The group_members INSERT policy must prevent self-joining
private groups as approved. Update the policy governing group_members inserts to
require public groups for self-service membership and restrict user-supplied
status so private-group requests cannot bypass approval; preserve the
pending/approved workflow for administrator-approved joins. Add a pgTAP
regression case in groups_rls.test.sql that verifies an authenticated user
cannot obtain approved membership by inserting into a private group.
- Around line 166-180: Add a WITH CHECK clause to the "Authors or group admins
can update posts" policy requiring the updated row’s group_id to belong to a
group where auth.uid() is a member, while preserving author/admin authorization
in the existing USING clause. Ensure post authors cannot reassign posts to
groups they do not belong to.
---
Outside diff comments:
In `@src/routes/feed.tsx`:
- Around line 415-437: Update the realtime subscriptions in the feed effect
around supabase.channel("realtime_feed") to handle comment INSERT events for
collapsed posts as well as expanded posts. Add a lightweight per-post
comment-count update that increments the affected post’s count without fetching
profiles or triggering a blanket refetchPosts(); preserve the existing
expanded-post comment subscription behavior and avoid double-counting events.
---
Minor comments:
In `@graphql/resolvers/index.ts`:
- Around line 294-302: Update the pagination flow around decodeCursor and
pageInfo.hasPreviousPage so an invalid after cursor does not report a previous
page when its filter was skipped. Track whether decodeCursor(after) succeeded
and use that state when computing hasPreviousPage, while preserving the existing
keyset filter for valid cursors; alternatively, reject invalid cursors with the
established GraphQL error mechanism.
In `@src/components/Editor/extensions/ClubMentionExtension.tsx`:
- Around line 18-23: Update handleClick in ClubMentionView and the
ClubMentionExtension options to accept and invoke a navigation callback for
`/clubs/${clubSlug}` instead of assigning window.location.href. Thread this
callback from the React Router-aware parent through the extension and
ClubMentionView, preserving preventDefault and the existing clubSlug guard.
In `@src/components/Editor/extensions/EventCardExtension.tsx`:
- Around line 108-118: Update EventCardExtension’s addAttributes() so all
internal event-card attributes are marked rendered: false before renderHTML
re-merges them into data-* props. Ensure raw attributes such as eventId, title,
bannerUrl, and url are not emitted or treated as native HTML attributes, while
preserving the intended data-* output.
In `@src/components/Editor/TiptapRichTextEditor.tsx`:
- Around line 90-94: Update the events query in the TiptapRichTextEditor flow to
use only the id.eq filter, removing the id.ilike fallback. Validate eventId as a
UUID before constructing the PostgREST filter and reject non-UUID input instead
of issuing a query with an invalid identifier.
In `@src/hooks/useCursorEventsQuery.ts`:
- Around line 96-113: Update fetchGraphQL to validate the fetch response’s HTTP
status before parsing or returning data. For non-2xx responses, throw a clear
error that includes the response status, and preserve the existing GraphQL error
handling and successful json.data return path for valid responses.
In `@supabase/functions/cleanup-orphaned-storage/index.ts`:
- Around line 137-155: The cleanup flow currently records files in
deletedFilesLog before confirming removal, so failed batches are reported as
deleted. Move the deletedFilesLog updates from the orphan-detection loop into
the successful branch after supabase.storage.remove completes, and ensure
partial batch failures are explicitly surfaced rather than returning those files
as deleted.
---
Nitpick comments:
In `@src/graphql/server.test.ts`:
- Around line 82-154: Extend the GraphQL pagination tests in “GraphQL
Cursor-Based Events Pagination” with a second-page request using the first
response’s pageInfo.endCursor as after, asserting the returned events reflect
the narrowed keyset results. Add a resolver-level test that sends a malformed
after cursor and verifies the expected invalid-cursor behavior, including the
related hasPreviousPage handling in the events resolver.
In `@src/hooks/useRealtimeComments.test.ts`:
- Around line 34-63: Extend the useRealtimeComments tests to cover the
postgres_changes handler: capture the callback passed through mockOn, invoke it
with a mock insert payload, mock the author-profile fetch, and assert
onNewComment receives the expected formatted comment. Add a dedicated enabled:
false test asserting no subscription occurs, and verify unmount cleanup calls
removeChannel.
In `@src/hooks/useRealtimeComments.ts`:
- Line 96: Update the realtime channel subscription in useRealtimeComments so
the .subscribe() callback receives and handles the subscription status,
explicitly logging or reporting CHANNEL_ERROR and TIMED_OUT failures while
preserving the existing successful subscription behavior.
In `@supabase/functions/cleanup-orphaned-storage/index.test.ts`:
- Around line 5-44: Add mocked Supabase-client tests for the destructive cleanup
flow exercised by the handler, covering failed reference queries, exact
bucket/path matching, nested and paginated storage listings, grace-period
filtering, and failed delete operations. Assert that active or recently modified
files are preserved and that removal results and errors are reported accurately.
In `@supabase/migrations/20260726030000_admin_audit_logs_immutability.sql`:
- Around line 24-27: Update the audit_logs migration’s admin_id foreign-key
creation to add the constraint as NOT VALID, avoiding an immediate existing-row
scan and long write-blocking lock. Add a separate validation step using the
named foreign-key constraint, keeping the old_value and new_value column
additions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01c57ee8-cf5b-46c2-970b-06ea006c50c3
📒 Files selected for processing (26)
graphql/resolvers/index.tssrc/components/Editor/TiptapExtensions.test.tsxsrc/components/Editor/TiptapReadOnlyViewer.tsxsrc/components/Editor/TiptapRichTextEditor.tsxsrc/components/Editor/extensions/ClubMentionExtension.tsxsrc/components/Editor/extensions/EventCardExtension.tsxsrc/components/EventCard.tsxsrc/components/Feed/CommentSection.tsxsrc/components/ui/EventCard/EventCardActions.tsxsrc/graphql/server.test.tssrc/hooks/useCursorEventsQuery.test.tssrc/hooks/useCursorEventsQuery.tssrc/hooks/useRealtimeComments.test.tssrc/hooks/useRealtimeComments.tssrc/routes/feed.tsxsupabase/functions/cleanup-orphaned-storage/index.test.tssupabase/functions/cleanup-orphaned-storage/index.tssupabase/functions/link-preview/index.tssupabase/migrations/20260726010000_get_user_attendance_rate.sqlsupabase/migrations/20260726020000_cron_cleanup_orphaned_storage.sqlsupabase/migrations/20260726030000_admin_audit_logs_immutability.sqlsupabase/migrations/20260726040000_realtime_comments_broadcast.sqlsupabase/migrations/20260726050000_private_groups_rls.sqlsupabase/tests/audit_logs.test.sqlsupabase/tests/get_user_attendance_rate.test.sqlsupabase/tests/groups_rls.test.sql
🛑 Comments failed to post (10)
src/hooks/useRealtimeComments.ts (1)
44-101: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Realtime comment channel is torn down/rebuilt on nearly every keystroke.
useRealtimeComments's effect depends on the rawonNewCommentcallback, and its only current caller passes a fresh inline arrow function every render; sinceCommentSectionre-renders on every keystroke while composing a comment, the WebSocket subscription churns constantly, risking missed inserts during each resubscribe gap.
src/hooks/useRealtimeComments.ts#L44-L101: storeonNewCommentin auseRef, update it in a separate effect, and drop it from the subscription effect's dependency array so the channel only re-subscribes onpostId/enabledchanges.src/components/Feed/CommentSection.tsx#L28-L37: no change needed once the hook is fixed — the inline callback becomes safe to pass as-is.📍 Affects 2 files
src/hooks/useRealtimeComments.ts#L44-L101(this comment)src/components/Feed/CommentSection.tsx#L28-L37🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useRealtimeComments.ts` around lines 44 - 101, The useRealtimeComments subscription effect depends on the unstable onNewComment callback, causing channel teardown and recreation on each render. In src/hooks/useRealtimeComments.ts lines 44-101, store onNewComment in a ref, update that ref in a separate effect, and have the subscription callback invoke the current ref while removing onNewComment from the subscription effect dependencies; retain postId and enabled as subscription dependencies. In src/components/Feed/CommentSection.tsx lines 28-37, make no direct change because the inline callback is safe once the hook is fixed.src/routes/feed.tsx (1)
439-511: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Duplicates
useRealtimeCommentsand rebuilds every channel on any toggle.This effect reimplements the same author-enrichment/format logic as
src/hooks/useRealtimeComments.ts(compare Lines 456-489 here with that hook), and on everyexpandedPostIdschange it tears down and recreates all per-post channels (Lines 441-511), not just the delta — expanding a 6th post resubscribes the 5 already-open ones too, with a brief gap where an insert on an already-open post could be missed.Consider extracting a tiny per-post subscriber component that wraps
useRealtimeComments, and rendering one perexpandedPostIdsentry so React's mount/unmount lifecycle naturally handles diffing instead of a manualchannelsarray:function PostCommentsRealtimeSubscriber({ postId, onNewComment, }: { postId: string; onNewComment: (c: Comment) => void; }) { useRealtimeComments({ postId, enabled: true, onNewComment }); return null; }Then render
Array.from(expandedPostIds).map((id) => <PostCommentsRealtimeSubscriber key={id} postId={id} onNewComment={...} />). This also eliminates the duplicated type/logic in Lines 456-489 in favor of the shared hook (once the ref-based fix suggested inuseRealtimeComments.tslands, so per-post callback identity no longer matters).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/feed.tsx` around lines 439 - 511, Replace the manual realtime subscription useEffect in the feed component with a per-post PostCommentsRealtimeSubscriber that delegates to useRealtimeComments. Render one keyed subscriber for each expandedPostIds entry, forwarding new comments into the existing setLazyComments update and preserving duplicate prevention; remove the duplicated author enrichment, formatting, channel array, and teardown logic.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Race with
commentMutation's own cache-bust can wipe an expanded post's comment thread.
commentMutation.onSuccess(Lines 651-659, unchanged) deleteslazyComments[postId]after a successful insert, then callsrefetchPosts(). The realtime insert handler added here (Lines 491-500) treats a missing cache entry as an empty list (prev[postId] || []), so when the realtime broadcast for that same insert arrives after the cache-bust,lazyComments[postId]gets overwritten with an array containing only the just-posted comment — the rest of the previously-loaded thread disappears from the UI until the post is collapsed/re-expanded.This will reproduce on essentially every comment a user posts to an expanded thread, since the client's own insert always races the corresponding realtime broadcast for it.
🔧 Suggested fix (stop busting the cache; let realtime append organically)
onSuccess: (_data, variables) => { - // Bust the lazy cache for this post so the new comment appears on re-fetch - setLazyComments((prev) => { - const next = { ...prev }; - delete next[variables.postId]; - return next; - }); refetchPosts(); },Alternatively, if the cache-bust must stay, guard the realtime handler so a missing/just-cleared cache entry triggers
fetchCommentsForPost(postId)instead of seeding a single-item array.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/feed.tsx` around lines 439 - 511, Prevent the realtime INSERT handler in the expanded-post subscription from replacing a cleared comment cache with only the newly inserted comment. Update the related commentMutation success flow to stop deleting lazyComments[postId] and let the subscription append the insert to the existing thread; if the cache-bust is retained, fetch the complete thread when the entry is missing instead of initializing it from the realtime payload.supabase/functions/cleanup-orphaned-storage/index.ts (3)
48-94: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For@supabase/supabase-jsv2.110.0, does a failedsupabase.from(...).select(...)return anerrorresult by default rather than throw, and how should optional-table failures be handled safely?💡 Result:
For
@supabase/supabase-jsv2.110.0, a failedsupabase.from(...).select(...)query returns a{ data, error }object by default rather than throwing an error [1][2][3]. Theerrorproperty will contain details if the request is unsuccessful [1][4]. To handle failures, you should check for the presence of theerrorobject [1][2]: const { data, error } = await supabase.from('table').select; if (error) { console.error(error); // Logs message, code, details, and hint [1][2] return; } Regarding optional-table failures or cases where data may not exist, handling depends on the modifiers used: 1. Use.maybeSingle instead of.single: If you are querying for a single row that might not exist, using.single()will throw a PGRST116 error when no rows are found [5][6]. Using.maybeSingle()instead will safely returnnullfor the data andnullfor the error [5][6]. 2. Check for null/empty results: Even if no error is returned, always verify ifdataisnullor empty if you expect the record to exist, especially when using modifiers like.maybeSingle()or.limit(1)[6]. 3. Programmatic branching: Useerror.codefor programmatic logic (e.g., handling specific Postgres errors like RLS violations), as error codes are stable across versions, whereas error messages may change [1]. If you specifically require a call to throw an error, you can append.throwOnError()to your query builder chain, which will cause the promise to reject if the operation fails [4][3].Citations:
- 1: https://supabase.com/docs/guides/api/handling-errors-in-supabase-js
- 2: https://supabase.com/docs/reference/javascript/select
- 3: supabase/supabase-js#32
- 4: https://supabase.com/docs/reference/javascript/using-modifiers-select
- 5: https://raw.githubusercontent.com/jeremylongshore/claude-code-plugins-plus-skills/HEAD/plugins/saas-packs/supabase-pack/skills/supabase-common-errors/SKILL.md
- 6: https://github.com/bdougie/contributor.info/blob/main/docs/technical/supabase-query-patterns.md
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate and inspect the cleanup function around the reported lines without executing repo code. fd -a 'index.ts' . | rg 'supabase/functions/cleanup-orphaned-storage/index\.ts$' echo '--- outline ---' ast-grep outline supabase/functions/cleanup-orphaned-storage/index.ts || true echo '--- relevant lines ---' sed -n '1,180p' supabase/functions/cleanup-orphaned-storage/index.ts | cat -n echo '--- error handling patterns in Supabase JS docs from installed/package metadata if present ---' if [ -f package.json ]; then jq -r '.dependencies["`@supabase/supabase-js`"], .devDependencies["`@supabase/supabase-js`"]' package.json 2>/dev/null || true fi rg -n "throwOnError|\.select\(|\.from\(|event_photos|Supabase client" supabase/functions/cleanup-orphaned-storage/index.ts package.json 2>/dev/null || trueRepository: krushit1307/CampusConnect
Length of output: 9648
Fail closed when loading active references.
With Supabase JS, failed
select()calls resolve with anerror; the mandatory selects forprofiles,clubs,events, andcertificatesignore it and can populateactiveUrlStringsincompletely. A file referenced only in a failed query will be treated as orphaned and deleted. Check the mandatory queryerrorand abort cleanup on failure; only handle the optionalevent_photosfailure as “table unavailable”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/cleanup-orphaned-storage/index.ts` around lines 48 - 94, Update the mandatory Supabase selects in the active-reference loading flow for profiles, clubs, events, and certificates to inspect each returned error and abort cleanup immediately when any query fails, before processing incomplete data. Preserve the existing event_photos try/catch as the only failure path treated as an unavailable optional table, and ensure cleanup does not proceed after a mandatory query failure.
93-135: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Match referenced objects by canonical bucket and path.
includes(file.name)is not an object identity check: an activebucket-a/foo.pngprotects an orphanedbucket-b/foo.png, while URL-encoded object names can fail to match and be deleted. Parse stored URLs into exact{ bucket, path }keys and compare against the listed object's full path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/cleanup-orphaned-storage/index.ts` around lines 93 - 135, Replace the substring-based check in the orphan detection loop with exact object matching: parse active database storage URLs into canonical bucket-and-path keys, decode URL-encoded object names, and compare each listed object’s full path together with bucket.name. Update the active URL key construction and the isReferenced logic while preserving the existing grace-period and deletion flow.
108-111: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
For@supabase/supabase-jsv2.110.0 Storagelist, confirm pagination semantics and whether listing a bucket prefix recursively returns nested objects.💡 Result:
In
@supabase/supabase-jsv2.110.0, the standardlistmethod (also referred to as the legacy list API) has specific behavior regarding recursion and pagination: Recursive Listing Thelistmethod does not recursively return nested objects [1]. It returns only the immediate children—both files and subfolders—within the specified prefix (folder path) [1]. If you need to access files inside subfolders, you must perform separatelistcalls for those subfolder paths [1]. Pagination Semantics Thelistmethod uses offset-based pagination [2][3]. You can control the results via the options object using: - limit: Specifies the maximum number of items to return (default is 100) [2][3][4]. - offset: Specifies the starting position for the result set (default is 0) [2][3][4]. Note: Supabase has introduced a newer, experimentallistV2method (available in recent versions of@supabase/storage-js) which supports cursor-based pagination and hierarchical grouping (via thewith_delimiteroption) to better handle complex bucket structures [5][2][6][3]. Summary of Key Behaviors - The standardlist(path, options)does not perform recursive lookups [1]. - Folders are returned as entries where fields likeid,updated_at,created_at, andmetadataarenull[7][4][8]. - Standard pagination relies on thelimitandoffsetparameters [2][4].Citations:
- 1: https://www.rapidevelopers.com/supabase-tutorial/how-to-list-files-in-supabase-storage-bucket
- 2: https://deepwiki.com/supabase/supabase-js/6.1-file-operations
- 3: https://github.com/supabase/supabase-js/blob/bd024171/packages/core/storage-js/src/packages/StorageFileApi.ts
- 4: https://supabase.com/docs/reference/javascript/file-buckets-list
- 5: https://github.com/supabase/supabase-js/blob/21e9d4b7/packages/core/storage-js/src/lib/types.ts
- 6: supabase/storage-js#219
- 7: https://cdn.jsdelivr.net/npm/@supabase/storage-js@2.110.8/dist/index.d.mts
- 8: supabase/supabase-js#2116
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'Find file:\n' fd -a 'index.ts$' . | sed 's#^\./##' | rg 'supabase/functions/cleanup-orphaned-storage/index.ts' || true printf '\nRelevant code:\n' sed -n '1,180p' supabase/functions/cleanup-orphaned-storage/index.ts 2>/dev/null || true printf '\nSearch for cleanup function and storage list usage:\n' rg -n "cleanup|list\\(|offset|cursor|with_delimiter|prefix|depth|listV2" supabase/functions/cleanup-orphaned-storage/index.ts supabase/functions 2>/dev/null || true printf '\nPackage versions mentioning supabase/storage-js or `@supabase/storage-js`:\n' rg -n '"`@supabase/`(storage-js|supabase-js)"|supabase-js|storage-js' . -g 'package.json' -g 'deno.json' -g 'supabase/functions/**/*.ts' 2>/dev/null || trueRepository: krushit1307/CampusConnect
Length of output: 12902
Traverse every bucket path page and nested prefix.
The storage
listcall only returns the first 1,000 root-level children of each bucket; it does not recursively include nested files and later pages are never scanned. Drive the scan until every listing is exhausted, and recurse through folder-like entries (or use an explicit prefix queue) so orphaned files deeper in the bucket are also evaluated and removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/cleanup-orphaned-storage/index.ts` around lines 108 - 111, The bucket scan around the storage list call must paginate through all root entries and recursively traverse folder-like prefixes, rather than processing only the first 1,000 results. Update the cleanup flow using the existing bucket iteration and list/delete logic to maintain a prefix/page queue, continue until each listing is exhausted, and evaluate nested files for orphan removal.supabase/migrations/20260726010000_get_user_attendance_rate.sql (1)
10-38: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing authorization check lets any user query another user's attendance rate.
get_user_attendance_rateisSECURITY DEFINERand granted toauthenticated, but never validates that the caller is requesting their own data (or has an admin role). Any authenticated user can callget_user_attendance_rate('<other-user-uuid>')and learn that user's private no-show/check-in behavior, since the function bypasses RLS onevent_rsvps/events.🔒️ Proposed fix
BEGIN + IF auth.uid() IS DISTINCT FROM target_user_id + AND NOT EXISTS (SELECT 1 FROM public.profiles WHERE id = auth.uid() AND role = 'club_admin'::user_role) + THEN + RAISE EXCEPTION 'Not authorized to view this user''s attendance rate'; + END IF; + SELECT COUNT(*)::INTEGER,📝 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.CREATE OR REPLACE FUNCTION public.get_user_attendance_rate(target_user_id UUID) RETURNS INTEGER LANGUAGE plpgsql STABLE SECURITY DEFINER SET search_path = public AS $$ DECLARE total_past_rsvps INTEGER; checked_in_count INTEGER; BEGIN IF auth.uid() IS DISTINCT FROM target_user_id AND NOT EXISTS (SELECT 1 FROM public.profiles WHERE id = auth.uid() AND role = 'club_admin'::user_role) THEN RAISE EXCEPTION 'Not authorized to view this user''s attendance rate'; END IF; SELECT COUNT(*)::INTEGER, COUNT(*) FILTER (WHERE r.checked_in = TRUE)::INTEGER INTO total_past_rsvps, checked_in_count FROM public.event_rsvps r JOIN public.events e ON e.id = r.event_id WHERE r.user_id = target_user_id AND COALESCE(e.end_date, e.start_date, e.event_date) < NOW(); IF total_past_rsvps = 0 THEN RETURN 0; END IF; RETURN ROUND((checked_in_count * 100.0) / total_past_rsvps)::INTEGER; END; $$;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260726010000_get_user_attendance_rate.sql` around lines 10 - 38, Update public.get_user_attendance_rate to authorize access before querying attendance data: allow requests for auth.uid() and permitted admin users, and reject all other target_user_id values. Preserve the existing attendance calculation for authorized callers, using the project’s established admin-role check and an appropriate authorization error for unauthorized requests.supabase/migrations/20260726020000_cron_cleanup_orphaned_storage.sql (1)
25-30: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -i -C2 \ 'create schema[[:space:]]+secrets|decrypted_secrets|vault\.decrypted_secrets' \ supabase || trueRepository: krushit1307/CampusConnect
Length of output: 1145
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== migration ==" cat -n supabase/migrations/20260726020000_cron_cleanup_orphaned_storage.sql echo echo "== schema/object references for secrets/vault/decrypted_secrets in tracked files ==" rg -n -i -C2 '(secrets|vault)\.[^.[:space:],;)]+|decrypted_secrets|supabase_vault|create schema|create (schema|view)|grant (select|usage)|pgbouncer|function pgbouncer|pg_temp|pg_catalog' . || trueRepository: krushit1307/CampusConnect
Length of output: 16818
Use the provisioned secrets view for the scheduled job.
secrets.decrypted_secretsis referenced here, but this repo does not create that schema or view. If Supabase Vault supplies the secrets asvault.decrypted_secrets, point the cron job at that source; otherwise the weekly cleanup job will fail when resolvingSUPABASE_URLandSUPABASE_SERVICE_ROLE_KEY.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260726020000_cron_cleanup_orphaned_storage.sql` around lines 25 - 30, Update the net.http_post call in the scheduled cleanup job to read SUPABASE_URL and SUPABASE_SERVICE_ROLE_KEY from the provisioned vault.decrypted_secrets view instead of secrets.decrypted_secrets, preserving the existing secret-name filters and request construction.supabase/migrations/20260726030000_admin_audit_logs_immutability.sql (1)
32-43: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect how 'club_admin' is used/defined elsewhere to confirm scope (global vs per-club) rg -n "club_admin" supabase/schema.sql supabase/migrations rg -n "CREATE TYPE user_role|user_role" supabase/schema.sqlRepository: krushit1307/CampusConnect
Length of output: 10978
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== profile and club-related definitions ==" rg -n "CREATE TABLE public\\.profiles|CREATE TABLE .*club_members|profiles.*role|club_members" supabase/schema.sql supabase/migrations | head -n 80 echo echo "== audit_logs policy/functions around migration ==" wc -l supabase/migrations/20260726030000_admin_audit_logs_immutability.sql cat -n supabase/migrations/20260726030000_admin_audit_logs_immutability.sql | sed -n '1,140p' echo echo "== rbac helper definitions ==" cat -n supabase/migrations/20260720000006_dynamic_club_roles.sql | sed -n '1,140p' echo echo "== read-only structural check: club_admin role grants and admin checks in migrations ==" python3 - <<'PY' from pathlib import Path import re files = sorted([p for p in Path('supabase/migrations').glob('*.sql')]) for p in files: text = p.read_text(errors='ignore') entries = [] for m in re.finditer(r'(?i)role\s*=?\s*[\'\"]([^\'\"]*club_admin[^\'\"]*|[^\'\"]*admin[^\'\"]*)[\'\"]', text): snippet = text[max(0, m.start()-220):min(len(text), m.end()+220)].replace('\n',' ') entries.append((m.start(), snippet)) for m in re.finditer(r'(?i)role[[:space:]]+user_role\b.*\(', text): entries.append((m.start(), text[max(0, m.start()-80):min(len(text), m.end()+220)].replace('\n',' ').strip())) for m in re.finditer(r'(?i)(is_system_admin|is_verified_club_admin|is_club_admin|club_admin)\b', text): entries.append((m.start(), text[max(0, m.start()-250):min(len(text), m.end()+250)].replace('\n',' ').strip())) if entries: print(f"\n--- {p} ---") for pos, snippet in sorted(set(entries), key=lambda x:x[0]): for line in snippet.split('\n'): line=line.strip() if any(k in line for k in ['role', 'admin', 'club_admin', 'system_admin']): print(line) PYRepository: krushit1307/CampusConnect
Length of output: 50381
Restrict audit log access to intended admins.
If this policy is meant to grant platform-wide audit access, scope the profile role to
system_admin; currently everyclub_admincan read allaudit_logsrows, which is over-broad relative to the app’s existing club-scoped admin checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260726030000_admin_audit_logs_immutability.sql` around lines 32 - 43, The "System admins can view audit logs" policy currently grants all club_admin users access; update its profiles role condition to require system_admin instead. Preserve the policy’s authenticated SELECT behavior and existing auth.uid() profile matching.supabase/migrations/20260726050000_private_groups_rls.sql (2)
21-29: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Any user can self-join a private group with
status = 'approved', bypassing private-group protection entirely.The INSERT
WITH CHECKonly verifiesauth.uid() = user_id; it never checksgroups.is_privateor restricts thestatusvalue. Combined withstatusdefaulting to'approved'(line 26), any authenticated user who knows a private group'sidcan insert their owngroup_membersrow and instantly gain full membership — reading privategroup_posts, posting, etc. — with no approval step, even though the schema clearly models a pending/approved workflow.🔒️ Proposed fix
DROP POLICY IF EXISTS "Users can join groups" ON public.group_members; CREATE POLICY "Users can join groups" ON public.group_members FOR INSERT TO authenticated - WITH CHECK (auth.uid() = user_id); + WITH CHECK ( + auth.uid() = user_id AND + ( + status = 'pending' OR + EXISTS ( + SELECT 1 FROM public.groups g + WHERE g.id = group_id AND (g.is_private IS FALSE OR g.is_private IS NULL) + ) + ) + );Please also add a pgTAP regression test in
groups_rls.test.sqlcovering self-join to a private group. Would you like me to draft that test?Also applies to: 134-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260726050000_private_groups_rls.sql` around lines 21 - 29, The group_members INSERT policy must prevent self-joining private groups as approved. Update the policy governing group_members inserts to require public groups for self-service membership and restrict user-supplied status so private-group requests cannot bypass approval; preserve the pending/approved workflow for administrator-approved joins. Add a pgTAP regression case in groups_rls.test.sql that verifies an authenticated user cannot obtain approved membership by inserting into a private group.
166-180: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Post author can reassign
group_idto any group, including private groups they don't belong to.The UPDATE policy on
group_posts(lines 174-180) has onlyUSING, noWITH CHECK. Postgres reuses theUSINGexpression for the new row whenWITH CHECKis omitted, so the new row must satisfyauth.uid() = author_id OR is_group_admin(...). Since an author never losesauth.uid() = author_idby changinggroup_id, they can move their post into any group — even a private one they aren't a member of — bypassing the membership requirement enforced by the INSERT policy (lines 166-172).🔒️ Proposed fix
DROP POLICY IF EXISTS "Authors or group admins can update posts" ON public.group_posts; CREATE POLICY "Authors or group admins can update posts" ON public.group_posts FOR UPDATE TO authenticated USING ( auth.uid() = author_id OR public.is_group_admin(group_id, auth.uid()) - ); + ) + WITH CHECK ( + (auth.uid() = author_id AND public.is_group_member(group_id, auth.uid())) OR + public.is_group_admin(group_id, auth.uid()) + );📝 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.DROP POLICY IF EXISTS "Group members can insert posts" ON public.group_posts; CREATE POLICY "Group members can insert posts" ON public.group_posts FOR INSERT TO authenticated WITH CHECK ( auth.uid() = author_id AND public.is_group_member(group_id, auth.uid()) ); DROP POLICY IF EXISTS "Authors or group admins can update posts" ON public.group_posts; CREATE POLICY "Authors or group admins can update posts" ON public.group_posts FOR UPDATE TO authenticated USING ( auth.uid() = author_id OR public.is_group_admin(group_id, auth.uid()) ) WITH CHECK ( (auth.uid() = author_id AND public.is_group_member(group_id, auth.uid())) OR public.is_group_admin(group_id, auth.uid()) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260726050000_private_groups_rls.sql` around lines 166 - 180, Add a WITH CHECK clause to the "Authors or group admins can update posts" policy requiring the updated row’s group_id to belong to a group where auth.uid() is a member, while preserving author/admin authorization in the existing USING clause. Ensure post authors cannot reassign posts to groups they do not belong to.
2 participants
Pull Request
Description
This pull request implements Relay-style cursor-based pagination for the GraphQL events query (
events(first: $first, after: $after)):typeDefsschema with Relay connection types (Event,PageInfo,EventEdge,EventConnection). Addedevents(first, after)query definition.encodeCursoranddecodeCursorhelpers using base64 timestamps and record IDs (created_at::id). Implemented keyset database queries (created_at.lt.X,and(...)) to prevent record skipping or duplication during concurrent inserts/deletes. AddedEvent.clubandEvent.organizerfield resolvers with batch DataLoaders to prevent N+1 queries.useCursorEventsQueryhook leveraging React Query'suseInfiniteQueryto consume the new cursor-paginated API.Type of Change
Related Issue
Closes #1354
Testing
Describe the testing performed.
Tested locally
Existing functionality verified
No new warnings or errors
Added GraphQL Yoga unit test suite (src/graphql/server.test.ts) testing Relay connection structure,
edges,nodes,pageInfo, and DataLoaders.Added React Query hook unit test suite (src/hooks/useCursorEventsQuery.test.ts) testing fetcher request format, parameters, and error handling.
Screenshots
N/A (Backend GraphQL Relay Connection API & React Query Hooks)
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests