Fix stale diff detection in softRefresh using raw mtime/byteSize#88
Merged
Fix stale diff detection in softRefresh using raw mtime/byteSize#88
Conversation
In `1commit+WC` mode (rangeToWorking), `softRefresh` could decide nothing changed and call `skipRender()` even after an actual WC edit. The page never morphed, the diff stayed stale, and the toast read "Up to date" — exactly the symptom users hit when iterating quickly on a recent commit. Root cause: `fileFingerprints` keyed off `lastModified` (a `diffForHumans` short-form string) and `fileSize` (`Number::fileSize` output). Both bucket aggressively. Combined with `additions/deletions` that don't budge for in-place edits to lines already modified by the pinned commit, two rapid refreshes around an edit could produce identical fingerprints. Fix: add raw `mtime` and `byteSize` fields to `FileListEntry` (populated alongside the existing human-readable variants in working-tree mode) and switch `fileFingerprints` to those. Display paths still use the formatted strings — the new fields are fingerprint-only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> https://claude.ai/code/session_019WzLy3s8FXh2gCXy5WoFQr
Two follow-ups to the 1commit+WC stale-diff fix:
1. Structural: drop the `skipRender()` shortcut from `softRefresh`. The
render is what causes children to rehydrate and pick up the
already-cleared diff cache; gating it on a fingerprint heuristic
was the root cause of the original bug class. softRefresh is
user-initiated (⌘R / click / head-advance), so an idempotent
render is bounded; a silently stale UI is not. The fingerprint
computation stays — it drives the post-refresh toast count.
2. Test coverage:
- On-disk producer tests in `GitDiffServiceTest`: real git repo,
real `getFileList`, asserting `mtime`/`byteSize` track the
filesystem in plain WT, untracked, and `rangeToWorking`
(1commit+WC / Since-base) modes, and stay null for immutable
commit-to-commit targets.
- Updated `ReviewPageSoftRefreshTest` to assert "always renders"
and reframed the in-place-edit test as a toast-accuracy
regression now that render isn't gated.
- Arch test in `BladeConventionsTest`: pins
`fileFingerprints` to `mtime`/`byteSize` and bans
`lastModified`/`fileSize` keys, so a future revert fires a
fixture-level signal before users hit it.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
https://claude.ai/code/session_019WzLy3s8FXh2gCXy5WoFQr
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR enriches file metadata collection by adding raw ChangesFile Change Detection via Raw Metadata
Sequence DiagramsequenceDiagram
participant Git as Git Repository
participant Service as GitDiffService
participant DTO as FileListEntry
participant Page as Review Page
Git->>Service: File metadata<br/>(mtime, size)
Note over Service: Extract raw mtime<br/>via getRawMtime()
Note over Service: Get byteSize<br/>via getRawByteSize()
Service->>DTO: Create entry with<br/>mtime & byteSize
DTO->>Page: toArray() includes<br/>raw fields
Page->>Page: softRefresh()<br/>always renders
Note over Page: fileFingerprints uses<br/>mtime & byteSize
Page->>Page: Detect changes<br/>via raw values
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
ScreenshotsUpdated for
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/Arch/BladeConventionsTest.php (2)
122-136: 💤 Low valueChain those
expects on the same subject.Per the repo's testing guideline ("Chain multiple expectations on the same subject when readable") the four
expect($body)statements collapse nicely into a single chain. As per coding guidelines: "Chain multiple expectations on the same subject when readable".♻️ Proposed tidy-up
- expect($body)->toContain("'mtime'"); - expect($body)->toContain("'byteSize'"); - expect($body)->not->toContain("'lastModified'"); - expect($body)->not->toContain("'fileSize'"); + expect($body) + ->toContain("'mtime'") + ->toContain("'byteSize'") + ->not->toContain("'lastModified'") + ->not->toContain("'fileSize'");🤖 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 `@tests/Arch/BladeConventionsTest.php` around lines 122 - 136, Collapse the four separate assertions on $body into a single chained expectation to follow the testing guideline: replace the four expect($body)->... lines with one expect($body)->toContain('mtime')->toContain('byteSize')->not->toContain('lastModified')->not->toContain('fileSize'), targeting the test function that extracts the fileFingerprints method body (variable $body) so the behavior is unchanged but the assertions are chained on the same subject.
126-128: 💤 Low valueRegex anchor on
^ \}is a touch brittle.The closing-brace match relies on exactly four spaces of indentation. If the method ever moves nesting depth or someone reformats the file with tabs, the regex silently stops matching and
test()->fail()fires — which is at least loud, but it's a maintenance trip-hazard for a test that should outlive several refactors. A non-greedy match up to the next siblingprivate/publicdeclaration, or a brace-counting loop, would be sturdier. Optional, since the failure mode is loud rather than silent.🤖 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 `@tests/Arch/BladeConventionsTest.php` around lines 126 - 128, The regex that extracts the body of private function fileFingerprints currently anchors the closing brace to "^ \}" which breaks if indentation changes; update the match logic in BladeConventionsTest (the code that calls preg_match for fileFingerprints) to stop relying on exact indentation — either replace the trailing anchor with a non-greedy pattern that stops at the next sibling method declaration (e.g. lookahead for "\n\s*(public|protected|private)\s+function") or implement a simple brace-counting loop that locates the method body by scanning from the opening "{" and matching braces until the corresponding "}" is found; ensure the change targets the preg_match usage that references fileFingerprints so the test becomes robust to indentation/reformatting.app/Services/GitDiffService.php (2)
406-418: 💤 Low valueTwo near-identical helpers — fine, but ripe for a tiny merge.
getRawMtimeandgetRawByteSizeeach redo theFile::isFile+ repo-relative path build. Given they're always called together for the same path, you could collapse them into a singlegetRawMetadata(): array{mtime: ?int, byteSize: ?int}(or just return[?int, ?int]) and halve the stat-cache traffic plus the repetition. Totally optional given how cheap stat is, just an aesthetic nit.♻️ Sketch
- private function getRawMtime(string $repoPath, string $path): ?int - { - $fullPath = $repoPath.'/'.$path; - - return File::isFile($fullPath) ? File::lastModified($fullPath) : null; - } - - private function getRawByteSize(string $repoPath, string $path): ?int - { - $fullPath = $repoPath.'/'.$path; - - return File::isFile($fullPath) ? File::size($fullPath) : null; - } + /** `@return` array{mtime: ?int, byteSize: ?int} */ + private function getRawMetadata(string $repoPath, string $path): array + { + $fullPath = $repoPath.'/'.$path; + + return File::isFile($fullPath) + ? ['mtime' => File::lastModified($fullPath), 'byteSize' => File::size($fullPath)] + : ['mtime' => null, 'byteSize' => null]; + }(Call-site adjustment left as an exercise — would unfortunately need an intermediate variable per construction.)
🤖 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/Services/GitDiffService.php` around lines 406 - 418, Merge the two helpers getRawMtime and getRawByteSize into a single method (e.g., getRawMetadata) that builds the repo-relative $fullPath once, checks File::isFile($fullPath) a single time, and returns both values (mtime via File::lastModified and byteSize via File::size) as an array or associative array like ['mtime'=>?int,'byteSize'=>?int]; then update all call sites that previously invoked getRawMtime(...) and getRawByteSize(...) to call getRawMetadata(...) and unpack the returned mtime/byteSize into the existing variables.
165-197: 💤 Low valueUntracked text branch still omits
fileSizewhile the binary branch sets it.Pre-existing oddity, not introduced here, but now that
byteSizeis populated in both branches the asymmetry stands out: untracked binary entries get human-readablefileSize(line 175) while untracked text entries don't (line 186-197). The sidebar tooltip currently only consumeslastModified, so it's harmless today, but it's a footgun for the next person who relies onfileSizebeing uniformly populated for working-tree entries. Worth a one-liner fix while you're in here, or leave it alone — your call.🤖 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/Services/GitDiffService.php` around lines 165 - 197, The untracked text branch (inside GitDiffService where FileListEntry is constructed for non-binary files) omits the human-readable fileSize while the binary branch sets it; update the non-binary FileListEntry creation (the block after the isBinary check) to include fileSize: $this->getHumanFileSize($repoPath, $file) so both branches populate fileSize consistently, keeping the other fields (byteSize, mtime, lastModified) unchanged.tests/Unit/GitDiffServiceTest.php (1)
219-236: 💤 Low valueThe double
touchdance reads cryptically.Sequence is
File::put("edit-1")→ read first →touch(+5)→File::put("edit-2")(which bumps mtime to now) →touch(+5)again (resetting it back). The twotouchcalls are bracketing aFile::putwhose mtime side-effect is then deliberately undone. It works, but the intent ("pin the second snapshot's mtime to first+5 regardless of wall-clock") would land cleaner if you reorder write-then-touch:♻️ Tighter version
- File::put($this->tmpDir.'/hello.txt', "edit-1\n"); - clearstatcache(true, $this->tmpDir.'/hello.txt'); - $first = $this->service->getFileList($this->tmpDir)[0]; - - // Bump mtime explicitly so the test isn't a same-second flake. - touch($this->tmpDir.'/hello.txt', $first->mtime + 5); - File::put($this->tmpDir.'/hello.txt', "edit-2\n"); - touch($this->tmpDir.'/hello.txt', $first->mtime + 5); - clearstatcache(true, $this->tmpDir.'/hello.txt'); - $second = $this->service->getFileList($this->tmpDir)[0]; + File::put($this->tmpDir.'/hello.txt', "edit-1\n"); + clearstatcache(true, $this->tmpDir.'/hello.txt'); + $first = $this->service->getFileList($this->tmpDir)[0]; + + // Write then pin mtime so the assertion isn't a same-second flake. + File::put($this->tmpDir.'/hello.txt', "edit-2\n"); + touch($this->tmpDir.'/hello.txt', $first->mtime + 5); + clearstatcache(true, $this->tmpDir.'/hello.txt'); + $second = $this->service->getFileList($this->tmpDir)[0];🤖 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 `@tests/Unit/GitDiffServiceTest.php` around lines 219 - 236, The mtime bump uses two touch calls around File::put which is confusing and undoes the write's mtime; instead, after capturing $first via $this->service->getFileList(...) keep File::put($this->tmpDir.'/hello.txt', "edit-2\n") and then call touch($this->tmpDir.'/hello.txt', $first->mtime + 5) once, followed by clearstatcache and reading $second with $this->service->getFileList(...); update the sequence around File::put, touch, clearstatcache, and the $first/$second reads so the second snapshot's mtime is deterministically first->mtime+5 without the redundant initial touch.
🤖 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/Services/GitDiffService.php`:
- Around line 406-418: Merge the two helpers getRawMtime and getRawByteSize into
a single method (e.g., getRawMetadata) that builds the repo-relative $fullPath
once, checks File::isFile($fullPath) a single time, and returns both values
(mtime via File::lastModified and byteSize via File::size) as an array or
associative array like ['mtime'=>?int,'byteSize'=>?int]; then update all call
sites that previously invoked getRawMtime(...) and getRawByteSize(...) to call
getRawMetadata(...) and unpack the returned mtime/byteSize into the existing
variables.
- Around line 165-197: The untracked text branch (inside GitDiffService where
FileListEntry is constructed for non-binary files) omits the human-readable
fileSize while the binary branch sets it; update the non-binary FileListEntry
creation (the block after the isBinary check) to include fileSize:
$this->getHumanFileSize($repoPath, $file) so both branches populate fileSize
consistently, keeping the other fields (byteSize, mtime, lastModified)
unchanged.
In `@tests/Arch/BladeConventionsTest.php`:
- Around line 122-136: Collapse the four separate assertions on $body into a
single chained expectation to follow the testing guideline: replace the four
expect($body)->... lines with one
expect($body)->toContain('mtime')->toContain('byteSize')->not->toContain('lastModified')->not->toContain('fileSize'),
targeting the test function that extracts the fileFingerprints method body
(variable $body) so the behavior is unchanged but the assertions are chained on
the same subject.
- Around line 126-128: The regex that extracts the body of private function
fileFingerprints currently anchors the closing brace to "^ \}" which breaks
if indentation changes; update the match logic in BladeConventionsTest (the code
that calls preg_match for fileFingerprints) to stop relying on exact indentation
— either replace the trailing anchor with a non-greedy pattern that stops at the
next sibling method declaration (e.g. lookahead for
"\n\s*(public|protected|private)\s+function") or implement a simple
brace-counting loop that locates the method body by scanning from the opening
"{" and matching braces until the corresponding "}" is found; ensure the change
targets the preg_match usage that references fileFingerprints so the test
becomes robust to indentation/reformatting.
In `@tests/Unit/GitDiffServiceTest.php`:
- Around line 219-236: The mtime bump uses two touch calls around File::put
which is confusing and undoes the write's mtime; instead, after capturing $first
via $this->service->getFileList(...) keep File::put($this->tmpDir.'/hello.txt',
"edit-2\n") and then call touch($this->tmpDir.'/hello.txt', $first->mtime + 5)
once, followed by clearstatcache and reading $second with
$this->service->getFileList(...); update the sequence around File::put, touch,
clearstatcache, and the $first/$second reads so the second snapshot's mtime is
deterministically first->mtime+5 without the redundant initial touch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e87da918-fa2f-424b-8af0-76c53d65f874
📒 Files selected for processing (8)
app/DTOs/FileListEntry.phpapp/Services/ExternalFilesService.phpapp/Services/GitDiffService.phpresources/views/pages/⚡review-page.blade.phptests/Arch/BladeConventionsTest.phptests/Unit/FileListEntryTest.phptests/Unit/GitDiffServiceTest.phptests/Unit/Livewire/ReviewPageSoftRefreshTest.php
- `tests/Arch/BladeConventionsTest`: chain the four `expect($body)` assertions per the repo's "chain expectations on the same subject" guideline. - `tests/Unit/GitDiffServiceTest`: drop the redundant pre-write touch in the mtime-advances test. The intent was always "write, then pin mtime so the assertion isn't a same-second flake" — write-then-touch reads cleaner than touch-write-touch. Skipped the other three nitpicks: regex-anchor brittleness (loud failure mode, CodeRabbit marked optional); merging the two raw-stat helpers (purely aesthetic, swaps clarity for terseness at call sites); adding `fileSize` to the untracked-text branch (pre-existing inconsistency, out of scope). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> https://claude.ai/code/session_019WzLy3s8FXh2gCXy5WoFQr
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
2 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.
Summary
Fixes a bug where the review page's softRefresh would fail to detect in-place file edits in "1commit+WC" and "Since-base" diff modes, causing the toast to incorrectly show "Up to date" while the diff had actually changed. The root cause was relying on formatted
lastModifiedandfileSizestrings for change detection, which bucket too aggressively to catch rapid edits.Key Changes
Added raw mtime and byteSize fields to FileListEntry: New
mtime(Unix timestamp) andbyteSize(byte count) fields capture the raw filesystem state, separate from the human-readable formatted strings.Updated GitDiffService.getFileList(): Now populates
mtimeandbyteSizefor all working-directory targets (plain WT, 1commit+WC, Since-base modes) via new helper methodsgetRawMtime()andgetRawByteSize().Updated ExternalFilesService: Also populates the new raw fields for external file entries.
Fixed softRefresh fingerprinting: Changed
fileFingerprints()to use rawmtimeandbyteSizeinstead of formattedlastModifiedandfileSizestrings. This catches in-place edits where line counts and human-readable timestamps haven't changed.Removed conditional skipRender(): softRefresh now always renders, even when
changedCount === 0. The previous heuristic-based skip was the root cause of the stale-diff bug—a false-negative fingerprint would latch the response into a no-op morph, preventing child components from rehydrating. User-initiated refreshes (⌘R, click, head-advance) have acceptable morph cost; silently stale UI does not.Added comprehensive test coverage: New tests in GitDiffServiceTest verify mtime/byteSize population across all scenarios (tracked, untracked, rangeToWorking, commit-to-commit). New tests in ReviewPageSoftRefreshTest verify the toast accuracy fix and that renders always occur.
Added architectural test: BladeConventionsTest now pins the design intent that fileFingerprints must use raw fields, preventing future regressions.
Implementation Details
[id, status, additions, deletions, mtime, byteSize]instead of including formatted strings.mtimeandbyteSizefields in mock file lists.https://claude.ai/code/session_019WzLy3s8FXh2gCXy5WoFQr
Summary by CodeRabbit
New Features
Tests