Add Root-Only Filter Feature in History Tab #872
Conversation
WalkthroughThe recent changes enhance the Devtools UI by introducing features that allow developers to filter out the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant History
participant Panel
participant Document
participant YorkieSource
User->>History: Click toggle button
History->>Panel: Update hidePresenceTab state
Panel-->>Document: Adjust styling based on hidePresenceTab
Panel-->>YorkieSource: Update event filtering based on hidePresenceEvents
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- tools/devtools/src/devtools/panel/index.tsx (4 hunks)
- tools/devtools/src/devtools/panel/styles.css (1 hunks)
- tools/devtools/src/devtools/tabs/History.tsx (2 hunks)
Additional comments not posted (7)
tools/devtools/src/devtools/panel/index.tsx (4)
44-44: LGTM!The addition of the
hidePresenceTabstate variable is straightforward and aligns with the PR objectives.
61-67: LGTM!The conditional styling of the
Documentcomponent based on thehidePresenceTabstate is efficient and maintains a clean UI.
127-128: LGTM!The addition of
hidePresenceTabandsetHidePresenceTabprops to theHistorycomponent is necessary for the new feature and is implemented correctly.
139-149: LGTM!The conditional rendering of the
Presencecomponent based on thehidePresenceTabstate aligns with the PR objectives and maintains a clean UI.tools/devtools/src/devtools/tabs/History.tsx (2)
66-67: LGTM!The addition of
hidePresenceTabandsetHidePresenceTabprops to theHistorycomponent is necessary for the new feature and is implemented correctly.
185-191: LGTM!The button to toggle the
hidePresenceTabstate is implemented correctly and provides a clear user interface. The button text updates accordingly based on the state.tools/devtools/src/devtools/panel/styles.css (1)
Line range hint
1-1: LGTM!The removal of styling properties related to width and border for the
.yorkie-rootclass aligns with the changes made to theDocumentcomponent's style inindex.tsx.
|
I've temporarily added a filter icon represented by Moreover, I'm encountering a TypeScript error in (I forgot to mention that the error has been occurring since I first pulled the repository. It happens in the 
|
|
@gwbaik9717
Please let me know if you have any questions or need further clarification. Looking forward to your updates! |
|
This commit adds a Root-Only Filter to the History tab, allowing users to view only changes to the Root of the Document. This feature helps developers focus specifically on content changes by filtering out the more frequent Presence events that track user presence information such as cursors.
gwbaik9717
force-pushed
the
enhance-devtools-ui-with-root-only-filter
branch
from
85cec51
to
e191153
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- tools/devtools/src/devtools/panel/index.tsx (5 hunks)
- tools/devtools/src/devtools/panel/styles.css (1 hunks)
- tools/devtools/src/devtools/tabs/History.tsx (6 hunks)
Files skipped from review as they are similar to previous changes (3)
- tools/devtools/src/devtools/panel/index.tsx
- tools/devtools/src/devtools/panel/styles.css
- tools/devtools/src/devtools/tabs/History.tsx
This enhancement allows users to toggle the visibility of the Presence tab, providing a more focused view of the content they are interested in.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (4)
tools/devtools/src/devtools/tabs/Document.tsx (1)
26-26: Update function signature in comments and documentation.The function signature has been updated to include
hidePresenceTabandsetHidePresenceTab. Ensure that comments and documentation reflect this change.tools/devtools/src/devtools/panel/index.tsx (3)
63-64: Ensure consistent naming for state variables.The state variable
hidePresenceTabis used to manage the visibility of the presence tab. Ensure consistent naming conventions for state variables throughout the codebase.
140-142: Ensure consistent prop naming in components.The props
hidePresenceEventandsetHidePresenceEventare passed to theHistorycomponent. Ensure consistent naming conventions for props throughout the codebase.
153-163: Ensure consistent styling and prop usage.The
Documentcomponent is updated to receivehidePresenceTabandsetHidePresenceTabas props. Ensure consistent styling and prop usage throughout the codebase.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- tools/devtools/src/devtools/panel/index.tsx (5 hunks)
- tools/devtools/src/devtools/panel/styles.css (3 hunks)
- tools/devtools/src/devtools/tabs/Document.tsx (2 hunks)
- tools/devtools/src/devtools/tabs/History.tsx (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- tools/devtools/src/devtools/panel/styles.css
- tools/devtools/src/devtools/tabs/History.tsx
Additional comments not posted (1)
tools/devtools/src/devtools/panel/index.tsx (1)
Line range hint
83-98: Ensure proper handling of side effects in useEffect.The
useEffecthook should handle side effects properly. Consider adding cleanup functions if necessary.
|
I've found a bug in the slider component: when there is only one dot, a redundant trail ( 
|
This commit refactors the Root-Only Filter feature in the History tab. The filter logic has been moved inside useTransactionEvents to prevent props drilling and to group related functionalities by domain.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- tools/devtools/src/devtools/contexts/YorkieSource.tsx (4 hunks)
- tools/devtools/src/devtools/panel/index.tsx (4 hunks)
- tools/devtools/src/devtools/tabs/History.tsx (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- tools/devtools/src/devtools/panel/index.tsx
Additional comments not posted (12)
tools/devtools/src/devtools/contexts/YorkieSource.tsx (6)
17-23: LGTM!The new imports for
DispatchandSetStateActionare appropriate for managing state in the context.
36-40: LGTM!The
TransactionEventsContexthas been appropriately enhanced to includeevents,hidePresenceEvents, andsetHidePresenceEvents.
53-55: LGTM!The state variable
hideTransactionPresenceEventsis necessary for managing the visibility of presence events.
110-116: LGTM!The
TransactionEventsContext.Providervalue has been correctly updated to includeevents,hidePresenceEvents, andsetHidePresenceEvents.
143-146: LGTM!The
TransactionEventTypeenum provides clear categorization of transaction events intoDocumentandPresence.
148-163: LGTM!The
getTransactionEventTypefunction correctly determines the type of transaction event based on its properties.tools/devtools/src/devtools/tabs/History.tsx (6)
22-26: LGTM!The new imports for
TransactionEventType,getTransactionEventType, anduseTransactionEventsare necessary for the enhanced functionality of theHistorycomponent.
74-75: LGTM!The
useTransactionEventshook is appropriately used to accessevents,hidePresenceEvents, andsetHidePresenceEvents.
84-90: LGTM!The
toggleHidePresenceEventfunction correctly manages the visibility of presence events and resets the selected event index.
103-117: LGTM!The
useEffecthook correctly sets the slider marks based on the transaction event type.
194-196: LGTM!The button correctly toggles the visibility of presence events and updates its label based on the current state.
213-213: LGTM!The modification of the slider step value from
1to0allows for finer control in the event selection.
|
@gwbaik9717 // History.tsx
<div
ref={scrollRef}
style={{ width: '100%', overflowX: 'auto', minHeight: '46px' }}
className="history-slider-wrap" // added
data-length={events.length} // added
>
<Slider dots ... />
</div>/* slider.css */
.history-slider-wrap[data-length='1'] .rc-slider-rail {
display: none;
} |
This commit fixes a bug in the slider component: when there is only one dot, a redundant trail (-) appears next to its node.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- tools/devtools/src/devtools/panel/slider.css (1 hunks)
- tools/devtools/src/devtools/tabs/History.tsx (7 hunks)
Files skipped from review due to trivial changes (1)
- tools/devtools/src/devtools/panel/slider.css
Additional comments not posted (6)
tools/devtools/src/devtools/tabs/History.tsx (6)
22-26: LGTM! New imports are necessary and correctly used.The new imports
TransactionEventType,getTransactionEventType, anduseTransactionEventsare essential for the added functionality.
74-75: LGTM! New state variables are correctly initialized and used.The new state variables
hidePresenceEventsandsetHidePresenceEventsare correctly initialized and used in the code.
84-90: LGTM! New methodtoggleHidePresenceEventis correctly implemented and used.The method
toggleHidePresenceEventis correctly implemented and updates the state as expected.
107-108: LGTM! New methodgetTransactionEventTypeis correctly used.The method
getTransactionEventTypeis correctly used to determine the transaction event type.
Line range hint
134-196:
LGTM! New UI components are correctly implemented and enhance the user experience.The new button to toggle presence events visibility and adjustments to the slider component are correctly implemented and enhance the user experience.
206-215: LGTM! Adjustments to the slider component are correctly implemented.The slider component's step value adjustment from 1 to 0 provides finer control over event selection.
|
Thanks for the reply! I've fixed the bug as you suggested. |
|
@chacha912 If it is okay, I'll assign myself to this issue and proceed with the update. (new Issue) |
There was a problem hiding this comment.
Thanks for doing this! Added some nits but overall I love what I'm seeing here :)
- When we filter out presence events, they won't be applied to the document, which makes the information in the Presence tab inaccurate. In this case, should we set hidePresenceTab to true? Or would it be better to add a note informing users about this limitation? I'd like to hear your thoughts on this approach.
|
Regarding the issue of inaccurate Presence tab information when presence events are filtered out, I have not yet fully understand the root cause of the issue. Currently, it seems like the const presences = [doc.getSelfForTest(), ...doc.getOthersForTest()];And the inaccurate presence tab issue occurs, because doc.getOthersFortTest returns an empty array 
However, the part I do not get is why it is empty. As soon as the presence events filter is on, below code will fire. useEffect(() => {
if (selectedEventIndexInfo.index === null) return;
const doc = new yorkie.Document(currentDocKey);
for (let i = 0; i <= selectedEventIndexInfo.index; i++) {
doc.applyTransactionEvent(events[i]);
}
setDoc(doc);
setSelectedEvent(events[selectedEventIndexInfo.index]);
}, [selectedEventIndexInfo]);I believe so I expected it would also set the presences information of filtered out event accordingly. There must be some parts I'm missing, so could you help me out here? |
|
Currently, we're building the document based on the point selected in the history slider. When useEffect(() => {
if (selectedEventIndexInfo.index === null) return;
// Create a new document and apply events up to the selected point
// to recreate the document state at that moment.
const doc = new yorkie.Document(currentDocKey);
for (let i = 0; i <= selectedEventIndexInfo.index; i++) {
doc.applyTransactionEvent(events[i]);
}
setDoc(doc);
setSelectedEvent(events[selectedEventIndexInfo.index]);
}, [selectedEventIndexInfo]);When building the document from events, we use the A potential solution could be to maintain an original, unfiltered array of events. Even when filtering is applied, we could use this original event array to build the document. This would ensure that all necessary information, including presence data, is incorporated into the document state. I'd be happy to discuss further or explore alternative solutions. |
This commit fixes the issue where presence events were filtered out, causing them not to be applied to the document and resulting in inaccurate information in the Presence tab.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- tools/devtools/src/devtools/contexts/YorkieSource.tsx (4 hunks)
- tools/devtools/src/devtools/panel/index.tsx (5 hunks)
- tools/devtools/src/devtools/tabs/History.tsx (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- tools/devtools/src/devtools/panel/index.tsx
- tools/devtools/src/devtools/tabs/History.tsx
Additional comments not posted (7)
tools/devtools/src/devtools/contexts/YorkieSource.tsx (7)
17-23: Ensure all necessary imports are present.The import statements look correct and necessary for the changes made.
36-40: Context definition looks good.The
TransactionEventsContextis correctly defined with the new state variables.
53-55: State variables are correctly initialized.The state variables for hiding presence events are correctly initialized.
110-116: Provider value is correctly set.The
TransactionEventsContext.Providervalue correctly includes the new state variables.
143-146: Enum definition looks good.The
TransactionEventTypeenum is correctly defined withDocumentandPresencetypes.
148-163: Function implementation looks good.The
getTransactionEventTypefunction is correctly implemented to determine the event type.
166-207: Hook implementation looks good.The
useTransactionEventshook is correctly implemented to include logic for filtering presence events.
This commit addresses the problem of certain icons appearing incorrectly or as squares in some browsers.
This commit adds presence event filter button which helps users understand the role of the button.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- tools/devtools/src/devtools/tabs/Document.tsx (2 hunks)
- tools/devtools/src/devtools/tabs/History.tsx (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- tools/devtools/src/devtools/tabs/Document.tsx
- tools/devtools/src/devtools/tabs/History.tsx
|
Thank you for the detailed explanation. I have resolved the issue regarding the inaccurate Presence tab by maintaining both the 'original' and 'filtered' arrays of events. (also applied previous comments in terms of icon issues, and title) Basically, It works as follows. When a user clicks an event in the History tab with the presence event filter applied, the original array is referenced to build the document up to that event. To map the filtered index to the original array index, I created variable named const presenceMarkedEvents = useMemo(() => {
if (!hidePresenceEvents) {
return events;
}
return events.map((event) => {
if (getTransactionEventType(event) === TransactionEventType.Presence) {
return null;
}
return event;
});
}, [events, hidePresenceEvents]);In theory, instead of using this separate mapping variable, I could have implemented by checking the type of each event (Presence or Document) up to the selected event every time the user clicks. Just to clarify, it would roughly look like below. useEffect(() => {
if (selectedEventIndexInfo.index === null) return;
const doc = new yorkie.Document(currentDocKey);
let eventIndex = 0;
let filteredEventIndex = 0;
while (
eventIndex < originalEvents.length &&
filteredEventIndex <= selectedEventIndexInfo.index
) {
// it is checking the type of every single event until the selected event, and it will repeat every time user selects
if (
getTransactionEventType(originalEvents[eventIndex]) ===
TransactionEventType.Document
) {
filteredEventIndex++;
}
doc.applyTransactionEvent(originalEvents[eventIndex]);
eventIndex++;
}
setDoc(doc);
setSelectedEvent(events[selectedEventIndexInfo.index]);
}, [selectedEventIndexInfo]);However, this approach would require recalculating the event types every time an event is clicked, which is less efficient compared to calculating the types once when the events change. Therefore, I opted for the current method for better performance. If you have better idea, please let me know. |
There was a problem hiding this comment.
Thank you for your hard work on this. I love all the changes you've made!
However, I had a small concern about needing to maintain presenceMarkedEvents to show the selected document. So, I've made a tiny modification to your changes - would you mind checking if it looks okay? Everything else looks great! :)
Make code more readable by adding enhancedEvents (which adds metadata to events)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- tools/devtools/src/devtools/contexts/YorkieSource.tsx (4 hunks)
- tools/devtools/src/devtools/panel/index.tsx (5 hunks)
- tools/devtools/src/devtools/tabs/History.tsx (7 hunks)
Files skipped from review as they are similar to previous changes (3)
- tools/devtools/src/devtools/contexts/YorkieSource.tsx
- tools/devtools/src/devtools/panel/index.tsx
- tools/devtools/src/devtools/tabs/History.tsx
This commit adds a Root-Only Filter in the History tab, allowing users to focus exclusively on changes to the Root of the Document. Previously, the History tab logs both content changes and Presence events (such as cursor movements), which can result in a high volume of events that may not always be relevant for debugging content changes.
This commit adds a Root-Only Filter in the History tab, allowing users to focus exclusively on changes to the Root of the Document. Previously, the History tab logs both content changes and Presence events (such as cursor movements), which can result in a high volume of events that may not always be relevant for debugging content changes.
What this PR does / why we need it?
This pull request adds a Root-Only Filter in the History tab, allowing users to focus exclusively on changes to the Root of the Document. Currently, the History tab logs both content changes and Presence events (such as cursor movements), which can result in a high volume of events that may not always be relevant for debugging content changes.
Any background context you want to provide?
What are the relevant tickets?
Fixes #854
Checklist
Summary by CodeRabbit
New Features
Style