Claude/e2e test failures ci wkst2y - #547
Merged
Merged
Conversation
Step 1 of wiring the <Database> generic. No behaviour change, no client wiring yet — this removes the thing that was blocking it. lib/supabase/server.ts omits <Database>, so `Schema` defaults to `any` and NOT ONE .from() or .rpc() call in this app is type-checked. That is a real gap, not a style choice: reviews.internal_notes was selected by the RepuGuard cron for months and failed every run for every org, because nothing compared the select string against the schema. There was nothing to compare it to. The blocker was that types/database.ts is hand-written and its interfaces do not satisfy postgrest-js's GenericSchema constraint (no index signatures, no Relationships). Binding them collapses every row type to `never`: hand-written types + <Database> 2267 errors (2163 of them that collapse) generated types + <Database> 138 errors across 44 files types/database.generated.ts is now committed, generated from the live schema, and types/database.ts re-exports Json and Database from it instead of declaring its own. Nothing imported Database before this, so the swap is inert — which is the point: it is a prerequisite, landing green and reviewable on its own. The 145 hand-written named interfaces STAY hand-written and stay the app's import surface. They were diffed field-by-field against the live schema first, and they are accurate: the only differences across 91 mapped tables were two PostgREST embed aliases (crew_members on TurnoverAssignment, turnover_assignments on Turnover — relations, not columns) and the deliberately-omitted deprecated work_orders.assigned_crew_id. Worth stating plainly, because it changes what this work is: the types were never the problem, and internal_notes was not type drift. Only the absence of checking was. What remains is the 138, which are a long tail rather than one mechanical fix — insert/update payloads carrying keys the table does not have, string vs string | null, Json shapes — each needing its own judgement. The generic gets wired in the commit that resolves them, not before: a half-wired client is worse than an unwired one because it looks checked. The exact next step is written into lib/supabase/server.ts where whoever picks it up will be reading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176sNbUryq9sVASSkyqpeue
It fired on every completed turnover, to the first PM email, whenever any required asset type was still undiscovered at that property. The daily wrap-up already reports exactly this. cron/daily-wrapup.ts builds its checklistSection from the SAME predicate over the SAME columns — missingAssetTypesFromDiscoveredSet() over the is_na/make/model/photo_url filter — per property, once a day. Verified before removing rather than taken on faith: the two computations are line-for-line equivalent. So it was the same number delivered twice, and the duplicate arrived on a trigger the PM cannot act on differently (asset discovery is not a turnover task) at a rate set by turnover volume. That is the shape that trains people to filter a sender, which costs more than the email was ever worth. Deleted rather than made conditional or throttled: there is no threshold at which a duplicate of the wrap-up's own content earns its own send. The in-app turnover-complete notification is untouched — completion still notifies, it just no longer emails. Also drops the four imports the step was the last user of (assetTypeDisplayName, missingAssetTypesFromDiscoveredSet, AssetType, getPmEmails) and adds a test for handleTurnoverCompleted, which had none. The test asserts the handler sends NO email, using a permissive chain double on purpose: it asserts an absence, so the doubles must not be the reason a send is missing — every step runs for real against a client that answers everything. Verified by reintroducing a send and watching it fail. Two ratchets caught the change and are updated with it: the n-plus-one EXCEPTIONS entry for the milestone-upsert loop (209 -> 180, same code, moved by the deletion) and supabase-error-handling's per-file baseline for this file (9 -> 7, since the removed step held two unhandled destructures). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176sNbUryq9sVASSkyqpeue
…entory_items
First fixes from the TS2345 class surfaced by wiring the generated Database
types. The wiring itself stays out until the class is clear — these two stand
on their own and land green.
The real defect. applyTemplateToProperties copies a template item's category
and unit straight into inventory_items, but the two sides disagree:
inventory_template_items.category text, NULLABLE
inventory_template_items.unit text, NULLABLE
inventory_items.category inventory_category enum, NOT NULL
inventory_items.unit text, NOT NULL
So a NULL category, a NULL unit, or any off-enum string in a template row
reaches the insert and Postgres rejects it — and this is a BULK insert of
every item for every selected property in one statement, so a single bad
template row fails the entire application for all of them, not just its own.
Latent today, not live: 228 template items in production, 0 null and 0
off-enum. The schema permits it; the data does not currently contain it.
Nothing stops a template item being created without a category, which is why
the boundary is the right place to fix it rather than the data.
The fallbacks are the column defaults declared in the schema itself
('other' / 'units'), not invented values, so a defaulted row is
indistinguishable from one the database would have defaulted. The valid enum
labels come from Constants — generated from the live schema — rather than a
hand-written list, because a second copy of an enum is a copy that drifts.
Also replaces the payload's hand-written shape with TablesInsert. That
annotation declared `category: string`, which WIDENED the enum the column
accepts; once widened nothing checks the value again, which is precisely how
the mismatch above stayed invisible. Deriving payload types from the schema
is the general fix for this class, and Tables/TablesInsert/TablesUpdate/Enums
and Constants are now re-exported from types/database.ts so other call sites
can do the same.
Progress on the 138: 2 resolved, 136 remain.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0176sNbUryq9sVASSkyqpeue
Continues the TS2345 class. The wiring stays out until the class is clear;
every change here stands on its own and lands green.
TWO PATTERNS, both of which made a query unverifiable rather than wrong:
1. `const patch: Record<string, unknown>` for an incrementally-built update.
A Record index signature accepts any key with any value, so the payload
was checked against nothing. Ten sites now use TablesUpdate<'table'>:
purchase_orders, work_orders, org_inventory_catalog,
maintenance_schedule_template_items, turnovers, organizations (x2),
guidebook_property_configs, property_assets, properties.
2. `LOCAL_SOURCE: Record<HospitableEntityKind, { table: string }>` in
hospitable-owner.ts. Typed as `string`, the table name widened to "any
table in the schema", so postgrest-js intersected the columns of all 94 of
them and resolved every argument to `never`. Narrowed to the literal union
'bookings' | 'properties' | 'reviews'. All three genuinely carry org_id /
external_id / external_source — checked against the live schema — which is
what makes the shared query legitimate; the union is what lets the type
system confirm it rather than give up.
Also types account/delete's `column` field as a literal union. It indexes the
organizations update payload, and a `string` index would make that payload
implicitly `any` — quietly surrendering the checking the write just gained.
A NOTE ON THE NUMBER, because it briefly looked far better than it is. After
the batch edit tsc reported 6 errors, down from 132. That was wrong: the
import insertion had broken ownerrez/initial-sync.ts's syntax, and a file
that will not parse suppresses errors in everything downstream of it. Fixing
the syntax put the real figure at 123. Worth recording — a sharp drop in an
error count is a reason to check the run, not to celebrate it.
Two n-plus-one EXCEPTIONS entries are re-pinned (initial-sync 171 -> 172,
guidebook/sync 135 -> 136); both moved by exactly the one import line added
above them, same code.
Progress: 138 -> 123 overall. TS2345 44 -> 32.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0176sNbUryq9sVASSkyqpeue
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR replaces handwritten Supabase database types with generated exports, applies schema-derived update and insert types across the codebase, validates inventory categories, and removes mandatory-asset email notifications from turnover completion handling. ChangesDatabase typing and inventory validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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: 3
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/asset-scan.ts (1)
87-111: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard against an empty update payload before Line 110.
Every assignment into
updatesis conditional. On a retried or duplicate run the object can stay empty:asset.scan_statusis already'completed'(Line 92),make,model,serial_number, andmanufacture_dateare already populated (Lines 98-103), andresult.capacityis falsy or already present innotes(Line 106).Line 110 then sends
.update({}). PostgREST rejects an empty payload, so Line 111 throws and the step fails. The function declaresretries: 2at Line 41, so this path repeats and the job ends in the dead-letter handler. The comments at Lines 89-91 and 104-105 show that duplicate runs are an expected input.Return early when there is nothing to write.
🐛 Proposed fix
if (result.capacity && !asset.notes?.includes(`Capacity: ${result.capacity}`)) { updates.notes = asset.notes ? `${asset.notes}\nCapacity: ${result.capacity}` : `Capacity: ${result.capacity}` } + // A retried run on an already-completed scan with every field filled + // leaves `updates` empty. PostgREST rejects an empty payload, which + // would fail the step and consume both retries for a no-op. + if (Object.keys(updates).length === 0) return + const { error } = await supabase.from('property_assets').update(updates).eq('id', asset_id).eq('org_id', org_id) if (error) throw new Error(`property_assets update failed: ${error.message}`)🤖 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/asset-scan.ts` around lines 87 - 111, Guard the update flow in the asset scan function after all conditional assignments to updates and before the Supabase update call: return early when updates has no fields, otherwise preserve the existing property_assets update and error handling.
🧹 Nitpick comments (2)
unit/inngest/turnover-events.test.ts (1)
181-192: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the retained in-app notification.
The test only verifies that
resend.emails.sendis not called. It passes ifnotify-pm-of-completionis removed with the email step. Mock or spy oncreatePmNotificationand assert one call with the expected organization, notification type, and dedupe key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@unit/inngest/turnover-events.test.ts` around lines 181 - 192, Update the handleTurnoverCompleted test to mock or spy on createPmNotification and assert it is called once with the expected organization, notification type, and dedupe key, while retaining the existing assertion that resend.emails.send is not called.unit/inventory/inventory-actions.test.ts (1)
693-707: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests do not exercise
toInventoryCategory.The describe block is named after
toInventoryCategory, but neither test calls it. Both tests assert properties of the generatedConstantsobject only.Two specific gaps:
- Line 694-699: the test asserts each generated enum label is truthy. A generated string tuple always satisfies this. The test cannot fail, and it does not verify that a valid label survives the conversion.
- No test covers the defect described in Lines 687-692: a
nullcategory, an off-enum category string, and anullunit. Those three inputs are what the change guards.Line 701-706 is useful as written. It confirms the fallback is a real enum member.
toInventoryCategoryis not exported fromapp/(dashboard)/inventory/actions.ts. To cover it, either export it, or assert the insert payload throughapplyTemplateToPropertieswith a template item that hascategory: nullandcategory: 'not_a_real_category'.🧪 Proposed test, via the exported action
it('keeps a valid enum label', async () => { const { Constants } = await import('`@/types/database`') - for (const label of Constants.public.Enums.inventory_category) { - expect(label).toBeTruthy() - } + expect(Constants.public.Enums.inventory_category).toContain('kitchen') }) + + it('coerces a null or off-enum template category to other, and a null unit to units', async () => { + // Arrange the template-items read to return one row per bad input, then + // assert the inventory_items insert payload. + const { applyTemplateToProperties } = await import('`@/app/`(dashboard)/inventory/actions') + // ...mock inventory_template_items to yield: + // { name: 'A', category: null, unit: null } + // { name: 'B', category: 'not_a_real_category', unit: 'rolls' } + const result = await applyTemplateToProperties('tmpl_1', ['prop_1']) + expect(result.applied).toBe(2) + // expect(insertPayload[0]).toMatchObject({ category: 'other', unit: 'units' }) + // expect(insertPayload[1]).toMatchObject({ category: 'other', unit: 'rolls' }) + })🤖 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/inventory/inventory-actions.test.ts` around lines 693 - 707, Replace the non-exercising truthiness test in the toInventoryCategory test block with coverage that invokes the conversion through an exported toInventoryCategory or via applyTemplateToProperties. Verify valid categories are preserved and that null or off-enum categories, including a null unit, produce the schema-default fallback; retain the existing assertion that 'other' is a real enum member.
🤖 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 `@CLAUDE.md`:
- Around line 592-609: Reconcile the surrounding CLAUDE.md guidance with the
two-file type workflow: revise the prose near the Supabase client typing
statement so it no longer claims return types come from types/database.ts while
the Database generic is omitted, and update both migration instructions near the
existing references to types/database.ts to require updating/regenerating
types/database.generated.ts as well. Preserve the documented roles of
types/database.generated.ts and types/database.ts.
In `@lib/supabase/server.ts`:
- Around line 23-27: Update the measurement-date comment near the Supabase
server client wiring to use the verified date for the reported error count; if
that date cannot be confirmed, remove the hard-coded count and date instead of
retaining inaccurate metrics.
- Around line 20-21: Update the commented createServerClient example to pass the
imported Database type explicitly as its generic argument, changing the call
from createServerClient(…) to createServerClient<Database>(…) while preserving
the existing arguments and wiring.
---
Outside diff comments:
In `@lib/inngest/functions/asset-scan.ts`:
- Around line 87-111: Guard the update flow in the asset scan function after all
conditional assignments to updates and before the Supabase update call: return
early when updates has no fields, otherwise preserve the existing
property_assets update and error handling.
---
Nitpick comments:
In `@unit/inngest/turnover-events.test.ts`:
- Around line 181-192: Update the handleTurnoverCompleted test to mock or spy on
createPmNotification and assert it is called once with the expected
organization, notification type, and dedupe key, while retaining the existing
assertion that resend.emails.send is not called.
In `@unit/inventory/inventory-actions.test.ts`:
- Around line 693-707: Replace the non-exercising truthiness test in the
toInventoryCategory test block with coverage that invokes the conversion through
an exported toInventoryCategory or via applyTemplateToProperties. Verify valid
categories are preserved and that null or off-enum categories, including a null
unit, produce the schema-default fallback; retain the existing assertion that
'other' is a real enum member.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8741ce0b-b94f-418a-bdc2-f63d1194f751
⛔ Files ignored due to path filters (1)
types/database.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (19)
CLAUDE.mdapp/(dashboard)/inventory/actions.tsapp/(dashboard)/maintenance/actions.tsapp/(dashboard)/templates/inventory/actions.tsapp/(dashboard)/templates/maintenance/actions.tsapp/(dashboard)/turnovers/actions.tsapp/api/account/delete/route.tslib/checklists/seed-default-room-templates.tslib/guidebook/sync.tslib/inngest/functions/asset-scan.tslib/inngest/functions/ownerrez/initial-sync.tslib/inngest/functions/turnover-events.tslib/integrations/providers/hospitable-owner.tslib/supabase/server.tstypes/database.tsunit/guardrails/n-plus-one-loops.test.tsunit/guardrails/supabase-error-handling.test.tsunit/inngest/turnover-events.test.tsunit/inventory/inventory-actions.test.ts
Comment on lines
+592
to
+609
| There are now TWO type files, and a migration touches both: | ||
|
|
||
| - `types/database.generated.ts` — GENERATED from the live schema, never | ||
| hand-edited. Regenerate with | ||
| `npx supabase gen types typescript --project-id vpmznjktllhmmbfnxuvk > types/database.generated.ts` | ||
| (or the Supabase MCP `generate_typescript_types` tool). It owns `Json` and | ||
| `Database`; `types/database.ts` re-exports both from it. It exists because | ||
| the hand-written interfaces do not satisfy postgrest-js's `GenericSchema` | ||
| constraint, which is why `lib/supabase/server.ts` still omits the | ||
| `<Database>` generic and no `.from()`/`.rpc()` call is type-checked yet — | ||
| see the comment in that file for the remaining work. | ||
| - `types/database.ts` — hand-written named interfaces (`Property`, | ||
| `WorkOrder`, `MemberRole`, …), the app's import surface. Diffed against the | ||
| live schema on 2026-08-02 and accurate: the only differences were two | ||
| PostgREST embed aliases (not columns) and the deliberately-omitted | ||
| deprecated `work_orders.assigned_crew_id`. `scripts/check-type-drift.mjs` | ||
| keeps it honest. | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the new text with the prose that follows it.
The new block states that a migration touches both files, and that no .from()/.rpc() call is type-checked yet. The unchanged text below still states the opposite in two places:
- Line 611-613: "The Supabase TypeScript client infers return types from this file — not from the live database schema." This contradicts Line 600-602, which states the
<Database>generic is omitted and nothing is type-checked. - Line 610 and Line 816: both instruct updating only
types/database.tsin the same commit as the migration.
A reader who follows Line 610 or Line 816 will skip the regeneration step the new block requires. Update those lines to name both files.
📝 Proposed wording change
-Whenever a DB migration adds or changes a column, update `types/database.ts`
-in the same commit. The Supabase TypeScript client infers return types from
-this file — not from the live database schema. A column that exists in the DB
-but not in `types/database.ts` causes TypeScript build failures even when
-the SQL query and select string are perfectly correct.
+Whenever a DB migration adds or changes a column, regenerate
+`types/database.generated.ts` AND update the matching hand-written interface
+in `types/database.ts`, both in the same commit. A column that exists in the
+DB but not in `types/database.ts` causes TypeScript build failures in every
+call site that reads it through a hand-written interface, even when the SQL
+query and select string are correct.And at Line 816:
-Always update `types/database.ts` in the same commit as the migration.
+Always regenerate `types/database.generated.ts` and update
+`types/database.ts` in the same commit as the migration.🤖 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 `@CLAUDE.md` around lines 592 - 609, Reconcile the surrounding CLAUDE.md
guidance with the two-file type workflow: revise the prose near the Supabase
client typing statement so it no longer claims return types come from
types/database.ts while the Database generic is omitted, and update both
migration instructions near the existing references to types/database.ts to
require updating/regenerating types/database.generated.ts as well. Preserve the
documented roles of types/database.generated.ts and types/database.ts.
Comment on lines
+20
to
+21
| // import type { Database } from '@/types/database' | ||
| // return createServerClient( |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cd "$tmpdir"
npm pack --silent --ignore-scripts `@supabase/ssr`@0.10.3 >/dev/null
tarball="$(find . -maxdepth 1 -name '*.tgz' -print -quit)"
tar -xzf "$tarball"
rg -n -A 8 -B 2 'export function createServerClient' packageRepository: smj1860/fieldstay
Length of output: 3717
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package snippets =="
rg -n 'createServerClient|createClient|supabaseUrl|supabaseKey' package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null || true
echo
echo "== target file excerpt =="
git ls-files | rg 'lib/supabase/server\.ts$' || true
if [ -f lib/supabase/server.ts ]; then
nl -ba lib/supabase/server.ts | sed -n '1,80p'
fiRepository: smj1860/fieldstay
Length of output: 269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package snippets =="
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock; do
[ -f "$f" ] && { echo "--- $f"; grep -n -C 2 '`@supabase/ssr`\|supabaseUrl\|supabaseKey\|createServerClient\|createClient' "$f" || true; }
done
echo
echo "== target file excerpt =="
if [ -f lib/supabase/server.ts ]; then
awk '{printf "%6d %s\n", NR, $0}' lib/supabase/server.ts | sed -n '1,90p'
fiRepository: smj1860/fieldstay
Length of output: 5599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lockfile createServerClient declaration, if available =="
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cd "$tmpdir"
# Fetch package tarball metadata/body without npm lock/install behavior
tarball="$(curl -LfsS https://registry.npmjs.org/@supabase/ssr/-/ssr-0.10.3.tgz)"
printf '%s\n' "$tarball" | tar -xzf -
rg -n 'createServerClient' package/package.json package/src/createServerClient.ts package/index.d.ts package/src/index.ts 2>/dev/null || true
rg -n 'Database = any|createServerClient<' package/src/createServerClient.ts || true
echo
echo "== repo call sites using generateServerClient/createServerClient generics =="
rg -n 'createServerClient<|createClient<|createSupabase<|SupabaseClient<|Database' lib . --glob '!*.map' --glob '!node_modules/**' 2>/dev/null | head -n 120 || trueRepository: smj1860/fieldstay
Length of output: 424
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cd "$tmpdir"
echo "== fetch and inspect package metadata/tarball =="
curl -LfsS 'https://registry.npmjs.org/@supabase/ssr/-/ssr-0.10.3.tgz' -o tarball.tgz
tar -xzf tarball.tgz
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package/package.json','utf8'));
console.log("pkg version:", pkg.version);
console.log("main:", pkg.main);
console.log("types:", pkg.types);
console.log("exports:", JSON.stringify(pkg.exports, null, 2).slice(0, 2000));
JS
rg -n 'createServerClient<|Database = any|Type = any|SchemaName|SupabaseClient' package/src package/index.d.ts package/package.json 2>/dev/null || true
echo
echo "== tsconfig type settings =="
for f in tsconfig.json tsconfig.app.json tsconfig.node.json; do
[ -f "$f" ] && { echo "--- $f"; grep -n -E 'strict|noImplicitAny|strictNullChecks|useUnknownInCatchVariables|strictFunctionTypes|allowUnreachableCode' "$f" || true; }
doneRepository: smj1860/fieldstay
Length of output: 1001
Show the Database generic in the wired example.
@supabase/ssr@0.10.3 declares createServerClient<Database = any, ...>, so importing Database but calling createServerClient( without <Database> leaves the example untyped. Update the example to createServerClient<Database>(...).
🤖 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/supabase/server.ts` around lines 20 - 21, Update the commented
createServerClient example to pass the imported Database type explicitly as its
generic argument, changing the call from createServerClient(…) to
createServerClient<Database>(…) while preserving the existing arguments and
wiring.
Comment on lines
+23
to
+27
| // Measured on that basis: 123 errors as of 2026-08-03 (was 138) — a long tail of | ||
| // insert/update payload mismatches, nullability, and Json shapes, each needing | ||
| // its own judgement rather than one mechanical fix. Wiring it is the next step | ||
| // and must land with those 138 resolved, not before; a half-wired client is | ||
| // worse than an unwired one because it looks checked. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the actual measurement date.
Line 23 states 2026-08-03, but the current review date is August 2, 2026. Replace it with the date when the error count was measured, or remove the hard-coded count until it is verified.
🤖 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/supabase/server.ts` around lines 23 - 27, Update the measurement-date
comment near the Supabase server client wiring to use the verified date for the
reported error count; if that date cannot be confirmed, remove the hard-coded
count and date instead of retaining inaccurate metrics.
Greptile SummaryThis PR tightens schema-aware database writes and removes a duplicate turnover completion email. The main changes are:
Confidence Score: 4/5Safe to merge with one non-blocking test coverage gap. Production changes are mostly schema typing hardening and a deliberate notification removal. The only accepted issue is a test assertion gap around the retained in-app PM notification. Files Needing Attention:
 T-Rex LogsWhat T-Rex did
|
| Filename | Overview |
|---|---|
| app/(dashboard)/inventory/actions.ts | Adds schema-derived inventory payload typing and enum/default normalization when applying templates; no blocking issues found. |
| lib/inngest/functions/turnover-events.ts | Removes the duplicate asset-discovery completion email while retaining in-app completion notifications and existing financial side effects. |
| scripts/check-type-drift.mjs | Updates the drift checker to parse the explicit hand-written row map and fail clearly on parse misses; no issue found. |
| types/database.ts | Re-exports generated schema helpers and adds an explicit HandWrittenRowMap for drift checking; no issue found. |
| unit/inngest/turnover-events.test.ts | Pins removal of the asset-discovery completion email, but does not assert the retained in-app notification behavior. |
Sequence Diagram
sequenceDiagram
participant CrewOrPM as Crew/PM action
participant TurnoverAction as updateTurnoverStatus
participant Inngest as turnover/completed handler
participant Notifications as notifications table
participant OwnerTx as owner_transactions
participant Outcomes as assignment_outcomes
CrewOrPM->>TurnoverAction: mark turnover completed
TurnoverAction->>TurnoverAction: "conditional UPDATE where status != completed"
alt row updated
TurnoverAction->>Inngest: send turnover/completed
Inngest->>Notifications: createPmNotification(dedupeKey)
Note over Inngest: asset-discovery email step removed
Inngest->>OwnerTx: upsert cleaning_fee by source_reference_id/source
Inngest->>Outcomes: update started_at/completed_at
else already completed
TurnoverAction-->>CrewOrPM: no duplicate event fired
end
Reviews (2): Last reviewed commit: "fix(ci): restore the drift gate's table ..." | Re-trigger Greptile
…e miss MY REGRESSION, from e424373. db-invariants failed with 92 findings that looked like catastrophic schema drift and were nothing of the kind. scripts/check-type-drift.mjs learns which hand-written interface models which live table by regex-parsing types/database.ts for `Tables: { table: { Row: Interface; ...` ... `Views:`. That block was doing two unrelated jobs: it was the postgrest schema type AND it was this mapping. When Database moved to types/database.generated.ts, the block went with it, the regex matched nothing, the map came back empty — and an empty map means every live table looks unmodelled. 92 tables, 92 findings, none real. Fixed by giving the mapping its own declaration, HandWrittenRowMap, whose only purpose is this. It cannot be carried off by an unrelated refactor again because there is nothing else it is for. All 90 entries recovered from the pre-change file; verified against the E2E project's own db_type_shape_report(): 93 live tables = 90 mapped + 3 allowlisted, zero unmodelled, zero stale. The gate deliberately still reads types/database.ts and NOT the generated file. The generated types are produced FROM the live schema, so diffing them against it can never fail — pointing the gate there would have turned 92 false failures into a check that passes forever and means nothing, which is worse. Two things so this class fails honestly next time: - The script now exits 1 with one clear message when the map parses empty, instead of emitting a finding per table. A parse miss and real drift should not look the same to whoever reads the log. - unit/guardrails/type-drift-map-parses.test.ts asserts the map still parses, that every interface it names exists, and that the script has not drifted back to the old block. This runs in `checks`, which always runs — the db-invariants job self-disarms without Supabase secrets, so on a fork PR or a local run the breakage would otherwise be invisible until merge. Verified by deleting the map and watching it fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176sNbUryq9sVASSkyqpeue
|
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
Bug Fixes
Changes
Documentation