refactor: combining shared code markdown renderer readme & changelog #2717
Open
WilcoSp wants to merge 236 commits into
Open
refactor: combining shared code markdown renderer readme & changelog #2717WilcoSp wants to merge 236 commits into
WilcoSp wants to merge 236 commits into
Conversation
…md -> html does need to change)
excludes changelog releases due to needing api calls
…og.md is being used.
…gelog info endpoint will return ChangelogMarkdownInfo instead of ChangelogReleaseInfo
provider icons are now suplied by the useProviderIcon composable
…properly anymore, this caused "package doesnt have changelog" to appear even if it does have it
removed <suspense> & now using `pending` to show the skeletons in the releases & markdown components.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/package-changelog/[[org]]/[name].vue (1)
132-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake loading/content/error branches mutually exclusive.
At Line 132, the skeleton uses
v-if="!changelog && !changelogError", while Line 139 starts a separatev-ifchain. This allows mixed states (e.g. skeleton plus fallback text) instead of a single deterministic UI state.Suggested fix
-const { data: changelog, error: changelogError } = usePackageChangelog(packageName, version) +const { data: changelog, error: changelogError, pending: changelogPending } = usePackageChangelog( + packageName, + version, +)- <section v-if="!changelog && !changelogError" class="flex flex-col gap-2 py-3"> + <section v-if="changelogPending" class="flex flex-col gap-2 py-3"> <ChangelogSkeleton /> </section> @@ - <LazyChangelogReleases - v-if="changelog?.type === 'release'" + <LazyChangelogReleases + v-else-if="changelog?.type === 'release'" @@ - <LazyChangelogMarkdown - v-else-if="changelog?.type === 'md'" + <LazyChangelogMarkdown + v-else-if="changelog?.type === 'md'"🤖 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 `@app/pages/package-changelog/`[[org]]/[name].vue around lines 132 - 173, The UI branches (ChangelogSkeleton, LazyChangelogReleases, LazyChangelogMarkdown and error) are not mutually exclusive because ChangelogSkeleton uses its own v-if while the lazy components start a separate v-if chain; change to a single conditional chain using the same root conditions (e.g., if changelogError -> show error, else-if changelog?.type==='release' -> render LazyChangelogReleases, else-if changelog?.type==='md' -> render LazyChangelogMarkdown, else -> render ChangelogSkeleton) and remove the standalone v-if on ChangelogSkeleton so only one branch (using the shared symbols changelog, changelogError, LazyChangelogReleases, LazyChangelogMarkdown, ChangelogSkeleton, resolvingPending) can render at a time.
🤖 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 `@app/composables/usePackageChangelog.ts`:
- Around line 27-38: The state is being initialized with the ComputedRef itself
instead of its boolean value; change the initializer passed to useState in
usePackageHasChangelogFromState (the call useState(key, () => hasChangelog)) to
return hasChangelog.value so the state stores a boolean, i.e. useState(key, ()
=> hasChangelog.value), ensuring key, hasChangelog and the returned state remain
consistent with consumers expecting a boolean.
In `@app/utils/header-version-matcher.ts`:
- Line 9: The versionRegex builds a RegExp from raw version text and only
escapes dots, allowing other regex metacharacters to break/weaponize the
pattern; fix by escaping all regex special characters in the incoming version
string before constructing versionRegex (add a small helper like escapeRegex
that scans the version value and prefixes every regex metacharacter with a
backslash), then use the escaped result when creating the RegExp assigned to
versionRegex (keep the existing surrounding anchors like (?<!\\d) and \\b
intact).
In `@server/api/changelog/md/`[provider]/[owner]/[repo]/[...path].get.ts:
- Line 34: getGithubMarkDown currently lets ungh.cc exhaustion responses
(FetchError with status 403 or 429) pass through unchanged via handleApiError,
so the endpoint returns raw statuses; update getGithubMarkDown to detect a
FetchError with status 403 or 429 (same logic as detectChangelog.ts lines
~100-105) and convert it to a 502 with the ERROR_UNGH_API_KEY_EXHAUSTED code
before returning, ensuring the endpoint normalises exhaustion errors to 502
rather than returning the upstream 403/429; keep handleApiError usage for other
errors but short-circuit exhaustion handling inside getGithubMarkDown.
In `@server/utils/changelog/detectChangelog.ts`:
- Line 90: The directory boundary check in detectChangelog.ts uses
path.startsWith(directory) which can match sibling prefixes; update the
condition to require a real path boundary by checking either path === directory
or that path.startsWith(directory + "/") (or use path.sep for cross-platform
safety), keeping the existing ROOT_ONLY_REGEX test; change the if that
references directory and path so only true subpaths of directory (or the
directory itself) pass.
In `@server/utils/changelog/markdown.ts`:
- Around line 28-32: The call to marked.use() is inside changelogRenderer(),
causing markedHeadingExtension to be registered repeatedly; move the
marked.use({ tokenizer: { heading: markedHeadingExtension } }) call to module
scope (outside changelogRenderer) so the extension is registered once when the
module loads, and remove the marked.use invocation from inside changelogRenderer
to prevent accumulation/recursion issues.
In `@server/utils/mdKit.ts`:
- Around line 256-262: The fallback empty permalink created when
htmlAnchorRe.test(displayHtml) is true produces an unnamed, focusable link;
update the rendering in the mdKit heading logic (the branch that returns
`<h${semanticLevel} id="${id}" ...>${displayHtml}<a
href="#${id}"></a></h${semanticLevel}>`) to include an accessible name (e.g.
aria-label or visually-hidden text) on that inner anchor using the existing
id/displayHtml to generate a descriptive label, and then update the sanitisation
allowlist to permit that aria attribute so it isn't stripped; apply the same
change to the other heading fallback branch referenced around lines 351-353 so
both cases have labeled permalinks (refer to htmlAnchorRe, displayHtml, id,
semanticLevel, preservedAttrs to locate the code and sanitiser).
- Line 203: The regex htmlAnchorRe is non-global so replace only rewrites the
first anchor; make it global (e.g. add the g flag) and ensure the replace call
uses a replacement function (callback) so every match is rewritten and
playground labels/URL collection logic runs per-anchor; apply the same change to
the other non-global anchor regex used later in this file (the second
anchor-processing block that handles playground collection) so all raw HTML
anchors are processed, not just the first.
In `@test/nuxt/app/utils/header-version-matcher.spec.ts`:
- Line 5: Update the test description string in the spec case for
header-version-matcher (the it(...) call in header-version-matcher.spec.ts) to
fix the typo: change "hould only match version 3.5.1" to "should only match
version 3.5.1" so the test description reads correctly; ensure you edit the
string passed to the it function (the test title) and save the spec.
---
Outside diff comments:
In `@app/pages/package-changelog/`[[org]]/[name].vue:
- Around line 132-173: The UI branches (ChangelogSkeleton,
LazyChangelogReleases, LazyChangelogMarkdown and error) are not mutually
exclusive because ChangelogSkeleton uses its own v-if while the lazy components
start a separate v-if chain; change to a single conditional chain using the same
root conditions (e.g., if changelogError -> show error, else-if
changelog?.type==='release' -> render LazyChangelogReleases, else-if
changelog?.type==='md' -> render LazyChangelogMarkdown, else -> render
ChangelogSkeleton) and remove the standalone v-if on ChangelogSkeleton so only
one branch (using the shared symbols changelog, changelogError,
LazyChangelogReleases, LazyChangelogMarkdown, ChangelogSkeleton,
resolvingPending) can render at a time.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69ba898c-81f3-4bfd-895e-e5659a10d3b7
📒 Files selected for processing (24)
app/components/Changelog/Markdown.vueapp/components/Changelog/Releases.vueapp/components/Changelog/Skeleton.vueapp/components/OgImage/Package.takumi.vueapp/components/Package/ExternalLinks.vueapp/components/Package/Header.vueapp/composables/useCommandPalettePackageCommands.tsapp/composables/usePackageChangelog.tsapp/composables/useProviderIcon.tsapp/pages/package-changelog/[[org]]/[name].vueapp/utils/header-version-matcher.tsapp/utils/router.tsserver/api/changelog/info/[name]/v/[version].get.tsserver/api/changelog/md/[provider]/[owner]/[repo]/[...path].get.tsserver/api/changelog/releases/[provider]/[owner]/[repo].get.tsserver/utils/changelog/detectChangelog.tsserver/utils/changelog/markdown.tsserver/utils/mdKit.tsserver/utils/readme.tstest/nuxt/a11y.spec.tstest/nuxt/app/utils/header-version-matcher.spec.tstest/unit/a11y-component-coverage.spec.tstest/unit/server/utils/changelog/markdown.spec.tstest/unit/server/utils/readme.spec.ts
gameroman
added
the
needs review
Contributor
Author
|
for #2717 (review) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/components/Changelog/Card.vue (1)
24-27: ⚡ Quick winDrop the non-null assertion on
injectand supply a safe default.
inject<{...}>('changelog-provider-linkattr')!only affects TypeScript; if any future caller (tests, Storybook, another consumer) rendersCard.vuewithout the parent providing'changelog-provider-linkattr', the destructuring will throw at runtime. Either provide a default object or guard the destructuring so the card degrades gracefully.♻️ Suggested refactor
-const { providerIcon, viewOnProvider } = inject<{ - providerIcon: ComputedRef<IconClass> - viewOnProvider: ComputedRef<string> -}>('changelog-provider-linkattr')! +const injected = inject<{ + providerIcon: ComputedRef<IconClass> + viewOnProvider: ComputedRef<string> +} | null>('changelog-provider-linkattr', null) +const providerIcon = computed(() => injected?.providerIcon.value) +const viewOnProvider = computed(() => injected?.viewOnProvider.value)You can then
v-if="providerIcon"around theLinkBaseto skip rendering when the context is absent.Also consider using a typed
InjectionKey<...>exported from a shared module instead of a raw string — it removes the need to repeat the generic at every call site and prevents key drift between provider and consumer.🤖 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 `@app/components/Changelog/Card.vue` around lines 24 - 27, The inject call in Card.vue uses a non-null assertion (inject<...>('changelog-provider-linkattr')!) which can throw at runtime if the provider is missing; change it to supply a safe default or guard the result before destructuring: call inject('changelog-provider-linkattr') without the bang, check for null/undefined and fall back to a default object providing safe ComputedRefs for providerIcon and viewOnProvider, then only render the LinkBase when providerIcon is present (e.g., v-if="providerIcon"); optionally replace the raw string key with an exported typed InjectionKey to keep provider/consumer keys consistent (referencing providerIcon, viewOnProvider, inject, and 'changelog-provider-linkattr').app/pages/package-changelog/[[org]]/[name].vue (2)
141-185: ⚡ Quick winRemove the commented-out
<Suspense>block instead of leaving it inline.Per the PR objectives the Suspense-based loading was intentionally replaced with
useLazyFetch'spendinginsideChangelogReleases/ChangelogMarkdown. Leaving ~45 lines of commented template (lines 141–185) creates noise, hides the actual control flow (thev-if/v-else-ifchain reads awkwardly with comments interleaved), and will rot quickly. Git history is sufficient for recovery.♻️ Suggested cleanup
- <!-- <Suspense v-else-if="changelog"> - <template `#default`>--> - <LazyChangelogReleases + <LazyChangelogReleases v-if="changelog?.type === 'release'" :info="changelog" @@ /> </LazyChangelogMarkdown> - <!-- </template> - <template `#fallback`> - <section class="flex flex-col gap-2 py-3"> - <SkeletonBlock class="h-8 w-40 rounded" /> - <ul class="ms-3 list-disc my-[1rem] ps-[1.5rem] marker:color-border-hover"> - <li class="mb-1" v-for="_n in 5"> - <SkeletonBlock class="h-7 w-full max-w-2xl rounded" /> - </li> - </ul> - - <SkeletonBlock class="h-5 w-5/6 max-w-2xl rounded" /> - <SkeletonBlock class="h-5 w-3/4 max-w-2xl rounded" /> - </section> - </template> - </Suspense> --> <!-- error handling -->🤖 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 `@app/pages/package-changelog/`[[org]]/[name].vue around lines 141 - 185, Remove the large commented-out Suspense block and its inner fallback template so the template contains only the active v-if/v-else-if chain; delete the commented "<Suspense> ... </Suspense>" section wrapping (and any commented fallback markup) that surrounds the LazyChangelogReleases and LazyChangelogMarkdown usage, leaving the LazyChangelogReleases, LazyChangelogMarkdown and their LazyChangelogErrorMsg children intact; ensure no other logic relies on that Suspense wrapper (the components ChangelogReleases/ChangelogMarkdown now handle loading via useLazyFetch pending) and run a quick lint/format pass to confirm the template remains valid.
151-170: 💤 Low valueRename
viewOnGitprop toviewOnProviderinChangelogErrorMsgcomponent for consistency.The variable was renamed to generic provider terminology in line 51 (
viewOnProvider), but the component's prop on lines 154 and 168 still uses:viewOnGit="viewOnProvider". Update the prop name inapp/components/Changelog/ErrorMsg.vueto maintain uniform abstraction.🤖 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 `@app/pages/package-changelog/`[[org]]/[name].vue around lines 151 - 170, The Changelog error component still defines/accepts the old prop name viewOnGit while callers (LazyChangelogErrorMsg) pass viewOnProvider; open app/components/Changelog/ErrorMsg.vue and rename the prop from viewOnGit to viewOnProvider (keep the same type/default/behavior), update any internal references in the component (props destructuring, template bindings, methods) to use viewOnProvider, and ensure the component export/props declaration matches the new name so the usages at lines where LazyChangelogErrorMsg is rendered work consistently.
🤖 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 `@app/components/Changelog/Card.vue`:
- Around line 24-27: The inject call in Card.vue uses a non-null assertion
(inject<...>('changelog-provider-linkattr')!) which can throw at runtime if the
provider is missing; change it to supply a safe default or guard the result
before destructuring: call inject('changelog-provider-linkattr') without the
bang, check for null/undefined and fall back to a default object providing safe
ComputedRefs for providerIcon and viewOnProvider, then only render the LinkBase
when providerIcon is present (e.g., v-if="providerIcon"); optionally replace the
raw string key with an exported typed InjectionKey to keep provider/consumer
keys consistent (referencing providerIcon, viewOnProvider, inject, and
'changelog-provider-linkattr').
In `@app/pages/package-changelog/`[[org]]/[name].vue:
- Around line 141-185: Remove the large commented-out Suspense block and its
inner fallback template so the template contains only the active v-if/v-else-if
chain; delete the commented "<Suspense> ... </Suspense>" section wrapping (and
any commented fallback markup) that surrounds the LazyChangelogReleases and
LazyChangelogMarkdown usage, leaving the LazyChangelogReleases,
LazyChangelogMarkdown and their LazyChangelogErrorMsg children intact; ensure no
other logic relies on that Suspense wrapper (the components
ChangelogReleases/ChangelogMarkdown now handle loading via useLazyFetch pending)
and run a quick lint/format pass to confirm the template remains valid.
- Around line 151-170: The Changelog error component still defines/accepts the
old prop name viewOnGit while callers (LazyChangelogErrorMsg) pass
viewOnProvider; open app/components/Changelog/ErrorMsg.vue and rename the prop
from viewOnGit to viewOnProvider (keep the same type/default/behavior), update
any internal references in the component (props destructuring, template
bindings, methods) to use viewOnProvider, and ensure the component export/props
declaration matches the new name so the usages at lines where
LazyChangelogErrorMsg is rendered work consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2996937-03cf-43bc-ac75-a25809c790e4
📒 Files selected for processing (4)
app/components/Changelog/Card.vueapp/pages/package-changelog/[[org]]/[name].vueserver/api/changelog/releases/[provider]/[owner]/[repo].get.tsshared/types/changelog.ts
✅ Files skipped from review due to trivial changes (1)
- server/api/changelog/releases/[provider]/[owner]/[repo].get.ts
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
3 participants
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.
🔗 Linked issue
This pr is part of #501
🧭 Context
Currently changelog uses a seperate marked markdown renderer from readme, this pr takes the shared code and makes it into a "markdown kit" so that code that can be shared will be shared
📚 Description
Moves shared code for both changelog & readme to a shared markdown kit while still allowing for each to have their unique needs.
additional changes
## heading-><hx><a href="#...>heading</a></hx>## [Example](https://example.com)-><hx><a href="https://example.com">Example</a><a href="#...></a></hx>package.takumi.vue&externallinks.vuethe provider icons are now given via theuseProviderIconcomposablependingis being used fromuseLazyFetchin theChangelogReleases&ChangelogMarkdowncomponents instead of using<suspense>at the package-changelog page, this should remove/reduce the blank screen between showing the skeleton laoders & showing data.AI usage
For
app/utils/header-version-matcher.tsI've used Gemini to get the regex for matching more exact the requested version in a toc item textnotes
The branch of this pr was based on #1233 initially, that's why there are so many commits, file changes does show correctly what has changed