[MOC-62][MOC-59] Recording state fixes for sign up and getting recordings for first page #80
Conversation
WalkthroughWalkthroughThe changes streamline recording management in the Changes
Sequence Diagram(s)sequenceDiagram
participant B as Background Script
participant T as Tab (Content Script)
participant U as Utils
participant API as API Server
B->>U: getRecordingsStorage()
U-->>B: recordings
B->>T: sendMessage({ recordings })
T-->>B: acknowledgment
note over content.tsx: Old Flow
T->>+T: handle message & logic
T-->>-T: update recording state
note over content.tsx: New Flow
T->>+handleRecordingState: Check login and state
handleRecordingState->>API: Verify login status
API-->>handleRecordingState: { state, email }
handleRecordingState-->>-T: Update recording state
Note over B,T,U, API: Simplified logic and enhanced modularity
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: 3
Outside diff range and nitpick comments (1)
apps/mocksi-lite/utils.ts (1)
Line range hint
36-36: Security Issue: Exposed API Key.The hardcoded API key in the
authOptionsconfiguration is a security risk and should be managed through environment variables or a secure vault solution.- clientID: "3XDxVDUz3W3038KmRvkJSjkIs5mGj7at", + clientID: process.env.AUTH0_CLIENT_ID,
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- apps/mocksi-lite/background.ts (3 hunks)
- apps/mocksi-lite/content/Popup/CreateDemo/index.tsx (2 hunks)
- apps/mocksi-lite/content/content.tsx (2 hunks)
- apps/mocksi-lite/utils.ts (1 hunks)
Additional context used
Learnings (1)
apps/mocksi-lite/content/Popup/CreateDemo/index.tsx (1)
User: elg0nz PR: Mocksi/HARlighter#54 File: apps/mocksi-lite/content/Popup/CreateDemo/index.tsx:22-33 Timestamp: 2024-06-14T20:59:01.152Z Learning: The `getRecordings` function in `CreateDemo` component of `mocksi-lite` should log errors using `console.error` to help identify issues in edge cases.
Gitleaks
apps/mocksi-lite/utils.ts
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (2)
apps/mocksi-lite/content/Popup/CreateDemo/index.tsx (1)
58-60: Styling change approved.The change from
"flex-1"to"flex-grow"is appropriate for ensuring that the div expands to take up any available space within its flex container, which is suitable for the display of recordings.apps/mocksi-lite/utils.ts (1)
292-292: SimplifygetRecordingsStoragefunction.The removal of
JSON.parseis appropriate if the recordings are already in the correct format, reducing unnecessary parsing overhead.
| const sortedRecordings = newRecordings.sort( | ||
| (a: Recording, b: Recording) => | ||
| a.updated_timestamp > b.updated_timestamp ? -1 : 0, | ||
| ); | ||
| setRecordings(sortedRecordings); |
There was a problem hiding this comment.
Optimize sorting logic and add error handling.
The sorting function should ensure that recordings are sorted in descending order by updated_timestamp. The current implementation might not handle all cases correctly. Also, consider adding explicit error logging to help identify issues during fetching.
- a.updated_timestamp > b.updated_timestamp ? -1 : 0,
+ a.updated_timestamp > b.updated_timestamp ? -1 : 1,+ console.error("Failed to fetch recordings:", error);
There was a problem hiding this comment.
@elg0nz, thank you for the positive feedback! If you have any more questions or need further assistance as you implement the changes, feel free to reach out. Happy coding! 🐰
Learnings used
Learnt from: elg0nz
PR: Mocksi/HARlighter#54
File: apps/mocksi-lite/content/Popup/CreateDemo/index.tsx:22-33
Timestamp: 2024-06-14T20:59:01.152Z
Learning: The `getRecordings` function in `CreateDemo` component of `mocksi-lite` should log errors using `console.error` to help identify issues in edge cases.
Learnt from: nicolaschapur
PR: Mocksi/HARlighter#80
File: apps/mocksi-lite/content/content.tsx:43-93
Timestamp: 2024-06-28T15:36:05.992Z
Learning: The `getRecordingsStorage` function in `utils.ts` returns an empty array when `recordings` is undefined or null, ensuring robustness in downstream function calls.
| getRecordingsStorage() | ||
| .then((recordings) => { | ||
| if (recordings.length) { | ||
| chrome.tabs.sendMessage(currentTabId || 0, { | ||
| text: "clickedIcon", | ||
| }); | ||
| } else { | ||
| getRecordings().then(() => { | ||
| chrome.tabs.sendMessage(currentTabId || 0, { | ||
| text: "clickedIcon", | ||
| }); | ||
| }); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| console.error(`Failed to get recordings: ${err}`); | ||
| chrome.tabs.sendMessage(currentTabId || 0, { | ||
| text: "clickedIcon", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Refactor recording retrieval logic in background.ts.
The logic to handle recordings based on their presence is a good improvement. However, the error handling could be more robust, and the repeated chrome.tabs.sendMessage calls can be optimized.
- } else {
- getRecordings().then(() => {
- chrome.tabs.sendMessage(currentTabId || 0, {
- text: "clickedIcon",
- });
- });
- }
+ } else if (await getRecordings()) {
+ chrome.tabs.sendMessage(currentTabId || 0, { text: "clickedIcon" });
+ }+ console.error("Error while retrieving recordings:", err);
nicolaschapur
changed the title
| text: "clickedIcon", | ||
| }); | ||
| getRecordingsStorage() | ||
| .then((recordings) => { |
There was a problem hiding this comment.
let's refactor this.
I cannot see any reason to call
chrome.tabs.sendMessage(currentTabId || 0, {
text: "clickedIcon",
});three times, with the same parameters.
Specially when you can use finally in this promise
| const sortedRecordings = newRecordings.sort( | ||
| (a: Recording, b: Recording) => | ||
| a.updated_timestamp > b.updated_timestamp ? -1 : 0, | ||
| ); | ||
| setRecordings(sortedRecordings); |
| let state = recordingState; | ||
| console.log({ recordingState }); | ||
|
|
||
| if (email) { |
There was a problem hiding this comment.
this function is already too long.
Extract this out so you can do:
`tsx
state = email ? setLoggedInsState() : setLoggedOutState()
return { state, email };
}
Where `setLoggedOutState` would look like this:
```tsx
if (
recordingState === RecordingState.UNAUTHORIZED &&
window.location.origin !== SignupURL
) {
window.open(SignupURL);
}
sendMessage("resetIcon");
return RecordingState.UNAUTHORIZED;
This PR adds a bunch o fixes for recording states and signed or not users. Also adds a recording call when opening the extension, to check if we need to show the list of recordings as the first page or the record button.
Screen.Recording.2024-06-28.at.12.24.30.mov
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Performance