[fix] Don't synchronize isReadOnly (#1396)

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

<!-- 💡 Indicate the type of change your pull request is. -->
<!-- 🤷‍♀️ If you're not sure, don't select anything -->
<!-- ✂️ Feel free to delete unselected options -->

<!-- To select one, put an x in the box: [x] -->

- [ ] `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.
This commit is contained in:
David Sheldrick 2023-05-17 11:45:43 +01:00 committed by GitHub
parent 01487be6fa
commit a3896fc492
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 75 additions and 27 deletions

View file

@ -32,6 +32,11 @@ export function generateSharedScripts(bublic: '<rootDir>' | '<rootDir>/bublic')
test: { test: {
baseCommand: 'yarn run -T jest', baseCommand: 'yarn run -T jest',
runsAfter: { 'refresh-assets': {} }, runsAfter: { 'refresh-assets': {} },
cache: {
inputs: {
exclude: ['*.tsbuildinfo'],
},
},
}, },
'test-coverage': { 'test-coverage': {
baseCommand: 'yarn run -T jest --coverage', baseCommand: 'yarn run -T jest --coverage',
@ -42,7 +47,7 @@ export function generateSharedScripts(bublic: '<rootDir>' | '<rootDir>/bublic')
runsAfter: { 'build-types': {} }, runsAfter: { 'build-types': {} },
cache: { cache: {
inputs: { inputs: {
exclude: ['**/*.tsbuildinfo'], exclude: ['*.tsbuildinfo'],
}, },
}, },
}, },

View file

@ -456,7 +456,7 @@ export class App extends EventEmitter<TLEventMap> {
// (undocumented) // (undocumented)
setPenMode(isPenMode: boolean): this; setPenMode(isPenMode: boolean): this;
setProp(key: TLShapeProp, value: any, ephemeral?: boolean, squashing?: boolean): this; setProp(key: TLShapeProp, value: any, ephemeral?: boolean, squashing?: boolean): this;
// (undocumented) // @internal (undocumented)
setReadOnly(isReadOnly: boolean): this; setReadOnly(isReadOnly: boolean): this;
setScribble(scribble?: null | TLScribble): this; setScribble(scribble?: null | TLScribble): this;
setSelectedIds(ids: TLShapeId[], squashing?: boolean): this; setSelectedIds(ids: TLShapeId[], squashing?: boolean): this;

View file

@ -1564,20 +1564,21 @@ export class App extends EventEmitter<TLEventMap> {
return this return this
} }
get isReadOnly() { private _isReadOnly = atom<boolean>('isReadOnly', false as any)
return this.userDocumentSettings.isReadOnly
}
/** @internal */
setReadOnly(isReadOnly: boolean): this { setReadOnly(isReadOnly: boolean): this {
if (isReadOnly !== this.isReadOnly) { this._isReadOnly.set(isReadOnly)
this.updateUserDocumentSettings({ isReadOnly }, true) if (isReadOnly) {
if (isReadOnly) { this.setSelectedTool('hand')
this.setSelectedTool('hand')
}
} }
return this return this
} }
get isReadOnly() {
return this._isReadOnly.value
}
/** @internal */ /** @internal */
private _isPenMode = atom<boolean>('isPenMode', false as any) private _isPenMode = atom<boolean>('isPenMode', false as any)

View file

@ -408,7 +408,8 @@ describe('When in readonly mode', () => {
props: { opacity: '1', w: 100, h: 100, url: '', doesResize: false }, 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', () => { it('Begins editing embed when double clicked', () => {

View file

@ -274,7 +274,7 @@ describe('creating groups', () => {
// │ A │ │ B │ │ C │ // │ A │ │ B │ │ C │
// └───┘ └───┘ └───┘ // └───┘ └───┘ └───┘
app.createShapes([box(ids.boxA, 0, 0), box(ids.boxB, 20, 0), box(ids.boxC, 40, 0)]) 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() app.selectAll()
expect(app.selectedIds.length).toBe(3) expect(app.selectedIds.length).toBe(3)
app.groupShapes() app.groupShapes()
@ -485,8 +485,8 @@ describe('ungrouping shapes', () => {
expect(app.selectedIds.length).toBe(3) expect(app.selectedIds.length).toBe(3)
app.groupShapes() app.groupShapes()
expect(app.selectedIds.length).toBe(1) expect(app.selectedIds.length).toBe(1)
app.setReadOnly(true)
app.updateUserDocumentSettings({ isReadOnly: true })
app.ungroupShapes() app.ungroupShapes()
expect(app.selectedIds.length).toBe(1) expect(app.selectedIds.length).toBe(1)
expect(onlySelectedShape().type).toBe(TLGroupUtil.type) expect(onlySelectedShape().type).toBe(TLGroupUtil.type)

View file

@ -1290,8 +1290,6 @@ export interface TLUserDocument extends BaseRecord<'user_document'> {
// (undocumented) // (undocumented)
isPenMode: boolean; isPenMode: boolean;
// (undocumented) // (undocumented)
isReadOnly: boolean;
// (undocumented)
isSnapMode: boolean; isSnapMode: boolean;
// (undocumented) // (undocumented)
lastUpdatedPageId: ID<TLPage> | null; lastUpdatedPageId: ID<TLPage> | null;

View file

@ -6,7 +6,7 @@ import { videoAssetMigrations } from './assets/TLVideoAsset'
import { instanceTypeMigrations } from './records/TLInstance' import { instanceTypeMigrations } from './records/TLInstance'
import { instancePageStateMigrations } from './records/TLInstancePageState' import { instancePageStateMigrations } from './records/TLInstancePageState'
import { rootShapeTypeMigrations, TLShape } from './records/TLShape' import { rootShapeTypeMigrations, TLShape } from './records/TLShape'
import { userDocumentTypeMigrations } from './records/TLUserDocument' import { userDocumentTypeMigrations, userDocumentVersions } from './records/TLUserDocument'
import { userPresenceTypeMigrations } from './records/TLUserPresence' import { userPresenceTypeMigrations } from './records/TLUserPresence'
import { storeMigrations } from './schema' import { storeMigrations } from './schema'
import { arrowShapeMigrations } from './shapes/TLArrowShape' 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 --- */ /* --- PUT YOUR MIGRATIONS TESTS ABOVE HERE --- */
for (const migrator of allMigrators) { for (const migrator of allMigrators) {

View file

@ -14,7 +14,6 @@ import { TLUserId } from './TLUser'
*/ */
export interface TLUserDocument extends BaseRecord<'user_document'> { export interface TLUserDocument extends BaseRecord<'user_document'> {
userId: TLUserId userId: TLUserId
isReadOnly: boolean
isPenMode: boolean isPenMode: boolean
isGridMode: boolean isGridMode: boolean
isDarkMode: boolean isDarkMode: boolean
@ -35,7 +34,6 @@ export const userDocumentTypeValidator: T.Validator<TLUserDocument> = T.model(
typeName: T.literal('user_document'), typeName: T.literal('user_document'),
id: idValidator<TLUserDocumentId>('user_document'), id: idValidator<TLUserDocumentId>('user_document'),
userId: userIdValidator, userId: userIdValidator,
isReadOnly: T.boolean,
isPenMode: T.boolean, isPenMode: T.boolean,
isGridMode: T.boolean, isGridMode: T.boolean,
isDarkMode: T.boolean, isDarkMode: T.boolean,
@ -49,20 +47,21 @@ export const userDocumentTypeValidator: T.Validator<TLUserDocument> = T.model(
// --- MIGRATIONS --- // --- MIGRATIONS ---
// STEP 1: Add a new version number here, give it a meaningful name. // STEP 1: Add a new version number here, give it a meaningful name.
// It should be 1 higher than the current version // It should be 1 higher than the current version
const Versions = { export const userDocumentVersions = {
Initial: 0, Initial: 0,
AddSnapMode: 1, AddSnapMode: 1,
AddMissingIsMobileMode: 2, AddMissingIsMobileMode: 2,
RemoveIsReadOnly: 3,
} as const } as const
/** @public */ /** @public */
export const userDocumentTypeMigrations = defineMigrations({ export const userDocumentTypeMigrations = defineMigrations({
firstVersion: Versions.Initial, firstVersion: userDocumentVersions.Initial,
// STEP 2: Update the current version to point to your latest version // 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 // STEP 3: Add an up+down migration for the new version here
migrators: { migrators: {
[Versions.AddSnapMode]: { [userDocumentVersions.AddSnapMode]: {
up: (userDocument: TLUserDocument) => { up: (userDocument: TLUserDocument) => {
return { ...userDocument, isSnapMode: false } return { ...userDocument, isSnapMode: false }
}, },
@ -70,7 +69,7 @@ export const userDocumentTypeMigrations = defineMigrations({
return userDocument return userDocument
}, },
}, },
[Versions.AddMissingIsMobileMode]: { [userDocumentVersions.AddMissingIsMobileMode]: {
up: (userDocument: TLUserDocument) => { up: (userDocument: TLUserDocument) => {
return { ...userDocument, isMobileMode: userDocument.isMobileMode ?? false } return { ...userDocument, isMobileMode: userDocument.isMobileMode ?? false }
}, },
@ -78,6 +77,14 @@ export const userDocumentTypeMigrations = defineMigrations({
return userDocument 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 */ /* STEP 4: Add your changes to the record type */
@ -91,7 +98,6 @@ export const TLUserDocument = createRecordType<TLUserDocument>('user_document',
}).withDefaultProperties( }).withDefaultProperties(
(): Omit<TLUserDocument, 'id' | 'typeName' | 'userId'> => ({ (): Omit<TLUserDocument, 'id' | 'typeName' | 'userId'> => ({
/* STEP 6: Add any new default values for properties here */ /* STEP 6: Add any new default values for properties here */
isReadOnly: false,
isPenMode: false, isPenMode: false,
isGridMode: false, isGridMode: false,
isDarkMode: false, isDarkMode: false,

View file

@ -181,9 +181,7 @@ function DebugMenuContent({
<DropdownMenu.Item <DropdownMenu.Item
onClick={() => { onClick={() => {
// We need to do this manually because `updateUserDocumentSettings` does not allow toggling `isReadOnly`) // We need to do this manually because `updateUserDocumentSettings` does not allow toggling `isReadOnly`)
app.store.put([ app.setReadOnly(!app.isReadOnly)
{ ...app.userDocumentSettings, isReadOnly: !app.userDocumentSettings.isReadOnly },
])
}} }}
> >
<span>Toggle read-only</span> <span>Toggle read-only</span>