fix(widget): respect tool allow-list on first route request#791
fix(widget): respect tool allow-list on first route request#791tomiiide wants to merge 2 commits into
Conversation
Seed enabled tools from the config allow-list at store creation so the first route request, before /tools loads, no longer drops the filter and returns disabled tools.
🦋 Changeset detectedLatest commit: fe485f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
E2E Examples — all passedAll examples passed in the latest run. |
E2E Playground resultsDetails
📥 Download full HTML report (open the run → Artifacts → |
🔍 QA Review — EMB-450 · PR #791Ticket: Stale localStorage allow-list returns disabled exchange routes 🔁 Re-Review — Resolution of Previous Change RequestsThree items were flagged in the initial review (2026-06-15T12:40). Developer ( ✅ Resolution Status
Additional notes from prior review — also addressed:
🔍 Re-Review Code AnalysisItem 1 — zero-intersection fix (code verified): The prior The new merge: const allowMap = toEnabledMap(configItemsByToolType[toolType]?.allow)
Object.assign(state, buildToolState(toolType, { ...allowMap, ...persistedEnabled }))Spreading No regressions from the new merge logic:
Items 2 & 3 are straightforward additions (test + comment). No logic changes. ✅ 🚦 Verdict: Pass ✅All three flagged items are resolved — two with both code changes and new tests, one with an inline documentation comment. The additional test-hygiene notes (version comment, Ticket coverage: Complete — the explicit AC is met, the known limitation is documented in code, and the key edge cases are covered by unit tests. Reviewed by lifi-qa-agent · EMB-450 · 2026-06-15 |
There was a problem hiding this comment.
Requesting changes on 3 items — each requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🟠 Medium | Test gap | packages/widget/src/stores/settings/createSettingsStore.test.ts — zero-intersection rehydration |
| 2 | 🟠 Medium | Test gap | packages/widget/src/stores/settings/createSettingsStore.test.ts — stale bridges merge path |
| 3 | 🟢 Low | Documentation | Deny-only known limitation undocumented in code |
1. [Medium] Test gap — createSettingsStore.test.ts — zero-intersection rehydration
The stale-rehydration test (Test 2) seeds _enabledExchanges: { fly: true, okx: true, kyber: true } — which deliberately includes the currently-allowed tool (fly). There is no test for the case where the persisted map has zero intersection with the current allow-list (e.g. returning user who was using okx before the integrator tightened to allow: ['fly']).
In this case buildToolState correctly filters out all keys, producing enabledExchanges = []. In useRoutes, allowedExchanges = enabledExchanges = [] — an empty array sent to the SDK. This is the strictest-correct interpretation of the AC ("no disabled tools allowed through"), but creates a transient "no routes found" state until /tools resolves. Without a test, this behaviour is invisible and any future refactor of buildToolState could silently change it.
- Missing: test with
seedPersistedState({ _enabledExchanges: { okx: true, kyber: true } })+config: { exchanges: { allow: ['fly'] } }assertingenabledExchanges = []and_enabledExchanges = {}
2. [Medium] Test gap — createSettingsStore.test.ts — stale bridges merge path
SettingsToolTypes = ['Bridges', 'Exchanges'] — the merge function loops over both. Test 2 only seeds _enabledExchanges; there is no test for the bridges merge path. The code paths are symmetric, but an uncovered path is a silent regression target.
- Missing: test with
seedPersistedState({ _enabledBridges: { across: true, hop: true, stargate: true } })+config: { bridges: { allow: ['across'] } }assertingenabledBridges = ['across']and_enabledBridges = { across: true }
3. [Low] Deny-only known limitation undocumented in code
The PR description clearly states: "Known limitation: deny-only / no-allow configs cannot be pre-seeded synchronously." However, this is not reflected in the source code itself. A reader of createSettingsStore.ts seeing the initialSettings seeding block has no indication that deny-only configs are intentionally not seeded, or what the first-request behaviour will be (allowedExchanges = [] until /tools resolves — stricter than before the fix).
Add a brief inline comment near the buildToolState calls in initialSettings (e.g. // deny-only configs cannot be seeded synchronously — the full tool universe is unknown until /tools resolves) so future maintainers understand the constraint without reading the PR description.
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
Overlay persisted toggles on the config allow seed in merge so a returning user whose persisted tools don't intersect the new allow-list still sends it, not an unrestricted request. Adds bridges and zero-intersection tests; documents the deny-only limitation.
|
Addressed in fe485f7. 1. Zero-intersection rehydration (Medium) — Fixed in code, not just tested. The empty-array case is actually unsafe: 2. Stale bridges merge path (Medium) — Added a test seeding 3. Deny-only limitation (Low) — Inline comment added next to Also from the notes: |
There was a problem hiding this comment.
QA re-review complete — all 3 change requests resolved in fe485f7. Zero-intersection fix verified correct, bridges test added, deny-only comment added. ✅ Pass.
vinzenzLIFI
left a comment
There was a problem hiding this comment.
Tested this by programatically pre-loading the browser with a saved session containing a broad list of allowed exchanges while the app was configured to only allow fly. Before the fix the first swap route request sent all pre-loaded exchanges to the API (disabled exchanges could appear in results. After the fix the first request correctly sends only the configured exchange regardless of what was saved in the browser.
Which Linear task is linked to this PR?
https://linear.app/lifi-linear/issue/EMB-450/stale-localstorage-allow-list-returns-disabled-exchange-routes
Why was it implemented this way?
The integrator allow-list (
exchanges.allow/bridges.allow) is enforced only through theenabledExchanges/enabledBridgesstore arrays, which are populated asynchronously by the/toolsfetch. Before that resolves the array is empty (cleared or fresh storage) or stale, anduseRoutesthen either omits theexchanges/bridgesfilter entirely (an empty array reads as "no restriction") or sends the stale list. So the first/advanced/routesrequest escapes the allow-list and can return disabled tools (OKX, Kyber, etc.); it self-corrects once/toolsresolves, which is why only the first request was affected.Fix:
enabled*from the config allow-list synchronously at store creation (covers empty/cleared localStorage).merge(covers a stale persisted list).This makes the store reflect the allow-list from the first render with no dependency on
/toolstiming. AuseRoutes-only fallback was considered but rejected: it scatters the config logic into a second place and leaves the persisted store and settings UI inconsistent.Known limitation: deny-only / no-allow configs cannot be pre-seeded synchronously (the full tool universe is unknown until
/tools). The reporting integrator usesallow, so this is covered.Visual showcase (Screenshots or Videos)
N/A (request-payload behavior). Repro: with
exchanges: { allow: ['fly'] }, clear localStorage and block/toolsin DevTools, then trigger a quote. The firstPOST /advanced/routesbody omitsoptions.exchangesbefore the fix and sends{ allow: ['fly'] }after.Checklist before requesting a review