-
-
Notifications
You must be signed in to change notification settings - Fork 0
refactor(Counter): recreate component
#3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,35 +1,251 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "use client"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import React, { useEffect, useState, useMemo } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Temporal } from '@js-temporal/polyfill'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { WDXL_Lubrifont_JP_N } from 'next/font/google'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Incident } from '@/types/global'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { DeleteForever, ExpandMore, ExpandLess, RestartAlt, Info } from '@mui/icons-material'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useEffect, useState } from "react"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const WdxlLubrifontJpN = WDXL_Lubrifont_JP_N({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| subsets: ['latin'], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| weight: ['400'], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function Counter() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [daysWithoutIncidents, setDaysWithoutIncidents] = useState<number>(0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function Counter({ props: { title, history, description } }: { props: Incident }) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const sortedHistory = useMemo( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| () => [...history].sort((a, b) => a.epochMilliseconds - b.epochMilliseconds), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [history] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [shownDate, setShownDate] = useState<Temporal.Instant>(sortedHistory.at(-1) || Temporal.Now.instant()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [now, setNow] = useState(() => Temporal.Now.instant()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [showAccordion, setShowAccordion] = useState(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Only using this to save the date of the last incident in localStorage | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const lastIncident = localStorage.getItem("lastIncidentDate"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (lastIncident) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const diff = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (Date.now() - new Date(lastIncident).getTime()) / (1000 * 60 * 60 * 24); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setDaysWithoutIncidents(Math.floor(diff)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const interval = setInterval(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setNow(Temporal.Now.instant()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, 1000); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
IvanGodinez21 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return () => clearInterval(interval); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+23
to
+26
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const interval = setInterval(() => { | |
| setNow(Temporal.Now.instant()); | |
| }, 1000); | |
| return () => clearInterval(interval); | |
| let interval: NodeJS.Timeout | null = null; | |
| function startInterval() { | |
| if (!interval) { | |
| interval = setInterval(() => { | |
| setNow(Temporal.Now.instant()); | |
| }, 60000); // Update every 60 seconds | |
| } | |
| } | |
| function stopInterval() { | |
| if (interval) { | |
| clearInterval(interval); | |
| interval = null; | |
| } | |
| } | |
| function handleVisibilityChange() { | |
| if (document.visibilityState === 'visible') { | |
| startInterval(); | |
| setNow(Temporal.Now.instant()); // Ensure immediate update on tab focus | |
| } else { | |
| stopInterval(); | |
| } | |
| } | |
| document.addEventListener('visibilitychange', handleVisibilityChange); | |
| // Start interval if visible on mount | |
| if (document.visibilityState === 'visible') { | |
| startInterval(); | |
| } | |
| return () => { | |
| stopInterval(); | |
| document.removeEventListener('visibilitychange', handleVisibilityChange); | |
| }; |
Copilot
AI
Nov 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mutating sortedHistory directly is problematic because it's a memoized value derived from the history prop. This mutation won't persist across re-renders and won't update the parent component's state. The resetDate function should call a callback prop (e.g., onAddHistory) to properly update the incident history in the parent component.
Copilot
AI
Nov 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mutating the sortedHistory array directly will not trigger React re-renders because it's a memoized value. This mutation also modifies the original history prop array. The function should create a new array and update the parent component's state through a callback prop, or at minimum avoid mutating the memoized value.
Copilot
AI
Nov 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getBgColor function is defined inside the component body but doesn't use any component state or props. Consider moving it outside the component to avoid recreating it on every render.
Copilot
AI
Nov 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeAgo function is defined inside the component body but doesn't use any component state or props besides what's passed as parameters. Consider moving it outside the component to avoid recreating it on every render.
Copilot
AI
Nov 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The longestDaysRecord function is defined inside the component body but doesn't use any component state or props besides what's passed as parameters. Consider moving it outside the component to avoid recreating it on every render.
Copilot
AI
Nov 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The longestDaysRecord function creates a reversed copy of sortedHistory but sortedHistory is already available in the component scope and was computed using useMemo. Consider passing only the necessary data to this function or computing the reversed array outside the function to avoid redundant operations.
Copilot
AI
Nov 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gap calculation has reversed order. Since sortedReversedHistory is in descending order (most recent first), a is more recent than b. Calling a.until(b) produces a negative duration. The calculation should be b.until(a).days or use Math.abs(gap) to get the correct positive gap value.
| const gap = a.until(b).days; | |
| const gap = b.until(a).days; |
Copilot
AI
Nov 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sortedReversedHistory array is created inside the longestDaysRecord function but sortedHistory is already passed as a parameter. This creates unnecessary array copies. Consider passing the sorted history in the desired order or optimize the algorithm to avoid the reversal.
| const sortedReversedHistory = [...sortedHistory].reverse(); | |
| const toPlainDate = (inst: Temporal.Instant) => Temporal.PlainDate.from(inst.toString().slice(0, 10)); | |
| let maxGap = 0; | |
| for (let i = 0; i < sortedReversedHistory.length - 1; i++) { | |
| const a = toPlainDate(sortedReversedHistory[i]); | |
| const b = toPlainDate(sortedReversedHistory[i + 1]); | |
| const gap = a.until(b).days; | |
| if (gap > maxGap) maxGap = gap; | |
| } | |
| const lastPlain = toPlainDate(sortedReversedHistory[sortedReversedHistory.length - 1]); | |
| const toPlainDate = (inst: Temporal.Instant) => Temporal.PlainDate.from(inst.toString().slice(0, 10)); | |
| let maxGap = 0; | |
| // Iterate over sortedHistory in reverse order | |
| for (let i = history.length - 1; i > 0; i--) { | |
| const a = toPlainDate(history[i]); | |
| const b = toPlainDate(history[i - 1]); | |
| const gap = a.until(b).days; | |
| if (gap > maxGap) maxGap = gap; | |
| } | |
| const lastPlain = toPlainDate(history[0]); |
IvanGodinez21 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Nov 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Delete button is missing an onClick handler, making it non-functional. Add an onClick handler or a callback prop to handle incident deletion.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,19 @@ | ||
| import { Incident } from "@/types/incident"; | ||
|
|
||
| import { Incident } from '@/types/global'; | ||
| interface Props { | ||
| incident: Incident; | ||
| } | ||
|
|
||
| export function IncidentCard({ incident }: Props) { | ||
| // Compute a deterministic date in DD-MM-YYYY using the ISO string to avoid hydration differences | ||
| const isoDate = new Date(incident.date).toISOString().split("T")[0]; | ||
| const [year, month, day] = isoDate.split("-"); | ||
| const isoDate = incident.history[0]?.toString().split('T')[0]; | ||
| const [year, month, day] = isoDate.split('-'); | ||
| const formattedDate = `${day}-${month}-${year}`; | ||
|
|
||
| return ( | ||
| <div className="bg-white p-4 rounded-lg shadow-sm border"> | ||
| <h3 className="font-semibold text-black">{incident.title}</h3> | ||
| <p className="text-sm text-gray-600 mt-1">{incident.description}</p> | ||
| <span className="text-xs text-red-600 mt-2 block">{formattedDate}</span> | ||
| <div className='bg-white p-4 rounded-lg shadow-sm border'> | ||
| <h3 className='font-semibold text-black'>{incident.title}</h3> | ||
| <p className='text-sm text-gray-600 mt-1'>{incident.description}</p> | ||
| <span className='text-xs text-red-600 mt-2 block'>{formattedDate}</span> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prop name 'props' is redundant and confusing. Consider renaming it to 'incident' to match the semantic meaning:
<Counter key={incident.id} incident={incident} />