fix(manual-edit): confirm element deletion#6070
Open
roian6 wants to merge 1 commit into
Open
Conversation
Contributor
|
🧪 This PR has changes that need a manual QA pass before merge — please hold off self-merging for now; we'll loop QA in once it's merge-ready (and design/product have signed off, where applicable). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3846
Why
The Manual Edit inspector currently deletes the selected element on the first click of the trash icon. That makes an easy-to-trigger destructive action immediate even though the existing translations and footer styles already describe a confirmation step.
This fixes the reported current-
mainpath without changing the underlying undoableremove-elementpatch contract.What users will see
Clicking the trash icon now replaces the normal footer actions with an inline warning plus Delete element and Cancel actions. The element is only removed after the explicit confirmation.
The pending confirmation is cancelled when the selected target changes. Keyboard focus moves to the destructive confirmation action and returns to the trash icon when cancellation keeps the same target selected.
Surface area
apps/weborapps/desktop(including Electron menu bar)odsubcommand or flag, newtools-dev/tools-pack/tools-prflag, or newOD_*env var/api/*endpoint, new SSE event, or changed shape inpackages/contractsskills/,design-systems/,design-templates/, orcraft/, or change to the skills protocolTRANSLATIONS.mdfor the locale workflow)package.json(dependenciesordevDependencies); workspace-packagepackage.jsonfiles are out of scope. Include a paragraph on what we get vs. what bytes we ship (seeCONTRIBUTING.md→ Code style)Screenshots
Bug fix verification
apps/web/tests/components/ManualEditPanel.test.tsxmainand green on this branch? Yes. The red test observedonApplyPatchbeing called by the first trash-icon click; it is now called only after confirmation.apps/web/tests/components/FileViewer.manual-edit.test.tsxnow verifies that the existing “cannot remove the final rendered root” guard is reached only after the second, confirming click.Validation
pnpm guard— passedpnpm typecheck— passed across the workspacepnpm --filter @open-design/web build— passedpnpm --filter @open-design/web exec vitest run -c vitest.config.ts tests/components/ManualEditPanel.test.tsx tests/components/FileViewer.manual-edit.test.tsx --maxWorkers=1— 40 passedApp.connectors.test.tsxprivacy-banner sync assertion. The same file passed in a cleanorigin/mainworktree and also passed once in this branch in isolation; it also reproduced intermittently in isolation without touching any files in this PR.Adjacent work
Open PR #5890 currently adds a separate direct-manipulation path (
od-edit-delete-request→removeManualEditTarget) that does not exist onmainand does not route throughManualEditPanel. It remains blocked with changes requested. If that PR lands, its overlay delete action should adopt the same explicit confirmation contract before merge so it does not reintroduce a bypass.