feat: add branching #195
feat: add branching #195
Conversation
WalkthroughAdd optional branch support across schema, config, CLI options, commands, tests, and fixtures; introduce branch formatting utility and config validation; add 404 error case; minor .npmrc and HACKING.md tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI
participant Config as "Local Config"
participant Command as "push/pull/compare"
participant API as "Tolgee API"
participant FS as "Local FS / Extractor"
Note over CLI,Config: User runs CLI (may pass -b/--branch) or rely on config.branch
User->>CLI: invoke command (with optional --branch)
CLI->>Config: load config (reads config.branch)
CLI->>Command: start command (opts.branch resolved)
Command->>API: request (includes branch in query/body)
API-->>Command: response
alt Fetch & extract flow
Command->>FS: extract/unzip (log includes appendBranch)
FS-->>Command: done
end
Command-->>CLI: complete (messages include appendBranch)
CLI-->>User: output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
![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)
src/utils/branch.ts (1)
1-3: Consider clarifying the no-branch message.The message
" (no or default branch)"is ambiguous—it's unclear whether the system is using a default branch or operating without any branch context. Consider making the message more explicit about the actual behavior.💡 Possible alternative wording
If no branch means operating on the default/main branch:
- return branch ? ` (branch "${branch}")` : ' (no or default branch)'; + return branch ? ` (branch "${branch}")` : ' (default branch)';Or if it means no branch filtering is applied:
- return branch ? ` (branch "${branch}")` : ' (no or default branch)'; + return branch ? ` (branch "${branch}")` : ' (all branches)';Choose based on the actual Tolgee API behavior when no branch is specified.
src/commands/push.ts (1)
247-247: Consider adjusting the message when no branch is specified.When
opts.branchis undefined,appendBranchreturns' (no or default branch)', which will display as"Importing (no or default branch)...". This might be confusing for users who don't use branching at all. Consider showing the branch suffix only when a branch is explicitly specified:🔎 Suggested change to appendBranch utility
// In src/utils/branch.ts export function appendBranch(branch?: string) { - return branch ? ` (branch "${branch}")` : ' (no or default branch)'; + return branch ? ` (branch "${branch}")` : ''; }Alternatively, if the current behavior is intentional to always indicate branch status, the message could be rephrased to
' (default branch)'for clarity.Also applies to: 263-263
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.npmrc(0 hunks)HACKING.md(1 hunks)schema.json(1 hunks)src/cli.ts(2 hunks)src/commands/pull.ts(3 hunks)src/commands/push.ts(4 hunks)src/config/tolgeerc.ts(2 hunks)src/options.ts(2 hunks)src/schema.d.ts(1 hunks)src/utils/branch.ts(1 hunks)test/__fixtures__/validTolgeeRc/withBranch.json(1 hunks)test/unit/config.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .npmrc
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T09:27:51.189Z
Learnt from: stepan662
Repo: tolgee/tolgee-cli PR: 185
File: src/schema.d.ts:150-152
Timestamp: 2025-08-13T09:27:51.189Z
Learning: In the Tolgee CLI codebase, schema validation uses JSON schema from schema.json as the source of truth. The src/schema.d.ts file contains automatically generated TypeScript types derived from the JSON schema for compile-time type checking only, not runtime exports. CLI validation happens through JSON schema validation, not through runtime TypeScript enums.
Applied to files:
src/schema.d.tssrc/config/tolgeerc.ts
🧬 Code graph analysis (4)
test/unit/config.test.ts (1)
src/config/tolgeerc.ts (1)
loadTolgeeRc(102-133)
src/cli.ts (1)
src/options.ts (1)
PROJECT_BRANCH(59-62)
src/commands/pull.ts (4)
src/utils/branch.ts (1)
appendBranch(1-3)src/utils/prepareDir.ts (1)
prepareDir(4-15)src/utils/logger.ts (1)
loading(125-155)src/utils/zip.ts (1)
unzipBuffer(32-45)
src/commands/push.ts (1)
src/utils/branch.ts (1)
appendBranch(1-3)
🔇 Additional comments (16)
HACKING.md (1)
17-18: LGTM: Grammar improvement.The addition of "to" improves the grammatical clarity of the sentence.
src/schema.d.ts (1)
83-86: LGTM: Type definition aligns with schema.The optional branch field is correctly defined and consistent with the schema.json update in this PR.
test/__fixtures__/validTolgeeRc/withBranch.json (1)
1-5: LGTM: Valid test fixture.The fixture appropriately tests configuration loading with a branch value.
test/unit/config.test.ts (1)
97-105: LGTM: Test case properly validates branch loading.The test follows established patterns and correctly verifies that branch values are loaded from configuration files.
src/commands/pull.ts (3)
13-13: LGTM: Proper import of branch utility.
45-45: LGTM: Branch filtering correctly integrated.The branch value is properly passed to the export API call via
filterBranch, enabling branch-aware data fetching.
65-72: LGTM: User messages enhanced with branch context.Both fetch and extract operations now provide clear branch context to users through the
appendBranchutility.src/cli.ts (2)
22-22: LGTM: Proper import of branch option.
184-184: LGTM: Branch option correctly wired with config default.The PROJECT_BRANCH option follows the established pattern for other global options, properly defaulting to the value from configuration.
schema.json (1)
8-12: Branch whitespace validation is properly handled in the config parser.The schema correctly enforces
minLength: 1for the branch property. The config parser atsrc/config/tolgeerc.ts(lines 42-49) validates that whitespace-only strings are rejected by checking!rc.branch.trim().lengthbefore accepting the value, and then trims the branch accordingly.src/commands/push.ts (2)
32-32: LGTM!Import is correctly added for the new branch utility function.
225-225: LGTM!Branch parameter is correctly included in the import request payload, consistent with the new branching feature.
src/options.ts (2)
36-36: LGTM!The optional
branchfield is correctly added toBaseOptions, consistent with other optional fields in the interface.
59-62: LGTM!The new
PROJECT_BRANCHoption follows the established patterns for other CLI options:
- Uses consistent flag naming (
-b, --branch)- Environment variable support via
.env('TOLGEE_BRANCH')- Clear description indicating when to use the option
src/config/tolgeerc.ts (2)
1-12: LGTM!Good modernization of imports:
- Using
node:prefixes for Node.js built-in modules is the recommended ESM convention- Type-only imports for
CosmiconfigResultandSchemaare appropriate since they're only used for type annotations
42-49: LGTM!The branch validation logic is well-implemented:
- Correctly handles undefined (skips validation)
- Validates non-empty string requirement
- Properly trims whitespace and stores the normalized value
- Error message is consistent with the existing validation style
The defensive
typeof rc.branch !== 'string'check provides good runtime safety for user-provided config values, which is appropriate given configs are loaded dynamically. Based on learnings, this runtime validation complements the JSON schema validation that runs afterwards.
zomp
force-pushed
the
janmolnar/branching
branch
from
5fc1647 to
fd4c053
Compare
There is `enable-pre-post-scripts` option which is set to true and is true by default, but it throws a warning, so I am removing it: npm warn Unknown project config "enable-pre-post-scripts". This will stop working in the next major version of npm.
zomp
force-pushed
the
janmolnar/branching
branch
from
fd4c053 to
5a93411
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.npmrcHACKING.mdschema.jsonsrc/cli.tssrc/client/errorFromLoadable.tssrc/client/internal/schema.generated.tssrc/commands/pull.tssrc/commands/push.tssrc/commands/sync/compare.tssrc/config/tolgeerc.tssrc/options.tssrc/schema.d.tssrc/utils/branch.tstest/__fixtures__/validTolgeeRc/withBranch.jsontest/e2e/compare.test.tstest/e2e/utils/api/common.tstest/unit/config.test.ts
💤 Files with no reviewable changes (1)
- .npmrc
🚧 Files skipped from review as they are similar to previous changes (12)
- src/commands/sync/compare.ts
- src/cli.ts
- src/options.ts
- src/config/tolgeerc.ts
- schema.json
- src/utils/branch.ts
- test/fixtures/validTolgeeRc/withBranch.json
- src/commands/pull.ts
- src/commands/push.ts
- test/unit/config.test.ts
- HACKING.md
- src/schema.d.ts
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/compare.test.ts (3)
test/e2e/utils/api/common.ts (4)
createProjectWithClient(49-94)createBranch(100-126)createKey(132-146)deleteProject(39-43)test/e2e/utils/api/project2.ts (1)
PROJECT_2(6-15)test/e2e/utils/run.ts (1)
run(106-112)
test/e2e/utils/api/common.ts (1)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
🔇 Additional comments (7)
src/client/errorFromLoadable.ts (1)
46-49: LGTM! Clean addition of 404 error handling.The new case for HTTP 404 follows the established pattern and provides a clear, actionable error message. The inline comment helpfully contextualizes the scenario (missing branch), which aligns well with the PR's branching feature.
test/e2e/compare.test.ts (4)
6-7: LGTM!The new imports are properly used in the branching test suite.
39-39: LGTM!Good consistency improvement to match the PROJECT_2 constant being used.
71-72: LGTM!The updated assertions with
+/-prefixes improve clarity and align with the enhanced output format of the compare command.Also applies to: 86-87, 111-116, 135-140
160-230: Well-structured branching test suite.The tests comprehensively cover:
- Keys existing in the feature branch
- Keys not existing in the main branch
- Error handling for non-existent branches
The setup and teardown logic is appropriate.
test/e2e/utils/api/common.ts (2)
56-58: LGTM!Adding the
filterCurrentUserOwnerfilter improves test reliability by ensuring the correct organization is selected.
128-146: LGTM!Clean and type-safe implementation with appropriate defaults and option handling.
zomp
force-pushed
the
janmolnar/branching
branch
from
5a93411 to
d8da832
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/e2e/compare.test.ts (1)
212-229: Consider asserting a more specific error code or message.The test correctly verifies that referencing a non-existent branch fails. However, checking
out.stderr + out.stdoutis a bit broad. If the CLI has a consistent error output location, consider checking only that stream. Also, verifying the exit code is non-zero is good, but you might want to assert a specific exit code if the CLI uses distinct codes for different error types.test/e2e/utils/api/common.ts (1)
96-125: Consider returning the created branch for potential future use.The function works correctly for current test needs. However, returning the API response would allow callers to access the created branch ID if needed later.
🔎 Proposed enhancement
export async function createBranch( client: TolgeeClient, name: string, options?: CreateBranchOptions ) { let originBranchId = options?.originBranchId; if (!originBranchId) { const branches = await client.GET('/v2/projects/{projectId}/branches', { params: { path: { projectId: client.getProjectId() }, }, }); const origin = branches.data?._embedded?.branches?.find((b) => b.isDefault); originBranchId = origin?.id; } if (!originBranchId) { throw new Error('Default branch not found'); } - await client.POST('/v2/projects/{projectId}/branches', { + return await client.POST('/v2/projects/{projectId}/branches', { params: { path: { projectId: client.getProjectId() } }, body: { name, originBranchId }, }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/errorFromLoadable.tssrc/utils/branch.tstest/e2e/compare.test.tstest/e2e/utils/api/common.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/branch.ts
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/utils/api/common.ts (2)
src/client/TolgeeClient.ts (1)
TolgeeClient(34-34)src/client/internal/schema.generated.ts (1)
components(5440-9752)
test/e2e/compare.test.ts (2)
test/e2e/utils/api/common.ts (4)
createProjectWithClient(49-94)createPak(159-165)createBranch(100-125)createKey(131-145)test/e2e/utils/api/project2.ts (1)
PROJECT_2(6-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
🔇 Additional comments (10)
src/client/errorFromLoadable.ts (1)
46-48: LGTM! Clean addition of 404 error handling.The implementation correctly handles HTTP 404 responses with a clear, actionable error message. The comment provides helpful context about branch-related scenarios, and the code follows the established pattern consistently.
test/e2e/compare.test.ts (6)
6-11: LGTM!The imports for
createBranchandcreateKeyare correctly added and used in the new Branching test suite.
39-39: LGTM!The describe label correctly reflects that this test suite uses
PROJECT_2data.
111-116: LGTM!The updated expectations correctly reflect the new output format with
+prefix for added keys and-prefix for removed keys.Also applies to: 135-140
160-179: LGTM!The Branching test setup is well-structured: creates project, PAK, feature branch, and adds keys to the branch. The
afterEachcleanup ensures proper teardown.
181-198: LGTM!Good test coverage - verifies that keys created on
feature-branchare detected as in-sync when explicitly targeting that branch via--branchoption.
200-210: LGTM!Correctly validates that branch-specific keys don't appear in the main branch, resulting in an "out of sync" status with 2 new keys reported.
test/e2e/utils/api/common.ts (3)
56-58: LGTM!The formatting change is minor and doesn't affect functionality.
127-145: LGTM!The
createKeyhelper is well-designed:
- Uses
Partial<Omit<...>>to make all fields optional exceptname- Correctly defaults
isPluraltofalseas required byCreateKeyDto- Returns the API response for callers who need it
147-157: LGTM!The expanded
DEFAULT_SCOPESinclude the necessary permissions (keys.create,keys.edit, etc.) for the new branch and key creation tests.
Summary by CodeRabbit
New Features
Validation
UX
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.