Moc 96 use dsl for editing #105
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe recent changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatToast
participant ContentApp
participant Reactor
User->>ContentApp: Trigger Content
ContentApp->>Reactor: Modify DOM
Reactor->>ChatToast: Update Toast
ChatToast->>Reactor: Serialize HTML to JSON
Reactor->>ContentApp: Return JSON Data
ContentApp->>User: Render Updated Content
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: 1
Outside diff range, codebase verification and nitpick comments (3)
packages/reactor/interfaces.ts (1)
42-56: The new eventsonUrlLoadedandonPageLoadedare defined but not used anywhere in the codebase.
onUrlLoadedandonPageLoadedare only defined inpackages/reactor/interfaces.ts.- There are no occurrences of these events being used, registered, or triggered in the codebase.
This indicates that the implementation is incomplete or these events are intended for future use. Please review and ensure that these events are integrated and utilized as intended.
Analysis chain
LGTM! Verify the usage of the new events.
The
ReactorEventsinterface defines events foronUrlLoadedandonPageLoaded, providing triggers for the plugin to request modifications based on the current URL or page context.Ensure that the new events are used correctly throughout the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new events `onUrlLoaded` and `onPageLoaded` in the codebase. # Test: Search for the usage of `onUrlLoaded` event. Expect: Occurrences of `onUrlLoaded`. rg --type ts 'onUrlLoaded' # Test: Search for the usage of `onPageLoaded` event. Expect: Occurrences of `onPageLoaded`. rg --type ts 'onPageLoaded'Length of output: 205
Script:
#!/bin/bash # Description: Verify the usage of the new events `onUrlLoaded` and `onPageLoaded` in the codebase. # Test: Search for event listeners or handlers that might use `onUrlLoaded`. rg --type ts 'onUrlLoaded' # Test: Search for event listeners or handlers that might use `onPageLoaded`. rg --type ts 'onPageLoaded' # Test: Search for any event registration or trigger related to `onUrlLoaded`. rg --type ts 'addEventListener' -A 5 # Test: Search for any event registration or trigger related to `onPageLoaded`. rg --type ts 'addEventListener' -A 5 # Test: Search for any event registration or trigger related to `onUrlLoaded`. rg --type ts 'dispatchEvent' -A 5 # Test: Search for any event registration or trigger related to `onPageLoaded`. rg --type ts 'dispatchEvent' -A 5Length of output: 6483
packages/reactor/utils.ts (2)
49-51: Improve error logging.The current error logging could be more informative by including the error message.
- console.warn(`Invalid selector: ${mod.selector ? mod.selector : mod.xpath}`); + console.warn(`Invalid selector: ${mod.selector ? mod.selector : mod.xpath}. Error: ${e.message}`);
56-60: Improve element not found logging.The current logging could be more informative by including the modification details.
- console.warn(`Element not found for selector: ${mod.selector ? mod.selector : mod.xpath}`); + console.warn(`Element not found for selector: ${mod.selector ? mod.selector : mod.xpath}. Modification: ${JSON.stringify(mod)}`);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (10)
- apps/mocksi-lite/content/ContentApp.tsx (2 hunks)
- apps/mocksi-lite/content/Toast/ChatToast.tsx (5 hunks)
- apps/mocksi-lite/package.json (1 hunks)
- apps/mocksi-lite/utils.ts (2 hunks)
- packages/reactor/index.ts (1 hunks)
- packages/reactor/interfaces.ts (2 hunks)
- packages/reactor/main.ts (2 hunks)
- packages/reactor/tests/index.test.ts (2 hunks)
- packages/reactor/tests/main.test.ts (1 hunks)
- packages/reactor/utils.ts (3 hunks)
Additional comments not posted (26)
packages/reactor/index.ts (1)
1-1: Ensure new functions are correctly exported.The export statement now includes
modifyDomandhtmlElementToJson. Ensure these functions exist and are correctly implemented in the module.Verification successful
The new functions are correctly exported.
The functions
modifyDomandhtmlElementToJsonare defined and exported inpackages/reactor/main.tsas expected.
modifyDomis defined inpackages/reactor/main.ts.htmlElementToJsonis defined inpackages/reactor/main.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of `modifyDom` and `htmlElementToJson` in the module. # Test: Search for the function definitions. Expect: Functions are defined in the module. rg --type ts 'export function modifyDom' packages/reactor/main.ts rg --type ts 'export function htmlElementToJson' packages/reactor/main.tsLength of output: 143
Script:
#!/bin/bash # Description: Verify the existence and export of `modifyDom` and `htmlElementToJson` in the module. # Test: Search for the function definitions and export statements. Expect: Functions are defined and exported in the module. rg 'modifyDom' --type ts rg 'htmlElementToJson' --type tsLength of output: 1304
apps/mocksi-lite/package.json (1)
29-29: Ensure the new dependency is correctly added.The new dependency
@repo/reactorhas been added. Ensure the dependency exists and is correctly versioned.Verification successful
Ensure the new dependency is correctly added.
The new dependency
@repo/reactorhas been correctly added and is being used in the project.
pnpm-lock.yamlpackages/reactor/package.jsonapps/mocksi-lite/package.jsonapps/mocksi-lite/content/ContentApp.tsxapps/mocksi-lite/content/Toast/ChatToast.tsxScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and version of `@repo/reactor`. # Test: Check the package version. Expect: Package is correctly versioned. npm show @repo/reactor versionLength of output: 103
Script:
#!/bin/bash # Description: Verify the existence of `@repo/reactor` dependency in the repository. # Test: Search for the `@repo/reactor` dependency declaration in the repository. rg '@repo/reactor'Length of output: 374
packages/reactor/tests/index.test.ts (2)
23-49: Ensure the new test case formodifyDomis correct and comprehensive.The new test case for
modifyDommodifies the DOM directly. Ensure the test case is correct, comprehensive, and follows best practices.
3-3: Ensure the new imports are correct.The new imports include
ModificationRequest,modifyDom, andmodifyHtml. Ensure these imports are correctly used in the test cases.Verification successful
The new imports are correctly used.
The imports for
ModificationRequest,modifyDom, andmodifyHtmlare present and correctly used in the test cases.
packages/reactor/tests/index.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of `ModificationRequest`, `modifyDom`, and `modifyHtml`. # Test: Check the imports. Expect: Imports are correctly used in the test cases. rg --type ts 'import { type ModificationRequest, modifyDom, modifyHtml }' packages/reactor/tests/index.test.tsLength of output: 262
Script:
#!/bin/bash # Description: Verify the existence and usage of `ModificationRequest`, `modifyDom`, and `modifyHtml`. # Test: Check the imports. Expect: Imports are correctly used in the test cases. rg --type ts 'import \{ type ModificationRequest, modifyDom, modifyHtml \}' packages/reactor/tests/index.test.ts # Test: Verify the usage of `ModificationRequest`, `modifyDom`, and `modifyHtml`. Expect: They are used in the test cases. rg --type ts 'ModificationRequest|modifyDom|modifyHtml' packages/reactor/tests/index.test.tsLength of output: 587
packages/reactor/interfaces.ts (4)
34-39: LGTM!The
DomJsonExportNodeinterface provides a detailed representation of elements, including their attributes and children.
3-8: LGTM! Verify the usage of new properties and action type.The new properties
selectorandxpath, and the action typereplaceAllenhance the flexibility of theModificationinterface.Ensure that the new properties and action type are used correctly throughout the codebase.
Verification successful
The new properties
selectorandxpath, and the action typereplaceAllare correctly integrated and used throughout the codebase.
selectoris used inpackages/reactor/utils.ts,packages/reactor/tests, andapps/mocksi-lite.xpathis used inpackages/reactor/utils.tsandpackages/reactor/tests.replaceAllis used inapps/mocksi-lite/utils.ts,packages/reactor/utils.ts,packages/reactor/tests, andpackages/dodom/receivers.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new properties and action type in the codebase. # Test: Search for the usage of `selector` property. Expect: Occurrences of `selector`. rg --type ts 'selector' # Test: Search for the usage of `xpath` property. Expect: Occurrences of `xpath`. rg --type ts 'xpath' # Test: Search for the usage of `replaceAll` action type. Expect: Occurrences of `replaceAll`. rg --type ts 'replaceAll'Length of output: 3498
30-32: LGTM! Verify the usage of the new property.The new property
modificationRequestin theAppliedModificationinterface provides a structured way to link applied modifications to their requests.Ensure that the new property is used correctly throughout the codebase.
Verification successful
Let's gather more context around the usages of
modificationRequestto ensure they align with the new interface definition and are used correctly.
Verified!
The new property
modificationRequestin theAppliedModificationinterface is used correctly and consistently throughout the codebase.
main.ts: The property is assigned and passed to functions handling modifications.index.test.ts: The property is used in test cases to ensure correct functionality.interfaces.ts: The interface documents the expected structure and usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new property `modificationRequest` in the codebase. # Test: Search for the usage of `modificationRequest` property. Expect: Occurrences of `modificationRequest`. rg --type ts 'modificationRequest'Length of output: 800
Script:
#!/bin/bash # Description: Gather context around the usage of `modificationRequest` property. # Extract lines around the occurrences of `modificationRequest` in `main.ts`, `interfaces.ts`, and `index.test.ts`. rg --type ts 'modificationRequest' -A 3 -B 3Length of output: 2899
64-107: LGTM! Verify the usage of the new methods and events.The
Reactorinterface provides a comprehensive set of methods and events for managing the reactor's lifecycle and modifications.Ensure that the new methods and events are used correctly throughout the codebase.
apps/mocksi-lite/content/ContentApp.tsx (2)
40-40: LGTM!The function
htmlElementToJsonis used to serialize the document body into JSON, enhancing the robustness of the JSON conversion process.
1-1: LGTM! Verify the usage of the new function.The import statement for
innerHTMLToJsonhas been removed and replaced withhtmlElementToJson.Ensure that the new function
htmlElementToJsonis used correctly in the file.Verification successful
The function
htmlElementToJsonis correctly imported and used inContentApp.tsx.The import statement for
innerHTMLToJsonhas been replaced withhtmlElementToJson, and the new function is correctly utilized in the file.
ContentApp.tsx: Correct usage ofhtmlElementToJsonobserved.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new function `htmlElementToJson` in the file. # Test: Search for the usage of `htmlElementToJson` function. Expect: Occurrences of `htmlElementToJson`. rg --type tsx 'htmlElementToJson'Length of output: 61
Script:
#!/bin/bash # Description: Verify the usage of the new function `htmlElementToJson` in the file. # Test: Search for the usage of `htmlElementToJson` function in .tsx files. Expect: Occurrences of `htmlElementToJson`. rg 'htmlElementToJson' --glob '*.tsx' -A 3Length of output: 1785
packages/reactor/main.ts (3)
25-35: LGTM!The function
modifyDomapplies modifications to the DOM based on aModificationRequestobject, encapsulating error handling and using the existinggenerateModificationsutility.
37-99: LGTM!The function
htmlElementToJsonconverts anHTMLElementinto a JSON representation, capturing the structure and attributes of the DOM elements, and includes logic to handle different node types and apply filters.
102-115: LGTM!The function
textElementFilterfilters out non-visible text nodes, ensuring that only meaningful nodes are processed in the JSON conversion.packages/reactor/utils.ts (7)
87-89: LGTM!The code changes are approved.
134-134: LGTM!The code changes are approved.
135-158: LGTM!The code changes are approved.
160-175: LGTM!The code changes are approved.
177-187: LGTM!The code changes are approved.
189-203: LGTM!The code changes are approved.
21-25: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
generateModificationsmatch the new logic and handle the updatedelementstype correctly.packages/reactor/tests/main.test.ts (1)
29-83: LGTM!The code changes are approved.
apps/mocksi-lite/content/Toast/ChatToast.tsx (4)
1-9: LGTM!The code changes are approved.
103-107: LGTM!The code changes are approved.
128-129: LGTM!The code changes are approved.
175-175: LGTM!The code changes are approved.
apps/mocksi-lite/utils.ts (1)
38-38: Verify the definition and import ofSignupURL.Ensure that
SignupURLis correctly defined and imported to avoid runtime errors.Verification successful
The definition and import of
SignupURLare correct.
SignupURLis defined inapps/mocksi-lite/consts.tsand is correctly used inapps/mocksi-lite/utils.tsand other files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and import of `SignupURL`. # Test: Search for the definition and import of `SignupURL`. Expect: Only occurances of the correct definition and import. rg --type js --type ts --type tsx 'SignupURL'Length of output: 73
Script:
#!/bin/bash # Description: Verify the definition and import of `SignupURL`. # Test: Search for the definition and import of `SignupURL`. Expect: Only occurrences of the correct definition and import. rg --type js --type ts 'SignupURL'Length of output: 556
packages/reactor/utils.ts
Outdated
| // construct a new NodeListOf<Element> from items found by the xpath | ||
| elements = []; | ||
| if (!mod.xpath.startsWith("//html")) { | ||
| mod.xpath = `//html/${mod.xpath}`; | ||
| } | ||
| const xpath = document.evaluate( | ||
| mod.xpath, | ||
| doc, | ||
| null, | ||
| XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, | ||
| null, | ||
| ); | ||
| for (let i = 0; i < xpath.snapshotLength; i++) { | ||
| const item = xpath.snapshotItem(i); | ||
| if (item !== null && item instanceof Element) { | ||
| elements.push(item); | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Improve XPath construction logic.
The current logic constructs a new XPath expression if it does not start with //html. This can be improved by ensuring the XPath expression is always valid and scoped correctly.
- if (!mod.xpath.startsWith("//html")) {
- mod.xpath = `//html/${mod.xpath}`;
- }
+ if (!mod.xpath.startsWith("/")) {
+ mod.xpath = `//${mod.xpath}`;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // construct a new NodeListOf<Element> from items found by the xpath | |
| elements = []; | |
| if (!mod.xpath.startsWith("//html")) { | |
| mod.xpath = `//html/${mod.xpath}`; | |
| } | |
| const xpath = document.evaluate( | |
| mod.xpath, | |
| doc, | |
| null, | |
| XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, | |
| null, | |
| ); | |
| for (let i = 0; i < xpath.snapshotLength; i++) { | |
| const item = xpath.snapshotItem(i); | |
| if (item !== null && item instanceof Element) { | |
| elements.push(item); | |
| } | |
| } | |
| } else { | |
| // construct a new NodeListOf<Element> from items found by the xpath | |
| elements = []; | |
| if (!mod.xpath.startsWith("/")) { | |
| mod.xpath = `//${mod.xpath}`; | |
| } | |
| const xpath = document.evaluate( | |
| mod.xpath, | |
| doc, | |
| null, | |
| XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, | |
| null, | |
| ); | |
| for (let i = 0; i < xpath.snapshotLength; i++) { | |
| const item = xpath.snapshotItem(i); | |
| if (item !== null && item instanceof Element) { | |
| elements.push(item); | |
| } | |
| } | |
| } else { |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/reactor/interfaces.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/reactor/interfaces.ts
|
I like it! Your approach reminds me of Reactive JS - it was really ahead of its time. It's great to see Reactor living up to its name. I named this package Reactor not only to signal it does more than just DOM changes, but also to hint at its event-based/reactive nature. Let's address your questions:
|
I think we can potentially reuse some of this observermutation setup to listen for SPA changes, I might push back on that being a short term goal though since we're disabling most interactions at the moment so for now we can probably get away with assuming you're on a static page and add that functionality in later. |
|
One thing we will need and not sure how it fits into reactor is the ability to say "I want to do this same edit to all matching nodes on the page" vs "I only want to target this specific node even if there are other identical ones on the page" Potentially we'll need to store a "path" to the node (ie. think like a folder tree, and map that concept to the dom tree). |
|
Can we maybe add a "bulk add" interface, I know under the hood it'll still be a loop but maybe would help clean up the usage if rather than the user needing to loop over each modification individually we can just pass an array of modifications to a function |
|
Does reactor itself need to have these |
|
For #2 I might argue that it makes sense to have Reactor do the highlighting since it knows where the modifications are and can keep better track of them, but maybe we do it through a function where reactor gives the consumer the x/y coordinates and the consumer is responsible for actually drawing the box, that way its a bit agnostic to changes or maybe different types of highlighting etc |
|
superseded by #148 but keeping around for reference. |
|
Closing this PR due to multiple semantic conflicts throughout the code. Please review the changes and resolve these conflicts before reopening the PR. |
Here are some thoughts on interfaces for a Reactor class that could be used to by the editor to add DSL support. The plugin would attach a Reactor class to the DOM. This class would maintain a list of ModificationRequests, and apply those modifications to the DOM. It would also listen for mutations and re-apply the modifications as appropriate.
The Reactor defines two core events: onUrlLoaded and onPageLoaded - these are triggers that the plugin could use to query the backend for the right list of modifications. The specification for a "page" load, which is meant to be when the page changes on an SPA, is TBD.
The Reactor does not handle any interactions with the backend or user. Those - for storing modifications, editing, etc - would be handled in the plugin. The Reactor would only be used to apply the modifications to the live DOM.
Below is some pseudo-code of how I imagine Reactor being used:
this one in the list, then remove the modification and reapply all later modifications
My current questions are:
Summary by CodeRabbit
New Features
replaceAll.Bug Fixes
Documentation
Tests