You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains 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
#8077 introduced a regression that broke input components that were calling onChange() as their initial useEffect was run. We do consider it an antipattern to modify the document as a side effect of opening it, but there are still some rare but valid cases for it, so we need it to work.
This PR fixes the problem by overwriting the patchRef ref in a useInsertionEffect instead of a useEffect so that the ref will be set before child components run their initial effects. The fact that we have to do this in the first place is not great, so this diff should be considered a workaround for an edge case, and is something we should look into landing a better fix for in the future.
efps — editor "frames per second". The number of updates assumed to be possible within a second.
Derived from input latency. efps = 1000 / input_latency
Detailed information
🏠 Reference result
The performance result of sanity@latest
Benchmark
latency
p75
p90
p99
blocking time
test duration
article (title)
52ms
68ms
92ms
551ms
1425ms
11.9s
article (body)
20ms
25ms
71ms
289ms
498ms
6.5s
article (string inside object)
42ms
43ms
47ms
174ms
294ms
7.0s
article (string inside array)
47ms
48ms
57ms
208ms
604ms
7.9s
recipe (name)
23ms
25ms
28ms
49ms
0ms
8.2s
recipe (description)
21ms
23ms
26ms
43ms
0ms
5.0s
recipe (instructions)
7ms
9ms
11ms
49ms
20ms
3.3s
synthetic (title)
56ms
59ms
64ms
308ms
1186ms
13.4s
synthetic (string inside object)
53ms
57ms
66ms
461ms
1307ms
9.0s
🧪 Experiment result
The performance result of this branch
Benchmark
latency
p75
p90
p99
blocking time
test duration
article (title)
43ms
68ms
85ms
521ms
1115ms
11.6s
article (body)
22ms
24ms
31ms
208ms
313ms
6.2s
article (string inside object)
40ms
44ms
50ms
160ms
161ms
6.9s
article (string inside array)
45ms
47ms
60ms
243ms
503ms
7.9s
recipe (name)
21ms
23ms
25ms
47ms
0ms
7.8s
recipe (description)
20ms
20ms
23ms
43ms
1ms
4.8s
recipe (instructions)
7ms
8ms
10ms
12ms
0ms
3.3s
synthetic (title)
54ms
57ms
64ms
340ms
667ms
12.4s
synthetic (string inside object)
49ms
52ms
64ms
98ms
231ms
8.0s
📚 Glossary
column definitions
benchmark — the name of the test, e.g. "article", followed by the label of the field being measured, e.g. "(title)".
latency — the time between when a key was pressed and when it was rendered. derived from a set of samples. the median (p50) is shown to show the most common latency.
p75 — the 75th percentile of the input latency in the test run. 75% of the sampled inputs in this benchmark were processed faster than this value. this provides insight into the upper range of typical performance.
p90 — the 90th percentile of the input latency in the test run. 90% of the sampled inputs were faster than this. this metric helps identify slower interactions that occurred less frequently during the benchmark.
p99 — the 99th percentile of the input latency in the test run. only 1% of sampled inputs were slower than this. this represents the worst-case scenarios encountered during the benchmark, useful for identifying potential performance outliers.
blocking time — the total time during which the main thread was blocked, preventing user input and UI updates. this metric helps identify performance bottlenecks that may cause the interface to feel unresponsive.
test duration — how long the test run took to complete.
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.
Description
#8077 introduced a regression that broke input components that were calling
onChange()
as their initial useEffect was run. We do consider it an antipattern to modify the document as a side effect of opening it, but there are still some rare but valid cases for it, so we need it to work.This PR fixes the problem by overwriting the
patchRef
ref in auseInsertionEffect
instead of auseEffect
so that the ref will be set before child components run their initial effects. The fact that we have to do this in the first place is not great, so this diff should be considered a workaround for an edge case, and is something we should look into landing a better fix for in the future.What to review
I've added a debug schema that calls
onChange
in a useEffect, before the fix is applied, you can see that it breaks both creating new documents and openig existing ones here: https://test-studio-bmgob5t04.sanity.dev/test/structure/input-debug;patchOnMountDebugHere's the same document type after the fix: https://test-studio-7d5t064qa.sanity.dev/test/structure/input-debug;patchOnMountDebug
Testing
Manual for now.
Notes for release