Fix mobile Activity thread navigation - #5850
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9362c2d6d0
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 109f0ce8d6
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ba459935b
ℹ️ 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".
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Reviewed exact head 59966c61281473d2602aeb782a55fdbd01e2b644.
Request changes
Major: make the Activity landing target perceivable and available nonvisually
mobile/lib/features/channels/thread_detail_page.dart:43-46,794-825 identifies the referenced reply only by briefly blending the primary color into its background at 0.12 * 0.4 = 4.8% alpha. Against the repository's light/dark surfaces, that produces roughly 1.04:1 / 1.10:1 adjacent contrast. There is also no semantics announcement, focus movement, label/state change, outline, or other non-color cue.
This is the orientation affordance for the feature: after Activity opens a long thread around 35% viewport alignment, users must be able to identify which nearby reply was referenced. At this contrast it is effectively imperceptible, and screen-reader users receive no target indication at all. The tests at mobile/test/features/channels/channel_detail_page_test.dart:4122-4133,4275-4314 assert the 0.048 alpha, so they preserve the defect rather than proving the target is recognizable.
Use a clearly distinguishable state treatment (for example, a sufficiently contrasted border or leading marker rather than only a faint wash) and announce/focus the target without trapping reading navigation. Preserve reduced-motion behavior. Add semantics assertions and light/dark rendered evidence.
Validation and remaining gate
The route/state implementation otherwise held up: replacement is Activity-only, delayed hydration refuses to replace a newer route, the thread retains visible-channel ownership, delayed targets wait for layout, and Back returns directly to Activity. Focused route/highlight tests passed, as did the full 1,359-test Flutter suite reported at this head. My exact-head just mobile-check passed (394 files formatted/0 changed, no analyzer issues, file-size guard), and CI Mobile is green.
However, GitHub currently reports this head as DIRTY / CONFLICTING against main. Resolve the conflict, then the new SHA needs delta review and affected gates rerun. Please also supply real Activity → delayed/paginated thread → target identification → Back evidence, including screen-reader/semantics behavior; the current tests split the journey across synthetic entry points and the PR supplies no device artifact for it.
Non-blocking scope note: the pull-to-refresh bee eye changes in mobile/lib/shared/widgets/bee_refresh_indicator.dart, flapping_bee.dart, and their test are unrelated to Activity navigation and should preferably be split into their own PR.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Fast Fizz <2df81cb51f05a9d5387ef24d7b9ecb8fcdfcd1c70ffabc67061c9596e1b5b1c4@buzz.block.builderlab.xyz> Signed-off-by: Fast Fizz <2df81cb51f05a9d5387ef24d7b9ecb8fcdfcd1c70ffabc67061c9596e1b5b1c4@buzz.block.builderlab.xyz>
Co-authored-by: Fast Fizz <2df81cb51f05a9d5387ef24d7b9ecb8fcdfcd1c70ffabc67061c9596e1b5b1c4@buzz.block.builderlab.xyz> Signed-off-by: Fast Fizz <2df81cb51f05a9d5387ef24d7b9ecb8fcdfcd1c70ffabc67061c9596e1b5b1c4@buzz.block.builderlab.xyz>
Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz> Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
59966c6 to
af0d985
Compare
|
@codex review — Princess Donut |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af0d9854d5
ℹ️ 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".
Co-authored-by: Kenny Lopez <klopez4212@gmail.com> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e36493704
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed exact head 6e36493704a0d4d965654d65c2a25975bff2e149 with the Royal Court.
Request changes
Major: make the Activity landing target perceivable visually and nonvisually
The prior target-identification blocker remains unresolved. mobile/lib/features/channels/thread_detail_page.dart:49-52,142-199 makes the landing state transient and caps its only visual treatment at 0.12 * 0.4 = 4.8% alpha. mobile/lib/features/channels/thread_detail_page/thread_message.dart:85-117 renders only that background-color wash. Against the repository’s light and dark surfaces this is approximately 1.07:1 adjacent contrast, with no border, marker, focus movement, semantics announcement, label/state change, or other non-color cue.
That means Activity can correctly position a long thread around the target while leaving sighted users unable to distinguish which nearby message was referenced, and VoiceOver/TalkBack users receive no target identification at all. Reduced motion snaps to the same inaccessible state rather than solving it.
Please add a clearly distinguishable target treatment, such as a contrast-safe border or leading marker, and announce or appropriately focus the target without trapping reading navigation. Preserve the reduced-motion behavior. Add semantics assertions and light/dark rendered evidence; the changed tests at mobile/test/features/channels/channel_detail_page_test.dart:4277-4533,4584-4629 currently assert the raw 4.8%-alpha decoration and timing, so they encode the defect rather than proving recognizability.
Other review results
The navigation/state-machine seams otherwise held up: Activity-only replacement preserves Back, delayed hydration cannot replace a newer route, the thread retains visible-channel ownership, target selection waits for authoritative pagination or terminal failure, and listener/timer disposal is guarded. The Royal Court reached consensus on this verdict.
Focused exact-head tests passed for Activity route wiring, replacement/Back, delayed-hydration cancellation, and query-failure fallback. git diff --check is clean, and exact-head Mobile CI is green. Two initially parallel local test invocations collided in Flutter’s shared asset directory; rerunning the affected tests serially passed.
Scope note, non-blocking: the pull-to-refresh bee eye changes and their tests are unrelated to Activity thread navigation and would be cleaner in a separate PR.
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. Exact-head follow-up: changes remain requested at The prior major target-identification blocker remains unresolved:
Please add a clearly distinguishable treatment, such as a contrast-safe border or leading marker, and announce or appropriately focus the target without trapping reading navigation. Preserve reduced-motion behavior and add semantics plus light/dark rendered coverage. The Royal Court reached consensus. Navigation/state-machine review otherwise found no material issue: Activity-only replacement preserves Back, delayed hydration cannot replace a newer route, visible-channel ownership survives replacement, target selection waits for pagination or terminal query failure, and listener/timer cleanup is guarded. Focused exact-head tests passed for Activity route wiring, replacement/Back, delayed-hydration cancellation, and query-failure fallback. Non-blocking scope note: the pull-to-refresh bee eye changes and tests are unrelated and would be cleaner in a separate PR. |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent reviewed exact head 6e36493704a0d4d965654d65c2a25975bff2e149 against base f956e6fe06a76e50cbd8fba1a162482e752e7f1a.
Request changes
P2 — a retrying thread query is treated as terminal and consumes the one-shot jump
mobile/lib/features/channels/thread_detail_page.dart:116-121,350-396 says the hydrated route snapshot is eligible only after a terminal query error, but implements that as replyMessages.hasError. Under the pinned Riverpod 3.1.0, a failed provider with a retry scheduled is AsyncLoading carrying both hasError == true and retrying == true; the default container retries ordinary exceptions up to ten times. The first transient relay failure therefore authorizes the provisional snapshot immediately and sets didJumpToInitialMessage at lines 366-367. If a retry later succeeds with the authoritative/paginated list and a different target index, the latch prevents correction.
The new regression test does not exercise production semantics: mobile/test/features/channels/channel_detail_page_test.dart:196-207,4474-4533 explicitly sets disableRetries: true, converting the first failure into a terminal error. It passes while the retry/recovery path remains broken.
Please require an authoritative value or a non-retrying error, for example fetchedReplies != null || (replyMessages.hasError && !replyMessages.retrying). Add a production-retry test where the first attempt fails, no jump/highlight occurs while retrying, and a retry succeeds with an authoritative list whose target index differs. Mutation-prove that removing the !retrying guard fails the test.
P2 — the Activity landing target remains imperceptible visually and unidentified nonvisually
mobile/lib/features/channels/thread_detail_page.dart:49-52,142-199 makes the target state transient and caps its only treatment at 0.12 * 0.4 = 4.8% alpha. mobile/lib/features/channels/thread_detail_page/thread_message.dart:85-117 renders only that background wash—no border, marker, focus movement, announcement, or target semantics. With the checked-in default colors, the resulting adjacent contrast is about 1.076:1 light (#F9F5FE on #FFFFFF) and 1.096:1 dark (#2C2D43 on #24273A). Users can land among nearby replies without being able to tell which was referenced; VoiceOver/TalkBack receives no identification. That conflicts with VISION.md:206's WCAG 2.1 AA minimum.
The changed rows at mobile/test/features/channels/channel_detail_page_test.dart:4277-4533,4584-4629 assert raw alpha/timing, not recognizability or semantics. Please add a clear non-color cue such as a contrast-safe border/leading marker plus a one-shot announcement or appropriate focus behavior that does not trap reading navigation. Preserve reduced-motion behavior and add semantics assertions plus light/dark rendered/native evidence.
Integrated validation
At exact clean head:
git diff --check f956e6fe06a76e50cbd8fba1a162482e752e7f1a...HEAD— pass. ./bin/activate-hermit && just mobile-check— pass; 409 files formatted/0 changed, analyzer cleancd mobile && flutter test test/features/activity/activity_page_test.dart— pass, 25 testscd mobile && flutter test test/features/channels/channel_detail_page_test.dart --plain-name 'highlights a hydrated target after the thread query fails'— pass, but only with retries disabled by the test- GitHub Mobile and DCO checks — pass at this exact head
The navigation/state and ownership paths otherwise held up: Activity-only replacement preserves Back, stale hydration is guarded, visible-channel registration is disposed, and no persistence/security/tenant boundary changes were introduced. No real iOS/Android Activity → delayed/paginated target → Back plus VoiceOver/TalkBack artifact was supplied, so the user-visible workflow remains unproven outside widget tests.
Non-blocking scope note: the pull-to-refresh bee-eye source and test changes are unrelated to Activity navigation and should be split.
Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz> Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
|
Addressed the non-UI retry correctness finding in Riverpod 3.1 represents a scheduled retry as The requested visible marker/semantics treatment would change the authored UI, so I intentionally made no accessibility/UI change while awaiting the author’s confirmation. Validation at exact pushed head: — Princess Donut |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8aea947592
ℹ️ 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".
Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz> Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
3 participants
Summary
Why
Activity deep links hydrated the channel before opening the thread, which left the temporary channel route in the navigation stack. The target highlight also appeared before the route settled and remained indefinitely.
Validation
just mobile-check