Skip to content

Fix Solve Mode animation and stability#76

Merged
markcoleman merged 2 commits intomainfrom
better-solve-mode
Feb 26, 2026
Merged

Fix Solve Mode animation and stability#76
markcoleman merged 2 commits intomainfrom
better-solve-mode

Conversation

@markcoleman
Copy link
Copy Markdown
Owner

Summary:

  • stabilize Solve Mode by wiring up the new engine/animator pipeline with deterministic state progression, proper move formatting guidance, and 3D renderer highlights
  • add human-friendly instructions, controls, and UI wiring so back/next/play work without locking the app or corrupting state
  • document the orientation assumptions, renderer hook points, and how to run the new tests in the README
  • add unit/snapshot tests covering moves, reducer logic, instructions, engine navigation, and ViewModel rendering expectations

Testing:

  • Not run (not requested)

@github-actions github-actions Bot added documentation Improvements or additions to documentation ui test size/extra-large labels Feb 23, 2026
@github-actions
Copy link
Copy Markdown
Contributor

⚠️ This PR is very large. Consider breaking it into smaller PRs for easier review.

PR Stats:

  • Lines added: 2003
  • Lines deleted: 115
  • Files changed: 26

Tips for large PRs:

  • Break into smaller, focused PRs
  • Ensure comprehensive test coverage
  • Add detailed descriptions for each change

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c5d561e3ca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@StateObject private var rendererBridge: SolveModeRendererBridge
@StateObject private var viewModel: SolveModeViewModel

@SceneStorage("solve_mode_step_index") private var persistedStepIndex = 0
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Namespace persisted step index by solve session

Using a fixed @SceneStorage("solve_mode_step_index") key causes progress from one cube/solution to be restored into unrelated Solve Mode sessions, so opening a new solve can jump to an arbitrary step (or directly to solved if the stored index exceeds the new move count). This corrupts guided playback for users who run multiple solves in one app session; the storage key should be scoped to the specific puzzle/solution (or cleared when starting a new solve).

Useful? React with 👍 / 👎.

Comment on lines +317 to +321
guard !didRestoreState else { return }
didRestoreState = true

guard persistedStepIndex > 0 else { return }
viewModel.jump(to: persistedStepIndex)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Defer restore completion until orientation lock is cleared

This marks restoration as done before attempting jump, but jump is a no-op while orientationConfirmed is false (SolveModeViewModel.jump guards on that), so any view using requireOrientationConfirmation can never actually restore persisted progress after the user confirms orientation. In practice, users always restart at step 0 despite a saved index; restoration should run again after confirmation or only set didRestoreState after a successful jump.

Useful? React with 👍 / 👎.

@markcoleman markcoleman merged commit 3b827d2 into main Feb 26, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/extra-large test ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant