fix(stats): filter legacy download_fail logs#2323
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds semver-based gating for ChangesPlugin Stats Version Gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/stats.test.ts`:
- Line 432: The test declared with it('filters legacy download_fail before saved
stats and logs', ...) should be changed to run concurrently by replacing it(...)
with it.concurrent(...); locate the test function with that exact description in
stats.test.ts and update the test declaration to it.concurrent('filters legacy
download_fail before saved stats and logs', async () => { ... }) so the test
runs in parallel with other tests (no other code changes required since
device_id and version_build are already unique).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 246f2e3d-1472-445f-9b7b-1890945255bf
📒 Files selected for processing (2)
supabase/functions/_backend/plugins/stats.tstests/stats.test.ts
hadessunxy-code
left a comment
There was a problem hiding this comment.
Reviewed the legacy download_fail filter. The shared helper is a good direction: it keeps saved logs and fail aggregates aligned, and the threshold tests cover both v7 and v6 boundaries.
One edge case worth checking before merge: the filter currently runs after the version lookup, so legacy download_fail events can still fail with version_not_found before they are suppressed if the noisy report references a missing/unknown target version. If old clients only ever send an existing/current version_name, this is fine. If not, moving the download_fail suppression ahead of getAppVersionPostgres would make the ignore behavior complete and avoid batch items turning into errors for the same legacy noise this PR is trying to drop.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/stats.test.ts (1)
434-450: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a malformed/missing
plugin_versioncase to this matrix.
shouldRecordStatsAction()also treats unparseable or absentplugin_versionas legacy, but these cases only cover valid semver inputs. That leaves the "parse failed => suppressdownload_fail" branch untested, so a regression there could slip through without failing this suite.As per coding guidelines: "Use version detection based on plugin_version to enable backward-compatible behavior changes; assume old versions for safety when version parsing fails"
🤖 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/stats.test.ts` around lines 434 - 450, Add a test case to the cases matrix that simulates a malformed/missing plugin_version so the "parse failed => suppress download_fail" branch is exercised: insert an object like { pluginVersion: 'not-a-version', shouldRecord: false, createVersion: true } (or { pluginVersion: '', shouldRecord: false, createVersion: true } if empty-string is preferred) into the cases array used in tests/stats.test.ts; this ensures shouldRecordStatsAction() behavior for unparseable/missing plugin_version is validated without changing the surrounding test logic that sets baseData.plugin_version and version_build.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/stats.test.ts`:
- Around line 434-450: Add a test case to the cases matrix that simulates a
malformed/missing plugin_version so the "parse failed => suppress download_fail"
branch is exercised: insert an object like { pluginVersion: 'not-a-version',
shouldRecord: false, createVersion: true } (or { pluginVersion: '',
shouldRecord: false, createVersion: true } if empty-string is preferred) into
the cases array used in tests/stats.test.ts; this ensures
shouldRecordStatsAction() behavior for unparseable/missing plugin_version is
validated without changing the surrounding test logic that sets
baseData.plugin_version and version_build.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2514404-2165-4802-b967-2034beef18ca
📒 Files selected for processing (2)
supabase/functions/_backend/plugins/stats.tstests/stats.test.ts
|



Summary (AI generated)
download_failevents from updater plugins below7.17.0and6.14.25before writing user logs.download_failplugin versions.Motivation (AI generated)
Old updater plugins could report
download_failwhen there was no update to download. The aggregate fail counter already avoided this in one path, but raw user logs still saved the false failure, which could make chart drilldowns disagree with chart totals.Business Impact (AI generated)
Users see cleaner update failure reporting and fewer false alarms from old clients. This improves trust in Capgo update analytics and avoids wasting debugging time on non-actionable failures.
Test Plan (AI generated)
bunx eslint supabase/functions/_backend/plugins/stats.ts tests/stats.test.tsbun run lint:backendbunx vitest run tests/stats.test.tsGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests