fix(web): expose manual edit undo/redo and save confirmation#3034
fix(web): expose manual edit undo/redo and save confirmation#3034leno23 wants to merge 1 commit into
Conversation
…#2866) Wire the existing history callbacks into the edit panel with Undo/Redo buttons, keyboard shortcuts, and a transient Saved status after writes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8281a87753
ℹ️ 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".
| const mod = event.metaKey || event.ctrlKey; | ||
| if (!mod || event.altKey) return; | ||
| if (event.key.toLowerCase() === 'z' && !event.shiftKey && canUndo) { | ||
| event.preventDefault(); | ||
| onUndo(); |
There was a problem hiding this comment.
Skip document undo hotkeys when focus is in editable fields
The new global keydown listener handles Cmd/Ctrl+Z without checking whether the focused element is an input, textarea, or contentEditable, so once canUndo is true, pressing undo while typing in any manual-edit form control triggers onUndo() and writes an older full document to disk instead of undoing text in that field. This is a destructive behavior change for normal editing workflows; the shortcut handler should bail out for editable targets (and typically for already-handled events).
Useful? React with 👍 / 👎.
lefarcen
left a comment
There was a problem hiding this comment.
Hi @leno23! Thanks for tying #2866 to the manual-edit history path — the Summary and Validation make the intent clear. One PR-body thing before pool review: could you update ## Surface area to use the checklist and tick UI? This touches visible ManualEditPanel controls/status, so the checklist helps reviewers and release verification scope it quickly.
Related: #2925 by @xxiaoxiong is also open in this area and overlaps on FileViewer.tsx, ManualEditPanel.tsx, and memory.css for #2905 save-confirmation feedback. You two may want to compare approaches; the maintainer team will pick what lands.
lefarcen
left a comment
There was a problem hiding this comment.
Hi @leno23! Thanks for tying #2866 to the manual-edit history path — the Summary and Validation make the intent clear. One PR-body thing before pool review: could you update ## Surface area to use the checklist and tick UI? This touches visible ManualEditPanel controls/status, so the checklist helps reviewers and release verification scope it quickly.
Related: #2925 by @xxiaoxiong is also open in this area and overlaps on FileViewer.tsx, ManualEditPanel.tsx, and memory.css for #2905 save-confirmation feedback. You two may want to compare approaches; the maintainer team will pick what lands.
lefarcen
left a comment
There was a problem hiding this comment.
Hi @leno23! Thanks for tying #2866 to the manual-edit history path — the Summary and Validation make the intent clear. One PR-body thing before pool review: could you update ## Surface area to use the checklist and tick UI? This touches visible ManualEditPanel controls/status, so the checklist helps reviewers and release verification scope it quickly.
Related: #2925 by @xxiaoxiong is also open in this area and overlaps on FileViewer.tsx, ManualEditPanel.tsx, and memory.css for #2905 save-confirmation feedback. You two may want to compare approaches; the maintainer team will pick what lands.
PerishCode
left a comment
There was a problem hiding this comment.
@leno23 — thanks for closing the loop on #2866 with the undo/redo controls, keyboard shortcuts, and the transient Saved ✓ confirmation in ManualEditPanel. The tests cover both the panel-level wiring and the FileViewer history regression (delete → undo restores the element), which is exactly the right shape for a bugfix PR.
One non-blocking inline finding (i18n consistency for the new status label).
Unrelated to this review's inline finding, the existing unresolved Codex thread on the global keydown listener is worth addressing before merge: ManualEditPanel contains several <input> elements (UnitRow, ColorRow, QuadCell), so Cmd/Ctrl+Z while focus is in one of those inputs will now hit the window-level handler instead of native input undo, overwriting the file with the previous persisted snapshot. A guard such as if (event.target instanceof HTMLInputElement || event.target instanceof HTMLTextAreaElement || (event.target as HTMLElement)?.isContentEditable) return; would resolve it.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
| {busy ? ( | ||
| <em className="manual-edit-status" data-testid="manual-edit-status"> | ||
| Saving… | ||
| </em> | ||
| ) : showSaved ? ( | ||
| <em className="manual-edit-status" data-testid="manual-edit-status"> | ||
| Saved ✓ | ||
| </em> | ||
| ) : null} |
There was a problem hiding this comment.
The two new status labels are hardcoded English: Saving… (line 130) and Saved ✓ (line 134). The surrounding titlebar text was migrated to t('manualEdit.title') in this same PR, and the new undo/redo buttons below use t('manualEdit.undo') / t('manualEdit.redo'), so a non-English locale (e.g. zh-CN, ja, fr) will render this titlebar as a mix of localized + raw English, which is jarring and breaks the i18n convention the rest of this PR follows.
Why it matters: per the root AGENTS.md i18n keys section, user-visible strings must be defined as typed keys in apps/web/src/i18n/types.ts and populated for all 18 locale files. The change to t('manualEdit.title') clearly intends to honor that contract; the new status strings sidestep it for the same surface in the same patch.
Suggested change: add two new keys to apps/web/src/i18n/types.ts (for example 'manualEdit.savingStatus': string and 'manualEdit.savedStatus': string), populate them in every file under apps/web/src/i18n/locales/ (the English values can stay Saving… / Saved ✓ — translators can localize later), and replace these two literals with t('manualEdit.savingStatus') and t('manualEdit.savedStatus'). Keep the existing <em className="manual-edit-status" data-testid="manual-edit-status"> wrapper and the visual treatment unchanged so the test selectors still match.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
|
Heads-up: PR #3055 is also open in this manual-edit feedback area. Both PRs touch You and @DonQuilatte may want to coordinate so effort does not get duplicated; maintainers will pick the final shape. |
















































Fixes #2866
Also helps #2905 — the edit panel now shows transient Saved ✓ / Saving… feedback after persisted manual edits (including deletes), even though the legacy HTML tab UI is no longer present.
Summary
Manual edit history (
undoManualEdit/redoManualEdit) already worked inFileViewer, butManualEditPanelnever rendered controls or keyboard shortcuts, so deletes felt irreversible.Surface area
apps/web/src/components/ManualEditPanel.tsx— Undo/Redo buttons, Cmd/Ctrl+Z shortcuts, saved status line.apps/web/src/components/FileViewer.tsx— trackmanualEditSavedAtafter successful writes/undo/redo.apps/web/src/styles/viewer/memory.css— titlebar status styling.ManualEditPanel.test.tsxandFileViewer.manual-edit-history.test.tsx.Validation
cd apps/web && pnpm test tests/components/ManualEditPanel.test.tsx tests/components/FileViewer.manual-edit-history.test.tsx— 27 passed.Made with Cursor