-
-
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?
Conversation
8ddc2b0 to
dfb2a11
Compare
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.
Pull Request Overview
This PR refactors the counter component by replacing JavaScript's native Date with the Temporal API from @js-temporal/polyfill. The changes include a new timeAgo utility function, migration of the incident date type to Temporal.Instant, and a complete redesign of the Counter component with a digital display aesthetic.
Key Changes:
- Migrated from
DatetoTemporal.Instantfor incident timestamps - Created a
timeAgoutility function for relative time formatting - Redesigned Counter component with digital display and real-time updates
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/timeAgo.ts | New utility for formatting relative time using Temporal API and Intl formatters |
| src/types/incident.ts | Changed incident date type from Date to Temporal.Instant |
| src/lib/mockData.ts | Updated mock data to use Temporal.Instant instead of Date |
| src/components/IncidentCard.tsx | Updated date formatting to work with Temporal.Instant |
| src/components/Counter.tsx | Complete refactor with new digital display UI and real-time updates |
| src/app/page.tsx | Removed standalone Counter, now renders Counter for each incident |
| package.json | Added @js-temporal/polyfill dependency |
Comments suppressed due to low confidence (1)
src/components/Counter.tsx:1
- Using string slicing on
toString()to extract date components is fragile. Use the Temporal API's built-in methods instead:lastIncidentDate.toZonedDateTimeISO(timeZone).toPlainDate()to properly convert Instant to PlainDate.
import React, { useEffect, useState } from 'react';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Counter): recreate component
dfb2a11 to
dd4959c
Compare
dd4959c to
0c73649
Compare
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.
Pull Request Overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0c73649 to
d2abf75
Compare
d2abf75 to
27a6fc5
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Pull Request Overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/lib/mockData.ts
Outdated
| Temporal.Instant.from('2025-10-01T14:20:00Z'), | ||
| Temporal.Instant.from('2025-09-12T08:15:00Z'), | ||
| Temporal.Instant.from('2025-08-30T19:45:00Z'), | ||
| Temporal.Instant.from('2025-10-01T14:20:00Z'), | ||
| Temporal.Instant.from('2025-09-12T08:15:00Z'), | ||
| Temporal.Instant.from('2025-08-30T19:45:00Z'), | ||
| Temporal.Instant.from('2025-10-01T14:20:00Z'), | ||
| Temporal.Instant.from('2025-09-12T08:15:00Z'), | ||
| Temporal.Instant.from('2025-08-30T19:45:00Z'), | ||
| Temporal.Instant.from('2025-10-01T14:20:00Z'), | ||
| Temporal.Instant.from('2025-09-12T08:15:00Z'), | ||
| Temporal.Instant.from('2025-08-30T19:45:00Z'), | ||
| Temporal.Instant.from('2024-08-30T19:45:00Z'), | ||
| Temporal.Instant.from('2022-08-30T19:45:00Z'), |
Copilot
AI
Oct 25, 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 history array contains duplicate entries of the same three timestamps repeated four times. This repetition appears to be test/placeholder data that should be replaced with unique, meaningful historical incidents.
| Temporal.Instant.from('2025-10-01T14:20:00Z'), | |
| Temporal.Instant.from('2025-09-12T08:15:00Z'), | |
| Temporal.Instant.from('2025-08-30T19:45:00Z'), | |
| Temporal.Instant.from('2025-10-01T14:20:00Z'), | |
| Temporal.Instant.from('2025-09-12T08:15:00Z'), | |
| Temporal.Instant.from('2025-08-30T19:45:00Z'), | |
| Temporal.Instant.from('2025-10-01T14:20:00Z'), | |
| Temporal.Instant.from('2025-09-12T08:15:00Z'), | |
| Temporal.Instant.from('2025-08-30T19:45:00Z'), | |
| Temporal.Instant.from('2025-10-01T14:20:00Z'), | |
| Temporal.Instant.from('2025-09-12T08:15:00Z'), | |
| Temporal.Instant.from('2025-08-30T19:45:00Z'), | |
| Temporal.Instant.from('2024-08-30T19:45:00Z'), | |
| Temporal.Instant.from('2022-08-30T19:45:00Z'), | |
| Temporal.Instant.from('2025-10-15T09:00:00Z'), | |
| Temporal.Instant.from('2025-09-28T16:30:00Z'), | |
| Temporal.Instant.from('2025-09-10T11:45:00Z'), | |
| Temporal.Instant.from('2025-08-25T22:10:00Z'), | |
| Temporal.Instant.from('2025-07-30T07:20:00Z'), | |
| Temporal.Instant.from('2025-07-01T13:55:00Z'), | |
| Temporal.Instant.from('2025-06-15T18:40:00Z'), | |
| Temporal.Instant.from('2025-05-20T21:05:00Z'), | |
| Temporal.Instant.from('2025-04-10T10:15:00Z'), | |
| Temporal.Instant.from('2025-03-05T14:50:00Z'), | |
| Temporal.Instant.from('2025-02-18T06:30:00Z'), | |
| Temporal.Instant.from('2024-12-25T23:59:00Z'), | |
| Temporal.Instant.from('2024-10-10T12:00:00Z'), | |
| Temporal.Instant.from('2024-08-30T19:45:00Z'), |
| function resetDate() { | ||
| const now = Temporal.Now.instant(); | ||
| history.push(now); | ||
| setShownDate(now); | ||
| } |
Copilot
AI
Oct 25, 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 props.history array directly will not trigger React re-renders and can cause unexpected behavior. Create a new array instead or use a state variable to manage history.
27a6fc5 to
840fd28
Compare
840fd28 to
1e8c4f2
Compare
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.
Pull Request Overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| incidents.map((incident) => ( | ||
| <IncidentCard key={incident.id} incident={incident} /> | ||
| )) | ||
| incidents.map((incident) => <Counter key={incident.id} props={incident} />) |
Copilot
AI
Oct 25, 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 prop name 'props' is redundant and confusing. Consider renaming it to 'incident' to match the semantic meaning: <Counter key={incident.id} incident={incident} />
| incidents.map((incident) => <Counter key={incident.id} props={incident} />) | |
| incidents.map((incident) => <Counter key={incident.id} incident={incident} />) |
1e8c4f2 to
2845ed3
Compare
2845ed3 to
b6b1b0f
Compare
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.
Pull Request Overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/IncidentForm.tsx
Outdated
| title, | ||
| description, | ||
| date: new Date(), | ||
| date: Temporal.Now.instant(), |
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 date property is being added to the Incident object but it doesn't exist in the Incident interface defined in global.d.ts. This creates a type mismatch. Either add date to the interface or remove it from this object.
src/components/IncidentCard.tsx
Outdated
| const isoDate = incident.date.toString().split('T')[0]; | ||
| const [year, month, day] = isoDate.split('-'); | ||
| const formattedDate = `${day}-${month}-${year}`; |
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 code attempts to access incident.date, but the Incident interface only defines history: Temporal.Instant[], not a date field. This will cause a runtime error.
| const isoDate = incident.date.toString().split('T')[0]; | |
| const [year, month, day] = isoDate.split('-'); | |
| const formattedDate = `${day}-${month}-${year}`; | |
| const isoDate = incident.history[0] ? incident.history[0].toString().split('T')[0] : ''; | |
| const [year, month, day] = isoDate ? isoDate.split('-') : ['', '', '']; | |
| const formattedDate = isoDate ? `${day}-${month}-${year}` : 'Unknown date'; |
src/components/Counter.tsx
Outdated
| const [now, setNow] = useState(() => Temporal.Now.instant()); | ||
| const [showAccordion, setShowAccordion] = useState(false); | ||
|
|
||
| const relative = timeAgo({ pastDate: shownDate, targetDate: now }); |
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 called before it's defined (line 17 calls it, but it's defined at line 45). While JavaScript hoisting allows function declarations to work, this function is defined as a function expression inside the component, which won't be hoisted. This will cause a reference error.
b6b1b0f to
35cee71
Compare
35cee71 to
c9376ea
Compare
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.
Pull Request Overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/IncidentCard.tsx
Outdated
| // 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.date.toString().split('T')[0]; |
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 incident.date property no longer exists after refactoring. The Incident interface now uses history: Temporal.Instant[] instead of date: Date. This code should access incident.history[0] or be updated to handle the new data structure.
|
|
||
| // Save the date of the last incident in localStorage | ||
| localStorage.setItem("lastIncidentDate", newIncident.date.toISOString()); | ||
| localStorage.setItem('lastIncidentDate', newIncident.history[0]?.toString()); |
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 optional chaining operator ?. will cause 'undefined' to be stored as a string in localStorage when history[0] is undefined. Consider adding a guard clause or default value to prevent storing undefined values.
| localStorage.setItem('lastIncidentDate', newIncident.history[0]?.toString()); | |
| localStorage.setItem( | |
| 'lastIncidentDate', | |
| newIncident.history[0] ? newIncident.history[0].toString() : '' | |
| ); |
c9376ea to
ec28a51
Compare
ec28a51 to
2c086ba
Compare
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.
Pull Request Overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| function resetDate() { | ||
| const now = Temporal.Now.instant(); | ||
| sortedHistory.push(now); |
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.
| title: 'Form Validation Error', | ||
| description: 'Email field was not accepting certain valid domains.', |
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 incident with id 4 has the exact same title and description as the incident with id 2 (lines 13-14). This appears to be duplicated mock data and should either be differentiated or removed to avoid confusion in testing.
| title: 'Form Validation Error', | |
| description: 'Email field was not accepting certain valid domains.', | |
| title: 'Username Validation Error', | |
| description: 'Username field was not accepting underscores as valid characters.', |
| const nowPlain = Temporal.PlainDate.from(now.toString().slice(0, 10)); | ||
| return Math.max(0, onlyDatePlain.until(nowPlain).days); | ||
| } | ||
| const sortedReversedHistory = [...sortedHistory].reverse(); |
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.
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.
Pull Request Overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function resetDate() { | ||
| const now = Temporal.Now.instant(); | ||
| sortedHistory.push(now); | ||
| setShownDate(now); | ||
| } |
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.
| }); | ||
| } | ||
|
|
||
| function longestDaysRecord({ history, now }: { history: Temporal.Instant[]; now: Temporal.Instant }) { |
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.
| return 'bg-green-500'; | ||
| } | ||
|
|
||
| function timeAgo({ |
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.
| setShownDate(now); | ||
| } | ||
|
|
||
| function getBgColor(days: number) { |
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.
| 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]); |
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]); |
Co-Authored-By: Cristian C. Giraldo <52677987+imchriistian@users.noreply.github.com>
2c086ba to
25c68d7
Compare
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.
Pull Request Overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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; |
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; |
| <button aria-label='Delete' className='bg-black text-red-400 px-3 py-1 rounded hover:bg-gray-800 transition'> | ||
| <span aria-hidden='true'> | ||
| <DeleteForever /> | ||
| </span> | ||
| </button> |
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.
| const interval = setInterval(() => { | ||
| setNow(Temporal.Now.instant()); | ||
| }, 1000); | ||
| return () => clearInterval(interval); |
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.
[nitpick] The interval updates state every second regardless of component visibility or user activity. Consider using requestAnimationFrame for better performance when the tab is active, or increase the interval (e.g., to 60 seconds) since the display shows days, not real-time seconds. Alternatively, add logic to pause the interval when the page is not visible using the Page Visibility API.
| 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); | |
| }; |
|
|
||
| // Save the date of the last incident in localStorage | ||
| localStorage.setItem("lastIncidentDate", newIncident.date.toISOString()); | ||
| localStorage.setItem('lastIncidentDate', newIncident.history[0]?.toString()); |
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.
If newIncident.history[0] is undefined, calling .toString() on undefined will result in the string 'undefined' being stored in localStorage. Since history is initialized with [Temporal.Now.instant()] on line 23, this should not happen, but the optional chaining suggests defensive programming. Either remove the optional chaining if the value is guaranteed to exist, or handle the undefined case explicitly with a fallback value or early return.
| localStorage.setItem('lastIncidentDate', newIncident.history[0]?.toString()); | |
| localStorage.setItem('lastIncidentDate', newIncident.history[0].toString()); |
|
@QuackDuster and @imchriistian, I request the approval, and please help me to resolve the @copilot comments. You can commit and/or amend commit, I will squash everything anyway |
|
@IvanGodinez21 I've opened a new pull request, #4, to work on those changes. Once the pull request is ready, I'll request review from you. |
Description
Refactor the counter component.
Note: This component may be renamed in the future; for now, we'll keep this name.
Related issues
Testing
Screenshots
Notes