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.

Sign up for GitHub

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

Jump to bottom

Display the author and commit title in the edit mode #4948

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+66 −9
Open

Display the author and commit title in the edit mode #4948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4cf370a
Create a method to find a commit by its ID
estib-vega Sep 18, 2024
22097dc
Remote commit service
estib-vega Sep 19, 2024
bd74b26
Edit Mode page: Display commit info
estib-vega Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions apps/desktop/src/lib/commits/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { invoke } from '$lib/backend/ipc';
import { Commit } from '$lib/vbranches/types';
import { plainToInstance } from 'class-transformer';

export class RemoteCommitService {
async find(projectId: string, commitOid: string): Promise<Commit | undefined> {
try {
const commit = await invoke<unknown>('find_commit', { projectId, commitOid });
return plainToInstance(Commit, commit);
} catch (error) {
console.error(error);
return undefined;
}
}
}
Comment on lines +1 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

Something in my mind here is that we want to move away from using console.error and silently consuming errors. I figure here your intention was to represent a commit not being found, but I think with a little bit of jiggery pokery we can make it a little nicer.

Suggested change
import { invoke } from '$lib/backend/ipc';
import { Commit } from '$lib/vbranches/types';
import { plainToInstance } from 'class-transformer';
export class RemoteCommitService {
async find(projectId: string, commitOid: string): Promise<Commit | undefined> {
try {
const commit = await invoke<unknown>('find_commit', { projectId, commitOid });
return plainToInstance(Commit, commit);
} catch (error) {
console.error(error);
return undefined;
}
}
}
import { invoke } from '$lib/backend/ipc';
import { Commit } from '$lib/vbranches/types';
import { plainToInstance } from 'class-transformer';
import type { Project } from '$lib/backend/projects';
export class CommitService {
constructor(private project: Project) {}
async find(commitOid: string): Promise<Commit> {
const commit = await this.findMaybe(commitOid);
if (!commit) {
throw new Error(`Commit ${commitOid} not found`);
}
return commit;
}
async findMaybe(commitOid: string): Promise<Commit | undefined> {
const commit = await invoke<unknown | undefined>('find_commit', { projectId: this.project.id, commitOid });
if (!commit) return;
return plainToInstance(Commit, commit);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We also know that the name "RemoteCommitSerice" is inaccurate, could we please call it "CommitService"

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be keen to hear your thoughts on this, but in my mind it makes sense to try and pass as many dependencies in the constructor as possible. I would be keen to pass the project through the constructor until we had a good reason as to why that was a bad pattern, as it just adds more boilerplate to the consumers.

22 changes: 16 additions & 6 deletions apps/desktop/src/lib/components/EditMode.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
<script lang="ts">
// import { Project } from '$lib/backend/projects';
import { Project } from '$lib/backend/projects';
import { RemoteCommitService } from '$lib/commits/service';
import ActionView from '$lib/layout/ActionView.svelte';
import { ModeService, type EditModeMetadata } from '$lib/modes/service';
import { UncommitedFilesWatcher } from '$lib/uncommitedFiles/watcher';
import { getContext } from '$lib/utils/context';
import { Commit, type RemoteFile } from '$lib/vbranches/types';
import Button from '@gitbutler/ui/Button.svelte';
import InfoButton from '@gitbutler/ui/InfoButton.svelte';
import Avatar from '@gitbutler/ui/avatar/Avatar.svelte';
import FileListItem from '@gitbutler/ui/file/FileListItem.svelte';
import type { RemoteFile } from '$lib/vbranches/types';
import type { FileStatus } from '@gitbutler/ui/file/types';

interface Props {
Expand All @@ -17,7 +18,8 @@

const { editModeMetadata }: Props = $props();

// const project = getContext(Project);
const project = getContext(Project);
const remoteCommitService = getContext(RemoteCommitService);
const uncommitedFileWatcher = getContext(UncommitedFilesWatcher);
const modeService = getContext(ModeService);

Expand All @@ -27,13 +29,20 @@
let modeServiceSaving = $state<'inert' | 'loading' | 'completed'>('inert');

let initialFiles = $state<RemoteFile[]>([]);
let commit = $state<Commit | undefined>(undefined);

$effect(() => {
modeService.getInitialIndexState().then((files) => {
initialFiles = files;
});
});

$effect(() => {
remoteCommitService.find(project.id, editModeMetadata.commitOid).then((maybeCommit) => {
commit = maybeCommit;
});
});

Comment on lines +40 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we should always have a commit so I'd stick with find over findMaybe

interface FileEntry {
name: string;
path: string;
Expand Down Expand Up @@ -144,13 +153,14 @@

<div class="commit-data">
<div class="card commit-card">
<h3 class="text-13 text-semibold commit-card__title">Awesome title</h3>
<h3 class="text-13 text-semibold commit-card__title">
{commit?.descriptionTitle ?? 'unknown'}
</h3>
<div class="text-11 commit-card__details">
<span class="">{editModeMetadata.commitOid.slice(0, 7)}</span>
<span class="commit-card__divider">•</span>
<span class="">Author</span>
<span class="">{commit?.author.name ?? 'unknown'}</span>
</div>

<div class="commit-card__type-indicator"></div>
</div>

Expand Down
2 changes: 2 additions & 0 deletions apps/desktop/src/routes/+layout.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
IpcNameNormalizationService,
setNameNormalizationServiceContext
} from '$lib/branches/nameNormalizationService';
import { RemoteCommitService } from '$lib/commits/service';
import AppUpdater from '$lib/components/AppUpdater.svelte';
import PromptModal from '$lib/components/PromptModal.svelte';
import ShareIssueModal from '$lib/components/ShareIssueModal.svelte';
Expand Down Expand Up @@ -59,6 +60,7 @@
setContext(HttpClient, data.cloud);
setContext(User, data.userService.user);
setContext(RemotesService, data.remotesService);
setContext(RemoteCommitService, data.remoteCommitService);
setContext(AIPromptService, data.aiPromptService);
setContext(LineManagerFactory, data.lineManagerFactory);
setNameNormalizationServiceContext(new IpcNameNormalizationService(invoke));
Expand Down
3 changes: 3 additions & 0 deletions apps/desktop/src/routes/+layout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { ProjectService } from '$lib/backend/projects';
import { PromptService } from '$lib/backend/prompt';
import { Tauri } from '$lib/backend/tauri';
import { UpdaterService } from '$lib/backend/updater';
import { RemoteCommitService } from '$lib/commits/service';
import { RemotesService } from '$lib/remotes/service';
import { RustSecretService } from '$lib/secrets/secretsService';
import { UserService } from '$lib/stores/user';
Expand Down Expand Up @@ -41,6 +42,7 @@ export const load: LayoutLoad = async () => {
const secretsService = new RustSecretService(gitConfig);
const aiService = new AIService(gitConfig, secretsService, httpClient);
const remotesService = new RemotesService();
const remoteCommitService = new RemoteCommitService();
const aiPromptService = new AIPromptService();
const lineManagerFactory = new LineManagerFactory();

Expand All @@ -54,6 +56,7 @@ export const load: LayoutLoad = async () => {
gitConfig,
aiService,
remotesService,
remoteCommitService,
aiPromptService,
lineManagerFactory,
secretsService
Expand Down
7 changes: 6 additions & 1 deletion crates/gitbutler-branch-actions/src/actions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
branch_manager::BranchManagerExt,
file::RemoteBranchFile,
remote,
remote::{RemoteBranch, RemoteBranchData},
remote::{RemoteBranch, RemoteBranchData, RemoteCommit},
VirtualBranchesExt,
};
use anyhow::{Context, Result};
Expand Down Expand Up @@ -462,6 +462,11 @@ pub fn update_commit_message(
vbranch::update_commit_message(&ctx, branch_id, commit_oid, message).map_err(Into::into)
}

pub fn find_commit(project: &Project, commit_oid: git2::Oid) -> Result<RemoteCommit> {
let ctx = CommandContext::open(project)?;
remote::get_commit_data(&ctx, commit_oid)
}

pub fn fetch_from_remotes(project: &Project, askpass: Option<String>) -> Result<FetchResult> {
let ctx = CommandContext::open(project)?;

Expand Down
2 changes: 1 addition & 1 deletion crates/gitbutler-branch-actions/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ mod actions;
// This is our API
pub use actions::{
amend, can_apply_remote_branch, create_change_reference, create_commit, create_virtual_branch,
create_virtual_branch_from_branch, delete_local_branch, fetch_from_remotes,
create_virtual_branch_from_branch, delete_local_branch, fetch_from_remotes, find_commit,
get_base_branch_data, get_remote_branch_data, get_uncommited_files,
get_uncommited_files_reusable, insert_blank_commit, integrate_upstream,
integrate_upstream_commits, list_local_branches, list_remote_commit_files,
Expand Down
9 changes: 9 additions & 0 deletions crates/gitbutler-branch-actions/src/remote.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,15 @@ pub(crate) fn get_branch_data(ctx: &CommandContext, refname: &Refname) -> Result
.context("failed to get branch data")
}

pub(crate) fn get_commit_data(ctx: &CommandContext, sha: git2::Oid) -> Result<RemoteCommit> {
let commit = ctx
.repository()
.find_commit(sha)
.context("failed to find commit")?;

Ok(commit_to_remote_commit(&commit))
}
Comment on lines +113 to +120
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub(crate) fn get_commit_data(ctx: &CommandContext, sha: git2::Oid) -> Result<RemoteCommit> {
let commit = ctx
.repository()
.find_commit(sha)
.context("failed to find commit")?;
Ok(commit_to_remote_commit(&commit))
}
pub(crate) fn get_commit_data(
ctx: &CommandContext,
sha: git2::Oid,
) -> Result<Option<RemoteCommit>> {
let commit = match ctx.repository().find_commit(sha) {
Ok(commit) => commit,
Err(error) => {
if error.code() == git2::ErrorCode::NotFound {
return Ok(None);
} else {
bail!(error);
}
}
};
Ok(Some(commit_to_remote_commit(&commit)))
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whenever you see this pattern, that an err is conditionally turned into an OK or is kept as the original (or altered) error, there is .or_else(|err| …) for you.


pub(crate) fn branch_to_remote_branch(
branch: &git2::Branch<'_>,
remotes: &git2::string_array::StringArray,
Expand Down
1 change: 1 addition & 0 deletions crates/gitbutler-tauri/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ fn main() {
virtual_branches::commands::normalize_branch_name,
virtual_branches::commands::upstream_integration_statuses,
virtual_branches::commands::integrate_upstream,
virtual_branches::commands::find_commit,
secret::secret_get_global,
secret::secret_set_global,
undo::list_snapshots,
Expand Down
14 changes: 13 additions & 1 deletion crates/gitbutler-tauri/src/virtual_branches.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub mod commands {
use gitbutler_branch_actions::upstream_integration::{BranchStatuses, Resolution};
use gitbutler_branch_actions::{
BaseBranch, BranchListing, BranchListingDetails, BranchListingFilter, RemoteBranch,
RemoteBranchData, RemoteBranchFile, VirtualBranches,
RemoteBranchData, RemoteBranchFile, RemoteCommit, VirtualBranches,
};
use gitbutler_command_context::CommandContext;
use gitbutler_project as projects;
Expand Down Expand Up @@ -594,6 +594,18 @@ pub mod commands {
Ok(())
}

#[tauri::command(async)]
#[instrument(skip(projects), err(Debug))]
pub fn find_commit(
projects: State<'_, projects::Controller>,
project_id: ProjectId,
commit_oid: String,
) -> Result<RemoteCommit, Error> {
let project = projects.get(project_id)?;
let commit_oid = git2::Oid::from_str(&commit_oid).map_err(|e| anyhow!(e))?;
Ok(gitbutler_branch_actions::find_commit(&project, commit_oid)?)
}

#[tauri::command(async)]
#[instrument(skip(projects), err(Debug))]
pub fn upstream_integration_statuses(
Expand Down
Loading