Repository navigation
Conversation
github-actions
Bot
added
the
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: |
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This works. Using opencode-drive with the simulated model, I made one request of the TUI's location sync fail with a 500 while the folder still existed (a test-only server patch, applied in both checkouts). The base replaced the session prompt with the false Session location unavailable dialog; with this PR the prompt stays. Screenshots are below.
The new test fails on the base and passes here. prompt-move and the test/context suites pass (94/94), and typecheck in packages/tui is clean. A missing folder still shows the dialog: location.get returns the server's typed LocationNotFoundError, which keeps its _tag through the TUI's location sync, matching how the web app already decides this.
The public API is unchanged.
One small point: unlike the web app, a failure that isn't LocationNotFoundError is now dropped and never retried. If it doesn't clear, the session can run with partly missing location data until the next reconnect or location change, with nothing shown to the user. That's still better than a false blocking dialog. #55258 adds a bounded retry for the same symptom, and the two touch the same code, so they could be combined.
Also see the inline note on the test's fixed sleep.
Fixes #55250
Related: #49352, #54943, #55331, #55035
| fixture.location.set({ directory }) | ||
| await fixture.app.waitFor(() => fixture.reads.includes(directory)) | ||
| // Let the sync rejection settle through LocationProvider's catch. | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) |
There was a problem hiding this comment.
This 100ms sleep is a timing guess: if the rejection takes longer to reach the catch, the test passes without checking anything. Could you wait until the sync has actually settled instead? For example, have the fixture record when the 503 response is served and await a microtask after that, or wait on a condition the way the not-found test does with waitFor.
There was a problem hiding this comment.
Good catch — replaced the fixed 100ms sleep with await Bun.sleep(0) after the request is observed. Yielding to the event loop guarantees the full microtask queue (fetch decode → sync rejection → provider catch) has drained before asserting, so the test can't pass vacuously. Red-green re-verified: fails on the pre-fix file, passes with the fix. Pushed as a2c34b9.
There was a problem hiding this comment.
Thanks, that addresses it. I ran the updated test against the base (pre-fix) code three times: the transient-failure case failed every time. So with Bun.sleep(0) after the request is seen, the assertion runs after the rejection is handled and still catches the bug.
|
Picking up the bot's suggestion from #57532 (auto-closed as a duplicate of this PR). This PR and #55258 each cover one half of the problem. #55258 documents its own limitation: "a location that stays unreachable past the retry window (~1.5s) still surfaces the recovery prompt, and a genuinely missing directory takes that long to report." Combining both halves — and splitting the location state into Two smaller things that belong with it:
Regression tests:
If a single combined PR is preferred, the implementation is on |
|
Merged PR #57594 supersedes this one — it landed the same behaviour (only the server's typed Two things are worth adding on top of it, both of which are now open:
No need to reconcile these with anything here — they are additive and touch different files. |
Issue for this PR
Closes #43415 (reported as duplicate in #46322)
Type of change
What does this PR do?
The TUI mounted the blocking "Session location unavailable" recovery dialog after transient location-sync failures (HTTP 503, timeouts, reconnect races), even though the folder still existed.
LocationProvider.syncinpackages/tui/src/context/location.tsxset the location error on every sync rejection. The server only throws the typedLocationNotFoundErrorwhen the folder genuinely does not resolve (packages/server/src/location.ts); the generated client preserves_tagon declared-error rejections, and the web client (packages/app/src/workspaces/location.tsx) already gates onisLocationNotFoundError. The TUI did not, so any failed sync replaced the prompt with the recovery dialog.The fix gates
setErroronisLocationNotFoundError(cause). Transient failures no longer mount recovery and self-heal through the existing re-sync triggers (location change,server.connected). Genuinely missing folders behave exactly as before.How did you verify your code works?
Added
packages/tui/test/cli/tui/location-error.test.tsx(TUI fixture harness):{message}rejection leaveslocation.errorundefined (fails before the fix, passes after — confirmed viagit stash){_tag: LocationNotFoundError}rejection setslocation.errorwith the directory (passes before and after)Also ran the existing
test/cli/tui/prompt-move.test.tsx(19 pass, 0 fail) andbun run typecheck(tsgo -b) inpackages/tui, clean.Screenshots / recordings
No visual change: the dialog renders exactly as before for genuinely missing folders. This change only narrows when it mounts, which the regression test above covers.
Checklist
Added by the opencode bot (AI-generated).
Fixes #55250
Related: #49352, #54943, #55331, #55035