Repository navigation
feat(transfers): progress and cancel for SSH uploads and all remote downloads #23849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Chihen-Tai
wants to merge
27
commits into
stablyai:main
Choose a base branch
from
Chihen-Tai:feat/remote-transfer-progress
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
fd7dd6f
feat(runtime): stream file uploads instead of buffering whole files
mmarabel dc62ff8
feat(runtime): show progress while uploading dropped files to a runtime
mmarabel 1c0124b
feat(runtime): per-file drop upload panel with cancel
mmarabel 8bb9c91
fix(runtime): let the drop panel state its outcome before it closes
mmarabel dce19e4
fix(runtime): re-anchor the drop panel to the bottom when it collapses
mmarabel c043957
fix(runtime): stop the drop panel duplicating when the list is collapsed
mmarabel b0dfb1b
fix(runtime): keep the drop panel a fixed width so collapsing does no…
mmarabel 2eed419
fix(runtime): rotate the drop panel chevron instead of swapping icons
mmarabel 1e96b70
fix(runtime): name the destination in the drop upload header
mmarabel 29288f4
fix(runtime): enforce the staged size and make over-limit errors legible
mmarabel c0c1f64
fix(runtime): drop late progress events and skip the panel for an emp…
mmarabel 5b02c76
Merge feat/stream-runtime-upload-staging into feat/remote-drop-upload…
mmarabel 4a52acd
fix(runtime): enforce the per-file ceiling where the bytes move
mmarabel f7c1deb
Merge feat/stream-runtime-upload-staging into feat/remote-drop-upload…
mmarabel df9d253
test(runtime): stop allocating gigabyte fixtures to exercise the ceil…
mmarabel e357019
Merge branch 'feat/stream-runtime-upload-staging' into feat/remote-dr…
mmarabel 3bb3397
Merge upstream/main into feat/remote-drop-upload-progress
mmarabel 5c0d7ed
fix(terminal): pluralize drop upload headings and settle test upload …
mmarabel bc0e1fa
Merge remote-tracking branch 'upstream/main' into feat/remote-drop-up…
mmarabel 7d12794
feat(runtime): show drop upload progress only in the workspace it tar…
mmarabel 33753f6
fix(runtime): close drop upload gaps found in review
mmarabel b10aea3
fix(runtime): report a failed copy even when another copy was cancelled
mmarabel 6c8bc3b
Merge remote-tracking branch 'upstream/main' into HEAD
mmarabel 363dea0
fix(runtime): harden upload cancellation ownership
mmarabel de80e94
test(runtime): cover cancel release race
mmarabel ae66b3f
Merge #16107 (runtime drop upload progress) into current main
Microdaery 43c7743
feat(transfers): one progress panel with cancel for remote uploads an…
Microdaery File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import { EventEmitter } from 'node:events' | ||
| import { afterEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| const handlers = new Map< | ||
| string, | ||
| (event: { sender: EventEmitter & { id: number } }, args: unknown) => unknown | ||
| >() | ||
|
|
||
| vi.mock('electron', () => ({ | ||
| ipcMain: { | ||
| handle: ( | ||
| channel: string, | ||
| handler: (event: { sender: EventEmitter & { id: number } }, args: unknown) => unknown | ||
| ) => handlers.set(channel, handler) | ||
| } | ||
| })) | ||
|
|
||
| import { registerRuntimeUploadCancelHandlers } from './runtime-upload-cancel-ipc' | ||
| import { | ||
| forgetRuntimeUploadCancellation, | ||
| isUploadCancelled, | ||
| scopeRuntimeUploadId | ||
| } from './runtime-upload-cancellation' | ||
|
|
||
| registerRuntimeUploadCancelHandlers() | ||
|
|
||
| function renderer(id: number): EventEmitter & { id: number } { | ||
| return Object.assign(new EventEmitter(), { id }) | ||
| } | ||
|
|
||
| function invoke(channel: string, sender: EventEmitter & { id: number }, args: unknown): unknown { | ||
| return handlers.get(channel)!({ sender }, args) | ||
| } | ||
|
|
||
| afterEach(() => { | ||
| for (const senderId of [1, 2, 3]) { | ||
| for (const uploadId of ['u', 'a', 'b']) { | ||
| forgetRuntimeUploadCancellation(scopeRuntimeUploadId(senderId, uploadId)) | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| describe('runtime upload cancel IPC', () => { | ||
| it('records and releases a cancel only for the renderer that sent it', () => { | ||
| const first = renderer(1) | ||
| invoke('fs:cancelRuntimeUpload', first, { uploadId: 'u' }) | ||
|
|
||
| expect(isUploadCancelled(scopeRuntimeUploadId(1, 'u'))).toBe(true) | ||
| expect(isUploadCancelled(scopeRuntimeUploadId(2, 'u'))).toBe(false) | ||
|
|
||
| invoke('fs:releaseRuntimeUpload', first, { uploadId: 'u' }) | ||
| expect(isUploadCancelled(scopeRuntimeUploadId(1, 'u'))).toBe(false) | ||
| }) | ||
|
|
||
| it('rejects malformed upload ids without changing cancellation state', () => { | ||
| const sender = renderer(1) | ||
| for (const args of [ | ||
| null, | ||
| undefined, | ||
| {}, | ||
| { uploadId: '' }, | ||
| { uploadId: ' ' }, | ||
| { uploadId: 3 } | ||
| ]) { | ||
| expect(() => invoke('fs:cancelRuntimeUpload', sender, args)).not.toThrow() | ||
| expect(() => invoke('fs:releaseRuntimeUpload', sender, args)).not.toThrow() | ||
| } | ||
|
|
||
| expect(isUploadCancelled(scopeRuntimeUploadId(1, ''))).toBe(false) | ||
| expect(isUploadCancelled(scopeRuntimeUploadId(1, ' '))).toBe(false) | ||
| }) | ||
|
|
||
| it('drops cancels when the renderer goes away and re-arms after reload', () => { | ||
| const sender = renderer(3) | ||
| invoke('fs:cancelRuntimeUpload', sender, { uploadId: 'a' }) | ||
| sender.emit('did-navigate') | ||
| expect(isUploadCancelled(scopeRuntimeUploadId(3, 'a'))).toBe(false) | ||
|
|
||
| invoke('fs:cancelRuntimeUpload', sender, { uploadId: 'b' }) | ||
| sender.emit('render-process-gone') | ||
| expect(isUploadCancelled(scopeRuntimeUploadId(3, 'b'))).toBe(false) | ||
| }) | ||
| }) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import { ipcMain, type WebContents } from 'electron' | ||
| import { abortWhenRendererGone } from './renderer-lifetime-abort' | ||
| import { | ||
| cancelRuntimeUpload, | ||
| forgetRuntimeUploadCancellation, | ||
| forgetRuntimeUploadCancellationsForSender, | ||
| scopeRuntimeUploadId | ||
| } from './runtime-upload-cancellation' | ||
|
|
||
| type CancelSender = Pick<WebContents, 'id' | 'once' | 'removeListener'> | ||
|
|
||
| const sendersWithCleanup = new WeakSet<CancelSender>() | ||
|
|
||
| function readUploadId(args: unknown): string | undefined { | ||
| if (!args || typeof args !== 'object' || !('uploadId' in args)) { | ||
| return undefined | ||
| } | ||
| return typeof args.uploadId === 'string' && args.uploadId.trim() !== '' | ||
| ? args.uploadId | ||
| : undefined | ||
| } | ||
|
|
||
| /** A reloaded or gone renderer can no longer release its remembered cancels. */ | ||
| function forgetCancelsWhenRendererGoes(sender: CancelSender): void { | ||
| if (sendersWithCleanup.has(sender)) { | ||
| return | ||
| } | ||
| sendersWithCleanup.add(sender) | ||
| const senderId = sender.id | ||
| const lifetime = abortWhenRendererGone(sender) | ||
| lifetime.signal.addEventListener( | ||
| 'abort', | ||
| () => { | ||
| lifetime.dispose() | ||
| forgetRuntimeUploadCancellationsForSender(senderId) | ||
| // A reload reuses WebContents, so its next cancel must arm cleanup again. | ||
| sendersWithCleanup.delete(sender) | ||
| }, | ||
| { once: true } | ||
| ) | ||
| } | ||
|
|
||
| export function registerRuntimeUploadCancelHandlers(): void { | ||
| ipcMain.handle('fs:cancelRuntimeUpload', (event, args: unknown): void => { | ||
| const uploadId = readUploadId(args) | ||
| if (uploadId) { | ||
| forgetCancelsWhenRendererGoes(event.sender) | ||
| cancelRuntimeUpload(scopeRuntimeUploadId(event.sender.id, uploadId)) | ||
| } | ||
| }) | ||
|
|
||
| ipcMain.handle('fs:releaseRuntimeUpload', (event, args: unknown): void => { | ||
| const uploadId = readUploadId(args) | ||
| if (uploadId) { | ||
| forgetRuntimeUploadCancellation(scopeRuntimeUploadId(event.sender.id, uploadId)) | ||
| } | ||
| }) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| import { afterEach, describe, expect, it } from 'vitest' | ||
| import { | ||
| cancelRuntimeUpload, | ||
| forgetRuntimeUploadCancellation, | ||
| forgetRuntimeUploadCancellationsForSender, | ||
| isRuntimeUploadCancelled, | ||
| isUploadCancelled, | ||
| registerCancellableUpload, | ||
| RuntimeUploadCancelledError, | ||
| scopeRuntimeUploadId | ||
| } from './runtime-upload-cancellation' | ||
|
|
||
| afterEach(() => { | ||
| forgetRuntimeUploadCancellation('u') | ||
| forgetRuntimeUploadCancellation('v') | ||
| }) | ||
|
|
||
| describe('runtime upload cancellation', () => { | ||
| it('aborts an upload that is already running', () => { | ||
| const upload = registerCancellableUpload('u') | ||
| expect(upload.signal.aborted).toBe(false) | ||
|
|
||
| cancelRuntimeUpload('u') | ||
|
|
||
| expect(upload.signal.aborted).toBe(true) | ||
| upload.release() | ||
| }) | ||
|
|
||
| it('aborts the next file when cancel lands between two files of one drop', () => { | ||
| const first = registerCancellableUpload('u') | ||
| first.release() | ||
| cancelRuntimeUpload('u') | ||
|
|
||
| const second = registerCancellableUpload('u') | ||
|
|
||
| expect(second.signal.aborted).toBe(true) | ||
| second.release() | ||
| }) | ||
|
|
||
| it('leaves other uploads running', () => { | ||
| const target = registerCancellableUpload('u') | ||
| const bystander = registerCancellableUpload('v') | ||
|
|
||
| cancelRuntimeUpload('u') | ||
|
|
||
| expect(target.signal.aborted).toBe(true) | ||
| expect(bystander.signal.aborted).toBe(false) | ||
| target.release() | ||
| bystander.release() | ||
| }) | ||
|
|
||
| it('stops applying a cancel once the drop is forgotten', () => { | ||
| cancelRuntimeUpload('u') | ||
| forgetRuntimeUploadCancellation('u') | ||
|
|
||
| const fresh = registerCancellableUpload('u') | ||
| expect(isUploadCancelled('u')).toBe(false) | ||
| expect(fresh.signal.aborted).toBe(false) | ||
| fresh.release() | ||
| }) | ||
|
|
||
| it('releasing a superseded registration does not evict the live one', () => { | ||
| const stale = registerCancellableUpload('u') | ||
| const live = registerCancellableUpload('u') | ||
| stale.release() | ||
|
|
||
| cancelRuntimeUpload('u') | ||
|
|
||
| expect(live.signal.aborted).toBe(true) | ||
| live.release() | ||
| }) | ||
|
|
||
| it('keeps windows apart even when they mint the same id', () => { | ||
| expect(scopeRuntimeUploadId(1, 'u')).not.toBe(scopeRuntimeUploadId(2, 'u')) | ||
| }) | ||
|
|
||
| it("forgets a gone renderer's remembered cancels and only that renderer's", () => { | ||
| cancelRuntimeUpload(scopeRuntimeUploadId(1, 'u')) | ||
| cancelRuntimeUpload(scopeRuntimeUploadId(2, 'u')) | ||
|
|
||
| forgetRuntimeUploadCancellationsForSender(1) | ||
|
|
||
| expect(isUploadCancelled(scopeRuntimeUploadId(1, 'u'))).toBe(false) | ||
| expect(isUploadCancelled(scopeRuntimeUploadId(2, 'u'))).toBe(true) | ||
| forgetRuntimeUploadCancellation(scopeRuntimeUploadId(2, 'u')) | ||
| }) | ||
|
|
||
| it('keeps a live upload aborted if its drop is released mid-flight', () => { | ||
| const live = registerCancellableUpload('u') | ||
| cancelRuntimeUpload('u') | ||
| forgetRuntimeUploadCancellation('u') | ||
|
|
||
| expect(live.signal.aborted).toBe(true) | ||
| live.release() | ||
| }) | ||
|
|
||
| it('keeps a live registration cancellable after its remembered cancel is released', () => { | ||
| const live = registerCancellableUpload('u') | ||
| forgetRuntimeUploadCancellation('u') | ||
|
|
||
| cancelRuntimeUpload('u') | ||
|
|
||
| expect(live.signal.aborted).toBe(true) | ||
| live.release() | ||
| }) | ||
|
|
||
| it('recognises its own error and nothing else', () => { | ||
| expect(isRuntimeUploadCancelled(new RuntimeUploadCancelledError())).toBe(true) | ||
| expect(isRuntimeUploadCancelled(new Error('disk full'))).toBe(false) | ||
| expect(isRuntimeUploadCancelled('cancelled')).toBe(false) | ||
| }) | ||
| }) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rewritten test covers reloads and renderer crashes, but no longer checks that closing a window clears its remembered upload cancels. If that cleanup path regresses, this IPC suite could pass while stale cancels remain in main. Please retain a closed-window assertion.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!