fix: harden cancellation and mobile remote controls - #105
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds cancellation-aware downloader teardown and cancelled-artifact cleanup. It also changes mobile remote controls to use relative pointer movement, button-only clicks, and long-press touch zoom. ChangesDownload cancellation
Mobile remote control
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant Downloader
participant DownloadTask
participant DownloadCleanup
Server->>Downloader: close()
Downloader->>DownloadTask: reject or abort active download
DownloadTask-->>Server: task completion or cancellation
Server->>DownloadCleanup: cleanup cancelled artifacts
DownloadCleanup-->>Server: removed artifact names
sequenceDiagram
participant TouchSession
participant MobileRemoteControls
participant RemoteBrowserPanel
TouchSession->>MobileRemoteControls: touch movement or release
MobileRemoteControls->>RemoteBrowserPanel: movePointerBy(deltaX, deltaY)
MobileRemoteControls->>RemoteBrowserPanel: zoomBy(deltaY, anchorX, anchorY)
MobileRemoteControls->>RemoteBrowserPanel: click(button)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hypeddit.ts (1)
771-779: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle failure from an already-running retry.
If cancellation starts after this callback begins,
clearTimeout(retryTimer)cannot stoppage.click(). Browser closure then rejectspage.click()without a handler.Catch and ignore this best-effort retry failure.
Proposed fix
const retryTimer = setTimeout(async () => { if (!downloadGuid) { - await page.click(Selectors.DW_DOWNLOAD_BUTTON); + try { + await page.click(Selectors.DW_DOWNLOAD_BUTTON); + } catch { + // The page can close during cancellation. + } } }, 10_000);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hypeddit.ts` around lines 771 - 779, Handle rejection from the asynchronous retry callback in the retryTimer setTimeout flow: wrap the best-effort page.click(Selectors.DW_DOWNLOAD_BUTTON) operation in rejection handling and ignore failures caused by cancellation or browser closure. Preserve the existing downloadGuid check, logging, and retry timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/downloadgater.ts`:
- Around line 699-702: Update the download flow around cancelPendingDownloadWait
and clickDownload so downloadCompletePromise has an attached rejection handler
before clickDownload can yield. Start both operations together and await them
with Promise.all(), preserving cancellation rejection behavior and preventing an
unhandled rejection while close() is pending.
In `@src/server.ts`:
- Around line 1191-1197: Update the cancellation-timeout flow around
runDownloadProcess(), cleanupCancelledJobDownloads(), and
jobQueue.releaseActive() so the shared download directory cannot be reused while
the cancelled task may still write. Keep the queue slot until task completion,
or, if releasing on timeout is unavoidable, schedule completion cleanup that is
isolated from artifacts created by the next job.
In `@webui/src/components/RemoteBrowserPanel.tsx`:
- Around line 672-682: Update currentRemotePointer in RemoteBrowserPanel to
store the pointer in canvas-relative or normalized remote coordinates instead of
viewport client coordinates. Recompute current client coordinates from the
latest canvas bounds immediately before clickRemote and movePointerBy dispatch,
preserving the pointer’s remote location across pan and zoom. Add panel-level
coverage verifying click and relative movement after both pan and zoom.
In `@webui/src/layouts/Layout.astro`:
- Around line 10-13: Update the viewport metadata in Layout.astro to remove
maximum-scale=1 and user-scalable=no, and remove the global html touch-action
restriction. Preserve gesture suppression only within .remote-browser-screen and
its viewport.
---
Outside diff comments:
In `@src/hypeddit.ts`:
- Around line 771-779: Handle rejection from the asynchronous retry callback in
the retryTimer setTimeout flow: wrap the best-effort
page.click(Selectors.DW_DOWNLOAD_BUTTON) operation in rejection handling and
ignore failures caused by cancellation or browser closure. Preserve the existing
downloadGuid check, logging, and retry timing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dbd98da-9241-4357-a9d1-c51ac601c329
📒 Files selected for processing (14)
src/downloadCleanup.test.tssrc/downloadCleanup.tssrc/downloadgater.tssrc/droploud.tssrc/gateCancellation.test.tssrc/hypeddit.tssrc/mypresskit.tssrc/server.tssrc/stillhype.tswebui/src/components/RemoteBrowserPanel.csswebui/src/components/RemoteBrowserPanel.tsxwebui/src/components/mobileRemoteControls.tswebui/src/components/remoteBrowser.test.tswebui/src/layouts/Layout.astro
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Cancelling an active gate could leave its provider task alive, retain the serialized queue slot, and leave incomplete download artifacts behind. Mobile remote-browser interactions could also move the pointer during taps and occasionally trigger the phone browser's native page zoom.
This change:
.crdownload/.partartifactsValidation:
bun test(140 passed)bun x tsc --noEmitinwebuigit diff --checkImplemented with GPT-5 through the Codex desktop agent harness.
Summary by CodeRabbit
New Features
Bug Fixes
Tests