Skip to content

feat(agents): issue-45-add-web-test-harness-rateslider-anchorin - #46

Merged
mctl-claude-remote[bot] merged 1 commit into
mainfrom
feat/agents-issue-45-add-web-test-harness-rateslider-anchorin
Jun 3, 2026
Merged

mctl-claude-remote[bot] merged 1 commit into
mainfrom
feat/agents-issue-45-add-web-test-harness-rateslider-anchorin

Conversation

@mashkovd

@mashkovd mashkovd commented Jun 3, 2026

Copy link
Copy Markdown
Member

Implements accepted proposal mctl-pairdesk/issue-45-add-web-test-harness-rateslider-anchorin.

Spec: https://github.com/mctlhq/mctl-gitops/tree/main/platform-gitops/agents-state/mctl-pairdesk/proposals/issue-45-add-web-test-harness-rateslider-anchorin/

Generated by mctl-agents Tier 2 implementer. Review carefully — this PR was opened automatically and the spec may have gaps.

Closes #45

Wires up vitest + jsdom + @testing-library/react as the web test
infrastructure and adds ten test cases (T1-T10) covering want/give
anchoring, slider interaction, wantAmount prop updates, pair changes,
edge-case input handling, and API failure fallback for RateSlider.
Adds npm test step to the web CI job.

Proposal: platform-gitops/agents-state/mctl-pairdesk/proposals/issue-45-add-web-test-harness-rateslider-anchorin/
@claude

claude Bot commented Jun 3, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 4m 7s —— View job


Review complete — approved

  • Read PR context and changed files
  • Review test setup (vitest config, package.json, CI)
  • Review RateSlider.test.tsx contents
  • Confirm no behavioral/runtime code changes (test-only)
  • Post inline findings if any
  • Submit final verdict

Verdict: Approved — no P1/P2 findings (1 P3 nit).

What this PR does

Test-only addition: a Vitest + jsdom + React Testing Library harness, a new npm test step in the web CI job (runs after type-check, before build), and 10 tests for the RateSlider anchoring behavior in web/src/components.tsx.

Why it's safe

  • No runtime/source changes — only .github/workflows/ci.yml (adds npm test), web/package*.json, web/vitest.config.ts, web/src/test-setup.ts, and the new test file. No edits to components.tsx, services, routes, or schema.
  • No concurrency surface touched (accept/order-reservation flow unchanged).
  • No serializer / sensitive-data surface touched.
  • No RBAC, requireApproved, or initData logic touched — ./api.js and ./tg.js are fully mocked in tests.

Test logic vs. the component (spot-checked)

  • T1/T2/T3 anchoring (lastEditedField ref): consistent with the two effects at components.tsx:602 and :618.
  • T4/T5/T6 rate resolution and pair-change reset: consistent with the pair-change effect at :587 calling onRateResolved(null) then refetching.
  • T7 non-finite input, T8 rate=0 division guard, T9 blur auto-fill, T10 unavailable fallback (/reference rate unavailable/i matches the hint at :657) — all line up with the corresponding code paths.

Findings

  • P3 (nit, non-blocking) — web/src/test-setup.ts: with @testing-library/jest-dom@^6 + vitest@^2, the documented entry that augments vi.Assertion matcher types is @testing-library/jest-dom/vitest. Bare import works at runtime but the /vitest subpath is safer for the type-check step that runs before npm test in CI. Suggested fix posted inline.
    · branch feat/agents-issue-45-add-web-test-harness-rateslider-anchorin

Comment thread web/src/test-setup.ts
@@ -0,0 +1 @@
import '@testing-library/jest-dom';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 — Nit, non-blocking. With @testing-library/jest-dom@^6.6 and vitest@^2, the recommended (and documented) entry point that augments vi.Assertion matcher types is the /vitest subpath. The bare import registers matchers at runtime but in some TS setups doesn't extend Vitest's expect typings, so editor / tsc may not know about .toBeInTheDocument(). Since CI runs type-check before npm test, switching to the /vitest path is the safer form.

Suggested change
import '@testing-library/jest-dom';
import '@testing-library/jest-dom/vitest';

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No P1/P2 findings (1 P3). Test-only addition (vitest harness + 10 RateSlider anchoring tests, CI npm test step, dev-deps). No runtime/source changes; no concurrency, serializer, RBAC, or initData impact. Test logic verified against components.tsx RateSlider — anchoring via lastEditedField ref, rate-resolution effects, and the rate=0 / blur-autofill / API-failure edge paths all line up. Good to merge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 12b0bb8365

ℹ️ 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".

Comment thread web/src/test-setup.ts
@@ -0,0 +1 @@
import '@testing-library/jest-dom';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use the Vitest jest-dom entrypoint

In the web CI job I checked, npm run type-check runs before npm test; this setup file imports the Jest entrypoint, which does not augment Vitest's Assertion type. Because the new test imports expect from vitest and uses toBeInTheDocument(), a clean install will fail type-checking; import @testing-library/jest-dom/vitest instead.

Useful? React with 👍 / 👎.

@mctl-claude-remote mctl-claude-remote Bot added the steward:owned pr-steward owned label Jun 3, 2026
@mctl-claude-remote
mctl-claude-remote Bot merged commit 38df5cd into main Jun 3, 2026
5 checks passed
@mctl-claude-remote
mctl-claude-remote Bot deleted the feat/agents-issue-45-add-web-test-harness-rateslider-anchorin branch June 3, 2026 21:59
@mctl-claude-remote mctl-claude-remote Bot added steward:merged pr-steward merged and removed steward:owned pr-steward owned labels Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

steward:merged pr-steward merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add web test harness + RateSlider anchoring tests

1 participant