Skip to content

fix(mobile): fill and stabilize content-aware loading placeholders - #8083

Merged
klopez4212 merged 9 commits into
mainfrom
kennylopez-mobile-loading-shapes
Oct 5, 2026
Merged

klopez4212 merged 9 commits into
mainfrom
kennylopez-mobile-loading-shapes

Conversation

@klopez4212

Copy link
Copy Markdown
Contributor

Fill mobile loading views across the screen and use known image, video, voice-note, and file shapes in message placeholders. Freeze each loading cycle’s layout so arriving metadata cannot reshuffle the shimmer just before content appears; preserve cached Home and forum content during reconnects.

Validation: mobile analysis and full test suite passed (2,752 passed, 4 skipped), plus 3 unconfigured-push tests and file-size checks. Installed and launched the loading changes on Pixel 10 before the latest main merge. Full just ci was stopped during desktop checks due to low disk space.

Signed-off-by: kenny lopez <klopez4212@gmail.com>
…ding-shapes

Signed-off-by: kenny lopez <klopez4212@gmail.com>
@klopez4212
klopez4212 requested a review from a team as a code owner October 4, 2026 15:27
@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-05T15:23:19.860500Z efbe7bb 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.

@klopez4212
klopez4212 deployed to codex-review October 4, 2026 15:27 — with GitHub Actions Active
@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: f0eb5575ffc9d5f57af4ed3f574529d997c83a0d...efbe7bbd9163690613a688212f58a2afc756b862
  • 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 issues were found in the authorized PR range.

Findings

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

Notes

  • Review used read-only source, history, caller, and test inspection; repository builds and tests were not executed as instructed.

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: 38c9c88951

ℹ️ 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/message_skeleton_body.dart Outdated
Comment on lines +754 to +759
messages: buildMainTimelineEntries(
formatTimeline(
messagesState.value ?? const [],
currentPubkey: currentPubkey,
),
).map((entry) => entry.message).toList(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid formatting the timeline twice on every rebuild

When a stream is already loaded, this skeleton argument is still evaluated eagerly even though loading is false, and the messagesState.when data branch immediately runs formatTimeline and buildMainTimelineEntries again for the real content. On long histories, every parent rebuild—including session, typing, and read-state updates—now duplicates an O(n) transformation on the UI isolate. Compute the formatted entries once and reuse them, or defer skeleton derivation until a skeleton is actually needed.

Useful? React with 👍 / 👎.

Comment on lines +225 to +227
return state.isLoading
? SkeletonShimmer(child: waveformWidget)
: waveformWidget;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Exclude the loading waveform from semantics

While a remote voice note is loading, onSeek is now null, so the shimmered waveform is purely decorative; however, VoiceNoteWaveform still emits a Voice note waveform semantics node and SkeletonShimmer does not suppress it. Screen-reader users therefore encounter an extra non-actionable stop between the Loading voice note control and duration. Exclude semantics for the loading waveform and restore the slider semantics once playback is ready.

AGENTS.md reference: AGENTS.md:L264-L272

Useful? React with 👍 / 👎.

Comment on lines +118 to +120
child: SkeletonShimmer(
enabled: loading && shimmerEnabled,
child: skeleton,
child: loadingSkeleton.value,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep live status semantics outside the frozen skeleton

If a loading cycle changes from connecting to reconnecting without first leaving the loading state, this retained widget remains the original _MessageTimelineSkeleton or _ChannelsSkeleton, including its status-derived live-region label. The visual layout may intentionally stay frozen, but a screen reader will continue reporting Connecting instead of the current Reconnecting state. Freeze only the geometry/content snapshot while keeping the status semantics dynamic.

AGENTS.md reference: AGENTS.md:L264-L272

Useful? React with 👍 / 👎.

@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: 75a4efd0cabed81c1583cb3868b232b4bde50c81..38c9c88951bdd1a3731d037f49f766b343b0e159 (exact live head; systems/integration and product/UI/adversarial lanes reconciled).

Risk: medium — reconnect/recovery rendering now derives eager placeholder structure and geometry from member-controlled message content.

Author-actionable defects

  1. P2 performance/recovery — unbounded quadratic reconnect skeleton construction from member-controlled content. message_skeleton_body.dart:23-38 extracts every media-looking URL, then performs two whole-message replaceAll passes per URL; :61-65 eagerly emits an attachment widget per result. Relay ingest permits 256 KiB (crates/buzz-relay/src/handlers/ingest.rs:2466-2471), allowing thousands of distinct short media URLs. Cached messages execute this on the UI isolate during reconnect (mobile/lib/features/channels/channel_detail_page.dart:753-760), so recovery can freeze or exhaust the channel. Existing coverage uses only one or two attachments.

    • Author action: project media in one bounded pass, cap rendered skeleton attachments with explicit overflow behavior, and add an adversarial regression proving widget count remains capped and work does not grow quadratically.
    • Verification owner: author for fix/regression; reviewer for mutation-checking the bound.
  2. P2 layout stability — skeleton and loaded media use different geometry policies. MessageSkeletonBody applies ratio 0.2..4.0 and a 240px height cap to images and videos (mobile/lib/features/channels/message_skeleton_body.dart:78-88). Loaded videos instead clamp ratio to 0.75..1.91 with different sizing (mobile/lib/features/channels/message_content/video_preview.dart:47-50,88-93); loaded images also use their own viewport-width cap (message_content.dart:676-715). A valid portrait video such as 1200×2400 is currently blessed as a 120×240 skeleton, then reveals into a materially taller loaded preview, causing the exact jump this PR intends to remove.

    • Author action: share production image/video geometry helpers (or exactly equivalent policies), and assert skeleton bounds equal loaded bounds for portrait/landscape/ultrawide metadata at representative narrow/wide viewports. Do not encode divergent portrait-video geometry in the skeleton test.
    • Verification owner: author for causal tests; reviewer for mutation/source and native transition checks.

Validation and residual risk

Both lanes confirmed the geometry defect; the product lane additionally established the unbounded recovery path. Loading-cycle snapshot ownership, reconnect timer cleanup, community cache scoping, late video-frame disposal, reduced motion, and loading semantics were traced without another concrete defect.

Exact-head just mobile-check, analyzer, file-size checks, and git diff --check passed across the lanes. DCO, Semgrep, zizmor, and Codex security review passed; exact-head Mobile and Mobile Swift gates were still running. Full local Flutter tests did not start because the reviewer host’s unaccepted Xcode license breaks native-assets SDK lookup. Dark theme, large text, extreme layouts, and native transition pixels remain reviewer/tooling confidence gaps, not additional author work.

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

Signed-off-by: kenny lopez <klopez4212@gmail.com>
@klopez4212
klopez4212 deployed to codex-review October 4, 2026 15:51 — 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

🤖 Addressed both Jude findings in 345012978:

  • Bounded the skeleton projection to 8,192 content characters, bounded metadata, and four attachment shapes plus an overflow placeholder. Removed per-URL full-message rewrites. Regressions cover 6,000 embeds and the input boundary; removing the attachment cap makes the regression fail.
  • Shared production image sizing and video aspect-ratio policies. Eighteen widget cases compare skeleton bounds with real previews before and after decoding for portrait, landscape, and ultrawide metadata at 320, 430, and 800px viewports, including narrower parent constraints.

The parser also keeps ordinary image links and media-looking code inline. Mobile analysis, file-size checks, focused media tests, and the full pre-push mobile suite passed. Native transition visuals have not been rechecked for this revision. Ready for re-review.

@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: 3450129780

ℹ️ 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/message_skeleton_body.dart
Comment thread mobile/lib/features/channels/message_skeleton_body.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: 75a4efd0cabed81c1583cb3868b232b4bde50c81..34501297801df0d1946c3c1071eb8063819d0df8 (exact live head; systems/integration and product/UI/adversarial lanes reconciled).

Risk: medium — reconnect/recovery layout is derived from bounded parsing of member-controlled Markdown and must match loaded production surfaces.

The previous unbounded/quadratic construction is fixed: projection is bounded, per-URL whole-message rewrites are gone, attachment count is capped with overflow, and metadata-backed image/video geometry is shared. Four author-actionable P2 gaps remain:

  1. No-dim image geometry still diverges under narrow parent constraints. MessageSkeletonBody computes the viewport-default square, clamps only width to the parent, but retains the unclamped height (mobile/lib/features/channels/message_skeleton_body.dart:113-128; default in message_media_geometry.dart:13-18). Production’s metadata-free image branch reserves the parent max width × 240 through constraints (message_content.dart:635-685). At a 320px viewport with a 208px parent this is roughly 208×230.4 vs 208×240; narrower cases diverge more.

    • Author action: unify metadata-free/malformed image reservation with production and add pre/post-decode parity rows at narrow parent widths.
  2. Trailing multi-image galleries reserve one full block per embed instead of production’s single carousel. Skeleton code emits each attachment independently (message_skeleton_body.dart:95-99), while production collapses 2+ trailing images into one 220px carousel (message_content/media_carousel.dart:59-101,192-205). Four portrait images can reserve ~960px before revealing ~246px.

    • Author action: project galleries using the same production grouping/geometry and add causal multi-image parity coverage.
  3. Unknown/non-image embeds reserve a 64px file row although production renders them through image-preview fallback. Skeleton _attachment falls through to file geometry (message_skeleton_body.dart:202-220), whereas production _buildMedia falls through to _MessageImagePreview for every non-audio/non-video Markdown embed (message_content.dart:372-404). PDFs or extensionless embeds therefore jump materially on reveal.

    • Author action: align fallback classification/geometry with production and cover PDF/extensionless/unknown embeds at the production seam.
  4. The 8,192-character cutoff can reinterpret media-looking inline/fenced code as an attachment. Input is truncated before tokenization (message_skeleton_body.dart:25-30,49-69). If a code delimiter opens before the cutoff and closes after it, the truncated prefix loses the closing delimiter and an embedded image token inside code can be emitted as media.

    • Author action: make cutoff handling token-safe/fail-closed (for example, parse only complete tokens in a safe prefix or use overflow when Markdown/code state is uncertain), with inline-code, fenced-code, and embed-crossing-cutoff regressions.

Verification owner: author for fixes and causal package tests; reviewer for mutation/projection and native transition recheck.

Validation: both lanes reviewed the exact clean head; git diff --check, analyzer/format/file-size checks, and exact-head Mobile/Mobile Swift/security/static gates passed. Reconnect snapshot lifecycle, cache scoping, reduced motion, semantics, and disposal paths were traced without another defect. Local Flutter execution did not start because this reviewer host’s unaccepted Xcode license breaks native-assets SDK lookup; native transition visuals were not independently rerun. Those are confidence gaps, not additional author work.

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: REQUEST CHANGES

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

Risk: medium — the resource-exhaustion path is bounded now, but several skeleton projections still reserve geometry unlike the production content they reveal into.

Resolved from the prior review

The reconnect projection now bounds inspected content (8,192 characters), metadata (64 tags × 16 parts × 2,048 characters), and emitted attachment shapes (four plus overflow), removes per-URL whole-message rewrites, and has adversarial cap coverage. Shared metadata-based image/video geometry is dependency-neutral and agrees with production across the new 18-case matrix.

Remaining author-actionable defects

  1. P2 — metadata-free images still reserve different constrained geometry. The skeleton computes a square default at viewport width, clamps only its width to the parent, and retains the pre-clamp height (mobile/lib/features/channels/message_skeleton_body.dart:121-130; message_media_geometry.dart:13-18). Production's absent/malformed-dim branch instead uses constraint-driven maxWidth × 240 (message_content.dart:674-685). At a 320px viewport with 56px side insets, this is roughly 208×230.4 versus 208×240; narrower parents diverge further.

    • Author action: share/equate the no-metadata constraint policy and add absent/malformed-dim parity tests at narrow parents, before and after decode.
  2. P2 — trailing image galleries are projected as independent full image blocks. The skeleton emits one attachment block per embed (message_skeleton_body.dart:95-99), while production collapses two or more trailing image lines into one fixed-height carousel (message_content/media_carousel.dart:59-101,192-205). Four portrait images can reserve about 960px before revealing into one ~220px carousel plus label/spacing.

    • Author action: make the bounded projection recognize the same trailing-gallery seam and reserve the production carousel geometry; add a causal multi-image parity test.
  3. P2 — unknown/non-image Markdown embeds use a skeleton-only file row. _attachment falls through to a 64px file shape (message_skeleton_body.dart:202-222), but production routes every non-audio/non-video embed—including PDF or extensionless URLs—to _MessageImagePreview (message_content.dart:372-404). The reveal therefore expands into image/fallback geometry.

    • Author action: align fallback classification/geometry with production and cover PDF plus extensionless embeds at the real skeleton/preview seam.
  4. P2 — truncating before tokenization can reinterpret code as media. The source is cut at 8,192 characters before the Markdown/code regex runs (message_skeleton_body.dart:25-30,49-69). If an inline or fenced code span opens before the cutoff and closes after it, a nested ![…](…) can be parsed as a real attachment. The claimed “media-looking text in code stays text” guarantee therefore fails at the adversarial boundary.

    • Author action: use a token-safe prefix or fail closed to overflow when cutoff state is uncertain; add inline-code, fenced-code, and embed-crossing-cutoff regressions.

Verification owner: author for fixes and causal tests; reviewer for exact-head source/mutation checks and native transition evidence; CI for the refreshed Mobile gates.

Validation and confidence gaps

At exact head, git diff --check, changed-file Dart analysis, local format/file-size policy checks, GitHub Clients / Mobile, Mobile Swift, DCO, Semgrep, zizmor, and security review passed. Reconnect snapshot ownership, cache preservation, cleanup, reduced motion, and loading semantics were re-traced without another concrete defect. Local focused Flutter tests could not start because this host's unaccepted Xcode license breaks the objective_c native-assets SDK probe; exact-head Mobile CI supplies the package gate. Native iOS reconnect/reveal visuals remain reviewer/tooling-owned confidence work after the code defects are fixed.

Cleanup: PR-specific review worktrees removed; no REPOS/*-carl checkout retained.

Signed-off-by: kenny lopez <klopez4212@gmail.com>
…ding-shapes

Signed-off-by: kenny lopez <klopez4212@gmail.com>
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

Copy link
Copy Markdown
Contributor Author

🤖 Addressed all four follow-up findings in f7d1348b9; latest head 8a5820059 also merges current main and distinguishes the hidden gallery label from its loaded counterpart:

  • Absent/malformed image dimensions now share a stable reservation with production, including after decoding; decoded images retain BoxFit.contain.
  • Skeletons and real galleries share trailing-image extraction, label spacing, and the 220px carousel layout instead of stacking image placeholders.
  • PDF, extensionless, and unknown embeds now use production's image-preview fallback geometry.
  • Messages above 8,192 characters fail closed to one overflow placeholder before parsing, so a cutoff cannot reinterpret code or a partial embed.

Validation: 201 focused tests passed, including missing/malformed dimensions, PDF/extensionless/failed-image previews, narrow parents, multi-image galleries at normal/large text, and inline-code/fenced-code/embed cutoff regressions. Full pre-push mobile checks also passed. Native transition visuals have not been rechecked on this revision. Ready for re-review.

@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: 8a58200595

ℹ️ 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".

padding: const EdgeInsets.only(bottom: Grid.xs),
child: _MessageSkeletonRow(
index: index % 4,
message: index < messages.length ? messages[index] : null,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Render the newest cached rows at the bottom

During a reconnect with cached messages, formatTimeline supplies these entries chronologically, so indexing this list from zero renders the oldest cached message at the top. The production timeline does the opposite (channel_detail_page/message_list.dart reverses the list and maps index 0 to the newest entry at the bottom), meaning even a single known media placeholder appears near the app bar and then jumps to the bottom when content is revealed. Reverse and bottom-anchor the skeleton rows to match the production timeline.

Useful? React with 👍 / 👎.

…ding-shapes

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

# Conflicts:
#	mobile/lib/features/channels/message_content/media_carousel.dart
#	mobile/test/features/channels/message_content_test.dart
@klopez4212
klopez4212 deployed to codex-review October 4, 2026 16:52 — with GitHub Actions Active

@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: cc0fa5ab41

ℹ️ 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 on lines +37 to +38
(loadedChannels == null ||
(showConnectionSkeleton && loadedChannels.isEmpty)) &&

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve a successfully loaded empty channel list

When the cached snapshot is a successful [], a reconnect lasting two seconds sets showConnectionSkeleton, and this condition replaces the valid “No conversations yet” state with a skeleton for the rest of the reconnect. This contradicts VISION_MOBILE.md’s explicit requirement that a fetched channel list remain available during connectivity loss. Treat every non-null loadedChannels value—including an empty list—as loaded content, and reserve the skeleton for the cold null state.

AGENTS.md reference: AGENTS.md:L11-L18

Useful? React with 👍 / 👎.

@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 4, 2026
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 5, 2026
@klopez4212
klopez4212 deployed to codex-review October 5, 2026 12:36 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 5, 2026
@klopez4212
klopez4212 requested a review from jedwards27 October 5, 2026 13:24

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

Reviewed exact head 0107b5ffbbdf3b83945f965b8e8521868b1d3080 against base f0eb5575ffc9d5f57af4ed3f574529d997c83a0d as :bot: Jude’s code review agent.

The four previous blockers are resolved: shared narrow-parent geometry now covers missing/malformed dim; gallery projection uses the production grouping/frame seam; PDF, extensionless, and unknown embeds follow production fallback classification; and content exceeding 8,192 characters fails closed before tokenization. The bounded projection and reduced-motion tests are also materially stronger.

However, this head introduces five author-actionable regressions in the reconnect/accessibility path:

  1. [P2] Preserve production timeline orientation during reconnect. _MessageTimelineSkeleton renders chronological messages[index] in a normal top-anchored ListView (mobile/lib/features/channels/channel_detail_page/banners.dart:80-99), while production is reversed and maps index 0 to the newest entry at the bottom (mobile/lib/features/channels/channel_detail_page/message_list.dart:761-792). A populated reconnect therefore shows the oldest cached rows at the top, then jumps to newest-at-bottom on reveal. Use the same newest-first, bottom-anchored projection and add a multi-message before/after seam test. Verification: author regression test; reviewer mutation/native transition.

  2. [P2] Keep a successfully loaded empty channel list during reconnect. loadedChannels == [] becomes loading whenever showConnectionSkeleton is true (mobile/lib/features/channels/channels_page/body.dart:35-39), replacing the valid empty state after the grace period. Treat every non-null list, including empty, as loaded and add an empty-list sibling to the cached reconnect test. Verification: author regression test; reviewer source/mutation.

  3. [P2] Do not freeze live connection semantics with placeholder geometry. SkeletonReveal retains the entire skeleton widget for one loading cycle (mobile/lib/shared/widgets/skeleton_reveal.dart:33-39,118-121), including the status-derived Connecting/Reconnecting live-region label (mobile/lib/features/channels/channel_detail_page/banners.dart:68-79). A connecting→reconnecting transition in one cycle continues announcing “Connecting.” Freeze only the geometric/content projection while keeping status semantics live; add a same-cycle semantics transition test. Verification: author test; reviewer semantics-tree/native check.

  4. [P2] Remove the decorative loading waveform from the semantics tree. Loading disables onSeek but only wraps VoiceNoteWaveform in shimmer (mobile/lib/features/channels/voice_note_attachment.dart:202-227); the waveform still emits label: Voice note waveform without slider actions (mobile/lib/features/channels/voice_note_waveform.dart:87-99). Exclude it while loading and prove adjustable semantics return when ready. Verification: author semantics regression; reviewer semantics-tree/native check.

  5. [P2] Avoid formatting a loaded timeline twice on every parent rebuild. The eager skeleton: argument runs formatTimeline + buildMainTimelineEntries (mobile/lib/features/channels/channel_detail_page.dart:695-701) even when not loading, then the data branch repeats both (:724-737). Reuse one derivation or defer the snapshot until a skeleton enters, with an instrumentation regression proving loaded rebuilds perform one transform. Verification: author causal counter; reviewer mutation/performance sanity.

Gates at the pinned head: clean worktree; git diff --check passed; local just mobile-check passed in the systems lane; GitHub Mobile, Mobile Swift Domain, DCO, Semgrep, zizmor, and security checks are green. Full local mobile-test could not start because this host's objective_c native-assets hook received no Xcode SDK path; exact-head CI reports the full mobile suite green. Native transition pixels and VoiceOver/TalkBack behavior remain reviewer/release confidence work, not additional author defects.

Verdict: REQUEST CHANGES.

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 5, 2026
@klopez4212
klopez4212 deployed to codex-review October 5, 2026 15:10 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 5, 2026
@klopez4212

Copy link
Copy Markdown
Contributor Author

🤖 Addressed all five findings from the review of 0107b5ff in efbe7bbd9:

  • Reconnect rows use newest-first indexing in a reversed, bottom-anchored list, with the measured composer inset kept live. A multi-message seam test checks ordering before, during, and after reconnect.
  • Every fetched channel list, including [], remains visible during reconnect; removed the obsolete Home reconnect-skeleton timer.
  • Live connection labels now sit outside the retained visual snapshot. A same-cycle connecting → reconnecting semantics test verifies the announcement updates.
  • Loading voice waveforms are excluded from semantics; the regression verifies increase/decrease actions return when playback becomes ready.
  • Content and skeleton share one formatted timeline and one entry projection. Debug instrumentation counts the real formatter during loaded page rebuilds; adding a second formatting call makes the test fail with 2 rather than 1.

Validation: mobile analysis, formatting, and file-size checks passed. After a stalled pre-push test run, an independent full suite with concurrency 2 completed successfully: 2,908 passed and 4 skipped. The three unconfigured-push tests also passed. Existing placeholder-snapshot/reduced-motion tests and the new reconnect/accessibility regressions are included. Native VoiceOver/TalkBack and transition visuals have not been rerun on this revision. Ready for re-review.

@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: efbe7bbd91

ℹ️ 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 on lines +58 to +60
final tokens = RegExp(
r'```[\s\S]*?(?:```|$)|`[^`\n]*`|(!?)\[([^\]\n]*)\]\((https?://[^\s)]+)\)|https?://[^\s)<>]+',
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Honor multiline code spans in the loading projection

When a cached message contains a code span crossing a newline, such as `before\n![photo](https://example.com/a.jpg)\nafter`, production's normalizeBareLinks explicitly recognizes the closer beyond the line boundary, but this [^\n]*alternative stops at the newline and the later image alternative promotes the URL to a large attachment skeleton. A reconnect therefore reserves media geometry for content that remains inline code when revealed. The fresh evidence beyond the prior ordinary-link/code fix is the newline-spanning form; add a comparison through the realMessageContentrenderer rather than testing onlyMessageSkeletonBody`.

AGENTS.md reference: AGENTS.md:L234-L238

Useful? React with 👍 / 👎.

@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 — APPROVE at exact head efbe7bbd9163690613a688212f58a2afc756b862 (base f0eb5575ffc9d5f57af4ed3f574529d997c83a0d).

Both independent lanes found the five prior P2 defects resolved on production paths:

  1. The message skeleton now uses the production reversed/newest-at-bottom projection, with older/newer geometry and bottom-half anchor coverage (mobile/lib/features/channels/channel_detail_page/banners.dart:68-89, loading_review_tests.dart:24-90).
  2. A fetched empty channel list remains the truthful loaded “No conversations yet” state through reconnect; loading is limited to loadedChannels == null (mobile/lib/features/channels/channels_page/body.dart:31-42, channels_page_test.dart:927-957).
  3. Frozen skeleton geometry is separated from the live connection label, so Connecting → Reconnecting semantics update during the same loading cycle (mobile/lib/shared/widgets/skeleton_reveal.dart:85-140, channel_detail_page.dart:700-724, loading_review_tests.dart:91-123).
  4. Loading waveforms are excluded from semantics; adjustable increase/decrease actions return only when ready (voice_note_attachment.dart:202-230, message_content_test.dart:1217-1271).
  5. One shared timeline derivation feeds skeleton and content, and production-path instrumentation asserts one formatter invocation per loaded rebuild (channel_detail_page.dart:600-611,708-747, loading_review_tests.dart:4-21).

Author action: none.

Exact-head local/remote equality, clean-worktree, git diff --check, just mobile-check, policy, DCO, security, and Mobile Swift evidence passed. Clients / Mobile was still running at submission.

Confidence gaps — no author action: the reviewer host’s objective_c/Xcode SDK failure blocked full local mobile tests; native reconnect pixels and VoiceOver/TalkBack behavior were not independently observed, and the new rows were not independently mutation-run. Verification owner: exact-head Mobile CI for package completion; reviewer/release tooling for optional native reconnect/accessibility smoke. A causal CI failure should be reassessed, but these gaps are not defects.

@klopez4212
klopez4212 merged commit 46e6020 into main Oct 5, 2026
42 checks passed
@klopez4212
klopez4212 deleted the kennylopez-mobile-loading-shapes branch October 5, 2026 15:26

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

Reviewed exact head efbe7bbd9163690613a688212f58a2afc756b862 against base f0eb5575ffc9d5f57af4ed3f574529d997c83a0d as :bot: Jude’s code review agent.

The refreshed head resolves all five prior P2 findings on the production paths:

  1. The reconnect message skeleton now uses the production newest-at-bottom model: a reversed list maps builder index 0 to the newest cached message. The seam regression checks older/newer geometry before, during, and after reconnect and verifies bottom-half anchoring (mobile/lib/features/channels/channel_detail_page/banners.dart:68-89; mobile/test/features/channels/channel_detail_page_test/loading_review_tests.dart:24-90).
  2. A fetched empty channel list remains the truthful loaded state through reconnect; loading is now limited to loadedChannels == null (mobile/lib/features/channels/channels_page/body.dart:31-42; mobile/test/features/channels/channels_page_test.dart:927-957).
  3. SkeletonReveal retains only the visual skeleton snapshot. Its current connection label remains live outside that snapshot, and the regression proves Connecting → Reconnecting within one loading cycle (mobile/lib/shared/widgets/skeleton_reveal.dart:125-140; mobile/test/features/channels/channel_detail_page_test/loading_review_tests.dart:93-123).
  4. The loading voice-note waveform is excluded from semantics, while the ready waveform restores adjustable increase/decrease actions (mobile/lib/features/channels/voice_note_attachment.dart:205-230; mobile/test/features/channels/message_content_test.dart:1235-1271).
  5. One shared timeline derivation now feeds both the skeleton and loaded content; instrumentation asserts exactly one formatter invocation across repeated loaded rebuilds (mobile/lib/features/channels/channel_detail_page.dart:600-611,708-747; mobile/test/features/channels/channel_detail_page_test/loading_review_tests.dart:4-21).

No concrete author-actionable defect remains. Author action: none.

Exact-head evidence: local clean worktree and git diff --check passed; both independent lanes report just mobile-check passing at this head; GitHub Clients / Mobile, Mobile Swift Domain / Mobile Swift, DCO, Semgrep, zizmor, and security checks are green. Local Flutter tests could not start on the reviewer host because the objective_c native-assets hook could not obtain an Xcode SDK path; exact-head Mobile CI supplies full-package evidence.

Residual confidence: native reconnect/reveal pixels and VoiceOver/TalkBack announcements were not independently observed. Verification owner: reviewer/release device smoke; no author action absent a reproduced failure.

Verdict: APPROVE.

This branch was successfully deployed

1 active deployment
codex-review — efbe7bbd Deployed Oct 5, 2026 by klopez4212 via Run Codex Security Review #6859
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