fix(focus): cut datafeed parse CPU ~10x + sync error hygiene - #572

supabase/functions/_shared/focusTransactionSyncHandler.ts (1)

354-360: 🗄️ Data Integrity & Integration | 🔵 Trivial

Ensure the cron job is deployed and monitored so unified_sales aggregation isn't silently dropped.

Removing the in-worker RPC makes the range path fully dependent on the external focus-transactions-unified-sales-sync pg_cron job. A successful range sync now returns status: 'ok' while unified_sales may lag by up to ~5 minutes (or indefinitely if the cron is disabled/failing). Consider adding alerting on cron freshness/failure and confirming the job is provisioned in every environment (including local/preview) that exercises this path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/functions/_shared/focusTransactionSyncHandler.ts` around lines 354 -
360, The range-sync path in focusTransactionSyncHandler now depends entirely on
the external focus-transactions-unified-sales-sync pg_cron job because the
in-worker unified_sales RPC was removed. Update the deployment/ops setup around
sync_all_focus_transactions_to_unified_sales and the custom-range flow so the
cron job is provisioned in every environment that uses it, and add
monitoring/alerting for cron freshness or failures so unified_sales lag is
visible instead of silent. Keep the handler behavior change intact, but make
sure the surrounding deployment and observability for the cron-backed
aggregation are explicitly configured.

tests/unit/focusSyncDataHandler.test.ts (1)

940-988: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Missing coverage for the legacy portal path's error-state behavior.

The new "error hygiene" suite only covers the Lynk incremental/backfill success paths. There's no test asserting the portal path's behavior on success, nor (more importantly) on a non-auth failure (processReportDay returning status: 'error') — which per the sibling comment on focusSyncDataHandler.ts currently never writes connection_status: 'error' to the DB in that case. Adding such a test would have caught the gap.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/focusSyncDataHandler.test.ts` around lines 940 - 988, Add coverage
for the legacy portal path in the error hygiene suite: the current tests only
verify Lynk success cases, so extend focusSyncDataHandler.test.ts to exercise
the portal flow via handleSyncData and makeProcessReportDay/processReportDay.
Add a success test that confirms connection_status becomes "connected" and
last_error fields clear, and add a non-auth failure test where processReportDay
returns status: "error" to assert the DB update still writes
connection_status="error" with the expected last_error values. Use the existing
handleSyncData, makeDeps, and mocks.updateMock patterns to locate and verify the
portal branch.

tests/unit/focusDatafeedParser.test.ts (2)

93-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

No test coverage for tag-attribute or self-closing <Checks> variants.

The new tests validate the fast-path against the fixture wrapped in large padding, and against a feed with no <Checks> tag at all — but there's no test for <Checks> appearing with attributes (e.g. <Checks count="3">) or as a self-closing empty tag (<Checks/>), which are the exact cases where extractChecksBlock's literal indexOf('<Checks>') match would silently misclassify a real payload as "config-only" (see companion comment on focusDatafeedParser.ts). Recommend adding a regression test once the real feed's tag shape is confirmed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/focusDatafeedParser.test.ts` around lines 93 - 117, Add regression
coverage in focusDatafeedParser.test for the exact <Checks> shapes that
extractChecksBlock matches literally: a real payload with <Checks> carrying
attributes (for example via parseFocusDatafeed on a fixture variant) and a
self-closing <Checks/> case. Use parseFocusDatafeed and wrapWithLargeConfig to
verify these inputs are not treated as config-only and that the resulting
checks/deletedCheckIds match the expected behavior from the existing
fixture-based assertions.

---

99-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor: shadowed wrapped variable.

wrapped at line 99 (the wrapped XML string) is shadowed by an inner wrapped (line 110, the matched check object) inside the for loop. Block-scoped const makes this legal, but the reuse of the name is confusing to read.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/focusDatafeedParser.test.ts` around lines 99 - 117, The test in
focusDatafeedParser.test.ts uses the name wrapped for both the wrapped XML
string and the matched check inside the loop, which is confusing. Rename the
inner const in the for-of loop in the parseFocusDatafeed test to a clearer,
distinct identifier (for example, matchedCheck) and update the subsequent
assertions to use that name, keeping the outer wrapped value unchanged.

supabase/functions/_shared/focusDatafeedParser.ts (1)

225-236: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Silent fallback failures — no observability into parse errors.

Both catch blocks (wrapped-parse failure and full-parse failure) swallow the error without logging, unlike the existing console.warn a few lines below (line 242) for dropped checks. If the wrapped sub-document or even the full document becomes unparseable in production, this will silently degrade to zero checks for that day with no trace in logs.

♻️ Add logging to both fallback paths

   let doc: any;
   try {
     doc = parser.parse(wrappedXml);
-  } catch {
+  } catch (err) {
+    console.warn('focusDatafeedParser: wrapped Checks-block parse failed, falling back to full parse', err);
     // Extraction produced unparseable XML (should not happen with well-formed
     // feeds but guards against edge-cases). Fall back to the full document.
     try {
       doc = parser.parse(xml);
-    } catch {
+    } catch (fullErr) {
+      console.warn('focusDatafeedParser: full-document parse also failed, returning empty result', fullErr);
       return { checks: [], deletedCheckIds: [] };
     }
   }

Based on retrieved learnings, console.* calls are the canonical observability sink for Supabase Edge Functions in this repo.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/functions/_shared/focusDatafeedParser.ts` around lines 225 - 236,
The wrapped XML and full XML parse fallbacks in focusDatafeedParser.ts currently
swallow parser errors silently; add console.* logging in both catch paths so
parse failures are observable in Supabase Edge Functions. In the
parser.parse(wrappedXml) fallback, log the wrapped-parse error before retrying
with xml, and in the parser.parse(xml) fallback, log the final parse failure
before returning empty checks. Use the existing parse flow around doc,
wrappedXml, xml, and parser.parse to keep the logs tied to the failure context.

Source: Learnings