Performance and code structure improvements

[Aiiion]

Summary

• Perf: moveRequestLogsToProcessing now uses a single Redis MULTI/EXEC transaction instead of N serial RPOPLPUSH round-trips (up to 40 → 1)
• Structure: collectProvider in weatherAggregator.service.mjs refactored to return a value instead of mutating shared arrays, eliminating the 7-argument signature
• Structure: Removed try/catch from requestLogs and errorLogs controllers — Express 5 auto-catches async rejections and routes to the global handleError middleware, fixing the inconsistent error response shape and ensuring failures are written to error_logs
• Tests: SMHI and MET services are now mocked with fixtures in api.test.mjs, making the suite fully deterministic (no more real HTTP calls to external APIs)
• Tests: WeatherAPI outage test strengthened to assert the errors array is populated and providers correctly excludes the failed provider for both currentWeather and forecastWeather

Test plan

• Full test suite passes in CI (Postgres + Redis required for integration tests)
• weatherAggregator.service.test.mjs — all aggregator unit tests pass
• api.test.mjs — WeatherAPI outage test passes with new assertions

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved weather handling when providers fail by returning clear provider-specific error details while still returning usable results when possible.
  • Standardized the “all providers failed” response for both current and forecast weather, including consistent error information.

• Performance / Reliability

  • Streamlined request-log and error-log listing with efficient pagination and optional filtering.

• Tests

  • Expanded API test coverage using provider fixtures, including verification of provider exclusion and error reporting behavior when WeatherAPI fails.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a3a7b9e2-3d98-4f73-9441-c66cde6e5cb8

📥 Commits

Reviewing files that changed from the base of the PR and between 3b4e286 and acba2ba.

📒 Files selected for processing (2)

• src/services/weatherAggregator.service.mjs
• src/tests/api.test.mjs

🚧 Files skipped from review as they are similar to previous changes (2)

• src/tests/api.test.mjs
• src/services/weatherAggregator.service.mjs

---

📝 Walkthrough

Walkthrough

Two log controllers (errorLogs, requestLogs) have their local try/catch blocks removed, delegating errors to Express middleware. The Redis moveRequestLogsToProcessing function is rewritten from a RPOPLPUSH while-loop to a single MULTI transaction. The weather aggregator gains a collectProvider helper that centralizes settled-result normalization and error logging, used by both processCurrentWeather and processForecastWeather. Tests add SMHI/MET mocks and expand the WeatherAPI-down scenario with provider and error array assertions.

Changes

Log Controller Error Handling Delegation

Layer / File(s)	Summary
Remove try/catch from log controllers src/controllers/v1/errorLogs.controller.mjs, src/controllers/v1/requestLogs.controller.mjs	Both index handlers drop local try/catch and the devError import/log. Pagination, where filtering, and 200 response shape are unchanged; errors now propagate to outer middleware.

Redis MULTI Transaction Refactor

Layer / File(s)	Summary
Atomic list-move with MULTI src/services/redis.service.mjs	moveRequestLogsToProcessing replaces an iterative RPOPLPUSH loop with a pipelined multi()/exec() of count lMove calls, returning the number of non-null results.

Weather Aggregator collectProvider Refactor + Tests

Layer / File(s)	Summary
collectProvider helper and processCurrentWeather refactor src/services/weatherAggregator.service.mjs	New collectProvider maps a PromiseSettledResult to { source, provider } or { error: { provider, message } } and calls logError on rejection. processCurrentWeather builds a collected array from it and derives sources, providers, and errors.
processForecastWeather refactor with timezone src/services/weatherAggregator.service.mjs	Removes manual array inits and replaces per-provider settled branching with collectProvider calls; passes the computed timezone to SMHI/MET DTO functions.
SMHI/MET mocks and WeatherAPI-down test src/tests/api.test.mjs	Adds fixture imports, smhiMocks/metMocks definitions, Jest module registrations, beforeEach resets, and a WeatherAPI-down test that asserts weatherapi.com is absent from providers and present in errors for both weather sections.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Aiiion/express-api#16: Modifies the same src/tests/api.test.mjs weather provider mock/test setup that this PR extends.
• Aiiion/express-api#47: Wires the SMHI provider into the weather aggregator; this PR's collectProvider refactor and SMHI mock additions build directly on that integration.
• Aiiion/express-api#48: Restructures processCurrentWeather/processForecastWeather in the same aggregator file that this PR further refactors with the collectProvider helper.

Poem

🐇 Hop, hop! No more catch to trip,
The errors now flow with a graceful skip.
A MULTI transaction, atomic and neat,
collectProvider makes the weather complete.
One helper to gather the fulfilled and failed—
This bunny's refactor has beautifully sailed! 🌤️

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Performance and code structure improvements' is partially related to the changeset—it accurately describes real improvements in performance (Redis optimization) and code structure (refactoring with DRY principles), but it does not capture the significant test coverage and suite improvements that are equally important to the PR objectives.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch CI.mvp

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 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/services/weatherAggregator.service.mjs`:
- Around line 419-428: The collectProvider function has two error-handling
issues that need fixing. First, on line 425, the fallback chain for the error
message uses String(result.reason) ?? 'Unknown error', but this is ineffective
because String() always returns a string; simplify this to just
result.reason?.message ?? String(result.reason) to properly handle cases where
the message property doesn't exist. Second, on line 421, if the dtoFn call
throws an exception (e.g., from invalid DTO data), the exception propagates
unhandled; wrap the dtoFn(result.value) call in a try/catch block to catch any
errors, log them using logError with the route context, and return an error
object in the same format as the rejected promise handler for consistency.

In `@src/tests/api.test.mjs`:
- Around line 167-185: The test for "should still return 200 with valid weather
data when WeatherAPI is down" is failing because the cache middleware returns
stale cached data from a previous test with the same query parameters
(exampleLatLon), preventing the mockRejectedValueOnce calls on
weatherApiMocks.currentWeather and weatherApiMocks.forecastWeather from being
reached. Fix this by clearing the Redis cache in the beforeEach hook (for
example by mocking getJsonValueMock.mockResolvedValue(null) or by calling a
cache flush method), ensuring each test starts with a clean cache state
independent of previous test results.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: bb496aee-6b78-4144-9880-5d79763e24c1

📥 Commits

Reviewing files that changed from the base of the PR and between 4000a34 and 3b4e286.

📒 Files selected for processing (5)

• src/controllers/v1/errorLogs.controller.mjs
• src/controllers/v1/requestLogs.controller.mjs
• src/services/redis.service.mjs
• src/services/weatherAggregator.service.mjs
• src/tests/api.test.mjs