Phase 01 + 02: E2E coverage + observability for transacting users - #174
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Playwright E2E infra and fixtures, retrofits UI components with deterministic selectors, implements trade observability (trade_id lifecycle + typed events), threads eventContext through deployment/store layers, updates CI and ESLint, and adds comprehensive tests and planning docs. ChangesUI-Driven E2E + Observability
Sequence DiagramsequenceDiagram
participant GlobalSetup as globalSetup()
participant Anvil as Anvil Fork
participant Preview as Vite Preview
participant Playwright as Playwright Test
participant Browser as Browser (E2E)
participant RPC as Local JSON-RPC
GlobalSetup->>Anvil: start fork @ FORK_BLOCK
GlobalSetup->>Preview: launch preview server
GlobalSetup->>Playwright: set PREVIEW_URL / ANVIL_URL
Playwright->>Browser: navigate to PREVIEW_URL/trade
Browser->>Preview: GET /trade/[id] (CSP header)
Browser->>Browser: inject EIP-1193 stub
Playwright->>Browser: interact via data-testid
Browser->>RPC: window.ethereum.request(eth_sendTransaction)
RPC->>Anvil: submit tx -> mine block
Anvil->>Playwright: state/logs updated
Playwright->>Playwright: assert success-toast + on-chain balances
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: af76dffbf1
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 6. Fill the spend amount. The TradeAmountInput is wrapped by the | ||
| // spend-input testid; use a CSS descendant selector to land on its | ||
| // actual <input> element. | ||
| await page.locator('[data-testid="spend-input"] input').first().fill('100'); |
There was a problem hiding this comment.
After selecting Buy + market mode, MarketOrder initializes with inputMode = 'amount', so only data-testid="asset-input" is rendered until the user toggles modes; spend-input is absent at this point. This locator will time out and fail the smoke pre-flight, which blocks the new test-e2e pipeline before the suite can run. Either click the Buy/Spend toggle first or fill asset-input in this path.
Useful? React with 👍 / 👎.
Comment on lines
+62
to
+63
| test-e2e: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
This job runs unconditionally, but the E2E harness hard-fails when BASE_RPC_URL is missing (globalSetup throws before tests execute). In environments where secrets.BASE_RPC_URL is unset (for example forks or repos not yet configured), this turns every push into a failing workflow instead of skipping E2E, despite the intended secret-based gating.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
.planning/ROADMAP.md-61-61 (1)
61-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent v1.0 Phase 1 status — table contradicts the rest of the document.
Line 61 now reports v1.0 Phase 1 ("Shrink the Surface, See What's Happening") as
1/9 | In Progress, but the same file states v1.0 shipped 2026-05-05 (line 5), Phase 1 was completed 2026-04-29 with 8/8 plans (line 20), and the milestone is closed (line 68). This row also does not correspond to anything changed by this PR (which lives under v1.1 Phase 1). Looks like an inadvertent overwrite of the previously-complete row.📝 Proposed restoration
-| 1. Shrink the Surface, See What's Happening | v1.0 | 1/9 | In Progress| | +| 1. Shrink the Surface, See What's Happening | v1.0 | 8/8 | Complete | 2026-04-29 |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/ROADMAP.md at line 61, The table row for "Shrink the Surface, See What's Happening" was accidentally changed to "v1.0 | 1/9 | In Progress" — restore that row to reflect the previously-completed v1.0 Phase 1 (match the rest of the document and closed milestone): change the cell values for the "Shrink the Surface, See What's Happening" row back to show v1.0 Phase 1 completed (8/8) and the completed/closed status and date consistent with the other entries (e.g., Phase 1 completed 2026-04-29 and milestone closed), or revert that single row to its prior content so it no longer contradicts the v1.0 shipped/closed entries; ensure this edit is limited to the table row for "Shrink the Surface, See What's Happening" and does not alter v1.1 rows..planning/phases/01-ui-driven-e2e-order-test-coverage/01-VALIDATION.md-23-23 (1)
23-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlaywright version mismatch with
package.json.The validation doc lists the framework as "Playwright 1.49.x", but
package.json(this PR) pins@playwright/testat^1.59.1. Update the doc so future readers don't chase a non-existent 1.49 baseline.📝 Proposed fix
-| **NEW UI E2E framework** | Playwright 1.49.x + Chromium (added in Plan 01-01) | +| **NEW UI E2E framework** | Playwright 1.59.x + Chromium (added in Plan 01-01) |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-VALIDATION.md at line 23, Update the validation doc entry that currently reads "| **NEW UI E2E framework** | Playwright 1.49.x + Chromium (added in Plan 01-01) |" to reflect the actual pinned dependency in package.json (i.e., use Playwright 1.59.x or the exact version specifier ^1.59.1) so the documented baseline matches the project's package.json; modify the string in .planning/phases/01-ui-driven-e2e-order-test-coverage/01-VALIDATION.md accordingly.tests/integration/ui/globalSetup.ts-35-45 (1)
35-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProbe check misses 404 responses — the exact failure mode Pitfall 7 produces.
With
adapter-vercel,vite previewdoesn't serve SvelteKit API routes; a request to/api/auth/csrfreturns 404, not 500. The current guardapiProbe.status >= 500treats a 404 as success, silently bypassing the Pitfall 7 fail-fast and deferring the failure to cryptic errors deep in individual specs.🛡️ Proposed fix — check for any non-2xx response
- if (!apiProbe || apiProbe.status >= 500) { + if (!apiProbe || apiProbe.status < 200 || apiProbe.status >= 300) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/ui/globalSetup.ts` around lines 35 - 45, The probe currently treats 404 as success because it only fails when apiProbe is falsy or apiProbe.status >= 500; update the guard around the apiProbe Response (the apiProbe variable and the block that throws the Error) to treat any non-2xx response as a failure — e.g. check !apiProbe.ok or apiProbe.status < 200 || apiProbe.status >= 300 — and keep the existing Error throw when that condition is true so Pitfall 7 (vite preview not serving /api/*) fails fast..planning/STATE.md-29-32 (1)
29-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPhase status fields are inconsistent.
Line 29 says
EXECUTINGwhile Line 31 says “Phase complete — ready for verification” and top-level progress is 100%. Please align these fields to a single state to avoid downstream state-machine/reporting drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/STATE.md around lines 29 - 32, The Phase status fields in .planning/STATE.md are inconsistent: the "Phase:" line shows "EXECUTING" while the "Status:" line reads "Phase complete — ready for verification" and overall progress is 100%; update these fields so they all reflect a single, consistent state (e.g., set "Phase:" to "COMPLETE" or change "Status:" to match "EXECUTING") by editing the "Phase:", "Status:", and any top-level progress fields in the file so they agree (ensure "Phase:", "Status:", "Plan:", and "Last activity:" are coherent)..planning/phases/01-ui-driven-e2e-order-test-coverage/01-RESEARCH.md-865-865 (1)
865-865:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape pipe characters inside the table cell to avoid broken rendering.
The
assert(!env.E2E || dev)snippet uses||inside a markdown table cell, which can split columns unexpectedly. Escape pipes (\|\|) or wrap with HTML<code>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-RESEARCH.md at line 865, The table cell contains the snippet assert(!env.E2E || dev) which includes the `||` pipe characters that break Markdown table rendering; update the cell to escape the pipes (e.g., replace `||` with `\|\|`) or wrap the entire snippet in an inline code/HTML tag (e.g., <code>assert(!env.E2E || dev)</code>) so the table columns render correctly and the guard pattern remains readable..planning/phases/01-ui-driven-e2e-order-test-coverage/01-03-PLAN.md-145-145 (1)
145-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHarden the regex example to avoid markdown link parsing issues
Line 145’s inline regex is being parsed as reversed-link syntax by markdownlint. Put this command in a fenced
bashblock (preferred) to avoid MD011 noise and keep docs lint clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-03-PLAN.md at line 145, The inline regex command on line 145 is being parsed as markdown link syntax (MD011); wrap the grep command string (`grep -E "from ['\"]\\\$lib/(services/marketOrderExecution|stores/transaction)['\"]" src/lib/components/orders/MarketOrder.svelte src/lib/components/orders/LimitOrder.svelte 'src/routes/(main)/trade/[id]/+page.svelte'`) in a fenced bash block in the .planning/phases/01-ui-driven-e2e-order-test-coverage/01-03-PLAN.md file so markdownlint stops treating it as a link; ensure the fence language is "bash" and keep the exact command inside the block to preserve escaping and readability..planning/phases/01-ui-driven-e2e-order-test-coverage/01-RUNBOOK.md-91-93 (1)
91-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced code block
This fenced block is missing a language identifier. Use
bash(ortext) to satisfy markdownlint and improve readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-RUNBOOK.md around lines 91 - 93, The fenced code block containing the line "advance = freshnessWindow + 60s = 360s past current block timestamp" is missing a language tag; update that triple-backtick fence to include a language identifier (e.g., use ```bash or ```text) so markdownlint passes and the snippet renders with proper formatting..planning/phases/01-ui-driven-e2e-order-test-coverage/01-07-PLAN.md-190-190 (1)
190-190:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix markdownlint breakage in the inline verification command
The regex-heavy inline command on Line 190 is triggering markdown parsing/lint issues. Move the command into a fenced
bashblock (or escape the problematic sequence) to keep markdownlint stable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-07-PLAN.md at line 190, The long inline automated command containing many regexes (the `automated` line that starts with "test -f tests/integration/ui/limitDeploy.spec.ts && grep -q ...") is breaking markdownlint; wrap that entire command into a fenced bash code block (```bash ... ```) or escape the regex sequences so they aren't parsed as markdown, preserving the exact command content and markers like grep -qE 'takeOrders|OrderAdded' and the failWith count check; ensure the surrounding `automated` tag content remains intact and that the fenced block uses bash to keep linting happy.
🧹 Nitpick comments (10)
.github/workflows/test.yml (1)
96-114: 💤 Low valueSmoke spec runs twice in CI.
npm run test:e2eresolves toplaywright test, which discovers and runs every spec — includingsmoke.spec.ts, which already ran in the pre-flight step. On a green run that's a couple of minutes wasted; on a flaky run it doubles the smoke noise.♻️ Two equally cheap options
Option A — exclude smoke from the full step:
- name: E2E full suite - run: nix develop -c npm run test:e2e + run: nix develop -c npx playwright test --ignore-snapshots --grep-invert '@smoke'(requires tagging the smoke describe/test with
@smoke).Option B — define a Playwright project for
smokeand run--project=smokein pre-flight,--project=full(or--project=!smoke) in the full step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 96 - 114, The workflow currently runs smoke.spec.ts in the pre-flight and then runs the full suite (npm run test:e2e → playwright test) which rediscovers and re-runs the smoke tests; fix by making the full step skip the smoke tests or by running distinct Playwright projects: either (A) tag your smoke tests (smoke.spec.ts / the smoke describe/tests) with `@smoke` and change the “E2E full suite” command to exclude that tag (use Playwright's grep/grepInvert or equivalent) so npm run test:e2e does not re-run smoke, or (B) define Playwright projects named “smoke” and “full” and run the pre-flight using --project=smoke (or the specific smoke spec) and run the full step with --project=!smoke or --project=full; update the .github workflow commands accordingly so smoke runs only once.src/hooks.server.ts (1)
182-183: 💤 Low valueUse
env.E2Einstead ofprocess.env.E2Efor consistency.The rest of this module accesses environment variables via the already-imported
envfrom$env/dynamic/private(lines 19–20). This is the onlyprocess.envusage in the file and breaks the established pattern. SvelteKit recommends$env/dynamic/privateover directprocess.envaccess for consistency across adapters and to leverage SvelteKit's filtering and TypeScript support.♻️ Proposed change
-const isE2E = process.env.E2E === '1'; +const isE2E = env.E2E === '1';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks.server.ts` around lines 182 - 183, Replace the direct process.env access with the already-imported env by changing the isE2E check to use env.E2E (i.e., update the isE2E constant that currently reads process.env.E2E === '1'); update any related logic that computes connectSrcExtras so it remains identical in behavior but reads env.E2E via the existing env import from $env/dynamic/private (symbols: isE2E, connectSrcExtras, env).tests/integration/ui/fixtures.ts (1)
36-54: 💤 Low value
TOKENS.USDCis missing theidfield present ontNVDA/tAMZN.Minor consistency gap:
tNVDAandtAMZNcarry anid(used as the/trade/[id]slug) butUSDCdoes not. If a future spec ever loads the USDC trade page through the same lookup, accessingtokens.USDC.idwill be a compile error. Either dropideverywhere and derive it fromaddressat the call site, or add anidfor USDC for symmetry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/ui/fixtures.ts` around lines 36 - 54, The TOKENS object is inconsistent: tNVDA and tAMZN include an id used as the /trade/[id] slug but TOKENS.USDC lacks it; add an id field to TOKENS.USDC (e.g., set id to the same value as its address '0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913') so consumers referencing TOKENS.USDC.id compile, or alternatively remove id from tNVDA/tAMZN and derive the slug from address across the board—prefer adding the id to TOKENS.USDC for symmetry with tNVDA and tAMZN.tests/helpers/eip1193Stub.ts (2)
28-31: 💤 Low valueUse
JSON.stringifywhen embedding strings into the generated source.Minor robustness nit: directly interpolating
opts.address,chainId, andopts.rpcUrlinto the IIFE template means any apostrophe, backslash, or template character in those values would break parsing. With current callers all passing trusted hardcoded values it isn't an active bug, butJSON.stringifyis the canonical way to embed values into evaluated source and removes the foot-gun if a future caller passes computed input.♻️ Proposed change
- return `(() => { - const ADDRESS = '${opts.address}'; - const CHAIN_ID_HEX = '0x${chainId.toString(16)}'; - const RPC_URL = '${rpcUrl}'; + return `(() => { + const ADDRESS = ${JSON.stringify(opts.address)}; + const CHAIN_ID_HEX = ${JSON.stringify('0x' + chainId.toString(16))}; + const RPC_URL = ${JSON.stringify(rpcUrl)};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/helpers/eip1193Stub.ts` around lines 28 - 31, The returned IIFE string is embedding raw values (opts.address, chainId, opts.rpcUrl) directly which can break parsing if they contain quotes or escapes; update the template that defines ADDRESS, CHAIN_ID_HEX and RPC_URL so each embedded value is wrapped with JSON.stringify (e.g. use JSON.stringify(opts.address), JSON.stringify('0x' + chainId.toString(16)) and JSON.stringify(rpcUrl)) before interpolation into the returned string to safely escape characters.
33-42: 💤 Low value
rawRpcalways sendsid: 1and ignores HTTP error status.Two small reliability gaps for the stub:
- Hardcoded
id: 1is fine for sequential calls but anvil/some middlewares can flag duplicate ids; consider a monotonically incrementing counter so concurrent in-flight requests don't collide on the same response.- If anvil returns a non-2xx (e.g. transient 502 from a flaky proxy),
r.json()will likely still resolve butj.errormay be undefined; you'll silently returnundefinedasj.result. Adding anif (!r.ok)check makes failure modes much easier to diagnose in flaky CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/helpers/eip1193Stub.ts` around lines 33 - 42, The rawRpc function currently sends a hardcoded id: 1 and ignores HTTP-level failures; change rawRpc to use a monotonically incrementing request id (e.g., a module-scoped counter used when building the JSON-RPC body) so concurrent requests don't collide, and add an HTTP status check (if (!r.ok)) before calling r.json() to throw a clear error including status/text or response body; update references to RAW_RPC usage only if needed and keep RPC_URL as the target.tests/integration/ui/marketFailures.spec.ts (1)
145-170: 💤 Low value
insufficient_balancetest depends on init-script registration order.The page fixture's
addInitScript(eip1193StubSource(FUNDED_ACCOUNT))runs first, then this test'saddInitScript(eip1193StubSource(UNFUNDED_ACCOUNT))is registered second. Both scripts unconditionally assignwindow.ethereum = {…}, so the test relies on Playwright executing init scripts in registration order so the unfunded stub wins.That assumption is correct per Playwright docs but it's load-bearing and undocumented in the comment. Recommend either (a) making the stub source idempotent / merge instead of overwrite, or (b) noting the registration-order dependency in the comment so a future refactor doesn't re-order it. A small
// see Playwright addInitScript orderingreference is enough.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/ui/marketFailures.spec.ts` around lines 145 - 170, The test relies on Playwright's init-script registration order because both eip1193StubSource(FUNDED_ACCOUNT) and eip1193StubSource(UNFUNDED_ACCOUNT) assign window.ethereum unconditionally, so either make the stub idempotent/merging or explicitly document the ordering: update eip1193StubSource to check for and merge into window.ethereum instead of overwriting (or guard with if (!window.ethereum) return assignment) so UNFUNDED_ACCOUNT can safely replace/augment the prior stub, or add a short comment above the page.addInitScript(eip1193StubSource({ address: UNFUNDED_ACCOUNT.address })) call referencing Playwright addInitScript ordering and why the re-injection must run after the fixture script; reference eip1193StubSource, UNFUNDED_ACCOUNT, FUNDED_ACCOUNT and page.addInitScript in the change so reviewers can locate the logic.tests/helpers/anvilControl.ts (1)
39-49: 💤 Low value
withSnapshotrevert order interacts with the per-test fixture snapshot.Heads-up rather than a defect:
fixtures.tsalready wraps each test in aclient.snapshot()/client.revert(). Reverting to the snapshot taken insidewithSnapshotis fine because anvil only drops snapshots taken after the reverted id. Just be aware that the inverse — reverting the outer fixture-level snapshot — invalidates any inner ids, so callers must not retain ids across the fixture boundary. A short note in the docblock would prevent that footgun for future authors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/helpers/anvilControl.ts` around lines 39 - 49, The withSnapshot helper (function withSnapshot) currently reverts to an inner snapshot id which interacts with the per-test fixture-level snapshot; add a docblock above withSnapshot explaining the revert ordering: that Anvil drops snapshots taken after a reverted id so reverting an inner snapshot is safe, but reverting the outer fixture snapshot will invalidate any inner snapshot ids, and therefore callers must not retain or reuse snapshot ids across the fixture boundary. Keep the note concise and reference that callers should only use withSnapshot's scope-local behavior and not persist the returned id.tests/integration/ui/globalTeardown.ts (1)
6-9: ⚡ Quick winGuard each shutdown so a preview-server failure doesn't leak the anvil fork.
If
stopPreviewServer()rejects,stopAnvilFork()never runs, leaving an orphan anvil process holding port 8545 and breaking the next CI run. Globally-paired teardown of two independent resources should run both regardless of either's outcome.♻️ Proposed change
export default async function globalTeardown(): Promise<void> { - await stopPreviewServer(); - await stopAnvilFork(); + const results = await Promise.allSettled([stopPreviewServer(), stopAnvilFork()]); + const failures = results.filter((r): r is PromiseRejectedResult => r.status === 'rejected'); + if (failures.length > 0) { + // Surface failures but don't mask either; both stops attempted. + throw new AggregateError(failures.map((f) => f.reason), 'globalTeardown failure'); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/ui/globalTeardown.ts` around lines 6 - 9, The globalTeardown currently calls stopPreviewServer() then stopAnvilFork() sequentially and will skip stopAnvilFork() if stopPreviewServer() rejects; update globalTeardown to run both shutdowns guarded so both are attempted regardless of failures (e.g., call stopPreviewServer() and stopAnvilFork() inside their own try/catch blocks or use Promise.allSettled), capture any thrown errors from stopPreviewServer and stopAnvilFork (referencing the functions stopPreviewServer and stopAnvilFork), log or aggregate them, and if any failed rethrow a combined/Error listing so CI still fails but no anvil process is leaked.src/lib/components/orders/MarketOrder.svelte (1)
967-977: 💤 Low valueRemove the redundant outer
market-formdiv wrapper.Two nested divs with identical
data-modeanddata-sideattributes create unnecessary DOM nesting. All test files exclusively query[data-testid="market-form-loaded"]; the outermarket-formwrapper is never referenced in the codebase and can be safely removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/orders/MarketOrder.svelte` around lines 967 - 977, Remove the redundant outer wrapper div that uses data-testid="market-form" in MarketOrder.svelte: delete the entire outer <div data-testid="market-form" data-mode="market" data-side={orderSide.toLowerCase()}>, leaving the inner <div class="space-y-4" data-testid="market-form-loaded" data-mode="market" data-side={orderSide.toLowerCase()}> as the single container; ensure the corresponding closing tag for the removed wrapper is also removed so markup remains valid and tests continue to target [data-testid="market-form-loaded"]..planning/phases/01-ui-driven-e2e-order-test-coverage/01-05-PLAN.md (1)
14-14: 💤 Low valueTighten must_haves to match shipped target receive amount.
Line 14 says "spend-anchored (receive 100 USDC)" but the task
<behavior>at line 68 and the shipped spec both use 10 USDC (with a 9.9 USDC slippage floor). Update for consistency:📝 Proposed correction
- - "Both asset-anchored (sell 0.1 tNVDA) and spend-anchored (receive 100 USDC) Sell paths are covered" + - "Both asset-anchored (sell 0.1 tNVDA) and spend-anchored (receive 10 USDC) Sell paths are covered"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-05-PLAN.md at line 14, Update the must-have sentence that currently reads "Both asset-anchored (sell 0.1 tNVDA) and spend-anchored (receive 100 USDC) Sell paths are covered" to use the shipped target receive amount of 10 USDC (and, where relevant, reference the 9.9 USDC slippage floor); locate the sentence by searching for the exact string "Both asset-anchored (sell 0.1 tNVDA) and spend-anchored (receive 100 USDC) Sell paths are covered" and the related <behavior> block that mentions the receive amount, then replace "100 USDC" with "10 USDC" and ensure consistency with the "9.9 USDC slippage floor" phrasing used in the shipped spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 62-114: The test-e2e job is not gated on secrets.BASE_RPC_URL so
it will run (and fail) for forks; add a conditional gate using the secret check
(e.g., if: secrets.BASE_RPC_URL) either at the job level for job "test-e2e" or
on the two steps "E2E smoke pre-flight (fast-fail per D-14)" and "E2E full
suite" to skip these Playwright steps when the secret is missing; ensure the
conditional expression matches GitHub Actions syntax (e.g., if: ${{
secrets.BASE_RPC_URL }} or equivalent) and apply it consistently to the steps
that consume BASE_RPC_URL.
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-01-PLAN.md:
- Around line 398-405: The planning doc currently embeds literal private keys in
the exported constants FUNDED_ACCOUNT and UNFUNDED_ACCOUNT; remove the raw key
material and replace the values with non-secret references or masked
placeholders (e.g., "anvil default account `#0`" / "anvil default account `#1`" or
"0x...<masked>") and add a short pointer comment to Foundry/Anvil defaults or
test fixture docs; keep the exported symbol names (FUNDED_ACCOUNT,
UNFUNDED_ACCOUNT) for consumers but ensure no plaintext private keys are stored
in the repo.
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-02-SUMMARY.md:
- Line 83: Replace the machine-specific absolute path shown in the verification
claim ("find /Users/alastairong/st0x/st0x/tests -name \"marketHours*\"") with a
repo-relative command so the runbook is portable; update the text on that line
to use a relative find invocation like `find tests -name "marketHours*"` (or a
portable placeholder such as `find $(pwd)/tests -name "marketHours*"`), and scan
the same summary for any other absolute local paths to remove or replace
similarly.
In `@eslint.config.js`:
- Around line 121-133: Update the glob patterns that use trailing asterisks to
use explicit subpath globs so sibling modules aren't blocked: replace entries
like '$lib/stores/transaction*', '$lib/services/marketOrderExecution*',
'$lib/services/orderDeployment*', and '$lib/services/walletService*' with a pair
of entries for each module — the base module string (e.g.
'$lib/stores/transaction') and the recursive subpath string using '/**' (e.g.
'$lib/stores/transaction/**') so only the module and its children are
restricted; also add a recursive variant for the type path
('$lib/types/orderPerspective/**') so type subpaths are correctly handled.
In `@src/lib/components/orders/LimitOrder.svelte`:
- Around line 567-576: The success toast is shown using
tradeSubmittedSuccessfully before the async deploy completes; change the logic
so the toast only appears after the deploy result is confirmed (either by
awaiting the deploy promise or by introducing a new boolean like
tradeDeployedSuccessfully). Locate the function that kicks off the async deploy
(e.g., submitOrder/handleDeploy/deployTrade) and move the state update that sets
tradeSubmittedSuccessfully into the success branch after the await/confirmation,
or add and set tradeDeployedSuccessfully on success and use that variable in the
template instead of tradeSubmittedSuccessfully; ensure the failure branch sets
an error state and that any tests/screens read the new confirmed-success flag.
- Around line 556-566: The branch that renders when belowMinTradeError is true
is mislabelled as an insufficient_balance error; update the LimitOrder.svelte
markup for the belowMinTradeError branch so data-error-class uses a distinct
identifier (e.g., "below_min_trade" or "min_trade_value") and change the
visible/error text from "insufficient_balance" to a matching minimum-trade
message, keeping the existing data-mode, data-side, role, and aria-live
attributes unchanged; locate the conditional that references belowMinTradeError
to make these replacements.
In `@src/lib/components/orders/MarketOrder.svelte`:
- Around line 311-328: Replace the brittle substring-based classification in the
reactive errorClass block with the explicit errorClass returned by the service:
stop deriving semantics from orderPreparationError and instead read and use a
discriminated errorClass field produced by executeMarketOrder (from
marketOrderExecution.ts); keep the existing checks for insufficientBalanceError,
noLiquidityError and priceError/priceErrorReason but remove the final fallback
that maps any orderPreparationError to 'slippage' so unknown prep errors yield
null or the service-provided class, and update any references to the previous
heuristics to prefer the service-provided errorClass to avoid coupling to
user-facing copy and to make TEST-08 deterministic.
- Around line 1264-1273: The hidden error-banner currently exposes internal
identifiers via role="alert" aria-live and textContent {errorClass}; update the
MarketOrder.svelte hidden element that uses data-testid="error-banner" (and
references errorClass and orderSide) to stop announcing internal values by
removing role="alert" and aria-live="polite" and instead mark it as non-aria
(add aria-hidden="true") so the visible, human-readable error copy remains the
single announcement; alternatively, if you prefer announcements from this node,
replace {errorClass} with the localized/error message mapping for those
errorClass keys rather than raw identifiers.
In `@src/routes/`(main)/trade/[id]/+page.svelte:
- Around line 1818-1841: The tests fail because the reactive that watches
panelStrategy (when set to 'limit' or 'dca') calls isVaultTutorialHidden() and,
on fresh browser contexts, shows the vault tutorial overlay which hides the
trade panel; to fix it, update the E2E init script invoked via
page.addInitScript to pre-set the client-side tutorial flag by calling
window.localStorage.setItem('st0x_hide_vault_tutorial','true') before the page
loads so isVaultTutorialHidden() returns true and the overlay won't trigger when
the mode-tab buttons change panelStrategy and set showTradePanel; modify the
page.addInitScript handler (the same one that stubs EIP-1193) to set that
localStorage key for the test account.
In `@tests/helpers/previewServer.ts`:
- Around line 55-60: In stopPreviewServer(), don't rely on a fixed 200ms sleep
after previewProc.kill('SIGTERM'); instead attach and await a Promise that
resolves on previewProc's 'exit' event (e.g. previewProc.on('exit', resolve))
before nulling previewProc so the port is released deterministically; ensure you
attach the listener before calling previewProc.kill() and handle the case where
the process may have already exited (remove listener after resolution or use
once) to avoid leaks.
In `@tests/integration/ui/limitDeploy.spec.ts`:
- Around line 169-184: The test's catch handler for getContractEvents (used to
populate orderAddedLogs) intends to allow ABI-parse failures to continue, but
the unconditional expect(orderAddedLogs.length).toBeGreaterThanOrEqual(1) forces
a hard fail; change the assertion to be conditional (only assert existence of
logs when orderAddedLogs.length > 0) or remove it entirely so the test proceeds
to the existing guard that checks if (orderAddedLogs.length > 0) before
exercising the counterparty fill; reference symbols: orderAddedLogs,
testClient.getContractEvents, ORDERBOOK_ADDRESS, ORDERBOOK_ABI, and the existing
if (orderAddedLogs.length > 0) block to ensure behavior matches the comment.
In `@tests/integration/ui/marketBuy.spec.ts`:
- Around line 50-54: The test fails because MarketOrder.svelte defaults
inputMode = 'amount' so the selector '[data-testid="spend-input"]' may not
exist; before the fill call in marketBuy.spec.ts (the block using
page.locator('[data-testid="spend-input"] input').first().fill('100')), add a
click on the input-mode toggle control to switch to 'spend' mode (the same
toggle used in the second test that targets '[data-testid="asset-input"]');
likewise update the related tests (the second test comment in this file,
tests/integration/ui/smoke.spec.ts at the noted line, and
tests/integration/ui/marketSell.spec.ts around the referenced line) to either
perform the toggle before using spend-input or directly target the correct
data-testid depending on the default mode, and remove the uncertain comment
references so the intent is explicit.
In `@tests/integration/ui/marketFailures.spec.ts`:
- Line 35: SATURDAY_03_UTC constant value/comment are mismatched and produce a
Friday ET dayOfWeek; update the test to use a UTC instant that maps to Saturday
daytime in ET (e.g. replace SATURDAY_03_UTC value with 1777132800 which is
2026-04-25 16:00:00 UTC → Sat 12:00 EDT) and update the surrounding comment and
any assertions that expect dayOfWeek === 6 so they match the new timestamp;
alternatively, if you prefer to keep the original epoch, update the comment to
state Fri 2025-04-25 03:00:00 UTC and adjust the test expectations to reflect ET
dayOfWeek === 5 (affecting references to isOutsideMarketHours / marketHours.ts
behavior).
---
Minor comments:
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-03-PLAN.md:
- Line 145: The inline regex command on line 145 is being parsed as markdown
link syntax (MD011); wrap the grep command string (`grep -E "from
['\"]\\\$lib/(services/marketOrderExecution|stores/transaction)['\"]"
src/lib/components/orders/MarketOrder.svelte
src/lib/components/orders/LimitOrder.svelte
'src/routes/(main)/trade/[id]/+page.svelte'`) in a fenced bash block in the
.planning/phases/01-ui-driven-e2e-order-test-coverage/01-03-PLAN.md file so
markdownlint stops treating it as a link; ensure the fence language is "bash"
and keep the exact command inside the block to preserve escaping and
readability.
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-07-PLAN.md:
- Line 190: The long inline automated command containing many regexes (the
`automated` line that starts with "test -f
tests/integration/ui/limitDeploy.spec.ts && grep -q ...") is breaking
markdownlint; wrap that entire command into a fenced bash code block (```bash
... ```) or escape the regex sequences so they aren't parsed as markdown,
preserving the exact command content and markers like grep -qE
'takeOrders|OrderAdded' and the failWith count check; ensure the surrounding
`automated` tag content remains intact and that the fenced block uses bash to
keep linting happy.
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-RESEARCH.md:
- Line 865: The table cell contains the snippet assert(!env.E2E || dev) which
includes the `||` pipe characters that break Markdown table rendering; update
the cell to escape the pipes (e.g., replace `||` with `\|\|`) or wrap the entire
snippet in an inline code/HTML tag (e.g., <code>assert(!env.E2E || dev)</code>)
so the table columns render correctly and the guard pattern remains readable.
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-RUNBOOK.md:
- Around line 91-93: The fenced code block containing the line "advance =
freshnessWindow + 60s = 360s past current block timestamp" is missing a language
tag; update that triple-backtick fence to include a language identifier (e.g.,
use ```bash or ```text) so markdownlint passes and the snippet renders with
proper formatting.
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-VALIDATION.md:
- Line 23: Update the validation doc entry that currently reads "| **NEW UI E2E
framework** | Playwright 1.49.x + Chromium (added in Plan 01-01) |" to reflect
the actual pinned dependency in package.json (i.e., use Playwright 1.59.x or the
exact version specifier ^1.59.1) so the documented baseline matches the
project's package.json; modify the string in
.planning/phases/01-ui-driven-e2e-order-test-coverage/01-VALIDATION.md
accordingly.
In @.planning/ROADMAP.md:
- Line 61: The table row for "Shrink the Surface, See What's Happening" was
accidentally changed to "v1.0 | 1/9 | In Progress" — restore that row to reflect
the previously-completed v1.0 Phase 1 (match the rest of the document and closed
milestone): change the cell values for the "Shrink the Surface, See What's
Happening" row back to show v1.0 Phase 1 completed (8/8) and the
completed/closed status and date consistent with the other entries (e.g., Phase
1 completed 2026-04-29 and milestone closed), or revert that single row to its
prior content so it no longer contradicts the v1.0 shipped/closed entries;
ensure this edit is limited to the table row for "Shrink the Surface, See What's
Happening" and does not alter v1.1 rows.
In @.planning/STATE.md:
- Around line 29-32: The Phase status fields in .planning/STATE.md are
inconsistent: the "Phase:" line shows "EXECUTING" while the "Status:" line reads
"Phase complete — ready for verification" and overall progress is 100%; update
these fields so they all reflect a single, consistent state (e.g., set "Phase:"
to "COMPLETE" or change "Status:" to match "EXECUTING") by editing the "Phase:",
"Status:", and any top-level progress fields in the file so they agree (ensure
"Phase:", "Status:", "Plan:", and "Last activity:" are coherent).
In `@tests/integration/ui/globalSetup.ts`:
- Around line 35-45: The probe currently treats 404 as success because it only
fails when apiProbe is falsy or apiProbe.status >= 500; update the guard around
the apiProbe Response (the apiProbe variable and the block that throws the
Error) to treat any non-2xx response as a failure — e.g. check !apiProbe.ok or
apiProbe.status < 200 || apiProbe.status >= 300 — and keep the existing Error
throw when that condition is true so Pitfall 7 (vite preview not serving /api/*)
fails fast.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 96-114: The workflow currently runs smoke.spec.ts in the
pre-flight and then runs the full suite (npm run test:e2e → playwright test)
which rediscovers and re-runs the smoke tests; fix by making the full step skip
the smoke tests or by running distinct Playwright projects: either (A) tag your
smoke tests (smoke.spec.ts / the smoke describe/tests) with `@smoke` and change
the “E2E full suite” command to exclude that tag (use Playwright's
grep/grepInvert or equivalent) so npm run test:e2e does not re-run smoke, or (B)
define Playwright projects named “smoke” and “full” and run the pre-flight using
--project=smoke (or the specific smoke spec) and run the full step with
--project=!smoke or --project=full; update the .github workflow commands
accordingly so smoke runs only once.
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-05-PLAN.md:
- Line 14: Update the must-have sentence that currently reads "Both
asset-anchored (sell 0.1 tNVDA) and spend-anchored (receive 100 USDC) Sell paths
are covered" to use the shipped target receive amount of 10 USDC (and, where
relevant, reference the 9.9 USDC slippage floor); locate the sentence by
searching for the exact string "Both asset-anchored (sell 0.1 tNVDA) and
spend-anchored (receive 100 USDC) Sell paths are covered" and the related
<behavior> block that mentions the receive amount, then replace "100 USDC" with
"10 USDC" and ensure consistency with the "9.9 USDC slippage floor" phrasing
used in the shipped spec.
In `@src/hooks.server.ts`:
- Around line 182-183: Replace the direct process.env access with the
already-imported env by changing the isE2E check to use env.E2E (i.e., update
the isE2E constant that currently reads process.env.E2E === '1'); update any
related logic that computes connectSrcExtras so it remains identical in behavior
but reads env.E2E via the existing env import from $env/dynamic/private
(symbols: isE2E, connectSrcExtras, env).
In `@src/lib/components/orders/MarketOrder.svelte`:
- Around line 967-977: Remove the redundant outer wrapper div that uses
data-testid="market-form" in MarketOrder.svelte: delete the entire outer <div
data-testid="market-form" data-mode="market"
data-side={orderSide.toLowerCase()}>, leaving the inner <div class="space-y-4"
data-testid="market-form-loaded" data-mode="market"
data-side={orderSide.toLowerCase()}> as the single container; ensure the
corresponding closing tag for the removed wrapper is also removed so markup
remains valid and tests continue to target [data-testid="market-form-loaded"].
In `@tests/helpers/anvilControl.ts`:
- Around line 39-49: The withSnapshot helper (function withSnapshot) currently
reverts to an inner snapshot id which interacts with the per-test fixture-level
snapshot; add a docblock above withSnapshot explaining the revert ordering: that
Anvil drops snapshots taken after a reverted id so reverting an inner snapshot
is safe, but reverting the outer fixture snapshot will invalidate any inner
snapshot ids, and therefore callers must not retain or reuse snapshot ids across
the fixture boundary. Keep the note concise and reference that callers should
only use withSnapshot's scope-local behavior and not persist the returned id.
In `@tests/helpers/eip1193Stub.ts`:
- Around line 28-31: The returned IIFE string is embedding raw values
(opts.address, chainId, opts.rpcUrl) directly which can break parsing if they
contain quotes or escapes; update the template that defines ADDRESS,
CHAIN_ID_HEX and RPC_URL so each embedded value is wrapped with JSON.stringify
(e.g. use JSON.stringify(opts.address), JSON.stringify('0x' +
chainId.toString(16)) and JSON.stringify(rpcUrl)) before interpolation into the
returned string to safely escape characters.
- Around line 33-42: The rawRpc function currently sends a hardcoded id: 1 and
ignores HTTP-level failures; change rawRpc to use a monotonically incrementing
request id (e.g., a module-scoped counter used when building the JSON-RPC body)
so concurrent requests don't collide, and add an HTTP status check (if (!r.ok))
before calling r.json() to throw a clear error including status/text or response
body; update references to RAW_RPC usage only if needed and keep RPC_URL as the
target.
In `@tests/integration/ui/fixtures.ts`:
- Around line 36-54: The TOKENS object is inconsistent: tNVDA and tAMZN include
an id used as the /trade/[id] slug but TOKENS.USDC lacks it; add an id field to
TOKENS.USDC (e.g., set id to the same value as its address
'0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913') so consumers referencing
TOKENS.USDC.id compile, or alternatively remove id from tNVDA/tAMZN and derive
the slug from address across the board—prefer adding the id to TOKENS.USDC for
symmetry with tNVDA and tAMZN.
In `@tests/integration/ui/globalTeardown.ts`:
- Around line 6-9: The globalTeardown currently calls stopPreviewServer() then
stopAnvilFork() sequentially and will skip stopAnvilFork() if
stopPreviewServer() rejects; update globalTeardown to run both shutdowns guarded
so both are attempted regardless of failures (e.g., call stopPreviewServer() and
stopAnvilFork() inside their own try/catch blocks or use Promise.allSettled),
capture any thrown errors from stopPreviewServer and stopAnvilFork (referencing
the functions stopPreviewServer and stopAnvilFork), log or aggregate them, and
if any failed rethrow a combined/Error listing so CI still fails but no anvil
process is leaked.
In `@tests/integration/ui/marketFailures.spec.ts`:
- Around line 145-170: The test relies on Playwright's init-script registration
order because both eip1193StubSource(FUNDED_ACCOUNT) and
eip1193StubSource(UNFUNDED_ACCOUNT) assign window.ethereum unconditionally, so
either make the stub idempotent/merging or explicitly document the ordering:
update eip1193StubSource to check for and merge into window.ethereum instead of
overwriting (or guard with if (!window.ethereum) return assignment) so
UNFUNDED_ACCOUNT can safely replace/augment the prior stub, or add a short
comment above the page.addInitScript(eip1193StubSource({ address:
UNFUNDED_ACCOUNT.address })) call referencing Playwright addInitScript ordering
and why the re-injection must run after the fixture script; reference
eip1193StubSource, UNFUNDED_ACCOUNT, FUNDED_ACCOUNT and page.addInitScript in
the change so reviewers can locate the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d800f46-5773-43bf-90fb-122acedc4613
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (48)
.github/workflows/test.yml.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/codebase/TESTING.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-01-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-01-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-02-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-02-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-03-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-03-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-04-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-04-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-05-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-05-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-06-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-06-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-07-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-07-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-08-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-08-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-09-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-09-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-AUDIT.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-PATTERNS.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-RESEARCH.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-RUNBOOK.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-VALIDATION.mdeslint.config.jspackage.jsonplaywright.config.tssrc/hooks.server.tssrc/lib/components/orders/LimitOrder.sveltesrc/lib/components/orders/MarketOrder.sveltesrc/routes/(main)/trade/[id]/+page.sveltetests/fixtures/eslint/ui-test-import-violation.tstests/helpers/anvilControl.tstests/helpers/eip1193Stub.tstests/helpers/previewServer.tstests/integration/ui/fixtures.tstests/integration/ui/globalSetup.tstests/integration/ui/globalTeardown.tstests/integration/ui/limitDeploy.spec.tstests/integration/ui/marketBuy.spec.tstests/integration/ui/marketFailures.spec.tstests/integration/ui/marketSell.spec.tstests/integration/ui/smoke.spec.tstests/lib/utils/marketHours.test.ts
|
|
||
| ## Self-Check Method Notes | ||
|
|
||
| The audit read the first ~30 lines of every test file under `tests/lib/services/`, `tests/lib/utils/`, `tests/lib/types/`, `tests/lib/stores/`, `tests/integration/marketOrder/`, plus `tests/lib/transactionStore.test.ts` + `tests/lib/validateDeploymentArgs.test.ts` (both at `tests/lib/` root). Every cell that names a test path was verified against the file's describe blocks + imports. The single absence claim (`tests/lib/utils/marketHours.test.ts`) was verified via `find /Users/alastairong/st0x/st0x/tests -name "marketHours*"` returning empty. |
There was a problem hiding this comment.
Avoid absolute local paths in verification receipts
Line 83 includes a machine-specific path (/Users/...) in a verification claim. This leaks local environment details and makes the runbook less reproducible for CI/other contributors. Prefer repo-relative commands (e.g., find tests -name "marketHours*").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-02-SUMMARY.md at
line 83, Replace the machine-specific absolute path shown in the verification
claim ("find /Users/alastairong/st0x/st0x/tests -name \"marketHours*\"") with a
repo-relative command so the runbook is portable; update the text on that line
to use a relative find invocation like `find tests -name "marketHours*"` (or a
portable placeholder such as `find $(pwd)/tests -name "marketHours*"`), and scan
the same summary for any other absolute local paths to remove or replace
similarly.
Comment on lines
+121
to
+133
| patterns: [ | ||
| { | ||
| group: [ | ||
| '$lib/services/marketOrderExecution', | ||
| '$lib/services/marketOrderExecution*', | ||
| '$lib/stores/transaction', | ||
| '$lib/stores/transaction*', | ||
| '$lib/services/orderDeployment', | ||
| '$lib/services/orderDeployment*', | ||
| '$lib/services/walletService', | ||
| '$lib/services/walletService*', | ||
| '$lib/types/orderPerspective' | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
eslint no-restricted-imports patterns group glob minimatch path separator
💡 Result:
In ESLint’s built-in rule no-restricted-imports, the “patterns” option uses gitignore-style patterns (not regex), and it supports a per-pattern object form with fields like group and message. [1] 1) How to use patterns/group - Object form supports patterns as an array. Each entry can be either: - a string gitignore-style pattern, or - an object with group (an array of gitignore-style patterns) and an optional message. [1] - Example (group + message): [1] "no-restricted-imports": ["error", { "patterns": [{ "group": ["import1/private/"], "message": "usage of import1 private modules not allowed." }] }] 2) Glob minimatch style vs glob modules - ESLint documents that patterns are “gitignore-style patterns” (string patterns or group entries). [1] - ESLint’s documentation also notes an important limitation: you can’t combine group (gitignore-style) with the regex option. [1] 3) Does it use a specific glob engine (e.g., minimatch)? - From the authoritative ESLint docs we have here, the documented contract is “gitignore-style patterns”. The docs do not name “minimatch” (or similar) as the underlying matcher. [1] - Therefore: the safest source-backed answer is that ESLint defines the syntax/behavior as gitignore-style patterns, without committing (in the docs we found) to a particular library such as minimatch. [1] 4) Path separator behavior - The ESLint docs we found describe patterns in terms of import paths like import-foo/ba and import1/private/* (using forward slashes in examples). [1] - However, the docs excerpt we retrieved does not explicitly state whether backslashes ("\") are treated specially or normalized. So we can’t assert backslash-vs-forward-slash behavior from the sourced material above. What we can say from examples is that patterns are written using forward slashes in the import-path syntax. [1] 5) Group/ordering and negation - When using gitignore-style patterns, you can re-include a module via negation using a leading!, and order matters (negated patterns must be placed last). [1] Quick reference (source-backed) - Use patterns: [] with gitignore-style strings or objects with group: [] and optional message. [1] - Support negation with!, and ensure negated patterns come last. [1] - Do not mix group with regex. [1] If you tell me an exact set of imports you want to restrict (with the separator characters you expect), I can suggest a concrete no-restricted-imports patterns configuration consistent with ESLint’s gitignore-style matching rules.
Citations:
🏁 Script executed:
# First, let's search for ESLint documentation or configuration details about no-restricted-imports
# and confirm if minimatch is used and its `*` behavior with path separators
rg -i "minimatch|no-restricted-imports|gitignore" --type-list | head -20Repository: SARKEX/st0x
Length of output: 445
🏁 Script executed:
# Check if the mentioned restricted modules have any subpath imports in the codebase
# that would demonstrate the pattern gap
rg '\$lib/(services/marketOrderExecution|stores/transaction|services/orderDeployment|services/walletService|types/orderPerspective)/' . --type ts --type js --type svelte 2>/dev/null | head -30Repository: SARKEX/st0x
Length of output: 37
🌐 Web query:
minimatch wildcard star does not match path separator
💡 Result:
Yes, in minimatch, the single wildcard star * does not match path separators (/), by design to emulate shell behavior like bash or zsh[1][2][3]. It matches any characters within a single path segment (directory or filename), but stops at /[4][2]. For example, xxx/* matches xxx/abc but not xxx/abc/def[2]. To match across path segments, use `` (globstar), which recurses into subdirectories when it's the only content in a path part, e.g., xxx//yyy matches `xxx/abc/yyy` or `xxx/abc/def/yyy`[3][15]. This path-segment-respecting behavior ensures `*` acts like shell globbing, not recursive like `find`[9]. Official docs confirm patterns use `/` only, treating it as a separator[4][5].
Citations:
- 1: Single asterisk and embedded folders isaacs/minimatch#172
- 2: https://github.com/motemen/minimatch-cheat-sheet
- 3: https://userjot.com/blog/match-file-patterns-with-minimatch-in-nodejs
- 4: https://github.com/isaacs/minimatch/blob/main/README.md
- 5: https://www.npmjs.com/package/minimatch?activeTab=readme
🌐 Web query:
ESLint no-restricted-imports minimatch implementation
💡 Result:
The ESLint rule no-restricted-imports implements pattern matching for the "patterns" option using gitignore-style patterns, processed via the "ignore" library (kaelzhang/node-ignore), not minimatch. This library parses.gitignore-like patterns (including! negations) to determine if an import path is restricted [1][2]. The official documentation confirms support for gitignore-style patterns or regex in patterns [3][4]. No sources indicate minimatch usage in ESLint core; minimatch appears in related projects like eslint-plugin-import discussions or VSCode rules [5][6]. For exact implementation, see ESLint source at https://github.com/eslint/eslint/blob/main/lib/rules/no-restricted-imports.js (inferred from docs and commits [7][8][9]).
Citations:
- 1: https://stackoverflow.com/questions/66330715/eslint-no-restricted-imports-only-restrict-importing-from-a-package-root
- 2: no-restricted-imports exact pattern match is not possible eslint/eslint#13857
- 3: https://eslint.org/docs/latest/rules/no-restricted-imports
- 4: https://github.com/eslint/eslint/blob/main/docs/src/rules/no-restricted-imports.md
- 5: Interest in adding "no-restricted-glob-paths" rule? import-js/eslint-plugin-import#1662
- 6: https://github.com/microsoft/vscode/blob/main/.eslint-plugin-local/code-import-patterns.ts
- 7: eslint/eslint@11144a2
- 8: eslint/eslint@b017f09
- 9: https://github.com/eslint/eslint/compare/147ad2e3c9c926136a37b0f245ee06006ef27a23..2a56777ab5165d702632d55a9c17de06324f49e1
🏁 Script executed:
# Check if these modules are files or directories
fd -t f '$lib/services/marketOrderExecution' 2>/dev/null || echo "Not found as file"
fd -t d '$lib/services/marketOrderExecution' 2>/dev/null || echo "Not found as directory"
fd -t f 'marketOrderExecution' 2>/dev/null | head -20
fd -t d 'marketOrderExecution' 2>/dev/null | head -20Repository: SARKEX/st0x
Length of output: 144
🏁 Script executed:
# Check the full eslint.config.js to understand context and pattern structure
wc -l eslint.config.js
head -150 eslint.config.js | tail -50Repository: SARKEX/st0x
Length of output: 1977
🏁 Script executed:
# Verify the ignore library behavior with gitignore patterns
cat > /tmp/test_ignore.js << 'EOF'
// Test how node-ignore handles * vs ** patterns
const ignore = require('ignore');
const patterns_star = [
'$lib/services/marketOrderExecution',
'$lib/services/marketOrderExecution*'
];
const patterns_globstar = [
'$lib/services/marketOrderExecution',
'$lib/services/marketOrderExecution/**'
];
const testPaths = [
'$lib/services/marketOrderExecution',
'$lib/services/marketOrderExecutionExtra',
'$lib/services/marketOrderExecution/foo',
'$lib/services/marketOrderExecution/foo/bar'
];
console.log('=== With * wildcard ===');
const ig1 = ignore().add(patterns_star);
testPaths.forEach(p => {
console.log(`${p}: ${ig1.ignores(p) ? 'BLOCKED' : 'ALLOWED'}`);
});
console.log('\n=== With /** globstar ===');
const ig2 = ignore().add(patterns_globstar);
testPaths.forEach(p => {
console.log(`${p}: ${ig2.ignores(p) ? 'BLOCKED' : 'ALLOWED'}`);
});
EOF
cd /tmp && npm install ignore > /dev/null 2>&1 && node test_ignore.js 2>/dev/null || echo "Could not test ignore library"Repository: SARKEX/st0x
Length of output: 492
🏁 Script executed:
# Check for variant module names
fd '(marketOrderExecution|transaction|orderDeployment|walletService)' src/lib --type fRepository: SARKEX/st0x
Length of output: 298
🏁 Script executed:
# Check the test fixture to understand the intent of the rule
cat tests/fixtures/eslint/ui-test-import-violation.tsRepository: SARKEX/st0x
Length of output: 1239
🏁 Script executed:
# Check if transactionShared is actually used or if it's also restricted
grep -r "transactionShared" src/lib --include="*.ts" --include="*.js" | head -20Repository: SARKEX/st0x
Length of output: 1376
🏁 Script executed:
# Verify the intent: check what transactionShared contains
head -30 src/lib/stores/transactionShared.tsRepository: SARKEX/st0x
Length of output: 1489
🏁 Script executed:
# Double-check: does the * pattern actually block transactionShared?
cat > /tmp/verify_pattern.js << 'EOF'
const ignore = require('ignore');
const patterns = [
'$lib/stores/transaction',
'$lib/stores/transaction*'
];
const ig = ignore().add(patterns);
const testPaths = [
'$lib/stores/transaction',
'$lib/stores/transactionShared',
'$lib/stores/transaction/foo'
];
console.log('Pattern list:', patterns);
testPaths.forEach(p => {
console.log(`${p}: ${ig.ignores(p) ? 'BLOCKED' : 'ALLOWED'}`);
});
EOF
cd /tmp && node verify_pattern.jsRepository: SARKEX/st0x
Length of output: 236
Replace * wildcards with /** to prevent accidentally blocking sibling modules.
The pattern $lib/stores/transaction* blocks both transaction.ts and the unintended transactionShared.ts. Since transactionShared is a shared leaf module legitimately imported throughout the codebase (including by marketOrderExecution.ts), it should not be restricted. Use /** instead to restrict only the exact module and its subpaths:
$lib/stores/transaction+$lib/stores/transaction/**blockstransaction.tsand subpaths only, allowingtransactionShared.ts- Apply the same fix to the other three modules:
marketOrderExecution,orderDeployment, andwalletService - Add
$lib/types/orderPerspective/**as a parallel variant to intentionally allow type subpaths
♻️ Proposed pattern set
patterns: [
{
group: [
'$lib/services/marketOrderExecution',
- '$lib/services/marketOrderExecution*',
+ '$lib/services/marketOrderExecution/**',
'$lib/stores/transaction',
- '$lib/stores/transaction*',
+ '$lib/stores/transaction/**',
'$lib/services/orderDeployment',
- '$lib/services/orderDeployment*',
+ '$lib/services/orderDeployment/**',
'$lib/services/walletService',
- '$lib/services/walletService*',
+ '$lib/services/walletService/**',
'$lib/types/orderPerspective',
+ '$lib/types/orderPerspective/**'
],Verify by extending tests/fixtures/eslint/ui-test-import-violation.ts with import { transactionStoreInternal } from '$lib/stores/transactionShared'; — it should not trigger the rule after the fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@eslint.config.js` around lines 121 - 133, Update the glob patterns that use
trailing asterisks to use explicit subpath globs so sibling modules aren't
blocked: replace entries like '$lib/stores/transaction*',
'$lib/services/marketOrderExecution*', '$lib/services/orderDeployment*', and
'$lib/services/walletService*' with a pair of entries for each module — the base
module string (e.g. '$lib/stores/transaction') and the recursive subpath string
using '/**' (e.g. '$lib/stores/transaction/**') so only the module and its
children are restricted; also add a recursive variant for the type path
('$lib/types/orderPerspective/**') so type subpaths are correctly handled.
Comment on lines
+55
to
+60
| export async function stopPreviewServer(): Promise<void> { | ||
| if (!previewProc) return; | ||
| previewProc.kill('SIGTERM'); | ||
| // give it a moment to clean up (mirrors stopAnvilFork()) | ||
| await new Promise((r) => setTimeout(r, 200)); | ||
| previewProc = null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current shutdown behavior and call ordering in E2E setup/teardown.
rg -n -C3 "export async function stopPreviewServer|kill\\('SIGTERM'\\)|startPreviewServer\\(|stopPreviewServer\\(" tests/helpers/previewServer.ts tests/integration/ui/globalSetup.ts tests/integration/ui/globalTeardown.tsRepository: SARKEX/st0x
Length of output: 2602
🏁 Script executed:
cat -n tests/helpers/previewServer.tsRepository: SARKEX/st0x
Length of output: 2498
🏁 Script executed:
rg -n -A10 "export.*function stopAnvilFork" tests/helpers/Repository: SARKEX/st0x
Length of output: 446
Await process exit instead of arbitrary sleep to prevent port collisions.
kill('SIGTERM') sends a signal but doesn't wait for the process to actually exit. Sleeping 200ms and nulling the reference provides no guarantee the server has released the port before the next test's startPreviewServer() is called, causing flaky EADDRINUSE failures.
The exit handler already exists (lines 46–50) but is never awaited. Replace the fixed sleep with await new Promise((r) => { previewProc.on('exit', r); previewProc.kill('SIGTERM'); }) to deterministically wait for the process to exit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/helpers/previewServer.ts` around lines 55 - 60, In stopPreviewServer(),
don't rely on a fixed 200ms sleep after previewProc.kill('SIGTERM'); instead
attach and await a Promise that resolves on previewProc's 'exit' event (e.g.
previewProc.on('exit', resolve)) before nulling previewProc so the port is
released deterministically; ensure you attach the listener before calling
previewProc.kill() and handle the case where the process may have already exited
(remove listener after resolution or use once) to avoid leaks.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/components/orders/DcaOrder.svelte (1)
192-272:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait the deploy flow before firing success or clearing
trade_id.
transactionStore.handleDcaDeploy(...)is fire-and-forget here, solimit_order_deployedis recorded even for rejected/failed deployments, andclearTradeId()runs before the store can emitsign_trade/broadcast/confirmed. That breaks OBS-09 correlation and makes the catch block miss async failures.Suggested fix
- const handleDcaDeploy = () => { + const handleDcaDeploy = async () => { // Check if user is connected if (!$isAuthenticated) { promptWalletConnection(); return; @@ - transactionStore.handleDcaDeploy( + await transactionStore.handleDcaDeploy( { outputToken: outputTok, inputToken: inputTok, @@ }, { order_type: 'dca' } ); - // Per Assumption A7 (02-RESEARCH): reuse `limit_order_deployed` event - // family for the deploy step — DO NOT introduce `dca_order_deployed`. - // The `order_type: 'dca'` property is the funnel breakdown dimension. trackTradeEvent('limit_order_deployed', { order_type: 'dca', order_side: orderSide.toLowerCase() as 'buy' | 'sell',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/orders/DcaOrder.svelte` around lines 192 - 272, The deploy call is fire-and-forget: transactionStore.handleDcaDeploy(...) must be awaited so async failures are caught and side-effects run in the right order; change the call to await transactionStore.handleDcaDeploy(...) inside the try block, only call trackTradeEvent('limit_order_deployed', ...) after the awaited deploy completes successfully, and move clearTradeId() so it runs after success or in the finally block only when the deploy promise has settled; keep the existing catch that uses classifyDeployError/error.message to report async failures.src/lib/components/orders/MarketOrder.svelte (2)
91-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
tradeSubmittedSuccessfullybefore a new submission.Once this flips to
true, it never goes back tofalse. The hidden success toast can survive later failures/retries, andonDestroywill stop emittingtrade_panel_abandonedfor any subsequent in-progress trade. Clear it when a new submit starts, and ideally when the form is edited again.Also applies to: 980-980, 1003-1007, 1323-1332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/orders/MarketOrder.svelte` at line 91, tradeSubmittedSuccessfully is only ever set true and never reset, causing stale success state and suppressing trade_panel_abandoned emissions; update the submit flow and form-edit handlers to clear it when a new submission starts and when any form field changes: reset tradeSubmittedSuccessfully = false at the start of the submit handler (e.g., submitMarketOrder/handleSubmit) and in the form input change handlers, and adjust the onDestroy/abandon logic to only skip emitting trade_panel_abandoned if tradeSubmittedSuccessfully is true at destroy time.
904-931:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit
trade_failedfor these post-click preflight exits.After
mintTradeId()andtrade_button_clicked, the token-config, stale-refetch, and no-quotes returns all exit silently. Those attempts never get a terminaltrade_failedwith the activetrade_id, so the funnel and correlation data undercount exactly the submit failures this PR is meant to observe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/orders/MarketOrder.svelte` around lines 904 - 931, Silent early exits after mintTradeId() and the trade_button_clicked path (token config checks, stale-refetch branch, and empty filteredQuotes) must emit a terminal trade_failed event with the active trade id and a failure reason before returning; find the early-return spots in MarketOrder.svelte around the token validation (checks that set orderPreparationError), the stale quotes refetch block that calls $orderbookQuotesQuery?.refetch() and fetchMarketPrice(), and the no-quotes branch using getQuotesWithPriceGuard(), and add a call to the same instrumentation used for trade events (the code path that emitted trade_button_clicked / the minted trade id from mintTradeId()) to emit trade_failed including trade_id and a short reason (e.g., token_config_error, stale_refetch_failed, no_quotes) immediately before setting orderPreparationError/priceError/priceErrorReason and returning; also ensure the priceError path after fetchMarketPrice() likewise emits trade_failed with the active trade id when priceError is true.src/lib/stores/deployTransactionStore.ts (1)
355-391:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReturn the deployment promise through the confirmation path.
showRainlangConfirmation()fire-and-forgetshandleStrategyDeployment(...), sohandleLimitDeploy()/handleDcaDeploy()resolve as soon as prep finishes, not when the deploy actually succeeds or fails. That makes the new callers cleartrade_id, emit success, and miss deploy failures while the transaction is still in flight.🛠️ Suggested direction
-export const showRainlangConfirmation = ( +export const showRainlangConfirmation = ( composedRainlang: string, deploymentArgs: DeploymentTransactionArgs, assetTokenInfo?: AssetTokenInfo, eventContext?: DeployEventContext ) => { const shouldReview = get(reviewStrategyOnDeploy); if (shouldReview) { - rainlangConfirmationModal.set({ - show: true, - rainlangCode: composedRainlang, - onDeploy: () => { - rainlangConfirmationModal.set({ - show: false, - rainlangCode: '', - onDeploy: null, - onCancel: null - }); - handleStrategyDeployment(deploymentArgs, assetTokenInfo, eventContext); - }, - onCancel: () => { - rainlangConfirmationModal.set({ - show: false, - rainlangCode: '', - onDeploy: null, - onCancel: null - }); - reset(); - } - }); + return new Promise((resolve, reject) => { + rainlangConfirmationModal.set({ + show: true, + rainlangCode: composedRainlang, + onDeploy: async () => { + rainlangConfirmationModal.set({ + show: false, + rainlangCode: '', + onDeploy: null, + onCancel: null + }); + try { + resolve(await handleStrategyDeployment(deploymentArgs, assetTokenInfo, eventContext)); + } catch (error) { + reject(error); + } + }, + onCancel: () => { + rainlangConfirmationModal.set({ + show: false, + rainlangCode: '', + onDeploy: null, + onCancel: null + }); + reset(); + reject(new Error('Deploy cancelled')); + } + }); + }); } else { - handleStrategyDeployment(deploymentArgs, assetTokenInfo, eventContext); + return handleStrategyDeployment(deploymentArgs, assetTokenInfo, eventContext); } }; @@ - showRainlangConfirmation(composedRainlang, deploymentArgs, assetTokenInfo, eventContext); + return showRainlangConfirmation(composedRainlang, deploymentArgs, assetTokenInfo, eventContext);Also applies to: 404-460
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/stores/deployTransactionStore.ts` around lines 355 - 391, showRainlangConfirmation currently fire-and-forgets handleStrategyDeployment when the confirmation modal path is taken, so callers like handleLimitDeploy/handleDcaDeploy can't await the actual deployment result; change showRainlangConfirmation to return the deployment Promise: have it return handleStrategyDeployment(...) in the "else" branch and, for the modal branch, create and return a Promise that calls handleStrategyDeployment(...) inside the onDeploy handler and resolves/rejects based on that returned Promise (also ensure you still close the modal and call reset() on cancel and reject the Promise on cancel), referencing rainlangConfirmationModal, showRainlangConfirmation, and handleStrategyDeployment so callers can await the real deploy outcome.
🧹 Nitpick comments (3)
.planning/phases/02-observability-for-transacting-users/deferred-items.md (1)
11-11: ⚡ Quick winMake the deferred build-secret requirement fully explicit.
Replacing
etc.with either the full required env var list (or a link to the canonical env schema/source) will make this follow-up item unambiguous for whoever picks it up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/phases/02-observability-for-transacting-users/deferred-items.md at line 11, Update the deferred-items note to explicitly list the production-only environment variables required during the SvelteKit analyse/post-build step (replace "etc." with the actual keys) — at minimum include SESSION_SECRET and BASE_RPC_URL and any other vars referenced by src/lib/server/auth.ts and src/lib/server/accessCodes.ts — or replace "etc." with a single authoritative link to the canonical environment schema/source that enumerates all required production secrets; ensure the text clearly states these are only needed for the production analyse step invoked by `npm run build` so consumers know why they must be provided..planning/phases/02-observability-for-transacting-users/02-01-PLAN.md (1)
154-184: 💤 Low valueConsider using fenced code blocks.
The code samples use indented format, which triggered markdownlint warnings. For better consistency and syntax highlighting, consider using triple-backtick fenced code blocks with language identifiers.
♻️ Example conversion to fenced format
Instead of indented blocks, use:
- ```typescript - import * as Sentry from '@sentry/sveltekit'; - ... - ``` +```typescript +import * as Sentry from '@sentry/sveltekit'; +... +```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/phases/02-observability-for-transacting-users/02-01-PLAN.md around lines 154 - 184, The markdown uses indented code which triggers markdownlint; replace the indented sample with a fenced code block using triple backticks and a language tag (e.g., ```typescript) around the example that contains TRADE_ID_HEADER, mintTradeId, getCurrentTradeId, and clearTradeId so the snippet is rendered and syntax-highlighted correctly; ensure you open with ```typescript and close with ``` and remove the leading indentation from the code lines.tests/lib/components/orders/LimitOrder.events.test.ts (1)
52-106: ⚡ Quick winThese assertions are too weak to guard the async lifecycle.
L4andL9only prove that certain strings exist somewhere in the source. They still pass when the deploy path is fire-and-forget andclearTradeId()runs before the deploy resolves, so this suite misses exactly the regression this PR is trying to prevent. Please replace the critical checks with a rendered component test that clicks the button and assertstrackTradeEvent/clearTradeIdordering against mocks. As per coding guidelines, "**/tests/**/*.{test,spec}.{ts,svelte}: Write component and logic tests using Vitest with jsdom environment and@testing-library/sveltefor component tests`."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/lib/components/orders/LimitOrder.events.test.ts` around lines 52 - 106, The current tests in LimitOrder.events.test.ts only assert source strings and miss async lifecycle regressions; replace the L4/L9 string-based checks with a rendered component test that mounts the LimitOrder Svelte component (using Vitest + jsdom + `@testing-library/svelte`), mock trackTradeEvent and clearTradeId (and any transactionStore.handleLimitDeploy call), simulate the user clicking the deploy button, then assert the mock call ordering (e.g., trackTradeEvent called with trade_failed / trade_panel events and clearTradeId is called only after the deploy promise settles) using async helpers like waitFor and fireEvent to reliably capture the async flow; also update the proceedWithDeploy/cancelDeploy coverage by rendering UI paths that trigger those functions and asserting they call clearTradeId via the same mocked spies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.planning/phases/02-observability-for-transacting-users/02-03-PLAN.md:
- Around line 79-80: Summary: The output summary incorrectly states "5 source
files modified" while the breakdown lists 3 components + 1 route + 2 services (6
files). Fix: update the summary count from 5 to 6 wherever the phrase "Output: 5
source files modified" appears in this document and ensure any related
"files_modified" summary or variables reflect 6; specifically edit the line
containing the Output summary and any adjacent summary text to read "Output: 6
source files modified (3 components + 1 route + 2 services)" so the total
matches the listed items.
In @.planning/phases/02-observability-for-transacting-users/02-04-PLAN.md:
- Around line 10-11: The plan currently allows two mutually exclusive artifact
exports (funnel-market.json / funnel-limit.json) while some places expect a
single combined funnel-trade.json; pick one canonical contract (either the two
separate artifacts or the single funnel-trade.json), update the front matter
export list and Task 3 verification text to require that single chosen artifact,
and then search and replace every reference to funnel-market.json,
funnel-limit.json, and funnel-trade.json in this document (including the
sections referencing Task 3, verification steps, and the front-matter export
list) so all mentions consistently require the chosen artifact name and shape.
In @.planning/phases/02-observability-for-transacting-users/02-RUNBOOK.md:
- Around line 44-47: The markdown has multiple fenced code blocks without
language identifiers (MD040); update each opening fence from ``` to ```text for
the screenshot/note blocks so they become fenced as language "text" (e.g., the
blocks containing [SCREENSHOT-1], [SCREENSHOT-2], [SCREENSHOT-3], the "Sentry
Replay essential-tool stance" block, and the [SCREENSHOT-4a]..[SCREENSHOT-4c]
blocks referenced in
.planning/phases/02-observability-for-transacting-users/02-RUNBOOK.md and the
other ranges (80-83, 153-157, 179-182, 225-239) so markdownlint no longer
errors).
In `@src/lib/services/orderDeployment.ts`:
- Around line 33-35: DeployEventContext currently only carries order_type and
the code re-derives UI-facing symbols from deployment-side args.inputToken /
args.outputToken when emitting asset_symbol / payment_symbol, which inverts
symbols for sell-side flows; update DeployEventContext to include explicit
UI-side symbols (e.g., asset_symbol and payment_symbol or
uiInputSymbol/uiOutputSymbol), pass those through from the component that
constructs the deployment, and change all emitters that currently map
args.inputToken/outputToken to use the new DeployEventContext symbols instead
(notably where sign_trade and component-level events are emitted); apply the
same change to the other occurrences flagged (the other two emit blocks that map
args.inputToken/outputToken) so all event emission consistently uses the UI
symbols.
In `@tests/lib/components/orders/MarketOrder.events.test.ts`:
- Around line 28-43: The test currently reimplements the classifyMarketError
helper (function classifyMarketError) instead of using the component's real
implementation; extract that helper from MarketOrder.svelte into a dedicated
TypeScript module (e.g., export function classifyMarketError(...)) and replace
the local copy in tests/lib/components/orders/MarketOrder.events.test.ts with an
import of that exported function, or alternatively change Test 5 to read the
MarketOrder.svelte source and assert the presence/ordering of the function
rather than duplicating it; update any references in the test (the local
classifyMarketError and the current source check around the component name) to
use the imported helper or the direct source assertion so the test fails if the
component helper changes.
In `@tests/lib/services/orderDeployment.events.test.ts`:
- Around line 90-92: The test 'Test P2: trackPageView call passes token_id from
$page.params.id' currently only asserts presence of token_id in tradePageSource;
update the expectation to assert the actual source expression by matching the
specific '$page.params.id' access (or the local variable name that is assigned
from it) in the emitted code. Concretely, replace the regex in the
expect(tradePageSource).toMatch(...) check to include the page-param pattern
(for example /\btoken_id:\s*\$page\.params\.id\b/ or
/\btoken_id:\s*tokenIdFromPage\b/ depending on whether the code assigns to a
local like tokenIdFromPage), so the test ensures token_id is wired to
$page.params.id rather than any constant.
---
Outside diff comments:
In `@src/lib/components/orders/DcaOrder.svelte`:
- Around line 192-272: The deploy call is fire-and-forget:
transactionStore.handleDcaDeploy(...) must be awaited so async failures are
caught and side-effects run in the right order; change the call to await
transactionStore.handleDcaDeploy(...) inside the try block, only call
trackTradeEvent('limit_order_deployed', ...) after the awaited deploy completes
successfully, and move clearTradeId() so it runs after success or in the finally
block only when the deploy promise has settled; keep the existing catch that
uses classifyDeployError/error.message to report async failures.
In `@src/lib/components/orders/MarketOrder.svelte`:
- Line 91: tradeSubmittedSuccessfully is only ever set true and never reset,
causing stale success state and suppressing trade_panel_abandoned emissions;
update the submit flow and form-edit handlers to clear it when a new submission
starts and when any form field changes: reset tradeSubmittedSuccessfully = false
at the start of the submit handler (e.g., submitMarketOrder/handleSubmit) and in
the form input change handlers, and adjust the onDestroy/abandon logic to only
skip emitting trade_panel_abandoned if tradeSubmittedSuccessfully is true at
destroy time.
- Around line 904-931: Silent early exits after mintTradeId() and the
trade_button_clicked path (token config checks, stale-refetch branch, and empty
filteredQuotes) must emit a terminal trade_failed event with the active trade id
and a failure reason before returning; find the early-return spots in
MarketOrder.svelte around the token validation (checks that set
orderPreparationError), the stale quotes refetch block that calls
$orderbookQuotesQuery?.refetch() and fetchMarketPrice(), and the no-quotes
branch using getQuotesWithPriceGuard(), and add a call to the same
instrumentation used for trade events (the code path that emitted
trade_button_clicked / the minted trade id from mintTradeId()) to emit
trade_failed including trade_id and a short reason (e.g., token_config_error,
stale_refetch_failed, no_quotes) immediately before setting
orderPreparationError/priceError/priceErrorReason and returning; also ensure the
priceError path after fetchMarketPrice() likewise emits trade_failed with the
active trade id when priceError is true.
In `@src/lib/stores/deployTransactionStore.ts`:
- Around line 355-391: showRainlangConfirmation currently fire-and-forgets
handleStrategyDeployment when the confirmation modal path is taken, so callers
like handleLimitDeploy/handleDcaDeploy can't await the actual deployment result;
change showRainlangConfirmation to return the deployment Promise: have it return
handleStrategyDeployment(...) in the "else" branch and, for the modal branch,
create and return a Promise that calls handleStrategyDeployment(...) inside the
onDeploy handler and resolves/rejects based on that returned Promise (also
ensure you still close the modal and call reset() on cancel and reject the
Promise on cancel), referencing rainlangConfirmationModal,
showRainlangConfirmation, and handleStrategyDeployment so callers can await the
real deploy outcome.
---
Nitpick comments:
In @.planning/phases/02-observability-for-transacting-users/02-01-PLAN.md:
- Around line 154-184: The markdown uses indented code which triggers
markdownlint; replace the indented sample with a fenced code block using triple
backticks and a language tag (e.g., ```typescript) around the example that
contains TRADE_ID_HEADER, mintTradeId, getCurrentTradeId, and clearTradeId so
the snippet is rendered and syntax-highlighted correctly; ensure you open with
```typescript and close with ``` and remove the leading indentation from the
code lines.
In @.planning/phases/02-observability-for-transacting-users/deferred-items.md:
- Line 11: Update the deferred-items note to explicitly list the production-only
environment variables required during the SvelteKit analyse/post-build step
(replace "etc." with the actual keys) — at minimum include SESSION_SECRET and
BASE_RPC_URL and any other vars referenced by src/lib/server/auth.ts and
src/lib/server/accessCodes.ts — or replace "etc." with a single authoritative
link to the canonical environment schema/source that enumerates all required
production secrets; ensure the text clearly states these are only needed for the
production analyse step invoked by `npm run build` so consumers know why they
must be provided.
In `@tests/lib/components/orders/LimitOrder.events.test.ts`:
- Around line 52-106: The current tests in LimitOrder.events.test.ts only assert
source strings and miss async lifecycle regressions; replace the L4/L9
string-based checks with a rendered component test that mounts the LimitOrder
Svelte component (using Vitest + jsdom + `@testing-library/svelte`), mock
trackTradeEvent and clearTradeId (and any transactionStore.handleLimitDeploy
call), simulate the user clicking the deploy button, then assert the mock call
ordering (e.g., trackTradeEvent called with trade_failed / trade_panel events
and clearTradeId is called only after the deploy promise settles) using async
helpers like waitFor and fireEvent to reliably capture the async flow; also
update the proceedWithDeploy/cancelDeploy coverage by rendering UI paths that
trigger those functions and asserting they call clearTradeId via the same mocked
spies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cff23e6-c6e0-4ba0-9cdd-fcc75c3ba5dc
📒 Files selected for processing (46)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/02-observability-for-transacting-users/02-01-PLAN.md.planning/phases/02-observability-for-transacting-users/02-01-SUMMARY.md.planning/phases/02-observability-for-transacting-users/02-02-PLAN.md.planning/phases/02-observability-for-transacting-users/02-02-SUMMARY.md.planning/phases/02-observability-for-transacting-users/02-03-PLAN.md.planning/phases/02-observability-for-transacting-users/02-03-SUMMARY.md.planning/phases/02-observability-for-transacting-users/02-04-PLAN.md.planning/phases/02-observability-for-transacting-users/02-04-SUMMARY.md.planning/phases/02-observability-for-transacting-users/02-CONTEXT.md.planning/phases/02-observability-for-transacting-users/02-DISCUSSION-LOG.md.planning/phases/02-observability-for-transacting-users/02-PRIVACY-REVIEW.md.planning/phases/02-observability-for-transacting-users/02-RESEARCH.md.planning/phases/02-observability-for-transacting-users/02-RUNBOOK.md.planning/phases/02-observability-for-transacting-users/02-VALIDATION.md.planning/phases/02-observability-for-transacting-users/artifacts/.gitkeep.planning/phases/02-observability-for-transacting-users/deferred-items.mdsrc/hooks.client.tssrc/hooks.server.tssrc/lib/components/orders/DcaOrder.sveltesrc/lib/components/orders/LimitOrder.sveltesrc/lib/components/orders/MarketOrder.sveltesrc/lib/server/csp.tssrc/lib/server/logger.tssrc/lib/services/marketOrderExecution.tssrc/lib/services/observability/captureTakeOrderFailure.tssrc/lib/services/observability/tradeEvents.tssrc/lib/services/observability/tradeId.tssrc/lib/services/orderDeployment.tssrc/lib/stores/deployTransactionStore.tssrc/routes/(main)/trade/[id]/+page.sveltetests/lib/components/orders/DcaOrder.events.test.tstests/lib/components/orders/LimitOrder.events.test.tstests/lib/components/orders/MarketOrder.events.test.tstests/lib/observability/sentryReplayConfig.test.tstests/lib/server/csp.test.tstests/lib/server/logger.tradeId.test.tstests/lib/services/marketOrderExecution.events.test.tstests/lib/services/observability/captureTakeOrderFailure.test.tstests/lib/services/observability/tradeEvents.privacy.test.tstests/lib/services/observability/tradeEvents.test.tstests/lib/services/observability/tradeId.test.tstests/lib/services/orderDeployment.events.test.tstests/lib/transactionStore.test.ts
✅ Files skipped from review due to trivial changes (6)
- tests/lib/server/csp.test.ts
- .planning/phases/02-observability-for-transacting-users/02-VALIDATION.md
- .planning/phases/02-observability-for-transacting-users/02-DISCUSSION-LOG.md
- .planning/phases/02-observability-for-transacting-users/02-04-SUMMARY.md
- .planning/phases/02-observability-for-transacting-users/02-02-SUMMARY.md
- .planning/ROADMAP.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .planning/REQUIREMENTS.md
- src/routes/(main)/trade/[id]/+page.svelte
Comment on lines
+79
to
+80
| Output: 5 source files modified (3 components + 1 route + 2 services); 5 test files (3 component + 2 service-level). | ||
| </objective> |
There was a problem hiding this comment.
Fix the source-file count in the output summary.
3 components + 1 route + 2 services is 6 source files, and files_modified also lists six source entries. The current “5 source files modified” summary contradicts the rest of the plan.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/02-observability-for-transacting-users/02-03-PLAN.md around
lines 79 - 80, Summary: The output summary incorrectly states "5 source files
modified" while the breakdown lists 3 components + 1 route + 2 services (6
files). Fix: update the summary count from 5 to 6 wherever the phrase "Output: 5
source files modified" appears in this document and ensure any related
"files_modified" summary or variables reflect 6; specifically edit the line
containing the Output summary and any adjacent summary text to read "Output: 6
source files modified (3 components + 1 route + 2 services)" so the total
matches the listed items.
Comment on lines
+10
to
+11
| - .planning/phases/02-observability-for-transacting-users/artifacts/funnel-market.json | ||
| - .planning/phases/02-observability-for-transacting-users/artifacts/funnel-limit.json |
There was a problem hiding this comment.
Pick one funnel-artifact contract.
This plan currently allows either two exports (funnel-market.json / funnel-limit.json) or one combined funnel-trade.json, but the front matter and later verification steps don't agree on which one is required. That makes Task 3 ambiguous and can leave the blocking verification in a “done but unverifiable” state. Standardize on one output shape and update every reference to match it.
Also applies to: 142-143, 320-321, 394-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/02-observability-for-transacting-users/02-04-PLAN.md around
lines 10 - 11, The plan currently allows two mutually exclusive artifact exports
(funnel-market.json / funnel-limit.json) while some places expect a single
combined funnel-trade.json; pick one canonical contract (either the two separate
artifacts or the single funnel-trade.json), update the front matter export list
and Task 3 verification text to require that single chosen artifact, and then
search and replace every reference to funnel-market.json, funnel-limit.json, and
funnel-trade.json in this document (including the sections referencing Task 3,
verification steps, and the front-matter export list) so all mentions
consistently require the chosen artifact name and shape.
Comment on lines
+44
to
+47
| ``` | ||
| [SCREENSHOT-1: Sentry project Replay settings page showing the Session Replay | ||
| toggle ON for the st0x project. Add the redacted screenshot here once captured.] | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy markdownlint (MD040).
These blocks should specify a language (e.g., text) to clear CI/documentation lint noise.
Suggested patch
-```
+```text
[SCREENSHOT-1: Sentry project Replay settings page showing the Session Replay
toggle ON for the st0x project. Add the redacted screenshot here once captured.]- +text
[SCREENSHOT-2: PostHog Settings → Replay page showing Session sample rate 0.05
and maskAllInputs reported as active. Add the redacted screenshot here.]
-```
+```text
[SCREENSHOT-3: PostHog dashboard "Trade Funnels — Phase 2 Observability"
rendering Funnel A and Funnel B side-by-side, each with the order_type breakdown
populated by real events from a smoke trade. Add the redacted screenshot here.]
- +text
Sentry Replay essential-tool stance approved by: ________________
Date: ____________
-```
+```text
[SCREENSHOT-4a: Sentry event detail page for the failure (or attached Replay
for an intentional failure). Verify the `trade_id` tag is visible in the tag
panel and the attached Replay loads. Add the redacted screenshot here.]
@@
[SCREENSHOT-4c: Vercel Logs filter showing pino lines with the matching
`trade_id` field. Verify at least one server-side log line carries the same
trade_id (proves X-Trade-Id header propagation works). Add the redacted
screenshot here.]
</details>
Also applies to: 80-83, 153-157, 179-182, 225-239
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/02-observability-for-transacting-users/02-RUNBOOK.md around
lines 44 - 47, The markdown has multiple fenced code blocks without language
identifiers (MD040); update each opening fence from totext for the
screenshot/note blocks so they become fenced as language "text" (e.g., the
blocks containing [SCREENSHOT-1], [SCREENSHOT-2], [SCREENSHOT-3], the "Sentry
Replay essential-tool stance" block, and the [SCREENSHOT-4a]..[SCREENSHOT-4c]
blocks referenced in
.planning/phases/02-observability-for-transacting-users/02-RUNBOOK.md and the
other ranges (80-83, 153-157, 179-182, 225-239) so markdownlint no longer
errors).
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+28
to
+43
| // Mirror of `classifyMarketError` defined inside MarketOrder.svelte. The pure | ||
| // helper is asserted here in test-land so a future drift breaks Test 5 immediately. | ||
| // Source-content check below ensures the function exists in the component file. | ||
| function classifyMarketError(err: unknown): ErrorClass { | ||
| const msg = String((err as { message?: string })?.message ?? err ?? '').toLowerCase(); | ||
| if (msg.includes('slippage')) return 'slippage_exceeded'; | ||
| if (msg.includes('liquidity') || msg.includes('no_walk_fills') || msg.includes('no_quotes')) | ||
| return 'no_liquidity'; | ||
| if (msg.includes('stale') || msg.includes('oracle')) return 'stale_oracle'; | ||
| if (msg.includes('insufficient') || msg.includes('balance')) return 'insufficient_balance'; | ||
| if (msg.includes('market') && msg.includes('closed')) return 'market_closed'; | ||
| if (msg.includes('user reject') || msg.includes('user denied') || msg.includes('rejected')) | ||
| return 'user_rejected'; | ||
| if (msg.includes('rpc') || msg.includes('network')) return 'rpc_error'; | ||
| return 'unknown'; | ||
| } |
There was a problem hiding this comment.
This test copies the classifier instead of verifying it.
Test 5 executes a local reimplementation, so it can keep passing even if the component's branch order or fallback changes. The only coupling to MarketOrder.svelte is the name check on Line 116. Extract the helper into a TS module and import it here, or assert directly on the component source.
Also applies to: 99-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lib/components/orders/MarketOrder.events.test.ts` around lines 28 - 43,
The test currently reimplements the classifyMarketError helper (function
classifyMarketError) instead of using the component's real implementation;
extract that helper from MarketOrder.svelte into a dedicated TypeScript module
(e.g., export function classifyMarketError(...)) and replace the local copy in
tests/lib/components/orders/MarketOrder.events.test.ts with an import of that
exported function, or alternatively change Test 5 to read the MarketOrder.svelte
source and assert the presence/ordering of the function rather than duplicating
it; update any references in the test (the local classifyMarketError and the
current source check around the component name) to use the imported helper or
the direct source assertion so the test fails if the component helper changes.
Comment on lines
+90
to
+92
| it('Test P2: trackPageView call passes token_id from $page.params.id', () => { | ||
| expect(tradePageSource).toMatch(/trackPageView\(\s*['"]trade['"][\s\S]*?token_id:/); | ||
| }); |
There was a problem hiding this comment.
Assert the token_id source, not just its presence.
This still passes if token_id comes from a constant or unrelated variable, so it doesn't actually guard the $page.params.id contract in the test name. Match the specific page-param expression, or the local variable that is derived from it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lib/services/orderDeployment.events.test.ts` around lines 90 - 92, The
test 'Test P2: trackPageView call passes token_id from $page.params.id'
currently only asserts presence of token_id in tradePageSource; update the
expectation to assert the actual source expression by matching the specific
'$page.params.id' access (or the local variable name that is assigned from it)
in the emitted code. Concretely, replace the regex in the
expect(tradePageSource).toMatch(...) check to include the page-param pattern
(for example /\btoken_id:\s*\$page\.params\.id\b/ or
/\btoken_id:\s*tokenIdFromPage\b/ depending on whether the code assigns to a
local like tokenIdFromPage), so the test ensures token_id is wired to
$page.params.id rather than any constant.
alastairong1
force-pushed
the
phase-01-ui-driven-e2e-tests
branch
from
May 15, 2026 08:50
a953e4d to
52de433
Compare
alastairong1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.github/workflows/test.yml (1)
62-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the
test-e2ejob onBASE_RPC_URLsecret.The PR description states this job is "gated on
secrets.BASE_RPC_URL", but there's noif:conditional. Without a gate, fork PRs or repos without the secret will run the job and fail instead of skipping cleanly.🛡️ Proposed fix
test-e2e: runs-on: ubuntu-latest + if: ${{ secrets.BASE_RPC_URL != '' }} steps:Alternatively, gate the individual Playwright steps (lines 96-104 and 106-114) if the job must remain visible for required checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 62 - 64, The test-e2e job is not gated on the BASE_RPC_URL secret so forks/repos without the secret will run and fail; add a conditional to the test-e2e job like if: ${{ secrets.BASE_RPC_URL }} to skip the entire job when the secret is missing, or if you must keep the job visible, add the same if: ${{ secrets.BASE_RPC_URL }} condition to the Playwright test steps (the Playwright-related step entries) so those steps are skipped when the secret is absent.tests/integration/ui/marketFailures.spec.ts (1)
35-35:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
SATURDAY_03_UTCtimestamp does not match the comment.The value
1745550000decodes to Fri 2025-04-25 03:00:00 UTC, not Sat 2026-04-25 as stated in the comment. At03:00 UTCon Friday, the ET timezone would show23:00 EDTon Thursday (or Friday depending on DST), meaningdayOfWeekin ET would be 4 or 5, not 6 (Saturday).The market-closed test currently passes because Friday late evening is also outside market hours, but for the wrong reason. If
marketHours.tsever differentiates weekend-closed from weekday-after-hours, this test will break.🛠️ Proposed fix — Use Saturday daytime in ET
-const SATURDAY_03_UTC = 1745550000; // Sat 2026-04-25 03:00:00 UTC — 01-RUNBOOK §"Saturday market-hours timestamp" +// Sat 2026-04-25 16:00:00 UTC = Sat 12:00 EDT — unambiguously weekend in ET. +const SATURDAY_NOON_ET_UTC = 1_777_132_800;Then update references on lines 190 and 192 to use
SATURDAY_NOON_ET_UTC.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/ui/marketFailures.spec.ts` at line 35, The timestamp constant SATURDAY_03_UTC is wrong for the intended Saturday 2026 ET test; replace it with a new constant SATURDAY_NOON_ET_UTC set to the Unix seconds for 2026-04-25 12:00:00 ET (which is 2026-04-25 16:00:00 UTC -> 1777219200) and update all test references that currently use SATURDAY_03_UTC to use SATURDAY_NOON_ET_UTC (rename usages and the constant in the tests/integration UI file so the test uses Saturday daytime in ET).
🧹 Nitpick comments (2)
src/lib/components/orders/DcaOrder.svelte (1)
203-203: 💤 Low valueRedundant guard check.
Lines 194-202 already return early if
!$isAuthenticatedor!$walletRegistered. The check at line 203 is redundant.♻️ Optional simplification
- if (!($isAuthenticated && $walletRegistered)) return; - mintTradeId();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/orders/DcaOrder.svelte` at line 203, Remove the redundant guard "if (!($isAuthenticated && $walletRegistered)) return;" in DcaOrder.svelte because the same checks for $isAuthenticated and $walletRegistered are already performed and return earlier (lines 194-202); locate the block containing $isAuthenticated and $walletRegistered (e.g., the submit/create order handler) and delete this duplicate check to simplify the control flow.tests/integration/ui/smoke.spec.ts (1)
15-15: ⚡ Quick winRemove unused imports.
TOKENSandFUNDED_ACCOUNTare imported but never used. The test uses the lowercase fixture versions (tokens,fundedAccount) instead.♻️ Proposed fix
-import { test, expect, fundErc20, TOKENS, FUNDED_ACCOUNT } from './fixtures'; +import { test, expect, fundErc20 } from './fixtures';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/ui/smoke.spec.ts` at line 15, The import line in smoke.spec.ts includes unused symbols TOKENS and FUNDED_ACCOUNT; remove these two from the import list and keep only the actual fixtures used (test, expect, fundErc20, tokens, fundedAccount) so the file imports the lowercase fixture names referenced by the tests (verify usage of tokens and fundedAccount in the test body and update the import accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 65-68: Update the GitHub Actions checkout step to use the
maintained version by replacing uses: actions/checkout@v2 with uses:
actions/checkout@v4 wherever it appears (the checkout steps in the workflow,
including the instances referenced around lines 8, 32, 65–68, and 119), and keep
the existing with: submodules: recursive and fetch-depth: 0 configuration
unchanged to preserve behavior.
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-RESEARCH.md:
- Line 865: The table row's mitigation text contains an unescaped pipe in the
expression "assert(!env.E2E || dev)" which breaks Markdown table parsing; update
the cell to escape the pipe or wrap the expression in inline code (e.g.,
backticks or HTML <code>) so the full row renders correctly and keep the same
wording referencing the `E2E=1`/`01-RUNBOOK` mitigation; specifically locate the
string containing "assert(!env.E2E || dev)" and replace it with an escaped pipe
or a code-wrapped variant to fix rendering.
In `@src/lib/services/observability/tradeId.ts`:
- Around line 41-48: The clearTradeId function currently uses an unsupported
workaround by calling Sentry.setTag('trade_id', undefined as unknown as string);
remove that call and instead reset Sentry's scope explicitly — replace the
setTag invocation with Sentry.configureScope(scope => scope.clear()) (or
alternatively remove all Sentry interaction here and ensure callers use
Sentry.withScope()/withIsolationScope() when setting the 'trade_id'); keep
current = null in clearTradeId and ensure Sentry is imported/available for
configureScope.
---
Duplicate comments:
In @.github/workflows/test.yml:
- Around line 62-64: The test-e2e job is not gated on the BASE_RPC_URL secret so
forks/repos without the secret will run and fail; add a conditional to the
test-e2e job like if: ${{ secrets.BASE_RPC_URL }} to skip the entire job when
the secret is missing, or if you must keep the job visible, add the same if: ${{
secrets.BASE_RPC_URL }} condition to the Playwright test steps (the
Playwright-related step entries) so those steps are skipped when the secret is
absent.
In `@tests/integration/ui/marketFailures.spec.ts`:
- Line 35: The timestamp constant SATURDAY_03_UTC is wrong for the intended
Saturday 2026 ET test; replace it with a new constant SATURDAY_NOON_ET_UTC set
to the Unix seconds for 2026-04-25 12:00:00 ET (which is 2026-04-25 16:00:00 UTC
-> 1777219200) and update all test references that currently use SATURDAY_03_UTC
to use SATURDAY_NOON_ET_UTC (rename usages and the constant in the
tests/integration UI file so the test uses Saturday daytime in ET).
---
Nitpick comments:
In `@src/lib/components/orders/DcaOrder.svelte`:
- Line 203: Remove the redundant guard "if (!($isAuthenticated &&
$walletRegistered)) return;" in DcaOrder.svelte because the same checks for
$isAuthenticated and $walletRegistered are already performed and return earlier
(lines 194-202); locate the block containing $isAuthenticated and
$walletRegistered (e.g., the submit/create order handler) and delete this
duplicate check to simplify the control flow.
In `@tests/integration/ui/smoke.spec.ts`:
- Line 15: The import line in smoke.spec.ts includes unused symbols TOKENS and
FUNDED_ACCOUNT; remove these two from the import list and keep only the actual
fixtures used (test, expect, fundErc20, tokens, fundedAccount) so the file
imports the lowercase fixture names referenced by the tests (verify usage of
tokens and fundedAccount in the test body and update the import accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 101304a2-f44b-46cc-b8d3-1ac531d574e2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (87)
.github/workflows/test.yml.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/codebase/TESTING.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-01-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-01-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-02-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-02-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-03-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-03-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-04-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-04-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-05-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-05-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-06-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-06-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-07-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-07-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-08-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-08-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-09-PLAN.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-09-SUMMARY.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-AUDIT.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-PATTERNS.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-RESEARCH.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-RUNBOOK.md.planning/phases/01-ui-driven-e2e-order-test-coverage/01-VALIDATION.md.planning/phases/02-observability-for-transacting-users/02-01-PLAN.md.planning/phases/02-observability-for-transacting-users/02-01-SUMMARY.md.planning/phases/02-observability-for-transacting-users/02-02-PLAN.md.planning/phases/02-observability-for-transacting-users/02-02-SUMMARY.md.planning/phases/02-observability-for-transacting-users/02-03-PLAN.md.planning/phases/02-observability-for-transacting-users/02-03-SUMMARY.md.planning/phases/02-observability-for-transacting-users/02-04-PLAN.md.planning/phases/02-observability-for-transacting-users/02-04-SUMMARY.md.planning/phases/02-observability-for-transacting-users/02-CONTEXT.md.planning/phases/02-observability-for-transacting-users/02-DISCUSSION-LOG.md.planning/phases/02-observability-for-transacting-users/02-PRIVACY-REVIEW.md.planning/phases/02-observability-for-transacting-users/02-RESEARCH.md.planning/phases/02-observability-for-transacting-users/02-RUNBOOK.md.planning/phases/02-observability-for-transacting-users/02-VALIDATION.md.planning/phases/02-observability-for-transacting-users/artifacts/.gitkeep.planning/phases/02-observability-for-transacting-users/deferred-items.mdeslint.config.jspackage.jsonplaywright.config.tssrc/hooks.client.tssrc/hooks.server.tssrc/lib/components/orders/DcaOrder.sveltesrc/lib/components/orders/LimitOrder.sveltesrc/lib/components/orders/MarketOrder.sveltesrc/lib/server/csp.tssrc/lib/server/logger.tssrc/lib/services/marketOrderExecution.tssrc/lib/services/observability/captureTakeOrderFailure.tssrc/lib/services/observability/tradeEvents.tssrc/lib/services/observability/tradeId.tssrc/lib/services/orderDeployment.tssrc/lib/stores/deployTransactionStore.tssrc/routes/(main)/trade/[id]/+page.sveltetests/fixtures/eslint/ui-test-import-violation.tstests/helpers/anvilControl.tstests/helpers/eip1193Stub.tstests/helpers/previewServer.tstests/integration/ui/fixtures.tstests/integration/ui/globalSetup.tstests/integration/ui/globalTeardown.tstests/integration/ui/limitDeploy.spec.tstests/integration/ui/marketBuy.spec.tstests/integration/ui/marketFailures.spec.tstests/integration/ui/marketSell.spec.tstests/integration/ui/smoke.spec.tstests/lib/components/orders/DcaOrder.events.test.tstests/lib/components/orders/LimitOrder.events.test.tstests/lib/components/orders/MarketOrder.events.test.tstests/lib/observability/sentryReplayConfig.test.tstests/lib/server/csp.test.tstests/lib/server/logger.tradeId.test.tstests/lib/services/marketOrderExecution.events.test.tstests/lib/services/observability/captureTakeOrderFailure.test.tstests/lib/services/observability/tradeEvents.privacy.test.tstests/lib/services/observability/tradeEvents.test.tstests/lib/services/observability/tradeId.test.tstests/lib/services/orderDeployment.events.test.tstests/lib/transactionStore.test.tstests/lib/utils/marketHours.test.ts
✅ Files skipped from review due to trivial changes (14)
- tests/lib/server/csp.test.ts
- .planning/REQUIREMENTS.md
- .planning/ROADMAP.md
- .planning/phases/02-observability-for-transacting-users/02-DISCUSSION-LOG.md
- .planning/phases/01-ui-driven-e2e-order-test-coverage/01-05-SUMMARY.md
- .planning/phases/01-ui-driven-e2e-order-test-coverage/01-02-SUMMARY.md
- .planning/phases/01-ui-driven-e2e-order-test-coverage/01-06-SUMMARY.md
- .planning/phases/01-ui-driven-e2e-order-test-coverage/01-AUDIT.md
- .planning/phases/02-observability-for-transacting-users/02-04-SUMMARY.md
- .planning/STATE.md
- .planning/phases/02-observability-for-transacting-users/02-VALIDATION.md
- .planning/phases/01-ui-driven-e2e-order-test-coverage/01-04-SUMMARY.md
- .planning/phases/02-observability-for-transacting-users/02-02-SUMMARY.md
- .planning/phases/02-observability-for-transacting-users/02-04-PLAN.md
🚧 Files skipped from review as they are similar to previous changes (33)
- package.json
- src/lib/services/marketOrderExecution.ts
- src/lib/services/observability/captureTakeOrderFailure.ts
- tests/integration/ui/globalTeardown.ts
- tests/integration/ui/marketBuy.spec.ts
- tests/lib/transactionStore.test.ts
- tests/lib/server/logger.tradeId.test.ts
- tests/lib/services/marketOrderExecution.events.test.ts
- tests/helpers/previewServer.ts
- tests/lib/components/orders/DcaOrder.events.test.ts
- src/routes/(main)/trade/[id]/+page.svelte
- tests/integration/ui/limitDeploy.spec.ts
- tests/integration/ui/globalSetup.ts
- tests/lib/components/orders/LimitOrder.events.test.ts
- src/lib/services/observability/tradeEvents.ts
- tests/lib/services/orderDeployment.events.test.ts
- src/lib/services/orderDeployment.ts
- tests/lib/services/observability/tradeEvents.test.ts
- src/lib/server/csp.ts
- src/hooks.client.ts
- playwright.config.ts
- tests/lib/observability/sentryReplayConfig.test.ts
- src/hooks.server.ts
- tests/lib/components/orders/MarketOrder.events.test.ts
- src/lib/server/logger.ts
- tests/lib/services/observability/tradeEvents.privacy.test.ts
- eslint.config.js
- tests/lib/utils/marketHours.test.ts
- tests/integration/ui/marketSell.spec.ts
- tests/lib/services/observability/tradeId.test.ts
- src/lib/stores/deployTransactionStore.ts
- src/lib/components/orders/LimitOrder.svelte
- src/lib/components/orders/MarketOrder.svelte
|
|
||
| | Pattern | STRIDE | Standard Mitigation | | ||
| |---------|--------|---------------------| | ||
| | Test env-var leakage to prod (`E2E=1` enabling CSP relaxation in prod) | Tampering | `E2E=1` is set ONLY by Playwright globalSetup; Vercel build pipeline never sets it. Document in 01-RUNBOOK and add an `assert(!env.E2E || dev)` guard pattern if paranoia warrants. | |
There was a problem hiding this comment.
Escape pipe characters inside the table cell to avoid broken rendering.
The inline expression at Line 865 includes ||, which is parsed as table delimiters and breaks the row. Escape pipes (or wrap with HTML code tags) so the mitigation text renders fully.
Suggested fix
-| Test env-var leakage to prod (`E2E=1` enabling CSP relaxation in prod) | Tampering | `E2E=1` is set ONLY by Playwright globalSetup; Vercel build pipeline never sets it. Document in 01-RUNBOOK and add an `assert(!env.E2E || dev)` guard pattern if paranoia warrants. |
+| Test env-var leakage to prod (`E2E=1` enabling CSP relaxation in prod) | Tampering | `E2E=1` is set ONLY by Playwright globalSetup; Vercel build pipeline never sets it. Document in 01-RUNBOOK and add an `assert(!env.E2E \|\| dev)` guard pattern if paranoia warrants. |🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 865-865: Table column count
Expected: 3; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/phases/01-ui-driven-e2e-order-test-coverage/01-RESEARCH.md at line
865, The table row's mitigation text contains an unescaped pipe in the
expression "assert(!env.E2E || dev)" which breaks Markdown table parsing; update
the cell to escape the pipe or wrap the expression in inline code (e.g.,
backticks or HTML <code>) so the full row renders correctly and keep the same
wording referencing the `E2E=1`/`01-RUNBOOK` mitigation; specifically locate the
string containing "assert(!env.E2E || dev)" and replace it with an escaped pipe
or a code-wrapped variant to fix rendering.
Comment on lines
+41
to
+48
| export function clearTradeId(): void { | ||
| current = null; | ||
| try { | ||
| Sentry.setTag('trade_id', undefined as unknown as string); | ||
| } catch (err) { | ||
| console.error('[tradeId] Sentry.setTag clear failed:', err); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the correct API for clearing Sentry tags in `@sentry/sveltekit`
rg -nP --type=ts -C3 'setTag.*null|setTag.*undefined|configureScope' node_modules/@sentry/Repository: SARKEX/st0x
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check Sentry type definitions for setTag
find node_modules/@sentry -name "*.d.ts" -type f | head -20Repository: SARKEX/st0x
Length of output: 1846
🏁 Script executed:
#!/bin/bash
# Look for setTag in Sentry's type definitions
rg -A3 'setTag\s*\(' node_modules/@sentry --type=ts --type=d.ts | head -50Repository: SARKEX/st0x
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Check how Sentry's setTag is used elsewhere in the codebase
rg 'setTag' src/ --type=ts -B2 -A2Repository: SARKEX/st0x
Length of output: 1643
🌐 Web query:
Sentry setTag clear tag documentation null undefined
💡 Result:
Sentry does not provide a dedicated "clearTag" or "removeTag" method in its SDKs [1]. Because tags are stored within the current scope, you cannot simply "unset" a specific tag once it has been added to that scope [1][2]. To manage tags effectively, consider the following approaches: 1. Use Scopes for Isolation: Instead of setting tags globally, use Sentry.withScope or Sentry.withIsolationScope to apply tags only to specific operations [3][2]. When the callback finishes, the scope is discarded, and those tags are automatically removed [2]. 2. Clearing All Tags: If you need to reset the state entirely, you can use the scope.clear method (or equivalent depending on your SDK), which resets the scope to its default state [2]. 3. Handling Null/Undefined: Passing null or undefined as a tag value is generally not recommended as a way to "unset" a tag [1]. Depending on the SDK and server-side processing, these values may be ignored, dropped, or sent as literal nulls, which can lead to inconsistent behavior in the Sentry UI [1]. If you are working in a Single Page Application (SPA) or a long-running process where context changes frequently, the recommended pattern is to wrap your code in a new scope rather than attempting to remove individual tags from an existing one [1][3].
Citations:
- 1: Unset tags getsentry/sentry-javascript#2218
- 2: https://develop.sentry.dev/sdk/foundations/state-management/scopes/
- 3: How to set tag on a transaction? getsentry/sentry-javascript#11029
Remove the workaround for clearing Sentry tags; Sentry doesn't support unsetting individual tags.
The type assertion undefined as unknown as string is indeed problematic, but replacing it with null won't actually clear the tag. According to Sentry documentation, tags cannot be removed once set in a scope—passing null or undefined to setTag() doesn't unset tags and may result in inconsistent behavior in the Sentry UI.
For proper state management, either isolate the tag to a specific operation using Sentry.withScope() or Sentry.withIsolationScope(), or call scope.clear() to reset the entire scope. The current implementation should be refactored to avoid this unsupported pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/services/observability/tradeId.ts` around lines 41 - 48, The
clearTradeId function currently uses an unsupported workaround by calling
Sentry.setTag('trade_id', undefined as unknown as string); remove that call and
instead reset Sentry's scope explicitly — replace the setTag invocation with
Sentry.configureScope(scope => scope.clear()) (or alternatively remove all
Sentry interaction here and ensure callers use
Sentry.withScope()/withIsolationScope() when setting the 'trade_id'); keep
current = null in clearTradeId and ensure Sentry is imported/available for
configureScope.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yml (1)
8-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate deprecated
actions/checkout@v2to@v4.All four jobs (
test,test-integration,test-e2e,lint) still useactions/checkout@v2, which is deprecated and will stop working on GitHub Actions runners. Static analysis confirms this action is too old.Update all occurrences to
actions/checkout@v4to prevent workflow failures.🔧 Proposed fix
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4 with: submodules: recursive fetch-depth: 0Apply this change at lines 8, 32, 78, and 142.
Based on static analysis hints.
Also applies to: 32-32, 78-78, 142-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml at line 8, Replace the deprecated actions/checkout@v2 usages with actions/checkout@v4 in the workflow; specifically update each checkout step used by the jobs named test, test-integration, test-e2e and lint (the occurrences currently using actions/checkout@v2) so they reference actions/checkout@v4 to avoid runner failures.
♻️ Duplicate comments (2)
eslint.config.js (1)
131-143: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftPattern wildcards may block unintended sibling modules.
The use of
*wildcards (e.g.,$lib/stores/transaction*) will match sibling modules liketransactionShared.tsin addition to the intendedtransaction.tsand its subpaths. The past review comment provides a detailed analysis showing thattransactionSharedis legitimately imported throughout the codebase and should not be restricted.Consider using the
/**pattern for subpaths as recommended in the previous review to restrict only the exact module and its children, not siblings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eslint.config.js` around lines 131 - 143, The patterns using shell-style '*' (e.g., '$lib/stores/transaction*', '$lib/services/marketOrderExecution*', '$lib/services/orderDeployment*', '$lib/services/walletService*') are too broad and will match sibling modules like transactionShared.ts; replace those trailing '*' entries with the more precise recursive subpath pattern '/**' so each pattern targets the exact module and its children (e.g., use '$lib/stores/transaction/**' instead of '$lib/stores/transaction*') while leaving the base module entries (like '$lib/stores/transaction') intact; update the patterns array where these symbols appear to prevent unintended sibling matches.src/lib/components/orders/MarketOrder.svelte (1)
1324-1335:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScreen reader will announce raw error identifiers.
The error-banner element has
role="alert"andaria-live="polite"but contains only{errorClass}as textContent. This means screen readers will literally announce internal identifiers like "no_liquidity", "stale_oracle", "insufficient_balance" — not user-friendly copy.The previous review (marked as "Addressed in commits e68ccb5 to d8260ee") recommended either:
- Remove
role="alert"andaria-live, addaria-hidden="true"(keeps testid, stops announcement), or- Map
errorClassto localized messages inside this elementThe visible UI already renders human-readable error copy, so this hidden element should be for Playwright only.
🛡️ Proposed fix — silence the announcement
{`#if` errorClass} <div data-testid="error-banner" data-error-class={errorClass} data-mode="market" data-side={orderSide.toLowerCase()} class="sr-only" - role="alert" - aria-live="polite" + aria-hidden="true" > {errorClass} </div> {/if}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/orders/MarketOrder.svelte` around lines 1324 - 1335, The hidden error-banner currently exposes internal identifiers (errorClass) to screen readers because it has role="alert" and aria-live="polite"; change the element so it is truly non-announced test-only content by removing role and aria-live and adding aria-hidden="true" while keeping data-testid="error-banner", data-error-class={errorClass}, data-mode="market", data-side={orderSide.toLowerCase()}, and class="sr-only" so Playwright can still assert on it; ensure visible human-readable error copy remains rendered elsewhere and do not change that mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/components/orders/MarketOrder.svelte`:
- Around line 313-330: The reactive errorClass block in MarketOrder.svelte is
brittle because it re-derives categories from orderPreparationError and blindly
falls back to 'slippage'; update it to prefer an explicit errorClass returned
from executeMarketOrder (check the shape returned by marketOrderExecution.ts and
use that field if present), remove or change the fallback that maps any unknown
orderPreparationError to 'slippage' (return null or a neutral value instead),
and if the service-side errorClass is not yet implemented add a short TODO
comment referencing executeMarketOrder/marketOrderExecution.ts to mark this
technical debt so future changes don't silently break the taxonomy.
---
Outside diff comments:
In @.github/workflows/test.yml:
- Line 8: Replace the deprecated actions/checkout@v2 usages with
actions/checkout@v4 in the workflow; specifically update each checkout step used
by the jobs named test, test-integration, test-e2e and lint (the occurrences
currently using actions/checkout@v2) so they reference actions/checkout@v4 to
avoid runner failures.
---
Duplicate comments:
In `@eslint.config.js`:
- Around line 131-143: The patterns using shell-style '*' (e.g.,
'$lib/stores/transaction*', '$lib/services/marketOrderExecution*',
'$lib/services/orderDeployment*', '$lib/services/walletService*') are too broad
and will match sibling modules like transactionShared.ts; replace those trailing
'*' entries with the more precise recursive subpath pattern '/**' so each
pattern targets the exact module and its children (e.g., use
'$lib/stores/transaction/**' instead of '$lib/stores/transaction*') while
leaving the base module entries (like '$lib/stores/transaction') intact; update
the patterns array where these symbols appear to prevent unintended sibling
matches.
In `@src/lib/components/orders/MarketOrder.svelte`:
- Around line 1324-1335: The hidden error-banner currently exposes internal
identifiers (errorClass) to screen readers because it has role="alert" and
aria-live="polite"; change the element so it is truly non-announced test-only
content by removing role and aria-live and adding aria-hidden="true" while
keeping data-testid="error-banner", data-error-class={errorClass},
data-mode="market", data-side={orderSide.toLowerCase()}, and class="sr-only" so
Playwright can still assert on it; ensure visible human-readable error copy
remains rendered elsewhere and do not change that mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c711443-eb15-472d-a1bd-bcc1ed145295
📒 Files selected for processing (41)
.github/workflows/test.ymleslint.config.jssrc/lib/api/orders.tssrc/lib/clients/raindex.tssrc/lib/components/QuickTrade.sveltesrc/lib/components/charts/TokenMarketCharts.sveltesrc/lib/components/orders/DcaOrder.sveltesrc/lib/components/orders/LimitOrder.sveltesrc/lib/components/orders/MarketOrder.sveltesrc/lib/queries/orderbook.tssrc/lib/queries/priceFeeds.tssrc/lib/server/accessCodes.test.tssrc/lib/server/accessCodes.tssrc/lib/server/adminWalletList.tssrc/lib/server/alerts.tssrc/lib/server/logger.tssrc/lib/server/referrals.tssrc/lib/server/signatureChallenge.tssrc/lib/server/snapshots/pyth.tssrc/lib/server/snapshots/scraper.test.tssrc/lib/server/walletSession.test.tssrc/lib/services/marketOrderExecution.tssrc/lib/services/observability/captureTakeOrderFailure.tssrc/lib/services/orderDeployment.tssrc/lib/stores/deployTransactionStore.tssrc/lib/stores/marketTakeStore.tssrc/lib/utils/marketOrderFill.tssrc/lib/utils/tokenMath.tssrc/lib/utils/transactionDisplay.tssrc/routes/(main)/platform-metrics/+page.sveltesrc/routes/(main)/trade/[id]/+page.sveltesrc/routes/admin/+page.sveltesrc/routes/api/admin/pool-wallets/+server.tssrc/routes/api/admin/team-wallets/+server.tssrc/routes/api/prices/spym/+server.tssrc/routes/api/public/trade-activity/+server.tssrc/routes/api/snapshots/preview/+server.tssrc/routes/api/st0x/[...path]/+server.tstests/helpers/anvil.tstests/integration/ui/globalSetup.tstests/lib/server/rpcMetrics.test.ts
✅ Files skipped from review due to trivial changes (27)
- src/lib/utils/transactionDisplay.ts
- src/routes/api/admin/pool-wallets/+server.ts
- src/routes/(main)/platform-metrics/+page.svelte
- src/routes/api/admin/team-wallets/+server.ts
- src/lib/server/signatureChallenge.ts
- src/routes/api/st0x/[...path]/+server.ts
- src/lib/server/walletSession.test.ts
- src/lib/queries/orderbook.ts
- src/lib/utils/marketOrderFill.ts
- src/routes/api/public/trade-activity/+server.ts
- src/lib/queries/priceFeeds.ts
- src/lib/utils/tokenMath.ts
- src/lib/clients/raindex.ts
- src/lib/server/accessCodes.ts
- src/lib/server/snapshots/scraper.test.ts
- src/lib/components/charts/TokenMarketCharts.svelte
- src/routes/api/snapshots/preview/+server.ts
- src/lib/server/adminWalletList.ts
- src/lib/server/accessCodes.test.ts
- src/routes/api/prices/spym/+server.ts
- src/lib/components/QuickTrade.svelte
- src/routes/admin/+page.svelte
- tests/lib/server/rpcMetrics.test.ts
- src/lib/server/snapshots/pyth.ts
- src/lib/api/orders.ts
- src/lib/server/referrals.ts
- src/lib/stores/marketTakeStore.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/lib/services/observability/captureTakeOrderFailure.ts
- src/lib/server/logger.ts
- tests/integration/ui/globalSetup.ts
- src/lib/services/orderDeployment.ts
- src/routes/(main)/trade/[id]/+page.svelte
- src/lib/components/orders/DcaOrder.svelte
- src/lib/components/orders/LimitOrder.svelte
- src/lib/services/marketOrderExecution.ts
- src/lib/stores/deployTransactionStore.ts
…build timeout `encodeVaultBalanceHex` now takes a single decimal-string argument and routes through `Float.parse(decimalString)` — same call shape as production (`src/lib/stores/marketTakeStore.ts:124`). The prior path went via `parseUnits(...) → bigint → Float.fromFixedDecimalLossy(raw, decimals)`, which threaded the token decimals through twice (once in parseUnits, once in fromFixedDecimalLossy) and silently discarded the SDK's `lossless` flag. Rain Float carries its own scale, so the string form is the canonical input; no decimals arg needed at the call site. Error path now bubbles `parsed.error.readableMsg` instead of crashing opaquely on a malformed balance string. Also bumped the playwright webServer build timeout 300s → 600s. Cold `npm run build` on dev laptops chews ~5-7 min through @dynamic-labs / ox / porto before vite-preview can listen on 4173; 300s was sometimes too tight even locally. Verified: marketBuy + marketSell each pass real-anvil. svelte-check clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Goldsky free-tier rate-limits hard after even a handful of cold E2E
runs, and once limited the trade page sits indefinitely on "Loading
token data..." (singleTokenQuery never resolves). marketBuy / marketSell /
limitDeploy / marketFailures all surfaced this as a flaky 180s timeout
waiting for `[data-testid="open-trade"]`.
**Disk cache (`tests/integration/ui/__fixtures__/goldsky-cache/`)**:
The in-memory `goldskyCache` Map now backs onto sharded JSON files keyed
by sha256(method|url|body). First run after a cache clear hits upstream
and writes; every subsequent run replays from disk and never touches
Goldsky. Files are committed to the repo so CI gets a warm start too.
Bust the cache by deleting the directory.
**Fixture-level route drain**: The trade page polls
`/api/st0x/v1/orders/token/*` every 15s. If a `route.fetch` is mid-flight
when Playwright tears down the page, the route handler throws "Target
page closed" and Playwright reports it as a test failure even though the
assertions all passed. Calling `page.unrouteAll({ behavior:
'ignoreErrors' })` in the page-fixture teardown drains pending handlers
gracefully — fixes one failure mode hit by wrapRatio + limitDeploy.
Verified delta on full UI suite (`npx playwright test`):
- Before: 1 passed, 4 failed, 3 skipped
- After: 3 passed, 2 failed, 3 skipped
Remaining failures (marketSell submit-disabled, marketFailures
insufficient_balance) are pre-existing flakes documented in the spec
file headers — unrelated to cache state.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Auto-warmed by the marketSell.spec.ts run after the disk-cache landing. Keeps CI's cache hit-rate high. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ounts The trade page's openTradePanel() returns early when walletRegistered is still null. The auto-check fires when wagmi connects (walletAddress subscribe → checkWalletAccess → /api/access/check → walletRegistered. set(true)), but on a cold-start page the first open-trade click can race the access-check fetch. The button gets focus but the panel never opens, and the next clickModeTab() times out waiting for `mode-tab`. New `openTradePanel(page, side)` helper in fixtures.ts retries the click until the mode-tab buttons (gated on showTradePanel) appear in the DOM, with a 30s budget and 0.5/1/2s backoff. Worst case the click already worked and the helper exits immediately; best case it covers the access-check race. Migrated all four specs: - marketBuy.spec.ts - marketSell.spec.ts - limitDeploy.spec.ts - marketFailures.spec.ts (4 callsites) This is a defensive change — at worst it's a no-op (single click succeeds), at best it covers the wallet-registration timing race. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hardens the openTradePanel helper with an explicit waitForSelector for the button visibility before the retry-click loop. On a cold-start trade page the button can briefly be in the DOM but not yet hydrated (Svelte not finished mounting), and the retry-click would race the hydration boundary. The visibility wait gives the page a chance to settle before we start hammering the button. 60s budget — same order as test.setTimeout(180_000) ceiling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deterministic indicator that the wallet auto-connect + access-check chain finished. Header.svelte:217 only renders the "My Dashboard ..XXXX" button when ALL THREE of $isAuthenticated, $walletAddress, and $walletRegistered are truthy — exactly the same predicate openTradePanel() needs. Waiting on this button (60s budget, catch-and-fallback to retry-loop for the Dynamic-auth header variant) eliminates the race window where the open-trade click fires before wagmi has finished its autoConnect lifecycle. Without it the first click is a no-op (openTradePanel returns early) and the next clickModeTab times out waiting for the mode-tab buttons that the panel never mounted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Auto-warmed by suite runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
marketSell.spec.ts: the trade-submit button is gated on spendingTokenBalance > 0n, which depends on TradeAmountInput's reactive balance read settling. The read is reactive over $walletAddress and $wagmiConfig — both null on first invocation, then a second invocation fires after wagmi connects. A race between the two leaves spendingTokenBalance at 0n in the bad path, and submit stays disabled. Wait for the visible "Balance: 1.000" line to render in the panel before filling the asset input — proves the second balancePromise resolution landed and the parent saw the update. 30s budget — matches the openTradePanel wait ceiling. In a clean run this passes in 22.4s isolated. In a full suite run it still flakes occasionally (the balance display can race with TradeAmount Input's own remount when assetToken prop reference changes), but this fix removes the deterministic-race subset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
marketSell's submit-enabled check requires both:
1. spendingTokenBalance > 0n (wagmi balance read for wtCOIN)
2. marketPrice computed (orderbook walk against the maker bid)
Filling the asset-input before EITHER of these settles produces a
silent failure mode: the input shows "0.05" but TradeAmountInput's
internal `inputAmount` → `amount` reactive needs amountDecimals to be
set (populated from the balance read), and MarketOrder's reactive
`fetchMarketPrice` requires $orderbookQuotesQuery?.data?.quotes to be
populated before walkOrderbook can produce marketPrice. Skip either
prerequisite and selectedAmount stays at 0n / marketPrice stays
null / submit stays disabled.
Wait for:
- "Balance: 1.000 wtCOIN" — proves wagmi balance + decimals settled
- "Bid Price $XXX" with a non-zero number — proves the orderbook
query landed and the synth-stub maker bid is visible
Then fill 0.05 + give Svelte one tick to flush the cascade before
asserting submit-enabled.
Verified: 4 passed / 1 failed in a full suite run (only the documented
marketFailures insufficient_balance pre-existing flake remains).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same pattern as marketBuy: the LIVE wtCOIN orderbook orders are typically priced at \$180-200, but Pyth oracle drifts to ~\$175 making them fall outside the 5% price-guard band. The form short-circuits to no_liquidity and the insufficient_balance classifier (which gates on marketPrice being truthy) never fires. Deploy a maker ask at \$170/wtCOIN before the test interaction so the orderbook has a valid quote inside the band regardless of oracle drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Deploy) Captures the work that converted marketBuy/Sell from Path-A (re-quoting LIVE orders) to Path-B (deploy maker, take via UI), plus the limitDeploy vault-tutorial root-cause fix. Supersedes the "What's NOT done yet" section of HANDOVER-2026-05-19.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alastairong1
force-pushed
the
phase-01-ui-driven-e2e-tests
branch
from
May 28, 2026 19:37
1abef07 to
e1a61ea
Compare
captureTakeOrderFailure imported ProcessedQuote from marketOrderExecution, which in turn imports captureTakeOrderFailure — a structural cycle in the services-layer DAG even though the import was type-only. ProcessedQuote is canonically defined in $lib/utils/orderbook (line 73); marketOrderExecution just re-exports it. Re-import from the canonical home so the DAG is acyclic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- tests/helpers/previewServer.ts — entire 74-LoC file unused. Playwright
config's `webServer` block now owns preview lifecycle; nothing imported
startPreviewServer / stopPreviewServer / waitForUrl.
- withSnapshot helper in tests/helpers/anvilControl.ts — exported, no
callers. Snapshot/revert lifecycle is in-lined in
tests/integration/ui/fixtures.ts where it's actually used.
- fundErc20ViaImpersonation re-export from tests/integration/ui/fixtures.ts
— no spec imports it; the fundToken wrapper uses it internally.
- TRADE_ID_HEADER constant + the matching server-side validator in
src/lib/server/logger.ts — the browser never sends the header, so the
UUIDv4 regex validation branch was unreachable in production ("alive
in tests, dead in prod"). Module docstring updated to note that a
fetch-interceptor seam is a planned follow-up; re-add both surfaces
with the interceptor in a future PR.
- tests/lib/server/logger.tradeId.test.ts deleted (tests a now-deleted
branch). Test 7 in tradeId.test.ts likewise removed.
svelte-check 0 errors. vitest 736 passed / 1 skipped (-6 vs prior:
5 deleted server-side test cases + 1 deleted lifecycle test).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ifiers The MarketOrder.svelte inline `classifyMarketError` and the byte-identical LimitOrder.svelte + DcaOrder.svelte `classifyDeployError` versions all mapped raw errors to the `ErrorClass` union. The MarketOrder comment explicitly noted the "extract to shared module at three call sites" threshold — which had been met. Hoist to src/lib/services/observability/classifyError.ts with a single function `classifyError(err, scope='deploy')` where `scope: 'market'` opts in to the four extra classes a market take can hit (slippage / no_liquidity / stale_oracle / market_closed). - MarketOrder calls `classifyError(err, 'market')` at both trade_failed call sites (was: classifyMarketError(err) at lines 957, 981). - LimitOrder + DcaOrder call `classifyError(err)` at their trade_failed call sites; the deploy scope is the default, matching prior behaviour. Test files updated to import + assert against the shared classifier. Test 5b (MarketOrder) now asserts the new shared-import line + the explicit `'market'` scope argument; Test L8 (LimitOrder) asserts the shared-import line. svelte-check 0 errors. vitest 736 passed / 1 skipped (no regression). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…caOrder
The trade-id lifecycle module exposed mintTradeId / clearTradeId as a
pair and required every submit handler to remember a `try { mint }
finally { clear }` Pitfall 2 (T-2-E) ceremony — the comment block
"Pitfall 2 — T-2-E mitigation" was repeated at 5 sites across the three
order forms.
Add `withTradeId<T>(fn)` to tradeId.ts as the single primitive that
encodes the discipline. Refactor MarketOrder.handleMarketOrder and
DcaOrder.handleDcaDeploy to call it; their inner try/catch for
trade_failed emission stays unchanged.
LimitOrder.handleDeploy still spans a UI event boundary (the
pre-deploy slippage warning modal defers the actual deploy to a
proceedWithDeploy callback). That lifecycle can't fit a try/finally
wrapper, so LimitOrder keeps the inline mint/clear discipline with the
existing deferredToProceed flag.
DcaOrder.handleDcaDeploy is now declared `async` so it can `await
withTradeId(...)`. No behavioral change at the call site (the on:click
binding doesn't care).
Tests updated:
- MarketOrder Test 2 + Test 3 + Test 7: assert withTradeId wrapper +
early-return ordering against the wrapper call.
- DcaOrder Test D1 + D5: assert wrapper usage; no open-coded
mint/clear allowed in the handler.
svelte-check 0 errors. vitest 736 passed / 1 skipped (no regression).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tradeEvents.ts module docstring forbade inline raw track() for trade events; the call sites violated the contract for the panel-level subset (trade_panel_opened, trade_panel_abandoned, trade_error_shown). Those events were emitted via the raw `track` import, which skips the `trade_id` enrichment the trackTradeEvent wrapper applies. Restore the contract: every name in `TradeEventName` now goes through `trackTradeEvent`. When no trade is in flight (panel-mount, abandoned without submit, early validation error), `getCurrentTradeId()` returns null and the event correctly emits `trade_id: null` — which is the intended funnel-correlation behavior (these events are NOT part of an active trade lifecycle). - MarketOrder: trade_panel_opened, trade_error_shown (×3), trade_panel_abandoned → trackTradeEvent. - LimitOrder: trade_panel_opened, trade_panel_abandoned → trackTradeEvent. - DcaOrder: trade_panel_opened → trackTradeEvent. The raw `track` import is now unused in all three components. tradeEvents.ts docstring updated to note the trade_id: null behavior on panel-level events so the rationale is explicit at the policy line. The order_side string-to-union cast pattern stays consistent across sites since trackTradeEvent's TradeEventProps is strict on that field. Tests: - Test 6 (MarketOrder) and Test L6 (LimitOrder) flip to assert trackTradeEvent routing + absence of the raw `track` import. - Test D1 + D2 (DcaOrder) likewise. svelte-check 0 errors. vitest 736 passed / 1 skipped (no regression). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- prettier reformatted long-line collapses in MarketOrder.svelte from the recent obs refactors (within 100-char target width). - Add the PR-174 review report (Code Reviewer agent output) to the planning trail so the simplification rationale is greppable. svelte-check 0 errors, vitest 736 passed / 1 skipped, eslint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…f live orders
Path A re-quoted LIVE production orders against the anvil fork by:
(a) letting the production REST proxy fetch the LIVE order list
(b) running a batched RaindexClient.getOrders + per-order getQuotes()
against anvil to substitute fork-derived ioRatio + maxOutput
It was brittle (oracle drift, NYSE-hours dependency, live order set
churn) and never asserted against by any spec. limitDeploy used it as
ambient orderbook noise but never takes against the served orders;
marketBuy/Sell/Failures all register their own makers (Path B) and short-
circuit Path A entirely.
Removed:
- tests/integration/ui/forkOrdersStub.ts (entire 217-LoC file).
- The `patchOrdersResponseAgainstFork` fallback branch in
tests/integration/ui/fixtures.ts that hit the production REST proxy
and substituted fork-derived values into the response.
- The "re-derive orderbook quotes against the anvil fork" comment block.
Path B (deploy maker → take through UI) is untouched: makerOrders.ts,
syntheticOrdersStub.ts, registerMakerOrders, and the
getMakerOrders().length > 0 dispatch in fixtures.ts all stay.
Specs that don't register a maker (limitDeploy) now receive an empty
orderbook from /api/st0x/v1/orders/token/<addr> — they deploy but never
take, so an empty orderbook is correct.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e-tests # Conflicts: # src/lib/stores/marketTakeStore.ts # src/routes/api/st0x/[...path]/+server.ts
Post-merge prettier formatting normalisation. No behaviour changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Live-source bugs:
1. belowMinTradeError mis-classified as insufficient_balance
LimitOrder.svelte error-banner now emits data-error-class="below_min_trade"
(was incorrectly "insufficient_balance"). "Order value below $1" is a
distinct condition from wallet insufficiency; mislabel polluted the
funnel and would have made TEST-08 assertions pass/fail for the wrong
reason.
2. sr-only error-banner announced raw taxonomy strings
MarketOrder + LimitOrder error-banners had role="alert" aria-live="polite"
on elements whose only textContent is the internal class identifier
("no_liquidity", "stale_oracle", …). Assistive tech literally announced
those strings. Switched to aria-hidden="true" + dropped role/aria-live —
the visible UI blocks above already announce the human-readable error;
these elements remain as stable Playwright selector hooks only.
3. LimitOrder success-toast lies about deploy state
tradeSubmittedSuccessfully flips synchronously on submit-click — BEFORE
the Rainlang confirmation modal opens, let alone the tx broadcasts. The
SR announcement was telling users "Order deployed" while the deploy was
still pending confirmation. Renamed to "Order submitted for confirmation"
with a code comment flagging that threading a real broadcast-callback
through transactionStore.handleLimitDeploy is the follow-up fix.
4. DeployEventContext re-derived symbols from maker-perspective args
src/lib/services/orderDeployment.ts:251,305 emitted
asset_symbol: args.inputToken.symbol
payment_symbol: args.outputToken.symbol
which inverts on sell-side deploys (CLAUDE.md §"Order Semantics":
Sell maker's orderInput = payment, orderOutput = asset). Extended
DeployEventContext to carry USER-perspective asset_symbol +
payment_symbol fields; callers (LimitOrder, DcaOrder) now pass them
explicitly. Sign-trade events now agree with the component-level events
for the same trade.
5. executeMarketOrder now returns a discriminated errorClass
MarketOrderResult gained an optional errorClass: ErrorClass field set
from a pure errorClassForReason() mapping over TakeOrderFailureReason.
The component prefers result.errorClass over its previous substring-
matching block (which had a misleading "fallback to slippage" branch
for any unrecognised prep error). Local-only signals
(insufficientBalanceError, noLiquidityError, priceError) still derive
locally — they don't go through the service.
6. Sentry tag-clear workaround documented
tradeId.clearTradeId() uses setTag('trade_id', undefined as unknown as
string) because Sentry has no first-class removeTag API. Added a code
comment explaining the cast so the next reader doesn't think it's a bug.
Hygiene:
7. CI: bumped actions/checkout@v2 → @v4 across all 4 jobs in
.github/workflows/test.yml. v2 is deprecated. (test-e2e is already
gated step-level on secrets.BASE_RPC_URL — line 91-97.)
8. Planning doc hygiene: 01-01-PLAN.md previously embedded literal
anvil-default private keys (public test keys, but tripped repo secret
scanners). Replaced with <ANVIL_ACCOUNT_N_KEY> placeholders + pointer
to the Foundry docs; concrete keys still live in
tests/integration/ui/fixtures.ts where they belong.
Verified locally: svelte-check 0/0, vitest 736 passed / 1 skipped,
eslint clean, prettier clean.
Co-Authored-By: Claude Opus 4.7 (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.
Bundles two adjacent shippable phases. The wrap-ratio UX work that was previously interleaved in this branch has been split out to PR #189.
Phase 01 — UI-driven E2E test coverage
New Playwright harness under
tests/integration/ui/: anvil fork + preview server (via PlaywrightwebServer) + wagmi EIP-1193 stub + Goldsky/ST0x synthetic stubs. Disk-cached Goldsky responses checked intotests/integration/ui/__fixtures__/goldsky-cache/to keep CI off the free-tier rate limit.Specs:
limitDeploy.spec.ts— deploy a limit order via UI, assert on-chain order countmarketBuy.spec.ts(Path-B) — deploy a maker ask + UI market buy, assert wtCOIN credited / USDC debitedmarketSell.spec.ts(Path-B) — deploy a maker bid + UI market sell, assert USDC credited / wtCOIN debitedmarketFailures.spec.ts—insufficient_balanceUI gate (3 other failure modes skipped, documented in spec header)Supporting:
test-e2ejob in.github/workflows/test.ymlTESTING.mdsection enforcingdata-testiddiscipline for UI testsdata-testidretrofit acrossMarketOrder/LimitOrder/OrdersTabletests/helpers/makerOrders.ts) — Path-B "deploy your own maker, take via UI" pattern via Rain fixed-limit strategy + Float-encoded vault balance for the synth stubPhase 02 — Observability (OBS-06 / OBS-07 / OBS-09)
tradeIdlifecycle module + typedtrackTradeEventwrappertrade_idplumbed into pino server logger, Sentry tags, and the canonical event taxonomyMarketOrder,LimitOrder,DcaOrder,marketOrderExecution,orderDeploymenteventContexton deploy;page_viewedevent renameVerification
npm run check— 0 errors / 0 warningsnpm test— 742 passed / 1 skippednpx playwright test(FORK_BLOCK=46344566) — 4 passed / 3 skipped in 2.3mKnown constraint
marketFailures > insufficient_balanceflakes in 3-way ordering after both marketBuy + marketSell run; documented inline in the spec header. Not a blocker.🤖 Generated with Claude Code