Polish mobile Settings layout and Usage placement - #786
Conversation
Deploying maple with
|
| Latest commit: |
5b79e27
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://784764d6.maple-ca8.pages.dev |
| Branch Preview URL: | https://mobile-settings-layout.maple-ca8.pages.dev |
Manual QAManual QA completed. Unless otherwise noted, each item was verified in the mobile app.
|
Independent code reviewReview complete. Three independent passes covered state/security, mobile UI, and minimality/tests. I found one merge-blocking account-isolation defect, one moderate navigation issue, and several clear simplification opportunities. Findings
Simplification and cruft
Keep the real account-transition and VisualViewport geometry/subscription tests; those protect meaningful behavior. Validation
|
Review responseAddressed in Accepted findings
The service-boundary regression coverage now includes late Account A generation after logout/switch, direct stored-credential account changes, stale query ownership, same-account provider rerenders, legacy-token migration, one-time 401 rotation, and API completion after an account transition. I retained the hook-level late-publication test because it protects a separate LocalState invariant; it is no longer the only account-transition coverage. Cleanup assessmentI agreed with and applied the focused simplifications:
I did not apply the blanket removal of the Settings/chat-history presentation seams, native safe-area/source assertions, refresh lifecycle coverage, or explanatory comments. Those seams encode the two device-reported Mobile WebKit sticky-hover regressions while preserving desktop hover behavior, and the source assertions cover explicit acceptance criteria for which this repo has no native browser-layout harness. The comments document non-obvious account, WebKit, nested-query, and safe-area invariants; I found no no-comments requirement in the task. Validation
The local authenticated integration stack was unavailable in this workspace, so the new account-switch evidence is deterministic real-service-boundary test coverage rather than a live billing-environment smoke. |
Safari QA UI automation reportTested the authenticated Cloudflare branch preview in Safari against head VerdictP1 / high-severity release blocker remains: compact project-chat history can still desynchronize the visible UI, URL, browser history, and reload state. Reproduction
Observed:
I reproduced this twice from a fresh compact root. The behavior is state-dependent: after navigating through project detail first, the same Back action synchronized correctly once, which may mask the regression during longer sessions. Control case: the non-project “Twenty Minute Short Story” history entry synchronized correctly across in-app Back, URL, reload, and Safari Back/Forward. Severity: I classify this as P1 rather than strict P0 because it breaks a core navigation invariant and should block release/merge, but I found no data loss, security exposure, or service-wide outage. A likely mechanism is competing URL ownership between the mobile stack and the mounted chat:
This can plausibly leave a history entry whose mobile snapshot says “menu” while its href still names the conversation. The exact timing should be verified with an instrumented component test. Passing UI coverage
Automated evidence and gap
Those tests cover pure stack and ownership helpers but do not mount
An integration/component regression test for that sequence is recommended. Test state and boundariesCreated test data remains in the authenticated account:
The chat is unpinned and project Custom Instructions were cleared. Permanent deletion was not submitted. Not validated in ordinary desktop Safari: native-iOS edge swipe, safe-area behavior, keyboard-adjusted |
QA automation follow-upI agree with the P1 / release-blocking classification. The proposed mechanism was correct, and tracing it exposed the exact timing:
That also explains the state dependence: the extra project-state update widens the race, while arriving through project detail can make that update a no-op. Fixed in
One nuance: after Maple has genuinely returned to a valid canonical root entry, the next Safari Back leaving Maple for the prior site is expected browser behavior. The defect was that the Maple root entry itself had been rewritten, which made Forward and reload disagree with the visible menu. Validation on the final diff:
The new regression is focused causal coverage, not a Safari UI E2E. My browser session reached Cloudflare Access rather than the authenticated preview, so I am not claiming a fresh deployed Safari replay. Please rerun the exact fresh compact project-chat → in-app Back → Safari Back/Forward → reload sequence once Cloudflare finishes deploying head |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| JSON.stringify({ | ||
| accountId: scope.accountId, | ||
| token: token.token | ||
| } satisfies StoredBillingCredential) |
There was a problem hiding this comment.
Confirmed: this finding concerns a pre-existing storage design, not clear-text persistence introduced by this PR.
The current origin/master already stores the billing bearer token directly in sessionStorage:
sessionStorage.setItem(TOKEN_STORAGE_KEY, token.token);git log -S traces that storage behavior to the initial commit (a8df346, January 27, 2025). This branch changes the stored value to an account-owned JSON record so a token cannot be reused across an account transition, but it does not broaden the storage lifetime or access model.
The CodeQL finding is technically valid and should not be labeled a false positive. However, redesigning billing credential storage is outside the scope of this mobile Settings PR. It should be tracked and addressed separately against master—for example, by replacing browser persistence with an account-scoped in-memory credential or another appropriately protected credential mechanism. No storage-architecture change will be made in this PR.
|
closing because it grew too much in scope and the downstream PR got closed |
2 participants
Summary
Stack
mobile-navigationValidation
nix develop .#ci -c ./scripts/ci/frontend.shMAPLE_WEB_ENVIRONMENT=pr nix develop .#ci -c ./scripts/ci/web.sh268225a7c815113fbae2b151177c3f323b43939a66ff0a34f35e0b7e7c57e75fgit diff --checkpassedAuthenticated post-fix screenshot QA remains unavailable in this workspace: the managed workspace CLI, test-user manifest, service listeners, local Maple app bundle, and physical iOS device session are absent. The supplied iPhone screenshots and recordings were inspected frame by frame to identify the doubled safe-area offset, content bleed, automatic focus outline, latched touch-hover states, premature bottom-safe-area history cutoff, and keyboard-driven dialog displacement. Exact source/CSS review covered 390×844, 430×932, 844×390, 767/768 breakpoint behavior, 1280px desktop/short-height layouts, Dynamic Island safe bounds, and short/tall dialog geometry. A physical iPhone keyboard pass remains the final runtime check for the new dialog placement.