adds top toolbar, including undo #187
Conversation
|
Warning Rate limit exceeded@fitzk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 37 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThis pull request introduces new components and refactors existing code to enhance iframe management and event handling within the application. The Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
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
Outside diff range and nitpick comments (1)
apps/mocksi-lite-next/src/pages/content/top-iframe.tsx (1)
6-7: Consider removing the unused state variable.The
minimizedstate variable is initialized but not used in the component. Consider removing it to improve code clarity.Apply this diff to remove the unused state variable:
const [show, setShow] = React.useState(false); -const [minimized, setMinimized] = React.useState(false);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- apps/mocksi-lite-next/src/pages/content/main-iframe.tsx (1 hunks)
- apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx (2 hunks)
- apps/mocksi-lite-next/src/pages/content/top-iframe.tsx (1 hunks)
- apps/mocksi-lite-next/src/pages/events.ts (1 hunks)
Additional context used
Biome
apps/mocksi-lite-next/src/pages/content/main-iframe.tsx
[error] 204-204: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
Additional comments not posted (13)
apps/mocksi-lite-next/src/pages/events.ts (1)
19-20: LGTM!The new events
HIDE_TOOLBARandSHOW_TOOLBARare a great addition to theDemoEditEventsenum. They follow the existing naming convention and will enable developers to manage toolbar visibility programmatically. This change aligns well with the PR objectives of introducing a new toolbar and enhancing the user experience.apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx (4)
2-4: LGTM!The new imports for
LayoutEvents,MainIframe, andTopIframeare correctly specified and follow the proper syntax. This change aligns with the refactoring approach mentioned in the PR summary to introduce a more modular structure for handling iframes.
31-33: LGTM!The error handling for the missing content root element is implemented correctly. Throwing an error with a descriptive message is a good practice to handle unexpected scenarios and provide meaningful feedback for debugging.
37-44: LGTM!The new
Iframescomponent is implemented correctly as a functional component and renders theTopIframeandMainIframecomponents using proper JSX syntax. This change aligns with the modular approach mentioned in the PR summary to encapsulate iframe functionality within separate components.
48-48: LGTM!The rendering logic is updated correctly to use the new
Iframescomponent within theroot.rendercall. The conditional rendering based on themountedflag ensures that the React tree is not remounted unnecessarily. This change aligns with the refactoring approach mentioned in the PR summary to delegate iframe rendering to the new component.apps/mocksi-lite-next/src/pages/content/top-iframe.tsx (3)
10-46: LGTM!The event handling logic using
chrome.runtime.onMessage.addListeneris implemented correctly. The component appropriately shows/hides the iframe and adjusts its dimensions based on the received events.
48-67: LGTM!The iframe rendering using React's
createPortalis implemented correctly. The iframe is styled appropriately and the source URL is set using an environment variable.
70-70: LGTM!The component is correctly exported as the default export for use in other parts of the application.
apps/mocksi-lite-next/src/pages/content/main-iframe.tsx (5)
24-72: LGTM!The function correctly computes and returns the styles for positioning the iframe based on the provided arguments. The error handling for missing properties is appropriate, and the switch statement is correctly implemented.
82-250: LGTM!The
MainIframecomponent is well-structured and correctly handles the various events and DOM modifications. The logic for preventing duplicate modifications and adjusting the iframe's visibility and size is properly implemented. The component effectively integrates with theReactorandHighlighterclasses to provide the desired functionality.Tools
Biome
[error] 204-204: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
91-145: LGTM!The
handleStartStopDemoEventfunction correctly handles starting and stopping demos based on the received request. The logic for preventing duplicate modifications and transitioning between states is well-implemented. The use of regular expressions to check the request message is appropriate and effective.
147-161: LGTM!The
findReplaceAllfunction correctly constructs and applies the find and replace modification using theReactorinstance. The use of thereplaceAllaction andbodyselector is appropriate for the desired functionality. The function is straightforward and effectively integrates with theReactorclass.
163-221: LGTM!The
useEffecthook correctly sets up a listener for messages from the Chrome extension and handles the various events effectively. The integration with theReactorinstance for applying modifications, exporting the DOM, and handling chat messages is properly implemented. The adjustment of the iframe's visibility and size based on layout events is also well-handled. The hook is comprehensive and covers all the necessary functionality.Tools
Biome
[error] 204-204: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
fitzk
changed the title
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-next/src/pages/background/index.ts (6 hunks)
- apps/mocksi-lite-next/src/pages/content/main-iframe.tsx (1 hunks)
- apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx (2 hunks)
- apps/mocksi-lite-next/src/pages/content/top-iframe.tsx (1 hunks)
- apps/mocksi-lite-next/src/pages/events.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/mocksi-lite-next/src/pages/content/top-iframe.tsx
Additional context used
Biome
apps/mocksi-lite-next/src/pages/content/main-iframe.tsx
[error] 82-82: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
Additional comments not posted (18)
apps/mocksi-lite-next/src/pages/events.ts (1)
19-21: LGTM!The new enum values align with the PR objectives and follow the correct naming conventions. They have clear and descriptive names that convey their purpose and do not conflict with existing values. These additions will enable the implementation of the planned features mentioned in the PR description.
apps/mocksi-lite-next/src/pages/content/main-iframe.tsx (7)
5-9: LGTM!The
IframePositionenum is defined correctly and follows the naming convention.
11-19: LGTM!The
IframeResizeArgsinterface is defined correctly and follows the naming convention.
21-69: LGTM!The
getIframeStylesfunction correctly handles the different iframe positions and returns the appropriate styles. The use of a switch statement and merging the common and position-specific styles is a clean and readable approach.The static analysis hint about wrapping the
stylesdeclaration in a block is a false positive because thestylesvariable is not accessed outside the switch clause.
71-121: LGTM!The
MainIframecomponent is implemented correctly and follows the best practices for React components. The use of theuseRefhook to get a reference to the iframe element, theuseEffecthook to add a message listener, and theReactDOM.createPortalmethod to render the iframe outside the React tree are all good approaches. The component correctly handles theLayoutEventsmessages and updates the iframe styles accordingly using thegetIframeStylesfunction.Tools
Biome
[error] 82-82: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
123-123: LGTM!The
MainIframecomponent is exported correctly as the default export.
1-4: LGTM!The required dependencies are imported correctly.
97-115: LGTM!The iframe element is defined correctly with the required attributes and styles. The
srcattribute is set to the correct URL using theimport.meta.env.VITE_NEST_APPenvironment variable, and thestyleattribute is set to the correct styles to position the iframe and handle the z-index.apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx (9)
5-8: LGTM!The updated imports align with the refactoring mentioned in the AI-generated summary. Modularizing the iframe handling into separate components should enhance maintainability and readability.
40-48: LGTM!The
Extensioncomponent serves as the main entry point for rendering the iframes and handling events. The use ofprevStartStopDemoEventRefshould help in handling demo start/stop events more efficiently and avoid duplicate events. The creation ofReactorandgetHighlighterinstances suggests that they will be used in the event handling logic.
49-103: LGTM!The
handleStartStopDemoEventfunction handles the logic for starting and stopping demos based on the request message. It uses regular expressions to determine if the request is to start or stop a demo, which allows for flexibility in the message format. The function ensures that modifications persist in the DOM when transitioning between edit and play states, which is a good optimization. It also avoids mounting the same modifications more than once for performance reasons.
105-119: LGTM!The
findReplaceAllfunction provides a convenient way to perform a find and replace operation on the DOM. It encapsulates the logic for creating a modification request and interacting with the reactor, making the code more readable and maintainable. The function returns the modifications, which can be used for further processing or updating the UI.
121-182: LGTM!The
useEffecthook is used to add a listener for messages from the Chrome runtime, which provides a centralized way to handle events and makes the code more organized and maintainable. The listener handles a wide range of events such as demo start/stop, new edits, chat messages, and undo operations, and performs the necessary actions using the reactor. It sends a response back to the background script with the relevant data, which can be used to update the UI or perform further actions. The use of theuseEffecthook ensures that the listener is added only once, which prevents unnecessary re-renders and improves performance.
183-189: LGTM!The
Extensioncomponent serves as the main entry point for rendering theTopIframeandMainIframecomponents, which makes the code more organized and maintainable. The use of separate components for the iframes allows for better separation of concerns and reusability. The use of a fragment allows returning multiple elements without adding an extra DOM node.
Line range hint
191-211: LGTM!The listener for messages from the Chrome runtime handles the
LayoutEvents.MOUNTevent and renders theExtensioncomponent if it's not already mounted. It finds the root container element usingdocument.querySelectorand creates a root usingcreateRoot. Themountedflag is used to ensure that the component is not mounted multiple times, which prevents unnecessary re-renders and improves performance. The listener is added outside the component, which means it will be executed only once when the script is loaded. This provides a way to mount theExtensioncomponent when the extension is loaded, which is necessary for the extension to function properly.
Line range hint
1-211: The code changes are consistent with the AI-generated summary.The AI-generated summary accurately captures the main points of the refactoring, such as:
- The modularization of iframe handling into separate components (
MainIframeandTopIframe).- The streamlining of event handling logic for messages from the Chrome runtime.
- The shift in how modifications are managed using the
ReactorandgetHighlighterinstances.The code changes are consistent with these points and implement the refactoring as described in the summary.
Line range hint
1-211: Verify the impact of the code changes on the rest of the codebase.The code changes seem to be mostly self-contained within the
mocksi-extension.tsxfile. The changes involve importing new components (MainIframeandTopIframe) and event types (AppEvents,DemoEditEvents,LayoutEvents), which suggests that these components and event types are defined elsewhere in the codebase.Please ensure that these new components and event types are properly defined and used in the rest of the codebase to avoid any potential issues.
#!/bin/bash # Description: Verify that the new components and event types are properly defined and used in the rest of the codebase. # Test 1: Search for the definition of the new components. echo "Searching for the definition of the new components..." fd -e tsx -e ts MainIframe TopIframe # Test 2: Search for the usage of the new components. echo "Searching for the usage of the new components..." rg -w MainIframe TopIframe # Test 3: Search for the definition of the new event types. echo "Searching for the definition of the new event types..." fd -e tsx -e ts AppEvents DemoEditEvents LayoutEvents # Test 4: Search for the usage of the new event types. echo "Searching for the usage of the new event types..." rg -w App </blockquote></details> <details> <summary>apps/mocksi-lite-next/src/pages/background/index.ts (1)</summary><blockquote> `1-6`: **Imports updated to include `DemoEditEvents`** The import statements have been correctly updated to include `DemoEditEvents`, enabling the handling of new demo editing events. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| mainIframeSrcPort.postMessage({ | ||
| ...request, | ||
| message: AppEvents.EDIT_DEMO_STOP, | ||
| }); |
There was a problem hiding this comment.
Add null checks before using postMessage on ports
To avoid runtime errors, ensure that mainIframeSrcPort and topIframeSrcPort are not null before calling postMessage.
Apply these changes to add null checks:
At lines 164-167:
+if (mainIframeSrcPort) {
mainIframeSrcPort.postMessage({
...request,
message: AppEvents.EDIT_DEMO_STOP,
});
+} else {
+ console.error('mainIframeSrcPort is not connected');
+}At lines 232-235:
+if (mainIframeSrcPort) {
await mainIframeSrcPort.postMessage({
...response,
status: "ok", // response handler expects status
});
+} else {
+ console.error('mainIframeSrcPort is not connected');
+}At lines 243-246:
+if (topIframeSrcPort) {
await topIframeSrcPort.postMessage({
...response,
status: "ok",
});
+} else {
+ console.error('topIframeSrcPort is not connected');
+}Also applies to: 232-235, 243-246
| let mainIframeSrcPort: chrome.runtime.Port; | ||
| let topIframeSrcPort: chrome.runtime.Port; | ||
|
|
||
| chrome.runtime.onConnectExternal.addListener((port) => { | ||
| console.log("connecting...", port); | ||
| if (port.name === "extension/main") { | ||
| mainIframeSrcPort = port; | ||
| } | ||
| if (port.name === "extension/top") { | ||
| topIframeSrcPort = port; | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Initialize ports to prevent undefined variables
mainIframeSrcPort and topIframeSrcPort may be used before they are assigned, leading to potential runtime errors if the ports are not yet connected.
Consider initializing the ports with null and adding type annotations to reflect that they can be null:
-let mainIframeSrcPort: chrome.runtime.Port;
-let topIframeSrcPort: chrome.runtime.Port;
+let mainIframeSrcPort: chrome.runtime.Port | null = null;
+let topIframeSrcPort: chrome.runtime.Port | null = null;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.
| let mainIframeSrcPort: chrome.runtime.Port; | |
| let topIframeSrcPort: chrome.runtime.Port; | |
| chrome.runtime.onConnectExternal.addListener((port) => { | |
| console.log("connecting...", port); | |
| if (port.name === "extension/main") { | |
| mainIframeSrcPort = port; | |
| } | |
| if (port.name === "extension/top") { | |
| topIframeSrcPort = port; | |
| } | |
| }); | |
| let mainIframeSrcPort: chrome.runtime.Port | null = null; | |
| let topIframeSrcPort: chrome.runtime.Port | null = null; | |
| chrome.runtime.onConnectExternal.addListener((port) => { | |
| console.log("connecting...", port); | |
| if (port.name === "extension/main") { | |
| mainIframeSrcPort = port; | |
| } | |
| if (port.name === "extension/top") { | |
| topIframeSrcPort = port; | |
| } | |
| }); |
| if ( | ||
| request.source === "extension/top" && | ||
| request.message === AppEvents.EDIT_DEMO_STOP | ||
| ) { | ||
| // notify extension/main that demo edit mode exited in extension/top | ||
| mainIframeSrcPort.postMessage({ | ||
| ...request, | ||
| message: AppEvents.EDIT_DEMO_STOP, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Handle potential errors when posting messages
Ensure that mainIframeSrcPort is connected before calling postMessage to prevent exceptions if the port is unavailable.
Add a null check at lines 164-167:
+if (mainIframeSrcPort) {
mainIframeSrcPort.postMessage({
...request,
message: AppEvents.EDIT_DEMO_STOP,
});
+} else {
+ console.error('mainIframeSrcPort is not connected');
+}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.
| if ( | |
| request.source === "extension/top" && | |
| request.message === AppEvents.EDIT_DEMO_STOP | |
| ) { | |
| // notify extension/main that demo edit mode exited in extension/top | |
| mainIframeSrcPort.postMessage({ | |
| ...request, | |
| message: AppEvents.EDIT_DEMO_STOP, | |
| }); | |
| } | |
| if ( | |
| request.source === "extension/top" && | |
| request.message === AppEvents.EDIT_DEMO_STOP | |
| ) { | |
| // notify extension/main that demo edit mode exited in extension/top | |
| if (mainIframeSrcPort) { | |
| mainIframeSrcPort.postMessage({ | |
| ...request, | |
| message: AppEvents.EDIT_DEMO_STOP, | |
| }); | |
| } else { | |
| console.error('mainIframeSrcPort is not connected'); | |
| } | |
| } |
| // send message to iframes and reactor in mocksi-extension | ||
| chrome.tabs.sendMessage(tab.id, request, async (response) => { | ||
| console.log("response from content script in background:", response); | ||
| if (response.message === DemoEditEvents.UNDO) { | ||
| // pass updated modifications from reactor to extension/main to store | ||
| await mainIframeSrcPort.postMessage({ | ||
| ...response, | ||
| status: "ok", // response handler expects status | ||
| }); | ||
| } | ||
| if ( | ||
| request.message === AppEvents.EDIT_DEMO_START || | ||
| request.message === DemoEditEvents.NEW_EDIT || | ||
| request.message === DemoEditEvents.CHAT_RESPONSE | ||
| ) { | ||
| // notify extension/top # of edits changed | ||
| await topIframeSrcPort.postMessage({ | ||
| ...response, | ||
| status: "ok", | ||
| }); | ||
| } | ||
| sendResponse(response); | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
Refactor nested conditions for clarity
The nested if statements within the message handling can be refactored to improve readability and maintainability.
Consider restructuring the conditions:
if (request.message === LayoutEvents.HIDE ||
request.message === LayoutEvents.RESIZE ||
request.message === LayoutEvents.SHOW) {
prevLayoutEvent = request.message;
}
-if (request.message === AppEvents.PLAY_DEMO_START) {
+else if (request.message === AppEvents.PLAY_DEMO_START) {
showPlayIcon(tab.id);
}
-if (request.message === AppEvents.PLAY_DEMO_STOP ||
- request.message === AppEvents.EDIT_DEMO_START ||
- request.message === AppEvents.EDIT_DEMO_STOP) {
+else if (request.message === AppEvents.PLAY_DEMO_STOP ||
+ request.message === AppEvents.EDIT_DEMO_START ||
+ request.message === AppEvents.EDIT_DEMO_STOP) {
showDefaultIcon(tab.id);
}Committable suggestion was skipped due to low confidence.
| chrome.tabs.sendMessage(tab.id, request, async (response) => { | ||
| console.log("response from content script in background:", response); | ||
| if (response.message === DemoEditEvents.UNDO) { | ||
| // pass updated modifications from reactor to extension/main to store | ||
| await mainIframeSrcPort.postMessage({ | ||
| ...response, | ||
| status: "ok", // response handler expects status | ||
| }); | ||
| } | ||
| if ( | ||
| request.message === AppEvents.EDIT_DEMO_START || | ||
| request.message === DemoEditEvents.NEW_EDIT || | ||
| request.message === DemoEditEvents.CHAT_RESPONSE | ||
| ) { | ||
| // notify extension/top # of edits changed | ||
| await topIframeSrcPort.postMessage({ | ||
| ...response, | ||
| status: "ok", | ||
| }); | ||
| } | ||
| sendResponse(response); | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
Ensure sendResponse is called correctly in asynchronous callbacks
When using asynchronous code within message listeners, ensure that sendResponse is called properly, and return true to keep the message channel open.
Modify the callback to properly handle the asynchronous response:
chrome.tabs.sendMessage(tab.id, request, async (response) => {
console.log("response from content script in background:", response);
// ... existing code ...
sendResponse(response);
- return true;
});
+ return true;
+});And ensure the listener returns true immediately after setting up the asynchronous operation:
(async () => {
// ... existing code ...
return true;
})();
+return true;Committable suggestion was skipped due to low confidence.
fitzk
force-pushed
the
MOC-274_top-toolbar
branch
from
7e2f929
to
c0dcdaf
Compare
Summary by CodeRabbit
New Features
MainIframeandTopIframe) for enhanced demo experiences.HIDE_TOOLBAR,SHOW_TOOLBAR, andUNDO).Refactor
These updates significantly improve user interaction and application responsiveness during demos.