Fix type errors in devtools package #938
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/devtools/src/devtools/contexts/YorkieSource.tsxOops! Something went wrong! :( ESLint: 8.19.0 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces a comprehensive refactoring of the event handling mechanism in the Yorkie DevTools extension. The changes primarily focus on renaming and restructuring event-related types, methods, and contexts from transaction-based events to document replay events. This involves modifications across multiple files in the SDK and DevTools packages, updating import statements, method signatures, and type definitions to support a more streamlined event processing approach. Changes
Assessment against linked issues
Poem
Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #938 +/- ##
==========================================
- Coverage 78.56% 78.53% -0.03%
==========================================
Files 62 63 +1
Lines 5417 5419 +2
Branches 998 997 -1
==========================================
Hits 4256 4256
- Misses 871 873 +2
Partials 290 290 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/devtools/src/devtools/components/Tree.tsx (1)
Line range hint
60-77: Consider improving TypeIcon type safetyThe
TypeIconcomponent could benefit from stricter typing. Consider defining a union type for the possible icon types.type IconType = 'YORKIE_OBJECT' | 'YORKIE_ARRAY' | 'YORKIE_PRIMITIVE' | 'YORKIE_TEXT' | 'YORKIE_TREE' | 'YORKIE_COUNTER' | 'USER'; const TypeIcon = ({ type }: { type: IconType }) => { // ... existing switch statement };packages/devtools/src/devtools/contexts/YorkieSource.tsx (2)
148-150: Consider renaming variables for clarity in 'getReplayDocEventType'.The parameter
eventingetReplayDocEventTyperepresents an array of events (EventsForDocReplay), which may cause confusion. Renaming it toeventscan improve clarity.Apply this diff to rename the parameter:
-export const getReplayDocEventType = ( - event: Devtools.EventsForDocReplay, +export const getReplayDocEventType = ( + events: Devtools.EventsForDocReplay, ): ReplayDocEventType => { - for (const docEvent of event) { + for (const docEvent of events) { if ( docEvent.type === DocEventType.StatusChanged || docEvent.type === DocEventType.Snapshot || docEvent.type === DocEventType.LocalChange || docEvent.type === DocEventType.RemoteChange ) { return ReplayDocEventType.Document; } } return ReplayDocEventType.Presence; };
180-187: Clarify variable naming when mapping over events.In the
useReplayDocEventshook, when mapping overevents, eacheventis actually an array of events (EventsForDocReplay). Consider renamingeventtoeventsGroupfor better readability.Apply this diff to rename the variable:
-return events.map((event) => { +return events.map((eventsGroup) => { - const replayDocEventType = getReplayDocEventType(event); + const replayDocEventType = getReplayDocEventType(eventsGroup); return { - event, + event: eventsGroup, replayDocEventType, isFiltered: hidePresenceEvents && replayDocEventType === ReplayDocEventType.Presence, }; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/devtools-publish.yml(1 hunks)packages/devtools/src/devtools/components/Detail.tsx(1 hunks)packages/devtools/src/devtools/components/Tree.tsx(2 hunks)packages/devtools/src/devtools/contexts/YorkieSource.tsx(5 hunks)packages/devtools/src/devtools/panel/index.tsx(3 hunks)packages/devtools/src/devtools/tabs/History.tsx(3 hunks)packages/sdk/src/devtools/index.ts(3 hunks)packages/sdk/src/devtools/protocol.ts(2 hunks)packages/sdk/src/devtools/types.ts(2 hunks)packages/sdk/src/document/document.ts(2 hunks)packages/sdk/src/yorkie.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/devtools/src/devtools/components/Detail.tsx
👮 Files not reviewed due to content moderation or server errors (4)
- packages/sdk/src/devtools/index.ts
- packages/devtools/src/devtools/panel/index.tsx
- packages/devtools/src/devtools/tabs/History.tsx
- packages/sdk/src/document/document.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (18.x)
🔇 Additional comments (5)
packages/sdk/src/yorkie.ts (1)
47-47: LGTM! Clean addition of Json type exportThe addition of the
Jsontype export is well-placed among other document-related types and aligns with the PR's objective of improving type safety.packages/devtools/src/devtools/components/Tree.tsx (2)
25-25: LGTM! Clean import statement updateThe addition of the
Jsontype import is well-organized and maintains proper import structure.
50-50: LGTM! Improved type referenceThe update from
Devtools.JsontoJsonprovides a more direct and cleaner type reference, improving code clarity.packages/sdk/src/devtools/types.ts (1)
103-151: Type definitions and utility functions are well-defined.The new types
EventForDocReplayandEventsForDocReplay, along with the validation functionsisEventForDocReplayandisEventsForDocReplay, are correctly implemented and enhance the codebase..github/workflows/devtools-publish.yml (1)
8-8: Consider expanding the trigger paths for critical files.While targeting only
package.jsonoptimizes the workflow triggers, it might miss important changes that require deployment, such as manifest updates or critical source file changes.Let's verify if there are other critical files that should trigger a deployment:
✅ Verification successful
Current trigger path is appropriate for this Plasmo extension
The workflow trigger on
packages/devtools/package.jsonis sufficient as this file contains all critical configuration, including the manifest settings. Since this is a Plasmo-based browser extension, the manifest.json is generated during build from the package.json content.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for manifest and configuration files in the devtools package # that might need to trigger deployments. # Look for manifest files echo "Checking for manifest files..." fd -e json "manifest" packages/devtools/ # Look for other configuration files echo "Checking for other configuration files..." fd -e json -e yaml -e yml "config|rc" packages/devtools/ # Check if the path exists echo "Verifying package.json path..." test -f packages/devtools/package.json && echo "✓ package.json exists" || echo "✗ package.json not found"Length of output: 460
Script:
#!/bin/bash # Check package.json content and search for manifest files echo "Package.json content:" cat packages/devtools/package.json echo -e "\nSearching for manifest files in src directory:" fd -e json "manifest" packages/devtools/src/Length of output: 1234
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/devtools/src/devtools/contexts/YorkieSource.tsx (1)
148-162: Consider adding error handling for unknown event types.The
getReplayDocEventTypefunction should handle cases where none of the conditions match.export const getReplayDocEventType = ( event: Devtools.DocEventsForReplay, ): ReplayDocEventType => { for (const docEvent of event) { if ( docEvent.type === DocEventType.StatusChanged || docEvent.type === DocEventType.Snapshot || docEvent.type === DocEventType.LocalChange || docEvent.type === DocEventType.RemoteChange ) { return ReplayDocEventType.Document; } } + if (!event.length) { + throw new Error('Empty event array'); + } return ReplayDocEventType.Presence; };packages/sdk/src/document/document.ts (1)
Line range hint
1719-1787: Consider adding error handling for invalid event sequences.The replay methods should handle cases where events are applied in an invalid order.
public applyDocEventsForReplay(event: Array<Devtools.DocEventForReplay<P>>) { + // Validate event sequence + const hasSnapshot = event.some(e => e.type === DocEventType.Snapshot); + const hasStatusChanged = event.some(e => e.type === DocEventType.StatusChanged); + if (hasSnapshot && !hasStatusChanged) { + throw new Error('Status must be changed before applying snapshot'); + } for (const docEvent of event) { this.applyDocEventForReplay(docEvent); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/devtools/src/devtools/contexts/YorkieSource.tsx(5 hunks)packages/devtools/src/devtools/panel/index.tsx(3 hunks)packages/devtools/src/devtools/tabs/History.tsx(3 hunks)packages/sdk/src/devtools/index.ts(3 hunks)packages/sdk/src/devtools/protocol.ts(2 hunks)packages/sdk/src/devtools/types.ts(2 hunks)packages/sdk/src/document/document.ts(11 hunks)packages/sdk/src/yorkie.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/devtools/src/devtools/panel/index.tsx
- packages/sdk/src/yorkie.ts
- packages/sdk/src/devtools/protocol.ts
🔇 Additional comments (22)
packages/sdk/src/devtools/types.ts (5)
20-33: LGTM! Comprehensive import of event types.The imports are well-organized and include all necessary event types for document replay functionality.
102-117: LGTM! Well-structured type definition for document replay events.The
DocEventForReplaytype union covers all necessary event types for document replay functionality, with proper generic type parameters for presence (P) and operations (T).
119-122: LGTM! Clear type alias for array of replay events.The
DocEventsForReplaytype provides a clean abstraction for working with collections of replay events.
124-142: Consider adding unit tests for event type validation.The
isDocEventForReplayfunction provides comprehensive validation of event types. However, given its critical role in event handling, it would benefit from thorough unit testing.Would you like me to help create unit tests for this function to ensure all event types are properly validated?
144-151: LGTM! Efficient array validation using every.The
isDocEventsForReplayfunction efficiently validates arrays by reusing the single event validation logic.packages/sdk/src/devtools/index.ts (5)
17-21: LGTM! Clean import organization.The imports are well-organized and include all necessary dependencies for the updated event handling.
28-32: LGTM! Clear documentation of storage purpose.The comment clearly explains the purpose of
replayEventsByDocKeyand indicates future plans for external storage.
34-35: LGTM! Consistent type declaration in window interface.The global declaration properly extends the window interface with the new event type.
Also applies to: 39-39
78-84: LGTM! Robust event validation and storage.The event handling logic correctly validates events using
isDocEventsForReplaybefore storing them.
135-135: LGTM! Consistent event retrieval.The event retrieval for panel sync uses the updated event storage.
packages/devtools/src/devtools/contexts/YorkieSource.tsx (5)
27-27: LGTM! Clean import of SDK types.The import statement correctly includes the necessary types from the SDK.
33-36: LGTM! Well-structured context type.The context type properly defines the shape of the replay events state and presence filtering.
46-47: LGTM! Clear state management.The state variables are properly typed and named according to their purpose.
Also applies to: 51-51
143-146: LGTM! Clear event type enumeration.The
ReplayDocEventTypeenum clearly defines the possible event categories.
165-187: LGTM! Comprehensive hook implementation.The
useReplayDocEventshook properly handles event filtering and presence management.packages/devtools/src/devtools/tabs/History.tsx (4)
18-18: LGTM! Clean import organization.The imports are well-organized and include all necessary types and hooks.
Also applies to: 23-24
29-29: LGTM! Updated function signature.The
getEventInfofunction signature correctly uses the new event type.
78-78: LGTM! Consistent hook usage.The hook usage is updated to use the new
useReplayDocEventshook.
112-112: LGTM! Consistent event type checks.The event type checks are updated to use the new
ReplayDocEventTypeenum.Also applies to: 116-116, 118-118
packages/sdk/src/document/document.ts (3)
231-234: LGTM! Clear type definition.The
DocEventstype clearly represents a collection of document events.
634-635: LGTM! Proper event stream typing.The event stream is correctly typed to use the new
DocEventstype.
772-772: LGTM! Consistent event array usage.The event array initialization and usage is consistent across different methods.
Also applies to: 1523-1523, 2035-2035, 2134-2134
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/sdk/src/devtools/types.ts (1)
124-151: Consider extracting event types array as a constant.The implementation is correct, but the array of event types could be extracted as a constant to improve maintainability and reusability.
+const REPLAY_EVENT_TYPES = [ + DocEventType.StatusChanged, + DocEventType.Snapshot, + DocEventType.LocalChange, + DocEventType.RemoteChange, + DocEventType.Initialized, + DocEventType.Watched, + DocEventType.Unwatched, + DocEventType.PresenceChanged, +] as const; export function isDocEventForReplay( event: DocEvent, ): event is DocEventForReplay { - const types = [ - DocEventType.StatusChanged, - DocEventType.Snapshot, - DocEventType.LocalChange, - DocEventType.RemoteChange, - DocEventType.Initialized, - DocEventType.Watched, - DocEventType.Unwatched, - DocEventType.PresenceChanged, - ]; - - return types.includes(event.type); + return REPLAY_EVENT_TYPES.includes(event.type); }packages/sdk/src/devtools/index.ts (1)
78-84: Add a null check for the event parameter.Consider adding a null check to handle cases where the event parameter might be undefined.
docEventsForReplayByDocKey.set(doc.getKey(), []); const unsub = doc.subscribe('all', (event) => { + if (!event) { + return; + } + if (!isDocEventsForReplay(event)) { return; }packages/sdk/src/document/document.ts (1)
Line range hint
1719-1781: Consider grouping related event types for better organization.The method handles all event types correctly, but readability could be improved by grouping related event types.
public applyDocEventForReplay(event: Devtools.DocEventForReplay<P>) { + // Status events if (event.type === DocEventType.StatusChanged) { this.applyStatus(event.value.status); if (event.value.status === DocStatus.Attached) { this.setActor(event.value.actorID); } return; } + // Snapshot events if (event.type === DocEventType.Snapshot) { const { snapshot, serverSeq, snapshotVector } = event.value; if (!snapshot) return; this.applySnapshot( BigInt(serverSeq), converter.hexToVersionVector(snapshotVector), converter.hexToBytes(snapshot), ); return; } + // Change events if ( event.type === DocEventType.LocalChange || event.type === DocEventType.RemoteChange ) { if (!event.rawChange) return; const change = Change.fromStruct<P>(event.rawChange); this.applyChange(change, event.source); } + // Client events if (event.type === DocEventType.Initialized) { const onlineClients: Set<ActorID> = new Set(); for (const { clientID, presence } of event.value) { onlineClients.add(clientID); this.presences.set(clientID, presence); } this.setOnlineClients(onlineClients); return; } + // Presence events if (event.type === DocEventType.Watched) { const { clientID, presence } = event.value; this.addOnlineClient(clientID); this.presences.set(clientID, presence); return; } if (event.type === DocEventType.Unwatched) { const { clientID } = event.value; this.removeOnlineClient(clientID); this.presences.delete(clientID); } if (event.type === DocEventType.PresenceChanged) { const { clientID, presence } = event.value; this.presences.set(clientID, presence); } }packages/devtools/src/devtools/contexts/YorkieSource.tsx (1)
143-167: Consider using a switch statement for better performance.The
getDocEventsScopefunction could be more efficient using a switch statement instead of multiple if conditions.export const getDocEventsScope = ( events: Devtools.DocEventsForReplay, ): DocEventScope => { for (const e of events) { - if ( - e.type === DocEventType.Snapshot || - e.type === DocEventType.LocalChange || - e.type === DocEventType.RemoteChange - ) { - return DocEventScope.Root; - } else if (e.type === DocEventType.StatusChanged) { - return DocEventScope.Document; - } + switch (e.type) { + case DocEventType.Snapshot: + case DocEventType.LocalChange: + case DocEventType.RemoteChange: + return DocEventScope.Root; + case DocEventType.StatusChanged: + return DocEventScope.Document; + } } return DocEventScope.Presence; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/devtools/src/devtools/contexts/YorkieSource.tsx(5 hunks)packages/devtools/src/devtools/panel/index.tsx(3 hunks)packages/devtools/src/devtools/tabs/History.tsx(3 hunks)packages/sdk/src/devtools/index.ts(3 hunks)packages/sdk/src/devtools/types.ts(2 hunks)packages/sdk/src/document/document.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/devtools/src/devtools/panel/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (18.x)
🔇 Additional comments (15)
packages/sdk/src/devtools/types.ts (3)
20-33: LGTM!The imports are well-organized and all imported types are used in the file.
102-117: LGTM!The
DocEventForReplaytype is well-defined and includes all necessary event types for document replay functionality. The use of generics (PandT) provides good flexibility for different use cases.
119-122: LGTM!The
DocEventsForReplaytype is clearly defined as an array ofDocEventForReplayevents.packages/sdk/src/devtools/index.ts (2)
17-21: LGTM!The imports are well-organized and all imported types and functions are used in the file.
28-39: LGTM!The
docEventsForReplayByDocKeyvariable is well-documented and properly typed. The global declaration and window assignment are handled correctly.packages/sdk/src/document/document.ts (5)
231-234: LGTM!The
DocEventstype is well-documented and correctly defined as an array ofDocEvent<P>.
634-635: LGTM!The event stream types are correctly updated to use the new
DocEvents<P>type.
674-676: LGTM!The event stream is correctly created using the new
DocEvents<P>type.
1783-1789: LGTM!The
applyDocEventsForReplaymethod is well-documented and correctly implements the replay functionality for multiple events.
Line range hint
2035-2134: LGTM!The event publishing in undo/redo methods is correctly updated to use the new
DocEvents<P>type and maintains consistency with other event publishing changes.packages/devtools/src/devtools/contexts/YorkieSource.tsx (3)
27-36: LGTM!The imports are well-organized and the
DocEventsForReplayContextis correctly typed with the new event types.
46-51: LGTM!The state management is correctly implemented using the new
DocEventsForReplaytype.
170-190: LGTM!The
useDocEventsForReplayhook is well-documented and correctly implements the event replay functionality with proper error handling.packages/devtools/src/devtools/tabs/History.tsx (2)
18-26: LGTM!The imports are well-organized and the
getEventInfofunction signature is correctly updated to use the newDocEventsForReplaytype.
Line range hint
111-116: LGTM!The event scope handling is correctly implemented for conditional rendering of icons based on the event type.
What this PR does / why we need it?
TransactionEventrepresented a collection ofDocEvents occurring within a single transaction (e.g. doc.update())BroadcastEvent,AuthErrorEvent, etc., we needed a more robust type definitionEventsForDocReplaytype to differentiate fromTransactionEvent, specifically designed for document replay functionality in devtoolsAny background context you want to provide?
What are the relevant tickets?
Fixes #873
Checklist
Summary by CodeRabbit
Workflow Changes
packages/devtools/package.jsonmodifications.DevTools Improvements
SDK Enhancements
Jsontype export for enhanced type definitions.These changes enhance the functionality and usability of the Yorkie DevTools and SDK.