-
Notifications
You must be signed in to change notification settings - Fork 425
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: adds support for Create-Studio integration #7635
base: next
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
No changes to documentation |
Component Testing Report Updated Oct 21, 2024 11:25 AM (UTC) ✅ All Tests Passed -- expand for details
|
⚡️ Editor Performance ReportUpdated Mon, 21 Oct 2024 11:28:12 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
@@ -23,4 +23,7 @@ export interface User { | |||
displayName?: string | |||
imageUrl?: string | |||
email?: string | |||
|
|||
/** global sanity user id */ | |||
sanityUserId?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field is already in the runtime object obtained from the API, its just absent from the type.
Happy to add other missing props too; there are more.
config, | ||
context: {...context, ...partialContext}, | ||
initialValue: initialDocumentActions, | ||
propertyName: 'document.actions', | ||
reducer: documentActionsReducer, | ||
}), | ||
}) | ||
return getStartInCreateSortedActions(actions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is controversial, but necessary: It forces "Start in Create" action to be first in the actions list, when present.
This again results in "Start in Create" being the primary action, and thus shown in the document footer, outside the action-overflow menu.
If a plugin or studio config filters out the action, this sort does nothing.
Alternative implementation suggestions welcome.
return { | ||
...beta?.create, | ||
startInCreateEnabled: !!beta?.create?.startInCreateEnabled, | ||
components: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice how we provide structure components from core via context here.
Downside: introduces indirection
Upside: keeps most Create integration code in a directory structure in core
import {createStartInCreateAction} from './start-in-create/StartInCreateAction' | ||
import {createAppIdCache} from './studio-app/appIdCache' | ||
|
||
export const createIntegration = definePlugin(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not everything needed for the Create integration can be expressed via the plugin API.
As much as possible has been put in here though. If anything else in this PR can be put here via existing apis, please let me know.
export const START_IN_CREATE_ACTION_NAME = | ||
'startInCreate' as unknown as DocumentActionComponent['action'] | ||
|
||
export function createStartInCreateAction(appIdCache: AppIdCache): DocumentActionComponent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, yes. Create is such a great app name :/
const isStaging = typeof __SANITY_STAGING__ !== 'undefined' && __SANITY_STAGING__ === true | ||
|
||
function getCreateBaseUrl(customHost?: string) { | ||
//@todo perhaps only support create staging through config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: think about how we want to handle this, if at all.
Is it bad to have a this sanity.build address here?
@@ -74,9 +74,11 @@ export function createUserStore({client: _client, currentUser}: UserStoreOptions | |||
|
|||
userLoader.prime('me', userFromCurrentUser) | |||
|
|||
if (userFromCurrentUser?.id) { | |||
//@todo figure out if we are ok with this removal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO/FIXME/COMMIT TO THIS:
Currently, useUser(userId)
will return a User
with sanityUserId
(global user id), unless the userId
happens to be the current user id. This is surprising.
The reason is this line of code, which puts currentUser
into the user cache. currentUser
obtained from /me
does NOT have all propereties obtained using the /user/:id
endpoint.
I make the case here that we should just delete this line, and allow the extra request, so that global token for current user can be obtained like so:
const currentUser = useCurrentUser()
const userId = currentUser?.id ?? ''
const [user] = useUser(userId)
// user.sanityUserId
If there are good reasons not to do this, I can restore this line, and add some bespoke /user/:currentUserId
fetch in the integration codepath that needs it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: We found issues with this approach and will remove the "global-id-in-url" approach.
As it turns out the user can log in with a different user in Create, resulting in the link-creation failing. Instead Create will handle all this, so we no longer need global user id in Studio.
TLDR: all code related to user.id will be reverted/removed once Create has been updated with this new behavior.
@@ -533,7 +535,8 @@ export const DocumentPaneProvider = memo((props: DocumentPaneProviderProps) => { | |||
isLocked || | |||
isDeleting || | |||
isDeleted || | |||
isLiveEditAndDraft | |||
isLiveEditAndDraft || | |||
isCreateLinked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear from the diff here, but this sets the documentPane readOnly
property.
Note: Not all codepaths respect document path readOnly
state. For instance, Paste document
can mutate a document pane in readOnly
mode. I contend that this is the right place to put this readOnly
code for Create though; code that does not respect it should be corrected elsewhere.
@@ -85,6 +88,9 @@ export function DocumentLayout() { | |||
const zOffsets = useZIndex() | |||
const previewUrl = usePreviewUrl(value) | |||
|
|||
const createLinkMetadata = getCreateLinkMetadata(value) | |||
const CreateLinkedBanner = useSanityCreateConfig().components?.documentLinkedBanner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done to avoid having to put component code belonging to the Create integration in the structure package.
const {editState, timelineStore, onChange: onDocumentChange} = useDocumentPane() | ||
const {title} = useDocumentTitle() | ||
|
||
const CreateLinkedActions = useSanityCreateConfig().components?.documentLinkedActions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done to avoid having to put component code belonging to the Create integration in the structure package.
// The "Start in Create" action must be sorted first, so we need a sort key; the action string – | ||
// we also don't want this string in the config interfaces, so we need the cheeky cast to smuggle it through | ||
export const START_IN_CREATE_ACTION_NAME = | ||
'startInCreate' as unknown as DocumentActionComponent['action'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions on alternative ways to approach this is more than welcome!
@@ -0,0 +1,32 @@ | |||
import {defineEvent} from '@sanity/telemetry' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @sanity-io/data-eng for review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Create side equivalents will land in a couple of days fyi
@@ -0,0 +1,9 @@ | |||
import {useCurrentUser, useUser} from '../store' | |||
|
|||
export function useGlobalUserId(): string | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All usage of this will be removed.
import {defineEvent} from '@sanity/telemetry' | ||
|
||
export const StartInCreateClicked = defineEvent({ | ||
name: 'Start in Create clicked', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All event names should be title cased, see "Event naming" here: https://www.notion.so/sanityio/Product-Telemetry-71b5857ac3e7489b9355317c928c789a
Description
This PR introduces the Studio side of the upcoming Create <-> Studio integration.
It allows users start their content editing journey in Create for new documents, by linking a Create document to a Studio document.
While linked, Create will sync content into the Studio document in real-time. Create linked Studio documents are read-only. Studio users can unlink the document at any point, to continue editing in the Studio. At this point Create will no longer sync content into the Studio document..
What to review
Read through this full PR text before jumping into the code.
Changes affect:
change to userStore needs extra attention(This is no longer relevant, see below)There might be some controversial changes in here. I have made PR comments everywhere I though there might be some eyebrows raised, but there might be more. I'm very much open to course corrections.
Feature details
For Sanity folks: first see video of the full Studio-Create flow here.
In Studio deployments with an exposed
create-manifest.json
, new, prestine documents will have a "Start in Sanity Create" button in the document pane footer. See previous manifest PR for context.Clicking it will open the following dialog (gfx pending):
Clicking "Learn more" will open a "How to Create<->Studio" article.
Clicking "Continue" will open a new tab which kicks you off to Create which will:
A studio document is considered "Create linked" whilst it has
_create.ejected === false
.While Create setting up the link (which entails setting the
_create
property), the following dialog will be shown after clicking "Continue" (copy pending):On the link is established, the document will be
readOnly
and the pane footer will be in a special "Create linked" mode:Clicking the info icon will open a popover (gfx pending).
Hover tooltip:
Click popover:
Clicking anywhere outside closes the popover.
Clicking "Edit in Sanity Create" will open up the Create document that points to this Studio document in a new tab.
Clicking "Unlink" will show the following dialog:
"Unlink now" will unset the
_create
property, thereby severing the link and making the document a regular Studio document. It will no longer bereadOnly
.High level implementation details
Code organization
I tried to:
For 1: this feature has some requirements that were not 100% plugin-supported, and required changes in core and structure:
For 2: I had to introduce a bit of indirection to make it possible to render stuff in structure.
SanityCreateConfigContext
contains two component implementations used by structure, provided from core.These are the components rendered when a document is Create linked. (Read only banner & Unlink actions).
For 3: I have added a BaseSchemaTypeOptions type. I always regretted not adding that for the initial v3 launch, since it allows plugins to add generic type-options much more easily. For instance, it could simplify the AI Assist type extensions quite a bit.
SanityCreateOptions
I have added
sanityCreate
options directly toBaseSchemaTypeOptions
. If it feels controversial to have them there, I can move them into a module declaration extension in core, but I feel that is only adding indirection with no upside.The idea is that
SanityCreateOptions
will be the DSL with which devs tailor their schema in Create (via the manifest file).Atm it supports
exclude
andpurpose
exclude
removes a type or field from appearing in Create. Excluded document types will not have a "Start in Sanity Create" button.purpose
supersedesdescription
, and will be used as metadata when describing the schema to an LLMWe need different options from AI Assist (which also has this exclude option), as Create has different needs than AI Assist.
Start in Create
We show the "Start in Sanity Create" button when:
beta.create.startInCreateEnabled: true
. Atm,startInCreateEnabled
defaults to true. We might want to flip this tofalse
before merging, depending on what Product wants._createdAt
)create-manifest-json
on a known url. It will only visit hosts listed under Studios in manage.documentOptions.sanityCreate.exclude !== true
Create link
A Studio document is considered linked to Create when it contains a
_create
metadata field.Specifically we check for
_create.ejected === false
. The idea is that we can keep the Create metadata around if we want to, but in this implementation we unset the full_create
field when unlinking.This ensures that this metadata does not end up in published documents.
If we want to do "soft unlinks" to keep a trace back to Create in the future (possibly opt-in via config), we can do that with with
_create.ejected: true
.Create link readOnly mode will always be enabled, regardless of what
beta.create.startInCreateEnabled
is.Getting the global user idEdit: We found issues with this approach and will remove the "global-id-in-url" approach.
As it turns out the user can log in with a different user in Create, resulting in the link-creation failing. Instead Create will handle all this, so we no longer need global user id in Studio.
I will update this PR once the Create side supports this behavior.
To build the "Start in Sanity Create" url, we need the global user id.For current user, this is currently not available – but it IS for all other userIds. I propose a change to that, by no longer pre-priming the userStore with currentUser (at the cost of one more network request).See PR code comment for change to userStore.App-id cache
To build "Start in Sanity Create" url, we need the appId (deployed studio id) from
<project>/user-applications
.It is ok to cache this info for the duration of the Studio lifetime (ie, until browser refresh).
I dont know if there are existing caching mechanisms in core I could use instead of rollinga bespoke fetch-cache for this.
Telemetry
I've added telemetry for the following actions:
Open questions
Known caveats
Create does not respect
initialValues
, so these will be nuked when Create starts syncing.TODOs before opening the PR for review
Testing
For Sanity folks: follow the instructions here before starting .
Beware: Cannot be fully tested until we release the Create side of the integration (Studio link/content mapping).
Locally this integration can be tested by running the new test studio:
dev/test-create-integration-studio
. It hasfallbackOrigin
set, and will therefor have "Start in Sanity Create" on localhost.The integration can also be tested on https://create-integration-test.sanity.studio – it has been deployed from this branch.
Notes for release
The Create <-> Studio integration needs a full documentation article. There are a lot of moving parts.
Depending on when this goes out, we might want to stealth launch it without a release note.