Skip to content

Revert tempoary recovery path for autosaved documents#4169

Open
timon-schelling wants to merge 1 commit into
masterfrom
revert-tmp-recovery
Open

Revert tempoary recovery path for autosaved documents#4169
timon-schelling wants to merge 1 commit into
masterfrom
revert-tmp-recovery

Conversation

@timon-schelling
Copy link
Copy Markdown
Member

@timon-schelling timon-schelling commented May 25, 2026

This reverts commit 62287d7.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +19 to 26
#[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),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
#[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
  1. It is acceptable to remove old data migrations for features that are being retired, even if it breaks backward compatibility.

Comment on lines +772 to 777
if !document_is_auto_saved {
responses.add(DialogMessage::DisplayDialogError {
title: "Failed to open document".to_string(),
description: e.to_string(),
});
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. It is acceptable to remove old data migrations for features that are being retired, even if it breaks backward compatibility.

Copy link
Copy Markdown
Member

@Keavon Keavon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please hold off merging till we make a plan with this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants