Harden board dialogs against focus and live-update clobber - #2397
Harden board dialogs against focus and live-update clobber#2397Chris0Jeky wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 3206753938
ℹ️ 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".
| if (!wasOpen || seededKey !== sourceKey || !options.isDirty()) { | ||
| options.seed(source) |
There was a problem hiding this comment.
When a same-board refresh arrives after the user edits only one field, this form-wide isDirty() gate skips seed() for every field. In PaperBoardSettingsDialog.save and BoardSettingsModal.handleSave, an untouched description consequently remains at its old value; because Save compares it with the current props.board.description, it is sent as a change and silently overwrites the collaborator's newer description. The column dialogs can similarly resubmit an obsolete WIP limit. Preserve dirty fields individually while adopting fresh untouched fields, or construct the payload from per-field dirty state.
Useful? React with 👍 / 👎.
| const isActivationControl = Boolean(target?.closest( | ||
| 'button, a[href], [role="button"], [role="menuitem"], [role="option"], [role="tab"]', | ||
| )) | ||
|
|
||
| if (isTyping && event.key !== 'Escape') { | ||
| if ( | ||
| (isTextEntry && event.key !== 'Escape') || | ||
| (isActivationControl && (event.key === 'Enter' || event.key === ' ')) |
There was a problem hiding this comment.
When a TdDialog is opened from the Legacy CardModal, the focus manager deliberately focuses the .td-dialog container itself, which does not match this activation-control selector. Because Legacy does not set paperDialogOpen, pressing Enter on that initial focus still reaches BoardView's global Enter binding and calls openSelectedCard behind a discard/delete confirmation; when the card was opened by pointer and the board selection is empty, this can programmatically open another card modal. Suppress board activation whenever an active dialog owns the keyboard, rather than only when the event target is an interactive control.
Useful? React with 👍 / 👎.
|
Final-base review at 8184d9a found a confirmed HIGH after the single fix round:
The shared button/drag reorder lock was independently checked and is sound. The visualViewport/dvh MEDIUM remains tracked on #2180. Disposition: parked, not merged. This is the second/final review round, so no further fix cycle is being started during wrap-up. Exact resume point: start from this pushed head and replace form-wide reseed suppression with per-field seed/draft reconciliation: a locally dirty field keeps the user edit, an untouched field adopts the latest live value, and save must not emit a stale untouched value. Add one-dirty/one-remote-changed regressions for Paper and Legacy board and column dialogs, then rerun the focused suite, lint, typecheck/build, current-base integration, and one bounded final judgment. |
Summary
TdDialogThis reduces board-maintenance friction by preventing dialogs from moving or discarding a user's in-progress edit while preserving the existing explicit-save, review-first interaction model.
Implementation notes
useDialogFocusManagement, shared byTdDialogandPaperBoardDialogShelluseRealtimeSafeDialogDraft, which compares the live seed against the last seed snapshot instead of treating a new object reference as a new draftaria-currentExplicitly retained behavior
The archive navigate/close-before-delete ordering is deliberately retained. #519 established that unmounting before store teardown prevents a load-bearing board-view freeze;
boardStoreremains the owner of the visible failure toast after navigation. Changing that ordering here would reintroduce the older failure mode rather than resolve #1975.Tests
Added or extended regressions for:
TdDialogbidirectional Tab trapping and focus lifecycleReview disposition
d7278525fdvhcompatibility expansion from this bounded fix round; it remains tracked on [CI][Nightly] mobile-safari E2E red since 2026-08-24: Edit Card dialog lands 8 px above the contracted visual viewport #2180Verification
At refreshed head
8184d9a193b669a4ee44f8e80a493f1c8fe47213, based on currentorigin/mainfe3a120a49e3c1e4737e802629f838d5eb4394af:npx vitest --run --maxWorkers=2 src/tests/i18n/catalogs.spec.ts src/tests/i18n/lazyLocales.spec.ts src/tests/views/paper/PaperBoardDialogShell.spec.ts src/tests/components/ui/TdDialog.spec.ts src/tests/composables/useKeyboardShortcuts.spec.ts src/tests/components/ColumnEditModal.spec.ts src/tests/components/BoardSettingsModal.spec.ts src/tests/views/paper/PaperBoardManagement.spec.ts src/tests/views/BoardView.keyboardRouting.spec.ts src/tests/views/paper/boardMutationCapabilityParity.spec.ts— 10 files / 136 tests passednpx eslint --max-warnings=0 ...— passednpm run build— typecheck and production build passedgit diff --check origin/main..HEAD— passedBefore the localized review fix and non-overlapping base merge,
npx vitest --run --maxWorkers=2passed all 358 files / 5,339 tests at implementation head320675393.Docs and residual verification
STATUS, masterplan, and testing-guide docs were intentionally not edited; the issue coordinator owns batch doc synchronizationCloses #1975