fix: prevent Advanced Matching checkbox from auto-unchecking#3094
fix: prevent Advanced Matching checkbox from auto-unchecking#3094khoazero123 wants to merge 1 commit intobluewave-labs:developfrom
Conversation
khoazero123
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Advanced Matching Auto-Enable Logic client/src/Pages/v1/Uptime/Create/index.jsx |
Updated onChange handler to automatically enable Advanced Matching when expectedValue or jsonPath fields receive non-empty values. Modified useEffect to check for existing values across matchMethod, expectedValue, and jsonPath fields. Narrowed dependency array to only watch monitor.matchMethod, monitor.expectedValue, monitor.jsonPath, isCreate, and isClone to prevent unintended state resets during editing. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
🐰 A checkbox that danced and then fled the scene,
But now stays put with our logic so clean—
When values are typed, Advanced Matching springs true,
No more unchecking as you type anew!
The dependency watch now runs lean and bright,
Uptime monitors configured just right! ✨
Pre-merge checks and finishing touches
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and directly describes the main fix: preventing the Advanced Matching checkbox from auto-unchecking, which is the core issue being resolved. |
| Description check | ✅ Passed | The PR description includes the issue reference (#3093) and confirms testing, but lacks detailed explanation of the changes made and does not provide UI screenshots despite UI changes. |
| Linked Issues check | ✅ Passed | The changes directly address all proposed solutions from issue #3093: narrowed useEffect dependency array to specific fields, updated condition to check all three fields, and added onChange logic to auto-enable Advanced Matching. |
| Out of Scope Changes check | ✅ Passed | All changes are focused on the single file client/src/Pages/v1/Uptime/Create/index.jsx and directly address the Advanced Matching checkbox auto-uncheck issue without introducing unrelated modifications. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/Pages/v1/Uptime/Create/index.jsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jesulayomy
Repo: bluewave-labs/Checkmate PR: 2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the `formatAndSet` function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using `const displayInterval = monitor?.interval / MS_PER_MINUTE || 1;` and converts user input back to milliseconds using `value = value * MS_PER_MINUTE` in the onChange handler.
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
Repo: bluewave-labs/Checkmate PR: 2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the `formatAndSet` function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using `const displayInterval = monitor?.interval / MS_PER_MINUTE || 1;` and converts user input back to milliseconds using `value = value * MS_PER_MINUTE` in the onChange handler.
Applied to files:
client/src/Pages/v1/Uptime/Create/index.jsx
🔇 Additional comments (1)
client/src/Pages/v1/Uptime/Create/index.jsx (1)
264-271: Good implementation of auto-checking on user input.The automatic activation of the Advanced Matching checkbox when users type in
expectedValueorjsonPathdirectly addresses the reported issue. Thevalue.trim()check properly prevents whitespace-only input from triggering the checkbox.
| useEffect(() => { | ||
| if (!isCreate || isClone) { | ||
| if (monitor.matchMethod) { | ||
| setUseAdvancedMatching(true); | ||
| } else { | ||
| setUseAdvancedMatching(false); | ||
| } | ||
| // Automatically check "Advanced matching" checkbox if any value exists in matchMethod, expectedValue, or jsonPath | ||
| if (monitor.matchMethod || monitor.expectedValue || monitor.jsonPath) { | ||
| setUseAdvancedMatching(true); | ||
| } else if (!isCreate || isClone) { | ||
| // Only uncheck when editing/cloning and no values exist | ||
| setUseAdvancedMatching(false); | ||
| } | ||
| }, [monitor, isCreate]); | ||
| }, [monitor.matchMethod, monitor.expectedValue, monitor.jsonPath, isCreate, isClone]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and check the mentioned lines
wc -l client/src/Pages/v1/Uptime/Create/index.jsxRepository: bluewave-labs/Checkmate
Length of output: 114
🏁 Script executed:
# Check the initial state and default values around lines 60 and 71
sed -n '55,75p' client/src/Pages/v1/Uptime/Create/index.jsxRepository: bluewave-labs/Checkmate
Length of output: 754
🏁 Script executed:
# Check the useEffect implementation at lines 305-313
sed -n '300,320p' client/src/Pages/v1/Uptime/Create/index.jsxRepository: bluewave-labs/Checkmate
Length of output: 896
🏁 Script executed:
# Search for matchMethod default behavior across the codebase
rg -n 'matchMethod.*equal' --type=js --type=jsx -A2 -B2Repository: bluewave-labs/Checkmate
Length of output: 94
🏁 Script executed:
# Let's search for matchMethod without specifying file type
rg -n 'matchMethod.*equal' -A2 -B2Repository: bluewave-labs/Checkmate
Length of output: 3867
🏁 Script executed:
# Also check how the monitor state is loaded when editing/cloning
rg -n 'useEffect.*monitor' client/src/Pages/v1/Uptime/Create/index.jsx | head -20Repository: bluewave-labs/Checkmate
Length of output: 49
🏁 Script executed:
# Check if there are any comments or documentation about the intended behavior of Advanced Matching
rg -n 'Advanced.*[Mm]atching|advanced.*matching' client/src/Pages/v1/Uptime/Create/index.jsxRepository: bluewave-labs/Checkmate
Length of output: 798
🏁 Script executed:
# Check if there are any other related useEffects or initialization logic that might clarify intent
sed -n '260,275p' client/src/Pages/v1/Uptime/Create/index.jsxRepository: bluewave-labs/Checkmate
Length of output: 520
🏁 Script executed:
# Check if there's any handling to distinguish between "default matchMethod" and "user-selected matchMethod"
rg -n 'useAdvancedMatching\|Advanced' client/src/Pages/v1/Uptime/Create/index.jsx | grep -A3 -B3 'useEffect\|onChange'Repository: bluewave-labs/Checkmate
Length of output: 49
🏁 Script executed:
# Check if there's any specific handling when loading existing monitors (edit/clone scenarios)
rg -n 'setMonitor\|fetchMonitor' client/src/Pages/v1/Uptime/Create/index.jsx | head -15Repository: bluewave-labs/Checkmate
Length of output: 49
🏁 Script executed:
# Look for any comments about the intended behavior or design decisions
sed -n '1,100p' client/src/Pages/v1/Uptime/Create/index.jsx | grep -A5 -B5 'Advanced\|matchMethod'Repository: bluewave-labs/Checkmate
Length of output: 758
🏁 Script executed:
# Check the complete context around the useEffect to understand the full logic flow
sed -n '295,320p' client/src/Pages/v1/Uptime/Create/index.jsxRepository: bluewave-labs/Checkmate
Length of output: 1025
🏁 Script executed:
# Look for where monitors are loaded from the server (to understand edit/clone scenarios)
rg -n 'useParams\|useQuery\|id.*monitor' client/src/Pages/v1/Uptime/Create/index.jsx | head -10Repository: bluewave-labs/Checkmate
Length of output: 49
Confirm the logic issue with Advanced Matching auto-check for new monitors.
The concern identified in the original review is valid. The useEffect condition at line 307 will always be truthy for new monitors because matchMethod defaults to "equal". Since expectedValue and jsonPath default to empty strings (falsy), only matchMethod being truthy triggers the auto-check.
For new monitor creation (where isCreate=true and isClone=false), the condition !isCreate || isClone evaluates to false, preventing the uncheck logic from running. This means the Advanced Matching checkbox will be auto-checked and advanced fields displayed immediately on mount for all new monitors, regardless of user intent.
The current logic conflates "default matchMethod value" with "user-intentionally-set advanced matching parameters." If Advanced Matching should be opt-in for new monitors, the condition needs to distinguish between default and user-entered values, or only auto-check when expectedValue or jsonPath are non-empty.
🤖 Prompt for AI Agents
In client/src/Pages/v1/Uptime/Create/index.jsx around lines 305-313, the
useEffect currently auto-checks Advanced Matching because monitor.matchMethod
defaults to "equal"; change the condition so Advanced Matching is only
auto-enabled when the user has provided actual advanced values
(monitor.expectedValue or monitor.jsonPath) or when an explicit "user-set" flag
for matchMethod exists; specifically, remove matchMethod from the truthy check
(or add a check for a user-set indicator) so only expectedValue || jsonPath true
will setUseAdvancedMatching(true), and keep the existing uncheck branch for
edits/clones.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions
This PR effectively fixes the Advanced Matching checkbox auto-unchecking issue but introduces minor inconsistencies and potential robustness concerns in the logic.
🌟 Strengths
- Correctly resolves the core bug by updating useEffect dependencies and adding onChange logic for immediate feedback.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P2 | client/.../index.jsx | Bug | Excludes matchMethod from auto-check, causing user confusion. | |
| P2 | client/.../index.jsx | Maintainability | Potential performance issues from redundant state updates. | |
| P2 | client/.../index.jsx | Bug | Brittle string trimming could break with non-string values. | |
| P2 | client/.../index.jsx | Bug | Unclear logic for editing/cloning checkbox state. |
🔍 Notable Themes
- UI State Synchronization: Multiple findings highlight the need for consistent handling of advanced matching fields to ensure seamless user experience.
- Defensive Programming: Issues around type assumptions and logic clarity suggest opportunities for more robust code practices.
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| setMonitor((prev) => { | ||
| const updatedMonitor = { ...prev, [name]: value }; | ||
| // Automatically check "Advanced matching" checkbox if user enters expectedValue or jsonPath | ||
| if ((name === "expectedValue" || name === "jsonPath") && value && value.trim()) { | ||
| setUseAdvancedMatching(true); | ||
| } | ||
| return updatedMonitor; | ||
| }); |
There was a problem hiding this comment.
P2 | Confidence: High
Speculative: The logic in the onChange handler only automatically checks the Advanced Matching checkbox when the user types in expectedValue or jsonPath fields, but notably excludes the matchMethod field. This creates an inconsistent user experience where:
- A user could select a
matchMethod(e.g., "JSON_PATH", "REGEX") from a dropdown without the checkbox being automatically checked. - This scenario is plausible given the UI likely includes a dropdown/select for
matchMethod. - The user might expect advanced matching to be active when they've selected a method, leading to confusion if the checkbox remains unchecked despite having a valid
matchMethod. - This inconsistency contradicts the
useEffectlogic which correctly checks for any of the three fields (matchMethod,expectedValue,jsonPath), creating a potential behavioral mismatch between immediate UI feedback (onChange) and derived state (useEffect).
| setMonitor((prev) => { | |
| const updatedMonitor = { ...prev, [name]: value }; | |
| // Automatically check "Advanced matching" checkbox if user enters expectedValue or jsonPath | |
| if ((name === "expectedValue" || name === "jsonPath") && value && value.trim()) { | |
| setUseAdvancedMatching(true); | |
| } | |
| return updatedMonitor; | |
| }); | |
| setMonitor((prev) => { | |
| const updatedMonitor = { ...prev, [name]: value }; | |
| // Automatically check "Advanced matching" checkbox if user enters matchMethod, expectedValue, or jsonPath | |
| if ((name === "matchMethod" || name === "expectedValue" || name === "jsonPath") && value && (typeof value === 'string' ? value.trim() : true)) { | |
| setUseAdvancedMatching(true); | |
| } | |
| return updatedMonitor; | |
| }); |
| useEffect(() => { | ||
| if (!isCreate || isClone) { | ||
| if (monitor.matchMethod) { | ||
| setUseAdvancedMatching(true); | ||
| } else { | ||
| setUseAdvancedMatching(false); | ||
| } | ||
| // Automatically check "Advanced matching" checkbox if any value exists in matchMethod, expectedValue, or jsonPath | ||
| if (monitor.matchMethod || monitor.expectedValue || monitor.jsonPath) { | ||
| setUseAdvancedMatching(true); | ||
| } else if (!isCreate || isClone) { | ||
| // Only uncheck when editing/cloning and no values exist | ||
| setUseAdvancedMatching(false); | ||
| } | ||
| }, [monitor, isCreate]); | ||
| }, [monitor.matchMethod, monitor.expectedValue, monitor.jsonPath, isCreate, isClone]); |
There was a problem hiding this comment.
P2 | Confidence: High
Speculative: The useEffect correctly handles the derived state based on field presence, but its dependency array may cause unnecessary re-renders. The logic conditionally calls setUseAdvancedMatching with the same value (true or false) on every dependency change, even when the resulting state wouldn't change (e.g., when useAdvancedMatching is already true and fields remain present). This could contribute to subtle performance issues in larger forms or with frequent typing. While React optimizes with batching, unnecessary state updates should be minimized for clarity and to prevent potential side-effects in more complex components.
Code Suggestion:
useEffect(() => {
const hasAdvancedFields = !!(monitor.matchMethod || monitor.expectedValue || monitor.jsonPath);
const shouldBeChecked = hasAdvancedFields || (!isCreate || isClone ? false : useAdvancedMatching);
// Only update state if it would change
if (shouldBeChecked !== useAdvancedMatching) {
setUseAdvancedMatching(shouldBeChecked);
}
}, [monitor.matchMethod, monitor.expectedValue, monitor.jsonPath, isCreate, isClone, useAdvancedMatching]);
| if ((name === "expectedValue" || name === "jsonPath") && value && value.trim()) { | ||
| setUseAdvancedMatching(true); | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: Medium
Speculative: The validation value.trim() assumes value is always a string, which may not hold true for all form fields. If the codebase evolves and expectedValue or jsonPath could receive non-string values (e.g., numbers, arrays, null), calling .trim() would throw a runtime error (TypeError: value.trim is not a function). While the current context suggests these fields are text inputs, this assumption creates a brittle pattern that could break during future refactoring or if field types change. Defensive programming with type checking would improve robustness.
| if ((name === "expectedValue" || name === "jsonPath") && value && value.trim()) { | |
| setUseAdvancedMatching(true); | |
| } | |
| if ((name === "expectedValue" || name === "jsonPath") && value && typeof value === 'string' && value.trim()) { | |
| setUseAdvancedMatching(true); | |
| } |
| } else if (!isCreate || isClone) { | ||
| // Only uncheck when editing/cloning and no values exist | ||
| setUseAdvancedMatching(false); | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: Medium
Speculative: The condition (!isCreate || isClone) has ambiguous logic. When isCreate is true (creating a new monitor) and isClone is false, the expression evaluates to (!true || false) = (false || false) = false, so the checkbox won't be unchecked—this seems correct for creation mode. However, when isCreate is false (editing) and isClone is true (cloning), the expression is (!false || true) = (true || true) = true, causing the checkbox to uncheck if no advanced fields exist. This might be intentional for cloning behavior, but the comment "Only uncheck when editing/cloning" doesn't clarify the nuance. The logic should be explicitly validated against the intended UX: Should a cloned monitor with no advanced fields start with the checkbox unchecked? If yes, the logic is correct but the comment should be more precise.
Code Suggestion:
} else if (!isCreate || isClone) {
// Uncheck when editing (isCreate=false) OR when cloning (isClone=true), if no advanced fields exist
setUseAdvancedMatching(false);
}
|
Closing as the repo went through major restructuring. |
2 participants
Describe your changes
Briefly describe the changes you made and their purpose.
Fixes #3093
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.