Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement Stack selected commit right half of workspace page #7340

Merged
merged 19 commits into from
Feb 21, 2025

Conversation

ndom91
Copy link
Contributor

@ndom91 ndom91 commented Feb 19, 2025

🧢 Changes

  • Add right-half StackDetails view when selecting a commit, including:
    • FileList
    • Commit header metadata
    • Show commit title + description as two separate styled text entities out of the 1 commit.message field
  • Add required redux queries and mutations for getting commit changes and updating commit message
  • Fix StackTabs dynamic border-radius changes
  • Modify UnifiedDiffView to take a whole TreeChange prop in order to more easily show both worktree changes and committed changes via that 1 component

image

☕️ Reasoning

Copy link

vercel bot commented Feb 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gitbutler-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 21, 2025 3:19pm
gitbutler-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 21, 2025 3:19pm

@ndom91 ndom91 force-pushed the feat-selected-commit-details branch from f584fc9 to 08320fa Compare February 19, 2025 15:51
@vercel vercel bot temporarily deployed to Preview – gitbutler-web February 19, 2025 15:51 Inactive
@ndom91 ndom91 force-pushed the feat-selected-commit-details branch from 08320fa to 63238f1 Compare February 19, 2025 15:54
@vercel vercel bot temporarily deployed to Preview – gitbutler-web February 19, 2025 15:54 Inactive
@ndom91 ndom91 force-pushed the feat-selected-commit-details branch from 63238f1 to e2bea97 Compare February 20, 2025 09:22
@vercel vercel bot temporarily deployed to Preview – gitbutler-web February 20, 2025 09:22 Inactive
@ndom91 ndom91 force-pushed the feat-selected-commit-details branch from e2bea97 to 349c531 Compare February 20, 2025 09:22
@vercel vercel bot temporarily deployed to Preview – gitbutler-web February 20, 2025 09:22 Inactive
@ndom91 ndom91 force-pushed the feat-selected-commit-details branch from 349c531 to e436608 Compare February 20, 2025 13:53
@vercel vercel bot temporarily deployed to Preview – gitbutler-web February 20, 2025 13:53 Inactive
@ndom91 ndom91 force-pushed the feat-selected-commit-details branch from e436608 to 61cb743 Compare February 20, 2025 14:29
@vercel vercel bot temporarily deployed to Preview – gitbutler-web February 20, 2025 14:29 Inactive
@vercel vercel bot temporarily deployed to Preview – gitbutler-web February 20, 2025 14:35 Inactive
}}
/>
{/each}
{#if diff.subject.hunks.length}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can drop this if statement? Iterating of a zero length array is a no-op.

Copy link
Contributor Author

@ndom91 ndom91 Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah logically you're probably right. I had it here in order to be able ot render at least something (in this case the string "No Content"), when there are no hunks.

Ideally I'd like to just not allow the user to toggle them open when there are no hunks, however, due to the way we have it architected now, we don't know the contents of the file change, i.e. the diff, until when the user opens it.

So its not straight forward to disable toggling open of a file which doesn't have a hunk (i.e. new empty file Addition) without checking them all beforehand, which is what we wanted to avoid, right. You have any good ideas? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

@krlvi I think in v2 code we generate a synthetic hunk when there is no diff. It's easier for the front end if we keep that behaviour, but what do you think?

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely we could add a synthetic hunk, but on face value this feels like masking another problem. It's totally valid to have no hunks in the file (eg. file executable bit added/removed) - and the app could/should show that.
At the moment I feel like I'd rather favor correctness (of the API) over supporting the existing semantic.

Is there something that I might be missing here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I guess the same applies for commits with no files in them - the app allows their creation, and it should be still possible to preview / edit the message

Copy link
Contributor Author

@ndom91 ndom91 Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krlvi Yeah +1

I was just reading through the types again and it seems like we could disable the opening of these types of file with those flags too. Would it be a lot of work on your end to add a EmptyFile TreeChange.status Flag?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndom91 my intuition is that we should still allow the file to be opened, but show a message where we otherwise have a diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay yeah I'm open to whatever. Maybe Pavel has a nice design idea haha

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, but we will at some point. For now we can just drop some text in its place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Currently there is some text there and it just says "No Content" i.e.:

image

@@ -1,27 +1,32 @@
<script lang="ts">
import UnifiedDiffView from './v3/UnifiedDiffView.svelte';
import ReduxResult from '$components/ReduxResult.svelte';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtsgrd you wanted to potentially get rid of this file btw, is that still the case?

I had to fix one or two TS-related things here in this file in order to get the project to build and then just kept going a bit further haha

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can drop SelectionView.svelte!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Dropped!

@ndom91 ndom91 enabled auto-merge (rebase) February 21, 2025 15:20
@ndom91 ndom91 merged commit c23f45c into master Feb 21, 2025
19 checks passed
@ndom91 ndom91 deleted the feat-selected-commit-details branch February 21, 2025 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants