CC-1296 add refresh from github button #1867
Conversation
WalkthroughThe changes introduce a new feature to sync the GitHub username in the profile settings section of an app. This enhancement includes updates to the tooltip text, a new refresh button to update the username, and corresponding backend changes to support this functionality, including a new route and user model method. Additionally, tests were adjusted to verify the new feature. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as UI Component
participant BE as Backend
participant GH as GitHub
User->>UI: Click Refresh Button
UI->>UI: Invoke refreshFromGitHub action
UI->>BE: POST /users/:id/sync-username-from-github
BE->>GH: Get new username from GitHub
GH-->>BE: Return updated username
BE-->>UI: Return updated user object with new username
UI-->>User: Display updated username in UI
Poem
TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Bundle ReportChanges will decrease total bundle size by 178.5kB ⬇️
|
Test Results 1 files ±0 1 suites ±0 9m 46s ⏱️ - 1m 38s Results for commit 9518266. ± Comparison against base commit 1168121. This pull request removes 541 and adds 539 tests. Note that renamed tests count towards both.This pull request removes 35 skipped tests and adds 35 skipped tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
app/models/user.ts (1)
213-213: Add a comprehensive description for the newsyncGitHubUsernamemethod.It's recommended to add a brief comment describing what this method does, especially since it involves synchronization with an external system (GitHub), which could have implications on the user's data.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- app/components/settings/profile-page/username-section.hbs (1 hunks)
- app/components/settings/profile-page/username-section.ts (2 hunks)
- app/models/user.ts (2 hunks)
- mirage/handlers/users.js (1 hunks)
- tests/acceptance/settings-page/profile-test.js (2 hunks)
- tests/pages/settings/profile-page.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/components/settings/profile-page/username-section.hbs
Additional context used
Biome
mirage/handlers/users.js
[error] 4-6: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 13-15: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 8-16: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 20-26: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 28-33: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.tests/acceptance/settings-page/profile-test.js
[error] 9-66: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.app/models/user.ts
[error] 104-106: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 156-158: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 207-207: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 225-227: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 241-243: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (4)
tests/pages/settings/profile-page.ts (1)
16-18: The new button element for refreshing GitHub username is correctly implemented.app/components/settings/profile-page/username-section.ts (2)
2-6: The necessary imports forAuthenticatorService,action,service, andtrackedare correctly added.
17-17: Theauthenticatorservice is properly declared and theisSyncingGitHubUsernametracking variable is correctly implemented.Also applies to: 19-19
app/models/user.ts (1)
247-254: Refactor to remove unnecessary else clause as per linting suggestions.The
elseclause in thesyncFeatureFlagsmethod can be omitted for cleaner and more efficient code, as suggested by the static analysis tool.this.store.pushPayload(response); - else { - return null; - }Likely invalid or redundant comment.
app/models/user.ts
Outdated
| UserModel.prototype.syncGitHubUsername = memberAction({ | ||
| path: 'sync-github-username', | ||
| type: 'post', | ||
|
|
||
| after(response) { | ||
| this.store.pushPayload(response); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Ensure proper error handling in the syncGitHubUsername method.
The after hook in the syncGitHubUsername method pushes the payload to the store but does not handle potential errors that could arise from the API call. Consider adding error handling logic to manage such cases effectively.
+ try {
this.store.pushPayload(response);
+ } catch (error) {
+ console.error('Failed to sync GitHub username:', error);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| UserModel.prototype.syncGitHubUsername = memberAction({ | |
| path: 'sync-github-username', | |
| type: 'post', | |
| after(response) { | |
| this.store.pushPayload(response); | |
| }, | |
| }); | |
| UserModel.prototype.syncGitHubUsername = memberAction({ | |
| path: 'sync-github-username', | |
| type: 'post', | |
| after(response) { | |
| try { | |
| this.store.pushPayload(response); | |
| } catch (error) { | |
| console.error('Failed to sync GitHub username:', error); | |
| } | |
| }, | |
| }); |
| @action | ||
| async refreshFromGitHub() { | ||
| this.isSyncingGitHubUsername = true; | ||
| await this.authenticator.currentUser?.syncGitHubUsername(null); | ||
| this.isSyncingGitHubUsername = false; | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The refreshFromGitHub method in app/components/settings/profile-page/username-section.ts does not include any error handling mechanisms such as try-catch blocks. It is important to implement error handling to manage potential failures during the GitHub username synchronization process.
- File:
app/components/settings/profile-page/username-section.ts - Lines: 21-26
- Method:
refreshFromGitHub
Analysis chain
Ensure that error handling is implemented in the refreshFromGitHub action method to manage potential failures during the GitHub username synchronization process.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in the `refreshFromGitHub` method.
ast-grep --lang typescript --pattern $'class UsernameSectionComponent {
$$$
refreshFromGitHub($_, $_) {
$$$
}
$$$
}'
Length of output: 129
Script:
#!/bin/bash
# Step 1: Search for the `UsernameSectionComponent` class definition in the codebase.
ast-grep --lang typescript --pattern 'class UsernameSectionComponent { $$$ }'
# Step 2: If the class is found, search for the `refreshFromGitHub` method within the located class.
ast-grep --lang typescript --pattern $'class UsernameSectionComponent {
$$$
async refreshFromGitHub() {
$$$
}
$$$
}'
Length of output: 209
Script:
#!/bin/bash
# Step 1: Search for all class definitions in the codebase to identify the correct class.
ast-grep --lang typescript --pattern 'class $_ { $$$ }'
# Step 2: If the class is found, search for the `refreshFromGitHub` method within the located class.
ast-grep --lang typescript --pattern $'class $_ {
$$$
async refreshFromGitHub() {
$$$
}
$$$
}'
Length of output: 37058
Script:
#!/bin/bash
# Step 1: Search for the `UsernameSectionComponent` class definition in the codebase.
rg 'class UsernameSectionComponent'
# Step 2: If the class is found, search for the `refreshFromGitHub` method within the located class.
rg 'async refreshFromGitHub'
Length of output: 289
Script:
#!/bin/bash
# Step 3: Check if error handling is implemented in the `refreshFromGitHub` method.
ast-grep --lang typescript --pattern $'class UsernameSectionComponent {
$$$
async refreshFromGitHub() {
try {
$$$
} catch (error) {
$$$
}
}
$$$
}' app/components/settings/profile-page/username-section.ts
Length of output: 236
Script:
#!/bin/bash
# Display the implementation of the `refreshFromGitHub` method for manual inspection.
sed -n '/async refreshFromGitHub/,/}/p' app/components/settings/profile-page/username-section.ts
Length of output: 283
mirage/handlers/users.js
Outdated
| server.post('/users/:id/sync-github-username', function (schema, request) { | ||
| let user = schema.users.find(request.params.id); | ||
| user.update({ username: 'updated-username' }); | ||
|
|
||
| return user; | ||
| }); |
There was a problem hiding this comment.
The implementation of the new POST endpoint for syncing the GitHub username is correct. However, consider using an arrow function for consistency with modern JavaScript practices.
- server.post('/users/:id/sync-github-username', function (schema, request) {
+ server.post('/users/:id/sync-github-username', (schema, request) => {
let user = schema.users.find(request.params.id);
user.update({ username: 'updated-username' });
return user;
});Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| server.post('/users/:id/sync-github-username', function (schema, request) { | |
| let user = schema.users.find(request.params.id); | |
| user.update({ username: 'updated-username' }); | |
| return user; | |
| }); | |
| server.post('/users/:id/sync-github-username', (schema, request) => { | |
| let user = schema.users.find(request.params.id); | |
| user.update({ username: 'updated-username' }); | |
| return user; | |
| }); |
Tools
Biome
[error] 28-33: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
| test('can refresh github username', async function (assert) { | ||
| testScenario(this.server); | ||
| signInAsSubscriber(this.owner, this.server); | ||
|
|
||
| let currentUser = this.server.schema.users.first(); | ||
|
|
||
| assert.strictEqual(currentUser.username, 'rohitpaulk'); | ||
|
|
||
| await profilePage.visit(); | ||
| await profilePage.refreshFromGitHubButton.click(); | ||
| await settled(); | ||
|
|
||
| assert.strictEqual(currentUser.reload().username, 'updated-username'); | ||
| }); |
There was a problem hiding this comment.
The new test for refreshing the GitHub username is correctly implemented. However, consider using an arrow function for the test definition to align with modern JavaScript best practices.
- test('can refresh github username', async function (assert) {
+ test('can refresh github username', async (assert) => {
testScenario(this.server);
signInAsSubscriber(this.owner, this.server);
let currentUser = this.server.schema.users.first();
assert.strictEqual(currentUser.username, 'rohitpaulk');
await profilePage.visit();
await profilePage.refreshFromGitHubButton.click();
await settled();
assert.strictEqual(currentUser.reload().username, 'updated-username');
});Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('can refresh github username', async function (assert) { | |
| testScenario(this.server); | |
| signInAsSubscriber(this.owner, this.server); | |
| let currentUser = this.server.schema.users.first(); | |
| assert.strictEqual(currentUser.username, 'rohitpaulk'); | |
| await profilePage.visit(); | |
| await profilePage.refreshFromGitHubButton.click(); | |
| await settled(); | |
| assert.strictEqual(currentUser.reload().username, 'updated-username'); | |
| }); | |
| test('can refresh github username', async (assert) => { | |
| testScenario(this.server); | |
| signInAsSubscriber(this.owner, this.server); | |
| let currentUser = this.server.schema.users.first(); | |
| assert.strictEqual(currentUser.username, 'rohitpaulk'); | |
| await profilePage.visit(); | |
| await profilePage.refreshFromGitHubButton.click(); | |
| await settled(); | |
| assert.strictEqual(currentUser.reload().username, 'updated-username'); | |
| }); |
| @action | ||
| async refreshFromGitHub() { | ||
| this.isSyncingGitHubUsername = true; | ||
| await this.authenticator.currentUser?.syncGitHubUsername(null); |
There was a problem hiding this comment.
do you think it's better to keep the 'refresh from github' name consistent everywhere
There was a problem hiding this comment.
sync is fine, we use it across many core actions so it's consistent with those. The actual text for the button might change over time (based on what makes most sense to a user), so not super important to keep that in sync with everything else.
Would recommend renaming to syncUsernameFromGitHub though, to avoid confusion around whether we're actually updating the username, or only user#githubUsername.
|
i feel the spinner is already good way to communicate progress and completion. right now, we're not handling the happy path. do you think it's better to have an alert saying the user should contact us? also, for the happy path, do you think it's better to add an alert saying the change was successful? |
|
|
||
| await profilePage.visit(); | ||
| await profilePage.refreshFromGitHubButton.click(); | ||
| await settled(); |
There was a problem hiding this comment.
This shouldn't be required, click() should already be calling await settled(). @libmartinito are you sure that this fails without a call to settled()?
There was a problem hiding this comment.
yep, works without the additional call to settled
| {{/if}} | ||
| </div> | ||
|
|
||
| {{!-- TODO: Bring this back once implemented }} | ||
| {{!-- <TertiaryButtonWithSpinner @size="extra-small" class="mt-4" @shouldShowSpinner={{false}}> | ||
| <TertiaryButtonWithSpinner |
There was a problem hiding this comment.
If a user has anonymous mode enabled, should we show this?
There was a problem hiding this comment.
If a user has anonymous mode enabled and clicked on this, what would happen?
There was a problem hiding this comment.
makes sense. user shouldn't be able to change github username if anonymous
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- app/components/settings/profile-page/username-section.hbs (1 hunks)
- app/components/settings/profile-page/username-section.ts (2 hunks)
- app/models/user.ts (2 hunks)
- mirage/handlers/users.js (1 hunks)
- tests/acceptance/settings-page/profile-test.js (1 hunks)
Files skipped from review due to trivial changes (1)
- app/components/settings/profile-page/username-section.ts
Files skipped from review as they are similar to previous changes (4)
- app/components/settings/profile-page/username-section.hbs
- app/models/user.ts
- mirage/handlers/users.js
- tests/acceptance/settings-page/profile-test.js
| @size="extra-small" | ||
| class="mt-4" | ||
| @shouldShowSpinner={{this.isSyncingUsernameFromGitHub}} | ||
| @isDisabled={{this.currentUser.hasAnonymousModeEnabled}} |
There was a problem hiding this comment.
@libmartinito let's add a tooltip when this button is disabled:
Your profile has anonymous mode enabled. Your GitHub username is not visible to others.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/components/settings/profile-page/username-section.hbs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/components/settings/profile-page/username-section.hbs
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
New Features
Bug Fixes
Tests