Skip to content

Store interface preferences per account (#55) - #85

Merged
dmccoystephenson merged 3 commits into
mainfrom
feature/per-account-preferences
Aug 10, 2026
Merged

Store interface preferences per account (#55)#85
dmccoystephenson merged 3 commits into
mainfrom
feature/per-account-preferences

Conversation

@dmccoystephenson

@dmccoystephenson dmccoystephenson commented Aug 10, 2026

Copy link
Copy Markdown
Member

Summary

  • Interface preferences are now stored per account, not just per browser. A new
    GET/PUT /api/session/preferences pair on the backend holds each signed-in player's display
    settings and sidebar arrangement (order, which panels are shown, which are open), so signing in
    on another browser or device brings the arrangement along instead of starting from the defaults.
  • localStorage is kept as the working copy: it is seeded from the account copy on load and
    receives every change afterwards. An unreachable backend therefore costs nothing beyond the
    arrangement not travelling. Where the two copies differ the account's wins; a preference the
    account has never held — chosen before signing in, or while the backend was down — is uploaded
    rather than discarded.
  • The rule for reconciling the two copies is a pure function (mergePreferences in
    game-logic.js), so it is covered by the Node test suite rather than living untested in the
    page's inline script.
  • Uploads are held until that reconciliation has completed. Without the gate, a preference written
    between sign-in and the account copy arriving would schedule an upload that could beat the
    response and put this browser's stale copy over the account's, losing the arrangement the request
    was about to return. A change made while the gate is shut is remembered and sent once it opens,
    so it is delayed rather than dropped.
  • The stored payload is opaque JSON (user_preferences table, keyed by username), capped at 8192
    characters: a preference added to the game page needs no backend change, while an account cannot
    be used as unbounded storage. An oversized or unreadable payload is answered with 400 rather
    than a 500, and preferences that can no longer be parsed are reported as unset so the page
    falls back to its own defaults.
  • Uploads are debounced (500 ms) so the several writes one layout change fans out into are
    coalesced into a single request.

The show/hide, reorder and remembered-collapsed-state half of #55 was shipped in earlier cycles;
what remained was the per-account persistence, which this completes.

Modules touched

Both. Backend gains the model, repository, service and endpoints; the web client gains the matching
proxy routes, the pure merge rule and the page wiring; the documentation sources of truth are
updated alongside.

Scope note

Eighteen files are touched, above the loop's ~10-file soft ceiling. Six of them are test files and
the rest are the dependency-coupled minimum for one endpoint pair (entity → repository → service →
controller, then the web-client caller that the ProxyRouteCoverageTest contract requires, plus the
three documentation sources of truth). Net non-test change is roughly 360 lines, inside the ~400-line
ceiling.

Coverage note

The page wiring in game.html is inline script, which neither the Maven build nor the Node suite
executes; a green CI run does not verify it. The parts that could be extracted (the merge rule) were,
and the proxy routes are held by ProxyRouteCoverageTest, but a manual smoke test of the game page —
change a setting and the panel arrangement, sign in from a second browser, confirm the arrangement is
there — is recommended before this is relied on.

Test plan

  • mvn -f backend/pom.xml test — green (277 tests; new PreferencesServiceTest, GameControllerPreferencesTest)
  • mvn -f web-client/pom.xml test — green (26 tests; new proxy cases in WebControllerTest; ProxyRouteCoverageTest confirms both new calls the page makes have routes)
  • node --test web-client/src/test/js/ — run by the Web Client CI job, which exercises the new mergePreferences cases
  • Manual smoke test of the game page (see the coverage note above)

Closes #55

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

dmccoystephenson and others added 2 commits August 10, 2026 00:15
The sidebar arrangement and display settings were kept only in localStorage,
so they stayed in one browser. They are now also stored against the signed-in
player's account through GET/PUT /api/session/preferences and applied wherever
that player next signs in.

localStorage remains the working copy: the account copy seeds it on load and
receives every change afterwards, so an unreachable backend costs nothing
beyond the arrangement not travelling, and an arrangement made before signing
in is uploaded rather than discarded. The reconciliation rule is a pure
function (mergePreferences) covered by the Node test suite.

The stored payload is opaque JSON, capped at 8192 characters, so a preference
added to the game page needs no backend change while an account still cannot
be used as unbounded storage.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A panel opened or closed while the account copy is applied raises its toggle
event asynchronously, after the guard against echoing that back up had already
been cleared. The guard now ends on the next task instead.

Adds the cookie-forwarding tests the other per-user BackendService calls have,
for the preference read and write.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review rubric (performed against the diff and the CI run on head 0c7b2a6):

  • Scope: PASS — every file is either the endpoint pair and its dependencies (entity → repository → service → controller), the web-client caller the proxy contract requires, its tests, or the three documentation sources of truth. No unrelated formatting or renames appear in git diff origin/main.
  • Tests-new: FAIL (mitigated, not fully fixable here) — the new Java and JS units are covered (PreferencesServiceTest, GameControllerPreferencesTest, three WebControllerTest cases, two BackendServiceTest cookie-forwarding cases, six mergePreferences cases confirmed in CI as ok 43ok 48, # pass 48 / # fail 0). The page wiring added to game.html (readLocalPreferences, writeLocalPreferences, applyStoredPreferences, queuePreferenceUpload, uploadPreferences, loadStoredPreferences) is inline script that neither Maven nor the Node suite executes, so it has no automated coverage. The reconcilable part was extracted into game-logic.js for that reason; the remainder is covered only by the manual smoke test recommended in the PR body.
  • Tests-fix: N/A — no bug fix is claimed, so the stash-and-run procedure does not apply.
  • Sibling structure: PASSUserPreferences mirrors SavedGame (@Entity/@Table/Lombok/Instant updatedAt/username-only constructor), UserPreferencesRepository mirrors SavedGameRepository, and PreferencesService follows SessionService's explicit-constructor and dedicated-ObjectMapper shape.
  • Sibling renames: PASS — the only identifier introduced by restructuring is restorePanelOpenState, extracted from an anonymous IIFE; it belongs to no parallel pair or series.
  • Docs: PASSREADME.md gains both endpoints and the account-sync feature bullet, PLAYER_GUIDE.md's two statements that preferences do not travel are corrected, CHANGELOG.md gains a Web Client and a Persistence entry. MVP.md and DOCS.md make no claim about preference storage (verified by grep), so neither needed a change.
  • Issue resolution: PASSMake the dashboard layout configurable and persist it per account #55's remaining acceptance criterion (the layout persisting across sessions and devices for a logged-in player) is what this adds; the show/hide, reorder and remembered-collapsed-state criteria were met in earlier cycles and are unaffected.
  • CI: PASS — Backend Build and Test and Web Client Build and Test both pass on head.
  • Both-modules: PASS — both jobs are green, and both were also run locally (mvn -f backend/pom.xml test, mvn -f web-client/pom.xml test).
  • API-contract: PASS — the two new backend endpoints have matching web-client proxy routes, ProxyRouteCoverageTest passes (it reads the calls out of the rendered page, so the PUT is checked too), and the README API section lists both.
  • Changelog: PASS — a player-visible entry under Web Client and a storage entry under Persistence.
  • Constructor-injection: PASSPreferencesService declares an explicit constructor; GameController keeps its Lombok @RequiredArgsConstructor. No field @Autowired is introduced.
  • Override-correct: N/A — no @Override is added.

Findings from the adversarial pass, two of which were fixed in 0c7b2a6:

  • web-client/src/main/resources/templates/game.html:664fixed. A panel opened or closed while the account copy is applied raises its toggle event asynchronously, so the original guard (cleared in a finally) had already been reset by the time savePanelState ran, and every load by a signed-in player would have echoed an unrequested upload back up. The guard now ends on the next task instead.
  • web-client/src/test/java/com/barony/webclient/service/BackendServiceTest.java:140fixed. The two new proxy methods were initially left without the cookie-forwarding cases every other per-user call in that class has, which is the one thing that would silently break them behind the documented docker-compose setup.
  • backend/src/main/java/com/barony/backend/service/PreferencesService.java:74 — a repository failure on save is allowed to surface as a 500, unlike SessionService.persist, which swallows one. The difference is deliberate: gameplay must not break on a persistence failure, whereas a preference save that silently vanishes would leave a player believing an arrangement had been stored. Flagged as a judgment call rather than changed.
  • web-client/src/main/resources/templates/game.html:686 — a preference changed during the window between page load and the account copy arriving is overwritten by that copy. This is inherent to seeding from the server on load, is confined to the first few hundred milliseconds of a load, and costs at most one repeated click; no defence was added.
  • backend/src/main/resources/application.properties:17 — no migration accompanies the new user_preferences table; spring.jpa.hibernate.ddl-auto=update creates it on first start, which is how saved_game and run_record are handled too. Noted so it is a conscious inheritance of the existing scheme rather than an oversight.

One further gap is restated from the PR body rather than scored: a green CI run does not verify the inline page wiring, since neither job executes it. A manual smoke test — change a setting and the panel arrangement, sign in from a second browser, confirm the arrangement is there — is recommended before this is relied on.

This review was performed and posted during a Gardener session (https://github.com/Stephenson-Software/gardener).

checkSession sets the username before the account copy is requested, so a
preference written in the window between the two - by the load-time panel
restore echoing a toggle, or by the player changing a setting - scheduled an
upload 500ms later that could beat the response. That put this browser's copy
over the account's and permanently lost the arrangement the request was about
to return.

Uploads are now gated until the merge has happened, and a change made while
the gate is shut is remembered and sent once it opens, so it is delayed rather
than dropped. A failed load opens the gate too, leaving the player able to save
from the browser they are on.

Corrects the player guide and changelog alongside: both said an offline change
is sent up on the next load, which contradicted the stated rule that the
account copy wins - it holds only for a preference the account has never held.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Verification review (head d71bf62)

This PR was inherited as unfinished work from an interrupted session, so its state was re-verified from scratch this run rather than being taken on the strength of the earlier self-review. Both modules were built and tested locally, the full diff was re-read, and one correctness defect was found and fixed.

Anchors

  • Backend Build and Test: PASS — green on head d71bf62; reproduced locally with mvn -f backend/pom.xml test (277 tests, 0 failures).
  • Web Client Build and Test: PASS — green on head; reproduced locally with mvn -f web-client/pom.xml test (26 tests, 0 failures). The job also runs node --test web-client/src/test/js/, so the mergePreferences cases are genuinely exercised rather than merely present.
  • Both-modules: PASS — each module was run separately, so neither job's result is standing in for the other's.

Finding fixed this run

web-client/src/main/resources/templates/game.html:667upload could clobber the account copy during the load window. checkSession() assigns username before loadStoredPreferences() issues its GET, so the !username guard is already open while the account copy is still in flight. Any preference write landing in that window — the load-time panel restore echoing a toggle, or the player changing a setting — scheduled an upload 500 ms later that could beat the response on a slow or cold backend. The PUT would then put this browser's stale copy over the account's, and the arrangement the in-flight GET was about to return would be permanently lost. This is the inverse of the benign window noted in the previous review (a local change being overwritten, costing one repeated click) and is materially worse, since the loss is server-side and persistent.

Uploads are now gated on reconciliation having completed, and a change made while the gate is shut is remembered and flushed once it opens, so such a change is delayed rather than dropped. A failed load opens the gate as well, leaving a player able to save from the browser they are on when the account copy cannot be read.

Documentation

PLAYER_GUIDE.md:337 and CHANGELOG.md:12corrected. Both stated that a change made while the server is unreachable is sent up on the next load, which contradicted the rule stated one sentence earlier that the account copy wins. The claim holds only for a preference the account has never held; where the account holds one, mergePreferences discards the browser's. Both passages now say so.

The remaining documentation was checked against source and found accurate: README.md's two endpoint entries match the controller (an empty object when nothing is stored, 400 above 8192 serialized characters), and MVP.md and DOCS.md make no claim about preference storage.

Standing gap

No automated coverage exists for the inline page wiring in game.html, since neither Maven nor the Node suite executes it — a green CI run does not verify that code, and the defect fixed above was reachable only there. The reconcilable part was extracted into game-logic.js and the proxy routes are held by ProxyRouteCoverageTest, but a manual smoke test remains advisable: a setting and the panel arrangement changed, then a sign-in from a second browser to confirm the arrangement travels.

Two judgment calls raised in the previous review were re-examined and are left as they stand: a repository failure on save surfacing as a 500 (deliberate, so a save that vanished is not reported as success), and the absence of a migration for user_preferences (consistent with how saved_game and run_record are created under ddl-auto=update).

This review was performed and posted during a Gardener session (https://github.com/Stephenson-Software/gardener).

@dmccoystephenson
dmccoystephenson merged commit 9ef46eb into main Aug 10, 2026
2 checks passed
@dmccoystephenson
dmccoystephenson deleted the feature/per-account-preferences branch August 10, 2026 01:11
dmccoystephenson added a commit that referenced this pull request Aug 11, 2026
* Cover the preference upload gate with the Node test suite (#86)

Move the state machine deciding when a signed-in player's preferences may be
sent to their account out of game.html's inline script and into game-logic.js
as createPreferenceSync, taking the reads, writes and requests as callbacks so
the orderings that carry the risk can be driven without a browser. The reads,
writes, requests and the DOM work stay inline as those callbacks.

The defect found by hand during the review of #85 - a preference changed while
the account copy was still in flight scheduling an upload that could beat the
response and put this browser's stale copy over the account's - now fails a
test rather than only a reviewer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Cover createPreferenceSync's browser-default timers and error reporter

The suite drives an injected clock everywhere else, leaving the setTimeout /
clearTimeout the browser is actually given, and the no-op onError a caller may
rely on, exercised by nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

Make the dashboard layout configurable and persist it per account

1 participant