Claude/hostile code audit rbz3f8 - #590
Merged
Merged
Conversation
…astern
Both guest-facing guidebook pages worked out "what time is it for this guest"
in a hardcoded America/New_York, for every property in the country.
properties.timezone was not even in the select.
Two guest-visible consequences, both worst in the evening:
• computeStay(). Once Eastern rolls past midnight, `today >= checkoutDate`
fires and the guidebook shows CHECKOUT instructions to a guest who is
still mid-stay. One hour early in Central, two in Mountain, three in
Pacific, five in Hawaii.
• hourOfDay(). It feeds getActiveSlotTypes(), which decides which SPONSOR
SLOTS render. Sponsors pay for that placement, so the wrong hour shows the
wrong paying businesses — and the entire 5pm–8pm evening dining window on
the west coast reads as 8pm–11pm. It also drives the sky state and the
breakfast/evening recommendation bands, and on the slug page (which has no
booking) it is the sole input to the arrival-vs-mid phase.
Live, not theoretical: production is already 4 of 27 properties in
America/Chicago, and the error only grows as the product moves west.
FALLBACK_TIMEZONE stays, but only as a fallback for a property somehow
lacking one, with a comment saying it is not the default and why that
distinction cost something.
The test pins the boundary directly — 2026-08-11T04:30:00Z is 11:30pm Aug 10
in Chicago and 12:30am Aug 11 in New York — and deliberately keeps an
assertion that the SAME instant still flips under America/New_York. Without
it, a fixture that failed to straddle a date boundary would pass for the wrong
reason and prove nothing. Hawaii covers the widest US offset, so the case does
not read as a one-hour rounding curiosity.
Verified: next build exits 0 (BUILD_ID confirmed from that run), tsc clean,
3048 unit + 112 component tests pass, lint 192 at the ceiling, check:ui-classes
clean, semgrep chokepoints exit 0, ratchet vs. the real PR base exit 0, no
ratchet count increased.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
…ot move Two findings in optInGuestSms. The consent LOGIC around them is careful — STOP honoured globally by phone across every org and booking, re-consent restricted to the handset, revocation records kept forever, degraded consent reads failing closed — which is what made these two stand out. 1. The 15-minute number-correction window was measured from `opted_in_at`, which the upsert REFRESHES on every submission. So it was never "15 minutes from the opt-in" as documented; it was 15 minutes from the LAST submission, and resubmitting just inside it restarted the clock. The window could be walked forward without limit — precisely the days-later repoint the comment above it says a leaked guidebook link enables. It now anchors on `created_at`, which the upsert never names and which therefore keeps the original row's timestamp through every conflict-update. No migration needed; the immutable anchor was already there. Both existing window tests had `opted_in_at` fixtures. Worth noting that the refusal test did not simply need updating — with `created_at` absent it fell back to epoch 0 and PASSED for entirely the wrong reason, which is the shape a fixture change can hide behind. Both now set `created_at`, and a third case covers the walk-forward directly: original opt-in 24h ago, resubmitted a minute ago, repoint refused. 2. The booking lookup discarded its error. The two consent reads that follow it both fail closed with an explicit note about why; this one did not, so a transient failure was indistinguishable from a bad token and a guest holding a perfectly valid link was told the link was invalid — with nothing logged and nothing reported. supabase-error-handling baseline for this file 3 -> 2. Also logs FUTURE_REMEDIATION item 24: the opt-in stores no EVIDENCE of the consent it relies on — no disclosure text or version, no IP or user agent, no record of which token was used. Under TCPA the burden of proving prior express written consent sits with the sender, and the strongest artefact available today is a row saying a number opted in at a timestamp. Not fixed here because it needs a migration and, more to the point, storing request metadata against a phone number is a PII expansion that needs its own retention rule — a decision for whoever owns the compliance posture, not a default to pick mid-audit. Two canaries, each reverted with the edit verified before the suite ran. Verified: next build exits 0 (BUILD_ID confirmed from that run), tsc clean, 3050 unit + 112 component tests pass, lint 192 at the ceiling, check:ui-classes clean, semgrep chokepoints exit 0, ratchet vs. the real PR base exit 0, no ratchet count increased. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
…n it skips
Every guest-SMS path in the guidebook used the same one-shot claim to prevent
double-sending, and every one of them released it on only half the failures.
`sendSMS` returns `{sent:false}` ONLY for a deliberate skip — SMS_ENABLED off,
the daily nudge budget, demo-org suppression — because dispatchToTelnyx throws
on a timeout or any non-2xx. So the branch every site had written handled the
case that isn't a failure, and a real failure walked out past the release with
the claim still held. The Inngest retry then matched zero rows on
`.is('<column>', null)`, returned `already_sent`, and reported success for a
message nobody ever received.
For guidebook-guest-opted-in that is a guest who never gets their door code.
The comment above its claim already describes this exact failure for a
different case; it was still open one layer down.
The same file was inverted in the other direction too: it THREW on a
deliberate skip, so with SMS_ENABLED=false (production's current state) every
guest opt-in produced a failing, retried run reading "SMS send failed" — untrue,
and noise that would mask real failures once SMS is switched on. A retry cannot
change an env var.
- guidebook-guest-opted-in: release + rethrow on a throw; release + return
`skipped` on a deliberate skip.
- guidebook-stay-extension-handler: same for both the guest and PM-SMS claims,
via a shared releaseSendClaim() that also binds the release's own result — a
failed release left the claim held forever with nothing logged.
- lib/sms/optin-claim: new sendClaimedDailySms() owns claim -> send ->
release-on-either-exit, so the three nudge call sites (two morning, one
evening) cannot drift apart again. Rendering moves inside the claimed
section: renderSmsBody can throw too, and releasing only around sendSMS
burned the day's slot on a template failure just the same.
Also closes the silent-read family across the same feature (9 sites, both
files now at 0 in the supabase-error-handling ratchet):
- stay-extension-handler's context read discarded both errors, so `booking`
came back null, `portalUrl` was null, and the ENTIRE guest-SMS block was
skipped while the PM email went out reading "checks out on undefined" — the
step reported success either way, so Inngest never retried.
- stay-extension-cron: a failed bookings read looked like "this org has no
checkouts", a failed existence check looked like "not yet handled", and a
failed next-booking read looked like "open calendar" — each ending in a
successful `dispatched: 0`. The insert now distinguishes 23505 (another run
won the UNIQUE(booking_id) race — benign) from every other error, which
previously took the same silent `continue`.
- Both claim UPDATEs and the PM-email stamp gain `.eq('org_id', ...)`.
Tests: the opt-in test that fed a `{sent:false}` and asserted a throw encoded
the old inverted behaviour; split into the two cases the fix distinguishes.
New unit/sms/optin-claim.test.ts covers the real helper directly, since the
cron tests stub it. Every fix canary-verified by reverting it individually.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
…criptions
`guidebook_sponsors.checkout_session_id` was written on every checkout and
then never read back from anywhere in the codebase. The field exists for
exactly one purpose — the work-order invoice route has used it that way all
along ("Store the session ID for potential reuse on duplicate clicks",
app/api/invoices/[invoiceId]/checkout/route.ts) — but the sponsor path only
ever wrote it.
So every click, reload, or retry minted a fresh Stripe Checkout Session, each
payable for 24 hours. Two of them paid means two subscriptions, two
checkout.session.completed webhooks with DISTINCT event ids (so
stripe_processed_events does not collapse them), and an activation handler
that overwrites stripe_subscription_id with whichever lands last — leaving the
other subscription billing the business monthly with nothing in FieldStay able
to reach it.
- Reuse an already-open session instead of creating a second one.
- Compare-and-swap the new session id against the value just read, rather than
blind-overwriting. The reuse check covers the repeat-click case; the CAS
covers the concurrent one. The loser expires its own session and returns the
winner's, so exactly one payable session exists per sponsor. A plain
check-then-write is the TOCTOU the audit checklist calls out — the
precondition belongs in the WHERE clause.
- Refuse checkout from `payment_failed`, not just `active`. payment_failed is
set from invoice.payment_failed, which does NOT end the subscription: Stripe
keeps dunning and guidebook-sponsor-payment-recovered flips the row straight
back to active. A sponsor who saw the failure notice could buy a second
subscription while the first was still being retried. `cancelled` stays
allowed — that subscription is genuinely gone.
- Bind the CAS result. With reuse in place this write is load-bearing: a
silent failure degrades straight back to "new session every time".
Also in the activation handler:
- Report when a second, DIFFERENT subscription id replaces a live one. Our own
flow should no longer produce this, but a subscription created outside it
still can, and orphaning one must not pass unnoticed. The write proceeds.
- Throw on the guidebook-unlock upsert, matching the identical upsert in
guidebook-sponsor-payment-recovered. Discarded, a failed unlock left the
guidebook locked while the step returned `wasUnlocked: true` and the audit
row recorded an unlock that never happened.
And the media-kit-token lookup now unwraps rather than discarding its error —
a sponsor holding a valid link was told it was invalid whenever the query
itself failed. Byte-for-byte the defect already fixed in optInGuestSms, one
function over in the same file.
Verified against production first: 2 sponsors, both `pending`, zero checkouts
ever completed — so no live billing was affected.
Every fix canary-verified by reverting it individually. One existing assertion
(the tenant-scope check on guidebook_sponsors) was a positional list that a new
read would have silently weakened; restated as "every access is scoped by both
id and org_id".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
…D_RE
`/api/guidebook/redeem` is public and unauthenticated, and both ids it takes
land in `uuid` columns — `guidebook_sponsors.id` and
`bookings.guidebook_token`. Postgres does not coerce: a non-UUID string is
error 22P02, not an empty result. The route only checked `typeof === 'string'`.
- A malformed `sponsorId` reached `.eq('id', …)`, 22P02'd, threw out of
unwrap(), and the catch turned it into `{ok:true}` PLUS a reportError. On an
unauthenticated endpoint that is a free way for anyone to burn the Sentry
quota and bury this route's genuine database failures in the noise — and the
limiter bounds it only while Redis is up, since this one deliberately fails
open. A malformed id is a bad request, so it now 400s before touching the DB.
- A malformed `bookingToken` was worse than unattributed. The 22P02 escaped the
try block entirely and hit the outer catch, so the redemption INSERT never
ran — the redemption was discarded, not merely anonymous, while the guest
still saw `{ok:true}`. The route's own stated fallback three lines down is an
anonymous redemption; an unusable token now skips the booking lookup and
takes exactly that path.
The identical UUID regex had been open-coded in four files (two crew routes,
crew work-order complete, lib/storage/object-path.ts) and was simply missing
from the fifth place that needed it, so it moves to lib/validation/uuid.ts and
those four now import it. Five copies is how the sixth gets forgotten.
Test-fixture note, and the reason the second fix nearly shipped unverified:
this suite used ids like 'sponsor_1' and 'tok_abc' that could never exist in
production — against the live schema they are 22P02, not a miss. So the mock
was accepting input the database rejects, and the first version of the
malformed-token test PASSED with the fix reverted. The double now answers
22P02 for a non-UUID filtered against a uuid column, which is what Postgres
does, and both canaries bite.
guidebook_offer_redemptions has zero rows in production and nothing reads it
yet, so no collected data was affected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
Confirmed the interaction: tapping a sponsor offer opens the redemption pass —
a full-screen "Guest Perk · Verified Live" card with a live clock, captioned
"Show this screen to staff — clock proves it's live". It is a coupon presented
at the counter, and its row count is the number a paying sponsor will judge
their slot by.
Opening that pass more than once is the normal case, not an edge case: a guest
looks at the offer from the couch, closes it, walks to the business, and opens
it again to show staff. Every reopen was its own row, so COUNT(*) overstated
real redemptions by however many times the guest looked at their own coupon.
- New partial unique index on (sponsor_id, booking_id, UTC date of opened_at),
applied to production AND the E2E project with matching ledger rows.
- The route treats 23505 as the success path — already counted today. Caught
rather than expressed as upsert({onConflict}) because the index is an
EXPRESSION index and PostgREST's on_conflict only takes plain column names.
Per DAY, not per stay: a daily perk is legitimately redeemable on each day of a
booking, so collapsing a whole stay would under-count. The UTC day boundary
splits a single evening's repeat opens only when they straddle UTC midnight
(6-8pm US local) — that errs toward one extra, never toward discarding a
genuinely distinct day, which is the safe direction here.
Anonymous redemptions (booking_id NULL, from the property-level /g/[slug]
guidebook, which has no booking token) are deliberately outside the predicate:
with no guest identity there is nothing to dedupe on, and collapsing them by
(sponsor, day) would merge different guests into one.
Guardrail: the constraint and the handler branch live in different files with
nothing tying them together, and both drift directions are silent — drop the
index and the 23505 branch becomes dead code while counts quietly inflate;
drop the branch and every reopen becomes a reported error, moving the noise
from the sponsor's number into Sentry. unit/guardrails/redemption-dedup-pairing
asserts they stay together. Both directions canary-verified.
Verified zero conflicting rows in production before applying.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
…ints guidebook_sponsors.media_kit_token is a `uuid`, and all three surfaces that take one only checked `typeof === 'string'`. Postgres does not coerce, so a well-formed-but-not-UUID token is error 22P02 — and each surface turned that into the wrong answer, in opposite directions: - /g/kit/[media_kit_token] and its /print sibling already carried the note that a failed READ must not read as an invalid token, and throw to the error boundary rather than notFound() for exactly that reason. The inverse held just as strongly and was unhandled: a plainly invalid URL 22P02'd, unwrap() threw, and the sponsor got "something went wrong on our side" for a bad link. Both failures now get their own honest surface — malformed 404s before any database round trip, a real query failure still reaches the boundary. - /api/guidebook/sponsor-checkout passed the token straight through to createSponsorCheckoutSession, where it 22P02'd out of unwrap() into the action's catch — reporting to Sentry and telling the sponsor "Unable to start checkout. Please try again." for a link that will never work however many times they try. It now returns the same "Invalid media kit link." a nonexistent token gets, without touching the DB or Sentry. All three are public and unauthenticated, so the Sentry-report-per-malformed- request path was also a free way to burn the quota and bury these surfaces' genuine failures in noise. Same fixture problem as the redeem route, and worth repeating because it is a pattern: this suite used 'kit-token-abc-123', which could not have existed in production — against the live schema it is 22P02, not a miss. The new page tests use a double that answers 22P02 for a non-UUID compared to a uuid column, so they fail when the guard is removed rather than passing regardless. All three guards canary-verified individually. The media-kit pages had no test file at all before this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
Two things you asked for after the dedup landed.
── Engagement counter ───────────────────────────────────────────────────────
The per-day dedup made the redemption count honest but discarded a real second
signal: how many times the pass was actually opened. "12 redemptions, opened 31
times" says something "12 redemptions" alone does not.
guidebook_offer_redemptions.open_count, incremented on the existing row rather
than added as a second table — the dedup key IS the natural grain, so the
counter stays bounded by the same constraint instead of growing per tap, and
both numbers come out of one query: COUNT(*) redemptions, SUM(open_count)
opens.
The write moves to record_guidebook_offer_open(). It has to be a function: the
arbiter is a partial EXPRESSION index and PostgREST's on_conflict takes only
plain column names, so the upsert is not expressible through the JS client at
all — and read-then-write in the route would be a TOCTOU where two taps racing
both read 1 and both write 2. p_booking_id is DEFAULT NULL so the anonymous
case can omit it; without that the generated Args type is a required
non-nullable string and the only ways to pass null are a cast or a second
write path.
Postgres grants EXECUTE to PUBLIC by default, which on Supabase means anon
could call it over /rest/v1/rpc/ with the publishable key and write rows to a
tenant table with no session. Revoked and granted to service_role only —
verified against the live DB, not just asserted in the file. The guardrail now
covers that, the ON CONFLICT arbiter matching the index, and the route not
regressing to a bare insert (which would leave open_count pinned at 1 and
flatline engagement silently). All three canary-verified.
── Arrival reminder ─────────────────────────────────────────────────────────
The morning cron's filter is `checkin_date <= today AND checkout_date >=
today`, so a guest whose stay STARTS today is included — but it fires 7-11 AM
and check-in is typically mid-afternoon. They were getting the full nudge
("it's 72°F at your rental, here's a coffee spot 0.4 mi away") hours before
they had keys.
New arrival_reminder template, sent instead of the nudge on check-in day, via
the same claim slot so a guest still gets exactly one morning message. Placed
before the lat/lng and weather guards — an arrival reminder needs neither, and
a property with no coordinates should still be able to send one. The check-in
sentence is omitted entirely rather than left blank when checkin_time is null
(27 of 27 production properties have one, but OwnerRez-synced properties write
null explicitly).
The outgoing guest on a same-day flip is deliberately unchanged, per your
call: `checkout_date >= today` keeps them on checkout morning, when they ARE
still in the house.
formatTime12h moves out of the guest guidebook Client Component to
lib/utils/time-of-day.ts so the Inngest step can use it — these are wall-clock
values with no timezone, so they must not be run through Intl with a timeZone.
Migration applied to production and E2E. Note: MCP apply_migration assigned its
own version, and a DIFFERENT one per project (…170639 vs …170648), matching
neither the committed filename — exactly the ledger drift CLAUDE.md documents.
Both ledgers normalized to the file's version.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Contributor
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (11)
📝 WalkthroughWalkthroughThe PR adds shared UUID and time utilities, timezone-aware guidebook calculations, daily redemption aggregation, Stripe checkout concurrency protection, explicit Supabase error propagation, and centralized SMS claim handling with check-in-day arrival reminders. ChangesGuidebook validation and timezone handling
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/inngest/functions/guidebook-sponsor-activated.ts (1)
17-57: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the sponsor subscription assignment atomic.
activate-sponsor-rowreadsstripe_subscription_idbefore it updates. Two concurrentguidebook/sponsor.checkout.completedevents can both seenull, then both condition-pass and overwrite the row. The last update wins and only one duplicate subscription is reported.Store a pending activation token in
guidebook_sponsorsor use a conditional update/RPC that accepts onlynullor the expected subscription ID. When no claim is made, report the conflict and stop further processing for manual reconciliation.🤖 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 `@lib/inngest/functions/guidebook-sponsor-activated.ts` around lines 17 - 57, Make the subscription assignment in the activate-sponsor-row flow atomic instead of relying on the preceding read in existingRes. Use a conditional update or RPC that only accepts a null stripe_subscription_id or the expected subscriptionId; when no row is claimed, report the duplicate-subscription conflict and stop further processing, while preserving normal activation for successful claims.
🧹 Nitpick comments (5)
app/actions/guidebook.ts (2)
499-510: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
tryUnwrapfor consistency with the sponsor read.The booking read binds
bookingRes.errormanually, whilecreateSponsorCheckoutSessionin the same file now usesunwrap. Both fail closed, so behaviour is correct. AtryUnwrap*helper would give the same explicit error branch, keep the friendly message, and match the repository convention for Supabase reads. This is optional and applies equally to the two consent reads below.As per coding guidelines: "Use
unwrap/unwrapList/unwrapCountortryUnwrap*for Supabase reads so query errors are not mistaken for empty results".🤖 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 `@app/actions/guidebook.ts` around lines 499 - 510, Update the Supabase reads in optInGuestSms, including the booking lookup and the two consent reads, to use the repository’s tryUnwrap helper pattern instead of manually inspecting bookingRes.error. Preserve the existing fail-closed behavior, friendly error response, logging, and reportError calls.Source: Coding guidelines
53-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDistinguish a missing session from a transient Stripe failure.
openSessionUrlmaps everyretrieverejection tonull. A genuinely expired or deleted session and a transient Stripe outage or rate-limit then produce the same result. On a transient failure the caller mints a second session, and the stored one is still payable because the compare-and-swap succeeds and nothing expires it. That is the double-subscription case this helper exists to prevent.Stripe returns
status: 'expired'for an expired session rather than throwing, so the catch mostly covers transport and permission faults. Report those instead of treating them as "no session".♻️ Proposed change to separate resource_missing from transient errors
async function openSessionUrl(sessionId: string | null): Promise<string | null> { if (!sessionId) return null try { const existing = await stripe.checkout.sessions.retrieve(sessionId) return existing.status === 'open' ? existing.url : null - } catch { - // Expired or not found — the caller mints a new one. + } catch (err) { + // Not found — the caller mints a new one. Anything else is a transient + // Stripe fault: minting a second session while the stored one may still + // be payable is the double-subscription risk this helper closes. + const code = (err as { code?: string }).code + if (code !== 'resource_missing') { + reportError(err, { site: 'serverAction.guidebook.openSessionUrl' }) + } return null } }🤖 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 `@app/actions/guidebook.ts` around lines 53 - 62, Update openSessionUrl so Stripe retrieve failures are no longer converted to null: return null only when the session is absent or has a non-open status, while propagating transient or permission errors from stripe.checkout.sessions.retrieve to the caller.unit/route-handlers/guidebook-sponsor-checkout.test.ts (1)
106-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering an absent
mediaKitToken.The route types the field as
mediaKitToken?: string, soundefinedis a reachable input. The new test covers a malformed string only. A case posting{}would pin the same 400 response for a missing field and guardisUuidagainst a future non-string regression.🤖 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 `@unit/route-handlers/guidebook-sponsor-checkout.test.ts` around lines 106 - 118, Add a test alongside the malformed-token case that posts an empty payload without mediaKitToken, then assert a 400 response with { error: 'Invalid media kit link.' } and verify createSponsorCheckoutSession is not called. This should cover the optional field’s undefined path and preserve validation before reaching the action.unit/guidebook/guidebook-actions.test.ts (1)
186-210: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the compare-and-swap precondition, not only its outcome.
makeSupabasereturns the queued response regardless of which filters the chain received. This test therefore passes even if the update reverts to a blind overwrite with nocheckout_session_idpredicate, because the second queued entry is{ data: null }either way. The defect under test is precisely that missing predicate.
makeSupabasealready records every filter incalls. Assert on it. The same applies to thecs_expiredcase at lines 166-184, which should assert.eq('checkout_session_id', 'cs_expired').💚 Proposed assertion using the recorded calls
const result = await createSponsorCheckoutSession('kit_token_abc') // A plain `if (already active)` before the write is the exact TOCTOU the // audit checklist calls out — the precondition has to be in the WHERE // clause, and the loser has to clean up after itself. + // The precondition must be IN the query, so assert it was sent. + expect(supabase.calls).toContainEqual({ + table: 'guidebook_sponsors', + method: 'is', + args: ['checkout_session_id', null], + }) expect(stripe.checkout.sessions.expire).toHaveBeenCalledWith('cs_loser') expect(result).toEqual({ url: 'https://checkout.stripe.com/cs_winner' })🤖 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 `@unit/guidebook/guidebook-actions.test.ts` around lines 186 - 210, Update the CAS tests for createSponsorCheckoutSession, including the cs_expired case and the concurrent-loss case, to inspect makeSupabase’s recorded calls and assert the update chain includes an eq filter for checkout_session_id with the previously active session ID. Keep the existing outcome assertions, but verify the precondition is applied to the write rather than only relying on queued responses.unit/inngest/guidebook-sms-evening-cron.test.ts (1)
35-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
sendClaimedDailySmsmock factory casts local mocks toany. Both test files copy the same factory. In each copy,claimDailySmsSlotandreleaseDailySmsSlotare locally declaredvi.fnvalues, so theas anycasts and theeslint-disablecomments are unnecessary. The guidelines forbidas any.
unit/inngest/guidebook-sms-evening-cron.test.ts#L35-L47: call the local mocks directly, or give them explicit parameter signatures, and delete the threeeslint-disablecomments.unit/inngest/guidebook-sms-morning-cron.test.ts#L35-L47: apply the same change; consider extracting the shared factory intounit/inngest/test-helpers.tsso the two copies cannot drift.As per coding guidelines: "Use concrete TypeScript types, never
anyoras any".🤖 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 `@unit/inngest/guidebook-sms-evening-cron.test.ts` around lines 35 - 47, Remove the unnecessary any casts and eslint-disable comments in the duplicated sendClaimedDailySms mock factories: call the locally declared claimDailySmsSlot and releaseDailySmsSlot mocks directly, or provide explicit parameter signatures. Apply this in unit/inngest/guidebook-sms-evening-cron.test.ts lines 35-47 and unit/inngest/guidebook-sms-morning-cron.test.ts lines 35-47; optionally extract the shared factory into unit/inngest/test-helpers.ts to keep both implementations consistent.Source: Coding guidelines
🤖 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 `@app/api/guidebook/sponsor-checkout/route.ts`:
- Around line 35-45: Update the request parsing before the action in the sponsor
checkout route to parse the JSON body as unknown, catch malformed JSON, and
return the invalid-link response with HTTP 400. Narrow nullable or non-object
bodies with a type guard, extract an optional mediaKitToken safely, and pass the
narrowed token to isUuid and the action without dereferencing null.
In `@app/g/`[slug]/page.tsx:
- Around line 118-120: Introduce or reuse a shared helper that validates an IANA
timezone and falls back to FALLBACK_TIMEZONE when invalid, then pass its result
to every affected Intl.DateTimeFormat call: app/g/[slug]/page.tsx lines 118-120,
app/g/b/[token]/page.tsx lines 36-42, and app/g/b/[token]/page.tsx lines
154-157. Update each site to use the helper rather than passing
properties.timezone directly.
In `@lib/inngest/functions/guidebook-sponsor-activated.ts`:
- Line 6: Add `import 'server-only'` as the first import in
`lib/inngest/functions/guidebook-sponsor-activated.ts` (6-6),
`lib/inngest/functions/guidebook-stay-extension-cron.ts` (4-4), and
`lib/inngest/functions/guidebook-stay-extension-handler.ts` (3-8), before their
existing imports, to mark all three Inngest modules as server-only.
In `@lib/inngest/functions/guidebook-stay-extension-handler.ts`:
- Around line 307-313: Update the pm_notified_at stamp handling in the guidebook
stay extension handler so a stampError is thrown after logging and reporting it,
allowing step.run() to retry the failed step. Only return { notified: true }
after the stamp succeeds; do not convert a failed write into a successful
notification result.
- Around line 33-60: Update releaseSendClaim so a failed claim-release update is
propagated instead of returning successfully, ensuring callers can retry the
non-idempotent SMS or notification delivery; preserve the existing logging and
reporting. Trace the callers of releaseSendClaim and handle the propagated
failure consistently, then add coverage for an update failure that verifies the
claim is recoverable and delivery is retried rather than reported as already
sent or already notified.
In `@lib/utils/time-of-day.ts`:
- Around line 19-27: Update formatTime12h to validate the complete time string
before formatting, including hours, minutes, and optional seconds, and return
null for malformed or out-of-range values such as 25:99:00, 12:30:garbage, and
-1:00:00. Support 00:00:00–23:59:59, and explicitly handle 24:00:00 according to
the intended behavior rather than allowing it through implicitly.
In `@lib/validation/uuid.ts`:
- Around line 38-40: Update the JSDoc for isUuid to describe that it recognizes
only canonical 8-4-4-4-12 UUID strings matching UUID_RE, rather than all UUID
representations accepted by PostgreSQL; leave the implementation unchanged.
In
`@supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql`:
- Around line 35-41: Update the migration before creating
uniq_guidebook_offer_redemptions_sponsor_booking_day: add the open_count column,
identify duplicate rows by sponsor_id, booking_id, and the UTC date derived from
opened_at, aggregate their open counts into one retained row, and delete
redundant rows. Then create the unique index so historical duplicates cannot
cause index creation to fail.
In `@unit/route-handlers/guidebook-redeem.test.ts`:
- Around line 54-60: Replace the any-typed chain in the from mock with a
concrete local query-chain type declaring select, eq, maybeSingle, and insert,
and type the chain implementation accordingly. Preserve the existing fluent
return behavior and recorded eq arguments without using any or as any.
---
Outside diff comments:
In `@lib/inngest/functions/guidebook-sponsor-activated.ts`:
- Around line 17-57: Make the subscription assignment in the
activate-sponsor-row flow atomic instead of relying on the preceding read in
existingRes. Use a conditional update or RPC that only accepts a null
stripe_subscription_id or the expected subscriptionId; when no row is claimed,
report the duplicate-subscription conflict and stop further processing, while
preserving normal activation for successful claims.
---
Nitpick comments:
In `@app/actions/guidebook.ts`:
- Around line 499-510: Update the Supabase reads in optInGuestSms, including the
booking lookup and the two consent reads, to use the repository’s tryUnwrap
helper pattern instead of manually inspecting bookingRes.error. Preserve the
existing fail-closed behavior, friendly error response, logging, and reportError
calls.
- Around line 53-62: Update openSessionUrl so Stripe retrieve failures are no
longer converted to null: return null only when the session is absent or has a
non-open status, while propagating transient or permission errors from
stripe.checkout.sessions.retrieve to the caller.
In `@unit/guidebook/guidebook-actions.test.ts`:
- Around line 186-210: Update the CAS tests for createSponsorCheckoutSession,
including the cs_expired case and the concurrent-loss case, to inspect
makeSupabase’s recorded calls and assert the update chain includes an eq filter
for checkout_session_id with the previously active session ID. Keep the existing
outcome assertions, but verify the precondition is applied to the write rather
than only relying on queued responses.
In `@unit/inngest/guidebook-sms-evening-cron.test.ts`:
- Around line 35-47: Remove the unnecessary any casts and eslint-disable
comments in the duplicated sendClaimedDailySms mock factories: call the locally
declared claimDailySmsSlot and releaseDailySmsSlot mocks directly, or provide
explicit parameter signatures. Apply this in
unit/inngest/guidebook-sms-evening-cron.test.ts lines 35-47 and
unit/inngest/guidebook-sms-morning-cron.test.ts lines 35-47; optionally extract
the shared factory into unit/inngest/test-helpers.ts to keep both
implementations consistent.
In `@unit/route-handlers/guidebook-sponsor-checkout.test.ts`:
- Around line 106-118: Add a test alongside the malformed-token case that posts
an empty payload without mediaKitToken, then assert a 400 response with { error:
'Invalid media kit link.' } and verify createSponsorCheckoutSession is not
called. This should cover the optional field’s undefined path and preserve
validation before reaching the action.
🪄 Autofix
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: ba995830-4baa-4061-891d-08ca03641f20
⛔ Files ignored due to path filters (1)
types/database.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (43)
.semgrep/baseline-counts.jsonFUTURE_REMEDIATION.mdapp/actions/guidebook.tsapp/api/crew/inventory-count/route.tsapp/api/crew/messages/route.tsapp/api/crew/work-orders/[id]/complete/route.tsapp/api/guidebook/redeem/route.tsapp/api/guidebook/sponsor-checkout/route.tsapp/g/[slug]/page.tsxapp/g/b/[token]/page.tsxapp/g/kit/[media_kit_token]/page.tsxapp/g/kit/[media_kit_token]/print/page.tsxcomponents/guidebook/guest-guidebook-view.tsxlib/inngest/functions/guidebook-guest-opted-in.tslib/inngest/functions/guidebook-sms-evening-cron.tslib/inngest/functions/guidebook-sms-morning-cron.tslib/inngest/functions/guidebook-sponsor-activated.tslib/inngest/functions/guidebook-stay-extension-cron.tslib/inngest/functions/guidebook-stay-extension-handler.tslib/sms/optin-claim.tslib/sms/template-registry.tslib/sms/templates.tslib/storage/object-path.tslib/utils/time-of-day.tslib/validation/uuid.tssupabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sqlsupabase/migrations/20260807170000_guidebook_offer_open_count.sqltypes/database.tsunit/guardrails/n-plus-one-loops.test.tsunit/guardrails/redemption-dedup-pairing.test.tsunit/guardrails/supabase-error-handling.test.tsunit/guidebook/guest-stay-timezone.test.tsunit/guidebook/guidebook-actions.test.tsunit/inngest/guidebook-guest-opted-in.test.tsunit/inngest/guidebook-sms-evening-cron.test.tsunit/inngest/guidebook-sms-morning-cron.test.tsunit/inngest/guidebook-sponsor-activated.test.tsunit/inngest/guidebook-stay-extension-handler.test.tsunit/lib/sms-template-registry.test.tsunit/pages/media-kit-token-shape.test.tsunit/route-handlers/guidebook-redeem.test.tsunit/route-handlers/guidebook-sponsor-checkout.test.tsunit/sms/optin-claim.test.ts
Comment on lines
+35
to
+45
| // Shape-checked, not just type-checked. guidebook_sponsors.media_kit_token | ||
| // is a `uuid`, so a malformed token reaches `.eq()` as Postgres 22P02, | ||
| // throws out of the action's unwrap(), and lands in its catch — which | ||
| // reports to Sentry and tells the sponsor "Unable to start checkout. | ||
| // Please try again." for a link that will never work no matter how many | ||
| // times they try. On a public unauthenticated endpoint that is also a free | ||
| // way to burn the Sentry quota. An unusable token is an invalid link, and | ||
| // gets the same message a nonexistent one does. | ||
| if (!isUuid(body.mediaKitToken)) { | ||
| return NextResponse.json( | ||
| { error: 'mediaKitToken is required' }, | ||
| { error: 'Invalid media kit link.' }, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return 400 for null and malformed request bodies.
At Line 43, body is dereferenced without a null check. A valid JSON body of null throws before isUuid. Malformed JSON rejects req.json() before validation. Both paths reach the catch block, report the error, and return 500 instead of the new invalid-link response. Parse into unknown, convert parse failures to a 400, extract an optional token, and pass the narrowed token to the action.
As per coding guidelines, “narrow unknown with a type guard; handle nullable fields explicitly.”
Suggested request parsing
- const body = await req.json() as { mediaKitToken?: string }
+ const body: unknown = await req.json().catch(() => null)
+ const mediaKitToken =
+ typeof body === 'object' && body !== null && 'mediaKitToken' in body
+ ? body.mediaKitToken
+ : undefined
- if (!isUuid(body.mediaKitToken)) {
+ if (!isUuid(mediaKitToken)) {
return NextResponse.json(
{ error: 'Invalid media kit link.' },
{ status: 400 }
)
}
- const result = await createSponsorCheckoutSession(body.mediaKitToken)
+ const result = await createSponsorCheckoutSession(mediaKitToken)📝 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.
Suggested change
| // Shape-checked, not just type-checked. guidebook_sponsors.media_kit_token | |
| // is a `uuid`, so a malformed token reaches `.eq()` as Postgres 22P02, | |
| // throws out of the action's unwrap(), and lands in its catch — which | |
| // reports to Sentry and tells the sponsor "Unable to start checkout. | |
| // Please try again." for a link that will never work no matter how many | |
| // times they try. On a public unauthenticated endpoint that is also a free | |
| // way to burn the Sentry quota. An unusable token is an invalid link, and | |
| // gets the same message a nonexistent one does. | |
| if (!isUuid(body.mediaKitToken)) { | |
| return NextResponse.json( | |
| { error: 'mediaKitToken is required' }, | |
| { error: 'Invalid media kit link.' }, | |
| const body: unknown = await req.json().catch(() => null) | |
| const mediaKitToken = | |
| typeof body === 'object' && body !== null && 'mediaKitToken' in body | |
| ? body.mediaKitToken | |
| : undefined | |
| // Shape-checked, not just type-checked. guidebook_sponsors.media_kit_token | |
| // is a `uuid`, so a malformed token reaches `.eq()` as Postgres 22P02, | |
| // throws out of the action's unwrap(), and lands in its catch — which | |
| // reports to Sentry and tells the sponsor "Unable to start checkout. | |
| // Please try again." for a link that will never work no matter how many | |
| // times they try. On a public unauthenticated endpoint that is also a free | |
| // way to burn the Sentry quota. An unusable token is an invalid link, and | |
| // gets the same message a nonexistent one does. | |
| if (!isUuid(mediaKitToken)) { | |
| return NextResponse.json( | |
| { error: 'Invalid media kit link.' }, | |
| { status: 400 } | |
| ) | |
| } | |
| const result = await createSponsorCheckoutSession(mediaKitToken) |
🤖 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 `@app/api/guidebook/sponsor-checkout/route.ts` around lines 35 - 45, Update the
request parsing before the action in the sponsor checkout route to parse the
JSON body as unknown, catch malformed JSON, and return the invalid-link response
with HTTP 400. Narrow nullable or non-object bodies with a type guard, extract
an optional mediaKitToken safely, and pass the narrowed token to isUuid and the
action without dereferencing null.
Source: Coding guidelines
Comment on lines
+118
to
+120
| new Intl.DateTimeFormat('en-US', { | ||
| hour: 'numeric', hour12: false, timeZone: property.timezone || FALLBACK_TIMEZONE, | ||
| }).format(new Date()) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: a database constraint or one validated writer path for timezone values.
rg -n -C 3 'timezone|time_zone' \
supabase/migrations types/database.ts app lib unit || trueRepository: smj1860/fieldstay
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate pages =="
for f in app/g/\[slug\]/page.tsx app/g/b/\[token\]/page.tsx; do
echo "--- $f"
wc -l "$f"
sed -n '1,230p' "$f" | nl -ba
done
echo "== Timezone helper =="
wc -l lib/utils/timezone.ts
sed -n '1,190p' lib/utils/timezone.ts | nl -ba
echo "== Writer/usages near upsert / normalize / verification =="
sed -n '1,170p' lib/properties/normalize.ts | nl -ba
sed -n '1,150p' lib/properties/upsert-normalized.ts | nl -ba
sed -n '1,90p' lib/tenancy/verify.ts | nl -ba
echo "== Intl invalid tz behavior probe =="
node - <<'JS'
const vals = ['America/Chicago', null, '', 'XYZ', '-0500'];
for (const tz of vals) {
const tzOrFallback = tz || 'America/New_York';
console.log(JSON.stringify({ tz, tzOrFallback }), ':');
try {
console.log(' Intl DateTimeFormat', new Intl.DateTimeFormat('en-US', { timeZone: tzOrFallback, timeZoneName: 'long' }).format(new Date()));
} catch (e) {
console.log(' ERROR', e.name + ': ' + e.message);
}
}
JS
echo "== Exact timezone column assignments in migrations excluding comments =="
rg -n "CREATE|ALTER TABLE.*properties|timezone\s*TEXT|timezone\s*VARCHAR|SET\sDEFAULT|CHECK|properties\.timezone|properties\.(timezone)" supabase/migrations --glob '*.sql' || trueRepository: smj1860/fieldstay
Length of output: 269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate pages first 220 lines =="
for f in 'app/g/[slug]/page.tsx' 'app/g/b/[token]/page.tsx'; do
echo "--- $f"
wc -l "$f"
cat -n "$f" | sed -n '1,220p'
done
echo "== Timezone helper first 210 lines =="
wc -l lib/utils/timezone.ts
cat -n lib/utils/timezone.ts | sed -n '1,210p'
echo "== Writer/usages normalized timezone first 200 lines =="
for f in lib/properties/normalize.ts lib/properties/upsert-normalized.ts lib/tenancy/verify.ts; do
echo "--- $f"
wc -l "$f"
cat -n "$f" | sed -n '1,180p'
done
echo "== Intl invalid tz behavior probe =="
node - <<'JS'
const vals = ['America/Chicago', null, '', 'XYZ', '-0500'];
for (const tz of vals) {
const tzOrFallback = tz || 'America/New_ York';
const tzOrFallbackFix = tz || 'America/New_York';
console.log(JSON.stringify({ tz, tzOrFallbackFix, locale: 'en-US', options: { hour: 'numeric', hour12: false, timeZone: tzOrFallbackFix } }));
try {
const formatter = new Intl.DateTimeFormat('en-US', { hour: 'numeric', hour12: false, timeZone: tzOrFallbackFix });
console.log(' result:', formatter.format(new Date()));
} catch (e) {
console.log(' ERROR', e.name + ': ' + e.message);
}
}
console.log('computeStay fallback:');
const computeInputs = ['2026-08-11', '2026-08-11', 'XYZ'];
const today = new Intl.DateTimeFormat('en-CA', { timeZone: computeInputs[2] || 'America/New_York' }).format(new Date());
console.log(today);
JS
echo "== Exact timezone column clauses in migrations =="
rg -n -i "timezone\s*TEXT|timezone\s*VARCHAR|properties\.timezone|ALTER TABLE public\.properties|CREATE TABLE public\.properties|CHECK.*timezone|pg_timezone_names" supabase/migrations unit --glob '*.sql' unit --glob '*.ts' | sed -n '1,220p' || trueRepository: smj1860/fieldstay
Length of output: 40916
Avoid passing raw properties.timezone directly into Intl.DateTimeFormat.
Database schema only documents properties.timezone as an IANA identifier. If the value is malformed, these Intl calls throw RangeError: app/g/[slug]/page.tsx#118-L120, app/g/b/[token]/page.tsx#36-L42, and app/g/b/[token]/page.tsx#154-L157. Use a shared validation/fallback helper before each Intl call.
📍 Affects 2 files
app/g/[slug]/page.tsx#L118-L120(this comment)app/g/b/[token]/page.tsx#L36-L42app/g/b/[token]/page.tsx#L154-L157
🤖 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 `@app/g/`[slug]/page.tsx around lines 118 - 120, Introduce or reuse a shared
helper that validates an IANA timezone and falls back to FALLBACK_TIMEZONE when
invalid, then pass its result to every affected Intl.DateTimeFormat call:
app/g/[slug]/page.tsx lines 118-120, app/g/b/[token]/page.tsx lines 36-42, and
app/g/b/[token]/page.tsx lines 154-157. Update each site to use the helper
rather than passing properties.timezone directly.
Source: MCP tools
| import { createServiceClient } from '@/lib/supabase/server' | ||
| import { getActiveSponsorCount } from '@/lib/guidebook/helpers' | ||
| import { logAuditEvent } from '@/lib/audit' | ||
| import { reportError } from '@/lib/observability/report-error' |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
for f in \
"lib/inngest/functions/guidebook-sponsor-activated.ts" \
"lib/inngest/functions/guidebook-stay-extension-cron.ts" \
"lib/inngest/functions/guidebook-stay-extension-handler.ts"
do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,80p' "$f" | cat -n
else
echo "MISSING $f"
fi
done
echo "== server-only usage in lib/inngest/functions =="
rg -n "import 'server-only'|createServiceClient|SupabaseClient|supabaseUrl|supabaseKey" lib/inngest/functions -g '*.ts' || true
echo "== server-only package/readiness checks =="
if [ -f package.json ]; then
node -e "const p=require('./package.json'); console.log(JSON.stringify({next:p.dependencies?.next || p.devDependencies?.next}, null, 2))"
fiRepository: smj1860/fieldstay
Length of output: 50373
Mark these Inngest modules as server-only.
These modules run background operations and load server-side clients/implementations. Add import 'server-only' as the first import in each file.
lib/inngest/functions/guidebook-sponsor-activated.ts#L6lib/inngest/functions/guidebook-stay-extension-cron.ts#L4lib/inngest/functions/guidebook-stay-extension-handler.ts#L3
📍 Affects 3 files
lib/inngest/functions/guidebook-sponsor-activated.ts#L6-L6(this comment)lib/inngest/functions/guidebook-stay-extension-cron.ts#L4-L4lib/inngest/functions/guidebook-stay-extension-handler.ts#L3-L8
🤖 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 `@lib/inngest/functions/guidebook-sponsor-activated.ts` at line 6, Add `import
'server-only'` as the first import in
`lib/inngest/functions/guidebook-sponsor-activated.ts` (6-6),
`lib/inngest/functions/guidebook-stay-extension-cron.ts` (4-4), and
`lib/inngest/functions/guidebook-stay-extension-handler.ts` (3-8), before their
existing imports, to mark all three Inngest modules as server-only.
Source: Coding guidelines
Comment on lines
+33
to
+60
| async function releaseSendClaim( | ||
| supabase: ClaimClient, | ||
| requestId: string, | ||
| column: 'sms_sent_at' | 'pm_notified_at', | ||
| orgId: string, | ||
| ): Promise<void> { | ||
| // Spelled out rather than computed: a `{ [column]: null }` payload widens to | ||
| // an index signature the generated table types reject, and casting past that | ||
| // would also cast away the protection against naming a column that isn't | ||
| // there. | ||
| const patch = column === 'sms_sent_at' | ||
| ? { sms_sent_at: null } | ||
| : { pm_notified_at: null } | ||
|
|
||
| const { error } = await supabase | ||
| .from('stay_extension_requests') | ||
| .update(patch) | ||
| .eq('id', requestId) | ||
| .eq('org_id', orgId) | ||
|
|
||
| if (error) { | ||
| console.error(`[guidebook-stay-extension-handler] ${column} claim release failed`, error.message) | ||
| reportError(error, { | ||
| site: 'inngest.guidebook-stay-extension-handler.claim-release', | ||
| orgId, | ||
| extra: { column }, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -i 'guidebook-stay-extension-handler|stay_extension|guidebook' . || true
echo
echo "Target outline:"
ast-grep outline lib/inngest/functions/guidebook-stay-extension-handler.ts --view expanded || true
echo
echo "Target relevant lines:"
nl -ba lib/inngest/functions/guidebook-stay-extension-handler.ts | sed -n '1,220p'
echo
echo "Search claim helpers/usages:"
rg -n "releaseSendClaim|sendSMS|already_sent|already_notified|sms_sent_at|pm_notified_at|mark.*sent|claimed" lib/inngest/functions -S || true
echo
echo "DB/model references:"
rg -n "sms_sent_at|pm_notified_at|stay_extension_requests|organization_members|assigned_crew_member_id" -S --glob '!node_modules' . || trueRepository: smj1860/fieldstay
Length of output: 4162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Handler relevant source:"
python3 - <<'PY'
from pathlib import Path
p=Path('lib/inngest/functions/guidebook-stay-extension-handler.ts')
lines=p.read_text().splitlines()
for i in range(max(0,25), min(len(lines), 180)):
print(f"{i+1:5d}\t{lines[i]}")
PY
echo
echo "Search claim helpers/usages in handler:"
python3 - <<'PY'
from pathlib import Path
import re
p=Path('lib/inngest/functions/guidebook-stay-extension-handler.ts')
text=p.read_text()
for name in ['releaseSendClaim','sendSMS','already_sent','already_notified','sms_sent_at','pm_notified_at','mark.*sent','claim']:
for m in re.finditer(re.escape(name) if '\*' not in name else name, text, re.I | re.S):
pass
m=re.finditer(r'\b'+name+r'\b', text, re.I | re.S)
for mm in m:
line=text[:mm.start()].count('\n')+1
line_max='\n'.join(text[:mm.start()].splitlines()[-4:]+[text[mm.start():text.find('\n', mm.start())]])
print(f"-- {name} at line {line}: {line_max.replace(chr(10), ' ')}")
PY
echo
echo "Exact function call sites:"
rg -n "releaseSendClaim|already_sent|already_notified|sendSMS|sms_sent_at|pm_notified_at|claim" lib/inngest/functions/guidebook-stay-extension-handler.ts lib/inngest/functions -S || true
echo
echo "Remaining source after call sites:"
python3 - <<'PY'
from pathlib import Path
p=Path('lib/inngest/functions/guidebook-stay-extension-handler.ts')
lines=p.read_text().splitlines()
for i in range(180, len(lines)):
print(f"{i+1:5d}\t{lines[i]}")
PY
echo
echo "Handler tests around release send claim:"
if [ -f unit/inngest/guidebook-stay-extension-handler.test.ts ]; then
sed -n '1,280p' unit/inngest/guidebook-stay-extension-handler.test.ts
else
echo "test file missing"
fi
echo
echo "Stay extension migrations/model references:"
rg -n "stay_extension_requests|sms_sent_at|pm_notified_at|is_claimed|claim" supabase/migrations lib/inngest/functions/guidebook-stay-extension-cron.ts unit/inngest/guidebook-stay-extension-cron.test.ts -S || trueRepository: smj1860/fieldstay
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Remaining handler test focused on releaseSendClaim behavior:"
python3 - <<'PY'
from pathlib import Path
p = Path('unit/inngest/guidebook-stay-extension-handler.test.ts')
for i, line in enumerate(p.read_text().splitlines(), 1):
if (i >= 1 and i <= 220) or any(s in line for s in ['releaseSendClaim', 'claim release', 'claim-release', 'failed', 'already_sent', 'already_notified', 'skip']):
print(f"{i:5d}\t{line}")
PY
echo
echo "Static verifier for releaseSendClaim control flow:"
python3 - <<'PY'
from pathlib import Path
import re
src = Path('lib/inngest/functions/guidebook-stay-extension-handler.ts').read_text()
checks = {
'helper_discards_error_result': re.search(r'if\s*\(\s*error\s*\)\s*\{[^}]*console\.error.*release.*failed[^}]*reportError[^{}]*(?:reportError[^{}]*|else[^{}]*)?\}', src, re.S),
'helper_returns_void_after_error': bool(re.search(r'async function releaseSendClaim\([^)]*\)\s*:\s*Promise<void>\s*\{[^\}]*if\s*\(\s*error\s*\)[^\{]*\{[^\}]*reportError[^\}]*\}\s*\}', src, re.S)),
'update_uses_is_is_null_in_calls': 'releaseSendClaim' in src and 'is(' in src,
'caller_rethrows_send_failure_after_releas': bool(re.search(r"catch\s*\([^)]*\)\s*\{[^\}]*releaseSendClaim[^\}]*throw\s+err", src, re.S)),
'caller_releases_claim_on_sent_false': bool(re.search(r"if\s*\(\s*!\s*result\.sent\s*\)[^\}]*releaseSendClaim", src, re.S)),
'claim_uses_is_null_guard': bool(re.search(r"\.update\(\s*\{\s*\w+_sent_at\s*:\s*new\s+Date\(\)\.toISOString\(\)\s*\}\)\s*\n\s*\.eq\('id',\s*requestId\)\s*\n\s*\.eq\('org_id',\s*orgId\)\s*\.is\('\w+_sent_at',\s*null\)", src, re.S)),
'claim_null_returns_already_sent': bool(re.search(r"if\s*\(\s*!\s*claimed\s*\)\s*return\s*\{\s*skipped:\s*'already_sent'\s*\}", src, re.S)),
}
for name, value in checks.items():
print(f"{name}: {value}")
if not checks['helper_returns_void_after_error']:
print("MISSING: return statement or equivalent early termination in helper after error branch")
PYRepository: smj1860/fieldstay
Length of output: 12688
Do not leave failed SMS claims permanent.
releaseSendClaim() logs release failures but returns successfully. When the update for sms_sent_at or pm_notified_at fails, the claim remains set, and the next retry hits the .is('<column>', null) guard, reads no row, and returns already_sent/already_notified without delivering the non-idempotent SMS. Make release failures durable and recoverable, e.g. throw or persist a lease, then add coverage that covers a failed claim-release update.
🤖 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 `@lib/inngest/functions/guidebook-stay-extension-handler.ts` around lines 33 -
60, Update releaseSendClaim so a failed claim-release update is propagated
instead of returning successfully, ensuring callers can retry the non-idempotent
SMS or notification delivery; preserve the existing logging and reporting. Trace
the callers of releaseSendClaim and handle the propagated failure consistently,
then add coverage for an update failure that verifies the claim is recoverable
and delivery is retried rather than reported as already sent or already
notified.
Comment on lines
+307
to
+313
| if (stampError) { | ||
| console.error('[guidebook-stay-extension-handler] pm_notified_at stamp failed', stampError.message) | ||
| reportError(stampError, { | ||
| site: 'inngest.guidebook-stay-extension-handler.pm-email-stamp', | ||
| orgId, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching guidebook stay extension handler:"
fd -i 'guidebook-stay-extension-handler\.ts' . || true
file="$(fd -i 'guidebook-stay-extension-handler\.ts' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo
echo "Outline:"
ast-grep outline "$file" || true
echo
echo "Relevant lines 260-340:"
sed -n '260,340p' "$file" | nl -ba -v260
echo
echo "Search for idempotency and step context:"
rg -n "pm_notified_at|resend|inngest|step|idempot|notify|Notified|notified" "$file" || true
fiRepository: smj1860/fieldstay
Length of output: 499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./lib/inngest/functions/guidebook-stay-extension-handler.ts"
echo "Line count:"
wc -l "$file"
echo
echo "Relevant lines 260-340:"
sed -n '260,340p' "$file"
echo
echo "Search for idempotency and step context in file:"
grep -nE "pm_notified_at|resend|InngestEvent|step|idempot|notify|Notified|notified|sendEmail" "$file" || true
echo
echo "Nearby function definition:"
sed -n '1,80p' "$file"
sed -n '160,230p' "$file"Repository: smj1860/fieldstay
Length of output: 12055
🌐 Web query:
Inngest createFunction step.run errors throw retry function semantics
💡 Result:
In Inngest, the step.run function acts as a checkpointed, retriable unit of work [1]. Its error handling and retry semantics are defined by the following principles: Automatic Retries When code inside a step.run throws an error, Inngest automatically catches it and triggers a retry for that specific step [2][3]. Each step.run has its own independent retry counter [4][5]. If a step is configured with a certain number of retries, that limit applies individually to that step, not as a shared pool across the entire function [4][6]. Error Handling and Exhaustion - During Retry: If a step fails, Inngest retries the operation, potentially with exponential backoff [4]. Successfully completed steps before the failed one are persisted and not re-executed, allowing the function to resume from the point of failure [2][1]. - Exhaustion: If a step exhausts all its retry attempts, it will throw an error (a StepError in many contexts) back into your main function handler [2][7][8]. At this point, the failure is no longer automatically retried, and you can catch it using standard language features like try/catch to perform custom logic, such as rollbacks or graceful degradation [2][8][3]. - Unhandled Failures: If a StepError is not caught by your code, it bubbles up to the function level, causing the entire function run to be marked as failed in the Inngest UI [8][3]. Customizing Retry Behavior - Non-Retriable Errors: You can throw a NonRetriableError to bypass remaining retries immediately, marking the step as failed without further attempts [4][7]. - Custom Delays: You can use RetryAfterError to specify a duration to wait before the next retry attempt [7]. - Attempt Tracking: The attempt count (zero-indexed) is available in the function's context, allowing you to alter logic based on how many times a step has been attempted [4][7]. In summary, step.run handles transient errors by automatically retrying until success or exhaustion, at which point it throws an error that your application must explicitly handle if you wish to prevent the function from failing entirely [2][3].
Citations:
- 1: https://www.inngest.com/docs/learn/inngest-steps
- 2: https://www.inngest.com/docs/guides/error-handling
- 3: https://github.com/inngest/website/blob/main/pages/docs/guides/error-handling.mdx
- 4: https://www.inngest.com/docs/features/inngest-functions/error-retries/retries
- 5: https://www.inngest.com/docs/reference/typescript/v4/functions/step-run
- 6: https://www.inngest.com/docs/reference/python/steps/run
- 7: https://www.inngest.com/docs/features/inngest-functions/error-retries/inngest-errors
- 8: https://www.inngest.com/docs/features/inngest-functions/error-retries/rollbacks
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search Inngest function declarations and retry settings in tracked files:"
grep -RInE "inngest\.createFunction|retries:|guidebook-stay-extension-handler" --glob '!**/node_modules/**' . || true
echo
echo "Search package Inngest version/config:"
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock bun.lockb; do
if [ -f "$f" ]; then
echo "--- $f ---"
case "$f" in
package.json) grep -E '"`@inngest/`(sdk|api)"|`@inngest/`(sdk|api)"' "$f" || true ;;
*) grep -E "`@inngest/`(sdk|api)@|\"`@inngest/`(sdk|api)\"" "$f" | head -n 40 || true ;;
esac
fi
doneRepository: smj1860/fieldstay
Length of output: 431
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search Inngest function declarations and retry settings in tracked files:"
grep -RInE "inngest\.createFunction|retries:|guidebook-stay-extension-handler" "$(git ls-files | grep -v 'node_modules/')" || true
echo
echo "Search package Inngest version/config:"
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock bun.lockb; do
if [ -f "$f" ]; then
echo "--- $f ---"
grep -Ei "`@inngest/`(sdk|api)|\"`@inngest/`(sdk|api)\"" "$f" | head -n 60 || true
fi
doneRepository: smj1860/fieldstay
Length of output: 50373
Throw when the PM email notification stamp fails.
step.run() treats an error as a retryable step failure, but this path converts a failed pm_notified_at update into nnotified: true. Return with { notified: true } only after the write succeeds, or throw the logged stamp error so Inngest retries the step.
🤖 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 `@lib/inngest/functions/guidebook-stay-extension-handler.ts` around lines 307 -
313, Update the pm_notified_at stamp handling in the guidebook stay extension
handler so a stampError is thrown after logging and reporting it, allowing
step.run() to retry the failed step. Only return { notified: true } after the
stamp succeeds; do not convert a failed write into a successful notification
result.
Comment on lines
+19
to
+27
| export function formatTime12h(time: string | null | undefined): string | null { | ||
| if (!time) return null | ||
| const [hourStr, minuteStr] = time.split(':') | ||
| const hour = Number(hourStr) | ||
| const minute = Number(minuteStr) | ||
| if (Number.isNaN(hour) || Number.isNaN(minute)) return null | ||
| const period = hour >= 12 ? 'PM' : 'AM' | ||
| const displayHour = hour % 12 === 0 ? 12 : hour % 12 | ||
| return `${displayHour}:${minute.toString().padStart(2, '0')} ${period}` |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'time-of-day\.ts$' . || true
echo "== file content =="
if [ -f lib/utils/time-of-day.ts ]; then
cat -n lib/utils/time-of-day.ts
fi
echo "== usages =="
rg -n "formatTime12h|parse.*time|time-of-day" -S . --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo "== postgresql 24:00:00 docs via web =="
python3 - <<'PY'
import urllib.request
for url in [
"https://www.postgresql.org/docs/current/datatype-datetime.html",
"https://www.postgresql.org/docs/16/datatype-datetime.html",
]:
try:
with urllib.request.urlopen(url, timeout=10) as r:
s=r.read(200000).decode('utf-8', errors='ignore')
print("FOUND", url, "24:00" in s.lower())
except Exception as e:
print("ERROR", url, e)
PY
echo "== deterministic parser behavior probe =="
node - <<'JS'
function formatTime12h(time) {
if (!time) return null
const [hourStr, minuteStr] = time.split(':')
const hour = Number(hourStr)
const minute = Number(minuteStr)
if (Number.isNaN(hour) || Number.isNaN(minute)) return null
const period = hour >= 12 ? 'PM' : 'AM'
const displayHour = hour % 12 === 0 ? 12 : hour % 12
return `${displayHour}:${minute.toString().padStart(2, '0')} ${period}`
}
for (const input of ["24:00:00", "25:99:00", "12:30:garbage", "0::30", "24:00", "0:0", "12:00", "-1:00:00", "24:01"]) {
console.log(JSON.stringify(input), "=>", JSON.stringify(formatTime12h(input)))
}
JSRepository: smj1860/fieldstay
Length of output: 3902
🌐 Web query:
PostgreSQL time 24:00:00 valid
💡 Result:
Yes, '24:00:00' is a valid input for the PostgreSQL time data type [1][2][3]. PostgreSQL defines the valid range for the time data type as '00:00:00' to '24:00:00' inclusive [2][3][4]. While '24:00:00' is accepted as a valid input, any value greater than '24:00:00' (such as '24:00:00.01' or '25:00:00') will result in an "out of range" error [1]. This behavior allows for proper handling of rounding [5]. For example, a time value like '23:59:59.9999999' will be rounded up to '24:00:00' [1][5]. Note that '24:00:00' is treated as distinct from '00:00:00' by PostgreSQL; they are not equivalent in value [2].
Citations:
- 1: https://github.com/postgres/postgres/blob/207cb2ab/src/test/regress/expected/time.out
- 2: https://stackoverflow.com/questions/62262026/postgres-time-00-and-24-hours
- 3: https://www.postgresql.org/docs/current/datatype-datetime.html
- 4: https://neon.com/postgresql/tutorial/time
- 5: https://www.postgresql.org/message-id/28026.1514742632%40sss.pgh.pa.us
🌐 Web query:
PostgreSQL time without time zone 24:00:00
💡 Result:
In PostgreSQL, the time without time zone data type supports values in the range from 00:00:00 to 24:00:00 inclusive [1][2][3]. While 24:00:00 is a valid input for the time data type, it is important to note that many external applications, libraries (such as Python's datetime.time), and client drivers may not support the 24:00:00 representation, as they often expect a standard 24-hour clock range of 00:00:00 to 23:59:59.999999 [4]. If you encounter issues when retrieving this value, you can normalize it within a PostgreSQL query using a CASE expression or similar logic [4]: SELECT CASE WHEN t = '24:00:00'::time THEN '00:00:00'::time ELSE t END FROM your_table; Be aware that 24:00:00 and 00:00:00 are technically distinct representations within the database, and their treatment in calculations or conversions should be handled according to your application's requirements [5][4].
Citations:
- 1: https://www.postgresql.org/docs/18/datatype-datetime.html
- 2: https://www.postgresql.org/docs/current/datatype-datetime.html
- 3: https://www.postgresql.org/docs/12/datatype-datetime.html
- 4: https://stackoverflow.com/questions/27919727/postgresql-time-conversion-formatting-from-240000-to-000000-in-select-st
- 5: https://stackoverflow.com/questions/62262026/postgres-time-00-and-24-hours
Validate the full time string before formatting.
Reject values outside 00:00:00–23:59:59, or handle 24:00:00 explicitly, because this parser also accepts malformed inputs like 25:99:00, 12:30:garbage, and -1:00:00 and formats them as if valid.
🤖 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 `@lib/utils/time-of-day.ts` around lines 19 - 27, Update formatTime12h to
validate the complete time string before formatting, including hours, minutes,
and optional seconds, and return null for malformed or out-of-range values such
as 25:99:00, 12:30:garbage, and -1:00:00. Support 00:00:00–23:59:59, and
explicitly handle 24:00:00 according to the intended behavior rather than
allowing it through implicitly.
Source: MCP tools
Comment on lines
+38
to
+40
| /** True when `value` is a string Postgres will accept for a `uuid` column. */ | ||
| export function isUuid(value: unknown): value is string { | ||
| return typeof value === 'string' && UUID_RE.test(value) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching uuid validation:"
fd -a 'uuid\.ts$' . || true
echo
echo "Target file with line numbers:"
if [ -f lib/validation/uuid.ts ]; then
wc -l lib/validation/uuid.ts
cat -n lib/validation/uuid.ts
else
echo "lib/validation/uuid.ts not found"
fi
echo
echo "Search for UUID_RE / isUuid usages:"
rg -n "UUID_RE|isUuid" . --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo
echo "PostgreSQL uuid formats from docs in memory? No - fetch docs excerpt only if available:"
python3 - <<'PY'
import re, urllib.request, urllib.error
for url in [
"https://www.postgresql.org/docs/current/datatype-uuid.html",
]:
try:
with urllib.request.urlopen(url, timeout=20) as r:
body = r.read().decode('utf-8', errors='ignore')
print("Found URL", url, "bytes", len(body))
# Print compacted relevant paragraphs containing valid UUID textual representation examples.
for line in body.splitlines():
if "valid textual representation" in line or "canonical PostgreSQL uses" in line or "lower-case" in line or "spaces or braces" in line:
print(line.strip())
break
except Exception as e:
print("Could not fetch URL", url, str(e))
PYRepository: smj1860/fieldstay
Length of output: 6101
🌐 Web query:
PostgreSQL datatype uuid valid textual representation brace-wrapped compact alternate hyphen forms
💡 Result:
In PostgreSQL, the uuid data type stores a 128-bit Universally Unique Identifier [1]. While the database always outputs UUIDs in the standard textual representation—consisting of 32 lower-case hexadecimal digits separated by hyphens in an 8-4-4-4-12 group format—it accepts several alternative forms for input [1][2]. Acceptable input formats include: - Standard canonical form (e.g., a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11) [1]. - Upper-case hexadecimal digits [1]. - The standard format enclosed in braces (e.g., {a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11}) [1]. - Compact forms omitting some or all hyphens (e.g., a0eebc999c0b4ef8bb6d6bb9bd380a11) [1]. - Forms with hyphens added after any group of four digits (e.g., a0ee-bc99-9c0b-4ef8-bb6d-6bb9-bd38-0a11 or {a0eebc99-9c0b4ef8-bb6d6bb9-bd380a11}) [1][2]. Regardless of the input format used, PostgreSQL internally processes these into its canonical 128-bit representation and will return them in the standard 8-4-4-4-12 hyphenated format upon retrieval [1][2].
Citations:
- 1: https://www.postgresql.org/docs/current/datatype-uuid.html
- 2: https://doxygen.postgresql.org/uuid_8c.html
Fix the isUuid JSDoc contract.
UUID_RE only accepts canonical 8-4-4-4-12 UUID strings, but the JSDoc claims it accepts every string PostgreSQL accepts for uuid. PostgreSQL also accepts brace-wrapped, compact, and alternate-hyphen forms.
Suggested contract correction
-/** True when `value` is a string Postgres will accept for a `uuid` column. */
+/** True when `value` is a canonical 8-4-4-4-12 UUID string. */📝 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.
Suggested change
| /** True when `value` is a string Postgres will accept for a `uuid` column. */ | |
| export function isUuid(value: unknown): value is string { | |
| return typeof value === 'string' && UUID_RE.test(value) | |
| /** True when `value` is a canonical 8-4-4-4-12 UUID string. */ | |
| export function isUuid(value: unknown): value is string { | |
| return typeof value === 'string' && UUID_RE.test(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 `@lib/validation/uuid.ts` around lines 38 - 40, Update the JSDoc for isUuid to
describe that it recognizes only canonical 8-4-4-4-12 UUID strings matching
UUID_RE, rather than all UUID representations accepted by PostgreSQL; leave the
implementation unchanged.
Source: MCP tools
Comment on lines
+35
to
+41
| CREATE UNIQUE INDEX IF NOT EXISTS uniq_guidebook_offer_redemptions_sponsor_booking_day | ||
| ON guidebook_offer_redemptions ( | ||
| sponsor_id, | ||
| booking_id, | ||
| ((opened_at AT TIME ZONE 'UTC')::date) | ||
| ) | ||
| WHERE booking_id IS NOT NULL; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql|supabase/migrations/.*guidebook_offer_redemptions.*\.sql' || true
echo
echo "Target migration:"
if [ -f supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql ]; then
cat -n supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql
fi
echo
echo "Search for guidebook_offer_redemptions table/index definitions:"
rg -n "guidebook_offer_redemptions|uniq_guidebook_offer_redemptions_sponsor_booking_day|create unique index|unique .*guidebook_offer_redemptions" supabase/migrations || trueRepository: smj1860/fieldstay
Length of output: 6097
Deduplicate historical rows after adding open_count.
The migration creates only the unique index on (sponsor_id, booking_id, UTC date). Existing rows with the same key will make CREATE UNIQUE INDEX fail before it adds. Add open_count first, aggregate duplicate UTC-day rows into one retained row, delete the redundant rows, then create the index.
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 35-41: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 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/20260807150000_guidebook_offer_redemptions_daily_dedup.sql`
around lines 35 - 41, Update the migration before creating
uniq_guidebook_offer_redemptions_sponsor_booking_day: add the open_count column,
identify duplicate rows by sponsor_id, booking_id, and the UTC date derived from
opened_at, aggregate their open counts into one retained row, and delete
redundant rows. Then create the unique index so historical duplicates cannot
cause index creation to fail.
Comment on lines
54
to
60
| const from = vi.fn((table: string) => { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const chain: any = {} | ||
| const eqArgs: [string, unknown][] = [] | ||
| chain.select = vi.fn(() => chain) | ||
| chain.eq = vi.fn(() => chain) | ||
| chain.eq = vi.fn((col: string, val: unknown) => { eqArgs.push([col, val]); return chain }) | ||
| chain.maybeSingle = vi.fn(() => { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace any with a typed query-chain mock.
chain: any disables type checking for this test double. Define the required select, eq, maybeSingle, and insert members in a concrete local type.
As per coding guidelines: “Use concrete TypeScript types, never any or as any.”
🤖 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 `@unit/route-handlers/guidebook-redeem.test.ts` around lines 54 - 60, Replace
the any-typed chain in the from mock with a concrete local query-chain type
declaring select, eq, maybeSingle, and insert, and type the chain implementation
accordingly. Preserve the existing fluent return behavior and recorded eq
arguments without using any or as any.
Source: Coding guidelines
Both semgrep findings were mine, and both were new-vs-base because my previous
commit touched those lines. Fixed at the site, not silenced.
`guidebook-sms-morning-cron.ts` — every read in the per-guest send now unwraps
(4 -> 0 in the error-handling ratchet, entry deleted). The opt-in one is the
one that mattered: `{ data: optin }` collapsed "this guest opted out" and "the
consent read failed" into the same null, and BOTH ended at `return false` — so
a transient failure silently suppressed the guest's message with nothing
logged and no retry, while an opt-out is correctly final. The two sponsor
reads had the same shape one layer down: a failed lookup produced an empty
pool, indistinguishable from an org that simply has no sponsor in that slot.
The property read also gains `.eq('org_id', orgId)`.
`guidebook-stay-extension-cron.ts` — the per-org bookings read is now
paginated via fetchAllRows (removing it from the unbounded-select baseline
entirely). One org's checkouts on one exact date is ~properties-per-org, so
this could not realistically hit the 1000-row cap — but "realistically" was
doing the work in that sentence, and that is precisely the reasoning that left
eight platform-wide crons silently truncated until the 2026-07-30 audit.
fetchAllRows costs an extra round trip only once the set actually exceeds a
page, and removes the assumption rather than restating it.
Sonar (unit/inngest/guidebook-sponsor-activated.test.ts L105): the tenant-scope
assertion compared two `.filter(...).length` expressions inside expect(), which
toHaveLength cannot express. Restructured into named counts, which reads better
anyway — and re-canaried: dropping `.eq('org_id', ...)` from the pre-read still
fails it.
Ratchets updated in the shrink-only direction only: error-handling
morning-cron 4 -> 0, unbounded-select entry removed, semgrep
read-without-error 157 -> 153 and unbounded-select-org-scoped 98 -> 97. The
n+1 EXCEPTIONS line reference moved 87 -> 98 with the code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…indings
My previous commit renamed those two reads while unwrapping them, which made
two LATENT findings new-vs-base under a different pair of rules. I traded one
pair of findings for another rather than fixing anything — the durable answer
is to actually bound the reads, which is what this does.
All three sponsor-pool reads (two morning, one evening) now go through
fetchAllRows. guidebook_sponsors is capped at SIX rows per org by the schema
itself — slot_number CHECK 1..6 plus UNIQUE(org_id, slot_number) — so this
drains in exactly one request and the pagination costs nothing at current
scale. A .limit() would have been cheaper to write and would start truncating
silently the day that ceiling moves; fetchAllRows throws instead.
Also finishes the evening cron's opt-in and property reads, which had the same
defect as the morning twin's and which I had left inconsistent: `{ data: optin
}` collapsed "this guest opted out" and "the consent read failed" into the same
null, and both ended at `return false` — opting out is final, a failed read
must retry. Both crons now hold at 0 in the error-handling ratchet, along with
both stay-extension files.
Two shared-helper extractions fell out of typing this honestly rather than
casting past it:
- SPONSOR_POOL_COLUMNS + SponsorPoolRow (lib/sms/pick-nearest-sponsor.ts). The
reads select a SUBSET of guidebook_sponsors; asserting the result was a full
GuidebookSponsor was a cast that happened to compile, not a fact. One column
list for all three so the pools cannot drift.
- pickNearestSponsor is now generic over anything carrying lat/lng — the only
fields it reads — so a narrow SELECT no longer has to cast its way in.
- asOfferType (lib/guidebook/offer.ts) narrows offer_type, a TEXT column with a
CHECK that PostgREST returns as bare string. Same pattern and reason as
asExtensionContactMethod; unrecognised input falls back to 'none', which
formatOffer already treats as "no offer" — a sponsor line omitted rather than
rendered from a value nothing understands.
Ratchets moved shrink-only: read-without-error 153 -> 150,
unbounded-select-in-list 34 -> 32, unbounded-select-org-scoped 97 -> 96, both
SMS crons deleted from the unbounded-select baseline, evening cron 3 -> 0 in
error-handling.
Process fix: semgrep is now installed here, so I ran the exact two gates CI
runs — `--config .semgrep/ratchet.yml --baseline-commit <base> --error` and
`--config .semgrep/chokepoints.yml --error` — both clean. That is what I
should have been able to do before the last two pushes; reading the rules and
reasoning about which lines moved was not a substitute.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…rship check The Sonar finding (use a Set, not an array, for existence checks) is not worth acting on by itself — `.includes()` over five frozen strings is if anything faster than a Set, and this runs once per SMS. But it pointed at a spot with a real weakness, which I introduced two commits ago. The array had no compile-time link back to GuidebookOfferType. Add a value to the union and the array silently would not contain it, so asOfferType() would downgrade it to 'none' and every sponsor using that offer type would render no offer at all — with nothing failing anywhere. That is the nasty shape of it: the fallback that makes UNKNOWN input safe is exactly what makes a MISSING entry invisible. A forgotten member looks identical to a genuinely unrecognised value, so no runtime assertion can tell them apart. The list has to be checked by the compiler rather than by whoever remembers to update it. Now a Record<GuidebookOfferType, true>, which TypeScript checks for exhaustiveness. Verified by adding 'bogo' to the union: tsc fails at offer.ts naming the missing property. With the previous array it stayed green. The lookup Set falls out of that (Object.keys), which incidentally satisfies Sonar — but the Record is the point, not the Set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes