fix(http): restore web UI yield for layout templates (#101)#107
fix(http): restore web UI yield for layout templates (#101)#107renecannao wants to merge 1 commit into
Conversation
After the martini→chi migration, renderHTML used stdlib html/template without registering the yield FuncMap that layout.tmpl expects, so every /web/* route returned 500. Render content first, inject it via yield into the layout, and add unit tests that cover all templates and guard the yield contract.
📝 WalkthroughWalkthroughHTML rendering now caches content templates separately, loads the shared layout source once, and injects buffered content per request. New tests validate yield registration, representative output, and rendering across all web templates. ChangesHTML rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant renderHTML
participant ContentTemplateCache
participant LayoutSource
participant HTTPResponse
renderHTML->>ContentTemplateCache: getContentTemplate(name)
ContentTemplateCache-->>renderHTML: content template
renderHTML->>renderHTML: render content into buffer
renderHTML->>LayoutSource: load layout source once
LayoutSource-->>renderHTML: layout template source
renderHTML->>HTTPResponse: execute layout with buffered yield content
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![gemini-code-assist[bot]](https://cdn.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors HTML rendering in go/http/render.go to support dynamic content injection into a layout template via a {{yield}} function, and introduces a comprehensive test suite in go/http/render_test.go. The feedback suggests converting templateDir to a variable so tests can override it instead of modifying the global working directory with os.Chdir. Additionally, it is recommended to execute the layout template into a temporary buffer first to prevent sending partial HTTP responses in case of execution failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| layoutLoadErr error | ||
| ) | ||
|
|
||
| const templateDir = "resources" |
There was a problem hiding this comment.
Changing templateDir from a const to a var allows tests to override it with an absolute path, avoiding the need to use os.Chdir in tests (which is not thread-safe and can cause flaky tests).
| const templateDir = "resources" | |
| var templateDir = "resources" |
| w.Header().Set("Content-Type", "text/html; charset=UTF-8") | ||
| w.WriteHeader(status) | ||
| if err := t.Execute(w, data); err != nil { | ||
| _ = log.Errorf("Error executing template %s: %+v", name, err) | ||
| if err := layout.Execute(w, data); err != nil { | ||
| _ = log.Errorf("Error executing layout for template %s: %+v", name, err) | ||
| } |
There was a problem hiding this comment.
Executing the layout template directly to w after writing the status header can result in a partial/broken 200 OK response if a template execution error occurs midway.
To ensure robust error handling, consider executing the layout template into a temporary buffer first. If execution succeeds, write the headers and the buffer to the response; if it fails, you can safely return a 500 Internal Server Error.
| w.Header().Set("Content-Type", "text/html; charset=UTF-8") | |
| w.WriteHeader(status) | |
| if err := t.Execute(w, data); err != nil { | |
| _ = log.Errorf("Error executing template %s: %+v", name, err) | |
| if err := layout.Execute(w, data); err != nil { | |
| _ = log.Errorf("Error executing layout for template %s: %+v", name, err) | |
| } | |
| var buf bytes.Buffer | |
| if err := layout.Execute(&buf, data); err != nil { | |
| _ = log.Errorf("Error executing layout for template %s: %+v", name, err) | |
| http.Error(w, "Internal Server Error", http.StatusInternalServerError) | |
| return | |
| } | |
| w.Header().Set("Content-Type", "text/html; charset=UTF-8") | |
| w.WriteHeader(status) | |
| _, _ = buf.WriteTo(w) |
| func chdirToRepoRoot(t *testing.T) { | ||
| t.Helper() | ||
| wd, err := os.Getwd() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| dir := wd | ||
| for { | ||
| if _, err := os.Stat(filepath.Join(dir, "resources", "templates", "layout.tmpl")); err == nil { | ||
| if err := os.Chdir(dir); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| t.Cleanup(func() { _ = os.Chdir(wd) }) | ||
| return | ||
| } | ||
| parent := filepath.Dir(dir) | ||
| if parent == dir { | ||
| t.Fatal("could not find repo root containing resources/templates/layout.tmpl") | ||
| } | ||
| dir = parent | ||
| } | ||
| } |
There was a problem hiding this comment.
Using os.Chdir in tests changes the working directory of the entire process, which is a global state. This can cause race conditions and flaky test failures if other tests run concurrently or in parallel.
Instead of changing the working directory, we can find the repository root and override the package-level templateDir variable (after changing it to a var in render.go). Note that you will also need to update the calls to chdirToRepoRoot(t) to setTemplateDirToRepoRoot(t).
// setTemplateDirToRepoRoot finds the repository root and overrides templateDir
// so template paths resolve during tests without changing the process's working directory.
func setTemplateDirToRepoRoot(t *testing.T) {
t.Helper()
wd, err := os.Getwd()
if err != nil {
t.Fatal(err)
}
dir := wd
for {
resourcesPath := filepath.Join(dir, "resources")
if _, err := os.Stat(filepath.Join(resourcesPath, "templates", "layout.tmpl")); err == nil {
oldTemplateDir := templateDir
templateDir = resourcesPath
t.Cleanup(func() { templateDir = oldTemplateDir })
return
}
parent := filepath.Dir(dir)
if parent == dir {
t.Fatal("could not find repo root containing resources/templates/layout.tmpl")
}
dir = parent
}
}| // (everything except layout). Discovered from disk so new templates are covered. | ||
| func contentTemplateNames(t *testing.T) []string { | ||
| t.Helper() | ||
| matches, err := filepath.Glob(filepath.Join("resources", "templates", "*.tmpl")) |
There was a problem hiding this comment.
To support running tests without changing the global working directory, use templateDir instead of the hardcoded "resources" string when locating templates.
| matches, err := filepath.Glob(filepath.Join("resources", "templates", "*.tmpl")) | |
| matches, err := filepath.Glob(filepath.Join(templateDir, "templates", "*.tmpl")) |
| } | ||
|
|
||
| func TestRenderHTMLYield(t *testing.T) { | ||
| chdirToRepoRoot(t) |
There was a problem hiding this comment.
Update to use setTemplateDirToRepoRoot(t) to avoid changing the global working directory.
| chdirToRepoRoot(t) | |
| setTemplateDirToRepoRoot(t) |
| // TestLayoutRequiresYield guards the martini-contrib/render contract: layout.tmpl | ||
| // uses {{yield}} to inject page content. Parsing layout without that FuncMap must fail. | ||
| func TestLayoutRequiresYield(t *testing.T) { | ||
| chdirToRepoRoot(t) |
There was a problem hiding this comment.
Update to use setTemplateDirToRepoRoot(t) to avoid changing the global working directory.
| chdirToRepoRoot(t) | |
| setTemplateDirToRepoRoot(t) |
| func TestLayoutRequiresYield(t *testing.T) { | ||
| chdirToRepoRoot(t) | ||
|
|
||
| layoutPath := filepath.Join("resources", "templates", "layout.tmpl") |
There was a problem hiding this comment.
Use templateDir instead of the hardcoded "resources" string to ensure the layout path resolves correctly when templateDir is overridden.
| layoutPath := filepath.Join("resources", "templates", "layout.tmpl") | |
| layoutPath := filepath.Join(templateDir, "templates", "layout.tmpl") |
| } | ||
|
|
||
| func TestRenderHTMLAllWebTemplates(t *testing.T) { | ||
| chdirToRepoRoot(t) |
There was a problem hiding this comment.
Update to use setTemplateDirToRepoRoot(t) to avoid changing the global working directory.
| chdirToRepoRoot(t) | |
| setTemplateDirToRepoRoot(t) |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
go/http/render_test.go (1)
29-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid modifying the process-wide working directory in tests.
os.Chdirchanges the working directory globally for the entire test process. Ift.Parallel()is ever introduced to this package, concurrent tests will experience unpredictable file lookup failures when the directory changes underneath them.Consider refactoring
templateDirinrender.gofrom aconstto avar. This would allow tests to override the base path securely without mutating global process state.🤖 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/http/render_test.go` around lines 29 - 52, Refactor the test setup around chdirToRepoRoot and templateDir to avoid calling os.Chdir or restoring the process-wide working directory. Change templateDir in render.go to an overridable variable, and have tests temporarily assign the repository’s template path with cleanup that restores the original variable value.
🤖 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/http/render.go`:
- Around line 67-83: The template loading flow should avoid duplicate parsing
when concurrent requests miss the cache. In the cache-miss path around
contentTemplateCache, acquire the write lock before parsing, recheck
contentTemplateCache.m[name], return the existing template if found, otherwise
parse and cache the template while holding the lock, then unlock on every return
path.
---
Nitpick comments:
In `@go/http/render_test.go`:
- Around line 29-52: Refactor the test setup around chdirToRepoRoot and
templateDir to avoid calling os.Chdir or restoring the process-wide working
directory. Change templateDir in render.go to an overridable variable, and have
tests temporarily assign the repository’s template path with cleanup that
restores the original variable value.
🪄 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
Run ID: 59d9d436-ffb1-4a38-9ce5-10cdd578d5bd
📒 Files selected for processing (2)
go/http/render.gogo/http/render_test.go
| contentTemplateCache.RLock() | ||
| if t, ok := contentTemplateCache.m[name]; ok { | ||
| contentTemplateCache.RUnlock() | ||
| return t, nil | ||
| } | ||
| templateCache.RUnlock() | ||
| contentTemplateCache.RUnlock() | ||
|
|
||
| layoutPath := filepath.Join(templateDir, layoutFile+".tmpl") | ||
| tmplPath := filepath.Join(templateDir, name+".tmpl") | ||
| t, err := template.ParseFiles(layoutPath, tmplPath) | ||
| t, err := template.ParseFiles(tmplPath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| templateCache.Lock() | ||
| templateCache.m[name] = t | ||
| templateCache.Unlock() | ||
| contentTemplateCache.Lock() | ||
| contentTemplateCache.m[name] = t | ||
| contentTemplateCache.Unlock() | ||
| return t, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fix TOCTOU race condition in template caching.
If multiple requests concurrently request the same un-cached template, they will all bypass the read lock and perform duplicate disk I/O and parsing. They will then sequentially acquire the write lock and overwrite the cache entry.
Move the parsing logic inside the write lock and add a second check to complete the double-checked locking pattern.
🛠️ Proposed fix
contentTemplateCache.RLock()
if t, ok := contentTemplateCache.m[name]; ok {
contentTemplateCache.RUnlock()
return t, nil
}
contentTemplateCache.RUnlock()
+ contentTemplateCache.Lock()
+ defer contentTemplateCache.Unlock()
+
+ if t, ok := contentTemplateCache.m[name]; ok {
+ return t, nil
+ }
+
tmplPath := filepath.Join(templateDir, name+".tmpl")
t, err := template.ParseFiles(tmplPath)
if err != nil {
return nil, err
}
- contentTemplateCache.Lock()
contentTemplateCache.m[name] = t
- contentTemplateCache.Unlock()
return t, nil📝 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.
| contentTemplateCache.RLock() | |
| if t, ok := contentTemplateCache.m[name]; ok { | |
| contentTemplateCache.RUnlock() | |
| return t, nil | |
| } | |
| templateCache.RUnlock() | |
| contentTemplateCache.RUnlock() | |
| layoutPath := filepath.Join(templateDir, layoutFile+".tmpl") | |
| tmplPath := filepath.Join(templateDir, name+".tmpl") | |
| t, err := template.ParseFiles(layoutPath, tmplPath) | |
| t, err := template.ParseFiles(tmplPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| templateCache.Lock() | |
| templateCache.m[name] = t | |
| templateCache.Unlock() | |
| contentTemplateCache.Lock() | |
| contentTemplateCache.m[name] = t | |
| contentTemplateCache.Unlock() | |
| return t, nil | |
| contentTemplateCache.RLock() | |
| if t, ok := contentTemplateCache.m[name]; ok { | |
| contentTemplateCache.RUnlock() | |
| return t, nil | |
| } | |
| contentTemplateCache.RUnlock() | |
| contentTemplateCache.Lock() | |
| defer contentTemplateCache.Unlock() | |
| if t, ok := contentTemplateCache.m[name]; ok { | |
| return t, nil | |
| } | |
| tmplPath := filepath.Join(templateDir, name+".tmpl") | |
| t, err := template.ParseFiles(tmplPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| contentTemplateCache.m[name] = t | |
| return t, nil |
🤖 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/http/render.go` around lines 67 - 83, The template loading flow should
avoid duplicate parsing when concurrent requests miss the cache. In the
cache-miss path around contentTemplateCache, acquire the write lock before
parsing, recheck contentTemplateCache.m[name], return the existing template if
found, otherwise parse and cache the template while holding the lock, then
unlock on every return path.
1 participant
Summary
/web/*route returning HTTP 500 after fresh install (function "yield" not defined) — closes UI not working after fresh install #101{{yield}}contract inrenderHTMLusing stdlibhtml/templatescript/test-unit)Root cause
go/http/render.gowas rewritten to use Go'shtml/templateduring the martini→chi migration, butresources/templates/layout.tmplstill calls{{yield}}. That function was previously provided bymartini-contrib/renderand was never re-registered.Fix
yieldFuncMap that injects that HTMLTest plan
go test ./go/http/ -run 'TestRenderHTML|TestLayoutRequiresYield' -vresources/templates/render with layout wrapperTestLayoutRequiresYieldfails parse withoutyieldFuncMap and succeeds with it./orchestrator --config=... httpthen open/web/clusters,/web/home,/web/about— expect 200 HTML, not 500Preventing recurrence
Unit tests (run by CI
script/test-unit) now:{{yield}}and that parsing layout without the FuncMap fails with the exact error from UI not working after fresh install #101No full browser/e2e suite is required for this class of bug — template parse/execute coverage is enough and fast.
Summary by CodeRabbit
Bug Fixes
Tests