feat(content-utils): add commit history tracking to git info#265
feat(content-utils): add commit history tracking to git info#265Fryuni wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds optional commit-history collection and lazy per-commit content access to the content-utils Git tracking subsystem, refactors plugin factories to accept a unified IntegrationState, and adds test fixtures plus CI and repo metadata updates for handling packed .git fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Setup as Integration Setup
participant Plugin as Dev/Build Plugin
participant GitCollector as Git Runtime/Collector
participant Lazy as Lazy Utility
participant Consumer as Runtime API / Page
Setup->>Plugin: pass IntegrationState (includes collectCommitHistory)
Plugin->>GitCollector: initialize with projectRoot & collectCommitHistory
alt collectCommitHistory = true
GitCollector->>GitCollector: run git log parsing (include hashes)
GitCollector->>GitCollector: build RawCommitInfo per file
GitCollector->>Lazy: wrap getFileContentAtCommit into Lazy getter
GitCollector-->>Plugin: attach commits (lazy content) to tracking info
else collectCommitHistory = false
GitCollector->>Plugin: return tracking info without commits
end
Consumer->>Plugin: getEntryGitInfo()
Plugin-->>Consumer: returns commits array where content is fetched only when accessed (Lazy resolves)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
@inox-tools/aik-mod
@inox-tools/aik-route-config
@inox-tools/astro-tests
@inox-tools/astro-when
@inox-tools/content-utils
@inox-tools/custom-routing
@inox-tools/cut-short
@inox-tools/inline-mod
@inox-tools/modular-station
@inox-tools/portal-gun
@inox-tools/request-nanostores
@inox-tools/request-state
@inox-tools/runtime-logger
@inox-tools/server-islands
@inox-tools/sitemap-ext
@inox-tools/star-warp
@inox-tools/utils
@inox-tools/velox-luna
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/content-utils/src/runtime/liveGit.ts (1)
68-71:⚠️ Potential issue | 🔴 CriticalPre-existing bug:
getOldestCommitDatereturnsinfo.latestinstead ofinfo.earliest.Line 71 returns
info.latest, but this function is supposed to return the oldest commit date. This should beinfo.earliest.🐛 Proposed fix
export async function getOldestCommitDate(...args: Parameters<typeof getEntry>): Promise<Date> { const [file, info] = await getEntryGitInfoInner(args); - if (info !== undefined) return info.latest; + if (info !== undefined) return info.earliest;
🤖 Fix all issues with AI agents
In @.gitattributes:
- Line 1: Add Git LFS to CI and docs: update the CI workflow checkout step using
actions/checkout@v3 to enable lfs by adding lfs: true to the step, and update
every other workflow that runs tests to do the same; then add contributor setup
instructions to README.md or CONTRIBUTING.md describing running git lfs install
and git lfs pull (and noting the dependency) so local clones and CI will fetch
the .git.tar.gz fixture.
In `@packages/content-utils/src/runtime/git.ts`:
- Around line 46-58: getRepoRoot() is currently invoking a new git subprocess on
every call (causing many redundant spawnSync calls from getFileContentAtCommit
and the pre-fetch loop); memoize the repo root by introducing a module-level
cachedRepoRoot and have getRepoRoot return the cached value when set, otherwise
run spawnSync once and store the result; update setProjectRoot to clear/reset
cachedRepoRoot (e.g., set cachedRepoRoot = undefined) so changes to the root
invalidate the cache; then ensure getFileContentAtCommit and any other callers
use the memoized getRepoRoot.
In `@packages/content-utils/tests/git-tracking.test.ts`:
- Around line 39-47: Tests fail because the fixture .git.tar.gz is a Git LFS
pointer and CI checks out only the pointer; update CI to fetch LFS objects or
fetch them before tests: modify the test job’s checkout step to use
actions/checkout@v4 with lfs: true, or add an explicit git lfs install && git
lfs pull step prior to running tests so the beforeAll block that spawns tar to
unpack '.git.tar.gz' (using fixturePath) operates on the real gzip archive
rather than a 130-byte pointer; alternatively, replace the LFS-tracked fixture
with a normal committed file if size permits.
🧹 Nitpick comments (5)
.gitignore (1)
62-63: Clean up duplicate entry.The
test-results/entry appears twice. Consider removing the duplicate for cleaner maintenance.🧹 Proposed cleanup
# Test results test-results/ -test-results/ playwright-report/packages/content-utils/src/runtime/liveGit.ts (1)
25-37: Duplicate CommitInfo construction logic — consider extracting a shared helper.This mapping logic (create
Record<string, unknown>, set fields,Object.definePropertyfor lazy content, cast toCommitInfo) is duplicated nearly verbatim ingit.ts(lines 214–226). Extracting a sharedbuildCommitInfo(c: RawCommitInfo): CommitInfohelper ingit.tsand importing it here would reduce duplication and ensure consistent construction.packages/content-utils/src/runtime/git.ts (1)
206-252: Hook-exposedGitTrackingInfowith commits is constructed then discarded — hook mutations to commits are lost.Lines 214–226 build
CommitInfo[]with lazy getters for the hook callback at line 235, but the function returnsrawFileInfo(line 251), notfileInfo. Whileearliest/latestare synced back (lines 248–249), any modifications a hook consumer makes tofileInfo.commits(orauthors/coAuthors) are silently dropped.Currently the hook only exposes
drop(), so this isn't actively broken, but it's a latent inconsistency that will surprise future hook consumers. Worth a TODO or syncing all fields back.packages/content-utils/src/integration/gitPlugin.ts (1)
96-102: Build-time pre-fetch mutatesRawCommitInfoobjects in place — acceptable but consider a brief comment.The
as anycasts to addcontentand deleterepoPathare pragmatic for preparing serialization, but they silently change the shape of objects typed asRawCommitInfo. A one-line comment explaining why the mutation is safe (objects are consumed only bydevalue.stringifyimmediately after) would help future readers.packages/content-utils/tests/git-tracking.test.ts (1)
26-36: Missing'/'in HTML entity unescaping.The unescape helper handles
&,<,>, and"but omits'(apostrophe), which Astro may also encode. This is unlikely to matter for the current JSON payloads (no single quotes in values), but worth noting for robustness.Proposed fix
const json = match[1] .replace(/&/g, '&') .replace(/</g, '<') .replace(/>/g, '>') - .replace(/"/g, '"'); + .replace(/"/g, '"') + .replace(/'/g, "'");
| @@ -0,0 +1 @@ | |||
| packages/*/tests/fixture/**/.git.tar.gz filter=lfs diff=lfs merge=lfs -text | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for .git.tar.gz files matching the pattern
echo "=== Searching for .git.tar.gz files in packages/*/tests/fixture ==="
find packages -type f -name "*.git.tar.gz" 2>/dev/null || echo "No .git.tar.gz files found"
echo ""
echo "=== Checking directory structure ==="
find packages -type d -name fixture 2>/dev/null | head -20 || echo "No fixture directories found"
echo ""
echo "=== Looking for any fixture files ==="
find packages -path "*/tests/fixture/*" -type f 2>/dev/null | head -20 || echo "No fixture files found"Repository: Fryuni/inox-tools
Length of output: 1905
🏁 Script executed:
#!/bin/bash
# Check for Git LFS documentation
echo "=== Searching for Git LFS documentation ==="
if [ -f README.md ]; then
echo "README.md exists, checking for LFS references:"
grep -i "git lfs\|lfs\|large file" README.md || echo "No LFS references in README.md"
fi
if [ -f CONTRIBUTING.md ]; then
echo "CONTRIBUTING.md exists, checking for LFS references:"
grep -i "git lfs\|lfs\|large file" CONTRIBUTING.md || echo "No LFS references in CONTRIBUTING.md"
fi
echo ""
echo "=== Checking for any setup/installation docs ==="
find . -maxdepth 2 -type f \( -name "*.md" -o -name "SETUP*" -o -name "INSTALL*" \) 2>/dev/null | head -20Repository: Fryuni/inox-tools
Length of output: 353
🏁 Script executed:
#!/bin/bash
# Check CI/CD configuration for Git LFS
echo "=== Searching for CI/CD configuration files ==="
find . -path "./.git*" -prune -o -type f \( -name "*.yml" -o -name "*.yaml" \) -print | grep -E "(github|gitlab|circle|travis|jenkins|workflows)" | head -20
echo ""
echo "=== Checking GitHub Actions workflows ==="
if [ -d ".github/workflows" ]; then
ls -la .github/workflows/
echo ""
echo "=== Checking for lfs in workflow files ==="
grep -r "lfs\|LFS" .github/workflows/ 2>/dev/null || echo "No LFS references in workflows"
else
echo "No .github/workflows directory found"
fiRepository: Fryuni/inox-tools
Length of output: 840
🏁 Script executed:
#!/bin/bash
# Check what Git LFS is currently tracking
echo "=== Current Git LFS tracking ==="
if command -v git lfs &> /dev/null; then
git lfs ls-files || echo "Git LFS command available but no files tracked yet"
else
echo "Git LFS not installed in this environment"
fi
echo ""
echo "=== Checking .gitattributes content ==="
if [ -f .gitattributes ]; then
cat -n .gitattributes
fiRepository: Fryuni/inox-tools
Length of output: 370
Add Git LFS configuration to CI/CD workflows and document the dependency for contributors.
The .gitattributes pattern correctly targets packages/content-utils/tests/fixture/git-tracking/.git.tar.gz, which exists in the repository. However, this introduces critical issues:
-
CI/CD will fail: The GitHub Actions workflows (ci.yml, changesets.yml, etc.) don't have Git LFS configured. Tests requiring the
.git.tar.gzfixture will fail because the file won't be pulled during checkout. -
Undocumented requirement: README.md and other setup documentation don't mention the Git LFS requirement, leaving contributors unable to clone or work with these fixtures locally.
Required fixes:
- Add
lfs: trueto theactions/checkout@v3step in.github/workflows/ci.ymland any other workflows that run tests - Add Git LFS setup documentation to README.md or CONTRIBUTING.md, including
git lfs installandgit lfs pullinstructions for developers
🤖 Prompt for AI Agents
In @.gitattributes at line 1, Add Git LFS to CI and docs: update the CI workflow
checkout step using actions/checkout@v3 to enable lfs by adding lfs: true to the
step, and update every other workflow that runs tests to do the same; then add
contributor setup instructions to README.md or CONTRIBUTING.md describing
running git lfs install and git lfs pull (and noting the dependency) so local
clones and CI will fetch the .git.tar.gz fixture.
| export function getFileContentAtCommit(hash: string, repoPath: string): string { | ||
| const result = spawnSync('git', ['show', `${hash}:${repoPath}`], { | ||
| cwd: getRepoRoot(), | ||
| encoding: 'utf-8', | ||
| }); | ||
|
|
||
| if (result.error || result.status !== 0) { | ||
| debug('Failed to retrieve file content at commit:', hash, repoPath, result.stderr); | ||
| return ''; | ||
| } | ||
|
|
||
| return result.stdout; | ||
| } |
There was a problem hiding this comment.
getRepoRoot() spawns a new git rev-parse process on every call — memoize it.
getFileContentAtCommit calls getRepoRoot() each time, which runs spawnSync('git', ['rev-parse', '--show-toplevel']). In dev mode, every lazy content access triggers this. In build mode, the pre-fetch loop in gitPlugin.ts calls this for every commit across all files. This could mean hundreds of redundant git subprocesses.
⚡ Proposed fix — memoize getRepoRoot
+let cachedRepoRoot: string | undefined;
+
function getRepoRoot(): string {
+ if (cachedRepoRoot !== undefined) return cachedRepoRoot;
debug('Retrieving git repo root', { projectRoot });
const result = spawnSync('git', ['rev-parse', '--show-toplevel'], {
cwd: projectRoot,
encoding: 'utf-8',
});
if (result.error) {
debug(`Failed to retrieve repo root:`, result.error, result.stderr);
debug('Falling back to contentPath:', projectRoot);
- return projectRoot;
+ cachedRepoRoot = projectRoot;
+ return cachedRepoRoot;
}
- return result.stdout.trim();
+ cachedRepoRoot = result.stdout.trim();
+ return cachedRepoRoot;
}Note: setProjectRoot should also reset the cache (cachedRepoRoot = undefined;) to stay consistent if the root changes.
🤖 Prompt for AI Agents
In `@packages/content-utils/src/runtime/git.ts` around lines 46 - 58,
getRepoRoot() is currently invoking a new git subprocess on every call (causing
many redundant spawnSync calls from getFileContentAtCommit and the pre-fetch
loop); memoize the repo root by introducing a module-level cachedRepoRoot and
have getRepoRoot return the cached value when set, otherwise run spawnSync once
and store the result; update setProjectRoot to clear/reset cachedRepoRoot (e.g.,
set cachedRepoRoot = undefined) so changes to the root invalidate the cache;
then ensure getFileContentAtCommit and any other callers use the memoized
getRepoRoot.
|
|
||
| # Nested git repos in test fixtures (unpacked from .git.tar.gz during tests) | ||
| packages/*/tests/fixture/**/.git |
There was a problem hiding this comment.
testing new pr review tool
1 participant
Summary
Adds commit history tracking to
getEntryGitInfo()in@inox-tools/content-utils. Each content entry now includes a full list of commits that modified it, with lazy content retrieval at each commit point.Features
Core Functionality
commitsfield onGitTrackingInfocontaining an array ofCommitInfoobjectshash- Full 40-character SHAdate- Date objectauthor+coAuthors- GitAuthor objectscontent- Lazy getter that retrieves file content at that commit (memoized viaLazy.wrap)Configuration
collectCommitHistoryoption (default:true) - Allows disabling for memory-constrained projectscommitsreturns empty array[]Implementation Details
git showexecution viaObject.definePropertygetterLazy.wrapfor API consistencygit showfails (deleted files, shallow clones)%Hin--format)Type Changes
Testing
Test Infrastructure
.git.tar.gz(tracked via Git LFS)collectCommitHistory: false) returns empty arrayTest Results
Commits
ab44d0a- chore: add LFS tracking for fixture git archives and ignore nested .git dirs717813b- feat(content-utils): add CommitInfo type and collectCommitHistory option36dc607- test(content-utils): add git-tracking fixture with archived git repoc0c52aa- feat(content-utils): add commit history parsing and content helper to git runtime2ea1a48- feat(content-utils): add commit history support for dev and build modes94179b7- test(content-utils): add integration tests for commit history trackingFiles Changed
17 files changed: 349 insertions(+), 24 deletions(-)
Modified
packages/content-utils/virtual.d.ts- Type definitionspackages/content-utils/src/integration/state.ts- State managementpackages/content-utils/src/integration/index.ts- Integration optionpackages/content-utils/src/runtime/git.ts- Core git parsing logicpackages/content-utils/src/runtime/liveGit.ts- Dev mode supportpackages/content-utils/src/integration/gitPlugin.ts- Dev/build plugins.gitattributes- Git LFS tracking for test archives.gitignore- Ignore unpacked.gitdirectoriesCreated
packages/content-utils/tests/fixture/git-tracking/- Complete test fixturepackages/content-utils/tests/git-tracking.test.ts- Integration tests (103 lines)Breaking Changes
None - Fully backward compatible. The
commitsfield is optional in types and always populated at runtime.Dependencies
None added - Uses existing
@inox-tools/utilspackage forLazy.wrap.Verification
Summary by CodeRabbit
New Features
Tests
Chores