feat: add --namespaces support to sync
#151
Conversation
stepan662
force-pushed
the
stepangranat/sync-namespaces
branch
from
b73e574 to
2c9cc42
Compare
stepan662
force-pushed
the
stepangranat/sync-namespaces
branch
from
2c9cc42 to
120782c
Compare
WalkthroughThis update introduces namespace filtering to the synchronization and push commands, allowing operations to be limited to specified namespaces. The schema, CLI, and internal logic are updated to support this feature. Additionally, the representation of the default namespace is unified as an empty string. Comprehensive end-to-end tests are refactored and expanded to cover these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI_User
participant CLI
participant SyncLogic
participant TolgeeAPI
CLI_User->>CLI: Run sync/push command with --namespaces option
CLI->>SyncLogic: Pass namespaces option
SyncLogic->>SyncLogic: Filter local and remote keys by namespaces
SyncLogic->>TolgeeAPI: Fetch or update keys (filtered by namespace)
TolgeeAPI-->>SyncLogic: Respond with filtered key data
SyncLogic->>CLI: Output results (added/removed keys)
CLI->>CLI_User: Display sync/push summary
Suggested labels
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e/utils/pushFilesConfig.ts (1)
4-19: Consider adding input validation for security.The utility function is well-designed and follows clear patterns. However, consider adding validation for the
namespacesparameter to prevent potential path traversal attacks, as namespace values are used directly in URL construction.export function pushFilesConfig(base: URL, namespaces: string[] = ['']) { + // Validate namespaces to prevent path traversal + const sanitizedNamespaces = namespaces.map(ns => { + if (ns.includes('..') || ns.includes('/') || ns.includes('\\')) { + throw new Error(`Invalid namespace: ${ns}. Namespaces cannot contain path separators or traversal sequences.`); + } + return ns; + }); + const result: FileMatch[] = []; - for (const ns of namespaces) { + for (const ns of sanitizedNamespaces) { result.push({Alternatively, you could use a simple regex validation:
/^[a-zA-Z0-9_-]*$/.test(ns)if namespaces should only contain alphanumeric characters, underscores, and hyphens.test/e2e/sync.test.ts (1)
1-480: Suggest consolidating duplicate test patternsWhile the test coverage is excellent, there's significant code duplication across the push test files (p1, p2, p3) in setup, teardown, and assertion patterns. Consider extracting common test utilities to reduce duplication and improve maintainability.
For example, you could create a shared utility for:
- Common setup/teardown patterns
- Standard API assertion helpers
- Reusable test data validation functions
This would make the tests more maintainable while preserving their comprehensive coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
schema.json(1 hunks)src/commands/sync/sync.ts(4 hunks)src/commands/sync/syncUtils.ts(2 hunks)src/extractor/runner.ts(1 hunks)src/schema.d.ts(1 hunks)test/__fixtures__/testProjectCode/Test3Mixed.tsx(0 hunks)test/e2e/compare.test.ts(1 hunks)test/e2e/push.p1.test.ts(1 hunks)test/e2e/push.p2.test.ts(1 hunks)test/e2e/push.p3.test.ts(1 hunks)test/e2e/push.test.ts(0 hunks)test/e2e/sync.test.ts(6 hunks)test/e2e/utils/data.ts(1 hunks)test/e2e/utils/pushFilesConfig.ts(1 hunks)
💤 Files with no reviewable changes (2)
- test/fixtures/testProjectCode/Test3Mixed.tsx
- test/e2e/push.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/e2e/utils/pushFilesConfig.ts (1)
src/schema.d.ts (1)
FileMatch(236-245)
src/commands/sync/sync.ts (1)
src/commands/sync/syncUtils.ts (1)
compareKeys(49-101)
🔇 Additional comments (23)
schema.json (1)
170-177: LGTM! Schema definition follows established patterns.The new
namespacesproperty for thesyncobject is well-defined and consistent with similar properties in thepushandpullobjects. The optional array of strings type and descriptive text are appropriate.src/schema.d.ts (1)
196-199: LGTM! TypeScript interface correctly reflects the schema.The auto-generated TypeScript interface properly defines the optional
namespacesproperty asstring[]with appropriate documentation matching the JSON schema.test/e2e/compare.test.ts (1)
177-179:✅ Verification successful
Verify the updated test expectations are correct.
The test now expects 4 unused keys instead of 3 and includes a specific assertion for "onions (namespace: food)". Please ensure these updated expectations accurately reflect the new namespace filtering behavior.
🏁 Script executed:
#!/bin/bash # Description: Verify the test fixture and understand the namespace changes # Expected: Find the test fixture file and see what keys are actually present # Look for the Test3Mixed.tsx fixture file fd "Test3Mixed.tsx" --type f # Show the content to understand what keys are being extracted cat test/__fixtures__/testProjectCode/Test3Mixed.tsx # Look for any other related test files that might give context rg -A 5 -B 5 "onions.*food" test/Length of output: 2858
🏁 Script executed:
#!/bin/bash # Print relevant snippet from the compare test echo "=== compare.test.ts (lines 150-200) ===" sed -n '150,200p' test/e2e/compare.test.ts # Locate translation.json in the test fixtures echo echo "=== translation.json files in fixtures ===" fd translation.json --type f test/__fixtures__/testProjectCode # Display the contents of each translation.json for file in $(fd translation.json --type f test/__fixtures__/testProjectCode); do echo echo "=== Content of $file ===" cat "$file" doneLength of output: 1448
🏁 Script executed:
#!/bin/bash # Locate PROJECT_3 constant in the compare test grep -R "PROJECT_3" -n test/e2e/compare.test.ts # List all files in the fixtures directory echo echo "=== All fixtures under test/__fixtures__ ===" find test/__fixtures__ -type f # Specifically list JSON files to find translation data echo echo "=== JSON files in fixtures ===" find test/__fixtures__ -type f -name "*.json"Length of output: 9467
🏁 Script executed:
#!/bin/bash # Find and display the contents of project3.js to inspect initial keys files=$(fd project3.js --type f test) for file in $files; do echo "=== Content of $file ===" sed -n '1,200p' "$file" doneLength of output: 40
🏁 Script executed:
#!/bin/bash # 1. Locate the project3 fixture file in the repository echo "=== Searching for project3.js ===" find . -type f | grep -i "project3.js\?$" # 2. If found, display its contents to inspect the initial keys loaded PROJECT3_PATH=$(find . -type f | grep -i "project3.js\?$" | head -n1) if [ -n "$PROJECT3_PATH" ]; then echo echo "=== Content of $PROJECT3_PATH ===" sed -n '1,200p' "$PROJECT3_PATH" else echo "project3.js not found in repository." fi # 3. Search for any .ts variant just in case echo echo "=== Searching for project3.ts ===" find . -type f | grep -i "project3.ts\?$"Length of output: 456
🏁 Script executed:
#!/bin/bash # Display the contents of project3.ts to inspect initial translation keys and settings echo "=== project3.ts ===" sed -n '1,200p' test/e2e/utils/api/project3.tsLength of output: 1898
🏁 Script executed:
#!/bin/bash # Display translation keys for Project 3 fixtures echo "=== test3/en.json ===" cat test/__fixtures__/tolgeeImportData/test3/en.json || echo "File not found" echo echo "=== test3/drinks/en.json ===" cat test/__fixtures__/tolgeeImportData/test3/drinks/en.json || echo "File not found" echo echo "=== test3/food/en.json ===" cat test/__fixtures__/tolgeeImportData/test3/food/en.json || echo "File not found"Length of output: 584
Test expectations for Project 3 compare() are correct
- Translation import and code usage yield 4 new keys (welcome, table (ns furniture), chair (ns furniture), cookies (ns food)) and 4 unused keys (table, chair, soda (ns drinks), onions (ns food)).
- The assertion for
4 unused keysand the specific check for- onions (namespace: food)accurately reflect the fixture data.- The other checks (
+ cookies (namespace: food),- soda (namespace: drinks),+ table (namespace: furniture),- table) also match the actual additions/removals.No changes required.
src/extractor/runner.ts (1)
142-142: LGTM: Consistent namespace representation standardization.The change from
NullNamespacesymbol to empty string''for the default namespace is part of a consistent refactoring across the codebase. This standardization improves code maintainability and aligns with the new namespace filtering functionality.test/e2e/utils/data.ts (1)
3-3: LGTM: Improved error handling with defensive programming.The addition of optional chaining and nullish coalescing (
data._embedded?.keys ?? []) prevents runtime errors when the API response structure is incomplete. This defensive approach improves test reliability by gracefully handling missing or undefined nested properties.src/commands/sync/syncUtils.ts (3)
57-57: LGTM: Consistent namespace standardization in key comparison.The changes to use empty string
''instead ofNullNamespacefor keys without namespaces are consistent with the overall refactoring. This standardization ensures uniform namespace representation throughout the comparison logic.Also applies to: 63-63
69-69: LGTM: Simplified namespace iteration.Removing the special handling of
NullNamespaceand using standard object key iteration is cleaner and more intuitive. The empty string namespace is now treated like any other namespace key.
77-77: LGTM: Consistent namespace fallback in added keys.The change to use empty string
''as the namespace fallback maintains consistency with the overall refactoring pattern and ensures proper namespace representation for keys being added.src/commands/sync/sync.ts (3)
27-27: LGTM: Proper type definition for namespaces option.The addition of
namespaces?: string[]to the Options type is correctly typed and follows TypeScript conventions for optional array properties.
85-91: LGTM: Correct local key filtering implementation.The local key filtering logic correctly clears keys from namespaces not specified in the filter list. This approach efficiently removes unwanted keys before comparison while preserving the existing data structure.
243-248: LGTM: Well-defined CLI option for namespaces.The CLI option definition is properly structured with clear description and correct default value resolution from configuration. The option accepts multiple namespace values as expected.
test/e2e/push.p2.test.ts (3)
1-29: LGTM: Well-structured test setupThe file imports, constants, and setup/teardown logic are well-organized and follow good testing practices with proper resource management.
30-66: LGTM: Proper conflict detection testThe test correctly verifies that push fails when conflicts exist (exit code 1) and validates that no changes are made to the remote data when conflicts are detected.
147-186: LGTM: Interactive confirmation test is well implementedThe test using
runWithStdinwith 'OVERRIDE' input properly simulates user interaction for conflict resolution. The assertion validates that the override actually took effect by checking the updated values.test/e2e/push.p1.test.ts (3)
31-65: LGTM: Comprehensive basic push testThe test properly validates that all languages from the files are pushed when no filters are applied, and correctly verifies the stored translations via API calls.
67-99: LGTM: Language filtering validationThe test correctly validates that only the specified language ('fr') is pushed when using the
-lflag, demonstrating proper language filtering functionality.
101-132: LGTM: Config-based language filteringThe test demonstrates that language filtering works consistently whether specified via CLI arguments or configuration file, using the
pushFilesConfigutility appropriately.test/e2e/push.p3.test.ts (3)
37-72: LGTM: Comprehensive namespace push testThe test properly validates that files with different namespace structures (default and 'drinks' namespace) are correctly pushed and stored with their respective namespace assignments.
74-106: LGTM: Namespace filtering functionalityThe test correctly validates that only the specified namespace ('drinks') is pushed when using the
-nflag, demonstrating proper namespace filtering.
232-284: Excellent test for namespace-scoped key removalThis test validates a critical edge case: when using
--remove-other-keyswith namespace filtering, only keys within the filtered namespaces should be considered for removal. Keys in other namespaces should remain untouched. The test correctly expects the 'knife' key to remain since it's outside the 'drinks' namespace filter.test/e2e/sync.test.ts (3)
34-35: LGTM: Clear constant renamingThe renaming from
CODE_PROJECT_3toCODE_PROJECT_3_DIFFand addition ofCODE_PROJECT_3_MIXEDimproves clarity about which test fixture is being used.
354-385: LGTM: Comprehensive namespace filtering testThe test properly validates that sync with
--namespaces foodonly adds keys from the 'food' namespace while leaving other namespaces untouched. The assertion checking for 'table' (in food namespace) but not 'welcome' (in other namespace) is particularly good.
414-448: Critical test for namespace-scoped deletionThis test validates that
--remove-unusedwith namespace filtering only deletes keys within the specified namespace ('food'), leaving keys in other namespaces intact. This is crucial for preventing accidental deletion of keys outside the scope of synchronization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/sync.test.ts (1)
387-396: Remove unused helper function.The
print()function is defined but never used, as flagged by ESLint.- async function print() { - const keys = await client.GET('/v2/projects/{projectId}/translations', { - params: { - path: { projectId: client.getProjectId() }, - }, - }); - - const stored = tolgeeDataToDict(keys.data); - console.log(stored); - }🧰 Tools
🪛 GitHub Check: ESLint
[warning] 387-387:
'print' is defined but never used. Allowed unused vars must match /^_/u
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/commands/sync/sync.ts(4 hunks)test/__fixtures__/testProjectCode/Test3Mixed.tsx(2 hunks)test/e2e/compare.test.ts(1 hunks)test/e2e/sync.test.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/compare.test.ts
- src/commands/sync/sync.ts
- test/fixtures/testProjectCode/Test3Mixed.tsx
🧰 Additional context used
🪛 GitHub Check: ESLint
test/e2e/sync.test.ts
[warning] 387-387:
'print' is defined but never used. Allowed unused vars must match /^_/u
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: Unit Tests (windows-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
🔇 Additional comments (6)
test/e2e/sync.test.ts (6)
34-35: LGTM: Constants renamed appropriately for new test fixtures.The renaming from
CODE_PROJECT_3toCODE_PROJECT_3_DIFFand addition ofCODE_PROJECT_3_MIXEDaligns with the test fixture changes to support namespace testing scenarios.
300-300: LGTM: Existing tests updated to use renamed constant.The existing namespace tests correctly use the renamed
CODE_PROJECT_3_DIFFconstant.Also applies to: 329-329
354-385: LGTM: Comprehensive test for namespace filtering via CLI args.This test effectively validates that the
--namespacesflag filters synchronization to only the specified namespace ('food'), correctly adding 1 string and identifying unused keys outside the filtered namespace.
398-425: LGTM: Comprehensive test for namespace filtering via config.This test validates namespace filtering through configuration files, correctly testing both default namespace ('') and 'food' namespace synchronization. The expected results (2 strings added, 6 unused keys) are consistent with the multi-namespace scenario.
427-466: LGTM: Excellent test for selective key deletion with namespace filtering.This test comprehensively validates that
--remove-unusedrespects namespace boundaries when using the--namespacesflag. The use of both--namespaces=(empty string for default namespace) and--namespaces=fooddemonstrates proper handling of multiple namespace specification.
468-497: LGTM: Config-based namespace deletion test validates key behavior.This test confirms that namespace-scoped deletion works correctly through configuration files, properly removing only keys within the specified 'food' namespace while preserving keys in other namespaces.
|
🎉 This PR is included in version 2.13.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Based on #140
Closes #137
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores