feat(display/http): Angular SSR through a resident engine/ts context#41
Conversation
Serve an Angular app with server-side rendering via the Go-native CoreTS engine — Angular's non-Node SSR contract (RequestHandlerFunction: (Request) => Response | null | Promise) with no Node.js sidecar. - engine/ts: add Engine.Load + persistent Context.Invoke (load a bundle once, call an exported function per request), retained module state, JSON arg/result marshalling, lifecycle cleanup, and Web Request/Response bridging (web.go). - display/http/framework: shared Loader/Renderer/Response boundary so framework/<name>/ (react, …) slot in later behind one contract. - display/http/framework/angular: the adapter — loads reqHandler once, translates the HTTP request (host, headers, body) to a Web Request, maps the returned Response (status, repeated headers, body, null → 404). - display/http: WithFramework mount option, static assets + legacy renderer fallback preserved. - Acceptance: server_example_test.go proves retained state across requests and null → 404 when consumed as a real caller. Co-Authored-By: Virgil <virgil@lethean.io>
📝 WalkthroughWalkthroughChangesServer-side rendering integration
Sequence Diagram(s)sequenceDiagram
participant HTTPClient
participant HTTPHandler
participant AngularRenderer
participant ResidentContext
participant AngularReqHandler
HTTPClient->>HTTPHandler: Send HTTP request
HTTPHandler->>AngularRenderer: Render request
AngularRenderer->>ResidentContext: Invoke reqHandler
ResidentContext->>AngularReqHandler: Dispatch WebRequest
AngularReqHandler-->>ResidentContext: Return WebResponse or nil
ResidentContext-->>AngularRenderer: Decode response
AngularRenderer-->>HTTPHandler: Return framework Response
HTTPHandler-->>HTTPClient: Write HTTP response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
|
![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
🧹 Nitpick comments (1)
go/display/http/framework/angular/server.go (1)
70-97: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffThroughput note:
r.muis held across the blockingInvoke.Holding
r.mufor the fullwebRequestFromHTTP+r.module.Invokepath serialises every SSR render through this renderer and also blocksCloseuntil an in-flight render completes. Combined with the single resident connection (eachInvokeis already serialised bycallMu), one slow render stalls all others. If concurrent SSR throughput becomes a requirement, consider a pool of resident contexts or narrowing the lock to theclosed/modulesnapshot only.🤖 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 `@go/display/http/framework/angular/server.go` around lines 70 - 97, The Render method currently holds r.mu across webRequestFromHTTP and the blocking r.module.Invoke call, serializing renders and delaying Close. Narrow the lock to snapshot and validate r.closed and r.module, then release it before request conversion and Invoke; preserve the closed-renderer error and use the captured module for the invocation.
🤖 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 `@go/display/http/framework/framework_test.go`:
- Around line 31-52: Add Good/Bad/Ugly triplet tests for the public Loader,
Renderer, and Response contracts in go/display/http/framework/framework_test.go,
using the existing TestFramework_Contracts and TestFramework_Response coverage
as context. Also add Good/Bad/Ugly triplets for WebRequest and WebResponse in
go/engine/ts/web_test.go, naming them TestWeb_WebRequest_{Good,Bad,Ugly} and
TestWeb_WebResponse_{Good,Bad,Ugly} and covering each contract’s relevant valid,
invalid, and edge behaviors.
In `@go/display/http/http_example_test.go`:
- Around line 15-17: Update ExampleWithFramework to invoke WithFramework with a
representative usage scenario and add an // Output: block containing the
deterministic expected output, so the example is executed and validated by Go’s
example tests.
In `@go/engine/ts/context.go`:
- Around line 290-301: Set a read deadline on each accepted resident connection
before reader.ReadString('\n') in loadContext, reusing its existing deadline
value; also update Invoke to derive and set a connection read deadline from ctx
or the call timeout before its blocking read. Preserve the existing cleanup and
error handling while ensuring both read paths cannot block indefinitely.
- Around line 133-149: Deregister each closed Context from the owning Engine’s
contexts map during the normal context-close path, while avoiding removal inside
Context.close() because Engine.Close invokes it under Engine.mu. Add a separate
Engine.mu-safe cleanup path that removes the matching context after close, and
ensure repeated Load/Close cycles do not retain closed *Context references.
---
Nitpick comments:
In `@go/display/http/framework/angular/server.go`:
- Around line 70-97: The Render method currently holds r.mu across
webRequestFromHTTP and the blocking r.module.Invoke call, serializing renders
and delaying Close. Narrow the lock to snapshot and validate r.closed and
r.module, then release it before request conversion and Invoke; preserve the
closed-renderer error and use the captured module for the invocation.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d6b1dd9b-bac0-4597-95a2-95ea1f8a3564
📒 Files selected for processing (18)
go/display/http/framework/angular/server.gogo/display/http/framework/angular/server_example_test.gogo/display/http/framework/angular/server_test.gogo/display/http/framework/framework.gogo/display/http/framework/framework_example_test.gogo/display/http/framework/framework_test.gogo/display/http/http.gogo/display/http/http_example_test.gogo/display/http/http_test.gogo/engine/ts/context.gogo/engine/ts/context_example_test.gogo/engine/ts/context_test.gogo/engine/ts/engine.gogo/engine/ts/engine_example_test.gogo/engine/ts/engine_test.gogo/engine/ts/web.gogo/engine/ts/web_example_test.gogo/engine/ts/web_test.go
| func TestFramework_Contracts(t *testing.T) { | ||
| var loader Loader = loaderContract{} | ||
| var renderer Renderer = rendererContract{} | ||
| context, err := loader.Load(core.Background(), "server.mjs") | ||
| core.AssertNoError(t, err) | ||
| core.AssertNotNil(t, context) | ||
| response, err := renderer.Render(core.Background(), &nethttp.Request{}) | ||
| core.AssertNoError(t, err) | ||
| core.AssertEqual(t, nethttp.StatusNoContent, response.Status) | ||
| core.AssertNoError(t, renderer.Close()) | ||
| } | ||
|
|
||
| func TestFramework_Response(t *testing.T) { | ||
| response := Response{ | ||
| Status: nethttp.StatusAccepted, | ||
| Header: nethttp.Header{"X-Framework": {"Angular"}}, | ||
| Body: []byte("<main>accepted</main>"), | ||
| } | ||
| core.AssertEqual(t, nethttp.StatusAccepted, response.Status) | ||
| core.AssertEqual(t, "Angular", response.Header.Get("X-Framework")) | ||
| core.AssertEqual(t, []byte("<main>accepted</main>"), response.Body) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required Good/Bad/Ugly test triplets for new public contracts.
go/display/http/framework/framework_test.go#L31-L52: addTestFramework_Loader_{Good,Bad,Ugly},TestFramework_Renderer_{Good,Bad,Ugly}, andTestFramework_Response_{Good,Bad,Ugly}coverage.go/engine/ts/web_test.go#L13-L37: addTestWeb_WebRequest_{Good,Bad,Ugly}andTestWeb_WebResponse_{Good,Bad,Ugly}coverage.
As per coding guidelines, “Public symbols in Go files must have a corresponding TestFoo_<Symbol>_{Good,Bad,Ugly} triplet test in foo_test.go”.
📍 Affects 2 files
go/display/http/framework/framework_test.go#L31-L52(this comment)go/engine/ts/web_test.go#L13-L37
🤖 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 `@go/display/http/framework/framework_test.go` around lines 31 - 52, Add
Good/Bad/Ugly triplet tests for the public Loader, Renderer, and Response
contracts in go/display/http/framework/framework_test.go, using the existing
TestFramework_Contracts and TestFramework_Response coverage as context. Also add
Good/Bad/Ugly triplets for WebRequest and WebResponse in
go/engine/ts/web_test.go, naming them TestWeb_WebRequest_{Good,Bad,Ugly} and
TestWeb_WebResponse_{Good,Bad,Ugly} and covering each contract’s relevant valid,
invalid, and edge behaviors.
Source: Coding guidelines
| func ExampleWithFramework() { | ||
| _ = WithFramework | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the WithFramework example executable.
It neither invokes WithFramework nor has an // Output: block, so it is not a runnable usage example.
Proposed fix
+import core "dappco.re/go"
+
func ExampleWithFramework() {
- _ = WithFramework
+ option := WithFramework(nil)
+ core.Println(option != nil)
+ // Output: true
}As per coding guidelines, “Add an Example with an // Output: block for every public feature.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func ExampleWithFramework() { | |
| _ = WithFramework | |
| } | |
| import core "dappco.re/go" | |
| func ExampleWithFramework() { | |
| option := WithFramework(nil) | |
| core.Println(option != nil) | |
| // Output: true | |
| } |
🤖 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 `@go/display/http/http_example_test.go` around lines 15 - 17, Update
ExampleWithFramework to invoke WithFramework with a representative usage
scenario and add an // Output: block containing the deterministic expected
output, so the example is executed and validated by Go’s example tests.
Source: Coding guidelines
| func (c *Context) close() error { | ||
| c.stateMu.Lock() | ||
| if c.closed { | ||
| err := c.closeErr | ||
| c.stateMu.Unlock() | ||
| return err | ||
| } | ||
| c.closed = true | ||
| connection := c.conn | ||
| service := c.service | ||
| identity := c.identity | ||
| workspace := c.workspace | ||
| c.conn = nil | ||
| c.reader = nil | ||
| c.service = nil | ||
| c.workspace = "" | ||
| c.stateMu.Unlock() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)go/engine/ts/context\.go$|engine|ts/context' || true
echo "== outline =="
ast-grep outline go/engine/ts/context.go --view compact || true
echo "== relevant loadContext/close/open sections =="
sed -n '280,360p' go/engine/ts/context.go
echo "---"
sed -n '120,175p' go/engine/ts/context.go
echo "== contexts usages =="
rg -n "contexts|loadContext|\.close\(|Close\(" go/engine/ts/context.goRepository: dAppCore/go-render
Length of output: 11183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Engine methods outline =="
ast-grep outline go/engine/ts/engine.go --view digest || true
echo "== Engine.go relevant sections =="
wc -l go/engine/ts/engine.go
sed -n '1,220p' go/engine/ts/engine.go
echo "--- contexts/delete references across ts package =="
rg -n "contexts|delete\(.*contexts|\.close\(" go/engine/ts -g '*.go'Repository: dAppCore/go-render
Length of output: 8994
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Engine.Close tail =="
sed -n '206,245p' go/engine/ts/engine.go
echo "== Test failure for repeated Load/Close =="
sed -n '220,260p' go/engine/ts/engine_test.go
echo "== Read-only lock-ordering model probe =="
python3 - <<'PY'
from pathlib import Path
def extract(block, pattern=''):
lines=open(block).read().splitlines()
for i,l in enumerate(lines,1):
if pattern in l or 'for context := range' in l:
print(f'{block}:{i}: {l}')
seg=lines[0]
for n,line in enumerate(lines):
if f'func {seg}' in line:
for row in lines[n:min(n+35,len(lines))]:
print(f'{block}:{n+1}: {row}')
extract('go/engine/ts/context.go', 'contexts')
extract('go/engine/ts/engine.go', 'contexts')
print('Engine.Lock before iteration and nulls after context.close')
print('Context.Close only owns c.stateMu; no Engine.mu acquisition in extracted context.close')
print('No e.contexts entry removal in extracted context.go (except e.contexts = nil in Engine.Close)')
PYRepository: dAppCore/go-render
Length of output: 2968
Deregister Context entries when closed
loadContext stores the new resident Context in e.contexts, but Context.close() only clears the context’s fields and never removes the map entry. Engine.Close is the only path that resets e.contexts, so repeated Load/Close cycles on the same engine leak closed *Context references. Moving the map removal into close() is not safe because Engine.Close already calls context.close() while holding Engine.mu; the deregistration must be done on a separate Engine.mu-safe path instead.
🤖 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 `@go/engine/ts/context.go` around lines 133 - 149, Deregister each closed
Context from the owning Engine’s contexts map during the normal context-close
path, while avoiding removal inside Context.close() because Engine.Close invokes
it under Engine.mu. Add a separate Engine.mu-safe cleanup path that removes the
matching context after close, and ensure repeated Load/Close cycles do not
retain closed *Context references.
| connection, err := listener.Accept() | ||
| if err != nil { | ||
| return nil, core.E("ts.Engine.Load", "accept resident call connection", err) | ||
| } | ||
| reader := core.NewBufReader(connection) | ||
| line, err := reader.ReadString('\n') | ||
| if err != nil { | ||
| if closeErr := connection.Close(); closeErr != nil { | ||
| core.Warn("TypeScript resident connection cleanup failed", "err", closeErr) | ||
| } | ||
| return nil, core.E("ts.Engine.Load", "read resident ready frame", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '^go/engine/ts/context\.go$' || true
echo
echo "Outline:"
ast-grep outline go/engine/ts/context.go || true
echo
echo "Relevant sections:"
wc -l go/engine/ts/context.go
sed -n '1,180p' go/engine/ts/context.go
sed -n '250,320p' go/engine/ts/context.goRepository: dAppCore/go-render
Length of output: 8227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Top-level imports/constants/variables:"
sed -n '1,40p' go/engine/ts/context.go
echo
echo "Engine loadContext complete:"
sed -n '187,335p' go/engine/ts/context.go
echo
echo "Search for deadlines and Conn operations in file:"
rg -n "SetDeadline|NewBuffer|NewBufReader|ReadString|ctx\.Err|connection|c\.conn|stateMu|callMu" go/engine/ts/context.goRepository: dAppCore/go-render
Length of output: 7946
Set read deadlines on accepted resident connections. The listener deadline does not carry over to either read path, so Invoke and loadContext can block indefinitely on reader.ReadString('\n'); Invoke also ignores ctx.Err() until after the read returns. Set a connection read deadline before each blocking read: reuse deadline for loadContext, and derive one from ctx/call timeout for Invoke.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 298-298: Define a constant instead of duplicating this literal "TypeScript resident connection cleanup failed" 3 times.
📍 Affects 1 file
go/engine/ts/context.go#L290-L301(this comment)go/engine/ts/context.go#L91-L97
🤖 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 `@go/engine/ts/context.go` around lines 290 - 301, Set a read deadline on each
accepted resident connection before reader.ReadString('\n') in loadContext,
reusing its existing deadline value; also update Invoke to derive and set a
connection read deadline from ctx or the call timeout before its blocking read.
Preserve the existing cleanup and error handling while ensuring both read paths
cannot block indefinitely.
1 participant
Serve an Angular app with SSR via the Go-native CoreTS engine — Angular's non-Node SSR contract (
RequestHandlerFunction: (Request) => Response | null | Promise), no Node.js sidecar. Foundation forframework/react/etc.What landed
engine/ts—Engine.Load+ persistentContext.Invoke: load a server bundle once, call an exported function per request. Retained module state, JSON arg/result marshalling, lifecycle cleanup, and WebRequest/Responsebridging (web.go).display/http/framework— sharedLoader/Renderer/Responseboundary soframework/<name>/slot in behind one contract.display/http/framework/angular— the adapter: loadsreqHandleronce, translates the HTTP request (host, headers, body) → a WebRequest, maps the returnedResponse(status, repeated headers, body,null→ 404).display/http—WithFrameworkmount option; static assets + legacy renderer fallback preserved.Acceptance proof
framework/angular/server_example_test.goconsumes the adapter as a real caller — retained state across requests,null→ 404:Gate
GOWORK=off go build ./...✓ ·go vet ./engine/ts/... ./display/http/...✓ · 33 tests green acrossengine/ts,display/http/framework,framework/angular.Refs: Angular non-Node SSR, RequestHandlerFunction.
🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
New Features
Bug Fixes