Skip to content

Commit 2e92971

Browse files
committed
fix: file recovery not working if file has comma in it. don't save not be treated as crash
We should not show file recovery dialog if the user selected dont save in when exiting phcode Our integrity checks saves the file as `checksum,filecontent`. but if file content itself has a comma, out logic used to break due to improper `string.split(",")` usage.
1 parent 07703e7 commit 2e92971

File tree

3 files changed

+39
-17
lines changed

3 files changed

+39
-17
lines changed

src/document/DocumentCommandHandlers.js

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1616,6 +1616,7 @@ define(function (require, exports, module) {
16161616
* @private - tracks our closing state if we get called again
16171617
*/
16181618
var _windowGoingAway = false;
1619+
let exitWaitPromises = [];
16191620

16201621
/**
16211622
* @private
@@ -1634,12 +1635,15 @@ define(function (require, exports, module) {
16341635

16351636
return CommandManager.execute(Commands.FILE_CLOSE_ALL, { promptOnly: true })
16361637
.done(function () {
1638+
exitWaitPromises = [];
16371639
_windowGoingAway = true;
16381640

16391641
// Give everyone a chance to save their state - but don't let any problems block
16401642
// us from quitting
16411643
try {
1642-
ProjectManager.trigger("beforeAppClose");
1644+
// if someone wats to do any deferred tasks, they should add
1645+
// their promise to the wait promises list.
1646+
ProjectManager.trigger("beforeAppClose", exitWaitPromises);
16431647
} catch (ex) {
16441648
console.error(ex);
16451649
}
@@ -2016,10 +2020,13 @@ define(function (require, exports, module) {
20162020
_isReloading = true;
20172021

20182022
return CommandManager.execute(Commands.FILE_CLOSE_ALL, { promptOnly: true }).done(function () {
2023+
exitWaitPromises = [];
20192024
// Give everyone a chance to save their state - but don't let any problems block
20202025
// us from quitting
20212026
try {
2022-
ProjectManager.trigger("beforeAppClose");
2027+
// if someone wats to do any deferred tasks, they should add
2028+
// their promise to the wait promises list.
2029+
ProjectManager.trigger("beforeAppClose", exitWaitPromises);
20232030
} catch (ex) {
20242031
console.error(ex);
20252032
}
@@ -2037,7 +2044,8 @@ define(function (require, exports, module) {
20372044

20382045
// Defer for a more successful reload - issue #11539
20392046
window.setTimeout(function () {
2040-
raceAgainstTime(window.PhStore.flushDB()) // wither wait for flush or time this out
2047+
exitWaitPromises.push(window.PhStore.flushDB());
2048+
raceAgainstTime(Promise.all(exitWaitPromises)) // wither wait for flush or time this out
20412049
.finally(()=>{
20422050
raceAgainstTime(_safeNodeTerminate(), 4000)
20432051
.finally(()=>{
@@ -2210,7 +2218,8 @@ define(function (require, exports, module) {
22102218
event.preventDefault();
22112219
_handleWindowGoingAway(null, closeSuccess=>{
22122220
console.log('close success: ', closeSuccess);
2213-
raceAgainstTime(_safeFlushDB())
2221+
exitWaitPromises.push(_safeFlushDB());
2222+
raceAgainstTime(Promise.all(exitWaitPromises))
22142223
.finally(()=>{
22152224
raceAgainstTime(_safeNodeTerminate())
22162225
.finally(()=>{

src/extensionsIntegrated/NavigationAndHistory/FileRecovery.js

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -162,20 +162,25 @@ define(function (require, exports, module) {
162162
if(!input){
163163
return null;
164164
}
165-
const parts = input.split(',', 2);
165+
// Find the first comma
166+
const firstCommaIndex = input.indexOf(',');
166167

167-
if (parts.length !== 2) {
168+
// If there's no comma or it's at the very beginning, the input is invalid
169+
if (firstCommaIndex === -1 || firstCommaIndex === 0) {
168170
return null;
169171
}
170172

173+
// Extract the parts
174+
const expectedLengthPart = input.slice(0, firstCommaIndex);
175+
const actualString = input.slice(firstCommaIndex + 1);
176+
171177
// Parse the length part (should be the first part before the comma)
172-
const expectedLength = parseInt(parts[0], 10);
178+
const expectedLength = parseInt(expectedLengthPart, 10);
173179
if (isNaN(expectedLength)) {
174180
return null;
175181
}
176182

177-
// The second part is the actual string after the comma
178-
const actualString = parts[1];
183+
// The second part is the actual string after the commas
179184
if (actualString.length === expectedLength) {
180185
return actualString;
181186
}
@@ -471,6 +476,14 @@ define(function (require, exports, module) {
471476
// for the startup project. So we call manually.
472477
projectOpened(null, currentProjectRoot);
473478
}
479+
ProjectManager.on("beforeAppClose", (_evt, waitPromises)=>{
480+
// this is a safe exit, we should delete all restore data.
481+
let exitProjectRoot = ProjectManager.getProjectRoot();
482+
if(exitProjectRoot) {
483+
const restoreRoot = getProjectRestoreRoot(exitProjectRoot.fullPath);
484+
waitPromises.push(silentlyRemoveDirectory(restoreRoot));
485+
}
486+
});
474487
}
475488

476489
function init() {

test/spec/Extn-NavigationAndHistory-integ-test.js

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ define(function (require, exports, module) {
3333
const tempRestorePath = SpecRunnerUtils.getTestRoot() + "/navigationTestsRestore/";
3434

3535
let FileViewController, // loaded from brackets.test
36-
ProjectManager, // loaded from brackets.test;
3736
CommandManager,
3837
Commands,
3938
testWindow,
@@ -57,7 +56,6 @@ define(function (require, exports, module) {
5756
brackets = testWindow.brackets;
5857
$ = testWindow.$;
5958
FileViewController = brackets.test.FileViewController;
60-
ProjectManager = brackets.test.ProjectManager;
6159
CommandManager = brackets.test.CommandManager;
6260
Commands = brackets.test.Commands;
6361
EditorManager = brackets.test.EditorManager;
@@ -76,7 +74,6 @@ define(function (require, exports, module) {
7674

7775
afterAll(async function () {
7876
FileViewController = null;
79-
ProjectManager = null;
8077
testWindow = null;
8178
brackets = null;
8279
await deletePath(testPath);
@@ -124,13 +121,16 @@ define(function (require, exports, module) {
124121

125122
}
126123

124+
// multiple , in file tested as we have , for `size,"content"` integrity checks
125+
const unsavedTextFragment = "hello,this is ,a test";
126+
127127
it("Should create restore folders and backup files", async function () {
128128
await initFileRestorer("file.js");
129129
let projectRestorePath = testWindow._FileRecoveryExtensionForTests.getProjectRestoreRoot(testPath);
130130

131131
// now edit a file so that its backup is created
132132
let editor = EditorManager.getActiveEditor();
133-
editor.document.setText("hello");
133+
editor.document.setText(unsavedTextFragment);
134134
await SpecRunnerUtils.waitTillPathExists(projectRestorePath.fullPath, true);
135135
await SpecRunnerUtils.waitTillPathExists(projectRestorePath.fullPath + "file.js", false);
136136
await closeSession();
@@ -142,7 +142,7 @@ define(function (require, exports, module) {
142142

143143
// now edit a file so that its backup is created
144144
let editor = EditorManager.getActiveEditor();
145-
editor.document.setText("hello");
145+
editor.document.setText(unsavedTextFragment);
146146
await SpecRunnerUtils.waitTillPathExists(projectRestorePath.fullPath + "toDelete1/file.js", false);
147147
await awaitsForDone(CommandManager.execute(Commands.FILE_SAVE_ALL), "saving all file");
148148
await SpecRunnerUtils.waitTillPathNotExists(projectRestorePath.fullPath + "toDelete1/file.js", false);
@@ -154,7 +154,7 @@ define(function (require, exports, module) {
154154
let projectRestorePath = testWindow._FileRecoveryExtensionForTests.getProjectRestoreRoot(testPath);
155155

156156
// now edit a file so that its backup is created
157-
const unsavedText = "hello" + Math.random();
157+
const unsavedText = unsavedTextFragment + Math.random();
158158
let editor = EditorManager.getActiveEditor();
159159
editor.document.setText(unsavedText);
160160
await SpecRunnerUtils.waitTillPathExists(projectRestorePath.fullPath + "toDelete1/file.js", false);
@@ -186,7 +186,7 @@ define(function (require, exports, module) {
186186
let projectRestorePath = testWindow._FileRecoveryExtensionForTests.getProjectRestoreRoot(testPath);
187187

188188
// now edit a file so that its backup is created
189-
const unsavedText = "hello" + Math.random();
189+
const unsavedText = unsavedTextFragment + Math.random();
190190
let editor = EditorManager.getActiveEditor();
191191
editor.document.setText(unsavedText);
192192
await SpecRunnerUtils.waitTillPathExists(projectRestorePath.fullPath + fileNameToRestore, false);
@@ -343,7 +343,7 @@ define(function (require, exports, module) {
343343
await openFile(file3);
344344
await _expectNavButton(false, true, "nav forward disabled due to new file open");
345345
}
346-
346+
347347
it("Should navigate back and forward between text files", async function () {
348348
await navigateResetStack();
349349
await _validateNavForFiles("test.css", "test.html", "test.js");

0 commit comments

Comments
 (0)