Skip to content

defold entrypoint#34

Open
cloud9c wants to merge 3 commits into
mainfrom
defold-first-class
Open

defold entrypoint#34
cloud9c wants to merge 3 commits into
mainfrom
defold-first-class

Conversation

@cloud9c
Copy link
Copy Markdown
Contributor

@cloud9c cloud9c commented May 29, 2026

Note

Medium Risk
Changes how builds register entrypoints (API payload + multi-HTML selection) for Godot/Unity/Defold dev and upload flows; mis-picked HTML could break playtests until dist layout matches expectations.

Overview
Adds Defold as a first-class engine across wavedash init, config parsing, local dev, and build upload.

Init & config: Detects game.project, defaults upload dir to dist, prompts for Defold version, and writes a [defold] section. EngineKind and validation now include Defold alongside Godot/Unity.

Entrypoint resolution: locate_html_entrypoint no longer returns the first nested .html file; it ranks candidates by newest mtime, prefers wasm-web/ then js-web/, then stable path order—while still using root index.html when present. fetch_entrypoint_params sends htmlPath (relative to upload dir) to the CLI API; dev and build push both use this for Godot, Unity, and Defold.

Build push: Defold builds resolve HTML + version, fetch server-derived entrypointParams, and pass them into temp R2 creds (same pattern as executable engines for JSDOS/Ruffle/Ren'Py).

Reviewed by Cursor Bugbot for commit b6db7b9. Bugbot is set up for automated code reviews on this repo. Configure here.

@claude
Copy link
Copy Markdown

claude Bot commented May 29, 2026

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


Code Review

Reviewed at extra-high effort (5 finder angles × 8 candidates → verify → sweep). Findings posted as inline comments on the PR.

  • Gather diff and PR context
  • Run 5 independent finder angles
  • Verify candidate findings
  • Sweep for gaps
  • Post inline review comments

Summary of findings (10 inline)

High severity

  1. src/dev/entrypoint.rs:61-64 — Sort key precedence is inverted: mtime dominates over the wasm-web//js-web/ architecture preference, so the architecture priority only acts as a tiebreaker when two files share an identical mtime.
  2. src/dev/entrypoint.rs:39-40Option<SystemTime> orders None < Some(_), so a file whose metadata().modified() fails sorts before valid-mtime files and wins selection.
  3. src/dev/entrypoint.rs:51-59architecture_score requires wasm-web//js-web/ to be the first path segment after upload_dir. Defold's default output is build/default/<GameTitle>/wasm-web/...; with upload_dir = "build/default" the preference is silently lost.

Medium severity
4. src/init.rs:323-329 & :254 — Default Defold version "1.0" is not a real Defold release (current is 1.10.x); accepting the default produces a wavedash.toml the server can't resolve.
5. src/init.rs:52default_upload_dir of "dist" is not a Defold convention (Defold bundles to build/default/<GameTitle>/).
6. src/dev/entrypoint.rs:17-21 — Unchanged root-index.html early-return short-circuits the new architecture-aware selection; a stale root index.html silently pre-empts wasm-web/index.html.
7. src/builds.rs:175-179upload_dir is not canonicalized here (unlike dev/mod.rs:101); a strip_prefix failure falls back via unwrap_or and silently leaks an absolute filesystem path to the server as htmlPath.
8. src/init.rs:136-145detect_defold has no version-extraction and no fallback heuristic (compare with detect_godot/detect_unity); version_hint is always None, forcing the prompt onto the hard-coded "1.0" default.

Lower severity
9. src/builds.rs:180-183 — Hardcoded "DEFOLD" literal instead of EngineKind::Defold.as_label(); dev/mod.rs uses the latter. Two sources of truth that can drift.
10. src/dev/entrypoint.rs:82-89 — The new htmlPath field is now unconditionally sent for Godot/Unity callers too (via dev/mod.rs:117 adding them to the same arm), which is a silent server-contract change for existing engines — worth confirming the backend handles it.
Branch: defold-first-class

Comment thread src/dev/entrypoint.rs
Comment on lines +61 to +64
modified_b
.cmp(&modified_a)
.then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b)))
.then_with(|| relative_a.cmp(&relative_b))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort key precedence likely inverted. Modification time is the primary key, so the wasm-web/ / js-web/ architecture preference (the whole point of architecture_score) only acts as a tiebreaker when two HTML files share an identical mtime.

For Defold, both wasm-web/index.html and js-web/index.html are written by the bundler at roughly the same time, but any subsequent activity (re-running just one target, cp -p, an IDE touching one of them, a file-stager re-copying) gives one a newer mtime than the other and that one wins — even if it's js-web/ and the score says wasm-web/ should be preferred.

If the intent is "prefer wasm-web, then js-web, then most-recently-modified," the comparator order should be flipped:

Suggested change
modified_b
.cmp(&modified_a)
.then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b)))
.then_with(|| relative_a.cmp(&relative_b))
architecture_score(&relative_a)
.cmp(&architecture_score(&relative_b))
.then_with(|| modified_b.cmp(&modified_a))
.then_with(|| relative_a.cmp(&relative_b))
});

Comment thread src/dev/entrypoint.rs
Comment on lines +39 to +40
let modified_a = a.metadata().and_then(|m| m.modified()).ok();
let modified_b = b.metadata().and_then(|m| m.modified()).ok();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option<SystemTime> orders None < Some(_), so any file whose metadata().modified() fails (broken symlink, FS that doesn't expose mtime, permissions error during stat) becomes None and — because of the modified_b.cmp(&modified_a) direction on line 61 — sorts before files with valid mtimes. The file with broken metadata wins selection.

Consider mapping None to SystemTime::UNIX_EPOCH (or Reverse) so that "no metadata" sorts to the bottom rather than the top.

Comment thread src/dev/entrypoint.rs
Comment on lines +51 to +59
let architecture_score = |relative: &str| {
if relative.starts_with("wasm-web/") {
0
} else if relative.starts_with("js-web/") {
1
} else {
2
}
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

architecture_score only matches when wasm-web/ / js-web/ is the FIRST path segment after upload_dir.

Defold's HTML5 bundle by default writes to build/default/<GameTitle>/{wasm-web,js-web}/.... If a user sets upload_dir = "build/default" (rather than build/default/<GameTitle>), the relative paths become e.g. MyGame/wasm-web/index.html and MyGame/js-web/index.html — both score 2, the architecture preference never kicks in, and the tiebreakers (mtime + alpha) decide arbitrarily.

Consider matching anywhere in the path, e.g. relative.contains("/wasm-web/") || relative.starts_with("wasm-web/").

Comment thread src/init.rs
Comment on lines +323 to +329
EngineType::Defold => {
let version: String = cliclack::input("Defold version")
.placeholder("1.0")
.default_input("1.0")
.interact()?;
Some(version)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default Defold version "1.0" is not a real Defold release. Defold ships versions like 1.10.x / 1.9.x — Godot's default is 4.4, Unity's is 2022.3, both plausible releases on their actual streams. A user who hits Enter at this prompt ends up with [defold] version = "1.0" in wavedash.toml, which the server can't resolve to any runtime.

Also note generate_toml on line 254 has the same "1.0" fallback; both should be updated to a recent Defold release (e.g. 1.10.4 or whatever the current minimum-supported is).

Comment thread src/init.rs
EngineType::Godot => "build",
EngineType::Unity => "build",
EngineType::Defold => "dist",
EngineType::Custom => "dist",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"dist" is not a Defold convention. Defold's bob.jar HTML5 bundler writes to build/default/<GameTitle>/ by default — most Defold users won't have a dist/ directory at all. Accepting the default in wavedash init produces a wavedash.toml whose upload_dir doesn't exist, and the next wavedash dev / build push errors immediately.

Consider defaulting to build/default (or prompting to enter the bundle path).

Comment thread src/builds.rs
Comment on lines +180 to +183
Some(
fetch_entrypoint_params("DEFOLD", ver, html_path, Some(&html_relative_path))
.await?,
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Engine label hardcoded as "DEFOLD" instead of routed through EngineKind::Defold.as_label(). The parallel call in src/dev/mod.rs:133 correctly uses engine_kind.unwrap().as_label() — these two call sites can now drift silently.

Suggested change
Some(
fetch_entrypoint_params("DEFOLD", ver, html_path, Some(&html_relative_path))
.await?,
)
Some(
fetch_entrypoint_params(EngineKind::Defold.as_label(), ver, html_path, Some(&html_relative_path))
.await?,
)

Comment thread src/builds.rs
Comment on lines +175 to +179
let html_relative_path = html_path
.strip_prefix(&upload_dir)
.unwrap_or(html_path)
.to_string_lossy()
.replace('\\', "/");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upload_dir is not canonicalized here, unlike dev/mod.rs:101. Both call sites compute html_relative_path the same way, but only handle_dev resolves symlinks/.//.. first. The unwrap_or(html_path) fallback means a strip-prefix failure silently sends the absolute filesystem path to the server as htmlPath — leaking the local layout and almost certainly producing a 4xx.

Easiest fix: canonicalize upload_dir here too (mirror the dev/mod.rs block), or replace unwrap_or with an explicit error so a mismatch becomes a clear bug rather than bad data.

Comment thread src/init.rs
Comment on lines +136 to +145
fn detect_defold(dir: &Path) -> Option<DetectedEngine> {
if dir.join("game.project").is_file() {
return Some(DetectedEngine {
engine_type: EngineType::Defold,
version_hint: None,
});
}

None
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

detect_defold has no version-extraction logic and no fallback heuristic. Compare with:

  • detect_godot (lines 64–108): parses config/features and config_version to populate version_hint.
  • detect_unity (lines 112–133): parses ProjectSettings/ProjectVersion.txt for the exact editor version AND falls back to detecting an Assets/ directory.

For Defold, version_hint is always None, so the prompt placeholder (line 325) is the hard-coded "1.0" — a value users may accept without thinking. Defold's game.project does carry a [bootstrap] section and tools like the .defignore / bob.jar runner could imply a version; even just defaulting to the latest known release would be safer than "1.0".

Comment thread src/dev/entrypoint.rs
Comment on lines 82 to 89
let response = client
.post(&endpoint)
.json(&serde_json::json!({
"engine": engine,
"engineVersion": engine_version,
"htmlContent": html_content,
"htmlPath": html_relative_path,
}))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New htmlPath field is always serialized, including for existing Godot/Unity callers that previously sent only engine/engineVersion/htmlContent (dev/mod.rs:117 now passes Some(&html_relative_path) for Godot/Unity too).

If the server validates request shape strictly, or if it uses htmlPath for resolution and didn't before, this is a silent behavior change for established engines. Worth confirming the backend coordinated change covers Godot/Unity, not just Defold.

Comment thread src/dev/entrypoint.rs
return Some(default_index);
}

let mut html_files: Vec<PathBuf> = Vec::new();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unchanged early-return on lines 17–21 short-circuits the new architecture-aware selection. If upload_dir/index.html exists for any reason — a stale leftover from a previous engine, a redirect a build tool placed at the root, a user-written launcher — it wins immediately, and wasm-web/index.html is never considered.

For a Defold project migrating onto wavedash, this is a real footgun: any pre-existing root index.html silently pre-empts the proper Defold entrypoint, and fetch_entrypoint_params parses the wrong HTML. Consider gating the early-return on engine kind, or only using it when no nested wasm-web/ / js-web/ directory is present.

@claude
Copy link
Copy Markdown

claude Bot commented May 29, 2026

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


Code Review — Follow-up on commit 10f7bb0

Re-reviewed the branch against origin/main. The refactor commit makes two changes:

  • bumps the Defold default version "1.0""1.12.4" in both init.rs:254 and init.rs:325/326
  • adds an explanatory comment to locate_html_entrypoint justifying the root-index.html short-circuit as intentional for Godot/Unity

That addresses 1 finding (#4) and intentionally pushes back on 1 finding (#6). The remaining 8 inline findings from the prior pass are still applicable — the code at those locations is unchanged.

Status of prior findings

# Severity Location Status
1 High src/dev/entrypoint.rs:64-67 — sort key precedence (mtime dominates architecture) Outstanding — comparator order unchanged
2 High src/dev/entrypoint.rs:42-43Option<SystemTime> orders None < Some Outstanding.ok() still produces Option, no UNIX_EPOCH fallback
3 High src/dev/entrypoint.rs:54-62architecture_score requires wasm-web/ as first segment after upload_dir Outstanding — still uses starts_with("wasm-web/")
4 Medium src/init.rs:254 & :325-326 — default Defold version "1.0" Fixed in 10f7bb0 (now "1.12.4")
5 Medium src/init.rs:51default_upload_dir "dist" is not a Defold convention Outstanding
6 Medium src/dev/entrypoint.rs:21-24 — root index.html early-return short-circuits architecture-aware selection Acknowledged as intentional (new comment at lines 17-19 documents it). Risk note: any pre-existing root index.html in a Defold project (stale leftover, build-tool artifact) will still silently pre-empt the proper wasm-web/index.html. Worth gating on engine kind if Defold projects ever ship with a root index.
7 Medium src/builds.rs:175-179upload_dir not canonicalized (unlike dev/mod.rs:101) Outstandingstrip_prefix still has silent unwrap_or fallback
8 Medium src/init.rs:136-145detect_defold has no version-extraction / fallback heuristic Outstandingversion_hint is still always None
9 Low src/builds.rs:181 — hardcoded "DEFOLD" literal instead of EngineKind::Defold.as_label() Outstanding — two sources of truth still
10 Low src/dev/entrypoint.rs:91htmlPath now sent for Godot/Unity callers too Outstandingdev/mod.rs:117 still passes Some(&html_relative_path) for Godot/Unity in addition to Defold

New observations from this pass

  • build push vs wavedash dev asymmetry on entrypoint params (src/builds.rs:167-189 vs src/dev/mod.rs:116-141). handle_dev fetches entrypointParams from /cli/entrypoint-params for Godot | Unity | Defold, but handle_build_push only does so for Defold — Godot/Unity builds fall through the _ => None arm. This asymmetry pre-dated this PR (it was equivalent to the old executable_entrypoint_params() call returning None for Godot/Unity), so it's not a regression — but worth confirming this is intentional (server computes params at build time for Godot/Unity, only Defold needs them sent up front), not an oversight when wiring Defold through both flows.

Suggested fix for findings #1, #2, #3 in one shot

let architecture_score = |relative: &str| {
    if relative.contains("/wasm-web/") || relative.starts_with("wasm-web/") {
        0
    } else if relative.contains("/js-web/") || relative.starts_with("js-web/") {
        1
    } else {
        2
    }
};

html_files.sort_by(|a, b| {
    let relative_a = a.strip_prefix(upload_dir).unwrap_or(a).to_string_lossy().replace('\\', "/");
    let relative_b = b.strip_prefix(upload_dir).unwrap_or(b).to_string_lossy().replace('\\', "/");
    let modified_a = a.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);
    let modified_b = b.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);

    architecture_score(&relative_a)
        .cmp(&architecture_score(&relative_b))
        .then_with(|| modified_b.cmp(&modified_a))
        .then_with(|| relative_a.cmp(&relative_b))
});

Fix entrypoint sort + Option ordering + nested path →

Fix builds.rs canonicalize + DEFOLD label →


  • Gather diff and PR context
  • Compare against prior review findings
  • Identify what was fixed vs still outstanding
  • Look for new issues introduced by the refactor
  • Post status summary
    Branch: defold-first-class

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 10f7bb0. Configure here.

Comment thread src/builds.rs
Some(EngineKind::JsDos | EngineKind::Ruffle | EngineKind::RenPy) => {
wavedash_config.executable_entrypoint_params()
}
_ => None,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wildcard match suppresses compiler exhaustiveness check for future engines

Low Severity

The entrypoint_params match in builds.rs uses _ => None as a catch-all, while the equivalent match in dev/mod.rs uses explicit None => None making it exhaustive over all EngineKind variants. If a future engine variant is added to EngineKind, the compiler will force an update in dev/mod.rs but silently accept the wildcard in builds.rs, risking a missing entrypoint-params computation in the build-push flow — exactly the kind of per-engine handling this PR itself demonstrates with Defold.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 10f7bb0. Configure here.

@claude
Copy link
Copy Markdown

claude Bot commented May 29, 2026

Claude finished @cloud9c's task in 6m 41s —— View job


Code Review — Extra-high effort (recall mode)

Reviewed at extra-high effort. Posted 7 inline findings on the latest commit, ranked most-severe first.

  • Gather diff and PR context (git diff origin/main...HEAD)
  • Run 5 independent finder angles
  • Verify candidate findings
  • Sweep for gaps
  • Post inline review comments

Findings posted as inline comments

High severity — entrypoint selection is broken in three independent ways that compound

  1. src/dev/entrypoint.rs:64-67Sort key precedence inverted. mtime is primary, architecture_score is the tiebreaker. The wasm-web preference is effectively dead code: any js-web/index.html with a newer mtime (very common — any partial rebuild, cp -p, or IDE touch produces this) wins over wasm-web/index.html. [discussion_r3327010584]
  2. src/dev/entrypoint.rs:42-43Option<SystemTime> orders None < Some(_), so a file with a failed metadata().modified() (broken symlink, FS without mtime, transient EACCES) sorts to the top under the descending mtime comparator and wins selection. [discussion_r3327011109]
  3. src/dev/entrypoint.rs:54-62architecture_score only matches when wasm-web/ / js-web/ is the first path segment after upload_dir. Defold's natural bob.jar output is build/default/<GameTitle>/wasm-web/…; if the user sets upload_dir = build/default, the preference silently never fires. [discussion_r3327011836]

Medium severity

  1. src/dev/entrypoint.rs:41-68Sort comparator does file I/O on every comparisonO(N log N) stat() syscalls per sort. Switch to sort_by_cached_key so each file's mtime/key is computed once, and the sort stays consistent if a file is unlinked mid-sort. (New observation from this pass.) [discussion_r3327012596]
  2. src/builds.rs:171-175upload_dir not canonicalized (unlike dev/mod.rs:101), and strip_prefix(...).unwrap_or(html_path) silently sends the absolute filesystem path to the server as htmlPath on any normalization mismatch. [discussion_r3327013225]

Low severity

  1. src/builds.rs:177Engine label hardcoded as "DEFOLD" instead of EngineKind::Defold.as_label(). dev/mod.rs:130 uses the latter — two sources of truth that can drift on any future wire-format change. [discussion_r3327013641]
  2. src/builds.rs:184Wildcard _ => None suppresses the compiler's exhaustiveness check. dev/mod.rs:137 uses explicit None => None; the asymmetry means the next new engine variant will silently bypass entrypoint-param fetching in build push. (Also noted by Cursor Bugbot.) [discussion_r3327014042]

Findings from prior passes still applicable but not re-posted

Avoiding inline duplicates — these remain valid in the current code state:

  • src/init.rs:51default_upload_dir = "dist" is not a Defold convention (Defold bundles to build/default/<GameTitle>/).
  • src/init.rs:135-145detect_defold has no version-extraction or fallback heuristic; version_hint is always None.
  • src/dev/entrypoint.rs:17-23 — root-index.html early-return short-circuits the new architecture-aware selection. Acknowledged as intentional via the new comment, but any stale leftover root index.html in a Defold project will silently pre-empt wasm-web/index.html — gating on engine kind would be safer.
  • src/dev/entrypoint.rs:91-94htmlPath is now serialized for Godot/Unity callers too (dev/mod.rs:130 passes Some(&relative) for all three). Confirm the backend handles this for previously-untouched engines.

Suggested combined fix for findings 1–3

let architecture_score = |relative: &str| {
    if relative.contains("/wasm-web/") || relative.starts_with("wasm-web/") { 0 }
    else if relative.contains("/js-web/") || relative.starts_with("js-web/") { 1 }
    else { 2 }
};

html_files.sort_by_cached_key(|p| {
    let relative = p.strip_prefix(upload_dir).unwrap_or(p).to_string_lossy().replace('\\', "/");
    let architecture = architecture_score(&relative);
    let modified = p.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);
    (architecture, std::cmp::Reverse(modified), relative)
});

Fix entrypoint sort + Option ordering + nested path + cached key →

Fix builds.rs canonicalize + DEFOLD label + exhaustive match →


Branch: defold-first-class

Comment thread src/dev/entrypoint.rs
Comment on lines +64 to +67
modified_b
.cmp(&modified_a)
.then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b)))
.then_with(|| relative_a.cmp(&relative_b))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort key precedence is inverted — architecture preference is effectively dead code.

modified_b.cmp(&modified_a) is the primary key; architecture_score only breaks ties when two HTML files share an identical SystemTime. In real bundle output, files written by the same bundler run differ by at least microseconds (and any subsequent cp -p, a partial rebuild of just one target, or an IDE touching one file changes mtime ordering deterministically). So js-web/index.html wins over wasm-web/index.html whenever it happens to be the newer of the two — even though the whole point of architecture_score is to prefer wasm-web.

Flip the comparator so architecture is primary and mtime is the tiebreaker:

Suggested change
modified_b
.cmp(&modified_a)
.then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b)))
.then_with(|| relative_a.cmp(&relative_b))
architecture_score(&relative_a)
.cmp(&architecture_score(&relative_b))
.then_with(|| modified_b.cmp(&modified_a))
.then_with(|| relative_a.cmp(&relative_b))
});

(Combine with the Option<SystemTime> fix below — see separate comment on line 42–43.)

Comment thread src/dev/entrypoint.rs
Comment on lines +42 to +43
let modified_a = a.metadata().and_then(|m| m.modified()).ok();
let modified_b = b.metadata().and_then(|m| m.modified()).ok();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option<SystemTime> orders None < Some(_), so a file whose metadata().modified() fails becomes "newest" under the current comparator.

.ok() produces Option<SystemTime>. If metadata() errors (broken symlink, FS without mtime support, transient EACCES during walk), modified_a is None. Option's Ord impl ranks None before Some(_), so modified_b.cmp(&modified_a) (descending) puts the None file at the top of the sort — it wins selection over every file with a valid mtime.

Map the failure to a far-past time so missing-mtime files sort to the bottom:

let modified_a = a.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);
let modified_b = b.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);

This also removes the Option wrapping so the Ord derivation behaves intuitively.

Comment thread src/dev/entrypoint.rs
Comment on lines +54 to +62
let architecture_score = |relative: &str| {
if relative.starts_with("wasm-web/") {
0
} else if relative.starts_with("js-web/") {
1
} else {
2
}
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

architecture_score only triggers when wasm-web/ / js-web/ is the FIRST path segment relative to upload_dir — Defold's natural bundle layout puts them under a game-name folder.

bob.jar HTML5 bundling writes to <output>/<GameTitle>/wasm-web/... and <output>/<GameTitle>/js-web/.... A user who sets upload_dir = "build/default" (rather than build/default/<GameTitle>) sees relative paths like MyGame/wasm-web/index.html — both architectures score 2, the preference never fires, and selection falls through to mtime + alpha (i.e. whichever bundler happened to write last, or alphabetic on game name).

Match anywhere in the path:

Suggested change
let architecture_score = |relative: &str| {
if relative.starts_with("wasm-web/") {
0
} else if relative.starts_with("js-web/") {
1
} else {
2
}
};
let architecture_score = |relative: &str| {
if relative.contains("/wasm-web/") || relative.starts_with("wasm-web/") {
0
} else if relative.contains("/js-web/") || relative.starts_with("js-web/") {
1
} else {
2
}
};

Comment thread src/dev/entrypoint.rs
Comment on lines +41 to +68
html_files.sort_by(|a, b| {
let modified_a = a.metadata().and_then(|m| m.modified()).ok();
let modified_b = b.metadata().and_then(|m| m.modified()).ok();
let relative_a = a
.strip_prefix(upload_dir)
.unwrap_or(a)
.to_string_lossy()
.replace('\\', "/");
let relative_b = b
.strip_prefix(upload_dir)
.unwrap_or(b)
.to_string_lossy()
.replace('\\', "/");
let architecture_score = |relative: &str| {
if relative.starts_with("wasm-web/") {
0
} else if relative.starts_with("js-web/") {
1
} else {
2
}
};

modified_b
.cmp(&modified_a)
.then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b)))
.then_with(|| relative_a.cmp(&relative_b))
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort comparator does file IO on every comparison — O(N log N) stat() syscalls per sort, not O(N).

Each invocation of the closure calls a.metadata() and b.metadata(). Rust's sort_by calls the comparator O(N log N) times, so for N HTML files we issue ~2·N·log₂(N) stat syscalls (plus another lazy traversal building the relative strings each call). For a typical Defold dist this is small, but for any upload_dir that incidentally contains many HTML files (engine demos, generated docs, asset bundles with HTML readmes), this is noticeable I/O — and it duplicates work that has a stable per-file answer.

Pre-compute keys once with sort_by_cached_key:

html_files.sort_by_cached_key(|p| {
    let relative = p.strip_prefix(upload_dir).unwrap_or(p).to_string_lossy().replace('\\', "/");
    let architecture = architecture_score(&relative);
    let modified = p.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);
    // Reverse mtime via Reverse() so newest sorts first
    (architecture, std::cmp::Reverse(modified), relative)
});

This also makes the comparator total/consistent if a file is unlinked mid-sort (the key is captured once at the start).

Comment thread src/builds.rs
Comment on lines +171 to +175
let html_relative_path = html_path
.strip_prefix(&upload_dir)
.unwrap_or(html_path)
.to_string_lossy()
.replace('\\', "/");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upload_dir is not canonicalized here (unlike dev/mod.rs:101), and the .unwrap_or(html_path) fallback silently leaks an absolute filesystem path on strip_prefix failure.

handle_dev resolves upload_dir via .canonicalize()? before walking it (src/dev/mod.rs:101). handle_build_push skips that step, so this call site walks a potentially-relative-or-symlinked path. If strip_prefix(&upload_dir) ever fails (mismatch in normalization between the path returned by WalkDir and upload_dir), unwrap_or(html_path) falls back to the full path — that absolute filesystem path gets sent to the server as htmlPath, leaking the local layout and almost certainly producing a 4xx.

Either canonicalize upload_dir first (mirror dev/mod.rs:101), or replace the fallback with an explicit error so a path mismatch surfaces as a clear bug:

let upload_dir = upload_dir.canonicalize().with_context(|| {
    format!("Failed to canonicalize upload_dir: {}", upload_dir.display())
})?;
// ...
let html_relative_path = html_path
    .strip_prefix(&upload_dir)
    .with_context(|| format!("HTML entrypoint {} is not under upload_dir {}", html_path.display(), upload_dir.display()))?
    .to_string_lossy()
    .replace('\\', "/");

Comment thread src/builds.rs
.to_string_lossy()
.replace('\\', "/");
Some(
fetch_entrypoint_params("DEFOLD", ver, html_path, Some(&html_relative_path))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Engine label hardcoded as "DEFOLD" instead of routed through EngineKind::Defold.as_label().

The parallel call in src/dev/mod.rs:130 correctly uses engine_kind.unwrap().as_label(). Two sources of truth for the same value can drift silently — e.g. if the wire format ever needs to change ("DEFOLD""defold" for normalization), dev and build push will diverge until both are updated.

Suggested change
fetch_entrypoint_params("DEFOLD", ver, html_path, Some(&html_relative_path))
Some(
fetch_entrypoint_params(EngineKind::Defold.as_label(), ver, html_path, Some(&html_relative_path))
.await?,
)

Comment thread src/builds.rs
Some(EngineKind::JsDos | EngineKind::Ruffle | EngineKind::RenPy) => {
wavedash_config.executable_entrypoint_params()
}
_ => None,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wildcard _ => None suppresses the compiler's exhaustiveness check for future engine variants.

dev/mod.rs:137 uses an explicit None => None, making the match exhaustive over every EngineKind variant. Here, adding a new engine to EngineKind will compile cleanly in builds.rs — silently routing the new engine through the _ => None arm — even when dev (correctly) forces the author to decide. That asymmetry is exactly the per-engine handling this PR demonstrates by wiring up Defold; the wildcard makes the next engine more likely to slip through build push.

Suggested change
_ => None,
Some(EngineKind::Godot | EngineKind::Unity) => None,
None => None,
};

(Or list each variant explicitly. Either way, drop the wildcard.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant