Repository navigation
refactor: split codegen/preview.zig into focused sub-modules (behavior-preserving) - #547
Conversation
…r-preserving) Pure extraction of the 1310-line preview-mode codegen string-template module into a `preview/` sub-package + thin barrel. Every `pub const` keeps its original name/identity; `preview.<Name>` call sites unchanged. Sub-modules: - preview/wasm_workaround.zig Zig 0.16 wasm32-emscripten panic/debug-io - preview/helpers.zig clock/getenv helper + raylib PBO bridges - preview/loop.zig loop-backend setup/readback/heartbeat/input - preview/callback.zig callback-backend init/cleanup/heartbeat - preview/sokol_gl.zig sokol GL PBO readback - preview/sokol_d3d11.zig sokol D3D11 staging-texture readback - preview/sokol_metal.zig sokol Metal/IOSurface readback (Path A) - preview/editor.zig editor-preview codegen (labelle-studio) Byte-identical output: golden suite (main_zig_tests) + preview_mode_tests green, no golden file changed. zig build + zig fmt --check clean on 0.16.0. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe monolithic ChangesPreview codegen modularization
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors src/codegen/preview.zig by splitting the large ~1300-line module into several smaller, focused sub-modules under the preview/ directory, including wasm_workaround.zig, helpers.zig, loop.zig, callback.zig, sokol_gl.zig, sokol_d3d11.zig, sokol_metal.zig, and editor.zig. The original preview.zig file has been converted into a thin barrel file that re-exports all public constants, preserving behavior and keeping existing call sites unchanged. There are no review comments to evaluate, and I have no feedback to provide.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/codegen/preview/helpers.zig (1)
29-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment no longer matches the body it documents.
The
///doc comment (lines 29-38) advertises "Raw GL externs + constants" and "GL 2.1 / 3.3 core values", but the actualPREVIEW_READBACK_HELPERSstring only containsengine.Previewbridge wrapper functions — no GL externs or constants at all. The inline comment inside the string (lines 41-47) correctly describes this bridge-only design ("All GL state + PBO ring... now lives in the external labelle-raylib package"), which suggests the outer doc comment is stale from before that refactor and wasn't updated when this content was carried into the new sub-module.✏️ Suggested doc fix
-/// Raw GL externs + constants needed for the raylib desktop PBO-based -/// async readback path. Raylib links the platform's OpenGL loader on -/// desktop (CGL / GLX / WGL), so PBO entry points are present as -/// regular `extern "c"` symbols — `@extern` here resolves them at link -/// time without any extra dependency. -/// -/// Concatenated into `module_vars` for raylib-desktop only. Other +/// Tiny `*anyopaque`-boundary bridge functions wrapping `engine.Preview` +/// readback methods (PBO + IOSurface variants) for the raylib desktop +/// backend. All actual GL state / PBO ring / IOSurface machinery now +/// lives in the external labelle-raylib package. +/// +/// Concatenated into `module_vars` for raylib-desktop only. Other /// loop backends (sdl/bgfx/wgpu) don't use these and don't ship them -/// (their readback story is a separate ticket). The constants are GL -/// 2.1 / 3.3 core values — stable across drivers and platforms. +/// (their readback story is a separate ticket).🤖 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 `@src/codegen/preview/helpers.zig` around lines 29 - 77, Update the doc comment above PREVIEW_READBACK_HELPERS so it matches the current body: it no longer contains raw GL externs/constants, only the bridge wrappers around engine.Preview methods. Remove the stale GL 2.1/3.3 wording and describe this constant as the preview readback bridge helpers used by the raylib-desktop path, keeping the comment aligned with the existing inline notes and the _preview_pbo_* bridge functions.src/codegen/preview/callback.zig (1)
60-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIndentation-vs-doc-comment mismatch (cosmetic).
The doc comment says this heartbeat has "one extra indent level vs. the loop variant", but the string body actually starts at 4 spaces here vs. 8 spaces in
PREVIEW_HEARTBEAT_LOOP— i.e. the loop variant has more indentation, not less. Sincezig fmtnormalizes the final generated output, this has no functional impact — just a confusing comment for future maintainers reasoning about paste-site indentation.🤖 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 `@src/codegen/preview/callback.zig` around lines 60 - 80, The doc comment on PREVIEW_HEARTBEAT_CALLBACK describes the indentation relationship backwards relative to PREVIEW_HEARTBEAT_LOOP, so update the comment to match the actual generated string bodies. Keep the explanation aligned with the callback generator’s real paste-site indentation and make sure the comparison to the loop variant is stated consistently in PREVIEW_HEARTBEAT_CALLBACK and the related PREVIEW_HEARTBEAT_LOOP constant.src/codegen/preview/loop.zig (1)
39-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comments describe an older, more elaborate PBO-ring design than the code below actually implements.
The doc block above
PREVIEW_READBACK_SETUP(lines 39-57) still talks about "the PBO ring + CPU staging buffer locals thatPREVIEW_READBACK_LOOPuses" and a deferred "GL/SHM teardown" with an_gl_delete_bufferssymbol — none of which appear in the actual template body (lines 58-77), which just wires bridge functions intowindow.preview_pboand deferswindow.preview_pbo.deinit(). Likewise, the doc abovePREVIEW_READBACK_LOOP(lines 133-165) extensively describes a "3-deep PBO ring" /glMapBuffer/ resize-handling algorithm that the actual body (lines 166-172) no longer implements — it just callswindow.preview_pbo.frame(), delegating everything to the backend package per thelabelle-assembler#140refactor mentioned inline at line 59-62.These are internal doc comments only (no functional impact), but they're actively misleading about where the readback logic now lives, which could send a future maintainer down the wrong path when debugging.
🤖 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 `@src/codegen/preview/loop.zig` around lines 39 - 77, Update the doc comments for PREVIEW_READBACK_SETUP and PREVIEW_READBACK_LOOP so they match the current implementation: remove references to the old PBO ring, CPU staging buffer locals, GL/SHM teardown, _gl_delete_buffers, glMapBuffer, and resize-handling logic. Keep the docs focused on the actual behavior in PREVIEW_READBACK_SETUP (wiring window.preview_pbo bridges and deferring window.preview_pbo.deinit()) and PREVIEW_READBACK_LOOP (delegating to window.preview_pbo.frame() via the backend package).
🤖 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 `@src/codegen/preview/editor.zig`:
- Around line 54-55: The generated comment text in editor preview is
grammatically broken because the string in the editor-preview emitter is used
verbatim in generated main.zig. Update the comment text near the editor_* export
handling in the editor-preview code path so it reads naturally (fix “this arms”
and the awkward “emcc link’s” phrasing) while preserving the same meaning about
EXPORTED_FUNCTIONS keeping the exports alive.
In `@src/codegen/preview/wasm_workaround.zig`:
- Around line 4-12: The stray PID note is misplaced in wasm_workaround.zig; this
file should only contain the two wasm workaround snippets. Remove the comment
block from this module and move any PID/hello-message explanation to the
handshake-related code that actually uses it, or delete it if it is no longer
needed. Use the existing snippet definitions in wasm_workaround.zig to keep the
file focused on workaround strings only.
---
Nitpick comments:
In `@src/codegen/preview/callback.zig`:
- Around line 60-80: The doc comment on PREVIEW_HEARTBEAT_CALLBACK describes the
indentation relationship backwards relative to PREVIEW_HEARTBEAT_LOOP, so update
the comment to match the actual generated string bodies. Keep the explanation
aligned with the callback generator’s real paste-site indentation and make sure
the comparison to the loop variant is stated consistently in
PREVIEW_HEARTBEAT_CALLBACK and the related PREVIEW_HEARTBEAT_LOOP constant.
In `@src/codegen/preview/helpers.zig`:
- Around line 29-77: Update the doc comment above PREVIEW_READBACK_HELPERS so it
matches the current body: it no longer contains raw GL externs/constants, only
the bridge wrappers around engine.Preview methods. Remove the stale GL 2.1/3.3
wording and describe this constant as the preview readback bridge helpers used
by the raylib-desktop path, keeping the comment aligned with the existing inline
notes and the _preview_pbo_* bridge functions.
In `@src/codegen/preview/loop.zig`:
- Around line 39-77: Update the doc comments for PREVIEW_READBACK_SETUP and
PREVIEW_READBACK_LOOP so they match the current implementation: remove
references to the old PBO ring, CPU staging buffer locals, GL/SHM teardown,
_gl_delete_buffers, glMapBuffer, and resize-handling logic. Keep the docs
focused on the actual behavior in PREVIEW_READBACK_SETUP (wiring
window.preview_pbo bridges and deferring window.preview_pbo.deinit()) and
PREVIEW_READBACK_LOOP (delegating to window.preview_pbo.frame() via the backend
package).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fe639e0-1f48-4a82-afaf-fb66e287d3d9
📒 Files selected for processing (9)
src/codegen/preview.zigsrc/codegen/preview/callback.zigsrc/codegen/preview/editor.zigsrc/codegen/preview/helpers.zigsrc/codegen/preview/loop.zigsrc/codegen/preview/sokol_d3d11.zigsrc/codegen/preview/sokol_gl.zigsrc/codegen/preview/sokol_metal.zigsrc/codegen/preview/wasm_workaround.zig
| \\ // The `editor_*` C exports this arms are kept alive by the emcc | ||
| \\ // link's -sEXPORTED_FUNCTIONS list (backend hook, editor-preview arm). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Grammar error in generated comment string.
"The editor_* C exports this arms are kept alive by the emcc link's..." is garbled — this comment text is emitted verbatim into generated main.zig for editor-preview builds.
✏️ Proposed fix
- \\ // The `editor_*` C exports this arms are kept alive by the emcc
- \\ // link's -sEXPORTED_FUNCTIONS list (backend hook, editor-preview arm).
+ \\ // The `editor_*` C exports are kept alive by the emcc link's
+ \\ // -sEXPORTED_FUNCTIONS list (backend hook, editor-preview arm).📝 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.
| \\ // The `editor_*` C exports this arms are kept alive by the emcc | |
| \\ // link's -sEXPORTED_FUNCTIONS list (backend hook, editor-preview arm). | |
| \\ // The `editor_*` C exports are kept alive by the emcc link's | |
| \\ // -sEXPORTED_FUNCTIONS list (backend hook, editor-preview arm). |
🤖 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 `@src/codegen/preview/editor.zig` around lines 54 - 55, The generated comment
text in editor preview is grammatically broken because the string in the
editor-preview emitter is used verbatim in generated main.zig. Update the
comment text near the editor_* export handling in the editor-preview code path
so it reads naturally (fix “this arms” and the awkward “emcc link’s” phrasing)
while preserving the same meaning about EXPORTED_FUNCTIONS keeping the exports
alive.
| // PID is purely informational in the `hello` message — the editor | ||
| // uses it for UI display, not for any process management. Earlier | ||
| // snippets tried a per-OS comptime branch (`std.posix.getpid()` on | ||
| // POSIX, kernel32 on Windows) but `std.posix.getpid` isn't exposed | ||
| // in Zig 0.15.2's stdlib (only `std.os.linux.getpid` and | ||
| // `std.c.getpid` exist, and the latter requires linking libc which | ||
| // not every backend does). Simplest portable fix: send 0. A | ||
| // follow-up can wire the real PID once we settle on a stdlib import | ||
| // that's universal across our backends. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- wasm_workaround.zig ---'
git ls-files src/codegen/preview/wasm_workaround.zig
cat -n src/codegen/preview/wasm_workaround.zig
echo
echo '--- search for PID / hello references in preview codegen ---'
rg -n --hidden --glob '!**/.git/**' -S 'getpid|PID|hello message|hello|handshake|kernel32|std\.posix\.getpid|std\.os\.linux\.getpid|std\.c\.getpid' src/codegen src || trueRepository: labelle-toolkit/labelle-assembler
Length of output: 7684
Remove the stray PID comment block
This note about the hello message/PID doesn’t belong in src/codegen/preview/wasm_workaround.zig; the file only defines the two wasm workaround snippets. Move it to the handshake code or delete it.
🤖 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 `@src/codegen/preview/wasm_workaround.zig` around lines 4 - 12, The stray PID
note is misplaced in wasm_workaround.zig; this file should only contain the two
wasm workaround snippets. Remove the comment block from this module and move any
PID/hello-message explanation to the handshake-related code that actually uses
it, or delete it if it is no longer needed. Use the existing snippet definitions
in wasm_workaround.zig to keep the file focused on workaround strings only.
1 participant
What
Pure, behavior-preserving extraction of the 1310-line preview-mode codegen string-template module
src/codegen/preview.ziginto apreview/sub-package + thin barrel. Follows the proven #539/#541 pattern (barrel re-exports the full public surface by name; sub-files path-@imported; no build.zig change).The whole module is
pub conststring literals (no logic, no functions, no cross-references), so the seams are clean and there are no shared helpers to hoist.Sections extracted (name / responsibility / original line-range)
preview/wasm_workaround.zigWASM_PANIC_WORKAROUND,WASM_DEBUG_IO_WORKAROUND)preview/helpers.zigPREVIEW_HELPERS,PREVIEW_READBACK_HELPERS)preview/loop.zigpreview/callback.zigpreview/sokol_gl.zigpreview/sokol_d3d11.zigpreview/sokol_metal.zigpreview/editor.zigThe D3D11/Metal decls were interleaved in the original (helpers → init → frame → cleanup, alternating backend); the split groups them by backend, so each sub-module reads top-to-bottom as one story.
Unchanged public surface
All 28
pub constsnippets are re-exported from the barrel under their original names/identities.preview.<Name>call sites (main_zig.zig,codegen/lifecycle/render.zig,codegen/blocks/imports.zig) are untouched.Verification
zig buildcompiles on 0.16.0zig build testgreen (exit 0; benign--listen=-expected-error stderr)main_zig_tests, incl.bgfx_wasm_editor_preview_main.zig) +preview_mode_testspass — zero golden diff, no golden file changed pergit status(onlypreview.zigmodified +preview/added)zig fmt --checkcleanLine counts
preview.zig= 1310 linespreview.zig= 79; sub-modules 74–387 each (largestsokol_d3d11.zig= 387); total 1376 (delta = per-file headers/barrel boilerplate)Pure extraction — any bug a reviewer spots in the moved code is pre-existing (copied verbatim); flag for follow-up rather than fixing here.
https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
Summary by CodeRabbit
New Features
Bug Fixes