address PR review: verify-screen message, security-code shape, localization, dead code

- Wrong-QR alert is expected-aware for the verify screen too (Incorrect security code!), not only the server screen (Invalid server address!). - isSecurityCode now matches the real verificationCode shape (2+ space-separated decimal groups, 32+ digits) so a bare long number is no longer taken for a code; added a regression test for it. - iOS classifier trims newlines (parity with Kotlin .trim()); formatted string uses String.localizedStringWithFormat per apps/ios/LOCALIZATION.md. - Drop the dead address-name fallback: ConnectionLink.linkType is non-null on master (removes qr_type_simplex_address and the iOS else branch).
2026-08-27 22:34:51 +00:00 · 2026-06-22 16:23:06 +00:00
parent 435a42c00b
commit 7581713f02
8 changed files with 93 additions and 67 deletions
@@ -32,16 +32,18 @@ private func showWrongQRCodeAlert(_ scanned: QRCodeType, expected: QRCodeKind, t
        ))
        return
    }
-    // Unrecognised: we cannot name the kind. Keep the existing "invalid" wording, expected-aware
-    // (the server screen keeps its own, more specific "Invalid server address!" title).
+    // Unrecognised: we cannot name the kind. Keep each screen's own existing "invalid" wording —
+    // the server screen says "Invalid server address!", the verify screen says "Incorrect security
+    // code!" (it scans a code, not a link), everything else the generic "not a SimpleX link".
    if case .unknown = scanned {
-        let forServer = expected == .serverAddress
-        AlertManager.shared.showAlert(mkAlert(
-            title: forServer ? "Invalid server address!" : "Invalid QR code",
-            message: forServer
-                ? "Check server address and try again."
-                : "The code you scanned is not a SimpleX link QR code."
-        ))
+        switch expected {
+        case .serverAddress:
+            AlertManager.shared.showAlert(mkAlert(title: "Invalid server address!", message: "Check server address and try again."))
+        case .securityCode:
+            AlertManager.shared.showAlert(mkAlert(title: "Incorrect security code!"))
+        default:
+            AlertManager.shared.showAlert(mkAlert(title: "Invalid QR code", message: "The code you scanned is not a SimpleX link QR code."))
+        }
        return
    }
    // Recognised wrong kind: "<what it is>\n\n<where to scan it>".
@@ -74,8 +76,7 @@ private extension QRCodeType {
    var kindName: String {
        switch self {
        case let .connectionLink(_, linkType):
-            if let lt = linkType { return String(format: NSLocalizedString("This is a %@.", comment: "qr type"), lt.description) }
-            return NSLocalizedString("This is a SimpleX address.", comment: "qr type")
+            return String.localizedStringWithFormat(NSLocalizedString("This is a %@.", comment: "qr type"), linkType.description)
        case .serverAddress: return NSLocalizedString("This is a SimpleX server address.", comment: "qr type")
        case .migrationLink: return NSLocalizedString("This is a link to migrate to another device.", comment: "qr type")
        case .desktopAddress: return NSLocalizedString("This is an address for linking a mobile to a SimpleX desktop app.", comment: "qr type")
@@ -21,7 +21,7 @@ enum QRCodeKind {

 // One unified type for every kind of QR code (or pasted string) the app can scan.
 enum QRCodeType {
-    case connectionLink(text: String, linkType: SimplexLinkType?)
+    case connectionLink(text: String, linkType: SimplexLinkType)
    case serverAddress(text: String)
    case migrationLink(text: String)
    case desktopAddress(text: String)
@@ -62,12 +62,13 @@ func strHasSimplexFileLink(_ text: String) -> Bool {
 // Classify a scanned/pasted string by composing the existing local parsers in priority
 // order, returning the first match, else `.unknown`.
 func parseQRCode(_ raw: String) -> QRCodeType {
-    let t = raw.trimmingCharacters(in: .whitespaces)
+    let t = raw.trimmingCharacters(in: .whitespacesAndNewlines)
    if strHasSimplexFileLink(t) { return .migrationLink(text: t) }
    if t.hasPrefix(desktopAddressScheme) { return .desktopAddress(text: t) }
-    // Match only when the WHOLE string is exactly one SimpleX link (parity with Kotlin's
-    // parseToMarkdown size == 1, and with the prior strIsSimplexLink check) — not merely one
-    // link embedded among other text, which strHasSingleSimplexLink would accept.
+    // Match only when the WHOLE string is exactly one SimpleX link — same as `strIsSimplexLink`
+    // and the master connect scanner — not a link embedded among other text. A SimpleX *name*
+    // (Format.simplexName) is therefore not a connectionLink here; it falls through to .unknown,
+    // exactly as the master scanner treats it (names are handled only by the paste path).
    if let md = parseSimpleXMarkdown(t), md.count == 1,
       case let .simplexLink(_, linkType, _, _) = md[0].format {
        return .connectionLink(text: t, linkType: linkType)
@@ -77,12 +78,15 @@ func parseQRCode(_ raw: String) -> QRCodeType {
    return .unknown(text: t)
 }

-// A contact's security / verification code QR encodes the raw code with no scheme. The core
-// builds it as `verificationCode = T.pack . unwords . chunks 5 . show . os2ip` — decimal digits
-// of a SHA-256 integer, in groups of 5 separated by spaces (e.g. "61889 38426 ... 25"). Recognise
-// it by shape: ≥32 ASCII decimal digits once the grouping whitespace is stripped. Checked last,
-// so real links/addresses never reach here; the scanned text keeps its spaces, so verifyCode matches.
+// A contact's security / verification code QR encodes the raw code with no scheme. The core builds
+// it as `verificationCode = T.pack . unwords . chunks 5 . show . os2ip` — decimal digits of a SHA
+// integer, in groups (of 5) separated by spaces (e.g. "61889 38426 ... 25"). Recognise it by that
+// exact shape: 2+ whitespace-separated groups of ASCII decimal digits, 32+ digits total. Requiring
+// the grouping keeps a bare long number from being mistaken for a code. Checked last; the scanned
+// text keeps its spaces, so verifyCode still matches.
 func isSecurityCode(_ t: String) -> Bool {
-    let digits = t.filter { !$0.isWhitespace }
-    return digits.count >= 32 && digits.allSatisfy { $0.isNumber && $0.isASCII }
+    let groups = t.split(whereSeparator: { $0.isWhitespace })
+    return groups.count >= 2
+        && groups.reduce(0) { $0 + $1.count } >= 32
+        && groups.allSatisfy { $0.allSatisfy { $0.isNumber && $0.isASCII } }
 }
@@ -37,14 +37,23 @@ private fun showWrongQRCodeAlert(rhId: Long?, scanned: QRCodeType, expected: KCl
    )
    return
  }
-  // Unrecognised: we cannot name the kind. Keep the existing "invalid" wording, expected-aware
-  // (the server screen keeps its own, more specific "Invalid server address!" title).
+  // Unrecognised: we cannot name the kind. Keep each screen's own existing "invalid" wording —
+  // the server screen says "Invalid server address!", the verify screen says "Incorrect security
+  // code!" (it scans a code, not a link), everything else the generic "not a SimpleX link".
  if (scanned is QRCodeType.Unknown) {
-    val forServer = expected == QRCodeType.ServerAddress::class
-    AlertManager.shared.showAlertMsg(
-      title = generalGetString(if (forServer) MR.strings.smp_servers_invalid_address else MR.strings.invalid_qr_code),
-      text = generalGetString(if (forServer) MR.strings.smp_servers_check_address else MR.strings.code_you_scanned_is_not_simplex_link_qr_code),
-    )
+    when (expected) {
+      QRCodeType.ServerAddress::class -> AlertManager.shared.showAlertMsg(
+        title = generalGetString(MR.strings.smp_servers_invalid_address),
+        text = generalGetString(MR.strings.smp_servers_check_address),
+      )
+      QRCodeType.SecurityCode::class -> AlertManager.shared.showAlertMsg(
+        title = generalGetString(MR.strings.incorrect_code),
+      )
+      else -> AlertManager.shared.showAlertMsg(
+        title = generalGetString(MR.strings.invalid_qr_code),
+        text = generalGetString(MR.strings.code_you_scanned_is_not_simplex_link_qr_code),
+      )
+    }
    return
  }
  // Recognised wrong kind: "<what it is>\n\n<where to scan it>".
@@ -72,7 +81,7 @@ private fun showWrongQRCodeAlert(rhId: Long?, scanned: QRCodeType, expected: KCl
 // runs into the instruction below). Reuses the existing link-type labels for connection links.
 private val QRCodeType.description: String
  get() = when (this) {
-    is QRCodeType.ConnectionLink -> linkType?.let { String.format(generalGetString(MR.strings.qr_type_connection), it.description) } ?: generalGetString(MR.strings.qr_type_simplex_address)
+    is QRCodeType.ConnectionLink -> String.format(generalGetString(MR.strings.qr_type_connection), linkType.description)
    is QRCodeType.ServerAddress -> generalGetString(MR.strings.qr_type_server_address)
    is QRCodeType.MigrationLink -> generalGetString(MR.strings.qr_type_migration_link)
    is QRCodeType.DesktopAddress -> generalGetString(MR.strings.qr_type_desktop_address)
@@ -11,8 +11,9 @@ import chat.simplex.common.views.migration.strHasSimplexFileLink
 sealed class QRCodeType {
  abstract val text: String

-  // A SimpleX connection link (contact / invitation / group / channel / relay).
-  data class ConnectionLink(override val text: String, val linkType: SimplexLinkType?): QRCodeType()
+  // A SimpleX connection link (contact / invitation / group / channel / relay). The link parser
+  // always yields a non-null linkType (there is no address-name case on master).
+  data class ConnectionLink(override val text: String, val linkType: SimplexLinkType): QRCodeType()
  // A SMP/XFTP chat server address (the UserServer is built by the caller that has rhId).
  data class ServerAddress(override val text: String): QRCodeType()
  // A database migration file link (move a profile to this device).
@@ -53,12 +54,14 @@ fun parseQRCode(raw: String): QRCodeType {
 }

 // A contact's security / verification code QR encodes the raw code with no scheme. The core
-// builds it as `verificationCode = T.pack . unwords . chunks 5 . show . os2ip` — decimal digits
-// of a SHA-256 integer, in groups of 5 separated by spaces (e.g. "61889 38426 ... 25"). Recognise
-// it by shape: ≥32 decimal digits once the grouping whitespace is stripped (no hex — `show` only
-// emits decimal). Checked last (after every real link/address parser), so those never reach here;
-// the scanned text keeps its spaces, so verifyCode still matches.
+// builds it as `verificationCode = T.pack . unwords . chunks 5 . show . os2ip` — decimal digits of
+// a SHA integer, in groups (of 5) separated by spaces (e.g. "61889 38426 ... 25"). Recognise it by
+// that exact shape: 2+ whitespace-separated groups of decimal digits, 32+ digits total. Requiring
+// the grouping keeps a bare long number from being mistaken for a code. Checked last (after every
+// real link/address parser); the scanned text keeps its spaces, so verifyCode still matches.
 internal fun isSecurityCode(t: String): Boolean {
-  val digits = t.filterNot { it.isWhitespace() }
-  return digits.length >= 32 && digits.all { it in '0'..'9' }
+  val groups = t.trim().split(Regex("\\s+"))
+  return groups.size >= 2 &&
+    groups.sumOf { it.length } >= 32 &&
+    groups.all { g -> g.all { it in '0'..'9' } }
 }
@@ -773,7 +773,6 @@
  <!-- unified QR scan: wrong-type alert -->
  <string name="wrong_qr_code">Wrong QR code</string>
  <string name="qr_type_connection">This is a %s.</string>
-  <string name="qr_type_simplex_address">This is a SimpleX address.</string>
  <string name="qr_type_server_address">This is a SimpleX server address.</string>
  <string name="qr_type_migration_link">This is a link to migrate to another device.</string>
  <string name="qr_type_desktop_address">This is an address for linking a mobile to a SimpleX desktop app.</string>
@@ -56,9 +56,15 @@ class ParseQRCodeTest {
    assertFalse(isSecurityCode("a3f5".repeat(16)))
  }

+  @Test
+  fun securityCode_ungroupedLongNumber_isNotRecognised() {
+    // A bare long number (no space grouping) is not the verificationCode shape — must not match.
+    assertFalse(isSecurityCode("6188938426639340957696390793898412485253"))
+  }
+
  @Test
  fun securityCode_shortNumber_isNotRecognised() {
-    assertFalse(isSecurityCode("12345 67890"))      // only 10 digits once spaces are stripped
+    assertFalse(isSecurityCode("12345 67890"))      // grouped, but only 10 digits total
  }

  @Test
@@ -48,13 +48,17 @@ when in doubt about behaviour, that doc is authoritative.

 ## As-built deltas vs this plan (branch `nd/qr-scan-clarity`, off `master`)

-The plan was verified against `stable`; the branch is off `master`, which is **behind** stable.
-The code therefore differs from the plan text in these (intentional, documented) ways:
+The branch is rebased onto current `master` (which has caught up to `stable` — it now has
+`strConnectTarget`/`ConnectTarget`/SimplexName). The code differs from the original plan text in
+these (intentional, documented) ways:

-1. **Classifier (master):** no `strConnectTarget`/`ConnectTarget`/SimplexName on master, so
-   `parseQRCode` uses `parseToMarkdown` (`size == 1`) + `Format.SimplexLink.linkType` — see the
-   updated C1.0 above. There is **no address-name case** on master, so `ConnectionLink.linkType`
-   is always non-null and ConnectView gains no "names now accepted" improvement.
+1. **Classifier uses `parseToMarkdown`, not `strConnectTarget`.** `parseQRCode` matches a
+   connection link with `parseToMarkdown` (`size == 1`) + `Format.SimplexLink.linkType` — this is
+   exactly master's own scanner check (`strIsSimplexLink`). A SimpleX **name** therefore does NOT
+   match (it is `Format.SimplexName`, not `Format.SimplexLink`), so it classifies as `Unknown`,
+   the same "not a SimpleX link" the master connect *scanner* shows today (names are handled only
+   by the **paste** path via `strConnectTarget`, untouched here). Consequently `ConnectionLink`
+   only ever wraps a real link, so `ConnectionLink.linkType` is **non-null** by construction.
 2. **iOS uses a small `enum QRCodeKind`** (`connectionLink/serverAddress/migrationLink/
   desktopAddress/code`) for `expected`, not a closure: Swift has no `KClass`, and the alert
   needs the expected kind for the Unknown-on-server message. This is the Code↔Unknown pairing
@@ -104,7 +108,7 @@ is NOT compiled** (no Xcode here) — and the two new Swift files still need add
 ```kotlin
 sealed class QRCodeType {
  abstract val text: String
-  data class ConnectionLink(override val text: String, val linkType: SimplexLinkType?): QRCodeType()
+  data class ConnectionLink(override val text: String, val linkType: SimplexLinkType): QRCodeType()  // non-null on master
  data class ServerAddress (override val text: String): QRCodeType()
  data class MigrationLink (override val text: String): QRCodeType()
  data class DesktopAddress(override val text: String): QRCodeType()
@@ -128,7 +132,7 @@ fun parseQRCode(raw: String): QRCodeType {              // no rhId; FFI parse on
      when {
        link != null                  -> QRCodeType.ConnectionLink(t, link.linkType)
        parseServerAddress(t) != null -> QRCodeType.ServerAddress(t)
-        isSecurityCode(t)             -> QRCodeType.SecurityCode(t)   // long hex/decimal digits, no scheme
+        isSecurityCode(t)             -> QRCodeType.SecurityCode(t)   // space-grouped decimal digits, no scheme
        else                          -> QRCodeType.Unknown(t)
      }
    }
@@ -137,12 +141,12 @@ fun parseQRCode(raw: String): QRCodeType {              // no rhId; FFI parse on

 // A contact's verification code QR is the raw code, no scheme. Core builds it as
 // `verificationCode = unwords . chunks 5 . show . os2ip` = DECIMAL digits grouped in 5s separated
-// by SPACES, so strip whitespace, then require decimal digits only (show emits no hex). Checked
-// last, so real links/addresses never reach here. NB: qr.text keeps the spaces, so verifyCode
-// (core noSpaces-normalises) still matches. Canonical fixture: tests/ChatTests/Direct.hs.
+// by SPACES. Match that exact shape — 2+ whitespace-separated groups of decimal digits, 32+ total
+// — so a bare long number isn't mistaken for a code. Checked last; qr.text keeps the spaces, so
+// verifyCode (core noSpaces-normalises) still matches. Canonical fixture: tests/ChatTests/Direct.hs.
 internal fun isSecurityCode(t: String): Boolean {
-  val digits = t.filterNot { it.isWhitespace() }
-  return digits.length >= 32 && digits.all { it in '0'..'9' }
+  val groups = t.trim().split(Regex("\\s+"))
+  return groups.size >= 2 && groups.sumOf { it.length } >= 32 && groups.all { g -> g.all { it in '0'..'9' } }
 }
 ```
 - [x] **C1.1 — unit test `parseQRCode`** (load-bearing). One fixture per kind + garbage, and
@@ -173,8 +177,9 @@ suspend fun handleScan(

  `showWrongQRCodeAlert(rhId, scanned, expected, close)` + the per-kind `description`/`whereToScan`
  metadata implement the design's message rules (recognised-wrong → `wrong_qr_code` +
-  `"<desc>\n\n<where>"`; relay → `relay_address_alert_title` + `relay_address_alert_message`; Unknown → `invalid_qr_code` +
-  expected-aware msg; **no-user** → no button + `qr_where_no_user`). Connect button dismisses the
+  `"<desc>\n\n<where>"`; relay → `relay_address_alert_title` + `relay_address_alert_message`; Unknown →
+  expected-aware: server → `smp_servers_invalid_address`, verify screen → `incorrect_code`, else
+  `invalid_qr_code`; **no-user** → no button + `qr_where_no_user`). Connect button dismisses the
  scanner first, then connects:
  `onConfirm = { close(); withBGApi { planAndConnect(rhId, qr.text, close = null) } }`.
  Nothing calls these yet. Build.
@@ -260,7 +265,6 @@ a full `Alert(…primaryButton…)` for Connect; `ScanCodeView.verify` returns `
 ```
 wrong_qr_code              = "Wrong QR code"
 qr_type_connection         = "This is a %s."   # %s = SimplexLinkType.description (e.g. "SimpleX group link")
-qr_type_simplex_address    = "This is a SimpleX address."
 qr_type_server_address     = "This is a SimpleX server address."
 qr_type_migration_link     = "This is a link to migrate to another device."
 qr_type_desktop_address    = "This is an address for linking a mobile to a SimpleX desktop app."
@@ -6,14 +6,13 @@ verified constraints. Exact code, signatures, string keys/values, and the commit
 Cx / Appendix A"). When the two disagree, the impl plan wins on code/strings; this doc wins on
 intent.

-> **Branch note (stable vs master).** This design was verified against `stable`; the
-> implementation branch is off `master`, which is **behind** stable. On master there is **no**
-> `strConnectTarget` / `ConnectTarget` / SimplexName, and short-link classification goes through
-> `parseToMarkdown` (size==1 → `Format.SimplexLink`), iOS through `strHasSingleSimplexLink`. So
-> wherever the sections below name `strConnectTarget`/`ConnectTarget`/`simplexName` or cite a
-> `file:line` (source map, type model, navigation/adversarial reviews), read them as the *stable*
-> reference that motivated the design; the **authoritative master mapping is the impl plan's
-> "As-built deltas vs this plan"** section. Name-link cases are **N/A on master**.
+> **Branch note.** This branch is rebased onto current `master`, which has caught up to `stable`
+> (it now has `strConnectTarget` / `ConnectTarget` / SimplexName). The scanner classifier uses
+> `parseToMarkdown` (size==1 → `Format.SimplexLink`) — equivalent to master's own scanner check
+> `strIsSimplexLink` — so a SimpleX **name** QR classifies as `Unknown` ("not a SimpleX link"),
+> exactly as master's connect *scanner* behaves today. Names are recognised only by the **paste**
+> path (`strConnectTarget` → "unsupported name" alert), which this change does not touch, so there
+> is **no scanner regression**. `file:line` references in the source map / reviews below are approximate.

 ## Goal

@@ -82,7 +81,8 @@ genuinely-unrecognised case (a non-SimpleX QR, or garbage).

 Six variants, each carrying the scanned `text`:
 - **ConnectionLink** — also carries the link sub-type (contact / invitation / group / channel /
-  relay), or none for a SimpleX address-*name*.
+  relay). On master the parser always yields a non-null sub-type (no address-*name* case — that
+  exists only on stable), so the field is non-nullable here.
 - **ServerAddress** — `text` only; the `UserServer` is built later in `onMatch`, so the
  classifier needs no `rhId`.
 - **MigrationLink**, **DesktopAddress** — `text` only.