Conversation
Investigation of two asks: quick scroll-to-bottom, and threads opening at the user's last-read reply. Findings: scroll-to-bottom already ships on both the channel timeline (MessageTimeline.tsx:862) and the thread panel (MessageThreadPanel.tsx:851). Thread resume is half-built - firstUnreadReplyId is computed and drives the UnreadDivider, but nothing seeds it as a scroll target, so every thread open bottom-pins (useAnchoredScroll.ts:613). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hn8yuxahWt1TXxB6vDKXG Signed-off-by: Michael Feth <michael@jira-flow.com>
thread_detail_page.dart was 996 lines by the ratchet's count (check-file-sizes-core.mjs splits on newlines, so a 995-line newline-terminated file counts 996) against a hard limit of 1000. Four lines of headroom left no room for feature work. Moves _NestedThreadSummaryRow and _ThreadMessage/_Avatar into part files under thread_detail_page/, matching the convention already used by channels_page.dart and activity_page.dart. Parent drops to 603. Pure relocation: reconstructing the original from the three files is byte-identical to the pre-split content. No behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hn8yuxahWt1TXxB6vDKXG Signed-off-by: Michael Feth <michael@jira-flow.com>
The channel timeline has had a frosted 'Latest' pill since it landed; the thread page had no way back to the newest reply short of manual scrolling. Extracts the channel's private _JumpToLatestButton into a shared JumpToLatestButton (both pages live in features/channels, so no cross-feature import). The render output is unchanged - only the surface ValueKey is now a parameter so each surface stays independently testable. The thread page needed a new signal to drive it: followsThreadTail means 'auto-scroll is armed', not 'at the tail'. It is set true on the first frame from the short route snapshot, so it never reports scrolled-away on a long thread. isAtThreadTail is instead driven from the existing threadTailIsVisible() geometry check, guarded on a non-empty position set so the pill cannot flash during first layout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hn8yuxahWt1TXxB6vDKXG Signed-off-by: Michael Feth <michael@jira-flow.com>
Opening a thread always landed on the newest reply, so anyone returning to a busy thread had to scroll back to find where they stopped. Captures each reply's read timestamp during build - before the existing post-frame effect marks every loaded reply read - and resumes at the oldest reply the snapshot says is unread, with a 'New' divider above it. The build-time capture is load-bearing: markers are monotonic, so a snapshot taken in an effect would already be the post-mark value and both the divider and the resume point would collapse on open. Deep links keep precedence, own replies never count as unread, and the resume opts out of tail-following so the arrival effect does not yank the reader back to the newest reply. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hn8yuxahWt1TXxB6vDKXG Signed-off-by: Michael Feth <michael@jira-flow.com>
Extracts the first-unread selection out of ThreadDetailPage.build into a pure firstUnreadThreadReplyId(), mirroring desktop's computeThreadUnreadMarker. Inline in build it was unreachable from flutter test; as a free function the interesting cases are directly testable. Covers the two rules that are easy to regress: a captured null means 'never read' (so membership is tested with containsKey, not ??), and own replies never count as unread. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hn8yuxahWt1TXxB6vDKXG Signed-off-by: Michael Feth <michael@jira-flow.com>
Two defects found reviewing the resume: 1. The open-time snapshot read a bare msg: marker. Opening a channel writes only the channel marker and thread: markers are never written, so on a thread's first open every reply resolved to null and the whole thread read as unread - the resume degenerated to 'jump to the oldest reply' and drew a New divider under the head. Worse, long-pressing that same reply offered 'Mark unread', because message_actions uses isMessageUnread, which folds the channel marker. Two surfaces one long-press apart disagreed. Now uses the shared effectiveMessageReadAt resolver, so the divider, the actions menu, and the unread badge agree. 2. scrollTo's default alignment pins the item's leading edge to the top of the viewport, but the pill's visibility predicate requires the trailing edge to be on screen. For a reply taller than the viewport the two could never agree: tapping 'Latest' left the pill up and a second tap did nothing. Corrects the alignment from the measured position afterwards. Also hides the pill on a reply-less thread, where there is no latest to jump to and the old code would have scrolled up to the top of the head. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hn8yuxahWt1TXxB6vDKXG Signed-off-by: Michael Feth <michael@jira-flow.com>
|
Warning Review limit reached
Next review available in: 30 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe mobile channel UI now uses a shared latest-scroll button. Thread detail views track tail position, resume at the first unread reply, show an unread divider, and use extracted message components. Tests cover the new button and unread-reply selection. ChangesChannel scrolling and thread resume
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/scroll-and-thread-resume-plan.md`:
- Around line 78-82: Update the inline code fence containing
scrollTargetHighlights by adding blank lines before and after the fence and
declaring its language as ts or tsx, so the Markdown satisfies MD031 and MD040.
- Line 111: Correct the count-fidelity documentation around the unread-pill test
so it reflects the existing predicate: count the 3 top-level messages and the
job-progress row, while excluding the two non-broadcast thread replies. Update
both referenced test descriptions to expect four new messages, or consistently
change the predicate if that is the intended behavior.
- Around line 69-71: Update selectThreadResumeTargetId to verify that
firstUnreadReplyId is present in visibleReplies before returning it, returning
null when the target is hidden. Preserve the existing thread, huddle, deep-link,
hasReplies, and non-null target gates, and ensure the downstream resume flow
does not auto-expand collapsed branches.
- Line 3: Remove the machine-specific worktree filesystem path from the plan’s
Base/worktree metadata line, while retaining the base commit reference if
needed. Ensure no local path remains in the committed document.
- Line 141: Add an explicit accessibility strategy for automatic thread resume:
either announce the resumed position or deliberately move focus to the relevant
thread content, while preserving user control. Update the thread-resume
implementation and the plan’s validation checklist so this behavior is verified
before marking resume complete.
- Line 135: Update the resume and divider marker flow around
computeThreadUnreadMarker so forced unread state from forcedUnreadMsgRef is
passed through the shared resolver and included in the resume contract. Ensure
manually marked-unread threads both render the unread divider and resume at the
correct position; otherwise document the intentional behavior change and add a
test covering it.
- Around line 38-54: Resolve the MD029 numbering warnings in the ordered lists
of scroll-and-thread-resume-plan.md by replacing the repeated numeric prefixes
with explicit step IDs in bullet-list form, or by applying the repository’s
supported configuration for intentional continuation numbering. Preserve the
existing step order and content, and ensure markdownlint passes without
warnings.
- Line 40: Complete the countPendingTimelineArrivals declaration in the design
plan by adding the intended missing parameter, then keep the documented
implementation signature and all test or call-site examples consistent with that
parameter list.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dfa7b95a-cb21-469d-b6e0-fa47c750fe33
📒 Files selected for processing (11)
docs/design/scroll-and-thread-resume-plan.mdmobile/lib/features/channels/channel_detail_page.dartmobile/lib/features/channels/channel_detail_page/message_list.dartmobile/lib/features/channels/jump_to_latest_button.dartmobile/lib/features/channels/thread_detail_page.dartmobile/lib/features/channels/thread_detail_page/nested_thread_summary.dartmobile/lib/features/channels/thread_detail_page/thread_message.dartmobile/lib/features/channels/thread_unread_marker.dartmobile/lib/features/channels/unread_divider.dartmobile/test/features/channels/jump_to_latest_button_test.dartmobile/test/features/channels/thread_unread_marker_test.dart
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Desktop Smoke E2E (1)
- GitHub Check: Desktop Smoke E2E (2)
- GitHub Check: Desktop Smoke E2E (3)
- GitHub Check: Desktop Smoke E2E (4)
- GitHub Check: Desktop Build (macOS)
- GitHub Check: Desktop E2E Relay
- GitHub Check: Desktop Core
- GitHub Check: Mobile
🧰 Additional context used
🪛 LanguageTool
docs/design/scroll-and-thread-resume-plan.md
[style] ~27-~27: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...(4 props, leaf). No module-level state. No text-size class of its own — sizing com...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~52-~52: Consider an alternative for the overused word “exactly”.
Context: ...h --experimental-strip-types. This is exactly why useAnchoredScroll.test.mjs import...
(EXACTLY_PRECISELY)
[style] ~97-~97: Consider an alternative for the overused word “exactly”.
Context: ...so positions survive remounts", that is exactly the leak useCommunityInit.ts:50-76 fo...
(EXACTLY_PRECISELY)
[style] ~132-~132: Consider replacing this word to strengthen your wording.
Context: ...rk landed. 4. S5 is a behavior change and will break existing specs correctly. ...
(AND_THAT)
[style] ~150-~150: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...way → false; return-to-bottom → true. - NEW `src/features/channels/lib/threadResume...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.23.2)
docs/design/scroll-and-thread-resume-plan.md
[warning] 38-38: Ordered list item prefix
Expected: 1; Actual: 7; Style: 1/2/3
(MD029, ol-prefix)
[warning] 39-39: Ordered list item prefix
Expected: 2; Actual: 8; Style: 1/2/3
(MD029, ol-prefix)
[warning] 40-40: Ordered list item prefix
Expected: 3; Actual: 9; Style: 1/2/3
(MD029, ol-prefix)
[warning] 44-44: Ordered list item prefix
Expected: 4; Actual: 10; Style: 1/2/3
(MD029, ol-prefix)
[warning] 48-48: Ordered list item prefix
Expected: 1; Actual: 11; Style: 1/2/3
(MD029, ol-prefix)
[warning] 49-49: Ordered list item prefix
Expected: 2; Actual: 12; Style: 1/2/3
(MD029, ol-prefix)
[warning] 50-50: Ordered list item prefix
Expected: 3; Actual: 13; Style: 1/2/3
(MD029, ol-prefix)
[warning] 51-51: Ordered list item prefix
Expected: 4; Actual: 14; Style: 1/2/3
(MD029, ol-prefix)
[warning] 52-52: Ordered list item prefix
Expected: 5; Actual: 15; Style: 1/2/3
(MD029, ol-prefix)
[warning] 53-53: Ordered list item prefix
Expected: 6; Actual: 16; Style: 1/2/3
(MD029, ol-prefix)
[warning] 54-54: Ordered list item prefix
Expected: 7; Actual: 17; Style: 1/2/3
(MD029, ol-prefix)
[warning] 78-78: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 78-78: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 82-82: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 173-173: Files should end with a single newline character
(MD047, single-trailing-newline)
🔇 Additional comments (14)
mobile/lib/features/channels/thread_detail_page/nested_thread_summary.dart (2)
3-53: LGTM!Also applies to: 59-113
54-58: 🎯 Functional CorrectnessNo conversion is required for
SizedBox.width.The expression starts with a
double, anddouble.operator +(num)returnsdouble. The result is assignable toSizedBox.width.> Likely an incorrect or invalid review comment.mobile/lib/features/channels/thread_detail_page/thread_message.dart (1)
3-284: LGTM!mobile/lib/features/channels/jump_to_latest_button.dart (2)
8-35: LGTM!Also applies to: 43-77
36-39: 🎯 Functional CorrectnessConfirm the Flutter SDK version before raising a compatibility issue.
mobile/pubspec.yamldeclares a Dart SDK constraint (sdk: ^3.11.4), not Flutter 3.11.4. The tracked repository has no Flutter SDK version pin, so the compile failure is not established for eitherColor.withValuescall.mobile/lib/features/channels/channel_detail_page.dart (1)
43-43: LGTM!mobile/lib/features/channels/channel_detail_page/message_list.dart (1)
553-557: LGTM!mobile/test/features/channels/jump_to_latest_button_test.dart (1)
8-80: LGTM!mobile/lib/features/channels/thread_detail_page.dart (1)
26-42: LGTM!Also applies to: 145-193, 273-335, 577-580, 664-678
mobile/lib/features/channels/thread_unread_marker.dart (1)
1-43: LGTM!mobile/lib/features/channels/unread_divider.dart (1)
1-30: LGTM!Also applies to: 34-59
mobile/test/features/channels/thread_unread_marker_test.dart (1)
1-105: LGTM!docs/design/scroll-and-thread-resume-plan.md (2)
4-4: 📐 Maintainability & Code QualityReconcile the implementation scope.
This document says both features are desktop-only, while the PR objective describes mobile thread navigation. Confirm which product surface this plan covers. If it is desktop, update the PR scope or link the mobile plan; if it is mobile, replace the desktop-specific implementation and validation references.
136-136: 🎯 Functional CorrectnessConfirm the muted-thread resume policy.
Muted threads skip the on-open mark-read, so their
msg:frontier never advances. Reopening the same thread will select the same reply indefinitely. Confirm that this is intentional; otherwise define a separate mark/resume rule and add regression coverage.
| 7. **`useBufferedTimelineMessages.ts:11`** — add `export function selectPendingTimelineArrivals<T extends {id:string}>({buffered, messages})`: returns a module-level frozen `EMPTY_PENDING` when lengths match, else `messages.filter(m => !bufferedIdSet.has(m.id))`. | ||
| 8. **`useBufferedTimelineMessages.ts:83-89`** — return `{ messages, pendingCount, pendingMessages }` wrapped in a `useMemo` over `(stableBuffered, messages)`. Both are reference-stable (the identity gate at `:76-84` is what makes the `Set` build affordable); keep `pendingCount` unchanged so the new filtered count can be diffed against it in a unit test. | ||
| 9. **NEW `desktop/src/features/messages/lib/pendingTimelineArrivals.ts`** — `countPendingTimelineArrivals(pending, )`. **Predicate: `message.parentId == null || isBroadcastReply(message.tags ?? [])` and nothing else.** | ||
| - **Do NOT filter by `pubkey`.** `virtualization.spec.ts:822-831` emits with no `pubkey` (self, `MOCK_IDENTITY_PUBKEY = DEFAULT_MOCK_IDENTITY.pubkey`, `e2eBridge.ts:1443`) and asserts `toContainText("1")`. An own-pubkey filter zeroes it. It is also wrong on the merits — an own message from another device *is* a new row. | ||
| - **Do NOT filter by `isConversationalUnreadKind`.** System rows group and render (`timelineItems.ts:228`) and job-lifecycle kinds are not filtered from rendering at all. Excluding them makes "3 new" release 4 rows. | ||
| - The criterion is *rows the reader will see on release*. Only the thread-reply filter serves it. | ||
| 10. **`MessageTimeline.tsx:872-878`** — replace the three-branch label with `pendingArrivalCount > 0 ? unreadCountLabel(pendingArrivalCount) : "Jump to latest"`, computed by a `useMemo` over `bufferedTimeline.pendingMessages`. Drop `newMessageCount` from the destructure at `:349` (grep confirms `:875-876` are its only uses in this file). **Leave `useAnchoredScroll`'s return alone** — `MessageThreadPanel.tsx:862` still consumes it. | ||
|
|
||
| ### A-3. Gate the read advance on being at the live edge | ||
|
|
||
| 11. **`MessageTimeline.tsx:125`** — add `onAtBottomChange?: (atBottom: boolean) => void`. Publish `isSemanticallyAtBottom` (rAF-coalesced at `:382-391`, transient-guarded at `:417`), **not** the raw pixel flag. Emit `true` from an unmount cleanup so "no timeline" never means "withhold read forever". **MessageTimeline is `React.memo`'d (`:892`)** — callers must pass a `useState` setter, never an inline arrow. | ||
| 12. **`ChannelPane.types.ts:167` / `ChannelPane.tsx:155` / `:604`** — forward as `onTimelineAtBottomChange`. ChannelPane is also memo'd (`:66`); a stable setter preserves both. | ||
| 13. **`ChannelScreen.tsx:150`** — `const [isTimelineAtBottom, setIsTimelineAtBottom] = React.useState(true)`, reset to `true` on `activeChannelId` change so a switch never inherits the previous channel's scrolled-away state. | ||
| 14. **Scope the publisher off for huddles.** There is exactly one `<MessageTimeline>` (`ChannelPane.tsx:558`) and one `<ChannelPane>`; the huddle transcript is a *prop mode* of the same mount (`isHuddleTranscript`, `ChannelPane.tsx:570-575`). Pass `onAtBottomChange={isHuddleTranscript ? undefined : onTimelineAtBottomChange}` or the transcript's scroll position gates the channel's marker. `useHuddleReadMarker.ts:78` is a second ungated `markChannelRead` path — **declare it explicitly out of scope in the PR body.** | ||
| 15. **NEW `desktop/src/features/channels/lib/channelReadAdvancePolicy.ts`** — `shouldAdvanceChannelReadMarker({ isChannelOpenCommit, isTimelineAtBottom })` returning `isChannelOpenCommit || isTimelineAtBottom`. **A separate pure module, not inside the hook** — `useChannelOpenReadState.ts:3` imports `@/app/AppShellContext`, and the node:test loader would drag that whole graph through `--experimental-strip-types`. This is exactly why `useAnchoredScroll.test.mjs` imports `./anchoredScrollPolicy.ts`. | ||
| 16. **`useChannelOpenReadState.ts:20-44`** — add a 4th param `isTimelineAtBottom`, a `markedChannelIdRef` to detect the open commit, and the policy early-return. **Set the ref only when `activeReadAt !== null`.** On the first commit after a switch `messagesQuery.data` is often unloaded → `latestActiveMessage === null` → `activeReadAt === null` → `resolveChannelReadMarker` yields `markAt === null` and marks nothing. Setting the ref anyway makes the real mark arrive as a non-open commit and, if the reader is not at bottom, it is withheld — a channel that never clears. | ||
| 17. **Leave the inbox-override loop on its existing dep-driven cadence.** `locallyUnreadFeedItems` is a live dep; restricting it to the open commit skips items that become locally-unread while the channel is open. Gate only the `markChannelRead` call. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the ordered-list numbering warnings.
The document uses 7. through 17. across separated lists, but markdownlint reports MD029 for these prefixes. Use explicit step IDs in bullet lists, or configure the rule for intentional continuation numbering. Do not leave the document with known lint warnings.
🧰 Tools
🪛 LanguageTool
[style] ~52-~52: Consider an alternative for the overused word “exactly”.
Context: ...h --experimental-strip-types. This is exactly why useAnchoredScroll.test.mjs import...
(EXACTLY_PRECISELY)
🪛 markdownlint-cli2 (0.23.2)
[warning] 38-38: Ordered list item prefix
Expected: 1; Actual: 7; Style: 1/2/3
(MD029, ol-prefix)
[warning] 39-39: Ordered list item prefix
Expected: 2; Actual: 8; Style: 1/2/3
(MD029, ol-prefix)
[warning] 40-40: Ordered list item prefix
Expected: 3; Actual: 9; Style: 1/2/3
(MD029, ol-prefix)
[warning] 44-44: Ordered list item prefix
Expected: 4; Actual: 10; Style: 1/2/3
(MD029, ol-prefix)
[warning] 48-48: Ordered list item prefix
Expected: 1; Actual: 11; Style: 1/2/3
(MD029, ol-prefix)
[warning] 49-49: Ordered list item prefix
Expected: 2; Actual: 12; Style: 1/2/3
(MD029, ol-prefix)
[warning] 50-50: Ordered list item prefix
Expected: 3; Actual: 13; Style: 1/2/3
(MD029, ol-prefix)
[warning] 51-51: Ordered list item prefix
Expected: 4; Actual: 14; Style: 1/2/3
(MD029, ol-prefix)
[warning] 52-52: Ordered list item prefix
Expected: 5; Actual: 15; Style: 1/2/3
(MD029, ol-prefix)
[warning] 53-53: Ordered list item prefix
Expected: 6; Actual: 16; Style: 1/2/3
(MD029, ol-prefix)
[warning] 54-54: Ordered list item prefix
Expected: 7; Actual: 17; Style: 1/2/3
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/scroll-and-thread-resume-plan.md` around lines 38 - 54, Resolve
the MD029 numbering warnings in the ordered lists of
scroll-and-thread-resume-plan.md by replacing the repeated numeric prefixes with
explicit step IDs in bullet-list form, or by applying the repository’s supported
configuration for intentional continuation numbering. Preserve the existing step
order and content, and ensure markdownlint passes without warnings.
Source: Linters/SAST tools
|
|
||
| 7. **`useBufferedTimelineMessages.ts:11`** — add `export function selectPendingTimelineArrivals<T extends {id:string}>({buffered, messages})`: returns a module-level frozen `EMPTY_PENDING` when lengths match, else `messages.filter(m => !bufferedIdSet.has(m.id))`. | ||
| 8. **`useBufferedTimelineMessages.ts:83-89`** — return `{ messages, pendingCount, pendingMessages }` wrapped in a `useMemo` over `(stableBuffered, messages)`. Both are reference-stable (the identity gate at `:76-84` is what makes the `Set` build affordable); keep `pendingCount` unchanged so the new filtered count can be diffed against it in a unit test. | ||
| 9. **NEW `desktop/src/features/messages/lib/pendingTimelineArrivals.ts`** — `countPendingTimelineArrivals(pending, )`. **Predicate: `message.parentId == null || isBroadcastReply(message.tags ?? [])` and nothing else.** |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Complete the countPendingTimelineArrivals API declaration.
countPendingTimelineArrivals(pending, ) has a missing parameter. It is not an implementable function contract. Use the intended parameter list and keep the implementation and test calls consistent.
Proposed correction
-9. **NEW `desktop/src/features/messages/lib/pendingTimelineArrivals.ts`** — `countPendingTimelineArrivals(pending, )`.
+9. **NEW `desktop/src/features/messages/lib/pendingTimelineArrivals.ts`** — `countPendingTimelineArrivals(pending)`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 9. **NEW `desktop/src/features/messages/lib/pendingTimelineArrivals.ts`** — `countPendingTimelineArrivals(pending, )`. **Predicate: `message.parentId == null || isBroadcastReply(message.tags ?? [])` and nothing else.** | |
| 9. **NEW `desktop/src/features/messages/lib/pendingTimelineArrivals.ts`** — `countPendingTimelineArrivals(pending)`. **Predicate: `message.parentId == null || isBroadcastReply(message.tags ?? [])` and nothing else.** |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 40-40: Ordered list item prefix
Expected: 3; Actual: 9; Style: 1/2/3
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/scroll-and-thread-resume-plan.md` at line 40, Complete the
countPendingTimelineArrivals declaration in the design plan by adding the
intended missing parameter, then keep the documented implementation signature
and all test or call-site examples consistent with that parameter list.
| 2. **NEW `desktop/src/features/channels/lib/threadResumeTarget.ts`** — `selectThreadResumeTargetId(input): string | null`. Returns `firstUnreadReplyId` only if **all** hold: `openThreadHeadId !== null`; `!isHuddleTranscript`; `routeTargetMessageId === null` **and** `externalScrollTargetId === null` (deep links win, matching mobile's `hasUnreadDeepLink` short-circuit at `mobile/…/message_list.dart:58-59`); `hasReplies` (content gate — see below); `firstUnreadReplyId !== null`. It computes no unread math; it only gates the already-computed marker. | ||
| 3. **NEW `desktop/src/features/channels/ui/useThreadOpenResumeTarget.ts`** — holds a per-thread-open one-shot latch: `capturedRef: Map<headId, string|null>`, `consumedRef: Set<headId>`, a `headIdRef` mirror (declare it — the earlier draft referenced an undeclared `openThreadHeadIdRef`), and a cleanup effect keyed on `headId` that deletes both entries on close/switch. **Capture during render, not in an effect** — modeled on `openFrontierRef` (`useChannelUnreadState.ts:85-91`), which writes during render for the same reason. An effect-seeded target lands after the child's layout effect has already bottom-pinned. | ||
| - **Readiness gate: `threadMessages.length > 0`, not `!threadRepliesQuery.isPending`.** A forum channel's query is `enabled: false` (`useThreadReplies.ts:118-121`) and is `pending` forever. And `visibleReplies` can be non-empty from `resolvedMessages` before the query resolves. The snapshot (`useChannelUnreadState.ts:250-261`) and the divider memo (`:286`) are computed in the same render pass, so the first commit with non-empty `threadMessages` already carries the correct `firstUnreadReplyId`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not select a hidden reply as a resume target.
selectThreadResumeTargetId gates on hasReplies, not on whether firstUnreadReplyId is in visibleReplies. The downstream scroll path can clear collapsedThreadHeadId, so opening a thread can expand a branch that the user collapsed. Gate on target visibility and return null for hidden replies, or explicitly make auto-expansion part of the feature and test it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/scroll-and-thread-resume-plan.md` around lines 69 - 71, Update
selectThreadResumeTargetId to verify that firstUnreadReplyId is present in
visibleReplies before returning it, returning null when the target is hidden.
Preserve the existing thread, huddle, deep-link, hasReplies, and non-null target
gates, and ensure the downstream resume flow does not auto-expand collapsed
branches.
| ``` | ||
| const isResumeTarget = threadScrollTargetId === null && threadResumeScrollTargetId !== null; | ||
| scrollTargetHighlights={!layoutScrollTargetId && !isResumeTarget} | ||
| scrollTargetId={layoutScrollTargetId ?? externalThreadScrollTargetId} | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the inline code fence lintable.
The fence around scrollTargetHighlights triggers MD031 and MD040. Add blank lines around the fence and specify ts or tsx.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 78-78: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 78-78: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 82-82: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/scroll-and-thread-resume-plan.md` around lines 78 - 82, Update
the inline code fence containing scrollTargetHighlights by adding blank lines
before and after the fence and declaring its language as ts or tsx, so the
Markdown satisfies MD031 and MD040.
Source: Linters/SAST tools
| 4. **S5 is a behavior change and will break existing specs correctly.** Any spec that leaves a channel scrolled away, takes arrivals, and asserts no unread dot after switching away now fails. Budget time to read each failure rather than tuning the gate to make them pass. | ||
| 5. **The huddle transcript aliases the channel timeline.** One `MessageTimeline` mount, one `ChannelPane` mount; the transcript is a prop mode (`ChannelPane.tsx:570-575`). Without the `isHuddleTranscript` scope-off, transcript scroll position gates the channel's read marker. `useHuddleReadMarker.ts:78` remains a second ungated path. | ||
| 6. **The `pinned-center` anchor is released within one commit — do not claim reflow protection.** Init calls `onTargetReached` synchronously (`useAnchoredScroll.ts:645`) → `resolveExternal` true when `layoutTargetId === null` → target nulled → `releasePinnedCenter()` at `:880-884`. Conversely, `releasePinnedCenter` does *not* scroll, so a failure to release freezes the reader in place rather than hiding new replies. | ||
| 7. **Manual "Mark unread" is invisible to the resume.** `computeThreadUnreadMarker`'s 4th param `isForcedUnread` defaults to `() => false` (`unreadMarker.ts:117`) and the divider memo at `:296-305` passes only three arguments. `forcedUnreadMsgRef` reaches the badge and the row menu but not the divider — nor now the resume. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Include manual unread state in the resume contract.
The plan says forced unread state reaches the badge and row menu, but not the marker used by the divider and resume. A thread can display as unread and still open at the bottom without a divider. Pass isForcedUnread through the shared resolver, or make this behavior an explicit product decision with a test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/scroll-and-thread-resume-plan.md` at line 135, Update the resume
and divider marker flow around computeThreadUnreadMarker so forced unread state
from forcedUnreadMsgRef is passed through the shared resolver and included in
the resume contract. Ensure manually marked-unread threads both render the
unread divider and resume at the correct position; otherwise document the
intentional behavior change and add a test covering it.
The plan for the desktop side of this work was committed here before the mobile branch diverged. It describes unbuilt desktop changes and is not part of this PR; carrying it also drags the whole desktop CI matrix onto a mobile-only diff. Still in history at ee8aada if it is wanted later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hn8yuxahWt1TXxB6vDKXG Signed-off-by: Michael Feth <michael@jira-flow.com>
dart format collapsed argument lists in both new test files that fit on one line. Rewritten so each statement is a single line under 80 columns, which leaves the formatter no wrapping decision to disagree with. No behaviour change; assertions are identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hn8yuxahWt1TXxB6vDKXG Signed-off-by: Michael Feth <michael@jira-flow.com>
flutter analyze flagged unnecessary_import. Extracting _JumpToLatestButton into the shared widget removed the library's only dart:ui-exclusive symbol (ImageFilter); everything it still uses comes from material.dart. The shared widget keeps its own dart:ui import, which is genuinely needed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hn8yuxahWt1TXxB6vDKXG Signed-off-by: Michael Feth <michael@jira-flow.com>
|
Done — served its purpose. The Mobile job went green on 9ebf39b (format 0 changed, analyze clean, 1276 tests passed, APK built), verifying block#5657 whose own CI run is held at action_required. Desktop Core's earlier ETXTBSY failure was a flake: 2402/2402 on re-run. |
## Summary Fix two PostgreSQL channel-roster test fixtures so they satisfy migration 0032's canonical kind-39002 fence before the channel/membership store extraction moves them. This is a test-only prerequisite in the issue #2 extraction stack. It emits the canonical four-field `p` tags with authoritative roles, keeps the stale-history mutation after insertion, and gives the colliding other-tenant fixture its own matching roster. ## Stack - Exact base: codex/issue-6-finish-replaceable-store at da01840 ([block#6777](block#6777)) - Exact head: codex/issue-7-roster-test-fixtures at 21d1b26 - Structural tracker: TheSentinel454#2 - Domain issue: TheSentinel454#7 - Acceptance: TheSentinel454#17 and TheSentinel454#19 - Next: `codex/issue-7-channel-membership-store` ## Why separate The existing ignored tests are red against a freshly migrated current `main`: migration 0032 rejects their legacy two-field roster tags before the assertions run. The extraction PR is intended to be a pure move, so this fixture correction is isolated here rather than mixed into the channel-membership ownership diff. ## Non-goals - No production Rust or SQL changes. - No schema, lock, transaction, timeout, retry, API, or client-visible behavior changes. - No change to migration 0032's canonical roster rules. - No store ownership movement; that remains in the child PR. ## Risk Low and test-only. The main risk is accidentally changing the scenario rather than only its representation. The tests retain the same member counts, stale-versus-complete distinction, tenant collision, signer isolation, and lock-freshness assertions. ## Blox verification Author workstation: `buzz-tornquist-issue-2-store-stack` (`2046520`), native PostgreSQL 17.11. - Red before the change on a freshly migrated database: both focused tests failed with migration 0032's `kind 39002 roster contains an invalid p tag` constraint. - `cargo fmt --all --check`: pass - `git diff --check`: pass - `cargo clippy -p buzz-db --all-targets -- -D warnings`: pass - `channel::tests::large_roster_reconciliation_candidates_respect_snapshot_count_and_signer`: pass - `channel::tests::locked_member_snapshot_blocks_post_capture_membership_mutation`: pass - Cumulative exact-tip PostgreSQL matrix: pass ## Superseded pre-comment restack verification PR block#6700 merged before publication completed. This layer was restacked onto current main through the exact parent named above; the final cumulative tip is 2ddcc8a. Cumulative author gates passed: formatting and diff checks; buzz-db and buzz-relay all-target clippy with -D warnings; DB lib 111 passed / 200 ignored; ownership 22/22; observability 1/1; the full isolated PostgreSQL domain matrix; and relay lib 910 passed / 49 ignored. - Workstation: `buzz-tornquist-pr-6819-final-review` (`2057618`), fresh shallow checkout - Base: `ffbeaaf00810aa359ab85818ff4820f92263e45f` - Head: `c60e793eadde79d9eab9f48bbb2ede0ad4831f9b` - Findings: none Reviewed the test-only roster-fixture correction. The affected large-roster and owner fixture events now use canonical four-field `p` tags and preserve the intended roster cardinalities. The distinct other-community fixture remains isolated and the production implementation, SQL, spans, and API are untouched. Verification: format and diff checks passed; `buzz-db --all-targets` clippy passed with `-D warnings`; DB lib tests passed (111 passed, 200 PostgreSQL tests ignored); ownership (1/1) and observability (1/1) guards passed; all 18 channel PostgreSQL tests passed on native PostgreSQL 17 with migrations 1-32 successful. Final worktree was detached at the exact head and clean. Complete evidence archive SHA-256: `da8379f4f0ae3989eb3573162f0f19cbde733400000756ab0f63af3322244d4b`. ## Comment-addressed restack Review follow-up on block#6777 removed only the low-value replaceable ownership source test. This PR was restacked onto its rewritten parent; its production patch is unchanged. - Exact base: `da018405cc83605362125c2d5e5a3f91492431ac` - Exact head: `21d1b265c133292e6707e766cd4204e6a43f08af` - Final cumulative tip: `6fa2f104d42c6ba85bdf62e7ccb74ceaf4a84f67` - Per-layer patch-ID and tree audits confirm this PR’s production diff is unchanged from its pre-comment head. - Cumulative Blox gate: formatting and diff checks; strict `buzz-db`/`buzz-relay` Clippy; DB lib 111 passed / 200 ignored; ownership 21/21; observability 1/1; every moved PostgreSQL test; relay lib 910 passed / 49 ignored. - Independent re-review at this exact head: no findings; fresh exact-parent/head Blox review passed fmt/diff, strict `buzz-db` Clippy, DB lib 111 passed / 200 ignored, observability, and all 18 channel PostgreSQL tests; relay compilation was not applicable to this test-only layer. Signed-off-by: tornquist <tornquist@squareup.com>
## Current reconstructed head Exact base: `codex/issue-7-roster-test-fixtures` at `5df440c411be9705eb29a57f0c41f7239767e007` Exact head: `codex/issue-7-channel-membership-store` at `8ad0782ee311f5f51b714494ce750c5937f127cc` This current head removes `crates/buzz-db/tests/store_ownership.rs`; no replacement path-sensitive ownership test is introduced. Apart from removing that complete test-file diff, the production patch is byte-for-byte identical to the previously reviewed slice. This remains part of tracker #2 and the #17/#19 acceptance work. Independent exact-head review from a separate clean Blox workstation found no issues. Current-head evidence passed formatting, strict `buzz-db` clippy, 111 non-PostgreSQL library tests with 200 PostgreSQL tests ignored, the observability source test, relay consumer compilation, exact ownership/unique-span review checks, and 3 channel and 19 membership PostgreSQL tests on native PostgreSQL where applicable. ## Why Complete the channel ownership slice of [tracker #2](TheSentinel454#2) and [domain issue #7](TheSentinel454#7) while preserving the runtime/store boundary established by block#6660 and block#6668. This child stacks on the test-only fixture prerequisite block#6819 above block#6777 and carries forward PR block#6700's membership/replacement lock timing without changing lock or transaction behavior. ## What - Keep channel lifecycle, metadata, TTL advisory locking, and lifecycle tests in `channel.rs` - Move membership/roster records, SQL, advisory-lock helpers, `Db` methods, focused tests, and datastore spans to a dedicated `channel_members.rs` - Preserve existing `buzz_db::channel::*` paths with compatibility re-exports while exposing the dedicated module - Move the four roster-fence PostgreSQL tests out of `lib.rs` ## Stack - Exact base: codex/issue-7-roster-test-fixtures at 21d1b26 ([block#6819](block#6819)) - Exact head: codex/issue-7-channel-membership-store at 25138bf - Tracker: TheSentinel454#2 - Domain: TheSentinel454#7 - Test/span acceptance: TheSentinel454#17 and TheSentinel454#19 ## Non-goals - No SQL, schema, retry, timeout, lock ordering, transaction boundary, or client-visible behavior changes - No change to channel TTL lifecycle ownership merely because lifecycle bootstrap writes an owner membership row - No store traits, domain-handle redesign, broad `PgExecutor` migration, raw pool accessor, new crate, or directory-wide reorganization - No changes to, retargeting of, or merge action on PR block#6700 or block#6777 ## Risk Assessment Moderate review surface, low semantic risk. The file split is large, but method signatures, SQL, bind order, membership and replacement lock namespaces, transaction boundaries, and span names remain unchanged. Compatibility re-exports preserve existing `buzz_db::channel::*` consumers. ## Blox Verification Author workstation: `buzz-tornquist-issue-2-store-stack` (`2046520`), exact head `8376e19d0da3ec77550590cd91cc3dfe284d95d6`. - `cargo fmt --all --check` — passed - `cargo clippy -p buzz-db -p buzz-relay --all-targets -- -D warnings` — passed - Native PostgreSQL channel lifecycle suite — 3 passed - Native PostgreSQL membership/roster suite — 17 passed; two pre-existing ignored-test fixture failures reproduced identically on the untouched parent `2de5444`: `large_roster_reconciliation_candidates_respect_snapshot_count_and_signer` and `locked_member_snapshot_blocks_post_capture_membership_mutation` both receive the migration-0032 `23514` invalid-`p`-tag rejection. This extraction intentionally does not fold a test-behavior fix into the move. - `cargo test -p buzz-relay --lib -- --test-threads=1` — 908 passed, 48 ignored; the existing load-sensitive mesh demo test returned 504, matching the block#6700/parent baseline - `cargo test -p buzz-relay --lib api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo -- --exact --test-threads=1` — passed Independent exact-head review: `buzz-tornquist-pr-6782-review` (`2048397`) found no remaining critical, important, or minor issues. The full implementation review also independently reproduced both stated PostgreSQL fixture failures on the exact parent and passed the relay library suite (909 passed, 48 ignored). Generated with Codex ## Superseded pre-comment restack verification PR block#6700 merged before publication completed. This layer was restacked onto current main through the exact parent named above; the final cumulative tip is 2ddcc8a. Cumulative author gates passed: formatting and diff checks; buzz-db and buzz-relay all-target clippy with -D warnings; DB lib 111 passed / 200 ignored; ownership 22/22; observability 1/1; the full isolated PostgreSQL domain matrix; and relay lib 910 passed / 49 ignored. - Workstation: `buzz-tornquist-pr-6782-final-review` (`2057620`), fresh shallow checkout - Base: `c60e793eadde79d9eab9f48bbb2ede0ad4831f9b` - Head: `fa09b6c81c4db3b3e1940a2117a97ab2186e49f7` - Findings: none Reviewed both commits in `base..head`. Channel lifecycle/metadata, TTL transitions, and their lock rationale remain in `channel.rs`; membership authorization, roster fencing/snapshots, membership advisory locking, membership records, and focused tests move together to `channel_members.rs`. SQL, transaction, and lock sequences are preserved. Verification: format and diff checks passed; `buzz-db --all-targets` clippy passed with `-D warnings`; DB lib tests passed (111 passed, 200 PostgreSQL tests ignored); ownership (2/2) and observability (1/1) guards passed; native PostgreSQL 17 passed 3 channel lifecycle tests plus 19 membership/roster tests with migrations 1-32 successful; relay lib test target compiled successfully. Final worktree was detached at the exact head and clean. Complete evidence archive SHA-256: `81ae374095f649ca7a25d8b9a4fc864257b7925d1b44657a69ca111523adf36e`. ## Comment-addressed restack Review follow-up on block#6777 removed only the low-value replaceable ownership source test. This PR was restacked onto its rewritten parent; its production patch is unchanged. - Exact base: `21d1b265c133292e6707e766cd4204e6a43f08af` - Exact head: `25138bfd6588e046170dbdbc4ed953bdc3cf7ed1` - Final cumulative tip: `6fa2f104d42c6ba85bdf62e7ccb74ceaf4a84f67` - Per-layer patch-ID and tree audits confirm this PR’s production diff is unchanged from its pre-comment head. - Cumulative Blox gate: formatting and diff checks; strict `buzz-db`/`buzz-relay` Clippy; DB lib 111 passed / 200 ignored; ownership 21/21; observability 1/1; every moved PostgreSQL test; relay lib 910 passed / 49 ignored. - Independent re-review at this exact head: no findings; fresh exact-parent/head Blox review passed fmt/diff, strict Clippy, DB lib 111 passed / 200 ignored, current ownership/observability guards, 3 channel plus 19 membership PostgreSQL tests, and relay compilation. Signed-off-by: OpenAI Codex <codex@openai.com> Co-authored-by: OpenAI Codex <codex@openai.com>
…e transport, version subagent cache - subagent.rs/kind.rs: 20003 documented as RESERVED (observer frames are the transport; frame agent tag = parent identity) — review #1/#2 - observerRelayStore: subagentsVersion bump on every real fold + reset, cache keyed on version not length — review #3 (stale-tree UI bug) - SPEC wire contract section updated to as-built Signed-off-by: Michael Feth <mfethe1@gmail.com>
…e transport, version subagent cache - subagent.rs/kind.rs: 20003 documented as RESERVED (observer frames are the transport; frame agent tag = parent identity) — review #1/#2 - observerRelayStore: subagentsVersion bump on every real fold + reset, cache keyed on version not length — review #3 (stale-tree UI bug) - SPEC wire contract section updated to as-built Signed-off-by: Michael Feth <mfethe1@gmail.com>
CI-only PR to exercise the mobile gate for block#5657, whose workflow run is blocked on maintainer approval. Not for merge.