Conversation
Co-authored-by: David Wells <hello@davidwells.io>
|
Cursor Agent can help with this pull request. Just |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR is a solid reliability and correctness pass. The changes fall into two broad categories:
Documentation & naming alignment — all doc files and examples are updated to use the canonical docs / /docs match-word pair and the correct --files CLI flag. The md.config.js preference over markdown.config.js is consistently applied, and the --path alias is plumbed through for backwards compatibility. These are mechanical but important for keeping the docs trustworthy.
Core runtime improvements:
createLineNumberResolver(block-parser) — replacing repeatedsubstr(0, pos).split('\n').lengthcalls with a single pre-built binary-search resolver is a clean O(n log n) improvement in the worst case. The implementation is correct and handles all boundary conditions.- Preloaded content / parsedBlocks caching (block-replacer, block-transformer, core/index.js) — avoids re-reading files and re-parsing blocks that were already processed during the initial scan. The
parsedBlocksshort-circuit inblockTransformeris guarded properly withArray.isArray(foundBlocks.blocks). createDependencyGraph(core/index.js) — extracting the graph-building logic into its own function and switching fromtoposort(graph)totoposort.array(itemIds, graph)fixes the crash when there are no dependency edges (the baretoposort([])call would return an empty array and lose all file IDs). Using aSetfor dependency deduplication is also a nice cleanup.remoteRequest— now handles non-2xx HTTP responses, validates and trims URLs before use, and stops returningundefinedfor the failed-fetch path (the old code silently swallowed the response body when thetryblock threw). The new test for 404 is a good addition.CODEtransform — validation is now much stricter: both missing markers and reversed/empty sections throw with useful messages, and the old bug wherestartLinecould silently default to0(masking a missingSTARTmarker) is gone.
Inline comments cover four small items: an unresolved @type annotation, a test that relies implicitly on a default, an empty doc heading, and a missing trailing newline.
| await markdownMagic(tempFile, { | ||
| open: 'docs', | ||
| close: '/docs', | ||
| outputDir: OUTPUT_DIR, | ||
| applyTransformsToSource: false, | ||
| silent: SILENT | ||
| }) |
There was a problem hiding this comment.
The 404 test doesn't set failOnMissingRemote: true explicitly, so it silently relies on the default being true. If that default ever changes, the test would stop asserting anything useful (threw would stay false and the final assertion would catch it, but the error message check inside the catch block would never run). Specifying it explicitly makes the test's intent clearer.
| await markdownMagic(tempFile, { | |
| open: 'docs', | |
| close: '/docs', | |
| outputDir: OUTPUT_DIR, | |
| applyTransformsToSource: false, | |
| silent: SILENT | |
| }) | |
| await markdownMagic(tempFile, { | |
| open: 'docs', | |
| close: '/docs', | |
| outputDir: OUTPUT_DIR, | |
| applyTransformsToSource: false, | |
| failOnMissingRemote: true, | |
| silent: SILENT | |
| }) |
| ### Types and Build | ||
|
|
||
| ### Building |
There was a problem hiding this comment.
### Types and Build is an empty heading — it has no content before the next ### Building heading immediately follows. Either add a brief description or merge them into a single ### Build and Types section.
| ### Types and Build | |
| ### Building | |
| ### Building and Types |
| __private: { | ||
| createDependencyGraph | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
File is missing a trailing newline (\ No newline at end of file in the diff). Most editors and linters expect one.
| } | |
| } |
| if (typeof content === 'string' && srcPath) { | ||
| // Allow callers to provide preloaded content for srcPath files | ||
| fileContents = content |
There was a problem hiding this comment.
The old guard (throw new Error("Can't set both \"srcPath\" & \"content\"")) was replaced to support the preloaded-content optimization. That's the right call for the caching use case, but note this is a silent behavioral change for any external caller that accidentally passed both: they previously got an explicit error, now their content is silently used as-is and the file on disk is ignored.
The comment captures the intent well. Just worth flagging in case there's external API surface to consider.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
|
@claude fix the test failure https://github.com/DavidWells/markdown-magic/actions/runs/22261219621/job/64399767520?pr=117 packages/block-replacer test: } |
|
💬 Claude Code execution complete ✅ 📊 Execution Info: Pull Request #117 | Next.js (126 files) | @DavidWells 🚀 Automated management executed:
|
|
Claude finished @DavidWells's task in 1m 15s —— View job Fix: test failure in
|
When both srcPath and content are provided, content is now used as preloaded file contents instead of throwing an error. Update the test to match the new intentional behavior. Co-authored-by: David Wells <DavidWells@users.noreply.github.com>
github-actions
bot
added
auto-generated
2 participants
Fixes multiple bugs and performance issues, and updates documentation to improve correctness, reliability, and maintainability.
This PR addresses several confirmed issues identified during a codebase review. Key fixes include preventing silent HTTP error page injection in remote transforms, correcting dependency graph ordering for files with multiple
srcdependencies, and validatingCODEblock ID extraction to prevent incorrect content slicing. Performance improvements were also made by reducing duplicate file I/O and optimizing line-number parsing. Additionally, CLI usage and documentation have been aligned to prevent user confusion.