feat: support database undo redo history - #402
Open
appflowy wants to merge 9 commits into
Open
Conversation
Reviewer's GuideImplements a unified Yjs-based database/row undo-redo system with hotkey handling, central history management, and comprehensive Playwright BDD tests, while ensuring relation-cell edits are excluded from history and that all UI changes (cells, rows, fields, filters, sorts, groups, calculations) participate in a global history timeline. Sequence diagram for grid undo hotkey using unified historysequenceDiagram
actor User
participant GridProvider
participant useDatabaseRowHistoryHotkeys
participant useDatabaseHistory
participant DatabaseHistoryManager
participant DatabaseHistorySourceController as RowHistorySource
participant YUndoManager as Y_UndoManager
User->>GridProvider: keydown (Cmd/Ctrl+Z)
GridProvider->>useDatabaseRowHistoryHotkeys: useDatabaseRowHistoryHotkeys(undefined, { enabled, useLatest: true })
useDatabaseRowHistoryHotkeys->>useDatabaseHistory: useDatabaseHistory(undefined)
useDatabaseHistory->>DatabaseHistoryManager: getOrCreateDatabaseHistoryManager(databaseDoc)
useDatabaseHistory-->>useDatabaseRowHistoryHotkeys: { canUndo, undo, redo }
useDatabaseRowHistoryHotkeys->>useDatabaseRowHistoryHotkeys: handleKeyDown(event)
useDatabaseRowHistoryHotkeys->>DatabaseHistoryManager: undo()
DatabaseHistoryManager->>RowHistorySource: undo()
RowHistorySource->>Y_UndoManager: undo()
Y_UndoManager-->>RowHistorySource: StackItem
RowHistorySource-->>DatabaseHistoryManager: StackItem
DatabaseHistoryManager-->>useDatabaseRowHistoryHotkeys: updated canUndo/canRedo
useDatabaseRowHistoryHotkeys-->>GridProvider: re-render via subscription
GridProvider-->>User: grid UI updated (cell/row/field/etc.)
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 - I've found 1 issue, and left some high level feedback:
- In
useDatabaseRowHistoryHotkeys, theuseEffectdepends oncanUndo/canRedo, causing the globalkeydownlistener to be torn down and re-attached on every history state change; consider using refs or reading from the history manager inside the handler to avoid this churn while still using the latest state. - The
executeDatabaseOperationshelper always wraps operations inconsole.time/console.timeEnd, which will log in production; consider guarding these with an environment check or removing them to avoid noisy logs in non-dev environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useDatabaseRowHistoryHotkeys`, the `useEffect` depends on `canUndo`/`canRedo`, causing the global `keydown` listener to be torn down and re-attached on every history state change; consider using refs or reading from the history manager inside the handler to avoid this churn while still using the latest state.
- The `executeDatabaseOperations` helper always wraps operations in `console.time/console.timeEnd`, which will log in production; consider guarding these with an environment check or removing them to avoid noisy logs in non-dev environments.
## Individual Comments
### Comment 1
<location path="src/application/database-yjs/history.ts" line_range="61-62" />
<code_context>
+ scope: Y.AbstractType<Y.YMapEvent<unknown>>,
+ readonly rowId?: RowId
+ ) {
+ this.undoManager = new Y.UndoManager(scope, {
+ trackedOrigins: new Set([DatabaseHistoryOrigin, DatabaseRowHistoryOrigin]),
+ captureTimeout: 0,
+ });
</code_context>
<issue_to_address>
**issue (bug_risk):** The undo manager `trackedOrigins` set won’t match the origin instances you pass into transactions, so history entries will not be recorded as intended.
In `DatabaseHistorySourceController`, `trackedOrigins` is a `Set` of classes (`DatabaseHistoryOrigin`, `DatabaseRowHistoryOrigin`), but `runDatabaseAction` / `runDatabaseRowAction` pass *instances* returned from `createDatabaseHistoryOrigin` / `createDatabaseRowHistoryOrigin` into `doc.transact`. Yjs compares `origin` by identity, so those instances will never match the class values in `trackedOrigins`, and nothing will be captured by this undo manager.
You can either:
- Drop `trackedOrigins` and just use origins for grouping, or
- Use shared origin objects/symbols that you both put in `trackedOrigins` and pass to `transact` (one shared instance per origin type if you need to distinguish row vs database).
As written, the undo manager’s stacks will stay empty, so undo/redo will not work as intended.
</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
+61
to
+62
| this.undoManager = new Y.UndoManager(scope, { | ||
| trackedOrigins: new Set([DatabaseHistoryOrigin, DatabaseRowHistoryOrigin]), |
There was a problem hiding this comment.
issue (bug_risk): The undo manager trackedOrigins set won’t match the origin instances you pass into transactions, so history entries will not be recorded as intended.
In DatabaseHistorySourceController, trackedOrigins is a Set of classes (DatabaseHistoryOrigin, DatabaseRowHistoryOrigin), but runDatabaseAction / runDatabaseRowAction pass instances returned from createDatabaseHistoryOrigin / createDatabaseRowHistoryOrigin into doc.transact. Yjs compares origin by identity, so those instances will never match the class values in trackedOrigins, and nothing will be captured by this undo manager.
You can either:
- Drop
trackedOriginsand just use origins for grouping, or - Use shared origin objects/symbols that you both put in
trackedOriginsand pass totransact(one shared instance per origin type if you need to distinguish row vs database).
As written, the undo manager’s stacks will stay empty, so undo/redo will not work as intended.
…story # Conflicts: # .github/workflows/playwright-test.yml # src/application/database-yjs/__tests__/field-type-conversion.test.tsx # src/application/database-yjs/__tests__/useAddDatabaseView.test.tsx # src/application/database-yjs/dispatch.ts # src/application/database-yjs/dispatch/cell.ts # src/application/database-yjs/dispatch/group.ts # src/application/database-yjs/dispatch/relation.ts # src/application/database-yjs/dispatch/row.ts # src/application/database-yjs/dispatch/sort-filter.ts # src/components/database/DatabaseContext.tsx # src/components/database/DatabaseViews.tsx # src/components/database/grid/GridProvider.tsx
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Undo/redo behavior contract
History records committed local database actions. A logical action that changes the database document and one or more row documents is one history entry.
Captured (can undo/redo)
Not captured (cannot undo/redo from database history)
Excluded actions, remote changes, and no-op actions do not consume history and do not clear an existing redo stack.
Row create/delete boundary
Row create and row delete are supported on both Web and Desktop. Undo/redo restores local row membership/order and retained row data. Relation prefill, backlinks, and cleanup in other rows or databases are outside the database history transaction, so undoing a row lifecycle action does not reconstruct or remove those external Relation side effects.
History and shortcuts
Cmd/Ctrl+Zundoes,Cmd/Ctrl+Shift+Zredoes, and non-macOS also supportsCtrl+Y.Validation
pnpm lintpnpm exec jest --runInBand --no-coverage— 155 suites / 1,839 tests passedgit diff --checkDesign references
Summary by Sourcery
Implement database-scoped undo and redo history with consistent shortcuts, policy-based exclusions, atomic cross-document actions, and comprehensive validation.
New Features:
Bug Fixes:
Enhancements:
CI:
Tests: