Show all operation fields in the transaction signing details view#2829
Show all operation fields in the transaction signing details view#2829piyalbasu wants to merge 7 commits into
Conversation
The signing-approval Details view dropped several operation fields whose values are falsy but meaningful, and rendered combined account-flag bitmasks as blank. Use presence checks instead of truthy guards, decode flag bitmasks bit-by-bit, surface data-entry deletions and home-domain clears, and add a warning when a setOptions op disables the master key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
piyalbasu
force-pushed
the
fix/sign-tx-show-all-operation-fields
branch
from
fe54f8a to
85bae01
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe54f8a4cd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR improves the transaction signing “Details” screen so that operation fields aren’t silently omitted when their values are “falsy” (e.g., 0, "", false), ensuring the UI reflects what will actually be signed. It also introduces an idle auto-lock system (configurable timeout + background alarm + UI activity pings + lock/unlock broadcasts) and wires it into popup routing and state.
Changes:
- Fix operation rendering to use presence checks (vs truthiness), decode flag bitmasks, and explicitly render “clearing/deleting/disabled” states; add a master-key disable warning and comprehensive renderer tests.
- Add configurable idle auto-lock timeout presets and settings plumbing, plus a new Auto-lock timer settings screen.
- Implement cross-surface session lock/unlock broadcast handling (background ↔ UI), activity-based timer resets, and expanded test coverage across popup and background.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| extension/src/popup/views/Wallets/hooks/tests/useGetWalletsData.test.tsx | Preloaded auth updates for hasPrivateKey. |
| extension/src/popup/views/UnlockAccount/index.tsx | Navigate on hasPrivateKey transition after unlock. |
| extension/src/popup/views/Security/index.tsx | Add nav entry for Auto-lock timer. |
| extension/src/popup/views/Preferences/index.tsx | Persist autoLockTimeoutMinutes when saving settings. |
| extension/src/popup/views/IntegrationTest.tsx | Include auto-lock timeout in settings payload. |
| extension/src/popup/views/AutoLockTimer/styles.scss | Styles for auto-lock preset list. |
| extension/src/popup/views/AutoLockTimer/index.tsx | New auto-lock timeout selection view. |
| extension/src/popup/views/tests/Swap.test.tsx | Mock settings include auto-lock timeout. |
| extension/src/popup/views/tests/SignTransaction.test.tsx | Mock settings include auto-lock timeout. |
| extension/src/popup/views/tests/ManageAssets.test.tsx | Mock settings include auto-lock timeout. |
| extension/src/popup/views/tests/GrantAccess.test.tsx | Add hasPrivateKey + auto-lock timeout in mocks. |
| extension/src/popup/views/tests/AddFunds.test.tsx | Add hasPrivateKey + auto-lock timeout in mocks. |
| extension/src/popup/views/tests/AccountHistory.test.tsx | Mock settings include auto-lock timeout. |
| extension/src/popup/views/tests/AccountCreator.test.tsx | Mock settings include auto-lock timeout. |
| extension/src/popup/views/tests/Account.test.tsx | Mock settings include auto-lock timeout. |
| extension/src/popup/Router.tsx | Register SessionLockListener; add AutoLockTimer route. |
| extension/src/popup/metrics/views.ts | Emit metric for AutoLockTimer view. |
| extension/src/popup/locales/pt/translation.json | Add new strings for operation rendering + auto-lock. |
| extension/src/popup/locales/en/translation.json | Add new strings for operation rendering + auto-lock. |
| extension/src/popup/helpers/hooks/useActivityPing.ts | New hook sending throttled USER_ACTIVITY pings. |
| extension/src/popup/helpers/hooks/tests/useActivityPing.test.ts | Tests for ping throttling + listener cleanup. |
| extension/src/popup/ducks/settings.ts | Add autoLockTimeoutMinutes to settings state/thunks/selectors. |
| extension/src/popup/ducks/accountServices.ts | Add lockAccount reducer for immediate UI lock state. |
| extension/src/popup/ducks/tests/settings.test.ts | Ensure saveSettings doesn’t mutate auth state. |
| extension/src/popup/ducks/tests/accountServices.test.ts | Test lockAccount clears key-derived state. |
| extension/src/popup/constants/routes.ts | Add /settings/security/auto-lock-timer route. |
| extension/src/popup/constants/metricsNames.ts | Add metric name for AutoLockTimer. |
| extension/src/popup/components/signTransaction/Operations/index.tsx | Render all op fields; bitmask decoding; warnings; explicit Enabled/Disabled. |
| extension/src/popup/components/signTransaction/Operations/tests/index.test.tsx | New renderer regression tests for falsy fields. |
| extension/src/popup/components/SessionLockListener/index.tsx | Listen for SESSION_LOCKED/UNLOCKED broadcasts; reroute + refresh auth. |
| extension/src/popup/components/ActivityTracker/index.tsx | Redux-aware wrapper enabling activity ping when unlocked. |
| extension/src/popup/components/tests/SessionLockListener.test.tsx | Tests for broadcast handling + navigation semantics. |
| extension/src/popup/components/tests/SearchAsset.test.tsx | Preloaded auth updates for hasPrivateKey. |
| extension/src/popup/App.tsx | Mount ActivityTracker globally under Provider. |
| extension/src/helpers/hooks/useGetAppData.tsx | Cache path now requires hasPrivateKey; reroute when locked. |
| extension/src/helpers/tests/useGetAssetDomainsWithBalances.test.tsx | Preloaded auth updates for hasPrivateKey. |
| extension/src/helpers/tests/useGetAppData.test.tsx | New regression tests for cache fast-path lock behavior. |
| extension/src/constants/localStorageTypes.ts | Add AUTO_LOCK_TIMEOUT_MINUTES_ID storage key. |
| extension/src/background/store.ts | Add flushSessionStore() and subscribe using it. |
| extension/src/background/messageListener/popupMessageListener.ts | Add USER_ACTIVITY handler; refine extension-origin sender checks. |
| extension/src/background/messageListener/helpers/test-helpers.ts | Mock session timer reads timeout from mock storage; track resets/stops. |
| extension/src/background/messageListener/helpers/login-all-accounts.ts | Await timer; flush store; broadcast SESSION_UNLOCKED; rethrow on failures. |
| extension/src/background/messageListener/helpers/broadcast-session-state.ts | Helper to broadcast SESSION_LOCKED/UNLOCKED safely. |
| extension/src/background/messageListener/handlers/userActivity.ts | Handler to rearm idle alarm on verified unlocked session. |
| extension/src/background/messageListener/handlers/signOut.ts | Stop timer; lock HW session; flush; broadcast SESSION_LOCKED. |
| extension/src/background/messageListener/handlers/saveSettings.ts | Persist timeout; coerce value; rearm timer if unlocked. |
| extension/src/background/messageListener/handlers/recoverAccount.ts | Await startSession() after recover. |
| extension/src/background/messageListener/handlers/migrateAccounts.ts | Await startSession() during migration. |
| extension/src/background/messageListener/handlers/loadSettings.ts | Load/coerce auto-lock timeout from storage. |
| extension/src/background/messageListener/handlers/importHardwareWallet.ts | Arm idle timer on HW-only import. |
| extension/src/background/messageListener/handlers/handleSignedHwPayload.ts | Reset idle timer on successful HW signature. |
| extension/src/background/messageListener/handlers/createAccount.ts | Await startSession() after create. |
| extension/src/background/messageListener/handlers/confirmPassword.ts | Unlock HW state on password unlock. |
| extension/src/background/messageListener/tests/userActivity.test.ts | Tests for timer reset gating on unlocked state. |
| extension/src/background/messageListener/tests/signOut.test.ts | Tests for stop→flush→broadcast ordering; HW lock regression. |
| extension/src/background/messageListener/tests/sidebar.test.ts | Allow extension-origin tab senders; polyfill shim for tests. |
| extension/src/background/messageListener/tests/login-all-accounts.test.ts | Tests for flush-before-broadcast + rethrow regressions. |
| extension/src/background/messageListener/tests/loadSaveSettings.test.ts | Tests for timeout coercion + reschedule semantics. |
| extension/src/background/messageListener/tests/importHardwareWallet.test.ts | Test that HW import arms the idle timer. |
| extension/src/background/messageListener/tests/handleSignedHwPayload.test.ts | Test that HW signing resets idle timer; selectors transitions. |
| extension/src/background/messageListener/tests/createAccount.test.ts | Adjust alarm mock typing for async timer. |
| extension/src/background/index.ts | Instantiate SessionTimer with localStore; broadcast on alarm lock. |
| extension/src/background/helpers/session.ts | SessionTimer reads persisted timeout; clearSession locks HW + flushes. |
| extension/src/background/helpers/tests/session.test.ts | Tests for SessionTimer + hasPrivateKey selector behavior. |
| extension/src/background/ducks/session.ts | Track HW lock flag; strengthen unlocked selector with temp-store check. |
| extension/src/background/tests/initAlarmListener.test.ts | Test alarm triggers clearSession + SESSION_LOCKED broadcast. |
| @shared/constants/services.ts | Add USER_ACTIVITY + SESSION_LOCKED/UNLOCKED service types. |
| @shared/constants/autoLock.ts | Add shared timeout presets + coercion + label formatting. |
| @shared/api/types/types.ts | Add autoLockTimeoutMinutes and SaveSettingsResponse type. |
| @shared/api/types/message-request.ts | Add autoLockTimeoutMinutes to SaveSettings; add UserActivity message. |
| @shared/api/internal.ts | Pass auto-lock timeout through saveSettings; type sendMessage generics. |
| @shared/api/helpers/extensionMessaging.ts | Make sendMessageToBackground generic over response type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-e3f3ec166ae3d5cf37fb (SDF collaborators only — install instructions in the release description) |
Use t("Warning") for the IconButton accessibility text so non-English
locales don't get an English-only string. Addresses PR review feedback.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc632c8ee6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The warning was a KeyValueList row, so its text landed in the right-aligned value column with white-space:nowrap and was truncated. Render it as a dedicated full-width banner (alert icon + wrapping text, role="alert") so the full message is always readable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5a23995cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Manual verificationLoaded the built extension unpacked on Testnet and signed a transaction for each affected operation via the setOptions — zero weights + added signerSigner + weight, all thresholds
setOptions — master weight 0 onlyRenders the field and the warning instead of an empty operation.
setOptions — non-zero values (control)Non-zero master weight / threshold still render, and no warning is shown.
setOptions — combined setFlags (REVOCABLE | CLAWBACK)Decodes each bit instead of rendering blank.
setOptions — combined clearFlags (REQUIRED | REVOCABLE)
setOptions — clearing home domain
setTrustlineFlags — flag turned offShows
setTrustlineFlags — flag turned onShows
manageData — deleting an entry
manageData — empty valueValue row renders rather than being omitted.
|
The pt locale was showing the master-key warning, home-domain-clear and data-entry-delete placeholders, and the unknown-flags fallback verbatim in English. Provide pt-BR translations consistent with existing terms (Domínio Principal, Assinante, Peso Mestre). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61bec6a94f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@minkyeongshin what do you think about the values displayed in the cases where fields are cleared/deleted? The transaction details view is meant to be lower-level / technical, but I'm wondering if there is a better way to visually describe these cases. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebd78a5f3e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
decodeAuthorizationFlags returned the raw English labels from AuthorizationMapToDisplay; wrap them in t() and register the four flag labels in the en/pt locales so combined setFlags/clearFlags render translated in non-English locales. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
decodeAuthorizationFlags returned only known labels when any known bit
was present, hiding unrecognized bits combined with a known flag (e.g.
1 | 16). Track remaining bits and append an Unknown ({{bits}}) entry for
them so no flag change is silently dropped.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TL;DR
When you approve a transaction in Freighter, the Details screen is supposed to show you exactly what you're signing. For a few operation types it was quietly leaving out fields whose value happened to be "empty-looking" — a zero, an empty value, a flag being turned off — even though those changes are real and important. This PR makes the approval screen show every field that's actually being set, so what you see matches what you sign.
What was getting dropped before vs. now shown:
0(clearing home domain)Enabled/Disabledshown explicitly(deleting entry)Implementation overview
All changes are in the dapp signing-approval operation renderer and its new test file:
extension/src/popup/components/signTransaction/Operations/index.tsxextension/src/popup/components/signTransaction/Operations/__tests__/index.test.tsxRoot cause: rows were gated with JS truthiness guards (
{field && <KeyValueList .../>}). The Stellar SDK decodes a present-but-zeroUint32as the number0and a present-but-empty value as a falsy/empty value, so these guards dropped legitimately-present fields. Account flags were additionally rendered via a single-key map lookup (AuthorizationMapToDisplay[bits.toString()]) that only had keys for single bits, so any combined bitmask resolved toundefined→ blank.Changes:
Presence checks over truthiness. In the
setOptionscase,highThreshold/medThreshold/lowThreshold/masterWeightguards changed from{x && …}to{x !== undefined && …}. Optional chaining on.toString()was removed because the guard now guarantees the value is defined. Verified against the bundled SDK that absent optional fields decode toundefined(notnull), so the direct.toString()is safe.Bitmask flag decoding. Added
decodeAuthorizationFlags(bits)which iteratesAuthorizationMapToDisplayand bitwise-ANDs each bit, joining matched labels with", ", falling back tot("Unknown ({{bits}})", { bits }). Applied to bothsetFlagsandclearFlags(which also moved to!== undefinedguards). Thresholds/masterWeightare plainUint32and are NOT routed through this decoder.Master-key warning. Added a
MasterKeyDisableWarningcomponent (mirrors the existingMemoRequiredWarningidiom:KeyValueList+IconButton variant="error"), rendered whenmasterWeight === 0.setTrustLineFlags. The SDK decodes this op into aflagsobject where a key is present (trueto enable,falseto clear) only for flags being changed. Guards changed toflags.<x> !== undefinedso a flag being turned off is no longer hidden, and the value is rendered ast("Enabled")/t("Disabled")— previously the raw boolean was passed as the value, which React does not render.manageData.valuedecodes to aBufferwhen set (truthy even when empty) andundefinedwhen the entry is being deleted. The Value row now always renders; deletion showst("(deleting entry)").setOptionshome domain.homeDomainclearing decodes to""(vsundefinedwhen unchanged); guard moved to!== undefinedwith""rendered ast("(clearing home domain)").Testing: new
__tests__/index.test.tsxbuilds each operation with the bundled SDK, round-trips through XDR (the exact decode path the signing flow uses), renders the realOperationscomponent under a ReduxProvider, and asserts on the rendered rows. Keys are minted withStrKey.encodeEd25519PublicKey(Buffer.alloc(...))to avoid the crypto RNG, which is unavailable in the jsdom test env. 14 tests, all passing; lint clean. No other operation cases inRenderOpByTypeuse truthy guards on meaningful numeric/value fields (offers, amounts, etc. render unconditionally).