Fix: callback not invoked on Dropbox filesDownload error in Store.js#8
Conversation
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
|
@copilot please fix build |
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Fixed in commit 6558261. The build was failing because running |
There was a problem hiding this comment.
Pull request overview
Fixes a Store initialization edge case where the provided callback was not invoked when Dropbox filesDownload fails, and adds regression tests around Dropbox initialization behavior.
Changes:
- Invoke the Store initialization callback from the Dropbox
filesDownload.catch(...)handler. - Add unit tests verifying callback invocation on Dropbox download success and failure.
- Update Mocha devDependency version.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/js/Store.js | Ensures the initialization callback is triggered when Dropbox download rejects. |
| test/store.test.js | Adds Dropbox init tests to assert callback invocation for success/error paths. |
| package.json | Bumps Mocha version used for running tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -30,6 +30,7 @@ var Store = function (callback) { | |||
| if (JSON.parse(error.error).error_summary !== "path/not_found/..") { | |||
| ipcRenderer.send('message', i18n("message.dropboxfailed")); | |||
| } | |||
| setTimeout(function() { callback.call(self); }, 1); | |||
There was a problem hiding this comment.
callback is invoked unconditionally here. If Store is constructed without a callback (the constructor already treats it as optional in the non-Dropbox path), this will throw a TypeError. Guard the invocation (e.g., only call when callback is a function) or provide a no-op default so the Dropbox error path can’t crash the app.
There was a problem hiding this comment.
Fixed in commit e713b5d. Both the .then (loadend) and .catch handlers now guard with typeof callback === 'function' before invoking callback.call(self).
| return Promise.resolve({ | ||
| fileBlob: new Blob([JSON.stringify({})], { type: 'application/json' }) |
There was a problem hiding this comment.
This test uses new Blob(...), but the unit tests run under plain Node (and this repo’s CI/build config includes Node 10 on Windows), where Blob is not available. Since the FileReader mock ignores the argument anyway, return a simple placeholder (or add a global.Blob polyfill in the test setup) to avoid ReferenceError: Blob is not defined in older Node environments.
| return Promise.resolve({ | |
| fileBlob: new Blob([JSON.stringify({})], { type: 'application/json' }) | |
| // Use a simple placeholder instead of Blob to avoid ReferenceError in Node | |
| return Promise.resolve({ | |
| fileBlob: {} |
There was a problem hiding this comment.
Fixed in commit e713b5d. Replaced new Blob([JSON.stringify({})], { type: 'application/json' }) with a plain {} object, which avoids the ReferenceError in Node environments where Blob is not globally available.
… in test Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
filesDownloaderror in Store.js.catchhandlercallback.call(self)withtypeof callback === 'function'in both.thenand.catchhandlers to prevent TypeError when no callback is providednew Blob(...)with plain{}object in test to avoid ReferenceError in older Node environments✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.