feat: add cleanup logic for weather api fetch using useEffect

[SaumyaDwivedi179]

What This PR Does

• Adds cleanup logic in App.jsx for the Weather API fetch using useEffect.
• Implements an ignore flag to prevent state updates if the component unmounts or if a new city search is triggered before the previous fetch completes.
• Adds array dependency [cityName] to run the effect only when the searched city changes.
• Keeps the weather search and card components fully functional.
• Logs key events (fetch start, fetch success, errors, and cleanup) in the console for easier debugging and verification.

Manual Testing Steps (Reviewer)

1. Run npm install & npm run dev to start the app.
2. Enter a city name in the search and click Search:
   • Verify the console logs Fetching weather… and Weather fetched: with the correct data.
3. Quickly type another city and search:
   • Confirm that previous fetches don’t update the state incorrectly.
4. Verify the loading indicator appears while fetching and disappears afterward.
5. Confirm the weather card updates correctly with the new city.
6. Ensure no console warnings or errors appear during testing.

Files Changed

• src/App.jsx and src/test/App.test.jsx→ Added cleanup logic using useEffect with ignore flag and [cityName] dependency.

✅ Definition of Done

Code Quality

• Clean, readable, and well-structured code
• Cleanup logic implemented safely for async fetches
• Removed unnecessary comments and redundant code

Testing

• Manual testing completed for cleanup functionality
• Verified that weather API features still work with multiple searches
• Confirmed no regressions or state update issues
• Console logs verify fetch and cleanup behavior

[mergemitra]

Change Summary

This PR refactors the weather search flow to store the searched cityName in state and trigger the API fetch via a useEffect that depends on [cityName].
It adds an “ignore” cleanup flag to prevent state updates when the effect re-runs or the component unmounts mid-fetch.
The App component is simplified by removing the unused weatherReports state and stripping theme/toggle wiring from Header, SearchForm, WeatherCard, and footer styling.
Tests are updated to align with the current error message shown for invalid city searches.

Note: The PR description mentions console logging and keeping existing components “fully functional”; the diff does not add logs and removes theme/toggle-related behavior. Also, a large block of the previous App.jsx implementation is left commented out.

File Changes

File	Summary
weather-app/src/App.jsx	Refactors fetching into useEffect with cleanup ignore flag; removes theme and weatherReports usage.
weather-app/src/test/App.test.jsx	Updates invalid-city assertion to match the exact error message emitted by App.jsx.

[mergemitra]

PR Scorecard

Score

Scoring Methodology

Communication Score

The overall communication score is a weighted average:

Dimension	Weight	Evaluates
Description Quality	60%	Title format (conventional commits) + Description clarity (what changed & why)
PR Size & Scope	25%	Appropriate sizing, scope cohesion, and justification for size
Commit Messages	15%	Conventional commits format, atomic & descriptive changes

Formula: (Description x 0.6) + (PR Size x 0.25) + (Commits x 0.15)

Scoring Framework

The scorecard evaluates code using 3 key reviewer questions:

Reviewer Question	Category	Subcategories
Is this the right solution, implemented the right way?	Code Correctness & Design Quality	Correctness, Robustness, Best Practices, Architecture
Would this catch bugs if the code broke tomorrow?	Test Quality & Coverage	Coverage, Test Design
Can someone new understand and safely modify this in 6 months?	Code Readability & Maintainability	Clarity, Structure, Consistency

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 Notes

Description Quality

• ✅ Description explains what changed, why, includes thorough manual testing steps, and a completed DoD checklist
• ❌ Description claims console logs for fetch start/success/error/cleanup but no console.log statements are present in src/App.jsx — either add the logs or remove the claim
• ❌ Files changed line has a typo App.test,jsx — actual modified test path is src/test/App.test.jsx; update the description to match exact paths

PR Size & Scope

• ✅ Small, well-scoped change: ~122 additions / 40 deletions across 2 files (App.jsx + test) focused on fetch cleanup and related tests

Commit Messages

• ❌ 1/1 commits do not follow conventional commits. Example fix: use test(app): add weather component integration test instead of the current message
• ❌ Single-commit PRs rely on that commit message for history; please adopt <type>(<scope>): <description> going forward (no need to rebase/squash here)

Notes

Code Correctness & Design Quality

• ❌🔴 Loading state can get stuck true if a valid-city fetch is in flight and the next search is an invalid city (new effect returns early without clearing isLoading, and old request’s finally won’t clear it due to ignore) at weather-app/src/App.jsx:89-122
• ❌🟠 Header, SearchForm, and WeatherCard props (theme, onThemeToggle) were removed; if those components rely on them this will break runtime behavior/styling and can crash on undefined access at weather-app/src/App.jsx:132-145
• ❌🟡 PR description claims console logging for fetch start/success/error/cleanup, but no logging is present; reviewers can’t verify behavior as described and this is a mismatch with stated deliverable at weather-app/src/App.jsx:89-130
• ❌🟠 The fetch is not actually cancelled (only state updates are ignored), so rapid searches still waste network/compute; use AbortController (and pass signal) to cancel in-flight requests at weather-app/src/App.jsx:89-130
• ❌🟡 Searching the same city twice won’t re-fetch because setCityName() sets the same string value and React may bail out, leaving no new request and surprising users who expect a retry at weather-app/src/App.jsx:85-87
• ❌🟡 useEffect defines and calls startFetching() without handling its returned promise; if it ever throws before internal try/catch, it becomes an unhandled rejection—wrap invocation or ensure all thrown paths are caught at weather-app/src/App.jsx:94-126
• ❌🟠 Theme support appears to have been removed from App (state + prop plumbing) while still referencing theme-specific styling previously; this couples unrelated “cleanup logic” work with a potentially breaking UI behavior change at weather-app/src/App.jsx:79-145

Test Quality & Coverage

• ❌🟠 No tests cover the new “ignore stale request” behavior (e.g., two searches where the first resolves after the second, ensuring stale results don’t render) at weather-app/src/test/App.test.jsx
• ❌🟠 No tests cover the loading-stuck scenario when switching from a valid city to an invalid city mid-flight (the highest-risk regression introduced by the new effect-based flow) at weather-app/src/test/App.test.jsx
• ❌🟡 Tests use fireEvent instead of userEvent, reducing fidelity vs real user interaction and missing async typing/click nuances at weather-app/src/test/App.test.jsx:1-52
• 💭 [nitpick] The assertions are wrapped in a broad waitFor; prefer awaiting the specific UI change (e.g., findByText) and then asserting the API call to make failures more precise at weather-app/src/test/App.test.jsx:36-43

Code Readability & Maintainability

• ❌🟡 Large blocks of commented-out old implementation make the file harder to read and review, and increase the chance of future accidental reintroduction/merge conflicts at weather-app/src/App.jsx:1-70
• 💭 [nitpick] ignore is a bit vague for intent; consider shouldIgnoreResponse/isStale to make the cleanup logic self-explanatory at weather-app/src/App.jsx:92-129
• ❌🟡 Fetch orchestration, input normalization, and error/display logic are all in App; extracting a small hook (e.g., useWeather(cityName)) would reduce coupling and make race-condition scenarios easier to test at weather-app/src/App.jsx:79-130
• 💭 [nitpick] Mixed quote style and stray whitespace (e.g., useState("") ) reduces consistency and makes diffs noisier at weather-app/src/App.jsx:80

[mergemitra]

Severity: 🟡 Moderate

Robustness: Same-city searches may not re-fetch (state set to same value can be skipped)

handleSearch sets cityName to a normalized string. If the user searches the same city twice, React may bail out (state value unchanged), so the effect won’t rerun and the user can’t “retry”.

Why this matters: Users commonly retry searches after transient failures; “nothing happens” is surprising.

Suggested fix (add a monotonically increasing request token):

const [cityName, setCityName] = useState('')
const [searchRequestId, setSearchRequestId] = useState(0)

const handleSearch = (cityNameInput) => {
  setCityName(cityNameInput.toLowerCase().trim())
  setSearchRequestId((id) => id + 1)
}

useEffect(() => {
  // ... fetch logic
}, [cityName, searchRequestId])

[mergemitra]

Severity: 🟡 Moderate

Correctness: PR description mentions console logging, but code has none

The PR description says fetch start/success/error/cleanup are logged for verification, but the effect currently doesn’t log anything.

Why this matters: Reviewers following the stated manual testing steps can’t validate the behavior as described, and it creates confusion about what’s intended to ship.

Suggested fix (if you want logs, gate them to dev to avoid noisy prod consoles):

const isDebug = import.meta.env.DEV

// before starting
if (isDebug) console.debug(`Fetching weather for: ${cityName}`)

// on success
if (isDebug) console.debug('Weather fetched:', weatherData)

// on error
if (isDebug) console.debug('Weather fetch error:', err)

// in cleanup
return () => {
  if (isDebug) console.debug('Cleanup: component unmounted or effect re-runs')
  ignore = true
}

(Alternatively, update the PR description/testing steps to match the code.)

[mergemitra]

Severity: 🟡 Moderate

Structure: Consider extracting fetch orchestration into a small hook (useWeather)

App currently owns input normalization, request lifecycle, error handling, and rendering. This is workable, but it increases coupling and makes race-condition behavior harder to test in isolation.

Why this matters: A dedicated hook makes cancellation/stale-response logic easier to reason about and unit test, and keeps App focused on composition.

Suggested fix (sketch):

function useWeather(cityName) {
  const [currentWeather, setCurrentWeather] = useState(null)
  const [isLoading, setIsLoading] = useState(false)
  const [errorMessage, setErrorMessage] = useState(null)

  useEffect(() => {
    // move the effect logic here
  }, [cityName])

  return { currentWeather, isLoading, errorMessage }
}

Then App becomes mostly const { currentWeather, isLoading, errorMessage } = useWeather(cityName).

[mergemitra]

Severity: 💭 Minor [nitpick]

Consistency: Mixed quote style + trailing whitespace

useState("") uses double quotes and has trailing whitespace.

Why this matters: Small consistency issues add noise to diffs and make formatting less predictable.

Suggested fix:

const [cityName, setCityName] = useState('')

[mergemitra]

Severity: 🔴 Critical

Correctness: isLoading can get stuck true when switching to an invalid city mid-flight

If a valid-city request has already set isLoading(true), and the next search is an invalid city, the new effect returns early without clearing isLoading. The previous request’s finally won’t clear it because it’s gated behind ignore.

Why this matters: Users can get stuck in a permanent loading state even though the latest search is already known to be invalid.

Suggested fix:

if (!coordinates) {
  setErrorMessage(`City "${cityName}" not found. Try London, Delhi, or Tokyo.`)
  setCurrentWeather(null)
  setIsLoading(false) // ensure prior in-flight request can't leave the spinner stuck
  return
}

(If you adopt an AbortController/request-id approach, make sure the same "clear loading on invalid query" behavior is preserved.)

[mergemitra]

Severity: 🟠 Major

Correctness: Removed props may break Header/SearchForm/WeatherCard at runtime

Header, SearchForm, and WeatherCard are now rendered without theme and onThemeToggle. If those components expect these props (for styling or behavior), this can cause broken UI or runtime errors.

Why this matters: This PR is scoped to fetch cleanup logic; dropping required props risks shipping a functional regression unrelated to the change.

Suggested fix (restore previous plumbing, or update the child components to not require these props):

const [theme, setTheme] = useState('dark')

const handleThemeToggle = () => {
  setTheme((previousTheme) => (previousTheme === 'dark' ? 'light' : 'dark'))
}

return (
  <div className={`${styles.appContainer} ${styles[theme]}`}>
    <div className={styles.mainWrapper}>
      <Header theme={theme} onThemeToggle={handleThemeToggle} />
      <main className={styles.mainSection}>
        <SearchForm onSearch={handleSearch} theme={theme} />
        {isLoading && <p className={styles.statusText}>Searching the skies...</p>}
        {errorMessage && <div className={styles.errorBox}>{errorMessage}</div>}
        {currentWeather && !isLoading && <WeatherCard data={currentWeather} theme={theme} />}
      </main>
      <footer className={`${styles.footer} ${styles[theme]}`}>
        Powered by Open-Meteo API
      </footer>
    </div>
  </div>
)

[mergemitra]

Severity: 💭 Minor [nitpick]

Clarity: ignore is vague; rename to reflect intent

ignore doesn’t communicate what is being ignored (responses/state updates) or under what condition.

Why this matters: Cleanup/race-condition code is already subtle; clearer naming reduces future mistakes.

Suggested fix:

let shouldIgnoreResponse = false

// ...
if (!shouldIgnoreResponse) {
  setCurrentWeather(...)
}

return () => {
  shouldIgnoreResponse = true
}

[mergemitra]

Severity: 🟠 Major

Robustness: Request isn’t cancelled; use AbortController to stop in-flight fetches

The current pattern only ignores state updates; it still performs the network work for stale requests. On rapid searches, this wastes network/CPU and can also keep server-side rate limits hotter than necessary.

Why this matters: Cancelling stale requests reduces wasted work and makes behavior more deterministic under rapid interactions.

Recommended approach (abort on cleanup, and pass signal into the API call):

useEffect(() => {
  if (!cityName) return

  const controller = new AbortController()
  const { signal } = controller

  const startFetching = async () => {
    const coordinates = CITY_COORDS[cityName]

    if (!coordinates) {
      setErrorMessage(`City "${cityName}" not found. Try London, Delhi, or Tokyo.`)
      setCurrentWeather(null)
      setIsLoading(false)
      return
    }

    setIsLoading(true)
    setErrorMessage(null)

    try {
      const weatherData = await fetchWeatherInfoByCoordinates(
        coordinates.lat,
        coordinates.lon,
        { signal }
      )

      setCurrentWeather({
        ...weatherData,
        city: cityName.charAt(0).toUpperCase() + cityName.slice(1)
      })
    } catch (err) {
      if (err?.name === 'AbortError') return
      setErrorMessage(err.message || 'Failed to fetch weather data.')
      setCurrentWeather(null)
    } finally {
      setIsLoading(false)
    }
  }

  void startFetching()
  return () => controller.abort()
}, [cityName])

This likely requires updating fetchWeatherInfoByCoordinates to accept an optional { signal } param and pass it to fetch(...).

[mergemitra]

Severity: 🟡 Moderate

Best Practices: startFetching() promise is not handled at call site

startFetching() is an async function invoked without await/catch. Even though you have a try/catch inside, any throw that happens before it (or a synchronous throw inside fetchWeatherInfoByCoordinates) can still surface as an unhandled rejection.

Why this matters: Unhandled rejections can fail tests, pollute logs, and make real errors harder to diagnose.

Suggested fix:

startFetching().catch((err) => {
  if (ignore) return
  setErrorMessage(err?.message || 'Failed to fetch weather data.')
  setCurrentWeather(null)
  setIsLoading(false)
})

(If you move to AbortController, you can also ignore AbortError here.)

[mergemitra]

Severity: 🟠 Major

Architecture: Theme removal makes this PR do more than “fetch cleanup logic”

This PR removes theme state/styling/props while also introducing new fetch orchestration. That couples two unrelated concerns, making regressions harder to attribute and review.

Why this matters: Keeping changes focused reduces the chance of accidental UI behavior changes and makes rollback/diagnosis much easier.

Suggested fix: revert theme-related changes in this PR (keep theme behavior identical), and ship theme removal (if intended) as a separate PR with its own test updates and component adjustments.

[mergemitra]

Severity: 🟡 Moderate

Clarity: Large commented-out prior implementation should be removed

The file contains ~70 lines of commented-out old code.

Why this matters: It makes review harder, increases merge-conflict risk, and encourages accidental reintroduction. Git history already preserves the old version.

Suggested fix: delete the commented-out block (lines 1–70) entirely. If you need to preserve context, link to the PR/commit in the PR description instead.

[mergemitra]

PR Analysis

Focus Areas for Architect Review

1. Async fetch lifecycle standardization: Decide whether the app should prefer AbortController (and propagate signal through service APIs) vs “ignore stale response” patterns, so cancellation/error semantics are consistent across features.
2. Component responsibility boundaries: This change shifts request orchestration into App; align on whether this belongs in a dedicated hook/service layer to keep UI composition and data-fetch concerns decoupled long-term.
3. Regression-proof testing approach: Confirm whether race-condition behavior should be validated via integration tests at App level or via a hook-level unit test strategy to keep tests less brittle.

Recommendation

Action: REQUEST CHANGES

Quick Wins:

• Explicitly clear loading when the city is invalid to avoid a stuck spinner.
• Handle the async effect invocation (void startFetching() / .catch(...)) to prevent potential unhandled promise rejections.
• Remove the large commented-out legacy App implementation block to reduce review/merge noise.

Author Insights

PR Type: feat

Missing Skills

• Correctness: Loading can stay stuck during fast invalid searches.
• Test Coverage: New cleanup paths are not tested yet.
• Code Clarity: Large commented blocks make the code hard.
• Commit Messages: Commit message format needs to be standard.

Strengths

• Description Quality: Clear goals and strong manual test steps.
• PR Size: Changes are small and easy to review.
• Architecture: Effect runs only when the city changes.