From 36a78ad431da1a79bb0f371db21ba2db23a4f642 Mon Sep 17 00:00:00 2001 From: abose Date: Sat, 25 Jan 2025 11:24:11 +0530 Subject: [PATCH 1/4] fix: bugsnag errors in git workflows due to race conditions The catch block was attached after a delay in some cases and errors before attaching the catch block in progress dialog used to get treated as uncaught exceptions and get logged in bugsnag fixed by modifying the timing chain and have a catch handler attached to promise at first itself. --- .../default/Git/src/dialogs/Progress.js | 38 +++++++++++-------- src/loggerSetup.js | 4 +- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/extensions/default/Git/src/dialogs/Progress.js b/src/extensions/default/Git/src/dialogs/Progress.js index 161e6d22fa..c338398bd9 100644 --- a/src/extensions/default/Git/src/dialogs/Progress.js +++ b/src/extensions/default/Git/src/dialogs/Progress.js @@ -85,16 +85,17 @@ define(function (require, exports) { onProgress(); } + let finalValue, finalError; function finish() { finished = true; if (dialog) { dialog.close(); } - promise.then(function (val) { - resolve(val); - }).catch(function (err) { - reject(err); - }); + if(finalError){ + reject(finalError); + } else { + resolve(finalValue); + } } if (!options.preDelay) { @@ -109,17 +110,24 @@ define(function (require, exports) { progressTracker.on(`${Events.GIT_PROGRESS_EVENT}.progressDlg`, (_evt, data)=>{ onProgress(data); }); - promise.finally(function () { - progressTracker.off(`${Events.GIT_PROGRESS_EVENT}.progressDlg`); - onProgress("Finished!"); - if (!options.postDelay || !dialog) { - finish(); - } else { - setTimeout(function () { + promise + .then(val => { + finalValue = val; + }) + .catch(err => { + finalError = err; + }) + .finally(function () { + progressTracker.off(`${Events.GIT_PROGRESS_EVENT}.progressDlg`); + onProgress("Finished!"); + if (!options.postDelay || !dialog) { finish(); - }, options.postDelay * 1000); - } - }); + } else { + setTimeout(function () { + finish(); + }, options.postDelay * 1000); + } + }); }); } diff --git a/src/loggerSetup.js b/src/loggerSetup.js index 93180e4b30..f58b06c3ee 100644 --- a/src/loggerSetup.js +++ b/src/loggerSetup.js @@ -18,7 +18,7 @@ * */ -/*globals Bugsnag, AppConfig, Phoenix*/ +/*globals Bugsnag, AppConfig*/ // window.AppConfig comes from appConfig.js built by gulp scripts at build time (function(){ @@ -32,7 +32,7 @@ "https://dev.phcode.dev", // dev url "https://staging.phcode.dev" // staging url ]; - let isBugsnagLoggableURL = false; + let isBugsnagLoggableURL = false; // to test bugsnag in dev builds, set this to true for(let loggableURL of loggableURLS){ if(window.location.href.startsWith(loggableURL)){ isBugsnagLoggableURL = true; From e25cf52a6d00bdc77ec3de71b3ed5321e24625ed Mon Sep 17 00:00:00 2001 From: abose Date: Sat, 25 Jan 2025 11:51:35 +0530 Subject: [PATCH 2/4] chore: reveiwed uncaught exception handlers in git --- src/extensions/default/Git/src/dialogs/Pull.js | 7 ++++--- src/extensions/default/Git/src/dialogs/Push.js | 1 + src/extensions/default/Git/src/dialogs/RemoteCommon.js | 1 + 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/extensions/default/Git/src/dialogs/Pull.js b/src/extensions/default/Git/src/dialogs/Pull.js index 3d05e8c18a..021779ae6d 100644 --- a/src/extensions/default/Git/src/dialogs/Pull.js +++ b/src/extensions/default/Git/src/dialogs/Pull.js @@ -1,16 +1,16 @@ define(function (require, exports) { // Brackets modules - var Dialogs = brackets.getModule("widgets/Dialogs"), + const Dialogs = brackets.getModule("widgets/Dialogs"), Mustache = brackets.getModule("thirdparty/mustache/mustache"); // Local modules - var Preferences = require("src/Preferences"), + const Preferences = require("src/Preferences"), RemoteCommon = require("src/dialogs/RemoteCommon"), Strings = brackets.getModule("strings"); // Templates - var template = require("text!src/dialogs/templates/pull-dialog.html"), + const template = require("text!src/dialogs/templates/pull-dialog.html"), remotesTemplate = require("text!src/dialogs/templates/remotes-template.html"); // Implementation @@ -54,6 +54,7 @@ define(function (require, exports) { function show(pullConfig) { return new Promise((resolve, reject) => { pullConfig.pull = true; + // collectInfo never rejects RemoteCommon.collectInfo(pullConfig).then(()=>{ _show(pullConfig, resolve, reject); }); diff --git a/src/extensions/default/Git/src/dialogs/Push.js b/src/extensions/default/Git/src/dialogs/Push.js index 86e569834d..06ba3eddd5 100644 --- a/src/extensions/default/Git/src/dialogs/Push.js +++ b/src/extensions/default/Git/src/dialogs/Push.js @@ -52,6 +52,7 @@ define(function (require, exports) { function show(pushConfig) { return new Promise((resolve, reject) => { pushConfig.push = true; + // collectInfo never rejects RemoteCommon.collectInfo(pushConfig).then(()=>{ _show(pushConfig, resolve, reject); }); diff --git a/src/extensions/default/Git/src/dialogs/RemoteCommon.js b/src/extensions/default/Git/src/dialogs/RemoteCommon.js index 89be66d975..7105470b7d 100644 --- a/src/extensions/default/Git/src/dialogs/RemoteCommon.js +++ b/src/extensions/default/Git/src/dialogs/RemoteCommon.js @@ -28,6 +28,7 @@ define(function (require, exports) { }); } + // this should never reject for now, just show error message and bail out exports.collectInfo = function (config) { return Git.getCurrentUpstreamBranch().then(function (upstreamBranch) { config.currentTrackingBranch = upstreamBranch; From ca0834cf409713e849d332da3d583c66ecca75fb Mon Sep 17 00:00:00 2001 From: abose Date: Sat, 25 Jan 2025 12:11:48 +0530 Subject: [PATCH 3/4] chore: reveiwed uncaught exception handlers in git --- src/extensions/default/Git/src/Branch.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/extensions/default/Git/src/Branch.js b/src/extensions/default/Git/src/Branch.js index 851d5bd8ac..51f3236c98 100644 --- a/src/extensions/default/Git/src/Branch.js +++ b/src/extensions/default/Git/src/Branch.js @@ -128,6 +128,11 @@ define(function (require, exports) { } } }); + }).catch(err => { + console.error("Error Getting branches", err); + // we need to strip all user entered info from git thrown exception for get branches which shouldn't fail, + // so we throw a blank error for bugsnag + throw new Error("Failed to get getBranches while doMerge"); }); } @@ -168,7 +173,7 @@ define(function (require, exports) { Git.getAllBranches().catch(function (err) { ErrorHandler.showError(err); - }).then(function (branches) { + }).then(function (branches = []) { var compiledTemplate = Mustache.render(newBranchTemplate, { branches: branches, @@ -335,7 +340,7 @@ define(function (require, exports) { Git.getBranches().catch(function (err) { ErrorHandler.showError(err, Strings.ERROR_GETTING_BRANCH_LIST); - }).then(function (branches) { + }).then(function (branches = []) { branches = branches.reduce(function (arr, branch) { if (!branch.currentBranch && !branch.remote) { arr.push(branch.name); From fc13a647f3d16475df4c90b42732151a134e54c4 Mon Sep 17 00:00:00 2001 From: abose Date: Sat, 25 Jan 2025 12:24:34 +0530 Subject: [PATCH 4/4] chore: reveiwed uncaught exception handlers in git --- src/extensions/default/Git/src/Panel.js | 33 +++++++++++++++++++------ src/nls/root/strings.js | 1 + 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/extensions/default/Git/src/Panel.js b/src/extensions/default/Git/src/Panel.js index b183ceacfe..d3a2c4fbe3 100644 --- a/src/extensions/default/Git/src/Panel.js +++ b/src/extensions/default/Git/src/Panel.js @@ -742,7 +742,7 @@ define(function (require, exports) { } p.catch(function (err) { - ErrorHandler.showError(err, "Preparing commit dialog failed"); + ErrorHandler.showError(err, Strings.ERROR_PREPARING_COMMIT_DIALOG); }).finally(function () { Utils.unsetLoading($gitPanel.find(".git-commit")); }); @@ -956,7 +956,12 @@ define(function (require, exports) { return Git.resetIndex(); }) .then(function () { - return handleGitCommit(lastCommitMessage[ProjectManager.getProjectRoot().fullPath], false, COMMIT_MODE.CURRENT); + return handleGitCommit(lastCommitMessage[ProjectManager.getProjectRoot().fullPath], + false, COMMIT_MODE.CURRENT); + }).catch((err)=>{ + console.error(err); + // rethrowing with stripped git error details as it may have sensitive info + throw new Error("Error commitCurrentFile in git panel.js. this should not have happened here."); }); } @@ -967,7 +972,12 @@ define(function (require, exports) { return Git.resetIndex(); }) .then(function () { - return handleGitCommit(lastCommitMessage[ProjectManager.getProjectRoot().fullPath], false, COMMIT_MODE.ALL); + return handleGitCommit(lastCommitMessage[ProjectManager.getProjectRoot().fullPath], + false, COMMIT_MODE.ALL); + }).catch((err)=>{ + console.error(err); + // rethrowing with stripped git error details as it may have sensitive info + throw new Error("Error commitAllFiles in git panel.js. this should not have happened here."); }); } @@ -1232,13 +1242,20 @@ define(function (require, exports) { .on("click", ".check-all", function () { if ($(this).is(":checked")) { return Git.stageAll().then(function () { - Git.status(); - }); - } else { - return Git.resetIndex().then(function () { - Git.status(); + return Git.status(); + }).catch((err)=>{ + console.error(err); + // rethrowing with stripped git error details as it may have sensitive info + throw new Error("Error stage all by checkbox in git panel.js. this should not have happened"); }); } + return Git.resetIndex().then(function () { + return Git.status(); + }).catch((err)=>{ + console.error(err); + // rethrowing with stripped git error details as it may have sensitive info + throw new Error("Error unstage all by checkbox in git panel.js. this should not have happened"); + }); }) .on("click", ".git-refresh", EventEmitter.getEmitter(Events.REFRESH_ALL, ["panel", "refreshBtn"])) .on("click", ".git-commit", EventEmitter.getEmitter(Events.HANDLE_GIT_COMMIT)) diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index 4f508f28ca..d37504a04e 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -1529,6 +1529,7 @@ define({ "ERROR_NO_REMOTE_SELECTED": "No remote has been selected for {0}!", "ERROR_BRANCH_LIST": "Getting branch list failed", "ERROR_FETCH_REMOTE": "Fetching remote information failed", + "ERROR_PREPARING_COMMIT_DIALOG": "Preparing commit dialog failed", "GIT_TOAST_TITLE": "Explore Git Features in Phoenix Code", "GIT_TOAST_MESSAGE": "Click the Git panel icon to manage your repository. Easily commit, push, pull, and view your project history—all in one place.
Learn more about the Git panel →",