Skip to content

Add preview links comment to PRs #1416

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

Merged
merged 9 commits into from
Jul 1, 2025
Merged

Add preview links comment to PRs #1416

merged 9 commits into from
Jul 1, 2025

Conversation

reakaleek
Copy link
Member

@reakaleek reakaleek commented Jun 23, 2025

Closes #1395

Changes

When the upload to s3 completes, comment on the PR with a list of changed files.

Related Issues

@reakaleek reakaleek force-pushed the feature/preview-comment branch from 6c1e4af to b626770 Compare June 23, 2025 12:15
@reakaleek reakaleek changed the title WIP: Create preview diff comment Add preview links comment to PRs Jun 23, 2025
@reakaleek reakaleek self-assigned this Jun 23, 2025
@reakaleek reakaleek requested a review from bmorelli25 June 23, 2025 12:18

if (links.length > 20) {
body.push('');
body.push(`<sub>There are ${links.length - 20} more links...</sub>`);
Copy link
Contributor

Choose a reason for hiding this comment

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

do these additional links get exposed anywhere?

Copy link
Member

Choose a reason for hiding this comment

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

Do you want them to be @shainaraskas ?

I guess we could hide the remainder in a <summary> in the comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to avoid having to track down a whole pile for very complex PRs - 20 is a little on the low side. a <summary> tag would be perfect.

wonder if there's a risk of this running away into the 1000s of links in case of some low-level find/replace or a bad/old branch, so we might still want to limit it to avoid perf issues (I'll let you decide whether this is necessary)

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking was that more than 20 links are more overwhelming than helpful.

The majority of the current open PRs have one to five links. There are two outliers with 15+ links.

I'm happy to increase the number, but as you said, showing 1000s of links won't be useful.

Also, we need to consider the max pr comment character limit.

Copy link
Contributor

@leemthompo leemthompo Jun 25, 2025

Choose a reason for hiding this comment

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

I think there's a happy medium between 20 and 1000s 😉?

Could say

  • if less than 10 just use standard comments as we have today
  • if more than 10, whack em in a collapsible
  • cap at something like 75-100 to handle edge cases without going crazy

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link

github-actions bot commented Jun 29, 2025

🔍 Preview links for changed docs

@reakaleek reakaleek added automation packaging, ci/cd. and removed DO NOT MERGE labels Jun 29, 2025
@reakaleek reakaleek enabled auto-merge (squash) June 29, 2025 21:12
@reakaleek reakaleek disabled auto-merge June 29, 2025 21:12
@reakaleek reakaleek enabled auto-merge (squash) July 1, 2025 22:05
@reakaleek reakaleek merged commit da10b01 into main Jul 1, 2025
15 checks passed
@reakaleek reakaleek deleted the feature/preview-comment branch July 1, 2025 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation packaging, ci/cd.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add doc-preview-comment.yml functionality to the doc build
6 participants