Skip to content

History Graph Tree#152

Open
kingdo10 wants to merge 1 commit into
pol-rivero:mainfrom
kingdo10:history_graph_tree
Open

History Graph Tree#152
kingdo10 wants to merge 1 commit into
pol-rivero:mainfrom
kingdo10:history_graph_tree

Conversation

@kingdo10
Copy link
Copy Markdown

@kingdo10 kingdo10 commented May 5, 2026

Do you like the below feature?

I'm always using a similar feature in other clients. I think it integrates well.

Below is a comparison of how the same repo displays in this PR and an alternative git client.

historytree1 historytree2

@kingdo10 kingdo10 force-pushed the history_graph_tree branch 2 times, most recently from 0ed6055 to 02caa7f Compare May 5, 2026 13:37
@pol-rivero
Copy link
Copy Markdown
Owner

I'm usually not a fan of Git graphs because they get very overwhelming in any remotely complex project with several people creating and merging branches at the same time.

image

However, I do understand that some people like them and I don't like vetoing features, especially if you've put in the work to submit a PR. I'm open to merging this if you are willing to do the following:

1. Minimize the diff size of your PR (in existing files)

Since we depend on the upstream, we cannot differ too much or maintaining this fork will stop being sustainable. For this reason, large PRs should follow these rules:

  1. Creating new files is completely fine. You can add as many as you need and make them as large as needed.
  2. Modifications on functions, classes, files, etc. that are not present in the upstream (added by us in the past) are also completely fine.
  3. A few additions to existing files are okay, but don't add too many.
  4. Deletions and modifications on code from the upstream should be avoided at all costs, unless absolutely necessary.

Therefore, the following files are not okay and should be changed:

  • copilot-store.ts: This seems like an unnecessary change that can be reverted.
  • commit-message-avatar.tsx, id-pool.ts, rich-text.tsx: These changes shouldn't be necessary, at first glance these seem like workarounds for a deeper bug that should be fixed.
  • compare.tsx: There are way too many changes in this file. I would probably extract this into a new component (probably something like CommitGraphSidebar) and conditionally use it instead of CompareSidebar in renderHistorySidebar. That way you can also trim down your component by removing the props and logic that you don't need.
  • repository.tsx: Is removing the state really needed?

All other files are okay as they are.

2. Fix some bugs that I found while testing

  • When using the dark theme, the text on tag/branch pills is not readable.
  • The app should remember the chosen view and respect it globally.
    • Repro steps: When I select: "History" tab -> "List" (instead of Tree) -> "Changes" tab -> "History" tab, the list is back to being a tree.
    • When changing to another repo, the view is also reset to "Tree" instead of "List".
  • The avatar images should use the existing GitHub profile picture circle component instead of this new square with initials.

@kingdo10
Copy link
Copy Markdown
Author

kingdo10 commented May 5, 2026

Thanks for the insights. I will try to move it to new files, see how feasible it is
🖕

@guplem
Copy link
Copy Markdown

guplem commented May 8, 2026

Thanks for the PR @kingdo10, and thanks @pol-rivero for the detailed review.

Before discussing this specific change, I want to raise a more strategic question about the direction of GitHub Desktop Plus. I had a similar feeling with the worktrees addition, and I think it is worth being explicit about it now instead of revisiting it on every new PR.

What I really love about this fork is that it adds a small set of nice-to-have things that, for whatever reason, the upstream has not added. The strength of GitHub Desktop has always been simplicity: it covers the day-to-day Git workflow cleanly, and the things it does, it does well. If we keep adding bigger features (a full graph view, worktrees, and so on), at some point we are no longer "GitHub Desktop with a few fixes." We start to look like GitKraken or Sourcetree, and I do not think we should be competing in that space.

I see two possible directions, and I think we should pick one explicitly:

  1. Quality-of-life fork. Tight scope. Small UX improvements, performance, error handling, and fixes for things upstream refuses to address. Easier to maintain and to keep in sync.
  2. Full Git client. Add features freely, eventually covering what GitKraken does. More work, harder to maintain, and we are catching up to tools with a big head start.

I lean strongly toward option 1. I have used GitHub Desktop solely and professionally for years, and the base feature set already covers what I actually need from a Git client. I genuinely believe we already have 99.9% of what almost all users need. Every drift from upstream makes future merges harder, and that cost compounds over time.

To be clear, this is not a veto on this PR, and I appreciate the work @kingdo10 has put into it. I would actually integrate this feature since I really like it and think it is of high value for most users, especially for those getting initiated into Git. But I would treat it as probably the last big feature we take on. My concern is the precedent and the cumulative maintenance cost more than this specific feature.

If we do merge bigger features like this one (and the worktrees work), I would ask for hard guardrails:

  • Completely optional: no regressions or flow changes for users who do not enable it.
  • As detached as possible from the existing code: new files, new components, minimal touches to upstream code (which @pol-rivero is already pushing for, and I fully agree).

On the flip side, there are features I would actively push back on regardless of those guardrails. A clear example is drag-and-drop on the commit tree (dragging a commit onto another branch to cherry-pick or move it, dragging branches around, etc.). It is genuinely a nice-to-have, and I have used it in other clients, but in practice it adds a lot of UI complexity and is a constant source of subtle bugs. I would rather leave that kind of advanced workflow to dedicated Git clients.

Going forward, I would rather we focus on smaller quality-of-life items that have been widely requested upstream and never fixed. For example, multi-window support (desktop#3606) is not a "big feature," but it has been asked for for years and would make the app meaningfully nicer to use.

I would rather have this scope conversation now than slowly accumulate features and end up with something none of us signed up to maintain. Curious to hear what you both think.

@pol-rivero
Copy link
Copy Markdown
Owner

@guplem I agree completely

@kingdo10 kingdo10 force-pushed the history_graph_tree branch 2 times, most recently from 15cfb84 to d4fc5dc Compare May 12, 2026 11:57
@kingdo10
Copy link
Copy Markdown
Author

kingdo10 commented May 12, 2026

Hi,

I've refactored this as you suggested to keep the majority of it in separate files for the feature.
Also tried to reduce any original code edits / removals as much as possible
addressed the bugs you mentioned.

if you want to see what you think of this version

Also the behaviour of this fork doesn't change, the History List view is still default and the user would have to switch to this Tree view.

No doubt needs quite a bit of testing still.

image

@kingdo10 kingdo10 force-pushed the history_graph_tree branch from d4fc5dc to aeb7af1 Compare May 12, 2026 12:49
@pol-rivero
Copy link
Copy Markdown
Owner

pol-rivero commented May 13, 2026

Now this looks a lot better! (both visually and in the code implementation). Thank you for your hard work!
I agree that this will require some extensive testing. If I can find some free time, I'll also start using this from time to time in search for bugs.

For now here's what I found:

  • The tip (last commit) of the currently checked-out branch is missing the "Amend commit" and "Undo commit" context menu options. These options do show up correctly in the list view.

  • I don't know how I did it, but I somehow managed to stumble upon an infinite loop that completely blocked the UI thread and required restarting the app. I'm pretty sure it's an infinite loop because the CPU core was stuck at 100%, and it can't be infinite recursion because it didn't crash due to a stack overflow, it just froze. I'll update this comment if I manage to reproduce it reliably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants