fix(extension): fixed a bug that caused to stop browser download before checking if surge is online#394
Conversation
…re checking if surge is online
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // always win the race regardless of window focus state. | ||
| // Only intercept when Surge is actually reachable. If the daemon is offline, | ||
| // leave the browser download alone so normal downloads keep working. | ||
| if (!await checkHealthSilent()) return; |
There was a problem hiding this comment.
Moving the health check before the cancel re-introduces the race that the original comment described: Chrome actively progresses the download during any await, so a fast download (e.g. a cached resource or small file) can complete in the time the health-check round-trip takes. The previous ordering cancelled first specifically to eliminate this window. The trade-off is intentional here (offline safety > race), but it's worth documenting explicitly — and consider whether checkHealthSilent() can be given a very short timeout (e.g. 500 ms) so the window stays narrow even on a slow but reachable daemon.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extension/entrypoints/background.ts
Line: 414
Comment:
**Introduced race window on slow/healthy daemons**
Moving the health check before the cancel re-introduces the race that the original comment described: Chrome actively progresses the download during any `await`, so a fast download (e.g. a cached resource or small file) can complete in the time the health-check round-trip takes. The previous ordering cancelled first specifically to eliminate this window. The trade-off is intentional here (offline safety > race), but it's worth documenting explicitly — and consider whether `checkHealthSilent()` can be given a very short timeout (e.g. 500 ms) so the window stays narrow even on a slow but reachable daemon.
How can I resolve this? If you propose a fix, please make it concise.| it('does not cancel the browser download when Surge is offline', async () => { | ||
| mockFetch.mockImplementation(async (url: string) => { | ||
| if (url.includes('/health')) return { ok: false }; | ||
| return { ok: false }; | ||
| }); | ||
|
|
||
| const downloadItem = { | ||
| id: 789, | ||
| url: 'https://example.com/offline-fallback.zip', | ||
| startTime: new Date().toISOString(), | ||
| }; | ||
|
|
||
| await __test__.handleDownloadCreated(downloadItem); | ||
|
|
||
| expect(browser.downloads.cancel).not.toHaveBeenCalled(); | ||
| expect(browser.downloads.erase).not.toHaveBeenCalled(); | ||
|
|
||
| const downloadCall = mockFetch.mock.calls.find(call => call[0].includes('/download')); | ||
| expect(downloadCall).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
checkHealthSilent throws
The new test covers the ok: false path but not the case where the health-check fetch itself rejects (network error, timeout, etc.). If checkHealthSilent() propagates an exception rather than returning false, handleDownloadCreated would also throw and the browser download would be left in an inconsistent state (neither cancelled nor cleanly skipped). A test that stubs fetch to reject would confirm the correct fall-through behavior.
Rule Used: What: All code changes must include tests for edge... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: extension/test/intercept.test.ts
Line: 113-132
Comment:
**Missing edge-case: `checkHealthSilent` throws**
The new test covers the `ok: false` path but not the case where the health-check fetch itself rejects (network error, timeout, etc.). If `checkHealthSilent()` propagates an exception rather than returning `false`, `handleDownloadCreated` would also throw and the browser download would be left in an inconsistent state (neither cancelled nor cleanly skipped). A test that stubs `fetch` to `reject` would confirm the correct fall-through behavior.
**Rule Used:** What: All code changes must include tests for edge... ([source](https://app.greptile.com/review/custom-context?memory=2b22782d-3452-4d55-b059-e631b2540ce8))
How can I resolve this? If you propose a fix, please make it concise.
| // Once health has passed, cancel the browser download immediately before any | ||
| // additional async work so the browser does not race ahead of the handoff. |
There was a problem hiding this comment.
The comment says "before any additional async work so the browser does not race ahead of the handoff," but the health check at line 414 is itself async work — Chrome is already racing during that await. The cancellation only closes the race window that exists after the health check returns, not before it. Readers may incorrectly conclude that the race is fully eliminated once the cancel runs.
| // Once health has passed, cancel the browser download immediately before any | |
| // additional async work so the browser does not race ahead of the handoff. | |
| // Cancel immediately after the health check so no further async work can let | |
| // the browser complete the download before we hand it off to Surge. Note: a | |
| // narrow race window still exists during the health-check round-trip itself. |
Rule Used: What: Comments must explain why code exists, not... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: extension/entrypoints/background.ts
Line: 416-417
Comment:
**Slightly misleading comment about the race window**
The comment says "before any additional async work so the browser does not race ahead of the handoff," but the health check at line 414 is itself async work — Chrome is already racing during that await. The cancellation only closes the race window that exists *after* the health check returns, not before it. Readers may incorrectly conclude that the race is fully eliminated once the cancel runs.
```suggestion
// Cancel immediately after the health check so no further async work can let
// the browser complete the download before we hand it off to Surge. Note: a
// narrow race window still exists during the health-check round-trip itself.
```
**Rule Used:** What: Comments must explain *why* code exists, not... ([source](https://app.greptile.com/review/custom-context?memory=4e45ef13-32c2-4753-8060-a44ef767144d))
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Greptile Summary
This PR fixes the ordering of the health check and download cancellation in
handleDownloadCreated: the daemon reachability check now runs beforecancel/erase, so a download is never silently swallowed when Surge is offline. A companion test verifies the offline path leaves the browser download untouched.Confidence Score: 5/5
Safe to merge — the fix is correct, the only remaining findings are P2 style suggestions.
The logic change is straightforward and the new test validates the primary fix. All remaining open concerns (race window during health-check, missing exception test) were raised in prior threads and are P2 trade-offs, not blocking defects. The single new comment here is also P2.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[handleDownloadCreated] --> B{isInterceptEnabled?} B -- No --> Z[return] B -- Yes --> C{shouldSkipUrl?} C -- Yes --> Z C -- No --> D{isFreshDownload?} D -- No --> Z D -- Yes --> E{checkHealthSilent} E -- false / offline --> Z[return — browser download proceeds normally] E -- true / online --> F[cancel browser download] F --> G[erase browser download record] G --> H{isDuplicateDownload?} H -- Yes --> I[queueDuplicateDownload] H -- No --> J[sendToSurge] J --> K{result.success?} K -- Yes --> L[openPopup + notification] K -- No --> M[handle failure]Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "Merge branch 'main' into extension-bug" | Re-trigger Greptile