Repository navigation
fix(bestax-mcp): give Theme, ConfigProvider, Portal and ClientOnly prop tables - #969
Conversation
…op tables The index shipped every helpers page as prose, so get_props answered these four components as if they were hooks, with no table, and sent the caller to get_helper_props, which describes none of them. Search could not reach their props either. Their records are now built like every other component's, from their props interfaces, and keep the page for include: ["reference"]. The extractor learns the two shapes that hid them, a component declared as a function (Portal, ClientOnly) and one the barrel re-exports with export * from a module of another name (ConfigProvider). ConfigProviderProps now documents its members in TSDoc, so its table has notes. A hook's get_component and get_props now answer with its API signature block and point at its reference page and its examples, and search names get_component as the next step for one. useBulmaClasses still names get_helper_props too, since its page is where the helper props it reads are documented.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 46 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (33)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (18)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe MCP index now extracts prop tables from component-like documentation pages and stores API signatures separately from full references. Search and retrieval responses provide prop tables or pointers to documentation. Indexed documentation links now resolve to hosted documentation URLs. ChangesComponent indexing and MCP access
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Caller
participant search_bestax
participant get_component
participant get_props
Caller->>search_bestax: Search component or prop
search_bestax-->>Caller: Return result with get_component route
Caller->>get_component: Request component details
get_component-->>Caller: Return API details or reference pointer
Caller->>get_props: Request component props
get_props-->>Caller: Return prop table or helper API details
Merge Risk: ⚪ Minimal · up to The prop-table and reference changes have no identified merge-blocking issue. Normal checks remain appropriate before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 12 files. (11 skipped: 11 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
Preview DeploymentPreview URL: https://27bebc97.bestax.pages.dev |
|
deep-review: fresh The previous run was cancelled at the runner limit before it posted anything, so this asks for the same full review again. The core is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
deep-review: fresh The last run on fddfd65 was cancelled at the runner limit after it posted three threads (one on |
…page, name the title rule Theme's table came back with a blank Notes cell on most of its rows, because its props were described in @Property tags the extractor does not read. Those blank rows also outranked the documented ones in search, since a row with no description is scored on its name alone. Each member of ThemeProps now carries its own TSDoc, naming the variable it sets, in place of the @Property block. A component documented on a prose page lost the line pointing at that page when it stopped being answered as a helper, so get_component and get_props now say where the page is and how long it is. A prose page with a capitalised title that names no export stops the index build, as it did before, and now says that the title is the thing to change. The rule is noted where GENERATED_EXEMPT is defined.
|
deep-review: fresh Two runs on fddfd65 were cancelled at the runner limit before posting a summary. Between them they posted four threads, on |
…ts first signature The extractor took the first function declaration of a name, and for an overloaded function that is a signature, which cannot carry parameter initializers. The props type still resolved, so the table would have come back with every default missing and no error. Only an implementation counts now. No component in the library is overloaded today, so every generator's output is unchanged.
|
deep-review: fresh Three runs on fddfd65 were cancelled at the runner limit before posting a summary. Between them they posted five threads: one on |
A hook's API block, a prose page and an Accessibility section are served on their own, but they kept the page's relative links. Away from the page, `./valid-values.md` names a file in the reader's own workspace and `#scheme-backgrounds-and-bulmahelperstyles` a section the answer does not carry. The generator now points each link at the URL the docs site serves for it, leaving fenced code alone. useBulmaClasses' API block also said "see table below" of a table it does not carry, so the page links that section by name instead.
|
deep-review: fresh Several runs on fddfd65 were cancelled at the runner limit before posting a summary. Between them they posted five threads: one on |
|
deep-review: fresh The review on 02551c9 covered this PR, and its threads are settled. Since then the branch has two fixes for those threads (e620347, e45b4b8), merges of main, and one new change: the bestax.io links inside index answers now carry |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Preview DeploymentPreview URL: https://7ca1f3b7.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 1 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Security | Three new includes('https://bestax.io') fast paths took CodeQL from green to 3 high alerts — the same rule 1a7dec61 already reworked a test for |
bestax-mcp/src/format.ts:162 |
| 2 | 🔵 Advisory | Coverage | The render-time tag has no exhaustive guard: the generator sweeps every string in the index, the renderer pins seven sampled calls | bestax-mcp/src/__tests__/server.test.ts:448 |
| 3 | 🔵 Advisory | Robustness | "no tag in a skill answer" is asserted on the whole response, which carries the tagged version note when there is drift | bestax-mcp/src/__tests__/server.test.ts:481 |
| 4 | 🔵 Advisory | Correctness | plugin README's "reference docs … stay untagged" reads onto include: ["reference"], which this PR made tagged |
plugin/README.md:100 |
Overall: The tagging change is well built and I could not break it. attributedLinks is a real inline-markdown reader, not a regex over raw text — shadowOf masks code spans and backslash escapes at equal length so shadow offsets map back exactly, closesLinkText walks the bracket nesting and rejects images, and SITE_TARGET is genuinely host-anchored (bestax.io.example.com and bestax.io@evil.com both fail the [/?#] class and the closing lookahead). Against the committed index it tags 77 of 77 site links in served prose and corrupts none: stripping the tag from all 20 changed fields reproduces each raw field byte-for-byte, and all 75 /docs/… targets resolve to a real page and a real anchor. The riskiest part is not the parser but the eleven hand-placed call sites it is applied through — finding 2 — because a missed one is invisible: the answer still reads right and the link still works, only the attribution is gone. Focus the human read there and on finding 1, which is the only thing standing between this PR and a green board.
Residual risk:
- A site link the server prints but does not tag. Refuted for the current tree by sweep, not by reading: every field path that carries a site link (
accessibility,summary,parts[].summary,doc,api,catalog.components[].purpose) has anattributedLinkscall on its emission path, and all 77 targets come back tagged throughget_component,get_props,list_components,search_bestax,get_helper_propsand thebestax://componentsresource. Untagged by design and tested: examples (get_examples), skill bodies, prompts and skill resources. - A link shape
attributedLinksdoes not read. Bare URLs, autolinks, raw<a href>and reference-style[ref]:definitions are all left alone, and so is anything inside a fence. Refuted as live: zero of any of those four shapes exists in any served field today (docsUrl/storybookare bare, and those go throughattributedin the footer). Open: a future page adding one ships untagged and nothing fails — the same gap as finding 2, which is why I posted that one rather than three. - Generator and renderer disagreeing about what is code.
absoluteLinks(gen-mcp-index.mjs:407) splits on fences only;attributedLinksis additionally code-span-aware. A markdown link inside a code span would therefore be rewritten by the generator and skipped by the renderer. Refuted as live: zero code spans anywhere underdocs/docs/apicontain](. The asymmetry is in the safe direction for the one case that matters — a literal backticked[x](https://bestax.io/…)passes the generator untouched (absolute targets return as-is) and the renderer's span awareness then keeps it so. - Cost of tagging on the hot path.
referenceOfruns a full pass over the prose page on everyget_component/get_propsfor a hook or a prose component, purely to quote a size. Measured rather than assumed: 0.46 ms for useBulmaClasses' 76,290 characters, 0.17 ms for Theme's 42,393 — three orders of magnitude under the event-loop stallsserver.ts:80was written about. Not a concern. - The merge resolution. #975's side (
SCHEMA_VERSION2,orderDeclarers,cssVarIndexas arrays) and this PR's side (absoluteLinks,withAbsoluteLinks,proseComponentInfo,COMPONENT_NAME) are both live ingen-mcp-index.mjs, their tests both kept ingen-mcp-index.test.mjsandserver.test.ts, andpnpm gen:mcp:checkre-runs the committed index byte-identical.cssVarIndexis built outsidewithAbsoluteLinks, so the new arrays are untouched by the rewrite. - Gates. 367/367 bestax-mcp jest, 26/26
gen-mcp-index.test.mjs, 81/81gen-skills-repo.test.mjs,check:conformance23/23,typecheck7/7, coverage 97.76 / 92.42 / 98.72 / 99.03 against the 95 / 78 gate with the new functions fully covered.pnpm test:scriptscould not be judged here (eslint-plugin/distabsent gives the documented "build first" failures), but CI's Build and Test ran it green on this SHA — its one failing step isAudit (high severity), which fails identically onmainat3ffa76b6, so that red is inherited, not this PR's. CodeQL's is finding 1.
🏄 This one paddles out with a proper hand-shaped board — a real little markdown reader that knows a code span from prose, 77 for 77 links tagged and not a byte bent out of shape. Only thing between it and a clean ride is CodeQL squinting at three
includes()calls like they're a riptide; swap 'em for an anchored pattern and this set's good to go.
…a substring CodeQL read the substring tests that skip text with no site link in it as URL sanitization. Both now use one pattern, the origin with nothing word-like after it, which admits everything SITE_TARGET does after the host. New cases pin a target that ends at the host, closed by each thing that may follow it there. The server test picks its examples with the same pattern.
The tag is applied field by field, and seven sampled calls were all that held it. The sweep asks every tool that renders a record for every record, each part and every catalog category, with every section get_component offers, and fails on any bestax.io link target outside code that lacks the tag. Each example holding a site URL is checked as served as written. Dropping any call site whose field carries a site link today fails it.
… tag get_skill ends its answer with the version note when the installed library has drifted from the index, and that note's link is tagged, so asserting no tag anywhere in the answer failed in a normal state. The test now reads the bundled SKILL.md, requires an untagged site link in it, and checks that get_skill, the prompt and the resource each open with that body exactly as written.
get_component serves a prose page under include: ["reference"], tagged, so "reference docs ... stay untagged" read backwards there. The plugin README now says only that skill bodies are served as written, and the telemetry guide ties its reference docs to the skills they belong to.
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on f60ee05. |
Preview DeploymentPreview URL: https://c17ce77e.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 4 resolved · 0 open
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Security | includes('https://bestax.io') fast paths red CodeQL — verified fixed (SITE_ORIGIN anchored pattern; CodeQL green on HEAD) |
bestax-mcp/src/format.ts:162 |
| 2 | 🔵 Advisory | Coverage | Render-time tag had no exhaustive guard — verified fixed (schema-driven sweep over every record, part and catalog entry) | bestax-mcp/src/__tests__/server.test.ts:460 |
| 3 | 🔵 Advisory | Robustness | "no tag in a skill answer" asserted on the envelope — verified fixed (asserts the body slice against the bundled markdown) | bestax-mcp/src/__tests__/server.test.ts:481 |
| 4 | 🔵 Advisory | Correctness | "reference docs … stay untagged" collided with include: ["reference"] — verified fixed (clause deleted) |
plugin/README.md:100 |
Overall: This pass settled threads and reviewed no commits. All four previously-open threads — one blocking, three advisory — are addressed in the current code and are now resolved. The one I checked hardest was the CodeQL fix: SITE_ORIGIN's (?![\w.-]) lookahead rejects nothing that SITE_TARGET accepts after the host (/, ?, #, whitespace, ), >), so it stays a strictly broader pre-check, and because shadowOf only substitutes \0 for backtick/backslash runs, a literal present in the shadow is present at the same offset in the raw block — the raw-vs-shadow split cannot silently drop a match. CodeQL on f60ee05d reports No new alerts in code changed by this pull request, and 374/374 bestax-mcp tests pass locally.
Residual risk: out of scope for a verify pass — this run raised no new findings. The one limitation already on the record in thread 2 stands: the exhaustive sweep is data-driven, so a dropped attributedLinks call on p.description, p.deprecationNote, t.summary or searchAll's prop description goes unnoticed until a site link appears in one of those fields. A reader who wants the current code reviewed afresh should ask with a deep-review: fresh steer.
🏄 Author paddled back out and cleaned up every last one — anchored the regex, made the sweep read its own schema, and trimmed the one sentence that read two ways. Board's waxed, CodeQL's glassy, nothing left floating out there.
github-actions
Bot
added
the
review-converged
|
🎉 This PR is included in version 5.27.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.15 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
The MCP index shipped every page under helpers/ as prose, so
get_propsanswered Theme, ConfigProvider, Portal and ClientOnly as if they were hooks: no table, and a pointer atget_helper_props, which describes none of them. Search could not reach their props either. The real hooks fared no better. useFocusTrap and useBulmaClasses were pointed atget_helper_propstoo, and search named it as the next step for a hook.A component documented on a helpers page is now built like every other component, from its props interface, and keeps its page for
include: ["reference"]. The props extractor learns the two shapes that hid these components. Portal and ClientOnly are declared as functions rather than constants, and ConfigProvider reaches the barrel through anexport *from a module of another name. Neither change moves the generated API pages. ConfigProviderProps and ThemeProps now document their members in TSDoc rather than in@propertytags, so their tables have notes, and each of Theme's variable props names the variable it sets. In bulma-ui that is a doc-comment change and nothing else.A hook's record now carries its
## APIsignature block.get_componentandget_propsanswer a hook with that block, then point at its whole page (giving the page's size, since useBulmaClasses' runs to tens of thousands of characters) and atget_examples. Search namesget_componentas the next step for a hook. useBulmaClasses still namesget_helper_propstoo, since its page is where the helper props it reads are documented.The index now resolves every link it ships to the URL bestax.io serves, so text cut from a page no longer points at a relative file or an anchor the answer does not carry. A component documented in prose says where its page is under its table. A prose page whose capitalised title names no component stops the build with the page and the rule, rather than shipping a component as prose.
pnpm allpasses locally.Fixes #933
Summary by CodeRabbit