fix(settings): prevent duplicate repo row id when a repo is both pinned and hidden#73
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Codex review: needs maintainer review before merge. Reviewed June 5, 2026, 5:19 PM ET / 21:19 UTC. Summary Reproducibility: yes. from source inspection: current main appends pinned and hidden manual rows against the same loaded-only key set, so an unloaded repo in both lists produces duplicate row ids. The PR body also includes before/after live app output for the same production path. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this focused fix after normal checks so pinned/hidden settings stay mutually exclusive and stale inconsistent settings render with unique table ids. Do we have a high-confidence way to reproduce the issue? Yes from source inspection: current main appends pinned and hidden manual rows against the same loaded-only key set, so an unloaded repo in both lists produces duplicate row ids. The PR body also includes before/after live app output for the same production path. Is this the best way to solve the issue? Yes, this is a narrow maintainable fix because it repairs both the persisted state mutation and the row/filter consumers that must tolerate stale settings. The added tests cover the main stale-state variants without adding a new user-facing mode. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 030a49f0ec99. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
4badff3 to
865f5bc
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
…ed and hidden A repository that ends up in both the pinned and hidden lists produced two settings-table rows sharing the same Identifiable id, which breaks SwiftUI Table rendering and selection. RepoBrowserRows.make computed loadedKeys only from loaded repositories, then appended a manual row for every pinned name not loaded and, separately, for every hidden name not loaded. The hidden loop never checked against rows the pinned loop had already appended, so a name in both lists (and not loaded) was emitted twice with the same id = normalized(fullName). The root enabler was addPinned, which appended to the pinned list without removing the repo from the hidden list, unlike its mirror hide() which already removes from pinned. So hide(X) then pin(X) left X in both lists. Fixes: - RepoBrowserRows.make now tracks every already-emitted key (loaded plus manual) and skips duplicates, so it can never emit two rows with the same id regardless of how the lists were populated. Pinned is processed first, so a name in both lists wins as pinned, matching the user's explicit intent. - addPinned now removes the repo from hiddenRepositories, mirroring hide(), so pin and hide stay mutually exclusive. Adds tests for both: make() with the same name pinned and hidden yields exactly one unique-id row, and addPinned drops the repo from the hidden list.
e6b4ab6 to
7962ecf
Compare
Prevent the Settings repository table from emitting two rows with the same id when a repo is in both the pinned and hidden lists.
Problem
A repository that ends up in both the pinned and hidden lists produces two settings-table rows that share the same
Identifiableid.RepoSettingsViewrendersTable(filteredRows, selection:)keyed onRepoBrowserRow.id, and duplicate ids are undefined behavior in SwiftUITable(broken rendering and selection).Root cause
Two cooperating issues:
RepoBrowserRows.makecomputesloadedKeysonly from loaded repositories, then appends a manual row for every pinned name not loaded and, separately, for every hidden name not loaded. The hidden loop never checks against rows the pinned loop already appended, so a name present in both lists (and not loaded) is appended twice withid = normalized(fullName).addPinnedappends to the pinned list but does not remove the repo from the hidden list, while its mirrorhide()already removes from pinned. Sohide(X)thenpin(X)leaves X in both lists, which is what triggers the double append.Fix
RepoBrowserRows.makenow tracks every already-emitted key (loaded rows plus any manual rows it appends) in aseenKeysset and skips duplicates, so it can never emit two rows with the same id regardless of how the lists were populated. Pinned is processed first, so a name in both lists wins as pinned, which matches the user's explicit intent when they pin a repo.addPinnednow removes the repo fromhiddenRepositories, mirroringhide(), so pin and hide stay mutually exclusive. It is also idempotent: the early-return for an already-pinned repo no longer skips the hidden cleanup, so re-pinning a repo that is already pinned but still hidden repairs that stale state instead of leaving it inconsistent.Relation to #32
Distinct code path. #32 handled crashes from duplicate loaded repos (
Dictionary(uniqueKeysWithValues:)inAppState+RefreshandNSMenudedup inStatusBarMenuBuilder). This fix is about the SettingsTable's manual pinned/hidden rows and theaddPinned/hideasymmetry, and touches neither of those files.Tests
make()with no loaded rows and the same name in pinned and hidden yields exactly one row, all ids unique (Set(ids).count == ids.count), visibility.pinned.addPinned("owner/X")when X is hidden removes X from hidden and keeps it pinned.addPinned("owner/X")when X is already pinned and still hidden removes X from hidden without adding a duplicate pin (regression for the early-return gap).Proof (real
swift testoutput)The buggy state is reproduced and fixed against the actual
RepoBarsource via the repo's ownRepoBarTeststarget (it builds and links headless).Before (committed code), the new regression test fails on the early-return gap:
After (this branch):
The
make()test directly verifies the invariant SwiftUITableneeds: before the fix it returned 2 rows with the duplicate idowner/x; after, it returns a single["owner/x"]row. Full suite: 562 tests in 98 suites pass; swiftformat--lintand swiftlint--strictclean on the changed files.Runtime proof (running app)
The fix exercised in the actual running app, not just the unit test. A debug
.appbuild (the repo's ownScripts/compile_and_run.sh) was launched, and on launch the productionRepoBrowserRows.makepath thatRepoSettingsViewfeeds into the SettingsTable(RepoSettingsView.swift:205) was run with a repo present in both the pinned and hidden lists.Before (pre-fix
make), the live app emits two rows sharing the same id — the duplicateIdentifiable.idthe SwiftUITablechokes on:After (this branch), the same state collapses to a single, uniquely-identified
.pinnedrow:Captured at runtime from the live app process on macOS.
For a pixel-level screenshot of the rendered Settings table, a maintainer can still invoke Mantis on this PR.
Found and fixed with the help of the Claude CLI.