refactor: comprehensive security, performance & architecture optimization#11
Merged
Conversation
…tion - Fix XSS vulnerability: escape HTML before markdown rendering in commit messages - Fix delimiter collision: replace `|` with `\x1F` (Unit Separator) in git pretty formats - Fix valaxy.config.ts: use `defineValaxyConfig` instead of `defineTheme` - Cache git-log.json client-side to avoid redundant fetches on navigation - Replace @octokit/rest with native fetch in client bundle (~100KB saved) - Optimize deduplicateContributors from O(n²) to O(n) via Map grouping - Replace sync execFileSync with async git.raw() in getLastUpdated - Only cache changelog in CI mode for fresh data during dev - Reduce maxConcurrentProcesses from 200 to 10 - Decouple GitLogChangelog.vue from @vueuse/metadata - Add configurable changelog filter (includeTypes, includeBreaking) - Add useGitLogState() composable with loading/error states - Define types locally, remove @vueuse/metadata type dependency - Move @octokit/rest to optionalDependencies Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Integrate upstream's import.meta.env.BASE_URL fix into the module-level fetch cache function, preserving both improvements. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the addon’s git/changelog pipeline to reduce build/runtime overhead, remove the VueUse metadata dependency, and harden commit-message rendering. It mainly affects how changelog/contributor data is parsed, cached, exposed, and rendered across build-time and client-side paths.
Changes:
- Switched changelog/contributor parsing to custom local types and a new field separator, plus added configurable changelog filtering.
- Added client-side git-log caching and a new
useGitLogState()composable, while simplifyingGitLogChangelogto consume provided/global changelog data. - Reworked contributor/changelog fetching internals, including async git calls, deduplication changes, and packaging changes around
@octokit/rest.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
valaxy.config.ts |
Swaps local config helper to defineValaxyConfig. |
utils/render.ts |
Adds HTML escaping before markdown rendering and adjusts issue-link output. |
types/index.ts |
Replaces imported metadata types with local interfaces and adds changelog filter config/helper. |
test/node/gitLog.test.ts |
Adds tests for new separator parsing and shouldIncludeCommit. |
shims.d.ts |
Updates virtual changelog module typing to local Changelog type. |
package.json |
Moves @octokit/rest to optional/dev deps and bumps tool versions. |
node/index.ts |
Lowers simple-git concurrency. |
node/gitLog.ts |
Refactors batch contributor/changelog parsing and filtering. |
node/contributor.ts |
Optimizes deduplication and makes last-updated lookup async. |
node/changeLog.ts |
Refactors changelog parsing/filtering and cache behavior. |
components/GitLogChangelog.vue |
Removes VueUse metadata coupling and accepts direct changelog input. |
client/services/fetchContributors.ts |
Replaces Octokit client usage with native fetch. |
client/composables/gitlog.ts |
Adds shared fetch cache and useGitLogState(). |
client/composables/changelog.ts |
Renames runtime computed state and documents runtime fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Sanitize URLs in markdown renderer: block javascript:, data:, vbscript: protocols in links and images to close XSS vector (copilot #3) - Document Changelog.body as deprecated — field was never reliably populated due to %b multiline conflicts with --name-only parsing (copilot #1, #2) - Move @octokit/rest back to dependencies to prevent silent feature loss when installed with --no-optional (copilot #4) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Block protocol-relative URLs (//evil.example) in sanitizeUrl regex - Add res.ok check before parsing git-log.json response - Fix comment/behavior mismatch on changelog cache condition - Replace O(n²) indexOf dedup with Set-based O(n) dedup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
Copilot Review Loop — SummaryPR: #11 (#11)
Commits:
Remaining items needing human review: None |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 15 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
node/contributor.ts:132
getContributors()appendscontributor.logArgsas a single argv entry. If users pass multiple flags (e.g. "--no-merges --reverse"), git will receive it as one argument and fail/ignore it. Consider splittinglogArgson whitespace (likebatchGetContributorsdoes) or changing the option type to an array of args.
try {
const gitArgs: string[] = ['log', '--no-merges', '--pretty=format:%an\x1F%ae']
const additionalArgs: string[] = [
filePath && `--`,
filePath,
contributor?.logArgs,
].filter((arg): arg is string => arg != null)
const gitLog = await git.raw([...gitArgs, ...additionalArgs])
- Fix nested <a> tags: only replace #issue refs in text content, not inside tags - Fix breaking change detection: match conventional commit `type!:` pattern instead of any `!` anywhere in message - Fix type matching: require `type:`, `type(`, or `type!` after prefix to prevent false positives like `feature:` matching `feat` - Fix contributor map key: use email instead of name to avoid merging distinct contributors with same display name Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 15 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
node/contributor.ts:131
- In
getContributors(),contributor.logArgsis appended after the--+filePatharguments. Anything after--is treated as a pathspec by git, sologArgswill be ignored/break whenfilePathis provided. Consider splittinglogArgsinto tokens and placing them before--(and beforefilePath) so custom git-log args work consistently.
const gitArgs: string[] = ['log', '--no-merges', '--pretty=format:%an\x1F%ae']
const additionalArgs: string[] = [
filePath && `--`,
filePath,
contributor?.logArgs,
].filter((arg): arg is string => arg != null)
- Prevent nested <a> tags when commit messages contain markdown links with issue references (e.g. `[#123](url)`) by splitting on anchor elements before replacing #N patterns - Include statusText and URL in fetchContributors error message for easier debugging of rate-limit (403) responses Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment on lines
+107
to
+119
| /** | ||
| * Shared filter for deciding which commits to include in changelog output. | ||
| */ | ||
| export function shouldIncludeCommit(message: string, options?: GitLogOptions['changelog']): boolean { | ||
| const types = options?.includeTypes ?? ['feat', 'fix'] | ||
| const includeBreaking = options?.includeBreaking ?? true | ||
|
|
||
| if (message.includes('chore: release')) | ||
| return true | ||
| if (includeBreaking && RE_BREAKING.test(message)) | ||
| return true | ||
| return types.some(type => matchesType(message, type)) | ||
| } |
Member
Author
There was a problem hiding this comment.
Fixed ✅ — Added export * from './types' to index.ts so shouldIncludeCommit (and other runtime helpers from the types module) are available at runtime, matching the existing index.d.ts re-exports. Now both typecheck and runtime resolve consistently for import { shouldIncludeCommit } from 'valaxy-addon-git-log'.
Comment on lines
+155
to
162
| /** | ||
| * Get the last updated timestamp of a file from git history. | ||
| * Uses async git command to avoid blocking the event loop. | ||
| */ | ||
| export async function getLastUpdated(filePath: string): Promise<number> { | ||
| const result = await git.raw(['log', '-1', '--format=%ct', '--', filePath]) | ||
| return Number.parseInt(result.trim(), 10) * 1000 | ||
| } |
Member
Author
There was a problem hiding this comment.
Fixed ✅ — Reverted getLastUpdated to a synchronous function using execFileSync to preserve the original API contract and avoid breaking consumers calling it sync. Added a new getLastUpdatedAsync for async pipelines that want non-blocking git calls.
- Re-export ./types from index.ts so runtime helpers like shouldIncludeCommit are available at runtime, not just types. - Restore getLastUpdated to sync (backward compatible) and add getLastUpdatedAsync for async pipelines. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment on lines
+116
to
+117
| if (includeBreaking && RE_BREAKING.test(message)) | ||
| return true |
Member
Author
There was a problem hiding this comment.
Fixed ✅ — When the commit message matches RE_BREAKING, shouldIncludeCommit now returns includeBreaking directly (short-circuiting the types.some(matchesType) path). This way feat!: / feat(scope)!: / fix!: are excluded when includeBreaking: false, regardless of whether their type token is in includeTypes.
| it('should respect includeBreaking: false', async () => { | ||
| const { shouldIncludeCommit } = await import('../../types') | ||
| const options = { includeBreaking: false } | ||
| expect(shouldIncludeCommit('refactor!: breaking', options)).toBe(false) |
Member
Author
There was a problem hiding this comment.
Fixed ✅ — Extended the includeBreaking: false test to assert feat!: ..., feat(scope)!: ..., and fix!: ... are excluded, while non-breaking commits of the same types (feat: ..., fix(scope): ...) still pass through.
Comment on lines
+69
to
+78
| * Replace #issue references in HTML text that is NOT inside an <a> element. | ||
| * Splits on anchor tags to avoid producing nested <a> elements. | ||
| */ | ||
| function replaceIssueRefs(html: string, repo: string): string { | ||
| // Split by <a ...>...</a> segments, only replace in non-anchor parts | ||
| return html.replace(/(<a\s[^>]*>[\s\S]*?<\/a>)|([^<]*(?:<(?!a\s|\/a>)[^<]*)*)/gi, (match, anchor) => { | ||
| if (anchor) | ||
| return match // preserve anchor tags untouched | ||
| return match.replace(RE_ISSUE, `<a href='${repo}/issues/$1'>#$1</a>`) | ||
| }) |
Member
Author
There was a problem hiding this comment.
Fixed ✅ — replaceIssueRefs now tokenizes the HTML into three kinds of segments: <a>...</a> blocks, single tags (e.g. <img>, <code>), and text nodes. Replacements only run on text nodes, so attributes like alt / src / href are no longer rewritten and nested anchors are still avoided.
Regex: /(<a\s[^>]*>[\s\S]*?<\/a>)|(<[^>]+>)|([^<]+)/gi — only the third capture (text) is processed, the others are returned verbatim.
- shouldIncludeCommit: when `includeBreaking: false`, also exclude breaking commits whose type token is in `includeTypes` (e.g. `feat!:`, `feat(scope)!:`). Previously they leaked through via the `types.some(matchesType)` path. - Cover the regression with explicit `feat!:` / `feat(scope)!:` / `fix!:` test cases under `includeBreaking: false`. - replaceIssueRefs: tokenize HTML into anchor blocks, single tags, and text nodes so `#N` is only rewritten in text. Prevents `<img alt='#1'/>` / `<code>#1</code>` etc. from being rewritten in attributes/tag bodies. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment on lines
196
to
200
| const raw = await git.raw([ | ||
| 'log', | ||
| '--name-only', | ||
| '--pretty=format:---CL_SEP---%H|%an|%ae|%aI|%s|%b', | ||
| `--pretty=format:---CL_SEP---%H${FIELD_SEP}%an${FIELD_SEP}%ae${FIELD_SEP}%aI${FIELD_SEP}%s`, | ||
| '--', |
Member
Author
There was a problem hiding this comment.
Fixed ✅ — Added --max-count=${maxCount * filePaths.length} to the git log invocation in batchGetChangelog. With multiple pathspecs git's --max-count is a global cap, so per-file truncation in JS is still required, but the new bound prevents reading the full history on large repos. The block comment now explains the trade-off.
Comment on lines
58
to
66
| // Simulate contributor git log output using \x1f separator | ||
| const contributorOutput = [ | ||
| '---COMMIT_SEP---Alice|alice@example.com', | ||
| `---COMMIT_SEP---Alice${FIELD_SEP}alice@example.com`, | ||
| 'pages/index.md', | ||
| '---COMMIT_SEP---Bob|bob@example.com', | ||
| `---COMMIT_SEP---Bob${FIELD_SEP}bob@example.com`, | ||
| 'pages/index.md', | ||
| '---COMMIT_SEP---Alice|alice@example.com', | ||
| `---COMMIT_SEP---Alice${FIELD_SEP}alice@example.com`, | ||
| 'pages/about.md', | ||
| ].join('\n') |
Member
Author
There was a problem hiding this comment.
Fixed ✅ — The test now calls flushGitLogBatch after handleGitLogInfo (parsing actually happens in the flush step), uses <cwd>/pages/index.md so it passes both the currentWorkingDirectory guard and the path.resolve(resolvedBase, file) matching, and asserts on fm.git_log.contributors content (Alice count=2, Bob count=1) — verifying both the \x1F parsing and the per-file grouping.
Comment on lines
+98
to
+106
| it('should parse contributor names containing pipe characters', async () => { | ||
| const { git } = await import('../../node/index') | ||
| const rawMock = vi.mocked(git.raw) | ||
|
|
||
| // Author name containing | should NOT break parsing with \x1f separator | ||
| const contributorOutput = [ | ||
| `---COMMIT_SEP---Alice|Bob${FIELD_SEP}alice@example.com`, | ||
| 'pages/index.md', | ||
| ].join('\n') |
Member
Author
There was a problem hiding this comment.
Fixed ✅ — Same restructure as the sibling test: flushGitLogBatch is now invoked, and the assertion explicitly checks fm.git_log.contributors[0].name === 'Alice|Bob' and the email is preserved. This actually exercises the parser and proves the \x1F separator no longer collides with | in author names.
- batchGetChangelog: bound git log with --max-count = maxCount * filePaths.length so multi-path calls no longer read the full repo history when a maxCount guard is set. Per-file slicing still happens in JS for correctness. - Rewrite the two batch-parsing tests so they actually call flushGitLogBatch (where parsing happens). Previously the mocked git.raw output was never consumed, so the tests only verified frontmatter.path. New assertions check that contributors are parsed with the \x1F separator and that pipe-in-name authors are preserved verbatim. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
YunYouJun
added a commit
that referenced
this pull request
May 7, 2026
…tion (#11) * refactor: comprehensive security, performance & architecture optimization - Fix XSS vulnerability: escape HTML before markdown rendering in commit messages - Fix delimiter collision: replace `|` with `\x1F` (Unit Separator) in git pretty formats - Fix valaxy.config.ts: use `defineValaxyConfig` instead of `defineTheme` - Cache git-log.json client-side to avoid redundant fetches on navigation - Replace @octokit/rest with native fetch in client bundle (~100KB saved) - Optimize deduplicateContributors from O(n²) to O(n) via Map grouping - Replace sync execFileSync with async git.raw() in getLastUpdated - Only cache changelog in CI mode for fresh data during dev - Reduce maxConcurrentProcesses from 200 to 10 - Decouple GitLogChangelog.vue from @vueuse/metadata - Add configurable changelog filter (includeTypes, includeBreaking) - Add useGitLogState() composable with loading/error states - Define types locally, remove @vueuse/metadata type dependency - Move @octokit/rest to optionalDependencies Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: update lock * fix: address copilot review feedback - Sanitize URLs in markdown renderer: block javascript:, data:, vbscript: protocols in links and images to close XSS vector (copilot #3) - Document Changelog.body as deprecated — field was never reliably populated due to %b multiline conflicts with --name-only parsing (copilot #1, #2) - Move @octokit/rest back to dependencies to prevent silent feature loss when installed with --no-optional (copilot #4) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address copilot review feedback (cycle 2) - Block protocol-relative URLs (//evil.example) in sanitizeUrl regex - Add res.ok check before parsing git-log.json response - Fix comment/behavior mismatch on changelog cache condition - Replace O(n²) indexOf dedup with Set-based O(n) dedup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address copilot review feedback (cycle 3) - Fix nested <a> tags: only replace #issue refs in text content, not inside tags - Fix breaking change detection: match conventional commit `type!:` pattern instead of any `!` anywhere in message - Fix type matching: require `type:`, `type(`, or `type!` after prefix to prevent false positives like `feature:` matching `feat` - Fix contributor map key: use email instead of name to avoid merging distinct contributors with same display name Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address remaining copilot review feedback - Prevent nested <a> tags when commit messages contain markdown links with issue references (e.g. `[#123](url)`) by splitting on anchor elements before replacing #N patterns - Include statusText and URL in fetchContributors error message for easier debugging of rate-limit (403) responses Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address copilot review feedback (cycle 4) - Re-export ./types from index.ts so runtime helpers like shouldIncludeCommit are available at runtime, not just types. - Restore getLastUpdated to sync (backward compatible) and add getLastUpdatedAsync for async pipelines. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: address copilot review feedback (cycle 5) - shouldIncludeCommit: when `includeBreaking: false`, also exclude breaking commits whose type token is in `includeTypes` (e.g. `feat!:`, `feat(scope)!:`). Previously they leaked through via the `types.some(matchesType)` path. - Cover the regression with explicit `feat!:` / `feat(scope)!:` / `fix!:` test cases under `includeBreaking: false`. - replaceIssueRefs: tokenize HTML into anchor blocks, single tags, and text nodes so `#N` is only rewritten in text. Prevents `<img alt='#1'/>` / `<code>#1</code>` etc. from being rewritten in attributes/tag bodies. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: address copilot review feedback (cycle 6) - batchGetChangelog: bound git log with --max-count = maxCount * filePaths.length so multi-path calls no longer read the full repo history when a maxCount guard is set. Per-file slicing still happens in JS for correctness. - Rewrite the two batch-parsing tests so they actually call flushGitLogBatch (where parsing happens). Previously the mocked git.raw output was never consumed, so the tests only verified frontmatter.path. New assertions check that contributors are parsed with the \x1F separator and that pipe-in-name authors are preserved verbatim. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
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.
Summary
|→\x1F)@octokit/restfrom bundle (~100KB), O(n) dedup algorithm, asyncgetLastUpdated, dev-mode cache invalidation@vueuse/metadata, configurable changelog filter, properdefineValaxyConfigAPI,useGitLogState()with loading/error statesChanges
utils/render.tsnode/gitLog.ts,node/changeLog.ts\x1Fseparator instead of|client/composables/gitlog.tsclient/services/fetchContributors.tsfetchreplaces@octokit/restnode/contributor.tsgetLastUpdatednode/index.tsmaxConcurrentProcesses: 10(was 200)node/changeLog.tstypes/index.tsshouldIncludeCommit+changelogconfigvalaxy.config.tsdefineValaxyConfig(wasdefineTheme)components/GitLogChangelog.vue@vueuse/metadataclient/composables/gitlog.tsuseGitLogState()with loading/errorpackage.json@octokit/rest→optionalDependenciesTest plan
pnpm test— 46 tests pass (including newshouldIncludeCommitand pipe-in-name tests)pnpm lint— 0 errorspnpm typecheck— cleangit-log.jsonfetched only once across navigations (Network tab)<img onerror=alert(1)>→ escaped)🤖 Generated with Claude Code