feat: explorer web live refresh - #1698
Conversation
The path-containment merge makes readAssembly async (buildConstructTreeAsync). Await it in the /tree and /policy-validation handlers and narrow the ApiOptions.readAssembly seam to Promise so the request path no longer blocks on a synchronous read.
Server-side SSE hub: tracks connected browsers and broadcasts a content-free assembly-changed signal so clients re-fetch through the existing GET endpoints. Evicts a client on request close or socket error so a vanished client is never written to.
startWebServer resolves the assembly dir once and shares it with the read endpoints and the watcher, registers GET /api/events (SseBroadcaster), and starts the assembly watcher whose onChange broadcasts an assembly-changed signal. stop() tears down the watcher then the broadcaster then the server.
The SPA opens an EventSource to /api/events and re-fetches the tree and violations on each assembly-changed signal, keeping the last good render on a transient read. The event name lives in the shared protocol contract so the server broadcaster and the client subscriber share one constant.
- Type the SSE event name (SseEventName) instead of broadcast(string). - Route watcher errors through the command IoHost (explore.ts), with safe Error extraction; stderr default in the lib. - Document that the SSE data line is required for EventSource dispatch; drop the broadcast try/catch (a write to a dead socket returns false, never throws). - Server integration test asserts via the shared constant.
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Failure
rix0rrr
left a comment
rix0rrr
left a comment
There was a problem hiding this comment.
The event carries no payload. The server holds no assembly state, so the client just re-reads /api/tree and /api/policy-validation on receipt.
Nice! Exactly what I would have done
| // Live-refresh stream: browsers subscribe here and re-fetch when the assembly | ||
| // changes. Registered before the /api catch-all so it is not treated as unknown. | ||
| const events = new SseBroadcaster(); | ||
| app.get('/api/events', events.handle); |
There was a problem hiding this comment.
This only works if events.handle doesn't use any this references.
It's a bit hard to explain and different from all other programming languages, but this is dynamically bound in JavaScript, and it only gets a value at the exact moment when you call a function.
If you "take" a function from an object and pass it around, this is unbound and when you call a function without an object it gets bound to the global object.
Now, it might be that that's not a problem because events.handle is a closure that never uses this, but I can't tell that from this call site, so this looks supicious. As a rule:
I would like to be able to reason about things locally. If they look locally correct, they should be globally correct. This might still be globally correct, but it looks locally incorrect, so breaks my heuristics.
Use the following instead:
// This bakes the value of "this" into the function
app.get('/api/events', events.handle.bind(events));I thought we had a linter rule for this... 🤔
There was a problem hiding this comment.
fixed, handle is now a normal method and I bind it at the call site: app.get('/api/events', events.handle.bind(events)). I didn't see a linter rule but I do agree anyway.
| url: `http://${host}:${port}`, | ||
| stop: () => { | ||
| if (stopped) return Promise.resolve(); | ||
| url: `http://127.0.0.1:${port}`, |
There was a problem hiding this comment.
Use localhost instead. On some operating systems it uses a faster network path (that skips a few layers), and it can't be wrong even if IPv4 is turned off.
There was a problem hiding this comment.
Also -- why hardcode this in a lot of places? Why not pass our preferred value in the host as an argument, and keep it deduplicated and easy to change?
There was a problem hiding this comment.
will fix to be localhost instead. I was trying to clean up options that we didn't need/ never used, but I can absolutely make this an argument as localhost to make it easy to change.
| * registers the client, removing it when the request closes or the socket | ||
| * errors so a vanished client is never written to. | ||
| */ | ||
| public readonly handle = (req: Request, res: Response): void => { |
There was a problem hiding this comment.
Why not just function syntax?
public handle(req: Request, res: Response): void {
// ...
}I think I have this comment in a lot of places, where you use function expression syntax instead of function declaration syntax. I personally find function declaration syntax a lot easier to grok, and I'd prefer we use that as much as possible.
To be frank I'm not even sure whether this binds the this that we think it does. I guess it has to because otherwise TypeScript would complain?
Hmm, thinking about how this must work, I think I see why this gets bound correctly and you also don't need the .bind() if you write it like this.
Still, when I look at the usage site I don't know any of that, so it still looks sus.
There was a problem hiding this comment.
Switched to function-declaration syntax. Now that it's a real method I bind it explicitly at the call site (see the /api/events thread).
Make SseBroadcaster.handle a function-declaration method instead of an arrow class field, and bind it where it is registered on /api/events so this is preserved and the call site is correct on its own. Addresses PR review feedback.
GitHub Actions
Failure
GitHub Actions
Failure
…and template Rebased aws#1706 onto the updated feat/cdk-explorer (now carrying feat/cdk-lsp) as a single integration commit. Adds source/template/tree navigation with syntax highlighting, a YAML template view, and a file picker, and serves each assembly read under the Toolkit read lock via the factory core extracted in aws#1715. Coexists with the SSE live-refresh from aws#1698: the reload effect and the navigation state share the same App shell.
2 participants
Adds live refresh to the cdk explore web UI. When the cloud assembly on disk changes, the server pushes an event and the SPA re-fetches, so the construct tree and violations stay current without a manual reload.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license