adds pause to action menu #945
Conversation
WalkthroughThe pull request introduces a new functionality to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![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)
Client/src/Pages/Monitors/Home/actionsMenu.jsx (1)
186-186: Prevent default behaviour in the pause/resume handler.Adding
e.preventDefault();ensures that any default actions are halted, focusing on the intended functionality.Modify the event handler:
onClick={(e) => { e.stopPropagation(); + e.preventDefault(); handlePause(e); }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Client/src/Pages/Monitors/Home/actionsMenu.jsx (5 hunks)
🧰 Additional context used
🔇 Additional comments (2)
Client/src/Pages/Monitors/Home/actionsMenu.jsx (2)
17-17: Great addition of the pauseUptimeMonitor import.Bringing in the
pauseUptimeMonitoraction is spot on for the new functionality.
30-31: Auth token extraction is solid.Extracting
authTokenfromauthStatekeeps the code clean and efficient.
| if (monitor?.isActive){ | ||
| createToast({ body: "Monitor paused successfully." }); | ||
| }else{ | ||
| createToast({ body: "Monitor resume successfully." }); |
There was a problem hiding this comment.
Fix grammatical error in toast message.
The success message should read "Monitor resumed successfully."
Apply this change:
- createToast({ body: "Monitor resume successfully." });
+ createToast({ body: "Monitor resumed successfully." });📝 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.
| createToast({ body: "Monitor resume successfully." }); | |
| createToast({ body: "Monitor resumed successfully." }); |
| {isAdmin && ( | ||
| <MenuItem | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| handlePause(e); | ||
| }} | ||
| > | ||
| {monitor?.isActive ? ( | ||
| "Pause" | ||
| ) : ( | ||
| "Resume" | ||
| )} | ||
| </MenuItem> | ||
| )} |
There was a problem hiding this comment.
Include 'isActive' in the monitor's PropTypes definition.
You're using monitor?.isActive, but isActive isn't specified in your PropTypes for monitor. Adding it ensures type checking is accurate.
Update the PropTypes like this:
monitor: PropTypes.shape({
_id: PropTypes.string,
url: PropTypes.string,
type: PropTypes.string,
+ isActive: PropTypes.bool,
}).isRequired,Committable suggestion was skipped due to low confidence.
| const handlePause = async (event) => { | ||
| try { | ||
| const action = await dispatch( | ||
| pauseUptimeMonitor({ authToken, monitorId: monitor._id }) | ||
| ); | ||
| if (pauseUptimeMonitor.fulfilled.match(action)) { | ||
| updateCallback(); | ||
| //window.location.reload(); | ||
| if (monitor?.isActive){ | ||
| createToast({ body: "Monitor paused successfully." }); | ||
| }else{ | ||
| createToast({ body: "Monitor resume successfully." }); | ||
| } | ||
| } else { | ||
| throw new Error(action.error.message); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error pausing monitor:", monitor._id, error); | ||
| createToast({ body: "Failed to pause monitor." }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Review toast message logic after pausing/resuming the monitor.
The current logic might not reflect the updated state of the monitor due to possible asynchronous updates. Since monitor?.isActive may not immediately represent the new state after dispatching pauseUptimeMonitor, consider using the action's response to determine the correct message.
Here's how you might adjust the code:
- if (monitor?.isActive){
- createToast({ body: "Monitor paused successfully." });
- } else {
- createToast({ body: "Monitor resumed successfully." });
- }
+ if (action.payload?.isActive === false){
+ createToast({ body: "Monitor paused successfully." });
+ } else {
+ createToast({ body: "Monitor resumed successfully." });
+ }📝 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 handlePause = async (event) => { | |
| try { | |
| const action = await dispatch( | |
| pauseUptimeMonitor({ authToken, monitorId: monitor._id }) | |
| ); | |
| if (pauseUptimeMonitor.fulfilled.match(action)) { | |
| updateCallback(); | |
| //window.location.reload(); | |
| if (monitor?.isActive){ | |
| createToast({ body: "Monitor paused successfully." }); | |
| }else{ | |
| createToast({ body: "Monitor resume successfully." }); | |
| } | |
| } else { | |
| throw new Error(action.error.message); | |
| } | |
| } catch (error) { | |
| console.error("Error pausing monitor:", monitor._id, error); | |
| createToast({ body: "Failed to pause monitor." }); | |
| } | |
| }; | |
| const handlePause = async (event) => { | |
| try { | |
| const action = await dispatch( | |
| pauseUptimeMonitor({ authToken, monitorId: monitor._id }) | |
| ); | |
| if (pauseUptimeMonitor.fulfilled.match(action)) { | |
| updateCallback(); | |
| //window.location.reload(); | |
| if (action.payload?.isActive === false){ | |
| createToast({ body: "Monitor paused successfully." }); | |
| } else { | |
| createToast({ body: "Monitor resumed successfully." }); | |
| } | |
| } else { | |
| throw new Error(action.error.message); | |
| } | |
| } catch (error) { | |
| console.error("Error pausing monitor:", monitor._id, error); | |
| createToast({ body: "Failed to pause monitor." }); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Client/src/Pages/Monitors/Home/actionsMenu.jsx (1)
50-71: Pause function's got the flow, but needs a little polish!Yo, this
handlePausefunction is droppin' beats like it's hot, but there's a few notes to hit:
That commented-out
window.location.reload()on line 57 is like leftover spaghetti - let's clean it up and toss it out.The error message on line 68 is vague like mom's secret recipe. How about we spice it up? Something like:
createToast({ body: `Failed to ${monitor?.isActive ? 'pause' : 'resume'} monitor. Error: ${error.message}` });Props for updating the success message logic! It's now as smooth as mom's spaghetti sliding down your throat.
Keep it real, and this function will be the MVP of the monitor game!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Client/src/Pages/Monitors/Home/actionsMenu.jsx (6 hunks)
🧰 Additional context used
🔇 Additional comments (4)
Client/src/Pages/Monitors/Home/actionsMenu.jsx (4)
17-17: Yo, this import's legit!The addition of
pauseUptimeMonitorto the imports is on point. It's like adding the secret sauce to mom's spaghetti - essential for the new pause feature to work its magic.
30-31: Auth token's in the house!Extracting that
authTokenfromauthStateis smooth like butter on mom's spaghetti. It's gonna be crucial for that pause action we're cookin' up.
Line range hint
183-206: This menu item's fresher than mom's spaghetti!Yo, this new pause/resume menu item is the real deal! It's like having a DJ's pause button right in your monitor mix. The way it switches up the text based on the monitor's vibe? That's smoother than mom's sauce. And keeping it VIP for the admins? Smart move, like saving the best meatballs for the family dinner.
Keep servin' up these hot features, and users will be lining up like it's spaghetti night at Eminem's house!
280-280: PropTypes got an upgrade, and it's fire!Adding
isActiveto themonitorPropTypes is like adding the secret spice to mom's spaghetti - it completes the recipe! You've taken that past feedback and turned it into gold. Now your type checking game is as tight as Eminem's rhymes.Keep this attention to detail, and your code will be winning rap battles left and right!
| createToast({ body: "Monitor paused successfully." }); | ||
| }else{ | ||
| createToast({ body: "Monitor resumed successfully." }); | ||
| } |
There was a problem hiding this comment.
The isActive property is actually a property of action.payload.data so this currently doesn't work and always shows the resumed toast.
I think a cleaner approach here might be this:
const state =
action?.payload?.data.isActive === false ? "paused" : "resumed";
createToast({ body: `Monitor ${state} successfully.` });
Now we don't need the if/else statement at all and only need to call createToast in one place.
| createToast({ body: "Monitor resumed successfully." }); | ||
| } | ||
| } else { | ||
| throw new Error(action.error.message); |
There was a problem hiding this comment.
It's a good idea to safely access nested properties:
throw new Error(action?.error?.message ?? "Failed to pause monitor.");
action isn't guaranteed to have error defined, so we shoudl fail gracefully if it isn't
| throw new Error(action.error.message); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error pausing monitor:", monitor._id, error); |
There was a problem hiding this comment.
We have a utility Logger that we should use for logging:
import { logger } from "../../../Utils/Logger";
...
logger.error("Error pausing monitor:", monitor._id, error);
The logger was added mid-projecyt, so if you see console.log statements throughout the app we just haven't gotten around to replacing them yet.
| "Pause" | ||
| ) : ( | ||
| "Resume" | ||
| )} |
There was a problem hiding this comment.
The regular braces are not needed here:
{monitor?.isActive === true ? "Pause" : "Resume"}
is sufficient 👍
| @@ -44,6 +47,28 @@ const ActionsMenu = ({ monitor, isAdmin, updateCallback }) => { | |||
| } | |||
| }; | |||
|
|
|||
| const handlePause = async (event) => { | |||
There was a problem hiding this comment.
event parameter is unused, we can safely remove it
| ); | ||
| if (pauseUptimeMonitor.fulfilled.match(action)) { | ||
| updateCallback(); | ||
| //window.location.reload(); |
There was a problem hiding this comment.
We can remove this commented out code too
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Client/src/Pages/Monitors/Home/actionsMenu.jsx (2)
51-67: Pause function's droppin' beats!Yo, this handlePause function's got more flow than Eminem on 8 Mile! It's handlin' business like a boss, but I got a little somethin' to add:
- const state = action?.payload?.data.isActive === false ? "paused" : "resumed"; + const state = action?.payload?.data?.isActive === false ? "paused" : "resumed";Just addin' another question mark to make sure we don't trip over our own shoelaces if that data ain't there. Other than that, this function's tighter than mom's spaghetti grip!
180-190: Menu's got a new hit single!This new menu item's fresher than mom's spaghetti straight outta the pot! But check it, we can make it even smoother:
- {monitor?.isActive === true ? "Pause" : "Resume"} + {monitor?.isActive ? "Pause" : "Resume"}We don't need that
=== truepart, it's like puttin' extra parmesan on your spaghetti when it's already cheesy enough. But overall, this menu's servin' up some hot functionality!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Client/src/Pages/Monitors/Home/actionsMenu.jsx (7 hunks)
🧰 Additional context used
🔇 Additional comments (4)
Client/src/Pages/Monitors/Home/actionsMenu.jsx (4)
6-6: Yo, this logger import's on point!Aight, this new import's lookin' fresh. It's gonna help us keep track of them errors like mom's spaghetti on a sweater. Good call bringin' in the big guns for error loggin'.
18-18: Pause action comin' in hot!Yo, we just dropped that pauseUptimeMonitor import like it's hot. It's gonna let us freeze them monitors faster than mom can whip up some spaghetti. Solid move, homie!
31-31: Auth token's ready to roll!We're pullin' out that authToken like it's the last strand of spaghetti in the pot. It's gonna keep our pause action locked down tighter than mom's secret recipe. Nice work, dawg!
274-274: PropTypes got an upgrade!Yo, we just beefed up our PropTypes like we're addin' more meat to the spaghetti sauce! That isActive prop's gonna keep our type checkin' game strong. It's all about that consistency, you feel me?
|
Thank you for your cooperation and quick feedback. I really enjoyed working on this project. |
This change resolves #931 by adding pause option in the actions menu on the dashboard.