Extract ProjectMapper class from GitHubProjectDataSource

[ulrikandersen]

Summary

• Extract repository-to-project mapping logic into a dedicated ProjectMapper class
• Add generic type support for provider-specific repository types
• Support optional diff URL generation

[Copilot]

Pull request overview

This PR extracts the repository-to-project mapping logic from GitHubProjectDataSource into a dedicated, reusable ProjectMapper class. This refactoring introduces generic type support to enable the mapper to work with different Git providers, while moving provider-specific URL generation logic into configurable URL builders. The change improves code organization by separating concerns: the data source now focuses on repository retrieval, while the mapper handles the transformation logic.

Key Changes:

• Created a new ProjectMapper class with generic type support for provider-agnostic repository mapping
• Introduced URLBuilders interface for provider-specific URL generation with optional diff and PR URL support
• Simplified GitHubProjectDataSource to delegate mapping logic to ProjectMapper

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
src/features/projects/domain/ProjectMapper.ts	New file containing extracted mapping logic with generic types (RepositoryRef, RepositoryWithRefs, URLBuilders, and ProjectMapper class) and utility functions
src/features/projects/data/GitHubProjectDataSource.ts	Refactored to use ProjectMapper with GitHub-specific URL builders, removing ~260 lines of mapping logic

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The getBlobURL function is missing path encoding that was present in the original implementation. The old code properly encoded each path segment using path.split('/').map(segment => encodeURIComponent(segment)).join('/'), but the new implementation directly uses the unencoded path. This could cause issues with files containing special characters in their names (e.g., spaces, #, ?, etc.).

Consider updating the function to:

export function getBlobURL(owner: string, repository: string, path: string, ref: string): string {
  const encodedPath = path.split('/').map(segment => encodeURIComponent(segment)).join('/')
  return `/api/blob/${owner}/${repository}/${encodedPath}?ref=${ref}`
}

Suggested change

	return `/api/blob/${owner}/${repository}/${path}?ref=${ref}`
	const encodedPath = path.split('/').map(segment => encodeURIComponent(segment)).join('/')
	return `/api/blob/${owner}/${repository}/${encodedPath}?ref=${ref}`

[Copilot]

Using non-null assertion operator (!) on repository.defaultBranchRef.id is unsafe. The RepositoryWithRefs type defines id as optional (readonly id?: string), but the actual GitHubRepository type from the domain layer requires it to be non-optional (readonly id: string). This mismatch could cause runtime errors if the mapper is used with other repository types that don't guarantee an id.

Consider either:

1. Making the id required in RepositoryRef and RepositoryWithRefs types if it's always expected, or
2. Providing a fallback value or throwing a descriptive error when id is missing

Suggested change

	return repository.defaultBranchRef.id!
	if (!repository.defaultBranchRef.id) {
	throw new Error("Repository defaultBranchRef.id is missing");
	}
	return repository.defaultBranchRef.id;

[Copilot]

Using non-null assertion operator (!) on ref.id is unsafe. The RepositoryRef type defines id as optional (readonly id?: string), but the actual GitHubRepositoryRef type from the domain layer requires it to be non-optional (readonly id: string). This mismatch could cause runtime errors if the mapper is used with other repository types that don't guarantee an id.

Consider either:

1. Making the id required in RepositoryRef type if it's always expected for GitHub, or
2. Providing a fallback value or throwing a descriptive error when id is missing

Suggested change

	return ref.id!
	if (!ref.id) {
	throw new Error("RepositoryRef.id is required for getBlobRef but was undefined");
	}
	return ref.id;

[Copilot]

The condition !ref.id is a new addition not present in the original implementation. The old code only checked if (!baseRefOid) before returning undefined. While this additional check may be intentional for safety given the optional nature of ref.id in the new types, it changes the behavior from the original implementation and could cause diff URLs to not be generated in cases where they previously would have been. If this is intentional, it should be documented; otherwise, consider matching the original logic.

Suggested change

	if (!ref.baseRefOid || !ref.id) {
	if (!ref.baseRefOid) {

[Copilot]

[nitpick] The mapRepositoryToProject method is public, but the equivalent method in the original implementation (mapProject) was private. Consider marking this as private unless there's a specific need to expose it as part of the public API, to maintain better encapsulation.

Suggested change

	mapRepositoryToProject(repository: T): Project {
	private mapRepositoryToProject(repository: T): Project {