feat(test): add basic fixture app for MCP e2e tests (#3)#15
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new basic test fixture under test/fixtures/basic and adds a corresponding test script to package.json. It also updates panel.css to support reduced motion preferences. Feedback points out that the @keyframes pulse animation was accidentally deleted, which breaks the scan indicator's visual effect. Additionally, it is recommended to update the fixture's npm script to automatically install dependencies before starting the development server to prevent failures in clean environments.
| @media (prefers-reduced-motion: reduce) { | ||
| .scan-indicator { | ||
| animation: none; | ||
| } | ||
|
|
||
| .spinner { | ||
| animation: none; | ||
| border-top-color: var(--accent-blue); | ||
| } | ||
|
|
||
| .mini-spinner { | ||
| animation: none; | ||
| } | ||
|
|
||
| .saving-indicator { | ||
| animation: none; | ||
| } | ||
|
|
||
| .btn-spinner { | ||
| animation: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
The @keyframes pulse animation was accidentally removed during this change. However, .scan-indicator (on line 889) still references pulse for its animation. This will break the pulsing visual effect for the scan indicator.
Please restore the @keyframes pulse definition alongside the new @media (prefers-reduced-motion: reduce) block.
@keyframes pulse {
0%, 100% { opacity: 1; }
50% { opacity: 0.4; }
}
@media (prefers-reduced-motion: reduce) {
.scan-indicator {
animation: none;
}
.spinner {
animation: none;
border-top-color: var(--accent-blue);
}
.mini-spinner {
animation: none;
}
.saving-indicator {
animation: none;
}
.btn-spinner {
animation: none;
}
}| "test:run": "vitest run", | ||
| "test:coverage": "vitest run --coverage" | ||
| "test:coverage": "vitest run --coverage", | ||
| "test:fixture:basic": "npm --prefix test/fixtures/basic run dev" |
There was a problem hiding this comment.
Running the fixture dev server directly via npm run test:fixture:basic will fail if the fixture's dependencies are not installed first.
To make this command robust and self-contained (especially for CI/CD pipelines and new local setups), consider adding a setup script or automatically running npm install for the fixture before starting the dev server.
"test:fixture:basic:install": "npm --prefix test/fixtures/basic install",
"test:fixture:basic": "npm run test:fixture:basic:install && npm --prefix test/fixtures/basic run dev"
SummaryFixes #8 Users who enable Reduce Motion in OS/browser accessibility settings Changes
How to test
Screenshots
Acceptance Criteria
|
hoainho
left a comment
hoainho
left a comment
There was a problem hiding this comment.
Thanks @Kunall7890 — solid foundational work. All issue #3 acceptance criteria are met (correct data-testid, explicit displayName, Vite dev server boots on 5173), and the fixture isolation is clean. You clearly read the codebase before contributing.
I ran a multi-agent review (security, code quality, best-practices) and found 2 issues that should be fixed before merge and a handful of warnings. Most of this is fixable in ~10 minutes.
🔴 Critical (block merge)
C-1. <StrictMode> will break the planned fiber-walk e2e tests.
main.tsx wraps the app in <StrictMode>. React 18 StrictMode intentionally mounts → unmounts → remounts every component in development. The MCP v1 spec scenario for get_fiber_node ("Found by displayName") expects to find UserCard once — with StrictMode the fiber tree will double-count during reconciliation, making the e2e tests racy: pass in production build, fail (or double-count) in dev. (Per react.dev StrictMode docs.)
Fix — remove StrictMode from the fixture (not the main app):
// test/fixtures/basic/src/main.tsx
// StrictMode intentionally omitted — this fixture is consumed by fiber-walk
// e2e tests that assert single-mount counts. See MCP v1 spec.
createRoot(document.getElementById('root')!).render(<App />);C-2. Scope creep — the panel.css change belongs to issue #8.
The @media (prefers-reduced-motion: reduce) block you added to src/panel/styles/panel.css implements issue #8, not #3. The CSS itself is good a11y work — no objection to the change — but bundling it here:
- Implicitly claims issue #8 without coordination (it may have been assigned to someone else).
- Hurts revert safety — if the fixture has a bug and we revert this PR, the a11y fix is also lost.
- Mixes scopes which makes the PR description misleading.
Fix: split into two PRs.
- This PR: keep
test/fixtures/basic/**+ thepackage.jsonscript + (optionally) the rootpackage-lock.jsonbump. - New PR against #8: just the
panel.csschange.
🟡 Warnings (please also fix)
- 1,732-line
package-lock.jsoncommitted for a fixture sub-project. Most extension repos gitignore fixture lockfiles (the fixture isn't published or deployed). Easiest fix: addtest/fixtures/**/package-lock.jsonto root.gitignoreand remove the committed file. Alternatively, document why it's committed in CONTRIBUTING.md (deterministic CI installs). - Root
package-lock.jsonbumped 2.0.1 → 2.0.3 unrelatedly. Looks likenpm installran against a stale lockfile. Rungit checkout main -- package-lock.jsonthen re-commit, or trust squash-merge to collapse it. test:fixture:basicscript name is misleading + dangerous in CI. Thetest:*namespace is reserved for vitest commands that exit with a pass/fail code. Your script starts a long-running dev server, so any CI runner callingnpm run test:fixture:basicwill hang indefinitely. Rename tofixture:basic:devorstart:fixture:basic.<img src={avatarUrl}>accepts unsafe URL schemes.javascript:,data:,vbscript:aren't filtered. TodayavatarUrlis hardcoded so the risk is zero, butUserCardis exported and reusable. Validate the scheme:const safe = avatarUrl && /^(https?:|\/)/.test(avatarUrl);.import React from 'react'inUserCard.tsxis redundant under"jsx": "react-jsx". Drop it; if you need types, useimport type { CSSProperties } from 'react'.
💡 Improvements (nice-to-have)
React.FCin UserCard.tsx is a repo-wide antipattern (yourApp.tsxcorrectly usesfunction App()— inconsistent within this PR). Usefunction UserCard({ name, role, avatarUrl }: UserCardProps): React.JSX.Element { ... }.UserCard.displayName = 'UserCard'is technically redundant — React DevTools readsFunction.nameand the const-name populates it automatically. It IS load-bearing if Vite ever minifies the dev build. A comment explaining why it's there would help:// Required if build minifies — fiber walker depends on displayName.- Pin exact dep versions in fixture
package.json("react": "18.3.1"not"^18.3.1") for deterministic CI. - All 6 new files end without a final newline (POSIX standard). This is a repo-wide gap (PR #17 has the same issue) — worth adding
.editorconfigwithinsert_final_newline = trueat repo root in a separate PR. vite.config.tsstrictPort: truewith hardcoded5173will fail in parallel CI. Considerport: Number(process.env.FIXTURE_PORT ?? 5173)for flexibility.
What you got right
✅ All issue #3 ACs met · ✅ App.tsx uses the correct repo pattern (plain function App()) · ✅ Fixture tsconfig matches parent strictness (strict: true, jsx: react-jsx) · ✅ Clean isolation — fixture has own node_modules, doesn't pollute parent deps · ✅ The CSS change you snuck in is GOOD a11y work (just needs to be in its own PR) · ✅ Conventional Commits title
Recommended next steps
- Remove
<StrictMode>frommain.tsxwith a comment explaining why. - Open a separate PR against #8 for the
panel.csschange; revert that hunk here. - Decide lockfile policy (easiest: gitignore + remove).
- Rename
test:fixture:basic→fixture:basic:dev. - (Optional but helpful) Pin deps, fix
React.FC, drop the React default import, scheme-checkavatarUrl. - Push to the same branch and ping me for re-review.
Full review report on disk at .opencode/reviews/PR_15_2026-05-31.md.
The scope-creep issue is the main process learning to take from this — when you spot another good change while doing the assigned task, OSS convention is to open a separate PR or at least flag it in the PR description so the maintainer can decide. Otherwise: really good contribution.
|
fix: address all review comments for fixture app (#3) SummaryAddresses all review comments from @hoainho on PR #15. Changes Made🔴 Critical Fixes
🟡 Required Fixes
Files Changed
How to test# Start fixture server
npm run fixture:basic:dev
# In another terminal verify data-testid exists
curl http://localhost:5173 | grep data-testidAcceptance Criteria
Closes #3 |
|
@hoainho I have addressed all the critical issues: C-1 ✅ Removed
C-2 ✅ Reverted
Additional fixes pushed:
This PR now only contains:
Ready for re-review. 🙏 |
hoainho
left a comment
hoainho
left a comment
There was a problem hiding this comment.
🎉 LGTM @Kunall7890 — every critical and warning is addressed. Verified file-by-file:
| Item | Status |
|---|---|
C-1 Remove <StrictMode> |
✅ main.tsx has the exact comment I suggested |
C-2 Split panel.css to its own PR |
✅ Diff has zero panel.css hits — now in PR #18 |
W-3 Rename test:fixture:basic → fixture:basic:dev |
✅ |
W-4 <img src> URL scheme safety |
✅ safeAvatarUrl with `/^(https?: |
| W-5 Drop redundant React default import | ✅ import type { CSSProperties } instead |
I-1 Plain function instead of React.FC |
✅ |
I-2 Explanatory comment on displayName |
✅ Verbatim |
Two tiny non-blocking observations, fix-in-follow-up acceptable:
- Lockfile policy still open —
test/fixtures/basic/package-lock.json(1732 lines) is still committed. If you want to address it: addtest/fixtures/**/package-lock.jsonto root.gitignorein a follow-up. Or we keep it for deterministic CI installs. Either is fine — just needs a decision later. - Scripts indent regression in root
package.json— the scripts block lost its 2-space indent. Tiny cosmetic issue. Will fix in a follow-up commit if you don't want to push another change here.
Approving. Thanks for the speed of the response — and especially for opening #18 as a separate PR instead of bundling. That's exactly the right OSS workflow. I'll approve #18 next and the maintainer (or me) will merge both shortly.
2 participants
What
Adds a minimal Vite + React 18 fixture app under test/fixtures/basic/
for MCP e2e Playwright tests.
Changes
test/fixtures/basic/package.jsontest/fixtures/basic/vite.config.tstest/fixtures/basic/tsconfig.jsontest/fixtures/basic/index.htmltest/fixtures/basic/src/main.tsxtest/fixtures/basic/src/App.tsxtest/fixtures/basic/src/UserCard.tsxpackage.json— addedtest:fixture:basicscriptHow to test
npm run test:fixture:basic
curl http://localhost:5173 | grep data-testid
Closes #3