fix: readConfig now logs errors and backs up corrupted config#469
fix: readConfig now logs errors and backs up corrupted config#469HiddenPuppy wants to merge 1 commit intomainfrom
Conversation
- Log parse/read errors via console.error - Rename corrupted config file with .corrupted-<timestamp> suffix - Keep returning null so first-run flow still kicks in Fixes #384
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness of the configuration loading process by replacing a silent failure with explicit error logging and a backup strategy for corrupted files. These changes ensure that users are aware of configuration issues while preserving the original state of the file for potential recovery, without disrupting the application's fallback behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Hi @HiddenPuppy, DetailsInstructions for interacting with me using comments are available here. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the configuration reading logic in the Electron main process to log errors and attempt to back up corrupted configuration files by renaming them with a timestamped suffix. A review comment suggests refining this behavior to only trigger the backup rename when a SyntaxError is encountered, preventing data loss from transient I/O or permission errors that do not necessarily indicate file corruption.
| } catch (err) { | ||
| console.error('[config] failed to read:', err); | ||
| const corruptedPath = `${cfgPath}.corrupted-${new Date().toISOString().replace(/[:.]/g, '-')}`; | ||
| try { | ||
| renameSync(cfgPath, corruptedPath); | ||
| } catch { | ||
| // best-effort backup rename | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The current implementation renames the configuration file as 'corrupted' for any error encountered during the read or migration process. This is quite aggressive and could lead to unnecessary data loss. For example, if an I/O error occurs during migration (like a full disk) or if there's a transient filesystem permission issue, the user's valid configuration will be moved aside, forcing them to reconfigure the app from scratch. It is safer to only perform the backup rename if the error is a SyntaxError, which specifically indicates that the file content is not valid JSON and thus truly corrupted.
} catch (err) {
console.error('[config] failed to read:', err);
if (err instanceof SyntaxError) {
const corruptedPath = cfgPath + '.corrupted-' + new Date().toISOString().replace(/[:.]/g, '-');
try {
renameSync(cfgPath, corruptedPath);
} catch {
// best-effort backup rename
}
}
return null;
}1 participant
Summary
readConfig()had a barecatch {}that silently returnednullon any parse/decrypt error, causing user config to disappear without trace.Changes
console.error('[config] failed to read:', err)in the catch blockconfig.json→config.json.corrupted-<ISO timestamp>so recovery is possiblenullso the first-run/onboarding flow kicks in as expectedTesting
pnpm check✅ (architecture guardrails passed, eslint + prettier clean)pnpm test✅ (256 tests, 42 test files — all passing)Fixes #384