Solitare of Olympus — Code Review

Bugs / Correctness Issues

1. IAM ARN double colon (false alarm)

deploy-static-site.yml:31

role-to-assume: arn:aws:iam::504242000181:role/GithubDeployCI

Standard ARN format for IAM is arn:aws:iam::<account-id>:role/... with no region (empty field), so the double colon is correct. Disregard.

2. Premature "out of gold" loss on first recycle

game.rs:188 and main.rs:270

When temple_gold is 0 and the player recycles waste back to stock, saturating_sub(1) keeps it at 0, but main.rs:270 checks self.game.temple_gold == 0 after recycle and ends the game. A player who hasn't scored any gold yet and recycles for the first time will immediately lose. This is likely unintentional — the gold penalty should probably only apply if gold was > 0 before the recycle, or the initial gold should be > 0.

3. Deprecated action in deploy workflow

deploy-static-site.yml:33

uses: actions-rs/toolchain@v1

actions-rs/toolchain is archived/unmaintained. The CI workflow already uses dtolnay/rust-toolchain@stable — the deploy workflow should match.

4. Unnecessary packages: write permission

deploy-static-site.yml:9

The deploy workflow doesn't publish any packages. This is an overly broad permission.

Code Quality Issues

5. Gold earned on tableau-to-tableau moves seems wrong

game.rs:421-430

Moving a run of cards between tableau columns awards gold equal to the number of cards moved (moved_count). Moving from waste or foundation to tableau awards 1 gold. This means players can farm gold by shuffling runs back and forth between tableau columns. Typically solitaire only scores on foundation placements.

6. temple_gold field does double duty

It's both a score and a loss-condition currency, which makes the game rules confusing. If gold increases on every move type (including tableau rearrangements), the "out of gold" mechanic barely matters.

7. No :disabled styling for buttons

style.css

The control row buttons can be disabled (locked or busy states), but there's no :disabled CSS rule. Disabled buttons will look clickable.

8. getrandom version mismatch

Cargo.toml:16-17

getrandom = { version = "0.2.15", features = ["js"] }
getrandom_04 = { package = "getrandom", version = "0.4.1", features = ["wasm_js"] }

Two different versions of getrandom are pulled in. The 0.2 version is likely a transitive requirement from rand's older dependencies. Check if the 0.2 dep can be dropped — it adds compile time and WASM bloat.

9. Dockerfile doesn't pin trunk version

Dockerfile:11

RUN cargo install trunk

No --locked and no version pin. Builds may break from upstream trunk changes. The CI workflows use --locked but the Dockerfile doesn't.

10. Stale absolute paths in README

README.md:38-51

References /Users/john/sandbox/solitare/ — these are local machine paths that won't work for anyone else and should be relative paths.

Minor Improvements

11. yamlfmt workflow runs on every push to every branch

yamlfmt.yml:3-5

on:
  push:
  pull_request:

No branch filter, so it runs on all pushes. Could be scoped to main like the other workflows.

12. Card id field is unused

game.rs:42

The id is set during deck creation but never read anywhere in game logic or rendering. It's dead weight on every Card copy.

13. commitlint runs on push to all branches

commit-lint.yml:8-9

Every push to any branch triggers it, which is noisy. Consider limiting to main + PRs.

14. Missing .dockerignore entries

.dockerignore likely only has target. Should also exclude .git, dist, .github, etc. to speed up Docker context.

15. Dockerfile uses unpinned base image

ghcr.io/jac18281828/rust:latest could break at any time. Consider pinning to a digest or tag.

Summary

The most impactful issues are:

• #2 (premature loss on first recycle)
• #5 (gold farming from tableau moves)
• #3 (deprecated CI action)
• #7 (no disabled button styling)

The game logic is otherwise solid with good test coverage of the core mechanics. The CSS and theming work is thorough and responsive.