diff --git a/CHANGELOG.md b/CHANGELOG.md index 47a1ef1ff2..e2e438bd59 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/TablePro/Core/Services/Infrastructure/SampleDatabaseService.swift b/TablePro/Core/Services/Infrastructure/SampleDatabaseService.swift index 2dd852dd6d..52af4f5d41 100644 --- a/TablePro/Core/Services/Infrastructure/SampleDatabaseService.swift +++ b/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 { diff --git a/TablePro/Core/Services/Infrastructure/UITestJsonFixture.swift b/TablePro/Core/Services/Infrastructure/UITestJsonFixture.swift new file mode 100644 index 0000000000..08ad0ea2ef --- /dev/null +++ b/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? + 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: "''") + } +} diff --git a/TablePro/Models/UI/JsonFieldEditingModel.swift b/TablePro/Models/UI/JsonFieldEditingModel.swift new file mode 100644 index 0000000000..8b86f24408 --- /dev/null +++ b/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 + } +} diff --git a/TablePro/Models/UI/MultiRowEditState.swift b/TablePro/Models/UI/MultiRowEditState.swift index 4939ce9b47..9d330a346d 100644 --- a/TablePro/Models/UI/MultiRowEditState.swift +++ b/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 } diff --git a/TablePro/Views/RowInspector/FieldEditors/JsonEditorView.swift b/TablePro/Views/RowInspector/FieldEditors/JsonEditorView.swift index 12193d05fd..d1e83f18de 100644 --- a/TablePro/Views/RowInspector/FieldEditors/JsonEditorView.swift +++ b/TablePro/Views/RowInspector/FieldEditors/JsonEditorView.swift @@ -10,7 +10,7 @@ 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 @@ -18,7 +18,20 @@ internal struct JsonEditorView: View { 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 { + 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) - } } diff --git a/TablePro/Views/RowInspector/InspectorFieldListView.swift b/TablePro/Views/RowInspector/InspectorFieldListView.swift index 470d182009..443c1ee2a6 100644 --- a/TablePro/Views/RowInspector/InspectorFieldListView.swift +++ b/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, diff --git a/TableProTests/Models/UI/JsonFieldEditingModelTests.swift b/TableProTests/Models/UI/JsonFieldEditingModelTests.swift new file mode 100644 index 0000000000..8043a7fd49 --- /dev/null +++ b/TableProTests/Models/UI/JsonFieldEditingModelTests.swift @@ -0,0 +1,142 @@ +// +// JsonFieldEditingModelTests.swift +// TableProTests +// + +import Foundation +import TableProPluginKit +import Testing + +@testable import TablePro + +/// The regression suite for #3051. +/// +/// The editor and the store are two mirrors of one value, and the hang was a fixpoint between +/// them: one keystroke produced 6,944 body evaluations in 30 seconds at 100% CPU, and the typed +/// character was thrown away. `settle` is the shape that catches it: drive the pair until nothing +/// moves, and fail if that takes more than a round. +@MainActor +@Suite("JSON field editing model") +struct JsonFieldEditingModelTests { + private static func makeState(value: String, type: ColumnType) -> MultiRowEditState { + let state = MultiRowEditState() + state.configure( + selectedRowIndices: [0], + allRows: [[value]], + columns: ["payload"], + columnTypes: [type] + ) + return state + } + + /// Runs the editor and the store against each other the way SwiftUI does: the editor publishes, + /// the store restates, the editor is told what the store now holds, and round again. Returns + /// how many rounds it took to stop moving. + @discardableResult + private static func settle( + _ model: inout JsonFieldEditingModel, + against state: MultiRowEditState, + limit: Int = 8 + ) -> Int { + for round in 1...limit { + let delivered = state.currentText(at: 0) + guard model.received(delivered) else { return round } + guard let published = model.typed(model.displayText) else { return round } + state.updateField(at: 0, value: published) + } + Issue.record("The editor and the store never agreed within \(limit) rounds") + return limit + } + + @Test("A keystroke survives the round trip and settles in one round") + func keystrokeSettles() { + let original = "{\"a\":1}" + let state = Self.makeState(value: original, type: .json(rawType: "jsonb")) + var model = JsonFieldEditingModel(storedValue: original) + #expect(model.displayText == JsonReindenter.reindent(original)) + + let typed = "{\n \"a\": 2\n}" + let published = model.typed(typed) + #expect(published == typed) + state.updateField(at: 0, value: published) + + let rounds = Self.settle(&model, against: state) + #expect(rounds == 1) + #expect(model.displayText == typed, "the editor keeps its layout, not the compact echo") + #expect(state.fields[0].pendingValue == "{\"a\":2}") + } + + /// The document is invalid for as long as the user is halfway through typing a value, which is + /// where `JsonReindenter`'s parse-failure fallback used to make the two sides disagree. + @Test("A transiently invalid document settles at every step") + func invalidIntermediateSettles() { + let original = "{\"a\": 1}" + let state = Self.makeState(value: original, type: .text(rawType: "text")) + var model = JsonFieldEditingModel(storedValue: original) + + for typed in ["{\"a\": 1,", "{\"a\": 1, \"", "{\"a\": 1, \"b\"", "{\"a\": 1, \"b\": 2}"] { + if let published = model.typed(typed) { + state.updateField(at: 0, value: published) + } + let rounds = Self.settle(&model, against: state) + #expect(rounds == 1, "stalled on \(typed)") + #expect(model.displayText == typed, "the keystroke must survive") + } + } + + /// The store drops a pending value that matches the stored one again, so the binding answers + /// with the original. That is still this editor's own echo and must not be adopted. + @Test("Editing back to the stored value settles rather than bouncing") + func revertToOriginalSettles() { + let original = "{\"a\":1}" + let state = Self.makeState(value: original, type: .json(rawType: "jsonb")) + var model = JsonFieldEditingModel(storedValue: original) + + state.updateField(at: 0, value: model.typed("{\n \"a\": 2\n}")) + #expect(state.fields[0].hasEdit) + + state.updateField(at: 0, value: model.typed(JsonReindenter.reindent(original))) + #expect(!state.fields[0].hasEdit) + let rounds = Self.settle(&model, against: state) + #expect(rounds == 1) + } + + @Test("The editor's own write is never adopted back") + func ownWriteIsNotAdopted() { + var model = JsonFieldEditingModel(storedValue: "{\"a\":1}") + let typed = "{\n \"a\": 2\n}" + let published = model.typed(typed) + #expect(published == typed) + let adopted = model.received("{\"a\":2}") + #expect(!adopted, "the compact echo of what was just typed") + #expect(model.displayText == typed, "the caret's layout must not be rewritten") + } + + @Test("A value the editor did not write is adopted") + func externalChangeIsAdopted() { + var model = JsonFieldEditingModel(storedValue: "{\"a\":1}") + let adopted = model.received("{\"b\":2}") + #expect(adopted) + #expect(model.displayText == JsonReindenter.reindent("{\"b\":2}")) + } + + /// Set NULL, Set DEFAULT and Set EMPTY all empty the field's editable text. Adopting that and + /// then publishing it again would clear the state the user just asked for. + @Test("An emptied field is adopted once and not published back") + func emptiedFieldIsNotRepublished() { + var model = JsonFieldEditingModel(storedValue: "{\"a\":1}") + let adopted = model.received("") + #expect(adopted) + #expect(model.displayText.isEmpty) + let republished = model.typed(model.displayText) + #expect(republished == nil, "adopting is not an edit") + } + + @Test("Text that does not parse is compared exactly, not reported equal") + func unparseableComparesExactly() { + var model = JsonFieldEditingModel(storedValue: "{\"a\": ") + #expect(model.displayText == "{\"a\": ", "text the parser refuses is shown as it is") + let adopted = model.received("{\"b\": ") + #expect(adopted) + } +} diff --git a/TableProUITests/RowInspector/InspectorJsonFieldEditUITests.swift b/TableProUITests/RowInspector/InspectorJsonFieldEditUITests.swift new file mode 100644 index 0000000000..fbbbe93ef6 --- /dev/null +++ b/TableProUITests/RowInspector/InspectorJsonFieldEditUITests.swift @@ -0,0 +1,94 @@ +// +// InspectorJsonFieldEditUITests.swift +// TableProUITests +// +// The row inspector's JSON field kept two mirrors of one value in two textual forms and +// reconciled them from an `onChange` action that re-read a context a render out of date. Typing +// one character made the editor put the previous value back and push it into the store again, at +// about 75 rounds a second: one keystroke produced 6,944 body evaluations in 30 seconds at 100% +// CPU, and the character was thrown away (#3051). +// +// So the assertion is not "the app is responsive", which is hard to state and easy to pass by +// accident. It is that the character is still there afterwards, which the loop could not manage. +// + +import XCTest + +final class InspectorJsonFieldEditUITests: UITestCase { + /// Chinook has no JSON column and no text value that parses as a JSON object, so the app seeds + /// one when this variable is set. Both sides spell the name out because a UI test target cannot + /// import the app. + private let jsonFixtureVariable = "TABLEPRO_UI_TEST_SEED_JSON_TABLE" + private let fixtureTable = "json_fixture" + private let typedMarker = "ZZTOP" + + func testTypingInTheJsonFieldKeepsWhatWasTyped() throws { + let app = try launchWithSampleDatabase(environment: [jsonFixtureVariable: "1"]) + let window = app.windows.firstMatch + + try openFixtureTable(in: app) + showInspector(in: app) + + let grid = window.tables.matching(identifier: "data-grid").firstMatch + XCTAssertTrue(grid.waitToExist(timeout: 30), "The fixture table must produce a result grid") + XCTAssertTrue( + waitForPredicate(timeout: 30) { grid.tableRows.firstMatch.exists }, + "The fixture table must return a row" + ) + gridPoint(in: grid, of: window, dy: 12).click() + + let field = window.textViews.matching(identifier: "inspector-json-field").firstMatch + XCTAssertTrue(field.waitToExist(timeout: 20), "The JSON column must open the JSON editor") + + let before = field.value as? String ?? "" + XCTAssertTrue(before.contains("\"name\""), "The fixture document must be in the editor, got '\(before)'") + + field.click() + app.typeText(typedMarker) + + XCTAssertTrue( + waitForPredicate(timeout: 20) { (field.value as? String)?.contains(self.typedMarker) == true }, + "Every typed character must survive; the editor holds '\(field.value as? String ?? "nil")'" + ) + + /// The loop put the pre-edit text back a moment after the keystroke, so one check could + /// pass on timing alone. Letting the run loop turn and asking again is what separates + /// "typed" from "kept". + _ = waitForPredicate(timeout: 3) { false } + XCTAssertTrue( + (field.value as? String)?.contains(typedMarker) ?? false, + "The typed text must still be there a moment later; the editor holds " + + "'\(field.value as? String ?? "nil")'" + ) + } + + // MARK: - Helpers + + private func openFixtureTable(in app: XCUIApplication) throws { + let browser = app.windows.firstMatch.outlines.firstMatch + XCTAssertTrue(browser.waitToExist(timeout: 30), "The object browser must list the sample's tables") + + let table = browser.staticTexts[fixtureTable].firstMatch + XCTAssertTrue( + table.waitToExist(timeout: 30), + "The seeded fixture table must appear in the object browser" + ) + table.doubleClick() + } + + /// The inspector remembers whether it was open, so the starting state is whatever the previous + /// launch left. The View menu item reads Hide Inspector once it is showing, which is the only + /// handle on that state. + private func showInspector(in app: XCUIApplication) { + let menuBar = app.menuBars.firstMatch + XCTAssertTrue(menuBar.waitToExist(timeout: 10)) + menuBar.menuBarItems["View"].click() + + let show = menuBar.menuItems["Show Inspector"] + if show.waitToExist(timeout: 5) { + show.click() + return + } + app.typeKey(.escape, modifierFlags: []) + } +}