view history #158
Open
view history #158
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideImplements versioned collaboration history by extending protobuf definitions, propagating document version through the sync protocol, persisting and fetching version snapshots via HTTP and IndexedDB, and integrating rollback capabilities in the client. ER diagram for new collab_versions table in IndexedDBerDiagram
collab_versions {
string viewId
string versionId
string parentId
string name
date createdAt
string[] uids
Uint8Array snapshot
}
collab_versions ||--o{ collab_versions : parentId
Class diagram for new and updated collaboration typesclassDiagram
class messages.HttpRealtimeMessage {
+string deviceId
+Uint8Array payload
+static create(properties)
+static encode(message, writer)
+static encodeDelimited(message, writer)
+static decode(reader, length)
+static decodeDelimited(reader)
+static verify(message)
+static fromObject(object)
+static toObject(message, options)
+toJSON()
+static getTypeUrl(typeUrlPrefix)
}
class collab.SyncRequest {
+collab.IRid lastMessageId
+Uint8Array stateVector
+string version
}
class collab.Update {
+collab.IRid messageId
+number flags
+Uint8Array payload
+string version
}
class collab.CollabDocStateParams {
+string objectId
+number collabType
+PayloadCompressionType compression
+Uint8Array sv
+Uint8Array docState
+static create(properties)
+static encode(message, writer)
+static encodeDelimited(message, writer)
+static decode(reader, length)
+static decodeDelimited(reader)
+static verify(message)
+static fromObject(object)
+static toObject(message, options)
+toJSON()
+static getTypeUrl(typeUrlPrefix)
}
class collab.PayloadCompressionType {
+NONE
+ZSTD
}
messages.HttpRealtimeMessage --> collab.SyncRequest
collab.SyncRequest --> collab.Update
collab.CollabDocStateParams --> collab.PayloadCompressionType
Class diagram for VersionedDoc and related typesclassDiagram
class VersionedDoc {
+Y.Doc doc
+string version
}
class CollabVersionRecord {
+string viewId
+string versionId
+string parentId
+string name
+Date createdAt
+Uint8Array snapshot
}
class EncodedCollab {
+Uint8Array stateVector
+Uint8Array docState
+string version
}
VersionedDoc --> CollabVersionRecord
VersionedDoc --> EncodedCollab
Class diagram for CollabHistoryService interfaceclassDiagram
class CollabHistoryService {
+getCollabHistory(workspaceId, viewId, since): Promise<CollabVersionRecord[]>
+createCollabVersion(workspaceId, viewId, name, snapshot): Promise<string>
+deleteCollabVersion(workspaceId, viewId, versionId): Promise<void>
+revertCollabVersion(workspaceId, viewId, collabType, versionId): Promise<EncodedCollab>
}
CollabHistoryService --> CollabVersionRecord
CollabHistoryService --> EncodedCollab
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the version-reset and document reinitialization logic in useSync’s effect into a separate helper or custom hook to reduce complexity and improve readability.
- The WebSocket and BroadcastChannel message handlers are nearly identical—consolidate them or extract a shared handler to avoid duplication and potential drift.
- The massive protobuf codegen changes are drowning the manual logic—please separate autogenerated file updates into their own commit or ignore them in reviews so the core feature diff is clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the version-reset and document reinitialization logic in useSync’s effect into a separate helper or custom hook to reduce complexity and improve readability.
- The WebSocket and BroadcastChannel message handlers are nearly identical—consolidate them or extract a shared handler to avoid duplication and potential drift.
- The massive protobuf codegen changes are drowning the manual logic—please separate autogenerated file updates into their own commit or ignore them in reviews so the core feature diff is clearer.
## Individual Comments
### Comment 1
<location> `src/application/db/tables/versions.ts:15-16` </location>
<code_context>
+ }>;
+};
+
+export const versionSchema = {
+ collab_versions: 'version',
+};
\ No newline at end of file
</code_context>
<issue_to_address>
**issue (bug_risk):** The primary key for collab_versions should match the actual key used in the table.
The schema defines 'version' as the primary key, while the table uses 'versionId'. Please update the schema to use 'versionId' for consistency and to prevent potential runtime errors.
</issue_to_address>
### Comment 2
<location> `src/application/services/js-services/history.ts:200` </location>
<code_context>
+ // Merge both arrays, using versionId as unique identifier
+ // Track records that need database updates
+ const versionMap = new Map<string, CollabVersion>();
+ const versions = await db.collab_versions.filter(v => v.viewId !== viewId).toArray();
+
+ let lastUpdate: Date|undefined;
</code_context>
<issue_to_address>
**issue (bug_risk):** The filter for collab_versions excludes the current viewId, which may be unintended.
The filter currently excludes all records matching the current viewId. If you intend to retrieve versions for this viewId, update the filter to 'v.viewId === viewId'.
</issue_to_address>
### Comment 3
<location> `src/application/services/js-services/history.ts:259-260` </location>
<code_context>
+ const toSnapshot = Y.decodeSnapshot(version.snapshot);
+
+ // get first non-deleted parent
+ let parent = null;
+ let current = versionMap.get(version.versionId);
+
+ while (current && current.parentId !== null) {
</code_context>
<issue_to_address>
**issue (bug_risk):** The parent lookup logic may not traverse the version chain correctly.
The loop should use 'current.parentId' to traverse the parent chain, rather than always referencing 'version.versionId'. This will ensure correct parent lookup.
</issue_to_address>
### Comment 4
<location> `src/application/services/js-services/history.ts:171` </location>
<code_context>
+const VERSION_EXPIRY_DAYS = 7;
+
+const cleanupExpiredVersions = async (versions: Map<string, CollabVersion>) => {
+ const expirationDate = Date.now() - (VERSION_EXPIRY_DAYS * 1000 * 60 * 60 * 24);
+ const toDelete = [];
+
</code_context>
<issue_to_address>
**nitpick:** VERSION_EXPIRY_DAYS is documented as 30 but set to 7.
Please ensure the comment and value for VERSION_EXPIRY_DAYS are consistent to avoid confusion.
</issue_to_address>
### Comment 5
<location> `src/application/services/js-services/history.ts:196` </location>
<code_context>
+ * it will let collab versions fill the information about which users made changes between specific version
+ * and its predecessor.
+ */
+export const collabVersions = async (workspaceId: string, viewId: string, users: Y.PermanentUserData|null) => {
+ // Merge both arrays, using versionId as unique identifier
+ // Track records that need database updates
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider adding concurrency control for collabVersions cache updates.
Concurrent updates to the cache for the same viewId may cause race conditions. Implement a locking mechanism or queue to ensure updates are serialized.
Suggested implementation:
```typescript
const collabVersionsLocks: Map<string, Promise<void>> = new Map();
export const collabVersions = async (workspaceId: string, viewId: string, users: Y.PermanentUserData|null) => {
// Acquire lock for this viewId to serialize cache updates
let releaseLock: (() => void) | null = null;
const lockPromise = new Promise<void>(resolve => { releaseLock = resolve; });
while (collabVersionsLocks.has(viewId)) {
// Wait for previous lock to release
await collabVersionsLocks.get(viewId);
}
collabVersionsLocks.set(viewId, lockPromise);
try {
// Merge both arrays, using versionId as unique identifier
// Track records that need database updates
const versionMap = new Map<string, CollabVersion>();
const versions = await db.collab_versions.filter(v => v.viewId !== viewId).toArray();
let lastUpdate: Date|undefined;
for (const version of versions) {
versionMap.set(version.versionId, version);
if ((lastUpdate?.getTime() || 0) < version.createdAt.getTime()) {
lastUpdate = version.createdAt;
}
}
// ... rest of your function logic ...
} finally {
// Release lock
collabVersionsLocks.delete(viewId);
releaseLock && releaseLock();
}
```
If collabVersions is called from multiple places, ensure all calls use the same locking logic.
If you use a worker/threaded environment, consider a more robust cross-process locking solution.
</issue_to_address>
### Comment 6
<location> `src/application/services/js-services/history.ts:250-259` </location>
<code_context>
+ // Update database if needed
</code_context>
<issue_to_address>
**suggestion (bug_risk):** BulkPut may overwrite existing records without merging user IDs.
If user IDs are added over time, merge new IDs with existing ones in the uids array to prevent data loss.
</issue_to_address>
### Comment 7
<location> `src/components/ws/useSync.ts:62` </location>
<code_context>
+ }
+}
+
export const useSync = (ws: AppflowyWebSocketType, bc: BroadcastChannelType, eventEmitter?: EventEmitter): SyncContextType => {
const { sendMessage, lastMessage } = ws;
const { postMessage, lastBroadcastMessage } = bc;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the complex in-hook logic into dedicated helper utilities and hooks to isolate concerns and simplify the main hook.
Consider pulling all of the in-`useEffect` logic out into small, focused helpers. For example, you can:
1. Extract your “apply message + version reset” flow into a single async util.
2. Extract your broadcast listener into its own hook.
3. Extract your revert flow into its own hook.
Here’s a sketch of what that might look like:
```ts
// src/utils/collab.ts
import { handleMessage } from '@/application/services/js-services/sync-protocol';
import { deleteDB, openCollabDB } from '@/application/db';
import * as awarenessProtocol from 'y-protocols/awareness';
import { versionChanged } from '@/hooks/useSync'; // keep your version check here
export async function applyCollabMessage(
message: ICollabMessage,
contexts: Map<string, SyncContext>,
register: (c: RegisterSyncContext) => SyncContext,
currentUser?: User
): Promise<UpdateCollabInfo|void> {
const objectId = message.objectId!;
let ctx = contexts.get(objectId);
if (!ctx) return;
if (versionChanged(ctx, message)) {
const newVersion = message.update?.version || message.syncRequest?.version || null;
ctx.doc.emit('reset', [ctx, newVersion]);
ctx.doc.destroy();
// remove and re-open IndexedDB
await deleteDB(objectId);
const { doc } = await openCollabDB(objectId, {
expectedVersion: newVersion,
currentUser: currentUser?.uid
});
const awareness = new awarenessProtocol.Awareness(doc);
ctx = register({ doc: awareness.doc, awareness, collabType: ctx.collabType, version: newVersion });
}
handleMessage(ctx, message);
const ts = message.update?.messageId?.timestamp;
return {
objectId,
publishedAt: ts ? new Date(ts) : undefined,
collabType: message.collabType as Types
};
}
```
```ts
// src/hooks/useSync.ts (excerpt)
import { applyCollabMessage } from '@/utils/collab';
useEffect(() => {
const msg = lastMessage?.collabMessage;
if (!msg) return;
let alive = true;
applyCollabMessage(msg, registeredContexts.current, registerSyncContext, currentUser)
.then(info => {
if (alive && info) setLastUpdatedCollab(info);
})
.catch(console.error);
return () => { alive = false; };
}, [lastMessage, currentUser, registerSyncContext]);
```
```ts
// src/hooks/useBroadcastCollab.ts
import { handleMessage } from '@/application/services/js-services/sync-protocol';
export function useBroadcastCollab(
lastBroadcast: ICollabMessage|undefined,
contexts: Ref<Map<string,SyncContext>>,
onUpdate: (info: UpdateCollabInfo) => void
) {
useEffect(() => {
const msg = lastBroadcast;
if (!msg) return;
const ctx = contexts.current.get(msg.objectId!);
if (!ctx) return;
handleMessage(ctx, msg);
onUpdate({
objectId: msg.objectId!,
publishedAt: msg.update?.messageId?.timestamp
? new Date(msg.update.messageId.timestamp)
: undefined,
collabType: msg.collabType as Types
});
}, [lastBroadcast, contexts, onUpdate]);
}
```
```ts
// src/hooks/useRevertCollabVersion.ts
import { deleteDB } from 'lib0/indexeddb';
import * as http from '@/application/services/js-services/http/http_api';
import { openCollabDB } from '@/application/db';
import * as awarenessProtocol from 'y-protocols/awareness';
export function useRevertCollabVersion(
contexts: Ref<Map<string,SyncContext>>,
register: (c: RegisterSyncContext) => SyncContext,
currentUser?: User
) {
return useCallback(async (viewId: string, version: string) => {
if (!currentUser) return;
await deleteDB(viewId);
const ctx = contexts.current.get(viewId);
if (!ctx) return;
const { docState } = await http.revertCollabVersion(
currentUser.latestWorkspaceId,
viewId,
ctx.collabType,
version
);
ctx.doc.emit('reset', [ctx, version]);
ctx.doc.destroy();
const { doc } = await openCollabDB(viewId, {
expectedVersion: version,
currentUser: currentUser.uid
});
Y.applyUpdate(doc, docState);
const awareness = new awarenessProtocol.Awareness(doc);
register({ doc: awareness.doc, awareness, collabType: ctx.collabType, version });
}, [contexts, register, currentUser]);
}
```
These changes shrink the big `useSync` hook down to three simple effects/callbacks and keep each concern isolated.
</issue_to_address>
### Comment 8
<location> `src/proto/messages.js:299-304` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 9
<location> `src/proto/messages.js:2210-2215` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 10
<location> `src/application/services/js-services/http/http_api.ts:827` </location>
<code_context>
const data = response.data;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {data} = response;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+15
to
+16
| export const versionSchema = { | ||
| collab_versions: 'version', |
There was a problem hiding this comment.
issue (bug_risk): The primary key for collab_versions should match the actual key used in the table.
The schema defines 'version' as the primary key, while the table uses 'versionId'. Please update the schema to use 'versionId' for consistency and to prevent potential runtime errors.
Horusiath
force-pushed
the
page-history
branch
from
bc0f8d5 to
8e5d659
Compare
Horusiath
force-pushed
the
page-history
branch
from
8e5d659 to
736d1eb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Implement full view history feature by pairing new HTTP endpoints and IndexedDB storage to manage collaboration version snapshots. Extend the sync layer and protobuf definitions to carry version metadata, enable version-specific document persistence in openCollabDB, and add UI hooks to fetch and revert document versions.
New Features:
Enhancements:
Tests: