[PE-31] feat: Add lock unlock archive restore realtime sync #5629
Conversation
WalkthroughThe changes in this pull request introduce several enhancements across various components and hooks related to collaborative editing. Key modifications include the addition of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Palanikannan1437
force-pushed
the
fix/lock-unlock-realtime
branch
from
da0dd08 to
c94fff9
Compare
Palanikannan1437
force-pushed
the
fix/lock-unlock-realtime
branch
from
5fa9160 to
cf69d96
Compare
Palanikannan1437
force-pushed
the
fix/lock-unlock-realtime
branch
from
cf69d96 to
c322122
Compare
Palanikannan1437
force-pushed
the
fix/lock-unlock-realtime
branch
from
1d6259b to
b8124a4
Compare
Palanikannan1437
force-pushed
the
fix/lock-unlock-realtime
branch
from
b8124a4 to
6f13c19
Compare
Palanikannan1437
force-pushed
the
fix/lock-unlock-realtime
branch
from
5d34895 to
52090f7
Compare
Palanikannan1437
force-pushed
the
fix/lock-unlock-realtime
branch
from
52090f7 to
6c9bcf1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (23)
packages/editor/src/core/types/document-events.ts (2)
3-4: Add JSDoc comments to improve type documentation.While the types are well-structured, adding JSDoc comments would make their purpose clearer to other developers.
Consider adding documentation like this:
+/** + * Type representing client-side document events extracted from DocumentRealtimeEvents. + * Used for handling real-time updates on the client side. + */ export type TDocumentEventsClient = (typeof DocumentRealtimeEvents)[DocumentEventKey]["client"]; +/** + * Type representing server-side document events extracted from DocumentRealtimeEvents. + * Used for handling real-time updates on the server side. + */ export type TDocumentEventsServer = (typeof DocumentRealtimeEvents)[DocumentEventKey]["server"];
6-9: Consider using a union type for the event parameter.To improve type safety, consider restricting the
eventparameter to valid event names instead of usingstring.Here's a suggested improvement:
+/** + * Interface for document event emitter with strongly-typed events and payloads. + */ export type TDocumentEventEmitter = { - on: (event: string, callback: (message: { payload: TDocumentEventsClient }) => void) => void; - off: (event: string, callback: (message: { payload: TDocumentEventsClient }) => void) => void; + on: (event: keyof typeof DocumentRealtimeEvents, callback: (message: { payload: TDocumentEventsClient }) => void) => void; + off: (event: keyof typeof DocumentRealtimeEvents, callback: (message: { payload: TDocumentEventsClient }) => void) => void; };This change ensures that only valid event names can be used when subscribing to or unsubscribing from events.
packages/editor/src/core/helpers/document-events.ts (1)
12-19: Consider using Object.entries() for better performanceWhile the current implementation works correctly, it could be more efficient and cleaner using modern JavaScript methods.
Here's a suggested improvement:
-export const getServerEventName = (clientEvent: TDocumentEventsClient): TDocumentEventsServer | undefined => { - for (const key in DocumentRealtimeEvents) { - if (DocumentRealtimeEvents[key as DocumentEventKey].client === clientEvent) { - return DocumentRealtimeEvents[key as DocumentEventKey].server; - } - } - return undefined; -}; +export const getServerEventName = (clientEvent: TDocumentEventsClient): TDocumentEventsServer | undefined => + Object.entries(DocumentRealtimeEvents) + .find(([_, value]) => value.client === clientEvent)?.[1] + .server;This version:
- Is more concise and functional
- Avoids type casting
- Uses built-in array methods
- Maintains the same functionality
live/src/core/hocuspocus-server.ts (1)
62-69: Consider rate limiting for real-time events.For operations like lock/unlock and archive/restore, consider implementing rate limiting to prevent potential abuse or performance issues from rapid state changes.
Suggestions:
- Implement a cooldown period for state-changing operations
- Consider using a rate limiter for broadcast operations
- Potentially batch updates if multiple state changes occur in quick succession
Example implementation approach:
import rateLimit from 'some-rate-limit-library'; const rateLimiter = rateLimit({ windowMs: 1000, // 1 second window maxRequests: 5 // max 5 state changes per second }); async onStateless({ payload, document }) { if (!rateLimiter.tryAcquire()) { console.warn('Rate limit exceeded for stateless events'); return; } // ... rest of the implementation }packages/editor/src/core/hooks/use-read-only-collaborative-editor.ts (2)
Line range hint
34-49: Consider security implications of storing user data in the tokenThe entire user object is being stringified and sent as a token. This might expose sensitive user information. Consider creating a minimal token with only the necessary user information.
-token: JSON.stringify(user), +token: JSON.stringify({ id: user.id, name: user.name }),
Line range hint
1-90: Consider implementing rate limiting for real-time syncSince this hook is part of a real-time synchronization system, consider implementing rate limiting to prevent excessive server load during rapid changes. This could be particularly important when multiple users are collaborating simultaneously.
Some suggestions:
- Implement debouncing for sync operations
- Add a backoff mechanism for reconnection attempts
- Consider batch updates for certain operations
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx (1)
35-37: Consider a more selective revalidation strategyEnabling all SWR revalidation options (
revalidateIfStale,revalidateOnFocus,revalidateOnReconnect) might lead to unnecessary API calls and potential performance issues. Consider:
- Using WebSocket events for real-time updates instead of polling
- Being more selective about which events trigger revalidation
For real-time page updates, a more efficient approach would be:
- Use
revalidateIfStale: truefor initial data freshness- Use WebSocket events to trigger manual revalidation (
mutate) when specific actions occur- Keep
revalidateOnFocusandrevalidateOnReconnectasfalseto avoid unnecessary API callspackages/editor/src/core/extensions/custom-image/components/image-node.tsx (2)
32-34: Consider adding debug logging in developmentWhile the simplified control flow is good, consider adding debug logging in development environments to help troubleshoot when the editor container is not found.
if (closestEditorContainer) { setEditorContainer(closestEditorContainer as HTMLDivElement); +} else if (process.env.NODE_ENV === 'development') { + console.debug('[CustomImageNode] Editor container not found'); }
Line range hint
48-54: Enhance error handling and performanceThe
getImageSourcefunction could benefit from proper error handling and performance optimizations:
- Add error handling for the image source retrieval
- Consider memoizing the image source resolution
useEffect(() => { const getImageSource = async () => { + try { // @ts-expect-error function not expected here, but will still work and don't remove await const url: string = await editor?.commands?.getImageSource?.(imgNodeSrc); setResolvedSrc(url as string); + } catch (error) { + console.error('[CustomImageNode] Failed to get image source:', error); + setFailedToLoadImage(true); + } }; getImageSource(); }, [imgNodeSrc]);packages/editor/src/core/extensions/slash-commands/command-menu.tsx (3)
Line range hint
49-49: Apply consistent null checking pattern.Similar to line 44, this line should also use optional chaining to prevent potential runtime errors when accessing items.
Apply this change:
-if (nextItem >= sections[currentSection].items.length) { +if (nextItem >= sections[currentSection]?.items?.length) {
Line range hint
95-107: Enhance error handling and accessibility.The component could benefit from improved error handling and accessibility:
- Add null checks for section.items before mapping
- Add ARIA attributes for keyboard navigation
- Consider adding error boundaries
Consider these improvements:
{sections.map((section, sectionIndex) => ( <div key={section.key} className="space-y-2"> {section.title && <h6 className="text-xs font-semibold text-custom-text-300">{section.title}</h6>} <div> - {section.items.map((item, itemIndex) => ( + {section.items?.map((item, itemIndex) => ( <CommandMenuItem key={item.key} + role="option" + aria-selected={sectionIndex === selectedIndex.section && itemIndex === selectedIndex.item} isSelected={sectionIndex === selectedIndex.section && itemIndex === selectedIndex.item} item={item}
Line range hint
1-124: Consider adding input validation and loading states.The component assumes the sections array is never empty and doesn't handle loading states. Consider adding:
- Input validation for sections prop
- Loading state handling
- Error boundaries for crash prevention
Would you like me to provide a code example implementing these improvements?
packages/editor/src/core/hooks/use-read-only-editor.ts (1)
120-121: Consider enhancing error handling and optimizing event handler bindingsWhile the implementation correctly enables real-time updates, consider these improvements:
- Add error handling for when provider is undefined
- Optimize the event handler bindings to prevent unnecessary function recreations
Consider this implementation:
- emitRealTimeUpdate: (message: TDocumentEventsServer) => provider?.sendStateless(message), - listenToRealTimeUpdate: () => provider && { on: provider.on.bind(provider), off: provider.off.bind(provider) }, + emitRealTimeUpdate: (message: TDocumentEventsServer) => { + if (!provider) { + console.warn('Provider not initialized. Real-time updates are disabled.'); + return; + } + provider.sendStateless(message); + }, + listenToRealTimeUpdate: () => { + if (!provider) { + console.warn('Provider not initialized. Real-time updates are disabled.'); + return null; + } + // Use useCallback in the component using this hook to memoize these handlers + return { + on: provider.on.bind(provider), + off: provider.off.bind(provider) + }; + },packages/editor/src/core/types/editor.ts (1)
17-18: Fix the import path resolution.The import from
src/libuses a relative path that might cause resolution issues. Consider using the proper module path or alias.-import { TDocumentEventsServer, TDocumentEventEmitter } from "src/lib"; +import { TDocumentEventsServer, TDocumentEventEmitter } from "@/lib";packages/editor/src/core/hooks/use-editor.ts (2)
258-258: Consider removing unused parameterInstead of prefixing with underscore, consider removing the unused parameter from the arrow function if it's not required by the API contract.
-state.doc.nodesBetween(from, to, (node, _pos, parent) => { +state.doc.nodesBetween(from, to, (node, _, parent) => {
299-300: Real-time sync implementation looks good but needs error handlingThe implementation aligns well with the PR objectives for real-time synchronization of lock/unlock/archive/restore events. However, consider adding error handling for undefined provider.
- emitRealTimeUpdate: (message: TDocumentEventsServer) => provider?.sendStateless(message), + emitRealTimeUpdate: (message: TDocumentEventsServer) => { + if (!provider) { + console.warn('Cannot emit real-time update: provider is not initialized'); + return; + } + provider.sendStateless(message); + },web/core/hooks/use-collaborative-page-actions.tsx (4)
22-42: Optimize dependencies inuseMemoHookThe
useMemohook foractionHandlerMapdepends on methods (page.lock,page.unlock,page.archive,page.restore) which could be redefined on each render ifpageis not a stable reference. This might cause unnecessary recalculations of the memoized value.To prevent this, consider depending on the
pageobject itself or ensure that these methods are stable. Here's how you can adjust the dependency array:Apply this diff to update the dependency array:
- [page.lock, page.unlock, page.archive, page.restore] + [page]
50-61: Log errors for improved debuggingIn the
catchblock ofexecuteCollaborativeAction, errors are caught and a toast notification is displayed. However, the actual error details are not logged, which can make debugging difficult.Consider logging the error to the console for easier troubleshooting.
You can modify the catch block as follows:
} catch (error) { setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: actionDetails.errorMessage, }); + console.error("Error executing collaborative action:", error); }
68-73: Guard against potential null referencesIn the
useEffecthook, when emitting real-time updates, it's good practice to ensure thateditorRefis not null to avoid potential runtime errors.You can add a null check before calling
emitRealTimeUpdate:if (currentActionBeingProcessed) { const serverEventName = getServerEventName(currentActionBeingProcessed); if (serverEventName) { + if (editorRef) { editorRef.emitRealTimeUpdate(serverEventName); + } } }
84-87: Handle missing payloads gracefullyIn the
handleStatelessMessagefunction, ifmessage.payloadis null or undefined, the function currently does nothing. Consider adding an else clause or logging to handle unexpected cases, which can aid in debugging.For example:
if (message.payload) { executeCollaborativeAction({ type: "receivedMessageFromServer", message: message.payload }); + } else { + console.warn("Received stateless message without payload"); }web/core/components/pages/editor/header/options-dropdown.tsx (3)
114-116: Provide user feedback after lock/unlock actions to enhance user experience.Currently, when the user locks or unlocks a page, there is no immediate feedback indicating the action was successful or if an error occurred. Consider providing success or error messages to inform the user about the result of their action, improving the user experience.
You might use the
setToastfunction to display feedback messages. For example:executeCollaborativeAction({ type: "sendMessageToServer", message: ACTION_MESSAGES.UNLOCK }) .then(() => setToast({ type: TOAST_TYPE.SUCCESS, title: "Success", message: "Page unlocked." })) .catch(() => setToast({ type: TOAST_TYPE.ERROR, title: "Error", message: "Failed to unlock page." }));
123-125: Provide user feedback after archive/restore actions to enhance user experience.Similar to the lock/unlock actions, consider providing feedback to the user after archiving or restoring a page to confirm the action was successful or handle any errors.
Implement feedback using the
setToastfunction. For example:executeCollaborativeAction({ type: "sendMessageToServer", message: ACTION_MESSAGES.ARCHIVE }) .then(() => setToast({ type: TOAST_TYPE.SUCCESS, title: "Success", message: "Page archived." })) .catch(() => setToast({ type: TOAST_TYPE.ERROR, title: "Error", message: "Failed to archive page." }));
68-68: Standardize icon types to simplify type definitions.Using a union type for the
iconproperty may indicate inconsistency in icon components. Consider standardizing the icon components to a single type or creating a common interface to improve type safety and simplify the codebase.You could create a common type for icons:
type IconType = LucideIcon | React.FC<ISvgIcons>;And use it in your
MENU_ITEMStype:icon: IconType;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
live/src/core/hocuspocus-server.ts(2 hunks)packages/editor/src/core/extensions/custom-image/components/image-block.tsx(0 hunks)packages/editor/src/core/extensions/custom-image/components/image-node.tsx(2 hunks)packages/editor/src/core/extensions/slash-commands/command-menu.tsx(1 hunks)packages/editor/src/core/helpers/document-events.ts(1 hunks)packages/editor/src/core/hooks/use-collaborative-editor.ts(2 hunks)packages/editor/src/core/hooks/use-editor.ts(4 hunks)packages/editor/src/core/hooks/use-read-only-collaborative-editor.ts(2 hunks)packages/editor/src/core/hooks/use-read-only-editor.ts(2 hunks)packages/editor/src/core/types/document-events.ts(1 hunks)packages/editor/src/core/types/editor.ts(5 hunks)packages/editor/src/core/types/index.ts(1 hunks)packages/editor/src/lib.ts(1 hunks)packages/ui/src/icons/index.ts(1 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx(2 hunks)web/core/components/pages/editor/editor-body.tsx(2 hunks)web/core/components/pages/editor/header/options-dropdown.tsx(3 hunks)web/core/hooks/use-collaborative-page-actions.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx
🔇 Additional comments (23)
packages/editor/src/core/types/index.ts (1)
11-11: LGTM! The export follows consistent patterns.
The new export statement maintains the established barrel file pattern and supports the real-time synchronization features being implemented.
Let's verify the existence of the document-events file:
✅ Verification successful
Export statement is valid and the target file exists
The verification confirms that document-events.ts exists at the correct location: packages/editor/src/core/types/document-events.ts, making this a valid export statement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the document-events.ts file exists in the correct location
# Expected: The file should exist in the same directory as index.ts
# Find the index.ts file first to determine the correct directory
fd --type f "index.ts$" packages/editor/src/core/types/ --exec dirname {} | \
xargs -I {} find {} -name "document-events.ts" -type f
Length of output: 186
packages/editor/src/core/types/document-events.ts (2)
1-1: LGTM!
The import statement correctly imports the necessary types using the absolute path.
1-9: Verify event types cover all required actions.
The implementation aligns well with the PR objectives for real-time synchronization. Let's verify that all required events (lock, unlock, archive, restore) are covered in the DocumentRealtimeEvents.
✅ Verification successful
Event types implementation is complete and correct
The DocumentRealtimeEvents object in packages/editor/src/core/helpers/document-events.ts covers all required actions:
- Lock/Unlock events
- Archive/Unarchive events
Each event has both client and server event names properly defined, and the type definitions in the reviewed file correctly reference these events, providing proper type safety for the event handling system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all required events are defined in DocumentRealtimeEvents
# Search for event definitions related to lock, unlock, archive, and restore
rg -A 10 "DocumentRealtimeEvents\s*=\s*" "@/helpers/document-events"
Length of output: 134
Script:
#!/bin/bash
# Let's try to find the correct path for document-events
fd "document-events" --type f
# Also search for DocumentRealtimeEvents in all files
rg -A 10 "DocumentRealtimeEvents\s*=\s*"
Length of output: 1237
packages/editor/src/core/helpers/document-events.ts (3)
3-8: LGTM! Well-structured event mapping
The implementation using a const object with as const assertion is a good choice here. It provides:
- Type safety
- Clear mapping between client and server events
- Immutable structure
- Easy extensibility for future events
10-10: LGTM! Type-safe key extraction
Good use of TypeScript's keyof typeof to create a type-safe union of event keys.
1-1: Verify type imports exist in the codebase
Let's ensure these types are properly exported from the lib directory.
packages/ui/src/icons/index.ts (1)
1-1: LGTM! Clean type export addition.
The new type export follows the established pattern and enhances type safety for icon components.
live/src/core/hocuspocus-server.ts (2)
7-10: LGTM! Clean import structure for real-time event handling types.
The new imports are well-organized and appropriately named for their purpose in handling real-time document events.
62-69: 🛠️ Refactor suggestion
Add error handling and input validation to improve robustness.
While the implementation is functional, it could benefit from additional error handling and validation to ensure reliability in production.
Consider applying these improvements:
async onStateless({ payload, document }) {
+ if (!payload) {
+ console.warn('Received empty payload in onStateless');
+ return;
+ }
+
+ try {
const response =
DocumentRealtimeEvents[payload as TDocumentEventsServer].client;
if (response) {
document.broadcastStateless(response);
}
+ } catch (error) {
+ console.error('Error processing stateless event:', error, { payload });
+ // Optionally broadcast an error event to clients
+ document.broadcastStateless({ type: 'SYNC_ERROR', data: { message: 'Failed to process event' } });
+ }
}Let's verify the event types being used:
packages/editor/src/core/hooks/use-read-only-collaborative-editor.ts (2)
50-51: LGTM! Dependency array is now complete
The dependency array correctly includes all values used in the provider configuration.
62-63: LGTM! Proper cleanup implementation
The cleanup logic correctly destroys both the provider and localProvider instances.
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx (1)
3-3: Verify if HocuspocusProviderWebsocket import is needed
The HocuspocusProviderWebsocket is imported but not directly used in this file. If it's used by a child component, consider moving the import closer to its usage.
packages/editor/src/core/hooks/use-collaborative-editor.ts (2)
61-64: LGTM! The IndexedDB persistence provider is properly memoized.
The implementation correctly creates a local persistence provider for offline support when an id is present.
Line range hint 1-108: Verify the implementation of real-time event handling.
The PR objectives mention implementing real-time sync for lock/unlock/archive/restore events, but the handling of these events is not visible in this file.
Let's verify where these events are being handled:
✅ Verification successful
Based on the search results, I can see that the real-time event handling for lock/unlock/archive/restore is defined in the DocumentRealtimeEvents constant in packages/editor/src/core/helpers/document-events.ts:
{
Lock: { client: "locked", server: "Lock" },
Unlock: { client: "unlocked", server: "Unlock" },
Archive: { client: "archived", server: "Archive" },
Unarchive: { client: "unarchived", server: "Unarchive" },
}Let me verify if these events are properly integrated with the HocuspocusProvider by searching for their usage:
Based on the search results, I can now provide a final response:
The real-time event handling for lock/unlock/archive/restore is properly implemented.
The implementation follows a complete event flow:
- Events are defined in
DocumentRealtimeEventsconstant with both client and server event names. - The collaborative editor setup in
use-collaborative-editor.tsprovides the infrastructure. - The events are handled in:
- Client side:
web/core/hooks/use-collaborative-page-actions.tsxmaps and executes the actions - Server side:
live/src/core/hocuspocus-server.tsbroadcasts the events usingdocument.broadcastStateless
- Client side:
The changes in the reviewed file are part of the supporting infrastructure that enables this real-time sync functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for lock/unlock/archive/restore event handling in the codebase
# Search for related event handlers
rg -A 5 "lock|unlock|archive|restore" --type ts
# Search for HocusPocus event handling patterns
ast-grep --pattern 'onStateless($$$) {
$$$
}'
Length of output: 101758
Script:
#!/bin/bash
# Search for usage of document events
rg -A 5 "DocumentRealtimeEvents" --type ts
# Search for onStateless event handler implementation
ast-grep --pattern 'onStateless: ($$$) => {
$$$
}'
Length of output: 6085
packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1)
55-55: Good optimization of effect dependencies
The dependency change from [imageFromFileSystem, node.attrs.src] to [imgNodeSrc] is a good optimization that reduces unnecessary effect triggers while maintaining the same functionality. The imageFromFileSystem state is only needed for rendering decisions and doesn't affect image source resolution.
packages/editor/src/core/hooks/use-read-only-editor.ts (1)
14-14: LGTM: Type import aligns with real-time sync requirements
The addition of TDocumentEventsServer type import supports the new real-time document event handling functionality.
packages/editor/src/core/types/editor.ts (3)
88-89: LGTM! Well-structured real-time event handling methods.
The new methods are well-typed and maintain the read-only nature of the API while enabling real-time updates.
128-128: Please clarify the purpose of the optional value prop.
The addition of the optional value prop seems unrelated to the PR's real-time sync objectives. This change could affect the editor's controlled/uncontrolled behavior.
Let's check for any related changes:
148-148: LGTM! Verify socket initialization and cleanup.
The socket property is correctly added to both collaborative interfaces. However, ensure proper socket initialization and cleanup are implemented in the corresponding components.
Let's verify the implementation:
Also applies to: 176-176
✅ Verification successful
Based on the search results, I can now generate the final response as we have found sufficient evidence of socket initialization and cleanup in the collaborative editor implementations:
Socket initialization and cleanup are properly implemented
The socket (HocuspocusProvider) is correctly initialized and cleaned up in both collaborative editor hooks:
- In
useCollaborativeEditor: Provider is initialized with proper configuration and destroyed on unmount - In
useReadOnlyCollaborativeEditor: Similar initialization pattern with cleanup on component unmount - Both implementations use
useEffectcleanup function to properly destroy the socket connection - Local persistence is also properly handled with cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for socket initialization and cleanup in collaborative components
ast-grep --pattern 'useEffect(() => {
$$$
socket.$$$
$$$
return () => {
$$$
socket.$$$
$$$
}
}, $$$)'
Length of output: 125
Script:
#!/bin/bash
# Let's try a broader search for socket-related cleanup patterns
rg -U "socket\." -A 5 -B 5
# Also search for useEffect patterns that might handle socket lifecycle
rg -U "useEffect.*socket" -A 10 -B 2
# Search for socket property usage
ast-grep --pattern 'socket = $_'
# Look for socket-related class methods or functions
ast-grep --pattern 'function $_(socket: $_) {
$$$
}'
Length of output: 1990
Script:
#!/bin/bash
# Let's search for HocuspocusProvider initialization and cleanup
rg -U "HocuspocusProvider" -A 10 -B 5
# Search for WebSocket connection handling
rg -U "connect|disconnect|close" -A 5 -B 5 packages/editor/src/
# Look for provider or connection setup in editor files
fd "editor" --type f --exec rg -U "provider|connection" -A 5 -B 5 {}
Length of output: 48217
web/core/components/pages/editor/editor-body.tsx (2)
126-126: LGTM: Proper dependency inclusion for serverHandler memoization
The addition of handleServerConnect and handleServerError to the dependencies array is correct and necessary to prevent stale closures, ensuring real-time connection status updates work reliably.
Line range hint 137-150: Verify implementation of lock/unlock/archive/restore events
The PR objectives mention implementing real-time sync for lock/unlock/archive/restore events, but I don't see explicit handling of these events in the WebSocket configuration. Please ensure these events are properly implemented and handled, either through the editor's built-in features or at a higher level.
Let's verify the implementation:
✅ Verification successful
Lock/unlock/archive/restore events are properly implemented
The implementation of lock/unlock/archive/restore events is complete and properly handled through the WebSocket configuration. Here's the evidence:
-
The WebSocket configuration in
editor-body.tsxcorrectly sets up the realtime connection with proper URL and query parameters. -
The
DocumentRealtimeEventsindocument-events.tsdefines all required events:- Lock/Unlock events
- Archive/Unarchive events
-
The
useCollaborativePageActionshook inuse-collaborative-page-actions.tsximplements comprehensive handling of these events with:- Proper action mapping for all events
- Bidirectional communication (client-to-server and server-to-client)
- Error handling and user feedback
- State management to prevent duplicate operations
The implementation follows a robust pattern for realtime collaboration, ensuring all events are properly synchronized between users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for lock/unlock/archive/restore event handling
# Look for related event handlers
echo "Searching for event handlers..."
ast-grep --pattern 'function $HANDLER($_) {
$$$
lock$$$
$$$
}'
# Look for related constants or types
echo "Searching for event-related types or constants..."
rg -t typescript "(?i)(lock|unlock|archive|restore).*event"
# Look for related WebSocket messages
echo "Searching for WebSocket message handling..."
rg -t typescript "(?i)(lock|unlock|archive|restore).*message"
Length of output: 545
Script:
#!/bin/bash
# Let's try a different approach without typescript file type specification
# Search for lock/unlock/archive/restore related handlers and events
echo "Searching for lock/unlock/archive/restore patterns..."
rg -g "*.{ts,tsx}" "(?i)(lock|unlock|archive|restore)"
# Look specifically in the editor components
echo "Searching in editor components..."
fd -e ts -e tsx . web/core/components/pages/editor | xargs cat
# Search for WebSocket related code
echo "Searching for WebSocket message handling..."
rg -g "*.{ts,tsx}" "onMessage|handleMessage|WebSocket"
Length of output: 155931
Script:
#!/bin/bash
# Let's search more specifically for WebSocket event handling related to lock/unlock/archive/restore
echo "Searching for WebSocket event handling..."
rg -g "*.{ts,tsx}" "DocumentRealtimeEvents|handleRealtimeEvent|onMessage.*lock|onMessage.*archive"
echo "Searching for realtime event types..."
rg -g "*.{ts,tsx}" "enum.*DocumentRealtimeEvents"
echo "Searching for realtime event handlers..."
ast-grep --pattern 'function $HANDLER($_) {
$$$
(lock|unlock|archive|restore)
$$$
}'
Length of output: 2391
Script:
#!/bin/bash
# Let's examine the DocumentRealtimeEvents definition and WebSocket configuration
echo "Checking DocumentRealtimeEvents definition..."
rg -A 10 "DocumentRealtimeEvents = {" packages/editor/src/core/helpers/document-events.ts
echo "Checking WebSocket URL and config setup..."
rg -A 10 "WS_LIVE_URL" web/core/
echo "Looking for realtime event handling implementation..."
rg -g "*.{ts,tsx}" "handleRealtimeEvent|onMessage" web/core/components/pages/editor/
Length of output: 2112
Script:
#!/bin/bash
# Let's try a different approach to find the DocumentRealtimeEvents definition
echo "Checking DocumentRealtimeEvents definition with simpler pattern..."
cat packages/editor/src/core/helpers/document-events.ts
echo "Looking for collaborative actions implementation..."
cat web/core/hooks/use-collaborative-page-actions.tsx
echo "Checking for WebSocket event handlers in editor..."
rg -g "*.{ts,tsx}" -B 2 -A 5 "onMessage|handleMessage|handleRealtimeEvent" web/core/components/pages/editor/
Length of output: 5091
packages/editor/src/core/hooks/use-editor.ts (1)
299-300: Verify real-time sync implementation usage
Let's verify that these new methods are properly utilized for lock/unlock/archive/restore events across the codebase.
✅ Verification successful
Let me generate additional verification scripts to check the real-time sync implementation for lock/unlock/archive/restore events.
Based on the verification results, I can now provide a final response about the real-time sync implementation:
Real-time sync implementation is properly integrated for lock/unlock/archive/restore events
The implementation is correctly structured and used across the codebase:
- The editor provides necessary methods through
emitRealTimeUpdateandlistenToRealTimeUpdatefor real-time communication - Events are properly typed with
TDocumentEventsServerandTDocumentEventsClient - Lock/unlock/archive/restore events are defined in
DocumentRealtimeEvents:Lock: { client: "locked", server: "Lock" }, Unlock: { client: "unlocked", server: "Unlock" }, Archive: { client: "archived", server: "Archive" }
- The
useCollaborativePageActionshook implements the full event handling cycle with proper state management to avoid duplicate operations - The implementation is used in the UI components (e.g.,
options-dropdown.tsx) to trigger collaborative actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of real-time sync methods for lock/unlock/archive/restore events
# Find files using the editor ref
echo "Files using editor ref:"
rg "useRef.*EditorRefApi"
# Find real-time event handling
echo "\nReal-time event handling:"
rg -A 5 "emitRealTimeUpdate|listenToRealTimeUpdate"
# Find lock/unlock/archive/restore related code
echo "\nLock/unlock/archive/restore related code:"
rg -A 5 "lock|unlock|archive|restore"
Length of output: 110434
Script:
#!/bin/bash
# Search for lock/unlock/archive/restore event handling in the codebase
echo "Lock/unlock/archive/restore event handling:"
rg -A 5 "TDocumentEventsServer|TDocumentEventsClient" --type ts
# Search for provider usage in real-time sync
echo "\nProvider real-time sync usage:"
rg -A 5 "provider\?.sendStateless|provider\.on|provider\.off" --type ts
# Search for collaborative actions
echo "\nCollaborative actions implementation:"
rg -A 10 "useCollaborativePageActions" --type ts
Length of output: 13541
web/core/hooks/use-collaborative-page-actions.tsx (1)
44-64: Include all necessary dependencies in useCallback Hook
The executeCollaborativeAction function uses setCurrentActionBeingProcessed and actionHandlerMap. While setCurrentActionBeingProcessed is a state setter and doesn't need to be included, ensure that any other dependencies used within the callback are accounted for.
Currently, the dependency array includes actionHandlerMap, which is correct. Double-check that all variables used within executeCollaborativeAction are included in the dependencies to prevent potential bugs.
Palanikannan1437
force-pushed
the
fix/lock-unlock-realtime
branch
from
4dd6660 to
173e93f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/editor/src/core/constants/document-collaborative-events.ts (1)
1-6: Add TypeScript types and documentationConsider enhancing type safety and maintainability with the following additions:
+type DocumentEventMapping = { + client: string; + server: string; +}; + +/** + * Maps bidirectional events between client and server for document collaborative actions. + * Used for real-time synchronization of document states across connected clients. + */ export const DocumentCollaborativeEvents = { Lock: { client: "locked", server: "Lock" }, Unlock: { client: "unlocked", server: "Unlock" }, Archive: { client: "archived", server: "Archive" }, Unarchive: { client: "unarchived", server: "Unarchive" }, -} as const; +} as const satisfies Record<string, DocumentEventMapping>;packages/editor/src/core/helpers/get-document-server-event.ts (2)
4-11: Consider performance and type-safety improvements.The implementation is correct but could be enhanced:
- Consider using
Object.entries()for better type safety:- for (const key in DocumentCollaborativeEvents) { - if (DocumentCollaborativeEvents[key as TDocumentEventKey].client === clientEvent) { - return DocumentCollaborativeEvents[key as TDocumentEventKey].server; - } + for (const [key, event] of Object.entries(DocumentCollaborativeEvents)) { + if (event.client === clientEvent) { + return event.server; + }
- If this function is called frequently, consider memoizing the results:
const eventMap = new Map<TDocumentEventsClient, TDocumentEventsServer>(); export const getServerEventName = (clientEvent: TDocumentEventsClient): TDocumentEventsServer | undefined => { if (eventMap.has(clientEvent)) return eventMap.get(clientEvent); for (const [_, event] of Object.entries(DocumentCollaborativeEvents)) { if (event.client === clientEvent) { eventMap.set(clientEvent, event.server); return event.server; } } return undefined; };
4-4: Add JSDoc documentation for better code maintainability.Consider adding JSDoc documentation to describe the function's purpose, parameters, and return value:
+/** + * Maps a client-side document event to its corresponding server-side event. + * @param clientEvent - The client-side document event to map + * @returns The corresponding server-side event, or undefined if no mapping exists + */ export const getServerEventName = (clientEvent: TDocumentEventsClient): TDocumentEventsServer | undefined => {packages/editor/src/core/types/editor.ts (1)
88-89: Consider architectural placement and add documentation
The
emitRealTimeUpdatemethod represents a write operation but is placed in theEditorReadOnlyRefApi. Consider moving it toEditorRefApito maintain the read-only contract.Please add JSDoc comments to document:
- The purpose and usage of these methods
- The types of events that can be emitted/listened to
- The scenarios where undefined might be returned from
listenToRealTimeUpdateExample documentation:
/** Emits a real-time update event for collaborative actions */ emitRealTimeUpdate: (action: TDocumentEventsServer) => void; /** * Subscribes to real-time updates from other collaborators * @returns An event emitter instance, or undefined if real-time sync is not available */ listenToRealTimeUpdate: () => TDocumentEventEmitter | undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
live/src/core/hocuspocus-server.ts(2 hunks)packages/editor/src/core/constants/document-collaborative-events.ts(1 hunks)packages/editor/src/core/helpers/get-document-server-event.ts(1 hunks)packages/editor/src/core/hooks/use-collaborative-editor.ts(2 hunks)packages/editor/src/core/hooks/use-read-only-collaborative-editor.ts(2 hunks)packages/editor/src/core/types/document-collaborative-events.ts(1 hunks)packages/editor/src/core/types/editor.ts(3 hunks)packages/editor/src/core/types/index.ts(1 hunks)packages/editor/src/lib.ts(1 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx(1 hunks)web/core/components/pages/editor/editor-body.tsx(1 hunks)web/core/hooks/use-collaborative-page-actions.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- live/src/core/hocuspocus-server.ts
- packages/editor/src/core/hooks/use-collaborative-editor.ts
- packages/editor/src/core/hooks/use-read-only-collaborative-editor.ts
- packages/editor/src/core/types/index.ts
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx
- web/core/components/pages/editor/editor-body.tsx
- web/core/hooks/use-collaborative-page-actions.tsx
🔇 Additional comments (8)
packages/editor/src/lib.ts (2)
2-4: LGTM! Exports align well with PR objectives
The new exports provide the necessary building blocks for implementing real-time synchronization of page actions:
- Constants define the supported collaborative events (lock, unlock, archive, restore)
- Helper functions manage server event mapping for real-time sync
- Types ensure type safety across the collaborative features
2-4: Verify new exports for potential circular dependencies
The new exports support the PR objectives by providing types and utilities for real-time page actions. Let's verify there are no circular dependencies.
✅ Verification successful
The previous script failed because it didn't account for the repository structure. Let's try again with a more accurate search:
The results show a clear dependency pattern, but let's verify the complete import structure of these files to ensure there are no circular dependencies:
No circular dependencies detected in new collaborative event modules
The dependency structure is clean and hierarchical:
constants/document-collaborative-events.tshas no importstypes/document-collaborative-events.tsimports only from constantshelpers/get-document-server-event.tsimports from both constants and types
None of these modules import from lib.ts, and the dependencies flow in one direction without creating cycles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for circular dependencies in the new collaborative event modules
echo "Checking imports in collaborative event modules:"
# Check if any of these modules import from lib.ts
rg -l "from.*lib" \
"constants/document-collaborative-events" \
"helpers/get-document-server-event" \
"types/document-collaborative-events"
echo -e "\nChecking cross-imports between modules:"
# Check if these modules import from each other
for file in "constants/document-collaborative-events" "helpers/get-document-server-event" "types/document-collaborative-events"; do
echo "Imports in $file:"
rg "from.*@/" "$file" || true
done
Length of output: 1679
Script:
#!/bin/bash
# First, let's find the actual paths of these files
echo "Finding actual file paths:"
fd -t f "document-collaborative-events|get-document-server-event" packages/editor
echo -e "\nChecking imports in these files:"
# Use fd to find the files and pipe them to rg for checking imports
fd -t f "document-collaborative-events|get-document-server-event" packages/editor -x rg -l "from.*lib" {} || true
echo -e "\nChecking cross-imports between modules:"
fd -t f "document-collaborative-events|get-document-server-event" packages/editor -x rg "from.*@/" {} || true
Length of output: 1045
Script:
#!/bin/bash
echo "Checking all imports in document-collaborative-events.ts:"
rg "^import" packages/editor/src/core/constants/document-collaborative-events.ts || true
echo -e "\nChecking all imports in get-document-server-event.ts:"
rg "^import" packages/editor/src/core/helpers/get-document-server-event.ts || true
echo -e "\nChecking all imports in types/document-collaborative-events.ts:"
rg "^import" packages/editor/src/core/types/document-collaborative-events.ts || true
Length of output: 935
packages/editor/src/core/constants/document-collaborative-events.ts (2)
1-6: Verify event handler implementations
Let's verify that these events are properly handled in the codebase.
✅ Verification successful
Events are properly implemented with client-server handlers
The verification shows a complete implementation of the collaborative events:
- Client events are defined in
DocumentCollaborativeEventswith proper client-server mapping - Server-side handling is implemented in
hocuspocus-server.tswhich broadcasts events - Client-side handlers are implemented in
use-collaborative-page-actions.tsxwith:- Lock/Unlock handlers with proper error messages
- Archive/Unarchive handlers with proper error messages
- Action execution and error handling
- Helper function
getServerEventNameproperly maps between client and server events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these events in handlers and subscribers
echo "Searching for event handlers:"
ast-grep --pattern 'locked|unlocked|archived|unarchived'
# Search for HocusPocus server implementation
echo -e "\nSearching for server-side event handling:"
rg -A 5 "DocumentCollaborativeEvents" --type ts
Length of output: 7003
5-5: Verify naming consistency with PR requirements
The PR mentions "restore" functionality, but the constant uses "Unarchive". Let's verify if this naming is consistent across the codebase.
✅ Verification successful
Based on the search results, I can now provide a final response:
Both "restore" and "unarchive" terms are used consistently in their respective contexts
The codebase shows a clear pattern where:
- "Unarchive" is specifically used for document/collaborative events in the constants file, matching the server-side event naming
- "restore" is used as the user-facing action term across the UI components and hooks
This is actually a correct and consistent implementation because:
- The server event is named "Unarchive" while the client event is "unarchived"
- The UI/UX layer uses the more user-friendly term "restore" for buttons and messages
- Both terms refer to the same conceptual action but at different layers of the application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for both "restore" and "unarchive" usage in the codebase
echo "Searching for restore/unarchive usage in components and hooks:"
rg -i "restore|unarchive" --type ts --type tsx
# Search specifically in API-related files to verify server terminology
echo -e "\nSearching in API-related files:"
fd -e ts -e tsx | rg -l "api|client" | xargs rg -i "restore|unarchive"
Length of output: 411
Script:
#!/bin/bash
# Search for both "restore" and "unarchive" usage in the codebase
echo "Searching for restore/unarchive usage:"
rg -i "restore|unarchive"
# Search specifically in constants and types
echo -e "\nSearching in constants and types:"
fd "constants|types" -e ts | xargs rg -i "restore|unarchive"
# Look for related event handlers or actions
echo -e "\nSearching for related handlers and actions:"
rg -i "handle.*restore|handle.*unarchive|on.*restore|on.*unarchive"
Length of output: 136866
packages/editor/src/core/types/document-collaborative-events.ts (2)
1-1: LGTM!
The import statement correctly uses the project's import conventions.
3-5: LGTM! Types are well-structured for real-time sync events.
The type definitions provide strong type safety for collaborative events. Let's verify that all required event types are present in the constant.
✅ Verification successful
Types are correctly defined and match the constant implementation
Based on the search results, I can verify that the DocumentCollaborativeEvents constant in packages/editor/src/core/constants/document-collaborative-events.ts contains all the required event types:
- Lock/Unlock events
- Archive/Unarchive events
- Each event has both client and server variants
The type definitions in the reviewed file correctly map to this constant's structure, providing proper type safety for the collaborative features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that DocumentCollaborativeEvents constant contains all required event types
# Test: Search for the constant definition and verify it includes Lock, Unlock, Archive, and Restore events
ast-grep --pattern 'const DocumentCollaborativeEvents = {
$$$
Lock: $$$,
Unlock: $$$,
Archive: $$$,
$$$
}'
Length of output: 118
Script:
#!/bin/bash
# Let's first find the file containing DocumentCollaborativeEvents definition
rg "DocumentCollaborativeEvents" -A 10
# As a backup, let's also search for any constant containing Lock, Unlock events
ast-grep --pattern 'const $_ = {
$$$
Lock: $_,
$$$
}'
Length of output: 8515
packages/editor/src/core/types/editor.ts (2)
11-12: LGTM: New types for real-time sync
The addition of TDocumentEventEmitter and TDocumentEventsServer types aligns with the PR's objective of implementing real-time synchronization.
128-128: Verify handling of undefined/null values
The value prop is now optional with type string | null. Consider:
- Refining the type to either
string | undefined(using?) orstring | null, but not both - Ensuring that components consuming this prop properly handle the undefined/null cases
✅ Verification successful
Proper null/undefined handling confirmed, but type can be simplified
Based on the code analysis, the value prop is properly handled throughout the codebase:
- The editor component has proper null checks:
if (value === null || value === undefined) return; - The value is safely used in content setting:
editor.commands.setContent(value, false, ...)
However, since the code already handles both null and undefined cases explicitly, and ? operator already makes the prop optional (undefined), having | null in the type is redundant. The type can be simplified to just:
value?: string;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of the value prop to verify proper null checks
ast-grep --pattern 'value={$_}'
# Search for potential issues with value access
rg -A 2 'props.value' packages/editor/
Length of output: 72
Script:
#!/bin/bash
# Search for usage of value prop in JSX/TSX files
rg -t tsx -t jsx "value={" packages/editor/
# Search for value prop access patterns
rg -t tsx -t jsx -t ts "props\.value|\.value\b" packages/editor/
# Search for Editor component usage
ast-grep --pattern 'Editor $_'
# Look for value-related type definitions
rg -t ts "value.*:" packages/editor/
Length of output: 3430
Script:
#!/bin/bash
# Fix the regex and search for value prop usage in JSX/TSX files
rg "value=\{" packages/editor/
# Search for value handling in the editor implementation
rg -A 3 "\.value" packages/editor/src/core/
# Look for null checks around value usage
rg -A 3 "(value\s*(\?\.|&&|\|\||===|!==)\s*|if\s*\(\s*value\s*\))" packages/editor/
Length of output: 14320
packages/editor/src/core/hooks/use-read-only-collaborative-editor.ts
Outdated
Show resolved
Hide resolved
Palanikannan1437
changed the title
Palanikannan1437
force-pushed
the
fix/lock-unlock-realtime
branch
from
c593106 to
1a66f3c
Compare
Palanikannan1437
force-pushed
the
fix/lock-unlock-realtime
branch
from
5958a82 to
429bbc4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
web/core/store/pages/page.ts (1)
438-452: Consider adding protection against concurrent lock/unlock operations.While the implementation correctly handles local state and server synchronization, it might be vulnerable to race conditions if multiple lock/unlock operations are triggered in quick succession. Consider:
- Adding a guard against concurrent operations
- Using a request cancellation mechanism
Here's a suggested implementation:
lock = async (shouldSync: boolean = true) => { const { workspaceSlug, projectId } = this.store.router; if (!workspaceSlug || !projectId || !this.id) return undefined; + if (this.is_locked) return undefined; // Guard against concurrent operations + const pageIsLocked = this.is_locked; runInAction(() => (this.is_locked = true)); if (shouldSync) { + const controller = new AbortController(); await this.pageService.lock(workspaceSlug, projectId, this.id).catch((error) => { runInAction(() => { this.is_locked = pageIsLocked; }); throw error; }); } };🧰 Tools
🪛 Biome (1.9.4)
[error] 443-443: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/editor/src/core/constants/document-collaborative-events.ts(1 hunks)web/core/components/pages/editor/header/options-dropdown.tsx(3 hunks)web/core/hooks/use-collaborative-page-actions.tsx(1 hunks)web/core/store/pages/page.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/editor/src/core/constants/document-collaborative-events.ts
- web/core/hooks/use-collaborative-page-actions.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/store/pages/page.ts
[error] 443-443: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 463-463: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (5)
web/core/components/pages/editor/header/options-dropdown.tsx (2)
6-16: LGTM: Icon imports are well-organized and relevant.
The new icon imports from lucide-react are properly organized and match their intended usage in the menu items.
60-61: LGTM: Type safety improvements and collaborative actions integration.
The changes enhance type safety for icons and properly integrate collaborative actions. Let's verify the hook implementation.
Also applies to: 68-68
✅ Verification successful
Let me gather more information about the hook implementation and its usage.
Let me try again with correct file type flags:
Let me try one last time with simpler commands:
Hook implementation and usage are properly integrated
The verification shows that:
- The hook is correctly defined in
web/core/hooks/use-collaborative-page-actions.tsx - It's properly imported and used in
options-dropdown.tsx - The
executeCollaborativeActionis used consistently for collaborative features (lock/unlock, archive/unarchive) - The implementation includes proper error handling and real-time message processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the useCollaborativePageActions hook implementation
# and its usage across the codebase
# Check the hook implementation
rg -A 10 "useCollaborativePageActions" --type typescript
# Check for consistent usage patterns
ast-grep --pattern 'const { executeCollaborativeAction } = useCollaborativePageActions($_, $_)'
Length of output: 340
Script:
#!/bin/bash
# Search for the hook definition and its usages with proper file type
rg -t tsx -t ts -A 10 "useCollaborativePageActions"
# Find any potential imports of this hook
rg -t tsx -t ts "import.*useCollaborativePageActions"
# Look for the collaborative actions implementation
rg -t tsx -t ts "executeCollaborativeAction"
Length of output: 236
Script:
#!/bin/bash
# Search for the hook definition and its usages
rg "useCollaborativePageActions" --type=typescript --type=typescriptreact -A 10
# Find any potential imports of this hook
rg "import.*useCollaborativePageActions" --type=typescript --type=typescriptreact
# Look for the collaborative actions implementation
rg "executeCollaborativeAction" --type=typescript --type=typescriptreact -A 5
Length of output: 346
Script:
#!/bin/bash
# Search for files containing the hook
fd -e ts -e tsx | xargs rg "useCollaborativePageActions" -A 10
# Find hook imports
fd -e ts -e tsx | xargs rg "import.*useCollaborativePageActions"
# Find collaborative action implementations
fd -e ts -e tsx | xargs rg "executeCollaborativeAction" -A 5
Length of output: 7415
web/core/store/pages/page.ts (3)
39-42: LGTM! Interface changes align with real-time sync requirements.
The addition of the optional shouldSync parameter to lock, unlock, archive, and restore methods provides the necessary flexibility for handling both local and synchronized state updates.
458-472: Same race condition consideration applies to unlock method.
The unlock implementation would benefit from the same race condition protection suggested for the lock method.
🧰 Tools
🪛 Biome (1.9.4)
[error] 463-463: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
506-525: Apply the same error handling improvements as archive method.
The restore implementation would benefit from the same improvements suggested for the archive method, particularly removing console.error and maintaining consistent error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/editor/src/core/hooks/use-editor.ts (1)
303-304: Consider adding resilience mechanisms for real-time syncThe real-time sync implementation could benefit from additional resilience mechanisms:
- Error Recovery: Implement retry logic for failed real-time updates
- Race Condition Handling: Consider using version vectors or timestamps
- Network Resilience: Add reconnection logic and offline support
- State Reconciliation: Implement conflict resolution for concurrent updates
Would you like help implementing any of these resilience mechanisms?
web/core/store/pages/page.ts (1)
454-459: Simplify the runInAction block.Consider simplifying the assignment in the runInAction block for better readability.
-runInAction(() => { - this.is_locked = pageIsLocked; -}); +runInAction(() => { + this.is_locked = pageIsLocked; +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/editor/src/core/hooks/use-collaborative-editor.ts(2 hunks)packages/editor/src/core/hooks/use-editor.ts(4 hunks)packages/editor/src/core/hooks/use-read-only-collaborative-editor.ts(3 hunks)packages/editor/src/core/hooks/use-read-only-editor.ts(2 hunks)packages/editor/src/core/types/editor.ts(2 hunks)packages/ui/src/icons/index.ts(1 hunks)web/core/components/pages/editor/editor-body.tsx(1 hunks)web/core/store/pages/page.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/ui/src/icons/index.ts
- web/core/components/pages/editor/editor-body.tsx
- packages/editor/src/core/hooks/use-collaborative-editor.ts
- packages/editor/src/core/hooks/use-read-only-collaborative-editor.ts
- packages/editor/src/core/types/editor.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/store/pages/page.ts
[error] 452-452: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 472-472: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (8)
packages/editor/src/core/hooks/use-editor.ts (3)
19-27: LGTM! Clean type imports organization
The type imports are well-organized and include all necessary types for real-time updates.
262-262: LGTM! Good practice using underscore prefix
Using _pos for an unused parameter follows TypeScript best practices.
303-304: 🛠️ Refactor suggestion
Duplicate: Add error handling and improve type safety for real-time updates
The same concerns about error handling and type safety apply to these methods as in use-read-only-editor.ts.
web/core/store/pages/page.ts (5)
39-42: LGTM! Interface changes are consistent with real-time sync requirements.
The addition of the optional shouldSync parameter to lock, unlock, archive, and restore methods aligns well with the PR objectives for implementing real-time synchronization.
447-461: LGTM! Optimistic updates with proper error handling.
The implementation follows best practices:
- Optimistic updates for better UX
- Proper error handling with state rollback
- Conditional API call based on shouldSync parameter
🧰 Tools
🪛 Biome (1.9.4)
[error] 452-452: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
467-481: LGTM! Implementation is consistent with lock method.
The unlock method follows the same robust pattern as the lock method, maintaining code consistency and reliability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 472-472: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
487-509:
Enhance error handling in archive method.
The current implementation has several issues:
- Using console.error in production code
- Inconsistent error handling compared to lock/unlock methods
- Missing previous state capture for proper rollback
Apply this diff to align with the established patterns:
archive = async (shouldSync: boolean = true) => {
const { workspaceSlug, projectId } = this.store.router;
if (!workspaceSlug || !projectId || !this.id) return undefined;
+ const previousState = this.archived_at;
try {
runInAction(() => {
this.archived_at = new Date().toISOString();
});
if (this.rootStore.favorite.entityMap[this.id])
this.rootStore.favorite.removeFavoriteFromStore(this.id);
if (shouldSync) {
const response = await this.pageService.archive(workspaceSlug, projectId, this.id);
runInAction(() => {
this.archived_at = response.archived_at;
});
}
} catch (error) {
- console.error(error);
runInAction(() => {
- this.archived_at = null;
+ this.archived_at = previousState;
});
+ throw error;
}
};515-534:
Remove console.error and propagate errors in restore method.
While the method correctly captures the previous state, it still has production logging and doesn't propagate errors consistently.
Apply this diff to align with best practices:
restore = async (shouldSync: boolean = true) => {
const { workspaceSlug, projectId } = this.store.router;
if (!workspaceSlug || !projectId || !this.id) return undefined;
const archivedAtBeforeRestore = this.archived_at;
try {
runInAction(() => {
this.archived_at = null;
});
if (shouldSync) {
await this.pageService.restore(workspaceSlug, projectId, this.id);
}
} catch (error) {
- console.error(error);
runInAction(() => {
this.archived_at = archivedAtBeforeRestore;
});
+ throw error;
}
};Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/editor/src/core/hooks/use-editor.ts (1)
Line range hint
1-316: Consider extracting real-time update logic into a shared hookBoth
useEditoranduseReadOnlyEditorhooks implement the same real-time update functionality. This could lead to maintenance issues if the implementation needs to change.Consider creating a custom hook like
useRealTimeUpdatesto share this functionality:const useRealTimeUpdates = (provider?: HocuspocusProvider) => { const emitRealTimeUpdate = useCallback((message: TDocumentEventsServer) => { if (!provider) { console.warn('Provider not initialized for real-time updates'); return; } provider.sendStateless(message); }, [provider]); const listenToRealTimeUpdate = useCallback(() => { if (!provider) { console.warn('Provider not initialized for real-time updates'); return null; } const boundOn = (event: TDocumentEventsServer['type'], handler: (data: any) => void) => provider.on(event, handler); const boundOff = (event: TDocumentEventsServer['type'], handler: (data: any) => void) => provider.off(event, handler); return { on: boundOn, off: boundOff }; }, [provider]); return { emitRealTimeUpdate, listenToRealTimeUpdate }; };This would:
- Reduce code duplication
- Centralize error handling
- Make it easier to maintain and update the real-time functionality
- Improve reusability across other components
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/editor/src/core/hooks/use-editor.ts(4 hunks)packages/editor/src/core/hooks/use-read-only-editor.ts(2 hunks)
🔇 Additional comments (6)
packages/editor/src/core/hooks/use-read-only-editor.ts (2)
14-20: LGTM!
The import statement is properly organized with other type imports.
129-130:
Add error handling and improve type safety for real-time updates
The real-time update methods need additional safeguards:
- Error handling for undefined provider
- Type safety for event handlers
- Cleanup for event listeners
Consider this implementation:
-emitRealTimeUpdate: (message: TDocumentEventsServer) => provider?.sendStateless(message),
-listenToRealTimeUpdate: () => provider && { on: provider.on.bind(provider), off: provider.off.bind(provider) },
+emitRealTimeUpdate: (message: TDocumentEventsServer) => {
+ if (!provider) {
+ console.warn('Provider not initialized for real-time updates');
+ return;
+ }
+ provider.sendStateless(message);
+},
+listenToRealTimeUpdate: () => {
+ if (!provider) {
+ console.warn('Provider not initialized for real-time updates');
+ return null;
+ }
+ const boundOn = (event: TDocumentEventsServer['type'], handler: (data: any) => void) =>
+ provider.on(event, handler);
+ const boundOff = (event: TDocumentEventsServer['type'], handler: (data: any) => void) =>
+ provider.off(event, handler);
+ return { on: boundOn, off: boundOff };
+}Likely invalid or redundant comment.
packages/editor/src/core/hooks/use-editor.ts (4)
19-26: LGTM!
The import statement is properly organized with other type imports.
73-73: LGTM!
The provider prop is properly positioned in the props order.
261-261: LGTM!
Good practice to prefix unused parameter with underscore.
302-303: Add error handling and improve type safety for real-time updates
The same issues with error handling and type safety exist in this implementation as in use-read-only-editor.ts. Please apply the same fixes here.
Description
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
IPageinterface to allow for more granular control over synchronization behavior in page state management.