fix(feed): preserve nested engagement count fallback

[NotThatKindOfDrLiz]

Closes #4496

Summary

• preserve nested bulk-stats fallback when a top-level engagement field is present but invalid
• keep UI normalization behavior for real engagement values while allowing deep search to continue on missing or unusable values
• add a regression test for invalid top-level counts with valid nested stats

Context

• follow-up to merged PR fix(feed): normalize invalid engagement counts #4455
• fixes the remaining bulk-stats parsing regression on main where invalid top-level engagement values could mask valid nested stats

Verification

• flutter test test/src/bulk_video_stats_entry_test.dart test/src/video_stats_test.dart (mobile/packages/models)
• pre-push hook analyzer + changed-file tests

[github-actions]

Mobile PR Preview

Preview refreshed for 00aa0ed

Last refresh: 00aa0ed at 2026-05-17 23:38:59 UTC (preview run)

Property	Value
Preview URL	https://f15ec7ff.openvine-app.pages.dev
Pages project	openvine-app
Preview branch	pr-4495
PR branch	fix/engagement-fallback
Commit	00aa0ed

[rabble]

Review: PR #4495 — preserve nested engagement count fallback

Verdict: Approve with a small note.

What the PR does

In _findEngagementCountDeep (the recursive search used for reactions / comments / reposts), a top-level key match that returned null from tryParseEngagementCount was still terminating the loop iteration without falling through to the recursive nested-stats lookup. This PR splits parseEngagementCount into a non-zeroing tryParseEngagementCount and wires the deep-search to skip unusable matches so nested fields can still be discovered. Targeted regression test (comments: '' at top level, stats: { comments: 5 }) lands in the right place.

Code correctness

• The fix is in the right layer (packages/models, used by repositories) — not BLoC / UI, so it satisfies the architecture rule that fallback/source-selection lives below the BLoC.
• tryParseEngagementCount cleanly factors out the "is this value usable?" decision; parseEngagementCount keeps its current zero-normalization for the UI surface. No behavior change for the public parseEngagementCount call sites.
• Legitimate-zero handling is preserved. A top-level comments: 0 parses to 0 (≥ 0), so tryParseEngagementCount returns 0, _findEngagementCountDeep returns at that match, and a different nested 5 is correctly ignored. The fallthrough only kicks in for unparseable values (empty string, negative, sentinel max-int), not for real zeros. This is the right behavior and aligns with #4477's "carried count survives a relay-emitted zero" pattern at the higher layer — this PR doesn't regress it.
• The same fallthrough pattern is not applied to _findIntDeep (used for loops / views). That's defensible — those fields are nullable and a sentinel/invalid value there is unlikely to mask a nested real value in current API shapes — but if the same bug shape appears for views later, the parallel fix will need to be applied there. Worth a follow-up issue if there's any field data showing this. Not blocking.

Tests

• Regression test covers the nested-present-while-top-level-invalid case. ✅
• The existing handles nested stats test continues to cover nested-only. ✅
• Gap (nit, non-blocking): no test explicitly pins the legitimate-zero contract — i.e., top-level comments: 0 with nested stats: { comments: 5 } should resolve to 0, not 5. The current code is correct, but without a test someone refactoring _findEngagementCountDeep could easily flip this and not notice. Worth a one-liner addition.
• Direct unit tests for tryParseEngagementCount itself (returns null for '', -1, sentinels; returns 0 for '0'; returns parsed value otherwise) would be nice given it's now public API of the parser file, but coverage is achieved via the bulk-stats tests.

Style / project conventions

• Diff is tiny, scoped, no unrelated churn.
• Doc comment on tryParseEngagementCount explains the "keep looking" intent — good.
• No l10n / theming surface touched; checklist N/A.
• No // TODO / dead code / commented blocks introduced.

Performance / security

• Negligible perf impact (one extra null check per top-level match).
• No security surface.

Suggestion

Add a single test asserting the legitimate-zero contract so the precedence rule is locked in:

test('top-level zero wins over nested non-zero (legitimate zero)', () {
  final entry = BulkVideoStatsEntry.fromJson(const {
    'event_id': 'test',
    'comments': 0,
    'stats': {'comments': 5},
  });
  expect(entry.comments, equals(0));
});

LGTM otherwise.