mdbook

[navicore]

No description provided.

[claude]

PR Review: mdbook Documentation Setup

Summary

This PR adds mdBook-based documentation infrastructure with GitHub Pages deployment. The implementation is clean and follows best practices, but there are a few items to address before merging.

---

✅ Strengths

1. Well-configured mdBook setup: The book.toml configuration is comprehensive with sensible defaults (Rust theme, search enabled, GitHub integration).

2. Proper GitHub Pages workflow: The workflow correctly uses the official GitHub Pages actions and follows the two-job pattern (build + deploy) with appropriate permissions.

3. Smart path filtering: The workflow only triggers on changes to docs/**, book.toml, or the workflow file itself, avoiding unnecessary builds.

4. Clean gitignore: Added book/ to gitignore to prevent committing build artifacts.

5. Comprehensive documentation structure: The SUMMARY.md provides a logical organization from getting started through advanced topics to design documents.

---

🔍 Issues Found

1. Inconsistent README.md content (Medium Priority)

The new docs/README.md duplicates content from the root README.md but with differences:

• docs/README.md (45 lines) has a condensed version
• Root README.md (286 lines) is much more comprehensive

Issue: The docs README links to files that exist at the root but references them as if they're relative to docs:

- **New to Seq?** Start with the [Glossary](GLOSSARY.md)
- **Learn by doing:** Try [seqlings](https://github.com/navicore/seqlings)
- **Reference:** See the [Language Guide](language-guide.md) and [Standard Library](STDLIB_REFERENCE.md)

These links work because all the files exist in docs/, but it creates inconsistency.

Recommendation: Consider whether you want:

• Option A: Keep docs/README.md as a landing page specifically for the mdBook site
• Option B: Symlink or reference the root README with appropriate path adjustments

2. Missing SUMMARY.md content (High Priority)

The docs/SUMMARY.md was created with only 35 lines listing the table of contents structure, but it appears to be a basic skeleton. After verifying, all referenced files DO exist in the docs/ directory, which is good.

However, the SUMMARY.md shows:

[Introduction](README.md)

This will use docs/README.md as the introduction, which is different from the root README. This is intentional but should be noted.

3. No workflow validation (Low Priority)

The docs.yml workflow doesn't validate that the book builds successfully before deploying. While mdBook failures will fail the workflow, there's no explicit test step.

Recommendation: Add a validation step before the pages setup:

- name: Build documentation
  run: mdbook build

- name: Test documentation
  run: mdbook test  # Validates code examples in docs

4. site-url configuration may need adjustment (Medium Priority)

In book.toml:

site-url = "/patch-seq/"

This assumes the site will be deployed to https://<username>.github.io/patch-seq/.

Verify: Is this the correct GitHub Pages URL for your repository? If you're using a custom domain or if this is a user/org site (not a project site), this path may need adjustment.

5. Missing version pinning for mdBook (Low Priority)

The workflow uses:

mdbook-version: 'latest'

Recommendation: Pin to a specific version to prevent unexpected breakage from mdBook updates:

mdbook-version: '0.4.40'  # or current latest

This follows the pattern used in your other workflows (e.g., Rust toolchain pinned to 1.89.0 in ci-linux.yml:28).

---

🔒 Security Considerations

✅ Good: Minimal permissions with explicit grants:

permissions:
  contents: read
  pages: write
  id-token: write

✅ Good: Concurrency group prevents concurrent deployments that could cause race conditions.

---

🚀 Performance Considerations

✅ Good: The workflow is efficient:

• Triggers only on relevant file changes
• Uses official, optimized GitHub actions
• No unnecessary dependencies or build steps

⚠️ Note: No caching is configured. For mdBook, this is typically fine since builds are fast, but if you add heavy preprocessors or plugins later, consider adding cargo cache for those.

---

📝 Test Coverage

❌ Missing: No tests for the documentation build process.

Recommendation: Before merging, verify:

1. The GitHub Pages deployment works (you may need to enable Pages in repo settings first)
2. All internal links resolve correctly
3. The site-url path is correct for your deployment
4. The search functionality works

You can test locally:

cargo install mdbook  # if not installed
mdbook serve --open   # Preview at http://localhost:3000

---

🎯 Action Items Before Merge

1. Test the build locally with mdbook build and mdbook serve
2. Verify site-url matches your actual GitHub Pages URL
3. Consider pinning mdBook version for reproducibility
4. Enable GitHub Pages in repository settings (Settings → Pages → Source: GitHub Actions)
5. Document the decision regarding docs/README.md vs root README.md

---

📋 Best Practices Alignment

Checking against your repository's existing patterns:

✅ Uses Actions v4 (matches ci-linux.yml:23)
✅ Follows workflow naming conventions
✅ Includes descriptive comments (like ci-linux.yml:7-14)
✅ Uses path-based triggers (similar to your other workflows)

---

Overall Assessment

Verdict: Good foundation, but needs testing and a few adjustments before merge.

Rating: 7/10 - Solid implementation with room for minor improvements.

The PR successfully adds documentation infrastructure, but should be tested to ensure the deployment URL is correct and all links work properly. The relationship between root README and docs README should be clarified.

---

Suggested merge checklist:

• Test mdbook build locally
• Verify GitHub Pages URL configuration
• Pin mdBook version
• Enable GitHub Pages in repo settings
• Merge to main
• Verify deployment succeeds
• Check deployed site renders correctly