Revert tempoary recovery path for autosaved documents#4169
Revert tempoary recovery path for autosaved documents#4169timon-schelling wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes legacy document upgrade and recovery infrastructure, including the zip dependency and associated dialogs for failed document loads. The document loading logic is simplified by removing batching and recovery paths for autosaves. Feedback focuses on improving error reporting: one comment suggests including the underlying error details in the DocumentDeserialization message for debugging, while another points out that autosave failures are now silently ignored and should instead be logged and displayed to the user.
| #[error( | ||
| "This document was created in an older version of the editor.\n\ | ||
| \n\ | ||
| Full backwards compatibility is not guaranteed in the current alpha release.\n\ | ||
| \n\ | ||
| If this document is critical, ask for support in Graphite's Discord community." | ||
| )] | ||
| DocumentDeserialization(String), |
There was a problem hiding this comment.
The DocumentDeserialization error variant now ignores its String payload. While Rule 3 allows breaking backward compatibility for retired features, including the specific reason for failure is valuable for debugging. The suggested message acknowledges the compatibility policy while providing the error details.
| #[error( | |
| "This document was created in an older version of the editor.\n\ | |
| \n\ | |
| Full backwards compatibility is not guaranteed in the current alpha release.\n\ | |
| \n\ | |
| If this document is critical, ask for support in Graphite's Discord community." | |
| )] | |
| DocumentDeserialization(String), | |
| #[error( | |
| "This document was created in an older version of the editor.\n\ | |
| \n\ | |
| Full backwards compatibility is not guaranteed in the current alpha release.\n\ | |
| \n\ | |
| If this document is critical, ask for support in Graphite's Discord community.\n\ | |
| \n\ | |
| Error: {0}" | |
| )] | |
| DocumentDeserialization(String), |
References
- It is acceptable to remove old data migrations for features that are being retired, even if it breaks backward compatibility.
| if !document_is_auto_saved { | ||
| responses.add(DialogMessage::DisplayDialogError { | ||
| title: "Failed to open document".to_string(), | ||
| description: e.to_string(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Failures to load autosaved documents are now silently ignored. While Rule 3 permits removing specialized recovery paths for retired features, the user should still be informed of the failure, and the error should be logged for diagnostic purposes.
log::error!("{e}");
responses.add(DialogMessage::DisplayDialogError {
title: "Failed to open document".to_string(),
description: e.to_string(),
});References
- It is acceptable to remove old data migrations for features that are being retired, even if it breaks backward compatibility.
Keavon
left a comment
There was a problem hiding this comment.
Please hold off merging till we make a plan with this
This reverts commit 62287d7.