feat(save-editor): edit what a merchant sells and how much ore he has - #84
Conversation
A merchant's shop is not his inventory. It lives in one global array (m_Traders) keyed by his unique name, and holds two maps: the live stock and the baseline he restocks toward. His ore sits in that same map as an ordinary line, because ore is the currency and what he holds is what he can pay with. Adds private.traders.list/.detail plus three edits: setStock (a bare i32 at the tail of its map entry, so it batches), and addItem/removeItem (structural splices, so they stand alone in their write and are listed in both the core guard and the app's splicingPaths). Two things the data forces: - Rows are addressed by array index, never by name. Two shipped rows are named `None`, are byte-identically long, and hold the same ore, so a name lookup has to refuse rather than guess. - A sold-out item is deleted from the map, not left at zero. setStock therefore refuses a line that does not exist and points at addItem instead of reporting success for a write it cannot do. Ore is optional: three merchants carry no ore line at all, which reads as null rather than zero so the UI does not claim they are broke. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… browsed Queued changes now sit above the list rather than inside it. An addition has no row yet and a removal's row is about to stop existing, so drawing either among the saved lines claimed a state the save does not have. Both use the same banner the inventory uses, extracted into pending_structural_row.dart. The two stock maps get a segmented switch instead of sitting on one page. That also settles which map the ore field belongs to: the ore card only appears for the live stock, where the number IS the merchant's purchasing power, and its line leaves the list so it is not shown twice. In the restock baseline ore is an ordinary row, because "purchasing power" means nothing there. Stock is grouped by item category behind the inventory's own sidebar, sorted by the localized name with the class id as tiebreak. Smaller corrections from testing against a real save: - The ore hint said what ore is for. It now says what the user cannot see otherwise: the in-game figure differs, because on load the game adds what accrued since the merchant's last trade. - Class ids followed the show-object-ids setting nowhere; now they do, and are dropped entirely when the title already is the id. - The character list badges a merchant, joined in the core so the list needs no second query. - The price note moved to the top, where it qualifies the ore as much as the counts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac681afa10
ℹ️ 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".
Two reports from the PR review. A count of 0 was accepted and written in place. Sold-out lines are deleted from the map rather than held at zero — no shipped or played save carries a zero-valued entry — so that write invented a record the game never produces. setStock now requires a positive count and names removeItem in the refusal, which is what "he no longer offers this" actually looks like. addItem already rejected zero for the same reason. The count field stops submitting it. The count field also kept the previous line's value. It refreshed only when the queued count changed, and rows carried no key, so the list reused one line's field state for whichever line landed at that position after a category or map switch — and submitting the leftover queued a setStock against the item now shown. Rows are keyed by item path, and the field now reacts to a changed saved value as well. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dca43e6870
ℹ️ 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".
…line removable Three reports from the second review pass. The core badges a merchant by matching lowercased names, while a character's unique name is the stored knowledge key where one exists — whose casing can differ from the trader row's. The app compared exactly, so a character the list badged as a merchant could open on "does not trade". It now folds case the same way the core does. Pulling the ore row out of the live-stock list left no way to drop it, and setStock no longer accepts zero — yet a merchant with no ore line at all is a state the game itself produces. The ore card carries a delete action now, and its field is disabled while that removal is queued. The count field accepted values above i32::MAX, which the core then refused at save time. It applies the same bound the add-item dialog already applies, so the refusal happens in the field instead of at the end of a save. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2897b09d21
ℹ️ 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".
apply_set_stock already refuses it; apply_add_item only refused a negative, leaving a Rust caller able to insert the zero-valued entry the module treats as an invalid state. The JSON parser was the only thing enforcing a positive count on that path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12a1091446
ℹ️ 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".
…led stock private.traders.detail compared trader names exactly, so a caller passing the uniqueName that private.characters.list returned — the stored knowledge key, whose casing can differ from the trader row's — got "no trader named" for a character the same list had marked a merchant. The Dart-side fold did not cover direct users of the command. The ambiguity check still runs, now on the folded comparison, so two rows differing only in case are still refused. A save whose per-difficulty stock is populated now disables editing and says why. The edits reach only m_Items and m_DefaultItems, so such a save would have taken a change, reported success, and left that stock standing. Empty in every save observed so far, which is exactly why it needed saying. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8274c5b8be
ℹ️ 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".
… the list away Replacing most of a merchant's stock queues enough banners to overflow the column they sat in as fixed children — 25 of them overflowed by 1484px — which hid the stock browser and took the buttons that cancel them with it. They now live in a height-capped scroll area above the list. The cap is a fixed value rather than a fraction of the pane: a LayoutBuilder placed directly in a Column is handed an unbounded height, so a fraction resolved to infinity and bounded nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1678fd7d7
ℹ️ 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".
…constants The banner cap was a constant, so it took no account of the header above it or the list below: a short window or a large UI scale overflowed the column and collapsed the browser. The cap now comes from the pane — a share of it, capped in absolute terms, and never so large that the list is left nothing. Measuring that share needs a bounded height, which a LayoutBuilder placed directly in a Column is not given; it sits at the section's root now, where the enclosing Expanded does bound it. The panel's own head had the same fault and overflowed a 620px-tall pane by 8px with nothing queued at all. It scrolls among itself once the pane no longer leaves the browser a usable slice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66943efe61
ℹ️ 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".
The app's own minimum is 960x600 (main.dart), where the character list leaves the detail pane about 164px wide — narrower than the ore card's field and delete button, and narrower than a stock row's ListTile trailing, both of which ask for a fixed ~180px and overflowed to the right. Both now lay their value under the name below a width threshold, and the field takes what is left instead of a fixed width. The regression test asks for that exact surface: the earlier one forced 1400x1000, which is why a panel that cannot fit the app's declared minimum still passed it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
A trader row is keyed by name alone, so it does not depend on a spawned actor — but the Characters tab sent every orphan to the generic no-actor pane, and the core hardcoded is_trader to false for them on the same unfounded assumption. A knowledge-only row whose name matches a trader was therefore unreachable and unbadged. Orphans reach the panel now, which already shows a clean non-merchant state when no row matches, and the core joins their names like everyone else's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
@cursor review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
… retry The core drops setStock from `writable` when no shop holds a line while still offering addItem, so a save full of empty shops announced that the editor could only read trader data — right beside a working Add button. The note waits until none of the three commands is available. The retry button on the load-error pane was the one Trade string still hardcoded in English. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f55892e8c
ℹ️ 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".
…missing One flag covers two shapes — a record with no stock map, and one whose descriptor the appliers cannot write — but the note named only the first. On a modded save with an empty map of another shape the behaviour was right and the explanation sent the reader looking for an absent property. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
@cursor review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0efdf4b4a0
ℹ️ 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".
Only setStock carried a target key, so a set placed before a removal of the same line passed the guards: the core applied the count and then deleted the line it lived in, reporting both edits as applied. A removal is just as declarative about one line, so it shares the key and the same-target rule rejects the pair. Different lines still carry different keys and keep batching. The editor never produced this pair — it drops a queued count when a removal is queued — but a caller coming straight through execute_json does not go through the editor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e4aa5cd7b
ℹ️ 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".
The character detail bar gained a sixth tab with Trade. A non-scrollable TabBar splits its width evenly, and the detail pane is only a fraction of the window, so each tab was left with far less room than a label needs and the labels clipped instead of shrinking. Below the breakpoint the tabs now carry their icon alone and name themselves on hover. Making the bar scrollable was the other option, but that hides four of the six tabs behind a horizontal scroll at every window size short of very wide, which trades a clipped label for a tab nobody finds. Tests that open a sub-tab go through a finder that accepts either form, since the test surface is always in the narrow regime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tooltip that names a label-less tab lands in the semantics node's `tooltip`, which platforms surface as help text rather than as the control's name, so the tab read out as "Tab 3 of 6". The icon now carries the name as its semantic label, where a labelled tab has it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Under 600px the stock pane drops the sidebar and lists every line at once. That list was passed straight through from the core, which orders by class id, while the grouped pane sorted by the localized name — so the one view with no categories to lean on was also the one whose order did not match the names on screen. The comparator now lives on its own and both views use it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@cursor review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8f20b44. Configure here.
|
@cursor review |
|
@codex review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8f20b44. Configure here.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
A merchant's shop is not his inventory. It lives in one global array
(
m_Traders) keyed by his unique name and holds two maps: the live stock andthe baseline he restocks toward. His ore sits in that same map as an ordinary
line, because ore is the colony's currency and what he holds is what he can pay
with.
Adds a Handel sub-tab between Inventar and Dialogwissen, and the core
commands behind it.
Core
private.traders.list/.detail, plus three edits:setStock— a bare i32 at the tail of its map entry, so it batches with peers.addItem/removeItem— structural splices, classified inmay_invalidate_caller_ordinalsandcarries_caller_ordinalso the core'sordering rule keeps them apart from index-addressed peers. The app mirrors
that classification, which is what decides how a save is split into writes.
Two things the data forces:
named
None, are byte-identically long and hold the same ore, so a namelookup refuses rather than guesses.
setStocktherefore refuses a line that does not exist and points at
addIteminsteadof reporting success for a write it cannot do.
Ore is optional: three merchants carry no ore line at all, which reads as
nullrather than0so the UI does not claim they are broke.private.characters.listgainsisTrader, joined in the core so the characterlist can badge merchants without a second query.
App
Stock is browsed through the inventory's own category sidebar, sorted by the
localized name. Queued additions and removals appear as banners above the list
— the shared
PendingStructuralRow, extracted from the inventory — because arow drawn among the saved lines would claim a state the save does not have.
A segmented switch selects one of the two maps. That also settles which map the
ore field belongs to: the ore card appears only for the live stock, where the
number IS the purchasing power, and its line leaves the list so it is not shown
twice.
Verified against a real save
Editing ore in
m_Itemsreaches the game, and the load-time regeneration isadditive: Fisk stored 12 → 777 in game (+765); after setting 40000 → 40765
in game, the same +765. So the value is the merchant's liquidity, and it is not
re-derived from
m_DefaultItems— the failure mode the faction-hostility workwarned about does not apply here.
Also verified on a copy of a real save: a batched
setStockpair, an insert, aremoval, that add+remove is byte-identical to a no-op, and that neighbouring
merchants are untouched.
Not included
Prices. The factor lives in AngelScript (8 merchant types × 18 item tags) and
in the difficulty settings, not in the savegame.
Tests
435 Rust (incl. an integration suite that runs against the embedded start save,
so it needs no game install), 557 Flutter,
flutter analyzeclean.🤖 Generated with Claude Code
Note
Medium Risk
Changes go through the save-game write path and new batching rules; wrong combinations could corrupt the wrong merchant row, though explicit conflict detection and index-only addressing mitigate that.
Overview
Adds a Handel / Trade sub-tab on the character detail pane so merchants can be edited outside their personal inventory: live stock, restock baseline (
m_DefaultItems), ore (purchasing power), and add/remove stock lines viaprivate.traders.*commands.The UI loads trader list/detail through new notifier APIs, queues edits with stable per-line pending keys, and shows queued structural changes in shared
PendingStructuralRowbanners (moved out of inventory). Save batching treatsaddItem/removeItemas splices;traderArrayConflictblocks saves that mix trader edits with rawm_Tradersarray operations, which would target the wrong row after renumbering.Character list rows gain
isTraderand a storefront badge (including orphans). The detail tab bar grows to six tabs with responsive icon-only labels on narrow widths.Reviewed by Cursor Bugbot for commit 8f20b44. Bugbot is set up for automated code reviews on this repo. Configure here.