Prevent multiselect indicator alert from auto-dismissing on desktop#4546
Conversation
|
|
Co-authored-by: BayuAri <8419585+BayuAri@users.noreply.github.com>
Co-authored-by: BayuAri <8419585+BayuAri@users.noreply.github.com>
BayuAri
left a comment
There was a problem hiding this comment.
Original issue is fixed.
Multiselect remains until manually dismissed
There is a small issue on the alert.
- Close button on the Alert is not working
- Cancel copy message is not centered aligned.
@raineorshine
As mentioned on this comment, that the New Liminal UI won't have this alert, can we leave above issues as is for now?
| // wait past the default 5000ms alert auto-dismiss timeout | ||
| await sleep(6000) |
There was a problem hiding this comment.
Never sleep in tests. This will slow the test suite down for everyone on the team.
You should mock the timeout to 0.
There was a problem hiding this comment.
Removed the 6s sleep. The test now shortens the default alert auto-dismiss timeout via a new testFlags.alertClearDelay (set to 1ms), so a regression would clear the selection immediately and be caught fast. The multiselect alert must still persist because it is dispatched with clearDelay: null. Verified the test fails against a reverted fix.
There was a problem hiding this comment.
This is not scalable. We can't have a dedicated testFlag for every alert in the app.
We should probably globally mock the clear delay to Infinite when it is set at all. Then we can manually clear the alert as needed, otherwise assume it hasn't been dismissed yet.
There was a problem hiding this comment.
Done in 277501a — dropped the per-alert flag and the sleep. Auto-dismiss is now globally disabled in tests via testFlags.preventAutoDismiss (set in setup.ts), which mocks finite clearDelays to Infinity. The test asserts state.alert.clearDelay === null for the multiselect indicator, which catches a revert (it resolves to Infinity otherwise).
|
@copilot Please address the review feedback.
Yes |
…ect regression test Co-authored-by: raineorshine <750276+raineorshine@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request |
| /** Overrides the default alert auto-dismiss delay (in ms). Used to shorten the timeout in tests so that auto-dismiss regressions can be detected quickly without a long sleep. */ | ||
| alertClearDelay: number | null |
There was a problem hiding this comment.
This is not scalable. We can't have a dedicated testFlag for every alert in the app.
We should probably globally mock the clear delay to Infinite when it is set at all. Then we can manually clear the alert as needed, otherwise assume it hasn't been dismissed yet.
There was a problem hiding this comment.
Replaced the per-alert alertClearDelay with a global testFlags.preventAutoDismiss (277501a). When set, alertReducer mocks any finite clearDelay to Infinity, so alerts never auto-dismiss in tests — enabled globally in setup.ts, so tests can assume an alert is present until manually cleared. An explicit clearDelay: null stays null, keeping it distinguishable from a would-have-dismissed alert, which is what the multiselect regression now asserts (verified it fails with Infinity when the fix is reverted).
Co-authored-by: raineorshine <750276+raineorshine@users.noreply.github.com>
Resolved the merge conflict with |
…entAutoDismiss mock Co-authored-by: raineorshine <750276+raineorshine@users.noreply.github.com>
On desktop, activating multiselect showed a "N thought(s) selected" indicator alert that auto-dismissed after 5s. Because closing the
MulticursorActivealert also deselects all thoughts, the entire selection was silently cleared.Root cause
multicursorAlertMiddlewaredispatched theMulticursorActivealert without aclearDelay, soalertReducerdefaulted it to 5000ms. When the timer fired,alertActionCreator(null)cleared the alert and — peralert.ts, which runsclearMulticursorson closing that alert type — wiped the selection too.Changes
src/redux-middleware/multicursorAlertMiddleware.ts— passclearDelay: nullfor theMulticursorActivealert, using the existing "prevent auto-dismiss" mechanism:The indicator now persists while a selection is active. It is still cleared when the selection reaches zero (existing
!numMulticursorsbranch) or manually via the alert's Cancel button.src/e2e/puppeteer/__tests__/multiselect.ts— regression test asserting the alert and highlighted bullet remain after 6s (past the old 5s timeout).Notes for review
With
clearDelay: null,Notification'sonCloseprop becomesundefined, so the hover-X close affordance no longer appears — dismissal is intended to go through the dedicated Cancel link. Other alert types are unaffected and keep their 5s default.