Skip to content

Jump into order creation from an unfilled orders-list row - #1401

Open
JorenC wants to merge 5 commits into
mainfrom
order-click-wizard
Open

JorenC wants to merge 5 commits into
mainfrom
order-click-wizard

Conversation

@JorenC

@JorenC JorenC commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

What this PR does

Ports the order-click-to-wizard feature out of the closed MegaPR (#1349), which bundled it with a lot of unrelated work that's either already on main via other PRs or still under discussion. This is a straight port of the original two commits, adapted for drift on main since they were written — no re-implementation.

Touching a province with "Order not provided" in the orders list now starts the same order-creation flow as clicking it on the map: selects it as the order source, pans the map to it (without zooming, since only one province is in frame), and opens the order-type menu. On mobile, where the map isn't shown alongside the list, this navigates to the map screen instead.

The target province travels through a source search param that GameMap consumes once and clears, so the same URL-driven approach works whether the map is already mounted (desktop) or needs to mount fresh (mobile). Also fixes the order source not being highlighted on the map until an order type was chosen, and makes useOrderWizard's select/reset stable so the new effect's dependencies are correct.

A dependency of the above: adds a focusKeepZoom option to MapView → GameMapCanvas → GameMapController, so panning to a single province doesn't zoom the camera in sharply. Existing callers (the tutorial) are unaffected since it defaults to false.

orders-desktop orders-mobile

The unfilled "Army Liverpool — Order not provided" row above is now a clickable entry point (the mocked GET /options/ endpoint returns no choices, so clicking it doesn't visibly change anything under MSW — the interaction is otherwise the same as clicking the province on the map).

Adaptation notes

The original commits (from the closed branch) applied with one merge conflict and one drift-induced test failure, both fixed here:

  • OrdersScreen.tsx's OrderRow/muted logic had since gained a deletePending prop and an isActivePhase guard on main; resolved by keeping both that and the new onSelectProvince/onClick wiring.
  • canModifyOrders now requires game.currentPhaseId to match the selected phase (added to main after this feature was originally written) — the new tests' mock game data didn't set it, so the entry point never appeared. Added currentPhaseId: 1 to the mock.
  • useIsMobile's mount effect triggers an extra render, which clobbered a mockReturnValueOnce on an unrelated pre-existing delete-button test once this render count changed. Queued the mock value twice to survive both renders.

Test plan

Ran npx vitest run (full suite, 793 tests passing), npx tsc -b --noEmit (clean), and npx eslint on the touched files (clean).

🤖 Generated with Claude Code

JorenC and others added 3 commits September 25, 2026 10:33
Touching a province with "Order not provided" in the orders list now
starts the same order-creation flow as clicking it on the map:
selects it as the order source, pans the map to it (without zooming,
since only one province is in frame), and opens the order-type menu.
On mobile, where the map isn't shown alongside the list, this
navigates to the map screen instead.

The target province travels through a `source` search param that
GameMap consumes once and clears, so the same URL-driven approach
works whether the map is already mounted (desktop) or needs to mount
fresh (mobile). Also fixes the order source not being highlighted on
the map until an order type was chosen, and makes useOrderWizard's
select/reset stable so the new effect's dependencies are correct.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ky3hh38k8AqnwGr1Nq37F
focusProvinces() always fit the camera to the province's padded
bounding box, which zooms in sharply when framing a single province.
Add a keepZoom option so a caller can pan to a province without
changing zoom, threaded through MapView -> GameMapCanvas -> the
controller. Existing callers (the tutorial) are unaffected since the
option defaults to false.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ky3hh38k8AqnwGr1Nq37F
canModifyOrders now requires game.currentPhaseId to match the
selected phase (added on main after this feature was branched), and
useIsMobile's mount effect triggers an extra render that clobbers a
single-shot delete-mutation mock.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Screenshot diff

0 screens changed of 276 captured.

No visual differences found.

@JorenC JorenC left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

PR Review

This PR ports the order-click-to-wizard feature from a closed branch onto main, threading a source search param from OrdersScreen to GameMap and adding a focusKeepZoom option through MapView→GameMapCanvas→GameMapController. I traced the routing (Router.tsx, GameDetailLayout.tsx) and the wizard's selectedArray/resolvedSelections semantics (deriveWizardStep.ts) to confirm the search-param handoff and the "highlight source before order type is chosen" fix are both correctly implemented, and found no correctness bugs in the application logic — the adaptation fixes for drift (currentPhaseId, mock render count) are properly targeted. The two comments are minor, verified issues: a latent dependency-array gap that's currently inert, and a test-isolation risk from an un-guarded global mutation.

2 inline comment(s) (0 blocking, 2 suggestion).

return;
}
controller.focusProvinces(props.focus);
controller.focusProvinces(props.focus, 1.4, true, props.focusKeepZoom);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

💡 suggestion: props.focusKeepZoom is read inside the pan effect but isn't in its dependency array ([focusKey, provincePaths]), so a future caller that toggles this flag without changing focus would pan using a stale value. Harmless today since the only caller (GameMap.tsx) passes a hardcoded focusKeepZoom literal, but worth adding to the deps or at least the eslint-disable's rationale.

});

it("navigates to the map screen with the source search param on mobile", async () => {
window.innerWidth = 500;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

💡 suggestion: window.innerWidth is mutated directly and only restored at the end of the test (line 762); if the assertion on 758 throws, the reset is skipped and later tests in this file run against a mobile-width jsdom window. Consider restoring in a finally or afterEach.

JorenC and others added 2 commits September 25, 2026 14:58
Include focusKeepZoom in the pan effect's dependency array so a
future caller toggling it independently of focus doesn't pan with a
stale value. Reset window.innerWidth in afterEach instead of at the
end of the test, so a failed assertion can't leak a mobile viewport
into later tests in the file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@johnpooch johnpooch added the deploy-to-staging Trigger staging environment deployment label Sep 27, 2026
@github-actions github-actions Bot removed the deploy-to-staging Trigger staging environment deployment label Sep 27, 2026
@railway-app
railway-app Bot temporarily deployed to devoted-rejoicing / staging-pr-1401 September 27, 2026 11:58 Inactive
@github-actions

Copy link
Copy Markdown
Contributor

Staging Environment Deployed

Frontend preview: https://deploy-preview-1401--diplicity-react.netlify.app
Staging backend: https://diplicity-react-staging-pr-1401.up.railway.app

The staging backend has a copy of the production database from the daily snapshot taken at Sun, 27 Sep 2026 07:31:24 GMT, with migrations from this PR applied.
Use email/password login to test (Google OAuth is not configured for staging).

Log in as test-user@example.com / password to find seeded games in every state (pending, active, finished, civil disorder, draws, solo wins and more). The seed runs on the backend at startup, so the games appear once the backend is ready.

The frontend preview derives its backend URL from the PR number at build time.
Refresh the preview page once the staging backend is ready.
To redeploy, add the deploy-to-staging label again.

@johnpooch

Copy link
Copy Markdown
Owner

@JorenC This is cool. I like this feature.

Two thoughts:

  1. The map doesn't pan to the right position on mobile which means that the player can select a provice that is off the screen. For example, try giving an order to "Liverpool" on mobile.
  2. I think it feels weird that this is only usable when an order has not been provided. I think we should expand this to work when editing an existing order too.

Another nice touch would be for the province to be highlighted when the user hovers on the item on the orders panel.

This branch was previously deployed

1 inactive deployment
devoted-rejoicing / staging-pr-1401 — a1a96f93 Deployed Sep 27, 2026 by railway-app[bot]
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.

3 participants