Skip to content

fix: repair the negotiation system (pricing, wallet guard, stable NPC) + one-shot-per-car rule - #62

Open
wpinrui wants to merge 16 commits into
mainfrom
fix/wallet-traits-lowdpi
Open

fix: repair the negotiation system (pricing, wallet guard, stable NPC) + one-shot-per-car rule#62
wpinrui wants to merge 16 commits into
mainfrom
fix/wallet-traits-lowdpi

Conversation

@wpinrui

@wpinrui wpinrui commented Mar 24, 2026

Copy link
Copy Markdown
Owner

Description rewritten 2026-07-13. The original description advertised three cosmetic fixes. The branch is 15 commits and contains a negotiation pricing overhaul, a gameplay rule change, and a debug panel — none of which were mentioned. What follows is an honest account of the whole branch. Review against this, not the old text.

What this PR actually is

This is the repair for the negotiation system, which is currently shipped and broken on main in both directions at once — you can buy cars you cannot afford (going to negative money) and be silently overcharged on Accept. It is not a polish PR.

Scope: 15 commits, 19 files, src/ only. No data, config, or docs touched.

Bugs on main that this fixes

# Bug on main Fix here
1 No wallet guard. closeNegotiation deducted the price with no affordability check — you could buy a car you couldn't afford and go to negative money. Affordability checks in submitOffer, acceptAtListPrice, and closeNegotiation, each raising a "You can't afford this." toast. The Accept button is also disabled when the player can't cover the asking price.
2 The asking price is a lie. The browse card showed listing.askingPrice; the modal labelled npc.targetPrice as "Asking price". These are unrelated numbers (targetPrice was market value × ~1.15). The player was quoted two different prices for the same car. NPC pricing is re-anchored to listing.askingPrice. calculateNpcPricing() now takes an anchorPrice, and NegotiationItem carries askingPrice.
3 The Accept button charged the wrong amount. The button read Accept $<last counter> but acceptListPrice() charged npc.targetPrice — always higher. Silent overcharge. Both the label and the charge now use item.askingPrice.
4 Trait badges rendered raw IDsimpatient instead of Impatient. Uses getTraitDefinition(traitId).name.
5 NPC regenerated on every open. RNG was seeded on action count, so re-opening the same listing produced a different seller with different traits and prices. Listing-ID hash seed → stable NPC per car.

⚠️ Gameplay rule change: "one shot per car"

This is new, it is not a bug fix, and it was not in the original description.

closeNegotiation now removes the listing from the market whenever the negotiation ends in anything other than an accepted sale (store/index.ts). Because Walk Away is wired directly to closeNegotiation, this means:

  • Walking away from a haggle permanently destroys that listing.
  • The NPC walking away on you permanently destroys that listing.

You get one negotiation per car, ever. This is a real design decision that deserves a real decision — flagging it loudly rather than letting it merge as an implementation detail.

⚠️ Debug panel included

DebugPanel.tsx / .css ship in this branch: an on-screen money setter, plus a debugSetMoney store action. It is gated on const DEBUG_MODE = truehardcoded in store/index.ts, not import.meta.env.DEV — so it renders in a production build as written.

The owner has seen this and chosen to keep it. Noted here so nobody merges it by accident.

Pricing overhaul detail

The load-bearing change; everything else in negotiation sits on top of it.

  • targetMultiplier base 1.15 → 1.0 — the NPC now targets the asking price rather than a markup over market value.
  • walkAwayPrice is clamped to at most 90% of the anchor: min(anchor × walkAwayMultiplier, anchor × 0.90). Base walkAwayMultiplier is still 0.80 and traits still move it; the clamp is an upper bound only, guaranteeing at least a 10% negotiation band on every car.
  • marketValue is still carried on NegotiationItem but no longer drives pricing.

UI polish

  • Toasts moved to bottom-centre with a slide-up animation.
  • Modal overlay opacity 0.6 → 0.25 across all five modals (two commits: 0.6 → 0.4, then → 0.25).
  • Scroll-container bottom-padding collapse fixed via ::after spacer (Newspaper, Browse Results).
  • Low-DPI map cards: removed overflow: hidden from .loc, border-radius moved onto the photo, responsive grid minimum min(390px, 100%), min-height: 0 on .map-body.

Closes #52.

Notes for the reviewer

  • traitVisibilityPerPoint is net-zero here. One commit raised it 0.01 → 0.10, a later commit reverted it. No data file is touched. Don't chase it through the commit list — the balance question is being ticketed separately.
  • A local two-dot diff (git diff main..) will show spurious .claude/ deletions. The branch predates PR chore: track .claude agent config in git #63; it never touched those files. A three-dot diff (git diff main...) — which is what GitHub's Files-changed tab shows — confirms src/-only, 19 files.

Known gaps this does not close

Real, on main, and out of scope — do not block on them, they have their own tickets:

  • You still cannot accept an NPC's counter-offer. Accept takes the asking price; counters descend below asking, so accepting is always worse than the seller's own latest offer. The workaround is to retype the counter as your own bid.
  • Cars can never be refuelled, so no car can ever be driven.
  • Fitness has no effect on labor earnings or rest efficiency.
  • negotiate_junker_purchase is a permanently dead activity card.

Test plan

Money path — the two financial bugs:

  • Try to buy a car you cannot afford (exact money, slightly under, well under) — purchase is blocked with a "You can't afford this." toast, and money never goes negative.
  • "Accept $X" is disabled with a tooltip when unaffordable.
  • The amount charged equals the number printed on the Accept button. No silent overcharge.
  • The price on the browse card equals the price in the negotiation modal.

Negotiation mechanics:

  • Close and re-open the same listing — the same NPC returns, with the same name, traits, and prices.
  • Trait badges read Impatient, not impatient.
  • Counter-offers stay above the walkaway floor and the band is never degenerate.

"One shot per car" (new rule — assess how it feels, not just whether it works):

  • Walk away from a negotiation → the listing is gone from the market.
  • The NPC walks away on you → the listing is gone.
  • Is losing a car permanently to a failed haggle tense or punishing? This is a product signal — say so either way.

UI:

  • Location cards show full content (photo + name + tags + travel info) at various viewport sizes and zoom levels.
  • Toasts appear bottom-centre; modals are legible at the new 0.25 overlay opacity.

@wpinrui wpinrui added the bug Something isn't working label Jul 13, 2026
@wpinrui wpinrui changed the title fix: wallet guard, trait badges, low DPI map cards fix: repair the negotiation system (pricing, wallet guard, stable NPC) + one-shot-per-car rule Jul 13, 2026
wpinrui added a commit that referenced this pull request Jul 13, 2026
Tricia previously guided the owner through long manual checklists. Her last
plan for PR #62 ran to ~20 items and required a specific stat build, a
page-refresh trick to escape a screen, and carried a warning that one check
might never become reachable. That is not a viable use of the owner's time.

Automation is now the default. Every check is triaged Automated or Manual;
Manual requires a stated reason, and the only valid ones are spatial layout,
colour/legibility, responsive reflow, clipping, animation/audio, and feel.
"Awkward to automate" is explicitly not a reason -- it now routes to the
Implementer as a testability defect rather than becoming owner homework.

The manual pass is capped at roughly eight items, each requiring exact
navigation, exact setup via the debug panel rather than grinding, and a stated
pass condition. Feel questions are separated from the checklist and routed to
the PM as product signal. Tests may never be weakened, skipped, or deleted to
get a green run.

Permission boundaries widen accordingly: Tricia may write test files, test
config, and test-only devDependencies, and may commit and push tests to the PR
branch under test. She may not touch application source, app dependencies, or
build config.

Project notes record that src/engine/ is pure and the RNG is seeded and
injectable, so negotiation and listing-lifecycle tests are deterministic with
zero new dependencies, and that bare `npm test` starts watch mode and hangs.
First automated tests in the repo. They assert the five fixes this PR claims,
plus the new one-shot-per-car rule:

- the wallet guard: no purchase path can drive money negative
- Accept deducts exactly the asking price printed on the button
- NPC pricing is anchored to listing.askingPrice, not to market value
- the walkaway floor never exceeds 90% of asking, for every legal trait set
- counter-offers stay inside the seller's own band
- the seller is stable per listing across intervening days and actions
- a failed negotiation removes the listing from the market

Each test was checked against a reintroduction of the bug it covers, so a
regression turns it red rather than leaving it a happy-path pass.

vitest.config.ts is kept separate from vite.config.ts so test runs don't drag
in the Electron plugins. Engine data is served from data/ through a
window.electronAPI stub in the setup file. No new dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Low DPI map card cutoff

1 participant