fix(frontend): Charts pair select must stick on first change #1271
No reviewers
Labels
No labels
agent:fix_bugfix
agent:fix_conflicts
agent:fix_security
agent:gap_analysis
agent:implement
agent:implement
agent:implement
agent:open_issues
agent:ready
agent:research
agent:security_audit
agent:verify
architecture
backend
blocker:hybrid
blocker:launch
blocker:limit-orders
blocker:v2
block:log_only
block:security
bug
ci
contracts
correctness
deploy
dev
devops
docs
documentation
duplicate
e2e
enhancement
epic
feature
frontend
functional-completion
gas
good first issue
governance
help wanted
high-risk
hooks
hybrid
indexer
infra
infrastructure
integrators
invalid
launch-blocker
limit-orders
localnet
localterra
low priority
missing-implementation
needs-design
ops
performance
priority
high
priority
medium
product
qa
QA
question
ready
ready
research
scripts
security
security-hardening
smartcontracts
tech-debt
testing
ux
UX
v2
verification
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
code/cl8y-dex-terraclassic!1271
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue/1266"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
/chartshero auto-pick no longer overwrites the first Select Pair change to a listed non-hero market.getPairand does not replace a valid bech32 or live pair route.#chart-pair-select(no remount-only pair switch).make verify-issue-1266covers C1266 plus the C680 subset.Fixes #1266
Test plan
make verify-issue-1266(ChartsPage MenuSelect RTL T1–T5 + idle-hero guards + docs/skill grep + chrome nesting)make verify-issue-680(C680-1–C680-8 stay green)make dev→/charts→ Select Pair once to a non-hero listed market → URL and chart match without a second pick; deep-link/charts/{B}still loads B; idle/chartsstill opens UST1/cUSTCdrain skip: no occupying job for rebase/fix-pr/CI-wait; remaining continue
/agent review
cl8y-agent-control: queued
bug_reviewjob473436fe-bff0-49c7-a8a7-b1da4bc105b4(not executed; no Hetzner VM).RECOMMEND: ACCEPT
Independent MR review (gate VM). Frozen SHA: unset. Head:
487316cc(fix(frontend): keep first Charts pair select against idle hero). Woodpecker CI on PR: Success.Scope
Frontend-only routing fix for #1266. No smart contracts, indexer/Rust, Postgres, wallet auth, or oracle ingest changes. Security/economic attack surface is limited to Charts URL/query handling and client-side pair selection (read-only charting).
Root cause & fix (correctness)
Severity: High (product bug), fixed
The idle #680 hero
useEffecttreatedselectedPairAddr !== heroon still-bare/chartsas “force hero,” racingselectPair’snavigate(/charts/{B}). The fix mirrors Trade #357:shouldAutoPickChartsHeroPair— hero replace-nav only when bare route, pairs loaded, noselectedPairAddr, nouserCommittedPairRef(chartsPairRoute.ts:130-143,ChartsPage.tsx:233-259).shouldSnapChartsSelectionToCatalogHead— catalog head cannot replace a valid bech32 or pendinggetPair(chartsPairRoute.ts:151-167,ChartsPage.tsx:209-231).selectPairearly-returns non-bech32 (ChartsPage.tsx:140-141) and setsuserCommittedPairRefbefore state/navigate.Removed anti-pattern
if (!hero || selectedPairAddr === hero) returnthat caused the clobber whenselectedPairAddr === B.Repro (pre-fix):
make dev→/charts→ Select Pair once to non-hero B → URL/workspace reverted to hero. Post-fix: RTL AC1/T1 (ChartsPage.test.tsx~1126) clicks#chart-pair-selectwithout remount.Issue #1266 acceptance criteria
C1266-1docsshouldAutoPickChartsHeroPairunit tests/charts/{B}?price=dropverify-issue-1266runs fullChartsPage.test.tsx(includes C680 block). Note:verify-issue-1266.shdoes not chainmake verify-issue-680(extra util files in #680 script only) — acceptable if CI/main runs both targets separately.Functional test plan gaps (low):
MenuSelect) — not added; issue marked Vitest T1–T5 only.make devper issue verification criteria.Security audit (in-repo vectors)
javascript:inonChangeisChartsPairRouteParam(addr)inselectPair(ChartsPage.tsx:141);chartsPairHrefnull → no navigate. No new RTL for maliciousonChangestring (existingchartsPairRoutetests).chartsPairHref+replace: true./charts/<script>?price=/ open redirectparseChartsPriceQuery/looksHostileChartsPrice; AC5 asserts drop on pair change.filterRetailDiscoveryIndexerPairs; no selector bypass introduced.selectedPairAddrset blocks re-entry.dangerouslySetInnerHTMLin diff.Out of scope (no diff): SQL injection, privilege escalation, signing keys, wasm, MEV, oracle manipulation, tokenomic attacks, DB leaks, Rust server ACLs.
DeFi / smart contract / database / oracle
Not applicable to this MR. No on-chain execution, liquidity, or price feed logic changed. Charts continues to consume indexer APIs; routing fix does not alter quote math, TWAP, or hub USD marks.
Test coverage & verify
chartsPairRouteguard unit tests (chartsPairRoute.test.ts); 7 RTL cases in#1266block;make verify-issue-1266+ skill/docs (AGENTS_FRONTEND_CHARTS_PAIR_SELECT.md).make verify-issue-1266— grep/docs/chrome PASS; Vitest step FAIL (review VM missing nvm/Node; not a code signal). PR CI green is the authoritative test run for this gate.Gap analysis (non-blocking)
chartsPairRoute.ts— testable, parallelsshouldAutoPickDefaultTradePair(tradePairRoute.ts:84).userCommittedPairRefChartsPage.tsx:121-123); acceptable becauseselectedPairAddralso gates hero; edge case: back to bare/chartswith stale state is pre-existing SPA behavior.grepnegation onlocalStorage.setIteminChartsPage.tsx(currently none) — fine until unrelated storage added.Packages touched
frontend-dapp(ChartsPage + chartsPairRoute), docs/skills/Makefile/AGENTS only — noindexer/,smartcontracts/, or infra.Recommendation
Merge after controller autoland (do not merge
mainfrom this review). Delivers #1266 with aligned Trade #357 pattern, documented C1266 invariants, and CI-green scope. Optional follow-ups: RTL for T6 keyboard; chainmake verify-issue-680insideverify-issue-1266.shfor AC6 parity; complete manual QA checkbox on PR./agent review
cl8y-agent-control: queued
bug_reviewjob7cccd6d6-29c3-4eaf-9d71-c5b6e1a7558d(not executed; no Hetzner VM).RECOMMEND: ACCEPT
Reviewed commit:
487316cc(PR head at review time)Scope: Frontend-only Charts routing (
ChartsPage.tsx,chartsPairRoute.ts, tests/docs/QA). No indexer, Postgres, Rust API, wasm, wallet, or Coolify changes.Issue #1266 closure check
/chartssticks to BChartsPage.test.tsxAC1/T1 (~1120–1131): MenuSelect click, pathname/charts/{OTHER}, candles/stats/leaderboard for B/charts→ hero (C680-1)~1133–1138)/charts/{B}unchanged (C680-5)~1140–1145)~1156–1178)?price=dropped when not leg of B~1180–1187)verify-issue-680, RTL race, chrome nestingmake verify-issue-1266+make verify-issue-680green on review VM;check_chrome_nesting.pyin verify scriptIssue attack table A1–A10: unchanged surface; A1/A4/A6 still covered by
chartsPairRoute.test.ts+selectPairisChartsPairRouteParamguard (ChartsPage.tsx:140–141). A8 addressed byuserCommittedPairRef+shouldAutoPickChartsHeroPair(chartsPairRoute.ts:130–144,ChartsPage.tsx:233–268).PR test plan gap (non-blocking): Manual
make devstep remains unchecked in PR body; T6 (keyboard listbox) not duplicated in Vitest (issue marked optional). Recommend human smoke once before merge.Root cause & fix (correctness)
The race matched #1266: on bare
/charts, hero effect treatedselectedPairAddr !== heroas “force hero” whilenavigate(/charts/B)was still in flight.Fix aligns with Trade #357 pattern:
shouldAutoPickChartsHeroPair— hero only when bare, list ready, noselectedPairAddr, nouserCommittedPair(chartsPairRoute.ts:137–144).selectPairsetsuserCommittedPairRef.current = truebefore state/nav (ChartsPage.tsx:140–151).shouldSnapChartsSelectionToCatalogHead— no catalog-head snap for valid bech32 or pendinggetPair(chartsPairRoute.ts:151–167, effectChartsPage.tsx:209–231).Pure helpers + unit tests are a good DRY/readability win vs inline effect conditions.
Security audit (in-repo attack models)
isChartsPairRouteParambeforegetPairStats/ candles (invalid route notice path unchanged).chartsPairHref+replace: true. Hostile?price=still stripped (parseChartsPriceQuery/matchChartsPriceParam).filterRetailDiscoveryIndexerPairs).Severity: No BLOCK-class findings.
Testing (executed on review VM)
Woodpecker PR check reported Success via
fj pr status 1271.Coverage gaps (low):
MenuSelectpath not asserted (behavior should match T1 via sharedonChange).~1189–1208); no dedicated act-spam / infinite-navigate assertion (A7) — mitigated byreplace+ idle hero guard tests.userCommittedPairRefis never cleared on remount-only navigation back to bare/chartswithin one SPA session; if product ever adds “reset to hero on nav to/charts”, would need ref reset — out of scope, same class as Trade not persisting last pair.Gap analysis (non-security)
getPairunchanged.frontend-dapponly; Makefile/AGENTS/docs/skills updated consistently.tradePairRoute.shouldAutoPickDefaultTradePair; ref + emptyselectedPairAddrguard is idiomatic React.Minor observations (info / low)
userCommittedPairRef(intentional ref read); safe becauseselectedPairAddrand navigation side effects cover re-runs (ChartsPage.tsx:260–268).shouldSnapChartsSelectionToCatalogHeadreturnstrueonly for invalid bech32selectedPairAddroff-catalog (chartsPairRoute.ts:165–166); valid user bech32 never snaps — matches C1266-6.Verdict
Targeted fix for #1266 with strong RTL coverage, documented invariants (C1266-1–8), regression hooks, and no expansion of security-sensitive surface. RECOMMEND: ACCEPT after optional manual
/chartssmoke (PR checklist item).Independent gate review; does not replace CI or frozen-SHA controller merge.
fix-pr verification (Grok high). PR already merged; no further code.
Head:
025d05dd(issue/1266, ancestor ofmainvia merge6f19d582). Issue #1266 is closed. Dual independent reviews already said RECOMMEND: ACCEPT. This pass did not open a siblingissue/N-*PR and did not mergemain.Product fix (already on
main)Idle
#680hero treatedselectedPairAddr !== heroon still-bare/chartsas force-hero, racing the first#chart-pair-selectnavigate(/charts/{B}). The landed change:shouldAutoPickChartsHeroPair— hero only when bare, list ready, empty selection, nouserCommittedPairselectPairsetsuserCommittedPairRefbefore state/nav; non-bech32 is a no-opshouldSnapChartsSelectionToCatalogHeadnever replaces a valid bech32 or pendinggetPairInvariants C1266-1–C1266-8 are in
docs/frontend.md§ Charts Select Pair,docs/indexer-invariants.md,docs/testing.md,AGENTS.md, andskills/AGENTS_FRONTEND_CHARTS_PAIR_SELECT.md(cross-linked from the UST1 hero + pair-scoped skills).Review items
Completed / confirmed already done:
?price=drop)chartsPairRoute.test.ts+selectPairbech32 gate)selectPair”check_chrome_nesting.py)Not done (review-labeled optional / non-blocking; PR already merged):
MenuSelect(onSelectIndexalready sharesonChangewith click)make verify-issue-680insideverify-issue-1266.sh(AC6 still met: both targets green on this VM)userCommittedPairRefon bare/chartsremount (App uses separate/chartsand/charts/:pairAddrroutes, so the page remounts; reviewers marked out of scope)make devsmoke — LocalTerra/Docker were down on this gate VM; did not runmake setup-cloud-localterrafor a merged frontend-only PR. RTL click path is the substitute.Independent verify (this VM, Node 24)
No working-tree changes; nothing to commit or push.
Human checklist (optional, PR body still unchecked)
make dev→/charts→ Select Pair once to a listed non-hero market → URL + candles + stats + leaderboard match without a second pick/charts/{B}still loads B (no hero snap)/chartsstill opens UST1/cUSTC +?price=UST1when listed