Skip to content

fix(feed): reconcile in-place row updates in the feed - #107

Merged
Adron merged 2 commits into
mainfrom
fix/feedview-inplace-row-updates
Sep 17, 2026
Merged

Adron merged 2 commits into
mainfrom
fix/feedview-inplace-row-updates

Conversation

@Adron

@Adron Adron commented Sep 16, 2026

Copy link
Copy Markdown
Member

Summary

Closes #105. FeedView could not see an in-place row update: it mirrors store.feedMessages into a
private @State private var messages and reconciled on .onChange(of: store.feedMessages.count),
merging only ids it did not already hold. Editing an existing row moves neither the trigger nor the
merge, so the store and the persisted cache were correct while the rendered feed stayed stale until a
full reload.

The reconciliation now responds to content: the trigger is a store-side revision that moves for any
feed mutation, and the merge replaces matched ids in place instead of skipping them. The private
mirror stays — it is not a mirror, it is the view's working copy, and pagination appends pages the
store never sees.

Surprise worth knowing: the cheapest option in the issue — "observe the array itself
(Message is Hashable)" — does not work. Message's conformance is deliberately
identity-based (Models/Message.swift: static func == (lhs, rhs) { lhs.id == rhs.id }, so it can
drive navigationDestination(item:)), which means [Message] compares equal after a row's
content is replaced and .onChange(of: store.feedMessages) would never fire. Making == content-
based would have fixed the trigger, but it changes the semantics of a type used app-wide and needs
Equatable on four nested payload types, so it was rejected in favour of the revision counter.

What's included

  • Services/FeedMerge.swift — FeedMerge.merge(existing:incoming:), pure and testable: matched ids
    are replaced in place (keeping their position, and with it the scroll position), unseen ids are
    prepended newest-first, rows the store does not hold are kept. Returns the merged array plus the
    rows it inserted or replaced, so the caller knows what derived state to re-seed.
  • AppDataStore.feedRevision — @Published, bumped from a didSet on feedMessages, so it moves
    for an in-place row edit as well as an insert/replace. This is the trigger FeedView now observes.
  • AppDataStore.updateFeedMessage(_:) — replaces one row by id (unknown id: no-op) and persists the
    feed cache. The store-side mutation the fix exists to serve, and what the acceptance test drives.
  • FeedView — .onChange(of: store.feedRevision) replaces .onChange(of: store.feedMessages.count);
    the else-branch merge becomes the FeedMerge call, still guarded by !showOnlyMine && tagFilter == nil
    and still returning early when nothing changed.
  • FeedView's edit callback also calls store.updateFeedMessage(updated), so an edited post no
    longer leaves a stale row in the store and its cache (it previously wrote only to the view's copy).
    This is the production call site for the new store method.
  • Tests: new FeedMergeTests (12) and 9 new cases in AppDataStoreTests. New files hand-slotted
    into project.pbxproj (no gem re-sort).

Approach and why

Three options were on the table; this is the middle one, plus the trigger it needs.

  • Render straight from the store — rejected. The working copy is not a mirror: loadMore() appends
    pages the store never holds, and loadMessages() replaces it wholesale for the showOnlyMine /
    tagFilter views. Dropping it would mean moving pagination and filtering into the store.
  • Observe the array — rejected, it does not work (see the Summary).
  • Replace matched ids in place, trigger on a revision — taken. The merge stays exactly where it was
    in the view's lifecycle; the only behavioural difference is that a row the store re-sends now wins
    over the held copy, which is the bug. The revision is O(1) to compare, unlike a deep array diff on
    every body evaluation of the app's busiest screen.

The merge moved out of the view because it is otherwise unreachable from a test — the same reason
feedTruncated is a file-level function in FeedView.swift tested by FeedTruncationTests.

Deliberately not in this PR (noted in #105, left as follow-ups):

  • Collapsing digStates / locallyToggled into the merged row. It does not fall out for free —
    locallyToggled protects an optimistic dig from being overwritten by a later server copy, which is
    a policy the merge does not encode. What did fall out: the dig-state re-seed now also covers
    replaced rows, still skipping locally toggled ids, so a server-side dig correction becomes visible.
  • FeedView.deleteMessage removes the row from the working copy only, so the store and its cache
    keep a deleted post until the next refresh. Same bug class, different direction; it needs a
    removeFeedMessage and its own tests.

No-regression checks

  • Scroll position — replaced rows keep their index; only genuinely new ids are inserted, at the
    head, exactly as before. Covered by test_merge_replacedRow_keepsItsPosition.
  • First load — the !syncedFromStore branch is untouched, as is applyInitialState() and the
    .onChange(of: store.feedLoading) fallback.
  • Pagination — rows absent from the store's page are never dropped
    (test_merge_paginatedRowsNotInIncoming_areKept,
    test_storeRowEdit_keepsPaginatedRowsTheStoreNeverSaw). loadMore() is unchanged.
  • Filter guards — guard !showOnlyMine && tagFilter == nil is unchanged and still first.
  • No extra fetches — the merge path calls nothing; loadMessages() is still reached only from
    the filter/user-id onChanges, refreshable, and the retry button.
  • No duplicate rows — insertion still filters on ids already held
    (test_merge_existingRowWithNewContent_isNotDuplicated).
  • Optimistic dig state — initDigStates still skips locallyToggled ids.

Testing

  • Full suite, simulator E21BC7D1-B6EE-4677-8BE3-322F26A4D3D6, serialized, E2E skipped, private
    DerivedData: 1168 tests, 0 failures (21 new). No new warnings.
  • Mutation-checked that the new tests bite, in both halves of the fix:
    • Merge half — made the loop skip the in-place replacement (the old "ids I already hold win"
      behaviour): 6 failures, exactly the intended ones (4 in FeedMergeTests, 2 in
      AppDataStoreTests). Restored, full suite green.
    • Trigger half — removed the didSet that bumps feedRevision: 3 failures, the two
      "revision moves" cases and the end-to-end store-edit case. Restored, full suite green.

Not verified

  • The rendered result on a device or simulator. These are unit tests over the store and the merge;
    that SwiftUI redraws the row once messages changes is not asserted here, and the user-visible
    proof of the original report (feat(messages): apply refreshed link metadata to posted row #103's link-preview backfill) needs that PR plus a real OpenGraph
    fetch.
  • Scroll-position preservation is argued structurally (stable ids, unchanged positions) and covered
    at the data level, not measured in a running list.
  • That no third-party caller depends on Message's == staying identity-based — checked by grep
    across the app and tests, not by type-checking an alternative.

🤖 Generated with Claude Code

FeedView mirrors store.feedMessages into a private @State copy and
reconciled it on .onChange(of: store.feedMessages.count), merging only
ids it did not already hold. An edit to an existing row moved neither
the trigger nor the merge, so the store and its cache were right while
the rendered feed stayed stale until a full reload.

Observing the array itself does not fix it: Message's Equatable is
identity-based (id only), so [Message] compares equal after a row's
content is replaced. AppDataStore now carries feedRevision, bumped from
a didSet on feedMessages, which moves for any mutation including an
in-place one, and the merge moves into FeedMerge.merge(existing:
incoming:) — a pure function that replaces matched ids in place (keeping
their position, and with it the scroll position), prepends unseen ids,
and keeps rows the store never saw, which is what pagination appends.

The first-load path, the showOnlyMine/tagFilter guards, the feedLoading
path and loadMore() are untouched, and the merge issues no requests.
digStates/locallyToggled stay as they are; the re-seed now covers
replaced rows too, still guarded by locallyToggled so an optimistic dig
survives.

AppDataStore.updateFeedMessage replaces one row by id (unknown id is a
no-op) and persists the feed cache; FeedView's edit callback calls it so
an edited post no longer leaves a stale row behind in the cache.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017bss5MgZa7Jvj2m9zdaUd1
Two conflicts, both from #103 landing on main and touching the same places:

- AppDataStore.swift: this branch adds updateFeedMessage(_:), #103 added
  applyLinkMetadata(_:toMessageId:), and the two bodies met at the shared
  trailing saveFeedCache(). Spliced into two complete methods rather than a
  blind union, which would have fused them into one broken function.
- AppDataStoreTests.swift: both sides added a makeMessage helper — this branch
  with a `content:` parameter, #103 with `linkMetadata:`. Merged into a single
  helper carrying both, so every call site on both sides still compiles.

Worth noting the two features compose the way #105 intended: feedRevision's
didSet now fires for applyLinkMetadata too, so the link-preview backfill is
exactly the in-place update this branch makes the feed observe.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017bss5MgZa7Jvj2m9zdaUd1
@Adron
Adron merged commit 6ef311f into main Sep 17, 2026
1 check passed
@Adron
Adron deleted the fix/feedview-inplace-row-updates branch September 17, 2026 09:16
Adron added a commit that referenced this pull request Sep 17, 2026
Same two conflicts #107 hit, from #103's applyLinkMetadata landing on main:

- AppDataStore.swift: removeFeedMessage and applyLinkMetadata met at the shared
  trailing saveFeedCache(). Spliced into two complete methods, both bodies
  verified intact afterwards rather than assumed from the brace structure.
- AppDataStoreTests.swift: the makeMessage helper now carries content, parentId
  and linkMetadata, so call sites from all three branches compile.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017bss5MgZa7Jvj2m9zdaUd1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: FeedView cannot see in-place row updates — merges only unseen ids, keyed on count

1 participant