[NC]: Update demo api #48
Conversation
# Conflicts: # apps/mocksi-lite/background.ts # apps/mocksi-lite/content/ContentApp.tsx # apps/mocksi-lite/content/content.tsx
# Conflicts: # apps/mocksi-lite/content/ContentApp.tsx # apps/mocksi-lite/networking.ts
|
Warning Review failedThe pull request is closed. WalkthroughWalkthroughThe changes revolve around enhancing the Changes
Sequence DiagramssequenceDiagram
participant UI as User Interface
participant Utils as utils.ts
participant Command as SaveModificationCommand
participant LocalStorage as localStorage
UI->>Utils: Call saveModification
Utils->>Utils: Build DOMModification object
Utils->>Command: Instantiate SaveModificationCommand
Command->>LocalStorage: Save modification to localStorage
Command->>Utils: Return success
Utils->>UI: Modification saved
sequenceDiagram
participant UI as User Interface
participant EditMode as editMode.ts
participant LocalStorage as localStorage
UI->>EditMode: Call setEditorMode(turnOn, recordingId)
alt turnOn is true
EditMode->>LocalStorage: Save recordingId
else turnOn is false
EditMode->>LocalStorage: Remove recordingId
end
EditMode->>UI: Editor mode set
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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- apps/mocksi-lite/background.ts (5 hunks)
- apps/mocksi-lite/consts.ts (1 hunks)
- apps/mocksi-lite/content/ContentApp.tsx (3 hunks)
- apps/mocksi-lite/content/EditMode/editMode.ts (2 hunks)
- apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (2 hunks)
- apps/mocksi-lite/content/Popup/CreateDemo/index.tsx (1 hunks)
Files skipped from review due to trivial changes (3)
- apps/mocksi-lite/background.ts
- apps/mocksi-lite/consts.ts
- apps/mocksi-lite/content/Popup/CreateDemo/index.tsx
Additional context used
Biome
apps/mocksi-lite/content/ContentApp.tsx
[error] 91-91: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
Additional comments not posted (5)
apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (2)
14-16: The update to includeuuidinsetEditorModealigns well with the new requirement to handle unique identifiers for editing sessions.
27-27: The removal of the arrow function in the onClick handler is a good practice to avoid unnecessary re-rendering of the component.apps/mocksi-lite/content/ContentApp.tsx (2)
3-3: Updates to imports and utility functions are correctly aligned with the new functionalities introduced.Also applies to: 6-6
46-49: ThehandleUpdatefunction correctly retrieves the recording ID from localStorage and sends a message for updating the demo. Ensure that error handling is robust in the message handling function.Verification successful
#!/bin/bash # Description: Extract and verify error handling in the sendMessage function implementation. # Extract the sendMessage function implementation from utils.ts ast-grep --lang ts --pattern $'export const sendMessage = ($_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_ --- ```shell #!/bin/bash # Description: Extract and verify error handling in the sendMessage function implementation. # Extract the sendMessage function implementation from utils.ts ast-grep --lang ts --pattern $'export const sendMessage = ($_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_,$_ --- The `sendMessage` function in `apps/mocksi-lite/utils.ts` does include basic error handling by logging an error message if the response status is not "success". This provides a minimal level of robustness in handling potential errors. - `apps/mocksi-lite/utils.ts`: The `sendMessage` function logs an error message if the response status is not "success". </details> <details> <summary>Scripts executed</summary> The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Description: Verify robust error handling in message handling function. # Test: Search for the function usage. Expect: Error handling mechanisms. rg --type typescript $'sendMessage'Length of output: 69
Script:
#!/bin/bash # Description: Verify robust error handling in message handling function. # Test: Search for the function usage. Expect: Error handling mechanisms. rg --type ts 'sendMessage'Length of output: 1303
Script:
#!/bin/bash # Description: Extract and verify error handling in the sendMessage function implementation. # Extract the sendMessage function implementation from utils.ts using a simplified pattern ast-grep --lang ts --pattern 'export const sendMessage = $_' --json | jq '.[] | .location.start.line' | xargs -I {} sh -c 'sed -n "$(({}-1)),$(({}+10))p" apps/mocksi-lite/utils.ts'Length of output: 2007
Script:
#!/bin/bash # Description: Extract and verify error handling in the sendMessage function implementation. # Locate the sendMessage function and extract surrounding lines for verification rg 'export const sendMessage' -A 10 apps/mocksi-lite/utils.tsLength of output: 343
apps/mocksi-lite/content/EditMode/editMode.ts (1)
44-53: The update tosetEditorModeto include arecordingIdparameter enhances the function's capability to handle session-specific data. Ensure that therecordingIdis consistently managed across all relevant components.Verification successful
The search results indicate that
recordingIdis consistently managed in thesetEditorModefunction. It is stored in localStorage when the mode is turned on and removed when the mode is turned off. This aligns with the intended functionality.
apps/mocksi-lite/consts.ts: DefinesMOCKSI_RECORDING_ID.apps/mocksi-lite/content/EditMode/editMode.ts: ManagesrecordingIdinsetEditorMode.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent management of `recordingId` across components. # Test: Search for the usage of `recordingId`. Expect: Consistent usage and removal. rg --type typescript $'recordingId'Length of output: 69
Script:
#!/bin/bash # Description: Verify consistent management of `recordingId` across components. # Test: Search for the usage of `recordingId`. Expect: Consistent usage and removal. rg --glob '*.ts' --glob '*.tsx' 'recordingId'Length of output: 426
| @@ -83,6 +88,9 @@ export default function ContentApp({ isOpen, email }: ContentProps) { | |||
| </div> | |||
| </div> | |||
| </div> | |||
| <div className="cursor-pointer text-[#009875]" onClick={handleUpdate}> | |||
There was a problem hiding this comment.
Add keyboard accessibility to the "Done" button to support keyboard-only users.
- <div className="cursor-pointer text-[#009875]" onClick={handleUpdate}>
+ <div className="cursor-pointer text-[#009875]" onClick={handleUpdate} onKeyUp={(event) => event.key === 'Enter' && handleUpdate()}>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.
| <div className="cursor-pointer text-[#009875]" onClick={handleUpdate}> | |
| <div className="cursor-pointer text-[#009875]" onClick={handleUpdate} onKeyUp={(event) => event.key === 'Enter' && handleUpdate()}> |
Tools
Biome
[error] 91-91: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- apps/mocksi-lite/background.ts (4 hunks)
- apps/mocksi-lite/commands/Command.ts (1 hunks)
- apps/mocksi-lite/content/ContentApp.tsx (3 hunks)
- apps/mocksi-lite/content/EditMode/editMode.ts (2 hunks)
- apps/mocksi-lite/utils.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- apps/mocksi-lite/background.ts
- apps/mocksi-lite/content/EditMode/editMode.ts
Additional context used
Biome
apps/mocksi-lite/content/ContentApp.tsx
[error] 91-91: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
Additional comments not posted (6)
apps/mocksi-lite/commands/Command.ts (4)
1-1: Ensure consistent usage of imported constants.
3-6: Well-definedCommandinterface with essential methods for command pattern.
8-12:DOMModificationinterface is clearly defined, ensuring structured data handling.
16-30: RefactorbuildQuerySelectorto improve performance and maintainability.
[REFACTOR_SUGGESTion]- const elements = document.querySelectorAll(keyToSave); - if (elements.length > 1) { - keyToSave += `[${[...elements].indexOf(parentElement)}]` - } + const index = Array.from(parentElement.parentNode.children).indexOf(parentElement); + if (index !== -1) { + keyToSave += `[${index}]`; + }This change avoids querying the entire document and directly computes the index within its parent, which is more efficient.
apps/mocksi-lite/utils.ts (1)
7-7:saveModificationfunction andcommandsExecutedarray are well-implemented to support the command pattern.Also applies to: 28-28, 34-44
apps/mocksi-lite/content/ContentApp.tsx (1)
46-49:handleUpdatefunction correctly implements the update logic. Ensure consistent naming conventions for localStorage keys.
apps/mocksi-lite/commands/Command.ts
Outdated
| class LoadModificationCommand implements Command { | ||
| execute(): void { | ||
| throw new Error("Method not implemented.") | ||
| } | ||
| undo(): void { | ||
| throw new Error("Method not implemented.") | ||
| } |
There was a problem hiding this comment.
LoadModificationCommand methods are placeholders. Ensure implementation or documentation of future plans.
Please implement or document the intended functionality for LoadModificationCommand methods or mark them as deprecated if they are not planned for use.
apps/mocksi-lite/commands/Command.ts
Outdated
| export class SaveModificationCommand implements Command { | ||
| private prevModifications; | ||
| constructor ( | ||
| private localStorage: Storage, | ||
| private modification: DOMModification | ||
| ) { | ||
| this.prevModifications = JSON.parse( | ||
| localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}", | ||
| ); | ||
| } | ||
|
|
||
| execute(): void { | ||
| const {keyToSave, nextText} = this.modification | ||
| const {previousText} = this.prevModifications[keyToSave] || {} | ||
| this.localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({...this.prevModifications, [keyToSave]: {nextText, previousText}}) | ||
| ) | ||
| } | ||
|
|
||
| undo(): void { | ||
| const {keyToSave, previousText} = this.modification | ||
| this.localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({...this.prevModifications, [keyToSave]: {previousText} || undefined}) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The SaveModificationCommand class does not handle potential JSON parsing errors in its constructor.
- The constructor uses
JSON.parsewithout a surroundingtry-catchblock to handle potential errors.
Consider adding error handling around the JSON.parse call to ensure robustness.
- Location:
apps/mocksi-lite/commands/Command.ts,SaveModificationCommandclass constructor
Analysis chain
SaveModificationCommand class implements the Command pattern effectively. However, consider handling potential JSON parsing errors.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify JSON handling in SaveModificationCommand constructor.
# Test: Check for try-catch implementation around JSON.parse.
ast-grep --lang typescript --pattern $'constructor($_, $_) {
$$$
JSON.parse($_)
$$$
}'
Length of output: 568
Script:
#!/bin/bash
# Description: Verify JSON handling in SaveModificationCommand constructor.
# Test: Check for try-catch implementation around JSON.parse.
ast-grep --lang typescript --pattern $'constructor($_) {
try {
$$$
JSON.parse($_)
$$$
} catch (e) {
$$$
}
}'
Length of output: 644
Script:
#!/bin/bash
# Description: Verify JSON handling in SaveModificationCommand constructor.
# Step 1: Locate the constructor in SaveModificationCommand.
# Step 2: Check for JSON.parse and any surrounding try-catch block.
# Locate the constructor in SaveModificationCommand
ast-grep --lang typescript --pattern $'constructor($_) {
$$$
}' --json | jq '.[] | select(.text | test("SaveModificationCommand")).text' > constructor_text.txt
# Check for JSON.parse and surrounding try-catch block within the constructor
if grep -q 'JSON.parse' constructor_text.txt; then
echo "JSON.parse found in constructor"
if grep -q 'try {' constructor_text.txt && grep -q 'catch (e)' constructor_text.txt; then
echo "try-catch block found around JSON.parse"
else
echo "No try-catch block found around JSON.parse"
fi
else
echo "JSON.parse not found in constructor"
fi
Length of output: 710
Script:
#!/bin/bash
# Description: Verify JSON handling in SaveModificationCommand constructor.
# Step 1: Locate the SaveModificationCommand class and its constructor.
# Step 2: Check for JSON.parse and any surrounding try-catch block.
# Locate the SaveModificationCommand class and its constructor
rg -A 10 'class SaveModificationCommand' > SaveModificationCommand_class.txt
# Check for JSON.parse and surrounding try-catch block within the constructor
if grep -q 'JSON.parse' SaveModificationCommand_class.txt; then
echo "JSON.parse found in constructor"
if grep -A 5 'JSON.parse' SaveModificationCommand_class.txt | grep -q 'try {' && grep -A 5 'JSON.parse' SaveModificationCommand_class.txt | grep -q 'catch (e)'; then
echo "try-catch block found around JSON.parse"
else
echo "No try-catch block found around JSON.parse"
fi
else
echo "JSON.parse not found in constructor"
fi
Length of output: 340
apps/mocksi-lite/utils.ts
Outdated
| export const persistModifications = ( | ||
| recordingId: string | ||
| ) => { | ||
| const alterations: Alteration[] = Object.entries<{nextText: string, previousText: string}>( | ||
| JSON.parse( | ||
| localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}", | ||
| ) | ||
| ).map( | ||
| ([querySelector, {nextText, previousText}]) => ({ | ||
| selector: querySelector, | ||
| action: previousText ? 'modified' : 'added', | ||
| dom_before: previousText || '', | ||
| dom_after: nextText | ||
| }) | ||
| ) | ||
| const updated_timestamp = new Date() | ||
| sendMessage('updateDemo', {id: recordingId, recording: { updated_timestamp, alterations }}) | ||
|
|
||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The persistModifications function lacks error handling and validation, particularly around the JSON.parse operation. This can lead to potential issues if the data in localStorage is not in the expected format.
- Implement a try-catch block around the
JSON.parsecall to handle any parsing errors. - Add validation to ensure the parsed data conforms to the expected structure before proceeding.
Code Locations:
apps/mocksi-lite/utils.ts(functionpersistModifications)
Analysis chain
persistModifications function effectively handles data transformation and messaging. Ensure robust error handling and validation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in persistModifications.
# Test: Check for try-catch blocks or error checks.
ast-grep --lang typescript --pattern $'function persistModifications($_) {
$$$
JSON.parse($_)
$$$
}'
Length of output: 107
Script:
#!/bin/bash
# Refined search for try-catch blocks around JSON.parse within persistModifications function
ast-grep --lang typescript --pattern $'function persistModifications($_) {
$$$
try {
JSON.parse($_)
} catch ($$) {
$$$
}
$$$
}'
# Search for any form of validation within persistModifications function
ast-grep --lang typescript --pattern $'function persistModifications($_) {
$$$
if ($_ !== null && $_ !== undefined) {
$$$
}
$$$
}'
Length of output: 290
Script:
#!/bin/bash
# Search for the persistModifications function definition in the codebase
ast-grep --lang typescript --pattern $'function persistModifications($_) {
$$$
}'
# Display the content of the file containing persistModifications function for manual inspection
fd -e ts -x grep -Hn 'function persistModifications' {} \; | cut -d: -f1 | xargs cat
Length of output: 174
Script:
#!/bin/bash
# Search for the persistModifications function definition in the codebase using ripgrep
rg 'function persistModifications' -t typescript
# Display the content of the file containing persistModifications function for manual inspection
rg 'function persistModifications' -t typescript -l | xargs cat
Length of output: 186
Script:
#!/bin/bash
# Correct the file type and search for the persistModifications function definition in the codebase using ripgrep
rg 'function persistModifications' -t ts
# Display the content of the file containing persistModifications function for manual inspection
rg 'function persistModifications' -t ts -l | xargs cat
Length of output: 100
Script:
#!/bin/bash
# Search for any definition or export of persistModifications in the codebase
rg 'persistModifications' -t ts
# Display the content of the file containing persistModifications function for manual inspection
rg 'persistModifications' -t ts -l | xargs cat
Length of output: 7960
| <div className="cursor-pointer text-[#009875]" onClick={() => setEditorMode(false, localStorage.getItem(MOCKSI_RECORDING_ID) || undefined)}> | ||
| Done | ||
| </div> |
There was a problem hiding this comment.
Add keyboard accessibility to the "Done" button to support keyboard-only users.
- <div className="cursor-pointer text-[#009875]" onClick={() => setEditorMode(false, localStorage.getItem(MOCKSI_RECORDING_ID) || undefined)}>
+ <div className="cursor-pointer text-[#009875]" onClick={() => setEditorMode(false, localStorage.getItem(MOCKSI_RECORDING_ID) || undefined)} onKeyUp={(event) => event.key === 'Enter' && setEditorMode(false, localStorage.getItem(MOCKSI_RECORDING_ID) || undefined)}>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.
| <div className="cursor-pointer text-[#009875]" onClick={() => setEditorMode(false, localStorage.getItem(MOCKSI_RECORDING_ID) || undefined)}> | |
| Done | |
| </div> | |
| <div className="cursor-pointer text-[#009875]" onClick={() => setEditorMode(false, localStorage.getItem(MOCKSI_RECORDING_ID) || undefined)} onKeyUp={(event) => event.key === 'Enter' && setEditorMode(false, localStorage.getItem(MOCKSI_RECORDING_ID) || undefined)}> | |
| Done | |
| </div> |
Tools
Biome
[error] 91-91: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
apps/mocksi-lite/content/EditMode/actions.ts (2)
18-18: Add parameter documentation foroldValue.Consider adding a comment to describe the purpose of the
oldValueparameter in theapplyChangesfunction for better code clarity.
35-35: Ensure thatpreviousTextis correctly documented and used.The variable
previousTextis used to hold the previous state before changes. It would be beneficial to add a comment explaining its role, especially since it's part of thesaveModificationcall.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- apps/mocksi-lite/background.ts (5 hunks)
- apps/mocksi-lite/commands/Command.ts (1 hunks)
- apps/mocksi-lite/content/ContentApp.tsx (3 hunks)
- apps/mocksi-lite/content/EditMode/actions.ts (1 hunks)
- apps/mocksi-lite/content/EditMode/decorator.ts (1 hunks)
- apps/mocksi-lite/content/EditMode/editMode.ts (2 hunks)
- apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (2 hunks)
- apps/mocksi-lite/utils.ts (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- apps/mocksi-lite/background.ts
- apps/mocksi-lite/commands/Command.ts
- apps/mocksi-lite/content/EditMode/editMode.ts
- apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx
Additional context used
Biome
apps/mocksi-lite/content/ContentApp.tsx
[error] 95-103: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
Additional comments not posted (1)
apps/mocksi-lite/content/EditMode/decorator.ts (1)
49-49: Ensure correct handling ofnewValueandvalue.There's potential ambiguity in how
newValueandvalueare used. It's crucial to ensure that these variables are being used correctly throughout the function.
| // const previousNode = nodeWithTextArea?.cloneNode(true) | ||
| // const asd = previousNode?.parentElement?.replaceChild( | ||
| // document.createTextNode(oldValue), | ||
| // previousNode | ||
| // ) |
There was a problem hiding this comment.
Remove commented-out code.
Commented-out code can clutter the codebase and should be removed if it's not intended for future use.
- // const previousNode = nodeWithTextArea?.cloneNode(true)
- // const asd = previousNode?.parentElement?.replaceChild(
- // document.createTextNode(oldValue),
- // previousNode
- // )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 previousNode = nodeWithTextArea?.cloneNode(true) | |
| // const asd = previousNode?.parentElement?.replaceChild( | |
| // document.createTextNode(oldValue), | |
| // previousNode | |
| // ) |
There was a problem hiding this comment.
This was a first try to apply undo when de user cancels the edited DOM
There was a problem hiding this comment.
@NicoMorenoSirius, thank you for clarifying the purpose of the commented-out code. It's great to see proactive attempts at implementing features like undo functionality.
Since this code might still be in consideration for future refinement, here's a suggestion: You might consider keeping such code in a separate branch or marked with a TODO comment that explains its future potential. This way, the main codebase remains clean, while you still retain the ability to revisit and refine this feature.
If you need further assistance with this or any other aspect of your implementation, feel free to ask. Happy coding!
There was a problem hiding this comment.
I don't fully disagree with what the🐰 is saying. But for now, it’s fine so we can focus on shipping the MVP.
That being said, we’ll be stricter in future PRs
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
apps/mocksi-lite/utils.ts
Outdated
| const modifications: DOMModifcationsType = JSON.parse( | ||
| localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}", | ||
| ); | ||
| for (const modification of Object.entries(modifications)) { | ||
| // value here is encoded, SHOULD NOT be a security risk to put it in the innerHTML | ||
| const [querySelector, value] = modification; | ||
| const [querySelector, { nextText }] = modification; | ||
| const hasIndex = querySelector.match(/\[[0-9]+\]/); | ||
| if (hasIndex) { | ||
| const index: number = +hasIndex[0].replace("[", "").replace("]", ""); | ||
| const elemToModify = document.querySelectorAll( | ||
| querySelector.replace(hasIndex[0], ""), | ||
| )[index]; | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = value; | ||
| elemToModify.innerHTML = nextText; | ||
| } else { | ||
| const [elemToModify] = document.querySelectorAll(querySelector); | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = value; | ||
| elemToModify.innerHTML = nextText; |
There was a problem hiding this comment.
Refactor loadModifications to reduce complexity and improve error handling.
The current implementation of loadModifications is complex and lacks robust error handling. Consider refactoring it to simplify the logic and add error handling mechanisms.
+ try {
+ const modifications: DOMModifcationsType = JSON.parse(
+ localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}",
+ );
+ modifications.forEach((modification, querySelector) => {
+ const element = document.querySelector(querySelector);
+ if (element) {
+ element.innerHTML = modification.nextText;
+ }
+ });
+ } catch (error) {
+ console.error("Failed to load modifications:", error);
+ }Committable suggestion was skipped due to low confidence.
| <div | ||
| className="cursor-pointer text-[#009875]" | ||
| onClick={() => | ||
| setEditorMode( | ||
| false, | ||
| localStorage.getItem(MOCKSI_RECORDING_ID) || undefined, | ||
| ) | ||
| } | ||
| > |
There was a problem hiding this comment.
Add keyboard accessibility to the "Done" button.
To support keyboard-only users, it's important to add keyboard event handlers to elements that have mouse event handlers.
+ <div className="cursor-pointer text-[#009875]" onClick={() => setEditorMode(false, localStorage.getItem(MOCKSI_RECORDING_ID) || undefined)} onKeyUp={(event) => event.key === 'Enter' && setEditorMode(false, localStorage.getItem(MOCKSI_RECORDING_ID) || undefined)}>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.
| <div | |
| className="cursor-pointer text-[#009875]" | |
| onClick={() => | |
| setEditorMode( | |
| false, | |
| localStorage.getItem(MOCKSI_RECORDING_ID) || undefined, | |
| ) | |
| } | |
| > | |
| <div | |
| className="cursor-pointer text-[#009875]" | |
| onClick={() => | |
| setEditorMode( | |
| false, | |
| localStorage.getItem(MOCKSI_RECORDING_ID) || undefined, | |
| ) | |
| } | |
| onKeyUp={(event) => event.key === 'Enter' && setEditorMode(false, localStorage.getItem(MOCKSI_RECORDING_ID) || undefined)} | |
| > |
Tools
Biome
[error] 95-103: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apps/mocksi-lite/background.ts (5 hunks)
- apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (2 hunks)
- apps/mocksi-lite/utils.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx
Files skipped from review as they are similar to previous changes (1)
- apps/mocksi-lite/background.ts
Additional comments not posted (1)
apps/mocksi-lite/utils.ts (1)
39-48: The changes to thesaveModificationfunction look good, integrating the command pattern effectively.
apps/mocksi-lite/utils.ts
Outdated
| previousText: string, | ||
| ) => { | ||
| const prevModifications = JSON.parse( | ||
| localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}", | ||
| ); | ||
| let keyToSave = parentElement.localName; | ||
| if (parentElement.id) { | ||
| keyToSave += `#${parentElement.id}`; | ||
| } | ||
| if (parentElement.className) { | ||
| keyToSave += `.${parentElement.className}`; | ||
| } | ||
| // here we check if the key we built is sufficient to get an unique element when querying | ||
| const elements = document.querySelectorAll(keyToSave); | ||
| if (elements.length === 1) { | ||
| localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | ||
| ); | ||
| } else { | ||
| // if not unique, we search for the index and put it on the key. | ||
| keyToSave += `[${[...elements].indexOf(parentElement)}]`; | ||
| localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | ||
| ); | ||
| } | ||
| // to successfully implement the do/undo commands, we must save somewhere in memory all commands being executed. | ||
| const saveModificationCommand = new SaveModificationCommand(localStorage, { | ||
| keyToSave: buildQuerySelector(parentElement), | ||
| nextText: newText, | ||
| previousText, // todo test previous | ||
| }); | ||
| commandsExecuted.push(saveModificationCommand); | ||
| saveModificationCommand.execute(); |
There was a problem hiding this comment.
Ensure the previous text functionality is tested as indicated by the TODO comment.
Would you like me to help by writing the unit tests for this functionality?
There was a problem hiding this comment.
@elg0nz, here are the unit tests for the saveModification functionality in the utils.ts file, focusing on the handling of previousText:
import { saveModification, Command, SaveModificationCommand } from './utils';
import { buildQuerySelector } from './commands/Command';
// Mocks
jest.mock('./commands/Command', () => ({
SaveModificationCommand: jest.fn().mockImplementation(() => ({
execute: jest.fn(),
undo: jest.fn()
})),
buildQuerySelector: jest.fn(() => 'mock-selector')
}));
describe('saveModification', () => {
let mockLocalStorage: Storage;
beforeEach(() => {
// Setup mock for localStorage
mockLocalStorage = {
setItem: jest.fn(),
getItem: jest.fn(),
removeItem: jest.fn(),
clear: jest.fn(),
} as unknown as Storage;
});
it('should create and execute a SaveModificationCommand with previous text', () => {
const parentElement = document.createElement('div');
const newText = 'New text';
const previousText = 'Old text';
saveModification(parentElement, newText, previousText);
expect(buildQuerySelector).toHaveBeenCalledWith(parentElement);
expect(SaveModificationCommand).toHaveBeenCalledWith(mockLocalStorage, {
keyToSave: 'mock-selector',
nextText: newText,
previousText,
});
const command = SaveModificationCommand.mock.instances[0];
expect(command.execute).toHaveBeenCalled();
});
});This test ensures that the previousText is correctly passed to the SaveModificationCommand and that the command is properly constructed and executed. The buildQuerySelector utility is also checked to ensure it is called with the correct arguments. Let me know if there are any more functionalities or scenarios you would like to cover in these tests!
apps/mocksi-lite/utils.ts
Outdated
| type DOMModifcationsType = { | ||
| [querySelector: string]: { nextText: string; previousText: string }; | ||
| }; |
There was a problem hiding this comment.
Correct the typographical error in the type name DOMModifcationsType.
- type DOMModifcationsType = {
+ type DOMModificationsType = {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.
| type DOMModifcationsType = { | |
| [querySelector: string]: { nextText: string; previousText: string }; | |
| }; | |
| type DOMModificationsType = { | |
| [querySelector: string]: { nextText: string; previousText: string }; | |
| }; |
There was a problem hiding this comment.
@elg0nz, thank you for pointing that out! I've checked the rest of the code in the vicinity of the typo correction you agreed with. If there are any more corrections or specific areas you'd like me to review, please let me know!
If the correction looks good to you, you can apply the suggested fix to ensure the type name is consistent throughout the codebase. If there's anything else you need, feel free to ask!
apps/mocksi-lite/utils.ts
Outdated
| // v2 of loading alterations, this is from backend | ||
| export const loadAlterations = (alterations: Alteration[]) => { | ||
| for(const alteration of alterations) { | ||
| const {selector, dom_after} = alteration | ||
| const hasIndex = selector.match(/\[[0-9]+\]/); | ||
| if (hasIndex) { | ||
| const index: number = +hasIndex[0].replace("[", "").replace("]", ""); | ||
| const elemToModify = document.querySelectorAll( | ||
| querySelector.replace(hasIndex[0], ""), | ||
| selector.replace(hasIndex[0], ""), | ||
| )[index]; | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = value; | ||
| elemToModify.innerHTML = dom_after; | ||
| } else { | ||
| const [elemToModify] = document.querySelectorAll(querySelector); | ||
| const [elemToModify] = document.querySelectorAll(selector); | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = value; | ||
| elemToModify.innerHTML = dom_after; | ||
| } | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Consider sanitizing dom_after to prevent potential XSS attacks when assigning to innerHTML.
+ const safeDomAfter = sanitizeHtml(dom_after); // Ensure you have a sanitizeHtml function available
elemToModify.innerHTML = safeDomAfter;Committable suggestion was skipped due to low confidence.
apps/mocksi-lite/utils.ts
Outdated
| export const persistModifications = (recordingId: string) => { | ||
| const alterations: Alteration[] = Object.entries<{ | ||
| nextText: string; | ||
| previousText: string; | ||
| }>(JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}")).map( | ||
| ([querySelector, { nextText, previousText }]) => ({ | ||
| selector: querySelector, | ||
| action: previousText ? "modified" : "added", | ||
| dom_before: previousText || "", | ||
| dom_after: nextText, | ||
| }), | ||
| ); | ||
| for (const modification of Object.entries(modifications)) { | ||
| // value here is encoded, SHOULD NOT be a security risk to put it in the innerHTML | ||
| const [querySelector, value] = modification; | ||
| const hasIndex = querySelector.match(/\[[0-9]+\]/); | ||
| const updated_timestamp = new Date(); | ||
| sendMessage("updateDemo", { | ||
| id: recordingId, | ||
| recording: { updated_timestamp, alterations }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add error handling around the JSON.parse operation to prevent potential runtime errors.
+ try {
+ const modifications = JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}");
+ // existing code
+ } catch (error) {
+ console.error("Error parsing modifications:", error);
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- apps/mocksi-lite/content/ContentApp.tsx (3 hunks)
- apps/mocksi-lite/content/EditMode/editMode.ts (2 hunks)
- apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (2 hunks)
- apps/mocksi-lite/utils.ts (2 hunks)
Additional comments not posted (4)
apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (2)
22-22: EnsuresetEditorModeis properly handling theuuidas the recording ID.Verification successful
The
setEditorModefunction inapps/mocksi-lite/content/EditMode/editMode.tscorrectly handles therecordingIdparameter, storing it inlocalStoragewhen turning on the editor mode and using it to persist modifications when turning off the editor mode. Theuuidparameter passed tosetEditorModeinapps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsxis thus properly utilized as the recording ID.
- In
setEditorMode, therecordingIdis stored inlocalStoragewhenturnOnistrue.- When
turnOnisfalse,recordingIdis used to callpersistModifications.Locations:
setEditorModedefinition:apps/mocksi-lite/content/EditMode/editMode.ts- Usage with
uuid:apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsxScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `setEditorMode` correctly utilizes the `uuid` parameter. # Test: Search for `setEditorMode` usage with `uuid` parameter. ast-grep --lang typescript --pattern $'setEditorMode($_, $_) { $$$ }'Length of output: 526
Script:
#!/bin/bash # Description: Verify that `setEditorMode` correctly utilizes the `uuid` parameter. # Test: Search for `setEditorMode` function usage and its implementation. ast-grep --lang typescript --pattern 'setEditorMode($_, $_)' --json # Additional step to locate the implementation of `setEditorMode` ast-grep --lang typescript --pattern 'function setEditorMode($_, $_) { $$$ }' --jsonLength of output: 162
Script:
#!/bin/bash # Description: Locate all function definitions and usages to identify `setEditorMode` or similar functions. # Search for all function definitions ast-grep --lang typescript --pattern 'function $_($_$) { $$$ }' --json # Search for all function calls ast-grep --lang typescript --pattern '$_($_$)' --jsonLength of output: 134
Script:
#!/bin/bash # Description: Search for the term `setEditorMode` in the codebase to gather more context. rg 'setEditorMode'Length of output: 687
Script:
#!/bin/bash # Description: Display the contents of `apps/mocksi-lite/content/EditMode/editMode.ts` to verify the utilization of `recordingId`. cat apps/mocksi-lite/content/EditMode/editMode.tsLength of output: 4601
38-38: Confirm whetherloadAlterationsproperly handles thealterationsdata structure.Verification successful
The
loadAlterationsfunction properly handles thealterationsdata structure by iterating over the array and modifying the DOM based on theselectoranddom_afterproperties of each alteration object. The function covers cases where the selector includes an index and where it does not.
loadAlterationsdefinition:export const loadAlterations = (alterations: Alteration[]) => { for (const alteration of alterations) { const { selector, dom_after } = alteration; const hasIndex = selector.match(/\[[0-9]+\]/); if (hasIndex) { const index: number = +hasIndex[0].replace("[", "").replace("]", ""); const elemToModify = document.querySelectorAll( selector.replace(hasIndex[0], ""), )[index]; //@ts-ignore elemToModify.innerHTML = dom_after; } else { const [elemToModify] = document.querySelectorAll(selector); //@ts-ignore elemToModify.innerHTML = dom_after; } } };This confirms that
loadAlterationsis correctly implemented to handle thealterationsdata structure.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `loadAlterations` is correctly implemented to handle alterations data structure. # Test: Search for `loadAlterations` function definition and usage. ast-grep --lang typescript --pattern $'function loadAlterations($_) { $$$ }'Length of output: 79
Script:
#!/bin/bash # Search for `loadAlterations` as a method within classes. ast-grep --lang typescript --pattern $'class $_ { $$$ loadAlterations($_) { $$$ } $$$ }' # Search for `loadAlterations` as an imported function. rg --glob '**/*.ts*' 'import { loadAlterations' --files-with-matchesLength of output: 288
Script:
#!/bin/bash # Correct the regex error and search for `loadAlterations` as an imported function. rg --glob '**/*.ts*' 'import { loadAlterations' --files-with-matches # Perform a broader search for `loadAlterations` definitions. rg --glob '**/*.ts*' 'loadAlterations'Length of output: 525
Script:
#!/bin/bash # Extract the definition of `loadAlterations` from `apps/mocksi-lite/utils.ts`. rg --glob 'apps/mocksi-lite/utils.ts' 'export const loadAlterations'Length of output: 158
Script:
#!/bin/bash # Extract the entire implementation of `loadAlterations` from `apps/mocksi-lite/utils.ts`. awk '/export const loadAlterations =/,/}/' apps/mocksi-lite/utils.tsLength of output: 221
Script:
#!/bin/bash # Extract the entire implementation of `loadAlterations` from `apps/mocksi-lite/utils.ts`. awk '/export const loadAlterations =/,/^}/' apps/mocksi-lite/utils.tsLength of output: 659
apps/mocksi-lite/utils.ts (2)
14-16: Correct the typographical error in the type nameDOMModificationsType.- type DOMModifcationsType = { + type DOMModificationsType = {Likely invalid or redundant comment.
34-48: Ensure thatcommandsExecutedis correctly utilized and thatSaveModificationCommandproperly implements the command pattern.
apps/mocksi-lite/utils.ts
Outdated
| elemToModify.innerHTML = dom_after; | ||
| } else { | ||
| const [elemToModify] = document.querySelectorAll(querySelector); | ||
| const [elemToModify] = document.querySelectorAll(selector); | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = value; | ||
| elemToModify.innerHTML = dom_after; |
There was a problem hiding this comment.
Consider sanitizing dom_after to prevent potential XSS attacks when assigning to innerHTML.
+ const safeDomAfter = sanitizeHtml(dom_after); // Ensure you have a sanitizeHtml function available
elemToModify.innerHTML = safeDomAfter;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.
| elemToModify.innerHTML = dom_after; | |
| } else { | |
| const [elemToModify] = document.querySelectorAll(querySelector); | |
| const [elemToModify] = document.querySelectorAll(selector); | |
| //@ts-ignore | |
| elemToModify.innerHTML = value; | |
| elemToModify.innerHTML = dom_after; | |
| elemToModify.innerHTML = dom_after; | |
| } else { | |
| const [elemToModify] = document.querySelectorAll(selector); | |
| //@ts-ignore | |
| const safeDomAfter = sanitizeHtml(dom_after); // Ensure you have a sanitizeHtml function available | |
| elemToModify.innerHTML = safeDomAfter; |
There was a problem hiding this comment.
If you want to add this, just use https://github.com/cure53/DOMPurify or similar library.
There was a problem hiding this comment.
@elg0nz, thanks for recommending the use of DOMPurify to sanitize the dom_after content before assigning it to innerHTML. Integrating DOMPurify can significantly enhance the security by ensuring that the content is free from potentially malicious scripts.
Here’s a quick example of how you might integrate DOMPurify in the existing code:
import DOMPurify from 'dompurify';
// v2 of loading alterations, this is from backend
export const loadAlterations = (alterations: Alteration[]) => {
for (const alteration of alterations) {
const { selector, dom_after } = alteration;
const hasIndex = selector.match(/\[[0-9]+\]/);
if (hasIndex) {
const index = +hasIndex[0].replace("[", "").replace("]", "");
const elemToModify = document.querySelectorAll(selector.replace(hasIndex[0], ""))[index];
elemToModify.innerHTML = DOMPurify.sanitize(dom_after); // Sanitized content
} else {
const [elemToModify] = document.querySelectorAll(selector);
elemToModify.innerHTML = DOMPurify.sanitize(dom_after); // Sanitized content
}
}
};If you need any further assistance with setting this up or have any other concerns, feel free to let me know!
Learnings added
User: elg0nz
PR: Mocksi/HARlighter#48
File: apps/mocksi-lite/utils.ts:0-0
Timestamp: 2024-06-13T22:08:01.155Z
Learning: Use DOMPurify to sanitize HTML content before assigning it to `innerHTML` to prevent XSS attacks.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
apps/mocksi-lite/utils.ts
Outdated
| export const persistModifications = (recordingId: string) => { | ||
| const alterations: Alteration[] = Object.entries<{ | ||
| nextText: string; | ||
| previousText: string; | ||
| }>(JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}")).map( | ||
| ([querySelector, { nextText, previousText }]) => ({ | ||
| selector: querySelector, | ||
| action: previousText ? "modified" : "added", | ||
| dom_before: previousText || "", | ||
| dom_after: nextText, | ||
| }), | ||
| ); | ||
| for (const modification of Object.entries(modifications)) { | ||
| // value here is encoded, SHOULD NOT be a security risk to put it in the innerHTML | ||
| const [querySelector, value] = modification; | ||
| const hasIndex = querySelector.match(/\[[0-9]+\]/); | ||
| const updated_timestamp = new Date(); | ||
| sendMessage("updateDemo", { | ||
| id: recordingId, | ||
| recording: { updated_timestamp, alterations }, | ||
| }); | ||
| localStorage.removeItem(MOCKSI_MODIFICATIONS); | ||
| }; |
There was a problem hiding this comment.
Add error handling around the JSON.parse operation to prevent potential runtime errors.
+ try {
+ const modifications = JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}");
+ // existing code
+ } catch (error) {
+ console.error("Error parsing modifications:", error);
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Yes, let's start adding more error handling. Our Rollbar is already full of exceptions 🙃
There was a problem hiding this comment.
@elg0nz, great to hear that we're on the same page regarding the importance of robust error handling! Here's a detailed example of how we can implement error handling around the JSON.parse operation in the persistModifications function to prevent runtime errors and potentially reduce the number of exceptions:
export const persistModifications = (recordingId: string) => {
try {
const modificationsString = localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}";
const modifications = JSON.parse(modificationsString);
const alterations = Object.entries<{
nextText: string;
previousText: string;
}>(modifications).map(([querySelector, { nextText, previousText }]) => ({
selector: querySelector,
action: previousText ? "modified" : "added",
dom_before: previousText || "",
dom_after: nextText,
}));
const updated_timestamp = new Date();
sendMessage("updateDemo", {
id: recordingId,
recording: { updated_timestamp, alterations },
});
localStorage.removeItem(MOCKSI_MODIFICATIONS);
} catch (error) {
console.error("Failed to parse modifications from localStorage:", error);
}
};This snippet wraps the JSON parsing and the subsequent logic in a try block, catching and logging any errors that occur during the parsing process. This should help in mitigating the impact of corrupt or malformed data in localStorage. Let me know if this aligns with your expectations or if there are other areas where you think we could improve error handling!
| const handleUpdate = () => { | ||
| const id = localStorage.getItem(MOCKSI_RECORDING_ID); | ||
| sendMessage("updateDemo", { id }); | ||
| }; |
There was a problem hiding this comment.
Add keyboard accessibility to the "Done" button to support keyboard-only users.
- <div className="cursor-pointer text-[#009875]" onClick={handleUpdate}>
+ <div className="cursor-pointer text-[#009875]" onClick={handleUpdate} onKeyUp={(event) => event.key === 'Enter' && handleUpdate()}>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 handleUpdate = () => { | |
| const id = localStorage.getItem(MOCKSI_RECORDING_ID); | |
| sendMessage("updateDemo", { id }); | |
| }; | |
| const handleUpdate = () => { | |
| const id = localStorage.getItem(MOCKSI_RECORDING_ID); | |
| sendMessage("updateDemo", { id }); | |
| }; | |
| <div className="cursor-pointer text-[#009875]" onClick={handleUpdate} onKeyUp={(event) => event.key === 'Enter' && handleUpdate()}> |
| export const setEditorMode = (turnOn: boolean, recordingId?: string) => { | ||
| if (turnOn) { | ||
| if (recordingId) localStorage.setItem(MOCKSI_RECORDING_ID, recordingId); | ||
| localStorage.setItem(MOCKSI_RECORDING_STATE, RecordingState.EDITING); | ||
| blockNodes(); | ||
| document.body.addEventListener("dblclick", onDoubleClickText); | ||
| } else { | ||
| if (recordingId) { | ||
| persistModifications(recordingId); | ||
| } else { | ||
| localStorage.removeItem(MOCKSI_MODIFICATIONS); | ||
| // undoModifications() | ||
| } | ||
| localStorage.setItem(MOCKSI_RECORDING_STATE, RecordingState.CREATE); | ||
| localStorage.removeItem(MOCKSI_RECORDING_ID); |
There was a problem hiding this comment.
Refactor setEditorMode to reduce complexity and improve error handling.
+ try {
+ const modifications: DOMModificationsType = JSON.parse(
+ localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}",
+ );
+ modifications.forEach((modification, querySelector) => {
+ const element = document.querySelector(querySelector);
+ if (element) {
+ element.innerHTML = modification.nextText;
+ }
+ });
+ } catch (error) {
+ console.error("Failed to load modifications:", error);
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
This is a good suggestion though
There was a problem hiding this comment.
@elg0nz, thank you for the positive feedback! If you find the suggestion fits well with your project goals, feel free to implement it. Should you need any more assistance or have further questions, just let me know!
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (1)
14-20: Ensure theuuidprop is documented in the component's prop types.Adding documentation for the new
uuidprop will help other developers understand its purpose and usage within this component.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- apps/mocksi-lite/content/ContentApp.tsx (3 hunks)
- apps/mocksi-lite/content/EditMode/editMode.ts (2 hunks)
- apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (2 hunks)
- apps/mocksi-lite/utils.ts (2 hunks)
Additional comments not posted (4)
apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (2)
22-22: Correctly passuuidtosetEditorModeto manage editor states based on unique identifiers.This is a good use of the
uuidto ensure that the editor mode is set correctly for specific instances.
38-39: Verify thatloadAlterationscorrectly handles thealterationsdata structure.Ensure that the
alterationsare structured as expected by theloadAlterationsfunction.Verification successful
The
loadAlterationsfunction has been located inapps/mocksi-lite/utils.tsand appears to handle thealterationsdata structure correctly by iterating over thealterationsarray and manipulating DOM elements based on the provided selectors and content.
loadAlterationsis defined as an arrow function.- It iterates over the
alterationsarray.- For each
alteration, it extractsselectoranddom_after.- It checks for indexed selectors and appropriately modifies the inner HTML of the targeted elements.
Based on this analysis, the function appears to handle the
alterationsdata structure as expected.
- File:
apps/mocksi-lite/utils.ts- Lines: 81-98
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of alterations data passed to `loadAlterations`. # Test: Search for the `loadAlterations` function usage. Expect: Correct handling of the data structure. ast-grep --lang typescript --pattern $'function loadAlterations($_) { $$$ $_.map($_ => $_.alterations) $$$ }'Length of output: 116
Script:
#!/bin/bash # Search for the definition of the `loadAlterations` function in the TypeScript files. ast-grep --lang typescript --pattern 'function loadAlterations($_) { $$$ }'Length of output: 79
Script:
#!/bin/bash # Search for `loadAlterations` defined as an arrow function or a method within an object. ast-grep --lang typescript --pattern 'const loadAlterations = ($_) => { $$$ }' ast-grep --lang typescript --pattern 'loadAlterations($_) { $$$ }'Length of output: 1751
apps/mocksi-lite/content/ContentApp.tsx (1)
95-115: Add keyboard accessibility to the "Done" button to support keyboard-only users.- <div className="cursor-pointer text-[#009875]" onClick={handleUpdate}> + <div className="cursor-pointer text-[#009875]" onClick={handleUpdate} onKeyUp={(event) => event.key === 'Enter' && handleUpdate()}>It's important to ensure that interactive elements are accessible via keyboard to support a wider range of users.
Likely invalid or redundant comment.
apps/mocksi-lite/content/EditMode/editMode.ts (1)
50-64: Ensure consistency in handling therecordingIdparameter insetEditorMode.The update to include
recordingIdenhances the functionality by allowing tracking of edits associated with specific recordings.
apps/mocksi-lite/utils.ts
Outdated
| previousText: string, | ||
| ) => { | ||
| const prevModifications = JSON.parse( | ||
| localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}", | ||
| ); | ||
| let keyToSave = parentElement.localName; | ||
| if (parentElement.id) { | ||
| keyToSave += `#${parentElement.id}`; | ||
| } | ||
| if (parentElement.className) { | ||
| keyToSave += `.${parentElement.className}`; | ||
| } | ||
| // here we check if the key we built is sufficient to get an unique element when querying | ||
| const elements = document.querySelectorAll(keyToSave); | ||
| if (elements.length === 1) { | ||
| localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | ||
| ); | ||
| } else { | ||
| // if not unique, we search for the index and put it on the key. | ||
| keyToSave += `[${[...elements].indexOf(parentElement)}]`; | ||
| localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | ||
| ); | ||
| } | ||
| // to successfully implement the do/undo commands, we must save somewhere in memory all commands being executed. | ||
| const saveModificationCommand = new SaveModificationCommand(localStorage, { | ||
| keyToSave: buildQuerySelector(parentElement), | ||
| nextText: newText, | ||
| previousText, // todo test previous | ||
| }); |
There was a problem hiding this comment.
Address the TODO comment about testing the previousText functionality.
Would you like me to help by writing the unit tests for this functionality?
There was a problem hiding this comment.
@CodeRabbit write unit tests
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
apps/mocksi-lite/utils.ts
Outdated
| export const persistModifications = (recordingId: string) => { | ||
| const alterations: Alteration[] = Object.entries<{ | ||
| nextText: string; | ||
| previousText: string; | ||
| }>(JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}")).map( | ||
| ([querySelector, { nextText, previousText }]) => ({ | ||
| selector: querySelector, | ||
| action: previousText ? "modified" : "added", | ||
| dom_before: previousText || "", | ||
| dom_after: nextText, | ||
| }), | ||
| ); | ||
| for (const modification of Object.entries(modifications)) { | ||
| // value here is encoded, SHOULD NOT be a security risk to put it in the innerHTML | ||
| const [querySelector, value] = modification; | ||
| const hasIndex = querySelector.match(/\[[0-9]+\]/); | ||
| const updated_timestamp = new Date(); | ||
| sendMessage("updateDemo", { | ||
| id: recordingId, | ||
| recording: { updated_timestamp, alterations }, | ||
| }); | ||
| localStorage.removeItem(MOCKSI_MODIFICATIONS); | ||
| }; |
There was a problem hiding this comment.
Add error handling around the JSON.parse operation in persistModifications to prevent potential runtime errors.
+ try {
+ const modifications = JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}");
+ // existing code
+ } catch (error) {
+ console.error("Error parsing modifications:", error);
+ }Ensure robust error handling to manage cases where the localStorage data might not be in the expected format.
Committable suggestion was skipped due to low confidence.
apps/mocksi-lite/utils.ts
Outdated
| export const loadAlterations = (alterations: Alteration[]) => { | ||
| for (const alteration of alterations) { | ||
| const { selector, dom_after } = alteration; | ||
| const hasIndex = selector.match(/\[[0-9]+\]/); | ||
| if (hasIndex) { | ||
| const index: number = +hasIndex[0].replace("[", "").replace("]", ""); | ||
| const elemToModify = document.querySelectorAll( | ||
| querySelector.replace(hasIndex[0], ""), | ||
| selector.replace(hasIndex[0], ""), | ||
| )[index]; | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = value; | ||
| elemToModify.innerHTML = dom_after; | ||
| } else { | ||
| const [elemToModify] = document.querySelectorAll(querySelector); | ||
| const [elemToModify] = document.querySelectorAll(selector); | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = value; | ||
| elemToModify.innerHTML = dom_after; |
There was a problem hiding this comment.
Consider sanitizing dom_after to prevent potential XSS attacks when assigning to innerHTML.
+ const safeDomAfter = sanitizeHtml(dom_after); // Ensure you have a sanitizeHtml function available
elemToModify.innerHTML = safeDomAfter;It's crucial to sanitize any data that is dynamically inserted into the DOM to prevent XSS vulnerabilities.
Committable suggestion was skipped due to low confidence.
| const handleUpdate = () => { | ||
| const id = localStorage.getItem(MOCKSI_RECORDING_ID); | ||
| sendMessage("updateDemo", { id }); | ||
| }; |
There was a problem hiding this comment.
Ensure error handling is implemented in handleUpdate when retrieving the ID from localStorage.
Consider adding a check to ensure that the ID retrieved from localStorage is not null before proceeding to send the message.
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
apps/mocksi-lite/utils.ts (1)
Line range hint
117-126: Enhance error handling insendMessageto cover more specific error scenarios and improve reliability.+ chrome.runtime.sendMessage({ message, body }, (response) => { + if (!response) { + console.error("No response from background script"); + return; + } + if (response?.status !== "success") { + console.error("Failed to send message to background script"); + return; + } + });
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- apps/mocksi-lite/commands/Command.ts (1 hunks)
- apps/mocksi-lite/content/EditMode/actions.ts (1 hunks)
- apps/mocksi-lite/content/EditMode/editMode.ts (2 hunks)
- apps/mocksi-lite/utils.ts (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- apps/mocksi-lite/commands/Command.ts
- apps/mocksi-lite/content/EditMode/actions.ts
- apps/mocksi-lite/content/EditMode/editMode.ts
Additional comments not posted (1)
apps/mocksi-lite/utils.ts (1)
34-48: The implementation ofsaveModificationusing the command pattern is well-executed.
apps/mocksi-lite/utils.ts
Outdated
| import { | ||
| MOCKSI_MODIFICATIONS, | ||
| MOCKSI_RECORDING_STATE, | ||
| RecordingState, | ||
| SignupURL, | ||
| } from "./consts"; | ||
|
|
||
| type DOMModifcationsType = { |
There was a problem hiding this comment.
Correct the typographical error in the type name.
- type DOMModifcationsType = {
+ type DOMModificationsType = {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.
| type DOMModifcationsType = { | |
| type DOMModificationsType = { |
There was a problem hiding this comment.
@elg0nz Great! You can proceed with the change. Make sure to update all references to DOMModifcationsType to DOMModificationsType in your codebase to maintain consistency. If you need any further assistance, feel free to ask!
| export const undoModifications = () => { | ||
| loadModifications(); | ||
| localStorage.removeItem(MOCKSI_MODIFICATIONS); | ||
| }; |
There was a problem hiding this comment.
Consider reordering operations in undoModifications to ensure modifications are loaded before removing the item from localStorage.
+ loadModifications();
+ localStorage.removeItem(MOCKSI_MODIFICATIONS);Committable suggestion was skipped due to low confidence.
apps/mocksi-lite/utils.ts
Outdated
| export const persistModifications = (recordingId: string) => { | ||
| const alterations: Alteration[] = Object.entries<{ | ||
| nextText: string; | ||
| previousText: string; | ||
| }>(JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}")).map( | ||
| ([querySelector, { nextText, previousText }]) => ({ | ||
| selector: querySelector, | ||
| action: previousText ? "modified" : "added", | ||
| dom_before: previousText || "", | ||
| dom_after: nextText, | ||
| }), | ||
| ); | ||
| let keyToSave = parentElement.localName; | ||
| if (parentElement.id) { | ||
| keyToSave += `#${parentElement.id}`; | ||
| } | ||
| if (parentElement.className) { | ||
| keyToSave += `.${parentElement.className}`; | ||
| } | ||
| // here we check if the key we built is sufficient to get an unique element when querying | ||
| const elements = document.querySelectorAll(keyToSave); | ||
| if (elements.length === 1) { | ||
| localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | ||
| ); | ||
| } else { | ||
| // if not unique, we search for the index and put it on the key. | ||
| keyToSave += `[${[...elements].indexOf(parentElement)}]`; | ||
| localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | ||
| ); | ||
| const updated_timestamp = new Date(); | ||
| sendMessage("updateDemo", { | ||
| id: recordingId, | ||
| recording: { updated_timestamp, alterations }, | ||
| }); | ||
| // localStorage.removeItem(MOCKSI_MODIFICATIONS); | ||
| }; |
There was a problem hiding this comment.
Add error handling around the JSON.parse operation to prevent potential runtime errors.
+ try {
+ const modifications = JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}");
+ // existing code
+ } catch (error) {
+ console.error("Error parsing modifications:", error);
+ }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.
| export const persistModifications = (recordingId: string) => { | |
| const alterations: Alteration[] = Object.entries<{ | |
| nextText: string; | |
| previousText: string; | |
| }>(JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}")).map( | |
| ([querySelector, { nextText, previousText }]) => ({ | |
| selector: querySelector, | |
| action: previousText ? "modified" : "added", | |
| dom_before: previousText || "", | |
| dom_after: nextText, | |
| }), | |
| ); | |
| let keyToSave = parentElement.localName; | |
| if (parentElement.id) { | |
| keyToSave += `#${parentElement.id}`; | |
| } | |
| if (parentElement.className) { | |
| keyToSave += `.${parentElement.className}`; | |
| } | |
| // here we check if the key we built is sufficient to get an unique element when querying | |
| const elements = document.querySelectorAll(keyToSave); | |
| if (elements.length === 1) { | |
| localStorage.setItem( | |
| MOCKSI_MODIFICATIONS, | |
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | |
| ); | |
| } else { | |
| // if not unique, we search for the index and put it on the key. | |
| keyToSave += `[${[...elements].indexOf(parentElement)}]`; | |
| localStorage.setItem( | |
| MOCKSI_MODIFICATIONS, | |
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | |
| ); | |
| const updated_timestamp = new Date(); | |
| sendMessage("updateDemo", { | |
| id: recordingId, | |
| recording: { updated_timestamp, alterations }, | |
| }); | |
| // localStorage.removeItem(MOCKSI_MODIFICATIONS); | |
| }; | |
| export const persistModifications = (recordingId: string) => { | |
| try { | |
| const alterations: Alteration[] = Object.entries<{ | |
| nextText: string; | |
| previousText: string; | |
| }>(JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}")).map( | |
| ([querySelector, { nextText, previousText }]) => ({ | |
| selector: querySelector, | |
| action: previousText ? "modified" : "added", | |
| dom_before: previousText || "", | |
| dom_after: nextText, | |
| }), | |
| ); | |
| const updated_timestamp = new Date(); | |
| sendMessage("updateDemo", { | |
| id: recordingId, | |
| recording: { updated_timestamp, alterations }, | |
| }); | |
| // localStorage.removeItem(MOCKSI_MODIFICATIONS); | |
| } catch (error) { | |
| console.error("Error parsing modifications:", error); | |
| } | |
| }; |
| export const loadAlterations = (alterations: Alteration[]) => { | ||
| for (const alteration of alterations) { | ||
| const { selector, dom_after } = alteration; | ||
| const hasIndex = selector.match(/\[[0-9]+\]/); | ||
| if (hasIndex) { | ||
| const index: number = +hasIndex[0].replace("[", "").replace("]", ""); | ||
| const elemToModify = document.querySelectorAll( | ||
| selector.replace(hasIndex[0], ""), | ||
| )[index]; | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = dom_after; | ||
| } else { | ||
| const [elemToModify] = document.querySelectorAll(selector); | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = dom_after; | ||
| } |
There was a problem hiding this comment.
Consider sanitizing dom_after to prevent potential XSS attacks when assigning to innerHTML.
+ const safeDomAfter = sanitizeHtml(dom_after); // Ensure you have a sanitizeHtml function available
elemToModify.innerHTML = safeDomAfter;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.
| export const loadAlterations = (alterations: Alteration[]) => { | |
| for (const alteration of alterations) { | |
| const { selector, dom_after } = alteration; | |
| const hasIndex = selector.match(/\[[0-9]+\]/); | |
| if (hasIndex) { | |
| const index: number = +hasIndex[0].replace("[", "").replace("]", ""); | |
| const elemToModify = document.querySelectorAll( | |
| selector.replace(hasIndex[0], ""), | |
| )[index]; | |
| //@ts-ignore | |
| elemToModify.innerHTML = dom_after; | |
| } else { | |
| const [elemToModify] = document.querySelectorAll(selector); | |
| //@ts-ignore | |
| elemToModify.innerHTML = dom_after; | |
| } | |
| export const loadAlterations = (alterations: Alteration[]) => { | |
| for (const alteration of alterations) { | |
| const { selector, dom_after } = alteration; | |
| const hasIndex = selector.match(/\[[0-9]+\]/); | |
| if (hasIndex) { | |
| const index: number = +hasIndex[0].replace("[", "").replace("]", ""); | |
| const elemToModify = document.querySelectorAll( | |
| selector.replace(hasIndex[0], ""), | |
| )[index]; | |
| //@ts-ignore | |
| const safeDomAfter = sanitizeHtml(dom_after); // Ensure you have a sanitizeHtml function available | |
| elemToModify.innerHTML = safeDomAfter; | |
| } else { | |
| const [elemToModify] = document.querySelectorAll(selector); | |
| //@ts-ignore | |
| const safeDomAfter = sanitizeHtml(dom_after); // Ensure you have a sanitizeHtml function available | |
| elemToModify.innerHTML = safeDomAfter; | |
| } | |
| } | |
| } |
apps/mocksi-lite/utils.ts
Outdated
| // This is from localStorage | ||
| export const loadModifications = () => { | ||
| const modifications = JSON.parse( | ||
| const modifications: DOMModifcationsType = JSON.parse( | ||
| localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}", | ||
| ); | ||
| for (const modification of Object.entries(modifications)) { | ||
| // value here is encoded, SHOULD NOT be a security risk to put it in the innerHTML | ||
| const [querySelector, value] = modification; | ||
| const [querySelector, { previousText }] = modification; | ||
| const hasIndex = querySelector.match(/\[[0-9]+\]/); | ||
| if (hasIndex) { | ||
| const index: number = +hasIndex[0].replace("[", "").replace("]", ""); | ||
| const elemToModify = document.querySelectorAll( | ||
| querySelector.replace(hasIndex[0], ""), | ||
| )[index]; | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = value; | ||
| elemToModify.innerHTML = previousText; | ||
| } else { | ||
| const [elemToModify] = document.querySelectorAll(querySelector); | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = value; | ||
| elemToModify.innerHTML = previousText; |
There was a problem hiding this comment.
Ensure that the content assigned to innerHTML is sanitized to prevent XSS vulnerabilities.
+ const safePreviousText = sanitizeHtml(previousText); // Ensure you have a sanitizeHtml function available
elemToModify.innerHTML = safePreviousText;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.
| // This is from localStorage | |
| export const loadModifications = () => { | |
| const modifications = JSON.parse( | |
| const modifications: DOMModifcationsType = JSON.parse( | |
| localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}", | |
| ); | |
| for (const modification of Object.entries(modifications)) { | |
| // value here is encoded, SHOULD NOT be a security risk to put it in the innerHTML | |
| const [querySelector, value] = modification; | |
| const [querySelector, { previousText }] = modification; | |
| const hasIndex = querySelector.match(/\[[0-9]+\]/); | |
| if (hasIndex) { | |
| const index: number = +hasIndex[0].replace("[", "").replace("]", ""); | |
| const elemToModify = document.querySelectorAll( | |
| querySelector.replace(hasIndex[0], ""), | |
| )[index]; | |
| //@ts-ignore | |
| elemToModify.innerHTML = value; | |
| elemToModify.innerHTML = previousText; | |
| } else { | |
| const [elemToModify] = document.querySelectorAll(querySelector); | |
| //@ts-ignore | |
| elemToModify.innerHTML = value; | |
| elemToModify.innerHTML = previousText; | |
| // This is from localStorage | |
| export const loadModifications = () => { | |
| const modifications: DOMModifcationsType = JSON.parse( | |
| localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}", | |
| ); | |
| for (const modification of Object.entries(modifications)) { | |
| // value here is encoded, SHOULD NOT be a security risk to put it in the innerHTML | |
| const [querySelector, { previousText }] = modification; | |
| const hasIndex = querySelector.match(/\[[0-9]+\]/); | |
| if (hasIndex) { | |
| const index: number = +hasIndex[0].replace("[", "").replace("]", ""); | |
| const elemToModify = document.querySelectorAll( | |
| querySelector.replace(hasIndex[0], ""), | |
| )[index]; | |
| //@ts-ignore | |
| const safePreviousText = sanitizeHtml(previousText); // Ensure you have a sanitizeHtml function available | |
| elemToModify.innerHTML = safePreviousText; | |
| } else { | |
| const [elemToModify] = document.querySelectorAll(querySelector); | |
| //@ts-ignore | |
| const safePreviousText = sanitizeHtml(previousText); // Ensure you have a sanitizeHtml function available | |
| elemToModify.innerHTML = safePreviousText; |
| @@ -0,0 +1,72 @@ | |||
| import { MOCKSI_MODIFICATIONS } from "../consts"; | |||
|
|
|||
| export interface Command { | |||
There was a problem hiding this comment.
Love this!
I'm literally working on writing a spec for some details about this interface, and this is a great start! Thanks
| export const setEditorMode = (turnOn: boolean, recordingId?: string) => { | ||
| if (turnOn) { | ||
| if (recordingId) localStorage.setItem(MOCKSI_RECORDING_ID, recordingId); | ||
| localStorage.setItem(MOCKSI_RECORDING_STATE, RecordingState.EDITING); | ||
| blockNodes(); | ||
| document.body.addEventListener("dblclick", onDoubleClickText); | ||
| } else { | ||
| if (recordingId) { | ||
| persistModifications(recordingId); | ||
| } else { | ||
| localStorage.removeItem(MOCKSI_MODIFICATIONS); | ||
| // undoModifications() | ||
| } | ||
| localStorage.setItem(MOCKSI_RECORDING_STATE, RecordingState.CREATE); | ||
| localStorage.removeItem(MOCKSI_RECORDING_ID); |
There was a problem hiding this comment.
This is a good suggestion though
| // const previousNode = nodeWithTextArea?.cloneNode(true) | ||
| // const asd = previousNode?.parentElement?.replaceChild( | ||
| // document.createTextNode(oldValue), | ||
| // previousNode | ||
| // ) |
There was a problem hiding this comment.
I don't fully disagree with what the🐰 is saying. But for now, it’s fine so we can focus on shipping the MVP.
That being said, we’ll be stricter in future PRs
apps/mocksi-lite/utils.ts
Outdated
| import { | ||
| MOCKSI_MODIFICATIONS, | ||
| MOCKSI_RECORDING_STATE, | ||
| RecordingState, | ||
| SignupURL, | ||
| } from "./consts"; | ||
|
|
||
| type DOMModifcationsType = { |
apps/mocksi-lite/utils.ts
Outdated
| type DOMModifcationsType = { | ||
| [querySelector: string]: { nextText: string; previousText: string }; | ||
| }; |
apps/mocksi-lite/utils.ts
Outdated
| previousText: string, | ||
| ) => { | ||
| const prevModifications = JSON.parse( | ||
| localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}", | ||
| ); | ||
| let keyToSave = parentElement.localName; | ||
| if (parentElement.id) { | ||
| keyToSave += `#${parentElement.id}`; | ||
| } | ||
| if (parentElement.className) { | ||
| keyToSave += `.${parentElement.className}`; | ||
| } | ||
| // here we check if the key we built is sufficient to get an unique element when querying | ||
| const elements = document.querySelectorAll(keyToSave); | ||
| if (elements.length === 1) { | ||
| localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | ||
| ); | ||
| } else { | ||
| // if not unique, we search for the index and put it on the key. | ||
| keyToSave += `[${[...elements].indexOf(parentElement)}]`; | ||
| localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | ||
| ); | ||
| } | ||
| // to successfully implement the do/undo commands, we must save somewhere in memory all commands being executed. | ||
| const saveModificationCommand = new SaveModificationCommand(localStorage, { | ||
| keyToSave: buildQuerySelector(parentElement), | ||
| nextText: newText, | ||
| previousText, // todo test previous | ||
| }); |
There was a problem hiding this comment.
@CodeRabbit write unit tests
apps/mocksi-lite/utils.ts
Outdated
| export const persistModifications = (recordingId: string) => { | ||
| const alterations: Alteration[] = Object.entries<{ | ||
| nextText: string; | ||
| previousText: string; | ||
| }>(JSON.parse(localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}")).map( | ||
| ([querySelector, { nextText, previousText }]) => ({ | ||
| selector: querySelector, | ||
| action: previousText ? "modified" : "added", | ||
| dom_before: previousText || "", | ||
| dom_after: nextText, | ||
| }), | ||
| ); | ||
| for (const modification of Object.entries(modifications)) { | ||
| // value here is encoded, SHOULD NOT be a security risk to put it in the innerHTML | ||
| const [querySelector, value] = modification; | ||
| const hasIndex = querySelector.match(/\[[0-9]+\]/); | ||
| const updated_timestamp = new Date(); | ||
| sendMessage("updateDemo", { | ||
| id: recordingId, | ||
| recording: { updated_timestamp, alterations }, | ||
| }); | ||
| localStorage.removeItem(MOCKSI_MODIFICATIONS); | ||
| }; |
There was a problem hiding this comment.
Yes, let's start adding more error handling. Our Rollbar is already full of exceptions 🙃
apps/mocksi-lite/utils.ts
Outdated
| elemToModify.innerHTML = dom_after; | ||
| } else { | ||
| const [elemToModify] = document.querySelectorAll(querySelector); | ||
| const [elemToModify] = document.querySelectorAll(selector); | ||
| //@ts-ignore | ||
| elemToModify.innerHTML = value; | ||
| elemToModify.innerHTML = dom_after; |
There was a problem hiding this comment.
If you want to add this, just use https://github.com/cure53/DOMPurify or similar library.
apps/mocksi-lite/utils.ts
Outdated
| previousText: string, | ||
| ) => { | ||
| const prevModifications = JSON.parse( | ||
| localStorage.getItem(MOCKSI_MODIFICATIONS) || "{}", | ||
| ); | ||
| let keyToSave = parentElement.localName; | ||
| if (parentElement.id) { | ||
| keyToSave += `#${parentElement.id}`; | ||
| } | ||
| if (parentElement.className) { | ||
| keyToSave += `.${parentElement.className}`; | ||
| } | ||
| // here we check if the key we built is sufficient to get an unique element when querying | ||
| const elements = document.querySelectorAll(keyToSave); | ||
| if (elements.length === 1) { | ||
| localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | ||
| ); | ||
| } else { | ||
| // if not unique, we search for the index and put it on the key. | ||
| keyToSave += `[${[...elements].indexOf(parentElement)}]`; | ||
| localStorage.setItem( | ||
| MOCKSI_MODIFICATIONS, | ||
| JSON.stringify({ ...prevModifications, [keyToSave]: newText }), | ||
| ); | ||
| } | ||
| // to successfully implement the do/undo commands, we must save somewhere in memory all commands being executed. | ||
| const saveModificationCommand = new SaveModificationCommand(localStorage, { | ||
| keyToSave: buildQuerySelector(parentElement), | ||
| nextText: newText, | ||
| previousText, // todo test previous | ||
| }); | ||
| commandsExecuted.push(saveModificationCommand); | ||
| saveModificationCommand.execute(); |
* [NC]: adds url on create recording and edit and play button validation * [NC]: biome fixes
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apps/mocksi-lite/background.ts (5 hunks)
- apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx (2 hunks)
- apps/mocksi-lite/content/Popup/CreateDemo/Form.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx
Files skipped from review as they are similar to previous changes (1)
- apps/mocksi-lite/background.ts
| @@ -49,7 +49,7 @@ const Form = ({ onCancel }: FormProps) => { | |||
| /> | |||
| </div> | |||
| <div className={"mt-[42px] flex justify-end gap-4"}> | |||
| <Button onClick={onCancel} variant={Variant.secondary}> | |||
| <Button onClick={() => onCancel()} variant={Variant.secondary}> | |||
There was a problem hiding this comment.
Consider simplifying the onClick handler for the "Cancel" button unless there are specific reasons for using an arrow function.
Using an arrow function is generally necessary when this binding is an issue, or when passing additional parameters dynamically. If onCancel does not utilize this and no additional parameters are passed, you could revert to the simpler onClick={onCancel}.
|
@elg0nz And yes, this is a first glance of implementing command patterns. |
Summary by CodeRabbit
New Features
recordingIdparameter to the editor mode functions, allowing for better recording management.DemoItemcomponent with a newuuidprop for improved item tracking.Bug Fixes
onClickevent handler for consistent behavior.Enhancements