migrate old file streaming to new modern interface (w/ flow control)#3096
migrate old file streaming to new modern interface (w/ flow control)#3096
Conversation
WalkthroughThis PR refactors the file streaming architecture by transitioning from channel-based RPC streaming commands to a stream-broker approach. Key changes include introducing Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying waveterm with
|
| Latest commit: |
09b4f7c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5cd491ac.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-remove-old-filestream.waveterm.pages.dev |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThe PR migrates from the old file streaming interface to a new modern interface with flow control. The changes are well-structured:
Key Changes Verified
Files Reviewed (18 files)
Reviewed by minimax-m2.5-20260211 · 1,193,623 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/wsh/cmd/wshcmd-connserver.go (1)
186-199:⚠️ Potential issue | 🔴 CriticalReturn
bareRouteIdinstead ofrouteIdfromsetupConnServerRpcClientWithRouter.The function creates two route IDs with distinct purposes:
bareRouteId(line 193): Random proc route for the bare client making RPC requestsrouteId(line 187): Connection route for the connserver receiving requestsLine 199 returns
routeId, but the callers at lines 239 and 364 assign the return value tobareRouteIdand pass it asRpcClientRouteIdfor stream reader operations. This variable is then used asreaderRouteIdinCreateStreamReader()calls (wshremote_file.go:174), where it represents the route of the client making the request. The reader route should be the bare client's route, not the connection route.Fix
- return connServerClient, routeId, nil + return connServerClient, bareRouteId, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/wsh/cmd/wshcmd-connserver.go` around lines 186 - 199, In setupConnServerRpcClientWithRouter, the function currently returns routeId but callers expect the bare client's route; change the returned route value to bareRouteId so callers receive the bare client's route ID. Update the return statement in setupConnServerRpcClientWithRouter to return bareRouteId (the identifier created as bareRouteId and used to register bareClient) instead of routeId, ensuring callers that assign the second return to bareRouteId/readerRouteId get the correct client route for CreateStreamReader and related operations.
🧹 Nitpick comments (1)
frontend/app/view/preview/preview-directory.tsx (1)
589-617: Clean migration to streaming directory listing.The implementation correctly:
- Uses
FileListStreamCommandwith async iteration- Safely handles optional
chunk?.fileinfo- Properly conditionally adds ".." based on
finfo.dir !== finfo.path(per theFileInfotype whereDirequalsPathfor root directories)- Adds helpful error logging
One consideration: the async generator stream isn't explicitly cancelled if the component unmounts mid-stream. In practice, this is often acceptable since the stream will naturally complete or error, but for very long directory listings you might want to use an
AbortControllerpattern or check a mounted flag.♻️ Optional: Add cancellation support for large directories
useEffect( () => fireAndForget(async () => { const entries: FileInfo[] = []; + let cancelled = false; try { const remotePath = await model.formatRemoteUri(dirPath, globalStore.get); const stream = env.rpc.FileListStreamCommand(TabRpcClient, { path: remotePath }, null); for await (const chunk of stream) { + if (cancelled) break; if (chunk?.fileinfo) { entries.push(...chunk.fileinfo); } } // ... rest of logic } catch (e) { + if (cancelled) return; console.error("Directory Read Error", e); // ... } + if (cancelled) return; setUnfilteredData(entries); - }), + }), + return () => { cancelled = true; }; [conn, dirPath, refreshVersion] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview-directory.tsx` around lines 589 - 617, The directory streaming loop using FileListStreamCommand can continue after the component unmounts; add cancellation so we don't call setUnfilteredData/setErrorMsg on an unmounted component: create an AbortController (or a `let cancelled = false` flag) inside the effect that calls model.formatRemoteUri and env.rpc.FileListStreamCommand, pass the abort signal to the RPC if supported or break the for-await loop when aborted, and in the effect cleanup abort the controller (or set cancelled = true); ensure before pushing entries or calling setUnfilteredData/setErrorMsg you check the abort/cancelled flag. Use the existing symbols FileListStreamCommand, stream, setUnfilteredData, setErrorMsg, model.formatRemoteUri and the enclosing useEffect to locate where to add the controller/flag and cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/wsh/cmd/wshcmd-connserver.go`:
- Around line 186-199: In setupConnServerRpcClientWithRouter, the function
currently returns routeId but callers expect the bare client's route; change the
returned route value to bareRouteId so callers receive the bare client's route
ID. Update the return statement in setupConnServerRpcClientWithRouter to return
bareRouteId (the identifier created as bareRouteId and used to register
bareClient) instead of routeId, ensuring callers that assign the second return
to bareRouteId/readerRouteId get the correct client route for CreateStreamReader
and related operations.
---
Nitpick comments:
In `@frontend/app/view/preview/preview-directory.tsx`:
- Around line 589-617: The directory streaming loop using FileListStreamCommand
can continue after the component unmounts; add cancellation so we don't call
setUnfilteredData/setErrorMsg on an unmounted component: create an
AbortController (or a `let cancelled = false` flag) inside the effect that calls
model.formatRemoteUri and env.rpc.FileListStreamCommand, pass the abort signal
to the RPC if supported or break the for-await loop when aborted, and in the
effect cleanup abort the controller (or set cancelled = true); ensure before
pushing entries or calling setUnfilteredData/setErrorMsg you check the
abort/cancelled flag. Use the existing symbols FileListStreamCommand, stream,
setUnfilteredData, setErrorMsg, model.formatRemoteUri and the enclosing
useEffect to locate where to add the controller/flag and cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e43edeb-c402-4904-9967-37870a07f6af
📒 Files selected for processing (18)
cmd/server/main-server.gocmd/wsh/cmd/wshcmd-connserver.gocmd/wsh/cmd/wshcmd-file-util.gocmd/wsh/cmd/wshcmd-file.gocmd/wsh/cmd/wshcmd-root.gofrontend/app/store/wshclientapi.tsfrontend/app/view/preview/preview-directory.tsxfrontend/app/view/preview/previewenv.tsfrontend/preview/mock/mockfilesystem.tsfrontend/preview/mock/mockwaveenv.test.tsfrontend/preview/mock/mockwaveenv.tsfrontend/types/gotypes.d.tspkg/remote/fileshare/fsutil/fsutil.gopkg/remote/fileshare/wshfs/wshfs.gopkg/wshrpc/wshclient/wshclient.gopkg/wshrpc/wshremote/wshremote_file.gopkg/wshrpc/wshrpctypes_file.gopkg/wshrpc/wshserver/wshserver.go
💤 Files with no reviewable changes (7)
- frontend/types/gotypes.d.ts
- cmd/wsh/cmd/wshcmd-file.go
- pkg/wshrpc/wshserver/wshserver.go
- pkg/wshrpc/wshrpctypes_file.go
- pkg/wshrpc/wshclient/wshclient.go
- frontend/app/store/wshclientapi.ts
- pkg/remote/fileshare/fsutil/fsutil.go
1 participant
No description provided.