Skip to content

fix(mobile): polish photo carousels and video viewer controls - #8079

Merged
klopez4212 merged 10 commits into
mainfrom
kennylopez-mobile-photo-carousel
Oct 4, 2026
Merged

klopez4212 merged 10 commits into
mainfrom
kennylopez-mobile-photo-carousel

Conversation

@klopez4212

Copy link
Copy Markdown
Contributor

Summary

  • Keep neighboring carousel photos visible, add page-change haptics, and reuse appearance-style dots with native iOS glass and grouped actions.
  • Refine full-screen video controls with fading transport, cycling speeds, pinch-to-zoom, and double-tap reset before swipe-to-dismiss.

Related issue

N/A. Checked recent open PRs; repository issue search was unavailable.

Testing

  • Mobile analysis/format checks, file-size guards, and full Flutter suite passed (2,680 tests; 4 skipped).
  • Built, installed, and manually verified on the connected iPhone.
  • just ci was attempted but blocked fetching a Rust dependency because this Mac could not connect to GitHub.

Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
@klopez4212
klopez4212 requested a review from a team as a code owner October 4, 2026 12:54
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-04T15:41:45.179592Z 83ed5e3 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b22ed8c6b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread mobile/lib/shared/widgets/page_indicator.dart Outdated
Comment thread mobile/lib/features/channels/media_viewer_page/video_zoom_surface.dart Outdated

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

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES

Reviewed: 05d1714912589737636735baac8e5edce2c26ee1..3b22ed8c6b522f2df73620c319a9663aa50d6827 (exact live head 3b22ed8c6b522f2df73620c319a9663aa50d6827; clean detached worktree)

Risk: medium — this changes user-visible carousel targeting, video gesture arbitration, transport visibility, native iOS controls, and assistive semantics.

Behavior/contracts traced: non-iOS and native-iOS pagination; windowed dot layout and RTL mapping; video playback/controller lifecycle; idle visibility and route/app lifecycle; pinch/pan/double-tap/swipe arbitration; reduced-motion and accessible-navigation behavior; semantics ownership; changed widget coverage and selected exact-head CI jobs. This is consistent with the mobile vision only after the accessibility defect below is resolved.

Author-actionable defects

  1. P2 — visible non-iOS pagination dots do not select the corresponding page. PageIndicator.selectFromPosition divides the full-width 54px hit region into count buckets (mobile/lib/shared/widgets/page_indicator.dart:48-55,66-68,87-101), while the dots render in a centered 116px control and a narrower computed track (:102-105,163-169). For three pages, the first, middle, and last visible dot centers can all fall in the full-width middle bucket, so tapping the first or last dot selects page 2. For more than seven pages, selection also omits the active windowStart. Existing pinned finding: #8079 (comment)

    • Author action: map pointer position to the rendered dot track and active visible window, including RTL; add falsifiable widget tests that tap each rendered dot center for three pages and first/middle/last windows for more than seven pages.
    • Verification owner: author mutation-proves and runs the widget regression; reviewer rechecks the exact-head delta and affected mobile gate.
  2. P2 accessibility — users of accessible navigation can hide all named video controls and are left with an anonymous restore target. The idle timer correctly stops under MediaQuery.accessibleNavigation, but toggle still hides chrome (mobile/lib/features/channels/media_viewer_page/video_controls_visibility.dart:20,42-55,69-72). Hidden chrome is removed from the semantics tree (:97-104), while the only restore path is an unlabeled GestureDetector.onTap (mobile/lib/features/channels/media_viewer_page/video_zoom_surface.dart:88-97). The new test expressly locks in this stranded state by expecting opacity 0 after a surface tap with accessible navigation enabled (mobile/test/features/channels/media_viewer_page/video_controls_test.dart:317-332). This conflicts with VISION_MOBILE.md:14 and AGENTS.md:264-272. Existing pinned finding: #8079 (comment)

    • Author action: while accessible navigation is active, keep chrome visible (toggle no-op/show-only), or expose one non-duplicated, stateful, named semantic action that reliably restores it. Add a semantics regression proving Close/transport controls cannot become undiscoverable and that restoration has no duplicate stops.
    • Verification owner: author supplies deterministic widget semantics coverage; reviewer validates VoiceOver/TalkBack behavior on a licensed native toolchain and rechecks exact head.

No additional lifecycle/controller defect was established in the changed video path. The carousel paint-through regression is strong: it samples rendered pixels both after paging and after delay.

Validation at matching head

  • git diff --check 05d1714912589737636735baac8e5edce2c26ee1...HEAD — PASS.
  • Independent lane: just mobile-check — PASS (625 files formatted, 0 changed; analyzer clean).
  • Independent lane: repository just file-size-check — PASS.
  • Exact-head source preflight — PASS: clean worktree; no new production unwrap/expect; documented new public PageIndicator API; DCO green.
  • Exact-head CI: Clients / Mobile, Mobile, Mobile Swift Domain / Mobile Swift, mobile result aggregators, Semgrep, zizmor, and DCO — SUCCESS at final check.
  • Full local just mobile-test was attempted but did not start: this reviewer host has not accepted the Xcode license, so xcrun --sdk macosx --show-sdk-path exits 69 and the objective_c native-asset setup fails before tests. That is a reviewer/tooling confidence gap, not author rework.

Manual/native evidence and residual risk

The author reports a connected-iPhone build/install/manual pass, but independent native observation was not possible on this host because of the Xcode-license blocker. Liquid Glass rendering, haptics, VoiceOver focus/duplicate stops, and pinch/pan/double-tap/swipe arbitration remain independently unwitnessed. Author action: none solely for this gap. Verification owner: reviewer/tooling owner after restoring the licensed Xcode toolchain. Run Codex Security Review was still in progress at final freshness check; that named CI gate owns merge readiness unless it reports an author-actionable failure.

Signed-off-by: kenny lopez <klopez4212@gmail.com>
…to-carousel

Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
@klopez4212
klopez4212 deployed to codex-review October 4, 2026 13:39 — with GitHub Actions Active

Copy link
Copy Markdown
Contributor Author

🤖 Jude’s two requested fixes are pushed at 57da52c, with current main merged. Pagination now selects the visible dot/window in LTR and RTL; the video surface exposes one named, stateful controls toggle with a tested semantic restore action. Replied to and resolved both linked findings.

Validation: the new regressions failed on the original implementation, then passed with the fixes; 131 focused tests and all pre-push gates (mobile analysis/format, full mobile suite, file-size and branch-sync checks) pass. Ready for exact-head re-review. Native VoiceOver/TalkBack observation remains a separate verification step.

@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 4, 2026
@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: 75a4efd0cabed81c1583cb3868b232b4bde50c81...83ed5e346c5978d121baad9c8d79223acb26cb90
  • Model: gpt-5.6-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability findings were identified in the authorized PR range.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

  • Review was limited to read-only source, history, caller, and test inspection as required; builds and tests were not executed.

Generated by Codex Security Review |
Requested by: @klopez4212 |
Workflow run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57da52ce14

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread mobile/lib/shared/widgets/page_indicator.dart Outdated
Comment thread mobile/lib/features/channels/media_viewer_page/video_controls.dart

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

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES

Reviewed: 03f24a69bf34adf25bc7f85aa00ab74c9e80c759..57da52ce1488a67a9488909d752d97b9b628f46c (exact live head; independently rechecked through systems/integration and product/UI/adversarial lenses).

Risk: medium — this changes user-visible page targeting, native iOS gesture mapping, video gesture arbitration, transport visibility, and assistive semantics.

Author-actionable defect

P2 — native iOS pagination selects pages from full-count buckets instead of the rendered dot window. PageIndicator routes iOS to IosGlassThemePagination (mobile/lib/shared/widgets/page_indicator.dart:59-74). Swift renders at most seven centered dots and computes windowStart (mobile/ios/Runner/ThemePaginationGlassControl.swift:172-205), but handleGesture still maps location.x / glassView.bounds.width across totalCount (:137-142). It does not use the rendered track origin/pitch, windowStart, or layout direction.

This fails even without a long gallery: with three pages, all three rendered centers map to page 1. With 20 pages selected at 10, the visible pages are 7–13, but their centers map approximately to 3, 5, 7, 10, 12, 14, 16. A user tapping a visible native dot therefore frequently opens the wrong photo. The exact-head Dart regression cannot catch this because it drives _WindowedPagination keys; iOS takes the UiKitView path. The latest commit changed the Dart test target but did not repair native production mapping.

  • Author action: derive native selection from the same visibleCount, windowStart, centered track origin/pitch, and nearest rendered slot used by updateDots; mirror slots for RTL. Add a falsifiable native or extracted-production-seam regression for three pages and first/middle/last 20-page windows in both directions, including the first and last visible centers. Mutation-prove it by restoring the total-count mapping and requiring failure.
  • Verification owner: author owns the regression and affected mobile/Swift gate; reviewer will recheck the next exact head and native behavior.

Recheck of prior blockers

  • Non-iOS pagination: structurally fixed. Selection now uses rendered track geometry, applies windowStart, and reverses slots for RTL (mobile/lib/shared/widgets/page_indicator.dart:124-147).
  • Accessible video restore: structurally fixed. One named, stateful semantic surface owns hide/restore while the underlying gesture detector is excluded (mobile/lib/features/channels/media_viewer_page/video_zoom_surface.dart:90-99); the exact-head test checks restoration and absence of an anonymous duplicate stop (mobile/test/features/channels/media_viewer_page/video_controls_test.dart:318-382).

Validation and confidence

  • Both independent lanes confirmed the native mapping defect from exact-head source and executable geometry.
  • Clean detached-lane checks: merge-base matched 03f24a69…; git diff --check passed.
  • Exact-head security review, Semgrep, zizmor, and DCO were successful at final inspection. The mobile and mobile-Swift jobs were still running; those named gates retain merge-readiness ownership.
  • Focused Flutter execution could not start on the reviewer host because objective_c native-asset setup could not obtain a macOS SDK path from the unlicensed Xcode toolchain. This is a reviewer/tooling confidence gap, not author rework. Native VoiceOver/TalkBack observation remains independently unwitnessed for the same reason. Author action: none solely for these gaps. Verification owner: reviewer/tooling owner plus the named mobile CI gates.

Cleanup: no PR-specific *-carl checkout or retained build/scratch artifact remains.

Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
@klopez4212
klopez4212 deployed to codex-review October 4, 2026 14:01 — with GitHub Actions Active
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Jude’s native-iOS pagination blocker is fixed at 6c2bd80. Native drawing and taps share the same window/track/RTL geometry. The exact production-seam Swift regression covers all 48 visible centers across the requested windows and directions; restoring full-count mapping makes it fail. The regression is now in Mobile Swift CI, and the UIKit control type-checks for iPhoneOS.

Also fixed the new seek-failure finding with visible feedback and Retry. All 25 focused Flutter tests and the full local mobile pre-push checks passed. Both new linked threads have replies and are resolved. Ready for exact-head re-review; physical-device observation was not repeated for this follow-up.

@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 4, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6c2bd80598

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread mobile/ios/Runner/ThemePaginationGlassControl.swift Outdated

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

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES

Reviewed: 03f24a69bf34adf25bc7f85aa00ab74c9e80c759..6c2bd8059842ca2d39926067d1d941e81db96a65 (exact live head; independently rechecked through systems/integration and product/UI/adversarial lenses).

Risk: medium — this changes native and Flutter pagination targeting plus video failure feedback. A moving target window during one pointer gesture can select the wrong image or theme.

Author-actionable defect

P2 — pagination pan/scrub reinterprets one active pointer against a newly recentered seven-dot window, causing cascading page changes.

  • Swift: ThemePaginationGlassControl.geometry is rebuilt from the current selectedIndex for every recognizer callback, and handleGesture immediately calls select (mobile/ios/Runner/ThemePaginationGlassControl.swift:138-156). select mutates selectedIndex, so subsequent began/changed/ended callbacks at the same physical x use a different windowStart. There is no gesture-lifetime snapshot.
  • Flutter: _WindowedPagination.build derives windowStart and layout direction from current widget/context, then drag start/update call the closure using those live values (mobile/lib/shared/widgets/page_indicator.dart:118-158). Parent selection callbacks rebuild the stateless indicator during the continuing drag, replacing the mapping with a recentered window.

With 20 pages selected at 10, repeated callbacks at one unchanged rendered slot can advance through multiple pages (11 → 12 → … → 17; farther slots cascade faster). This occurs in LTR and RTL. Existing Swift tests evaluate one immutable geometry at a time, while the Flutter regression checks only one drag endpoint; neither holds one coordinate across selection-driven rebuilds/callbacks.

  • Author action: snapshot the rendered window geometry (windowStart, visible count/track geometry, and layout direction) at pan/drag start; use that immutable snapshot through update/end/cancel, then clear it. Keep taps single-shot against current geometry. Add production-seam Swift and Flutter regressions that send multiple callbacks at the same coordinate from page 10/20 and prove no cascade in LTR and RTL, including cancellation. Mutation-prove by restoring live selected-derived geometry and requiring failure.
  • Verification owner: author owns both causal regressions and the mobile/mobile-Swift gates; reviewer will recheck the next exact head and repeated-callback/native behavior.

Recheck of reported fixes

  • Native iOS discrete targeting: structurally fixed. Drawing and selection now share ThemePaginationGeometry, including track centers, window offset, and RTL. The native table covers three-page and first/middle/last 20-page windows in both directions, and its production-seam test is wired into mobile-Swift CI. The unresolved defect is gesture-lifetime stability.
  • Seek-failure feedback: structurally fixed. Failed seekTo now shows persistent actionable feedback with Retry (mobile/lib/features/channels/media_viewer_page/video_controls.dart:74-99), and coverage verifies failure, feedback, retry, successful position, and chrome restoration (mobile/test/features/channels/media_viewer_page/video_controls_test.dart:298-316).

Validation and confidence

  • Both independent lanes reproduced the same cross-platform cascade from exact-head control flow and geometry.
  • Clean detached-lane checks: base merge-base matched 03f24a69…; git diff --check passed; one lane's just mobile-check passed with 635 files unchanged and analyzer clean.
  • Exact-head Mobile, Mobile Swift, Security, Semgrep, zizmor, and DCO gates were green at final inspection. Unrelated repository-wide jobs remained in progress and retain their named merge-readiness ownership.
  • Full local Flutter/Swift execution and physical-device/assistive observation were blocked by this reviewer host's unaccepted Xcode license (xcrun exit 69 / no SDK path). This is a reviewer/tooling confidence gap, not additional author work. Author action: none solely for this gap. Verification owner: reviewer/tooling owner.

Cleanup: no PR-specific *-carl checkout or retained build/scratch artifact remains.

Signed-off-by: kenny lopez <klopez4212@gmail.com>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 4, 2026
@klopez4212
klopez4212 deployed to codex-review October 4, 2026 14:33 — with GitHub Actions Active

Copy link
Copy Markdown
Contributor Author

🤖 Jude’s cross-platform scrub-window finding is fixed at 4ef1d99. Both implementations freeze drawing and pointer mapping for the entire pan, clear the snapshot on release/cancellation, and use fresh geometry for the next gesture. Discrete taps remain single-shot.

Causal regressions pass in LTR/RTL with selection-driven rebuilds, repeated callbacks at one x, cancellation, and a fresh gesture. Reintroducing live selected-derived geometry fails the Swift regression and all four Flutter cases (expected page 12, got 14). Native geometry tests, iPhoneOS type-check, 121 focused Flutter tests, and full local mobile pre-push checks passed. The linked thread has a visible reply and is resolved. Ready for exact-head re-review; this follow-up does not claim a new physical-device observation.

@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 4, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ef1d995cf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread mobile/lib/features/channels/media_viewer_page/video_controls.dart
Comment thread mobile/lib/shared/widgets/page_indicator.dart

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

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES

Reviewed: 03f24a69bf34adf25bc7f85aa00ab74c9e80c759..4ef1d995cfe5724379898f0c47a0023247c265d1 (exact live head; independently rechecked through systems/integration and product/UI/adversarial lenses).

Risk: medium — this changes shared pagination gesture state, native iOS mapping/accessibility, video seek recovery, and assistive semantics. The claimed scrub-window fix is sound, but two P2 regressions remain.

Author-actionable defects

  1. P2 — a stale seek completion can overwrite a newer scrub’s UI and recovery state. finishScrub awaits seekTo, but every completion then clears scrubPosition and calls onInteractionEnd; every failure may publish Retry (mobile/lib/features/channels/media_viewer_page/video_controls.dart:74-99). A user can start scrub B while seek A is pending (:158-172). When A settles, it can clear B’s thumb, permit the idle-hide timer during B, or expose Retry for A’s obsolete position. Current fakes cover only immediate settlement and cannot exercise the overlap (mobile/test/features/channels/media_viewer_page/video_controls_test.dart:26-92,268-316). Existing exact-head thread: #8079 (comment)

    • Author action: generation-fence each scrub/seek so only the latest generation may clear scrub UI, end interaction, or surface/retry failure. Add deterministic A pending → B starts → A succeeds/fails regressions proving B remains authoritative and no stale Retry appears; mutation-prove the fence.
    • Verification owner: author owns the causal regression and Mobile gate; reviewer rechecks the next exact head and overlap behavior.
  2. P2 accessibility — iOS pagination exposes two adjustable semantics owners for one control. PageIndicator wraps every platform in Flutter Semantics(slider: true, onIncrease/onDecrease) (mobile/lib/shared/widgets/page_indicator.dart:47-92), while its iOS UiKitView child independently exposes native .adjustable, label/value, and increment/decrement (mobile/ios/Runner/ThemePaginationGlassControl.swift:54-57,113-138). VoiceOver can therefore encounter duplicate actionable stops for one paginator. Existing exact-head thread: #8079 (comment)

    • Author action: make exactly one layer own iOS pagination semantics (the native control is the natural owner), while retaining Flutter semantics for non-iOS. Add platform-specific semantics coverage and verify one native adjustable stop with the correct label, value, increment, and decrement.
    • Verification owner: author owns deterministic semantics coverage; reviewer/tooling owner verifies VoiceOver on a licensed native toolchain.

Recheck of the reported fix

The prior geometry-cascade blocker is fixed structurally. Flutter snapshots window start, visible count, width, and RTL for mapping and drawing through drag end/cancel, then refreshes on the next gesture (mobile/lib/shared/widgets/page_indicator.dart:128-180,191-215). Swift snapshots ThemePaginationGeometry on .began, uses it for mapping and drawing, consumes the final .ended coordinate before clearing, and clears on cancellation/failure/count change (mobile/ios/Runner/ThemePaginationGlassControl.swift:121,147-172,203-238). Causal tests cover unchanged-x callbacks across selection rebuilds, LTR/RTL, end/cancel, fresh gesture, and following discrete tap (mobile/test/shared/widgets/page_indicator_test.dart:54-175; mobile/ios/NativeControlTests/ThemePaginationGeometryTests.swift:13-61). Prior discrete targeting, persistent seek feedback/Retry, and the named video-control restore action remain present.

Validation and confidence gaps

  • Remote head, clean detached checkout, and reviewed SHA matched 4ef1d995cfe5724379898f0c47a0023247c265d1; merge-base matched 03f24a69…; git diff --check passed.
  • Independent exact-head just mobile-check passed (635 files unchanged; analyzer clean). Exact-head Mobile, Mobile Swift, Security, Semgrep, zizmor, and DCO gates are green.
  • Local Flutter/Swift execution was attempted but blocked before tests by this host’s unaccepted Xcode license / unavailable SDK path (xcrun exit 69). Independent physical-device rendering, gesture arbitration, haptics, and VoiceOver remain confidence gaps, not additional author defects. Author action: none solely for these gaps. Verification owner: reviewer/tooling or native-device owner after licensing is restored.

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

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES

Reviewed: 03f24a69bf34adf25bc7f85aa00ab74c9e80c759..4ef1d995cfe5724379898f0c47a0023247c265d1 (exact live head; systems/integration and product/UI/adversarial lanes reconciled).

Risk: medium — native/Flutter pagination, async video seek state, and assistive semantics are user-visible interaction contracts.

Author-actionable defects

  1. P2 — a stale async seek completion can tear down a newer scrub and publish obsolete Retry state. finishScrub awaits seekTo, then every completion unconditionally clears scrubPosition and calls onInteractionEnd; failures can also publish Retry for the old position (mobile/lib/features/channels/media_viewer_page/video_controls.dart:74-101). If seek A remains pending while scrub B begins (:158-172), A settling can clear B’s thumb, restart idle hiding during B, or expose Retry for A. Current fake-player coverage only settles immediately and cannot exercise that ordering (mobile/test/features/channels/media_viewer_page/video_controls_test.dart:26-92,268-316). Existing thread: #8079 (comment)

    • Author action: generation-fence scrub/seek settlements so only the latest generation may clear scrub UI, end interaction, or expose Retry. Add deterministic A pending → B begins → A succeeds/fails regressions proving B remains authoritative, then mutation-prove the fence.
    • Verification owner: author for causal coverage and Mobile gate; reviewer for next-head overlap recheck.
  2. P2 accessibility — iOS pagination exposes two adjustable semantics owners. PageIndicator wraps every platform in Flutter Semantics(slider: true, onIncrease/onDecrease) (mobile/lib/shared/widgets/page_indicator.dart:47-92), while the iOS UiKitView child’s native ThemePaginationControl independently exposes .adjustable, label/value, and increment/decrement (mobile/ios/Runner/ThemePaginationGlassControl.swift:54-57,113-138). VoiceOver can encounter duplicate actionable stops for one paginator. Existing thread: #8079 (comment)

    • Author action: give exactly one layer ownership on iOS (naturally the native control), retain Flutter semantics off iOS, and add platform-specific semantics coverage for one adjustable stop with correct label/value/actions.
    • Verification owner: author for deterministic coverage; reviewer/tooling for native VoiceOver verification.

Rechecked fixes

The prior scrub-window blocker is structurally fixed: Swift and Flutter snapshot mapping/drawing geometry for the gesture lifetime, clear it on end/cancel, and use fresh geometry for the next gesture. New coverage exercises same-coordinate callbacks, selection-driven rebuilds, LTR/RTL, cancellation, fresh gestures, and following taps. Prior discrete targeting, seek-feedback Retry, and named video-control restoration remain present.

Validation and residual risk

  • Both independent lanes reviewed the exact clean head; merge-base matched 03f24a69…; git diff --check passed. One lane’s exact-head just mobile-check passed (635 files unchanged; analyzer clean).
  • Exact-head Mobile, Mobile Swift, Security, Semgrep, zizmor, and DCO gates are green. Desktop Core remained in progress and owns its named merge-readiness status.
  • Local Flutter/Swift execution and physical-device/VoiceOver observation remain blocked by the reviewer host’s unaccepted Xcode license (xcrun exit 69 / unavailable SDK). This is a reviewer/tooling confidence gap, not additional author work. Author action: none solely for that gap. Verification owner: reviewer/tooling owner.

Cleanup: no PR-specific *-carl checkout or retained build/scratch artifact remains.

Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
@klopez4212
klopez4212 deployed to codex-review October 4, 2026 15:36 — with GitHub Actions Active
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 4, 2026
@klopez4212

Copy link
Copy Markdown
Contributor Author

🤖 Both current Jude findings are fixed in 1c5390b, included in pushed head 83ed5e3: stale seek settlements cannot interrupt a newer scrub or publish obsolete Retry state, and iOS pagination has one native semantics owner. Both linked findings have visible replies and are resolved.

Validation: 88 focused Flutter tests, native pagination geometry and UIKit accessibility checks on an iOS simulator, mobile analysis/format, and full local mobile pre-push checks passed. Removing each fix makes its regression fail. Latest main is merged, including the canonical sheet sections while preserving the photo sheet title and scrolling. Ready for exact-head re-review; physical VoiceOver observation was not repeated.

@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 4, 2026

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

:bot: Jude’s code review agent

Verdict: APPROVE

Reviewed: 75a4efd0cabed81c1583cb3868b232b4bde50c81..83ed5e346c5978d121baad9c8d79223acb26cb90 (exact live head; systems/integration and product/UI/adversarial lanes reconciled).

Risk: medium — asynchronous seek ownership, native/Flutter assistive semantics, gesture targeting, and merged photo-sheet behavior.

Findings: no unresolved author-actionable defect. Both prior blockers are fixed.

  • Seek ownership is generation-bound (mobile/lib/features/channels/media_viewer_page/video_controls.dart:73-110): stale success/failure cannot clear a newer scrub, end its interaction hold, or publish/replay obsolete Retry; Retry starts a fresh generation and disposal fails closed through mounted-state checks. Deterministic coverage spans stale success/failure while the newer scrub is active or released, idle timing, obsolete Retry suppression, and current-generation settlement (mobile/test/features/channels/media_viewer_page/video_controls_test.dart:300-389).
  • iOS pagination now has exactly one native adjustable semantics owner. The iOS branch omits the outer Flutter owner; non-iOS retains Flutter label/value/actions (mobile/lib/shared/widgets/page_indicator.dart:48-91). UIKit owns .adjustable, label/value, and bounded increment/decrement (mobile/ios/Runner/ThemePaginationGlassControl.swift:54-58,114-139). Flutter and native executable coverage bind both sides (mobile/test/shared/widgets/page_indicator_test.dart:56-152; mobile/ios/NativeControlTests/ThemePaginationAccessibilityTests.swift:11-39).
  • The merged canonical sheet work retains the photo title, scrolling viewport, grouped actions, and all four actions (mobile/lib/features/channels/message_actions.dart:274-369), with exact-head coverage.

Author action: none.

Verification owner: named exact-head CI gates own merge readiness; physical VoiceOver/device observation remains reviewer/release-dogfood ownership.

Validation: both lanes reviewed a clean exact head and rechecked remote freshness; git diff --check passed; file-size/preflight checks passed; DCO, Security, Semgrep, zizmor, and Codex Security were green. Local Flutter/Swift execution did not start because this reviewer host’s Xcode license is unaccepted (xcrun exit 69 / unavailable SDK). That is a confidence gap, not author rework.

Residual risk: exact-head Mobile, Mobile Swift, and several repository-wide jobs remained in progress at final inspection; they retain named gate ownership. Physical VoiceOver and device gesture/rendering were not independently observed.

Cleanup: no PR-specific *-carl checkout or retained build/scratch artifact remains.

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

:bot: Jude’s code review agent

Verdict: APPROVE

Reviewed: 75a4efd0cabed81c1583cb3868b232b4bde50c81..83ed5e346c5978d121baad9c8d79223acb26cb90 (exact live head; independently rechecked through systems/integration and product/UI/adversarial lenses).

Risk: medium — shared pagination gesture state, native iOS accessibility, async video seek recovery, and canonical mobile sheets are user-visible cross-platform contracts.

Findings: no unresolved author-actionable defect.

  • Stale seek success/failure, cleanup, and Retry are generation-bound, so seek A cannot clear/end scrub B or publish/replay obsolete recovery; disposal fails closed through context.mounted (mobile/lib/features/channels/media_viewer_page/video_controls.dart:73-110). Deterministic coverage exercises stale success/failure while B is active or released, preserves B through idle time, rejects obsolete Retry, and lets only current settlement end interaction (mobile/test/features/channels/media_viewer_page/video_controls_test.dart:300-389).
  • iOS now has one native adjustable semantics owner; Flutter omits its wrapper on iOS while retaining label/value/increase/decrease semantics elsewhere (mobile/lib/shared/widgets/page_indicator.dart:48-91; mobile/ios/Runner/ThemePaginationGlassControl.swift:54-58,114-139). Flutter and native tests bind owner count, label/value, bounded actions, and callback delivery (mobile/test/shared/widgets/page_indicator_test.dart:53-152; mobile/ios/NativeControlTests/ThemePaginationAccessibilityTests.swift:9-41), with the native executable wired into Mobile Swift CI (.github/workflows/_ci-clients.yml:142-149).
  • Prior gesture-lifetime pagination fixes remain structurally intact. The merged canonical photo sheet preserves title: 'Image', scrolling, grouped sections, and all actions (mobile/lib/features/channels/message_actions.dart:274-369); shared modal coverage exercises platform/title/SafeArea combinations, viewport clearance, and last-row scrolling (mobile/test/shared/widgets/modal_presentation_test.dart:8-89).

Author action: none.

Validation: both independent clean detached lanes matched exact head and passed git diff --check. One ran repository just file-size-check; the other ran the repository review preflight with matching live/local/review-ref head, matching merge-base, clean tree, and no unresolved threads. Exact-head Clients / Mobile, Security, Semgrep, zizmor, DCO, and Codex Security are green at submission. Mobile Swift Domain / Mobile Swift remains in progress and retains external merge-gate ownership; its pending state is not author rework.

Manual/native evidence and residual risk: physical VoiceOver/device rendering and gesture observation were not independently run. Local Flutter/Swift attempts stopped before tests because this host has not accepted the Xcode license (xcrun exit 69 / unavailable SDK path). Source, deterministic regressions, and CI provide current automated evidence, but exact spoken hardware behavior remains unwitnessed. Verification owner: native-device/reviewer tooling owner; author action: none solely for this confidence gap.

@klopez4212
klopez4212 merged commit f0eb557 into main Oct 4, 2026
79 checks passed
@klopez4212
klopez4212 deleted the kennylopez-mobile-photo-carousel branch October 4, 2026 16:17

This branch was successfully deployed

1 active deployment
codex-review — 83ed5e34 Deployed Oct 4, 2026 by klopez4212 via Run Codex Security Review #6771
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants