Add information about daily notable deaths #444
Conversation
WalkthroughThe changes update the backend's status controller by adding daily data fetching capabilities. A new 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
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 resetTodayDeces = async () => { | ||
| let today = new Date().toISOString().split('T')[0].replaceAll("-","") | ||
| const response = await axios(`http://elasticsearch:9200/deces/_search?q=DATE_DECES:${today}`); | ||
| const records = response.data.hits.hits.filter((item: any) => item._id in wikidata) | ||
| if (records.length > 0) { | ||
| return records.map((item: any) => buildResultSingle(item)) | ||
| } else { | ||
| return [] | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type definitions to resetTodayDeces.
The function needs improvements in error handling and type safety.
Apply these improvements:
-const resetTodayDeces = async () => {
+const resetTodayDeces = async (): Promise<Array<any>> => {
try {
let today = new Date().toISOString().split('T')[0].replaceAll("-","")
- const response = await axios(`http://elasticsearch:9200/deces/_search?q=DATE_DECES:${today}`);
+ const response = await axios({
+ url: `http://elasticsearch:9200/deces/_search`,
+ params: {
+ q: `DATE_DECES:${today}`
+ },
+ timeout: 5000
+ });
const records = response.data.hits.hits.filter((item: any) => item._id in wikidata)
if (records.length > 0) {
return records.map((item: any) => buildResultSingle(item))
} else {
return []
}
+ } catch (error) {
+ console.error('Failed to fetch today\'s deaths:', error);
+ return [];
+ }
}📝 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 resetTodayDeces = async () => { | |
| let today = new Date().toISOString().split('T')[0].replaceAll("-","") | |
| const response = await axios(`http://elasticsearch:9200/deces/_search?q=DATE_DECES:${today}`); | |
| const records = response.data.hits.hits.filter((item: any) => item._id in wikidata) | |
| if (records.length > 0) { | |
| return records.map((item: any) => buildResultSingle(item)) | |
| } else { | |
| return [] | |
| } | |
| } | |
| const resetTodayDeces = async (): Promise<Array<any>> => { | |
| try { | |
| let today = new Date().toISOString().split('T')[0].replaceAll("-","") | |
| const response = await axios({ | |
| url: `http://elasticsearch:9200/deces/_search`, | |
| params: { | |
| q: `DATE_DECES:${today}` | |
| }, | |
| timeout: 5000 | |
| }); | |
| const records = response.data.hits.hits.filter((item: any) => item._id in wikidata) | |
| if (records.length > 0) { | |
| return records.map((item: any) => buildResultSingle(item)) | |
| } else { | |
| return [] | |
| } | |
| } catch (error) { | |
| console.error("Failed to fetch today's deaths:", error); | |
| return []; | |
| } | |
| } |
| 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.
Summary by CodeRabbit