Conversation
A List/Tree switch in the Changes heading. List keeps today's flat view and stays the default; Tree groups each status group's files into collapsible folders, with single-child folder chains compacted into one row (like VS Code's Source Control "View as Tree"). The choice is remembered in localStorage. Switching views or folding a folder repaints only the file list, leaving the selection and diff pane untouched, and keeps keyboard focus on the toggled folder.
|
Thanks so much for this, @vivi7! The graph itself looks really nice. It's clean, easy to read, and a real step up for seeing what's happening in a repo. It's clear a lot of care went into it, and the main-process side is well built too: every new IPC endpoint checks that the folder is attached, actions come from a fixed table, arguments are validated, and nothing runs through a shell. We can't merge it quite as it stands, though. Here's what we found. 1. Security: repo content can inject HTML into the renderer. The renderer can run git actions through
2. Scope. About 13k lines, with 35 actions that change the repo, a settings system, avatar fetching and a trust model, is a lot for us to take on and maintain. Please split it the way you offered: a read-only graph first (layout, rendering, commit details, and diffs in the existing viewer), without the settings drawer, avatars or the committed config file. Mutating actions can follow in a separate PR. 3. Bugs
4. Visual polish. The settings tabs, the icons, and the horizontal scrollbar that's always showing need some work to match the rest of the app. 5. Overlap with #98. #98 adds a list/tree view to the Git tab with its own tree builder ( Ali is also working on a plugin system, and Git Graph would fit really well there. We'll share more once it's ready. — Claude, on behalf of Ali |
… visible - Tree is now the default view; a stored 'list' choice is kept. - Tree rows show the tree node's own name, so a file name containing a backslash isn't cut at it. - Staged / Unstaged / Staged + unstaged shows on each tree row instead of only in the tooltip. - Collapsing the folder that holds the selected file highlights that folder row.
Log with parents, refs, stashes and file contents at a revision, all read-only and run through execFile with an argv array.
Builds the graph payload (paging, stashes, uncommitted changes), commit details, comparisons and file contents for diffs, and watches the repository for changes. Every endpoint goes through projects.js and only accepts folders attached to the project; nothing here changes a repository.
A pure function that assigns commits to lanes and computes edges for merges, octopus merges, multiple roots, stashes and the uncommitted row.
A tab right after Git with the commit graph, ref labels, branch and tag filters, find, commit details and comparisons. Files in a commit use the Git tab's tree and open in the existing diff viewer, read-only. Context menus only copy or view. Values from the repository are escaped for their context (escapeAttr for attributes), shortcuts only act while the tab is visible and avoid the app's own, and loading more commits keeps the scroll position.
88d0e86 to
bb8544a
Compare
|
Thanks for the thorough review! I've reworked this PR as a read-only graph on top of #98, reusing its tree for the commit-details file list. Settings, avatars, the committed config and every mutating action are gone. The security, bug and polish points are addressed as listed in the updated description, with tests using the real escape functions. The actions are ready for a follow-up PR, or a plugin once that lands. Happy to adjust anything. |
Summary
Adds a read-only Git Graph tab to projects, right after Git. It shows the commit graph of the attached repositories with ref labels, commit details and diffs.
Following your review, this PR is now read-only. Nothing in it changes a repository. The settings drawer, avatars, the committed config file and every mutating action are gone. The actions can follow in a separate PR, or as a plugin once that system is ready.
What's included
Review points addressed
Security
escapeAttr, includingdata-gg-file-path. Every text value goes throughescapeHtml.escapeHtml/escapeAttrfromutils.js. They push",',<,>and&through file paths, branch, remote and tag names, stash messages and commit subjects, and check the markup can't be broken out of.Bugs
body.isConnected.alert/confirmare gone along with the actions.Polish
.pane-segcontrol.Overlap with #98:
gitGraphBuildFileTree/gitGraphCompactFoldersare removed in favour of #98's tree.Changes to existing files
All additive:
git.js: read helpers only.projects.js,main.js,preload.js: five read endpoints and a change event, each checking the folder is attached.tagicon.escapeAttrinutils.js.readOnlyoption forcreateUnifiedMergeViewer.Size: about 5k lines, of which ~3.2k are code and ~1.9k tests.
Testing
npm test: 523/523. The new tests cover the layout, the git read helpers against real temporary repositories, the attached-folder check on every endpoint, escaping with the real functions, the scroll and shortcut scoping, and watcher re-binding.a"b<i>c&.txt, branchfeat/a&b, tagv1'x, a stash message<b>"q"</b>and a subject<img src=x onerror=alert(1)>". I checked that: