Fix/tip keyword matching - #407
Merged
Merged
Conversation
Documents the approach to prevent re-import of deleted bank transactions using a tombstone table pattern. Covers Stripe sync, CSV/PDF import, fingerprint matching, and restore functionality. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9-task TDD plan covering: tombstone table, updated delete RPCs, Stripe sync filtering, CSV/PDF import filtering, deleted tab UI, restore/permanent-delete functionality. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Covers 5 improvements: DragOverlay for visible drag ghost, always-visible day indicators, day-vs-shift assignment popover, PDF+CSV export in planner, and auto-select employee on Add. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6-task plan covering: DragOverlay, day indicators, assignment popover, PDF+CSV export, auto-select employee on Add, and final verification. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… and SQL
Replace .includes('tip') and LIKE '%tip%' with hasTipKeyword() regex to prevent
false positives (e.g. 'Stipend Liability' being classified as tips).
Fixes: useMonthlyMetrics, useRevenueBreakdown, passThroughAdjustments, get_monthly_sales_metrics SQL
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion Subtype takes precedence over name matching to prevent misclassifying items like "Auto Gratuity" (service_charge subtype) as tips. - TIP_SUBTYPES: 'tips', 'tips_payable', 'tips payable' → always tip - GENERIC_SUBTYPES: '', 'liability', 'other_current_liability' → use name matching - Specific subtypes (service_charge, etc.) → skip name matching, go to other - Applied consistently across all 7 locations (hooks, edge functions, SQL)
…ath bug - Export hasTipKeyword, TIP_SUBTYPES, GENERIC_SUBTYPES from passThroughAdjustments.ts - Create supabase/functions/_shared/tipClassification.ts for edge functions - Fix logic bug in useRevenueBreakdown fallback path (missing 'tips payable', 'liability', 'other' subtypes) - Replace 11-pass adjustment classification with single-pass bucketing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s_tips_by_date Prevents potential double-counting when account_subtype contains a space variant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s_tips_by_date Add generic subtype guard around regex match in NOT EXISTS clause to mirror the categorized_tips WHERE clause exactly. Prevents over-excluding sales with specific subtypes like 'service_charge' that happen to have tip-related account names. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… pgTAP) - Switch get_monthly_sales_metrics from SECURITY DEFINER to SECURITY INVOKER with explicit auth check (CodeRabbit critical) - Fix double-counting in useRevenueBreakdown.tsx fallback path: filter out categorized adjustments before bucketing since they're already merged into categoryMap via mergeCategorizedAdjustments - Add other_current_liabilities and other_liabilities to GENERIC_SUBTYPES across all 4 locations (TS + SQL) for broader POS compatibility - Fix pgTAP test fixtures: tip account now uses account_type='liability', account_subtype='tips' to match the new stricter matching logic - Add unit tests for new generic subtypes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…to generic subtypes account_subtype is an enum (account_subtype_enum) — 'tips' is not a valid value. Real tip accounts use 'payroll_liabilities' with name 'Tips Payable'. Fix the pgTAP test fixture to use valid enum values and add payroll_liabilities to GENERIC_SUBTYPES across all locations so name-based tip matching works for real account data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The get_unified_sales_totals SQL function classified rows solely by item_type, but adjustment rows (tip, tax, service_charge, fee) get item_type='sale' by DEFAULT. This caused tips and taxes to be counted as revenue instead of pass-through. Changes: - SQL migration: check adjustment_type before item_type in every CASE branch so adjustment rows are correctly classified - Client code: set item_type on each adjustment entry in createManualSaleWithAdjustments (belt-and-suspenders) - pgTAP test: 8 tests verifying revenue excludes adjustments - E2E test: seed sale+tip+tax, verify totals on POS Sales page Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Manual sale entries via createManualSale and createManualSaleWithAdjustments
were not setting item_type, causing it to default to null. This meant:
- Revenue rows weren't counted as revenue (item_type != 'sale')
- Adjustment rows (tip, tax) could be misclassified
Changes:
- createManualSale: derive item_type from adjustment_type ('sale' for revenue)
- createManualSaleWithAdjustments: set item_type='sale' on main revenue entry
- E2E test: verifies manual sale with tip shows correct Revenue and Pass-Through
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Existing manual sale entries had item_type=NULL or 'sale' on adjustment rows (tip, tax, etc.), causing SQL functions to misclassify them as revenue. This migration sets item_type to match adjustment_type for all affected rows, and sets item_type='sale' on revenue rows with NULL. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Hoist regex to module-level constant in hasTipKeyword - Add isTipLiability(subtype, accountName) to passThroughAdjustments.ts - Replace 4 inline tip classification expressions in useRevenueBreakdown.tsx - Replace 1 inline tip classification expression in useMonthlyMetrics.tsx - Move imports to file top in periodMetrics.ts and monthlyMetrics.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix misleading pgTAP test comment in get_pos_tips_by_date - Add Number() coercion for total_price assertions in E2E test - Remove unreachable dead code in voids CASE branch - Replace weak Test 6 with "overtipped" (real embedded false positive) - Add negative assertion for $64.00 in pos-sale-with-tip E2E test - Remove flaky waitForTimeout before page.reload() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Deploy Preview for easyshifthq ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Updates to Preview Branch (fix/tip-keyword-matching) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
Contributor
📝 WalkthroughWalkthroughStandardizes tip detection to word-boundary regex and exact subtype matching across frontend hooks, edge/shared utilities, and SQL; centralizes tip classification; surfaces per-source tip breakdowns in UI; adds SQL migrations and extensive unit, pgTAP, and E2E tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FrontendHook as Frontend Hook
participant SharedTip as Tip Classification (shared)
participant EdgeFunc as Edge/SQL Layer
participant DB as Database
Client->>FrontendHook: Request metrics / render dashboard
FrontendHook->>SharedTip: isTipLiability(subtype, accountName) / hasTipKeyword(name)
SharedTip-->>FrontendHook: tip classification (true/false)
FrontendHook->>EdgeFunc: request aggregated metrics / pos tips
EdgeFunc->>DB: call get_monthly_sales_metrics / get_pos_tips_by_date (uses word-boundary regex & exact subtype matches)
DB-->>EdgeFunc: aggregated results (per-source breakdowns)
EdgeFunc-->>FrontendHook: metrics + tip source breakdown
FrontendHook-->>Client: render totals and per-source tip rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17dedb002c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| COALESCE(SUM( | ||
| CASE | ||
| WHEN us.adjustment_type = 'discount' THEN ABS(us.total_price) | ||
| WHEN us.item_type = 'discount' AND COALESCE(us.adjustment_type, 'discount') != 'void' THEN ABS(us.total_price) |
There was a problem hiding this comment.
Restrict legacy discount fallback to null adjustment_type
The item_type='discount' fallback now runs even when adjustment_type is set to a different value, so desynced rows (for example, records later edited from discount to tip/fee) are counted as discounts here and as pass-through in the new adjustment-type-first branch. That inflates both discounts and pass_through_amount for the same row. This legacy fallback should only apply when adjustment_type IS NULL.
Useful? React with 👍 / 👎.
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)
src/hooks/useUnifiedSales.tsx (1)
510-513:⚠️ Potential issue | 🟠 Major
updateManualSalecan leaveitem_typestale after adjustment changes.At Line 512,
adjustment_typeis updated, butitem_typeis not. A row changed fromfeetotip(or tonull) will keep an outdateditem_type, causing classification drift.Proposed fix
try { + const adjType = saleData.adjustmentType ?? null; + const itemType = adjType === 'fee' ? 'other' : (adjType ?? 'sale'); + const { error } = await supabase .from('unified_sales') .update({ item_name: saleData.itemName, - adjustment_type: saleData.adjustmentType || null, + adjustment_type: adjType, + item_type: itemType, quantity: saleData.quantity, unit_price: saleData.unitPrice, total_price: saleData.totalPrice, sale_date: saleData.saleDate, sale_time: saleData.saleTime, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useUnifiedSales.tsx` around lines 510 - 513, updateManualSale currently updates adjustment_type but leaves item_type stale; modify the update call in updateManualSale to set item_type as well (e.g., item_type: saleData.itemType ?? null or derive item_type from saleData.adjustmentType) so that when adjustment_type changes to 'fee', 'tip' or null the stored item_type is overwritten accordingly; ensure you reference the .update({ ... }) block in updateManualSale and include item_type alongside item_name, adjustment_type, and quantity.
🧹 Nitpick comments (3)
supabase/tests/tip_keyword_matching.sql (1)
4-42: Group pgTAP checks under category headers.The tests are clear, but adding
TEST CATEGORYheaders will match repo convention and keep this file consistent with other SQL test suites.As per coding guidelines, "Group related pgTAP tests with comment headers (TEST CATEGORY 1, TEST CATEGORY 2)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/tip_keyword_matching.sql` around lines 4 - 42, Add pgTAP comment headers to group the tests: insert a "TEST CATEGORY: Function existence" header above the two has_function checks for get_monthly_sales_metrics and get_pos_tips_by_date, and a "TEST CATEGORY: Tip keyword regex matching" header above the regex ok() checks (the tests that reference 'tips payable', 'tip - credit', 'stipend liability', and 'overtipped'). Ensure headers use the same comment style as other suites (e.g., prefixed with "-- TEST CATEGORY") and place them immediately before the relevant test groups so the function-existence checks and regex checks are visually separated.supabase/tests/35_get_unified_sales_totals.sql (1)
55-129: Add explicit TEST CATEGORY headers for readability.The numbered tests are clear, but grouping related assertions under category headers will match the SQL test convention used across the repo.
As per coding guidelines, "Group related pgTAP tests with comment headers (TEST CATEGORY 1, TEST CATEGORY 2)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/35_get_unified_sales_totals.sql` around lines 55 - 129, Add explicit TEST CATEGORY comment headers to group related pgTAP assertions in this file: insert clear comment blocks (e.g., -- TEST CATEGORY: Function existence, -- TEST CATEGORY: Revenue/Pass-through/Discounts/Voids, -- TEST CATEGORY: Legacy adjustments) before the relevant test groups that call get_unified_sales_totals and the INSERTs into unified_sales so readers can quickly find tests for get_unified_sales_totals, revenue/pass_through_amount/discounts/voids/collected_at_pos, and legacy/adjustment_type behavior.tests/unit/passThroughAdjustments.test.ts (1)
81-241: Prefix critical test cases withCRITICAL:per repository convention.Please update the newly added business-critical classification tests to use the required prefix for easier CI triage.
✍️ Example rename pattern
-it('does not classify non-tip words containing "tip" as tips', () => { +it('CRITICAL: should classify as other when liability name contains "tip" only as substring', () => {Based on learnings: "Applies to tests/unit/**/*.test.{ts,tsx} : Prefix critical business logic tests with 'CRITICAL:' in test description".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/passThroughAdjustments.test.ts` around lines 81 - 241, Several newly added unit tests that validate core classification logic (those calling classifyPassThroughItem) must have their test descriptions prefixed with "CRITICAL:" per repo convention; update each it(...) description that asserts pass-through classification behavior (e.g., the tests whose descriptions include phrases like 'classifies "Gratuity Collected" as tip', 'classifies "Tip - CREDIT" as tip (Toast POS format)', 'classifies account with subtype "tips" exactly', and other tests that call classifyPassThroughItem) to start with "CRITICAL: " so CI and triage can identify them. Ensure you only change the human-readable test description strings passed to it(...) and not the test bodies or the classifyPassThroughItem calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-03-02-fix-tip-keyword-matching-plan.md`:
- Line 13: The MD001 error is caused by the "### Task 1: Create worktree and
branch with existing changes" heading jumping directly from the top-level "#"
heading; fix it by either promoting that line to a level-2 heading ("## Task 1:
Create worktree and branch with existing changes") or by inserting an
appropriate level-2 parent heading above it so the "###" has a "##" parent;
ensure the heading text remains unchanged and keep the rest of the document
structure intact.
In `@src/components/MonthlyBreakdownTable.tsx`:
- Around line 646-654: The tip rows use unstable keys (category.account_id and
an index key for adjustments) which can cause reconciliation bugs; update the
map keys to use a stable, unique identifier for each item (e.g. for tip
categories use a composite stable key like
`${category.account_id}-${category.account_name}` or a dedicated unique id
field, and for adjustments use a unique adjustment identifier such as
`adj.adjustment_id` or a composite `adj.id-${adj.adjustment_type}`); if those
unique fields don't exist, add a stable unique id to the data source before
rendering so map keys in the breakdown.tip_categories and
breakdown.adjustments.filter(...) renders are deterministic.
In `@src/hooks/useRevenueBreakdown.tsx`:
- Around line 546-560: The adjBuckets logic in useRevenueBreakdown is including
classifyPassThroughItem(...) results of 'other' which causes adjustmentOtherC to
be nonzero and skews liabilities versus the RPC path; modify the code that
builds adjBuckets (or the subsequent extraction) to ignore items where
classifyPassThroughItem(a) === 'other' so adjBuckets never accumulates an
'other' bucket, ensuring adjustmentOtherC remains 0 like the RPC path
(reference: useRevenueBreakdown, classifyPassThroughItem, adjBuckets,
adjustmentOtherC).
In `@supabase/migrations/20260302120001_fix_pos_tips_keyword_matching.sql`:
- Around line 42-46: The regex-based tip name match currently allows any account
when coa.account_subtype is generic; restrict that branch by requiring
coa.account_type = 'liability' so only liability accounts are matched by the
account_name regex. Specifically, update the OR condition that combines
LOWER(COALESCE(coa.account_subtype::TEXT, '')) IN ('', 'liability', ...) AND
LOWER(COALESCE(coa.account_name, '')) ~ '...' to also include coa.account_type =
'liability'. Apply the same change to the second occurrence that spans the other
block (the similar condition at lines 64-68).
In
`@supabase/migrations/20260302120002_fix_unified_sales_totals_adjustment_type.sql`:
- Around line 49-71: The CASE logic must give precedence to adjustment_type over
legacy item_type: change the discounts branch to only treat rows as discounts
when adjustment_type = 'discount' OR (adjustment_type IS NULL AND item_type =
'discount'), and change the voids branch to treat rows as voids when
adjustment_type = 'void' OR (adjustment_type IS NULL AND item_type = 'void');
then update the pass-through CASE to treat rows as pass-through only when
(adjustment_type IS NOT NULL AND adjustment_type NOT IN ('discount','void')) OR
(adjustment_type IS NULL AND item_type NOT IN ('sale','discount','void')) so
legacy void item_type cannot fall into pass-through. Ensure you keep the
ABS(...) wrapping for discounts and voids as in the original.
In `@supabase/tests/26_get_pos_tips_by_date.sql`:
- Around line 30-35: The INSERT into chart_of_accounts uses ON CONFLICT (id) DO
UPDATE but only sets account_name, leaving account_type and account_subtype
stale if the fixture changes; update the DO UPDATE clause for the INSERT in the
tests/26_get_pos_tips_by_date.sql diff to assign EXCLUDED.account_name,
EXCLUDED.account_type and EXCLUDED.account_subtype (and any other columns that
may change for the fixture such as normal_balance or account_code if applicable)
so the test data insert is idempotent and always refreshes those fields when the
row exists.
In `@supabase/tests/35_get_unified_sales_totals.sql`:
- Around line 28-54: The INSERT fixtures into unified_sales (e.g., rows with ids
'00000000-0000-0000-0000-000000000100' through '...106') are not idempotent and
will fail on rerun; update each INSERT to include ON CONFLICT (id) DO UPDATE and
set the necessary columns (e.g., restaurant_id, pos_system, external_order_id,
item_name, quantity, total_price, sale_date, item_type, adjustment_type) to
EXCLUDED.<column> so the row is upserted instead of erroring, and apply the same
ON CONFLICT pattern to the other fixture inserts mentioned in the review (lines
107-120).
In `@tests/e2e/manual-sale-tip-not-doubled.spec.ts`:
- Around line 109-127: Replace the fixed sleep and broad restaurant-level query
with a deterministic poll scoped to the specific sale: remove
page.waitForTimeout(1000) and instead poll inside page.evaluate (or use
page.waitForFunction) to repeatedly query
supabase.from('unified_sales').select(...) with a filter for the specific
order/sale identifier (e.g., .eq('order_id', orderId) or other unique key you
obtain earlier) until the expected 3 rows exist or a timeout is reached, then
assert results.length === 3; update the references in the test where
page.evaluate reads restaurantId to instead use the known sale/order id to make
the DB verification deterministic.
In `@tests/e2e/pos-sale-with-tip.spec.ts`:
- Line 20: The seeded sale_date uses UTC via new Date().toISOString().slice(0,
10) (the today variable) which can shift the day versus the local UI; replace
that expression with a local-calendar date construction (e.g., derive
year/month/day from new Date() using getFullYear/getMonth/getDate with
zero-padding or use toLocaleDateString('en-CA')) so today represents the local
YYYY-MM-DD value used when seeding sale_date in the pos-sale-with-tip spec.
---
Outside diff comments:
In `@src/hooks/useUnifiedSales.tsx`:
- Around line 510-513: updateManualSale currently updates adjustment_type but
leaves item_type stale; modify the update call in updateManualSale to set
item_type as well (e.g., item_type: saleData.itemType ?? null or derive
item_type from saleData.adjustmentType) so that when adjustment_type changes to
'fee', 'tip' or null the stored item_type is overwritten accordingly; ensure you
reference the .update({ ... }) block in updateManualSale and include item_type
alongside item_name, adjustment_type, and quantity.
---
Nitpick comments:
In `@supabase/tests/35_get_unified_sales_totals.sql`:
- Around line 55-129: Add explicit TEST CATEGORY comment headers to group
related pgTAP assertions in this file: insert clear comment blocks (e.g., --
TEST CATEGORY: Function existence, -- TEST CATEGORY:
Revenue/Pass-through/Discounts/Voids, -- TEST CATEGORY: Legacy adjustments)
before the relevant test groups that call get_unified_sales_totals and the
INSERTs into unified_sales so readers can quickly find tests for
get_unified_sales_totals,
revenue/pass_through_amount/discounts/voids/collected_at_pos, and
legacy/adjustment_type behavior.
In `@supabase/tests/tip_keyword_matching.sql`:
- Around line 4-42: Add pgTAP comment headers to group the tests: insert a "TEST
CATEGORY: Function existence" header above the two has_function checks for
get_monthly_sales_metrics and get_pos_tips_by_date, and a "TEST CATEGORY: Tip
keyword regex matching" header above the regex ok() checks (the tests that
reference 'tips payable', 'tip - credit', 'stipend liability', and
'overtipped'). Ensure headers use the same comment style as other suites (e.g.,
prefixed with "-- TEST CATEGORY") and place them immediately before the relevant
test groups so the function-existence checks and regex checks are visually
separated.
In `@tests/unit/passThroughAdjustments.test.ts`:
- Around line 81-241: Several newly added unit tests that validate core
classification logic (those calling classifyPassThroughItem) must have their
test descriptions prefixed with "CRITICAL:" per repo convention; update each
it(...) description that asserts pass-through classification behavior (e.g., the
tests whose descriptions include phrases like 'classifies "Gratuity Collected"
as tip', 'classifies "Tip - CREDIT" as tip (Toast POS format)', 'classifies
account with subtype "tips" exactly', and other tests that call
classifyPassThroughItem) to start with "CRITICAL: " so CI and triage can
identify them. Ensure you only change the human-readable test description
strings passed to it(...) and not the test bodies or the classifyPassThroughItem
calls.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
docs/plans/2026-03-02-fix-tip-keyword-matching-design.mddocs/plans/2026-03-02-fix-tip-keyword-matching-plan.mdsrc/components/MonthlyBreakdownTable.tsxsrc/components/PnLIntelligenceReport.tsxsrc/hooks/useMonthlyMetrics.tsxsrc/hooks/useRevenueBreakdown.tsxsrc/hooks/useUnifiedSales.tsxsrc/hooks/utils/passThroughAdjustments.tssupabase/functions/_shared/monthlyMetrics.tssupabase/functions/_shared/periodMetrics.tssupabase/functions/_shared/tipClassification.tssupabase/migrations/20260302120000_fix_monthly_tip_keyword_matching.sqlsupabase/migrations/20260302120001_fix_pos_tips_keyword_matching.sqlsupabase/migrations/20260302120002_fix_unified_sales_totals_adjustment_type.sqlsupabase/migrations/20260303120000_backfill_item_type_from_adjustment_type.sqlsupabase/tests/26_get_pos_tips_by_date.sqlsupabase/tests/35_get_unified_sales_totals.sqlsupabase/tests/tip_keyword_matching.sqltests/e2e/manual-sale-tip-not-doubled.spec.tstests/e2e/pos-sale-with-tip.spec.tstests/unit/dashboardScenarios.test.tstests/unit/passThroughAdjustments.test.ts
|
|
||
| --- | ||
|
|
||
| ### Task 1: Create worktree and branch with existing changes |
Contributor
There was a problem hiding this comment.
Fix heading level jump (MD001).
Line 13 starts at ### immediately after an # heading. Add an ## parent level (or demote to ##) to satisfy markdownlint.
Suggested markdown fix
+## Tasks
+
### Task 1: Create worktree and branch with existing changes🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-02-fix-tip-keyword-matching-plan.md` at line 13, The MD001
error is caused by the "### Task 1: Create worktree and branch with existing
changes" heading jumping directly from the top-level "#" heading; fix it by
either promoting that line to a level-2 heading ("## Task 1: Create worktree and
branch with existing changes") or by inserting an appropriate level-2 parent
heading above it so the "###" has a "##" parent; ensure the heading text remains
unchanged and keep the rest of the document structure intact.
Comment on lines
+646
to
+654
| {breakdown.tip_categories?.map((category) => ( | ||
| <div key={category.account_id} className="flex items-center justify-between px-2 py-1 text-[11px] text-muted-foreground"> | ||
| <span>{category.account_name}</span> | ||
| <span>{formatCurrency(category.total_amount)}</span> | ||
| </div> | ||
| ))} | ||
| {breakdown.adjustments?.filter(a => a.adjustment_type === 'tip').map((adj, idx) => ( | ||
| <div key={`adj-tip-${idx}`} className="flex items-center justify-between px-2 py-1 text-[11px] text-muted-foreground"> | ||
| <span>POS Tip Adjustments</span> |
Contributor
There was a problem hiding this comment.
Use stable, unique keys in the new tip source rows.
Line 647 (key={category.account_id}) can collide, and Line 653 uses an index-based key. This can produce incorrect row reconciliation when rows change.
🔧 Suggested fix
- {breakdown.tip_categories?.map((category) => (
- <div key={category.account_id} className="flex items-center justify-between px-2 py-1 text-[11px] text-muted-foreground">
+ {breakdown.tip_categories?.map((category) => (
+ <div
+ key={`${category.account_id}-${category.account_subtype}-${category.account_name}`}
+ className="flex items-center justify-between px-2 py-1 text-[11px] text-muted-foreground"
+ >
<span>{category.account_name}</span>
<span>{formatCurrency(category.total_amount)}</span>
</div>
))}
- {breakdown.adjustments?.filter(a => a.adjustment_type === 'tip').map((adj, idx) => (
- <div key={`adj-tip-${idx}`} className="flex items-center justify-between px-2 py-1 text-[11px] text-muted-foreground">
+ {breakdown.adjustments?.filter(a => a.adjustment_type === 'tip').map((adj) => (
+ <div key={`adj-tip-${adj.adjustment_type}-${adj.total_amount}`} className="flex items-center justify-between px-2 py-1 text-[11px] text-muted-foreground">
<span>POS Tip Adjustments</span>
<span>{formatCurrency(adj.total_amount)}</span>
</div>
))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/MonthlyBreakdownTable.tsx` around lines 646 - 654, The tip
rows use unstable keys (category.account_id and an index key for adjustments)
which can cause reconciliation bugs; update the map keys to use a stable, unique
identifier for each item (e.g. for tip categories use a composite stable key
like `${category.account_id}-${category.account_name}` or a dedicated unique id
field, and for adjustments use a unique adjustment identifier such as
`adj.adjustment_id` or a composite `adj.id-${adj.adjustment_type}`); if those
unique fields don't exist, add a stable unique id to the data source before
rendering so map keys in the breakdown.tip_categories and
breakdown.adjustments.filter(...) renders are deterministic.
Comment on lines
+546
to
+560
| const adjBuckets = new Map<PassThroughType, { totalC: number; count: number }>(); | ||
| (allAdjustments || []).filter(a => !a.is_categorized || !a.chart_account).forEach(a => { | ||
| const type = classifyPassThroughItem(a); | ||
| const entry = adjBuckets.get(type) || { totalC: 0, count: 0 }; | ||
| entry.totalC += type === 'discount' ? Math.abs(toC(a.total_price || 0)) : toC(a.total_price || 0); | ||
| entry.count += 1; | ||
| adjBuckets.set(type, entry); | ||
| }); | ||
|
|
||
| const adjustmentTaxC = adjBuckets.get('tax')?.totalC || 0; | ||
| const adjustmentTipsC = adjBuckets.get('tip')?.totalC || 0; | ||
| const adjustmentServiceChargeC = adjBuckets.get('service_charge')?.totalC || 0; | ||
| const adjustmentDiscountsC = adjBuckets.get('discount')?.totalC || 0; | ||
| const adjustmentFeesC = adjBuckets.get('fee')?.totalC || 0; | ||
| const adjustmentOtherC = adjBuckets.get('other')?.totalC || 0; |
Contributor
There was a problem hiding this comment.
other adjustment bucketing can skew liabilities and diverges from the RPC path.
classifyPassThroughItem(...) can return 'other' for unsupported adjustment types, and Line 608 adds that bucket into other_liabilities. The RPC path does not do this, so totals can differ depending on which path executes.
🛠️ Suggested fix
const adjBuckets = new Map<PassThroughType, { totalC: number; count: number }>();
(allAdjustments || []).filter(a => !a.is_categorized || !a.chart_account).forEach(a => {
const type = classifyPassThroughItem(a);
+ if (type === 'other') return; // keep fallback aligned with RPC classification buckets
const entry = adjBuckets.get(type) || { totalC: 0, count: 0 };
entry.totalC += type === 'discount' ? Math.abs(toC(a.total_price || 0)) : toC(a.total_price || 0);
entry.count += 1;
adjBuckets.set(type, entry);
});
const adjustmentTaxC = adjBuckets.get('tax')?.totalC || 0;
const adjustmentTipsC = adjBuckets.get('tip')?.totalC || 0;
const adjustmentServiceChargeC = adjBuckets.get('service_charge')?.totalC || 0;
const adjustmentDiscountsC = adjBuckets.get('discount')?.totalC || 0;
const adjustmentFeesC = adjBuckets.get('fee')?.totalC || 0;
-const adjustmentOtherC = adjBuckets.get('other')?.totalC || 0;
// Combine categorized amounts with adjustment amounts
const combinedTaxC = totalTaxC + adjustmentTaxC;
const combinedTipsC = totalTipsC + adjustmentTipsC;
-const combinedOtherLiabilitiesC = totalOtherLiabilitiesC + adjustmentServiceChargeC + adjustmentFeesC + adjustmentOtherC;
+const combinedOtherLiabilitiesC = totalOtherLiabilitiesC + adjustmentServiceChargeC + adjustmentFeesC;
const combinedDiscountsC = totalDiscountsC + adjustmentDiscountsC;Also applies to: 608-609
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useRevenueBreakdown.tsx` around lines 546 - 560, The adjBuckets
logic in useRevenueBreakdown is including classifyPassThroughItem(...) results
of 'other' which causes adjustmentOtherC to be nonzero and skews liabilities
versus the RPC path; modify the code that builds adjBuckets (or the subsequent
extraction) to ignore items where classifyPassThroughItem(a) === 'other' so
adjBuckets never accumulates an 'other' bucket, ensuring adjustmentOtherC
remains 0 like the RPC path (reference: useRevenueBreakdown,
classifyPassThroughItem, adjBuckets, adjustmentOtherC).
Comment on lines
+42
to
+46
| AND ( | ||
| LOWER(COALESCE(coa.account_subtype::TEXT, '')) IN ('tips', 'tips_payable', 'tips payable') | ||
| OR (LOWER(COALESCE(coa.account_subtype::TEXT, '')) IN ('', 'liability', 'other_current_liability', 'other_current_liabilities', 'other_liabilities', 'payroll_liabilities', 'other') | ||
| AND LOWER(COALESCE(coa.account_name, '')) ~ '(^|[^a-z])(tip|tips|gratuity)([^a-z]|$)') | ||
| ) |
Contributor
There was a problem hiding this comment.
Restrict regex tip matching to liability accounts.
Line 42 and Line 64 currently allow generic-subtype + keyword matches without enforcing coa.account_type = 'liability', which can pull non-liability accounts into tip totals.
Proposed SQL fix
FROM unified_sales us
INNER JOIN unified_sales_splits uss ON us.id = uss.sale_id
INNER JOIN chart_of_accounts coa ON uss.category_id = coa.id
WHERE us.restaurant_id = p_restaurant_id
AND us.sale_date >= p_start_date
AND us.sale_date <= p_end_date
+ AND coa.account_type = 'liability'
AND (
LOWER(COALESCE(coa.account_subtype::TEXT, '')) IN ('tips', 'tips_payable', 'tips payable')
OR (LOWER(COALESCE(coa.account_subtype::TEXT, '')) IN ('', 'liability', 'other_current_liability', 'other_current_liabilities', 'other_liabilities', 'payroll_liabilities', 'other')
AND LOWER(COALESCE(coa.account_name, '')) ~ '(^|[^a-z])(tip|tips|gratuity)([^a-z]|$)')
)
@@
AND NOT EXISTS (
SELECT 1 FROM unified_sales_splits uss
INNER JOIN chart_of_accounts coa ON uss.category_id = coa.id
WHERE uss.sale_id = us.id
+ AND coa.account_type = 'liability'
AND (
LOWER(COALESCE(coa.account_subtype::TEXT, '')) IN ('tips', 'tips_payable', 'tips payable')
OR (LOWER(COALESCE(coa.account_subtype::TEXT, '')) IN ('', 'liability', 'other_current_liability', 'other_current_liabilities', 'other_liabilities', 'payroll_liabilities', 'other')
AND LOWER(COALESCE(coa.account_name, '')) ~ '(^|[^a-z])(tip|tips|gratuity)([^a-z]|$)')
)
)Also applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260302120001_fix_pos_tips_keyword_matching.sql` around
lines 42 - 46, The regex-based tip name match currently allows any account when
coa.account_subtype is generic; restrict that branch by requiring
coa.account_type = 'liability' so only liability accounts are matched by the
account_name regex. Specifically, update the OR condition that combines
LOWER(COALESCE(coa.account_subtype::TEXT, '')) IN ('', 'liability', ...) AND
LOWER(COALESCE(coa.account_name, '')) ~ '...' to also include coa.account_type =
'liability'. Apply the same change to the second occurrence that spans the other
block (the similar condition at lines 64-68).
Comment on lines
+49
to
+71
| -- Discounts: adjustment_type='discount' OR legacy item_type='discount' (non-void) | ||
| COALESCE(SUM( | ||
| CASE | ||
| WHEN us.adjustment_type = 'discount' THEN ABS(us.total_price) | ||
| WHEN us.item_type = 'discount' AND COALESCE(us.adjustment_type, 'discount') != 'void' THEN ABS(us.total_price) | ||
| ELSE 0 | ||
| END | ||
| ), 0)::NUMERIC AS discounts, | ||
|
|
||
| -- Voids: adjustment_type='void' | ||
| COALESCE(SUM( | ||
| CASE | ||
| WHEN us.adjustment_type = 'void' THEN ABS(us.total_price) | ||
| ELSE 0 | ||
| END | ||
| ), 0)::NUMERIC AS voids, | ||
|
|
||
| -- Pass-through: adjustment rows that are NOT discount/void, plus legacy non-sale/non-discount item_types | ||
| COALESCE(SUM( | ||
| CASE | ||
| WHEN us.adjustment_type IS NOT NULL AND us.adjustment_type NOT IN ('discount', 'void') THEN us.total_price | ||
| WHEN us.adjustment_type IS NULL AND us.item_type NOT IN ('sale', 'discount') THEN us.total_price | ||
| ELSE 0 |
Contributor
There was a problem hiding this comment.
adjustment_type precedence is still incomplete in discount/void branches.
Line 53 can still classify rows as discount based on item_type even when adjustment_type is non-null, and legacy item_type='void' rows can fall into pass-through at Line 70.
🧮 Suggested CASE fixes
-- Discounts: adjustment_type='discount' OR legacy item_type='discount' (non-void)
COALESCE(SUM(
CASE
WHEN us.adjustment_type = 'discount' THEN ABS(us.total_price)
- WHEN us.item_type = 'discount' AND COALESCE(us.adjustment_type, 'discount') != 'void' THEN ABS(us.total_price)
+ WHEN us.adjustment_type IS NULL AND us.item_type = 'discount' THEN ABS(us.total_price)
ELSE 0
END
), 0)::NUMERIC AS discounts,
-- Voids: adjustment_type='void'
COALESCE(SUM(
CASE
- WHEN us.adjustment_type = 'void' THEN ABS(us.total_price)
+ WHEN us.adjustment_type = 'void'
+ OR (us.adjustment_type IS NULL AND us.item_type = 'void') THEN ABS(us.total_price)
ELSE 0
END
), 0)::NUMERIC AS voids,
-- Pass-through: adjustment rows that are NOT discount/void, plus legacy non-sale/non-discount item_types
COALESCE(SUM(
CASE
WHEN us.adjustment_type IS NOT NULL AND us.adjustment_type NOT IN ('discount', 'void') THEN us.total_price
- WHEN us.adjustment_type IS NULL AND us.item_type NOT IN ('sale', 'discount') THEN us.total_price
+ WHEN us.adjustment_type IS NULL AND us.item_type NOT IN ('sale', 'discount', 'void') THEN us.total_price
ELSE 0
END
), 0)::NUMERIC AS pass_through_amount,📝 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
| -- Discounts: adjustment_type='discount' OR legacy item_type='discount' (non-void) | |
| COALESCE(SUM( | |
| CASE | |
| WHEN us.adjustment_type = 'discount' THEN ABS(us.total_price) | |
| WHEN us.item_type = 'discount' AND COALESCE(us.adjustment_type, 'discount') != 'void' THEN ABS(us.total_price) | |
| ELSE 0 | |
| END | |
| ), 0)::NUMERIC AS discounts, | |
| -- Voids: adjustment_type='void' | |
| COALESCE(SUM( | |
| CASE | |
| WHEN us.adjustment_type = 'void' THEN ABS(us.total_price) | |
| ELSE 0 | |
| END | |
| ), 0)::NUMERIC AS voids, | |
| -- Pass-through: adjustment rows that are NOT discount/void, plus legacy non-sale/non-discount item_types | |
| COALESCE(SUM( | |
| CASE | |
| WHEN us.adjustment_type IS NOT NULL AND us.adjustment_type NOT IN ('discount', 'void') THEN us.total_price | |
| WHEN us.adjustment_type IS NULL AND us.item_type NOT IN ('sale', 'discount') THEN us.total_price | |
| ELSE 0 | |
| -- Discounts: adjustment_type='discount' OR legacy item_type='discount' (non-void) | |
| COALESCE(SUM( | |
| CASE | |
| WHEN us.adjustment_type = 'discount' THEN ABS(us.total_price) | |
| WHEN us.adjustment_type IS NULL AND us.item_type = 'discount' THEN ABS(us.total_price) | |
| ELSE 0 | |
| END | |
| ), 0)::NUMERIC AS discounts, | |
| -- Voids: adjustment_type='void' | |
| COALESCE(SUM( | |
| CASE | |
| WHEN us.adjustment_type = 'void' | |
| OR (us.adjustment_type IS NULL AND us.item_type = 'void') THEN ABS(us.total_price) | |
| ELSE 0 | |
| END | |
| ), 0)::NUMERIC AS voids, | |
| -- Pass-through: adjustment rows that are NOT discount/void, plus legacy non-sale/non-discount item_types | |
| COALESCE(SUM( | |
| CASE | |
| WHEN us.adjustment_type IS NOT NULL AND us.adjustment_type NOT IN ('discount', 'void') THEN us.total_price | |
| WHEN us.adjustment_type IS NULL AND us.item_type NOT IN ('sale', 'discount', 'void') THEN us.total_price | |
| ELSE 0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@supabase/migrations/20260302120002_fix_unified_sales_totals_adjustment_type.sql`
around lines 49 - 71, The CASE logic must give precedence to adjustment_type
over legacy item_type: change the discounts branch to only treat rows as
discounts when adjustment_type = 'discount' OR (adjustment_type IS NULL AND
item_type = 'discount'), and change the voids branch to treat rows as voids when
adjustment_type = 'void' OR (adjustment_type IS NULL AND item_type = 'void');
then update the pass-through CASE to treat rows as pass-through only when
(adjustment_type IS NOT NULL AND adjustment_type NOT IN ('discount','void')) OR
(adjustment_type IS NULL AND item_type NOT IN ('sale','discount','void')) so
legacy void item_type cannot fall into pass-through. Ensure you keep the
ABS(...) wrapping for discounts and voids as in the original.
Comment on lines
30
to
35
| INSERT INTO chart_of_accounts (id, restaurant_id, account_code, account_name, account_type, account_subtype, normal_balance) VALUES | ||
| ('00000000-0000-0000-0000-000000000010'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'TIP001', 'Tips Revenue', 'revenue', 'sales', 'credit'), | ||
| ('00000000-0000-0000-0000-000000000010'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'TIP001', 'Tips Payable', 'liability', 'payroll_liabilities', 'credit'), | ||
| ('00000000-0000-0000-0000-000000000011'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'OTH001', 'Other Income', 'revenue', 'other_income', 'credit'), | ||
| ('00000000-0000-0000-0000-000000000012'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'FOOD01', 'Food Sales', 'revenue', 'sales', 'credit') | ||
| ('00000000-0000-0000-0000-000000000012'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'FOOD01', 'Food Sales', 'revenue', 'food_sales', 'credit') | ||
| ON CONFLICT (id) DO UPDATE SET account_name = EXCLUDED.account_name; | ||
|
|
Contributor
There was a problem hiding this comment.
Expand conflict updates for fixture rows.
Line 34 only updates account_name, but this change also modifies account_type and account_subtype (Lines 31 and 33). If the row already exists, the test can run with stale subtype/type values.
Suggested fix
-ON CONFLICT (id) DO UPDATE SET account_name = EXCLUDED.account_name;
+ON CONFLICT (id) DO UPDATE SET
+ account_name = EXCLUDED.account_name,
+ account_type = EXCLUDED.account_type,
+ account_subtype = EXCLUDED.account_subtype,
+ normal_balance = EXCLUDED.normal_balance;As per coding guidelines, "Use ON CONFLICT ... DO UPDATE for idempotent test data inserts in pgTAP".
📝 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
| INSERT INTO chart_of_accounts (id, restaurant_id, account_code, account_name, account_type, account_subtype, normal_balance) VALUES | |
| ('00000000-0000-0000-0000-000000000010'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'TIP001', 'Tips Revenue', 'revenue', 'sales', 'credit'), | |
| ('00000000-0000-0000-0000-000000000010'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'TIP001', 'Tips Payable', 'liability', 'payroll_liabilities', 'credit'), | |
| ('00000000-0000-0000-0000-000000000011'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'OTH001', 'Other Income', 'revenue', 'other_income', 'credit'), | |
| ('00000000-0000-0000-0000-000000000012'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'FOOD01', 'Food Sales', 'revenue', 'sales', 'credit') | |
| ('00000000-0000-0000-0000-000000000012'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'FOOD01', 'Food Sales', 'revenue', 'food_sales', 'credit') | |
| ON CONFLICT (id) DO UPDATE SET account_name = EXCLUDED.account_name; | |
| INSERT INTO chart_of_accounts (id, restaurant_id, account_code, account_name, account_type, account_subtype, normal_balance) VALUES | |
| ('00000000-0000-0000-0000-000000000010'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'TIP001', 'Tips Payable', 'liability', 'payroll_liabilities', 'credit'), | |
| ('00000000-0000-0000-0000-000000000011'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'OTH001', 'Other Income', 'revenue', 'other_income', 'credit'), | |
| ('00000000-0000-0000-0000-000000000012'::uuid, '00000000-0000-0000-0000-000000000001'::uuid, 'FOOD01', 'Food Sales', 'revenue', 'food_sales', 'credit') | |
| ON CONFLICT (id) DO UPDATE SET | |
| account_name = EXCLUDED.account_name, | |
| account_type = EXCLUDED.account_type, | |
| account_subtype = EXCLUDED.account_subtype, | |
| normal_balance = EXCLUDED.normal_balance; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/tests/26_get_pos_tips_by_date.sql` around lines 30 - 35, The INSERT
into chart_of_accounts uses ON CONFLICT (id) DO UPDATE but only sets
account_name, leaving account_type and account_subtype stale if the fixture
changes; update the DO UPDATE clause for the INSERT in the
tests/26_get_pos_tips_by_date.sql diff to assign EXCLUDED.account_name,
EXCLUDED.account_type and EXCLUDED.account_subtype (and any other columns that
may change for the fixture such as normal_balance or account_code if applicable)
so the test data insert is idempotent and always refreshes those fields when the
row exists.
Comment on lines
+28
to
+54
| INSERT INTO unified_sales (id, restaurant_id, pos_system, external_order_id, item_name, quantity, total_price, sale_date) | ||
| VALUES ('00000000-0000-0000-0000-000000000100'::uuid, '00000000-0000-0000-0000-000000000099'::uuid, 'manual', 'ord-totals-1', 'Burger', 1, 50.00, '2024-06-15'); | ||
|
|
||
| -- Tip row: $10 tip — has adjustment_type='tip' but item_type defaults to 'sale' (the bug) | ||
| INSERT INTO unified_sales (id, restaurant_id, pos_system, external_order_id, item_name, quantity, total_price, sale_date, adjustment_type) | ||
| VALUES ('00000000-0000-0000-0000-000000000101'::uuid, '00000000-0000-0000-0000-000000000099'::uuid, 'manual', 'ord-totals-1', 'Tip', 1, 10.00, '2024-06-15', 'tip'); | ||
|
|
||
| -- Tax row: $4 tax — has adjustment_type='tax' but item_type defaults to 'sale' | ||
| INSERT INTO unified_sales (id, restaurant_id, pos_system, external_order_id, item_name, quantity, total_price, sale_date, adjustment_type) | ||
| VALUES ('00000000-0000-0000-0000-000000000102'::uuid, '00000000-0000-0000-0000-000000000099'::uuid, 'manual', 'ord-totals-1', 'Sales Tax', 1, 4.00, '2024-06-15', 'tax'); | ||
|
|
||
| -- Service charge row: $3 — has adjustment_type='service_charge', item_type defaults to 'sale' | ||
| INSERT INTO unified_sales (id, restaurant_id, pos_system, external_order_id, item_name, quantity, total_price, sale_date, adjustment_type) | ||
| VALUES ('00000000-0000-0000-0000-000000000103'::uuid, '00000000-0000-0000-0000-000000000099'::uuid, 'manual', 'ord-totals-1', 'Service Charge', 1, 3.00, '2024-06-15', 'service_charge'); | ||
|
|
||
| -- Discount row: -$5 discount — has adjustment_type='discount', item_type defaults to 'sale' | ||
| INSERT INTO unified_sales (id, restaurant_id, pos_system, external_order_id, item_name, quantity, total_price, sale_date, adjustment_type) | ||
| VALUES ('00000000-0000-0000-0000-000000000104'::uuid, '00000000-0000-0000-0000-000000000099'::uuid, 'manual', 'ord-totals-1', 'Discount', 1, -5.00, '2024-06-15', 'discount'); | ||
|
|
||
| -- Fee row: $2 — has adjustment_type='fee', item_type defaults to 'sale' | ||
| INSERT INTO unified_sales (id, restaurant_id, pos_system, external_order_id, item_name, quantity, total_price, sale_date, adjustment_type) | ||
| VALUES ('00000000-0000-0000-0000-000000000105'::uuid, '00000000-0000-0000-0000-000000000099'::uuid, 'manual', 'ord-totals-1', 'Platform Fee', 1, 2.00, '2024-06-15', 'fee'); | ||
|
|
||
| -- Also add a properly-typed tip row (item_type='tip') to verify it still works | ||
| INSERT INTO unified_sales (id, restaurant_id, pos_system, external_order_id, item_name, quantity, total_price, sale_date, item_type, adjustment_type) | ||
| VALUES ('00000000-0000-0000-0000-000000000106'::uuid, '00000000-0000-0000-0000-000000000099'::uuid, 'toast', 'ord-totals-2', 'Toast Tip', 1, 8.00, '2024-06-15', 'tip', 'tip'); | ||
|
|
Contributor
There was a problem hiding this comment.
Make fixture inserts idempotent with conflict handling.
The fixed UUID fixture rows are inserted without ON CONFLICT, so reruns can fail if prior state exists. Adding conflict handling keeps the pgTAP suite resilient.
Suggested pattern
INSERT INTO unified_sales (...)
-VALUES (...);
+VALUES (...)
+ON CONFLICT (id) DO UPDATE SET
+ total_price = EXCLUDED.total_price;As per coding guidelines, "Use ON CONFLICT ... DO UPDATE for idempotent test data inserts in pgTAP".
Also applies to: 107-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/tests/35_get_unified_sales_totals.sql` around lines 28 - 54, The
INSERT fixtures into unified_sales (e.g., rows with ids
'00000000-0000-0000-0000-000000000100' through '...106') are not idempotent and
will fail on rerun; update each INSERT to include ON CONFLICT (id) DO UPDATE and
set the necessary columns (e.g., restaurant_id, pos_system, external_order_id,
item_name, quantity, total_price, sale_date, item_type, adjustment_type) to
EXCLUDED.<column> so the row is upserted instead of erroring, and apply the same
ON CONFLICT pattern to the other fixture inserts mentioned in the review (lines
107-120).
Comment on lines
+109
to
+127
| // Wait for data to be inserted | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // Query the database to verify item_type is set correctly on adjustment rows | ||
| const results = await page.evaluate(async () => { | ||
| const supabase = (window as any).__supabase; | ||
| const restaurantId = await (window as any).__getRestaurantId(); | ||
|
|
||
| const { data, error } = await supabase | ||
| .from('unified_sales') | ||
| .select('item_name, item_type, adjustment_type, total_price') | ||
| .eq('restaurant_id', restaurantId) | ||
| .order('created_at', { ascending: true }); | ||
|
|
||
| if (error) throw new Error(error.message); | ||
| return data; | ||
| }); | ||
|
|
||
| expect(results.length).toBe(3); // 1 sale + 1 tip + 1 tax |
Contributor
There was a problem hiding this comment.
Make the DB verification deterministic.
Line 110 uses a fixed sleep, then the query checks all rows for the restaurant and asserts results.length === 3. This can flake if inserts are delayed or any extra rows exist. Prefer polling until expected rows are present and scope the query to the specific sale (e.g., order ID or known row set).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/manual-sale-tip-not-doubled.spec.ts` around lines 109 - 127,
Replace the fixed sleep and broad restaurant-level query with a deterministic
poll scoped to the specific sale: remove page.waitForTimeout(1000) and instead
poll inside page.evaluate (or use page.waitForFunction) to repeatedly query
supabase.from('unified_sales').select(...) with a filter for the specific
order/sale identifier (e.g., .eq('order_id', orderId) or other unique key you
obtain earlier) until the expected 3 rows exist or a timeout is reached, then
assert results.length === 3; update the references in the test where
page.evaluate reads restaurantId to instead use the known sale/order id to make
the DB verification deterministic.
| // Seed unified_sales rows: sale $50, tip $10, tax $4 | ||
| await exposeSupabaseHelpers(page); | ||
|
|
||
| const today = new Date().toISOString().slice(0, 10); |
Contributor
There was a problem hiding this comment.
Use local calendar date for seeded sale_date.
Line 20 uses UTC (toISOString()), which can shift the day relative to local UI filters and cause intermittent misses.
Suggested fix
- const today = new Date().toISOString().slice(0, 10);
+ const now = new Date();
+ const today = `${now.getFullYear()}-${String(now.getMonth() + 1).padStart(2, '0')}-${String(now.getDate()).padStart(2, '0')}`;📝 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
| const today = new Date().toISOString().slice(0, 10); | |
| const now = new Date(); | |
| const today = `${now.getFullYear()}-${String(now.getMonth() + 1).padStart(2, '0')}-${String(now.getDate()).padStart(2, '0')}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/pos-sale-with-tip.spec.ts` at line 20, The seeded sale_date uses
UTC via new Date().toISOString().slice(0, 10) (the today variable) which can
shift the day versus the local UI; replace that expression with a local-calendar
date construction (e.g., derive year/month/day from new Date() using
getFullYear/getMonth/getDate with zero-padding or use
toLocaleDateString('en-CA')) so today represents the local YYYY-MM-DD value used
when seeding sale_date in the pos-sale-with-tip spec.
When no categorized liability accounts exist, ensureCategory creates synthetic categories from adjustment data (e.g., $1 tax from get_pass_through_totals). Then combinedTaxC added adjustmentTaxC again: $1 + $1 = $2 (doubled). Fix: track whether real categorized entries existed before ensureCategory. Only add adjustment amounts when there were already categorized amounts (meaning they come from different data sources). When ensureCategory created the synthetic entry, the adjustment amount is already included. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/hooks/useRevenueBreakdown.tsx (1)
553-567:⚠️ Potential issue | 🟠 Major
otheradjustments are still added to liabilities in fallback, causing path divergence.
classifyPassThroughItem(...)can return'other', and Line 615 addsadjustmentOtherCinto liabilities. This makes fallback totals differ from the RPC path and can hide amounts (no'other'entry is emitted inadjustmentsBreakdown).Suggested fix
const adjBuckets = new Map<PassThroughType, { totalC: number; count: number }>(); (allAdjustments || []).filter(a => !a.is_categorized || !a.chart_account).forEach(a => { const type = classifyPassThroughItem(a); + if (type === 'other') return; const entry = adjBuckets.get(type) || { totalC: number; count: number } = { totalC: 0, count: 0 }; entry.totalC += type === 'discount' ? Math.abs(toC(a.total_price || 0)) : toC(a.total_price || 0); entry.count += 1; adjBuckets.set(type, entry); }); const adjustmentTaxC = adjBuckets.get('tax')?.totalC || 0; const adjustmentTipsC = adjBuckets.get('tip')?.totalC || 0; const adjustmentServiceChargeC = adjBuckets.get('service_charge')?.totalC || 0; const adjustmentDiscountsC = adjBuckets.get('discount')?.totalC || 0; const adjustmentFeesC = adjBuckets.get('fee')?.totalC || 0; -const adjustmentOtherC = adjBuckets.get('other')?.totalC || 0; ... -const combinedOtherLiabilitiesC = totalOtherLiabilitiesC + adjustmentServiceChargeC + adjustmentFeesC + adjustmentOtherC; +const combinedOtherLiabilitiesC = totalOtherLiabilitiesC + adjustmentServiceChargeC + adjustmentFeesC;Also applies to: 615-615
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRevenueBreakdown.tsx` around lines 553 - 567, The fallback buckets currently include items classified as 'other', which causes adjustmentOtherC to be added into liabilities and diverge from the RPC path; update the aggregation so adjBuckets excludes items where classifyPassThroughItem(a) === 'other' (i.e., skip or filter out 'other' before creating/updating adjBuckets), so adjustmentOtherC will remain zero in the fallback and adjustmentsBreakdown/liabilities behavior matches the RPC path; ensure this change is applied around the adjBuckets population logic and any use of adjustmentOtherC.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/hooks/useRevenueBreakdown.tsx`:
- Around line 553-567: The fallback buckets currently include items classified
as 'other', which causes adjustmentOtherC to be added into liabilities and
diverge from the RPC path; update the aggregation so adjBuckets excludes items
where classifyPassThroughItem(a) === 'other' (i.e., skip or filter out 'other'
before creating/updating adjBuckets), so adjustmentOtherC will remain zero in
the fallback and adjustmentsBreakdown/liabilities behavior matches the RPC path;
ensure this change is applied around the adjBuckets population logic and any use
of adjustmentOtherC.
jdelgado2002
added a commit
that referenced
this pull request
Mar 3, 2026
Major fixes: - Add account_type='liability' guard to get_pos_tips_by_date (prevents non-liability accounts from being matched as tips) - Fix legacy discount fallback to require adjustment_type IS NULL (prevents void rows from being counted as discounts) - Add catch-all for unknown adjustment types in revenue breakdown (parity between RPC and all-months code paths) Minor fixes: - Use stable composite keys in MonthlyBreakdownTable - Make pgTAP test 35 fixtures idempotent with ON CONFLICT DO UPDATE - Expand ON CONFLICT in pgTAP test 26 to include account_type/subtype - Replace waitForTimeout with toPass() polling in E2E test - Use local date formatting instead of UTC in E2E test - Add ## Tasks heading to plan doc (fix MD001) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jdelgado2002
added a commit
that referenced
this pull request
Mar 4, 2026
* docs: add tombstone design for deleted bank transactions
Documents the approach to prevent re-import of deleted bank
transactions using a tombstone table pattern. Covers Stripe sync,
CSV/PDF import, fingerprint matching, and restore functionality.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: add implementation plan for deleted transactions tombstone
9-task TDD plan covering: tombstone table, updated delete RPCs,
Stripe sync filtering, CSV/PDF import filtering, deleted tab UI,
restore/permanent-delete functionality.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: add design for shift planner UI enhancements
Covers 5 improvements: DragOverlay for visible drag ghost,
always-visible day indicators, day-vs-shift assignment popover,
PDF+CSV export in planner, and auto-select employee on Add.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: add implementation plan for shift planner UI enhancements
6-task plan covering: DragOverlay, day indicators, assignment popover,
PDF+CSV export, auto-select employee on Add, and final verification.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: use word-boundary regex for tip keyword matching in client hooks and SQL
Replace .includes('tip') and LIKE '%tip%' with hasTipKeyword() regex to prevent
false positives (e.g. 'Stipend Liability' being classified as tips).
Fixes: useMonthlyMetrics, useRevenueBreakdown, passThroughAdjustments, get_monthly_sales_metrics SQL
* fix: tighten subtype matching from includes('tip') to exact match
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: use exact subtype match + hasTipKeyword in periodMetrics.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: use hasTipKeyword in edge function monthlyMetrics.ts
* fix: tighten tip matching in get_pos_tips_by_date SQL function
* test: add edge case tests for tip keyword matching
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* test: add pgTAP tests for SQL tip keyword regex matching
* feat: add tip source breakdown to Monthly Breakdown Table
* feat: add tip source breakdown to P&L Intelligence Report
* fix: use TIP_SUBTYPES/GENERIC_SUBTYPES sets for robust tip classification
Subtype takes precedence over name matching to prevent misclassifying
items like "Auto Gratuity" (service_charge subtype) as tips.
- TIP_SUBTYPES: 'tips', 'tips_payable', 'tips payable' → always tip
- GENERIC_SUBTYPES: '', 'liability', 'other_current_liability' → use name matching
- Specific subtypes (service_charge, etc.) → skip name matching, go to other
- Applied consistently across all 7 locations (hooks, edge functions, SQL)
* refactor: deduplicate tip classification constants and fix fallback path bug
- Export hasTipKeyword, TIP_SUBTYPES, GENERIC_SUBTYPES from passThroughAdjustments.ts
- Create supabase/functions/_shared/tipClassification.ts for edge functions
- Fix logic bug in useRevenueBreakdown fallback path (missing 'tips payable', 'liability', 'other' subtypes)
- Replace 11-pass adjustment classification with single-pass bucketing
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: add missing 'tips payable' to NOT EXISTS subtype check in get_pos_tips_by_date
Prevents potential double-counting when account_subtype contains a space variant.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: align NOT EXISTS exclusion with categorized_tips logic in get_pos_tips_by_date
Add generic subtype guard around regex match in NOT EXISTS clause to
mirror the categorized_tips WHERE clause exactly. Prevents over-excluding
sales with specific subtypes like 'service_charge' that happen to have
tip-related account names.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: address PR review findings (security, double-counting, subtypes, pgTAP)
- Switch get_monthly_sales_metrics from SECURITY DEFINER to SECURITY
INVOKER with explicit auth check (CodeRabbit critical)
- Fix double-counting in useRevenueBreakdown.tsx fallback path: filter
out categorized adjustments before bucketing since they're already
merged into categoryMap via mergeCategorizedAdjustments
- Add other_current_liabilities and other_liabilities to GENERIC_SUBTYPES
across all 4 locations (TS + SQL) for broader POS compatibility
- Fix pgTAP test fixtures: tip account now uses account_type='liability',
account_subtype='tips' to match the new stricter matching logic
- Add unit tests for new generic subtypes
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: use valid enum values in pgTAP test and add payroll_liabilities to generic subtypes
account_subtype is an enum (account_subtype_enum) — 'tips' is not a
valid value. Real tip accounts use 'payroll_liabilities' with name
'Tips Payable'. Fix the pgTAP test fixture to use valid enum values
and add payroll_liabilities to GENERIC_SUBTYPES across all locations
so name-based tip matching works for real account data.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: prevent tip doubling in unified sales totals
The get_unified_sales_totals SQL function classified rows solely by
item_type, but adjustment rows (tip, tax, service_charge, fee) get
item_type='sale' by DEFAULT. This caused tips and taxes to be counted
as revenue instead of pass-through.
Changes:
- SQL migration: check adjustment_type before item_type in every CASE
branch so adjustment rows are correctly classified
- Client code: set item_type on each adjustment entry in
createManualSaleWithAdjustments (belt-and-suspenders)
- pgTAP test: 8 tests verifying revenue excludes adjustments
- E2E test: seed sale+tip+tax, verify totals on POS Sales page
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: set item_type on manual sale entries to prevent tip doubling
Manual sale entries via createManualSale and createManualSaleWithAdjustments
were not setting item_type, causing it to default to null. This meant:
- Revenue rows weren't counted as revenue (item_type != 'sale')
- Adjustment rows (tip, tax) could be misclassified
Changes:
- createManualSale: derive item_type from adjustment_type ('sale' for revenue)
- createManualSaleWithAdjustments: set item_type='sale' on main revenue entry
- E2E test: verifies manual sale with tip shows correct Revenue and Pass-Through
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: backfill item_type on existing unified_sales adjustment rows
Existing manual sale entries had item_type=NULL or 'sale' on adjustment
rows (tip, tax, etc.), causing SQL functions to misclassify them as
revenue. This migration sets item_type to match adjustment_type for all
affected rows, and sets item_type='sale' on revenue rows with NULL.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: extract isTipLiability helper to eliminate 5 inline duplicates
- Hoist regex to module-level constant in hasTipKeyword
- Add isTipLiability(subtype, accountName) to passThroughAdjustments.ts
- Replace 4 inline tip classification expressions in useRevenueBreakdown.tsx
- Replace 1 inline tip classification expression in useMonthlyMetrics.tsx
- Move imports to file top in periodMetrics.ts and monthlyMetrics.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: address CodeRabbit review findings (round 1)
- Fix misleading pgTAP test comment in get_pos_tips_by_date
- Add Number() coercion for total_price assertions in E2E test
- Remove unreachable dead code in voids CASE branch
- Replace weak Test 6 with "overtipped" (real embedded false positive)
- Add negative assertion for $64.00 in pos-sale-with-tip E2E test
- Remove flaky waitForTimeout before page.reload()
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: prevent double-counting of tax and tips in revenue breakdown
When no categorized liability accounts exist, ensureCategory creates
synthetic categories from adjustment data (e.g., $1 tax from
get_pass_through_totals). Then combinedTaxC added adjustmentTaxC
again: $1 + $1 = $2 (doubled).
Fix: track whether real categorized entries existed before ensureCategory.
Only add adjustment amounts when there were already categorized amounts
(meaning they come from different data sources). When ensureCategory
created the synthetic entry, the adjustment amount is already included.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: address PR #407 review comments
Major fixes:
- Add account_type='liability' guard to get_pos_tips_by_date (prevents
non-liability accounts from being matched as tips)
- Fix legacy discount fallback to require adjustment_type IS NULL
(prevents void rows from being counted as discounts)
- Add catch-all for unknown adjustment types in revenue breakdown
(parity between RPC and all-months code paths)
Minor fixes:
- Use stable composite keys in MonthlyBreakdownTable
- Make pgTAP test 35 fixtures idempotent with ON CONFLICT DO UPDATE
- Expand ON CONFLICT in pgTAP test 26 to include account_type/subtype
- Replace waitForTimeout with toPass() polling in E2E test
- Use local date formatting instead of UTC in E2E test
- Add ## Tasks heading to plan doc (fix MD001)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: use targeted REVENUE assertion instead of broad $64 text match
The $64.00 value legitimately appears as COLLECTED total on the POS sales
page. Changed assertion to verify REVENUE label specifically doesn't show
$64.00, rather than asserting the text doesn't exist anywhere on the page.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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
1 participant
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.
This pull request addresses inconsistencies in how "tip" liabilities are detected and reported throughout the codebase, and improves transparency for users by providing a detailed tip source breakdown in dashboard components. The changes standardize tip keyword matching logic, fix bugs in tip categorization, and enhance both the backend and frontend to ensure tip data is accurate and clearly explained.
Standardization and Bug Fixes:
hasTipKeywordandisTipLiability) to replace all substring-based logic, preventing false positives such as matching "Stipend" or "Participation" as tips. This is now used consistently in both TypeScript and SQL code. [1] [2] [3] [4] [5] [6]Frontend Enhancements for Tip Transparency:
MonthlyBreakdownTableandPnLIntelligenceReportcomponents to display a breakdown of tip sources, listing each tip-related account and any POS tip adjustments, so users can see exactly where tip totals originate. [1] [2] [3]Backend and Data Model Improvements:
useUnifiedSalesto set the newitem_typefield appropriately for all adjustment types, aligning with the new classification scheme. [1] [2] [3] [4] [5] [6] [7]These changes collectively fix longstanding bugs in tip categorization, make tip reporting transparent to users, and lay the groundwork for reliable financial reporting across the application.
Summary by CodeRabbit
Bug Fixes
New Features
Database
Tests