Skip to content

Purchase UX: lock the crate, report in-page, deliver safely (#106) - #108

Merged
CaYatur merged 2 commits into
mainfrom
fix/purchase-delivery
Jul 28, 2026
Merged

Purchase UX: lock the crate, report in-page, deliver safely (#106)#108
CaYatur merged 2 commits into
mainfrom
fix/purchase-delivery

Conversation

@CaYatur

@CaYatur CaYatur commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Purchase UX: lock the crate, report in-page, deliver safely (#106)

Four problems with buying something, from the click to the item landing in
an inventory.

The crate modal could be dismissed mid-roll. The animation runs for as long
as the crate's own setting says, and until it finishes the reward is only on
the server; clicking the backdrop threw away the one screen that ever showed
what was won. crateLocked is set when the roll starts and cleared a beat
after it resolves; closeCrate returns early while it is set, and the OK
button carries .is-waiting so the modal does not look frozen.

Purchase results came out of alert(). Replaced with an in-page toast stack
(sfNotice) that renders the product's own icon at item size, so a bought
item looks like the thing that was bought rather than a browser dialog.

Delivery was unsafe in both directions. The old rule ran the reward commands
if the buyer's name was in the console's online list, else queued and fired
1500ms after the join line. On a server with a login plugin that is the worst
moment to give: before /login the player is in a holding state where the
item drops on the floor, and the plugin usually restores a saved inventory
straight after, overwriting whatever arrived. Worse, runCommands returned
void and did nothing when neither RCON nor stdin was up — while the caller
had already dequeued the entry, so the reward simply vanished.

src/shared/delivery.ts now decides from evidence: the server must be up and
reachable, the player connected, and then either the bridge can see them in a
world or online-mode=true makes the session trustworthy. A cracked server
with no bridge holds for operator approval by default. Every branch either
delivers or keeps the reward; a 32-combination sweep in the smoke asserts no
input can drop one. runCommands returns a boolean and the queue entry is
only removed once it has.

Delivered rewards now announce themselves in game (tellraw, JSON-escaped),
on the spot or on the join that releases them, so a player who bought
something while offline finds out it arrived.

New: GET /servers/:id/store/admin/pending and POST /servers/:id/store/admin/deliver
for the held queue. Grace and the hold-when-unverified switch are config
(store.deliveryGraceMs, store.holdUnverifiedDeliveries).

Four problems with buying something, from the click to the item landing in
an inventory.

The crate modal could be dismissed mid-roll. The animation runs for as long
as the crate's own setting says, and until it finishes the reward is only on
the server; clicking the backdrop threw away the one screen that ever showed
what was won. `crateLocked` is set when the roll starts and cleared a beat
after it resolves; `closeCrate` returns early while it is set, and the OK
button carries `.is-waiting` so the modal does not look frozen.

Purchase results came out of `alert()`. Replaced with an in-page toast stack
(`sfNotice`) that renders the product's own icon at item size, so a bought
item looks like the thing that was bought rather than a browser dialog.

Delivery was unsafe in both directions. The old rule ran the reward commands
if the buyer's name was in the console's online list, else queued and fired
1500ms after the join line. On a server with a login plugin that is the worst
moment to `give`: before `/login` the player is in a holding state where the
item drops on the floor, and the plugin usually restores a saved inventory
straight after, overwriting whatever arrived. Worse, `runCommands` returned
void and did nothing when neither RCON nor stdin was up — while the caller
had already dequeued the entry, so the reward simply vanished.

`src/shared/delivery.ts` now decides from evidence: the server must be up and
reachable, the player connected, and then either the bridge can see them in a
world or `online-mode=true` makes the session trustworthy. A cracked server
with no bridge holds for operator approval by default. Every branch either
delivers or keeps the reward; a 32-combination sweep in the smoke asserts no
input can drop one. `runCommands` returns a boolean and the queue entry is
only removed once it has.

Delivered rewards now announce themselves in game (tellraw, JSON-escaped),
on the spot or on the join that releases them, so a player who bought
something while offline finds out it arrived.

New: GET /servers/:id/store/admin/pending and POST /servers/:id/store/admin/deliver
for the held queue. Grace and the hold-when-unverified switch are config
(`store.deliveryGraceMs`, `store.holdUnverifiedDeliveries`).
Copilot AI review requested due to automatic review settings July 28, 2026 17:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Two ways the queue still lost a paid-for reward.

`attemptDelivery` persisted on hold and delivered on deliver, and on `wait` it
did neither — the entry lived in a setTimeout closure and nowhere else, so
quitting during the grace lost it. The 32-combination sweep could not see this:
it tests the pure decision function, and the loss was in the caller.

`queueReason()` now states the rule once — null only for a hand-over — and the
caller persists whenever it returns anything. A waiting entry queues as
`just-joined`, and a `scheduled` set stops a join mid-grace from delivering it a
second time. The sweep asserts the rule and now sweeps joinedAgoMs, the axis the
wait branch actually depends on.

`runCommands` returned true whenever `isConnected` was true, ignoring that
`rcon.tryCommand` answers null on failure. A connection dropping between the
check and the send made the caller dequeue a reward that was never given —
the exact bug this branch was written to close. Each command is now checked, and
a failed RCON send falls back to stdin while the process is up.
@CaYatur

CaYatur commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Self-review

Two defects, both of them the same defect this PR exists to fix, reintroduced
one layer below where I was looking.

1. The wait branch kept a paid-for reward in a setTimeout closure

attemptDelivery persisted on hold and delivered on deliver, and on wait
it did neither — it scheduled a retry and returned. So a purchase made inside
the grace window after a join existed only in a timer. Quitting the app during
that window (the grace is deliberately as long as a login plugin needs, and
defaults to 20s) lost the reward: money deducted, nothing owed on disk.

That is exactly the failure the module's own header claims it prevents, and the
32-combination sweep could not see it, because the sweep tests the pure decision
function and the loss happens in the caller.

Fixed by making the durability rule a function rather than a habit:
queueReason(decision) returns null only for a hand-over, and attemptDelivery
persists whenever it returns anything. wait now queues with a new
'just-joined' reason, so the panel's pending list tells the truth about it.

Persisting a waiting entry makes it visible to deliverQueued, so a join
arriving mid-grace would have run the same commands twice. A scheduled set
keyed by server+entry id gives the timer ownership until it fires. It is
in-memory on purpose: after a restart there is no timer, so the persisted entry
must be free for the next join to pick up.

The sweep now asserts the rule and was proved able to fail — reverting
queueReason's wait branch to null produces
FAIL - a "wait" decision named no queue reason, so nothing would persist it.
It also now sweeps joinedAgoMs, which it held fixed before; that is the axis
the wait branch depends on, so without it the sweep never reached the branch
at all.

2. runCommands reported success when RCON silently dropped the command

rcon.tryCommand returns null on failure — it is the "try" variant, correct
for a fire-and-forget console command and wrong here. runCommands ignored the
result and returned true because isConnected was true at the top. So a
connection that dropped between the check and the send made attemptDelivery
call removeFromQueue for a reward that was never given.

The whole PR is about not dequeuing something that did not land. I closed that
hole in the caller and left it open in the function the caller trusts.

Now each command is checked, and a failed RCON send falls back to stdin when the
process is up rather than giving up — a dead socket to a live server is a reason
to use the other channel. Only when neither carries it does it return false,
which holds the entry.

Not covered by a test: forcing "RCON connected but the send fails" needs a live
server plus a socket killed mid-command, which no existing gate provides. The
partial-delivery case is a real residual — commands 1 and 2 land, 3 does not,
the entry is held, and releasing it re-runs all three. Duplicating an item is
the better failure than losing one, and it is visible in the pending list, but
it is a trade rather than a fix.

Verified

Twelve gates green after the fixes. queueReason also removed the duplicated
reason-mapping that would otherwise sit in both the wait and hold branches.

@CaYatur
CaYatur merged commit afe4a13 into main Jul 28, 2026
1 check passed
@CaYatur
CaYatur deleted the fix/purchase-delivery branch July 28, 2026 17:09
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.

2 participants