Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): inherit readOnly state from ancestors in copyPaste function #7643

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

pedrobonamin
Copy link
Contributor

@pedrobonamin pedrobonamin commented Oct 21, 2024

Description

When pasting values into a field which inherits the readOnly from the parent this was not verified., so it was possible to paste into read only fields.

This PR adds a function resolveReadOnlyAncestor to check if any ancestor of the path in which the user is pasting is read only, if it is, it won't allow pasting.

E.g, pasting into inheritedReadOnlyField was allowed, the same is happening with arrays.

    {
      name: 'readOnlyObject',
      title: 'Read only object',
      type: 'object',
      readOnly: true,
      fields: [
        {
          name: 'selfDefinedReadOnlyField',
          title: 'Read only field',
          description: 'ReadOnly defined in field',
          type: 'string',
          readOnly: true,
        },
        {
          name: 'inheritedReadOnlyField',
          title: 'Read only field',
          description: 'ReadOnly inherited from object',
          type: 'string',
        },
      ],
    }
Screen.Recording.2024-10-21.at.18.11.24.mov

What to review

Is the implementation correct? Accessing the parents using the resolveReadOnlyAncestor is that in line with the existing approaches?

Testing

New tests have been added into the copy paste to guard this functionality.
You can test it manually visiting https://test-next-studio-git-sdx-1643.sanity.dev/test/structure/playlist;7d12a507-75ef-4e28-ace6-95c237092e2b and trying to copy-paste values into the disabled fields

Notes for release

fixes an issue in copy paste in which the readOnly state was not correctly inherited to the children fields.

@pedrobonamin pedrobonamin requested a review from a team as a code owner October 21, 2024 16:15
@pedrobonamin pedrobonamin requested review from binoy14 and sjelfull and removed request for a team and binoy14 October 21, 2024 16:15
Copy link

vercel bot commented Oct 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 4:19pm
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 4:19pm
test-compiled-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 4:19pm
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 4:19pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 4:19pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Oct 21, 2024 4:19pm

Copy link
Contributor

No changes to documentation

Copy link
Contributor

Component Testing Report Updated Oct 21, 2024 4:23 PM (UTC)

✅ All Tests Passed -- expand for details
File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 45s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 8s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 30s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 40s 11 7 0
formBuilder/inputs/PortableText/copyPaste/CopyPasteFields.spec.tsx ✅ Passed (Inspect) 0s 0 12 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 18s 6 0 0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx ✅ Passed (Inspect) 10s 3 0 0
formBuilder/inputs/PortableText/DragAndDrop.spec.tsx ✅ Passed (Inspect) 2m 32s 1 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 45s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 40s 21 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 18s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 9s 3 9 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 26s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 18s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 35s 12 0 0
formBuilder/tree-editing/TreeEditing.spec.tsx ✅ Passed (Inspect) 0s 0 3 0
formBuilder/tree-editing/TreeEditingNestedObjects.spec.tsx ✅ Passed (Inspect) 0s 0 3 0

Copy link
Contributor

⚡️ Editor Performance Report

Updated Mon, 21 Oct 2024 16:26:40 GMT

Benchmark reference
latency of sanity@latest
experiment
latency of this branch
Δ (%)
latency difference
article (title) 16.4 efps (61ms) 16.7 efps (60ms) -1ms (-1.6%)
article (body) 53.5 efps (19ms) 59.5 efps (17ms) -2ms (-10.2%)
article (string inside object) 17.2 efps (58ms) 17.4 efps (58ms) -1ms (-0.9%)
article (string inside array) 14.1 efps (71ms) 14.3 efps (70ms) -1ms (-1.4%)
recipe (name) 30.3 efps (33ms) 30.3 efps (33ms) +0ms (-/-%)
recipe (description) 35.7 efps (28ms) 35.7 efps (28ms) +0ms (-/-%)
recipe (instructions) 99.9+ efps (6ms) 99.9+ efps (6ms) +0ms (-/-%)
synthetic (title) 14.5 efps (69ms) 14.5 efps (69ms) +0ms (-/-%)
synthetic (string inside object) 14.6 efps (69ms) 15.6 efps (64ms) -5ms (-6.6%)

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) 61ms 67ms 74ms 133ms 482ms 14.2s
article (body) 19ms 20ms 23ms 196ms 227ms 5.8s
article (string inside object) 58ms 61ms 73ms 190ms 266ms 8.7s
article (string inside array) 71ms 75ms 86ms 199ms 983ms 9.8s
recipe (name) 33ms 38ms 68ms 74ms 3ms 9.0s
recipe (description) 28ms 31ms 36ms 51ms 0ms 5.8s
recipe (instructions) 6ms 8ms 9ms 11ms 0ms 3.6s
synthetic (title) 69ms 75ms 85ms 372ms 1601ms 15.6s
synthetic (string inside object) 69ms 73ms 84ms 176ms 1229ms 9.8s

🧪 Experiment result

The performance result of this branch

Benchmark latency p75 p90 p99 blocking time test duration
article (title) 60ms 61ms 73ms 211ms 329ms 13.9s
article (body) 17ms 19ms 25ms 170ms 366ms 5.5s
article (string inside object) 58ms 60ms 64ms 182ms 370ms 9.1s
article (string inside array) 70ms 74ms 76ms 186ms 941ms 9.7s
recipe (name) 33ms 35ms 57ms 71ms 7ms 8.7s
recipe (description) 28ms 30ms 36ms 71ms 0ms 5.9s
recipe (instructions) 6ms 8ms 9ms 9ms 0ms 3.2s
synthetic (title) 69ms 73ms 81ms 422ms 1569ms 15.8s
synthetic (string inside object) 64ms 68ms 83ms 705ms 1444ms 10.1s

📚 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.

Copy link
Member

@sjelfull sjelfull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants