feat(licensing): watch for the purchase after buy() and write the licence automatically - #1008
Conversation
…ence automatically Buying happens in an external browser tab, and until now nothing brought the result back: the editor kept showing "Not licensed" until the user guessed they should click "Check again". The purchase flow ended in a dead end at its most important moment. buy() now opens the purchase page AND starts a purchase watch (deviceLicense.awaitingPurchase). While it runs, the existing refresh() executes every 20s — one Modbus read frame plus one HTTP round-trip — and on the first tick after the completion webhook lands, that same refresh() activates the licence and WRITES the blob to the device, no click required. The watch ends on the first licensed report (whoever produced it: the poll, a manual re-check, the connect flow), after a 30-tick / 10-minute budget, on "Stop waiting", or when the board is switched / disconnected (clearDeviceLicense). Badge behaviour while waiting: - reads "Waiting for purchase…" steadily — it outranks the isChecking flicker each tick would otherwise cause; - withdraws "Buy licence" — offering it mid-wait invites a double buy; - offers "Stop waiting". Ticks that would overlap a call still in flight (slow device, 30s HTTP timeout) are skipped instead of stacking a second call on the same link. The interval calls refresh through a ref: its identity follows the device port, and an interval keyed on it would reset the tick budget on every change. Companion change (autonomy-edge, EDGE-593): the /buy page now polls the same truth and shows "License issued" only when the webhook actually landed it. Verified: 7 new hook tests (watch start, no-URL no-watch, per-tick refresh, overlap skip, licensed ends it, tick budget, cancel), 6 new badge tests, 4 new slice tests; device-slice 127/127, badge 26/26, tsc --noEmit clean, eslint 0 errors (the 4 unbound-method warnings in use-device-license.ts pre-date this change, same count). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015uUH3ZL5ehreMUf2dtanWD
WalkthroughThe device license flow now stores a purchase deadline, polls for licensing completion, supports cancellation, and updates the board UI with waiting status and automatic license-writing messaging. ChangesDevice license purchase watch
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DeviceLicenseStatus
participant useDeviceLicense
participant DeviceStore
participant LicensingAPI
User->>DeviceLicenseStatus: Select purchase
DeviceLicenseStatus->>useDeviceLicense: Open purchase URL
useDeviceLicense->>DeviceStore: Set purchase-watch deadline
loop Until licensed, cancelled, or deadline expiry
useDeviceLicense->>LicensingAPI: Refresh license
LicensingAPI-->>useDeviceLicense: License report
end
useDeviceLicense->>DeviceStore: Clear watch when licensed
User->>DeviceLicenseStatus: Stop waiting
DeviceLicenseStatus->>useDeviceLicense: Cancel purchase watch
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/hooks/__tests__/use-device-license.test.ts`:
- Around line 10-19: Remove the fixture’s type assertions by defining explicit
MockState and license-state types, then type mockUseOpenPLCStore as a Jest mock
rather than casting through unknown. Initialize BOARD with all required
BoardInfo fields—compiler, core, preview, and specs—and update setLicenseState
to use the typed value directly instead of an as object assertion.
In `@src/frontend/hooks/use-device-license.ts`:
- Around line 206-210: Update the purchase-opening flow in the hook containing
setAwaitingPurchase: synchronously guard with a purchaseOpeningRef before
awaiting system.openExternalLink, return early when that ref is set or the store
already has an active purchase watch, and clear the ref in a finally block while
preserving the existing awaitingPurchase update.
- Around line 161-162: Update refreshRef in the useDeviceLicense hook by moving
refreshRef.current = refresh into a committed useEffect, ensuring the interval
only observes refresh from committed renders while preserving the existing ref
usage.
- Line 175: Update the polling invocation in the effect around
refreshRef.current() to attach a rejection handler for promises rejected outside
run(). Preserve the existing fire-and-forget behavior while ensuring rejected
refresh promises are handled.
In `@src/frontend/store/slices/device/slice.ts`:
- Around line 613-621: Reset deviceLicense.awaitingPurchase to false in
clearDeviceDefinitions and the changed-board reset branch of setDeviceBoard,
matching clearDeviceLicense. Ensure every direct license-reset path clears the
purchase watch while preserving the existing phase and report resets.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ff0be0b-76b1-455a-b04b-d876d07e9853
📒 Files selected for processing (8)
src/frontend/components/_features/[workspace]/editor/device/configuration/__tests__/device-license-status.test.tsxsrc/frontend/components/_features/[workspace]/editor/device/configuration/board.tsxsrc/frontend/components/_features/[workspace]/editor/device/configuration/components/device-license-status.tsxsrc/frontend/hooks/__tests__/use-device-license.test.tssrc/frontend/hooks/use-device-license.tssrc/frontend/store/__tests__/device-slice.test.tssrc/frontend/store/slices/device/slice.tssrc/frontend/store/slices/device/types.ts
The format check runs prettier --check over src/**; the multiline className in the waiting-state badge was hand-wrapped differently than prettier wants it. No behavioural change — the badge suite still passes 26/26. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015uUH3ZL5ehreMUf2dtanWD
JulioSergioFS
left a comment
JulioSergioFS
left a comment
There was a problem hiding this comment.
PR Review — openplc-editor #1008
Verdict: Request changes
The dead end this closes is real, and the design decisions are mostly the right ones for the right reasons — ending the watch on the report rather than on the poll's own return value, calling refresh through a ref so the tick budget survives a port change, letting the store action stay dumb while the hook owns the lifecycle. Those are the non-obvious calls and they were all made correctly.
What blocks is finding 1: the watch does not end when the board is switched or the project is closed, contrary to what the description says, and in that state it keeps running licensing sequences — including potential device writes — against a board the user never asked to buy for, with no UI left to stop it. The tests that would have caught it were updated in a way that guarantees they can't.
What I verified against the code
clearDeviceLicensehas exactly one call site,use-device-connect.ts:182, on deliberate Disconnect. A link that merely drops does not go through it, by design (clearDeviceConnection's comment).Check againis alreadydisabled={isChecking}, so the manual-vs-poll race against main'sdeviceLicenseSequenceInFlightguard (which returnscheck-failed: "A license check is already running on this device") is closed at the button. Good — that would otherwise have clobbered the last good report with a spurious failure.- The overlap guard reads live state, not a stale closure.
useOpenPLCStore.getState().deviceLicense.phaseinside the interval is correct; a capturedphasewould have been permanently stale given the effect's deps. startCheck()is synchronous beforerun()'s first await, so the guard actually latches within the same timer batch. This matters for finding 5.- The ref rationale holds.
refresh→rundepends ondevice.readLicense/refreshLicenseandtarget, both of which follow the port and the board, so an interval keyed onrefreshreally would rebuild and resettickson every port change. - The per-tick cost is larger than the description says — see finding 9.
- The tick-budget arithmetic matches the test.
ticks += 1thenif (ticks > 30)means ticks 1–30 refresh and tick 31 closes the watch; the test'stoHaveBeenCalledTimes(MAX_TICKS)is consistent.
Strengths
- Ending the watch on the report, not on the poll's return value. This is the seam that lets a manual "Check again" or the connect flow conclude the purchase, and it is the difference between a poll and a state machine. The comment says exactly this, in the right place.
- The badge-label precedence is argued and tested, not asserted. "Every tick flips
isChecking, so alternating labels read as flapping when it is one continuous wait" is a real observation about a real artefact, and there is a test named after it. - Withdrawing "Buy licence" mid-wait. The double-purchase risk is the expensive failure in this flow, and it is handled — with a test.
- The slice action is deliberately dumb, and there is a test pinning that landing an unlicensed report does not end the watch. That test is the one that stops a future refactor from making the first poll tick kill the watch it serves.
- The store comment on
clearDeviceLicensestates the intent ("a watch for a device that is no longer there would keep hitting it over a dead link") — which is exactly right, and exactly why finding 1 matters.
Findings
1. (High) The watch survives a board switch and a project close, and keeps running licensing sequences against the new board
clearDeviceLicense was updated to clear awaitingPurchase (slice.ts:621). But it is not the path a board switch or a project close takes. Both reset the licence inline and were not updated:
setDeviceBoard—slice.ts:417-418setsdeviceLicense.phase = 'idle'andreport = nullwhen the board actually changes.awaitingPurchaseis untouched.clearDeviceDefinitions—slice.ts:152-153, same two lines, same omission.
So: start a watch, switch the board (or close the project). The resulting state is { phase: 'idle', report: null, awaitingPurchase: true }. From there:
DeviceLicenseStatusreturnsnullbecausereportis null — the badge disappears, and with it the "Stop waiting" button. The user has no way to cancel a watch they can no longer see.- The interval in
useDeviceLicensekeys only onawaitingPurchase(use-device-license.ts:164, 178), so it keeps ticking, unaffected. refreshRef.currenthas meanwhile followedboardInfo→target→ the new board'spackageId(use-device-license.ts:161-162). Every 20 seconds for the next 10 minutes, the editor runshandleDeviceRefreshLicensefor the new VPP against the connected device: read the board-id anchor, ask the backend for entitlement, and — if one exists — write a licence blob to the device and read it back.
That last part is the serious half. The user switched boards; nothing they did asked for a licensing write on the new one, and there is no visible affordance telling them it is happening or letting them stop it. It ends only at tick 31.
The description states: "The watch ends … when the board is switched / disconnected (clearDeviceLicense)." The parenthetical is the bug — board switch does not go through clearDeviceLicense.
Fix: clear awaitingPurchase in setDeviceBoard's change branch and in clearDeviceDefinitions. Better, since this is the third copy of the same three-line reset: extract a resetDeviceLicense(deviceLicense) helper in the slice so a fourth reset path can't drift again — that drift is precisely what happened here.
2. (High) The two slice tests that cover those paths cannot fail
Both were touched by this PR, and both were made to assert awaitingPurchase: false without ever setting it to true:
it('clearDeviceBoard change drops the licence …')
setDeviceLicenseReport(LICENSED) // never setAwaitingPurchase(true)
setDeviceBoard('Raspberry Pi 4')
expect(deviceLicense).toEqual({ phase:'idle', report:null, awaitingPurchase:false })
The false they assert is the slice's initial value. Same for the clearDeviceDefinitions case. Meanwhile the one path that was fixed got a purpose-built test that does set the flag first ("clearDeviceLicense ends the watch").
This is worth calling out separately from finding 1, because the test edits look like coverage — a reviewer scanning the diff sees awaitingPurchase: false asserted on all three reset paths and concludes all three are handled. Whatever the fix, both tests need setAwaitingPurchase(true) before the act, so the assertion is about the reset rather than about the default.
3. (Medium) The badge hides a failing check for the full ten minutes
const badgeLabel = awaitingPurchase ? 'Waiting for purchase…' : isChecking ? 'Checking licence…' : label
awaitingPurchase outranks everything, including check-failed. And negative && !awaitingPurchase strips the dashed underline, which is the only affordance saying "there is something to read in here."
So if the link goes flaky mid-watch — a Modbus timeout, requireControl failing without a deliberate disconnect, a collision with main's deviceLicenseSequenceInFlight — every tick lands a check-failed report and the user reads a calm "Waiting for purchase…" for ten minutes, with the visual cue that would invite them to open the panel actively removed.
The comment above the interval claims the opposite:
refreshreports its own failures ascheck-failedreports, so a flaky tick shows in the badge instead of silently killing the watch.
The first half is true (the report records it). The second is not: the badge is exactly where it does not show.
This is the same failure mode #358 exists to eliminate on the web side — telling someone who just paid that things are fine when they may not be. The editor should hold the same line. Either let check-failed outrank the waiting label, or compose them ("Waiting for purchase — last check failed"), or at minimum keep the dashed underline so the panel still reads as worth opening. Whichever is chosen, the comment needs to match.
4. (Medium) buy() starts the watch even when the browser never opened
await system.openExternalLink(url)
setAwaitingPurchase(true)
SystemPort.openExternalLink returns Promise<{ success: boolean }> (system-port.ts:46). The result is awaited and discarded.
On a failed launch — no handler registered, a sandboxed environment, the accelerator path failing — the user gets no purchase page and a ten-minute "Waiting for purchase…", with "Buy licence" withdrawn (finding: correctly, per the double-buy rule) in the one state where they most need it back. The only escape is to find and click "Stop waiting" in a panel that is telling them to wait.
Guard on success, and on failure leave the badge alone so the Buy button stays offered. The hook's own test mocks { success: true }, so a { success: false } case is a two-line addition to the existing suite.
5. (Medium-low) Two live instances of the watch per board screen
board.tsx mounts useDeviceLicense(currentBoardInfo) directly at line 69, and useDeviceConnect(currentBoardInfo) at line 65, which mounts its own useDeviceLicense(boardInfo) (use-device-connect.ts:45). Both instances now run the interval effect against the same shared awaitingPurchase flag.
In practice the second one is saved by the overlap guard: both timers were created in the same render and fire in the same timer batch, and run() calls startCheck() synchronously before its first await, so instance A flips phase to 'checking' before instance B's callback executes. B skips.
But that is an accident of timer batching plus where startCheck() happens to sit — not a designed invariant, and nothing documents or tests it. Move startCheck() after an await, or introduce any asynchrony before it, and the editor starts firing two overlapping licensing sequences per tick on the same Modbus link; main's deviceLicenseSequenceInFlight would then catch it and land a check-failed (masked by finding 3).
Meanwhile instance B silently burns its entire 30-tick budget doing nothing.
Cleanest fix is a single owner: hoist the watch effect out of useDeviceLicense into whichever component owns the screen, or make the hook take a flag for whether this instance drives the watch. Failing that, at minimum a test that asserts one tick produces exactly one refresh when the hook is mounted twice.
6. (Low) The overlap skip spends tick budget
ticks += 1 runs before the phase === 'checking' check (use-device-license.ts:167 vs :174), so a skipped tick still costs a tick. A device slow enough to straddle intervals — the exact case where a longer window is most warranted — gets a window shorter than the advertised ten minutes, and with the duplicate mount in finding 5, instance B's budget is consumed entirely by skips.
Move the increment below the guard, or count only ticks that actually issued a refresh.
7. (Low) The ten-minute budget is not a bound — it resets on remount
ticks is effect-local. Navigating off the device screen tears the interval down but leaves awaitingPurchase: true in the store; navigating back mounts a fresh effect with ticks = 0 and a full new budget. A user cycling between tabs keeps the watch alive indefinitely, and there is no state in which "the watch has been running for 10 minutes" is actually known.
An absolute deadline in the store — awaitingPurchaseUntil: number set by buy(), checked by the effect — bounds it for real, survives unmount, and makes the intent inspectable in devtools. It also gives finding 1 a natural backstop.
8. (Low) The first check is 20 seconds away, always
Nothing fires immediately on watch start or on remount; the interval's first callback is at t+20s. Paddle's completion webhook frequently lands before the user has switched back to the editor, so the common happy path pays a full interval for a result that was already available.
One immediate tick on effect start (subject to the overlap guard) makes the best case feel instant and costs one extra request per watch.
9. (Nit) The per-tick cost in the description understates what runs
While it runs, the existing
refresh()executes every 20s — one Modbus read frame + one HTTP round-trip.
handleDeviceRefreshLicense reads the board-id anchor (readLicenseAnchor, one frame) and then runs resolveDeviceLicense, which is read → HTTP → write → read-back per its own docblock at main.ts:2438. All of it rides the control channel behind the frame mutex, competing with run/stop and the liveness status poll.
Not a defect — the mutex serialises it correctly and noteTraffic() keeps the liveness poll from declaring the link dead mid-sequence, both of which are handled. But the cost is worth stating accurately in the description, because it is the number someone will reach for when they next tune the interval.
Summary of what I'd fix before merge
| # | Severity | Fix |
|---|---|---|
| 1 | High | Clear awaitingPurchase in setDeviceBoard and clearDeviceDefinitions (extract one reset helper) |
| 2 | High | Make the two slice tests set the flag before asserting it was cleared |
| 3 | Medium | Don't let the waiting label mask check-failed; fix the contradicting comment |
| 4 | Medium | Only start the watch when openExternalLink reports success |
| 5 | Medium-low | One owner for the watch effect, or a test pinning one-tick-one-refresh |
| 6–9 | Low / nit | Tick accounting, real deadline, immediate first tick, description accuracy |
Note that findings 1, 3 and 4 share a shape: each is a state where the UI keeps saying "waiting for purchase" while something other than a pending webhook is going on. Given the companion PR's entire thesis is that a post-payment screen must report what is actually true, they're worth fixing together.
Mirror: every finding here ships byte-identically to openplc-web#669 (all five product files verified sha256-identical). Fixes must land here and be re-mirrored; #669 cannot diverge.
Review fixes on the post-purchase watch: - reset the watch on EVERY licence reset path: a real board change and project close previously left it polling (and potentially writing a licence) against the next board's package id. One resetDeviceLicense helper now serves all three reset paths so a fourth cannot drift. - replace the 30-tick budget with an absolute deadline persisted in the store (awaitingPurchaseUntil, PURCHASE_WATCH_WINDOW_MS): a remount resumes the SAME window, an overlap-skipped tick costs nothing, and the state is inspectable. - let a check-failed report outrank the "Waiting for purchase..." badge label so a dead link cannot hide behind a calm wait for ten minutes; the failure label holds steady across poll ticks. - start the watch only when openExternalLink actually opened the page, and fire the first check immediately instead of at t+20s. - give the watch a single owner (the board screen passes ownsWatch) so the useDeviceConnect instance no longer double-polls the same flag. - product-neutral panel copy: "OpenPLC checks periodically...". - slice tests arm the watch before asserting the resets, so the two reset assertions can actually fail (verified by mutation). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015uUH3ZL5ehreMUf2dtanWD
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/store/slices/device/slice.ts`:
- Line 435: In the previousBoard !== deviceBoard branch around
resetDeviceLicense, call projectActions.syncVariableAliases() after
recalculating IEC addresses and before completing the board-change handling.
Ensure aliases are synchronized whenever the target board changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a1188cf-6aa3-4606-9426-da94c79189c9
📒 Files selected for processing (8)
src/frontend/components/_features/[workspace]/editor/device/configuration/__tests__/device-license-status.test.tsxsrc/frontend/components/_features/[workspace]/editor/device/configuration/board.tsxsrc/frontend/components/_features/[workspace]/editor/device/configuration/components/device-license-status.tsxsrc/frontend/hooks/__tests__/use-device-license.test.tssrc/frontend/hooks/use-device-license.tssrc/frontend/store/__tests__/device-slice.test.tssrc/frontend/store/slices/device/slice.tssrc/frontend/store/slices/device/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/frontend/components/_features/[workspace]/editor/device/configuration/board.tsx
- src/frontend/store/tests/device-slice.test.ts
- src/frontend/components/_features/[workspace]/editor/device/configuration/components/device-license-status.tsx
- src/frontend/components/_features/[workspace]/editor/device/configuration/tests/device-license-status.test.tsx
2 participants
Problem
Buying happens in an external browser tab, and nothing brought the result back: after clicking Buy licence the editor kept showing "Not licensed" until the user guessed they should click "Check again". The purchase flow ended in a dead end at its most important moment.
What changed
buy()now opens the purchase page and starts a purchase watch (deviceLicense.awaitingPurchase):refresh()executes every 20s — one Modbus read frame + one HTTP round-trip. On the first tick after the completion webhook lands, that samerefresh()activates the licence and writes the blob to the device, no click required.licensedreport (whoever produced it: the poll, a manual re-check, the connect flow), after a 30-tick / 10-minute budget, on Stop waiting, or when the board is switched / disconnected (clearDeviceLicense).refreshthrough a ref: its identity follows the device port, and an interval keyed on it would reset the tick budget on every change.Badge while waiting:
isCheckingflicker each tick would otherwise cause;Files
store/slices/device/{slice,types}.ts—awaitingPurchase+setAwaitingPurchase;clearDeviceLicensealso ends the watchhooks/use-device-license.ts— the watch effect,buy()starting it,cancelPurchaseWatchdevice-license-status.tsx+board.tsx— badge states and wiringCompanion PR in autonomy-edge (#358, EDGE-593): the
/buypage polls the same truth and shows "License issued" only when the webhook actually landed it.Verification
tsc --noEmitclean; eslint 0 errors (the 4unbound-methodwarnings inuse-device-license.tspre-date this change, same count)🤖 Generated with Claude Code
https://claude.ai/code/session_015uUH3ZL5ehreMUf2dtanWD
Summary by CodeRabbit
New Features
Bug Fixes