Skip to content

Conversation

@3w36zj6
Copy link
Member

@3w36zj6 3w36zj6 commented Dec 5, 2025

@netlify
Copy link

netlify bot commented Dec 5, 2025

Deploy Preview for typst-docs-web ready!

Name Link
🔨 Latest commit f0d83cc
🔍 Latest deploy log https://app.netlify.com/projects/typst-docs-web/deploys/6932d79b61ba970008114750
😎 Deploy Preview https://deploy-preview-44--typst-docs-web.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@YDX-2147483647
Copy link
Member

May I push to this branch to fix netlify?

@3w36zj6
Copy link
Member Author

3w36zj6 commented Dec 5, 2025

Yes, please go ahead and push to this branch to fix Netlify.

@YDX-2147483647 YDX-2147483647 force-pushed the feature/add-docs-assets-download-script branch 3 times, most recently from 8f61e84 to efe9228 Compare December 5, 2025 13:00
@YDX-2147483647 YDX-2147483647 force-pushed the feature/add-docs-assets-download-script branch from efe9228 to f0d83cc Compare December 5, 2025 13:01
@YDX-2147483647
Copy link
Member

Done. Apart from modifications in netlify, I added the --origin parameter to fetch-docs-assets.sh.

@3w36zj6
Copy link
Member Author

3w36zj6 commented Dec 5, 2025

The additions in the commit seem to be fine.

@3w36zj6 3w36zj6 requested a review from Copilot December 5, 2025 14:14
Copilot finished reviewing on behalf of 3w36zj6 December 5, 2025 14:17
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 refactors the documentation build process by extracting the docs assets download logic into a reusable shell script (fetch-docs-assets.sh). This consolidation reduces code duplication between the Netlify build script and GitHub Actions CI workflow, making maintenance easier and ensuring consistency across different build environments.

Key Changes:

  • Created a new parameterized shell script to handle downloading docs.json, assets, favicon, and generating metadata.json
  • Updated both netlify-build.sh and the CI workflow to use the new script instead of duplicating the download logic
  • Added shell linting tools (shfmt and shellcheck) to the development toolchain and CI pipeline
  • Migrated from deprecated metadata fields (githubRepositoryUrl, discordServerUrl) to the new socialLinks array

Reviewed changes

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

Show a summary per file
File Description
scripts/fetch-docs-assets.sh New reusable script that downloads and prepares documentation assets from GitHub releases with configurable tag, base path, destination, and origin URL
scripts/netlify-build.sh Refactored to use the new fetch script, updated indentation to tabs, added support for v0.14.1, and reordered ja-JP asset preparation
.github/workflows/ci.yaml Simplified build step to use the new fetch script, added new ci-shell job for shell script linting
mise.toml Added shfmt and shellcheck tools for shell script formatting and linting
netlify.toml Updated build command path to reference scripts directory
Comments suppressed due to low confidence (2)

scripts/netlify-build.sh:82

  • The DEPLOY_URL variable is used without a fallback value. While the comment indicates it will be set by Netlify, the script should handle the case where it's unset. Consider using:
--origin "${DEPLOY_URL:-https://example.com/}"

This ensures the script doesn't fail when DEPLOY_URL is not set, using the same fallback as the default in fetch-docs-assets.sh.
scripts/netlify-build.sh:104

  • [nitpick] The base path "/irrelevant/" is used here but the downloaded metadata.json will still have this value, which could cause confusion. Consider adding a comment explaining that the metadata.json will be overwritten by fetch-docs-ja-jp, or explicitly remove it after downloading the assets:
bash scripts/fetch-docs-assets.sh --tag "v0.13.1" --base "/irrelevant/"
rm public/{docs,metadata}.json  # Will be replaced by fetch-docs-ja-jp

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

@3w36zj6 3w36zj6 merged commit ba9a4d7 into main Dec 5, 2025
22 checks passed
@3w36zj6 3w36zj6 deleted the feature/add-docs-assets-download-script branch December 5, 2025 15:50
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