feature/moc 19 create image replacer pt3 #62
Conversation
WalkthroughWalkthroughThese changes transition the Changes
Poem
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 Configration 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: 9
Outside diff range and nitpick comments (2)
apps/mocksi-lite/content/content.tsx (1)
Line range hint
31-68: Refactor the error handling to separate concerns and improve maintainability.- chrome.storage.local.clear(); - console.log("Error getting data from storage: ", error); - window.open(SignupURL); + handleStorageError(error);Consider adding a new function
handleStorageErrorthat encapsulates the error handling logic. This would make the code cleaner and more maintainable.apps/mocksi-lite/content/EditMode/editMode.ts (1)
Line range hint
11-30: Refactor to separate concerns between UI updates and storage operations.- if (recordingId) { - persistModifications(recordingId); - } - undoModifications(); - await chrome.storage.local.set({ - [MOCKSI_RECORDING_STATE]: RecordingState.CREATE, - }); - await chrome.storage.local.remove(MOCKSI_RECORDING_ID); - document.body.removeEventListener("dblclick", onDoubleClickText); - restoreNodes(); - ContentHighlighter.removeHighlightNodes(); - cancelEditWithoutChanges(document.getElementById("mocksiSelectedText")); - document.normalize(); + handleStorageAndUIChanges(recordingId);Introducing a new function
handleStorageAndUIChangesthat encapsulates the storage and UI related operations will make the code cleaner and more maintainable, separating the concerns effectively.
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 (14)
- apps/mocksi-lite/commands/Command.ts (1 hunks)
- apps/mocksi-lite/content/ContentApp.tsx (5 hunks)
- apps/mocksi-lite/content/EditMode/actions.ts (2 hunks)
- apps/mocksi-lite/content/EditMode/editMode.ts (4 hunks)
- apps/mocksi-lite/content/RecordButton.tsx (2 hunks)
- apps/mocksi-lite/content/content.tsx (2 hunks)
- apps/mocksi-lite/package.json (2 hunks)
- apps/mocksi-lite/utils.ts (6 hunks)
- packages/dodom/index.ts (1 hunks)
- packages/dodom/package.json (1 hunks)
- packages/dodom/receivers/ShadowDOMManipulator.ts (2 hunks)
- packages/dodom/tests/receivers/ShadowDOMManipulator.test.ts (1 hunks)
- packages/dodom/tests/setupTests.ts (1 hunks)
- packages/dodom/tsconfig.json (1 hunks)
Files not reviewed due to errors (1)
- apps/mocksi-lite/utils.ts (no review received)
Files skipped from review due to trivial changes (3)
- packages/dodom/index.ts
- packages/dodom/package.json
- packages/dodom/tsconfig.json
Additional context used
Biome
packages/dodom/receivers/ShadowDOMManipulator.ts
[error] 89-89: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
Additional comments not posted (16)
packages/dodom/tests/setupTests.ts (1)
11-11: The lint ignore comment is appropriate for test environments where performance is less critical.apps/mocksi-lite/package.json (1)
5-6: Updated type definitions are appropriate and help maintain compatibility and accuracy in type checking.apps/mocksi-lite/content/EditMode/actions.ts (1)
22-33: Integration ofShadowDOMManipulatoraligns with the PR goals. Consider verifying or testing the necessity ofapplyPatterns()as indicated by the TODO comment.Verification successful
Integration of
ShadowDOMManipulatoraligns with the PR goals. Consider verifying or testing the necessity ofapplyPatterns()as indicated by the TODO comment.
[task review_file_3/4 comments_1/1] Re-evaluate the approach to verify the necessity and functionality of
applyPatterns().
[observations] The previous script failed because it used an unrecognized file type forrg.
[decomposition] Identify the correct file type or language used for tests and re-run the search.
[task review_file_3/4 comments_1/1 sub-task_1/2] Determine the file type or language used for tests.
[analysis] Review common file types used for tests in JavaScript/TypeScript projects (e.g.,.js,.ts,.test.js,.spec.js, etc.).
[actions] Create a new script to search forapplyPatterns()in these file types.-->
The search results indicate that
applyPatterns()is indeed being called in test files, specifically inpackages/dodom/tests/receivers/ShadowDOMManipulator.test.ts. There is also a FIXME comment suggesting thatapplyPatternsshould be called automatically when the DOM changes, which aligns with the TODO comment in the provided code snippet.
packages/dodom/tests/receivers/ShadowDOMManipulator.test.ts: Contains test cases that callapplyPatterns().packages/dodom/receivers/ShadowDOMManipulator.ts: Contains the implementation ofapplyPatterns().Given this context, it is evident that
applyPatterns()is being tested, and there is an indication that its necessity is under consideration.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the necessity and functionality of `applyPatterns()` in different scenarios. # Test: Search for test cases covering `applyPatterns()`. Expect: Specific test cases that validate its functionality. rg --type python $'applyPatterns()'Length of output: 67
Script:
#!/bin/bash # Description: Verify the necessity and functionality of `applyPatterns()` in different scenarios. # Test: Search for test cases covering `applyPatterns()`. Expect: Specific test cases that validate its functionality. rg --type python $'applyPatterns()'Length of output: 67
Script:
#!/bin/bash # Description: Verify the necessity and functionality of `applyPatterns()` in different scenarios. # Test: Search for test cases covering `applyPatterns()`. Expect: Specific test cases that validate its functionality. rg 'applyPatterns()' --type js --type ts --type 'test.js' --type 'spec.js'Length of output: 104
Script:
#!/bin/bash # Description: Verify the necessity and functionality of `applyPatterns()` in different scenarios. # Test: Search for test cases covering `applyPatterns()`. Expect: Specific test cases that validate its functionality. rg 'applyPatterns()' --glob '*.js' --glob '*.ts' --glob '*.test.js' --glob '*.test.ts' --glob '*.spec.js' --glob '*.spec.ts'Length of output: 1029
apps/mocksi-lite/commands/Command.ts (1)
44-97: Transition tochrome.storage.localand use of async methods are well-implemented. Ensure robust error handling, especially for operations like JSON parsing and storage interactions.apps/mocksi-lite/content/content.tsx (1)
60-60: Good use of existing utilities to manage UI state based on storage data.The integration of
setRootPositionwith thechrome.storage.localdata retrieval is well-implemented, ensuring that UI updates reflect the current state accurately.apps/mocksi-lite/content/RecordButton.tsx (1)
61-71: Good implementation of state transitions usingchrome.storage.local.The use of
chrome.storage.localto manage state transitions is well implemented, ensuring that state changes are persisted across sessions.packages/dodom/tests/receivers/ShadowDOMManipulator.test.ts (3)
36-38: Correctly implemented test for error handling when an image is not found.This test correctly asserts that an error is thrown when an image source does not exist, which is crucial for robust error handling in the image replacement feature.
66-73: Correct implementation of multiple pattern handling in tests.The test case effectively checks the application of multiple text replacement patterns, ensuring that the
ShadowDOMManipulatorcan handle complex scenarios.
40-46: Verify that the text replacement actually modifies the DOM as expected.This script ensures that changes made by the
textContentproperty are accurately reflected in the DOM, which is crucial for the integrity of the test.Verification successful
The
addPatternmethod in theShadowDOMManipulatorclass correctly implements the logic to replace text nodes matching a given pattern. It converts the pattern to a regular expression, adds it to the list of patterns, and applies these patterns to the shadow DOM.
- The
seekAndReplacemethod walks through all text nodes in the shadow DOM and replaces the text matching the pattern.- The
applyPatternsmethod applies all stored patterns to the shadow DOM.- The test case sets up the DOM, adds a pattern, and verifies the text content of a div to ensure the replacement.
Therefore, the test accurately reflects the expected behavior of the
ShadowDOMManipulatorclass.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that text replacement updates the DOM. # Test: Search for calls to `textContent` that may not reflect actual DOM changes. rg --type javascript 'textContent' | grep 'manipulator'Length of output: 88
Script:
#!/bin/bash # Locate the implementation of the addPattern method in the ShadowDOMManipulator class. ast-grep --lang typescript --pattern $'class ShadowDOMManipulator { $$$ addPattern($_, $_) { $$$ } $$$ }'Length of output: 12815
apps/mocksi-lite/content/ContentApp.tsx (3)
57-57: Transition tochrome.storage.localfor state management is implemented correctly.
41-41: Ensure initial state handling aligns with application requirements.Also applies to: 44-52
Verification successful
The
RecordingStateis well-defined and includesUNAUTHORIZED. The usage ofRecordingState.UNAUTHORIZEDis consistent across the codebase, especially inContentApp.tsx, ensuring that the initial state handling aligns with the application requirements.
RecordingState.UNAUTHORIZEDis used inContentApp.tsxto set the initial state.- Other instances also reference
RecordingState.UNAUTHORIZED, ensuring consistent state management.Locations to note:
apps/mocksi-lite/content/ContentApp.tsx: Lines 41, 44, 52Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the initial state is correctly set and handled across different components and interactions. # Test: Search for usage of MOCKSI_RECORDING_STATE in all files to ensure consistent initial state handling. rg --type python $'MOCKSI_RECORDING_STATE'Length of output: 72
Script:
#!/bin/bash # Search for the definition of RecordingState to verify its values. rg 'RecordingState' # Search for the usage of RecordingState.UNAUTHORIZED to verify how it is used across the codebase. rg 'RecordingState.UNAUTHORIZED'Length of output: 6442
95-104: Review the use ofsetEditorModewithin asynchronous event handlers to ensure it aligns with user actions.Also applies to: 126-135
Verification successful
The review comment asks to verify the use of
setEditorModewithin asynchronous event handlers inContentApp.tsx. The context from the shell scripts shows thatsetEditorModeis used inContentApp.tsxandDemoItem.tsx, and its implementation is ineditMode.ts.Here are the key points:
setEditorModeis called withinonClickandonKeyUpevent handlers inContentApp.tsx.- In
DemoItem.tsx,setEditorModeis called in thehandleEditfunction.- The implementation of
setEditorModeineditMode.tssets some state in the local storage and adds an event listener ifturnOnis true.The usage of
setEditorModeinContentApp.tsxaligns with user actions (click and key press events), and it does not appear to be called redundantly. However, it is important to ensure that these calls are necessary and correctly aligned with user interactions.
ContentApp.tsx:
- Lines 95-104 and 126-135:
setEditorModeis called when the user clicks or presses a key (Escape or Enter) on specific elements.
DemoItem.tsx:
- The
handleEditfunction callssetEditorModewhen editing starts, which aligns with user action.Based on the gathered information, the usage of
setEditorModeappears to be correctly aligned with user actions and does not seem redundant.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `setEditorMode` is triggered correctly and only when necessary. # Test: Search for all usages of `setEditorMode` to ensure it is not called redundantly or missed in any user flow. rg --type python $'setEditorMode'Length of output: 63
Script:
#!/bin/bash # Description: Verify that `setEditorMode` is triggered correctly and only when necessary. # Test: Search for all usages of `setEditorMode` in TypeScript files to ensure it is not called redundantly or missed in any user flow. rg --type ts 'setEditorMode'Length of output: 769
Script:
#!/bin/bash # Extract the surrounding code for the `setEditorMode` calls in `ContentApp.tsx` rg -A 5 -B 5 'setEditorMode' apps/mocksi-lite/content/ContentApp.tsx # Extract the surrounding code for the `setEditorMode` calls in `DemoItem.tsx` rg -A 5 -B 5 'setEditorMode' apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx # Extract the implementation of `setEditorMode` in `editMode.ts` rg -A 10 -B 2 'export const setEditorMode' apps/mocksi-lite/content/EditMode/editMode.tsLength of output: 2698
packages/dodom/receivers/ShadowDOMManipulator.ts (4)
8-9: Initialization ofpatternsandobserverwithin the constructor enhances modularity.Also applies to: 14-14
45-49: Pattern addition and immediate application logic are correctly implemented.
51-63: Ensure thatremovePatternproperly manages pattern removal and observer disconnection.
66-71: Conversion of string patterns toRegExpis handled efficiently.
| if (!recordingState) { | ||
| chrome.storage.local.set({ | ||
| [MOCKSI_RECORDING_STATE]: RecordingState.READY, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure consistent use of asynchronous operations with chrome.storage.local.
- apps/mocksi-lite/utils.ts: Line with
chrome.storage.local.setis missingawait. - apps/mocksi-lite/content/content.tsx: Line 54 is missing
await. - apps/mocksi-lite/content/RecordButton.tsx: Lines with
chrome.storage.local.setare missingawait. - apps/mocksi-lite/content/ContentApp.tsx: Line with
chrome.storage.local.setis missingawait. - apps/mocksi-lite/background.ts: Line with
chrome.storage.local.setis missingawait.
Analysis chain
Ensure consistent use of asynchronous operations with chrome.storage.local.
This script checks for any synchronous calls to chrome.storage.local.set which might lead to inconsistent state management if not handled properly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all storage operations are handled asynchronously.
# Test: Search for synchronous storage operations. Expect: None.
rg --type typescript $'chrome.storage.local.set' | grep -v 'await'
Length of output: 98
Script:
#!/bin/bash
# Description: Verify that all storage operations are handled asynchronously.
# Test: Search for synchronous storage operations. Expect: None.
rg --type ts $'chrome.storage.local.set' | grep -v 'await'
Length of output: 695
There was a problem hiding this comment.
I agree, but how can we do this on places like hooks that we cannot decorate with async?
There was a problem hiding this comment.
@elg0nz, you're right in noting that certain hooks cannot be directly decorated with async due to their nature. However, you can handle asynchronous operations within these hooks by defining an async function inside the hook and then calling it. Here's an example using useEffect:
import { useEffect } from 'react';
function MyComponent() {
useEffect(() => {
async function fetchData() {
const response = await fetch('https://api.example.com/data');
const data = await response.json();
// process data
}
fetchData().catch(console.error); // Handle errors from fetchData
}, []); // Dependency array
return <div>...</div>;
}In this pattern, fetchData is an asynchronous function defined within useEffect. We then call fetchData() and use .catch to handle any potential errors. This approach keeps the effect itself synchronous while effectively managing asynchronous operations.
Let me know if this helps or if there's anything else you'd like to explore!
| const loadRecordingId = async () => { | ||
| return new Promise<string | undefined>((resolve) => { | ||
| chrome.storage.local.get([MOCKSI_RECORDING_ID], (result) => { | ||
| resolve(result[MOCKSI_RECORDING_ID]); | ||
| }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Consider simplifying the promise handling in loadRecordingId.
- return new Promise<string | undefined>((resolve) => {
- chrome.storage.local.get([MOCKSI_RECORDING_ID], (result) => {
- resolve(result[MOCKSI_RECORDING_ID]);
- });
- });
+ const result = await chrome.storage.local.get([MOCKSI_RECORDING_ID]);
+ return result[MOCKSI_RECORDING_ID];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.
| const loadRecordingId = async () => { | |
| return new Promise<string | undefined>((resolve) => { | |
| chrome.storage.local.get([MOCKSI_RECORDING_ID], (result) => { | |
| resolve(result[MOCKSI_RECORDING_ID]); | |
| }); | |
| }); | |
| }; | |
| const loadRecordingId = async () => { | |
| const result = await chrome.storage.local.get([MOCKSI_RECORDING_ID]); | |
| return result[MOCKSI_RECORDING_ID]; | |
| }; |
| const walker = document.createTreeWalker( | ||
| this.shadowRoot, | ||
| NodeFilter.SHOW_TEXT, | ||
| { | ||
| acceptNode: (node) => { | ||
| if (!node || !node.nodeValue) { | ||
| return NodeFilter.FILTER_REJECT; | ||
| } | ||
| return pattern.test(node.nodeValue) ? NodeFilter.FILTER_ACCEPT : NodeFilter.FILTER_REJECT; | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| let node: Text; | ||
| const globalPattern = new RegExp(pattern.source, pattern.flags.includes('g') ? pattern.flags : `${pattern.flags}g`); | ||
| while (node = walker.nextNode() as Text) { | ||
| if (!node || !node.nodeValue) { | ||
| return; | ||
| } | ||
| node.nodeValue = node.nodeValue.replaceAll(globalPattern, replace); | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize text node replacement to avoid potential performance issues with large DOMs.
- while (node = walker.nextNode() as Text) {
+ for (let node = walker.nextNode() as Text; node != null; node = walker.nextNode() as Text) {Assignment within the loop condition can lead to unintended side effects and is generally discouraged.
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.
| private seekAndReplace(pattern: RegExp, replace: string) { | |
| const walker = document.createTreeWalker( | |
| this.shadowRoot, | |
| NodeFilter.SHOW_TEXT, | |
| { | |
| acceptNode: (node) => { | |
| if (!node || !node.nodeValue) { | |
| return NodeFilter.FILTER_REJECT; | |
| } | |
| return pattern.test(node.nodeValue) ? NodeFilter.FILTER_ACCEPT : NodeFilter.FILTER_REJECT; | |
| } | |
| } | |
| ); | |
| let node: Text; | |
| const globalPattern = new RegExp(pattern.source, pattern.flags.includes('g') ? pattern.flags : `${pattern.flags}g`); | |
| while (node = walker.nextNode() as Text) { | |
| if (!node || !node.nodeValue) { | |
| return; | |
| } | |
| node.nodeValue = node.nodeValue.replaceAll(globalPattern, replace); | |
| } | |
| } | |
| private seekAndReplace(pattern: RegExp, replace: string) { | |
| const walker = document.createTreeWalker( | |
| this.shadowRoot, | |
| NodeFilter.SHOW_TEXT, | |
| { | |
| acceptNode: (node) => { | |
| if (!node || !node.nodeValue) { | |
| return NodeFilter.FILTER_REJECT; | |
| } | |
| return pattern.test(node.nodeValue) ? NodeFilter.FILTER_ACCEPT : NodeFilter.FILTER_REJECT; | |
| } | |
| } | |
| ); | |
| let node: Text; | |
| const globalPattern = new RegExp(pattern.source, pattern.flags.includes('g') ? pattern.flags : `${pattern.flags}g`); | |
| for (let node = walker.nextNode() as Text; node != null; node = walker.nextNode() as Text) { | |
| if (!node || !node.nodeValue) { | |
| return; | |
| } | |
| node.nodeValue = node.nodeValue.replaceAll(globalPattern, replace); | |
| } | |
| } |
Tools
Biome
[error] 89-89: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
| root.render(<ContentApp isOpen={true} email={email || ""} />); | ||
| }) | ||
| .catch((error) => { | ||
| localStorage.clear(); | ||
| chrome.storage.local.clear(); |
There was a problem hiding this comment.
Consider improving the user experience during error handling.
- window.open(SignupURL);
+ showErrorModal("Failed to load user data. Please try again.");Instead of redirecting the user immediately, providing an error message through a modal might enhance user experience by giving them more context about the error.
Committable suggestion was skipped due to low confidence.
| state !== RecordingState.ANALYZING ? handleToggleRecording : undefined | ||
| } | ||
| onKeyUp={(event) => { | ||
| event.key === "Escape" && | ||
| (state !== RecordingState.ANALYZING | ||
| ? () => handleToggleRecording() | ||
| : () => undefined); | ||
| if (event.key === "Escape" && state !== RecordingState.ANALYZING) { | ||
| handleToggleRecording(); | ||
| } |
There was a problem hiding this comment.
Enhance accessibility by adding keyboard support for all states.
- if (event.key === "Escape" && state !== RecordingState.ANALYZING) {
+ if (event.key === "Escape") {
handleToggleRecording();
}Allowing the 'Escape' key to function regardless of the recording state could improve accessibility and user experience.
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.
| state !== RecordingState.ANALYZING ? handleToggleRecording : undefined | |
| } | |
| onKeyUp={(event) => { | |
| event.key === "Escape" && | |
| (state !== RecordingState.ANALYZING | |
| ? () => handleToggleRecording() | |
| : () => undefined); | |
| if (event.key === "Escape" && state !== RecordingState.ANALYZING) { | |
| handleToggleRecording(); | |
| } | |
| state !== RecordingState.ANALYZING ? handleToggleRecording : undefined | |
| } | |
| onKeyUp={(event) => { | |
| if (event.key === "Escape") { | |
| handleToggleRecording(); | |
| } |
| const waitTime = 2000; // 2 seconds | ||
|
|
||
| export const RecordButton = ({ state, onRecordChange }: RecordButtonProps) => { | ||
| // biome-ignore lint/correctness/useExhaustiveDependencies: hook will only run once | ||
| useEffect(() => { | ||
| const storageState = | ||
| (localStorage.getItem(MOCKSI_RECORDING_STATE) as RecordingState) || | ||
| RecordingState.READY; | ||
| chrome.storage.local.get([MOCKSI_RECORDING_STATE], (result) => { | ||
| const storageState = | ||
| (result[MOCKSI_RECORDING_STATE] as RecordingState) || | ||
| RecordingState.READY; | ||
| onRecordChange(storageState); | ||
|
|
||
| onRecordChange(storageState); | ||
| const waitTime = 2000; // 2 seconds | ||
| if (storageState === RecordingState.ANALYZING) { | ||
| setTimeout(() => { | ||
| onRecordChange(RecordingState.CREATE); | ||
| localStorage.setItem( | ||
| MOCKSI_RECORDING_STATE, | ||
| RecordingState.CREATE.toString(), | ||
| ); | ||
| }, waitTime); | ||
| } | ||
| }, []); | ||
| if (storageState === RecordingState.ANALYZING) { | ||
| setTimeout(() => { | ||
| onRecordChange(RecordingState.CREATE); | ||
| chrome.storage.local.set({ | ||
| [MOCKSI_RECORDING_STATE]: RecordingState.CREATE.toString(), | ||
| }); | ||
| }, waitTime); | ||
| } | ||
| }); | ||
| }, [onRecordChange]); |
There was a problem hiding this comment.
Ensure proper cleanup of timeouts to prevent memory leaks.
+ let timeoutId;
if (storageState === RecordingState.ANALYZING) {
- setTimeout(() => {
+ timeoutId = setTimeout(() => {
onRecordChange(RecordingState.CREATE);
chrome.storage.local.set({
[MOCKSI_RECORDING_STATE]: RecordingState.CREATE.toString(),
});
}, waitTime);
}
+ return () => clearTimeout(timeoutId);Adding cleanup for the setTimeout will prevent potential memory leaks, especially in components that might mount and unmount frequently.
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.
| const waitTime = 2000; // 2 seconds | |
| export const RecordButton = ({ state, onRecordChange }: RecordButtonProps) => { | |
| // biome-ignore lint/correctness/useExhaustiveDependencies: hook will only run once | |
| useEffect(() => { | |
| const storageState = | |
| (localStorage.getItem(MOCKSI_RECORDING_STATE) as RecordingState) || | |
| RecordingState.READY; | |
| chrome.storage.local.get([MOCKSI_RECORDING_STATE], (result) => { | |
| const storageState = | |
| (result[MOCKSI_RECORDING_STATE] as RecordingState) || | |
| RecordingState.READY; | |
| onRecordChange(storageState); | |
| onRecordChange(storageState); | |
| const waitTime = 2000; // 2 seconds | |
| if (storageState === RecordingState.ANALYZING) { | |
| setTimeout(() => { | |
| onRecordChange(RecordingState.CREATE); | |
| localStorage.setItem( | |
| MOCKSI_RECORDING_STATE, | |
| RecordingState.CREATE.toString(), | |
| ); | |
| }, waitTime); | |
| } | |
| }, []); | |
| if (storageState === RecordingState.ANALYZING) { | |
| setTimeout(() => { | |
| onRecordChange(RecordingState.CREATE); | |
| chrome.storage.local.set({ | |
| [MOCKSI_RECORDING_STATE]: RecordingState.CREATE.toString(), | |
| }); | |
| }, waitTime); | |
| } | |
| }); | |
| }, [onRecordChange]); | |
| const waitTime = 2000; // 2 seconds | |
| export const RecordButton = ({ state, onRecordChange }: RecordButtonProps) => { | |
| useEffect(() => { | |
| let timeoutId; | |
| chrome.storage.local.get([MOCKSI_RECORDING_STATE], (result) => { | |
| const storageState = | |
| (result[MOCKSI_RECORDING_STATE] as RecordingState) || | |
| RecordingState.READY; | |
| onRecordChange(storageState); | |
| if (storageState === RecordingState.ANALYZING) { | |
| timeoutId = setTimeout(() => { | |
| onRecordChange(RecordingState.CREATE); | |
| chrome.storage.local.set({ | |
| [MOCKSI_RECORDING_STATE]: RecordingState.CREATE.toString(), | |
| }); | |
| }, waitTime); | |
| } | |
| }); | |
| return () => clearTimeout(timeoutId); | |
| }, [onRecordChange]); |
|
|
||
| it('should replace image source and undo the replacement', () => { |
There was a problem hiding this comment.
Ensure consistent use of image sources in tests to avoid confusion.
- manipulator.replaceImage('https://example.com/old.jpg', 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUA...=');
+ manipulator.replaceImage('https://example.com/old.jpg', 'data:image/png;base64,ACTUAL_BASE64_DATA...');Replacing the placeholder base64 string with actual data or a clearly fake placeholder could improve clarity and prevent misunderstandings during test maintenance.
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.
| it('should replace image source and undo the replacement', () => { | |
| it('should replace image source and undo the replacement', () => { | |
| manipulator.replaceImage('https://example.com/old.jpg', 'data:image/png;base64,ACTUAL_BASE64_DATA...'); |
| it('should remove pattern', () => { | ||
| shadowRoot.innerHTML = '<div>Test text</div>'; | ||
| manipulator.addPattern('Test', 'Replaced'); | ||
| manipulator.removePattern('Test'); | ||
| shadowRoot.innerHTML += '<div>Test text</div>'; | ||
| manipulator.applyPatterns(); | ||
| expect(manipulator.serializeShadowDOM()).toBe('<div xmlns="http://www.w3.org/1999/xhtml">Replaced text</div><div xmlns="http://www.w3.org/1999/xhtml">Test text</div>'); | ||
|
|
||
| }); |
There was a problem hiding this comment.
Ensure removal of patterns is tested for all cases.
+ // Test removing patterns when multiple patterns are applied
+ manipulator.addPattern('another', 'different');
+ manipulator.removePattern('Test');
+ manipulator.applyPatterns();
+ expect(manipulator.serializeShadowDOM()).toBe('<div xmlns="http://www.w3.org/1999/xhtml">Test text and different text</div>');Expanding this test to cover scenarios where multiple patterns are applied ensures comprehensive testing of the pattern removal functionality.
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.
| it('should remove pattern', () => { | |
| shadowRoot.innerHTML = '<div>Test text</div>'; | |
| manipulator.addPattern('Test', 'Replaced'); | |
| manipulator.removePattern('Test'); | |
| shadowRoot.innerHTML += '<div>Test text</div>'; | |
| manipulator.applyPatterns(); | |
| expect(manipulator.serializeShadowDOM()).toBe('<div xmlns="http://www.w3.org/1999/xhtml">Replaced text</div><div xmlns="http://www.w3.org/1999/xhtml">Test text</div>'); | |
| }); | |
| it('should remove pattern', () => { | |
| shadowRoot.innerHTML = '<div>Test text</div>'; | |
| manipulator.addPattern('Test', 'Replaced'); | |
| manipulator.removePattern('Test'); | |
| shadowRoot.innerHTML += '<div>Test text</div>'; | |
| manipulator.applyPatterns(); | |
| expect(manipulator.serializeShadowDOM()).toBe('<div xmlns="http://www.w3.org/1999/xhtml">Replaced text</div><div xmlns="http://www.w3.org/1999/xhtml">Test text</div>'); | |
| // Test removing patterns when multiple patterns are applied | |
| manipulator.addPattern('another', 'different'); | |
| manipulator.removePattern('Test'); | |
| manipulator.applyPatterns(); | |
| expect(manipulator.serializeShadowDOM()).toBe('<div xmlns="http://www.w3.org/1999/xhtml">Test text and different text</div>'); | |
| }); |
| it('should apply patterns on DOM changes', () => { | ||
| shadowRoot.innerHTML = '<div>Test text</div>'; | ||
| expect(manipulator.serializeShadowDOM()).toBe('<div xmlns="http://www.w3.org/1999/xhtml">Test text</div>'); | ||
|
|
||
| manipulator.addPattern('Test', 'Replaced'); | ||
| expect(manipulator.serializeShadowDOM()).toEqual('<div xmlns="http://www.w3.org/1999/xhtml">Replaced text</div>'); | ||
|
|
||
| shadowRoot.innerHTML += '<div>Test new text</div>'; | ||
| expect(manipulator.serializeShadowDOM()).toEqual('<div xmlns="http://www.w3.org/1999/xhtml">Replaced text</div><div xmlns="http://www.w3.org/1999/xhtml">Test new text</div>'); | ||
|
|
||
| // FIXME: applyPatterns should be called automatically when DOM changes | ||
| manipulator.applyPatterns(); | ||
| const divs = shadowRoot.querySelectorAll('div'); | ||
| expect(manipulator.serializeShadowDOM()).toBe('<div xmlns="http://www.w3.org/1999/xhtml">Replaced text</div><div xmlns="http://www.w3.org/1999/xhtml">Replaced new text</div>'); | ||
| expect(divs[0]?.textContent).toBe('Replaced text'); | ||
| expect(divs[1]?.textContent).toBe('Replaced new text'); | ||
| }); |
There was a problem hiding this comment.
Address the FIXME regarding automatic pattern application on DOM changes.
- // FIXME: applyPatterns should be called automatically when DOM changes
+ observer.observe(shadowRoot, { childList: true, subtree: true });Implementing an observer to automatically apply patterns when the DOM changes addresses the FIXME and enhances the functionality of the ShadowDOMManipulator.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (1 hunks)
- apps/mocksi-lite/utils.ts (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/mocksi-lite/utils.ts
Additional comments not posted (1)
apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (1)
27-28: Extracting the domain from the URL and using it in the UI is implemented correctly.
| <a href={url} target={"_blank"} rel={"noreferrer"}> | ||
| <TextField className={"text-xs underline"}>{domain}</TextField> | ||
| </a> |
There was a problem hiding this comment.
The logic to disable buttons based on the current hostname is secure and prevents cross-origin actions. Consider abstracting this condition into a well-named function for clarity and reusability.
+ function isSameOrigin(url) {
+ return url.includes(window.location.hostname);
+ }
- disabled={!url.includes(window.location.hostname)}
+ disabled={!isSameOrigin(url)}Also applies to: 49-50, 55-56
Summary by CodeRabbit
New Features
localStoragetochrome.storage.localfor saving and retrieving application states, ensuring better data synchronization and reliability.ShadowDOMManipulatorto handle text replacements within the shadow DOM more effectively.Improvements
Bug Fixes
Tests
ShadowDOMManipulatorto ensure proper handling of patterns, errors, and DOM changes.Chores
package.jsonandtsconfig.jsonfor better project structure and output management.