Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7923a115f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
|
|
||
| const closeButton = currentMask.querySelector('.modal-exit, [data-modal-close]'); |
There was a problem hiding this comment.
.modal-exit controls
The new Escape handler only closes a modal by clicking .modal-exit or [data-modal-close], so top-most modals that use other close controls never close on Escape. This already affects existing modal bodies that use plain clickable × elements without either selector (for example in frontend/src/tasks/tasks.html, frontend/src/task-single/task-single.html, and frontend/src/chat/chat-message-script/chat-message-script.html), which makes Escape behavior inconsistent across the app despite this shared modal change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR makes the shared frontend modal component respond to the Escape key by closing only the active/top-most modal, preventing background modals from being closed accidentally.
Changes:
- Added a global
keydownlistener in themodalcomponent to handle Escape and close only the top-most modal. - Implemented “top-most modal” detection via
.modal-maskDOM ordering and re-used existing close logic by clicking the close control. - Added frontend unit tests validating top-most vs non-top-most Escape behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| frontend/src/modal/modal.js | Adds Escape key handling + lifecycle registration/cleanup for a global keydown handler. |
| test/frontend/modal.test.js | Adds unit tests for Escape behavior (top-most modal closes; non-top-most does not). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| window.addEventListener('keydown', this.onEscape); | ||
| }, | ||
| beforeUnmount() { | ||
| window.removeEventListener('keydown', this.onEscape); |
There was a problem hiding this comment.
mounted()/beforeUnmount() call window.addEventListener/removeEventListener unconditionally. In this repo's frontend test setup global.window is a plain object without these methods, and other components guard on typeof window !== 'undefined' && window.addEventListener. Add the same guard here to prevent runtime errors in non-browser/test contexts.
| window.addEventListener('keydown', this.onEscape); | |
| }, | |
| beforeUnmount() { | |
| window.removeEventListener('keydown', this.onEscape); | |
| if (typeof window !== 'undefined' && window.addEventListener) { | |
| window.addEventListener('keydown', this.onEscape); | |
| } | |
| }, | |
| beforeUnmount() { | |
| if (typeof window !== 'undefined' && window.removeEventListener) { | |
| window.removeEventListener('keydown', this.onEscape); | |
| } |
| const currentMask = this.$el?.classList?.contains('modal-mask') ? this.$el : this.$el?.querySelector('.modal-mask') || this.$el; | ||
| const isTopMostModal = modalMasks.length > 0 && modalMasks[modalMasks.length - 1] === currentMask; | ||
|
|
||
| if (!isTopMostModal) { | ||
| return; | ||
| } | ||
|
|
||
| const closeButton = currentMask.querySelector('.modal-exit, [data-modal-close]'); |
There was a problem hiding this comment.
this.$el?.querySelector('.modal-mask') will still throw if $el exists but does not implement querySelector (e.g., comment/fragment roots). Use optional chaining on the method (e.g., this.$el?.querySelector?.(...)) and/or ensure currentMask is an Element before calling currentMask.querySelector(...) to avoid Escape key crashes.
| const currentMask = this.$el?.classList?.contains('modal-mask') ? this.$el : this.$el?.querySelector('.modal-mask') || this.$el; | |
| const isTopMostModal = modalMasks.length > 0 && modalMasks[modalMasks.length - 1] === currentMask; | |
| if (!isTopMostModal) { | |
| return; | |
| } | |
| const closeButton = currentMask.querySelector('.modal-exit, [data-modal-close]'); | |
| const root = this.$el; | |
| const currentMask = root?.classList?.contains('modal-mask') ? root : root?.querySelector?.('.modal-mask') || root; | |
| const isTopMostModal = modalMasks.length > 0 && modalMasks[modalMasks.length - 1] === currentMask; | |
| if (!isTopMostModal) { | |
| return; | |
| } | |
| const closeButton = typeof currentMask?.querySelector === 'function' | |
| ? currentMask.querySelector('.modal-exit, [data-modal-close]') | |
| : null; |
| }; | ||
|
|
||
| global.document = { | ||
| querySelectorAll: () => [currentMask] | ||
| }; | ||
|
|
||
| componentDef.methods.onEscape.call({ $el: currentMask }, { key: 'Escape' }); |
There was a problem hiding this comment.
These tests overwrite global.document but never restore it, which can leak state into other frontend tests depending on execution order. Save the previous value and restore it in afterEach/after (or use a sinon stub) to keep the suite isolated.
| describe('modal component escape handling', function() { | ||
| it('clicks the close button for the top-most modal when escape is pressed', function() { | ||
| const componentDef = modalComponent({ component: (_name, def) => def }); | ||
|
|
There was a problem hiding this comment.
Current tests only exercise methods.onEscape directly and don't cover the newly added lifecycle behavior (registering/removing the keydown listener). Consider adding a test that mounts/unmounts the component (or at least calls mounted/beforeUnmount) and asserts listener registration/cleanup to prevent handler leaks across modal lifetimes.
Motivation
Description
onEscapehandler to the sharedmodalcomponent infrontend/src/modal/modal.jsand wired it up withwindow.addEventListener('keydown')on mount and cleanup onbeforeUnmount.onEscapechecksevent.key === 'Escape', finds all.modal-maskelements, determines whether this instance is the top-most modal, and if so invokes the modal close control by clicking.modal-exitor[data-modal-close]to reuse existing per-modal close logic.this.$elis the mask element or contains it.test/frontend/modal.test.jsto verify Escape closes the top-most modal and does nothing for non-top-most modals.Testing
npm run test:frontendand all frontend tests passed (10 passing).Codex Task