Code Review — findings & fixes (commit 6880e3e)

All issues below have been fixed and pushed to the PR branch. Ranked by severity.

🔴 HIGH — Bug: thread close via scrim/back-nav silently drops root edits

File: thread_view.dart

The code comment said "a back/scrim dismiss should still surface that edit to the caller" but only the explicit × button called Navigator.pop(ThreadResult.edited(_root)). Tapping the scrim or pressing the Android back button returned null, so any root-post edit made inside the thread was lost when the user dismissed without using the close button.

Fix: Wrapped the Dialog in PopScope(canPop: false) and extracted a _popWithResult() helper used by both the close button and the onPopInvokedWithResult callback. All dismissal paths now propagate the result consistently.

🔴 HIGH — Performance: _sorted called O(n) times per scroll event

File: forum_view.dart

_sorted is a getter that allocates a new list and runs an O(n log n) sort every time it is called. The ListView.separated builder called it once for itemCount and once more for each item as it scrolled into view:

itemCount: _sorted.length,      // sort #1
itemBuilder: (ctx, i) {
  final post = _sorted[i];      // sort #2 … #N+1

For a forum with 50 posts, scrolling through all items triggered 51 separate sorts.

Fix: Assign _sorted to a local sorted variable at the top of build(). The closure captures it so every itemBuilder call reuses the same already-sorted list.

🟠 MEDIUM — Bug: _togglePin mutates AccordMessage in place, bypasses _posts

File: forum_view.dart

Every other mutation (_editPost, _deletePost, _openPost) replaces the entry in _posts by spreading a new list. _togglePin instead mutated the object's field directly and left _posts pointing at the same list — inconsistent and fragile if the same object is referenced from a snapshot or the sorted closure.

Fix: Now follows the same [...?_posts] → mutate → _posts = list pattern used everywhere else:

setState(() {
  final list = [...?_posts];
  final i = list.indexWhere((m) => m.id == post.id);
  if (i >= 0) list[i].pinned = !post.pinned;
  _posts = list;
});

🟠 MEDIUM — UX: destructive "Delete" button unstyled in confirmation dialog

File: thread_view.dart (confirmDeletePost)

The delete confirmation used a plain TextButton for the destructive action, indistinguishable from "Cancel". Convention (and the destructive: true flag already used in the context menu) is to render it in the theme's error colour.

Fix: Added style: TextButton.styleFrom(foregroundColor: Theme.of(context).colorScheme.error).

🟡 LOW — Redundant showActions variable always equals _canDelete

File: thread_view.dart (_MessageLineState.build)

// _canDelete = widget.isOwn || widget.canManageMessages
final showActions = widget.isOwn || _canDelete;
// = widget.isOwn || widget.isOwn || widget.canManageMessages = _canDelete

showActions was always identical to _canDelete. Removed the intermediate variable; the if (_canDelete) guard on the actions IconButton now reads directly.

🟡 LOW — _postTitle unexposed prevented unit testing

File: forum_view.dart

The title-fallback chain (title → first body line → "Untitled post") was the explicit fix called out in the PR description, but as a _-private function it could not be exercised from tests.

Fix: Renamed to resolveForumPostTitle (no underscore). All three call sites updated.

🟢 TESTS — Added test/features/messaging/forum_view_logic_test.dart

New test file covering:

• resolveForumPostTitle — 8 cases: title present, whitespace trimming, null/empty/whitespace title, first-body-line fallback, all-blank-lines fallback, non-String title field
• ThreadResult constructors — edited carries the root and deleted=false; deleted has root=null and deleted=true

🤖 Generated with Claude Code

Generated by Claude Code