Add information about daily notable deaths #444
Conversation
Update dependencies
🥅 Handle erros in updated page
WalkthroughThe changes add daily data fetching and caching of death records in the backend's status controller. A new asynchronous function Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SC as StatusController
participant ES as ElasticSearch
Client->>SC: Request version info
SC->>SC: Check if todayDeces exists
alt todayDeces is empty
SC->>ES: Query for today's records
ES-->>SC: Return records
SC->>SC: Filter and map records (using wikidata & buildResultSingle)
else
Note over SC: Use cached todayDeces value
end
SC-->Client: Return response with version info and todayDeces
Note over SC: resetAtMidnight schedules a reset at midnight daily
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| import { wikidata } from '../wikidata'; | ||
| import { buildResultSingle } from '../models/result'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving state management inside the class.
The todayDeces variable is declared outside the class scope. This could lead to state management issues if multiple instances of the controller are created.
Consider these improvements:
- Move the variable inside the class
- Add proper typing instead of
[]
-let todayDeces: [];
@Route('')
export class StatusController extends Controller {
+ private todayDeces: Array<any> = [];Also applies to: 11-11
| if (! todayDeces) { | ||
| todayDeces = await resetTodayDeces() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider parallelizing requests.
The version method could be improved in terms of error handling and performance.
- Add error handling for
resetTodayDeces - Consider parallelizing the independent requests
- if (! todayDeces) {
- todayDeces = await resetTodayDeces()
- }
+ if (!todayDeces) {
+ try {
+ todayDeces = await resetTodayDeces();
+ } catch (error) {
+ console.error('Failed to fetch today\'s deaths:', error);
+ todayDeces = [];
+ }
+ }Consider using Promise.all to parallelize the requests:
const [countResponse, lastRecordResponse, updateResponse, todayResponse] = await Promise.all([
!uniqRecordsCount ? axios(`http://elasticsearch:9200/deces/_count`) : null,
(!lastRecordDate || !lastDataset) ? axios(`http://elasticsearch:9200/deces/_search?sort=SOURCE:desc,DATE_DECES.raw:desc&size=1`) : null,
!updateDate ? axios(`http://elasticsearch:9200/_cat/indices/deces?h=creation.date.string`) : null,
!todayDeces ? resetTodayDeces() : null
]);| const resetAtMidnight = () => { | ||
| const now = new Date(); | ||
| const night = new Date( | ||
| now.getFullYear(), | ||
| now.getMonth(), | ||
| now.getDate() + 1, | ||
| 0, 0, 0 | ||
| ); | ||
| const msToMidnight = night.getTime() - now.getTime(); | ||
|
|
||
| setTimeout(() => { | ||
| resetTodayDeces(); | ||
| resetAtMidnight(); | ||
| }, msToMidnight); | ||
| } | ||
|
|
||
| resetAtMidnight(); |
There was a problem hiding this comment.
Address potential memory leaks in resetAtMidnight.
The current implementation could lead to memory leaks and lacks proper error handling.
Consider these improvements:
+let midnightTimer: NodeJS.Timeout;
+
const resetAtMidnight = () => {
const now = new Date();
const night = new Date(
now.getFullYear(),
now.getMonth(),
now.getDate() + 1,
0, 0, 0
);
const msToMidnight = night.getTime() - now.getTime();
- setTimeout(() => {
- resetTodayDeces();
- resetAtMidnight();
- }, msToMidnight);
+ // Clear existing timer to prevent memory leaks
+ if (midnightTimer) {
+ clearTimeout(midnightTimer);
+ }
+
+ midnightTimer = setTimeout(async () => {
+ try {
+ todayDeces = await resetTodayDeces();
+ resetAtMidnight();
+ } catch (error) {
+ console.error('Failed to reset at midnight:', error);
+ // Retry in 5 minutes if failed
+ setTimeout(resetAtMidnight, 5 * 60 * 1000);
+ }
+ }, msToMidnight);
}
+// Add cleanup method
+export const cleanup = () => {
+ if (midnightTimer) {
+ clearTimeout(midnightTimer);
+ }
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resetAtMidnight = () => { | |
| const now = new Date(); | |
| const night = new Date( | |
| now.getFullYear(), | |
| now.getMonth(), | |
| now.getDate() + 1, | |
| 0, 0, 0 | |
| ); | |
| const msToMidnight = night.getTime() - now.getTime(); | |
| setTimeout(() => { | |
| resetTodayDeces(); | |
| resetAtMidnight(); | |
| }, msToMidnight); | |
| } | |
| resetAtMidnight(); | |
| let midnightTimer: NodeJS.Timeout; | |
| const resetAtMidnight = () => { | |
| const now = new Date(); | |
| const night = new Date( | |
| now.getFullYear(), | |
| now.getMonth(), | |
| now.getDate() + 1, | |
| 0, 0, 0 | |
| ); | |
| const msToMidnight = night.getTime() - now.getTime(); | |
| // Clear existing timer to prevent memory leaks | |
| if (midnightTimer) { | |
| clearTimeout(midnightTimer); | |
| } | |
| midnightTimer = setTimeout(async () => { | |
| try { | |
| todayDeces = await resetTodayDeces(); | |
| resetAtMidnight(); | |
| } catch (error) { | |
| console.error('Failed to reset at midnight:', error); | |
| // Retry in 5 minutes if failed | |
| setTimeout(resetAtMidnight, 5 * 60 * 1000); | |
| } | |
| }, msToMidnight); | |
| } | |
| resetAtMidnight(); | |
| // Add cleanup method | |
| export const cleanup = () => { | |
| if (midnightTimer) { | |
| clearTimeout(midnightTimer); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/src/controllers/status.controller.ts (4)
14-21: Add type definition for the log function parameter.The
logfunction's parameter should have a more specific type thananyto ensure type safety.-const log = (json:any) => { +interface LogData { + resetTodayDecesError?: unknown; + [key: string]: unknown; +} +const log = (json: LogData): void => {
74-82: Define a proper interface for the version endpoint response.The response type is currently
any, which reduces type safety. Define an interface for the response structure.+interface VersionResponse { + backend: string; + uniqRecordsCount: number; + lastRecordDate: string; + lastDataset: string; + updateDate: string; + todayDeces: Array<any>; // Replace 'any' with proper type from buildResultSingle +} - public async version(): Promise<any> { + public async version(): Promise<VersionResponse> {
92-92: Consider making the size limit configurable.The hardcoded size limit of 3 might be too restrictive. Consider making it configurable through environment variables.
+const DAILY_DEATHS_LIMIT = process.env.DAILY_DEATHS_LIMIT || '10'; + const resetTodayDeces = async (): Promise<Array<any>> => { try { const response = await axios({ url: `http://elasticsearch:9200/deces/_search`, params: { q: `DATE_DECES:${today}`, - size: 3 + size: parseInt(DAILY_DEATHS_LIMIT, 10) },
124-124: Control the initialization of the midnight reset.The immediate invocation of
resetAtMidnightmight cause issues during testing or server restarts. Consider controlling this through the class initialization.-resetAtMidnight(); +export const initializeResetSchedule = () => { + resetAtMidnight(); +};Then initialize it in your server startup code where you have better control over the timing and can skip it during tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/controllers/status.controller.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: 🐳 Build docker image
| const resetAtMidnight = () => { | ||
| const now = new Date(); | ||
| const night = new Date( | ||
| now.getFullYear(), | ||
| now.getMonth(), | ||
| now.getDate() + 1, | ||
| 0, 0, 0 | ||
| ); | ||
| const msToMidnight = night.getTime() - now.getTime(); | ||
|
|
||
| setTimeout(() => { | ||
| resetTodayDeces(); | ||
| resetAtMidnight(); | ||
| }, msToMidnight); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timezone handling for accurate midnight calculation.
The midnight calculation doesn't account for timezones, which could cause issues in different regions.
+const TIMEZONE = process.env.TIMEZONE || 'UTC';
+
const resetAtMidnight = () => {
- const now = new Date();
+ const now = new Date().toLocaleString('en-US', { timeZone: TIMEZONE });
const night = new Date(
now.getFullYear(),
now.getMonth(),
now.getDate() + 1,
0, 0, 0
);
+ // Convert to timezone-specific timestamp
+ night.setTime(night.getTime() + night.getTimezoneOffset() * 60 * 1000);
const msToMidnight = night.getTime() - now.getTime();Committable suggestion skipped: line range outside the PR's diff.
rhanka
force-pushed
the
dev
branch
2 times, most recently
from
67eba46 to
0ebe212
Compare
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/controllers/search.spec.ts(2 hunks)backend/src/controllers/status.controller.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/controllers/status.controller.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/src/controllers/search.spec.ts (1)
backend/src/controllers/status.controller.ts (1)
resetTodayDeces(130-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: 🐳 Build docker image
| @@ -1,4 +1,5 @@ | |||
| import { SearchController } from './search.controller'; | |||
| import { resetTodayDeces } from './status.controller'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move test to the appropriate file.
The import suggests that this test file is testing functionality from status.controller.ts, but the tests should be colocated with their respective controllers.
Consider moving the test to status.spec.ts instead of importing the function here.
🤖 Prompt for AI Agents
In backend/src/controllers/search.spec.ts at line 2, the test imports a function
from status.controller, indicating it tests status controller functionality. To
keep tests organized, move this test code from search.spec.ts to status.spec.ts
so that tests are colocated with their respective controllers. Remove the import
and related tests from search.spec.ts after moving.
| describe('status.controller.ts - today deaths', () => { | ||
| it('today deaths', async () => { | ||
| const todayDeces = await resetTodayDeces('20200620'); | ||
| expect(todayDeces.length).to.equal(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test robustness and relocate to appropriate file.
This test has several issues that should be addressed:
- Wrong location: The test is in
search.spec.tsbut tests functionality fromstatus.controller.ts - Brittle assertion: Expecting exactly 1 result for a hardcoded date could fail if test data changes
- Limited validation: Only checks array length, not the actual content or structure
Move this test to status.spec.ts and improve the test:
-describe('status.controller.ts - today deaths', () => {
- it('today deaths', async () => {
- const todayDeces = await resetTodayDeces('20200620');
- expect(todayDeces.length).to.equal(1);
- });
-});In backend/src/controllers/status.spec.ts:
describe('status.controller.ts - today deaths', () => {
it('should return array of death records for valid date', async () => {
const todayDeces = await resetTodayDeces('20200620');
expect(Array.isArray(todayDeces)).to.equal(true);
expect(todayDeces.length).to.be.greaterThanOrEqual(0);
expect(todayDeces.length).to.be.lessThanOrEqual(3);
// Verify structure if records exist
if (todayDeces.length > 0) {
expect(todayDeces[0]).to.have.property('name');
expect(todayDeces[0]).to.have.property('death');
}
});
it('should return empty array for invalid date', async () => {
const todayDeces = await resetTodayDeces('invalid-date');
expect(todayDeces).to.be.an('array');
expect(todayDeces.length).to.equal(0);
});
});🤖 Prompt for AI Agents
In backend/src/controllers/search.spec.ts around lines 157 to 162, the test for
today deaths is misplaced and fragile. Move this test to
backend/src/controllers/status.spec.ts and improve it by checking that the
result is an array, validating the length is within a reasonable range, and
verifying the structure of the records if any exist. Also, add a test case for
invalid dates that expects an empty array. This will make the tests more robust
and correctly located.
Summary by CodeRabbit