refactor(redux): replace injected selectors with getters - #30837
Open
peter-sanderson wants to merge 16 commits into
Open
refactor(redux): replace injected selectors with getters#30837peter-sanderson wants to merge 16 commits into
peter-sanderson wants to merge 16 commits into
Conversation
peter-sanderson
added
the
no-project
peter-sanderson
marked this pull request as ready for review
August 4, 2026 20:15
36 tasks
peter-sanderson
force-pushed
the
refactor/extra-selector-getters
branch
from
August 5, 2026 07:20
3cba561 to
df7a373
Compare
Getter services created by `toGetter` now carry the wrapped selector under a `selector` property, so the same injected dependency can be consumed in React components via `useSelector(getSomething.selector)`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Getter services created by `toGetter` now carry the wrapped selector under a `selector` property, so the same injected dependency can be consumed in React components via `useSelector(getSomething.selector)`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
✅ Previously successful run of [Test] suite-native Android E2E workflow has been found. |
`Getter.selector` wrapped the original selector, so a component using it stayed coupled to the state shape — its tests had to build store state instead of just mocking the getter. The selector now ignores the state it is handed and reads through the getter's own `getState`; the state argument exists only so `useSelector` re-evaluates on every store change. `Getter` therefore no longer needs its state type parameter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A getter called during render is read once, so the component never re-renders when the value changes — an easy mistake, since getters look like ordinary services. - `Getter` carries a type-only brand, `asGetter` puts it on getters not built from a selector (test mocks, constants). - `useGetter(selectGetterDep)` takes the same dependency selector as `useServices`, holding a single getter, and returns its current value, subscribed through `useSelector`. The store state never reaches the getter, it reads through its own `getState`, so a component and its test stay independent of the state shape and only have to mock the getter. - `useServices` refuses to hand out branded getters: its result type resolves to an explanation, so the call site fails on the first property access. `ExperimentalFeatureConfig.onToggle` declared a `SuiteServices` field no feature ever read, so the whole bag is no longer threaded into it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`useServices` refuses to hand out branded getters, but a dependency bag can legitimately be taken as a whole and only used imperatively — `ExperimentalFeatureConfig.onToggle` receives all `SuiteServices` and is called from an event handler, nothing is read during render. `useImperativeServices` selects such dependencies without the getter check, which keeps the experimental features API unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
peter-sanderson
commented
Aug 5, 2026
peter-sanderson
commented
Aug 5, 2026
peter-sanderson
commented
Aug 5, 2026
peter-sanderson
commented
Aug 5, 2026
Reading one getter never needed params: dropping them collapses the hook's types to one conditional and makes a second argument a plain arity error. Review feedback: use typedObjectValues over a cast Object.values, and keep the type-only rejections in the type-test rather than dressing them up as runtime tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Getters can take params, so useGetter takes them after the dependency selector and passes them on: `useGetter(selectGetIsSelectedWalletDep, descriptor)`. The subscription closes over them, so re-renders stay as precise as with a plain selector. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The params are only picked up because the subscription selector is rebuilt every render. Prove it: the value follows a changed param, and the new param is what gets watched afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
peter-sanderson
commented
Aug 5, 2026
peter-sanderson
commented
Aug 5, 2026
| export const selectEffectiveFirmwareChannel = | ||
| (allowPrerelease: boolean) => | ||
| (state: RootState): FirmwareChannel => | ||
| // When a user is in the Early Access Program, the firmware channel is forced to `production-early-access`. |
Contributor
Author
There was a problem hiding this comment.
preserve a original comment
Getters and plain services are consumed differently — a getter holds a value that changes over time and must be subscribed to, a service is just called — so a dependency lumping both together cannot be consumed correctly as a whole. Point at selecting the individual services instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment still described the removed `.selector` property. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
peter-sanderson
commented
Aug 5, 2026
peter-sanderson
commented
Aug 5, 2026
peter-sanderson
commented
Aug 5, 2026
peter-sanderson
commented
Aug 5, 2026
Review feedback: keep general type helpers in one place. `UnionToIntersection` already lives there, so useServices takes both from the package instead of declaring its own — the local copy would conflict on the next rebase anyway. Also restores the original comment on the firmware channel selectors, which the value parameter change had reworded and moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Redux selectors injected through ExtraDependenciesStatic make suite-common thunks depend on an unknown state shape supplied by the upper application store. This removes the selectors group, creates zero-argument services with toGetter in the desktop and native composition roots, and updates all consumers, mocks, affected tests, and documentation. The remaining any-based SuiteCompatibleSelector alias is also removed; firmware selector factories and authenticity hooks now preserve the platform selector state generically.
🌐 Preview deployments
🌐 Suite Web preview: https://dev.suite.sldev.cz/suite-web/refactor/extra-selector-getters/web/
🔍 Currents Test Results
🔍 Suite web test results: View in Currents
🔍 Suite desktop test results: View in Currents
🔍 Suite native android test results: View in Currents
🔒 Quarantined E2E Tests
Trezor Suite (desktop) — 2 test(s)
Updated: 2026-08-05T09:48:37.695Z • 2 test(s) total
Trezor Suite (web) — 3 test(s)
Updated: 2026-08-05T09:47:33.065Z • 3 test(s) total
🤖 LLM Test Recommendations
Summary: The change set is a cross-package refactor of dependency-injection/extra-dependencies wiring with concentrated changes in firmware, TrezorConnect init, trading initialization, and app startup. Risk is broad but shallow: the highest-impact regressions would appear in firmware flows, Connect popup lifecycle, trading initial data loading, and onboarding/app startup. The recommended set focuses on these areas with a small number of representative end-to-end tests rather than the full static-mapping list.
Changed files (30)
packages/suite/src/actions/suite/initAction.test.tspackages/suite/src/components/suite/Preloader/Preloader.tsxpackages/suite/src/support/extraDependencies.tspackages/suite/src/views/settings/SettingsDebug/FirmwareUpdateEnvironmentSelect.tsxsuite-common/connect-init/src/connectInitThunks.tssuite-common/dependency-injection/src/index.tssuite-common/dependency-injection/src/toGetter.test.tssuite-common/dependency-injection/src/toGetter.tssuite-common/firmware-authenticity/package.jsonsuite-common/firmware-authenticity/src/useReportDeviceCompromised.tssuite-common/firmware-authenticity/tsconfig.jsonsuite-common/firmware/src/firmwareReducer.tssuite-common/firmware/src/firmwareThunks.tssuite-common/firmware/src/getBinFilesBaseUrlThunk.tssuite-common/redux-utils/src/extraDependenciesType.tssuite-common/redux-utils/src/notImplemented.tssuite-common/redux-utils/src/types.tssuite-common/suite-types/package.jsonsuite-common/suite-types/src/firmware.tssuite-common/suite-types/src/index.tssuite-common/suite-types/tsconfig.jsonsuite-common/test-utils/src/extraDependenciesCommonMock.tssuite-common/token-definitions/src/tokenDefinitionsThunks.tssuite-common/trading/src/thunks/common/loadInitialDataThunk.test.tssuite-common/trading/src/thunks/common/loadInitialDataThunk.tssuite-common/wallet-core/src/accounts/accountsThunks.tssuite-common/wallet-core/src/blockchain/blockchainThunks.tssuite-common/wallet-core/src/fiat-rates/fiatRatesThunks.tssuite-native/app/src/hooks/useGlobalHooks.tsxsuite-native/state/src/extraDependencies.tsRecommended tests (10)
🔴 High priority (6)
suite/e2e/tests/firmware/custom-firmware.test.ts— Directly exercises custom firmware installation, which is served by the changed firmware thunks/reducer and the debug firmware-update-environment selector. A regression in firmware binary URL resolution or thunk wiring would surface here.suite/e2e/tests/onboarding/authenticity-check.test.ts— Runs the device authenticity path during onboarding, which is affected by firmware-authenticity reporting and firmware thunk changes. It asserts discovery finishes after the authenticity flow, validating end-to-end wiring.suite/e2e/tests/onboarding/t2t1/t2t1-entropy-check-failure.test.ts— Specifically asserts the 'device compromised' modal behavior governed by useReportDeviceCompromised and entropy-check handling. This is the most direct user-facing coverage for the firmware-authenticity change.suite/e2e/tests/suite/initial-run.test.ts— Verifies onboarding/initial-run state persistence and app loading across reloads, exercising the Preloader component and the suite init/extra-dependencies wiring that underlies all other flows.suite/e2e/tests/trading/buy-bitcoin.test.ts— Exercises the full trading buy flow, which depends on loadInitialDataThunk and the wallet-core account/blockchain/fiat-rates thunks. A regression in trading initial-data loading or thunk dependencies would be visible in the quotes/confirmation payloads.suite/e2e/tests/trezor-connect/connectPopupWeb.test.ts— Directly covers TrezorConnect popup initialization and permission flows, which rely on connectInitThunks. Changes to connect init wiring or extra dependencies would break the popup lifecycle assertions in this test.🟡 Medium priority (4)
suite/e2e/tests/browser/safari.test.ts— Validates the unsupported-browser gate and subsequent Suite load, which passes through the Preloader/initialization path. Useful as a lightweight smoke test that the app still loads correctly after Preloader changes.suite/e2e/tests/onboarding/t3t1/t3t1-create-wallet-offline.test.ts— Covers offline onboarding including firmware revision checks and the no-connection banner. This validates firmware thunk behavior when firmware metadata cannot be fetched and the Preloader/offline state is active.suite/e2e/tests/trading/swap-coin-to-token.test.ts— Exercises a cross-chain token swap, which depends on trading loadInitialDataThunk, token definitions, and wallet-core thunks. Provides broader trading coverage beyond the buy flow.suite/e2e/tests/wallet/discovery.test.ts— Activates many coins and verifies discovery completes after reload, exercising the wallet-core accounts/blockchain/fiat-rates thunks and persistence logic. Good representative coverage for the broad wallet-core changes.suite-common/redux-utils/src/extraDependenciesType.tssuite-common/redux-utils/src/notImplemented.tssuite-common/redux-utils/src/types.tssuite-common/suite-types/src/firmware.tssuite-common/suite-types/src/index.tssuite-native/app/src/hooks/useGlobalHooks.tsxsuite-native/state/src/extraDependencies.tsUpdated: 2026-08-05T09:48:59.227Z