Skip to content

Commit

Permalink
fix(editor) Logged out handling for collaboration features. (#4774)
Browse files Browse the repository at this point in the history
- Test dispatch function now waits for 2 calls to update the project
  server state to apply before continuing.
- Added `loggedIn` parameter to `checkProjectOwned`.
- `checkProjectOwned` now handles if the user is not logged in and
  if it's running in a test environment to return true against `isOwner`.
  This is because actions need to run but at a point that does not regularly
  occur when running normally as the user is either logged in or the
  project exists locally.
- Added a `loggedIn` condition in `CollaborationStateUpdater`.
- `allowedToEditProject` and `useAllowedToEditProject` now check the login status
  of the user.
- Removed a call to `updateProjectServerStateInStore` that was running on any
  change to `SET_LOGIN_STATE`, because it was effectively running at a random
  point in time.
- `getProjectServerState` now doesn't itself handle `IS_TEST_ENVIRONMENT` as that
  handling has been pushed down into the various functions it calls.
- Added timestamps to `test-browser-full-output`.
- `getLoginState`, `checkProjectOwnership` and `fetchProjectMetadata` now all
  check `IS_TEST_ENVIRONMENT` so that they don't try to make calls to the server
  during tests.
  • Loading branch information
seanparsons authored Jan 24, 2024
1 parent 1811a2c commit 45478a0
Show file tree
Hide file tree
Showing 20 changed files with 378 additions and 190 deletions.
19 changes: 17 additions & 2 deletions editor/src/components/canvas/ui-jsx.test-utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ import {
treeToContents,
} from '../assets'
import { testStaticElementPath } from '../../core/shared/element-path.test-utils'
import { createFakeMetadataForParseSuccess } from '../../utils/utils.test-utils'
import { createFakeMetadataForParseSuccess, wait } from '../../utils/utils.test-utils'
import {
mergeWithPrevUndo,
saveDOMReport,
Expand Down Expand Up @@ -146,7 +146,10 @@ import { carryDispatchResultFields } from './editor-dispatch-flow'
import type { FeatureName } from '../../utils/feature-switches'
import { setFeatureEnabled } from '../../utils/feature-switches'
import { unpatchedCreateRemixDerivedDataMemo } from '../editor/store/remix-derived-data'
import { emptyProjectServerState } from '../editor/store/project-server-state'
import {
emptyProjectServerState,
getUpdateProjectServerStateInStoreRunCount,
} from '../editor/store/project-server-state'

// eslint-disable-next-line no-unused-expressions
typeof process !== 'undefined' &&
Expand Down Expand Up @@ -626,6 +629,9 @@ label {
</React.Profiler>,
)

// Capture how many times the project server state update has been triggered.
const beforeLoadUpdateStoreRunCount = getUpdateProjectServerStateInStoreRunCount()

await act(async () => {
await new Promise<void>((resolve, reject) => {
void load(
Expand Down Expand Up @@ -656,6 +662,15 @@ label {
})
})

// Check that the project server state update has been triggered twice, which (currently at least)
// is the number of times that it runs as a result of the above logic. Once without a project ID and
// the second time with the project ID '0', which should then mean that it has stabilised and unless other
// changes occur the `UPDATE_PROJECT_SERVER_STATE` action shouldn't be triggered anymore after that.
while (getUpdateProjectServerStateInStoreRunCount() <= beforeLoadUpdateStoreRunCount + 1) {
// eslint-disable-next-line no-await-in-loop
await wait(1)
}

return {
dispatch: async (
actions: ReadonlyArray<EditorAction>,
Expand Down
13 changes: 7 additions & 6 deletions editor/src/components/editor/editor-component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ import type { Storage, Presence, RoomEvent, UserMeta } from '../../../liveblocks
import LiveblocksProvider from '@liveblocks/yjs'
import { isRoomId, projectIdToRoomId } from '../../core/shared/multiplayer'
import { EditorModes } from './editor-modes'
import { checkIsMyProject } from './store/collaborative-editing'
import { useCanComment, useDataThemeAttributeOnBody } from '../../core/commenting/comment-hooks'
import { useDataThemeAttributeOnBody } from '../../core/commenting/comment-hooks'
import { CollaborationStateUpdater } from './store/collaboration-state'
import { GithubRepositoryCloneFlow } from '../github/github-repository-clone-flow'
import { getPermissions } from './store/permissions'
import { useIsLoggedIn } from '../../core/shared/multiplayer-hooks'

const liveModeToastId = 'play-mode-toast'

Expand Down Expand Up @@ -230,8 +230,8 @@ export const EditorComponentInner = React.memo((props: EditorProps) => {
...handleKeyDown(
event,
editorStoreRef.current.editor,
editorStoreRef.current.projectServerState,
editorStoreRef.current.userState.loginState,
editorStoreRef.current.projectServerState,
metadataRef,
navigatorTargetsRef,
namesByKey,
Expand Down Expand Up @@ -363,8 +363,6 @@ export const EditorComponentInner = React.memo((props: EditorProps) => {
[dispatch],
)

const canComment = useCanComment()

useSelectorWithCallback(
Substores.userStateAndProjectServerState,
(store) => ({ projectServerState: store.projectServerState, userState: store.userState }),
Expand Down Expand Up @@ -550,6 +548,8 @@ export function EditorComponent(props: EditorProps) {
'EditorComponent forkedFromProjectId',
)

const loggedIn = useIsLoggedIn()

const dispatch = useDispatch()

useDataThemeAttributeOnBody()
Expand All @@ -572,8 +572,9 @@ export function EditorComponent(props: EditorProps) {
projectId={projectId}
forkedFromProjectId={forkedFromProjectId}
dispatch={dispatch}
loggedIn={loggedIn}
>
<CollaborationStateUpdater projectId={projectId} dispatch={dispatch}>
<CollaborationStateUpdater projectId={projectId} dispatch={dispatch} loggedIn={loggedIn}>
<EditorComponentInner {...props} />
</CollaborationStateUpdater>
</ProjectServerStateUpdater>
Expand Down
4 changes: 2 additions & 2 deletions editor/src/components/editor/global-shortcuts.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -357,8 +357,8 @@ export function preventBrowserShortcuts(editor: EditorState, event: KeyboardEven
export function handleKeyDown(
event: KeyboardEvent,
editor: EditorState,
projectServerState: ProjectServerState,
loginState: LoginState,
projectServerState: ProjectServerState,
metadataRef: { current: ElementInstanceMetadataMap },
navigatorTargetsRef: { current: Array<NavigatorEntry> },
namesByKey: ShortcutNamesByKey,
Expand All @@ -367,7 +367,7 @@ export function handleKeyDown(
// Stop the browser from firing things like save dialogs.
preventBrowserShortcuts(editor, event)

const allowedToEdit = allowedToEditProject(projectServerState)
const allowedToEdit = allowedToEditProject(loginState, projectServerState)
const canComment = isFeatureEnabled('Multiplayer') && hasCommentPermission(loginState)

// Ensure that any key presses are appropriately recorded.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ function getNewProjectId(): Promise<string> {
return Promise.resolve(`Project_${projectCounter++}`)
}

function checkProjectOwned(_projectId: string): Promise<ProjectOwnership> {
function checkProjectOwned(_loggedIn: boolean, _projectId: string): Promise<ProjectOwnership> {
return Promise.resolve({ isOwner: true, ownerId: 'the-owner' })
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -811,9 +811,9 @@ export function createPersistenceMachine<ModelType, FileType>(
{
services: {
getNewProjectId: backendAPI.getNewProjectId,
checkProjectOwned: (_, event) => {
checkProjectOwned: (context, event) => {
if (event.type === 'BACKEND_CHECK_OWNERSHIP') {
return backendAPI.checkProjectOwned(event.projectId)
return backendAPI.checkProjectOwned(context.loggedIn, event.projectId)
} else {
throw new Error(
`Incorrect event type triggered check ownership, ${JSON.stringify(event)}`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export type ProjectOwnership = {

export interface PersistenceBackendAPI<ModelType, FileType> {
getNewProjectId: () => Promise<string>
checkProjectOwned: (projectId: string) => Promise<ProjectOwnership>
checkProjectOwned: (loggedIn: boolean, projectId: string) => Promise<ProjectOwnership>
loadProject: (projectId: string) => Promise<ProjectLoadResult<ModelType>>
saveProjectToServer: (
projectId: string,
Expand Down
18 changes: 16 additions & 2 deletions editor/src/components/editor/persistence/persistence-backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import type {
import { fileWithFileName, projectWithFileChanges } from './generic/persistence-types'
import type { PersistentModel } from '../store/editor-state'
import { isFeatureEnabled } from '../../../utils/feature-switches'
import { IS_TEST_ENVIRONMENT } from '../../../common/env-vars'

let _lastThumbnailGenerated: number = 0
const THUMBNAIL_THROTTLE = 300000
Expand Down Expand Up @@ -67,18 +68,31 @@ async function getNewProjectId(): Promise<string> {
return createNewProjectID()
}

export async function checkProjectOwned(projectId: string): Promise<ProjectOwnership> {
export async function checkProjectOwned(
loggedIn: boolean,
projectId: string,
): Promise<ProjectOwnership> {
const existsLocally = await projectIsStoredLocally(projectId)
if (existsLocally) {
return {
ownerId: null,
isOwner: true,
}
} else {
} else if (loggedIn) {
const ownerState = await checkProjectOwnership(projectId)
return ownerState === 'unowned'
? { ownerId: null, isOwner: true }
: { ownerId: ownerState.ownerId, isOwner: ownerState.isOwner }
} else if (IS_TEST_ENVIRONMENT) {
return {
ownerId: null,
isOwner: true,
}
} else {
return {
ownerId: null,
isOwner: false,
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion editor/src/components/editor/project-owner-hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export function useDisplayOwnershipWarning(): void {
Substores.fullStore,
(store) => {
return {
allowedToEdit: allowedToEditProject(store.projectServerState),
allowedToEdit: allowedToEditProject(store.userState.loginState, store.projectServerState),
projectID: store.editor.id,
}
},
Expand Down
Loading

0 comments on commit 45478a0

Please sign in to comment.