fix(oas:sync): match the platform's OAS-upload output (tag index pages, root ordering, slug casing)#35
fix(oas:sync): match the platform's OAS-upload output (tag index pages, root ordering, slug casing)#35rossrdme wants to merge 3 commits into
Conversation
oas:sync generated a different tree than uploading the same spec to ReadMe, so a repo initialized by upload drifted after a local sync: - Tag category pages (<tag>/index.md) were never generated, silently dropping the tag's description from the spec's top-level tags array. - The root reference/_order.yaml was never written, losing top-level ordering. - Slugs kept the operationId casing (getUserById.md) where the platform lowercases (getuserbyid.md), diverging URLs. - Generated pages omitted hidden: false. Generate the tag index.md (title from tag name, excerpt from tag description, never overwriting an existing one), maintain the root _order.yaml, lowercase slugs, and include hidden: false. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughOpenAPI synchronization now generates visible operation pages and per-tag Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/commands/oas-sync.js (1)
253-253: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider generating tag index pages even when operation pages already exist.
Because this
continueskips the rest of the loop for already-synced operations, missing tag category pages (index.md) and their corresponding_order.yamlupdates won't be backfilled for tags whose operation pages were generated in previous runs.If the goal is to fully align with the platform and ensure all tags have an
index.mdregardless of their operations' page status, consider moving the tag index generation logic (lines 268-277) and the relevant_order.yamlupdates above this check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/oas-sync.js` at line 253, Update the operation-processing loop so tag index generation and its corresponding _order.yaml updates execute before the pagesByOpId.has(opId) early continue. Preserve the continue for already-existing operation pages, while still backfilling missing tag index.md pages and ordering entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/oas-sync.js`:
- Line 257: Update the slug derivation in the operation-page generation flow
around safeSegment to reserve the "index" slug: when the normalized operation
slug equals "index", append a distinct suffix before constructing pagePath,
while preserving existing slugs for all other operations.
---
Nitpick comments:
In `@src/commands/oas-sync.js`:
- Line 253: Update the operation-processing loop so tag index generation and its
corresponding _order.yaml updates execute before the pagesByOpId.has(opId) early
continue. Preserve the continue for already-existing operation pages, while
still backfilling missing tag index.md pages and ordering entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc454943-2f4a-4b94-9bcc-52610b01a54c
📒 Files selected for processing (2)
src/commands/oas-sync.jstest/oas-sync.test.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
| file: oasFilename, | ||
| operationId, | ||
| }, | ||
| hidden: false, |
There was a problem hiding this comment.
Why do we need to add this in here? Can we rely on the defaults in our backend?
There was a problem hiding this comment.
Good call — dropped it. Removed the explicit hidden: false from both the generated operation pages and the tag index.md, relying on the backend default. (86bd1b4)
There was a problem hiding this comment.
Correction on my earlier reply here — I'd said we'd drop this, but after testing the actual UI upload I put hidden: false back (363d3de). The upload always stamps hidden and forces false on a newly-added endpoint even when its tag and all siblings are hidden: true, so the backend isn't inferring false from a missing field that we could safely omit — omitting it would make a synced endpoint diverge from an uploaded one. Kept on both operation pages and the tag index.md. Added an @todo to honor the x-internal extension (gitto#2095 / RM-4616 / CX-3303) once it's out, at which point we can derive visibility from the spec instead of hardcoding false.
Per review on #35: - Reserve <tag>/index.md exclusively for the tag category page. An operation whose slug normalizes to "index" now gets the first free numeric slug (index-1, index-2, ...) instead of clobbering the category page. Uses numeric suffixes rather than a fixed "index-operation" suffix, which could itself collide with a real operation. - Drop the explicit `hidden: false` from generated op and tag index pages; rely on the backend default instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
not ready yet, pushed an update too quickly, standby |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commands/oas-sync.js (1)
263-297: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBackfill tag index pages and root order entries for existing operations.
Currently, the check
if (pagesByOpId.has(opId)) continue;at the beginning of the loop skips the generation of the tag'sindex.mdand updates to the root_order.yamlif the operation page already exists. This means users upgrading to this version won't have tag index pages generated for their existing APIs unless they also add a new operation to that tag.Consider moving the tag index and root order generation logic above the
continuestatement. This ensures the directory structure and metadata are fully synchronized for all tags in the specification on the next sync.♻️ Proposed refactor
for (const [opId, op] of specOps) { - if (pagesByOpId.has(opId)) continue; - const rawTag = op.tag || 'Other'; const tag = safeSegment(rawTag, 'Other'); const pageDir = path.join(refDir, infoTitle, tag); + + // Ensure the directory exists so index.md and _order.yaml can be safely written + fs.mkdirSync(pageDir, { recursive: true }); + + // Generate the tag's category landing page (index.md) if it doesn't exist + const indexPath = path.join(pageDir, 'index.md'); + if (!fs.existsSync(indexPath)) { + fs.writeFileSync(indexPath, buildTagIndexContent(rawTag, tagDescriptions.get(rawTag))); + changes.added.push(path.relative(refDir, indexPath)); + } + + // Maintain root and infoTitle order entries for the tag + addToOrder(path.join(refDir, infoTitle, '_order.yaml'), tag); + addToOrder(path.join(refDir, '_order.yaml'), infoTitle); + + if (pagesByOpId.has(opId)) continue; + const slug = reserveOperationSlug(pageDir, safeSegment(opId, 'operation').toLowerCase()); const pagePath = path.join(pageDir, `${slug}.md`); // Never overwrite an existing file: it belongs to a manual page, another // spec, or a different operation whose sanitized name collides with this // one. Skipping (rather than clobbering) keeps repeated syncs stable. if (!isWithin(refDir, pagePath) || fs.existsSync(pagePath)) { changes.skipped.push({ path: path.relative(refDir, pagePath), operationId: opId }); continue; } - fs.mkdirSync(pageDir, { recursive: true }); - // The tag's category landing page (index.md), like the platform generates - // on upload. Never overwrite one that already exists. - const indexPath = path.join(pageDir, 'index.md'); - if (!fs.existsSync(indexPath)) { - fs.writeFileSync(indexPath, buildTagIndexContent(rawTag, tagDescriptions.get(rawTag))); - changes.added.push(path.relative(refDir, indexPath)); - } - const content = buildPageContent({ oasFilename, operationId: opId }); fs.writeFileSync(pagePath, content); addToOrder(path.join(pageDir, '_order.yaml'), slug); - addToOrder(path.join(refDir, infoTitle, '_order.yaml'), tag); - addToOrder(path.join(refDir, '_order.yaml'), infoTitle); changes.added.push(path.relative(refDir, pagePath)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/oas-sync.js` around lines 263 - 297, Update the specOps loop so tag directory/index generation and root/tag _order.yaml updates run for every operation, including entries already present in pagesByOpId. Move the existing metadata synchronization around pageDir, indexPath, and addToOrder above the pagesByOpId continue, while keeping new page creation and changes.added/skipped handling limited to missing operation pages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/commands/oas-sync.js`:
- Around line 263-297: Update the specOps loop so tag directory/index generation
and root/tag _order.yaml updates run for every operation, including entries
already present in pagesByOpId. Move the existing metadata synchronization
around pageDir, indexPath, and addToOrder above the pagesByOpId continue, while
keeping new page creation and changes.added/skipped handling limited to missing
operation pages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cdf1de0c-d18f-4089-822c-1d255e906488
📒 Files selected for processing (2)
src/commands/oas-sync.jstest/oas-sync.test.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
…ex, keep hidden Addresses the #35 review: - hidden: keep `hidden: false` on generated pages (operation + tag index). Testing the UI upload shows it always stamps hidden and forces false on a new endpoint even when the tag/siblings are hidden, so the backend default can't be relied on. Reverts the drop in 86bd1b4. Added an @todo to honor the x-internal extension (gitto#2095) once available. - index-slug collisions: replace the tag-local check with reference-wide, folder-aware slug reservation. Reference page slugs are one flat namespace (docs/ is separate), so uniquify against every slug in reference/ — files and category folders (folder/index.md) alike — appending -1, -2, ... The reserved `index` slug always yields a numbered operation page. - backfill: generate a tag's category index.md in its own pass over every tag in the spec, so references first synced by an older CLI (op pages, no index) get their category pages on the next sync. Never overwrites an existing index; idempotent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed 363d3de addressing the review. Summary of changes since the last review:
Out of scope, tracked as follow-ups: full |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/oas-sync.js`:
- Around line 227-251: Replace the Set-based slug tracking in
collectReferenceSlugs with lowercase-slug reference counts so duplicate
pages/categories retain their ownership. Update the deletion logic around the
slug release at line 311 to decrement the count and remove the slug only when it
reaches zero. Add a regression covering two existing pages with the same slug
where deleting one leaves that slug reserved by the retained page.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0688a58f-cab2-4e97-9d5e-5cdfaf317478
📒 Files selected for processing (2)
src/commands/oas-sync.jstest/oas-sync.test.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
| function collectReferenceSlugs(refDir) { | ||
| const slugs = new Set(); | ||
|
|
||
| function walk(dir) { | ||
| for (const entry of fs.readdirSync(dir)) { | ||
| const full = path.join(dir, entry); | ||
| let stat; | ||
| try { | ||
| stat = fs.statSync(full); | ||
| } catch { | ||
| continue; | ||
| } | ||
| if (stat.isDirectory()) { | ||
| walk(full); | ||
| } else if (entry.endsWith('.md')) { | ||
| // A folder's index.md contributes the folder name as a slug; any other | ||
| // page contributes its own filename. | ||
| const slug = entry === 'index.md' ? path.basename(dir) : path.basename(entry, '.md'); | ||
| slugs.add(slug.toLowerCase()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| walk(refDir); | ||
| return slugs; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve slug ownership when deleting one of several duplicate pages.
Set collapses duplicate legacy slugs. Line 311 therefore releases a slug even when another page or category still owns it, allowing a later operation to reuse that supposedly reserved slug. This is especially relevant when migrating trees previously permitted to reuse slugs across tags.
Track reference counts per lowercase slug, decrementing on deletion and removing only at zero. Add a regression with two existing pages sharing a slug, one stale and one retained.
Also applies to: 303-311
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/oas-sync.js` around lines 227 - 251, Replace the Set-based slug
tracking in collectReferenceSlugs with lowercase-slug reference counts so
duplicate pages/categories retain their ownership. Update the deletion logic
around the slug release at line 311 to decrement the count and remove the slug
only when it reaches zero. Add a regression covering two existing pages with the
same slug where deleting one leaves that slug reserved by the retained page.
There was a problem hiding this comment.
@rossrdme What do you think about this? If they have duplicate slugs in their directory this isn't going to support that. Probably fine since we don't support that in bidi anyways?
|
ok @erunion I believe I have addressed your concerns and coderabbit's concern. Please let me know if anything else persists. Also, I added a new PR, #37 since I noticed some issues when working on that. Lastly, we're about to ship support for the custom extension that hides endpoints, but I didn't want to address that here since it means some medium-sized changes. Thanks for all you've done thus far, I'm excited, and please let me know if this PR or #37 still needs work. |
2 participants
Problem
oas:syncproduced a different tree than uploading the same OAS spec to ReadMe, so a repo initialized by upload drifted after a local sync:<tag>/index.md) were never generated — the tag'sdescriptionfrom the spec's top-leveltagsarray was silently dropped.reference/_order.yamlwas never written, losing top-level ordering.getUserById.md) where the platform lowercases (getuserbyid.md), diverging page URLs.Changes
Tag category pages. Generate
<tag>/index.md(title = tag name, excerpt = tag description) for every tag in the spec, in its own pass — so it's created even when a tag's operation pages already exist. This backfills category pages for references first synced by an older CLI that didn't generate them, and recreates a deleted one. Never overwrites an existingindex.md(hand-written categories are safe); idempotent.Root ordering + slug casing. Maintain
reference/_order.yaml; lowercase generated slugs to match upload output.Page visibility (
hidden). Generated pages are writtenhidden: false, matching the platform's OAS upload, which always stampshiddenand forcesfalseon a new endpoint even when its tag/siblings are hidden — so the backend default can't be relied on to omit it. A@todotracks honoring thex-internalextension (gitto#2095 / RM-4616 / CX-3303) once released; the resync-side of that (re-applying x-internal to existing pages, parent hide-ratchet) is intentionally out of scope for this create-only command.Reference-wide, folder-aware slug uniqueness. Reference page slugs share one flat namespace (docs/ is a separate namespace and is never consulted). A generated slug is uniquified against every slug already in
reference/— both<slug>.mdfiles and category folders (<slug>/index.md) — appending-1,-2, … until free.indexis reserved for the tag category page, so an operation whose slug normalizes toindexalways gets a numbered page (index-1,index-2, …).Verification
reference/to just the spec, ran the patched sync, diffed against the platform's actual upload output — identical file trees, byte-identical_order.yaml, matching frontmatter.operationId: indexspec confirmed the platform itself producesindex-1/index-2, matching this PR.index.mdgets the category page on next sync; re-run reports "already in sync"._order.yaml; existing index never overwritten; lowercased slugs;indexcollision; reference-wide cross-tag uniqueness; category-folder-taken slug; missing-index backfill).Known follow-ups (out of scope, tracked)
x-internalvisibility resolution (create + resync) —@todoinbuildPageContent.index.md/ stale_order.yamlentry when a tag loses its last operation).🤖 Generated with Claude Code