feat: Algolia config error handling#4126
Conversation
Summary by CodeRabbit
WalkthroughAdds a local development guard to the Algolia search function that validates required configuration credentials and returns empty results if missing, along with corresponding test fixtures and test cases to verify the behavior in both local and non-local environments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![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)
frontend/src/server/fetchAlgoliaData.ts (1)
60-64: Consider simplifying the redundantAppErrorcheck.The condition
error instanceof Error && error.name === 'AppError'is redundant sinceerror instanceof AppErroralready covers all properAppErrorinstances. TheAppErrorclass explicitly setsthis.name = 'AppError'in its constructor, so any instance passinginstanceof AppErrorwill also have that name.♻️ Simplify condition
} catch (error) { - if (error instanceof AppError || (error instanceof Error && error.name === 'AppError')) { + if (error instanceof AppError) { throw error } throw new AppError(500, 'Search service error') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/server/fetchAlgoliaData.ts` around lines 60 - 64, In the catch block of fetchAlgoliaData (handling errors from the search call), simplify the redundant check by only testing error instanceof AppError and rethrowing it; remove the extra (error instanceof Error && error.name === 'AppError') branch so non-AppError errors are wrapped with new AppError(500, 'Search service error') as before.backend/tests/apps/core/api/internal/algolia_test.py (1)
30-37: Consider addingENVIRONMENTto the mock settings for robustness.The fixture sets the two Algolia credentials, but
get_search_resultsalso accessessettings.ENVIRONMENT. Current tests work because they mockget_search_resultsor its dependencies, but future tests calling through to the real implementation would get a Mock object forENVIRONMENT.🔧 Suggested improvement
`@pytest.fixture`(autouse=True) def mock_algolia_settings(): """Mock Algolia settings.""" with patch("apps.core.api.internal.algolia.settings") as mock_settings: mock_settings.ALGOLIA_APPLICATION_ID = "test-app-id" mock_settings.ALGOLIA_WRITE_API_KEY = "test-api-key" + mock_settings.ENVIRONMENT = "test" yield mock_settings🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/core/api/internal/algolia_test.py` around lines 30 - 37, The mock_algolia_settings fixture is missing settings.ENVIRONMENT which get_search_results expects; update the fixture (mock_algolia_settings) to set mock_settings.ENVIRONMENT to a concrete string (e.g., "test" or "development") so tests that exercise get_search_results or the real implementation see a real environment value rather than a Mock object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/chapters/page.tsx`:
- Around line 30-51: The useEffect that fetches map data currently varies
hitsPerPage by currentPage and re-runs on [currentPage], causing geoLocData
(used for the map) to be replaced with only 25 items when paginating; update the
effect in page.tsx so it always requests hitsPerPage = 1000 from
fetchAlgoliaData and runs only once on mount (useEffect dependency []), calling
fetchAlgoliaData(indexName: 'chapters', query: '', currentPage: 1, hitsPerPage:
1000) and storing the result via setGeoLocData so the map always has the full
dataset independent of pagination state.
---
Nitpick comments:
In `@backend/tests/apps/core/api/internal/algolia_test.py`:
- Around line 30-37: The mock_algolia_settings fixture is missing
settings.ENVIRONMENT which get_search_results expects; update the fixture
(mock_algolia_settings) to set mock_settings.ENVIRONMENT to a concrete string
(e.g., "test" or "development") so tests that exercise get_search_results or the
real implementation see a real environment value rather than a Mock object.
In `@frontend/src/server/fetchAlgoliaData.ts`:
- Around line 60-64: In the catch block of fetchAlgoliaData (handling errors
from the search call), simplify the redundant check by only testing error
instanceof AppError and rethrowing it; remove the extra (error instanceof Error
&& error.name === 'AppError') branch so non-AppError errors are wrapped with new
AppError(500, 'Search service error') as before.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/core/api/internal/algolia.pybackend/tests/apps/core/api/internal/algolia_test.pyfrontend/src/app/chapters/page.tsxfrontend/src/app/page.tsxfrontend/src/server/fetchAlgoliaData.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Hi @arkid15r, the PR is ready for review. Please let me know if any changes are required. |
|
Hi! Just a quick ping on this PR since the original issue deadline just passed. No rush but please let me know if you need me to make any changes. |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
This might work but seems a bit to much to me.
@Ketan-Agarwal let's narrow it down to local environment and backend only.
When running backend it should just show a good error message notifying about the misconfiguration. Not having Algolia configured should never be the case for frontend.
- Remove redundant error handling - Add `mock_settings.ENVIRONMENT` for Algolia test suite
Ketan-Agarwal
force-pushed
the
feat/algolia-config-error-handling
branch
from
7255954 to
ed3d09f
Compare
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/apps/core/api/internal/algolia_test.py (1)
296-325: Minor: Test name references "warning" but testslogger.error.The test method name
test_algolia_search_logs_warning_when_not_configured_in_localand the docstring mention "warning", but the implementation correctly testslogger.error(line 319), which matches the production code. Consider renaming for consistency:Suggested rename
`@patch`("apps.core.api.internal.algolia.settings") - def test_algolia_search_logs_warning_when_not_configured_in_local(self, mock_settings): - """Test warning is logged and empty results returned locally. + def test_algolia_search_logs_error_when_not_configured_in_local(self, mock_settings): + """Test error is logged and empty results returned locally. Verify that when Algolia is not configured in local environment, - a warning is logged and empty results are returned. + an error is logged and empty results are returned. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/core/api/internal/algolia_test.py` around lines 296 - 325, The test name and docstring for test_algolia_search_logs_warning_when_not_configured_in_local are inconsistent with the assertion that checks logger.error; rename the test method (and update its docstring) to reference "error" instead of "warning" — e.g., test_algolia_search_logs_error_when_not_configured_in_local — and keep the assertion against logger.error and the call to algolia_search unchanged so the behavior and assertions remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/tests/apps/core/api/internal/algolia_test.py`:
- Around line 296-325: The test name and docstring for
test_algolia_search_logs_warning_when_not_configured_in_local are inconsistent
with the assertion that checks logger.error; rename the test method (and update
its docstring) to reference "error" instead of "warning" — e.g.,
test_algolia_search_logs_error_when_not_configured_in_local — and keep the
assertion against logger.error and the call to algolia_search unchanged so the
behavior and assertions remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c52a5cc3-904b-490f-a0d6-ddbdcac8e5e8
📒 Files selected for processing (2)
backend/apps/core/api/internal/algolia.pybackend/tests/apps/core/api/internal/algolia_test.py
2 participants
Proposed change
Resolves #4107
This PR introduces error handling for Algolia search configuration and data fetching across backend and frontend. It specifically addresses cases where Algolia credentials may be missing or incorrectly configured, preventing unhandled exceptions and providing a better contributing experience.
Checklist
make check-testlocally: all warnings addressed, tests passed