add renpy

[cloud9c]

No description provided.

[claude]

Claude finished @cloud9c's task in 2m 17s —— View job

---

Code Review

Adds Ren'Py as a fifth engine. The change is small (+27/−1) and faithfully follows the JsDos/Ruffle pattern across EngineKind, WavedashConfig, executable_entrypoint_params, executable_files_to_validate, and the handle_dev match arm. Wiring looks correct end-to-end — engine_type(), engine_version(), validation in file_staging.rs:36, and the build-creation path in builds.rs all pick it up automatically through the shared helpers.

Two issues worth fixing before merge, plus one optional cleanup.

Issues

1. Stale error message in engine_type() — src/config.rs:263 (inline comment posted)

The conflict-detection branch now also fires for [renpy] combinations, but its message still reads:

Config must have at most one engine section: [godot], [unity], [jsdos], or [ruffle]

A user who puts [godot] + [renpy] in their config will hit this and be told the valid engines, but [renpy] won't be in the list. Fix this →

2. Stale doc comments referencing only JSDOS/Ruffle (inline comment posted)

Four spots still say "For JSDOS/Ruffle" but now also apply to Ren'Py:

• src/config.rs:297 — doc on executable_entrypoint_params
• src/config.rs:322 — doc on executable_files_to_validate
• src/file_staging.rs:35 — // Validate executable and loader_url files exist (for JSDOS/Ruffle)
• src/builds.rs:71 — // Add entrypointParams if provided (for JSDOS/Ruffle)

Fix this →

Optional — refactor to reduce duplication

JsDosSection, RuffleSection, and RenPySection are now byte-for-byte identical (version, executable, loader_url). Adding a fourth engine of this shape will mean touching five branches in config.rs plus the dev/mod.rs match arm. Worth considering a single ExecutableEngineSection struct (or a small helper that returns the active (executable, &Option<loader_url>) tuple) so future additions become one line. Not blocking this PR.

Notes

• wavedash init doesn't scaffold Ren'Py — same as JsDos/Ruffle today, so this is consistent, not a regression. Flagging in case onboarding parity matters.
• Branch is behind origin/main — main has advanced (version bumped to 0.1.80, dev-app/src/main.ts changed). You'll need to merge/rebase before this can ship cleanly.
• No tests — there are no tests in src/ covering engine selection today, so this PR isn't introducing a test gap. If you want to add one, engine_type() is the cheapest place: a unit test asserting that two engine sections produce the conflict error would also catch issue Chunk uploads to <500mb chunks so we can cache on r2-asset-proxy #1 above.

Todos

• Read PR diff against origin/main
• Read affected files (src/config.rs, src/dev/mod.rs) and surrounding callers
• Analyze changes for bugs, security, and consistency
• Post inline comments + final review
  · Branch: add-renpy-engine

[claude]

This branch is the third near-identical copy of the same shape (executable + optional loader_url). Two related observations:

1. The doc comment on executable_entrypoint_params (line 297) and on executable_files_to_validate (line 322) still say "For JSDOS/Ruffle engines" — they need an update to include Ren'Py. Same for the comments at src/file_staging.rs:35 and src/builds.rs:71.
2. Since JsDosSection, RuffleSection, and RenPySection are now byte-for-byte identical (version, executable, loader_url), consider extracting a shared ExecutableEngineSection struct and a small helper that yields (executable, loader_url) for whichever variant is active. Would collapse the three if/else branches here and in executable_files_to_validate and engine_version into a single match, and make adding the next engine a one-line change. Not blocking — just calling out that the duplication has crossed the threshold where it's starting to cost.

[claude]

Claude finished @cloud9c's task in 2m 31s —— View job

---

Code Review (re-run after sync)

Re-reviewed after the push. The renpy diff is unchanged from the prior review — none of the previously flagged issues have been addressed yet. The two new commits on the branch (fd2ff46 title-bar padding, 022ffa8 release) are unrelated to the renpy feature.

Outstanding from prior review (all still present)

1. Stale error message — src/config.rs:263 (inline comment posted)

The conflict-detection branch now fires for [renpy] combinations but still tells users:

Config must have at most one engine section: [godot], [unity], [jsdos], or [ruffle]

[renpy] is missing. Fix this →

2. Stale doc comments referencing only JSDOS/Ruffle (inline comment posted)

Four spots still say "For JSDOS/Ruffle" but now also apply to Ren'Py:

• src/config.rs:297 — doc on executable_entrypoint_params
• src/config.rs:322 — doc on executable_files_to_validate
• src/file_staging.rs:35 — // Validate executable and loader_url files exist (for JSDOS/Ruffle)
• src/builds.rs:71 — // Add entrypointParams if provided (for JSDOS/Ruffle)

Fix this →

3. Duplication (optional refactor)

JsDosSection, RuffleSection, and RenPySection are byte-for-byte identical, and the same if/else over them is now copied three times (engine_version, executable_entrypoint_params, executable_files_to_validate). Extracting a shared ExecutableEngineSection plus a small helper that returns the active (executable, loader_url) would collapse all three into a match and make the next engine a one-line change. Not blocking — calling out that the duplication is now load-bearing for future additions.

New since last review

fd2ff46 "Fix title bar padding" — unrelated to renpy. Reduces the titlebar strip from 38px → 24px and adds a CSS rule to shift fixed-position headers/navs down by the strip height. One minor concern flagged inline: the selector :is(header, nav)[class*="fixed"][class*="top-0"] uses substring matching, so it will also catch lg:top-0, top-0.5, etc. Probably fine for Wavedash's current SiteHeader markup, but worth knowing.

022ffa8 "release" — version bump in Cargo.{toml,lock}. Looks fine.

Notes

• Scope creep: this PR is now titled "add renpy" but bundles a UI fix and a release bump from a different author. Consider splitting if you want a clean commit history; not strictly required.
• The previous review also noted no tests cover engine selection. engine_type() is still the cheapest place to add one — a test asserting that two engine sections produce the conflict error would catch issue Chunk uploads to <500mb chunks so we can cache on r2-asset-proxy #1 automatically.

Todos

• Read PR diff against origin/main
• Read affected files and check for prior review follow-through
• Analyze changes for bugs, security, and consistency
• Post inline comments + final review
  · Branch: add-renpy-engine

[claude]

Still unaddressed from the prior review — two related items:

1. Stale doc comments. Four spots still describe these helpers as "For JSDOS/Ruffle" only:

• src/config.rs:297 — doc on executable_entrypoint_params
• src/config.rs:322 — doc on executable_files_to_validate
• src/file_staging.rs:35 — // Validate executable and loader_url files exist (for JSDOS/Ruffle)
• src/builds.rs:71 — // Add entrypointParams if provided (for JSDOS/Ruffle)

All four now also apply to Ren'Py.

2. Duplication (optional). JsDosSection, RuffleSection, and RenPySection are byte-for-byte identical, and this is now the third copy of the same if/else shape (also in engine_version and executable_files_to_validate). Extracting a shared ExecutableEngineSection plus a small helper that yields (executable, loader_url) for whichever variant is active would collapse three branches into a match and make the next engine a one-line change. Not blocking.

[claude]

Minor: [class*="fixed"][class*="top-0"] is a substring match, so it also catches Tailwind responsive variants like lg:top-0 (which intentionally only pin at one breakpoint) or fractional classes like top-0.5. Likely fine in practice given Wavedash's current SiteHeader markup, but worth knowing: this rule will pin those down by TITLEBAR_STRIP_PX unconditionally, ignoring breakpoint logic.

Not blocking — and this commit looks unrelated to the renpy feature; flagging here only because it landed in the same PR.

[claude]

Claude finished @cloud9c's task in 1m 9s —— View job

---

Code Review (re-run after sync)

The latest push addresses all blocking feedback from the prior two reviews. Branch is now a clean single commit (c63cff3 add renpy) on top of origin/main, and the unrelated titlebar/release commits flagged earlier are gone. Diff matches the PR summary (+32/−6).

Previously flagged — now fixed

• ✅ Error message at src/config.rs:263 now includes [renpy]:

  Config must have at most one engine section: [godot], [unity], [jsdos], [ruffle], or [renpy]
  

• ✅ All four stale doc comments updated to "JSDOS/Ruffle/Ren'Py":
  • src/config.rs:297 — doc on executable_entrypoint_params
  • src/config.rs:322 — doc on executable_files_to_validate
  • src/file_staging.rs:35
  • src/builds.rs:71
• ✅ Branch scope cleanup — titlebar-padding and release commits dropped, branch is now solely the renpy feature.

Still open (non-blocking)

Duplication. JsDosSection, RuffleSection, and RenPySection remain byte-for-byte identical (version, executable, loader_url), and the same if/else shape is now copied three times across engine_version, executable_entrypoint_params, and executable_files_to_validate. Extracting a shared ExecutableEngineSection plus a small helper returning the active (executable, &Option<loader_url>) would collapse the three branches into a match and reduce the next engine to a one-line change. Not blocking — flagging that the duplication has crossed the threshold where it's actively load-bearing for future additions. Fix this →

Notes

• wavedash init still doesn't scaffold Ren'Py, same as JsDos/Ruffle — consistent with current onboarding, not a regression.
• No tests cover engine selection. engine_type() is the cheapest place to add one if you want regression coverage — a test asserting two engine sections produce the conflict error would also lock in issue Chunk uploads to <500mb chunks so we can cache on r2-asset-proxy #1's fix.

Verdict

LGTM. All blocking items addressed; only the optional refactor remains, which I'd leave to a follow-up.

Todos

• Read PR diff against origin/main
• Read affected files and check for prior review follow-through
• Analyze changes for bugs, security, and consistency
• Post inline comments + final review
  · Branch: add-renpy-engine

[claude]

Claude finished @cloud9c's task in 1m 8s —— View job

---

Code Review (re-run after refactor push)

The latest push goes beyond addressing the prior feedback — it implements the optional refactor that was flagged but not blocking. Net diff is now +46/−50 (line reduction despite adding an engine), and the duplication concern from the last three reviews is fully resolved.

Previously flagged — all fixed

• ✅ Error message at src/config.rs:251 now includes [renpy]:

  Config must have at most one engine section: [godot], [unity], [jsdos], [ruffle], or [renpy]
  

• ✅ All four stale doc comments updated to "JSDOS/Ruffle/Ren'Py" (src/config.rs:289, src/config.rs:301, src/file_staging.rs:35, src/builds.rs:71).
• ✅ Refactor applied — JsDosSection/RuffleSection/RenPySection collapsed into a single ExecutableEngineSection (src/config.rs:152-157); a new executable_section() helper (src/config.rs:259) returns the active variant via .or() chaining; engine_version, executable_entrypoint_params, and executable_files_to_validate all now route through it. Adding the next executable-style engine is genuinely a one-line change now.

Quality of the refactor

• ExecutableEngineSection is the right abstraction — the three engines were byte-for-byte identical, and the shared shape is documented with a clear comment about which engines use it (src/config.rs:150-151).
• executable_section() does jsdos.or(ruffle).or(renpy), which has an implicit preference order. This is safe in practice because engine_type() rejects configs with multiple engine sections set, and the helper's doc comment (src/config.rs:256-258) explicitly states this invariant. Worth being aware of: if a future caller invokes executable_entrypoint_params() without first validating via engine_type(), the silent fallthrough could surprise. The invariant is documented, but not enforced at the type level — acceptable trade-off for the simpler API.
• executable_files_to_validate now uses let-else (src/config.rs:304) and vec! macro — cleaner than the prior if/else if push pattern.
• engine_version uses early-return style instead of the long if/else if/else chain — easier to read.

Notes (non-blocking)

• wavedash init still doesn't scaffold Ren'Py — same as JsDos/Ruffle, consistent with current onboarding, not a regression.
• No tests cover engine selection — engine_type() is still the cheapest place to add one (asserting two engine sections produce the conflict error). Optional.

Verdict

LGTM. All prior feedback addressed, including the optional refactor. Clean implementation.

Todos

• Read PR diff against origin/main
• Read affected files and check for prior review follow-through
• Analyze changes for bugs, security, and consistency
• Post inline comments + final review
  · Branch: add-renpy-engine

[claude]

Claude finished @cloud9c's task in 3m 52s —— View job

---

Code Review (re-run after inject /renpy-loader.js push)

The new commit on the branch (0945002) adds the dev-server-side piece of Ren'Py support: a passthrough rule plus an extra <script> injection in HTML responses. The renpy config/refactor work from the prior commit (bff48a4) is unchanged from the version the previous review LGTM'd. Net branch diff: +61/−55 across 5 files.

0945002 inject /renpy-loader.js for RENPY local builds

Small, focused change in dev-app/src/server.ts only:

• /renpy-loader.js added to PASSTHROUGH_PREFIXES (server.ts:46) so the local dev server reverse-proxies the request to the real play worker, same pattern as /embed.js//embed.css.
• EMBED_BOOTSTRAP_WITH_RENPY injects two adjacent sync <script> tags (server.ts:60-62) in the order embed.js then renpy-loader.js. Order is correct: sync scripts execute in source order, so the shim installs after window.Wavedash is available but before any user/Ren'Py-engine scripts in the body. The "Order is load-bearing" comment captures the why clearly — good.
• Detection is via ?engine=RENPY on the entrypoint URL (server.ts:300), which keeps the dev server stateless about which build is currently active. serveLocalFile was widened to take URLSearchParams to support this.

Things to note (non-blocking)

Implicit cross-codebase contracts (inline comment posted at server.ts:46-62). Two things have to be true in the play worker for this to actually fire:

1. Origin must serve /renpy-loader.js.
2. The /local-embed → /<entrypoint> redirect must append ?engine=RENPY.

Both are reasonable assumptions given the production play worker injects embed.js the same way, but neither is verifiable from this repo. If the play-worker side hasn't shipped yet, the renpy loader will silently no-op (worst case: tag injects but /renpy-loader.js 404s on passthrough; best case: redirect doesn't include the query param and no tag is injected). Worth a quick smoke test before relying on it.

Direct-URL access skips injection. If a user navigates to /<entrypoint> directly without going through /local-embed, there's no ?engine=RENPY and the loader isn't injected. Fine for the typical dev flow.

RENPY_ENGINE = "RENPY" duplicates EngineKind::RenPy.as_label() from src/config.rs:197. Effectively part of the API contract already (the same label is sent to the server in create_local_build), so the duplication is benign. Mentioning only because a silent rename on one side would break the other.

bff48a4 add renpy (unchanged from prior LGTM)

All previously flagged blockers stay fixed:

• ✅ Error message at src/config.rs:251 includes [renpy]
• ✅ All four "JSDOS/Ruffle/Ren'Py" doc comments updated
• ✅ ExecutableEngineSection + executable_section() helper replaces the three duplicate section structs; engine_version, executable_entrypoint_params, and executable_files_to_validate all route through it. Adding the next executable-style engine is a one-line change.

The executable_section() preference order (jsdos.or(ruffle).or(renpy)) is safe in practice because engine_type() rejects configs with multiple engine sections set, and the helper's doc comment (src/config.rs:256-258) documents that invariant. Acceptable trade-off; the invariant is documented but not type-enforced.

Notes

• wavedash init still doesn't scaffold Ren'Py — consistent with JsDos/Ruffle today, not a regression.
• Still no tests covering engine selection. engine_type() is the cheapest place to add one if you want regression coverage; not blocking.

Verdict

LGTM. The renpy section work is unchanged from the previously approved state; the new commit is a small, well-commented addition with no functional issues on the local-server side. Just verify the play-worker side (serving /renpy-loader.js and appending ?engine=RENPY on the redirect) is in sync before relying on this end-to-end.

Todos

• Read PR diff against origin/main
• Read affected files and check prior review follow-through
• Analyze changes for bugs, security, and consistency
• Post inline comments + final review
  · Branch: add-renpy-engine

[claude]

Two implicit cross-codebase contracts are now load-bearing here — both worth knowing about even though neither is fixable in this PR:

1. /renpy-loader.js must be served by origin. It's in PASSTHROUGH_PREFIXES, so the local dev server reverse-proxies the request to the real play worker. If the play worker isn't yet serving /renpy-loader.js in whichever environment a developer is pointed at (staging vs prod), the <script> tag will inject but the fetch will 404 and the game silently fails to boot.

2. The ?engine=RENPY query param must be appended by the play worker's /local-embed → /<entrypoint> 302. The detection at server.ts:300 (searchParams.get("engine") === RENPY_ENGINE) only fires when the browser arrives at the entrypoint with that query string. The CLI tells the API the build's engine via create_local_build (src/dev/mod.rs:132), but it's the play worker's redirect that has to translate that into a query-param tag on the Location header. If that side isn't deployed in sync, the loader tag won't be injected even when [renpy] is set in wavedash.toml.

Also: RENPY_ENGINE = "RENPY" here is a string duplicate of EngineKind::RenPy.as_label() in src/config.rs:197. They're effectively part of the API contract already (the label is also sent to the server), so the duplication is acceptable — flagging only because a silent rename on one side would break the other.

Not blocking. Worth a smoke test in wavedash dev against a real [renpy] config before merging if the play worker changes haven't shipped yet.