feat(diff): commit-history diff viewer for wiki pages#123
Open
nmgarza5 wants to merge 24 commits into
Open
Conversation
Greptile SummaryThis PR adds a GitHub-style commit diff viewer to the wiki history panel, replacing the previous full-body re-render at old revisions with a structured
Confidence Score: 4/5Safe to merge after fixing the untyped fixture parameters, which will otherwise break the strict basedpyright CI check on push. The core diff parsing, endpoint logic, and frontend wiring are well-implemented and tested. The one concrete defect is in backend/tests/test_wiki_diff_api.py — fixture parameter annotations need to be added before CI will pass. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as page.tsx
participant Lib as wiki.ts (apiFetch)
participant API as GET /api/wiki/file/diff
participant Diff as wiki/diff.py
participant Git as wiki/git.py
UI->>UI: user clicks non-HEAD commit in History panel
UI->>Lib: fetchFileDiff(path, sha)
Lib->>API: "GET /api/wiki/file/diff?path=&sha="
API->>API: validate SHA regex + ACL check
API->>Diff: parse_commit_diff(sha, rel)
Diff->>Git: parent_sha(sha)
Git-->>Diff: parent SHA or None (root commit)
Diff->>Git: diff_for_commit(sha, rel)
Git-->>Diff: raw unified diff text
Diff->>Diff: _parse_unified() → hunks
Diff->>Diff: _promote_word_diff() per hunk
Diff-->>API: FileDiffResponse
API-->>Lib: JSON response
Lib-->>UI: FileDiffResponse
UI->>UI: setDiffData(r) → render DiffView
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
backend/tests/test_wiki_diff_api.py:14-16
**Missing type annotations on fixture parameters**
The `client` fixture's `tmp_db` and `tmp_repo` parameters lack type annotations and the function has no return type. basedpyright in strict mode raises `reportMissingParameterType` for unannotated parameters, so this will fail CI pre-commit checks. Compare with `doc_with_two_commits` in `test_wiki_diff.py`, which correctly annotates `tmp_repo: None`.
### Issue 2 of 3
backend/app/wiki/diff.py:10-14
`diff.py` claims to be "Pure helpers" with no direct I/O, but it imports `subprocess` to catch `CalledProcessError`. This couples a business-logic module to the subprocess layer directly, bypassing the git wrapper seam. The git module (or a shared exceptions module) should be the only place that surfaces this exception type to callers.
```suggestion
import re
import subprocess # only for CalledProcessError — wiki_git owns all git I/O
from app.models.file_system import DiffHunk, DiffLine, FileDiffResponse, WordDiff
from app.wiki import git as wiki_git
```
### Issue 3 of 3
frontend/src/components/wiki/DiffLineRow.module.css:53-66
Raw `border-radius: 2px` violates the design-token rule — radii must come from the centralized theme variables. `DiffHunk.module.css` already uses `var(--radius-xs)` consistently; these two classes should match.
```suggestion
.wordRemoved {
background: var(--color-state-danger-bg);
color: var(--color-state-danger-fg);
text-decoration: line-through;
padding: 0 2px;
border-radius: var(--radius-xs);
}
.wordAdded {
background: var(--color-state-success-bg);
color: var(--color-state-success-fg);
padding: 0 2px;
border-radius: var(--radius-xs);
}
```
Reviews (1): Last reviewed commit: "refactor(diff): clear diffData on save f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+14
to
+16
| @pytest.fixture | ||
| def client(tmp_db, tmp_repo): | ||
| return TestClient(create_app()) |
There was a problem hiding this comment.
The client fixture's tmp_db and tmp_repo parameters lack type annotations and the function has no return type. basedpyright in strict mode raises reportMissingParameterType for unannotated parameters, so this will fail CI pre-commit checks. Compare with doc_with_two_commits in test_wiki_diff.py, which correctly annotates tmp_repo: None.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/tests/test_wiki_diff_api.py
Line: 14-16
Comment:
**Missing type annotations on fixture parameters**
The `client` fixture's `tmp_db` and `tmp_repo` parameters lack type annotations and the function has no return type. basedpyright in strict mode raises `reportMissingParameterType` for unannotated parameters, so this will fail CI pre-commit checks. Compare with `doc_with_two_commits` in `test_wiki_diff.py`, which correctly annotates `tmp_repo: None`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+10
to
+14
| import re | ||
| import subprocess | ||
|
|
||
| from app.models.file_system import DiffHunk, DiffLine, FileDiffResponse, WordDiff | ||
| from app.wiki import git as wiki_git |
There was a problem hiding this comment.
diff.py claims to be "Pure helpers" with no direct I/O, but it imports subprocess to catch CalledProcessError. This couples a business-logic module to the subprocess layer directly, bypassing the git wrapper seam. The git module (or a shared exceptions module) should be the only place that surfaces this exception type to callers.
Suggested change
| import re | |
| import subprocess | |
| from app.models.file_system import DiffHunk, DiffLine, FileDiffResponse, WordDiff | |
| from app.wiki import git as wiki_git | |
| import re | |
| import subprocess # only for CalledProcessError — wiki_git owns all git I/O | |
| from app.models.file_system import DiffHunk, DiffLine, FileDiffResponse, WordDiff | |
| from app.wiki import git as wiki_git |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/app/wiki/diff.py
Line: 10-14
Comment:
`diff.py` claims to be "Pure helpers" with no direct I/O, but it imports `subprocess` to catch `CalledProcessError`. This couples a business-logic module to the subprocess layer directly, bypassing the git wrapper seam. The git module (or a shared exceptions module) should be the only place that surfaces this exception type to callers.
```suggestion
import re
import subprocess # only for CalledProcessError — wiki_git owns all git I/O
from app.models.file_system import DiffHunk, DiffLine, FileDiffResponse, WordDiff
from app.wiki import git as wiki_git
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+53
to
+66
| .wordRemoved { | ||
| background: var(--color-state-danger-bg); | ||
| color: var(--color-state-danger-fg); | ||
| text-decoration: line-through; | ||
| padding: 0 2px; | ||
| border-radius: 2px; | ||
| } | ||
|
|
||
| .wordAdded { | ||
| background: var(--color-state-success-bg); | ||
| color: var(--color-state-success-fg); | ||
| padding: 0 2px; | ||
| border-radius: 2px; | ||
| } |
There was a problem hiding this comment.
border-radius: 2px violates the design-token rule — radii must come from the centralized theme variables. DiffHunk.module.css already uses var(--radius-xs) consistently; these two classes should match.
Suggested change
| .wordRemoved { | |
| background: var(--color-state-danger-bg); | |
| color: var(--color-state-danger-fg); | |
| text-decoration: line-through; | |
| padding: 0 2px; | |
| border-radius: 2px; | |
| } | |
| .wordAdded { | |
| background: var(--color-state-success-bg); | |
| color: var(--color-state-success-fg); | |
| padding: 0 2px; | |
| border-radius: 2px; | |
| } | |
| .wordRemoved { | |
| background: var(--color-state-danger-bg); | |
| color: var(--color-state-danger-fg); | |
| text-decoration: line-through; | |
| padding: 0 2px; | |
| border-radius: var(--radius-xs); | |
| } | |
| .wordAdded { | |
| background: var(--color-state-success-bg); | |
| color: var(--color-state-success-fg); | |
| padding: 0 2px; | |
| border-radius: var(--radius-xs); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/wiki/DiffLineRow.module.css
Line: 53-66
Comment:
Raw `border-radius: 2px` violates the design-token rule — radii must come from the centralized theme variables. `DiffHunk.module.css` already uses `var(--radius-xs)` consistently; these two classes should match.
```suggestion
.wordRemoved {
background: var(--color-state-danger-bg);
color: var(--color-state-danger-fg);
text-decoration: line-through;
padding: 0 2px;
border-radius: var(--radius-xs);
}
.wordAdded {
background: var(--color-state-success-bg);
color: var(--color-state-success-fg);
padding: 0 2px;
border-radius: var(--radius-xs);
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
- backend: pass --unified=99_999 so every hunk includes the whole file as surrounding context lines; renderer paints the entire doc with +/- highlights instead of just hunk windows - frontend: drop the sha === headSha special case in onPickCommit so clicking the HEAD commit row in History shows the HEAD vs parent diff, same as any other commit; only the 'Latest (working tree)' row falls through to the body render - viewingOld now means 'a specific commit is selected', not 'an older one', so the warning banner + DiffView mount for HEAD commit too
GitHub-style: white-space: pre-wrap + overflow-wrap: anywhere on .content, align-items: start on the grid row so gutters stay anchored to the top when content wraps to multiple visual lines. min-width: 0 on the 1fr grid cell so the grid actually honors the wrap (without it, the cell stretches and overflow-wrap never kicks in).
- backend: introduce UnknownSha exception in wiki_git; diff_for_commit translates subprocess.CalledProcessError → UnknownSha so diff.py no longer imports subprocess directly (keeps the git seam pure) - tests: annotate client fixture parameters (tmp_db: None, tmp_repo: None) and -> TestClient return — matches doc_with_two_commits pattern in test_wiki_diff.py and basedpyright strict app-level rules - frontend: word-chip border-radius uses var(--radius-xs) instead of raw 2px, matching the design-token rule and DiffHunk's existing use
- backend: _promote_word_diff now walks the hunk and promotes every adjacent 1-remove/1-add edit block independently. Previously, with unified=99_999 producing one big hunk, two single-line edits in the same hunk would fail the 'exactly 1 remove + 1 add' check and both fall back to red/green block rendering. Now each pair gets the inline strikethrough/added word-diff treatment. - frontend: DiffLineRow renders the line content through ReactMarkdown with remark-gfm, paragraphs unwrapped to <> so each row stays inline. Headers, bold, italic, links render as wiki markdown does; +/- gutters and row backgrounds still apply. - new test pins two independent 1+1 edit blocks in one hunk both promote to kind=word.
Match the Figma design (Onyx-Wiki node 266-20322): the diff view now reads like the normal wiki page, with added blocks on a green background and removed blocks on a red background with strikethrough. No +/- gutter, no line numbers, no @@ hunk header. Implementation: DiffHunk groups consecutive same-kind lines into blocks and renders each block as one ReactMarkdown chunk, so headers + paragraphs + lists inside an add or remove block render with their real markdown semantics. Word-diff lines stay inline with the existing strikethrough + green chip treatment. DiffLineRow.tsx/.module.css are removed — DiffHunk now owns the whole row layout. The hunk-header strip is also dropped.
Each diff block now wraps its rendered ReactMarkdown in the global .markdown class so headers, paragraphs, lists, inline code, links get the same fonts, sizes, weights, and spacing as the normal wiki page preview. The colored band still wraps each block, but heading and paragraph margins are zeroed at the first/last child boundary so the band hugs the content instead of inheriting the wiki's heading/paragraph stack-spacing. Word lines also pick up .markdown typography by wrapping in a <p> so they read at paragraph body size, with inline <del>/<ins> chips still rendering the strikethrough/added word styling.
Heading, list, blockquote, code-fence, and HR lines were getting promoted to word-diff, which renders as plain text and exposes the literal ## / - / > syntax instead of running the line through ReactMarkdown. Now _promote_word_diff checks both sides of each 1+1 edit block against a markdown-block-prefix regex and falls back to block remove + block add for those lines, so the remove block renders as a struck-through heading and the add block renders as a green heading — same shape as the surrounding markdown preview. Tests pin the heading and list-item cases.
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
1 participant
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.
Description
When viewing a wiki page's History panel, picking a non-HEAD commit now renders a structured diff (commit vs parent) instead of re-rendering the full body at that revision.
+/-gutter, green/red row backgrounds viacolor.state.*tokens).Backend: new
app/wiki/diff.pyparses unified diff into typed hunks and promotes single-line hunks to word-diff. NewGET /api/wiki/file/diff?path=&sha=endpoint serves aFileDiffResponse. Unknown SHA → clean 404 (no 500). Newapp/wiki/git.py:parent_sha(sha)helper handles the root-commit case.Frontend: new
src/lib/wiki.tstyped lib + three components (DiffView→DiffHunk→DiffLineRow) undersrc/components/wiki/. Page swaps<ReactMarkdown>for<DiffView>whenviewingOld && diffData.Spec + plan land alongside the code at
Wiki Project/Specific Features/diff_commit_history/{design.md, implementation_plan.md}on the wiki. Blame view is tracked as a sub-bullet under the same TODO.How Has This Been Tested?