feat(lectio-docs): links to files you don't publish go to the forge - #44
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates @eventuras/lectio-docs so relative markdown links can be resolved to a repo-relative file path even when the target is not part of the collected/published page set, enabling hosts to redirect such links to the forge instead of leaving them to 404.
Changes:
- Extends
ResolvedLinkto includepathand allowpage: PageMeta | null, and addsContentSource.sourceHref()for forge URL generation. - Adds
DocsConfig.sourceUrl(persisted into the manifest) andDocSource.ignoreto exclude subtrees without complex globs. - Fixes glob base detection by deferring to fast-glob’s task base calculation (with a noted edge case in the current implementation).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/lectio-docs/src/content/types.ts | Updates public types: manifest gains sourceUrl, ResolvedLink can have page: null, and ContentSource gains sourceHref. |
| packages/lectio-docs/src/content/content-source.ts | Implements new ResolvedLink shape and adds sourceHref; changes link resolution semantics for unpublished targets. |
| packages/lectio-docs/src/collector/config.ts | Adds DocSource.ignore and DocsConfig.sourceUrl configuration options. |
| packages/lectio-docs/src/collector/collect.ts | Validates {path} placeholder for both templates, wires ignore into collection, writes sourceUrl into the manifest, and changes glob base detection. |
| packages/lectio-docs/README.md | Documents the new behavior and configuration options. |
| .changeset/links-to-unpublished-files.md | Adds a changeset describing the feature and bug fix (version bump level needs review). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const target = resolveRelativePath(fromSource, safeDecode(path)); | ||
| const page = bySource.get(target); | ||
| return page === undefined ? null : { page, suffix }; | ||
| return { page: bySource.get(target) ?? null, path: target, suffix }; |
There was a problem hiding this comment.
Confirmed and fixed in 61f2d8d. You're right that this undid the guarantee established one PR ago — reproduced before touching anything:
../../../../secrets.md → https://github.com/org/repo/blob/main/../../../secrets.md
resolveLink now returns null when the resolved target sits above the collection root, as you suggested. There is no file there to name, so there is nothing for a host to link to — and handing it a path anyway is exactly the outcome the ..-preserving behaviour in paths.ts exists to produce.
Verified that the case this PR is actually for still works:
../../../../secrets.md null
../CONTRIBUTING.md no page → …/blob/main/CONTRIBUTING.md
terms.md /terms → …/blob/main/docs/terms.md
| sourceHref(path) { | ||
| const template = manifest.sourceUrl; | ||
| return template === undefined ? null : template.replaceAll('{path}', path); | ||
| }, |
There was a problem hiding this comment.
Fixed in 61f2d8d — sourceHref now encodes per segment (so / survives while spaces and the rest don't) and returns null for a path containing . or ...
docs/my file.md → …/blob/main/docs/my%20file.md
sourceHref('../x.md') → null
One correction on the # example: it can't reach sourceHref through resolveLink, since # is split off as the fragment before the path is resolved — weird#name.md returns null already. But sourceHref is public API and can be called directly with any path, so encoding it here is right regardless. The ./.. guard is belt-and-braces now that the sibling comment's fix stops those arriving via resolveLink, but it costs nothing and this function shouldn't depend on its caller to be safe.
| function getGlobBase(glob: string): string { | ||
| const parts = glob.split('/'); | ||
| const staticParts: string[] = []; | ||
| for (const part of parts) { | ||
| if (part.includes('*') || part.includes('{') || part.includes('?')) break; | ||
| staticParts.push(part); | ||
| } | ||
| return staticParts.join('/') || '.'; | ||
| return fg.generateTasks(glob)[0]?.base ?? '.'; | ||
| } |
There was a problem hiding this comment.
Confirmed and fixed in 61f2d8d. This is the sharpest of the four: the commit claims to fix ../ in slugs, and left the bug live for brace patterns. Reproduced end to end against docs/{a,b}/**/*.md:
slug: "/one" ← docs/a/one.md
slug: "/../b/two" ← docs/b/two.md
getGlobBase now reduces the task bases to the deepest directory they share, rather than taking the first. Same fixture after:
docs/{a,b}/**/*.md docs/**/*.md
"/a/one" "/a/one"
"/b/two" "/b/two"
Identical to the non-brace glob over the same tree, which is the property that was missing.
| --- | ||
| "@eventuras/lectio-docs": minor | ||
| --- |
There was a problem hiding this comment.
Keeping this at minor — deliberately, not by oversight.
You're right that ResolvedLink is a breaking change for a strictNullChecks consumer. But the package is at 0.4.0, and changesets bumps a major on a 0.x release straight to 1.0.0. So marking it major wouldn't communicate "breaking" — it would publish 1.0 and, with it, a stability guarantee this package isn't ready to make. Pre-1.0 is precisely the range where interfaces are still allowed to move; that is what the leading zero is for.
The break is called out in the changeset body, so it lands in the changelog where a consumer will read it.
Worth revisiting the moment we cut 1.0 — from then on this is exactly the right call.
20304ea to
61f2d8d
Compare
Documentation routinely links to files outside the collected set — a README the globs didn't cover, a section a deployment leaves out.
resolveLinkreturnednullfor those, indistinguishable from "not a link at all", so a host could only leave them as authored and let them 404.Resolved, with no page
ResolvedLink.pageis nowPageMeta | null, and the link carries the repo-relativepathit resolved to. A file the manifest doesn't hold still resolves — the host just learns there is no page for it, and can do something better than a dead end.sourceUrlis a{path}template likeeditUrl, and it is recorded in the manifest rather than read from config at render time — the host that renders links has the manifest, not the collector's config. Without a template,sourceHrefreturns null and links are left as the author wrote them.DocSource.ignoreLeaves part of a tree out of a source without contorting the glob:
Collector bug this uncovered
The static base of a glob was found by scanning for
*,{and?. That misses character classes and extglob, sodocs/!(ADR)/**/*.mdwas rooted at the literal directorydocs/!(ADR)— and every page collected through it got a slug containing../. fast-glob is now asked for the base instead of being second-guessed, which is also whyignoreis the better tool for the job than a cleverer glob.Minor changeset for
@eventuras/lectio-docs;editUrlandsourceUrlnow share one placeholder check.🤖 Generated with Claude Code