From 323609a60fe0679e77925094dc2214c231b71e83 Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Sat, 1 Aug 2026 16:27:52 +0000 Subject: [PATCH] plans: record the iOS port and its Swift-specific traps --- .../2026-07-30-new-profile-for-invitation.md | 168 +++++++++++++----- 1 file changed, 126 insertions(+), 42 deletions(-) diff --git a/plans/2026-07-30-new-profile-for-invitation.md b/plans/2026-07-30-new-profile-for-invitation.md index 843e9e03a8..6989e5d4da 100644 --- a/plans/2026-07-30-new-profile-for-invitation.md +++ b/plans/2026-07-30-new-profile-for-invitation.md @@ -1,6 +1,7 @@ # Feature: create a new profile when choosing a profile for an invitation -Branch `nd/new-profile-for-invitation`, PR #7329. Core + Android/desktop; iOS follows. +Branch `nd/new-profile-for-invitation`, PR #7329. Core, Android/desktop and iOS — +the iOS part is unverified, see §7. > Accept this invitation as someone new. @@ -38,8 +39,9 @@ and re-invite from another profile", not "accept as another profile". Two facts collide: -- **Creating a user activates it** — `createUserRecordAt` runs - `UPDATE users SET active_user = 0` and the handler sets the `currentUser` TVar. +- **Creating a user activates it** — the handler hardcoded `True` for the `activeUser` + parameter `createUserRecordAt` already had, which deactivates the rest and sets the + `currentUser` TVar. (The `UPDATE users SET active_user = 0` is gated on it.) - **The reassignment resolves the chat under the *active* user** — `APIChangePreparedContactUser` is `withUser $ \user -> getContact db cxt user contactId`. @@ -60,33 +62,51 @@ Rejected alternatives: With the flag the sequence reads as the story, with nothing to undo: create (active user untouched) → reassign (old profile still active, still owns it) → switch once. +Known, deliberately not fixed: `activateNewUser` comes from a `readTVarIO`, and +`deleteChatUser` clears `currentUser` when the *only visible* profile is deleted — so a +concurrent delete could leave none active. It needs deleting your last profile while +creating one from an invitation picker, which the UI cannot reach; master has the same +window with the opposite outcome. + `BoolDef` gives `omittedField = False`, so absent = today's behaviour and iOS, the CLI -and older callers are untouched. `createUserRecordAt` already took an `activeUser :: Bool`; -the handler was hardcoding `True`. The flag is ignored when there is no active user to -keep, which would otherwise leave none at all. +and older callers are untouched. The flag is ignored when there is no active user to keep, +which would otherwise leave none at all. Response stays `CRActiveUser` — it carries the created user, which on this path is not the active one. Documented at the field; no client decoder changes. ## 3. Commits +In branch order. + 1. **parameterise the create-profile form's submit action** — `CreateProfile` chose between two submit paths internally, invisible at its two call sites. The callback passes *raw fields*, not a `Profile`: the two paths build different ones (no-profile-setup drops `shortDescr`), so a `Profile`-shaped callback would silently - change behaviour. Net −6 lines; the `chatModel` parameter was redundant anyway. -2. **core: `keepActiveUser`** — §2. Also regenerates three client artifacts (§5). -3. **core: tests** — `keepActiveUser` works; and omitting it still activates (guards an - iOS-breaking regression, since iOS never sends the field). -4. **fix: don't switch profile when the reassignment failed** — pre-existing bug. + change behaviour. The branch itself moves to a named `createProfileFromForm` that both + existing call sites delegate to — copying it into each would have left two copies to + drift. Net +12 lines; the `chatModel` parameter was redundant anyway. +2. **fix: don't switch profile when the reassignment failed** — pre-existing bug. `changeActiveUser_` sat outside the null guards, so a failed reassign still switched profile and stranded the invitation. Reachable today. iOS unaffected (its API throws, so control flow skips the switch). -5. **feature: surface 1** — see gotchas. -6. **feature: surface 2** — extracts `selectProfile` so the new row takes exactly the - same path as picking an existing profile. -7. **fix: profile image lost when creating the first profile** — pre-existing on master, - unrelated, safe to drop. +3. **core: `keepActiveUser`** — §2. Also regenerates three client artifacts and + hand-syncs two more that no test covers (§5). +4. **core: tests** — `keepActiveUser` works; and omitting it still activates (guards an + iOS-breaking regression, since iOS never sends the field). +5. **feature: surface 1** — see gotchas. The create-then-hand-off flow lives in a single + `createProfileForInvitation(rhId, creating) { newUser -> … }` beside the other + profile-creation helpers; both surfaces call it, differing only in what they do with + the created profile. Written out twice it was ~35 duplicated lines free to drift. +6. **extract `selectProfile`** — pure move of the row handler's body out of the lambda, + so the next commit can take the same path instead of duplicating it. No behaviour + change; review it by checking the body is unchanged apart from indentation. +7. **feature: surface 2** — 32 added lines, nothing removed, now that the move is its own + commit. +8. **fix: profile image lost when creating the first chat profile** — pre-existing on + master, unrelated, safe to drop. Desktop-only: reachable just from the user picker's + "Create chat profile" row, shown when there is no local profile. Onboarding never hits + it — those screens take a name only. Uses the existing `users_add` ("Add profile") string — **zero new translation keys**. @@ -103,17 +123,20 @@ Uses the existing `users_add` ("Add profile") string — **zero new translation whenever search text filters the active profile out. - **`keepingChatId` does not open the chat**, it only preserves its place in the reloaded list. Set `chatModel.chatId.value = chat.id` after switching, or you land on the new - profile's chat list instead of the invitation. + profile's chat list instead of the invitation. The *old* `chat.id` stays valid because + the reassignment updates in place — `updatePreparedContactUser` runs + `UPDATE contacts SET user_id = ? WHERE contact_id = ?` and re-reads the same id, and the + group path does the same for `group_id`. Do not "fix" this to use the returned chat. - **In-flight flags must not be `rememberSaveable`, nor scoped to a lazy item** — either strands them `true` (process death skips the resetting `finally`; scrolling disposes the item) and the row dies permanently. - **`listUsers` throws and `withBGApi` does not catch** (`wrapWithLogging` has no catch), so an exception after creation aborts silently. Use `runCatching` for the cosmetic refresh, and the safe `changeActiveUser` wrapper rather than `changeActiveUser_`. -- **Create first, close only on success** — `apiCreateActiveUser` shows its own alert and - returns null, so dismissing first discards everything typed on a duplicate name. Guard - dismissal-during-creation with a `ModalViewId` + `isLastModalOpen`, or a back-tap still - switches profile. +- **Create first, close only on success** — the shared `createUser` helper behind both + wrappers shows its own alert and returns null, so dismissing first discards everything + typed on a duplicate name. Guard dismissal-during-creation with a `ModalViewId` + + `isLastModalOpen`, or a back-tap still switches profile. - **Stale remote host**: an older core ignores the unknown field and activates anyway. There is no version to gate on — but the response carries `activeUser`, so check it and resync rather than issuing a reassign that must fail. @@ -121,32 +144,47 @@ Uses the existing `users_add` ("Add profile") string — **zero new translation new profile always has the SimpleX Team/Status cards. Core returns the updated contact; do not pre-check. -## 5. Generated artifacts — easy to miss +## 5. Generated and hand-synced artifacts — easy to miss `NewUser` is a documented API type, and `apiDocsTest` generates **11 files** from those definitions (markdown, TypeScript and Python clients). Adding one field changed three of -them, one line each. `testGenerate` writes the file *then* asserts it matched, so a stale -artifact fails the test and the run repairs it. +them, one line each. `testGenerate` writes the file *then* asserts — so a run repairs a +stale artifact, and **an unchanged tree afterwards is the real assertion**. + +The generator emits `BoolDef` fields as **required** in the client types (unlike `Maybe`, +which becomes `profile?`/`NotRequired`). So the two client libraries that are kept in sync +by hand — `simplex-chat-python/api.py` and `simplex-chat-nodejs/api.ts`, both of which +build a `NewUser` literal listing every bool — stop type-checking until the new field is +added there too. Neither is generated, so no test catches it; the precedent is +`a4e3a1ea1`, which added `clientService` to both. (`simplex-chat-client/typescript` has +its own separate types and is deliberately untouched — it was not updated for +`clientService` either.) **Before touching any type in `Simplex.Chat.Types`, run the `Bot API docs` tests.** The -Haskell compiles fine without them; only that test catches the drift. This was missed for -four review rounds because earlier runs were *targeted* — a narrow selection is not -regression coverage. +Haskell compiles fine without them; only that test catches the drift, and only a full run +does — a targeted selection is not regression coverage. -⚠ **Master is currently broken here**: `CPTUnknown Text` (commit `61012d208`) makes the -generator `error` on an enum constructor with fields. Worse, `testGenerate` truncates the -file before the exception fires, so running the suite on master **empties three -checked-in files** (~13,500 lines). Restore them; never commit the truncation. +**Fixed on master since this was written.** `CPTUnknown Text` (`61012d208`) made the +generator `error` on an enum constructor with fields — and since the write precedes the +assert, running the suite **emptied three checked-in files** (~13,500 lines). `31faceef7` +excludes `CPTUnknown`, as `BTUnknown` already was. On older bases expect the truncation; +restore the files, never commit it. Do **not** commit the regenerated `*_query_plans.txt` — stale on master for unrelated -reasons, and the suite rewrites them on every run. +reasons, and the suite rewrites them on every run (confirmed again on this base). ## 6. Testing -Full suite: **981 examples, 6 failures, 41 pending** — all six reproduce on a master -control (multi-user TTL ×3, broadcast bot, multicast discovery, query plans), so the -branch adds **no regressions**. The TTL ones fail reproducibly in isolation, not only -under load; only the control settled that. +Full suite, re-run after rebasing onto `34b74d1c4`: **986 examples, 6 failures, 41 +pending** (it was 981 at base `cbae9c5e8`; master's `DirectoryTests.hs` added the 5). The +six failures are the *same six* both times — multi-user TTL ×3, broadcast bot, multicast +discovery, query plans — so the branch adds **no regressions**. The TTL ones fail +reproducibly in isolation, not only under load; only a master control settled that. That +control was run on the older master; it still stands, since the set is unchanged and the +branch touches none of those paths. + +`Bot API docs` passes 17/17 leaving the tree unchanged, which is what proves the three +regenerated artifacts match the current generator. Both new `keepActiveUser` tests pass. Manual matrix (Android + desktop) — the part that actually finds bugs: @@ -155,6 +193,7 @@ Manual matrix (Android + desktop) — the part that actually finds bugs: | Prepared contact/group → Add profile → connect | connects as the new profile; **lands in the chat**, not the chat list | | Chosen avatar | becomes the **profile picture**, not the description | | Row position | top of the expanded list (screenshot it; reversed layout) | +| Dividers, with **one** profile | row follows the divider-above-each-row convention: a line at the top edge, none below it — same gap master has above the selected row. Confirm it reads as intentional | | Scroll row out of view and back, then tap | still works | | Duplicate name | typed name and avatar survive; form stays open | | Hidden current profile | works, no password prompt | @@ -167,10 +206,55 @@ Manual matrix (Android + desktop) — the part that actually finds bugs: **Every user-visible bug in this feature was found by running the app** — none by compilation, the test suite, or ten rounds of review. -## 7. iOS +## 7. iOS — done, but **never compiled** -Structure mirrors Kotlin. Commit 4 has **no** iOS counterpart (already correct), and the -positional-argument bug cannot occur. Needs: `NewUser` and `createActiveUser` gain the -field, an `apiCreateProfileKeepingActive`, the row in `ContextProfilePickerView` and -`ActiveProfilePicker`, and the same `chatId` auto-open. iOS's picker is **not** -reverse-laid-out — the row goes first to appear at the top. +Three commits mirroring 1/5/7. Commit 2 has no iOS counterpart (already correct: the API +throws, so a failed reassign skips the switch — and the neither-direct-nor-group +fall-through is unreachable, since `nextConnectPrepared` gates the picker and is false for +every other case), and the positional-argument bug cannot occur — Swift's `Profile.init` +requires labels. + +Differences from Kotlin, each deliberate: + +- **Surface 1's row goes first**, not last: that list is not reverse-laid-out, so + emitting first is what puts it at the top. Surface 2's row stays **last** on both + platforms — that list is not reversed on either, so last means bottom on both. +- **`onSubmit` is optional**, not required — it touches no existing call site, which + matters more when the change cannot be compiled. +- **The two create flows are *not* extracted into one**, unlike Kotlin. The 12 identical + lines include `defer { creating = false }`, which must stay in the function that owns + the whole flow — inside a helper it would fire on the helper's return, clearing the + in-flight flag before the caller has reassigned and letting a second submit through. + That leaves ~5 safely extractable lines, and the rest differs anyway (alert mechanism: + global `showAlert` vs the view's `alert` state; final action; an extra `profiles` + refresh). Kotlin's extraction was worth it because the shared flow was 35 lines and + compile-checked; this one is neither. +- Reuses `"Add profile"` (16 locales) and `"Error changing chat profile"` (10) — again + zero new translation entries. + +Swift-specific traps found while reviewing, all fixed: + +- **Two `.sheet` modifiers on one view conflict** in SwiftUI, and both pickers already + present `IncognitoHelp` from the root — so the new sheet goes on the picker itself: a + descendant of the root, but *not* the row. Rows live in a `LazyVStack`/`List`, which + may dispose them and take the presented sheet with them. +- **Surface 1's picker height is computed from the row count** — + `USER_ROW_SIZE * min(MAX_VISIBLE_USER_ROWS, users.count + 1)` — unlike Kotlin's + content-sized `heightIn(max = ...)`. Adding a row without making that `+ 2` clips one, + and since the list scrolls to `BOTTOM_ANCHOR` on appear, the clipped one is "Add + profile" at the top: invisible to exactly the single-profile user this is for. +- **Trailing-closure syntax binds to the last init parameter**, which for a memberwise + init is not necessarily `onSubmit`; both call sites pass `onSubmit:` explicitly. +- `if creatingProfile { … }` then setting it is a **non-atomic check-and-set**, and reads + `@State` off the main actor; both are done inside one `MainActor.run`. +- **Presenting an alert while a sheet is dismissing swallows it.** The form is dismissed + only on the success path, so failures leave it open with the alert over it — which is + also what any other failure does. +- The `onChange(of: selectedProfile)` handler that surface 2 reuses **returns early unless + `profileSwitchStatus == .switchingUser`**, so both must be assigned in the same + `MainActor.run` before SwiftUI's next update. + +⚠ **No Swift toolchain on the machine this was written on**, so none of it is compiled or +run — treat it as unverified until it builds in Xcode, for the reason at the end of §6. +First thing to check: open a prepared chat with **one** profile and confirm "Add profile" +is visible without scrolling.