Skip to content

Fix tip pooling: POS tips query and settings persistence - #361

Merged
jdelgado2002 merged 9 commits into
mainfrom
copilot/fix-tip-entry-issue
Feb 11, 2026
Merged

Fix tip pooling: POS tips query and settings persistence#361
jdelgado2002 merged 9 commits into
mainfrom
copilot/fix-tip-entry-issue

Conversation

Copilot AI commented Feb 10, 2026

Copy link
Copy Markdown
Contributor

Tip split screen showed "No POS tips found" despite tips existing in unified_sales. Settings dialog changes (e.g., switching to "POS Import") reverted on close/reopen.

Changes

SQL function enhancement

  • get_pos_tips_by_date now queries both categorized tips (unified_sales_splits) and uncategorized tips (unified_sales where item_type='tip')
  • Prevents double-counting via NOT EXISTS on existing splits
  • Toast/Square/Clover sync tips to unified_sales before manual categorization, making them invisible to the original query

Auto-save hook fix

  • Removed early return when settings is null, which blocked first-time setup
  • Change detection now handles both update (compare with existing) and create (check if configured) cases
// Before: blocked first-time saves
if (!settings) return;

// After: allows initial setup
const hasChanges = settings
  ? tipSource !== settings.tip_source || ...
  : selectedEmployees.size > 0;

Migration

Apply migration: npx supabase db push

Migration file: 20260210234900_fix_get_pos_tips_by_date.sql


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Summary by CodeRabbit

  • Bug Fixes

    • Resolved issue where POS tips were not displaying in the system.
    • Fixed settings not persisting during initial tip configuration setup.
  • Documentation

    • Added comprehensive visual guides detailing POS tip display fixes and settings persistence, including testing scenarios and verification steps.

@netlify

netlify Bot commented Feb 10, 2026

Copy link
Copy Markdown

Deploy Preview for easyshifthq ready!

Name Link
🔨 Latest commit 1678ca2
🔍 Latest deploy log https://app.netlify.com/projects/easyshifthq/deploys/698bf74809b58200086e4ec8
😎 Deploy Preview https://deploy-preview-361--easyshifthq.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 21
Accessibility: 98
Best Practices: 92
SEO: 98
PWA: 90
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@vercel

vercel Bot commented Feb 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
easyshifthq Ready Ready Preview, Comment Feb 11, 2026 3:28am

@supabase

supabase Bot commented Feb 10, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project ncdujvdgqtaunuyigflp because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

Copilot AI and others added 2 commits February 10, 2026 23:51
Co-authored-by: jdelgado2002 <2649871+jdelgado2002@users.noreply.github.com>
Co-authored-by: jdelgado2002 <2649871+jdelgado2002@users.noreply.github.com>
Co-authored-by: jdelgado2002 <2649871+jdelgado2002@users.noreply.github.com>
Co-authored-by: jdelgado2002 <2649871+jdelgado2002@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix issue with tips not displaying in tip split screen Fix tip pooling: POS tips query and settings persistence Feb 10, 2026
Copilot AI requested a review from jdelgado2002 February 10, 2026 23:59
@jdelgado2002
jdelgado2002 marked this pull request as ready for review February 11, 2026 00:01
@jdelgado2002

Copy link
Copy Markdown
Member

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Feb 11, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Feb 11, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

A database migration enhances the get_pos_tips_by_date function to aggregate tips from both categorized and uncategorized sources with deduplication, while a hook modification enables auto-save during first-time tip settings setup, fixing missing tips and persistence issues.

Changes

Cohort / File(s) Summary
Documentation
TIP_POOLING_FIX_SUMMARY.md, TIP_POOLING_FIX_VISUAL_GUIDE.md
Detailed documentation of the dual fixes (POS tip detection and settings persistence) including migration details, hook behavior changes, before/after visuals, test scenarios, and rollback guidance.
Auto-Save Hook
src/hooks/useAutoSaveTipSettings.ts
Modified change detection logic to support first-time setup by enabling save when selectedEmployees is populated (even if settings is null), while preserving existing field-by-field comparison for subsequent updates.
Database Migration
supabase/migrations/20260210234900_fix_get_pos_tips_by_date.sql
New PostgreSQL function aggregating tips from dual sources: categorized tips via unified_sales_splits and uncategorized tips from unified_sales with item_type='tip' or adjustment_type='tip', with deduplication to prevent double-counting, grouped by date and POS source.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • PR #285: Overlaps with modifications to src/hooks/useAutoSaveTipSettings.ts and its integration in tip pooling pages, indicating shared code paths.
  • PR #358: Modifies the same get_pos_tips_by_date RPC function for POS-categorized tips aggregation and integration.
  • PR #164: Complements this PR's uncategorized tip row processing from unified_sales by providing the upstream writes of tip adjustment rows into that table.

Poem

🐰 Hop hop, the tips now flow,
From both the splits and sales below,
First-time saves work just right,
No double-counts in sight!
Database magic, code delight!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix tip pooling: POS tips query and settings persistence' directly and specifically describes the main changes—fixing POS tips query retrieval and settings persistence issues.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch copilot/fix-tip-entry-issue

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Fix all issues with AI agents
In `@src/hooks/useAutoSaveTipSettings.ts`:
- Around line 34-44: When settings is null the current hasChanges only checks
selectedEmployees.size, so first-time changes to
tipSource/shareMethod/splitCadence (or roleWeights) are ignored; update the
null-settings branch in the hasChanges computation to also treat non-default
values as a change: check tipSource !== DEFAULT_TIP_SOURCE, shareMethod !==
DEFAULT_SHARE_METHOD, splitCadence !== DEFAULT_SPLIT_CADENCE, or
JSON.stringify(roleWeights) !== JSON.stringify(DEFAULT_ROLE_WEIGHTS) (or other
app defaults) OR the existing selectedEmployees check (use the same
JSON.stringify(Array.from(selectedEmployees).sort(...)) comparison used above
for enabled_employee_ids) so any non-default field triggers hasChanges when
settings is null.

In `@supabase/migrations/20260210234900_fix_get_pos_tips_by_date.sql`:
- Around line 69-72: The NOT EXISTS filter on unified_sales_splits (alias uss)
currently removes any unified_sales row (alias us) that has any split, causing
tip rows with non-tip splits to be excluded from both CTEs; change the condition
in both CTEs (uncategorized_tips and categorized_tips) so the NOT EXISTS only
excludes rows when there is an existing split that maps to a tip account (e.g.,
add the split account check such as uss.account_name ILIKE '%tip%' or the
equivalent account_id -> tip-account mapping) — i.e., make the WHERE clause in
the EXISTS/NOT EXISTS include the tip-account predicate so only tip-related
splits cause exclusion.
- Around line 11-93: Add pgTAP test coverage for the uncategorized_tips path in
get_pos_tips_by_date by inserting at least one unified_sales row for the target
restaurant with item_type='tip' or adjustment_type='tip' that has no
corresponding unified_sales_splits row (so it is uncategorized), then call
get_pos_tips_by_date and assert that the returned rows include that tip's date,
pos_system, and amount in the aggregated total_amount_cents and
transaction_count alongside existing categorized tips; reference the function
get_pos_tips_by_date and the uncategorized_tips CTE to locate where this
behavior should be validated.

In `@TIP_POOLING_FIX_VISUAL_GUIDE.md`:
- Around line 357-365: The documentation uses hardcoded commit SHAs in the git
revert examples (the lines showing "git revert 3e7537d", "git revert 07f1654",
"git revert 14a0d97"); update these examples to avoid brittle, branch-specific
hashes by recommending safer identifiers (PR number, branch name, or tag) or
noting that the SHAs are branch-specific and may change after rebase/squash;
e.g., replace the concrete SHAs with placeholders or an instruction like "git
revert <commit-or-PR>" and/or include an alternative workflow using a PR number
or tag for locating the commits.
🧹 Nitpick comments (4)
src/hooks/useAutoSaveTipSettings.ts (1)

39-41: JSON.stringify on objects is key-order-dependent.

JSON.stringify(roleWeights) vs JSON.stringify(settings.role_weights) will report a false change if the keys happen to be in a different insertion order. This would cause unnecessary saves. Consider sorting the keys before comparing:

Proposed fix
-        JSON.stringify(roleWeights) !== JSON.stringify(settings.role_weights) ||
+        JSON.stringify(Object.keys(roleWeights).sort().reduce((acc, k) => ({ ...acc, [k]: roleWeights[k] }), {})) !==
+          JSON.stringify(Object.keys(settings.role_weights || {}).sort().reduce((acc, k) => ({ ...acc, [k]: (settings.role_weights || {})[k] }), {})) ||

Alternatively, a shallow-comparison helper would be more readable. This is a minor edge-case but can cause phantom saves.

TIP_POOLING_FIX_SUMMARY.md (1)

261-269: Consider including these recommended indexes in the migration itself.

These indexes are documented as recommendations but aren't part of the actual migration file. If they're needed for performance (especially the NOT EXISTS subquery on unified_sales_splits(sale_id)), they should be added to the migration to ensure they're applied automatically. Using CREATE INDEX IF NOT EXISTS is safe for idempotency.

TIP_POOLING_FIX_VISUAL_GUIDE.md (1)

6-6: Add language identifiers to fenced code blocks (markdownlint MD040).

The ASCII-art diagram blocks are flagged by markdownlint for missing language specifiers. Adding ```text would silence the warnings while preserving rendering.

Also applies to: 31-31, 50-50, 79-79, 193-193, 220-220

supabase/migrations/20260210234900_fix_get_pos_tips_by_date.sql (1)

36-53: Consider adding 'tip' to the account_subtype_enum for more explicit categorization.

The current LIKE '%tip%' pattern on account_name and account_subtype works but relies on naming conventions. While the account_subtype::TEXT LIKE '%tip%' check is harmless (the enum has no matching values), the account_name check could theoretically match unintended strings if accounts are named poorly. The uncategorized_tips query already uses precise equals checks (item_type = 'tip' OR adjustment_type = 'tip'), so adding 'tip' as a formal value in account_subtype_enum would bring consistency and make tip classification explicit rather than implicit.

Comment thread src/hooks/useAutoSaveTipSettings.ts Outdated
Comment on lines +34 to +44
const hasChanges = settings
? // Compare with existing settings
tipSource !== settings.tip_source ||
shareMethod !== settings.share_method ||
splitCadence !== settings.split_cadence ||
JSON.stringify(roleWeights) !== JSON.stringify(settings.role_weights) ||
JSON.stringify(Array.from(selectedEmployees).sort((a, b) => a.localeCompare(b))) !==
JSON.stringify((settings.enabled_employee_ids || []).sort((a, b) => a.localeCompare(b)))
: // No settings exist - trigger save if user has selected non-default values
// Only save if user has actually interacted (e.g., changed from default 'manual')
selectedEmployees.size > 0; // If employees selected, assume user has configured

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, find and examine the useAutoSaveTipSettings.ts file
fd "useAutoSaveTipSettings" --type f

Repository: toyiyo/nimble-pnl

Length of output: 181


🏁 Script executed:

#!/bin/bash
# Get the full file to understand context and defaults
find . -name "useAutoSaveTipSettings.ts" -type f -exec wc -l {} \;

Repository: toyiyo/nimble-pnl

Length of output: 101


🏁 Script executed:

#!/bin/bash
# Look for where default values might be defined (initial state, form defaults, etc.)
rg "tipSource.*manual|tipSource.*default|tipSource.*=.*'|shareMethod.*equal|shareMethod.*default|splitCadence.*daily|splitCadence.*default" --type=ts -B2 -A2

Repository: toyiyo/nimble-pnl

Length of output: 18215


🏁 Script executed:

#!/bin/bash
# Check if unit tests exist for this hook
find . -path "*/tests/unit/*" -name "*useAutoSaveTipSettings*" -o -path "*/tests/*" -name "*useAutoSaveTipSettings*"

Repository: toyiyo/nimble-pnl

Length of output: 149


🏁 Script executed:

#!/bin/bash
# Also check for any .test.ts files related to tip settings
fd ".*useAutoSaveTipSettings.*test" --type f

Repository: toyiyo/nimble-pnl

Length of output: 145


🏁 Script executed:

#!/bin/bash
cat -n src/hooks/useAutoSaveTipSettings.ts

Repository: toyiyo/nimble-pnl

Length of output: 2212


🏁 Script executed:

#!/bin/bash
# Get the test file to see what scenarios are covered
cat -n tests/unit/useAutoSaveTipSettings.test.ts

Repository: toyiyo/nimble-pnl

Length of output: 12436


🏁 Script executed:

#!/bin/bash
# Find type definitions for ShareMethod to see valid values
rg "type ShareMethod|enum ShareMethod|shareMethod.*=" --type=ts -B2 -A5 | head -40

Repository: toyiyo/nimble-pnl

Length of output: 3257


🏁 Script executed:

#!/bin/bash
# Also check if 'equal' is used anywhere as a shareMethod value
rg "'equal'|\"equal\"" --type=ts -B2 -A2 | grep -i "share" | head -20

Repository: toyiyo/nimble-pnl

Length of output: 43


🏁 Script executed:

#!/bin/bash
# Find the exact ShareMethod type definition
fd "useTipPoolSettings" --type f | xargs grep -l "type ShareMethod\|export.*ShareMethod"

Repository: toyiyo/nimble-pnl

Length of output: 93


🏁 Script executed:

#!/bin/bash
# Get the type definitions file
find . -name "useTipPoolSettings*" -type f

Repository: toyiyo/nimble-pnl

Length of output: 95


🏁 Script executed:

#!/bin/bash
cat -n src/hooks/useTipPoolSettings.tsx | head -60

Repository: toyiyo/nimble-pnl

Length of output: 2336


First-time save won't trigger if user only changes tipSource without selecting employees.

When settings is null, hasChanges is solely determined by selectedEmployees.size > 0 (Line 44). If a first-time user changes "Tip Source" to "POS Import" but hasn't yet selected any employees, the setting change is silently dropped — which is exactly the original bug scenario described in the PR.

Consider also checking for non-default tipSource, shareMethod, and splitCadence in the null-settings branch:

Proposed fix
-      : // No settings exist - trigger save if user has selected non-default values
-        // Only save if user has actually interacted (e.g., changed from default 'manual')
-        selectedEmployees.size > 0; // If employees selected, assume user has configured
+      : // No settings exist - trigger save if user has configured anything
+        selectedEmployees.size > 0 ||
+        tipSource !== 'manual' ||
+        shareMethod !== 'hours' ||
+        splitCadence !== 'daily';
📝 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 hasChanges = settings
? // Compare with existing settings
tipSource !== settings.tip_source ||
shareMethod !== settings.share_method ||
splitCadence !== settings.split_cadence ||
JSON.stringify(roleWeights) !== JSON.stringify(settings.role_weights) ||
JSON.stringify(Array.from(selectedEmployees).sort((a, b) => a.localeCompare(b))) !==
JSON.stringify((settings.enabled_employee_ids || []).sort((a, b) => a.localeCompare(b)))
: // No settings exist - trigger save if user has selected non-default values
// Only save if user has actually interacted (e.g., changed from default 'manual')
selectedEmployees.size > 0; // If employees selected, assume user has configured
const hasChanges = settings
? // Compare with existing settings
tipSource !== settings.tip_source ||
shareMethod !== settings.share_method ||
splitCadence !== settings.split_cadence ||
JSON.stringify(roleWeights) !== JSON.stringify(settings.role_weights) ||
JSON.stringify(Array.from(selectedEmployees).sort((a, b) => a.localeCompare(b))) !==
JSON.stringify((settings.enabled_employee_ids || []).sort((a, b) => a.localeCompare(b)))
: // No settings exist - trigger save if user has configured anything
selectedEmployees.size > 0 ||
tipSource !== 'manual' ||
shareMethod !== 'hours' ||
splitCadence !== 'daily';
🤖 Prompt for AI Agents
In `@src/hooks/useAutoSaveTipSettings.ts` around lines 34 - 44, When settings is
null the current hasChanges only checks selectedEmployees.size, so first-time
changes to tipSource/shareMethod/splitCadence (or roleWeights) are ignored;
update the null-settings branch in the hasChanges computation to also treat
non-default values as a change: check tipSource !== DEFAULT_TIP_SOURCE,
shareMethod !== DEFAULT_SHARE_METHOD, splitCadence !== DEFAULT_SPLIT_CADENCE, or
JSON.stringify(roleWeights) !== JSON.stringify(DEFAULT_ROLE_WEIGHTS) (or other
app defaults) OR the existing selectedEmployees check (use the same
JSON.stringify(Array.from(selectedEmployees).sort(...)) comparison used above
for enabled_employee_ids) so any non-default field triggers hasChanges when
settings is null.

Comment on lines +11 to +93
CREATE OR REPLACE FUNCTION get_pos_tips_by_date(
p_restaurant_id UUID,
p_start_date DATE,
p_end_date DATE
)
RETURNS TABLE (
tip_date DATE,
total_amount_cents INTEGER,
transaction_count INTEGER,
pos_source TEXT
)
LANGUAGE plpgsql
SECURITY INVOKER
AS $$
BEGIN
-- Authorization check
IF NOT EXISTS (
SELECT 1 FROM user_restaurants
WHERE restaurant_id = p_restaurant_id
AND user_id = auth.uid()
) THEN
RAISE EXCEPTION 'Access denied: User does not have access to restaurant %', p_restaurant_id;
END IF;

RETURN QUERY
WITH categorized_tips AS (
-- Tips that have been categorized in splits
SELECT
us.sale_date AS tip_date,
SUM(uss.amount * 100)::INTEGER AS total_amount_cents,
COUNT(DISTINCT us.external_order_id)::INTEGER AS transaction_count,
us.pos_system AS pos_source
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 (
LOWER(COALESCE(coa.account_name, '')) LIKE '%tip%'
OR LOWER(COALESCE(coa.account_subtype::TEXT, '')) LIKE '%tip%'
)
GROUP BY us.sale_date, us.pos_system
),
uncategorized_tips AS (
-- Tips that haven't been categorized yet (item_type='tip' or adjustment_type='tip')
-- Exclude items that already have splits to avoid double-counting
SELECT
us.sale_date AS tip_date,
SUM(COALESCE(us.total_price, us.unit_price * us.quantity, 0) * 100)::INTEGER AS total_amount_cents,
COUNT(DISTINCT us.external_order_id)::INTEGER AS transaction_count,
us.pos_system AS pos_source
FROM unified_sales us
WHERE us.restaurant_id = p_restaurant_id
AND us.sale_date >= p_start_date
AND us.sale_date <= p_end_date
AND (us.item_type = 'tip' OR us.adjustment_type = 'tip')
-- Exclude items that have already been categorized (have splits)
AND NOT EXISTS (
SELECT 1 FROM unified_sales_splits uss
WHERE uss.sale_id = us.id
)
GROUP BY us.sale_date, us.pos_system
),
combined_tips AS (
-- Combine both sources
SELECT tip_date, total_amount_cents, transaction_count, pos_source
FROM categorized_tips
UNION ALL
SELECT tip_date, total_amount_cents, transaction_count, pos_source
FROM uncategorized_tips
)
-- Aggregate by date and POS system
SELECT
ct.tip_date,
SUM(ct.total_amount_cents)::INTEGER AS total_amount_cents,
SUM(ct.transaction_count)::INTEGER AS transaction_count,
ct.pos_source
FROM combined_tips ct
GROUP BY ct.tip_date, ct.pos_source
ORDER BY ct.tip_date DESC;
END;
$$;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check for existing pgTAP tests related to this function
rg "get_pos_tips_by_date" --type=sql

# List test files in supabase/tests
fd . supabase/tests/ --type f --extension sql 2>/dev/null | head -20

# Check if there's a test file for this migration
ls -la supabase/tests/ 2>/dev/null | head -20

Repository: toyiyo/nimble-pnl

Length of output: 4509


🏁 Script executed:

cat supabase/tests/26_get_pos_tips_by_date.sql

Repository: toyiyo/nimble-pnl

Length of output: 6668


🏁 Script executed:

# Check unified_sales table definition for item_type and adjustment_type
rg "CREATE TABLE.*unified_sales" -A 20 supabase/migrations/

# Also check if test data includes uncategorized items
grep -n "item_type\|adjustment_type" supabase/tests/26_get_pos_tips_by_date.sql

Repository: toyiyo/nimble-pnl

Length of output: 4704


🏁 Script executed:

# Search for item_type and adjustment_type in all migrations
rg "item_type|adjustment_type" supabase/migrations/ -A 2 -B 2

# Also check the specific migration file being reviewed
grep -n "item_type\|adjustment_type" supabase/migrations/20260210234900_fix_get_pos_tips_by_date.sql

Repository: toyiyo/nimble-pnl

Length of output: 43264


🏁 Script executed:

# Check the test data more carefully - look for items without splits
grep -A 5 "INSERT INTO unified_sales" supabase/tests/26_get_pos_tips_by_date.sql

# Also check if any test items are inserted with item_type or adjustment_type='tip'
grep -E "item_type|adjustment_type" supabase/tests/26_get_pos_tips_by_date.sql

# Check the comment at the start of the migration to understand what's being fixed
head -15 supabase/migrations/20260210234900_fix_get_pos_tips_by_date.sql

Repository: toyiyo/nimble-pnl

Length of output: 2034


Test coverage for uncategorized tips scenario is missing.

While pgTAP tests exist for this function, they don't cover the core new functionality added by this migration. The uncategorized_tips CTE (lines 56-70) handles items with item_type='tip' or adjustment_type='tip' that haven't been categorized yet—which is the entire purpose of this fix. Current test data only exercises the categorized_tips path; all test items have splits and thus never trigger the uncategorized path.

Add test fixture data with at least one uncategorized tip item (no splits, with item_type='tip' or adjustment_type='tip') and verify it's correctly aggregated alongside categorized tips.

🤖 Prompt for AI Agents
In `@supabase/migrations/20260210234900_fix_get_pos_tips_by_date.sql` around lines
11 - 93, Add pgTAP test coverage for the uncategorized_tips path in
get_pos_tips_by_date by inserting at least one unified_sales row for the target
restaurant with item_type='tip' or adjustment_type='tip' that has no
corresponding unified_sales_splits row (so it is uncategorized), then call
get_pos_tips_by_date and assert that the returned rows include that tip's date,
pos_system, and amount in the aggregated total_amount_cents and
transaction_count alongside existing categorized tips; reference the function
get_pos_tips_by_date and the uncategorized_tips CTE to locate where this
behavior should be validated.

Comment on lines +69 to +72
AND NOT EXISTS (
SELECT 1 FROM unified_sales_splits uss
WHERE uss.sale_id = us.id
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Potential gap: tip row with a non-tip split is excluded from both CTEs.

The NOT EXISTS check excludes any unified_sales tip row that has any split, regardless of whether that split maps to a tip account. If someone miscategorizes a tip record (e.g., assigns it to a "food" account), it will have a split → excluded from uncategorized_tips, but the split's account won't match '%tip%' → excluded from categorized_tips too. The tip silently disappears.

Consider narrowing the exclusion to tip-related splits only:

Proposed fix
       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 (
+          LOWER(COALESCE(coa.account_name, '')) LIKE '%tip%'
+          OR LOWER(COALESCE(coa.account_subtype::TEXT, '')) LIKE '%tip%'
+        )
       )

This keeps the tip in the uncategorized CTE until it's specifically categorized as a tip, preventing silent data loss from miscategorization.

📝 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
AND NOT EXISTS (
SELECT 1 FROM unified_sales_splits uss
WHERE uss.sale_id = us.id
)
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 (
LOWER(COALESCE(coa.account_name, '')) LIKE '%tip%'
OR LOWER(COALESCE(coa.account_subtype::TEXT, '')) LIKE '%tip%'
)
)
🤖 Prompt for AI Agents
In `@supabase/migrations/20260210234900_fix_get_pos_tips_by_date.sql` around lines
69 - 72, The NOT EXISTS filter on unified_sales_splits (alias uss) currently
removes any unified_sales row (alias us) that has any split, causing tip rows
with non-tip splits to be excluded from both CTEs; change the condition in both
CTEs (uncategorized_tips and categorized_tips) so the NOT EXISTS only excludes
rows when there is an existing split that maps to a tip account (e.g., add the
split account check such as uss.account_name ILIKE '%tip%' or the equivalent
account_id -> tip-account mapping) — i.e., make the WHERE clause in the
EXISTS/NOT EXISTS include the tip-account predicate so only tip-related splits
cause exclusion.

Comment thread TIP_POOLING_FIX_VISUAL_GUIDE.md Outdated
Comment on lines +357 to +365
```bash
# Revert code changes
git revert 3e7537d # Revert documentation
git revert 07f1654 # Revert auto-save fix
git revert 14a0d97 # Revert SQL migration

# Or restore SQL function manually
npx supabase db execute "$(cat rollback_script.sql)"
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Hardcoded commit hashes will become stale after squash/merge.

Lines 359-361 reference specific commit SHAs for git revert. These hashes will change if the branch is rebased or commits are squashed on merge. Consider referencing the PR number or a tag instead, or noting that the hashes are branch-specific.

🤖 Prompt for AI Agents
In `@TIP_POOLING_FIX_VISUAL_GUIDE.md` around lines 357 - 365, The documentation
uses hardcoded commit SHAs in the git revert examples (the lines showing "git
revert 3e7537d", "git revert 07f1654", "git revert 14a0d97"); update these
examples to avoid brittle, branch-specific hashes by recommending safer
identifiers (PR number, branch name, or tag) or noting that the SHAs are
branch-specific and may change after rebase/squash; e.g., replace the concrete
SHAs with placeholders or an instruction like "git revert <commit-or-PR>" and/or
include an alternative workflow using a PR number or tag for locating the
commits.

…erage

- Auto-save hook: check all fields (tipSource, shareMethod, splitCadence)
  against defaults on first-time setup, not just employee selection
- SQL function: narrow NOT EXISTS to only exclude tips with tip-account
  splits, preventing miscategorized tips from silently disappearing
- Fix plpgsql column name ambiguity (tip_date output param vs CTE alias)
- Add 4 pgTAP tests: uncategorized tips, adjustment_type tips,
  miscategorized tips, and double-counting prevention
- Delete excessive copilot-generated doc files

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test was passing non-default values (selectedEmployees with entries)
while expecting no save to trigger. With the fix that detects non-default
field values on first-time setup, this correctly triggers a save now.
Updated test to use all-default values and added a new test verifying
that changing tipSource to 'pos' does trigger a first-time save.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… UTC

Toast provides businessDate (Order) and paidBusinessDate (Payment) as
YYYYMMDD integers representing the restaurant's actual business day,
accounting for the closeout hour (default 4 AM). The sync was using
UTC-derived dates from closedDate/paidDate, causing late-night orders
and tips to appear on the next calendar day.

- Add parseBusinessDate() to convert YYYYMMDD integer to ISO date
- Prefer order.businessDate over closedDate in parseOrderDateTime()
- Use payment.paidBusinessDate for payment_date in upsertPayment()
- Migration fixes existing toast_payments, toast_orders, and
  unified_sales dates from raw_json business date fields

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@jdelgado2002
jdelgado2002 merged commit c8e1d8c into main Feb 11, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants