MOC-226: play mode icon #164
Conversation
WalkthroughWalkthroughThe changes involve updates to a Chrome extension's background script and a React application's iframe component. The background script now tracks the last request state and communicates it to the content script. The iframe's styling and behavior have been refined for improved responsiveness and visibility management based on various message types, enhancing overall performance. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
apps/mocksi-lite-next/public/play-icon.pngis excluded by!**/*.png
Files selected for processing (2)
- apps/mocksi-lite-next/src/pages/background/index.ts (5 hunks)
- apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx (4 hunks)
Additional comments not posted (5)
apps/mocksi-lite-next/src/pages/background/index.ts (3)
6-9: LGTM!The initialization of
prevRequestis correct and straightforward.The code changes are approved.
88-99: LGTM!The updates to the
chrome.action.onClickedevent listener improve the continuity of user interactions by maintaining the state of the last request and updating the extension icon accordingly.The code changes are approved.
Line range hint
118-186: LGTM!The updates to the
chrome.runtime.onMessageExternalevent listener provide better debugging information and ensure that the last state is preserved for future interactions.The code changes are approved.
apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx (2)
Line range hint
94-131: LGTM!The updates to the iframe's visibility and positioning based on different message types enhance the responsiveness and visibility management of the iframe.
The code changes are approved.
156-163: LGTM!The addition of the
loading="lazy"attribute to the iframe improves performance by deferring loading until the iframe is in the viewport.The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/mocksi-lite-next/src/pages/background/index.ts (5 hunks)
- apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- apps/mocksi-lite-next/src/pages/background/index.ts
- apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/mocksi-lite-next/src/pages/background/index.ts (5 hunks)
- apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- apps/mocksi-lite-next/src/pages/background/index.ts
- apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx
| @@ -80,6 +85,18 @@ addEventListener("install", () => { | |||
| chrome.action.onClicked.addListener((tab) => { | |||
| if (tab.id) { | |||
There was a problem hiding this comment.
Lets get rid of the tab.id nesting by refactoring this to:
if (!tab?.id) {
console.log("No tab found, could not mount extension");
return;
}
chrome.tabs.sendMessage(tab.id, { message: "mount-extension" });
if (prevRequest.message) {
chrome.tabs.sendMessage(tab.id, {
data: prevRequest.data,
message: prevRequest.message,
});
}
if (prevRequest.message === "PLAY") {
chrome.action.setIcon({
path: "play-icon.png",
tabId: tab.id,
});
}| @@ -98,7 +115,10 @@ chrome.runtime.onMessage.addListener( | |||
|
|
|||
| chrome.runtime.onMessageExternal.addListener( | |||
| (request, _sender, sendResponse) => { | |||
| console.log("Received message from external:", request); | |||
| // This logging is useful and only shows up in the service worker | |||
| @@ -132,6 +152,19 @@ chrome.runtime.onMessageExternal.addListener( | |||
| console.log("No active tab found, could not send message"); | |||
| return true; | |||
| } | |||
|
|
|||
| if (request.message === "PLAY") { | |||
There was a problem hiding this comment.
Let's refactor this code to use a switch statement for better readability and scalability:
switch (request.message) {
case "PLAY":
await chrome.action.setIcon({
path: "play-icon.png",
tabId: tab.id,
});
break;
case "MINIMIZED":
// No action needed for "MINIMIZED"
break;
default:
chrome.action.setIcon({
path: "mocksi-icon.png",
tabId: tab.id,
});
break;
}This refactor handles the different cases of request.message more cleanly. If there are more cases to handle in the future, adding them to the switch statement will be much easier.
| height: "150px", | ||
| top: "0px", | ||
| /* top right bottom left */ |
There was a problem hiding this comment.
nit: Remove the comment /* top right bottom left */
Tip: To remember the order of these CSS properties, think of it as moving clockwise like on a clock face: starting at 12 (top), then 3 (right), 6 (bottom), and finally 9 (left).
| @@ -91,6 +91,7 @@ chrome.runtime.onMessage.addListener((request) => { | |||
| ) { | |||
| reactor.detach(); | |||
| } | |||
|
|
|||
| // resize iframe | |||
| if (iframeRef.current) { | |||
There was a problem hiding this comment.
Hey, awesome job on this! I wanted to suggest a small tweak that could make the code a bit cleaner. I’ve found that when an if statement goes over five lines, it’s usually a sign that it could benefit from some refactoring. It just keeps things more readable and easier to maintain.
For the part where you’re setting styles based on request.message, we could simplify things by moving that logic into a separate function. This way, the main code stays focused, and it’ll be easier to update or debug if we need to adjust the styles down the line.
Here’s a quick refactor idea:
// Function to get styles based on the message
function getIframeStyles(message: string): Partial<CSSStyleDeclaration> {
switch (message) {
case "ANALYZING":
case "PLAY":
case "RECORDING":
return {
display: "block",
height: "150px",
inset: "0px 10px auto auto",
width: "400px",
};
case "MINIMIZED":
return {
display: "none",
inset: "0px 0px auto auto",
};
case "EDITING":
case "INIT":
case "LIST":
case "READYTORECORD":
case "SETTINGS":
case "STOP_EDITING":
case "STOP_PLAYING":
case "UNAUTHORIZED":
return {
display: "block",
height: "600px",
inset: "auto 10px 10px auto",
width: "500px",
};
default:
return {};
}
}
// Resize iframe with the new styles
if (iframeRef.current) {
const styles = getIframeStyles(request.message);
Object.assign(iframeRef.current.style, styles);
}This should help keep things more modular and easier to manage. Let me know what you think!
There was a problem hiding this comment.
for sure changed, I have plans to clean this up
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/mocksi-lite-next/src/pages/background/index.ts (5 hunks)
- apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- apps/mocksi-lite-next/src/pages/background/index.ts
- apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx
https://github.com/Mocksi/nest/pull/32
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Performance Improvements