From 085a84abb294136511d27f91d3f6a0e7b1a7eeb4 Mon Sep 17 00:00:00 2001 From: rzen Date: Sun, 9 Aug 2026 10:17:26 -0400 Subject: [PATCH] =?UTF-8?q?Navigation's=20Move=20Left/Right=20learn=20a=20?= =?UTF-8?q?real=20card=20=E2=80=94=20the=20context=20menu's=20disabled=20r?= =?UTF-8?q?ows=20compute=20a=20per-card=20cross-lane=20destination=20and?= =?UTF-8?q?=20land=20it=20through=20the=20drop's=20own=20rank=20machinery?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CardMoveTarget (BoardCommands.swift), LaneMoveTarget's cousin: validates the clicked card's own current lane against the live lane order rather than the board's live selection, refuses a target that reaches outside that lane (no coherent left for a spread), and answers an index — the clicked card's own position in its lane, clamped — for the adjacent live lane in either direction. Trash is never a candidate (not a Lane); a collapsed lane is a fine landing (a fold hides cards, it doesn't close the lane). CardFaceView's Navigation rows now call moveCardAcrossLane(by:), which hands CardMoveTarget's answer straight to BoardStore.moveCards(_:toLane:at:) — the exact call a released drag makes, so rank-minting, the undo step, the watcher echo and the banner all come free. Targeting is Copy/Cut's own widening (targetIDs): the clicked card, or the live selection when the clicked card is a member of it. Enablement stays render-safe the way isSelected/selectedCount already are: three new CardFaceView parameters (hasLeftNeighbor, hasRightNeighbor, selectionSpansLanes) are hoisted once per lane in LaneView.scrollableCards and handed down as compared parameters, never read from inside a card face's own .disabled. Caught and fixed a real regression here during development: an early cut of the multi-lane-spread check answered true for any lane that simply didn't contain the selected card, which flipped a compared parameter for most of the board on an ordinary single-card select and defeated CardFaceView's equality gate wholesale (BoardRenderPerformanceTests.selectionStillRepaints caught it at 151 of 180 card faces). Tests: CardMoveTargetTests (KeyboardGrammarTests.swift) pins the pure predicate — leftmost/rightmost lane, single lane, index clamping, a widened group anchoring on the clicked member rather than its own extent, the multi-lane-spread refusal, and an integration test feeding the answer straight through moveCards. CardFaceViewEquatableTests gains a case pinning the three new compared parameters. Claude-Session: https://claude.ai/code/session_014PtZdPwqZuqEDLc6wZMtEy --- Kanban/UI/Board/BoardCommands.swift | 115 ++++++++++++++ Kanban/UI/Board/CardFaceView.swift | 193 ++++++++++++++++++----- Kanban/UI/Board/LaneView.swift | 15 +- KanbanTests/KeyboardGrammarTests.swift | 205 +++++++++++++++++++++++++ KanbanTests/ViewEquatableTests.swift | 34 ++++ 5 files changed, 519 insertions(+), 43 deletions(-) diff --git a/Kanban/UI/Board/BoardCommands.swift b/Kanban/UI/Board/BoardCommands.swift index 7f08721..0a7752b 100644 --- a/Kanban/UI/Board/BoardCommands.swift +++ b/Kanban/UI/Board/BoardCommands.swift @@ -362,6 +362,121 @@ struct MoveLaneCommands: View { } } +// MARK: - Card context menu ▸ Navigation (cross-lane card move) + +/// **The pure predicate behind the card context menu's Navigation ▸ Move Left / Move Right** +/// (`CardFaceView`, 2026-08-09 ▸ "Give the card context menu's Navigation rows real card-move +/// behavior", card 06322636) — `LaneMoveTarget`'s cousin, not its reuse: that predicate answers `nil` +/// for a card id unconditionally ("a card id is in no lane order" — its own doc comment), because it +/// validates the **board's live selection** against the *lane* order. This one validates a *card's* +/// own current lane against the same order, which is the whole difference between "move the selected +/// lane one slot" and "move this card into the neighbouring lane". +/// +/// **Targeting is Copy/Cut's own widening** (`CardFaceView.targetIDs`, `.clipboardTarget`): the +/// clicked card alone, widened to the live selection when the clicked card is a member of it. Passed +/// in as `targeting` rather than re-derived here — this type never reads `store.selection`, so the +/// same function serves a render-safe `.disabled` predicate (`CardFaceView`'s hoisted +/// `hasLeftNeighbor`/`hasRightNeighbor`/`selectionSpansLanes`) and the action underneath it without +/// either one re-deriving the other's answer. +/// +/// **Destination is the adjacent *live* lane** — owner ruling: "skip nothing — lanes are lanes; the +/// trash is not a lane and is never a destination; collapsed lanes ARE valid destinations". That is +/// exactly `SelectionGrammar.lanes(in:)`'s own list: every lane in `snapshot.lanes`, board order, +/// unfiltered by fold state, and the trash never in it because it is not a `Lane` at all +/// (`BoardModel.trash`/`trashedLanes` are separate arrays). Reused rather than re-walked, the same +/// list `LaneMoveTarget` steps by `delta`. +/// +/// **Position is index-preserving** — owner ruling: "preserve the moved group's internal order and +/// land at the clicked card's relative visual index in the destination lane, clamped". The *clicked* +/// card's index in its own lane's display order (`Lane.cards`, `moveCards`'s own counting space) is +/// the anchor — not the group's leading or trailing edge — clamped to the destination lane's card +/// count exactly as `moveCards` clamps any index handed to it (`DropSlotMath.applied`'s convention, +/// restated at the call site once more so the clamp never depends on `moveCards` catching an +/// out-of-range value silently). **Internal order** is `moveCards`' own: it walks `snapshot.lanes` in +/// display order and takes members as it finds them (`BoardStore.draggedCards`), so a `targeting` set +/// entirely inside one lane always arrives at `moveCards` already in that lane's own order, whatever +/// order the `Set` iterates in — nothing here has to re-sort it. +/// +/// **A multi-lane spread answers `nil`** — owner ruling: "no coherent 'left' for a spread". `targeting` +/// must be non-empty and entirely inside the clicked card's own lane, or this refuses; there is no +/// widened-but-not-really-this-lane fallback. +enum CardMoveTarget { + + /// The live lane one step from `laneID`, board order, or `nil` at a wall or for an id + /// `snapshot.lanes` no longer holds — the shared half of `hasNeighbor(of:delta:in:)` and + /// `destination(clicked:targeting:snapshot:delta:)`. + private static func adjacentLane(of laneID: ItemID, delta: Int, in snapshot: BoardModel) -> ItemID? { + let lanes = SelectionGrammar.lanes(in: snapshot) + guard let from = lanes.firstIndex(of: laneID) else { return nil } + let to = from + delta + guard lanes.indices.contains(to) else { return nil } + return lanes[to] + } + + /// Whether `laneID` has a live neighbour one step in `delta`'s direction — `hasLeftNeighbor` / + /// `hasRightNeighbor`'s pure half, meant to be called **once per lane** at the arrangement level + /// (`LaneView.scrollableCards`) and handed to every card face in it as a compared parameter, the + /// exact `isSelected`/`selectedCount` pattern: cheap (`O(lane count)`, not `O(board cards)`), and + /// never read from inside a card face's own `.disabled` (`CardFaceView`'s own render-safety note). + static func hasNeighbor(of laneID: ItemID, delta: Int, in snapshot: BoardModel) -> Bool { + adjacentLane(of: laneID, delta: delta, in: snapshot) != nil + } + + /// Whether `selectedIDs` — the live board selection, already narrowed to `.board`'s container by + /// the caller (`LaneView`'s own `selectedIDs` hoist) — has **both** a member inside `laneCardIDs`, + /// this lane's own cards, **and** a member outside it: a genuine spread, not merely "the selection + /// exists and this happens to be some other lane". The other half of the enablement fold, + /// `hasNeighbor`'s twin: also lane-hoisted, also free of any `store.selection`/`store.snapshot` + /// read of its own (the caller already did the one read this needs). + /// + /// **The `inLane` guard is load-bearing, not a shortcut.** A selection with no member in this lane + /// at all must answer `false` here — every card in an untouched lane has no `isSelected` face to + /// gate in the first place, so the *value* has to stay `false` (and therefore unchanged) whenever + /// the live selection moves entirely outside this lane, or every face in every other lane on the + /// board becomes a compared difference on a selection change that has nothing to do with them + /// (`CardFaceView.==`'s gate would stop suppressing rebuilds it exists to suppress — caught by + /// `BoardRenderPerformanceTests.selectionStillRepaints`, which asserts a selection change costs a + /// handful of faces, not the board). + /// + /// Only matters when the clicked card is itself selected — an unselected clicked card's target is + /// always `{card.id}` alone, trivially confined — but is cheap enough to hoist unconditionally + /// rather than gate on `isSelected` a second time. + static func selectionSpansOtherLanes(_ selectedIDs: Set, laneCardIDs: Set) -> Bool { + guard !selectedIDs.isEmpty, selectedIDs.contains(where: { laneCardIDs.contains($0) }) else { + return false + } + return selectedIDs.contains { !laneCardIDs.contains($0) } + } + + /// Where `targeting` would land one lane over, or `nil` when it cannot move at all: `clicked` is + /// not a live board card, `targeting` is empty or reaches outside `clicked`'s own lane (the + /// multi-lane-spread refusal), or that lane has no live neighbour in `delta`'s direction. + /// + /// The index is `clicked`'s own position in its lane's display order, clamped to the destination + /// lane's card count — see the type comment's "Position is index-preserving". The caller hands the + /// answer straight to `BoardStore.moveCards(_:toLane:at:)`, unmodified: rank-minting, the undo + /// step, the watcher echo and the banner all come from that one call, exactly as they do for a + /// released drag. + static func destination( + clicked: ItemID, + targeting: Set, + snapshot: BoardModel, + delta: Int + ) -> (laneID: ItemID, index: Int)? { + guard let sourceLane = snapshot.lanes.first(where: { lane in lane.cards.contains { $0.id == clicked } }), + let clickedIndex = sourceLane.cards.firstIndex(where: { $0.id == clicked }), + !targeting.isEmpty, + targeting.allSatisfy({ id in sourceLane.cards.contains { $0.id == id } }) + else { return nil } + + guard let destinationLaneID = adjacentLane(of: sourceLane.id, delta: delta, in: snapshot) + else { return nil } + let destinationCount = snapshot.lanes.first(where: { $0.id == destinationLaneID })?.cards.count ?? 0 + + return (laneID: destinationLaneID, index: min(clickedIndex, destinationCount)) + } +} + // MARK: - Creation items /// File ▸ New Card (⌘N) and File ▸ New Lane (⇧⌘N) — 11-command-nexus.md's two creation rows. diff --git a/Kanban/UI/Board/CardFaceView.swift b/Kanban/UI/Board/CardFaceView.swift index d0c5724..e6a6fff 100644 --- a/Kanban/UI/Board/CardFaceView.swift +++ b/Kanban/UI/Board/CardFaceView.swift @@ -183,6 +183,63 @@ struct CardFaceView: View, Equatable { /// face, and one is it. let selectedCount: Int + /// Whether this face's own lane has a live neighbour one step to the **left** — Navigation ▸ Move + /// Left's render-safe enablement input (2026-08-09 ▸ "Give the card context menu's Navigation rows + /// real card-move behavior", card 06322636), `isSelected`'s exact pattern one row over: the parent + /// (`LaneView`) reads `store.snapshot` **once per lane** (`CardMoveTarget.hasNeighbor`) and hands + /// every face in it the answer, so a click never costs a per-face board walk. Defaults `false` — + /// the trash side never constructs Navigation rows at all, so its faces never need a real answer + /// here (`CardFaceRole`). + let hasLeftNeighbor: Bool + + /// `hasLeftNeighbor`'s mirror, for Move Right. + let hasRightNeighbor: Bool + + /// Whether the **live board selection** reaches outside this face's own lane — the "no coherent + /// left for a spread" refusal (owner ruling, card 06322636): a selection spanning more than one + /// lane disables Navigation for every one of its own selected members, because the widened target + /// (`targetIDs`) would name cards `CardMoveTarget.destination` can never place in one direction. + /// Only read together with `isSelected` — an unselected face's target is always itself alone, + /// trivially confined to its own lane regardless of what this says (`moveLeftEnabled`). + /// + /// Lane-hoisted like the two neighbour flags above (`CardMoveTarget.selectionSpansOtherLanes`): + /// the parent already reads the selection once for `isSelected`/`selectedCount`, so this costs one + /// more `Set` comparison over cards it already has in hand, never a second `store.selection` read. + let selectionSpansLanes: Bool + + /// Explicit only for `hasLeftNeighbor`/`hasRightNeighbor`/`selectionSpansLanes` — the three + /// Navigation inputs, all defaulted to `false` (every row reads disabled) so the trash side's one + /// call site (`TrashLaneView.cardRow`, whose faces never draw Navigation at all) and the pre-move + /// equatable-gate tests need no opinion about a feature their construction never exercises. Every + /// other parameter stays required — the memberwise init's own shape — because the board side + /// (`LaneView.scrollableCards`) always has a real answer for all eight and a silently-defaulted + /// `card` or `isSelected` would be the wrong kind of convenience. + init( + store: BoardStore, + card: Card, + role: CardFaceRole, + marquee: MarqueeControl, + drops: BoardDropContext, + hero: URL?, + isSelected: Bool, + selectedCount: Int, + hasLeftNeighbor: Bool = false, + hasRightNeighbor: Bool = false, + selectionSpansLanes: Bool = false + ) { + self.store = store + self.card = card + self.role = role + self.marquee = marquee + self.drops = drops + self.hero = hero + self.isSelected = isSelected + self.selectedCount = selectedCount + self.hasLeftNeighbor = hasLeftNeighbor + self.hasRightNeighbor = hasRightNeighbor + self.selectionSpansLanes = selectionSpansLanes + } + /// The app-wide quick-style recents — see `LaneView`'s own note. @Environment(AppModel.self) private var appModel @@ -253,6 +310,14 @@ struct CardFaceView: View, Equatable { /// the deferred `styleTarget` read the selection the same way, inside actions, which is why they /// stayed as they were. /// + /// **The three Navigation inputs joined 2026-08-09** (card 06322636), `isSelected`'s exact + /// reason one more time: `hasLeftNeighbor`/`hasRightNeighbor`/`selectionSpansLanes` are what let + /// `moveLeftEnabled`/`moveRightEnabled` answer without a `store.snapshot`/`store.selection` read + /// inside this face's own `.disabled` — so a gate that swallowed them would leave a face's + /// Navigation rows wearing a stale enabled state after a neighbouring lane folded, a wall lane's + /// board-edge shifted, or the live selection grew across lanes, none of which touch `card`, `hero` + /// or the other selection figures. + /// /// Deliberately NOT compared: `@State` (per-identity, preserved across updates anyway), /// `@Environment` values (SwiftUI invalidates on those itself), and the `.board` role's /// `openCard` closure (see `CardFaceRole.isEquivalent(to:)`). @@ -262,6 +327,9 @@ struct CardFaceView: View, Equatable { && lhs.hero == rhs.hero && lhs.isSelected == rhs.isSelected && lhs.selectedCount == rhs.selectedCount + && lhs.hasLeftNeighbor == rhs.hasLeftNeighbor + && lhs.hasRightNeighbor == rhs.hasRightNeighbor + && lhs.selectionSpansLanes == rhs.selectionSpansLanes && lhs.store === rhs.store && lhs.marquee.isEquivalent(to: rhs.marquee) && lhs.drops.isEquivalent(to: rhs.drops) @@ -688,10 +756,14 @@ struct CardFaceView: View, Equatable { /// 3. Navigation ▸ (Move Left, Move Right) /// 4. Send to Trash /// - /// Every row below routes through the *existing* command it twins — nothing here is a new - /// capability, only a new arrangement of ones the app already has (`OpenCardCommand`, - /// `BoardRenameCommand`, `StyleCommand`/`StyleEditorSession`, `ClipboardStore`, - /// `LaneMoveTarget`/`MoveLaneCommands`, `store.delete`/`TrashCommands`). + /// Every row below routes through an *existing* write primitive it twins — nothing here invents a + /// new way to touch disk, only new arrangements and, for one group, a new targeting rule over ones + /// the app already has (`OpenCardCommand`, `BoardRenameCommand`, `StyleCommand`/`StyleEditorSession`, + /// `ClipboardStore`, `store.delete`/`TrashCommands`). **Navigation is the one exception to "new + /// arrangement, not new capability"**: this group shipped structurally in fe66c461 (wired but + /// unconditionally disabled — no per-card cross-lane move predicate existed yet) and gained the + /// real one in 06322636, `CardMoveTarget`, over the same `BoardStore.moveCards(_:toLane:at:)` a + /// released drag already calls — see that section below for the full story. /// /// ### Two deviations from the card's literal list, both kept and both journaled /// @@ -724,29 +796,39 @@ struct CardFaceView: View, Equatable { /// which only asked for the *card* menu). Flagged for owner review: easy to bring back as a third /// row under Style if the drop was not intended. /// - /// ### Navigation ▸ Move Left / Move Right — wired, but unconditionally disabled (flagged) + /// ### Navigation ▸ Move Left / Move Right — a real per-card cross-lane move (2026-08-09, card + /// 06322636) /// - /// `LaneMoveTarget.destination` — Move Left/Right's one predicate, shared with the menu-bar row — - /// reads the **board's live selection**, not the clicked card, and answers `nil` for any card id - /// unconditionally ("a card id is in no lane order" — see its own doc comment). A card's own - /// context menu can therefore never itself be a case where this predicate answers `true` for - /// *this* card; the only way the row could ever be live is if some unrelated lane happened to be - /// the board's live selection at the same time — a coincidence with nothing to do with the card - /// that was right-clicked, and confusing to expose ("Move Left" on a card silently moving some - /// other lane). + /// **Not `LaneMoveTarget.destination`** — that predicate reads the **board's live selection** and + /// answers `nil` for any card id unconditionally ("a card id is in no lane order" — its own doc + /// comment), because it is Board ▸ Move Left/Right's *lane*-selection predicate, shared with the + /// menu-bar row. These two rows validate against `CardMoveTarget.destination` instead — the + /// cousin built for exactly this card, over this face's own `targetIDs` (Copy/Cut's widening: + /// the clicked card, or the live selection when the clicked card is a member of it) rather than + /// the live selection's lane membership. /// - /// Reading the live selection to chase that coincidence would also cost real render performance: - /// unlike every other row here, `LaneMoveTarget.destination` needs `store.selection` and - /// `store.snapshot`, and `.contextMenu`'s builder is **not lazy** — SwiftUI evaluates `boardMenu` - /// (and every `.disabled` inside it) on every ordinary body pass, the exact regression - /// `isSelected`/`selectedCount` exist to prevent (this struct's own top-of-file note; - /// `BoardRenderPerformanceTests.selectionStillRepaints`). So the two rows are wired to the real - /// store call (`moveLane`, via `LaneMoveTarget.destination`) for when this is revisited, but - /// `.disabled(true)` unconditionally rather than paying an O(board) selection read for a row that - /// is a structural mismatch for a card menu in the first place. Flagged for owner review: a - /// genuine per-card "move this card's lane" affordance would be new targeting behavior — out of this - /// card's "menu structure, not new behavior" scope — and the owner's card body may simply not have - /// anticipated the sole-lane restriction Move Left/Right already carries (11-command-nexus.md). + /// **Destination is the adjacent live lane**, skipping nothing (owner ruling): the trash is never + /// a candidate (it is not a `Lane`) and a collapsed lane is a perfectly good landing — a fold + /// hides cards, it does not close the lane. **Position is index-preserving** — the clicked card's + /// own index in its lane's display order, carried over to the destination and clamped to its card + /// count, with the rest of a widened group riding along in their existing relative order + /// (`CardMoveTarget`'s own type comment has the full reasoning). The write is the ordinary + /// `BoardStore.moveCards(_:toLane:at:)` — the exact call a released drag makes — so rank-minting, + /// the undo step, the watcher echo and the banner all come free from that one path. + /// + /// **Enablement stays render-safe the same way every other row here does.** `CardMoveTarget` + /// needs `store.snapshot` (for the neighbouring lane) and, when the clicked card is itself + /// selected, `store.selection` (for the multi-lane-spread refusal) — both of which are forbidden + /// inside this face's own `.disabled` (this struct's top-of-file note; `.contextMenu`'s builder is + /// not lazy). So neither is read here: `hasLeftNeighbor`/`hasRightNeighbor` and + /// `selectionSpansLanes` are lane-hoisted compared parameters, `isSelected`'s exact pattern — + /// `LaneView.scrollableCards` reads `store.snapshot`/`store.selection` **once per lane** and hands + /// every face in it the answer, so a click costs `O(lane count)`, never `O(board cards)` + /// (`moveLeftEnabled`/`moveRightEnabled`; `BoardRenderPerformanceTests` stays green throughout). + /// + /// **A selection spanning more than one lane disables both rows** (owner ruling: "no coherent + /// left for a spread") — `selectionSpansLanes`, consulted only when `isSelected` is true, since an + /// unselected clicked card's own target is always itself alone. /// /// ### Copy / Cut / Paste / Paste Special — targeting /// @@ -799,6 +881,14 @@ struct CardFaceView: View, Equatable { /// kind-checks are always satisfied and the rows reduce to `isEditingInline`/`acceptsBoardMutations` /// (`copyEnabled`/`store.acceptsBoardMutations`) without reading the target at all for `.disabled` /// — the target is still read, correctly, inside each action (`clipboardTarget`). + /// + /// **Navigation ▸ Move Left / Move Right take the third route**: unlike Paste and Copy/Cut, there + /// is no algebraic reduction that makes `CardMoveTarget.destination` free of `store.snapshot`/ + /// `store.selection` — a neighbouring lane and a cross-lane selection spread are both genuinely + /// board-wide facts. So the parent resolves them instead, **once per lane**, and hands this face + /// the answer as three more compared parameters (`hasLeftNeighbor`, `hasRightNeighbor`, + /// `selectionSpansLanes`) — `isSelected`/`selectedCount`'s own precedent, not a new one + /// (`moveLeftEnabled`/`moveRightEnabled`). @ViewBuilder private func boardMenu(openCard: @escaping (ItemID) -> Void) -> some View { // Group 1 — Open, Copy Link, Rename, Style ▸ (Symbol, Color). One run, no divider inside it: @@ -840,17 +930,17 @@ struct CardFaceView: View, Equatable { Divider() - // Group 3 — Navigation ▸ Move Left / Move Right. Wired, unconditionally disabled — see the - // type comment's own section on why. + // Group 3 — Navigation ▸ Move Left / Move Right, a real per-card cross-lane move — see the + // type comment's own section for the destination/position rules and the render-safety proof. Menu("Navigation") { - Button("Move Left") { moveLane(by: -1) } - .disabled(true) - Button("Move Right") { moveLane(by: 1) } - .disabled(true) + Button("Move Left") { moveCardAcrossLane(by: -1) } + .disabled(!moveLeftEnabled) + Button("Move Right") { moveCardAcrossLane(by: 1) } + .disabled(!moveRightEnabled) } - // Both rows are unconditionally disabled (see above), so the submenu itself greys out too - // rather than opening onto two dead rows. - .disabled(true) + // The submenu greys out only when *both* rows would — a card in the leftmost lane can still + // move right, so the submenu itself has to stay open to it. + .disabled(!moveLeftEnabled && !moveRightEnabled) Divider() @@ -1043,16 +1133,35 @@ struct CardFaceView: View, Equatable { // MARK: - Navigation (Move Left / Move Right) - /// Navigation ▸ Move Left/Right's action — `MoveLaneCommands.move(by:)`'s own shape, over - /// `LaneMoveTarget.destination`, the exact predicate the menu-bar row validates against. The rows - /// calling this are `.disabled(true)` unconditionally (see the type comment's own section on why), - /// so in practice this never fires from the UI; it stays real rather than a stub so a future pass - /// that relaxes the disable has the correct call already in place. - private func moveLane(by delta: Int) { + /// Navigation ▸ Move Left's render-safe enablement — `hasLeftNeighbor` (this face's own lane + /// against the live board order, lane-hoisted the way `isSelected` is), `acceptsBoardMutations` + /// (a narrow, rarely-changing `BoardStore` flag — every other row here reads it directly, the same + /// proof applies), and the multi-lane-spread refusal, consulted only when this card is itself + /// selected (an unselected clicked card's target is always itself alone, trivially confined to + /// this lane whatever `selectionSpansLanes` says — see the type comment's own section). + private var moveLeftEnabled: Bool { + store.acceptsBoardMutations && hasLeftNeighbor && !(isSelected && selectionSpansLanes) + } + + /// `moveLeftEnabled`'s mirror, for Move Right. + private var moveRightEnabled: Bool { + store.acceptsBoardMutations && hasRightNeighbor && !(isSelected && selectionSpansLanes) + } + + /// Navigation ▸ Move Left/Right's action — `CardMoveTarget.destination`, the per-card cousin of + /// `LaneMoveTarget.destination` (`MoveLaneCommands.move(by:)`'s own shape one type over), over + /// this face's own widened target (`targetIDs`, Copy/Cut's precedent). `destination` re-derives + /// exactly what `moveLeftEnabled`/`moveRightEnabled` already checked — a card menu names its + /// target by where it was invoked, never by a lingering `.disabled` read, `copyLink()`'s reason — + /// so a stale enablement can only ever make this a no-op, never a wrong move. The write is the + /// ordinary `moveCards`: rank-minting, the undo step, the watcher echo and the banner all come + /// free from that one call, exactly as a released drag's do. + private func moveCardAcrossLane(by delta: Int) { guard store.acceptsBoardMutations, - let target = LaneMoveTarget.destination(selection: store.selection, snapshot: store.snapshot, delta: delta) + let target = CardMoveTarget.destination( + clicked: card.id, targeting: targetIDs, snapshot: store.snapshot, delta: delta) else { return } - store.moveLane(target.lane, toIndex: target.index) + store.moveCards(targetIDs, toLane: target.laneID, at: target.index) } // MARK: - Title row diff --git a/Kanban/UI/Board/LaneView.swift b/Kanban/UI/Board/LaneView.swift index 5e468ba..fd450a0 100644 --- a/Kanban/UI/Board/LaneView.swift +++ b/Kanban/UI/Board/LaneView.swift @@ -1051,6 +1051,16 @@ struct LaneView: View, Equatable { // per card. A face cannot resolve its own path — it knows its card and its container, not // where the card sits — and finding it from the snapshot would be a board walk per face. let cardsFolder = store.rootURL.appendingPathComponent(lane.id.rawValue, isDirectory: true) + // **The card menu's Navigation inputs, hoisted the same way** (2026-08-09, card 06322636): + // one `store.snapshot` read for the lane's own neighbours (`CardMoveTarget.hasNeighbor`) and + // one `Set` comparison over the selection already read above for the multi-lane-spread + // refusal (`CardMoveTarget.selectionSpansOtherLanes`) — never a per-face read, `selectedIDs`' + // own reason. `store.snapshot` costs this body nothing new: `headerInk` already subscribes + // the whole lane to it. + let hasLeftNeighbor = CardMoveTarget.hasNeighbor(of: lane.id, delta: -1, in: store.snapshot) + let hasRightNeighbor = CardMoveTarget.hasNeighbor(of: lane.id, delta: 1, in: store.snapshot) + let selectionSpansLanes = CardMoveTarget.selectionSpansOtherLanes( + selectedIDs, laneCardIDs: Set(lane.cards.map(\.id))) return ScrollView(.vertical) { // Cards stay standard width whatever the lane spans: at a slot width of // `units × standard + (units - 1) × gap`, `MasonryLayout` divides back into exactly @@ -1071,7 +1081,10 @@ struct LaneView: View, Equatable { // 1 for an unselected face: the replica's fan and count badge want // "how many ride along", and a card outside the selection drags // alone (`CardFaceView.draggedIDs`). - selectedCount: selectedIDs.contains(card.id) ? selectedCount : 1 + selectedCount: selectedIDs.contains(card.id) ? selectedCount : 1, + hasLeftNeighbor: hasLeftNeighbor, + hasRightNeighbor: hasRightNeighbor, + selectionSpansLanes: selectionSpansLanes ) // **The value gate** (`CardFaceView.==`) — the lane's own, one level // down: this body re-runs on every proposal change while a drag is over diff --git a/KanbanTests/KeyboardGrammarTests.swift b/KanbanTests/KeyboardGrammarTests.swift index 2dfd695..96ef011 100644 --- a/KanbanTests/KeyboardGrammarTests.swift +++ b/KanbanTests/KeyboardGrammarTests.swift @@ -1109,3 +1109,208 @@ struct LaneMoveTargetTests { #expect(LaneMoveTarget.destination(selection: trashSelection, snapshot: store.snapshot, delta: 1) == nil) } } + +// MARK: - CardMoveTarget's destination predicate + +/// `CardMoveTarget.destination` — the pure predicate behind the card context menu's Navigation ▸ +/// Move Left/Move Right (2026-08-09 ▸ "Give the card context menu's Navigation rows real card-move +/// behavior", card 06322636), `LaneMoveTarget.destination`'s cousin: same board-order lane stepping, +/// but validated against a **card's own current lane** rather than the live selection's lane +/// membership. `hasNeighbor` and `selectionSpansOtherLanes` are the two halves `CardFaceView` hoists +/// to the arrangement level (`LaneView.scrollableCards`) as `hasLeftNeighbor`/`hasRightNeighbor`/ +/// `selectionSpansLanes` — pinned here directly, `LaneMoveTargetTests`' own reason. +/// +/// Three cards in lane one, one in lane two, one in lane three — enough room for an index that has to +/// clamp landing in a lane, a card in every lane an edge case needs one in, and a third lane to prove +/// a target spanning two of them is refused regardless of which two. +@MainActor +private func makeCardMoveBoard() throws -> WriterFixture { + let fixture = try WriterFixture() + try fixture.item("", Item.board) + try fixture.item(Ident.lane1, Item.rich(order: "1024", title: "Todo")) + try fixture.item("\(Ident.lane1)/\(Ident.card1)", Item.rich(order: "1024", title: "First")) + try fixture.item("\(Ident.lane1)/\(Ident.card2)", Item.rich(order: "2048", title: "Second")) + try fixture.item("\(Ident.lane1)/\(Ident.card3)", Item.rich(order: "3072", title: "Third")) + try fixture.item(Ident.lane2, Item.rich(order: "2048", title: "Doing")) + try fixture.item("\(Ident.lane2)/\(Ident.card4)", Item.rich(order: "1024", title: "Fourth")) + try fixture.item(Ident.lane3, Item.rich(order: "3072", title: "Done")) + try fixture.item("\(Ident.lane3)/\(More.card5)", Item.rich(order: "1024", title: "Fifth")) + return fixture +} + +@MainActor +@Suite("CardMoveTarget ▸ destination") +struct CardMoveTargetTests { + + @Test("The leftmost lane's own card refuses a further-left move") + func leftmostLaneRefusesLeft() throws { + let fixture = try makeCardMoveBoard() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + + #expect(CardMoveTarget.destination( + clicked: card1, targeting: [card1], snapshot: store.snapshot, delta: -1) == nil) + #expect(CardMoveTarget.hasNeighbor(of: lane1, delta: -1, in: store.snapshot) == false) + } + + @Test("The rightmost lane's own card refuses a further-right move") + func rightmostLaneRefusesRight() throws { + let fixture = try makeCardMoveBoard() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + + #expect(CardMoveTarget.destination( + clicked: card5, targeting: [card5], snapshot: store.snapshot, delta: 1) == nil) + #expect(CardMoveTarget.hasNeighbor(of: lane3, delta: 1, in: store.snapshot) == false) + } + + @Test("A mid-board card's own card answers both directions, landing at its own index") + func midBoardCardAnswersBothWays() throws { + let fixture = try makeCardMoveBoard() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + + // Fourth is lane two's sole card — index 0 — and lane two sits between lane one (three + // cards) and lane three (one card), so both directions have somewhere to land. + let right = CardMoveTarget.destination( + clicked: card4, targeting: [card4], snapshot: store.snapshot, delta: 1) + #expect(right?.laneID == lane3) + #expect(right?.index == 0) + + let left = CardMoveTarget.destination( + clicked: card4, targeting: [card4], snapshot: store.snapshot, delta: -1) + #expect(left?.laneID == lane1) + #expect(left?.index == 0) + } + + @Test("An index past the destination lane's card count clamps rather than trapping") + func indexClamps() throws { + let fixture = try makeCardMoveBoard() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + + // Third is index 2 in lane one; lane two holds exactly one card, so the only valid insertion + // indices there are 0 and 1 — 2 must clamp down to 1, not trap or land past the end silently. + let target = CardMoveTarget.destination( + clicked: card3, targeting: [card3], snapshot: store.snapshot, delta: 1) + #expect(target?.laneID == lane2) + #expect(target?.index == 1) + } + + @Test("A widened group lands at the CLICKED member's index, not the group's own extent") + func groupAnchorsOnTheClickedMember() throws { + let fixture = try makeCardMoveBoard() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + let group: Set = [card1, card3] + + // Clicked First (index 0) — the group's own leading edge. + let clickedFirst = CardMoveTarget.destination( + clicked: card1, targeting: group, snapshot: store.snapshot, delta: 1) + #expect(clickedFirst?.laneID == lane2) + #expect(clickedFirst?.index == 0, "anchored on the clicked card's own index, not the group's") + + // Clicked Third (index 2) — the same group, the other member clicked, clamped the same way + // `indexClamps` pins for a single-card target. + let clickedThird = CardMoveTarget.destination( + clicked: card3, targeting: group, snapshot: store.snapshot, delta: 1) + #expect(clickedThird?.laneID == lane2) + #expect(clickedThird?.index == 1) + } + + @Test("A target reaching outside the clicked card's own lane answers nil — no coherent left for a spread") + func multiLaneTargetAnswersNil() throws { + let fixture = try makeCardMoveBoard() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + let spread: Set = [card1, card5] + + #expect(CardMoveTarget.destination( + clicked: card1, targeting: spread, snapshot: store.snapshot, delta: 1) == nil) + #expect(CardMoveTarget.selectionSpansOtherLanes(spread, laneCardIDs: [card1, card2, card3])) + } + + @Test("An empty target, and a clicked id the board no longer holds, both answer nil") + func emptyOrUnknownClickedAnswersNil() throws { + let fixture = try makeCardMoveBoard() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + + #expect(CardMoveTarget.destination( + clicked: card1, targeting: [], snapshot: store.snapshot, delta: 1) == nil) + #expect(CardMoveTarget.destination( + clicked: ItemID(rawValue: Ident.indexless), targeting: [ItemID(rawValue: Ident.indexless)], + snapshot: store.snapshot, delta: 1) == nil) + } + + @Test("A single-lane board has no neighbour in either direction") + func singleLaneHasNoNeighbor() throws { + let fixture = try WriterFixture() + try fixture.item("", Item.board) + try fixture.item(Ident.lane1, Item.rich(order: "1024", title: "Solo")) + try fixture.item("\(Ident.lane1)/\(Ident.card1)", Item.rich(order: "1024", title: "Only")) + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + + #expect(CardMoveTarget.hasNeighbor(of: lane1, delta: -1, in: store.snapshot) == false) + #expect(CardMoveTarget.hasNeighbor(of: lane1, delta: 1, in: store.snapshot) == false) + #expect(CardMoveTarget.destination( + clicked: card1, targeting: [card1], snapshot: store.snapshot, delta: -1) == nil) + #expect(CardMoveTarget.destination( + clicked: card1, targeting: [card1], snapshot: store.snapshot, delta: 1) == nil) + } + + @Test("hasNeighbor answers false for a lane id the snapshot no longer holds") + func hasNeighborAnswersFalseForAnUnknownLane() throws { + let fixture = try makeCardMoveBoard() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + let unknown = ItemID(rawValue: Ident.indexless) + + #expect(CardMoveTarget.hasNeighbor(of: unknown, delta: -1, in: store.snapshot) == false) + #expect(CardMoveTarget.hasNeighbor(of: unknown, delta: 1, in: store.snapshot) == false) + } + + @Test("selectionSpansOtherLanes: confined selections and the empty selection both answer false") + func selectionSpansOtherLanesFalseCases() throws { + #expect(CardMoveTarget.selectionSpansOtherLanes([], laneCardIDs: [card1, card2]) == false) + #expect(CardMoveTarget.selectionSpansOtherLanes([card1], laneCardIDs: [card1, card2]) == false) + #expect(CardMoveTarget.selectionSpansOtherLanes([card1, card2], laneCardIDs: [card1, card2]) == false) + } + + /// **The regression this predicate exists not to reintroduce**: a selection with no member in + /// this lane at all — the ordinary case for every lane the user did not click into — must answer + /// `false`, the same as an empty selection, or every untouched lane's faces would read a spread + /// whenever *anything* elsewhere on the board got selected (caught the hard way, by + /// `BoardRenderPerformanceTests.selectionStillRepaints` going from 8 faces to 151 on a single + /// click — see this function's own doc comment). + @Test("A selection with no member in this lane at all answers false, not true") + func selectionEntirelyOutsideTheLaneAnswersFalse() throws { + #expect(CardMoveTarget.selectionSpansOtherLanes([card4], laneCardIDs: [card1, card2, card3]) == false) + } + + /// **Feeding `destination`'s own answer to `BoardStore.moveCards` lands the card exactly there** — + /// the reuse the owner ruling asks for ("rank minting through the existing gapped-rank machinery + /// … reuse, don't reinvent") closes the loop: the predicate's `(laneID, index)` is not merely + /// shaped like what `moveCards` wants, it actually produces the destination this test reads back + /// off disk. + @Test("The predicate's answer, handed to moveCards, actually lands the card there") + func destinationFeedsMoveCardsCorrectly() throws { + let fixture = try makeCardMoveBoard() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + + let target = try #require(CardMoveTarget.destination( + clicked: card2, targeting: [card2], snapshot: store.snapshot, delta: 1)) + store.moveCards([card2], toLane: target.laneID, at: target.index) + + let moved = try BoardLoader.load(boardRoot: fixture.root).model + let sourceLane = try #require(moved.lanes.first { $0.id == lane1 }) + let destinationLane = try #require(moved.lanes.first { $0.id == lane2 }) + #expect(sourceLane.cards.map(\.id) == [card1, card3], "Second really left lane one") + // Lane two held one card (Fourth) before the move; index 1 among it is "after Fourth", + // Second's own index (1) carried over from a three-card lane one, clamped by `moveCards`. + #expect(destinationLane.cards.map(\.id) == [card4, card2], "and landed after Fourth") + #expect(store.banners.oneShots.isEmpty) + } +} diff --git a/KanbanTests/ViewEquatableTests.swift b/KanbanTests/ViewEquatableTests.swift index 65aa37e..57d4ae9 100644 --- a/KanbanTests/ViewEquatableTests.swift +++ b/KanbanTests/ViewEquatableTests.swift @@ -313,6 +313,40 @@ struct CardFaceViewEquatableTests { )) } + /// **The Navigation inputs are compared too** (2026-08-09 ▸ "Give the card context menu's + /// Navigation rows real card-move behavior", card 06322636) — `selectednessIsADifference`'s own + /// reason: `hasLeftNeighbor`/`hasRightNeighbor`/`selectionSpansLanes` are what let + /// `moveLeftEnabled`/`moveRightEnabled` answer without a `store` read inside this face's own + /// `.disabled`, so a gate that swallowed any of the three would leave a face's Navigation rows + /// wearing a stale enabled state after a purely lane-side change nothing else here would compare. + @Test("Each Navigation input is a compared difference on its own") + func navigationInputsAreADifference() throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + let card = try firstCard(fixture.snapshot()) + let marquee = MarqueeControl( + session: MarqueeSession(), registry: MarqueeTargetRegistry(), store: store + ) + let drops = makeDrops(store: store, session: DragSession(), registry: LaneDropRegistry()) + + func face( + hasLeftNeighbor: Bool = false, hasRightNeighbor: Bool = false, selectionSpansLanes: Bool = false + ) -> CardFaceView { + CardFaceView( + store: store, card: card, role: .board(openCard: { _ in }), + marquee: marquee, drops: drops, hero: nil, isSelected: true, selectedCount: 1, + hasLeftNeighbor: hasLeftNeighbor, hasRightNeighbor: hasRightNeighbor, + selectionSpansLanes: selectionSpansLanes + ) + } + + #expect(face() == face(), "identical inputs, defaults included, still compare equal") + #expect(face() != face(hasLeftNeighbor: true)) + #expect(face() != face(hasRightNeighbor: true)) + #expect(face() != face(selectionSpansLanes: true)) + } + /// **The hero is a compared input too** (03-board-ui.md § Card face ▸ Hero image) — a resolution /// the parent does, like selected-ness, and the one input that changes the face's *height*. A gate /// that swallowed it would leave a card banding a picture it no longer names, or naming one it