fix: resolve docs/notebooks symlink so notebook pages render - #745
Merged
Conversation
The symlink `docs/notebooks -> ../notebooks` breaks both `find` (can't discover .ipynb files) and zensical/mkdocs (can't discover generated .md files during site build). Instead of adding `-L` flags everywhere, resolve the symlink to a real directory copy at the start of the nbdocs target. This way both nbconvert and zensical see a real directory and work correctly. Fixes gdsfactory#743 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jackgdsf
requested review from
ThomasPluck,
joamatab and
nikosavola
as code owners
August 1, 2026 00:50
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates the nbdocs Makefile target so that notebook markdown files under docs/notebooks are discoverable by the docs build system, by resolving and replacing a symlink with a real directory before running nbconvert over all notebooks. Flow diagram for updated nbdocs Makefile targetflowchart TD
A[nbdocs target invoked] --> B{docs/notebooks is symlink?}
B -- yes --> C[readlink -f docs/notebooks]
C --> D[rm docs/notebooks]
D --> E[cp -r target docs/notebooks]
B -- no --> F[use existing docs/notebooks directory]
E --> G[find docs -name *.ipynb]
F --> G[find docs -name *.ipynb]
G --> H[jupyter nbconvert --to markdown --embed-images]
H --> I[mkdocs/zensical scans docs_dir and finds generated .md]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The use of
readlink -fis non-portable and will fail on macOS/BSD; consider a POSIX-compatible alternative or a small helper script to resolve the symlink target more robustly. - Dropping
-Lfrom thefindcall changes behavior for any other symlinked notebook locations underdocs; if those are expected, you may want to keep-Lor otherwise ensure they’re still discovered.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The use of `readlink -f` is non-portable and will fail on macOS/BSD; consider a POSIX-compatible alternative or a small helper script to resolve the symlink target more robustly.
- Dropping `-L` from the `find` call changes behavior for any other symlinked notebook locations under `docs`; if those are expected, you may want to keep `-L` or otherwise ensure they’re still discovered.
## Individual Comments
### Comment 1
<location path="Makefile" line_range="69-70" />
<code_context>
@echo "Converting notebooks to markdown..."
- @find -L docs -name "*.ipynb" | while read nb; do \
+ @# Resolve symlink so zensical can discover the generated .md files
+ @if [ -L docs/notebooks ]; then \
+ target=$$(readlink -f docs/notebooks) && \
+ rm docs/notebooks && \
+ cp -r "$$target" docs/notebooks; \
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `readlink -f` may break on systems where `-f` isn't supported (e.g. macOS).
On macOS and some BSD systems `readlink` doesn’t support `-f`, so this logic will fail and break `nbdocs` on those platforms. If cross-platform support is required, please replace this with a portable approach (e.g. a small `python -c` to resolve the symlink, or a POSIX-compatible `cd`/`pwd` pattern).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
joamatab
approved these changes
Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #744. The previous fix (
find -L) solved notebook discovery but the pages still 404 because zensical/mkdocs also doesn't follow symlinks when scanningdocs_dirfor source files. The generated.mdfiles exist (build logs show all 32 notebooks converted) but zensical never picks them up.This PR replaces the symlink with a real directory copy at the start of the
nbdocstarget:This resolves both issues at once —
findand zensical both see a real directory.Fixes #743
Test plan
Reminder: AI-created PRs still require human review of the actual code changes before merge. I'll open the PR, but a human must review and approve it.
🤖 Generated with Claude Code
Summary by Sourcery
Bug Fixes: