CLI: Implement auth status command - #2028
Conversation
📊 Performance Test ResultsComparing ddc45d0 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
| } catch ( error ) { | ||
| throw new LoggerError( __( 'Failed to revoke token' ), error ); | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a lot of things in this file that should be "common" between CLI and App.
Add comprehensive documentation for the new `studio auth login` command including usage instructions, flow description, and important notes about token lifetime and authentication sharing between CLI and desktop app. Co-authored-by: Fredrik Rombach Ekelund <undefined@users.noreply.github.com>
|
Claude finished @fredrikekelund's task —— View job Documentation updated successfullyTasks
SummaryI've reviewed the PR changes and updated the CLI documentation in What was done:
Implementation details:
The documentation now provides clear usage examples and notes for all three authentication commands: --- | |
|
Claude finished @fredrikekelund's task —— View job Code Review Complete ✅I've completed a comprehensive code review of this PR. Overall, this is a well-implemented, clean addition with good test coverage. Here are my findings: SummaryThis PR successfully implements the Detailed Review✅ Cross-Platform Compatibility - EXCELLENT
✅ Code Quality - VERY GOODStrengths:
Minor Suggestions:
✅ Security (OWASP Top 10) - EXCELLENT
✅ Performance - EXCELLENT
✅ Testing - VERY GOODStrengths:
Suggestions:
📝 Documentation - NEEDS UPDATEAction Required: Suggested addition to CLAUDE.md after line 82: #### `studio auth logout`
Log out and clear WordPress.com authentication.
**Usage:**
```bash
node dist/cli/main.js auth logoutDescription:
|
Add documentation for `auth logout` and `auth status` commands. Co-authored-by: Fredrik Rombach Ekelund <undefined@users.noreply.github.com>
![claude[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ID: z.number(), | ||
| email: z.string().email(), | ||
| display_name: z.string(), | ||
| username: z.string(), |
There was a problem hiding this comment.
Code Reusability & Architecture - I agree with @youknowriad's earlier comment. The getUserInfo, validateAccessToken, and revokeAuthToken functions are generic WordPress.com API operations that would benefit from being in a shared location like src/lib/ or common/lib/.
This would:
- Allow the main Electron app to reuse these functions for authentication UI
- Follow the DRY principle
- Make the API operations testable from both CLI and app contexts
- Align with the architecture documented in CLAUDE.md where common code goes in
/common
Consider creating common/lib/wpcom-user-api.ts or similar for these user-related API functions.
| const logger = new Logger< LoggerAction >(); | ||
|
|
||
| logger.reportStart( LoggerAction.STATUS_CHECK, __( 'Checking authentication status…' ) ); | ||
| let token: Awaited< ReturnType< typeof getAuthToken > >; |
There was a problem hiding this comment.
Code Style - The type annotation Awaited< ReturnType< typeof getAuthToken > > is correct but creates an unused variable declaration since token is reassigned in the try block. Consider simplifying:
logger.reportStart( LoggerAction.STATUS_CHECK, __( 'Checking authentication status…' ) );
try {
const token = await getAuthToken();
const userData = await getUserInfo( token.accessToken );
logger.reportSuccess(
sprintf( __( 'Successfully authenticated with WordPress.com as `%s`' ), userData.username )
);
} catch ( error ) {
// Handle getAuthToken errors
if ( error instanceof LoggerError && error.message.includes( 'Authentication required' ) ) {
logger.reportError( new LoggerError( __( 'Authentication token is invalid or expired' ) ) );
return;
}
// Handle getUserInfo errors
// ...
}This would eliminate the uninitialized variable and flatten the nesting slightly. However, the current approach with two separate try-catch blocks does make the error handling more explicit, so this is a minor style preference.
| email: 'existing@example.com', | ||
| displayName: 'Existing User', | ||
| expiresIn: 1209600, | ||
| expirationTime: Date.now() + 1209600000, |
There was a problem hiding this comment.
Testing - Edge Case Coverage - Great test coverage overall! Consider adding one more test case for better completeness:
it( 'should report error when token is expired (past expirationTime)', async () => {
const expiredToken = {
...mockToken,
expirationTime: Date.now() - 1000, // Expired 1 second ago
};
( getAuthToken as jest.Mock ).mockRejectedValue( new Error( 'Token expired' ) );
const { runCommand } = await import( '../status' );
await runCommand();
expect( mockLogger.reportError ).toHaveBeenCalledWith( expect.any( LoggerError ) );
expect( getUserInfo ).not.toHaveBeenCalled();
} );This explicitly tests the expiration time check logic that's in getAuthToken at cli/lib/appdata.ts:152.
bcotrim
left a comment
bcotrim
left a comment
There was a problem hiding this comment.
LGTM! 👍
Tested and works as described on macOS and Windows
|
Claude finished @fredrikekelund's task —— View job Documentation updated successfullyTasks
SummaryI've reviewed the PR changes and updated the CLI documentation in What was done:
Implementation details:
The documentation now provides clear usage examples and notes for all three authentication commands: Note: The |
Co-authored-by: Fredrik Rombach Ekelund <undefined@users.noreply.github.com>
3 participants
Related issues
Proposed Changes
This PR is based on @youknowriad's work in #1958. It implements a
studio auth statuscommand that:Testing Instructions
npm run cli:buildnode dist/cli/main.js auth logoutnode dist/cli/main.js auth statusnode dist/cli/main.js auth statusPre-merge Checklist