Skip to content

favorites and image details model tests#155

Merged
chizberg merged 2 commits into
mainfrom
more-tests
Jul 25, 2026
Merged

favorites and image details model tests#155
chizberg merged 2 commits into
mainfrom
more-tests

Conversation

@chizberg

@chizberg chizberg commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Two more model test suites.

FavoritesModelTests — pins that favorites dedup keys off cid alone rather than the whole struct (Model.Image.== compares only cid):

  • re-adding a photo with an existing cid is a no-op: the first entry is kept and the storage effect is not re-invoked;
  • removal finds the stored entry by cid even when every other field of the passed instance differs (e.g. one rehydrated from Storage.Image); an unknown cid touches neither state nor storage.

ImageDetailsModelTests — description-link routing, which hinges on a branchy URL parse (host + /p/ + integer cid):

  • a link to a pastvu photo loads it from the remote and presents a nested details screen; loadingAnotherImage is set synchronously and cleared once the nested model is presented;
  • an external link is handed to the browser;
  • a pastvu link that is not a photo page (e.g. a user profile) also goes to the browser instead of being mistaken for a photo.

Rebased onto fresh origin/main. Locally: xcodebuild test — TEST SUCCEEDED; swiftlint --strict and swiftformat . --lint — clean.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI 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.

Pull request overview

Adds two new test suites to pin important model behaviors: favorites deduplication keyed by Model.Image.cid, and URL routing from the Image Details description links (recursive navigation for pastvu.com/p/<cid> vs. opening everything else in the browser).

Changes:

  • Introduces FavoritesModelTests covering add/remove deduplication semantics by cid (including no-op persistence behavior).
  • Introduces ImageDetailsModelTests covering link routing for PastVu photo links vs. external/non-photo PastVu links.
  • Adds local async polling helper (eventually) in the new Image Details tests to observe async reducer effects.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
RewindTests/FavoritesModelTests.swift New tests that lock in favorites add/remove semantics and persistence no-ops when deduping by cid.
RewindTests/ImageDetailsModelTests.swift New tests that lock in description-link routing behavior for PastVu photo links vs. browser hand-off.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +77 to +80
remote: Remote { [weak self] cid in
self?.requestedCids.append(cid)
return modified(.mock) { $0.cid = cid }
},

@chizberg chizberg Jul 25, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🤖 Written by Claude Code (AI agent), posted from @chizberg's account.

Checked this — it's a false positive; the closure body does run on the main actor.

Remote.impl is declared as (Args) async throws -> Response (Rewind/Utils/Remote.swift:11), without @Sendable. A non-@Sendable closure literal formed inside an isolated context inherits that isolation, and Harness.makeModel() is @MainActor-isolated. So the closure is @MainActor, and await impl(args) from the reducer's Task hops back to the main actor before entering the body.

Two checks:

  1. Runtime. Temporarily put MainActor.assertIsolated() as the first line of the closure body and ran the target in Debug (where assertIsolated is live): TEST SUCCEEDED, no trap. The closure definitely executes — pastvuPhotoLinkOpensNestedDetails asserts requestedCids == [linkedCid].

  2. Compiler. Forced a clean recompile of the test files: zero isolation/Sendable diagnostics for ImageDetailsModelTests.swift. Meanwhile the already-merged MapModelTests.swift, which writes await self.record(params) in the exact same position, gets three no 'async' operations occur within 'await' expression warnings (lines 370, 371, 379) — the compiler considers those awaits redundant precisely because that closure is likewise main-actor-isolated.

One caveat worth noting: this rests on isolation inheritance under Swift 5 language mode (SWIFT_VERSION = 5.0). Under Swift 6, converting a @MainActor closure to a non-isolated function type would be diagnosed — but that would affect MapModelTests and production call sites equally, so it's a project-wide migration question rather than something specific to this PR.

Added a comment at the harness so this isn't re-derived next time (06fb802).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@chizberg
chizberg merged commit 066becc into main Jul 25, 2026
3 checks passed
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.

2 participants