Skip to content

Verify page accessibility for removed/renamed docs in preview comments#18972

Merged
harupy merged 4 commits intomasterfrom
copilot/update-preview-comment-js
Nov 22, 2025
Merged

Verify page accessibility for removed/renamed docs in preview comments#18972
harupy merged 4 commits intomasterfrom
copilot/update-preview-comment-js

Conversation

Copy link
Contributor

Copilot AI commented Nov 22, 2025 •
edited by harupy
Loading

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Added runtime verification of removed/renamed documentation pages in preview comment workflow. The script now fetches each removed/renamed page to check if redirects exist, displaying clear status indicators:

  • ✅ redirect exists - Page returns 200 or 3xx (redirect configured)
  • ❌ add a redirect - Page returns 404 (redirect missing)
  • ⚠️ failed to check - Accessibility check failed (network error or other exception)

Example output:

**Changed Pages (3)**

- [guide/existing-page](https://...) (modified)
- [guide/removed-no-redirect](https://...) (removed, ❌ add a redirect)
- [guide/removed-with-redirect](https://...) (removed, ✅ redirect exists)
- [guide/network-error](https://...) (removed, ⚠️ failed to check)

How is this PR tested?

  • Existing unit/integration tests
  • New unit/integration tests
  • Manual tests

Tested with mock scenarios covering:

  • Pages with redirects (301, 200) → ✅ indicator
  • Pages without redirects (404) → ❌ indicator
  • Modified/added pages → No checks performed
  • Network errors → ⚠️ failed to check with error logging

Does this PR require documentation update?

  • No. You can skip the rest of this section.
  • Yes. I've updated:
    • Examples
    • API references
    • Instructions

Release Notes

Is this a user-facing change?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release notes for MLflow users.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

  • area/tracking: Tracking Service, tracking client APIs, autologging
  • area/models: MLmodel format, model serialization/deserialization, flavors
  • area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
  • area/scoring: MLflow Model server, model deployment tools, Spark UDFs
  • area/evaluation: MLflow model evaluation features, evaluation metrics, and evaluation workflows
  • area/gateway: MLflow AI Gateway client APIs, server, and third-party integrations
  • area/prompts: MLflow prompt engineering features, prompt templates, and prompt management
  • area/tracing: MLflow Tracing features, tracing APIs, and LLM tracing functionality
  • area/projects: MLproject format, project running backends
  • area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
  • area/build: Build and test infrastructure for MLflow
  • area/docs: MLflow documentation pages

How should the PR be classified in the release notes? Choose one:

  • rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
  • rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
  • rn/feature - A new user-facing feature worth mentioning in the release notes
  • rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
  • rn/documentation - A user-facing documentation change worth mentioning in the release notes

Should this PR be included in the next patch release?

  • Yes (this PR will be cherry-picked and included in the next patch release)
  • No (this PR will be included in the next minor release)
Original prompt

Update preview-comment.js to verify page accessibility for removed/renamed pages using fetch. Define a helper function like below.

async function isUrlAccessible(url) {
try {
const res = await fetch(url, { method: "GET", redirect: "manual" });
// treat 200 and 3xx as "accessible", 404 as "missing"
return res.ok || (res.status >= 300 && res.status < 400);
} catch (e) {
return false;
}
}

Switch the message depending on the result

removed, ✅
removed, ❌

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI self-assigned this Nov 22, 2025
@harupy harupy marked this pull request as ready for review November 22, 2025 04:40
@github-actions
Copy link
Contributor

github-actions bot commented Nov 22, 2025

@Copilot Thank you for the contribution! Could you fix the following issue(s)?

⚠ Invalid PR template

This PR does not appear to have been filed using the MLflow PR template. Please copy the PR template from here and fill it out.

@harupy
Add accessibility check for removed/renamed pages in preview-comment.js
b3f88bf 
- Add isUrlAccessible helper function to check page accessibility using fetch
- Update getCommentTemplate to accept isAccessible property for each page
- Check accessibility for removed/renamed pages during preview generation
- Display ✅ when redirect exists (200 or 3xx response)
- Display ❌ when redirect is missing (404 or other errors)
- Display ⚠️ as fallback when accessibility check fails

Co-authored-by: harupy <17039389+harupy@users.noreply.github.com>
Copilot AI changed the title [WIP] Update preview-comment.js for URL accessibility checks Verify page accessibility for removed/renamed docs in preview comments Nov 22, 2025
Copilot AI requested a review from harupy November 22, 2025 04:52
@github-actions github-actions bot added area/build Build and test infrastructure for MLflow rn/none List under Small Changes in Changelogs. labels Nov 22, 2025
harupy
harupy reviewed Nov 22, 2025
View reviewed changes
harupy
harupy reviewed Nov 22, 2025
View reviewed changes
harupy
harupy reviewed Nov 22, 2025
View reviewed changes
harupy
harupy reviewed Nov 22, 2025
View reviewed changes
@harupy
Verify page accessibility for removed/renamed docs in preview comments
ab0e73d 
Co-authored-by: harupy <17039389+harupy@users.noreply.github.com>
Copilot AI requested a review from harupy November 22, 2025 05:17
@harupy
Copy link
Member

harupy commented Nov 22, 2025 •
edited by github-actions bot
Loading

/review

🚀 Review workflow started

✅ Review completed.

Review Output

Review Complete

I've reviewed PR #18972 "Verify page accessibility for removed/renamed docs in preview comments" and found 1 issue:

Issue found:

  • JSDoc type inaccuracy (.github/workflows/preview-comment.js:12-18): The function's JSDoc claims it returns Promise<boolean>, but it actually returns Promise<boolean|undefined> since it returns undefined on errors. The description "false otherwise" is also misleading.

I've added a review comment with a suggestion to fix the JSDoc to accurately reflect the actual return type. The rest of the implementation looks good - the error handling logic correctly uses the three-state pattern (true/false/undefined) throughout the code.

github-actions[bot]
github-actions bot reviewed Nov 22, 2025
View reviewed changes
@harupy harupy requested review from Copilot and removed request for harupy November 22, 2025 05:36
Copilot AI requested a review from harupy November 22, 2025 05:39
Copilot AI reviewed Nov 22, 2025
View reviewed changes
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

This PR enhances the documentation preview workflow by adding runtime verification of removed/renamed documentation pages. The script now checks if redirects exist for removed/renamed pages and displays clear status indicators in PR comments.

Key Changes:

  • Added isUrlAccessible() helper function to verify page accessibility using native fetch API
  • Modified page processing to check accessibility for removed/renamed pages
  • Updated comment template to display status indicators (✅ redirect exists, ❌ add a redirect, ⚠️ failed to check)

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

@harupy
Copy link
Member

harupy commented Nov 22, 2025

@copilot address #18972 (comment)

@harupy
Update JSDoc return type for isUrlAccessible to include undefined
63b9ae5 
The function can return undefined when an error occurs, so the return
type documentation should reflect Promise<boolean|undefined> instead of
just Promise<boolean>.

Co-authored-by: harupy <17039389+harupy@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot address #18972 (comment)

Updated in 63b9ae5. The JSDoc now correctly reflects that the function can return boolean|undefined.

@harupy harupy added this pull request to the merge queue Nov 22, 2025
Merged via the queue into master with commit 4d5b214 Nov 22, 2025
55 of 76 checks passed
@harupy harupy deleted the copilot/update-preview-comment-js branch November 22, 2025 09:24
Tian-Sky-Lan pushed a commit to Tian-Sky-Lan/mlflow that referenced this pull request Nov 24, 2025
@harupy @Tian-Sky-Lan
Verify page accessibility for removed/renamed docs in preview comments (
2e3564d 
mlflow#18972)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: Tian Lan <sky.blue266000@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

Copilot code review Copilot Copilot left review comments

+1 more reviewer

@harupy harupy harupy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@harupy harupy

Copilot code review Copilot

Labels

area/build Build and test infrastructure for MLflow rn/none List under Small Changes in Changelogs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@harupy