Restore backup - #119
Draft
tijmenbruggeman wants to merge 23 commits into
Draft
Conversation
Replace <form method="dialog"> with div + command/commandfor attributes, aligning with the Invoker Commands API. Cancel no longer auto-closes via form submission; both buttons now use `command="close"` explicitly. JS listener simplified: drop named handler + self-removal, check for `returnValue === 'submit'` instead of `'confirm'`.
Clearer dialog title/body split, added spinner, descriptive confirmation text, and fixed HTML nesting.
Contributor
|
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
|
✅ Action performedReview finished.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds original-image backup lifecycle methods, authenticated admin restoration, backup controls in compression details, cleanup on attachment deletion, and unit coverage for restoration and metadata behavior. ChangesImage backup restoration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdminUI
participant admin.js
participant restore_backup_image
participant Tiny_Image
participant WordPressFilesystem
AdminUI->>admin.js: Confirm backup restoration
admin.js->>restore_backup_image: POST tiny_restore_backup with attachment ID
restore_backup_image->>Tiny_Image: restore_backup()
Tiny_Image->>WordPressFilesystem: Copy backup over original
restore_backup_image-->>admin.js: Return compression details
admin.js-->>AdminUI: Replace media container
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/class-tiny-image.php`:
- Around line 720-772: Handle exceptions from Tiny_Helpers::get_wp_filesystem()
in all backup methods: create_backup(), get_backup(), restore_backup(), and
delete_backup(). Wrap each filesystem initialization in try/catch and return
false when initialization fails, preserving normal behavior when it succeeds so
media rendering and compression degrade gracefully.
- Around line 774-825: Update restore_backup() to reuse get_original_image() for
the restored original image instead of duplicating the
ORIGINAL_UNSCALED/ORIGINAL lookup. After wp_generate_attachment_metadata()
succeeds, rebuild $this->sizes from the new attachment metadata so
get_image_sizes() and get_image_size() immediately reflect regenerated, added,
removed, or renamed thumbnails; retain the existing metadata update and success
behavior.
In `@src/js/admin.js`:
- Around line 22-64: Move the restore workflow from the dialog’s cancel listener
to the confirm button’s click handler, so Escape only dismisses the dialog and
never calls restoreBackup. In the relevant admin.js handler, explicitly remove
any previously registered confirm handler before adding or assigning the new
one, rather than relying on { once: true }, so cancelling and reopening cannot
accumulate listeners. Update the confirm button markup to use type="button" and
remove command="request-close" and commandfor.
In `@src/views/compress-details-backup.php`:
- Around line 18-49: Fix the conditional markup around the backup view in the
template: remove the paragraph wrapper that contains the flow-content dialog,
ensure the no-backup branch emits no orphaned closing tag, and keep the dialog
as valid sibling markup. Also update the “original upload” translation call to
use esc_html_e() without echo, add the tiny-compress-images text domain to “View
uncompressed file,” and use esc_url() for the $backup href.
In `@src/views/compress-details.php`:
- Around line 116-121: Update the modal_title construction and Thickbox name
attribute around $modal_title to escape the translated title exactly once for
its HTML attribute context. Keep the filename unescaped while formatting the
translated string, then rely on esc_attr() at output without changing the
displayed title or surrounding link behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a9aa22b-2fe7-40ef-b27c-4d843c3c1e32
📒 Files selected for processing (9)
src/class-tiny-image.phpsrc/class-tiny-plugin.phpsrc/css/admin.csssrc/js/admin.jssrc/views/compress-details-backup.phpsrc/views/compress-details.phptest/helpers/wordpress.phptest/unit/TinyPluginBackupTest.phptest/wp-includes-for-tests/file.php
Comment on lines
+22
to
+64
| const dialog = document.getElementById(dialogID); | ||
| if (!dialog) { | ||
| return; | ||
| } | ||
|
|
||
| const attachmentId = trigger.data('id'); | ||
| const container = document.querySelector(`[data-tiny-media-id="${attachmentId}"]`); | ||
|
|
||
| dialog.showModal(); | ||
|
|
||
| dialog.addEventListener('cancel', async (e) => { | ||
| e.preventDefault(); | ||
| const spinner = dialog.querySelector('.spinner'); | ||
| try { | ||
| if (spinner) { | ||
| spinner.style.visibility = 'visible'; | ||
| } | ||
| const result = await restoreBackup(attachmentId); | ||
| dialog.close(); | ||
|
|
||
| // refresh thickbox | ||
| const modal = container.querySelector('.modal'); | ||
| const ajaxContent = document.getElementById('TB_ajaxContent'); | ||
| if (modal && ajaxContent) { | ||
| modal.append(...ajaxContent.children); | ||
| } | ||
|
|
||
| container.innerHTML = result; | ||
| if (typeof tb_remove === 'function') { | ||
| tb_remove(); | ||
| } | ||
| } catch (err) { | ||
| const errorEl = dialog.querySelector('.tiny-dialog-error'); | ||
| if (errorEl) { | ||
| errorEl.textContent = err.responseText || 'Failed to restore backup.'; | ||
| errorEl.hidden = false; | ||
| } | ||
| } finally { | ||
| if (spinner) { | ||
| spinner.style.visibility = 'hidden'; | ||
| } | ||
| } | ||
| }, { once: true }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Pressing Escape (or any dialog-close-request) triggers the destructive restore action.
The confirm ("Yes, Restore") button uses command="request-close". Per spec, that command fires a cancel event on the dialog — the same event that fires when the user presses the Escape key to dismiss the dialog. Since the only listener attached is on dialog's cancel event, a user pressing Escape to back out of the confirmation will inadvertently execute the restore (overwriting all compressed sizes), not cancel it.
Separately, a new one-time cancel listener is added on every click of "Restore Backup" (line 32), but it's never removed if the dialog is dismissed via the "Cancel" button (command="close", which does not fire cancel). Re-opening the same dialog (same DOM node, since only a successful restore replaces container.innerHTML) after a prior cancel therefore stacks multiple listeners — a subsequent confirm click would fire restoreBackup() once per accumulated listener.
Bind the confirm action to the button's own click instead of overloading the shared cancel event, and don't rely on { once: true } alone to prevent duplicate handlers.
🐛 Proposed fix
dialog.showModal();
- dialog.addEventListener('cancel', async (e) => {
- e.preventDefault();
- const spinner = dialog.querySelector('.spinner');
+ const confirmButton = dialog.querySelector('button[value="submit"]');
+ if (!confirmButton) {
+ return;
+ }
+
+ const onConfirm = async () => {
+ const spinner = dialog.querySelector('.spinner');
try {
if (spinner) {
spinner.style.visibility = 'visible';
}
const result = await restoreBackup(attachmentId);
dialog.close();
// refresh thickbox
const modal = container.querySelector('.modal');
const ajaxContent = document.getElementById('TB_ajaxContent');
if (modal && ajaxContent) {
modal.append(...ajaxContent.children);
}
container.innerHTML = result;
if (typeof tb_remove === 'function') {
tb_remove();
}
} catch (err) {
const errorEl = dialog.querySelector('.tiny-dialog-error');
if (errorEl) {
errorEl.textContent = err.responseText || 'Failed to restore backup.';
errorEl.hidden = false;
}
} finally {
if (spinner) {
spinner.style.visibility = 'hidden';
}
}
- }, { once: true });
+ };
+
+ confirmButton.addEventListener('click', onConfirm, { once: true });
});This also requires dropping command="request-close"/commandfor from the confirm button in src/views/compress-details-backup.php (lines 39-41) in favor of a plain type="button", so it no longer triggers the dialog's cancel event at all.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 48-48: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: container.innerHTML = result
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(inner-outer-html)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/js/admin.js` around lines 22 - 64, Move the restore workflow from the
dialog’s cancel listener to the confirm button’s click handler, so Escape only
dismisses the dialog and never calls restoreBackup. In the relevant admin.js
handler, explicitly remove any previously registered confirm handler before
adding or assigning the new one, rather than relying on { once: true }, so
cancelling and reopening cannot accumulate listeners. Update the confirm button
markup to use type="button" and remove command="request-close" and commandfor.
Comment on lines
+18
to
+49
| <?php if ( $backup ) { ?> | ||
| <p><?php echo esc_html_e( 'The original upload has been backed up.', 'tiny-compress-images' ); ?></p> | ||
|
|
||
| <p> | ||
| <a href="<?php echo esc_attr( $backup ); ?>" target="_blank"> | ||
| <?php esc_html_e( 'View uncompressed file' ); ?> | ||
| </a> | ||
| | ||
| <a class="button button-small" href="#" data-dialog-id="<?php echo esc_attr( $modal_id ); ?>" data-id="<?php echo absint( $tiny_image->get_id() ); ?>"> | ||
| <?php esc_html_e( 'Restore Backup', 'tiny-compress-images' ); ?> | ||
| </a> | ||
| <dialog id="<?php echo esc_attr( $modal_id ); ?>" class="tiny-dialog"> | ||
| <strong class="tiny-dialog-title"><?php esc_html_e( 'Are you sure you want to restore the original uncompressed image?', 'tiny-compress-images' ); ?></strong> | ||
| <p><?php esc_html_e( 'This action will replace all the compressed images with uncompressed images', 'tiny-compress-images' ); ?></p> | ||
| <p class="tiny-dialog-error" hidden></p> | ||
|
|
||
| <div class="tiny-dialog-actions"> | ||
| <span class="spinner"></span> | ||
| <button value="cancel" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="close" class="button"> | ||
| <?php esc_html_e( 'Cancel', 'tiny-compress-images' ); ?> | ||
| </button> | ||
| <button value="submit" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="request-close" class="button button-primary" autofocus> | ||
| <?php esc_html_e( 'Yes, Restore', 'tiny-compress-images' ); ?> | ||
| </button> | ||
| </div> | ||
| </dialog> | ||
| <?php } else { ?> | ||
| <span> | ||
| <?php esc_html_e( 'No backup available', 'tiny-compress-images' ); ?> | ||
| </span> | ||
| <?php } ?> | ||
| </p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unbalanced <p>/<dialog> markup in the "no backup" branch, plus a couple of WP-conventions nits.
- The
<p>opened at line 21 only exists inside theif ( $backup )branch, but its closing</p>at line 49 is emitted unconditionally after theif/else. When$backupis falsy, this leaves an orphaned</p>with no matching opener. - Separately, a
<dialog>element (flow content) is nested inside that<p>(which only permits phrasing content) — browsers will auto-close the<p>before the<dialog>, so the actual DOM won't match the source nesting. - Line 19:
echo esc_html_e(...)—esc_html_e()already echoes and returns nothing; wrapping it inechois redundant/incorrect usage. - Line 23:
esc_html_e( 'View uncompressed file' )is missing the'tiny-compress-images'text domain, so this string won't be picked up for translation. - Line 22:
esc_attr( $backup )for ahref— WordPress convention isesc_url()for URL attributes.
🛠️ Proposed fix
<?php if ( $backup ) { ?>
- <p><?php echo esc_html_e( 'The original upload has been backed up.', 'tiny-compress-images' ); ?></p>
-
+ <p><?php esc_html_e( 'The original upload has been backed up.', 'tiny-compress-images' ); ?></p>
+
<p>
- <a href="<?php echo esc_attr( $backup ); ?>" target="_blank">
- <?php esc_html_e( 'View uncompressed file' ); ?>
+ <a href="<?php echo esc_url( $backup ); ?>" target="_blank">
+ <?php esc_html_e( 'View uncompressed file', 'tiny-compress-images' ); ?>
</a>
<a class="button button-small" href="#" data-dialog-id="<?php echo esc_attr( $modal_id ); ?>" data-id="<?php echo absint( $tiny_image->get_id() ); ?>">
<?php esc_html_e( 'Restore Backup', 'tiny-compress-images' ); ?>
</a>
+ </p>
+ <dialog id="<?php echo esc_attr( $modal_id ); ?>" class="tiny-dialog">
+ ...
+ </dialog>
+ <?php } else { ?>
+ <p>
+ <span>
+ <?php esc_html_e( 'No backup available', 'tiny-compress-images' ); ?>
+ </span>
+ </p>
+ <?php } ?>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <?php if ( $backup ) { ?> | |
| <p><?php echo esc_html_e( 'The original upload has been backed up.', 'tiny-compress-images' ); ?></p> | |
| <p> | |
| <a href="<?php echo esc_attr( $backup ); ?>" target="_blank"> | |
| <?php esc_html_e( 'View uncompressed file' ); ?> | |
| </a> | |
| | |
| <a class="button button-small" href="#" data-dialog-id="<?php echo esc_attr( $modal_id ); ?>" data-id="<?php echo absint( $tiny_image->get_id() ); ?>"> | |
| <?php esc_html_e( 'Restore Backup', 'tiny-compress-images' ); ?> | |
| </a> | |
| <dialog id="<?php echo esc_attr( $modal_id ); ?>" class="tiny-dialog"> | |
| <strong class="tiny-dialog-title"><?php esc_html_e( 'Are you sure you want to restore the original uncompressed image?', 'tiny-compress-images' ); ?></strong> | |
| <p><?php esc_html_e( 'This action will replace all the compressed images with uncompressed images', 'tiny-compress-images' ); ?></p> | |
| <p class="tiny-dialog-error" hidden></p> | |
| <div class="tiny-dialog-actions"> | |
| <span class="spinner"></span> | |
| <button value="cancel" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="close" class="button"> | |
| <?php esc_html_e( 'Cancel', 'tiny-compress-images' ); ?> | |
| </button> | |
| <button value="submit" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="request-close" class="button button-primary" autofocus> | |
| <?php esc_html_e( 'Yes, Restore', 'tiny-compress-images' ); ?> | |
| </button> | |
| </div> | |
| </dialog> | |
| <?php } else { ?> | |
| <span> | |
| <?php esc_html_e( 'No backup available', 'tiny-compress-images' ); ?> | |
| </span> | |
| <?php } ?> | |
| </p> | |
| <?php if ( $backup ) { ?> | |
| <p><?php esc_html_e( 'The original upload has been backed up.', 'tiny-compress-images' ); ?></p> | |
| <p> | |
| <a href="<?php echo esc_url( $backup ); ?>" target="_blank"> | |
| <?php esc_html_e( 'View uncompressed file', 'tiny-compress-images' ); ?> | |
| </a> | |
| | |
| <a class="button button-small" href="#" data-dialog-id="<?php echo esc_attr( $modal_id ); ?>" data-id="<?php echo absint( $tiny_image->get_id() ); ?>"> | |
| <?php esc_html_e( 'Restore Backup', 'tiny-compress-images' ); ?> | |
| </a> | |
| </p> | |
| <dialog id="<?php echo esc_attr( $modal_id ); ?>" class="tiny-dialog"> | |
| <strong class="tiny-dialog-title"><?php esc_html_e( 'Are you sure you want to restore the original uncompressed image?', 'tiny-compress-images' ); ?></strong> | |
| <p><?php esc_html_e( 'This action will replace all the compressed images with uncompressed images', 'tiny-compress-images' ); ?></p> | |
| <p class="tiny-dialog-error" hidden></p> | |
| <div class="tiny-dialog-actions"> | |
| <span class="spinner"></span> | |
| <button value="cancel" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="close" class="button"> | |
| <?php esc_html_e( 'Cancel', 'tiny-compress-images' ); ?> | |
| </button> | |
| <button value="submit" commandfor="<?php echo esc_attr( $modal_id ); ?>" command="request-close" class="button button-primary" autofocus> | |
| <?php esc_html_e( 'Yes, Restore', 'tiny-compress-images' ); ?> | |
| </button> | |
| </div> | |
| </dialog> | |
| <?php } else { ?> | |
| <p> | |
| <span> | |
| <?php esc_html_e( 'No backup available', 'tiny-compress-images' ); ?> | |
| </span> | |
| </p> | |
| <?php } ?> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/views/compress-details-backup.php` around lines 18 - 49, Fix the
conditional markup around the backup view in the template: remove the paragraph
wrapper that contains the flow-content dialog, ensure the no-backup branch emits
no orphaned closing tag, and keep the dialog as valid sibling markup. Also
update the “original upload” translation call to use esc_html_e() without echo,
add the tiny-compress-images text domain to “View uncompressed file,” and use
esc_url() for the $backup href.
get_wp_filesystem() now returns WP_Filesystem_Base|false and logs via error_log() on failure instead of throwing. Removes the separate get_filesystem() wrapper. All callers guard against false.
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
2 participants
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.
#98 added backups of the original image. This will add the restore functionality.
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Style