Skip to content

chore(infinite-video-feed): delete the loop seek nothing calls - #6489

Merged
hm21 merged 3 commits into
mainfrom
chore/drop-dead-loop-enforcement
Jul 29, 2026
Merged

chore(infinite-video-feed): delete the loop seek nothing calls#6489
hm21 merged 3 commits into
mainfrom
chore/drop-dead-loop-enforcement

Conversation

@hm21

@hm21 hm21 commented Jul 29, 2026

Copy link
Copy Markdown
Member

Description

PR #5551 fixed the audible loop seam (#5544) by stopping the feed from passing maxLoopDuration — restarting a video with a Dart seekTo(Duration.zero) at the 6.3s recording limit both truncated classic Vines and left a seam at the join. The fix was made at the call site; the mechanism itself stayed behind, reachable through a public widget parameter and kept alive only by its own unit tests. A code comment was the only thing standing between us and a regression of a bug users have now reported three separate times (#5544, #6386, #6442). Seamless looping is the thing people cite as what made Vine feel like Vine, so the guard should be structural rather than advisory.

This deletes the machinery:

  • subscribeToLoopEnforcement and its _loop subscription map, plus the matching cancel/dispose handling. This was the seam-producing seek.
  • InfiniteVideoFeed.maxLoopDuration — field, constructor parameter, and the if (maxLoopDuration != null) wiring block — along with the now-orphaned _loopSeekInProgress guard set.
  • The maxLoopDuration plumbing in subscribeToAutoAdvance. Auto-advance detection itself is live and needed; the parameter only fed effectiveEnd = min(maxLoopDuration, duration), and with it always null that reduces to effectiveEnd = duration, collapsing four dead branches across the initial-state and stream-listener paths.

Behaviour-neutral on main by construction — every deleted branch was already unreachable, because no production code supplied the parameter.

The 7s playback cap is untouched. It is a separate mechanism: maxPlaybackDuration (AppConstants.maxFeedPlaybackDuration), applied as a native clip end (VideoClip.end) so the loop point stays inside the platform player. Both tests that pin it — the per-VideoClip cap-parity check and the app-side wiring assertion — are left in place.

Two deviations from the issue's proposed change, both deliberate:

  • The issue asks to delete the explanatory comment at feed_videos.dart:367. Half of it is live rationale for why maxPlaybackDuration exists and why the cap sits above the recording limit, so only the "deliberately NOT maxLoopDuration" warning is gone.
  • The issue asks to retire the source-string assertion in feed_looping_contract_test.dart. The maxLoopDuration half is retired — the type system enforces it now — but it is replaced with isNot(contains('seekTo(Duration.zero)')). That is a different property, not type-enforced, and it is the tripwire the issue actually wants: someone hand-rolling a Dart loop-restart trips it. It scans all three layers that hold a controller — the pooled player, controller_subscriptions.dart (where the deleted seek's stream listener lived), and the app-side call site — because scanning only the file the wiring sat in would have let the same seek back in one file over. Verified it can fail from both directions: the repo's dart_code_only.awk filter over origin/main's copy of the pooled player hits the string once, and injecting the seek into subscribeToAutoAdvance turns the test red on the service path.

One test is added rather than deleted. Collapsing effectiveEnd rewrote subscribeToAutoAdvance's arming condition, and that condition had no negative case — replacing it with a bare armed = true; left the suite green. Since onVideoLoopCompleted drives auto-advance, an over-eager arm would skip the viewer to the next video on any backward jump, including the stale-playback recovery seek. The new case pins it: a jump back from mid-clip must not fire.

Also corrected a stale doc line in fullscreen_feed_bloc.dart that still credited "loop enforcement" to the FeedVideos player configuration.

Related Issue: Closes #6445

The tripwire's comment also names what it does not catch, from review: it matches the literal spelling, and _seekKick already seeks with a computed duration, so widening to a seekTo( regex would collide with stale-playback recovery. seekTo(const Duration()) reaches zero unseen. It is a tripwire for the regression that shipped three times, not a proof.

Out of Scope

Verification

  • flutter analyze lib test integration_test — clean
  • flutter test --coverage in mobile/packages/infinite_video_feed — 228/228 pass, 726/726 lines (100%, the package's effective floor)
  • flutter test test/widgets/video_feed_item/ test/blocs/fullscreen_feed/ — 394/394 pass
  • git grep maxLoopDuration returns nothing across the repo
  • Mutation-checked both new guards: a bare armed = true; fails only the new arming case, and a seekTo(Duration.zero) injected into subscribeToAutoAdvance fails the widened contract test
  • Pre-push hook green
  • Manual check on a real Samsung Galaxy: not run, deliberately. The sole InfiniteVideoFeed construction site never supplied maxLoopDuration, so every deleted branch was already unreachable in a shipped build, and the surviving conditions are algebraically identical to the ones they replace. There is no runtime path here for a device to exercise that CI does not. Recording it rather than implying otherwise.

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

PR #5551 stopped the feed passing maxLoopDuration, because restarting a
video with a Dart seek at the 6.3s recording limit both truncated classic
Vines and left an audible seam. The mechanism itself stayed, reachable
through a public widget parameter and held alive only by its own tests —
so a comment was all that stood between us and a regression users have
now reported three times.

Deleting subscribeToLoopEnforcement and the maxLoopDuration parameter
makes the guard structural: there is no longer anything to pass. With the
parameter gone, subscribeToAutoAdvance's effectiveEnd reduces to the
clip's own duration, which the 7s native clip end has already shortened,
so its four dead branches collapse too.

Closes #6445
@hm21 hm21 self-assigned this Jul 29, 2026
@github-actions

This comment has been minimized.

…iring

The tripwire this branch added only scanned the file the deleted wiring
sat in. The seek itself fired from the stream listener in
controller_subscriptions.dart, so the same restart could come back one
file over and stay green. Scan every layer that holds a controller.

Collapsing effectiveEnd also rewrote subscribeToAutoAdvance's arming
condition, which had no negative case — replacing it with a bare
`armed = true;` left all 17 tests passing. onVideoLoopCompleted drives
auto-advance, so an over-eager arm skips the viewer to the next video on
any backward jump, including the stale-playback recovery seek. Pin it.
@hm21
hm21 marked this pull request as ready for review July 29, 2026 15:43
@github-actions

This comment has been minimized.

@realmeylisdev realmeylisdev 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.

Verified the deletion is behaviour-neutral: with maxLoopDuration always null, the old effectiveEnd reduced to duration on both the initial-state and listener paths, so the collapsed conditions are equivalent. Re-ran both mutations locally — a bare armed = true; in either arming path fails only the new mid-clip case, and a seekTo(Duration.zero) injected into controller_subscriptions.dart turns the widened contract test red. Package CI and all shards green.

Two non-blocking notes inline.

Comment thread mobile/lib/blocs/fullscreen_feed/fullscreen_feed_bloc.dart
Comment thread mobile/test/widgets/video_feed_item/feed_looping_contract_test.dart
A string guard reads as stronger than it is. `_seekKick` already seeks
with a computed duration, so the regex cannot be widened without matching
it — which leaves `seekTo(const Duration())` unseen. Name that in the
test, so the next reader treats it as a tripwire rather than a proof.
@github-actions

Copy link
Copy Markdown

Mobile PR Preview

Preview refreshed for d6dacd2

Last refresh: d6dacd2 at 2026-07-29 19:01:22 UTC (preview run)

Property Value
Preview URL https://56c39567.openvine-app.pages.dev
Pages project openvine-app
Preview branch pr-6489
PR branch chore/drop-dead-loop-enforcement
Commit d6dacd2

@hm21
hm21 merged commit 291c416 into main Jul 29, 2026
16 checks passed
@hm21
hm21 deleted the chore/drop-dead-loop-enforcement branch July 29, 2026 19:01
hm21 added a commit that referenced this pull request Jul 31, 2026
The private _LoadingIndicator/_LoadingIndicatorState pair in
pooled_fullscreen_video_feed_screen.dart was never instantiated. It is
file-private and the file has no part directive, so nothing outside can
reach it either.

It also carried the last non-doc reference to the seek-based loop
enforcement removed in #6489: its _delay constant was justified with
"flashes that occur during play/pause and loop-enforcement seeks".
Deleting the widget is the right fix rather than trimming a comment on
code that never builds.
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.

chore(infinite-video-feed): delete dead seek-based loop enforcement that can silently reintroduce the loop seam

2 participants