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

feat(core): fetch structure tool settings from backend #5901

Merged
merged 6 commits into from Mar 7, 2024

Conversation

cngonzalez
Copy link
Member

@cngonzalez cngonzalez commented Mar 5, 2024

Description

Scaffolds a Dataloader that hits an API endpoint for user settings -- in this case, specifically those settings relevant to structure tools.

This is meant to be just a scaffold. In the following day, please expect:

  • unit tests for hooks to handle malformed or null data
  • any additional error handling that can be called out here
  • possible fallbacks for situations if the API is not available.

What to review

The Dataloader backend specifically -- does it work as we expect? Any gotchas? Are errors being handled correctly?

Testing

End-to-end tests are already in place. They've been refactored since they referred to localStorage changes.

Copy link

vercel bot commented Mar 5, 2024

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

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Mar 7, 2024 5:01pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 7, 2024 5:01pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Mar 7, 2024 5:01pm

Copy link
Contributor

github-actions bot commented Mar 5, 2024

No changes to documentation

Copy link
Contributor

github-actions bot commented Mar 5, 2024

Component Testing Report Updated Mar 7, 2024 5:05 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 31s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 11s 3 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 12s 4 2 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 12s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 31s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 59s 15 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 0s 18 0 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 12s 6 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 19s 9 0 0

import {type KeyValueStore, type KeyValueStoreValue} from './types'

/** @internal */
export function createKeyValueStore(): KeyValueStore {
const storageBackend = resolveBackend()
Copy link
Member Author

Choose a reason for hiding this comment

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

@jtpetty has recommended we fall back to localStorage if the API is not available here. I've not included it in this PR since I didn't find a trivial way to deal with the asynchronous state of pinging the backend from within the resourceCache context. Hopefully it's possible.

@cngonzalez cngonzalez marked this pull request as ready for review March 6, 2024 14:04
@cngonzalez cngonzalez requested a review from a team as a code owner March 6, 2024 14:04
Copy link
Contributor

@binoy14 binoy14 left a comment

Choose a reason for hiding this comment

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

LGTM one nit pick comment and question regarding failing test

'{"by":[{"field":"name","direction":"asc"}],"extendedProjection":"name"}',
)
const nameResult = await sanityClient.request({
uri: `/users/me/keyvalue/${SORT_KEY}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 probably need to set version to vX to fix the failing tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh maybe thinking about it more, is it because it's hitting prod?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do the e2e tests in CI take place in staging?

Copy link
Contributor

Choose a reason for hiding this comment

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

no they are against production

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought so! Can certainly change the API version if we want to, but that'll be (hopefully) be a short lived change (and won't currently fix tests 😅 )

@cngonzalez cngonzalez requested a review from a team as a code owner March 7, 2024 16:57
@cngonzalez cngonzalez requested review from ninaandal and removed request for a team March 7, 2024 16:57
@cngonzalez cngonzalez changed the base branch from feat/fetch-settings-from-backend to next March 7, 2024 17:01
@cngonzalez cngonzalez changed the base branch from next to feat/fetch-settings-from-backend March 7, 2024 17:01
Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@babel/preset-env@7.24.0 environment Transitive: filesystem, unsafe +105 9.46 MB nicolo-ribaudo
npm/@babel/traverse@7.24.0 Transitive: environment +13 5.85 MB nicolo-ribaudo
npm/@codemirror/autocomplete@6.13.0 Transitive: environment +6 2.57 MB marijn
npm/@codemirror/view@6.25.0 None +1 1.53 MB marijn
npm/@sanity/block-tools@3.32.0 None 0 439 kB ricokahler
npm/@sanity/client@6.15.1 None +4 1.97 MB sanity-io
npm/@sanity/diff@3.32.0 None +1 757 kB ricokahler
npm/@sanity/export@3.32.0 filesystem +12 2.95 MB ricokahler
npm/@sanity/import@3.32.0 filesystem, network Transitive: environment +6 1.66 MB ricokahler
npm/@sanity/migrate@3.32.0 network +11 10.7 MB ricokahler
npm/@sanity/mutator@3.32.0 None +3 1.24 MB ricokahler
npm/@sanity/portable-text-editor@3.32.0 environment +13 6.58 MB ricokahler
npm/@sanity/presentation@1.11.3 None +17 9.27 MB sanity-io
npm/@sanity/react-loader@1.9.2 Transitive: environment +6 2.93 MB sanity-io
npm/@sanity/schema@3.32.0 None +10 3.54 MB ricokahler
npm/@sanity/types@3.32.0 None +8 2.71 MB ricokahler
npm/@sanity/util@3.32.0 None +9 2.86 MB ricokahler
npm/@sanity/vision@3.32.0 Transitive: environment +26 10.2 MB ricokahler
npm/@sanity/visual-editing@1.6.0 environment +8 5.61 MB sanity-io
npm/sanity@3.32.0 Transitive: environment, filesystem, network, unsafe +91 63.7 MB ricokahler

🚮 Removed packages: npm/@babel/preset-env@7.23.9, npm/@babel/traverse@7.23.9, npm/@codemirror/autocomplete@6.12.0, npm/@codemirror/view@6.24.1, npm/@sanity/block-tools@3.30.1, npm/@sanity/cli@3.30.1, npm/@sanity/client@6.15.0, npm/@sanity/diff@3.30.1, npm/@sanity/export@3.30.1, npm/@sanity/import@3.30.1, npm/@sanity/migrate@3.30.1, npm/@sanity/mutator@3.30.1, npm/@sanity/portable-text-editor@3.30.1, npm/@sanity/presentation@1.11.2, npm/@sanity/react-loader@1.9.0, npm/@sanity/schema@3.30.1, npm/@sanity/types@3.30.1, npm/@sanity/util@3.30.1, npm/@sanity/vision@3.30.1, npm/@sanity/visual-editing@1.5.2, npm/sanity@3.30.1

View full report↗︎

@cngonzalez cngonzalez merged commit 9c1d928 into feat/fetch-settings-from-backend Mar 7, 2024
37 of 39 checks passed
@cngonzalez cngonzalez deleted the sdx-1031 branch March 7, 2024 17:12
github-merge-queue bot pushed a commit that referenced this pull request Mar 14, 2024
* feat(core): fetch structure tool settings from backend (#5901)

* feat(core): store recent search history in backend (#5940)

* feat(core): fetch inspect mode from backend (#5938)

* chore: update studio version for production-ready cellar (#5993)

* fix: prevent key value store from firing setKey for each open subscription (#5997)

* fix: update tests to match api (#6000)

* fix: update tests to match api

* fix: comment out flaky tests for now

* chore: remove unused resolve logic
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.

None yet

3 participants