refactor(code-quality): apply code review feedback for naming conventions and readability - #5
vidyashreebv wants to merge 4 commits into
Conversation
Change SummaryThis PR refactors the counter UI components to improve naming consistency and readability across the app. File Changes
|
PR ScorecardScoreScoring MethodologyCommunication ScoreThe overall communication score is a weighted average:
Formula: Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
Each category score is the average of its subcategories. Score Bands: 90-100 Excellent | 75-89 Good | 60-74 Adequate | 40-59 Needs Work | 0-39 Critical PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
|
![mergemitra[bot]](https://avatars.githubusercontent.com/in/2493799?s=60&v=4){kind=small}
| it('renders with primary variant by default', () => { | ||
| render(<Button>Click me</Button>) | ||
| const button = screen.getByText('Click me') | ||
| expect(button).toHaveClass('bg-blue-600') | ||
| expect(button).toHaveClass('bg-green-600') | ||
| }) |
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Test Design: Tailwind class assertions are brittle
Asserting bg-green-600 directly couples tests to styling decisions rather than behavior.
Why this matters: Refactors like design tweaks or class renames can break tests even though the component still behaves correctly.
Recommended approach: Keep behavior-focused tests (click, disabled, a11y). If you want to keep variant coverage, consider asserting a stable signal (e.g., a data-variant attribute) instead of exact Tailwind class strings.
| const [key, setKey] = useState(0) | ||
|
|
||
| const handleApply = () => { | ||
| const handleClickApply = () => { | ||
| // Force Counter to re-render with new props by changing key | ||
| setKey(prevKey => prevKey + 1) | ||
| } |
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Architecture: Remounting via key to "apply" settings couples UI to lifecycle
Using a key flip to force remount works, but it tightly couples the settings panel behavior to Counter mount semantics.
Why this matters: As features grow (e.g., preserving count history, animations, persistence), lifecycle-coupled resets become harder to reason about than explicit state transitions.
Recommended approach (explicit reset signal rather than remount):
// App
const [resetToken, setResetToken] = useState(0)
const handleClickApply = () => {
// ...set initialValue/step...
setResetToken(prev => prev + 1)
}
<Counter initialValue={initialValue} step={step} resetToken={resetToken} />
// Counter
useEffect(() => {
setCount(initialValue)
}, [initialValue, resetToken])This keeps the component instance stable while making the reset an explicit behavior.
| @@ -1,16 +1,24 @@ | |||
| import { useState } from 'react' | |||
| import Counter from './components/organisms/Counter/Counter' | |||
| import { Counter } from './components/organisms/Counter/Counter' | |||
There was a problem hiding this comment.
Severity: 🟠 Major
Consistency: Named export switch can be breaking if any import sites were missed
Button, Display, and Counter were converted to named exports; any remaining default-import usage elsewhere in the repo will fail at runtime/build time.
Why this matters: This kind of break is easy to miss if a path isn’t hit by tests or local manual flows.
Suggested fix: Do a repo-wide search for default imports and update them (or ensure tests/build already cover every entry point). For example:
rg "import\s+\w+\s+from\s+'.*components/atoms/Button/Button'"
rg "import\s+\w+\s+from\s+'.*components/atoms/Display/Display'"
rg "import\s+\w+\s+from\s+'.*components/organisms/Counter/Counter'"If these modules are consumed outside this app (e.g., published/shared), consider temporarily keeping export default as a compatibility layer during the transition.
| const handleOnChangeInitialValue = (event) => { | ||
| setInitialValue(Number(event.target.value)) | ||
| } | ||
|
|
||
| const handleOnChangeStepValue = (event) => { | ||
| setStep(Number(event.target.value)) | ||
| } |
There was a problem hiding this comment.
Severity: 💭 Minor [nitpick]
Clarity: Handler names read awkwardly (handleOnChangeInitialValue)
handleOnChangeInitialValue / handleOnChangeStepValue are a bit hard to scan compared to the common handleXChange pattern, especially next to handleClickApply.
Why this matters: Handlers are read constantly during UI work; small naming improvements compound over time.
Suggested fix:
const handleInitialValueChange = (event) => {
setInitialValue(Number(event.target.value))
}
const handleStepChange = (event) => {
setStep(Number(event.target.value))
}(and update JSX to onChange={handleInitialValueChange} / onChange={handleStepChange}).
| const handleClickIncrement = () => setCount(count + step) | ||
| const handleClickDecrement = () => setCount(count - step) | ||
| const handleClickReset = () => setCount(initialValue) |
There was a problem hiding this comment.
Severity: 🟠 Major
Best Practices: Use functional state updates for counter math
setCount(count + step)/setCount(count - step) closes over a potentially stale count value.
Why this matters: With React batching/concurrent rendering (or if you later trigger multiple updates in one tick), stale closures can cause missed increments/decrements.
Suggested fix:
const handleClickIncrement = () => setCount(prevCount => prevCount + step)
const handleClickDecrement = () => setCount(prevCount => prevCount - step)handleClickReset can remain setCount(initialValue) since it’s an explicit reset.
| const handleOnChangeInitialValue = (event) => { | ||
| setInitialValue(Number(event.target.value)) | ||
| } | ||
|
|
||
| const handleOnChangeStepValue = (event) => { | ||
| setStep(Number(event.target.value)) | ||
| } |
There was a problem hiding this comment.
Severity: 🟠 Major
Robustness: Guard against invalid/partial numeric input (NaN/""/"-")
Number(event.target.value) can produce NaN (or surprising values) for intermediate/invalid states in <input type="number"> (e.g., -, ., 1e), which then propagates into Counter math and the UI.
Why this matters: Once initialValue/step becomes NaN, the counter can render NaN and all subsequent arithmetic stays NaN, effectively breaking the primary flow.
Suggested fix (store draft strings; parse/validate on Apply):
const toFiniteNumber = (value, fallback) => {
const parsed = Number(value)
return Number.isFinite(parsed) ? parsed : fallback
}
function App() {
const [initialValue, setInitialValue] = useState(10)
const [step, setStep] = useState(5)
const [initialValueInput, setInitialValueInput] = useState('10')
const [stepInput, setStepInput] = useState('5')
const handleInitialValueChange = (event) => {
setInitialValueInput(event.target.value)
}
const handleStepChange = (event) => {
setStepInput(event.target.value)
}
const handleClickApply = () => {
setInitialValue(toFiniteNumber(initialValueInput, 0))
setStep(toFiniteNumber(stepInput, 1))
setKey(prevKey => prevKey + 1)
}
}This keeps the inputs editable in intermediate states while ensuring only valid numbers reach Counter.
PR AnalysisFocus Areas for Architect Review
RecommendationAction: NEEDS DISCUSSION Quick Wins:
Author InsightsPR Type: refactor Missing Skills
Strengths
|
vidya-cw
added
resolved PR comments
Closed and Can't merge
3 participants
What does this PR do?
eto fulleventfor consistencybuttonClassNamevariable for better readabilitydisplayAriaLabelvariable for better readabilityWhat steps does your reviewer have to take to test this PR manually?
refactor/apply-code-review-feedbackbranch and runnpm installnpm run devto start the development server and verify the app works correctlynpm testto verify all 27 tests passScreenshots
Pull Request standards checklist - Please check off
Testing checklist - Please check off
Definition of Done - Please check off