Skip to content

Conversation

@sozua
Copy link

@sozua sozua commented Oct 15, 2025

Description

Adds a GitHub Actions workflow that scans PR blog posts for future publish dates and keeps an automated review comment updated, with tests covering both the detector and formatter.

Validation

Related Issues

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run pnpm format to ensure the code follows the style guide.
  • I have run pnpm test to check if all tests are passing.
  • I have run pnpm build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@sozua sozua requested a review from a team as a code owner October 15, 2025 03:14
@Copilot Copilot AI review requested due to automatic review settings October 15, 2025 03:14
@sozua sozua requested a review from a team as a code owner October 15, 2025 03:14
@vercel
Copy link

vercel bot commented Oct 15, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
nodejs-org Ready Ready Preview Oct 15, 2025 3:41am

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a CI workflow that detects future-dated blog posts in PRs and posts/updates an automated review comment summarizing them, with unit tests for the detector and comment formatter.

  • Introduces a script to compute future-dated posts and expose them as a workflow output
  • Adds a script to create or update a PR comment summarizing unresolved future-dated posts
  • Adds a GitHub Actions workflow to run the checks on PRs and post results; includes unit tests for both scripts

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
apps/site/scripts/check-blog-dates/index.mjs Core logic to find future-dated posts and to set FUTURE_POSTS_JSON for the workflow
apps/site/scripts/check-blog-dates/create-review.mjs Builds/updates a PR comment listing unresolved future-dated posts
apps/site/scripts/check-blog-dates/tests/index.test.mjs Unit tests for future-date detection and action output behavior
apps/site/scripts/check-blog-dates/tests/create-review.test.mjs Unit tests for comment formatting and upsert behavior
.github/workflows/check-post-publish-date.yml Workflow to run the check and post the results on PRs

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@sozua sozua marked this pull request as draft October 15, 2025 03:23
fix(workflow): update comments

fix: correct date comparison logic in createReviewForFutureDates function
Copy link
Member

@avivkeller avivkeller left a comment

Choose a reason for hiding this comment

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

A few concerns:

  • Firstly, I don't really think this is needed, but I won't block given that others may like this
  • This is quite a large and complex PR for something that can probably be very simple. We can probably just use a RegEx / dependency to get the date, and check if it's in the future, and do all other processing in the workflow itself.
  • Is there a lint plugin that'll do this for us? Can one be made instead?

Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

++ to Aviv's concerns.

I'm not sure I understand why we particularly need this, has this been an issue where future dates posts have been merged in the past? And even if we do need this, over 1,000 LoC to check some dates seems very excessive and smells like it'll be a maintenance burden down the line?

@avivkeller
Copy link
Member

has this been an issue where future dates posts have been merged in the past?

It happened once, but IMO it's the responsibility of the author to ensure that doesn't happen, not ours.

I've added the original issue to the agenda for discussion.

@sozua
Copy link
Author

sozua commented Oct 15, 2025

Hi guys! First off, thanks for the review and for being so quick!

About the PR size/complexity: I agree - I think it can definitely be simplified into something much smaller since it isnt something that happens very often. I ended up misunderstanding the requirements from that discussion.

I liked the suggestions, and i think the idea of creating a plugin to trigger eslint warnings locally based on the front-matter metadata could work. If that sounds good, I can try work on this solution in the next hours and update this PR when done. What do you think, @avivkeller?

@avivkeller
Copy link
Member

That's definitely a step in the right direction, but I've added the original issue to our agenda to discuss whether this is really needed.

Nevertheless, you certainly put a lot of effort here, and we are extremely grateful.

@sozua
Copy link
Author

sozua commented Oct 16, 2025

That's definitely a step in the right direction, but I've added the original issue to our agenda to discuss whether this is really needed.

Nevertheless, you certainly put a lot of effort here, and we are extremely grateful.

Ok! I'll wait for the discussion and I'm excited to help if needed

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