Conversation
… readability - Updated timer tests to replace direct text checks with `seeDuration` for consistency. - Refactored custom duration input tests to use dedicated methods for entering values. - Simplified preset selection tests by introducing `choosePreset` method. - Enhanced timer actor with new methods for starting, pausing, and resetting the timer. - Removed deprecated story files for articles and calculator components. - Added new utility methods in items testing for filtering and checking item visibility. - Improved the TimerPage component to manage button states based on timer status.
…n and responsibilities
WalkthroughThis PR consolidates component-level Storybook tests into integration-level stories. It expands test actor APIs with new assertion and interaction helpers, refactors integration story tests to use actors instead of raw DOM selectors, removes individual component story files, updates TimerPage component logic for better testability, and enhances documentation to reflect the new testing architecture. ChangesTest Architecture Consolidation
Possibly Related PRs
Poem
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Fallow audit reportNo GitHub PR/MR findings. Generated by fallow. |
There was a problem hiding this comment.
Pull request overview
This PR consolidates per-page Storybook tests into reusable actor/locator helpers and moves the actual test execution into the integration story files. Page-level *.stories.tsx files that previously held duplicated test logic are removed, while each page's testing.ts actor is extended with higher-level interaction helpers (e.g., start, pause, choosePreset, enterCustomDurationByBlur, press/pressMany, seeDisplay, applyCategoryFilter, openArticle, seeArticleDetailContent, seeDetailLoading). Integration stories under src/app/integration/ are updated to call these helpers and gain new coverage for Items filtering/sorting and Calculator arithmetic. The PR also tweaks TimerPage.tsx to memoize a couple of atom reads into locals and refactor inline wrap() callbacks into named handlers, and updates docs/testing.md and docs/tooling.md to reflect the new test layout and a tooling source-of-truth note.
Changes:
- Remove per-page
*.stories.tsxtest files and migrate their assertions intosrc/app/integration/*.stories.tsx, expanding integration coverage for Calculator, Items, and Articles. - Extend page-level
testing.tsactors with reusable interaction and assertion helpers; broadentimerLoc.displayto accept dynamic values. - Minor refactor in
TimerPage.tsxto extractisRunning/isStartDisabledand namedhandleStart/handlePause/handleResetcallbacks; docs updates for testing layout and Vite+ source-of-truth.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pages/timer/ui/TimerPage.tsx | Extract repeated atom reads and inline wrap() callbacks into local consts/handlers. |
| src/pages/timer/ui/TimerPage.stories.tsx | Delete page-level Timer story tests (migrated to integration). |
| src/pages/timer/testing.ts | Add display(value), customInput, preset helper, and interaction helpers (start, pause, reset, choose*, enterCustomDurationBy*, waitForDuration). |
| src/pages/items/ui/ItemsPage.stories.tsx | Delete page-level Items story tests. |
| src/pages/items/testing.ts | Add seeItem/dontSeeItem/seeOnlyItems and filter/sort/price helpers. |
| src/pages/calculator/ui/CalculatorPage.stories.tsx | Delete page-level Calculator story tests. |
| src/pages/calculator/testing.ts | Add display, key, press, pressMany, seeDisplay helpers. |
| src/pages/articles/ui/{list,detail}/*.stories.tsx, ArticleStatusBadge.stories.tsx, ArticlesPageLoading.stories.tsx | Delete page-level Articles story tests. |
| src/pages/articles/testing.ts | Add openArticle, seeNoSelection, seeArticleDetailContent, seeArticleDetailDescription, seeArticleDetailStatus, seeDetailLoading. |
| src/app/integration/Timer.stories.tsx | Rewrite tests to use new timer actor helpers. |
| src/app/integration/Items.stories.tsx | Use new actor helpers and add filter/sort integration stories. |
| src/app/integration/Calculator.stories.tsx | Add arithmetic/decimal/sign/percent/clear/error integration tests using new actor. |
| src/app/integration/Articles.stories.tsx | Use new actor helpers; collapse repeated click+wait+scope blocks. |
| docs/tooling.md | Document vite.config.ts as primary source of truth for tool config. |
| docs/testing.md | Update test-location table and add notes on shared test DSL refactoring opportunity. |
| .config/mise/conf.d/tasks-quality.toml | lint:fallow:health now depends on test:coverage and passes --coverage .var/coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| seeOnlyItems: async (...names: string[]) => { | ||
| for (const name of names) { | ||
| await I.see(text(name)) | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/integration/Items.stories.tsx (1)
274-283: 💤 Low valueVerify soft assertion testing pattern.
This test appears to be verifying the
hopeThatsoft assertion mechanism itself (lines 281-282 expectnoErrors()to throw after a failed assertion). While this validates the test infrastructure, it might be more appropriate insrc/shared/test/actor.test.stories.tsxrather than in a product integration test.Consider moving this test infrastructure verification to the actor self-tests, and keep this story focused on the product behavior (empty state when no items match filters).
🤖 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/app/integration/Items.stories.tsx` around lines 274 - 283, The test FilteredToEmpty.test currently mixes product behavior checks with a verification of the soft-assert API (using I.hopeThat and expecting I.hopeThat.noErrors() to throw); move the soft-assert verification into the actor self-tests (e.g., src/shared/test/actor.test.stories.tsx) and simplify FilteredToEmpty.test to only assert UI behavior: keep the initial positive visibility checks (I.seeItem('Wireless Headphones'), I.seeItem('Standing Desk')), apply the filters with I.applyCategoryFilter('Electronics') and I.applyStockFilter('Out of Stock'), then assert the resulting empty-state behavior (expect I.hopeThat(() => I.seeItem(...)).toBe(false) or a direct empty-state check) and remove the I.hopeThat.noErrors() throw expectation from this integration story.src/pages/items/testing.ts (1)
54-58: ⚡ Quick winConsider renaming
seeOnlyItemsto clarify its behavior.The method name
seeOnlyItemssuggests it verifies only the specified items are present (and others are absent), but the implementation only asserts that all specified items are present. It does not verify that no other items exist.Consider renaming to
seeItemsorseeAllItemsto better match the actual behavior, or extend the implementation to include negative assertions for completeness.🤖 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/pages/items/testing.ts` around lines 54 - 58, The helper seeOnlyItems currently only asserts presence of each provided name but its name is misleading; rename the function seeOnlyItems to seeItems (or seeAllItems) wherever it's declared and imported/used, leaving the body that iterates over names and calls I.see(text(name)) unchanged, or alternatively extend the implementation: after asserting each provided name with I.see(text(name)) also assert absence of any items not in the list (e.g., query the page for item elements and assert none have text outside the provided names). Update all references to the symbol seeOnlyItems to the new name (seeItems/seeAllItems) to keep code consistent.
🤖 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/pages/timer/testing.ts`:
- Around line 47-54: enterCustomDurationByBlur currently fills but never blurs,
and enterCustomDurationByEnter types via I.press without clearing existing
content which can append stale text; update enterCustomDurationByBlur to
explicitly blur timerLoc.customInput after filling (use
I.blur(timerLoc.customInput)), and change enterCustomDurationByEnter to clear
the field first then type the new value and submit (use a clear action like
I.clearField or select-all+backspace on timerLoc.customInput, then type/fill the
value and send '[Enter]'); reference the helpers enterCustomDurationByBlur and
enterCustomDurationByEnter and the selector timerLoc.customInput when making the
changes.
---
Nitpick comments:
In `@src/app/integration/Items.stories.tsx`:
- Around line 274-283: The test FilteredToEmpty.test currently mixes product
behavior checks with a verification of the soft-assert API (using I.hopeThat and
expecting I.hopeThat.noErrors() to throw); move the soft-assert verification
into the actor self-tests (e.g., src/shared/test/actor.test.stories.tsx) and
simplify FilteredToEmpty.test to only assert UI behavior: keep the initial
positive visibility checks (I.seeItem('Wireless Headphones'),
I.seeItem('Standing Desk')), apply the filters with
I.applyCategoryFilter('Electronics') and I.applyStockFilter('Out of Stock'),
then assert the resulting empty-state behavior (expect I.hopeThat(() =>
I.seeItem(...)).toBe(false) or a direct empty-state check) and remove the
I.hopeThat.noErrors() throw expectation from this integration story.
In `@src/pages/items/testing.ts`:
- Around line 54-58: The helper seeOnlyItems currently only asserts presence of
each provided name but its name is misleading; rename the function seeOnlyItems
to seeItems (or seeAllItems) wherever it's declared and imported/used, leaving
the body that iterates over names and calls I.see(text(name)) unchanged, or
alternatively extend the implementation: after asserting each provided name with
I.see(text(name)) also assert absence of any items not in the list (e.g., query
the page for item elements and assert none have text outside the provided
names). Update all references to the symbol seeOnlyItems to the new name
(seeItems/seeAllItems) to keep code consistent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4564bf94-ef36-4d31-b6c1-972512f6bb61
📒 Files selected for processing (24)
.config/mise/conf.d/tasks-quality.tomldocs/testing.mddocs/tooling.mdsrc/app/integration/Articles.stories.tsxsrc/app/integration/Calculator.stories.tsxsrc/app/integration/Items.stories.tsxsrc/app/integration/Timer.stories.tsxsrc/pages/articles/testing.tssrc/pages/articles/ui/ArticleStatusBadge.stories.tsxsrc/pages/articles/ui/ArticlesPageLoading.stories.tsxsrc/pages/articles/ui/detail/ArticleDetail.stories.tsxsrc/pages/articles/ui/detail/ArticleDetailLoadingState.stories.tsxsrc/pages/articles/ui/detail/ArticleNoSelection.stories.tsxsrc/pages/articles/ui/detail/ArticleNotFound.stories.tsxsrc/pages/articles/ui/list/ArticleList.stories.tsxsrc/pages/articles/ui/list/ArticleListItem.stories.tsxsrc/pages/articles/ui/list/ArticleListLoading.stories.tsxsrc/pages/calculator/testing.tssrc/pages/calculator/ui/CalculatorPage.stories.tsxsrc/pages/items/testing.tssrc/pages/items/ui/ItemsPage.stories.tsxsrc/pages/timer/testing.tssrc/pages/timer/ui/TimerPage.stories.tsxsrc/pages/timer/ui/TimerPage.tsx
💤 Files with no reviewable changes (12)
- src/pages/articles/ui/list/ArticleListLoading.stories.tsx
- src/pages/articles/ui/list/ArticleList.stories.tsx
- src/pages/timer/ui/TimerPage.stories.tsx
- src/pages/articles/ui/detail/ArticleDetailLoadingState.stories.tsx
- src/pages/articles/ui/detail/ArticleDetail.stories.tsx
- src/pages/calculator/ui/CalculatorPage.stories.tsx
- src/pages/articles/ui/ArticlesPageLoading.stories.tsx
- src/pages/articles/ui/detail/ArticleNoSelection.stories.tsx
- src/pages/articles/ui/ArticleStatusBadge.stories.tsx
- src/pages/articles/ui/detail/ArticleNotFound.stories.tsx
- src/pages/items/ui/ItemsPage.stories.tsx
- src/pages/articles/ui/list/ArticleListItem.stories.tsx
| enterCustomDurationByBlur: async (value: string) => { | ||
| await I.fill(timerLoc.customInput, value) | ||
| }, | ||
| enterCustomDurationByEnter: async (value: string) => { | ||
| await I.click(timerLoc.customInput) | ||
| await I.press(value) | ||
| await I.press('[Enter]') | ||
| }, |
There was a problem hiding this comment.
enterCustomDurationByBlur never blurs, and Enter-path may append stale input.
This can make duration commit tests pass/fail for the wrong reason. Explicitly blur in the blur helper and clear before typing in the Enter helper.
💡 Proposed fix
enterCustomDurationByBlur: async (value: string) => {
- await I.fill(timerLoc.customInput, value)
+ await I.click(timerLoc.customInput)
+ await I.clear(timerLoc.customInput)
+ await I.fill(timerLoc.customInput, value)
+ await I.press('[Tab]')
},
enterCustomDurationByEnter: async (value: string) => {
await I.click(timerLoc.customInput)
+ await I.clear(timerLoc.customInput)
await I.press(value)
await I.press('[Enter]')
},#!/bin/bash
set -euo pipefail
timer_page="$(fd -a 'TimerPage.tsx' | head -n1)"
echo "Inspecting: ${timer_page}"
# Verify how custom duration is committed (blur, enter, change handlers)
rg -n -C3 'onBlur|onChange|onInput|onKeyDown|Enter|duration|custom' "${timer_page}"
# Verify how helpers are currently used in stories
rg -n -C2 'enterCustomDurationByBlur|enterCustomDurationByEnter|clearCustomDuration' src/app/integration/Timer.stories.tsx🤖 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/pages/timer/testing.ts` around lines 47 - 54, enterCustomDurationByBlur
currently fills but never blurs, and enterCustomDurationByEnter types via
I.press without clearing existing content which can append stale text; update
enterCustomDurationByBlur to explicitly blur timerLoc.customInput after filling
(use I.blur(timerLoc.customInput)), and change enterCustomDurationByEnter to
clear the field first then type the new value and submit (use a clear action
like I.clearField or select-all+backspace on timerLoc.customInput, then
type/fill the value and send '[Enter]'); reference the helpers
enterCustomDurationByBlur and enterCustomDurationByEnter and the selector
timerLoc.customInput when making the changes.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests