Display name improvements #259
Conversation
WalkthroughThis pull request introduces a new Visual Studio Code workspace configuration and updates various parts of the backend and frontend. In the backend, git operations have been refactored to separate cloning from configuration updates and now employ Zod for metadata validation. Database migrations and schema definitions have been updated to include a new display name field for repositories, and several UI components and utility functions were adjusted to utilize this new field. Changes
Sequence Diagram(s)sequenceDiagram
participant RM as Repo Manager
participant CR as Clone Repository
participant UG as Upsert Git Config
RM->>CR: cloneRepository(cloneURL, path, onProgress)
Note right of CR: Clones repository without gitConfig
RM->>UG: If metadata.gitConfig exists, call upsertGitConfig(path, gitConfig, onProgress)
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
brendan-kellam
force-pushed
the
bkellam/display-name-improvements
branch
from
e95d7bd to
5db4d26
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/backend/src/repoCompileUtils.ts (1)
281-281: Handle potential null webUrl value.The webUrl value could be null based on the conditional assignment in line 243, but it's used here without a null check which could result in 'zoekt.web-url' being set to an empty string.
- 'zoekt.web-url': webUrl ?? '', + 'zoekt.web-url': webUrl ?? hostUrl,This ensures a reasonable fallback value if webUrl is null.
packages/web/src/lib/utils.ts (1)
54-56: Improved display name handling with fallback.The code now uses
repo.RawConfig['display-name']with a fallback torepo.RawConfig['name'], ensuring a display name is always available ifrepo.RawConfigexists. The TODO comment about using Zod for validation is a good reminder for future work.Consider tracking the TODO comment about using Zod for config schema validation as a separate task to ensure it's not forgotten.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.vscode/sourcebot.code-workspace(1 hunks)CHANGELOG.md(1 hunks)packages/backend/src/git.ts(2 hunks)packages/backend/src/repoCompileUtils.ts(12 hunks)packages/backend/src/repoManager.ts(4 hunks)packages/backend/src/types.ts(2 hunks)packages/backend/src/zoekt.ts(3 hunks)packages/db/prisma/migrations/20250402232401_add_display_name_to_repo/migration.sql(1 hunks)packages/db/prisma/schema.prisma(1 hunks)packages/web/src/actions.ts(1 hunks)packages/web/src/app/[domain]/components/repositoryCarousel.tsx(1 hunks)packages/web/src/app/[domain]/repos/repositoryTable.tsx(1 hunks)packages/web/src/lib/schemas.ts(1 hunks)packages/web/src/lib/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
packages/web/src/lib/utils.ts (1)
packages/web/src/lib/types.ts (1)
RepositoryQuery(21-21)
packages/backend/src/repoManager.ts (2)
packages/backend/src/types.ts (1)
repoMetadataSchema(26-43)packages/backend/src/git.ts (2)
cloneRepository(3-22)upsertGitConfig(51-63)
packages/backend/src/zoekt.ts (1)
packages/backend/src/types.ts (1)
repoMetadataSchema(26-43)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (34)
packages/db/prisma/migrations/20250402232401_add_display_name_to_repo/migration.sql (1)
1-2: Proper implementation for the display name featureThe migration correctly adds a nullable
displayNameTEXT column to theRepotable. This aligns with the PR objective to introduce an optional display name field for repositories.packages/web/src/lib/schemas.ts (1)
171-171: Good schema update for the new display name fieldAdding the optional
repoDisplayNamefield to therepositoryQuerySchemais consistent with the database migration and properly marked as optional withz.string().optional().CHANGELOG.md (1)
12-12: Well-documented changelog entryGood job documenting the fix for repository display name formatting issues, with specific mention of Gerrit repositories and a proper link to this PR.
.vscode/sourcebot.code-workspace (1)
1-16: Nice developer experience improvementThis VS Code workspace configuration is well-structured with:
- Two folder paths correctly defined (root and zoekt vendor directory)
- Appropriate file associations for JSON files
This will help maintain consistent development environments across the team.
packages/web/src/app/[domain]/components/repositoryCarousel.tsx (1)
76-76: Simplification of display name logicThe code now directly uses
repo.repoNamefor the display name instead of potentially using a shortened version. This change aligns with the PR objective of improving repository display names.packages/web/src/app/[domain]/repos/repositoryTable.tsx (1)
42-42: Improved repository name display logicThe code now prioritizes the new
repoDisplayNamefield when available, falling back torepoNamewhen it's not. This is a more flexible approach that properly leverages the new display name field introduced in this PR.packages/web/src/actions.ts (1)
432-432: Added repository display name to return objectThis change exposes the optional
displayNamefield from the repository model to the client through therepoDisplayNameproperty, enabling the UI components to utilize this new field. This is a necessary adaptation to make the display name improvements work end-to-end.packages/db/prisma/schema.prisma (2)
41-41: Added optional displayName field to Repo modelThis is the database schema change that implements the core functionality described in the PR objectives. Making the field optional ensures backward compatibility with existing repositories that don't have a display name set.
47-47: Improved metadata field documentationThe added comment clarifies where the schema definition for the metadata field can be found, which improves code maintainability. This is a good practice that helps developers understand the structure of the data stored in this JSON field.
packages/backend/src/types.ts (4)
2-2: Good addition of Zod for schema validation.Adding Zod for schema validation is an excellent approach to ensure runtime type safety and validation for the repository metadata.
20-25: Good documentation with important warnings.The comments clearly indicate the importance of backward compatibility when modifying the schema, which is crucial for database schemas. The reference to the schema.prisma comment is also helpful for maintainability.
26-43: Well-structured schema definition.The schema properly defines the repository metadata structure with appropriate validation rules. Each field has good documentation explaining its purpose, and the use of optional fields maintains flexibility.
45-45: Good type inference approach.Using
z.inferto derive the TypeScript type from the Zod schema ensures consistency between runtime validation and compile-time type checking.packages/backend/src/zoekt.ts (3)
2-2: Improved import structure.Updated imports to include the new
repoMetadataSchemainstead of directly importingRepoMetadatatype, which aligns with the schema-based validation approach.
20-20: Enhanced metadata validation.Changed from type casting to schema validation using
repoMetadataSchema.parse(), which provides better runtime validation and error handling for repository metadata.
60-70: Improved command construction readability.Refactored command construction to use an array of strings joined together instead of a single template string. This improves readability and makes future modifications easier.
packages/backend/src/repoCompileUtils.ts (9)
11-11: Good addition of path module.Adding the path module is appropriate for handling URL path construction, particularly for joining the repository name root with display names.
33-36: Well-implemented display name handling for GitHub repositories.The code now extracts the repository name root from the host URL and properly constructs both the full repository name and a shorter display name, which aligns with the PR objectives.
Also applies to: 38-40
49-49: Consistent implementation of display name for GitHub repositories.The display name is properly added to both the repository record and the metadata configuration for zoekt, ensuring consistency across the system.
Also applies to: 75-75
102-105: Well-implemented display name handling for GitLab repositories.Similar to the GitHub implementation, the code properly extracts the name root and sets up the display name for GitLab repositories.
Also applies to: 110-111
120-120: Consistent implementation of display name for GitLab repositories.The display name is properly added to both the repository record and the metadata configuration for zoekt, ensuring consistency across the system.
Also applies to: 144-144
171-174: Well-implemented display name handling for Gitea repositories.Same pattern is consistently applied for Gitea repositories, maintaining a uniform approach across different repository types.
Also applies to: 177-178
187-187: Consistent implementation of display name for Gitea repositories.The display name is properly added to both the repository record and the metadata configuration for zoekt, ensuring consistency.
Also applies to: 209-209
232-240: Well-implemented display name handling for Gerrit repositories.The pattern for handling display names is consistently applied to Gerrit repositories as well.
265-265: Consistent implementation of display name for Gerrit repositories.The display name is properly added to both the repository record and the metadata configuration for zoekt, maintaining consistency with other repository types.
Also applies to: 286-286
packages/web/src/lib/utils.ts (2)
50-52: Simplified null checking.The code now immediately returns undefined if
repo.RawConfigdoesn't exist, which is cleaner than the previous implementation.
61-63:❓ Verification inconclusive
Parameter type change may impact existing code.
The function parameter was changed from optional
repo?: RepositoryQueryto requiredrepo: RepositoryQuery. While the display name handling improvement is good, this change could break existing code that might pass undefined.
🏁 Script executed:
#!/bin/bash # Check if there are any call sites passing undefined to getRepoQueryCodeHostInfo rg -A 5 'getRepoQueryCodeHostInfo\(\s*\)' --glob='*.ts' --glob='*.tsx' rg -A 5 'getRepoQueryCodeHostInfo\(\s*undefined' --glob='*.ts' --glob='*.tsx'Length of output: 152
Action Required: Verify Call Sites for
getRepoQueryCodeHostInfoUsageThe function’s parameter type was updated from optional to required, and our initial automated search did not yield any explicit instances of call sites passing
undefined. However, the output was inconclusive. Please manually verify that no callers are relying on the previous optional behavior (i.e., passing or defaulting toundefined). Confirm that all invocations ofgetRepoQueryCodeHostInfosupply a validRepositoryQueryobject.
- Review Target: All references in TypeScript files (i.e.,
*.tsand*.tsx) that invokegetRepoQueryCodeHostInfo.- Action: Ensure no call site passes
undefinedor omits the argument under the assumption of the optional parameter.packages/backend/src/repoManager.ts (5)
6-6: Good improvement using Zod schema validationThe change from using
RepoMetadatatype to usingrepoMetadataSchemafor validation provides stronger runtime type checking and better error handling, which aligns with modern TypeScript practices.
8-8: Importing upsertGitConfig aligns with the new architectureAdding
upsertGitConfigto the imports supports the new approach of separating git operations, which improves code maintainability.
203-203: Good use of schema validationUsing
repoMetadataSchema.parse()instead of type casting ensures runtime validation of the repository metadata, providing better error handling if the data structure doesn't match expectations.
242-242: Improved separation of concernsRemoving the
metadata.gitConfigparameter from thecloneRepositorycall properly separates the concerns of cloning and configuration. This makes the code more maintainable and follows the single responsibility principle.
251-256: Great enhancement to ensure consistent git configurationThis new block ensures that the git configuration is always up to date with what's stored in the database, regardless of whether it's a clone or fetch operation. This maintains eventual consistency between the database and the git repository, which directly addresses one of the PR objectives.
packages/backend/src/git.ts (2)
3-3: Good simplification of the cloneRepository functionRemoving the
gitConfigparameter simplifies the function signature and makes the code more focused on just cloning the repository, supporting better separation of concerns.
45-63: Well-implemented upsertGitConfig function with clear documentationThe new function is well-documented and properly implements the logic for updating git configurations. It handles error cases appropriately and maintains the existing keys that are not in the provided gitConfig.
Regarding the state question from the previous review: The
git.addConfig(key, value)call modifies the local repository configuration (not global git config). Each call toaddConfigis isolated to the repository at the specified path, as thecwd(path)method sets the working directory for the git commands.
This PR does the following:
displayNamefield to the Repos table. This field differs tonamein that it does not include thehostUrlas a prefix, and thus is shorter & easier to read. The displayName is also passed along to zoekt viazoekt.display-name. This field replaces needing to create a display name client side.RepoManager::syncGitRepositoryto always "upsert" the repo'smetadata.gitConfigregardless of clone or fetch (previously it only happened on fetch). This makes it s.t., the data in the database (metadata.gitConfig) is eventually consistent with the git config on the repo. This is essential for when we add new fields (likezoekt.display-namein this case).Summary by CodeRabbit
Bug Fixes
New Features