Order transactions by date#2810
Conversation
|
PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-72b7ef5553367b73d788 (SDF collaborators only — install instructions in the release description) |
There was a problem hiding this comment.
Pull request overview
This PR ensures account history is displayed in true reverse-chronological order by sorting all fetched operations by created_at on the client, rather than relying on backend grouping (e.g., failed vs successful) that can break global ordering.
Changes:
- Sort fetched account history operations by
created_atdescending before storing in state/cache. - Add an e2e regression test that interleaves failed and successful transactions out of order and asserts the UI re-sorts correctly (including within the asset detail view).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| extension/src/helpers/hooks/useGetHistory.tsx | Sorts fetched (or cached) history operations by created_at desc before dispatching/saving. |
| extension/e2e-tests/accountHistory.test.ts | Adds an e2e test to verify failed + successful transactions are ordered together by date, and updates stubs import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
piyalbasu
left a comment
There was a problem hiding this comment.
Three observations before merge — none blocking, the fix is fine to ship.
1. The sort is load-bearing for the section reducer in useGetHistoryData.tsx. That reducer creates a new monthYear bucket each time the month changes between adjacent items, so it requires sorted input — without this sort, history would render with duplicate non-contiguous month headers (e.g., two separate 'March 2025' sections). Worth a one-line comment on the sort here noting that downstream grouping depends on it, otherwise a future refactor that drops the sort produces a silent visual bug with no type/test failure.
2. Worth a follow-up at the backend. With Mercury disabled in prod, freighter-backend/src/helper/horizon-rpc.ts fetchAccountHistory is the only path serving real users — a thin passthrough of one Horizon call (order("desc").includeFailed(true)). If Horizon really is returning grouped data, a one-line sort there gives every consumer (extension, mobile, any future v2 reimplementation) the invariant without each client re-implementing it. The client sort here is still useful as defense-in-depth (and the section reducer needs it locally), but the canonical fix probably belongs upstream.
3. Inline notes on the rationale comment and the sort itself below.
| // Backends can return failed and successful operations in separate | ||
| // groupings, so sort by created_at desc to guarantee chronological order. |
There was a problem hiding this comment.
This comment plants a server-side cause that hasn't been reproduced against the actual prod path. With Mercury disabled in prod, /account-history is a thin Horizon passthrough (order("desc").includeFailed(true)) — Horizon orders operations by id desc, which is chronological including failed. So either Horizon itself is returning grouped data (a quirk of includeFailed+join("transactions") worth verifying with a real curl), or the reshuffle is happening somewhere we haven't found yet.
Two options:
- Soften to a defensive rationale that doesn't make an unverified claim:
// Defensive: guarantee desc order regardless of upstream behavior. The section reducer in useGetHistoryData.tsx requires sorted input. - Or do a 10-minute repro against a reproducing account and document what you actually saw — that's load-bearing context for whoever picks this up next.
As written, a future maintainer reads this, assumes the backend is at fault, and never investigates further — while freighter-mobile (or any future client) still sees the bug.
There was a problem hiding this comment.
@leofelix077 Wanted to follow up on the above Claude-generated comment:
Looks like you have a test account that you were able to replicate this. Would you mind quickly using Claude to check if the reason this is coming back in the wrong order is because of something freighter-backend is doing or because stellar-sdk/Horizon is returning these in the wrong order. Ideally, the BE would return things in the right order. Maybe there's some learnings that we can apply to the freighter-backend-v2 version of this.
Either way, I think it's fine to apply this client-side fix for now
There was a problem hiding this comment.
This is actually coming from the e2e test, not my own account to reproduce it
Checking against the backend it confirms your suspicions regarding Mercury, so I updated the comment not to mention backend but just force the sorting, as suggsted above
| const data = fetched | ||
| .map((op) => ({ op, ts: Date.parse(op.created_at) })) | ||
| .sort((a, b) => b.ts - a.ts) | ||
| .map(({ op }) => op); |
There was a problem hiding this comment.
Two small things on the sort itself:
Schwartzian is overkill here. N is capped at TRANSACTIONS_LIMIT=100 and Date.parse on ISO strings is ~microseconds — the decorate-sort-undecorate idiom isn't paying for itself, and readers have to mentally unwind it to confirm what it does. The plain form is shorter and equivalent in practical cost:
const data = [...fetched].sort(
(a, b) => Date.parse(b.created_at) - Date.parse(a.created_at),
);Date.parse(undefined) returns NaN, and a NaN-returning comparator yields implementation-defined non-stable order. Types say created_at is required string, so this is defensive — but a single malformed/missing record silently corrupts the entire sort. Cheapest guard inline: Date.parse(x.created_at) || 0 (or || -Infinity to bucket bad records at the bottom).
* bump versions to 5.41.1 (#2817) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Add CTA to let users know they can paste mnemonic phrase into box (#2803) * add subtext copy to paste mnemonic phrase * update snapshots for import screen * Restore submitProduction Sentry secrets fix on master (re-apply #2783) (#2819) * Fix submitProduction workflow secrets references The Sentry env-var guards used the `secrets` context inside step-level `if:` expressions, which GitHub Actions does not allow ("Unrecognized named-value: 'secrets'"). Map the Sentry secrets into the workflow-level `env:` block and check them via `env.*`, matching the existing `INDEXER_*` pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Scope Sentry secrets to the validation step only Drop the workflow-level env entries for SENTRY_ORG / SENTRY_PROJECT / SENTRY_AUTH_TOKEN and collapse the three separate guard steps into one "Validate Sentry secrets" step with a local env: block. This keeps the credentials out of every other step's process environment (including third-party actions in the job) and replaces the gh-run-cancel dance with a plain exit 1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Cássio Marcos Goulart <cassiomgoulart@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Truncate long memos dynamically (#2812) * adjust long memo truncation * simplify logic of memo truncation centralized * cleanup ellipsize logic from claude review * Strip Amplitude autocapture remote-code from extension bundle The Chrome Web Store / Firefox AMO rejected the build for shipping remotely-hosted code: https://cdn.amplitude.com/libs/visual-tagging-selector-1.0.0-alpha.js.gz This URL (and a sibling background-capture URL) comes from @amplitude/plugin-autocapture-browser, which @amplitude/analytics-browser imports statically. We initialize Amplitude with `autocapture: false`, so the code never runs — but the runtime flag does not remove the URLs from the bundle, and store review scans the static bundle. Alias the autocapture plugin to a no-op stub via webpack resolve.alias. This removes the visual-tagging-selector URL directly; because nothing else imports getOrCreateWindowMessenger / enableBackgroundCapture from @amplitude/analytics-core, tree-shaking also drops the core messenger code holding the background-capture URL. Verified against a clean production build: zero matches for cdn.amplitude.com, visual-tagging-selector, background-capture, or loadScriptOnce in the emitted .js, with the Amplitude analytics/experiment SDKs otherwise intact. The autocapture SDK first shipped in 5.39.0 (commit a3b2b50, PR #2659). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Order transactions by date (#2810) * add transaction date ordering * cleanup mocks and sort logic * adjust comment to reflect ordering reason * Don't embed literal blocked CDN URLs in stub comment Addresses review feedback: the production build emits source maps with sourcesContent and the store submission zips the build dir recursively, so a literal blocked remote-code URL in this comment could ride into the shipped .map artifact and re-trigger the store rejection. Describe the libs by name instead of writing the full fetchable URLs. (Verified the stub's source is currently DCE'd out of all emitted maps, so this is defense-in-depth against future config/bundler changes.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: leofelix077 <leonardoaalf077@hotmail.com> Co-authored-by: Cássio Marcos Goulart <cassiomgoulart@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Quick fix PR to get all fetched transactions and order them by their created at date, instead of depending on the returned groupings of the backend, which can group failed together, and successful together, and order them by date internally, instead of ordering the entire result/outcome by date