feat(worktree): use human-readable names for worktree branches#2340
Open
MukundaKatta wants to merge 1 commit into
Open
feat(worktree): use human-readable names for worktree branches#2340MukundaKatta wants to merge 1 commit into
MukundaKatta wants to merge 1 commit into
Conversation
Worktrees were named with random 4-digit numbers that read like SHA snippets, making it momentarily unclear which worktree you were on. Generate adjective-noun-NN names instead (e.g. "swift-otter-42") from a small curated word list. Names stay filesystem-safe and short. Fixes PostHog#1844
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/git/src/worktree-name.test.ts:10-31
The team's standard is to prefer parameterised tests over manual loops. The three loop-based cases ("varies", "filesystem-safe", "compact") can each be expressed as `it.each` calls, making each individual sample a named test case and avoiding an imperative loop that obscures the intent.
```suggestion
it("produces varied names over multiple calls", () => {
const names = new Set(Array.from({ length: 50 }, generateHumanReadableName));
// With 36 * 36 * 90 = ~116k combinations, 50 draws should yield
// many unique values. Allow generous slack for randomness.
expect(names.size).toBeGreaterThan(20);
});
it.each(Array.from({ length: 25 }, generateHumanReadableName))(
"uses only filesystem-safe characters: %s",
(name) => {
expect(name).toMatch(/^[a-z0-9-]+$/);
},
);
it.each(Array.from({ length: 25 }, generateHumanReadableName))(
"stays compact (under 32 chars): %s",
(name) => {
expect(name.length).toBeLessThanOrEqual(32);
},
);
```
Reviews (1): Last reviewed commit: "feat(worktree): use human-readable names..." | Re-trigger Greptile |
Comment on lines
+10
to
+31
| it("produces varied names over multiple calls", () => { | ||
| const names = new Set<string>(); | ||
| for (let i = 0; i < 50; i++) { | ||
| names.add(generateHumanReadableName()); | ||
| } | ||
| // With 36 * 36 * 90 = ~116k combinations, 50 draws should yield | ||
| // many unique values. Allow generous slack for randomness. | ||
| expect(names.size).toBeGreaterThan(20); | ||
| }); | ||
|
|
||
| it("uses only filesystem-safe characters", () => { | ||
| for (let i = 0; i < 25; i++) { | ||
| const name = generateHumanReadableName(); | ||
| expect(name).toMatch(/^[a-z0-9-]+$/); | ||
| } | ||
| }); | ||
|
|
||
| it("stays compact (under 32 chars)", () => { | ||
| for (let i = 0; i < 25; i++) { | ||
| expect(generateHumanReadableName().length).toBeLessThanOrEqual(32); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
The team's standard is to prefer parameterised tests over manual loops. The three loop-based cases ("varies", "filesystem-safe", "compact") can each be expressed as
it.each calls, making each individual sample a named test case and avoiding an imperative loop that obscures the intent.
Suggested change
| it("produces varied names over multiple calls", () => { | |
| const names = new Set<string>(); | |
| for (let i = 0; i < 50; i++) { | |
| names.add(generateHumanReadableName()); | |
| } | |
| // With 36 * 36 * 90 = ~116k combinations, 50 draws should yield | |
| // many unique values. Allow generous slack for randomness. | |
| expect(names.size).toBeGreaterThan(20); | |
| }); | |
| it("uses only filesystem-safe characters", () => { | |
| for (let i = 0; i < 25; i++) { | |
| const name = generateHumanReadableName(); | |
| expect(name).toMatch(/^[a-z0-9-]+$/); | |
| } | |
| }); | |
| it("stays compact (under 32 chars)", () => { | |
| for (let i = 0; i < 25; i++) { | |
| expect(generateHumanReadableName().length).toBeLessThanOrEqual(32); | |
| } | |
| }); | |
| it("produces varied names over multiple calls", () => { | |
| const names = new Set(Array.from({ length: 50 }, generateHumanReadableName)); | |
| // With 36 * 36 * 90 = ~116k combinations, 50 draws should yield | |
| // many unique values. Allow generous slack for randomness. | |
| expect(names.size).toBeGreaterThan(20); | |
| }); | |
| it.each(Array.from({ length: 25 }, generateHumanReadableName))( | |
| "uses only filesystem-safe characters: %s", | |
| (name) => { | |
| expect(name).toMatch(/^[a-z0-9-]+$/); | |
| }, | |
| ); | |
| it.each(Array.from({ length: 25 }, generateHumanReadableName))( | |
| "stays compact (under 32 chars): %s", | |
| (name) => { | |
| expect(name.length).toBeLessThanOrEqual(32); | |
| }, | |
| ); |
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/git/src/worktree-name.test.ts
Line: 10-31
Comment:
The team's standard is to prefer parameterised tests over manual loops. The three loop-based cases ("varies", "filesystem-safe", "compact") can each be expressed as `it.each` calls, making each individual sample a named test case and avoiding an imperative loop that obscures the intent.
```suggestion
it("produces varied names over multiple calls", () => {
const names = new Set(Array.from({ length: 50 }, generateHumanReadableName));
// With 36 * 36 * 90 = ~116k combinations, 50 draws should yield
// many unique values. Allow generous slack for randomness.
expect(names.size).toBeGreaterThan(20);
});
it.each(Array.from({ length: 25 }, generateHumanReadableName))(
"uses only filesystem-safe characters: %s",
(name) => {
expect(name).toMatch(/^[a-z0-9-]+$/);
},
);
it.each(Array.from({ length: 25 }, generateHumanReadableName))(
"stays compact (under 32 chars): %s",
(name) => {
expect(name.length).toBeLessThanOrEqual(32);
},
);
```
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Closes #1844
New worktrees were labeled with random 4-digit numbers (
crypto.randomInt(1000, 10000)), which read like short SHA snippets and made it momentarily unclear which worktree you were on.Changes
packages/git/src/worktree-name.tswith a small curated adjective-noun word list and agenerateHumanReadableName()helper that returns names likeswift-otter-42.WorktreeManager.generateWorktreeName()now delegates to this helper. All existing collision-handling paths and the${name}${Date.now()}defensive fallback are unchanged.-), and short (under 32 chars).~116kcombinations from 36 adjectives x 36 nouns x 90 suffix values keep collisions rare in practice.How did you test this?
packages/git/src/worktree-name.test.tscovering the name shape, character safety, length bound, and rough uniqueness over 50 draws.pnpm --filter @posthog/git test-> 159 passed (8 files), including the 4 new ones.pnpm --filter @posthog/git typecheck-> clean.biome checkon the touched files -> clean.Publish to changelog?
no