Preload Discover Trending list - #109
Open
graeme wants to merge 10 commits into
Open
Conversation
The analytics DTO decoded the ranked wire object into an unordered `[String: [Entry]]` dictionary, discarding Homebrew's ordering; the Discover repository then re-derived a ranking by sorting on install count with a name tie-break. That re-ranking is the client doing the backend's job. Decode the per-entry `number` (the backend's authoritative rank) and reassemble the published order from it. `topPackages` now enriches the counts in rank order and drops its count-sort entirely. While here, make the Decodable type a thin, faithful mirror of the wire (synthesized decode, snake_case CodingKeys, `count` as the raw String). The flatten/validate/rank/parse transformation moves out of `init(from:)` into `rankedPackageCounts()`, so decoding stays dumb and mapping owns the domain logic. This trades away the old lossy int tolerance for `total_*`, which the wire never needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`sortedSection` re-sorted each section by install count then name, which re-derived a ranking the sources already provide: trending now arrives in the backend's install-rank order, and catalogue search arrives in the catalogue's match order (prefix matches first). Re-sorting discarded both. Rename it to `section` and make it filter-only, so rows render in the order they were handed down. This also fixes search results, which were being flattened to plain alphabetical order over the catalogue's ranking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… truth Mirror BrewInstalledPackagesRepository: convert the repository from an `actor` returning a snapshot into an `@Observable @MainActor` type that owns `state: LoadState<[DiscoveryBrewPackage], any Error>`. IO and persistence still live in the DiscoverAnalyticsCache actor; only the in-memory UI state moves here. - Protocol becomes the observable contract (`state` + cache-first `load(forceRefresh:)`), replacing `loadTopPackages`. - `load` is cache-first and coalesced: fresh in-memory data returns instantly, a stale window revalidates while the prior list stays on screen, and a single `loadTask` merges the launch preload with the tab's on-appear load into one fetch. Analytics staleness (the 24h TTL) stays the single source of truth for both the in-memory revalidation and the network refetch. - DiscoverViewModel projects `trending` from the repository's `state` and delegates load/retry; the top-packages limit and window move onto the repository. - Stub/Unimplemented/preview doubles become observable. This lets the trending list be preloaded at launch and survive tab switches (wired up next), while the tab still shows loading mid-fetch and revalidates on return once the cache goes stale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Warm the catalogue and analytics caches, then kick off the Discover repository's load in the same window-creation task, so the trending list is already loaded (or loading) by the time the user opens the tab. The prepares are awaited first so enrichment reads the just-prepared disk caches instead of racing them into a needless network fetch. Opening the tab mid-load still shows the loading state (the on-appear load joins the in-flight preload), and a session outliving the analytics TTL revalidates on return — both handled by the repository's cache-first load. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The live analytics endpoint keys entries by package name alphabetically and each object carries only `formula`/`cask` + a string `count` — there is no `number` (or any) rank field. Decoding it as a required `Entry.number: Int` failed on every real response, so trending errored every time it loaded. Drop `number` from the DTO and reconstruct the ranking from `count` (descending, name tie-break) in `rankedPackageCounts()` — the only ranking signal the API actually provides. The view model still stays filter-only; ranking lives in one place in the mapping layer. Adds DiscoverTrendingIntegrationTests: an end-to-end path from DiscoverViewModel through the real BrewDiscoverPackagesRepository, decoding a payload shaped like the live API (name-keyed, alphabetical, string counts, no rank). It reproduced the failure and now guards the decode/rank seam. Fixtures drop the fictional `number` field. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
BrewAnalyticsJSON.rankedPackageCounts() currently drops empty buckets silently, which can mask malformed payloads and produce incomplete rankings without surfacing an error.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Preloads the Discover tab’s Trending list at app launch by making BrewDiscoverPackagesRepository an app-scoped observable source of truth with a LoadState, and reshapes the Discover flow so the view model renders repository state directly. It also refactors analytics decoding/mapping so Trending ranking is derived from install counts (since the live payload has no rank field), and adds integration coverage for the live-shaped payload.
Changes:
- Converted
DiscoverPackagesRepositoryto an observable,@MainActorprotocol exposingstate+load(forceRefresh:), and updated consumers/stubs accordingly. - Updated
BrewDiscoverPackagesRepositoryto coalesce in-flight loads, use cache-first semantics with TTL revalidation, and expose a single session-scoped Trending list. - Refactored
BrewAnalyticsJSONinto a thin wire mirror with mapping viarankedPackageCounts(), plus added/updated unit + end-to-end integration tests.
File summaries
| File | Description |
|---|---|
| Tests/BrewRepositoriesTests/BrewDiscoverPackagesRepositoryTests.swift | Updates repository tests to assert against observable state and new load semantics/limits. |
| Tests/BrewRepositoriesTests/BrewAPIClientConcurrentIntegrationTests.swift | Updates integration tests to use repository load() and validate combined formula+cask ordering via state. |
| Tests/BrewNetworkingTests/BrewAPIClientURLSessionIntegrationTests.swift | Switches assertions to rankedPackageCounts() to validate ranking/mapping behavior. |
| Tests/BrewFeatureDiscoverTests/DiscoverViewModelTests.swift | Adapts Discover VM tests to repository state projection and removes VM-side reranking assumptions. |
| Tests/BrewFeatureDiscoverTests/DiscoverTrendingIntegrationTests.swift | Adds end-to-end test that locks in live-shaped analytics payload decoding + ranking behavior. |
| Tests/BrewCoreTests/BrewAnalyticsJSONTests.swift | Reworks analytics DTO tests to separate decode-time vs mapping-time failures and verify ranking rules. |
| Sources/BrewRepositoryInterfaces/Protocols/DiscoverPackagesRepository.swift | Redefines the Discover repository interface as observable state + cache-first load(forceRefresh:). |
| Sources/BrewRepositoryInterfaces/Fakes/Stubs.swift | Updates shared stub discover repository to the new observable state contract. |
| Sources/BrewRepositories/BrewDiscoverPackagesRepository.swift | Refactors concrete repository into @Observable @MainActor, adds coalesced loading + TTL revalidation, returns a single ranked list. |
| Sources/BrewFeatureDiscover/Views/DiscoverPackagesView.swift | Updates view sectioning to partition only (no reranking) via new VM helper. |
| Sources/BrewFeatureDiscover/ViewModels/DiscoverViewModel.swift | Projects repository state into view-facing LoadState, removes reranking, and delegates load/refresh to repository. |
| Sources/BrewCore/Models/BrewAnalyticsJSON.swift | Makes analytics DTO a wire mirror and introduces rankedPackageCounts() mapping/ranking. |
| Sources/BrewAppEnvironment/UnimplementedRepositories.swift | Updates unimplemented Discover repository to match the new protocol surface. |
| Package.swift | Adds BrewRepositories/BrewNetworking dependencies to enable end-to-end Trending integration tests. |
| Homebrew/BrewApp.swift | Warms caches then preloads Trending at window creation to make first-tab open instant. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Drop comments that merely restate the code or a self-explanatory name; keep public-API documentation and the non-obvious rationale (endpoint key reuse, no-rank-field, load coalescing, cache-order invariant). Also correct the topPackages note: rows are ranked by count here, not by a backend order. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rankedPackageCounts() dropped any key whose entry array was empty via compactMap(\.first), so a malformed payload could silently produce an incomplete ranking. Iterate key/value pairs and throw emptyPackageEntries(key:) naming the offending key instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Keep only public-API doc comments (plus MARK markers and file headers); remove the inline and internal-declaration comments introduced across the branch's Discover work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The new integration tests should explicitly run on @MainActor (or await actor-isolated reads) and one updated test name encodes an incorrect “backend rank” assumption.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
Tests/BrewFeatureDiscoverTests/DiscoverTrendingIntegrationTests.swift:16
- These tests read
@MainActor-isolatedDiscoverViewModelproperties (e.g.trending,visiblePackages,selectedPackage) withoutawait, which is only safe/legal if the test itself runs on the MainActor. Make the test type (or individual test functions)@MainActorto ensure isolation is explicit and resilient to stricter concurrency checking.
struct DiscoverTrendingIntegrationTests {
Tests/BrewRepositoriesTests/BrewDiscoverPackagesRepositoryTests.swift:40
- Test name says it "honors backend rank", but the production code now reconstructs ranking from install
countand a name tie-break (the backend carries no rank field). Renaming this test avoids encoding incorrect assumptions about the API contract.
@Test @MainActor func `load parses string counts and honors backend rank`() async throws {
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
graeme
marked this pull request as ready for review
August 3, 2026 12:37
`honors backend rank` predated removing the `number` field. With equal counts and no rank, the alpha/beta order comes from the name tie-break, so rename to `breaks equal counts by name` to match what the fixture asserts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MikeMcQuaid
approved these changes
Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR: Preload Discover trending and make its repository observable
Summary
Preloads the Discover tab's "Trending" list at app launch so the screen feels
instant on first visit, and reshapes the Discover data source to match the
observable pattern already used by
BrewInstalledPackagesRepository. Also fixesa decode bug that made trending fail on every load.
Changes
BrewDiscoverPackagesRepositoryfrom an actor into an@Observable @MainActorsource of truth exposing aLoadState<[DiscoveryBrewPackage], any Error>. Loads are cache-first, coalescea single in-flight task, and re-fetch on return to the tab once the 24h TTL
lapses. The protocol now vends
stateplusload(forceRefresh:).BrewAppwarms the catalogue and analytics caches, then preloads the trendinglist at window creation, ordered after its dependencies.
DiscoverViewModelnow reads repositorystatethrough a computed property andno longer re-ranks rows; sectioning is filter-only.
BrewAnalyticsJSONis now a thin wiremirror;
rankedPackageCounts()derives order from install count with a nametie-break.
Why this split
The DTO and ranking change lands first as its own step so the observable refactor
sits on a correct, well-tested data layer.
Bug fix
The live Homebrew analytics API keys entries by name alphabetically and carries no
rank field. An earlier assumption decoded a required
numberfield, so every realresponse failed and trending showed an error every time. Ranking is now
reconstructed from
count, and a kept end-to-end integration test drives the viewmodel through the real repository against a live-shaped payload to lock the shape in.
Testing
scripts/test(624 tests pass)PR checklist
built, tested, and linted, and the API shape was verified against the live
endpoint before the fix landed.