From a3896fc49247c6901048d9600c0c894414dbc4d5 Mon Sep 17 00:00:00 2001 From: David Sheldrick Date: Wed, 17 May 2023 11:45:43 +0100 Subject: [PATCH] [fix] Don't synchronize isReadOnly (#1396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We were storing the state of whether or not a document is read-only in the store. It does not need to be stored there, and it was creating consistency problems for us, so let's not store it in there. fixes https://github.com/tldraw/brivate/issues/1864 ### Change Type - [ ] `patch` — Bug Fix - [ ] `minor` — New Feature - [x] `major` — Breaking Change - [ ] `dependencies` — Dependency Update (publishes a `patch` release, for devDependencies use `internal`) - [ ] `documentation` — Changes to the documentation only (will not publish a new version) - [ ] `tests` — Changes to any testing-related code only (will not publish a new version) - [ ] `internal` — Any other changes that don't affect the published package (will not publish a new version) ### Test Plan 1. Create a multiplayer room 2. Create a read-only link for the room 3. Paste the link into a new browser tab (not incognito, needs to have the same session state) 4. Check the room is read-only in the new tab 5. Check the room is still writable in the previous tab. ### Release Notes - Removes the isReadOnly value from the `user_document_settings` record type. --- lazy.config.ts | 7 +++- packages/editor/api-report.md | 2 +- packages/editor/src/lib/app/App.ts | 17 ++++---- .../src/lib/test/tools/TLSelectTool.test.ts | 3 +- .../editor/src/lib/test/tools/groups.test.ts | 4 +- packages/tlschema/api-report.md | 2 - packages/tlschema/src/migrations.test.ts | 41 ++++++++++++++++++- .../tlschema/src/records/TLUserDocument.ts | 22 ++++++---- packages/ui/src/lib/components/DebugPanel.tsx | 4 +- 9 files changed, 75 insertions(+), 27 deletions(-) diff --git a/lazy.config.ts b/lazy.config.ts index abb3d47b3..98c3fe9a5 100644 --- a/lazy.config.ts +++ b/lazy.config.ts @@ -32,6 +32,11 @@ export function generateSharedScripts(bublic: '' | '/bublic') test: { baseCommand: 'yarn run -T jest', runsAfter: { 'refresh-assets': {} }, + cache: { + inputs: { + exclude: ['*.tsbuildinfo'], + }, + }, }, 'test-coverage': { baseCommand: 'yarn run -T jest --coverage', @@ -42,7 +47,7 @@ export function generateSharedScripts(bublic: '' | '/bublic') runsAfter: { 'build-types': {} }, cache: { inputs: { - exclude: ['**/*.tsbuildinfo'], + exclude: ['*.tsbuildinfo'], }, }, }, diff --git a/packages/editor/api-report.md b/packages/editor/api-report.md index b9a066275..ed82044ee 100644 --- a/packages/editor/api-report.md +++ b/packages/editor/api-report.md @@ -456,7 +456,7 @@ export class App extends EventEmitter { // (undocumented) setPenMode(isPenMode: boolean): this; setProp(key: TLShapeProp, value: any, ephemeral?: boolean, squashing?: boolean): this; - // (undocumented) + // @internal (undocumented) setReadOnly(isReadOnly: boolean): this; setScribble(scribble?: null | TLScribble): this; setSelectedIds(ids: TLShapeId[], squashing?: boolean): this; diff --git a/packages/editor/src/lib/app/App.ts b/packages/editor/src/lib/app/App.ts index 1a0005ced..0b3b6a690 100644 --- a/packages/editor/src/lib/app/App.ts +++ b/packages/editor/src/lib/app/App.ts @@ -1564,20 +1564,21 @@ export class App extends EventEmitter { return this } - get isReadOnly() { - return this.userDocumentSettings.isReadOnly - } + private _isReadOnly = atom('isReadOnly', false as any) + /** @internal */ setReadOnly(isReadOnly: boolean): this { - if (isReadOnly !== this.isReadOnly) { - this.updateUserDocumentSettings({ isReadOnly }, true) - if (isReadOnly) { - this.setSelectedTool('hand') - } + this._isReadOnly.set(isReadOnly) + if (isReadOnly) { + this.setSelectedTool('hand') } return this } + get isReadOnly() { + return this._isReadOnly.value + } + /** @internal */ private _isPenMode = atom('isPenMode', false as any) diff --git a/packages/editor/src/lib/test/tools/TLSelectTool.test.ts b/packages/editor/src/lib/test/tools/TLSelectTool.test.ts index b40114663..5f1fcf717 100644 --- a/packages/editor/src/lib/test/tools/TLSelectTool.test.ts +++ b/packages/editor/src/lib/test/tools/TLSelectTool.test.ts @@ -408,7 +408,8 @@ describe('When in readonly mode', () => { props: { opacity: '1', w: 100, h: 100, url: '', doesResize: false }, }, ]) - app.updateUserDocumentSettings({ isReadOnly: true }) + app.setReadOnly(true) + app.setSelectedTool('select') }) it('Begins editing embed when double clicked', () => { diff --git a/packages/editor/src/lib/test/tools/groups.test.ts b/packages/editor/src/lib/test/tools/groups.test.ts index f7a1b6fb4..90696bd48 100644 --- a/packages/editor/src/lib/test/tools/groups.test.ts +++ b/packages/editor/src/lib/test/tools/groups.test.ts @@ -274,7 +274,7 @@ describe('creating groups', () => { // │ A │ │ B │ │ C │ // └───┘ └───┘ └───┘ app.createShapes([box(ids.boxA, 0, 0), box(ids.boxB, 20, 0), box(ids.boxC, 40, 0)]) - app.updateUserDocumentSettings({ isReadOnly: true }) + app.setReadOnly(true) app.selectAll() expect(app.selectedIds.length).toBe(3) app.groupShapes() @@ -485,8 +485,8 @@ describe('ungrouping shapes', () => { expect(app.selectedIds.length).toBe(3) app.groupShapes() expect(app.selectedIds.length).toBe(1) + app.setReadOnly(true) - app.updateUserDocumentSettings({ isReadOnly: true }) app.ungroupShapes() expect(app.selectedIds.length).toBe(1) expect(onlySelectedShape().type).toBe(TLGroupUtil.type) diff --git a/packages/tlschema/api-report.md b/packages/tlschema/api-report.md index 3ed19ad00..bfe1d04a6 100644 --- a/packages/tlschema/api-report.md +++ b/packages/tlschema/api-report.md @@ -1290,8 +1290,6 @@ export interface TLUserDocument extends BaseRecord<'user_document'> { // (undocumented) isPenMode: boolean; // (undocumented) - isReadOnly: boolean; - // (undocumented) isSnapMode: boolean; // (undocumented) lastUpdatedPageId: ID | null; diff --git a/packages/tlschema/src/migrations.test.ts b/packages/tlschema/src/migrations.test.ts index 8a35f6ebf..74ad8228e 100644 --- a/packages/tlschema/src/migrations.test.ts +++ b/packages/tlschema/src/migrations.test.ts @@ -6,7 +6,7 @@ import { videoAssetMigrations } from './assets/TLVideoAsset' import { instanceTypeMigrations } from './records/TLInstance' import { instancePageStateMigrations } from './records/TLInstancePageState' import { rootShapeTypeMigrations, TLShape } from './records/TLShape' -import { userDocumentTypeMigrations } from './records/TLUserDocument' +import { userDocumentTypeMigrations, userDocumentVersions } from './records/TLUserDocument' import { userPresenceTypeMigrations } from './records/TLUserPresence' import { storeMigrations } from './schema' import { arrowShapeMigrations } from './shapes/TLArrowShape' @@ -665,6 +665,45 @@ describe('Adding check-box to geo shape', () => { }) }) +describe('Removing isReadOnly from user_document', () => { + const { up, down } = userDocumentTypeMigrations.migrators[userDocumentVersions.RemoveIsReadOnly] + const prev = { + id: 'user_document:123', + typeName: 'user_document', + userId: 'user:123', + isReadOnly: false, + isPenMode: false, + isGridMode: false, + isDarkMode: false, + isMobileMode: false, + isSnapMode: false, + lastUpdatedPageId: null, + lastUsedTabId: null, + } + + const next = { + id: 'user_document:123', + typeName: 'user_document', + userId: 'user:123', + isPenMode: false, + isGridMode: false, + isDarkMode: false, + isMobileMode: false, + isSnapMode: false, + lastUpdatedPageId: null, + lastUsedTabId: null, + } + + test('up removes the isReadOnly property', () => { + expect(up(prev)).toEqual(next) + }) + test('down adds the isReadOnly property', () => { + expect(down(next)).toEqual(prev) + }) +}) + +/* --- PUT YOU + /* --- PUT YOUR MIGRATIONS TESTS ABOVE HERE --- */ for (const migrator of allMigrators) { diff --git a/packages/tlschema/src/records/TLUserDocument.ts b/packages/tlschema/src/records/TLUserDocument.ts index b6f767b00..eaaa61004 100644 --- a/packages/tlschema/src/records/TLUserDocument.ts +++ b/packages/tlschema/src/records/TLUserDocument.ts @@ -14,7 +14,6 @@ import { TLUserId } from './TLUser' */ export interface TLUserDocument extends BaseRecord<'user_document'> { userId: TLUserId - isReadOnly: boolean isPenMode: boolean isGridMode: boolean isDarkMode: boolean @@ -35,7 +34,6 @@ export const userDocumentTypeValidator: T.Validator = T.model( typeName: T.literal('user_document'), id: idValidator('user_document'), userId: userIdValidator, - isReadOnly: T.boolean, isPenMode: T.boolean, isGridMode: T.boolean, isDarkMode: T.boolean, @@ -49,20 +47,21 @@ export const userDocumentTypeValidator: T.Validator = T.model( // --- MIGRATIONS --- // STEP 1: Add a new version number here, give it a meaningful name. // It should be 1 higher than the current version -const Versions = { +export const userDocumentVersions = { Initial: 0, AddSnapMode: 1, AddMissingIsMobileMode: 2, + RemoveIsReadOnly: 3, } as const /** @public */ export const userDocumentTypeMigrations = defineMigrations({ - firstVersion: Versions.Initial, + firstVersion: userDocumentVersions.Initial, // STEP 2: Update the current version to point to your latest version - currentVersion: Versions.AddMissingIsMobileMode, + currentVersion: userDocumentVersions.RemoveIsReadOnly, // STEP 3: Add an up+down migration for the new version here migrators: { - [Versions.AddSnapMode]: { + [userDocumentVersions.AddSnapMode]: { up: (userDocument: TLUserDocument) => { return { ...userDocument, isSnapMode: false } }, @@ -70,7 +69,7 @@ export const userDocumentTypeMigrations = defineMigrations({ return userDocument }, }, - [Versions.AddMissingIsMobileMode]: { + [userDocumentVersions.AddMissingIsMobileMode]: { up: (userDocument: TLUserDocument) => { return { ...userDocument, isMobileMode: userDocument.isMobileMode ?? false } }, @@ -78,6 +77,14 @@ export const userDocumentTypeMigrations = defineMigrations({ return userDocument }, }, + [userDocumentVersions.RemoveIsReadOnly]: { + up: ({ isReadOnly: _, ...userDocument }: TLUserDocument & { isReadOnly: boolean }) => { + return userDocument + }, + down: (userDocument: TLUserDocument) => { + return { ...userDocument, isReadOnly: false } + }, + }, }, }) /* STEP 4: Add your changes to the record type */ @@ -91,7 +98,6 @@ export const TLUserDocument = createRecordType('user_document', }).withDefaultProperties( (): Omit => ({ /* STEP 6: Add any new default values for properties here */ - isReadOnly: false, isPenMode: false, isGridMode: false, isDarkMode: false, diff --git a/packages/ui/src/lib/components/DebugPanel.tsx b/packages/ui/src/lib/components/DebugPanel.tsx index 3e8ff30cc..974accdc6 100644 --- a/packages/ui/src/lib/components/DebugPanel.tsx +++ b/packages/ui/src/lib/components/DebugPanel.tsx @@ -181,9 +181,7 @@ function DebugMenuContent({ { // We need to do this manually because `updateUserDocumentSettings` does not allow toggling `isReadOnly`) - app.store.put([ - { ...app.userDocumentSettings, isReadOnly: !app.userDocumentSettings.isReadOnly }, - ]) + app.setReadOnly(!app.isReadOnly) }} > Toggle read-only