fix(connections): make the group system report its refusals and survive a broken graph

[datlechin]

Found while investigating #1311, which asks for the connection group hierarchy inside the main
window. Before a second surface can render groups, the group system has to be able to report a
change and report a refusal. It can do neither today, and four more defects fall out of that.

This is the prerequisite. The #1311 feature builds on this branch.

What was wrong

A parent cycle crashed the connection list. computeGroupTreeIndices falls back to the
unguarded maxDescendantDepth for any group its root walk cannot reach, which is exactly what a
cycle produces, and the two recurse into each other until the stack runs out. The welcome list
recomputes those indices on every rebuild. A cycle got there because applyRemoteGroup wrote a
remote record straight through saveGroups, bypassing wouldCreateCircle and validateDepth,
the only callers of either. Two Macs that reparent P under C and C under P inside one sync window
each apply the other's record and end up with no root for either group.

Nothing announced a group change. GroupStorage never touched AppEvents, so a group created
in the connection form did not appear in an open welcome window until relaunch. connectionUpdated
already documents itself as covering group changes, and SyncCoordinator already sends it for a
remote group, so only the local path skipped the contract.

addGroup swallowed three refusals. A duplicate sibling name, a cycle and a nesting past the
cap were all a bare early return on a Void function. ConnectionGroupPicker then set
selectedGroupId to the id of a group that was never saved, so the connection was stored with a
groupId no group answers to and rendered as ungrouped, with nothing said.

Drag reorder moved the wrong row. .onMove reports positions in the rendered node list;
moveUngroupedConnections and moveGroupedConnections mapped those offsets into the unfiltered
connections array. rebuildTree hides every top-level favorite and a tag filter hides whatever it
does not match, so with one favorite present, dragging the last row moved the one before it.

Two Macs traded the whole group list forever. saveGroups marks every group dirty and
collectDirtyGroups uploads every dirty group, and applyRemoteGroup wrote a remote record
without asking whether it differed from the one already stored. One changed group therefore
re-uploaded all of them, the other Mac wrote them and re-uploaded them back, and neither side
settled. IOSSyncCoordinator.mergeGroups has always had the equality guard that stops this.

One unreadable entry lost every group. loadGroups caught the decode error, returned [] and
cached it. Every mutation rewrites the whole array, so the next group operation, including a single
incoming sync record, persisted that emptiness over the user's groups.

What changed

• One reading of the group graph, in groupsWithReachableParents: a group whose parent chain never
  reaches the root is presented at the top level. Both tree builders, depthOf,
  maxDescendantDepth and connectionCount go through it, so no stored graph can hide a group or
  send a walk round a cycle. maxDescendantDepth also carries a visited set of its own.
• applyRemoteGroup on GroupStorage writes the record exactly as it arrived, and the pull calls
  repairHierarchy once the whole batch has landed. A record cannot be judged on its own: a pull
  carries no dependency order, so a hierarchy the other Mac reversed legally, rooting B and then
  moving A under B, arrives as two records and whichever lands first reads as a cycle against the
  half that has not arrived. Judging per record rooted A for good and pushed that back as a revert
  of the user's own move. Neither raises a notification, because the pull already raises one
  coalesced notification for the batch.
• deleteGroup computes its delete set from the graph the list draws. A group left on a cycle is
  drawn as a top-level row while each cycle member is still the other's descendant in the raw
  stored graph, so deleting one displayed root used to delete the other and tombstone both.
• addGroup and updateGroup throw GroupStorageError. The create sheet keeps itself open and
  says which rule refused it; rename, recolour and move report through a new alert on the welcome
  window. saveGroups returns Bool, matching ConnectionStorage's documented convention.
• The mutators announce connectionUpdated after a successful write. saveGroups stays silent so
  the sync pull keeps its single coalesced event.
• One placement rule, canPlaceGroup, replacing the copy in WelcomeViewModel.moveGroup and the
  copy in the move menu. It is subtree-aware, which the storage's own rule was not: moving a
  two-level subtree one level down used to be allowed and pushed its children past the cap, where
  the tree stops drawing them. ConnectionGroup.maxNestingDepth replaces the three literal 3s.
• Reordering resolves by identity: the view hands over the ids it drew, and rows the list did not
  draw keep the slots they held.
• loadGroups decodes element by element, so one unreadable entry costs that entry. A payload it
  cannot read at all leaves the store untouched and refuses the write instead.
• applyRemoteGroup skips a record identical to the stored one and returns whether it changed
  anything, so the pull's coalesced notification is not raised for a no-op either.
• The import loop re-reads per group; two groups sharing a name in one envelope both passed a
  snapshot taken before either was added.
• The six new user-facing strings are in Localizable.xcstrings with all five translations.
  xcodebuild extracts them into .stringsdata but never writes them back to the catalog, which
  only Xcode does, so a CLI-verified branch ships them in English. Added through
  scripts/localization.py's own serializer: 204 insertions, no deletions, and
  scripts/localization.py verify still round-trips both catalogs byte for byte.

Verification

• verify.sh build: PASS
• verify.sh test GroupStorageTests ConnectionGroupTreeTests WelcomeViewModelTests StringCatalogIntegrityTests GroupMenuEntriesTests: PASS, 95 cases
• verify.sh lint on every changed app file: clean. The three changed test files carry the same
  seven pre-existing violations as at HEAD and no new ones; .swiftlint.yml scopes included to
  TablePro, so they have never been linted.
• New tests: cycles rooted in both tree builders and in the indices, maxDescendantDepth
  terminating on a cycle, each GroupStorageError, the subtree-aware move rule, an event per
  mutation and none from a remote apply, an unreadable store left untouched, an unreadable entry
  not taking the list down, an unchanged remote record leaving nothing dirty, and reordering under
  both a hidden favorite and an active tag filter.

No UI automation: every one of these is a storage or model rule, and the two UI surfaces are an
alert and a sheet field whose content the unit tests already pin.

Known residual

A remote group record that cannot be persisted is still acknowledged. applyPullResult saves the
new server token before applying anything (SyncCoordinator.swift:464-467), so any apply that
fails is not re-fetched until a full resync, for every record type and not only groups. This PR
adds one more way a group apply can fail, the unreadable-store refusal, but not the cause. Fixing
it means moving the token commit behind a successful apply across every record type, which is a
sync change well past this one.

Not in this PR

TableProMobile has its own group storage with no validation at all, and IOSSyncCoordinator .mergeGroups writes remote records unvalidated. Porting the rule was planned and then dropped:
parentId appears nowhere in the iOS target, so every group it creates is top-level and no iOS
path can author a cycle or exceed the cap. Validation there would have no reachable input today.

https://claude.ai/code/session_01L81TaoPWxkLd15CGw2riPq

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.