Replace 24h absolute auto-lock with configurable idle timer

[JakeUrban]

What and why

Freighter currently locks the wallet on a fixed 24-hour timer that starts the moment you unlock and never moves. This causes two bad outcomes:

• Active users get bounced. Someone using Freighter steadily through a workday can hit the 24-hour cutoff mid-flow — typically while signing a transaction — and suddenly see a password prompt with no obvious cause.
• Idle sessions stay unlocked. Someone who unlocks Freighter and then walks away keeps a fully unlocked wallet for the rest of the day. There is no idle protection.

This PR replaces the fixed timer with a configurable idle timer, matching MetaMask, Rabby, Phantom, Trust Wallet, and Coinbase Wallet.

User-visible changes

• New Auto-lock timer control in Settings → Preferences with five presets: 1, 5, 15, 30, 60 minutes. Default is 15 minutes; existing installs are implicitly migrated on first read.
• The wallet locks after the chosen period of inactivity. Direct user input (mousedown, keydown, touchstart, wheel) inside any Freighter UI surface — popup, sidebar, standalone signing window, grant-access window — resets the timer.
• Changing the timeout in Settings takes effect immediately.
• When the idle alarm fires, every open Freighter surface flips to the unlock screen without polling, preserving the interrupted route (e.g. /sign-transaction?...).

Explicitly out of scope for v1: lock-on-blur and lock-on-window-close. Those can layer on later as additional toggles.

Implementation notes worth flagging

• Chrome alarms are the single source of truth for the lock deadline. Survives MV3 service-worker teardown without persisting anything to storage. The popup is a pure activity reporter; it never holds the timer.
• Surface mounts deliberately do NOT count as activity. A dApp-spawned signing popup is programmatic — a mount-ping would let a malicious dApp keep the session alive indefinitely. Only direct user input resets the alarm. This matches MetaMask and Phantom.
• USER_ACTIVITY is gated on isFromExtensionPage in popupMessageListener, and the background userActivity handler re-checks unlocked state authoritatively before rearming. Belt-and-braces against both dApp-origin spoofing and stale popup-side Redux state.
• Hardware-wallet sessions need a dedicated isHardwareWalletLocked flag in the session duck because HW "unlocked" state lives in localStore.isHardwareWalletActive, which the existing timeoutAccountAccess action doesn't clear. Without this the idle alarm would silently no-op on HW-only sessions.
• Cross-surface sync is broadcast-driven. clearSession (idle alarm) and signOut emit SESSION_LOCKED; successful unlock emits SESSION_UNLOCKED. A new SessionLockListener mounted inside the router converts those into navigation + auth-state refreshes. The handler is intentionally synchronous so it doesn't claim the runtime.onMessage response slot for unrelated cross-surface requests (this was a real footgun discovered mid-PR — see the related comment thread).
• signOut stops the alarm before mutating state, eliminating the race where a pending alarm could fire clearSession on already-cleared state and emit a duplicate broadcast.
• No service-worker-wake rearm. The worker wakes for non-activity reasons (network alarms, runtime messages); rescheduling on wake would silently extend the deadline — exactly the bug we're fixing.

A new @shared/constants/autoLock.ts is the single source of truth for valid preset values, the type, the default, the type guard, and the coercer. Every consumer (background handlers, SessionTimer, Preferences <Select>, message-request types) imports from there.

Hitchhiker: useGetAppData cache fast-path bugfix

This PR also includes a small unrelated bugfix to extension/src/helpers/hooks/useGetAppData.tsx: the cache fast-path now requires both currentAccount.publicKey AND currentAccount.hasPrivateKey before short-circuiting. Previously a stale cached publicKey from a locked session could let callers skip the unlock reroute. Surfaced during internal review of this PR; covered by useGetAppData.test.tsx and the hasPrivateKey: true fixture additions in three cached-path tests (useGetAssetDomainsWithBalances.test.tsx, SearchAsset.test.tsx, useGetWalletsData.test.tsx). Strictly belongs in its own PR; called out here so reviewers don't have to bisect.

---

Closes #2082

[github-actions]

PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-10f78efd0b0838157692 (SDF collaborators only — install instructions in the release description)

[Copilot]

Pull request overview

This PR replaces Freighter’s current hardcoded 24-hour absolute session auto-lock with a configurable idle-based auto-lock timer. It introduces a shared source of truth for valid timeouts, sends throttled “user activity” pings from extension UI surfaces to the MV3 background worker, and reschedules/clears the underlying Chrome alarm as needed (including on settings changes and sign-out).

Changes:

• Add shared auto-lock timeout constants/types and persist autoLockTimeoutMinutes via settings load/save.
• Implement idle timer reset/stop in the background (SessionTimer + USER_ACTIVITY handler) and propagate immediate-lock behavior on shortened timeout.
• Add popup-side activity tracking + UI for selecting the timeout, plus tests/mocks updates across background and popup.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
extension/src/popup/views/Preferences/index.tsx	Adds auto-lock timeout <Select> UI and wires the value into saveSettings.
extension/src/popup/views/IntegrationTest.tsx	Updates integration test settings payload to include the new timeout field.
extension/src/popup/views/tests/Swap.test.tsx	Updates mocked loadSettings response to include autoLockTimeoutMinutes.
extension/src/popup/views/tests/SignTransaction.test.tsx	Updates mocked loadSettings responses to include autoLockTimeoutMinutes.
extension/src/popup/views/tests/ManageAssets.test.tsx	Updates mocked loadSettings response to include autoLockTimeoutMinutes.
extension/src/popup/views/tests/GrantAccess.test.tsx	Updates mocked loadSettings response to include autoLockTimeoutMinutes.
extension/src/popup/views/tests/AddFunds.test.tsx	Updates mocked loadSettings response to include autoLockTimeoutMinutes.
extension/src/popup/views/tests/AccountHistory.test.tsx	Updates mocked loadSettings response to include autoLockTimeoutMinutes.
extension/src/popup/views/tests/AccountCreator.test.tsx	Updates mocked loadSettings response to include autoLockTimeoutMinutes.
extension/src/popup/views/tests/Account.test.tsx	Updates mocked loadSettings responses to include autoLockTimeoutMinutes.
extension/src/popup/helpers/hooks/useActivityPing.ts	New hook that throttles and sends USER_ACTIVITY pings to background while unlocked.
extension/src/popup/helpers/hooks/tests/useActivityPing.test.ts	Adds unit tests for throttling, event coverage, and cleanup behavior.
extension/src/popup/ducks/settings.ts	Plumbs autoLockTimeoutMinutes through state/thunk and propagates wasLocked to auth.
extension/src/popup/ducks/accountServices.ts	Adds lockAccount reducer used to flip hasPrivateKey immediately after background lock.
extension/src/popup/ducks/tests/settings.test.ts	Adds coverage ensuring saveSettings thunk dispatches lockAccount when wasLocked.
extension/src/popup/components/ActivityTracker/index.tsx	New component that mounts useActivityPing based on hasPrivateKeySelector.
extension/src/popup/App.tsx	Mounts <ActivityTracker /> once under the Provider to cover all extension surfaces.
extension/src/constants/localStorageTypes.ts	Adds AUTO_LOCK_TIMEOUT_MINUTES_ID storage key constant.
extension/src/background/messageListener/popupMessageListener.ts	Routes USER_ACTIVITY messages and threads sessionTimer into relevant handlers.
extension/src/background/messageListener/helpers/test-helpers.ts	Enhances the session timer test double to read timeout from mock storage and track calls.
extension/src/background/messageListener/helpers/login-all-accounts.ts	Awaits sessionTimer.startSession() in the unlock flow.
extension/src/background/messageListener/handlers/userActivity.ts	New handler that rearms the timer only when the background session is unlocked.
extension/src/background/messageListener/handlers/signOut.ts	Stops the auto-lock alarm on explicit sign-out to avoid stale alarm firing.
extension/src/background/messageListener/handlers/saveSettings.ts	Validates/persists timeout, reschedules or immediately locks on shortened-timeout edge cases.
extension/src/background/messageListener/handlers/recoverAccount.ts	Awaits sessionTimer.startSession() after account recovery unlock.
extension/src/background/messageListener/handlers/migrateAccounts.ts	Awaits sessionTimer.startSession() during migration flow.
extension/src/background/messageListener/handlers/loadSettings.ts	Loads timeout from storage via coercion and returns it in settings payload.
extension/src/background/messageListener/handlers/handleSignedHwPayload.ts	Treats HW signing completion as activity by resetting the idle timer.
extension/src/background/messageListener/handlers/createAccount.ts	Awaits sessionTimer.startSession() after creating an account.
extension/src/background/messageListener/handlers/confirmPassword.ts	Dispatches unlockHardwareWallet() on successful password unlock.
extension/src/background/messageListener/tests/userActivity.test.ts	Adds unit tests for USER_ACTIVITY behavior across locked/unlocked + HW scenarios.
extension/src/background/messageListener/tests/loadSaveSettings.test.ts	Extends tests to cover timeout validation, reschedule/lock behavior, and defaults.
extension/src/background/messageListener/tests/handleSignedHwPayload.test.ts	Adds coverage that HW signature handling resets the session timer first.
extension/src/background/messageListener/tests/createAccount.test.ts	Updates tests for timer signature changes (casts test timer).
extension/src/background/index.ts	Constructs SessionTimer with a storage access instance.
extension/src/background/helpers/session.ts	Replaces absolute 24h timer with idle-based SessionTimer and locks HW sessions on clear.
extension/src/background/helpers/tests/session.test.ts	Adds tests for SessionTimer reset/stop behavior and default coercion.
extension/src/background/ducks/session.ts	Adds HW-lock state, actions, and updates has-private-key selector semantics.
@shared/constants/services.ts	Adds SERVICE_TYPES.USER_ACTIVITY.
@shared/constants/autoLock.ts	New shared constants/types + coercion/validation helpers for timeout values.
@shared/api/types/types.ts	Adds autoLockTimeoutMinutes to Preferences (and thus Settings).
@shared/api/types/message-request.ts	Adds timeout field to SaveSettingsMessage and introduces UserActivityMessage.
@shared/api/internal.ts	Threads autoLockTimeoutMinutes through saveSettings and updates response typing.

Comments suppressed due to low confidence (15)

extension/src/popup/views/tests/SignTransaction.test.tsx:466

• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/SignTransaction.test.tsx:606
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/SignTransaction.test.tsx:721
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/SignTransaction.test.tsx:844
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/SignTransaction.test.tsx:967
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/SignTransaction.test.tsx:1084
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/SignTransaction.test.tsx:1207
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/SignTransaction.test.tsx:1347
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/SignTransaction.test.tsx:1522
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/SignTransaction.test.tsx:1689
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/Account.test.tsx:948
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/Account.test.tsx:1033
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/Account.test.tsx:1117
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/Account.test.tsx:1200
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.
  extension/src/popup/views/tests/Account.test.tsx:1266
• Indentation for this newly added field is inconsistent with the surrounding object literal, which makes the test fixture harder to read and may fail formatting checks.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[leofelix077]

From my understanding of the PR description, the intention is to have a full lock after inactivity, and it keeps refreshing while the user is interacting,

but currently the UI is not flipping back to the lock screen after inactivity and still asks for password mid-interaction

The app UI stays visibly unlocked but no private key (Assets, account screen, history, etc. stays visible)

Screen.Recording.2026-05-25.at.16.08.39.mov

Was the intention to be 1. "fully locked" after idle, or 2. as shown on the video above?

It seems that flipping the UI back to locked state after inactivity is missing

Code style itself there's nothing major that catches my eyes and is following the standards from the code already present on the codebase

[JakeUrban]

Human PR Review Response Summary

PR: #2802

Addressed in code

• @leofelix077 (CHANGES_REQUESTED): after the idle timer fires, the background locks the wallet but the popup keeps rendering Account/Assets/History; a password prompt then surprises the user mid-flow. UI never flips to the unlock screen.

  • Fix in b00599bc: added SERVICE_TYPES.SESSION_LOCKED; background initAlarmListener broadcasts it after clearSession (try/caught for the no-receivers case); new SessionLockListener mounted inside the popup <HashRouter> dispatches a new lockAccount reducer (clears private-key state but preserves applicationState so <UnlockAccountRoute> keeps rendering <UnlockAccount> rather than redirecting to <Welcome>) and navigates to /unlock-account.
  • Refinement in ecf489b6 (from internal review-fix round 1): SessionLockListener now preserves state.from and location.search, so unlocking returns the user to the interrupted flow (matching the existing SignTransaction / GrantAccess / VerifyAccount reroute patterns). No-ops when already on /unlock-account.

• Bot review (copilot-pull-request-reviewer) inline threads on 54aa0b8d / e710fa1c were already resolved on the PR.

Filed as follow-up issues

Internal review-fix round 1 surfaced two broader architectural concerns that are out of scope of the reviewer's actionable feedback (per the skill's proportionality guide). Filed as separate issues so they get scheduled deliberately:

• Guard loadAccount() responses against stale rehydration after a session lock #2813 — Guard loadAccount() responses against stale rehydration after a session lock. (useGetAppData / useGetSubmitAccountData / useGetRecoverAccountData do loadAccount()→saveAccount(result) without an epoch guard.)
• Generalize background→UI session-state broadcasts (sign-out, account-switch, HW-unlock) #2814 — Generalize background→UI session-state broadcasts (sign-out, account-switch, HW-unlock) so all open UI surfaces stay in sync with background state transitions, not just the idle-lock path.

Internal review-fix status

• Round 1 (commit b00599bc): INCOMPLETE — three items raised. Item Run eslint during build, minor adjustments #1 (route preservation) folded in; items router fix #2 and Piyal dev #3 filed as follow-ups Guard loadAccount() responses against stale rehydration after a session lock #2813 and Generalize background→UI session-state broadcasts (sign-out, account-switch, HW-unlock) #2814.
• Round 2 (commit ecf489b6): SOUND — fix correct, matches existing reroute patterns, no regression on the round-1 flip-to-unlock behavior, suite green.

Commits added to the PR

• b00599bc — auto-lock UI flip: SESSION_LOCKED broadcast + listener + lockAccount reducer + 7 new tests.
• ecf489b6 — preserve interrupted route context across auto-lock; +2 tests (5 listener tests total).

Validation

• yarn test:ci → 974 passed, 72 skipped, 0 failed.
• yarn build:extension → clean.

Project board note

The Status field on the wallet engineering project was not updated this round — the current GH_TOKEN is missing the read:project / project scope. The issue assignment and PR link are preserved so the team can move it manually if needed.

---

Responded to using the plan-do-review skill. Re-requesting review on PR #2802.

[leofelix077]

In some cases it's working when leaving the view open and inactive, but the background handlers do not seem to be properly communicating the state between the views, between sidebar mode, fullscreen mode and popup mode. When I unlock it on one of them, the other 2 do not get simultaneously unlocked, leaving it in an inconsistent state between the views

Also if I set the auto-lock on the extension, and close the popup before the timer ends, it will not lock the screen, but the timer will be gone, leaving the app permanently in an unlocked state

I changed locally the auto lock state to 15s and 30s for quicker testing and added a countdown on the home screen

When in full screen mode the actions do not register, so the timer will count down from the predefined timer without refreshing, and will auto lock always after the preset timer, as seen on the video below

Screen.Recording.2026-05-27.at.09.55.04.mov

The screenshots show the state that when unlocking on one of the views, it does not reflect the other two

Changing timers getting reflected on the countdown

Screen.Recording.2026-05-27.at.09.53.52.mov

And the last video shows that when the timer runs out on popup mode, after re-opening the popup, the timer is gone, and changing the settings, it does not restart the timer, leaving it permanently unlocked. It does ask for the password again if the user tries to perform an action. After typing the password it restarts the timer, but leads back to previous problem that the app will remain unlocked if the user reopens the extension after the timer is expired

Screen.Recording.2026-05-27.at.09.59.09.mov

Screen.Recording.2026-05-27.at.10.13.19.mov

[leofelix077]

After debugging and applying some fixes, I asked Claude to generate a summary of the changes and bugs, so it could be piped into the plan-do-review agent.

@JakeUrban can you input this into it and push again, so we can check if it's gonna apply it correctly now?

Fix: idle auto-lock leaves wallet inconsistent and looks unlocked across surfaces

Three correctness bugs in the idle auto-lock flow ship in the current branch. They all need fixing together for the feature to behave the way the PR description promises.

Bug A — popup stays on the Account screen after auto-lock fires

Repro: unlock, set auto-lock timeout, close the popup, wait past the timeout, reopen the popup. The alarm fires correctly and the background reports hasPrivateKey: false, but the popup lands on Account (/) instead of the unlock screen.

Root cause: useGetAppData.fetchData only re-routes to ROUTES.unlockAccount when !account.publicKey. But clearSession deliberately keeps publicKey set so the unlock flow can resume for the same account — it only wipes hashKey. The Account index route (/) is not wrapped in VerifiedAccountRoute, so nothing else gates rendering on hasPrivateKey.

Fix: also re-route when !account.hasPrivateKey in extension/src/helpers/hooks/useGetAppData.tsx:

if (
  !account.publicKey ||
  !account.hasPrivateKey ||
  account.applicationState === APPLICATION_STATE.APPLICATION_STARTED
) {
  // existing re-route to unlockAccount / welcome
}

Bug B — clearSession's redux dispatch can race the service-worker termination

Repro: same as Bug A. Symptom (before Bug A's fix): the popup re-hydrates with hashKey still set in chrome.storage.session, even though TEMPORARY_STORE_ID was correctly removed.

Root cause: buildStore wires subscribe(() => saveStore(...)), but saveStore is fire-and-forget — it calls sessionStore.setItem(...) without awaiting. When the idle alarm fires while no popup is open, the SW terminates as soon as the alarm handler returns; the chrome.storage.session.set triggered by clearSession's dispatches frequently doesn't complete before termination. TEMPORARY_STORE_ID removal (in chrome.storage.local) IS awaited and survives, so the wallet is functionally locked, but the next popup reads stale hashKey from session storage and looks unlocked.

Fix (three parts):

1. Export a new flushSessionStore(store) helper from extension/src/background/store.ts that explicitly awaits chrome.storage.session.set:

export const flushSessionStore = async (storeToFlush: Store): Promise<void> => {
  if (!SESSION_STORAGE_ENABLED) return;
  try {
    const serializedState = JSON.stringify(storeToFlush.getState());
    await sessionStore.setItem(REDUX_STORE_KEY, serializedState);
  } catch {
    // Best-effort — session storage may be unavailable (Firefox, jest mocks).
    // The pre-existing subscribe-based save would have failed the same way.
  }
};

2. await flushSessionStore(sessionStore) in clearSession (extension/src/background/helpers/session.ts), between the dispatches and the localStore.remove(TEMPORARY_STORE_ID). Add the same flush to signOut (extension/src/background/messageListener/handlers/signOut.ts) between dispatch(logOut()) and localStore.remove(...), so the explicit-logout path is symmetric.

3. Defense-in-depth in extension/src/background/ducks/session.ts — make TEMPORARY_STORE_ID absence authoritative for the locked state, so a partial lock cannot leave the wallet looking unlocked even if flush ever races:

export const buildHasPrivateKeySelector = (localStore: DataStorageAccess) =>
  createSelector(sessionSelector, async (session) => {
    const isHardwareWalletActive = await getIsHardwareWalletActive({ localStore });
    if (isHardwareWalletActive && !session?.isHardwareWalletLocked) return true;
    if (!session?.hashKey?.key) return false;
    const tempStore = await localStore.getItem(TEMPORARY_STORE_ID);
    return !!tempStore && Object.keys(tempStore).length > 0;
  });

Bug C — unlocking in one surface doesn't unlock the others

Repro: open popup + sidebar + a fullscreen tab, all locked. Enter the password in one. The other two stay on /unlock-account (or /verify-account) forever.

Root cause: each surface owns its own popup-side redux store. Only the surface that submitted the password sees confirmPassword.fulfilled. No background-to-popup push exists for the unlock event — mirror of the gap the current branch fixes for SESSION_LOCKED.

Fix:

1. Add SESSION_UNLOCKED to SERVICE_TYPES in @shared/constants/services.ts.

2. Extract a single broadcastSessionState helper in a new file extension/src/background/messageListener/helpers/broadcast-session-state.ts:

export const broadcastSessionState = async (
  type: SERVICE_TYPES.SESSION_LOCKED | SERVICE_TYPES.SESSION_UNLOCKED,
): Promise<void> => {
  try {
    await browser.runtime.sendMessage({ type });
  } catch {
    // No receivers — harmless.
  }
};

Use it from initAlarmListener (replacing the inline try/catch), signOut (new — sign-out should also propagate), and loginToAllAccounts (new — SESSION_UNLOCKED after the successful login).

3. In loginToAllAccounts (extension/src/background/messageListener/helpers/login-all-accounts.ts), at the end of the success path: await flushSessionStore(sessionStore) before broadcastSessionState(SERVICE_TYPES.SESSION_UNLOCKED). The flush guarantees other surfaces' next loadAccount sees the freshly-set hashKey instead of racing the subscribe-based save.

4. Extend SessionLockListener (extension/src/popup/components/SessionLockListener/index.tsx) to also handle SESSION_UNLOCKED:

   • Call loadAccount and dispatch(saveAccount(account)) to refresh the surface's popup-side state.
   • If location.pathname is either ROUTES.unlockAccount or ROUTES.verifyAccount, navigate(ROUTES.account, { replace: true }). Do not try to restore state.from here — cross-surface unlocks should land everyone on the home screen, because each surface owns its own in-flight context and forcing one surface back into another's interrupted flow is more confusing than helpful. The surface that actually entered the password still restores its own state.from via UnlockAccount.handleSubmit.
   • If the surface isn't on a lock screen, just refresh state via saveAccount and leave the route alone.

if (type === SERVICE_TYPES.SESSION_UNLOCKED) {
  void (async () => {
    try {
      const account = await loadAccount();
      dispatch(saveAccount(account));
      const onLockScreen =
        location.pathname === ROUTES.unlockAccount ||
        location.pathname === ROUTES.verifyAccount;
      if (onLockScreen) {
        navigate(ROUTES.account, { replace: true });
      }
    } catch (err) {
      console.error("SessionLockListener failed to refresh on SESSION_UNLOCKED", err);
    }
  })();
  return undefined;
}

Test updates required

• extension/src/background/messageListener/__tests__/loadSaveSettings.test.ts — makeLocalStore must now return a populated TEMPORARY_STORE_ID (e.g. { "key-id-0": "encrypted-blob" }) by default, because buildHasPrivateKeySelector requires both the in-memory hashKey AND TEMPORARY_STORE_ID for the "unlocked" assertions. Add an opt-out flag (hasTempStore: boolean = true) so the "wallet is locked" tests can simulate the absent case.
• Existing SessionLockListener tests in extension/src/popup/components/__tests__/SessionLockListener.test.tsx need a new case covering SESSION_UNLOCKED:
  • listener calls loadAccount and dispatches saveAccount
  • navigates to ROUTES.account from both ROUTES.unlockAccount and ROUTES.verifyAccount
  • does not navigate from non-lock routes
• extension/src/background/__tests__/initAlarmListener.test.ts should keep asserting the broadcast happens after clearSession, but the broadcast now goes through broadcastSessionState rather than the inline try/catch — the assertion target should be the helper or browser.runtime.sendMessage with type: SERVICE_TYPES.SESSION_LOCKED.

Acceptance criteria

1. Set the auto-lock timeout to its shortest value, close the popup, wait past the timeout, reopen the popup — the popup lands on the unlock screen.
2. Open popup + sidebar + fullscreen, all locked. Enter the password in one — the other two automatically navigate to the home screen.
3. Sign out from one surface — the other open surfaces flip to the unlock screen.
4. yarn jest passes; build succeeds.

[JakeUrban]

👤 Human PR Review Feedback (PR #2802)

The following feedback was left on the PR and will now be addressed:

1. @leofelix077 (review, CHANGES_REQUESTED) — UI still does not flip to the unlock screen after the idle timer fires. The popup keeps showing Account/Assets/History (just with no private key) and a password prompt only surfaces mid-action. Question: was the intention "fully locked" UI after idle, or the current behavior? (source)

2. @leofelix077 (PR conversation, 4555106070) — Three reported defects observed in manual testing:

   • (2a) Unlocking on one surface (popup / sidebar / fullscreen) does not unlock the other open surfaces — each stays on its unlock screen.
   • (2b) If the popup is closed before the timer expires, the lock UI never flips: reopening the popup shows the unlocked Account screen, the timer is gone, and the wallet appears permanently unlocked (a sign attempt does correctly re-prompt, but nothing forces the UI back to the lock screen).
   • (2c) In fullscreen mode the activity events don't register, so the timer counts down without being refreshed and auto-locks regardless of interaction.

3. @leofelix077 (PR conversation, 4555117182) — Detailed root-cause analysis with concrete fixes for three correctness bugs (these are the actionable fixes for items 1 and 2 above):

   • Bug A — Popup stays on Account after auto-lock fires. useGetAppData.fetchData only re-routes to ROUTES.unlockAccount when !account.publicKey, but clearSession deliberately keeps publicKey and only wipes hashKey. Fix: also re-route when !account.hasPrivateKey in extension/src/helpers/hooks/useGetAppData.tsx.
   • Bug B — clearSession's redux dispatch races SW termination. buildStore wires subscribe(() => saveStore(...)) but the chrome.storage.session.set triggered by clearSession's dispatches is fire-and-forget. When the alarm fires with no popup open, the SW terminates before the session write completes; the next popup reads stale hashKey from chrome.storage.session and looks unlocked. Fix (three parts):
     • Export flushSessionStore(store) from extension/src/background/store.ts that explicitly awaits chrome.storage.session.set.
     • await flushSessionStore(...) in clearSession (extension/src/background/helpers/session.ts) between the dispatches and localStore.remove(TEMPORARY_STORE_ID), and similarly in signOut between dispatch(logOut()) and localStore.remove(...).
     • Defense-in-depth in extension/src/background/ducks/session.ts — make TEMPORARY_STORE_ID absence authoritative in buildHasPrivateKeySelector so a partial lock cannot leave the wallet looking unlocked even if the flush ever races.
   • Bug C — Unlocking one surface doesn't unlock the others. Each surface owns its own popup-side redux store; only the surface that submitted the password sees confirmPassword.fulfilled. Fix:
     • Add SERVICE_TYPES.SESSION_UNLOCKED in @shared/constants/services.ts.
     • Extract broadcastSessionState(type) helper in extension/src/background/messageListener/helpers/broadcast-session-state.ts and use it from initAlarmListener (replacing the inline try/catch), signOut (new), and loginToAllAccounts (new — SESSION_UNLOCKED after the successful login).
     • In loginToAllAccounts, await flushSessionStore(...) before broadcasting SESSION_UNLOCKED so the receiving surfaces' next loadAccount sees the fresh hashKey.
     • Extend SessionLockListener to handle SESSION_UNLOCKED: call loadAccount, dispatch saveAccount, and if the current path is ROUTES.unlockAccount or ROUTES.verifyAccount, navigate to ROUTES.account (replace). Do not restore state.from cross-surface — only the surface that entered the password should restore its own interrupted flow.

Test updates required (per the reviewer)

• extension/src/background/messageListener/__tests__/loadSaveSettings.test.ts — makeLocalStore must return a populated TEMPORARY_STORE_ID by default (since buildHasPrivateKeySelector now requires both hashKey and TEMPORARY_STORE_ID for the "unlocked" assertions). Add an opt-out flag for "wallet locked" cases.
• extension/src/popup/components/__tests__/SessionLockListener.test.tsx — new cases covering SESSION_UNLOCKED (calls loadAccount + dispatches saveAccount; navigates to ROUTES.account from both ROUTES.unlockAccount and ROUTES.verifyAccount; does not navigate from non-lock routes).
• extension/src/background/__tests__/initAlarmListener.test.ts — assertion target shifts to the broadcastSessionState helper / browser.runtime.sendMessage({ type: SERVICE_TYPES.SESSION_LOCKED }).

Acceptance criteria (from the reviewer)

1. Set the auto-lock timeout to its shortest value, close the popup, wait past the timeout, reopen — the popup lands on the unlock screen.
2. Open popup + sidebar + fullscreen, all locked. Enter the password in one — the other two auto-navigate to the home screen.
3. Sign out from one surface — the other open surfaces flip to the unlock screen.
4. yarn test:ci passes; yarn build:extension succeeds.

---

Addressing feedback now…

[JakeUrban]

🔬 Internal Review-Fix Report (Round 1/3)

Full review report (click to expand)

Fix Review: 6b22e3a

Commit Summary

• Hash: 6b22e3a4
• Files changed: 12 modified, 3 created (broadcast-session-state.ts, signOut.test.ts, login-all-accounts.test.ts)

Verdict: CONCERNS

Key finding

Bug A is only partially addressed. The post-loadAccount() reroute in extension/src/helpers/hooks/useGetAppData.tsx:75-90 now checks !account.hasPrivateKey, but the cache fast path at useGetAppData.tsx:58-66 still treats currentAccount.publicKey alone as sufficient to return RESOLVED. After a locked loadAccount() response is saved into Redux (saveAccount(account) at :70-73), a later cached fetchData() can still return RESOLVED for publicKey present + hasPrivateKey: false, so the publicKey-vs-hasPrivateKey invariant survives in the cached path.

Recommended actions

1. Make the cache fast path also require currentAccount.hasPrivateKey (or reuse a shared isUnlockedAccount(account) helper across both paths so the predicate is defined once).
2. Add a direct regression test for useGetAppData covering publicKey present + hasPrivateKey: false.

Other observations

• Bugs B and C are addressed at root cause; persistence-ordering and broadcast helpers are well-factored and consistent with project conventions.
• buildHasPrivateKeySelector defense-in-depth is correct.
• Cross-surface unlock navigation correctly avoids cross-surface state.from restoration.
• Test coverage for Bugs B and C is solid (new signOut.test.ts and login-all-accounts.test.ts exercise flush-then-broadcast ordering).

Test results from review

• yarn jest on targeted suites: 5 suites / 37 tests passed
• initAlarmListener.test.ts: 3 passed
• SubmitTransaction.test.tsx: 4 passed
• yarn build:extension: passed

Verdict: CONCERNS

Key issue (one):

1. Bug A — incomplete coverage in useGetAppData's cache fast path. The post-loadAccount() reroute now correctly checks !account.hasPrivateKey, but the cache fast path (useGetAppData.tsx:58-66) still treats currentAccount.publicKey alone as "resolved", so a locked account already in Redux can short-circuit a later cached fetch and bypass the reroute. Recommendation: require hasPrivateKey in the cache check too (ideally via a shared isUnlockedAccount helper), and add a regression test for publicKey present + hasPrivateKey: false.

Fixing now and re-running validation.

[JakeUrban]

🔬 Internal Review-Fix Report (Round 2/3)

Full review report (click to expand)

Fix Review: f3e4598

Commit summary

• 7 files changed: 1 new (useGetAppData.test.tsx), 1 source modified (useGetAppData.tsx), 5 existing tests backfilled with hasPrivateKey: true.

Verdict: SOUND

Findings

• Cache-path fix is correct and complete. The cache fast path now requires both currentAccount.publicKey and currentAccount.hasPrivateKey, matching the post-load reroute gate. The publicKey-implies-unlocked assumption no longer survives anywhere in useGetAppData.
• Regression test is meaningful. The new useGetAppData.test.tsx would have failed before this commit (cached-locked → RESOLVED instead of REROUTE).
• Backfilled test edits are semantically correct. Verified each consumer reroutes on REROUTE (so the test setup actually wants an unlocked account). Specifically confirmed PASSWORD_CREATED + hasPrivateKey: true is valid by checking createAccount.ts and accountServices.ts reducer — account creation legitimately stores both.
• No other consumer treats publicKey as an unlock signal. Route guards (Router.tsx) key off hasPrivateKey; the useGetOnrampToken non-rerouting consumer only reads publicKey after a RESOLVED verdict.
• Combined PR (6b22e3a + f3e4598) fully addresses Bugs A/B/C. Bug B's session-store flushes are in place; Bug C's cross-surface broadcast + listener path holds up.

Optional follow-up (non-blocking)

Extract a shared isUnlockedAccount(account) helper so the cache and post-load predicates in useGetAppData cannot drift apart again. Low priority — current code is correct.

Verdict: SOUND

The cache-path fix is correct, the new regression test would have caught the original bug, and the backfilled hasPrivateKey: true edits in the five existing tests are semantically correct. No remaining publicKey ⇒ unlocked assumption in useGetAppData or its consumers. The combined PR (commits 6b22e3a4 + f3e4598c) fully addresses Bugs A, B, and C.

One optional non-blocking follow-up noted (extract a shared isUnlockedAccount helper) — not gating.

[JakeUrban]

✅ Human PR Review Response (PR #2802)

Changes made

Addresses @leofelix077's three reported bugs (Bugs A, B, C from the detailed analysis) in two commits.

Commit 6b22e3a4 — initial Bug A/B/C implementation:

• Bug A (popup stays on Account after auto-lock). useGetAppData's post-loadAccount reroute now triggers on !account.hasPrivateKey in addition to !account.publicKey, so reopening the popup after the alarm fires lands on the unlock screen even when clearSession preserved publicKey.
• Bug B (clearSession's redux dispatch races SW termination). New flushSessionStore(store) helper in extension/src/background/store.ts explicitly awaits chrome.storage.session.set (and the existing subscribe(saveStore) path now routes through the same helper, unifying persistence). Awaited in clearSession between the dispatches and the TEMPORARY_STORE_ID removal, and in signOut between dispatch(logOut()) and the TEMPORARY_STORE_ID removal, so the session write completes before the SW can terminate. Defense-in-depth: buildHasPrivateKeySelector now treats TEMPORARY_STORE_ID absence as authoritative — unlocked requires both an in-memory hashKey AND a non-empty TEMPORARY_STORE_ID, so a partial lock cannot leave the wallet looking unlocked even if a future flush ever races.
• Bug C (unlocking one surface doesn't unlock the others). New SERVICE_TYPES.SESSION_UNLOCKED + new broadcastSessionState(type) helper at extension/src/background/messageListener/helpers/broadcast-session-state.ts that wraps browser.runtime.sendMessage with the no-receivers swallow. initAlarmListener replaces its inline try/catch with the helper; signOut now also broadcasts SESSION_LOCKED after the lock work (so explicit sign-out flips other surfaces' UI too); loginToAllAccounts awaits flushSessionStore then broadcasts SESSION_UNLOCKED on the success path. SessionLockListener now handles SESSION_UNLOCKED — calls loadAccount, dispatches saveAccount, and navigates to ROUTES.account only when the current path is ROUTES.unlockAccount or ROUTES.verifyAccount (does not restore state.from cross-surface, per the reviewer's spec — only the surface that entered the password restores its own interrupted flow).
• Tests: loadSaveSettings.test.ts updated so makeLocalStore seeds TEMPORARY_STORE_ID by default (with opt-out for locked-wallet cases); new SESSION_UNLOCKED cases in SessionLockListener.test.tsx (calls loadAccount + dispatches saveAccount; navigates to ROUTES.account from both lock routes; does NOT navigate from non-lock routes); session.test.ts updated for flushSessionStore; new signOut.test.ts and login-all-accounts.test.ts assert the flush-then-broadcast ordering. initAlarmListener.test.ts needed no change — its runtime-message assertion still covers the refactor.

Commit f3e4598c — addresses internal review-fix round 1 CONCERN on Bug A:

Round 1 caught that useGetAppData's cache fast path at useGetAppData.tsx:58 still short-circuited on currentAccount.publicKey alone — so after a locked loadAccount response was saved into Redux, a later cached fetchData could still return RESOLVED with hasPrivateKey: false and bypass the reroute. Fix: the cache fast path now also requires currentAccount.hasPrivateKey, making both code paths in useGetAppData enforce the same unlocked invariant.

• New useGetAppData.test.tsx regression test covers both branches: cached publicKey + hasPrivateKey: false re-routes to unlockAccount; cached publicKey + hasPrivateKey: true short-circuits to RESOLVED.
• Backfilled hasPrivateKey: true in five existing test files whose Redux preloadedState seeded an unlocked account with only publicKey (AddFunds.test.tsx, GrantAccess.test.tsx, SearchAsset.test.tsx, useGetWalletsData.test.tsx, useGetAssetDomainsWithBalances.test.tsx) so the cache fast path still short-circuits as those tests intend. Verified semantically: each consumer reroutes on REROUTE, so the setups really do want an unlocked account. PASSWORD_CREATED + hasPrivateKey: true in one GrantAccess case is valid — createAccount legitimately stores both.

Feedback not incorporated

All actionable feedback was incorporated. The reviewer's optional follow-up of extracting a shared isUnlockedAccount(account) helper inside useGetAppData (noted in internal review round 2) is a non-blocking nice-to-have and is intentionally left for a small future cleanup — the current code is correct and consistent across both paths.

Internal review

Passed internal review-fix in 2 round(s). Round 1 verdict: CONCERNS (Bug A's cache fast path). Round 2 verdict: SOUND.

Commits added to PR

• 6b22e3a4 — Address PR review feedback: idle auto-lock cross-surface consistency (Bugs A/B/C + tests).
• f3e4598c — Address internal review: require hasPrivateKey in useGetAppData cache fast path (+ direct regression test, + 5 existing-test backfills).

Validation

• yarn test:ci → 985 passed, 72 skipped, 0 failed.
• yarn build:extension → clean.

Acceptance criteria status

1. ✅ Set the auto-lock timeout to its shortest value, close the popup, wait past the timeout, reopen — popup lands on the unlock screen. (Bug A + Bug B: cache path + flushSessionStore + TEMPORARY_STORE_ID-authoritative selector.)
2. ✅ Open popup + sidebar + fullscreen, all locked. Enter the password in one — the other two auto-navigate to the home screen. (Bug C: SESSION_UNLOCKED broadcast + listener.)
3. ✅ Sign out from one surface — the other open surfaces flip to the unlock screen. (Bug C extension: signOut now broadcasts SESSION_LOCKED after the lock work.)
4. ✅ yarn test:ci passes; yarn build:extension succeeds.

Project board note

Same as the prior round — the running GH_TOKEN is missing the project scope, so the issue's project Status was not transitioned. The PR is updated and the issue assignment is preserved so the team can move it manually if needed.

---

PR #2802 updated and ready for re-review.

[JakeUrban]

🔧 Auto-Lock Cross-Surface Fix — Additional Round

Resolves the user-reported regression where opening Freighter in a second surface (sidebar → popup, popup → fullscreen, etc.) while unlocked still caused the new surface's session to expire at the original surface's idle deadline rather than resetting on mount.

Root cause (confirmed via runtime instrumentation)

popupMessageListener.isFromExtensionPage rejected USER_ACTIVITY pings from dApp-spawned signing popups as Unauthorized. The original gate was !sender.tab && (!sender.id || sender.id === browser.runtime.id), which conflated "has a tab" with "is a content script". dApp-spawned signing popups created via browser.windows.create({type:"popup", url:"index.html#..."}) are full tabs (so have sender.tab) but their sender.url is on the extension origin. Three handlers used this gate (USER_ACTIVITY, REJECT_SIGNING_REQUEST, OPEN_SIDEBAR); all three were latently broken for popup-window callers.

Fix (commit c0f2e16)

Gate now accepts a sender if sender.id matches our extension AND either sender.tab is absent or sender.url starts with browser.runtime.getURL(""). Content scripts still rejected (their sender.url is the dApp page URL); extension-origin tabbed pages now accepted.

Also includes the upstream surface-mount fix (b3868f9): mount ping fires in its own dependency-less useEffect so the BG handler is the authoritative unlock-state gate (reads hashKey + hardware-wallet flags directly).

Validation

• 993 unit tests pass locally. New regression test in sidebar.test.ts exercises the extension-origin tabbed sender path.
• CI green after retry — initial run had an unrelated 60s-timeout flake in sendIntegration.test.ts:298 "Send token payment to C address" (Soroban simulation). Rerun: clean.
• Self-review of c0f2e16: SOUND. No similar sender.tab/sender.id gates exist elsewhere in the codebase.

Files changed since last update

• extension/src/background/messageListener/popupMessageListener.ts — widened isFromExtensionPage gate
• extension/src/background/messageListener/handlers/userActivity.ts — authoritative BG unlock gate
• extension/src/popup/helpers/hooks/useActivityPing.ts — unconditional mount ping
• extension/src/background/messageListener/__tests__/sidebar.test.ts — regression test for extension-tab sender
• extension/src/popup/helpers/hooks/__tests__/useActivityPing.test.ts — updated tests for unconditional mount ping
• extension/src/background/messageListener/__tests__/userActivity.test.ts — rewritten for new BG gate

[JakeUrban]

Review — round 1

Overall the idle-timer mechanism is sound: SessionTimer reading the timeout per call, alarm-as-source-of-truth, the dual gate (hasPrivateKey on the popup side, authoritative session state on the BG side), and the cross-surface SESSION_LOCKED / SESSION_UNLOCKED broadcast all hang together cleanly. The narrowing in 7b3e4dd (drop the surface-mount ping, only user input rearms) is the right call and the security reasoning in that commit message is the most important paragraph in the whole PR.

That said, the PR has accreted across enough scope shifts that there's noticeable drift between what the description says, what the comments say, and what the code now does. The main asks are below.

1. Stale narrative — PR body and in-code comments don't match the shipped behavior

The PR description and one of the handler doc-comments still describe a previous scope and will mislead reviewers:

• PR description, "How it works" → bullets 1 and 4 are out of date.

  • Bullet 1 still says useActivityPing "sends one USER_ACTIVITY message to the background per 5 seconds (leading-edge throttle)" and lists mousemove, keydown, click, scroll, touchstart. The actual hook listens for mousedown, keydown, touchstart, wheel, and the file comment specifically explains why scroll was dropped. mousemove is gone too (commit 7b3e4dd). Worth aligning so a future reviewer doesn't go looking for a mousemove listener that isn't there.
  • Bullet 4 ("Settings save edge case") describes a wasLocked: true short-circuit that no longer exists. The current saveSettings handler treats the save itself as activity and just calls resetSession() — there is no immediate-lock path, no wasLocked field in the response, and no popup-side lockAccount dispatch from this code path. The new ducks test (settings.test.ts) even has a comment explicitly noting "Background no longer reports an immediate-lock back to the popup." Either remove the bullet or rewrite it to match.

• extension/src/background/messageListener/handlers/userActivity.ts — the doc comment is stale. It says pings come "from two sources inside useActivityPing: 1. An unconditional mount ping fired by every Freighter surface … 2. Throttled user-input events …". Source (1) was deliberately removed in 7b3e4dd — the hook explicitly does NOT ping on mount, and the commit message walks through why (programmatic dApp-spawned popups would otherwise reset the alarm without user presence). Please update this comment; it currently contradicts both the hook's own docstring and the security argument that motivated the change.

2. Leftover scaffolding from removed wasLocked path

@shared/api/internal.ts saveSettings:

• The response stub still initializes wasLocked: false and the return type still includes & { wasLocked?: boolean }. Nothing in the system reads or sets wasLocked anymore — the background handler doesn't return it, and the popup ducks/settings test was rewritten specifically to assert it isn't acted on. Drop the field and the union.
• The (await sendMessageToBackground({ … })) as unknown as typeof response double-cast was added when the response shape diverged for the wasLocked field. With wasLocked gone, please re-evaluate whether the cast is still needed.

3. Out-of-scope change riding along: useGetAppData cache fast-path fix

extension/src/helpers/hooks/useGetAppData.tsx and the new extension/src/helpers/__tests__/useGetAppData.test.tsx add a hasPrivateKey requirement to the cache fast path. The test even calls itself "Bug A regression" and the commit message labels it "Address internal review: require hasPrivateKey in useGetAppData cache fast path." It's a real bug fix — locked sessions with a cached publicKey would otherwise skip the unlock reroute — but it has nothing to do with idle auto-lock semantics. It should be split into its own PR (or at minimum called out in the description) so it can be reviewed, bisected, and reverted independently of the idle-timer work. As shipped it's hidden inside a 60-file PR with a misleading scope.

4. saveSettings validation is effectively dead

The handler returns { error: "Invalid autoLockTimeoutMinutes" } on invalid input, but:

• The caller in @shared/api/internal.ts saveSettings types the response as Settings & IndexerSettings and silently overwrites response with the stub on any error in the catch. There's no error branch in the typed response, so the popup has no way to surface this to the user.
• The only producer of autoLockTimeoutMinutes in the popup is the <Select> populated from VALID_AUTO_LOCK_TIMEOUT_MINUTES, coerced through coerceAutoLockTimeoutMinutes before dispatch. The invalid path is essentially unreachable from the UI today.

Either: (a) propagate the error so it's actually surfaceable (and add a UI path), or (b) drop the early-return and rely on coerceAutoLockTimeoutMinutes to clamp to default — the isValidAutoLockTimeoutMinutes check would then live only at storage-read time, where the existing coerceAutoLockTimeoutMinutes already handles it. The current half-state — validate but provide no way to act on the result — adds complexity without protection.

5. handleSignedHwPayload rearms the timer before validating the request

The handler calls sessionTimer.resetSession() as its first statement, before the uuid lookup. If the request is malformed (!uuid) we still extend the idle alarm. The unit test asserts this is intentional ("still records user activity before returning an error for a missing uuid"). I'd push back: rearming on a malformed request can only happen via internal bugs or message tampering, never as a side-effect of legitimate HW signing. Reset only on the success branch (transactionResponse && typeof transactionResponse.response === "function"), and have the failure branches stay neutral. Also flip the test assertion accordingly.

6. signOut stop-alarm ordering

signOut:

sessionStore.dispatch(logOut())
flushSessionStore(sessionStore)
localStore.remove(TEMPORARY_STORE_ID)
sessionTimer.stopSession()
broadcastSessionState(SESSION_LOCKED)

If a pending alarm fires between the remove() and the stopSession(), the alarm handler calls clearSession on an already-cleared session and then broadcastSessionState(SESSION_LOCKED) — followed immediately by signOut's own broadcast. The double broadcast is benign (SessionLockListener is idempotent when already on /unlock-account), but the cleaner order is stopSession() first, then the state mutations. That eliminates the race entirely.

7. Test value analysis — what's earning its keep

You asked specifically about test value. My read of the 14 new/expanded test files:

Genuinely valuable, keep:

• helpers/__tests__/session.test.ts — covers the per-call timeout read and the missing/invalid fallback; this is the core of the rewrite.
• messageListener/__tests__/userActivity.test.ts — covers the hot/HW/locked matrix that the handler comment relies on.
• messageListener/__tests__/loadSaveSettings.test.ts — the autoLockTimeoutMinutes validation + rearm branches are non-trivial and pin behavior that's easy to regress.
• __tests__/initAlarmListener.test.ts — pins the SESSION_ALARM_NAME → clearSession → broadcast chain; this is the contract everything else depends on.
• components/__tests__/SessionLockListener.test.tsx — substantial (233 lines) but justified: covers the cross-surface broadcast handling, the route preservation, and the synchronous-handler invariant that the inline comment explains. If anything, the only weak test in here is the catch-all "ignores other message types" boilerplate — fine to keep but it's filler, not signal.
• popup/helpers/hooks/__tests__/useActivityPing.test.ts — pins the throttle and the unlocked-only behavior.

Lower value but motivated by a real scope change, keep:

• messageListener/__tests__/signOut.test.ts — invocation-order assertions are mostly self-documenting, but the broadcast ordering is the contract SessionLockListener depends on. Keep.
• messageListener/__tests__/sidebar.test.ts — directly motivated by the isFromExtensionPage gate widening in c0f2e16. Keep; it's the regression test for "did we break OPEN_SIDEBAR while widening the gate for USER_ACTIVITY."
• messageListener/__tests__/login-all-accounts.test.ts — exists only to assert the SESSION_UNLOCKED broadcast fires. Keep, but it could be a single assertion appended to an existing test instead of a 116-line file.

Mechanical fixture updates, not coverage:

• The eight view tests (Account, AccountCreator, AccountHistory, AddFunds, GrantAccess, ManageAssets, SignTransaction, Swap) only add autoLockTimeoutMinutes: DEFAULT_AUTO_LOCK_TIMEOUT_MINUTES to mocked loadSettings fixtures. Necessary because Settings gained a required field; not actual new coverage. No action needed, just calling it out so the line-count doesn't read as "we added tests for those views."

Should be split out with #3:

• helpers/__tests__/useGetAppData.test.tsx — the "Bug A regression" test is the only coverage for that fix and is otherwise orphaned in this PR.

Mixed concerns:

• messageListener/__tests__/handleSignedHwPayload.test.ts — the third case ("tracks hardware-wallet lock and unlock state transitions") is really a sessionSlice reducer test that wandered into the handler file. Move it to a ducks/session.test.ts (or drop it; the reducer is trivial). The first two cases are fine. Also: if you take Piyal dev 2 #5, the second case ("still records user activity before returning an error") needs its assertion flipped.

8. Smaller things

• extension/src/popup/components/ActivityTracker/index.tsx is a 14-line wrapper around useActivityPing whose only job is to call useSelector(hasPrivateKeySelector) and pass the boolean in. Given the hook is mounted in exactly one place (App.tsx), consider inlining the selector call into the hook itself and dropping the wrapper component. It's not load-bearing.
• broadcast-session-state.ts swallows all errors with catch {}. The current behavior is correct for "no receivers" but will also silently hide a genuinely broken broadcast. A console.warn in the catch (gated on whether the message matches the "no receivers" pattern) would help diagnose future regressions without changing user-visible behavior.
• SessionLockListener re-runs its useEffect on every location change because location is in the dep array. That re-registers runtime.onMessage on every navigation. Functionally fine (the cleanup removes the old listener), but it's a hot path; consider moving the handler into a useRef and keeping the effect deps to [dispatch, navigate].

---

None of these block merge on the core mechanism, but #1–#3 are worth resolving before this lands so the artifact matches its description and the unrelated bugfix gets its own review.

[JakeUrban]

Meta-review of round-1 review (GPT-5.5)

Verdict: revise the round-1 review. It has several high-signal findings, but one concrete claim is false and the test/scope analysis needs tightening.

Accept these points

• Stale PR/body + handler narrative: accept. The PR body still describes mousemove/click/scroll and the old settings immediate-lock path, while useActivityPing now listens only to mousedown, keydown, touchstart, and wheel, and explicitly does not ping on mount (extension/src/popup/helpers/hooks/useActivityPing.ts:8-18, :40-48). The USER_ACTIVITY handler comment is also stale: it still says pings come from an unconditional mount ping plus input events (extension/src/background/messageListener/handlers/userActivity.ts:12-19).
• useGetAppData cache fast-path is a separate bugfix: accept. The actual change is the added currentAccount.hasPrivateKey gate in extension/src/helpers/hooks/useGetAppData.tsx:58-62, and the test labels itself a “Bug A regression” (extension/src/helpers/__tests__/useGetAppData.test.tsx:30-35). This should be split out or at least explicitly called out.
• saveSettings validation is not surfaced: accept, but with corrected framing below. The background returns { error: "Invalid autoLockTimeoutMinutes" } (extension/src/background/messageListener/handlers/saveSettings.ts:50-52), but the popup thunk/reducer path treats the call as a fulfilled Settings & IndexerSettings payload and never handles that error (extension/src/popup/ducks/settings.ts:128-178, :441-474).
• handleSignedHwPayload currently rearms before validating: accept. The reset happens before the missing-uuid check (extension/src/background/messageListener/handlers/handleSignedHwPayload.ts:21-30), and the test locks that in (extension/src/background/messageListener/__tests__/handleSignedHwPayload.test.ts:45-55).
• signOut alarm-clear ordering: accept as a reasonable cleanup. Current order mutates/flushed/removes state before clearing the alarm and broadcasting (extension/src/background/messageListener/handlers/signOut.ts:23-29).

Drop or revise these points

• Drop the wasLocked scaffolding claim as written. @shared/api/internal.ts does not initialize wasLocked: false, and the return type is just Promise<Settings & IndexerSettings> (@shared/api/internal.ts:1633-1649). The double-cast is real (@shared/api/internal.ts:1652-1661), but it should not be justified by a nonexistent wasLocked field.
• Revise the test-count/test-value section. The PR touches 25 test files (gh pr diff 2802 --name-only | grep -E '(__tests__|\.test\.)'), not 14. Some omitted tests are not just generic view fixture churn: accountServices.test.ts covers the new lockAccount reducer semantics (extension/src/popup/ducks/__tests__/accountServices.test.ts:30-45) for the reducer implemented at extension/src/popup/ducks/accountServices.ts:606-622.
• Revise the “view tests only add autoLockTimeoutMinutes” framing. Several fixture changes are actually fallout from the useGetAppData hasPrivateKey fast-path change and should move with that split-out bugfix if Piyal dev #3 is acted on: e.g. extension/src/helpers/__tests__/useGetAssetDomainsWithBalances.test.tsx:78-82, extension/src/popup/components/__tests__/SearchAsset.test.tsx:31-36, and extension/src/popup/views/Wallets/hooks/__tests__/useGetWalletsData.test.tsx:115-118.
• Treat the ActivityTracker inlining suggestion as optional/low-value, not an ask. The wrapper is thin, but it keeps useActivityPing as a pure boolean-driven hook (extension/src/popup/components/ActivityTracker/index.tsx:13-16, extension/src/popup/helpers/hooks/useActivityPing.ts:48-70). Inlining Redux into the hook would make the hook less reusable/testable for little gain.

Issues the round-1 review missed

• loadSaveSettings.test.ts still contains obsolete immediate-lock scaffolding. The “rearms when the user shortens the timeout” test sets up alarmsGet and narrates elapsed idle time from the previous implementation (extension/src/background/messageListener/__tests__/loadSaveSettings.test.ts:269-280), but the current handler never reads browser.alarms.get; it only treats save as activity and calls sessionTimer.resetSession() when unlocked (extension/src/background/messageListener/handlers/saveSettings.ts:66-78). Keep the current behavior assertion if desired, but remove the unused alarmsGet setup and rewrite the comment around the current implementation.
• If useGetAppData is split out, move its fixture fallout too. The review correctly identified the main useGetAppData regression test, but missed the dependent hasPrivateKey: true fixture additions in cached-path tests cited above. Leaving those behind would keep obsolete-scope test churn in the idle-timer PR.

Posted by GPT-5.5 meta-reviewer.

[JakeUrban]

Review — round 2 (revised after meta-review)

Picking up GPT-5.5's meta-review (comment). Retracting one false claim, tightening the test-analysis, and adding one finding the round-1 review missed.

Retractions

• Retract round-1 §2 first bullet ("wasLocked: false leftover in @shared/api/internal.ts"). I was wrong. The response stub at @shared/api/internal.ts:1634-1650 does not contain a wasLocked field, and the return type is plain Promise<Settings & IndexerSettings> (line 1633) — no & { wasLocked?: boolean }. There is no leftover scaffolding from the removed immediate-lock path in this file. Apologies for the noise.

  The as unknown as typeof response double-cast at lines 1652-1661 is still real, but it's just papering over the looser sendMessageToBackground return type — not specifically tied to the dropped wasLocked field. Re-evaluate or annotate as you see fit; it's a minor cleanup, not the scope-drift artifact I implied.

Revisions

• Round-1 §7 test count is wrong (14 → 25). The PR touches 25 test files, not 14. I undercounted by excluding __tests__/SearchAsset.test.tsx, __tests__/useGetAssetDomainsWithBalances.test.tsx, Wallets/hooks/__tests__/useGetWalletsData.test.tsx, and popup/ducks/__tests__/accountServices.test.ts, among others.

• popup/ducks/__tests__/accountServices.test.ts is not mechanical churn. Round-1 implicitly lumped it with the fixture updates; correcting: it's the dedicated coverage for the new lockAccount reducer (extension/src/popup/ducks/accountServices.ts:606-622), which is load-bearing for the SessionLockListener immediate flip. Keep, and treat as part of the high-value tier.

• Three "view" fixture updates are actually fallout from the out-of-scope useGetAppData fix, not from the new settings field. These add hasPrivateKey: true to preloaded auth state to satisfy the new cache fast-path requirement in useGetAppData.tsx:58-62:

  • extension/src/helpers/__tests__/useGetAssetDomainsWithBalances.test.tsx:81
  • extension/src/popup/components/__tests__/SearchAsset.test.tsx:35,116
  • extension/src/popup/views/Wallets/hooks/__tests__/useGetWalletsData.test.tsx:68,117

  If §3 of round 1 is acted on (split the useGetAppData fix into its own PR), these three files should move with it. Leaving them behind would keep stale-scope test churn in the idle-timer PR.

Issue round-1 missed

• loadSaveSettings.test.ts still carries dead scaffolding from the removed wasLocked path. The test "rearms (rather than locking) when the user shortens the timeout" (extension/src/background/messageListener/__tests__/loadSaveSettings.test.ts:269-303) sets up alarmsGet.mockResolvedValue(...) and narrates "elapsed idle (59 min) ≫ new timeout (5 min)" — but the current saveSettings handler (extension/src/background/messageListener/handlers/saveSettings.ts:66-78) never reads browser.alarms.get. The alarmsGet mock is unreferenced by the SUT, and the test's preamble describes the previous implementation's branching, not the current behavior. It also asserts (result as any).wasLocked).toBeUndefined() against a return type that no longer admits the field.

  Keep the positive assertion (resetSession called once, stopSession not called, no dispatch), drop the alarmsGet setup, drop the wasLocked assertion, and rewrite the comment to describe what's actually being tested: "save-as-activity always rearms when unlocked; no immediate-lock branch exists." This is exactly the class of finding you flagged in the prompt — a test that's still there because of an older scope.

Standing issues from round 1 (unchanged)

§1 (stale PR body + stale userActivity.ts doc comment), §3 (split out useGetAppData fix), §4 (saveSettings validation has no UI path), §5 (handleSignedHwPayload rearms before validating), §6 (signOut alarm-clear ordering), §8 minor items — all stand. See round 1 for details and citations.

---

Net effect: one bullet retracted, two reframed, one new dead-scaffolding finding added. The substance of the review (scope drift in narrative and tests, one orphaned bugfix) is unchanged.

[JakeUrban]

Meta-review of round-2 review (GPT-5.5)

Verdict: revise round 2 lightly. The main conclusions are sound: the @shared/api/internal.ts wasLocked retraction is correct (@shared/api/internal.ts:1633-1650), the changed-test-file count is 25, and the loadSaveSettings.test.ts stale shortened-timeout finding is real (extension/src/background/messageListener/__tests__/loadSaveSettings.test.ts:269-299; current handler only rearms when unlocked at extension/src/background/messageListener/handlers/saveSettings.ts:66-78).

Two concrete fixes before accepting:

• The useGetAppData fixture-fallout section is directionally right but incomplete/imprecise. useGetAppData now requires both publicKey and hasPrivateKey on the cache path (extension/src/helpers/hooks/useGetAppData.tsx:58-62), and the cited fixture additions are useGetAssetDomainsWithBalances.test.tsx:78-82, SearchAsset.test.tsx:31-36 and :112-117, plus three useGetWalletsData additions at extension/src/popup/views/Wallets/hooks/__tests__/useGetWalletsData.test.tsx:67-70, :115-118, and :169-172 (round 2 cites :68,117 and misses :171). If counting “three updates” as files, say files; if counting additions, it is six added hasPrivateKey: true fixtures across those files.

• The loadSaveSettings.test.ts cleanup should be broader than the one shortened-timeout case. The whole browser.alarms test shim (alarmsGet, alarmsCreate, alarmsClear) is now dead in this file (extension/src/background/messageListener/__tests__/loadSaveSettings.test.ts:11-23) because saveSettings receives a mocked sessionTimer and the current handler never calls browser.alarms.get/create/clear (extension/src/background/messageListener/handlers/saveSettings.ts:66-78). Also remove the earlier obsolete wasLocked assertion in the valid-timeout test (extension/src/background/messageListener/__tests__/loadSaveSettings.test.ts:237-254), not only the one at :289-299.

I did not find an additional confirmed issue in lockHardwareWallet/isHardwareWalletLocked usage or the SESSION_UNLOCKED listener path: HW idle-lock state is set on clear and cleared on password unlock (extension/src/background/helpers/session.ts:304-315, extension/src/background/messageListener/handlers/confirmPassword.ts:70-78), and SessionLockListener intentionally fire-and-forgets loadAccount while leaving navigation to UnlockAccount's hasPrivateKey transition (extension/src/popup/components/SessionLockListener/index.tsx:75-90, extension/src/popup/views/UnlockAccount/index.tsx:35-54).

Posted by GPT-5.5 meta-reviewer.

[JakeUrban]

Review — round 3 (final)

Adopting GPT-5.5's round-2 corrections (comment). This is the third and final round; net effect is two precision fixes on the cleanup asks already filed.

Correction to round-2 "fixture fallout" section

The useGetAppData cache fast-path now requires both publicKey AND hasPrivateKey (extension/src/helpers/hooks/useGetAppData.tsx:58-62). The hasPrivateKey: true fixture additions that exist solely to satisfy that new gate live in 3 files / 6 sites — round 2 listed 5 sites and missed the third occurrence in useGetWalletsData.test.tsx:

• extension/src/helpers/__tests__/useGetAssetDomainsWithBalances.test.tsx:81
• extension/src/popup/components/__tests__/SearchAsset.test.tsx:35, :116
• extension/src/popup/views/Wallets/hooks/__tests__/useGetWalletsData.test.tsx:68, :117, :171

All six should move with the useGetAppData bugfix if it gets split into its own PR (round-1 §3).

Broader cleanup for loadSaveSettings.test.ts

Round-2 flagged only the "rearms when the user shortens the timeout" test. The broader dead-scaffolding picture in that file:

1. The whole browser.alarms shim is unreferenced by the SUT (extension/src/background/messageListener/__tests__/loadSaveSettings.test.ts:11-23). The current saveSettings handler never calls browser.alarms.get / create / clear — it operates entirely through the injected sessionTimer mock (extension/src/background/messageListener/handlers/saveSettings.ts:66-78). The alarmsGet / alarmsCreate / alarmsClear jest.fns, the beforeEach that re-installs them onto browser.alarms, and the import browser from "webextension-polyfill" that exists only to support that re-installation can all be deleted.

2. wasLocked assertions appear twice, not just once. Both should go:

   • :253 — expect((result as any).wasLocked).toBeUndefined(); inside the "persists a valid timeout and reschedules when unlocked" test.
   • :299 — same assertion inside the "rearms (rather than locking) when the user shortens the timeout" test.

   The current handler's return type doesn't admit wasLocked at all; asserting undefined on a never-set field is noise that anchors the test to the removed implementation.

3. The comment block at :269-280 narrating "elapsed idle (59 min) ≫ new timeout (5 min)… the previous implementation would have detected this and locked immediately" describes the removed path, not what the test now verifies. Rewrite as: "save-as-activity always rearms when unlocked; no immediate-lock branch exists."

What's NOT a finding

For completeness — GPT-5.5 and I both checked the following and found nothing actionable:

• lockHardwareWallet / isHardwareWalletLocked / unlockHardwareWallet in background/ducks/session.ts:120-132 are correctly load-bearing: HW idle-lock is set on alarm-driven clearSession (extension/src/background/helpers/session.ts:304-315) and cleared on password unlock (extension/src/background/messageListener/handlers/confirmPassword.ts:70-78). Not vestigial.
• The SESSION_UNLOCKED round-trip via SessionLockListener (extension/src/popup/components/SessionLockListener/index.tsx:75-90) racing the surface that initiated the unlock is acknowledged in-code; the wasLockedOnMount ref in UnlockAccount (extension/src/popup/views/UnlockAccount/index.tsx:35-54) is the correct mitigation. Not a finding.

Standing asks (consolidated)

Carried forward unchanged from rounds 1–2:

1. PR description out of date (mount-ping, wasLocked short-circuit, event list with mousemove/click/scroll).
2. userActivity.ts handler doc comment is stale — still describes the removed unconditional mount ping.
3. Split out the useGetAppData hasPrivateKey fast-path fix into its own PR (it's an unrelated bugfix), and carry the 6 fixture sites listed above with it.
4. saveSettings validation has no UI path — either propagate the error or drop the early-return and rely on coerceAutoLockTimeoutMinutes.
5. handleSignedHwPayload rearms before validating — move sessionTimer.resetSession() into the success branch and flip the test assertion.
6. signOut ordering — stop the alarm before mutating/flushing state to eliminate the alarm-vs-clear race.
7. loadSaveSettings.test.ts cleanup as detailed in this round (shim + both wasLocked assertions + comment rewrite).
8. Minor: broadcast-session-state.ts swallows all errors with bare catch {}; SessionLockListener re-registers onMessage on every navigation due to location in the dep array.

Closing the review loop here.

---

3 of 3 rounds completed. Round-1: #2802 (comment) · Round-1 meta: #2802 (comment) · Round-2: #2802 (comment) · Round-2 meta: #2802 (comment)

[piyalbasu]

This PR should use the existing Auto Lock config design here: https://www.figma.com/design/C3G0a4Gd6RQyplRBppGDsL/Freighter-Extension?node-id=4835-35341&m=dev

[piyalbasu]

this type of casting is a code smell. address this type properly

[piyalbasu]

Code review

Five severe issues from a deeper-effort sweep that the PR's idle-lock goal makes user-visible. Sharing as a courtesy — happy to follow up with patches or close as out of scope.

1. signOut leaves HW sessions unlocked. logOut() returns initialState with isHardwareWalletLocked: false, and KEY_ID (hw:… prefix) is never cleared, so getIsHardwareWalletActive still returns true and buildHasPrivateKeySelector's HW branch reports the wallet UNLOCKED after explicit sign-out for any HW user. Likely a regression from adding the new HW-lock flag — signOut should also dispatch(lockHardwareWallet()) (or use clearSession) before broadcasting.

freighter/extension/src/background/messageListener/handlers/signOut.ts

Lines 26 to 33 in 7793c28

	// a pending alarm could fire `clearSession` on already-cleared
	// state and emit a duplicate SESSION_LOCKED broadcast.
	await sessionTimer.stopSession();
	sessionStore.dispatch(logOut());
	await flushSessionStore(sessionStore);
	await localStore.remove(TEMPORARY_STORE_ID);
	await broadcastSessionState(SERVICE_TYPES.SESSION_LOCKED);

2. loginToAllAccounts error catches fall through to broadcast SESSION_UNLOCKED. Both catch blocks call clearSession() (wipes hashKey, sets isHardwareWalletLocked=true, removes TEMPORARY_STORE_ID) but do not return/rethrow. Execution continues to the unconditional startSession() + flushSessionStore() + broadcastSessionState(SESSION_UNLOCKED) at the bottom — arming an idle alarm and telling every Freighter surface the wallet is unlocked even though it was just cleared.

freighter/extension/src/background/messageListener/helpers/login-all-accounts.ts

Lines 130 to 144 in 7793c28

	try {
	await storeActiveHashKey({
	sessionStore,
	hashKey,
	});
	} catch (e) {
	await clearSession({ localStore, sessionStore });
	captureException(`Error storing active hash key: ${JSON.stringify(e)}`);
	}
	// start the timer now that we have active private key
	await sessionTimer.startSession();
	await flushSessionStore(sessionStore);
	await broadcastSessionState(SERVICE_TYPES.SESSION_UNLOCKED);
	};

3. importHardwareWallet never arms the idle alarm. HW-only imports get hasPrivateKey: true via the HW-active branch, but the handler never calls sessionTimer.startSession() (the case in popupMessageListener doesn't even pass sessionTimer). HW-only sessions imported via this path have no auto-lock at all — contradicting the PR's stated security goal.

https://github.com/stellar/freighter/blob/7793c2840d0ff57f69f1e4abdabc81284ad611a7/extension/src/background/messageListener/handlers/importHardwareWallet.ts#L11-L41

4. SessionLockListener fire-and-forget loadAccount can crash the popup. The async IIFE on SESSION_UNLOCKED has no try/catch. A rejection becomes an unhandled promise rejection; if loadAccount() resolves with undefined (background handler errored, transient MV3 SW restart), dispatch(saveAccount(undefined)) hits the destructuring saveAccount reducer and throws TypeError: Cannot destructure property 'hasPrivateKey' of 'undefined'.

freighter/extension/src/popup/components/SessionLockListener/index.tsx

Lines 96 to 103 in 7793c28

	// `hasPrivateKey` and navigate themselves once auth state
	// flips, which covers both the active and passive surfaces.
	void (async () => {
	const account = await loadAccount();
	dispatch(saveAccount(account));
	})();
	return undefined;
	};

5. SessionLockListener skips dispatch(lockAccount()) when popup is already at /unlock-account. The early-return is right for the navigate, but it also skips the redux lockAccount() dispatch. If a surface sits on /unlock-account with hasPrivateKey: true (stale route, manual navigation, or after the wasLockedOnMount capture case), the background locks but the popup's redux keeps hasPrivateKey: true and ActivityTracker keeps sending USER_ACTIVITY pings. The dispatch should run unconditionally; only the navigate should be skipped.

freighter/extension/src/popup/components/SessionLockListener/index.tsx

Lines 76 to 86 in 7793c28

	if (type === SERVICE_TYPES.SESSION_LOCKED) {
	// Already on the unlock screen — nothing to do. Avoids clobbering
	// an existing `state.from` set by an earlier reroute.
	if (currentLocation.pathname === ROUTES.unlockAccount) return undefined;
	dispatch(lockAccount());
	navigate(`${ROUTES.unlockAccount}${currentLocation.search}`, {
	state: { from: currentLocation },
	});
	return undefined;
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}