feat(config): align init/config UX with atlassian-cli patterns#31
Merged
feat(config): align init/config UX with atlassian-cli patterns#31
Conversation
- Refactor init command to use huh forms for configuration inputs - Keep OAuth flow and browser opening logic intact - Pre-populate form fields from existing config values - Fix client ID masking to use 8 asterisks: first4********last4 - Update tests for new masking format Closes #30
Contributor
Author
Test Coverage Assessment for PR #31SummaryThis PR makes two types of changes:
Coverage Analysis
|
| Test Case | Status |
|---|---|
Empty client ID ("") |
Covered |
| Long client ID (>8 chars) | Covered |
| Short client ID (<=8 chars) | Covered |
| Boundary: exactly 8 chars | Covered |
| Boundary: exactly 9 chars | Covered |
The tests verify the new masking format (first4********last4) and the updated threshold (8 chars instead of 12). All edge cases are properly tested.
runInit Changes (initcmd/init.go) - TESTING GAP EXISTS
The existing tests cover:
- Command construction (
TestNewCommand) - Auth code extraction logic (
TestExtractAuthCode) - Flag registration
Not covered by automated tests:
- The new
huhform integration - Form value pre-population from existing config
- CLI flag priority over config values
- Form validation (client ID required)
- Default instance URL application
Mitigating factors:
- The
huhlibrary is a well-tested third-party library - we're testing their library, not ours - The business logic (config priority: CLI flag > existing config > default) is straightforward
- The core OAuth flow (callback server, token exchange, verification) is unchanged
- The PR description indicates manual testing was performed
- Testing interactive terminal UIs (TUIs) is notoriously difficult and often provides low ROI
Recommendation
Acceptable for merge. The test coverage is pragmatically appropriate:
- Critical utility function (
maskClientID) has comprehensive edge case coverage - The
huhform is a presentation layer change with manual verification - Adding unit tests for the form would require mocking the terminal, which adds complexity with minimal value
If you wanted to improve testability in the future, you could extract the config priority logic into a separate function:
func resolveConfigValue(flagValue, configValue, defaultValue string) stringThis would be easily unit-testable, but it's not a blocker for this PR given the straightforward nature of the logic.
Automated review by Claude
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.
Summary
Aligns sfdc init and config commands with UX patterns established in atlassian-cli.
Changes
first4...last4tofirst4********last4Note: salesforce-cli already had
--forceflag on config clear and no config set command, so those were already aligned.Test plan
make buildpassesmake testpassesmake lintpassessfdc initshows styled formsfdc config showmasks client ID correctlyCloses #30