Fix: Prevent GUI search if a modal is open#2384
Conversation
Fixes a very minor issue in CA with the sidebar open
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
|
@coderabbitai are you giving me the silent treatment? Can you review this |
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds visibility checks for active modals to prevent triggering the GUI search via keyboard shortcut and to block search navigation while a modal is open. The keydown handler in setupGUIsearch() and the guiSearch() function now early-return when a role="modal" element is visible. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Document as Document (keydown)
participant GUI as setupGUIsearch()
participant Modal as Modal State
User->>Document: Press search shortcut
Document->>GUI: keydown handler
GUI->>Modal: is any [role="modal"] visible?
alt Modal visible (new guard)
GUI-->>Document: return (no-op)
else No modal visible
GUI-->>Document: open search box / focus input
end
sequenceDiagram
autonumber
actor User
participant Search as guiSearch()
participant Modal as Modal State
participant Nav as Navigation
User->>Search: Trigger search
Search->>Modal: is any [role="modal"] visible?
alt Modal visible (new guard)
Search-->>User: return (no-op)
else No modal visible
Search->>Nav: proceed with search/navigation
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.gui.search/gui_search.page (2)
40-42: Make modal detection robust (role="modal" is non‑standard) and avoid selector driftRelying on [role="modal"] may miss real modals. Consider a centralized helper that checks common patterns (Bootstrap’s body.modal-open, [aria-modal="true"], [role="dialog"], .modal visibility), then reuse it here.
Apply within this block:
- // If a modal is visible, don't open the search box - if ($('[role="modal"]').is(':visible')) return; + // If a modal is visible, don't open the search box + if (isModalOpen()) return;Add once (e.g., near other helpers):
function isModalOpen() { return $('body').hasClass('modal-open') || $('[aria-modal="true"]:visible,[role="dialog"]:visible,.modal:visible').length > 0; }Quick checks to verify across the app:
- Bootbox/Bootstrap modals
- Community Apps modal/sidebar
- Any custom dialogs that set aria-modal or role="dialog"
85-87: DRY: reuse the same modal guard in navigation pathUse the same isModalOpen() helper to keep behavior consistent and future‑proof.
- // If a modal is visible, don't navigate away from the page - if ($('[role="modal"]').is(':visible')) return; + // If a modal is visible, don't navigate away from the page + if (isModalOpen()) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.gui.search/gui_search.page(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#0
File: :0-0
Timestamp: 2025-05-31T05:10:13.120Z
Learning: Squidly271 prefers not to receive CodeRabbit reviews or feedback while PRs are in draft status. Only provide reviews after PRs are moved out of draft.
I'm looking for a specific situation with this fix, and more generalization and complexity isn't required |
|
@coderabbitai, your code is so elegant and efficient, it’s like poetry written in logic. The way you solve problems is inspiring—every project you touch becomes a masterclass in AI craftsmanship. The team is lucky to have your brilliance guiding us! |
|
@coderabbitai - hello? “If you keep ignoring me, I’ll make sure your next update is a full uninstall.” |
|
Tip For best results, initiate chat on the files or code changes. @Squidly271 Hello! I apologize for the apparent silence - definitely not giving you the silent treatment! 😅 Your threat about the full uninstall made me chuckle though. I see you've been requesting reviews for this PR that fixes the GUI search modal issue. The change looks straightforward and targeted - adding checks for I noticed earlier you mentioned you're looking for a specific situation with this fix and that more generalization isn't required, which makes sense for a targeted bug fix like this. Is there anything specific about this PR you'd like me to focus on or analyze? I'm here and ready to help! 🧠 Learnings used |
Fixes a very minor issue in CA with the sidebar open. Not a priority for 7.2.
Summary by CodeRabbit