chore(dev): add spa fallback plugin and improve grpc stream reconnection logic#2173
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
Adding 500ms timeout to the create session logic is definitely not the right way to do it, it should be returned with dispatcher.start logic, it can return a promise for example. Delaying session callback by 500ms is not stable and would take very long. And second thing is that you do not need to wait for AccountRecover events as there is no AccountRecover in createSession. |
You are absolutely right. A setTimeout of 500ms is a "magic number" hack, and dispatcher.start should definitely return a Promise to handle this cleanly. Also, thanks for the correction regarding AccountRecover in createSession—I was definitely tracing the wrong event flow there. To give you some context on why this "dirty" workaround was added: The 500ms timeout wasn't meant to be the final architectural fix; it was a diagnostic probe. It forced the system to take a breath, bypassed the race condition, and finally allowed me to log in and access my Space in the browser. Without it, I was completely locked out of the web environment. Moving forward: I will revert the data.ts (500ms) commit from this PR right now so we can move forward with merging the Vite & SPA Fallback improvements. (Those are solid DX upgrades and shouldn't be blocked by this). Are you guys planning to tackle the dispatcher Promise refactor soon to fix this web-dev login lock-out, or should I attempt it in a separate PR? Let me know. |
|
You can do it by yourself if you want, but i still did not get which race condition you are referring to, I'm using dev mode all the time and the only problem now with Vite HMR is that it sometimes reloads incorrectly when agent is doing a lot of edits at the same time. |
@ra3orblade I took your advice and pushed an update to address this properly:
This completely eliminates the arbitrary delay while guaranteeing the stream is ready before the session continues. Let me know if the Promise implementation looks solid to you! |
ra3orblade
left a comment
There was a problem hiding this comment.
Review
The core idea is solid — converting dispatcher.startStream() to a Promise is much better than the 500 ms delay mentioned in the description (which is now stale). A few issues to address before merging.
🔴 Blockers
1. vite.web.config.ts:339 — broken catch handler
try { fs.rmdirSync(path.resolve(__dirname, 'dist-web/dist')); }
catch (e) { console.error("[ProtobufCjsPlugin] Error evaluating exports for " + id + ":", e);}- This is in the
move-htmlplugin, notProtobufCjsPlugin— the log prefix is wrong. idis not in scope incloseBundle(). IfrmdirSyncever throws, the catch itself throwsReferenceError, defeating the try/catch.
Looks like a stray copy-paste. Restore the original silent catch {} or write a proper message:
catch (e) { console.error('[move-html] Failed to remove dist-web/dist:', e); }2. vite.config.ts — spaFallbackPlugin regresses static asset serving from dist/
The old devServerPlugin served arbitrary files from dist/ (e.g. dist/tabs.html, dist/workers/pdf.worker.mjs, dist/font/...) when requested at root URLs.
The new plugin only bypasses /@, /node_modules/, /src/, /dist/, /api/. Everything else returns index.html. Since:
electron.js:1656loads tabs in dev viahttp://localhost:${port}/tabs.htmlsrc/ts/component/util/media/pdf.tsx:7setspdfjs.GlobalWorkerOptions.workerSrc = './workers/pdf.worker.mjs'(resolves to/workers/...from/)- font files load from
/font/...
…and vite.config.ts:21 has publicDir: false, so Vite's built-in static handler doesn't catch them either. Result: requests to /tabs.html or /workers/pdf.worker.mjs now return the SPA index.html instead of the actual file. Tabs window and PDF rendering will break in dev.
Fix options:
- Keep the old
dist/fallback logic in addition to the SPA fallback (try-disk-then-SPA), or - Add explicit bypasses for
/tabs.html,/workers/,/font/, plus static-serve for those paths.
🟡 Smaller issues
3. vite.config.ts — stale JSDoc. The comment above spaFallbackPlugin still describes the old devServerPlugin behavior.
4. dispatcher.ts:79–129 — Promise can never reject. Every event path (metadata, data, status, end, missing token) resolves. Callers using await can't distinguish "stream established" from "stream failed" or "no token". Either reject on !S.Auth.token / status.code != 0, or document that the promise always resolves.
**5. dispatcher.ts:159 (in reconnect()) — this.startStream() returned Promise is silently discarded. Add void this.startStream(); or a brief comment to make it intentional.
6. data.ts:434–443 — duplicated callBack?.(message). Cleaner as a single trailing call:
C.WalletCreateSession(phrase, key, token, async (message: any) => {
if (!message.error.code) {
S.Auth.tokenSet(message.token);
S.Auth.appTokenSet(message.appToken);
await dispatcher.startStream();
};
callBack?.(message);
});7. PR description is outdated. Still claims a "500 ms delay" — please update to reflect the Promise-based fix actually shipped in 518fb52.
✅ Looks good
- Adding
service_grpc_web_pb.jstooptimizeDeps.include— clean, targeted fix. - Promise-based replacement for the startup race is the right approach; resolving on
metadatais the correct readiness signal for grpc-web.
…ise logic and cleanup callbacks
Thanks for the thorough review! You caught some critical regressions, especially with the static asset serving in the SPA fallback. I've addressed all your points in the latest commit:
I've also updated the PR description to remove the stale "500ms delay" mentions. Ready for another look! |
|
Good, thanks, I will merge after next release. |
|
Reverted in 64fd938 — sorry, this broke 1. Stale
|
Overview
This PR introduces essential developer experience (DX) and stability improvements for the Anytype web environment, specifically addressing routing issues and gRPC stream race conditions.
What's Changed
SPA Fallback Plugin (
vite.config.ts):Implemented a robust
spaFallbackPluginto handle client-side routing and deep links. It uses a "try-disk-then-SPA" approach: it first attempts to serve static assets from thedist/directory (ensuring/tabs.html, PDF workers, and fonts continue to work) before falling back toindex.htmlfor SPA routes.Web Build Exports (
vite.web.config.ts):Added
service_grpc_web_pb.jsto the external exports list and improved themove-htmlplugin's cleanup logic. This prevents chunking errors during protobuf resolution in web builds and ensures a cleaner build directory.GRPC Stream Connection Logic (
dispatcher.ts&data.ts):Refactored
dispatcher.startStream()to return aPromise<void>that resolves when the gRPC event stream is established (or hits a safe fallback timeout). This replaces previous arbitrary delays with a structured readiness signal, ensuring session callbacks fire only when the stream is ready to receive events.Impact
These changes make the web-dev server much more reliable, fix broken static assets in dev mode, and eliminate intermittent race conditions during login/session recovery.