Display the author and commit title in the edit mode

[estib-vega]

Display the title of the commit and the author name in the commit instead of the placeholders

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
gitbutler-web	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 19, 2024 0:09am

[Caleb-T-Owens]

Hey, I'm really not keen on this approach as it means we're effectively introducing another structure that is playing the role of a Commit.

I was hoping to have a service which can return a struct RemoteCommit for any given commit, so we can look up the commit details from the client side via the Oid

[Caleb-T-Owens]

comment above

[Caleb-T-Owens]

Thanks for iterating on this so quickly! Especially on a day when we're hosting a conference.

Very much in a good direction. I just think that we could represent error cases vs missing commits a little bit better

[Caleb-T-Owens]

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)))
	}

[Caleb-T-Owens]

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);
	}
	}

[Caleb-T-Owens]

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

[Caleb-T-Owens]

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.

[Caleb-T-Owens]

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