Feat/webhook event publisher 1862 - #1938
Conversation
github-actions
Bot
added
backend
database
ECSoC26
|
Warning Review limit reached
Next review available in: 47 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 ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds an XState event-creation wizard, outbound webhook management and delivery, static metadata caching and invalidation, nested event routing, and a reusable MultiSelect component with supporting tests and documentation. ChangesEvent wizard
Outbound webhooks
Static metadata caching
Events and MultiSelect UI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
|
@MILAN-123865 it looks like this PR has some merge conflicts; mind resolving them so we can get it merged? 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
src/pages/Events/EmptyState.tsx-11-11 (1)
11-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun Prettier on the changed JSX. These reported formatting violations will fail the lint check.
src/pages/Events/EmptyState.tsx#L11-L11: wrap the long paragraph content per Prettier.src/components/EventCard.tsx#L260-L260: reformat the conditional active-card class expression.🤖 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/pages/Events/EmptyState.tsx` at line 11, Run Prettier on the changed JSX: wrap the long paragraph in src/pages/Events/EmptyState.tsx at lines 11-11 and reformat the conditional active-card class expression in src/components/EventCard.tsx at lines 260-260 so both files pass lint.Source: Linters/SAST tools
src/pages/Events/EventsList.tsx-1030-1030 (1)
1030-1030: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSupport short-ID routes when marking the active event.
EventDetailacceptsshort_id, but list data does not select it and this check compares only UUIDs. A valid/events/:short_iddeep link therefore never highlights its card. Selectshort_idin all list queries and usee.id === eventId || e.short_id === eventId.🤖 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/pages/Events/EventsList.tsx` at line 1030, Update all event list queries used by EventsList to select each event’s short_id, then modify the active-card check near EventDetail navigation to match either e.id or e.short_id against eventId. Preserve the existing UUID matching behavior while enabling short-ID routes to highlight the correct event.src/components/MultiSelect/MultiSelectItem.tsx-15-17 (1)
15-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the stable tag identity as the cmdk value.
Tag.valueis the stable identity for selected/deselected state, but theCommand.Itemcurrently usestag.labeland the group key also usestag.value. If labels can be duplicated, cmdk can behave unpredictably for filtering or item matching. Usevalue={tag.value}and add a customCommandfilter that treatstag.labelas a search keyword.🤖 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/MultiSelect/MultiSelectItem.tsx` around lines 15 - 17, Update the Command.Item in MultiSelectItem to use tag.value for its cmdk value instead of tag.label, and configure the surrounding Command with a custom filter that searches tag.label as the user-facing keyword while preserving stable identity matching.supabase/functions/shared/cache.ts-10-16 (1)
10-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize
ResponseInit.headersbefore overlaying cache headers.
ResponseInit.headersacceptsHeadersand tuple arrays; object spreading tuple arrays turns header pairs into numeric object keys, while spreadingHeadersproduces headers likecontent-type,1rather thancontent-type,application/json. Buildnew Headers(init?.headers)first, then copy static/cache defaults on top, or letResponsenormalize before overriding.🤖 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/shared/cache.ts` around lines 10 - 16, Update createCachedResponse to normalize init?.headers with new Headers before merging them with STATIC_CACHE_HEADERS, preserving all supported ResponseInit header forms including Headers and tuple arrays. Overlay the static cache headers after normalization so they remain the effective defaults, then pass the normalized header collection to Response.docs/caching.md-9-11 (1)
9-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSpecify a language for the fenced code block.
Use
textorhttpafter the opening fence to satisfy MD040.🤖 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 `@docs/caching.md` around lines 9 - 11, Specify the fenced code block language in docs/caching.md by adding text or http to the opening fence containing the Cache-Control header, preserving the header content unchanged.Source: Linters/SAST tools
src/services/api.ts-1-7 (1)
1-7: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize base URLs consistently across the frontend and purge function.
Trailing slashes can produce double-slash URLs and cause routing or CDN cache-key mismatches.
src/services/api.ts#L1-L7: strip trailing slashes fromVITE_API_BASE_URLbefore building endpoints.supabase/migrations/20260730000000_cache_invalidation.sql#L23-L23: normalizepublic_site_urlbefore appending/api/and the table name.🤖 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/services/api.ts` around lines 1 - 7, Normalize the base URL in src/services/api.ts by removing trailing slashes from VITE_API_BASE_URL before constructing ENDPOINTS. Apply the same trailing-slash normalization to public_site_url in supabase/migrations/20260730000000_cache_invalidation.sql before appending /api/ and the table name; update both sites consistently without changing endpoint paths otherwise.docs/caching.md-32-34 (1)
32-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the actual purge provider or implement Vercel support.
The documentation claims Cloudflare/Vercel support, but
supabase/migrations/20260730000000_cache_invalidation.sqlhardcodes only the Cloudflare API.🤖 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 `@docs/caching.md` around lines 32 - 34, Update the caching documentation to name only the Cloudflare purge provider and remove the unsupported Vercel reference, unless the implementation is expanded to support Vercel as well. Keep the description of notify_cdn_purge() and its endpoint invalidation behavior accurate.src/components/EventWizard/StepIndicator.tsx-3-6 (1)
3-6: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the indicator reflect the free-event path.
Free events skip ticketing, but the static list marks it completed and displays Location as step 3. Build the displayed steps from
isPaidand passwizard.context.formData.isPaidfromEventWizard.🤖 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/EventWizard/StepIndicator.tsx` around lines 3 - 6, Update StepIndicator to accept an isPaid prop and build its displayed STEPS list conditionally, excluding "ticketing" for free events so subsequent steps and progress reflect the free-event path. Update EventWizard to pass wizard.context.formData.isPaid into StepIndicator, while preserving the existing paid-event sequence.src/components/steps/TicketingStep.tsx-28-29 (1)
28-29: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve zero and avoid storing
NaNfor an empty price.Lines 28-29 render a valid
0as blank and storeNaNwhen the field is cleared. That invalid value is persisted asnull, so a resumed wizard can contain a malformed price. Use nullish fallback and explicitly represent an empty field.Proposed fix
- value={price || ""} - onChange={(e) => updateForm({ price: parseFloat(e.target.value) })} + value={price ?? ""} + onChange={(e) => { + const value = e.currentTarget.value; + updateForm({ price: value === "" ? undefined : Number(value) }); + }}🤖 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/steps/TicketingStep.tsx` around lines 28 - 29, Update the price input in TicketingStep to use a nullish fallback so a valid 0 remains displayed, and change its onChange handler to represent an empty field explicitly instead of passing NaN to updateForm. Preserve numeric parsing for non-empty values.Source: Linters/SAST tools
src/pages/ClubSettings/Webhooks.tsx-27-40 (1)
27-40: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against out-of-order responses when
clubIdchanges mid-fetch.
loadWebhookssets state after anawaitinside auseEffectkeyed onclubIdwith no cancellation guard. If this component can re-mount/re-render for a different club before a prior fetch resolves (e.g., club switch), a stale response could overwrite the correct list.🔒️ Suggested fix
useEffect(() => { - loadWebhooks(); + let ignore = false; + (async () => { + try { + const data = await webhookService.getWebhooks(clubId); + if (!ignore) setWebhooks(data); + } catch (error) { + console.error("Error loading webhooks:", error); + } finally { + if (!ignore) setLoading(false); + } + })(); + return () => { + ignore = true; + }; }, [clubId]);🤖 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/pages/ClubSettings/Webhooks.tsx` around lines 27 - 40, Update the useEffect/loadWebhooks flow to ignore results from an obsolete clubId request after the effect is cleaned up or clubId changes. Guard setWebhooks and setLoading so only the latest request updates state, while preserving error handling and cleanup behavior.Source: Linters/SAST tools
src/components/DeliveryHistory.tsx-29-48 (1)
29-48: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against out-of-order responses when
webhook.idchanges mid-fetch.
fetchHistorysets state after anawaitwith no cancellation guard, so if the effect re-runs for a different webhook before the previous fetch resolves, the stale response can overwrite the newer one.🔒️ Suggested fix
useEffect(() => { - fetchHistory(); + let ignore = false; + (async () => { + setLoading(true); + try { + const data = await webhookService.getDeliveries(webhook.id); + if (!ignore) { + setDeliveries(data); + setError(""); + } + } catch (err: unknown) { + if (!ignore) setError(err instanceof Error ? err.message : String(err)); + } finally { + if (!ignore) setLoading(false); + } + })(); + return () => { + ignore = true; + }; }, [webhook.id]);🤖 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/DeliveryHistory.tsx` around lines 29 - 48, Update the fetchHistory/useEffect flow to ignore results from requests started for an earlier webhook.id when the effect reruns. Add an effect-scoped cancellation or active-request guard, check it before setDeliveries and setError in both success and error paths, and invalidate it during cleanup while preserving setLoading(false) only for the current request.Source: Linters/SAST tools
docs/webhooks.md-7-15 (1)
7-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument only events that are actually emitted.
The supplied trigger publishes only
event.created; no trigger is included for the other listed events. Mark them as planned or add their publishing paths before advertising them as supported.🤖 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 `@docs/webhooks.md` around lines 7 - 15, Update the “Currently supported events” section in webhooks documentation to list only event.created, since it is the sole event currently emitted. Mark post.created, club.updated, member.joined, and member.left as planned or remove them until corresponding publishing triggers exist.
🧹 Nitpick comments (9)
tests/metadataFetch.test.ts (1)
5-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore
global.fetchafter the suite.The module-level assignment can leak the mock into other tests sharing the same global environment. Capture the original implementation and restore it in
afterAll, or use scoped Vitest global stubbing.Proposed fix
-import { describe, it, expect, vi } from "vitest"; +import { afterAll, describe, it, expect, vi } from "vitest"; import { customFetch } from "../src/utils/fetch"; +const originalFetch = global.fetch; + global.fetch = vi .fn() .mockImplementation(async (url: string | URL | Request, init?: RequestInit) => { @@ }); +afterAll(() => { + global.fetch = originalFetch; +});🤖 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 `@tests/metadataFetch.test.ts` around lines 5 - 12, Update the module-level global.fetch mock in the metadata fetch test suite to preserve the original fetch implementation and restore it in an afterAll hook, or replace it with scoped Vitest global stubbing. Ensure the mock remains available for this suite while preventing changes from leaking into other tests.src/hooks/useMetadata.ts (1)
14-34: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard asynchronous state updates after unmount.
Promise.allcan settle after the component unmounts, allowingsetData,setError, orsetLoadingto commit from an obsolete effect instance. Add cleanup with an active flag, or propagate anAbortSignalthrough the metadata fetchers.Proposed fix
useEffect(() => { + let active = true; + async function loadAllMetadata() { try { setLoading(true); @@ - setData({ majors, semesters, terms, departments }); + if (active) { + setData({ majors, semesters, terms, departments }); + } } catch (err) { - setError(err instanceof Error ? err : new Error(String(err))); + if (active) { + setError(err instanceof Error ? err : new Error(String(err))); + } } finally { - setLoading(false); + if (active) { + setLoading(false); + } } } loadAllMetadata(); + return () => { + active = 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/useMetadata.ts` around lines 14 - 34, Update the useEffect metadata-loading flow and its loadAllMetadata helper to track whether the effect is still active, set the flag false in the cleanup function, and guard setData, setError, and setLoading calls so no asynchronous state update occurs after unmount.supabase/functions/majors/index.ts (1)
2-2: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the Supabase JS dependency consistently. All four functions resolve the floating
@2import at deployment time, allowing behavior and supply-chain inputs to change without a source diff.
supabase/functions/majors/index.ts#L2-L2: pin the import to the audited exact project version.supabase/functions/semesters/index.ts#L2-L2: pin the import to the same audited exact version.supabase/functions/terms/index.ts#L2-L2: pin the import to the same audited exact version.supabase/functions/departments/index.ts#L2-L2: pin the import to the same audited exact version.🤖 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/majors/index.ts` at line 2, Pin the `@supabase/supabase-js` import to the audited exact project version in supabase/functions/majors/index.ts:2-2, supabase/functions/semesters/index.ts:2-2, supabase/functions/terms/index.ts:2-2, and supabase/functions/departments/index.ts:2-2, using the same version in all four files instead of the floating `@2` specifier.src/components/WebhookForm.tsx (1)
73-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
readOnlyinstead ofdisabledfor the secret field.Disabled MUI
TextFields are excluded from the tab order and their content typically cannot be selected/copied in most browsers, which defeats the field's purpose — the admin needs to copy this value into their receiving webhook service to verify signatures.♻️ Suggested fix
<TextField label="Webhook Secret (HMAC Signature Key)" value={secret} - disabled + InputProps={{ readOnly: true }} fullWidth helperText="Used to sign requests so you can verify they came from us." />🤖 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/WebhookForm.tsx` around lines 73 - 79, Update the secret TextField in WebhookForm to use readOnly behavior instead of disabled, preserving its displayed value while allowing administrators to focus, select, and copy it.supabase/migrations/20260730171000_create_webhooks_table.sql (1)
9-10: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
updated_atis never refreshed on row updates.The column defaults to
now()at insert but nothing bumps it onUPDATE webhooks, so it will silently go stale and misrepresent the last-modified time (e.g., when a URL, secret, or subscription list changes).♻️ Suggested trigger
CREATE OR REPLACE FUNCTION set_updated_at() RETURNS TRIGGER AS $$ BEGIN NEW.updated_at = now(); RETURN NEW; END; $$ LANGUAGE plpgsql; CREATE TRIGGER webhooks_set_updated_at BEFORE UPDATE ON webhooks FOR EACH ROW EXECUTE FUNCTION set_updated_at();🤖 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/20260730171000_create_webhooks_table.sql` around lines 9 - 10, Add update-time maintenance for the webhooks.updated_at column by defining a trigger function and a BEFORE UPDATE trigger on webhooks in the migration. Ensure every row update assigns the current timestamp to NEW.updated_at, while preserving the existing insert default.supabase/migrations/20260730171001_create_webhook_deliveries.sql (1)
2-15: 🧹 Nitpick | 🔵 TrivialConsider a retention/cleanup strategy for delivery rows.
Every webhook attempt (including retries) inserts a row here with no expiry. At scale this table will grow indefinitely. Consider a scheduled job or
pg_crontask to prune oldsuccess/permanent_failurerows past a retention window.🤖 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/20260730171001_create_webhook_deliveries.sql` around lines 2 - 15, The webhook_deliveries schema lacks a retention mechanism for completed delivery rows. Add a scheduled cleanup strategy for old success and permanent_failure records, using an explicit retention window and an existing scheduling mechanism such as pg_cron where available; leave pending, processing, and failed retryable rows untouched.src/components/DeliveryHistory.tsx (1)
32-32: 🚀 Performance & Scalability | 🔵 TrivialNo pagination/limit on delivery history fetch.
getDeliveries(webhook.id)appears to fetch the entire history unbounded. As retries accumulate, this table view could grow large. Consider adding a limit/offset or "load more" pattern.🤖 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/DeliveryHistory.tsx` at line 32, Update the delivery history loading flow around getDeliveries in DeliveryHistory to fetch a bounded page using the service’s supported limit/offset parameters, and add or preserve a load-more mechanism if needed to access older records. Ensure the table renders only the currently loaded page rather than requesting the entire unbounded history.tests/webhookPublisher.test.ts (2)
1-13: 📐 Maintainability & Code Quality | 🔵 TrivialOnly the payload shape is tested — actual HTTP delivery is not.
The comment on Lines 1-2 acknowledges this is a placeholder pending mock-server coverage. Given the PR explicitly requires delivery verification "through a request-catcher endpoint, including payload and signature validation," want me to draft a
mock fetch/msw-based test exercisingpublisher.ts's actual send path (headers, signature, retry-on-failure)?🤖 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 `@tests/webhookPublisher.test.ts` around lines 1 - 13, Extend the tests beyond buildPayload to exercise publisher.ts’s actual delivery path using a mock fetch or request-catcher setup. Verify the outgoing POST payload and headers, including signature validation, and cover retry behavior after an initial failed request while preserving the existing payload-shape assertions.
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAll webhook tests import from the deprecated
deno.land/stdHTTP specifier.
deno.land/stdnow only receives critical security patches; new code should import fromjsr:@std/assert`` instead. All five webhook test files importassertEqualsfrom the same old `@0.177.0` URL specifier.
tests/webhookPublisher.test.ts#L3-L3: replace withimport { assertEquals } from "jsr:@std/assert";tests/webhookRetry.test.ts#L1-L1: replace withimport { assertEquals } from "jsr:@std/assert";tests/webhookSignature.test.ts#L1-L1: replace withimport { assertEquals } from "jsr:@std/assert";tests/webhookTrigger.test.ts#L4-L4: replace withimport { assertEquals } from "jsr:@std/assert";tests/webhookValidation.test.ts#L1-L1: replace withimport { assertEquals } from "jsr:@std/assert";🤖 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 `@tests/webhookPublisher.test.ts` at line 3, Replace the deprecated assertEquals import with the jsr:`@std/assert` specifier in tests/webhookPublisher.test.ts (lines 3-3), tests/webhookRetry.test.ts (lines 1-1), tests/webhookSignature.test.ts (lines 1-1), tests/webhookTrigger.test.ts (lines 4-4), and tests/webhookValidation.test.ts (lines 1-1), preserving the existing imported symbol.
🤖 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 `@docs/caching.md`:
- Around line 10-18: Update the Cache-Control guidance in the caching
documentation to prevent browser reuse from bypassing CDN invalidation, while
retaining the documented s-maxage behavior. Use browser revalidation such as
max-age=0, or document an equivalent versioned-URL/client invalidation
mechanism, and revise the breakdown to match the chosen policy.
In `@src/components/MultiSelect/MultiSelectPopover.tsx`:
- Around line 19-20: Update the MultiSelectPopover popover configuration by
removing the onOpenAutoFocus handler that calls preventDefault, or replace it
with logic that explicitly focuses Command.Input when the popover opens.
Preserve the existing popover width styling and ensure keyboard focus reaches
the search input.
In `@src/components/MultiSelect/MultiSelectTrigger.tsx`:
- Around line 17-19: Remove the hard-coded aria-controls attribute from the
MultiSelect trigger element, while preserving role="combobox",
aria-expanded={open}, and the existing Popover.Trigger asChild behavior so each
instance uses its generated content ID.
In `@src/components/WebhookForm.tsx`:
- Line 11: Correct the Webhook type imports to use the services path from
components: update src/components/WebhookForm.tsx lines 11-11,
src/components/WebhookList.tsx lines 6-6, and src/components/DeliveryHistory.tsx
lines 18-18 to import from "../services/webhookService" instead of
"../../services/webhookService".
In `@src/hooks/useEventWizard.ts`:
- Around line 22-25: The hydration restore path in useEventWizard must apply
both the persisted initialState.context and initialState.stateValue so the
wizard resumes at its saved step instead of restarting at basics. Update the
RESTORE dispatch and add regression coverage for resuming a persisted
location/review/ticketing flow.
In `@src/machines/eventCreationMachine.ts`:
- Around line 79-80: Update the actors configuration in eventCreationMachine to
register submitEvent with XState v5 promise-actor logic by wrapping it with
fromPromise, and import fromPromise from xstate. Preserve submitEvent’s existing
async implementation and invocation behavior.
In `@src/machines/eventMachine.types.ts`:
- Line 28: The RESTORE flow currently restores only EventContext and resets the
machine to basics, discarding the persisted stateValue. Update the RESTORE
handling and its callers to rehydrate the saved step (ticketing, location,
review, etc.) alongside context, using the persisted XState snapshot where
supported; when restoring submitting, map it to review so the submit action is
not replayed.
In `@src/services/webhookService.ts`:
- Around line 29-36: Update getWebhooks to replace select("*") with an explicit
projection containing only browser-safe webhook fields, excluding secret. Ensure
webhook creation and update secret handling uses a privileged Edge Function or
RPC, while keeping signing secrets server-only and preserving the existing error
and return behavior.
In `@supabase/functions/majors/index.ts`:
- Around line 22-26: Update the catch handlers in
supabase/functions/majors/index.ts:22-26,
supabase/functions/semesters/index.ts:22-26,
supabase/functions/terms/index.ts:22-26, and
supabase/functions/departments/index.ts:22-26 to narrow the caught error, log
its full details server-side, and return a generic 500 JSON response without
exposing backend messages or using status 400.
In `@supabase/functions/publish-webhooks/index.ts`:
- Around line 12-18: Update the request handling in the publish-webhooks serve
callback to authenticate the internal dispatch request before trusting type or
record.club_id. Require the dedicated internal credential (or validate the
caller’s club membership against record.club_id), reject missing or invalid
authorization with an appropriate unauthorized response, and only then continue
to the service-role webhook dispatch.
- Around line 41-46: Keep the publishPromises flow limited to newly triggered
webhooks, and add a scheduled worker or queue consumer that selects deliveries
whose next_retry_at is due, atomically claims each row, and republishes it with
its deliveryId. Ensure claiming prevents concurrent workers from processing the
same retry and preserves retry scheduling for failures.
In `@supabase/functions/publish-webhooks/publisher.ts`:
- Around line 74-102: The delivery audit flow must persist the real event name
and payload and surface Supabase write failures. In
supabase/functions/publish-webhooks/publisher.ts lines 74-102, update
recordDelivery and its callers to receive and store those values, and handle
insert/update errors by logging or throwing them; preserve retry metadata and
delivery timestamps. In tests/webhookDelivery.test.ts lines 1-7, replace the
placeholder with mocked insert and update assertions covering event name,
payload, retry metadata, and delivery timestamps.
In `@supabase/functions/publish-webhooks/validator.ts`:
- Around line 11-38: Extend the URL validation flow around the
disallowedHostnames and ipv4Regex checks to resolve every hostname and reject
any private, loopback, link-local, metadata, reserved, or ULA A/AAAA address,
including ranges beyond the explicitly listed literals. Before each webhook
fetch redirect, disable redirects or revalidate the next destination and its
resolved addresses so no redirect hop can reach an internal network.
In `@supabase/migrations/20260730000000_cache_invalidation.sql`:
- Around line 18-19: Update public.notify_cdn_purge() to stop reading Cloudflare
configuration from caller-controlled current_setting values; load the zone, API
token, base URL, and public site URL from Supabase Vault or another
owner-controlled store. Validate all required values together before invoking
net.http_post, and raise an explicit error when any required configuration is
missing instead of silently skipping invalidation.
In `@supabase/migrations/20260730171002_create_webhook_trigger.sql`:
- Around line 12-17: Update the webhook trigger’s net.http_post dispatch to
validate that app.settings.edge_function_url and app.settings.service_role_key
are present before constructing the request. Treat missing settings as a failure
that is surfaced or queued/alerted rather than swallowed by the WHEN OTHERS
handler, while preserving successful event inserts when dispatch settings are
configured.
In `@tests/webhookTrigger.test.ts`:
- Around line 6-8: Replace the tautological “Trigger mockup placeholder test”
with an integration test that inserts an event row and verifies the database
trigger publishes the expected event by asserting a corresponding
webhook_deliveries row or queued HTTP call. Use the project’s established pgTAP
or Supabase local-testing approach and remove the unconditional
assertEquals(true, true) placeholder.
In `@tests/webhookValidation.test.ts`:
- Around line 4-25: Extend isValidWebhookUrl to reject all IPv4 addresses in
169.254.0.0/16 and IPv6 addresses in fe80::/10 and fc00::/7, while preserving
existing SSRF and valid-URL behavior; add tests covering representative
addresses from each range. Also resolve the webhook hostname immediately before
delivery and validate every resolved IP against the same blocked ranges to
address DNS rebinding, using the existing delivery flow and validation symbols.
---
Minor comments:
In `@docs/caching.md`:
- Around line 9-11: Specify the fenced code block language in docs/caching.md by
adding text or http to the opening fence containing the Cache-Control header,
preserving the header content unchanged.
- Around line 32-34: Update the caching documentation to name only the
Cloudflare purge provider and remove the unsupported Vercel reference, unless
the implementation is expanded to support Vercel as well. Keep the description
of notify_cdn_purge() and its endpoint invalidation behavior accurate.
In `@docs/webhooks.md`:
- Around line 7-15: Update the “Currently supported events” section in webhooks
documentation to list only event.created, since it is the sole event currently
emitted. Mark post.created, club.updated, member.joined, and member.left as
planned or remove them until corresponding publishing triggers exist.
In `@src/components/DeliveryHistory.tsx`:
- Around line 29-48: Update the fetchHistory/useEffect flow to ignore results
from requests started for an earlier webhook.id when the effect reruns. Add an
effect-scoped cancellation or active-request guard, check it before
setDeliveries and setError in both success and error paths, and invalidate it
during cleanup while preserving setLoading(false) only for the current request.
In `@src/components/EventWizard/StepIndicator.tsx`:
- Around line 3-6: Update StepIndicator to accept an isPaid prop and build its
displayed STEPS list conditionally, excluding "ticketing" for free events so
subsequent steps and progress reflect the free-event path. Update EventWizard to
pass wizard.context.formData.isPaid into StepIndicator, while preserving the
existing paid-event sequence.
In `@src/components/MultiSelect/MultiSelectItem.tsx`:
- Around line 15-17: Update the Command.Item in MultiSelectItem to use tag.value
for its cmdk value instead of tag.label, and configure the surrounding Command
with a custom filter that searches tag.label as the user-facing keyword while
preserving stable identity matching.
In `@src/components/steps/TicketingStep.tsx`:
- Around line 28-29: Update the price input in TicketingStep to use a nullish
fallback so a valid 0 remains displayed, and change its onChange handler to
represent an empty field explicitly instead of passing NaN to updateForm.
Preserve numeric parsing for non-empty values.
In `@src/pages/ClubSettings/Webhooks.tsx`:
- Around line 27-40: Update the useEffect/loadWebhooks flow to ignore results
from an obsolete clubId request after the effect is cleaned up or clubId
changes. Guard setWebhooks and setLoading so only the latest request updates
state, while preserving error handling and cleanup behavior.
In `@src/pages/Events/EmptyState.tsx`:
- Line 11: Run Prettier on the changed JSX: wrap the long paragraph in
src/pages/Events/EmptyState.tsx at lines 11-11 and reformat the conditional
active-card class expression in src/components/EventCard.tsx at lines 260-260 so
both files pass lint.
In `@src/pages/Events/EventsList.tsx`:
- Line 1030: Update all event list queries used by EventsList to select each
event’s short_id, then modify the active-card check near EventDetail navigation
to match either e.id or e.short_id against eventId. Preserve the existing UUID
matching behavior while enabling short-ID routes to highlight the correct event.
In `@src/services/api.ts`:
- Around line 1-7: Normalize the base URL in src/services/api.ts by removing
trailing slashes from VITE_API_BASE_URL before constructing ENDPOINTS. Apply the
same trailing-slash normalization to public_site_url in
supabase/migrations/20260730000000_cache_invalidation.sql before appending /api/
and the table name; update both sites consistently without changing endpoint
paths otherwise.
In `@supabase/functions/shared/cache.ts`:
- Around line 10-16: Update createCachedResponse to normalize init?.headers with
new Headers before merging them with STATIC_CACHE_HEADERS, preserving all
supported ResponseInit header forms including Headers and tuple arrays. Overlay
the static cache headers after normalization so they remain the effective
defaults, then pass the normalized header collection to Response.
---
Nitpick comments:
In `@src/components/DeliveryHistory.tsx`:
- Line 32: Update the delivery history loading flow around getDeliveries in
DeliveryHistory to fetch a bounded page using the service’s supported
limit/offset parameters, and add or preserve a load-more mechanism if needed to
access older records. Ensure the table renders only the currently loaded page
rather than requesting the entire unbounded history.
In `@src/components/WebhookForm.tsx`:
- Around line 73-79: Update the secret TextField in WebhookForm to use readOnly
behavior instead of disabled, preserving its displayed value while allowing
administrators to focus, select, and copy it.
In `@src/hooks/useMetadata.ts`:
- Around line 14-34: Update the useEffect metadata-loading flow and its
loadAllMetadata helper to track whether the effect is still active, set the flag
false in the cleanup function, and guard setData, setError, and setLoading calls
so no asynchronous state update occurs after unmount.
In `@supabase/functions/majors/index.ts`:
- Line 2: Pin the `@supabase/supabase-js` import to the audited exact project
version in supabase/functions/majors/index.ts:2-2,
supabase/functions/semesters/index.ts:2-2,
supabase/functions/terms/index.ts:2-2, and
supabase/functions/departments/index.ts:2-2, using the same version in all four
files instead of the floating `@2` specifier.
In `@supabase/migrations/20260730171000_create_webhooks_table.sql`:
- Around line 9-10: Add update-time maintenance for the webhooks.updated_at
column by defining a trigger function and a BEFORE UPDATE trigger on webhooks in
the migration. Ensure every row update assigns the current timestamp to
NEW.updated_at, while preserving the existing insert default.
In `@supabase/migrations/20260730171001_create_webhook_deliveries.sql`:
- Around line 2-15: The webhook_deliveries schema lacks a retention mechanism
for completed delivery rows. Add a scheduled cleanup strategy for old success
and permanent_failure records, using an explicit retention window and an
existing scheduling mechanism such as pg_cron where available; leave pending,
processing, and failed retryable rows untouched.
In `@tests/metadataFetch.test.ts`:
- Around line 5-12: Update the module-level global.fetch mock in the metadata
fetch test suite to preserve the original fetch implementation and restore it in
an afterAll hook, or replace it with scoped Vitest global stubbing. Ensure the
mock remains available for this suite while preventing changes from leaking into
other tests.
In `@tests/webhookPublisher.test.ts`:
- Around line 1-13: Extend the tests beyond buildPayload to exercise
publisher.ts’s actual delivery path using a mock fetch or request-catcher setup.
Verify the outgoing POST payload and headers, including signature validation,
and cover retry behavior after an initial failed request while preserving the
existing payload-shape assertions.
- Line 3: Replace the deprecated assertEquals import with the jsr:`@std/assert`
specifier in tests/webhookPublisher.test.ts (lines 3-3),
tests/webhookRetry.test.ts (lines 1-1), tests/webhookSignature.test.ts (lines
1-1), tests/webhookTrigger.test.ts (lines 4-4), and
tests/webhookValidation.test.ts (lines 1-1), preserving the existing imported
symbol.
🪄 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: f0003a04-ff5a-48bb-b271-22a15edf47b3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (84)
docs/caching.mddocs/event-state-machine.mddocs/webhooks.mdpackage.jsonsrc/App.tsxsrc/components/DeliveryHistory.tsxsrc/components/EventCard.tsxsrc/components/EventWizard/EventWizard.tsxsrc/components/EventWizard/Navigation.tsxsrc/components/EventWizard/StepIndicator.tsxsrc/components/EventWizard/WizardLayout.tsxsrc/components/MultiSelect/EmptyState.tsxsrc/components/MultiSelect/LoadingState.tsxsrc/components/MultiSelect/MultiSelect.tsxsrc/components/MultiSelect/MultiSelectItem.tsxsrc/components/MultiSelect/MultiSelectList.tsxsrc/components/MultiSelect/MultiSelectPopover.tsxsrc/components/MultiSelect/MultiSelectTrigger.tsxsrc/components/MultiSelect/SelectedPill.tsxsrc/components/MultiSelect/hooks.tssrc/components/MultiSelect/index.tssrc/components/MultiSelect/types.tssrc/components/WebhookForm.tsxsrc/components/WebhookList.tsxsrc/components/steps/BasicsStep.tsxsrc/components/steps/ErrorStep.tsxsrc/components/steps/LocationStep.tsxsrc/components/steps/ReviewStep.tsxsrc/components/steps/SubmittingStep.tsxsrc/components/steps/SuccessStep.tsxsrc/components/steps/TicketingStep.tsxsrc/hooks/useEventWizard.tssrc/hooks/useMetadata.tssrc/hooks/useMultiSelect.tssrc/machines/eventCreationMachine.tssrc/machines/eventMachine.types.tssrc/machines/index.tssrc/machines/services.tssrc/pages/ClubSettings/Webhooks.tsxsrc/pages/Events/EmptyState.tsxsrc/pages/Events/EventDetail.tsxsrc/pages/Events/EventsLayout.tsxsrc/pages/Events/EventsList.tsxsrc/services/api.tssrc/services/metadata.tssrc/services/webhookService.tssrc/styles/multiselect.csssrc/tests/MultiSelect.test.tsxsrc/tests/MultiSelectAccessibility.test.tsxsrc/tests/MultiSelectKeyboard.test.tsxsrc/utils/fetch.tssrc/utils/sessionPersistence.tssrc/utils/validation.tssupabase/functions/departments/index.tssupabase/functions/majors/index.tssupabase/functions/publish-webhooks/index.tssupabase/functions/publish-webhooks/payload.tssupabase/functions/publish-webhooks/publisher.tssupabase/functions/publish-webhooks/retry.tssupabase/functions/publish-webhooks/signature.tssupabase/functions/publish-webhooks/types.tssupabase/functions/publish-webhooks/validator.tssupabase/functions/semesters/index.tssupabase/functions/shared/cache.tssupabase/functions/shared/headers.tssupabase/functions/terms/index.tssupabase/migrations/20260730000000_cache_invalidation.sqlsupabase/migrations/20260730000001_triggers.sqlsupabase/migrations/20260730171000_create_webhooks_table.sqlsupabase/migrations/20260730171001_create_webhook_deliveries.sqlsupabase/migrations/20260730171002_create_webhook_trigger.sqltests/accessibility.test.tsxtests/cacheHeaders.test.tstests/eventMachine.test.tstests/guards.test.tstests/metadataFetch.test.tstests/persistence.test.tstests/webhookDelivery.test.tstests/webhookPublisher.test.tstests/webhookRetry.test.tstests/webhookSignature.test.tstests/webhookTrigger.test.tstests/webhookValidation.test.tstests/wizardFlow.test.tsx
| Cache-Control: public, max-age=86400, s-maxage=604800, stale-while-revalidate=86400 | ||
| ``` | ||
|
|
||
| ### Breakdown: | ||
|
|
||
| - **`public`**: The response can be cached by any cache (browser, CDN). | ||
| - **`max-age=86400`**: The browser will cache the data locally for 1 day (86,400 seconds). | ||
| - **`s-maxage=604800`**: The CDN (Cloudflare/Vercel) will cache the data for 7 days. | ||
| - **`stale-while-revalidate=86400`**: The CDN will serve stale data while fetching the updated data in the background if the cache is older than 7 days, up to an additional 1 day. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Browser caching defeats the documented invalidation guarantee.
max-age=86400 allows browsers to reuse metadata for 24 hours without contacting the CDN, while notify_cdn_purge() only purges Cloudflare. After a database change, users can still receive stale metadata despite the trigger.
Use browser revalidation (for example, max-age=0 while retaining s-maxage), versioned URLs, or another client invalidation mechanism.
🤖 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 `@docs/caching.md` around lines 10 - 18, Update the Cache-Control guidance in
the caching documentation to prevent browser reuse from bypassing CDN
invalidation, while retaining the documented s-maxage behavior. Use browser
revalidation such as max-age=0, or document an equivalent versioned-URL/client
invalidation mechanism, and revise the breakdown to match the chosen policy.
| style={{ width: "var(--radix-popover-trigger-width)" }} | ||
| onOpenAutoFocus={(e) => e.preventDefault()} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)MultiSelectPopover\.tsx$|MultiSelect|cmdk|Command' || true
echo "== target file outline =="
ast-grep outline src/components/MultiSelect/MultiSelectPopover.tsx --view expanded || true
echo "== target file lines =="
cat -n src/components/MultiSelect/MultiSelectPopover.tsx | sed -n '1,220p'
echo "== command input/focus usage =="
rg -n "Command\.Input|Command\.Input|onKeyboardAssign|contentRef|ref|useRef|autoFocus|onOpenAutoFocus|Popover\.Content|Popover\.Trigger" src/components/MultiSelect -S || trueRepository: krushit1307/CampusConnect
Length of output: 3815
🌐 Web query:
Radix UI Popover Content onOpenAutoFocus preventDefault focus first tabbable element in content
💡 Result:
To prevent the default focus behavior in Radix UI Popover and instead focus a specific element (such as the first tabbable element or a custom target), you can use the onOpenAutoFocus prop on the Popover.Content component [1][2]. By calling event.preventDefault within this handler, you stop Radix UI from performing its default auto-focus logic [3][4][5]. You can then manually call.focus on your desired element [6]. Example implementation: <Popover.Content onOpenAutoFocus={(event) => { // 1. Prevent the default focus behavior event.preventDefault; // 2. Manually focus the target element // Example: Select the first button or link inside the content const firstTabbable = event.currentTarget.querySelector('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'); if (firstTabbable) { (firstTabbable as HTMLElement).focus; } }} > {/* Popover content */} </Popover.Content> Important considerations: - Accessibility: Radix UI manages focus to ensure modal or non-modal popovers remain accessible [2]. When overriding this, ensure that your chosen element is focusable and that you do not leave the popover in a state where keyboard users cannot interact with it or easily escape it [3]. - Default Behavior: Radix UI intentionally skips certain elements (like anchor tags) in its default focus logic to adhere to specific accessibility patterns [7][6]. If you need to focus an element that Radix normally ignores, this manual approach is the recommended solution [6]. - Scope: The onOpenAutoFocus prop is a direct interface to the underlying FocusScope, making it the correct place for this customization [1].
Citations:
- 1: https://github.com/radix-ui/primitives/blob/7d884d2bddf9501187be77ae1ba406b8ea15ce24/packages/react/popover/src/Popover.tsx
- 2: https://www.radix-ui.com/primitives/docs/components/popover
- 3: Prevent focus on dialog radix-ui/primitives#935
- 4: How can I make make a popover be triggered by an input? radix-ui/primitives#2705
- 5: Custom element for initial focus when opening a popover radix-ui/primitives#915
- 6: [FocusScope] Allow links/anchors to get focus radix-ui/primitives#2373
- 7: [Popover][FocusScope] Allow me to modify the default autofocus behavior of FocusScope radix-ui/primitives#3027
🌐 Web query:
Radix UI Popover ContentFocusScope tabindex default behavior onOpenAutoFocus first focusable element
💡 Result:
By default, when a Radix UI Popover opens, the FocusScope component automatically attempts to set focus to the first focusable element inside the Popover content [1][2]. Key details regarding this behavior include: 1. Automatic Focus Selection: The component identifies the first focusable element within the Popover.Content and focuses it upon mount [1][2]. Note that this internal focus-finding logic explicitly excludes certain elements (such as anchor tags) from being automatically selected as the initial focus target [3][4]. 2. Customizing Behavior: You can override or disable this default behavior using the onOpenAutoFocus prop on the Popover.Content component [5][6]. 3. Preventing Defaults: By calling event.preventDefault() inside the onOpenAutoFocus handler, you stop the default automatic focus logic [7][8][4]. This allows you to implement custom focus management, such as focusing a specific element of your choosing or preventing focus altogether if necessary [9][8][4]. Example of overriding the default focus: <Popover.Content onOpenAutoFocus={(event) => { event.preventDefault; // Execute custom focus logic here document.getElementById('your-element-id')?.focus; }} >... </Popover.Content> Because Popover.Content is rendered with tabIndex={-1} by default, it is technically focusable itself [7]; however, the FocusScope logic typically traverses inside it to find the first interactive child [1][2]. If you encounter issues with specific elements not receiving focus, the recommended approach is to use the onOpenAutoFocus prop to explicitly manage the focus state [7][4].
Citations:
- 1: https://radix-ui-primitives.mintlify.app/api/focus-scope
- 2: https://mintlify.wiki/radix-ui/primitives/api/focus-scope
- 3: [Popover][FocusScope] Allow me to modify the default autofocus behavior of FocusScope radix-ui/primitives#3027
- 4: Prevent focus on dialog radix-ui/primitives#935
- 5: https://github.com/radix-ui/primitives/blob/7d884d2bddf9501187be77ae1ba406b8ea15ce24/packages/react/popover/src/Popover.tsx
- 6: https://www.radix-ui.com/primitives/docs/components/popover
- 7: [FocusScope] Allow links/anchors to get focus radix-ui/primitives#2373
- 8: Custom element for initial focus when opening a popover radix-ui/primitives#915
- 9: How can I make make a popover be triggered by an input? radix-ui/primitives#2705
🌐 Web query:
Radix FocusScope default focusContent false onMount focus first focusable element
💡 Result:
The Radix UI FocusScope component does not have a "focusContent" prop [1][2]. By default, when a FocusScope component mounts, it automatically attempts to focus the first focusable element within its scope [1][3]. If you wish to prevent this default auto-focus behavior, you should use the onMountAutoFocus prop [1][4]. You can prevent the default behavior by calling event.preventDefault within this handler [1][4][5]: <FocusScope onMountAutoFocus={(event) => { event.preventDefault; // Optional: perform custom focus logic here }} > {/* Your content */} Internally, FocusScope manages focus by dispatching a custom event on mount [3]. If that event's default behavior is not prevented, the component executes a function that searches for the first focusable element (excluding certain types like links) and sets focus to it [3]. Because onMountAutoFocus is exposed as a prop, it is the standard and recommended way to override this behavior without needing to manipulate the internal component structure [1][5].
Citations:
- 1: https://radix-ui-primitives.mintlify.app/api/focus-scope
- 2: https://app.unpkg.com/@radix-ui/react-focus-scope@1.1.10/files/dist/index.mjs.map
- 3: https://forge.makearmy.io/makearmy/makearmy-app/raw/commit/aa8436da93e069a9d2e3a09c04f72bfebf9828f4/node_modules/@radix-ui/react-focus-scope/dist/index.mjs
- 4: https://mintlify.wiki/radix-ui/primitives/api/focus-scope
- 5: Prevent focus on dialog radix-ui/primitives#935
Do not suppress focus into the search input.
Preventing Radix’s open autofocus leaves focus on the popover content/trigger region, so keyboard users cannot type or navigate the command list after opening it. Remove this handler, or explicitly focus Command.Input.
Proposed fix
)}
style={{ width: "var(--radix-popover-trigger-width)" }}
- onOpenAutoFocus={(e) => e.preventDefault()}
>📝 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.
| style={{ width: "var(--radix-popover-trigger-width)" }} | |
| onOpenAutoFocus={(e) => e.preventDefault()} | |
| style={{ width: "var(--radix-popover-trigger-width)" }} |
🤖 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/MultiSelect/MultiSelectPopover.tsx` around lines 19 - 20,
Update the MultiSelectPopover popover configuration by removing the
onOpenAutoFocus handler that calls preventDefault, or replace it with logic that
explicitly focuses Command.Input when the popover opens. Preserve the existing
popover width styling and ensure keyboard focus reaches the search input.
| role="combobox" | ||
| aria-controls="radix-:r1:" | ||
| aria-expanded={open} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep -F 'src/components/MultiSelect/MultiSelectTrigger.tsx' || true
echo "== file snippet =="
if [ -f src/components/MultiSelect/MultiSelectTrigger.tsx ]; then
nl -ba src/components/MultiSelect/MultiSelectTrigger.tsx | sed -n '1,120p'
fi
echo "== multi-select component files =="
fd -a . src/components/MultiSelect 2>/dev/null | sed 's#^\./##'
echo "== occurrences of aria-controls or PopoverTrigger =="
rg -n "aria-controls|radix-|Popover\.Trigger|Role = 'combobox'|role=\"combobox\"" src/components/MultiSelect src || trueRepository: krushit1307/CampusConnect
Length of output: 290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file snippet =="
if [ -f src/components/MultiSelect/MultiSelectTrigger.tsx ]; then
awk '{printf "%5d\t%s\n", NR, $0}' src/components/MultiSelect/MultiSelectTrigger.tsx | sed -n '1,140p'
fi
echo "== multi-select component files =="
find src/components/MultiSelect -maxdepth 2 -type f 2>/dev/null | sort
echo "== occurrences =="
rg -n "aria-controls|radix-|Popover\.Trigger|role=\"combobox\"|role='combobox'" src || trueRepository: krushit1307/CampusConnect
Length of output: 13986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== MultiSelect files =="
for f in src/components/MultiSelect/MultiSelect.tsx src/components/MultiSelect/MultiSelectPopover.tsx src/components/MultiSelect/hooks.ts; do
echo "--- $f ---"
awk 'NR>=1 && NR<=180 {printf "%5d\t%s\n", NR, $0}' "$f"
done
echo "== package radix versions =="
if [ -f package.json ]; then
jq '.dependencies["`@radix-ui/react-popover`"], .devDependencies["`@radix-ui/react-popover`"], .dependencies["`@radix-ui/react-slot`"], .devDependencies["`@radix-ui/react-slot`"]' package.json
fiRepository: krushit1307/CampusConnect
Length of output: 4818
🌐 Web query:
Radix UI Popover Trigger asChild aria-controls Content data-radix-popover-content generated ID
💡 Result:
In Radix UI, the Popover component manages the connection between the Popover Trigger and Content using a generated ID [1]. When you use the Popover.Trigger, Radix automatically assigns an aria-controls attribute to the trigger element [1][2]. This attribute points to the generated ID of the Popover.Content [1]. This ensures that the trigger is programmatically linked to the content it controls, improving accessibility [2]. Key technical details regarding this mechanism include: 1. ID Generation: Radix uses an internal ID generation utility to ensure unique identifiers for the Popover.Content [3]. This ID is assigned to the Content component (id={context.contentId}) [1]. 2. ARIA Linkage: The Popover.Trigger is assigned aria-controls={context.contentId} [1]. As of recent updates to improve accessibility and prevent referencing non-existent elements, this aria-controls attribute is applied conditionally; it typically only appears or points to a valid ID when the popover is active or the content is present in the DOM [3][2]. 3. Composition with asChild: When you use the asChild prop on Popover.Trigger, Radix clones your child component and merges the required props—including the aria-controls attribute and event handlers—onto it [4][1]. If your custom component does not accept or spread these props, the accessibility features and functionality will fail [4]. 4. Data Attributes: The Popover.Content component is automatically assigned a data-radix-popover-content attribute (often appearing in the DOM as data-radix-popover-content or part of the internal wrapper structure) to allow for specific CSS targeting [1][5][6]. If you are encountering issues with these attributes, ensure that your custom component used with asChild correctly spreads all props and accepts a ref, as Radix relies on these to inject the necessary ARIA attributes and event listeners [4][1].
Citations:
- 1: https://github.com/radix-ui/primitives/blob/7d884d2bddf9501187be77ae1ba406b8ea15ce24/packages/react/popover/src/Popover.tsx
- 2: refactor: adjust aria-controls to triggers according to conditions radix-ui/primitives#3591
- 3: https://github.com/radix-ui/primitives/blob/main/packages/react/popover/CHANGELOG.md
- 4: https://www.radix-ui.com/primitives/docs/guides/composition
- 5: [Popper + dependents] Add wrapper data attribute radix-ui/primitives#397
- 6: Add support for popper-content-wrapper styling radix-ui/primitives#3892
Remove the hard-coded aria-controls.
Popover.Trigger asChild already assigns the generated Content ID to the trigger, so this can only make instances point at the same invalid radix-:r1: ID.
Proposed fix
ref={ref}
role="combobox"
- aria-controls="radix-:r1:"
- aria-expanded={open}
+ aria-expanded={open}
data-disabled={disabled}📝 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.
| role="combobox" | |
| aria-controls="radix-:r1:" | |
| aria-expanded={open} | |
| ref={ref} | |
| role="combobox" | |
| aria-expanded={open} | |
| data-disabled={disabled} |
🤖 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/MultiSelect/MultiSelectTrigger.tsx` around lines 17 - 19,
Remove the hard-coded aria-controls attribute from the MultiSelect trigger
element, while preserving role="combobox", aria-expanded={open}, and the
existing Popover.Trigger asChild behavior so each instance uses its generated
content ID.
| Typography, | ||
| Box, | ||
| } from "@mui/material"; | ||
| import { Webhook } from "../../services/webhookService"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm actual file locations to validate the required relative depth.
fd -HI webhookService.ts src
fd -HI 'WebhookForm.tsx|WebhookList.tsx|DeliveryHistory.tsx' src/componentsRepository: krushit1307/CampusConnect
Length of output: 291
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
base = Path("src/components")
target = Path("src/services/webhookService.ts")
candidates = [Path("WebhookForm.tsx"), Path("WebhookList.tsx"), Path("DeliveryHistory.tsx")]
for c in candidates:
from_path = base / c
try:
resolved = (from_path.parent / "../../services/webhookService").resolve()
except Exception as e:
resolved = None
import_target = (from_path.parent / "../services/webhookService").resolve()
print(f"{c}: current '../../services/webhookService' resolves={resolved}; direct '../services/webhookService' exists={import_target.exists()}; current==direct={resolved==import_target}")
current_exists = resolved.exists() if resolved else False
if current_exists and resolved == import_target:
print(" NOTE: ../../ resolves identically to ../ in this repo because there is no real git root above src/components in our sandbox resolution.")
PYRepository: krushit1307/CampusConnect
Length of output: 713
Use the correct relative import for src/services/webhookService.ts.
src/components is one level up from src/services, so these imports resolve outside the source tree and will fail module resolution.
src/components/WebhookForm.tsx#L11:import { Webhook } from "../services/webhookService";src/components/WebhookList.tsx#L6:import { Webhook } from "../services/webhookService";src/components/DeliveryHistory.tsx#L18:import { webhookService, WebhookDelivery, Webhook } from "../services/webhookService";
📍 Affects 3 files
src/components/WebhookForm.tsx#L11-L11(this comment)src/components/WebhookList.tsx#L6-L6src/components/DeliveryHistory.tsx#L18-L18
🤖 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/WebhookForm.tsx` at line 11, Correct the Webhook type imports
to use the services path from components: update src/components/WebhookForm.tsx
lines 11-11, src/components/WebhookList.tsx lines 6-6, and
src/components/DeliveryHistory.tsx lines 18-18 to import from
"../services/webhookService" instead of "../../services/webhookService".
| useEffect(() => { | ||
| if (initialState) { | ||
| send({ type: "RESTORE", context: initialState.context }); | ||
| // To strictly jump to the state, one might need a more advanced restore approach. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline src/machines/eventCreationMachine.ts --items all
rg -n -C 5 '\bRESTORE\b|stateValue|context' \
src/machines/eventCreationMachine.ts tests/persistence.test.tsRepository: krushit1307/CampusConnect
Length of output: 8879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== outline useEventWizard =="
ast-grep outline src/hooks/useEventWizard.ts --items all || true
echo "== useEventWizard relevant content =="
cat -n src/hooks/useEventWizard.ts | sed -n '1,140p'
echo "== persistence hook implementation =="
fd -a 'useEventWizard|persistence|wizard' src tests | sed 's#^\./##' | head -50
rg -n -C 8 '#|function|const.*saveWizardState|export .*saveWizardState|loadWizardState|RESTORE|stateValue|context' src hooks src/machines tests
echo "== event creation types and machine full machine section =="
cat -n src/machines/eventCreationMachine.ts | sed -n '1,230p'
cat -n src/machines/eventMachine.types.ts | sed -n '1,220p'Repository: krushit1307/CampusConnect
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tracked candidates =="
git ls-files | rg '(^|/)(useEventWizard\.ts|eventCreationMachine\.ts|EventWizard\.tsx|persistence\.test\.ts|EventWizardManager|persistence)' || true
echo "== useEventWizard =="
if [ -f src/hooks/useEventWizard.ts ]; then cat -n src/hooks/useEventWizard.ts | sed -n '1,180p'; fi
echo "== wizard implementation candidates =="
for f in $(git ls-files | rg 'EventWizard/EventWizard|EventWizardManager|EventWizardProvider' || true); do
echo "-- $f"
sed -n '1,220p' "$f" | cat -n
done
echo "== persistence candidates =="
for f in $(git ls-files | rg 'persistence|wizard' || true); do
echo "-- $f"
if rg -q 'saveWizardState|loadWizardState|stateValue|context' "$f"; then
sed -n '1,220p' "$f" | cat -n
fi
done
echo "== focused machine hooks usage =="
rg -n -C 4 'useEventWizard|restore wizard state|RESTORE|saveWizardState|loadWizardState|stateValue' src/components src/routes tests | sed -n '1,260p'Repository: krushit1307/CampusConnect
Length of output: 16680
Restore the persisted wizard step on hydration.
saveWizardState stores both stateValue and context, but restore only applies context even after saving location/review/ticketing progress (line 29-32 in tests/persistence.test.ts). This resumes the form data on basics instead of the saved step. Align the restore path to jump to initialState.stateValue as well as restoring context, and cover the resumed flow in the regression test.
🤖 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/useEventWizard.ts` around lines 22 - 25, The hydration restore path
in useEventWizard must apply both the persisted initialState.context and
initialState.stateValue so the wizard resumes at its saved step instead of
restarting at basics. Update the RESTORE dispatch and add regression coverage
for resuming a persisted location/review/ticketing flow.
| // Reject common internal ranges and localhost | ||
| const disallowedHostnames = [ | ||
| "localhost", | ||
| "127.0.0.1", | ||
| "0.0.0.0", | ||
| "169.254.169.254", // AWS metadata | ||
| "[::1]", | ||
| ]; | ||
|
|
||
| if (disallowedHostnames.includes(hostname)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Reject internal IP ranges (10.x.x.x, 172.16.x.x - 172.31.x.x, 192.168.x.x) | ||
| const ipv4Regex = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/; | ||
| const match = hostname.match(ipv4Regex); | ||
| if (match) { | ||
| const octet1 = parseInt(match[1], 10); | ||
| const octet2 = parseInt(match[2], 10); | ||
|
|
||
| if (octet1 === 10) return false; | ||
| if (octet1 === 172 && octet2 >= 16 && octet2 <= 31) return false; | ||
| if (octet1 === 192 && octet2 === 168) return false; | ||
| } | ||
|
|
||
| // TODO: Ideally resolve DNS and check resolved IP to prevent DNS rebinding | ||
|
|
||
| return true; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Complete SSRF enforcement before performing the fetch.
This permits addresses such as 127.0.0.2, 169.254.1.1, IPv6 loopback/link-local/ULA ranges, and hostnames that resolve to private addresses. A validated public URL can also redirect to an internal destination. Validate all resolved A/AAAA addresses, reject private/link-local/reserved IPv4 and IPv6 ranges, and disable or revalidate redirects at every hop.
🤖 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/publish-webhooks/validator.ts` around lines 11 - 38,
Extend the URL validation flow around the disallowedHostnames and ipv4Regex
checks to resolve every hostname and reject any private, loopback, link-local,
metadata, reserved, or ULA A/AAAA address, including ranges beyond the
explicitly listed literals. Before each webhook fetch redirect, disable
redirects or revalidate the next destination and its resolved addresses so no
redirect hop can reach an internal network.
| cdn_zone_id := current_setting('app.settings.cloudflare_zone_id', true); | ||
| cdn_api_token := current_setting('app.settings.cloudflare_api_token', true); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
: "${DATABASE_URL:?Set DATABASE_URL}"
psql "$DATABASE_URL" -v ON_ERROR_STOP=1 <<'SQL'
set app.settings.cloudflare_zone_id = 'review-zone';
select current_setting('app.settings.cloudflare_zone_id', true);
reset app.settings.cloudflare_zone_id;
SQLRepository: krushit1307/CampusConnect
Length of output: 208
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== locate migration =="
fd -a '20260730000000_cache_invalidation\.sql' . || true
echo "== migration excerpt =="
file="$(fd '20260730000000_cache_invalidation\.sql' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
cat -n "$file"
fi
echo "== search CDN settings usage =="
rg -n "cloudflare_?|current_setting\\(|app\\.settings|supabase_vault|security definer|SECURITY DEFINER|config_api|publicURL|siteUrl|base_url|cdn" supabase src 2>/dev/null | head -n 200 || trueRepository: krushit1307/CampusConnect
Length of output: 19397
🌐 Web query:
PostgreSQL SECURITY DEFINER SET current_setting missing_ok true custom GUC session override owner-controlled
💡 Result:
In PostgreSQL, the SECURITY DEFINER option on a function ensures it executes with the privileges of the function's owner [1][2]. When combined with the SET clause in a CREATE FUNCTION or CREATE PROCEDURE statement, you can enforce specific configuration parameters (GUCs) for the duration of that function's execution [1][2]. Regarding your specific interests: 1. missing_ok: The missing_ok parameter is available for the current_setting() function, which allows you to safely check for a setting's existence without triggering an error if it is undefined (it returns NULL instead) [3][4][5]. However, missing_ok is not a parameter for the SET command or the SET clause in CREATE FUNCTION [1][2][6]. The SET clause will throw an error if you attempt to set a non-existent or unrecognized GUC. 2. Custom GUCs: You can use custom GUCs (variables with a dot in their name, e.g., my_app.setting) within a SET clause [7][8][9]. PostgreSQL allows these placeholders to be set even if they were not explicitly defined at the server level, provided the user has the appropriate privileges to set variables [9]. 3. Owner-controlled/Session Override: Because SECURITY DEFINER functions execute as the owner, any SET clause attached to the function will be applied with the owner's privileges [1][2]. This effectively makes the configuration change owner-controlled. If the function also executes an explicit SET command inside its body, that command will generally override the SET clause's initial state for the remainder of the function's execution, mirroring standard PostgreSQL session behavior [1][10]. If your goal is to set a GUC only if it exists, you cannot do this declaratively via the SET clause. Instead, you would need to use a SECURITY DEFINER function that performs a check using current_setting('name', true) inside its body, and only calls set_config() if the setting is found [3][4][5].
Citations:
- 1: https://postgrespro.ru/docs/postgresql/14/sql-createprocedure?lang=en
- 2: https://pgdocs.ru/manual/sql-createfunction.html
- 3: https://www.postgresql.org/docs/current/functions-admin.html
- 4: https://www.postgresql.org/docs/devel/functions-admin.html
- 5: https://www.postgresql.org/docs/18/functions-admin.html
- 6: https://www.postgresql.org/docs/19/sql-set.html
- 7: https://www.postgresql.org/message-id/E1ZAlIK-0002UF-6L%40gemulon.postgresql.org
- 8: https://www.postgresql.org/message-id/46069B54-AAEC-479C-98EA-5861416E206D%40endpoint.com
- 9: https://www.postgresql.org/message-id/3358121.1697731221%40sss.pgh.pa.us
- 10: https://www.postgresql.org/docs/18/sql-set.html
Do not use caller-controlled GUCs as the purge configuration source.
In public.notify_cdn_purge(), current_setting(..., true) uses any session-default or caller-set value for the Cloudflare token/zone/base URL; missing_ok also makes cache invalidation silently disappear when settings are unset or causes a missing public_site_url to be skipped by the zone/token guard. Load these values from Supabase Vault or another owner-controlled store, validate the Cloudflare token/zone/public URL together before calling net.http_post, and fail explicitly when required config is missing.
🤖 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/20260730000000_cache_invalidation.sql` around lines 18 -
19, Update public.notify_cdn_purge() to stop reading Cloudflare configuration
from caller-controlled current_setting values; load the zone, API token, base
URL, and public site URL from Supabase Vault or another owner-controlled store.
Validate all required values together before invoking net.http_post, and raise
an explicit error when any required configuration is missing instead of silently
skipping invalidation.
| PERFORM net.http_post( | ||
| url := current_setting('app.settings.edge_function_url', true) || '/publish-webhooks', | ||
| headers := jsonb_build_object( | ||
| 'Content-Type', 'application/json', | ||
| 'Authorization', 'Bearer ' || current_setting('app.settings.service_role_key', true) | ||
| ), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --glob '*.sql' \
'app\.settings\.(edge_function_url|service_role_key)|ALTER\s+(DATABASE|ROLE).*SET' \
supabaseRepository: krushit1307/CampusConnect
Length of output: 481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the webhook trigger migration around the current_setting usages.
cat -n supabase/migrations/20260730171002_create_webhook_trigger.sql | sed -n '1,120p'
echo '--- all sql files mentioning current_setting ---'
rg -n --glob '*.sql' "current_setting\\(" supabase || true
echo '--- migrations that create/drop settings or alter database/service role settings ---'
rg -n --glob '*.sql' \
"CREATE\s+(EXTENSION|SCHEMA|TABLE|FUNCTION|TRIGGER|COLLATION|TYPE\W)|DROP\s+(EXTENSION|SCHEMA|TABLE|FUNCTION|TRIGGER|COLLATION|TYPE\W)|ALTER\s+(DATABASE|ROLE|SCHEMA|TABLE|FUNCTION|TRIGGER|COLLATION|TYPE\W)|INSERT\s+INTO\s+[\"']?pg_settings[\"']?|pg_settings|SET\s+(app|public|.*settings|search_path)|SHOW\s+(app|public|.*settings|search_path)" \
supabase || true
echo '--- deterministic behavior probe for current_setting(second argument true) semantics ---'
python3 - <<'PY'
# PostgreSQL current_setting(setting_name [, missing_ok]) returns null when setting is absent
# and missing_ok is true; this matches the behavior relied on by || and Authorization header.
print("When app.settings.edge_function_url is absent with true, PostgreSQL current_setting returns NULL.")
print("When app.settings.service_role_key is absent with true, PostgreSQL current_setting returns NULL.")
print("NULL || text = NULL; jsonb_build_object includes NULL value; authorization becomes 'Bearer ' or 'Bearer NULL'.")
PYRepository: krushit1307/CampusConnect
Length of output: 50381
Fail loudly when webhook dispatch settings are missing.
This trigger does not provision app.settings.edge_function_url or app.settings.service_role_key. With current_setting(..., true), an absent setting returns NULL, so the request is built with a missing endpoint/authorization and the WHEN OTHERS handler lets events inserts succeed without publishing the webhook. Add the settings as a required dependency, or check before calling net.http_post and queue/alert the event instead of swallowing the failure.
🤖 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/20260730171002_create_webhook_trigger.sql` around lines
12 - 17, Update the webhook trigger’s net.http_post dispatch to validate that
app.settings.edge_function_url and app.settings.service_role_key are present
before constructing the request. Treat missing settings as a failure that is
surfaced or queued/alerted rather than swallowed by the WHEN OTHERS handler,
while preserving successful event inserts when dispatch settings are configured.
| Deno.test("Trigger mockup placeholder test", () => { | ||
| assertEquals(true, true); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Tautological placeholder test provides no real coverage of the DB trigger.
assertEquals(true, true) always passes, so this file gives false confidence that "trigger fires event.created publishing" (a core PR objective) is actually verified. Since the trigger is central to the whole feature working end-to-end, want me to draft a pgTAP test (or a Supabase local-testing script) that inserts an event row and asserts a corresponding row appears in webhook_deliveries/an HTTP call is queued?
🤖 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 `@tests/webhookTrigger.test.ts` around lines 6 - 8, Replace the tautological
“Trigger mockup placeholder test” with an integration test that inserts an event
row and verifies the database trigger publishes the expected event by asserting
a corresponding webhook_deliveries row or queued HTTP call. Use the project’s
established pgTAP or Supabase local-testing approach and remove the
unconditional assertEquals(true, true) placeholder.
| Deno.test("isValidWebhookUrl prevents SSRF and accepts valid URLs", () => { | ||
| // Valid | ||
| assertEquals(isValidWebhookUrl("https://example.com/webhook"), true); | ||
| assertEquals(isValidWebhookUrl("https://my-api.dev/events"), true); | ||
|
|
||
| // Invalid - HTTP | ||
| assertEquals(isValidWebhookUrl("http://example.com/webhook"), false); | ||
|
|
||
| // Invalid - Localhost / Loopback | ||
| assertEquals(isValidWebhookUrl("https://localhost/webhook"), false); | ||
| assertEquals(isValidWebhookUrl("https://127.0.0.1/webhook"), false); | ||
| assertEquals(isValidWebhookUrl("https://[::1]/webhook"), false); | ||
| assertEquals(isValidWebhookUrl("https://0.0.0.0/webhook"), false); | ||
|
|
||
| // Invalid - AWS Metadata | ||
| assertEquals(isValidWebhookUrl("https://169.254.169.254/latest/meta-data/"), false); | ||
|
|
||
| // Invalid - Private IP ranges | ||
| assertEquals(isValidWebhookUrl("https://10.0.0.1/webhook"), false); | ||
| assertEquals(isValidWebhookUrl("https://192.168.1.1/webhook"), false); | ||
| assertEquals(isValidWebhookUrl("https://172.16.0.1/webhook"), false); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm current validator.ts contents to check whether these gaps still exist.
cat -n supabase/functions/publish-webhooks/validator.tsRepository: krushit1307/CampusConnect
Length of output: 1583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant webhook validation files:"
git ls-files | rg "(^|/)tests/webhookValidation\.test\.ts$|validator\.ts$|publish-webhooks"
echo
echo "validator.ts:"
cat -n supabase/functions/publish-webhooks/validator.ts 2>/dev/null || true
echo
echo "webhookValidation tests around isValidWebhookUrl:"
sed -n '1,220p' tests/webhookValidation.test.ts 2>/dev/null | cat -n || true
echo
echo "All isValidWebhookUrl usages:"
rg -n "isValidWebhookUrl|publish-webhooks|webhook" . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: krushit1307/CampusConnect
Length of output: 26473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Validator behavior probe:"
node - <<'JS'
function isValidWebhookUrl(url) {
try {
const parsedUrl = new URL(url);
if (parsedUrl.protocol !== "https:") {
return false;
}
const hostname = parsedUrl.hostname;
const disallowedHostnames = [
"localhost",
"127.0.0.1",
"0.0.0.0",
"169.254.169.254",
"[::1]",
];
if (disallowedHostnames.includes(hostname)) {
return false;
}
const ipv4Regex = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/;
const match = hostname.match(ipv4Regex);
if (match) {
const octet1 = parseInt(match[1], 10);
const octet2 = parseInt(match[2], 10);
if (octet1 === 10) return false;
if (octet1 === 172 && octet2 >= 16 && octet2 <= 31) return false;
if (octet1 === 192 && octet2 === 168) return false;
}
return true;
} catch {
return false;
}
}
const cases = [
["https://169.254.169.254/latest/meta-data/", true],
["https://169.254.1.1/webhook", true],
["https://169.254.0.1/webhook", true],
["https://[fe80::1]/webhook", true],
["https://[fc00::1]/webhook", true],
["https://[fd00::1]/webhook", true],
["https://localhost/webhook", true],
];
for (const [url, shouldBeAcceptedByCurrentLogic] of cases) {
console.log(`${JSON.stringify(url)} -> ${isValidWebhookUrl(url)}`);
}
JSRepository: krushit1307/CampusConnect
Length of output: 463
Block reserved link-local and IPv6 private ranges in isValidWebhookUrl.
isValidWebhookUrl only rejects literal 169.254.169.254 and [::1], so other addresses like https://169.254.1.1/webhook, https://[fe80::1]/webhook, or https://[fd00::1]/webhook can still be accepted and delivered to internal/link-local services. Add test coverage and implementation checks for the full 169.254.0.0/16 range plus IPv6 fe80::/10 and fc00::/7 ranges, and address the DNS-rebinding TODO by validating resolved IPs at delivery time.
🤖 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 `@tests/webhookValidation.test.ts` around lines 4 - 25, Extend
isValidWebhookUrl to reject all IPv4 addresses in 169.254.0.0/16 and IPv6
addresses in fe80::/10 and fc00::/7, while preserving existing SSRF and
valid-URL behavior; add tests covering representative addresses from each range.
Also resolve the webhook hostname immediately before delivery and validate every
resolved IP against the same blocked ranges to address DNS rebinding, using the
existing delivery flow and validation symbols.
2 participants
Pull Request
Description
Provide a brief description of the changes introduced in this pull request.
Type of Change
Related Issue
Closes #
Testing
Describe the testing performed.
Screenshots
If applicable, add screenshots of the changes.
Checklist
Related Issue
Closes #1862
Summary
Introduces a secure outbound webhook system that enables clubs to integrate CampusConnect with external services such as Discord, Slack, or custom applications. The implementation supports event subscriptions, signed payloads, delivery tracking, retry handling, and protection against SSRF attacks.
Changes
webhooksandwebhook_deliveriestables for configuration, auditing, and retry management.event.creatednotifications automatically after event creation.event.created,post.created).Benefits
Testing