Stop OwnerRez silently dropping owner revenue past 1000 bookings, and… - #617
Merged
Conversation
… default crew sync to v2
INTEGRATION SYNC AUDIT — one real finding, in OwnerRez.
Both syncs upserted the whole booking set in ONE call and built the
external_id -> id map from that call's returned rows. The WRITE is not capped;
the returned REPRESENTATION is, at max_rows = 1000, with a 200 and no
truncation signal — the same cap that applies to a read. So past 1000 bookings
the map came back short, and selectOwnerRezBookingsToPostRevenue's closing
`.filter((b) => !!b.bookingId)` silently dropped every booking missing from it.
The bookings themselves were written, so the calendar looked complete. Only
the owner_transactions REVENUE rows were absent. No error, no log, nothing in
the UI — an owner's P&L simply short by an amount that grows with portfolio
size.
initial-sync is where it bites hardest: it fetches every booking for every
property in one call, so an account with two years of history crosses 1000 on
its FIRST sync — the one run whose entire job is to get the historical ledger
right. incremental-sync has the same shape with a smaller blast radius.
Fixed by chunking the upsert so no single response can be truncated, at HALF
of max_rows rather than at it, so a future change returning more columns or a
lower server-side max_rows cannot quietly re-introduce it. A short chunk now
throws rather than returning a partial map — a partial map is
indistinguishable from success downstream, so failing loudly is the only way
this class surfaces at all. The chunk loop is numeric, which is structurally
exempt from the N+1 guardrail, and costs one round trip per 500 bookings.
Not triggered on live data (57 bookings), but this is CLAUDE.md's own stated
target scale and structurally the same defect as the inventory truncation
that silently duplicated items past property ~8.
The other two integrations are clean. iCal guards its tenant-supplied feed
URLs with safeFetch (re-validating every redirect hop) and paginates the whole
fan-out. Hospitable's one unbounded read is scoped to a single property AND a
date window; a truncated result there leaves a stale Blocked row that
self-heals on the next run.
CREW SYNC V2 — now the default, v1 retained.
The acceptance test (Phase 5b) passed, and NEXT_PUBLIC_CREW_SYNC_V2 is set in
Vercel, so v2 has been the serving path since the first build after it was
set. Inverted the client default from `=== 'true'` to `!== 'false'`, which
closes a real hole rather than just flipping a switch: absent used to mean v1,
so any environment not carrying the variable — a new preview, a rebuilt
environment, a setting lost in a project migration — silently served crew the
old path. NEXT_PUBLIC_ vars are inlined at BUILD time, so that state was
invisible in config and observable only from behaviour.
The v1 branch stays in the tree, compiling and guardrail-covered. It is the
rollback: set the variable to 'false' and rebuild, no code change. Commenting
it out was considered and rejected — it is already unreachable when the flag
is on, so commenting buys nothing while costing type-checking, lint, guardrail
coverage, and a one-variable rollback. Phase 5d deletes it after the soak.
Updated the 2026-08-09 doc note, which claimed a redeploy was still pending
and had been stale for three days, plus the Phase 5 status row.
Three guardrails caught things worth keeping:
* error-reporting-coverage flagged the two log-only catch blocks the first
version added. Removed both — the helper now carries the connection label
in its thrown message, so catching to re-log and re-throw added no context.
* supabase-error-handling flagged this file's own DOC COMMENT, which quoted
the buggy `const { … } = await supabase…` verbatim. The scan reads raw
source and cannot tell an illustration of a defect from the defect;
rewritten as prose with a note explaining why.
* n-plus-one-loops' pinned exception at initial-sync.ts:184 moved to :185
when the import landed. Repinned, along with the sync.ts entry citing it.
Canaried: unchunking the upsert fails three of the six new tests, including an
end-to-end one that reproduces the original revenue gap through
selectOwnerRezBookingsToPostRevenue rather than asserting it in a comment.
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
📝 WalkthroughWalkthroughCrew Sync v2 is now enabled by default unless disabled at build time. OwnerRez initial and incremental syncs now use chunked booking upserts with complete ID mappings and explicit truncation errors. Tests cover large batches and revenue posting. ChangesOwnerRez booking persistence
Crew Sync v2 rollout
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OwnerRezSync
participant upsertBookingsReturningIds
participant Supabase
OwnerRezSync->>upsertBookingsReturningIds: submit booking rows
upsertBookingsReturningIds->>Supabase: upsert bounded chunks and select IDs
Supabase-->>upsertBookingsReturningIds: return external booking IDs and database IDs
upsertBookingsReturningIds-->>OwnerRezSync: return merged ID mapping
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 2
🤖 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/CREW_SYNC_V2_PHASES.md`:
- Around line 39-55: Synchronize the remaining Phase 5 documentation with the
current v2-live state: update the flag-default statements near the rollout
instructions, reconcile the 5a quota-check status everywhere, revise the
operator steps around the 5b acceptance test so they no longer require enabling
or promoting an already-live default, and mark the 5e guardrail section as
completed. Preserve historical notes only when clearly labeled as superseded,
leaving one unambiguous current rollout procedure.
In `@unit/inngest/ownerrez-upsert-bookings.test.ts`:
- Around line 47-53: The test fixtures in
unit/inngest/ownerrez-upsert-bookings.test.ts at lines 47-53 and 107-108 use
three as any casts; replace them with explicit types. Define a typed
OwnerRezBookingRow fixture builder and use it for both row sets, and type the
mocked client against the minimal contract consumed by
upsertBookingsReturningIds, narrowing that function’s parameter if necessary.
🪄 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: f70667f7-4477-4e3d-937b-dc6a362fc2ce
📒 Files selected for processing (7)
docs/CREW_SYNC_V2_PHASES.mdlib/dexie/context.tsxlib/inngest/functions/ownerrez/incremental-sync.tslib/inngest/functions/ownerrez/initial-sync.tslib/inngest/functions/ownerrez/upsert-bookings.tsunit/guardrails/n-plus-one-loops.test.tsunit/inngest/ownerrez-upsert-bookings.test.ts
Comment on lines
+39
to
+55
| | 5 | Rollout, acceptance test, old-code deletion, convention + guardrail | 🟡 **In progress** — 5e (convention + `unit/guardrails/crew-sync-coverage.test.ts`) done 2026-07-29. 5b two-device acceptance test passed 2026-08-12; flag set in Vercel and client default inverted to on the same day, so **v2 is the serving path**. Still open: 5a Realtime quota check, 5c soak, 5d deletion of the v1 path | | ||
|
|
||
| > **2026-08-09 — flag flipped, redeploy still required.** `NEXT_PUBLIC_*` is | ||
| > inlined at BUILD time (see the note at `lib/dexie/context.tsx`'s | ||
| > `CREW_SYNC_V2` and `clientInlinedOnly: true` in `lib/env.ts`), so setting the | ||
| > var in Vercel does nothing to a bundle that was already built. v2 turns on at | ||
| > the next **rebuild**, not the next restart. Until then v1 is still serving. | ||
| > **2026-08-12 — v2 is the DEFAULT and is live.** `NEXT_PUBLIC_CREW_SYNC_V2` | ||
| > is set to `true` in Vercel and many builds have shipped since it was set, so | ||
| > v2 has been the serving path for some time — the 2026-08-09 note this | ||
| > replaces ("redeploy still required") was already stale when written against a | ||
| > branch that redeployed on every push. | ||
| > | ||
| > Everything else on the DB side is verified live in production (2026-08-09): | ||
| > migration `20260725191358` in the ledger, `notify_crew_sync` present, | ||
| > 12 broadcast triggers attached, RLS policy `crew_receive_own_sync_broadcasts` | ||
| > on `realtime.messages`, and the trigger's `crew_members.user_id` join matches | ||
| > both the client's `crew:${userId}` topic and the policy's `auth.uid()`. | ||
| > `realtime.send()` was probed end to end and does land a row. | ||
| > The client default is now inverted (`!== 'false'`), so v2 serves even where | ||
| > the variable is absent. That closes a real hole: `NEXT_PUBLIC_*` is inlined | ||
| > at BUILD time, so an environment missing the variable silently served v1 with | ||
| > nothing in config to show it. Rollback is `NEXT_PUBLIC_CREW_SYNC_V2=false` | ||
| > plus a rebuild — still a build-time change, not a runtime toggle. | ||
| > | ||
| > Still open before the soak: 5b (two-device acceptance test — never run), then | ||
| > 5c. Do NOT do 5d yet: the v1 `postgres_changes` code is the rollback path, and | ||
| > rolling back is another rebuild. | ||
| > | ||
| > Expect one deliberate behaviour change: v1 had a `postgres_changes` channel on | ||
| > `property_assets`; v2 has no trigger for it (`SAFETY_POLL_ONLY` in | ||
| > `unit/guardrails/crew-sync-coverage.test.ts`). Asset edits reach the device on | ||
| > screen-open or the safety poll rather than in ~2 s. | ||
| > | ||
| > Watch the Realtime DB pool (2 → 15, per 5a). Private-channel RLS | ||
| > authorization takes a connection on every join AND every reconnect, v2 is | ||
| > all-private-channel, and the reconnect jitter is 5–35 s — so a Realtime node | ||
| > restart puts the whole fleet through that pool inside 30 seconds. | ||
| > 5b (two-device acceptance test) passed 2026-08-12. Still open: 5a's Realtime | ||
| > quota check, the 5c soak, and 5d deletion of the v1 path — which stays in the | ||
| > tree, compiling and guardrail-covered, until then. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the Phase 5 status and instructions.
The new status says v2 is live and the client default is on, but this document still contains conflicting rollout state:
- Lines 37-38 say the flag defaults off.
- Lines 695-700 mark the 5a quota check complete, while Lines 39 and 53-55 list 5a as open.
- Lines 701-702 still instruct the operator to set the flag and promote after acceptance.
- Lines 750-763 still describe the 5e guardrail as work to add, although Line 39 marks 5e complete.
Update or label these statements so the document has one current rollout procedure.
🤖 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/CREW_SYNC_V2_PHASES.md` around lines 39 - 55, Synchronize the remaining
Phase 5 documentation with the current v2-live state: update the flag-default
statements near the rollout instructions, reconcile the 5a quota-check status
everywhere, revise the operator steps around the 5b acceptance test so they no
longer require enabling or promoting an already-live default, and mark the 5e
guardrail section as completed. Preserve historical notes only when clearly
labeled as superseded, leaving one unambiguous current rollout procedure.
Comment on lines
+47
to
+53
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| return { client: { from } as any, calls } | ||
| } | ||
|
|
||
| const rows = (n: number) => | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| Array.from({ length: n }, (_, i) => ({ external_id: `e${i}` })) as any |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP '\bas\s+any\b|\bany\b|`@ts-ignore`' unit/inngest/ownerrez-upsert-bookings.test.tsRepository: smj1860/fieldstay
Length of output: 506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- file outline ---'
ast-grep outline unit/inngest/ownerrez-upsert-bookings.test.ts
printf '%s\n' '--- relevant test sections ---'
sed -n '1,125p' unit/inngest/ownerrez-upsert-bookings.test.ts
printf '%s\n' '--- related booking row types and test-client patterns ---'
rg -n "OwnerRezBookingRow|ownerrez.*booking|create.*Test|mock.*from|as any" --glob '*.{ts,tsx}' .Repository: smj1860/fieldstay
Length of output: 19440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- upsert implementation ---'
sed -n '1,115p' lib/inngest/functions/ownerrez/upsert-bookings.ts
printf '%s\n' '--- OwnerRez booking types and revenue selector ---'
sed -n '540,675p' lib/integrations/providers/ownerrez.ts
printf '%s\n' '--- Supabase client type declarations ---'
rg -n "export type Supabase|SupabaseClient|createServerClient|Database" lib unit types --glob '*.{ts,tsx}' 2>/dev/null | head -120Repository: smj1860/fieldstay
Length of output: 21402
Replace the three as any casts with explicit types.
Use a typed OwnerRezBookingRow fixture builder for both row sets. Type the mock against the minimal client contract used by upsertBookingsReturningIds, or narrow that function’s parameter to the contract.
📍 Affects 1 file
unit/inngest/ownerrez-upsert-bookings.test.ts#L47-L53(this comment)unit/inngest/ownerrez-upsert-bookings.test.ts#L107-L108
🤖 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/ownerrez-upsert-bookings.test.ts` around lines 47 - 53, The test
fixtures in unit/inngest/ownerrez-upsert-bookings.test.ts at lines 47-53 and
107-108 use three as any casts; replace them with explicit types. Define a typed
OwnerRezBookingRow fixture builder and use it for both row sets, and type the
mocked client against the minimal contract consumed by
upsertBookingsReturningIds, narrowing that function’s parameter if necessary.
Source: Coding guidelines
This was referenced Aug 13, 2026
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.
… default crew sync to v2
INTEGRATION SYNC AUDIT — one real finding, in OwnerRez.
Both syncs upserted the whole booking set in ONE call and built the external_id -> id map from that call's returned rows. The WRITE is not capped; the returned REPRESENTATION is, at max_rows = 1000, with a 200 and no truncation signal — the same cap that applies to a read. So past 1000 bookings the map came back short, and selectOwnerRezBookingsToPostRevenue's closing
.filter((b) => !!b.bookingId)silently dropped every booking missing from it.The bookings themselves were written, so the calendar looked complete. Only the owner_transactions REVENUE rows were absent. No error, no log, nothing in the UI — an owner's P&L simply short by an amount that grows with portfolio size.
initial-sync is where it bites hardest: it fetches every booking for every property in one call, so an account with two years of history crosses 1000 on its FIRST sync — the one run whose entire job is to get the historical ledger right. incremental-sync has the same shape with a smaller blast radius.
Fixed by chunking the upsert so no single response can be truncated, at HALF of max_rows rather than at it, so a future change returning more columns or a lower server-side max_rows cannot quietly re-introduce it. A short chunk now throws rather than returning a partial map — a partial map is indistinguishable from success downstream, so failing loudly is the only way this class surfaces at all. The chunk loop is numeric, which is structurally exempt from the N+1 guardrail, and costs one round trip per 500 bookings.
Not triggered on live data (57 bookings), but this is CLAUDE.md's own stated target scale and structurally the same defect as the inventory truncation that silently duplicated items past property ~8.
The other two integrations are clean. iCal guards its tenant-supplied feed URLs with safeFetch (re-validating every redirect hop) and paginates the whole fan-out. Hospitable's one unbounded read is scoped to a single property AND a date window; a truncated result there leaves a stale Blocked row that self-heals on the next run.
CREW SYNC V2 — now the default, v1 retained.
The acceptance test (Phase 5b) passed, and NEXT_PUBLIC_CREW_SYNC_V2 is set in Vercel, so v2 has been the serving path since the first build after it was set. Inverted the client default from
=== 'true'to!== 'false', which closes a real hole rather than just flipping a switch: absent used to mean v1, so any environment not carrying the variable — a new preview, a rebuilt environment, a setting lost in a project migration — silently served crew the old path. NEXT_PUBLIC_ vars are inlined at BUILD time, so that state was invisible in config and observable only from behaviour.The v1 branch stays in the tree, compiling and guardrail-covered. It is the rollback: set the variable to 'false' and rebuild, no code change. Commenting it out was considered and rejected — it is already unreachable when the flag is on, so commenting buys nothing while costing type-checking, lint, guardrail coverage, and a one-variable rollback. Phase 5d deletes it after the soak.
Updated the 2026-08-09 doc note, which claimed a redeploy was still pending and had been stale for three days, plus the Phase 5 status row.
Three guardrails caught things worth keeping:
const { … } = await supabase…verbatim. The scan reads raw source and cannot tell an illustration of a defect from the defect; rewritten as prose with a note explaining why.Canaried: unchunking the upsert fails three of the six new tests, including an end-to-end one that reproduces the original revenue gap through selectOwnerRezBookingsToPostRevenue rather than asserting it in a comment.
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
Summary by CodeRabbit
New Features
Bug Fixes
Tests