fix(coordinator): stop the row inspector's JSON field undoing every keystroke

CHANGELOG.md

@@ -70,6 +70,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Unresponsive app and a dropped keystroke when typing in the row inspector's JSON field. (#3051)
 - Raw Oracle driver error in the schema switch failure dialog. (#3053)
 - Oracle health check closing a connection a statement was still running on. (#3053)
 - Global saved query inside a folder missing from every other connection. (#3045)

TablePro/Core/Services/Infrastructure/SampleDatabaseService.swift

@@ -85,6 +85,7 @@ internal final class SampleDatabaseService {
         }
 
         if fileManager.fileExists(atPath: installed.path) {
+            seedUITestFixturesIfRequested(at: installed)
             return
         }
 
@@ -96,6 +97,15 @@ internal final class SampleDatabaseService {
         } catch {
             throw SampleDatabaseError.copyFailed(message: error.localizedDescription)
         }
+        seedUITestFixturesIfRequested(at: installed)
+    }
+
+    /// Chinook carries nothing a JSON editor will open, so a UI test that needs one asks for a
+    /// fixture table at launch. Seeded on every install, including the one that finds the file
+    /// already there, so a case that edited the row does not hand it to the next case.
+    private func seedUITestFixturesIfRequested(at installed: URL) {
+        guard UITestJsonFixture.isRequested else { return }
+        UITestJsonFixture.seed(into: installed)
     }
 
     internal func resetToBundled() throws {
@@ -123,6 +133,7 @@ internal final class SampleDatabaseService {
         } catch {
             throw SampleDatabaseError.copyFailed(message: error.localizedDescription)
         }
+        seedUITestFixturesIfRequested(at: installed)
     }
 
     private func removeInstalledDatabaseFiles() throws {

TablePro/Core/Services/Infrastructure/UITestJsonFixture.swift

@@ -0,0 +1,72 @@
+//
+//  UITestJsonFixture.swift
+//  TablePro
+//
+
+import Foundation
+import os
+import SQLite3
+
+/// A table with a JSON document in it, added to the installed sample database so a UI test can
+/// reach the row inspector's JSON field editor.
+///
+/// Chinook has no JSON column and no text value that parses as a JSON object, so
+/// `FieldEditorResolver` never returns `.json` against it and `JsonEditorView` never appears. That
+/// left the editor with no deterministic fixture at all, which is why the update loop of #3051
+/// shipped with no UI coverage over it.
+///
+/// Written straight through SQLite rather than through a driver plugin: this runs while the sample
+/// file is being installed, before any connection to it exists, and the app already links SQLite
+/// for its own stores. Gated on the storage sandbox and on an explicit launch variable, so a
+/// shipped build cannot reach it even by accident.
+internal enum UITestJsonFixture {
+    internal static let launchVariable = "TABLEPRO_UI_TEST_SEED_JSON_TABLE"
+    internal static let tableName = "json_fixture"
+
+    nonisolated private static let logger = Logger(subsystem: "com.TablePro", category: "UITestJsonFixture")
+
+    /// A document with enough structure to lay out over several lines, and a value a test can
+    /// assert on after typing into it.
+    internal static let document = """
+    {"id": 1, "name": "Acme", "active": true, "tags": ["alpha", "beta"], "limits": {"seats": 25}}
+    """
+
+    internal static var isRequested: Bool {
+        guard AppStorageEnvironment.shared.isIsolated else { return false }
+        let raw = ProcessInfo.processInfo.environment[launchVariable]?
+            .trimmingCharacters(in: .whitespacesAndNewlines)
+        return !(raw ?? "").isEmpty
+    }
+
+    /// Adds the fixture table to a SQLite file, replacing any earlier copy so a re-installed sample
+    /// never inherits a row a previous test edited.
+    internal static func seed(into fileURL: URL) {
+        var handle: OpaquePointer?
+        guard sqlite3_open_v2(fileURL.path, &handle, SQLITE_OPEN_READWRITE, nil) == SQLITE_OK, let handle else {
+            logger.error("Could not open the sample database to seed the JSON fixture")
+            sqlite3_close(handle)
+            return
+        }
+        defer { sqlite3_close(handle) }
+
+        let statements = [
+            "DROP TABLE IF EXISTS \(tableName)",
+            "CREATE TABLE \(tableName) (id INTEGER PRIMARY KEY, label TEXT NOT NULL, payload TEXT NOT NULL)",
+            "INSERT INTO \(tableName) (id, label, payload) VALUES (1, 'First', '\(escaped(document))')",
+            "INSERT INTO \(tableName) (id, label, payload) VALUES (2, 'Second', '\(escaped(document))')"
+        ]
+        for statement in statements {
+            var message: UnsafeMutablePointer<CChar>?
+            guard sqlite3_exec(handle, statement, nil, nil, &message) == SQLITE_OK else {
+                logger.error("Seeding the JSON fixture failed: \(String(cString: message ?? strdup("")), privacy: .public)")
+                sqlite3_free(message)
+                return
+            }
+            sqlite3_free(message)
+        }
+    }
+
+    private static func escaped(_ value: String) -> String {
+        value.replacingOccurrences(of: "'", with: "''")
+    }
+}

TablePro/Models/UI/JsonFieldEditingModel.swift

@@ -0,0 +1,60 @@
+//
+//  JsonFieldEditingModel.swift
+//  TablePro
+//
+
+import Foundation
+
+/// The editing model behind the row inspector's JSON field: what the editor shows, and when that
+/// text and the stored value have to be reconciled.
+///
+/// It lives outside the view because the defect it exists to prevent is a fixpoint between two
+/// mirrors of one value, and a fixpoint is only testable if it can be driven without a window.
+///
+/// The rule is ``lastSynced``: the editor's text and the stored value are known to agree at that
+/// string, so a value arriving from the store that still means the same thing is this editor's own
+/// write coming back and is ignored. What the field stages is a compacted form of what the editor
+/// shows, so the two agree in meaning rather than in bytes, and a comparison against the editor's
+/// displayed text cannot tell an echo from an external change on its own.
+///
+/// Adopting an echo is what let a keystroke be overwritten by the value it had just replaced, and
+/// then pushed back, forever: one keystroke produced 6,944 body evaluations in 30 seconds at 100%
+/// CPU, and the typed character was discarded (#3051).
+internal struct JsonFieldEditingModel: Equatable, Sendable {
+    private(set) var displayText: String
+
+    /// The last text the editor published, in display form. Never the stored form, which has been
+    /// through `JsonReindenter.normalize` and would compare unequal to everything the editor holds.
+    private var lastSynced: String
+
+    internal init(storedValue: String) {
+        let opening = JsonReindenter.reindent(storedValue)
+        self.displayText = opening
+        self.lastSynced = opening
+    }
+
+    /// The user typed. Returns the value to write into the field's binding, or nil when the text
+    /// already agrees with the store and writing it would only start another round.
+    internal mutating func typed(_ text: String) -> String? {
+        displayText = text
+        guard text != lastSynced else { return nil }
+        lastSynced = text
+        return text
+    }
+
+    /// The field's binding delivered a value. Returns whether the editor adopted it.
+    ///
+    /// Anything that still means what ``lastSynced`` means is this editor's own write coming back,
+    /// compacted by `MultiRowEditState` or dropped by it because the value matched the stored one
+    /// again. Everything else is a real external change: Set NULL, Set DEFAULT, Set EMPTY, an SQL
+    /// function, or a commit from the pop-out value window.
+    ///
+    /// `JsonReindenter.normalize` returns its source unchanged when the document does not parse, so
+    /// a half-typed value is compared exactly rather than reported equal to something else.
+    internal mutating func received(_ value: String) -> Bool {
+        guard JsonReindenter.normalize(value) != JsonReindenter.normalize(lastSynced) else { return false }
+        displayText = JsonReindenter.reindent(value)
+        lastSynced = displayText
+        return true
+    }
+}

TablePro/Models/UI/MultiRowEditState.swift

@@ -253,6 +253,13 @@ final class MultiRowEditState: ObservableObject {
         }
     }
 
+    /// What a field's editor is showing right now, read from the store rather than from a copy an
+    /// earlier render captured. This is what a field's value binding answers.
+    func currentText(at index: Int) -> String {
+        guard fields.indices.contains(index) else { return "" }
+        return FieldValueState.resolve(fields[index]).editableText
+    }
+
     /// Update a field's pending value
     func updateField(at index: Int, value: String?) {
         guard index < fields.count else { return }

TablePro/Views/RowInspector/FieldEditors/JsonEditorView.swift

@@ -10,15 +10,28 @@ internal struct JsonEditorView: View {
     var onPopOut: ((String) -> Void)?
     var isExpanded = false
 
-    @State private var displayText: String
+    @State private var model: JsonFieldEditingModel
     @AppStorage(PreferenceKeys.rowInspectorJsonFieldHeight.name, store: AppStorageEnvironment.shared.defaults) private var fieldHeight = ResizableFieldMetrics
         .defaultJsonHeight
 
     init(context: FieldEditorContext, onPopOut: ((String) -> Void)? = nil, isExpanded: Bool = false) {
         self.context = context
         self.onPopOut = onPopOut
         self.isExpanded = isExpanded
-        self._displayText = State(wrappedValue: JsonReindenter.reindent(context.value.wrappedValue))
+        self._model = State(wrappedValue: JsonFieldEditingModel(storedValue: context.value.wrappedValue))
+    }
+
+    /// The editor writes through the model, which decides whether the text is worth publishing.
+    /// Binding `$model.displayText` straight to the editor would publish the model's own
+    /// corrections back into the store.
+    private var editorText: Binding<String> {
+        Binding(
+            get: { model.displayText },
+            set: { typed in
+                guard !context.isReadOnly, let value = model.typed(typed) else { return }
+                context.value.wrappedValue = value
+            }
+        )
     }
 
     var body: some View {
@@ -27,19 +40,26 @@ internal struct JsonEditorView: View {
             range: ResizableFieldMetrics.jsonHeightRange,
             expandedHeight: isExpanded ? ResizableFieldMetrics.expandedHeight : nil
         ) {
-            JSONCodeEditor(text: $displayText, isEditable: !context.isReadOnly)
+            JSONCodeEditor(text: editorText, isEditable: !context.isReadOnly)
                 .clipShape(RoundedRectangle(cornerRadius: 5))
                 .overlay(RoundedRectangle(cornerRadius: 5).strokeBorder(Color(nsColor: .separatorColor)))
                 .overlay(alignment: .bottomTrailing) { actionButtons }
+                .accessibilityIdentifier("inspector-json-field")
+        }
+        /// `newValue`, never a re-read of `context.value.wrappedValue`. An `onChange` action
+        /// closure belongs to the render that registered it, so its captured context is a render
+        /// behind and answers with the value this edit has just replaced. Re-reading it made the
+        /// editor undo every keystroke and push the undone text back, at 75 rounds a second and
+        /// without ever settling (#3051).
+        .onChange(of: context.value.wrappedValue) { newValue in
+            _ = model.received(newValue)
         }
-        .onChange(of: displayText) { _ in propagateEdit() }
-        .onChange(of: context.value.wrappedValue) { _ in syncFromBinding() }
     }
 
     private var actionButtons: some View {
         HStack(spacing: 2) {
             if let onPopOut {
-                Button { onPopOut(displayText) } label: {
+                Button { onPopOut(model.displayText) } label: {
                     Image(systemName: "arrow.up.forward.app")
                         .font(.caption2)
                         .padding(4)
@@ -52,15 +72,4 @@ internal struct JsonEditorView: View {
         }
         .padding(4)
     }
-
-    private func propagateEdit() {
-        guard !context.isReadOnly,
-              JsonReindenter.normalize(displayText) != JsonReindenter.normalize(context.value.wrappedValue) else { return }
-        context.value.wrappedValue = displayText
-    }
-
-    private func syncFromBinding() {
-        guard JsonReindenter.normalize(context.value.wrappedValue) != JsonReindenter.normalize(displayText) else { return }
-        displayText = JsonReindenter.reindent(context.value.wrappedValue)
-    }
 }

TablePro/Views/RowInspector/InspectorFieldListView.swift

@@ -167,14 +167,20 @@ internal struct InspectorFieldListView: View {
         isEditable: Bool
     ) -> FieldEditorContext {
         let state = FieldValueState.resolve(field)
+        let columnIndex = field.columnIndex
         return FieldEditorContext(
             columnName: field.columnName,
             columnType: field.columnTypeEnum,
             isLongText: field.isLongText,
+            /// The getter reads the store, not the `state` resolved above. That value is a copy
+            /// taken when this context was built, so a getter closing over it answers whatever the
+            /// field held during that render, and an `onChange` action, which belongs to the
+            /// render that registered it, is a render behind again. An editor comparing its text
+            /// against that answer sees the value it had just replaced and puts it back (#3051).
             value: isEditable
                 ? Binding(
-                    get: { state.editableText },
-                    set: { editState.updateField(at: field.columnIndex, value: $0) }
+                    get: { editState.currentText(at: columnIndex) },
+                    set: { editState.updateField(at: columnIndex, value: $0) }
                 )
                 : .constant(state.editableText),
             originalValue: field.originalValue,