Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MOC-56] waiting until update finishes before showing rec list #70

Merged
merged 10 commits into from
Jun 21, 2024
Merged

[MOC-56] waiting until update finishes before showing rec list #70

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5274d5c
now waiting til we update the value on storage before opening recordi…
NicoMorenoSirius Jun 21, 2024
68c41b7
run lint
NicoMorenoSirius Jun 21, 2024
97abe78
Merge remote-tracking branch 'origin' into moc-56-demo-list-update
NicoMorenoSirius Jun 21, 2024
fb61a03
merge with main
NicoMorenoSirius Jun 21, 2024
64ff21a
run lint
NicoMorenoSirius Jun 21, 2024
6317e68
undoing changes after pressing stop button
NicoMorenoSirius Jun 21, 2024
807175d
fixed recording list style
NicoMorenoSirius Jun 21, 2024
701f74c
code rabbit reviews
NicoMorenoSirius Jun 21, 2024
3aad6fd
state update fix
NicoMorenoSirius Jun 21, 2024
059e284
run lint
NicoMorenoSirius Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/mocksi-lite/content/EditMode/editMode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const setEditorMode = async (turnOn: boolean, recordingId?: string) => {
return;
}
if (recordingId) {
persistModifications(recordingId);
await persistModifications(recordingId);
}
undoModifications();
await chrome.storage.local.set({
Expand All @@ -31,6 +31,7 @@ export const setEditorMode = async (turnOn: boolean, recordingId?: string) => {
restoreNodes();
cancelEditWithoutChanges(document.getElementById("mocksiSelectedText"));
document.normalize();
return;
};

function onDoubleClickText(event: MouseEvent) {
Expand Down
20 changes: 12 additions & 8 deletions apps/mocksi-lite/content/Toast/EditToast.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,22 @@ const EditToast = ({ state, onChangeState }: EditToastProps) => {
};

const handleClose = async () => {
onChangeState(RecordingState.CREATE);
const recordingId = await loadRecordingId();
setEditorMode(false, recordingId);
await setEditorMode(false, recordingId);
onChangeState(RecordingState.CREATE);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling in asynchronous calls.

The handleClose function now awaits the setEditorMode before changing the recording state. This is a good change for ensuring the sequence of operations, but it's crucial to handle potential errors that might arise from these asynchronous operations to prevent the UI from being in an inconsistent state.

-		await setEditorMode(false, recordingId);
-		onChangeState(RecordingState.CREATE);
+		try {
+			await setEditorMode(false, recordingId);
+			onChangeState(RecordingState.CREATE);
+		} catch (error) {
+			console.error('Failed to set editor mode:', error);
+			// Handle error appropriately here
+		}
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.

Suggested change
await setEditorMode(false, recordingId);
onChangeState(RecordingState.CREATE);
try {
await setEditorMode(false, recordingId);
onChangeState(RecordingState.CREATE);
} catch (error) {
console.error('Failed to set editor mode:', error);
// Handle error appropriately here
}

};
return (
<Toast className={"mt-3 min-w-64 p-3 flex flex-row items-center gap-6"}>
<div
className="cursor-pointer"
onClick={handleClose}
onClick={() => {
setEditorMode(false);
onChangeState(RecordingState.CREATE);
}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to remove redundant code.

The onClick handler here sets the editor mode to false and then changes the state to CREATE. This logic is repeated multiple times throughout the file. Consider refactoring this into a separate function to reduce redundancy and improve maintainability.

+	const resetEditorAndChangeState = async () => {
+		try {
+			await setEditorMode(false);
+			onChangeState(RecordingState.CREATE);
+		} catch (error) {
+			console.error('Error resetting editor and changing state:', error);
+		}
+	};

-	onClick={() => {
-		setEditorMode(false);
-		onChangeState(RecordingState.CREATE);
-	}}
+	onClick={resetEditorAndChangeState}
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.

Suggested change
onClick={() => {
setEditorMode(false);
onChangeState(RecordingState.CREATE);
}}
const resetEditorAndChangeState = async () => {
try {
await setEditorMode(false);
onChangeState(RecordingState.CREATE);
} catch (error) {
console.error('Error resetting editor and changing state:', error);
}
};
onClick={resetEditorAndChangeState}

onKeyUp={async (event) => {
if (event.key === "Escape") {
handleClose();
setEditorMode(false);
onChangeState(RecordingState.CREATE);
}
}}
>
Expand All @@ -59,15 +63,15 @@ const EditToast = ({ state, onChangeState }: EditToastProps) => {
<div
className="cursor-pointer text-[#009875]"
onClick={async () => {
onChangeState(RecordingState.CREATE);
const recordingId = await loadRecordingId();
setEditorMode(false, recordingId);
await setEditorMode(false, recordingId);
onChangeState(RecordingState.CREATE);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistent error handling and refactoring suggestion.

This onClick handler is similar to the previous handlers but lacks error handling. Apply the same error handling as suggested earlier and consider using the refactored function to handle this event.

}}
onKeyUp={async (event) => {
if (event.key === "Enter") {
onChangeState(RecordingState.CREATE);
const recordingId = await loadRecordingId();
setEditorMode(false, recordingId);
await setEditorMode(false, recordingId);
onChangeState(RecordingState.CREATE);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistent error handling and refactoring suggestion.

The onKeyUp handler for the "Enter" key is another instance where the editor mode is set and the state is changed. This should also use the proposed refactored function to ensure consistency and error handling across all similar event handlers.

}
}}
>
Expand Down
9 changes: 6 additions & 3 deletions apps/mocksi-lite/content/Toast/PlayToast.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import closeIcon from "../../public/close-icon.png";
import editIcon from "../../public/edit-icon.png";
import labeledIcon from "../../public/labeled-icon.png";
import stopIcon from "../../public/stop-icon.png";
import { getAlterations, loadAlterations, sendMessage } from "../../utils";
import { getAlterations, loadAlterations, sendMessage, undoModifications } from "../../utils";
import { setEditorMode } from "../EditMode/editMode";
import Toast from "./index";

Expand All @@ -15,9 +15,9 @@ interface PlayToastProps {
const PlayToast = ({ onChangeState, close }: PlayToastProps) => {
const handleEdit = async () => {
const alterations = await getAlterations();
onChangeState(RecordingState.EDITING);
loadAlterations(alterations, true);
setEditorMode(true);
onChangeState(RecordingState.EDITING);
};
const handleHideToast = () => {
sendMessage("updateToPauseIcon");
Expand All @@ -39,7 +39,10 @@ const PlayToast = ({ onChangeState, close }: PlayToastProps) => {
<div className={"flex gap-2"}>
<Button
variant={Variant.icon}
onClick={() => onChangeState(RecordingState.CREATE)}
onClick={() => {
undoModifications()
onChangeState(RecordingState.CREATE)
}}
>
<img src={stopIcon} alt={"stopIcon"} />
</Button>
Expand Down
30 changes: 29 additions & 1 deletion apps/mocksi-lite/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,17 @@ export const saveModification = (
saveModificationCommand.execute();
};

export const persistModifications = (recordingId: string) => {
export const persistModifications = async (recordingId: string) => {
const alterations: Alteration[] = buildAlterations(domainModifications);
chrome.storage.local.set({
[MOCKSI_MODIFICATIONS]: JSON.stringify(domainModifications),
});
const updated_timestamp = new Date();
await updateRecordingsStorage({
uuid: recordingId,
updated_timestamp,
alterations,
});
sendMessage("updateDemo", {
id: recordingId,
recording: { updated_timestamp, alterations },
Expand Down Expand Up @@ -274,6 +279,29 @@ export const getRecordingsStorage = async (): Promise<Recording[]> => {
}
};

export const updateRecordingsStorage = async ({
uuid,
updated_timestamp,
alterations,
}: { uuid: string; updated_timestamp: Date; alterations: Alteration[] }) => {
try {
const recordingsFromStorage = await getRecordingsStorage();
const modifiedRecordings = recordingsFromStorage.map((recording) =>
recording.uuid === uuid
? { ...recording, uuid, updated_timestamp, alterations }
: recording,
);
const sorted = modifiedRecordings.sort((a: Recording, b: Recording) =>
a.updated_timestamp > b.updated_timestamp ? -1 : 0,
);
const recordingsStringified = JSON.stringify(sorted);
console.log("modified", recordingsFromStorage, sorted);
chrome.storage.local.set({ recordings: recordingsStringified });
} catch (err) {
console.error("Failed to save modifications from LS:", err);
throw err;
}
};
Comment on lines +282 to +304
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Unit tests for updateRecordingsStorage function are missing.

The search results indicate that the updateRecordingsStorage function is defined and used within apps/mocksi-lite/utils.ts, but there are no references to it in any test files. This suggests that the function is not currently covered by unit tests.

  • Ensure that unit tests are written for the updateRecordingsStorage function to verify its behavior under various scenarios.
Analysis chain

New Feature: Added updateRecordingsStorage function to handle storage updates.

This addition is significant as it centralizes the logic for updating recordings in storage, which can improve maintainability and testing. The function also handles errors robustly, which is crucial for reliability. Ensure that this function is covered by unit tests to verify its behavior under various scenarios.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify if unit tests cover the new `updateRecordingsStorage` function.
rg --type typescript -g '*test.ts' 'updateRecordingsStorage'

Length of output: 95

Script:

#!/bin/bash
# Verify if unit tests cover the new `updateRecordingsStorage` function.
rg --type ts -g '*test.ts' 'updateRecordingsStorage'

Length of output: 52

Script:

#!/bin/bash
# Find all test files in the repository
fd --type f --extension ts --exec rg 'updateRecordingsStorage' {}

Length of output: 148

Script:

#!/bin/bash
# Find all test files in the repository and their occurrences of `updateRecordingsStorage`
fd --type f --extension ts --exec rg --with-filename --line-number 'updateRecordingsStorage' {}

Length of output: 241

export const loadRecordingId = async () => {
return new Promise<string | undefined>((resolve) => {
chrome.storage.local.get([MOCKSI_RECORDING_ID], (result) => {
Expand Down
Loading