Make subscript(id:) O(1) on Catalog, add it to Lens and GroupedLens - #43
Merged
Merged
Conversation
Prompt: Take a look at issue #42 for me Response: Let me check the `Resource` protocol and benchmark setup to see what the existing patterns look like.I've read the issue and the relevant code. Before drafting a plan, I want to clarify scope on a couple of points the issue itself flags as open.I'll run /grill-me to stress-test the plan before writing it, as the workflow requires.Good question — let me verify by grepping rather than assume.Confirmed — `subscript(id:)` only exists on `Catalog` today. `Lens` and `GroupedLens` are the only other types holding a sequence of `Resource` values (`Selection`/`Job`/`Setting`/`Credential` are all single-value). So the candidate set is exactly those three. Continuing the grill:One thing I want to verify before locking in `didSet`: does `@Observable` cleanly support `didSet` on its instrumented properties, or do we hit a macro/setter quirk? Let me check.`Setting.value` already uses `@Observable` + `didSet` (Setting.swift:21-29), so the pattern is proven in this codebase. Locking in `didSet`. I have all the decisions I need. Summarizing: - **Scope:** Catalog (existing subscript), Lens (new subscript), GroupedLens (new subscript). Confirmed those are the only sequence-of-Resource holders. - **Duplicate ids:** `Dictionary(_:uniquingKeysWith: { first, _ in first })` to preserve current "first wins" silent behavior. - **Observation:** dict is `private @ObservationIgnored`, subscript does `_ = items` first so observers register on `items` (the source of truth). - **Rebuild site:** `didSet` on `items` — single rule, can't be forgotten across the 3 assignment sites in Catalog or the 1 in each Lens. - **Lens.subscript[id:] semantics:** lookup in the lens's filtered `items` (an item filtered out returns nil). The only sensible reading. Now writing the plan.Plan approved. Before coding, I need to flag one assumption I should verify: Swift's `didSet` does **not** fire for assignments inside `init` itself. For Catalog's `init(initialItems:)`, that means `self.items = initialItems` would not seed `itemsByID`. For Lens/GroupedLens, `init` calls `refresh()` which assigns `items` from inside a method call — semantics are murkier there. I'll handle this defensively by calling `rebuildItemsByID()` explicitly at the end of each init regardless of whether `didSet` would fire. Idempotent and safe. Loading TodoWrite to track the implementation, then writing tests first per the project's TDD rule.Let me first read the existing test files completely so I match their conventions before adding tests.Now writing the new failing Catalog tests.Run those new Catalog tests now to confirm three of the four fail (the seeded one will already pass since `items.first` works there).The 4 new Catalog tests pass against the current O(n) implementation — that's expected since the refactor preserves behavior. They're regression guards (especially the duplicate-ID and seeded cases). The Lens/GroupedLens tests will be the ones that actually fail-then-pass, since they exercise new API that doesn't exist yet. The SourceKit diagnostic is unrelated (IDE metadata, not a build error — `swift build` succeeded).Let me verify the Lens tests actually fail to compile (since `subscript(id:)` doesn't exist on `Lens` yet).Confirmed: the Lens tests fail to compile because `subscript(id:)` doesn't exist yet. Now adding GroupedLens tests.Now implementing. Catalog first.Now running tests to confirm Catalog still works.Test-bundle compile fails because Lens/GroupedLens don't have the new subscript yet. Implementing those next.Now GroupedLens.Running the test suite now to verify implementation. The key concern: did `didSet` fire when `refresh()` is called from Lens/GroupedLens init?All 145 tests pass. `didSet` fires correctly when `refresh()` is called from init. Now updating CHANGELOG, then running `script/test` for the coverage gate.Now running `script/test` for the full suite + coverage gate, and `script/lint` in parallel.I need to see the coverage gate result, which runs after tests. Let me check.100% coverage on all Splint sources. Lint also passed (empty output = clean). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
1 task
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
2 participants
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.
Summary
Resolves #42.
Catalog.subscript(id:)was an O(n) linear scan overitems. In selection-driven SwiftUI code a single body evaluation can resolve "which item is selected right now" several times across child views (sidebar, content, detail) on every keystroke, every.onChange, every selection transition that observers care about — the cost of the scan amplifies under high-churn observation.Each of the three sequence-of-
Resourcetypes now keeps a private@ObservationIgnoredDictionary<Item.ID, Item>rebuilt viadidSetonitems. Subscripts read_ = itemsfirst so observers register on the public source of truth, then look up via the dict — O(1).Catalog.subscript(id:)is now O(1) (was O(n)).Lens.subscript(id:)andGroupedLens.subscript(id:)returning items from the lens's filtered projection (an item filtered out returns nil even if present in the source).items.first { \$0.id == id }semantics.Notes for reviewers
didSetdoes not fire for assignments inside an init body, soCatalog.init(initialItems:)callsrebuildItemsByID()explicitly after seeding. Lens/GroupedLens inits callrefresh()(a method call), wheredidSetdoes fire — verified by the newsubscriptReturnsMatchingItemtests, which exercise the subscript immediately after construction with no async wait.@Observable+didSetis an existing pattern in this codebase (Setting.swift:21-29).@ObservationIgnoredso the cache is invisible to the observation system;itemsremains the only observed signal.Test plan
swift test— all 145 tests pass.script/test— full suite + 100% line coverage gate on `Sources/Splint/` (Catalog 67/67, Lens 43/43, GroupedLens 92/92).script/lint— clean.🤖 Generated with Claude Code