fix(coordinator): stop the row inspector's JSON field undoing every keystroke - #3056
Merged
Merged
Conversation
Member
Author
|
The whitespace defect described under Deliberately not fixed here is now tracked as #3057, with the measurements, the competitor survey and all three review rounds' findings. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3051.
What was happening
Typing one character in the row inspector's JSON field sent the app to 100% CPU for one to two minutes, and threw the character away.
Measured in an instrumented Debug build against a SQLite table holding a JSON document in a
TEXTcolumn. One keystroke, then 30 seconds of idling:JsonEditorView.bodyevaluationsSourceEditor.updateNSViewControllerpassespropagateEdit()syncFromBinding()MultiRowEditState.updateFieldupdateControllerWithStateCPU sat at 100.8%. The editor ended up showing the pre-edit text, with no modified marker.
Root cause
Two things compose. Neither hangs on its own.
The field's value
Bindinggetter read a per-render snapshot, not the store.InspectorFieldListViewbuilt it asBinding(get: { state.editableText }, ...)over aFieldValueStatecaptured by value when that render ran.An
onChangeaction closure belongs to the render that registered it, so it sees a context one render older still.JsonEditorViewwrote.onChange(of: context.value.wrappedValue) { _ in syncFromBinding() }, discarding thenewValueSwiftUI hands it and re-reading from the capturedself. In the log the same binding answers1171at24.584and952at24.587with no write in between.The field stages a compacted form of what the editor displays, so
syncFromBindingcompared the new text against the pre-edit value, decided the editor was stale, and overwrote it, dropping the keystroke.propagateEditthen pushed that back,resolvePendingValuemapped it tonil, the next snapshot read the original again, and round it went at about 75 rounds a second.The
scrollPoint:andsetFrame:frames in the crash reports are real but are passengers: the scroll arm fired zero times andRepresentableSyncPhasewasidleon every pass. They are the editor's normal per-update work, re-entered 75 times a second because the view tree above it was looping.The issue's first suspicion, that writing state from
onChangeis illegal, is not right. A probe confirms a@Statewrite insideupdateNSViewControllerlogsModifying state during view updateand is dropped, while the same write from anonChangeaction logs nothing and is handled normally. Apple's own samples write state fromonChange. The declaration is explicit that the parameter is the new value:The fix
JsonEditorViewtakes thenewValuethe change delivered and never re-reads the context.JsonFieldEditingModelholds the reconciliation rule outside the view, so the fixpoint can be driven without a window. It compares an arriving value againstlastSynced, the editor's own last published text, rather than against what the editor currently displays, which is what tells an echo from a real external change.MultiRowEditState.currentText(at:). That closes the stale-snapshot hazard for every field editor, not just JSON.Storage behaviour is unchanged: a JSON field still opens laid out and still stages the compacted form.
Verification
verify.shbuild,test,docsandlintall PASS (logs under.analysis/fix-3051-json-field-editor-loop/logs/).JsonFieldEditingModelTestsdrives the editor and the store against each other until they stop moving and asserts that takes one round, including a keystroke sequence through a transiently invalid document, the revert-to-original echo, and the Set NULL echo.InspectorJsonFieldEditUITeststypes into the field and asserts the character is still there. Chinook has no JSON column and no JSON-looking text value, so the app seeds ajson_fixturetable into the sample whenTABLEPRO_UI_TEST_SEED_JSON_TABLEis set, gated on the storage sandbox.Deliberately not fixed here
The investigation found a second, independent defect and a first attempt at it is not in this PR.
MultiRowEditState.resolvePendingValuecompacts every JSON value before it reaches the database, andisJsoncomes fromcolumnTypeEnum.isJsonType || (originalValue ?? "").looksLikeJson. That rewrites whitespace the server would have preserved on PostgreSQLjson("the json type stores an exact copy of the input text"), on MariaDB (whoseJSONis aLONGTEXTalias), on SQLite, DuckDB, libSQL, Turso and Cloudflare D1, and on any text column whose value happens to parse.I built a per-engine policy for it and reverted it. Three review rounds each found new real defects in that code, the last of which is structural: the policy answered "does this server preserve JSON source text?" from the saved connection label, and MariaDB reports its JSON columns as
"JSON", byte-identical to MySQL, so only the live server banner can tell them apart. A connection saved as MySQL can reach a MariaDB server.It needs its own design, around live driver capability rather than a static table, plus the
json[]array element path and the field-identity reuse case. Filed as a follow-up with the measurements, the competitor survey and all three rounds' findings.