@lovasoa

Refactor: split src/render.rs into a render/ module with a BodyRenderer trait and an explicit "bytes committed" state

This is a refinement of the "split render.rs" item in #1249, with a concrete target structure and a correctness-risk angle.

Why this is the file to split first

render.rs is the streaming response renderer. It is the one place where SQLPage can emit 200 OK + half a page + an error after the status line is already on the wire. The header phase relies on a "no body byte sent yet" contract so that an early error can still become a proper error response, and the body phase has to recover from mid-stream errors without that option. The logic that must keep all of this correct is smeared across 6 unrelated responsibilities in a single 1272-line file (wc -l src/render.rs = 1272 at HEAD 162f996), with almost no seams to unit-test any of it in isolation.

This is a maintainability + correctness-risk refactor. I am not reporting a reproduced bug. The point is that the current structure makes such bugs both easy to introduce and hard to test against.

1. One file, six responsibilities that share almost no state

• PageContext state machine: render.rs#L71-L83
• HeaderContext + 9 header-component handlers: status_code, http_header, cookie, redirect, json, csv, authentication, download, log
• Three body renderers: JsonBodyRenderer, CsvBodyRenderer, HtmlRenderContext
• The handlebars SplitTemplateRenderer
• Argon2 password verification
• base64 / percent-encoded data-URL decoding (inside download)

2. Duplicated dispatch over the output format

BodyRenderer is an enum (render.rs#L481), and AnyRenderBodyContext re-spells the same match { Html | Json | Csv } in five methods:

• handle_row#L514
• handle_error#L533 (plus a second match at #L528)
• finish_query#L543
• flush#L551
• close#L562

Adding a 4th output format means editing all five match sites and not forgetting any. Replacing the enum with a BodyRenderer trait collapses these to a single dynamic dispatch and makes a new format one new file.

3. Construction with side effects, in the wrong phase

HtmlRenderContext::new is a constructor that:

1. synthesizes the shell row,
2. emits the shell bytes to the client (render_start at #L818),
3. then replays buffered rows back through handle_row (#L829-L831).

So "construct the body renderer" already produces output and can fail after bytes are on the wire. That is exactly the situation the header-phase "no body sent yet" error contract is meant to avoid, and here it is happening inside what looks like a pure new. Shell resolution (which shell, with what data) and shell emission should be separable, with resolution being pure and testable.

4. Mid-stream error recovery spread across ~4 entry points in 2 files

• HeaderContext::handle_error#L137 (can still bubble up to a real error response, because no body is committed yet)
• AnyRenderBodyContext::handle_error#L521
• HtmlRenderContext::handle_error#L899
• the caller's nested-error fallback in http.rs#L109-L123

These carry implicit ordering contracts. As one example of the fragility: render_end for the current component is invoked both from close_component#L997 and from close#L1004, with nothing structural preventing a component's closing from running twice on an error path.

Proposed structure

Split into a render/ module:

• render/header.rs — HeaderContext + the header-component handlers
• render/body/{json,csv,html}.rs — each behind a BodyRenderer trait (replacing the enum in point 2)
• render/template.rs — SplitTemplateRenderer
• render/mod.rs — PageContext as thin orchestration

Plus:

• Separate shell resolution (a pure, testable resolve_shell(first_row, embedded)) from shell emission, fixing point 3.
• Make "bytes committed?" an explicit state on PageContext::Body instead of an implicit invariant.
• Route all mid-stream errors through one sink, with a guard so a component's render_end cannot run twice.

Risks and mitigations

This is a large code move, which is the main risk. Mitigation:

1. Extract the BodyRenderer trait first (behavior identical, enum -> trait), as an isolated diff.
2. Then move code into files, no logic changes.
3. Then introduce the explicit "committed" state and single error sink.

The existing rendering tests pin the exact output of templates and components, so any behavioral drift during the move is caught.

Expected win

• Adding an output format becomes one new file behind a trait, not five edits.
• Shell resolution becomes unit-testable without a live writer.
• A single, guarded mid-stream error path instead of four implicitly-ordered ones.