Skip to content

refactor : move dayjs.extend to module scope and extract useLastModified hook#442

Open
adithya-naik wants to merge 3 commits into
github:mainfrom
adithya-naik:bug/last_modified
Open

refactor : move dayjs.extend to module scope and extract useLastModified hook#442
adithya-naik wants to merge 3 commits into
github:mainfrom
adithya-naik:bug/last_modified

Conversation

@adithya-naik

@adithya-naik adithya-naik commented Jan 7, 2026

Copy link
Copy Markdown

What does this PR do?

This PR refactors how the last activity time is calculated in RepositoryItem.


What was changed?

  • Moved dayjs.extend(relativeTime) to module scope so it runs once instead of on every render
  • Extracted useLastModified into a proper custom hook outside the component
  • Initialized hook state eagerly using a lazy initializer to avoid blank render on SSR

What this PR does NOT do

This does not fix the root cause of #440. The underlying issue is that generated.json on main has stale last_modified values from Oct–Nov 2023. That is a data generation/pipeline problem, not a rendering problem. A separate issue/PR should address that.


Technical Notes

  • No new dependencies added
  • No UI changes
  • Fully backward-compatible
  • Follows React Hooks rules

Checklist

  • React Hooks rules followed
  • dayjs.extend runs at module scope only
  • No blank flash on SSR first render
  • No breaking changes

@adithya-naik

Copy link
Copy Markdown
Author

Hi Team,

Its been too long, please do consider this PR

@adithya-naik

Copy link
Copy Markdown
Author

@mrecachinas @csmlo is there any thing going wrong with the changes ? Please let me know

@adithya-naik

adithya-naik commented Jun 11, 2026

Copy link
Copy Markdown
Author

Hi, I am happy to make any recommended changes. Please consider review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 updates RepositoryItem so each repository item computes and displays its own “Last activity” relative timestamp consistently, by moving Day.js relative-time initialization to module scope and extracting the last-modified computation into a dedicated custom hook.

Changes:

  • Moved dayjs.extend(relativeTime) out of the component to avoid re-running it on every render.
  • Extracted useLastModified to module scope so hook definitions are not recreated per render.
  • Minor string formatting change in the issues wrapper className expression.
Show a summary per file
File Description
components/RepositoryItem.tsx Centralizes Day.js relative-time setup and refactors last-activity computation into a reusable hook for per-repository correctness.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment thread components/RepositoryItem.tsx Outdated
Comment on lines +17 to +20
const [lastModified, setLastModified] = useState("");

useEffect(() => setLastModified(dayjs(date).fromNow()), [date]);

@mrecachinas mrecachinas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Apologies for the delay.

I don’t think this PR closes #440 as-is. The change is a nice cleanup -- e.g., dayjs.extend(relativeTime) no longer runs during every render and useLastModified is no longer redefined inside RepositoryItem.

But the rendered value is still just dayjs(repository.last_modified).fromNow(), so it will continue to display whatever is in generated.json. On main, the checked-in generated.json has all last_modified values from Oct–Nov 2023, which is why every repository collapses to the same “2/3 years ago” label. This PR doesn’t change the data generation/deploy path or refresh the underlying last_modified values.

My suggestion is either retitling this as a small React cleanup, or updating the PR to address the real data freshness issue before closing #440.

@adithya-naik

Copy link
Copy Markdown
Author

@mrecachinas Thanks for the review! That totally makes sense. I will retitle it as a React cleanup and remove closes #440 link.

@adithya-naik adithya-naik changed the title Fix last_modified refactor : move dayjs.extend to module scope and extract useLastModified hook Jun 12, 2026
@adithya-naik

Copy link
Copy Markdown
Author

@mrecachinas This is ready for review, Thanks again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants