Skip to content

feat: add alias completion in command mode#76

Merged
yimsk merged 16 commits intodevelopfrom
feat/issue-70
Jan 2, 2026
Merged

feat: add alias completion in command mode#76
yimsk merged 16 commits intodevelopfrom
feat/issue-70

Conversation

@yimsk
Copy link
Copy Markdown
Contributor

@yimsk yimsk commented Jan 2, 2026

Summary

  • Add alias suggestions in command mode (e.g., typing cf suggests cfn, cf)
  • Cache GetAliases() and GetAliasesForService() with sync.Once for performance
  • Refactor command parsing to use strings.CutPrefix/CutSuffix idiom
  • Extract fuzzy match logic to shared match.go module

Changes

  • internal/view/match.go - New: fuzzyMatch, matchNamesWithFallback
  • internal/view/command_input.go - Alias completion, CutPrefix refactor
  • internal/registry/registry.go - Add GetAliases(), caching
  • custom/ecs/tasks/render.go - CutPrefix refactor

Closes #70

@claude

This comment was marked as outdated.

@claude

This comment was marked as resolved.

@claude

This comment was marked as resolved.

@claude

This comment was marked as resolved.

@claude

This comment was marked as outdated.

@claude
Copy link
Copy Markdown

claude Bot commented Jan 2, 2026

Code Review Summary

This PR successfully adds alias completion to command mode and implements several quality refactorings. The implementation is well-structured with excellent test coverage. Below are my findings:


✅ Strengths

Code Quality & Best Practices

  • Excellent refactoring patterns: The use of strings.CutPrefix/CutSuffix throughout is idiomatic Go 1.20+ code that's cleaner than the previous HasPrefix/TrimPrefix pattern
  • Good separation of concerns: Extracting fuzzy match logic to internal/view/match.go creates a reusable module with clear responsibilities
  • Consistent naming: Functions and variables follow Go conventions with clear, descriptive names
  • Clean command parsing: The refactored command parsing in command_input.go is more readable and maintainable

Performance

  • Smart caching strategy: Using sync.Once for GetAliases() and GetAliasesForService() is the right approach since aliases are immutable after initialization
  • Defensive copies: Returning slices.Clone() from cached results (registry.go:309, 327) prevents external mutation - excellent defensive programming
  • Efficient algorithms: Prefix matching with fuzzy fallback provides optimal UX without unnecessary computation

Test Coverage

  • Comprehensive test suite: New functionality has thorough test coverage with diverse test cases
  • Concurrency testing: Registry cache tests include concurrent access scenarios (registry_test.go:295-329)
  • Edge cases covered: Empty inputs, case insensitivity, self-referential aliases, and boundary conditions all tested

🔍 Observations & Minor Suggestions

1. Potential Race Condition in Cache Initialization (Low Priority)

Location: internal/registry/registry.go:293-309

The current implementation has a subtle issue: sync.Once.Do() guards initialization, but the RLock inside the callback is taken after other goroutines might have already passed the Once check.

r.serviceAliasesOnce.Do(func() {
    r.mu.RLock()  // Lock taken inside Once callback
    defer r.mu.RUnlock()
    // ... build cache
})
return slices.Clone(r.serviceAliasesCache[service])  // Access without lock

Issue: Between Once.Do() returning and the final map access, r.serviceAliasesCache could theoretically be accessed before it's fully initialized if multiple goroutines hit this simultaneously on first call.

Recommendation: Move the RLock outside or restructure:

r.serviceAliasesOnce.Do(func() {
    r.mu.RLock()
    defer r.mu.RUnlock()
    // build cache
})
r.mu.RLock()  // Ensure cache is visible
defer r.mu.RUnlock()
return slices.Clone(r.serviceAliasesCache[service])

However, in practice this is likely safe due to Go's memory model with sync.Once, but it's worth considering for absolute correctness.

2. Case Sensitivity Consistency

Location: internal/view/match.go:9-11

The fuzzyMatch function converts both str and pattern to lowercase (lines 10-11), which is correct. However, matchNamesWithFallback also lowercases the pattern (line 30), causing it to be lowercased twice when fuzzy matching is used.

Impact: No functional issue (lowercasing twice is idempotent), but slightly inefficient.

Suggestion: Minor optimization to lowercase pattern once at the start of matchNamesWithFallback.

3. Error Message Improvement

Location: internal/view/command_input.go:253

Nice improvement quoting the profile name in error messages! This makes invalid profile names clearer to users.

4. Alphabetical Sorting Enhancement

Location: internal/view/command_input.go:469

Good addition of slices.Sort(suggestions) to ensure consistent ordering of service and alias suggestions.


🔒 Security

No security concerns identified. The changes:

  • Don't introduce command injection risks
  • Don't expose sensitive data
  • Don't modify authentication/authorization logic
  • Handle user input safely with existing validation

⚡ Performance Considerations

Positive impacts:

  • Caching eliminates repeated alias map iterations
  • Prefix matching avoids fuzzy matching when possible
  • Defensive copies prevent accidental O(n) mutations

No performance regressions detected. The caching strategy is particularly well-suited for this use case.


🧪 Testing

Excellent test coverage with:

  • Unit tests for all new functions
  • Concurrency tests for cache safety
  • Edge case coverage (empty inputs, case insensitivity, self-referential aliases)
  • Integration tests for command completion

Test quality observations:

  • Good use of table-driven tests
  • Clear test names describing scenarios
  • Proper use of subtests for isolation

📝 Documentation

The PR description clearly explains the changes. Code comments are appropriate. The commit history is clean and well-organized.


Final Verdict

LGTM with minor observations. This is high-quality work that:

  • ✅ Implements the requested feature correctly
  • ✅ Improves code maintainability through refactoring
  • ✅ Includes comprehensive tests
  • ✅ Follows Go best practices
  • ✅ Has no significant bugs or security issues

The one potential race condition mentioned is very low risk given sync.Once semantics, but worth considering if you're aiming for absolute correctness.

Great job on this PR! The fuzzy matching with prefix fallback is a nice UX touch, and the caching optimization is well-implemented.

@yimsk yimsk merged commit 017ebd4 into develop Jan 2, 2026
5 checks passed
@yimsk yimsk deleted the feat/issue-70 branch January 2, 2026 10:53
@yimsk yimsk mentioned this pull request Jan 2, 2026
yimsk added a commit that referenced this pull request Jan 2, 2026
* Align naming conventions with AWS CLI (#71)

* Align registry names with AWS CLI conventions

Rename 7 services to match AWS CLI naming:
- computeoptimizer → compute-optimizer
- cognito → cognito-idp
- config → configservice
- costexplorer → ce
- eventbridge → events
- macie → macie2
- sfn → stepfunctions

Old names preserved as aliases for backward compatibility.

Closes #65

* Fix pseudo-ARN prefix to match registry service name (ce)

* Add comment explaining pseudo-ARN format for Cost Explorer

* Rename directories to align with AWS CLI conventions

- Service dirs: costexplorer→ce, eventbridge→events, sfn→stepfunctions,
  cognito→cognito-idp, config→configservice, macie→macie2,
  computeoptimizer→compute-optimizer, servicequotas→service-quotas,
  licensemanager→license-manager, networkfirewall→network-firewall
- Resource dirs: hyphenate compound names (e.g., loggroups→log-groups)
- Update imports and ARN mappings

Closes #65

* Rename bedrock dirs to kebab-case for AWS CLI consistency

* Fix DAO error strings and deduplicate client helpers

* Fix error strings and deduplicate client helpers

* Fix package names to match directory names (events, stepfunctions)

* Add develop branch to CI triggers

* feat: propagate ALL_PROXY to HTTP_PROXY/HTTPS_PROXY (#74)

* feat: support ALL_PROXY environment variable (#69)

* feat: propagate ALL_PROXY to HTTP_PROXY and configure NO_PROXY

Extend proxy support based on issue #69 feedback:
- Propagate ALL_PROXY to both HTTP_PROXY and HTTPS_PROXY
- Auto-configure NO_PROXY for AWS credential endpoints:
  - 169.254.169.254 (EC2 IMDS)
  - 169.254.170.2 (ECS Task Role)
  - 169.254.170.23 (EKS Pod Identity)
- Preserve existing NO_PROXY entries, only add missing endpoints

Refs #69, #67

* fix: default NO_PROXY to IMDS only and remove proxy value from logs

- NO_PROXY now only includes EC2 IMDS (169.254.169.254) by default
- ECS/EKS endpoints can be added via config (TODO #67)
- Remove proxy URL from log output to avoid credential exposure

* refactor: simplify splitNoProxy and use tagged switch in parseFlags

* refactor: separate NO_PROXY config from ALL_PROXY propagation

configureNoProxy() now runs independently when any proxy is set,
not tied to ALL_PROXY propagation. Cleaner separation of concerns.

* refactor: remove lowercase env var support for simplicity

* refactor: remove NO_PROXY auto-configuration

* refactor: skip proxy propagation for --help/--version and cleanup test

* test: add test case for lowercase all_proxy not supported

* feat: add alias completion in command mode (#76)

* feat: add alias completion in command mode (#70)

Include aliases in GetSuggestions() so :cost+Tab suggests cost-explorer.
Self-referential aliases (sfn→sfn) excluded from suggestions.

* refactor: use prefix+fuzzy for diff completion

* refactor: use CutPrefix and cache GetAliases

* refactor: extract match functions to shared module

- Move fuzzyMatch and matchNamesWithFallback to match.go
- Make :tag/:tags clear behavior explicit
- Sort suggestions alphabetically (services + aliases)

* refactor: cache GetAliasesForService and sort match results

* refactor: unify command parser pattern for sort and login

* test: add cache concurrency tests and improve comments

* refactor: use CutPrefix in ecs tasks render

* refactor: use CutSuffix in parseNumericValue

* fix: quote profile name in error message for clarity

* fix: lowercase pattern in fuzzyMatch for case insensitivity

* chore: remove .claude symlink

* chore: add .claude to gitignore

* fix: lowercase pattern in matchNamesWithFallback prefix check

* fix: return defensive copy from alias cache methods
@yimsk yimsk mentioned this pull request Jan 2, 2026
yimsk added a commit that referenced this pull request Jan 2, 2026
* Align naming conventions with AWS CLI (#71)

* Align registry names with AWS CLI conventions

Rename 7 services to match AWS CLI naming:
- computeoptimizer → compute-optimizer
- cognito → cognito-idp
- config → configservice
- costexplorer → ce
- eventbridge → events
- macie → macie2
- sfn → stepfunctions

Old names preserved as aliases for backward compatibility.

Closes #65

* Fix pseudo-ARN prefix to match registry service name (ce)

* Add comment explaining pseudo-ARN format for Cost Explorer

* Rename directories to align with AWS CLI conventions

- Service dirs: costexplorer→ce, eventbridge→events, sfn→stepfunctions,
  cognito→cognito-idp, config→configservice, macie→macie2,
  computeoptimizer→compute-optimizer, servicequotas→service-quotas,
  licensemanager→license-manager, networkfirewall→network-firewall
- Resource dirs: hyphenate compound names (e.g., loggroups→log-groups)
- Update imports and ARN mappings

Closes #65

* Rename bedrock dirs to kebab-case for AWS CLI consistency

* Fix DAO error strings and deduplicate client helpers

* Fix error strings and deduplicate client helpers

* Fix package names to match directory names (events, stepfunctions)

* Add develop branch to CI triggers

* feat: propagate ALL_PROXY to HTTP_PROXY/HTTPS_PROXY (#74)

* feat: support ALL_PROXY environment variable (#69)

* feat: propagate ALL_PROXY to HTTP_PROXY and configure NO_PROXY

Extend proxy support based on issue #69 feedback:
- Propagate ALL_PROXY to both HTTP_PROXY and HTTPS_PROXY
- Auto-configure NO_PROXY for AWS credential endpoints:
  - 169.254.169.254 (EC2 IMDS)
  - 169.254.170.2 (ECS Task Role)
  - 169.254.170.23 (EKS Pod Identity)
- Preserve existing NO_PROXY entries, only add missing endpoints

Refs #69, #67

* fix: default NO_PROXY to IMDS only and remove proxy value from logs

- NO_PROXY now only includes EC2 IMDS (169.254.169.254) by default
- ECS/EKS endpoints can be added via config (TODO #67)
- Remove proxy URL from log output to avoid credential exposure

* refactor: simplify splitNoProxy and use tagged switch in parseFlags

* refactor: separate NO_PROXY config from ALL_PROXY propagation

configureNoProxy() now runs independently when any proxy is set,
not tied to ALL_PROXY propagation. Cleaner separation of concerns.

* refactor: remove lowercase env var support for simplicity

* refactor: remove NO_PROXY auto-configuration

* refactor: skip proxy propagation for --help/--version and cleanup test

* test: add test case for lowercase all_proxy not supported

* feat: add alias completion in command mode (#76)

* feat: add alias completion in command mode (#70)

Include aliases in GetSuggestions() so :cost+Tab suggests cost-explorer.
Self-referential aliases (sfn→sfn) excluded from suggestions.

* refactor: use prefix+fuzzy for diff completion

* refactor: use CutPrefix and cache GetAliases

* refactor: extract match functions to shared module

- Move fuzzyMatch and matchNamesWithFallback to match.go
- Make :tag/:tags clear behavior explicit
- Sort suggestions alphabetically (services + aliases)

* refactor: cache GetAliasesForService and sort match results

* refactor: unify command parser pattern for sort and login

* test: add cache concurrency tests and improve comments

* refactor: use CutPrefix in ecs tasks render

* refactor: use CutSuffix in parseNumericValue

* fix: quote profile name in error message for clarity

* fix: lowercase pattern in fuzzyMatch for case insensitivity

* chore: remove .claude symlink

* chore: add .claude to gitignore

* fix: lowercase pattern in matchNamesWithFallback prefix check

* fix: return defensive copy from alias cache methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant