Skip to content

fix(frontend): build ladle correctly#5027

Merged
jog1t merged 1 commit into
mainfrom
05-06-fix_frontend_build_ladle_correctly
May 15, 2026
Merged

fix(frontend): build ladle correctly#5027
jog1t merged 1 commit into
mainfrom
05-06-fix_frontend_build_ladle_correctly

Conversation

@jog1t

@jog1t jog1t commented May 11, 2026

Copy link
Copy Markdown
Contributor

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@claude

claude Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Code Review: fix(frontend): build ladle correctly

Note: This PR is already merged. This review is retrospective.

Overview

This PR fixes the frontend Docker builds (cloud.Dockerfile, inspector.Dockerfile, ladle.Dockerfile) by:

  1. Copying the rivetkit-wasm package into each image's build context, which was previously missing.
  2. Setting SKIP_WASM_BUILD=1 to skip the wasm-pack step that requires a Rust toolchain not present in these images.

The change is minimal, targeted, and correctly addresses the root cause.


What's Good

  • Clear comment. The block comment above ENV SKIP_WASM_BUILD=1 is well-written and explains both what it does and why — especially the nuance that only the committed index.d.ts is needed for DTS resolution, not the compiled wasm artifact itself. This is exactly the right level of detail for a non-obvious env override.
  • Correct placement. The new COPY rivetkit-typescript/packages/rivetkit-wasm/ line is inserted alphabetically among the other rivetkit package copies, maintaining the existing ordering convention.
  • Targeted fix. No unrelated changes; the diff is easy to reason about.

Concerns / Suggestions

1. Triplication of identical lines (minor)
The same six lines (COPY + ENV + comment block) appear verbatim in all three Dockerfiles. If the SKIP_WASM_BUILD convention changes or a Rust toolchain is added later, all three files need updating. Consider extracting a shared base Dockerfile stage (e.g., frontend/base.Dockerfile) that handles the rivetkit copies and env setup, then FROM base in each. That said, this pattern is common in multi-target Docker builds and the current footprint is small — not blocking.

2. SKIP_WASM_BUILD convention is implicit
The env var works only if the rivetkit-wasm package's postinstall / build script honors it. There's no in-diff evidence of where this env var is consumed. If the convention is internal to rivetkit-wasm, it would be worth a cross-reference (e.g., # see rivetkit-typescript/packages/rivetkit-wasm/package.json postinstall) so future maintainers can trace the full flow without guessing.

3. PR description is blank
The description template was left unfilled (all checkboxes unchecked, no test plan). For a build fix, a one-liner like "Verified the ladle/cloud/inspector images build successfully without a Rust toolchain" in the description would help future bisectors understand when and how this was validated.


Summary

Correct and well-reasoned fix. The comment quality is above average. The main follow-up to consider is documenting (or linking to) where SKIP_WASM_BUILD is consumed, so the implicit contract between the Dockerfile and the package's build script is discoverable.

@jog1t jog1t force-pushed the 05-06-feat_frontend_improve_runners_errors_ui branch from 998ae2d to 3e2bff1 Compare May 11, 2026 18:49
@jog1t jog1t force-pushed the 05-06-fix_frontend_build_ladle_correctly branch from d1dc356 to 331cbe7 Compare May 11, 2026 18:49
@jog1t jog1t force-pushed the 05-06-fix_frontend_build_ladle_correctly branch from 331cbe7 to da858a7 Compare May 15, 2026 19:30
@jog1t jog1t force-pushed the 05-06-feat_frontend_improve_runners_errors_ui branch from 3e2bff1 to 919b36c Compare May 15, 2026 19:30
@jog1t jog1t mentioned this pull request May 15, 2026
11 tasks

jog1t commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

Merge activity

  • May 15, 7:36 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • May 15, 7:46 PM UTC: Graphite rebased this pull request as part of a merge.
  • May 15, 7:46 PM UTC: @jog1t merged this pull request with Graphite.

@jog1t jog1t changed the base branch from 05-06-feat_frontend_improve_runners_errors_ui to graphite-base/5027 May 15, 2026 19:43
@jog1t jog1t changed the base branch from graphite-base/5027 to main May 15, 2026 19:44
@jog1t jog1t force-pushed the 05-06-fix_frontend_build_ladle_correctly branch from da858a7 to 883a833 Compare May 15, 2026 19:45
@jog1t jog1t merged commit e596d31 into main May 15, 2026
4 of 7 checks passed
@jog1t jog1t deleted the 05-06-fix_frontend_build_ladle_correctly branch May 15, 2026 19:46
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