feat: remove token storage, use acquireTokenSilent #40
Conversation
WalkthroughThe updates primarily enhance the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant GraphqlService
participant MsalService
participant GraphQLServer
Client->>GraphqlService: query/mutate request
GraphqlService->>MsalService: getToken()
MsalService-->>GraphqlService: Observable<AuthenticationResult>
GraphqlService-->>GraphqlService: extract token
GraphqlService->>GraphQLServer: GraphQL request with Authorization header
GraphQLServer-->>GraphqlService: Response
GraphqlService-->>Client: Response
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 (
|
Harish-osmosys
changed the title
Harish-osmosys
changed the title
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
apps/portal/src/app/app.component.ts (2)
Line range hint
1-1: Optimize imports to only include what is necessary.- import { Component, OnInit, OnDestroy, Inject } from '@angular/core'; - import { - MsalService, - MsalBroadcastService, - MSAL_GUARD_CONFIG, - MsalGuardConfiguration, - } from '@azure/msal-angular'; - import { - InteractionStatus, - RedirectRequest, - AuthenticationResult, - EventMessage, - EventType, - } from '@azure/msal-browser'; - import { Subject } from 'rxjs'; - import { filter, takeUntil } from 'rxjs/operators'; - import { environment } from '../environments/environment'; + import { Component, OnInit, OnDestroy, Inject } from '@angular/core'; + import { MsalService, MsalBroadcastService, MSAL_GUARD_CONFIG } from '@azure/msal-angular'; + import { InteractionStatus, RedirectRequest, AuthenticationResult, EventMessage, EventType } from '@azure/msal-browser'; + import { Subject, filter, takeUntil } from 'rxjs'; + import { environment } from '../environments/environment';Also applies to: 1-7, 7-14
Line range hint
1-1: Ensure that named imports are used correctly and remove any that are only used as types.Also applies to: 1-7, 7-14
apps/portal/src/app/graphql/graphql.service.ts (1)
Line range hint
1-2: Optimize imports to only include what is necessary.- import { Injectable } from '@angular/core'; - import { DocumentNode } from 'graphql'; - import { QueryRef, Apollo, MutationResult } from 'apollo-angular'; - import { Observable, throwError } from 'rxjs'; - import { ApolloQueryResult } from '@apollo/client'; - import { catchError, switchMap } from 'rxjs/operators'; - import { MsalService } from '@azure/msal-angular'; - import { AuthenticationResult } from '@azure/msal-browser'; - import { environment } from '../../environments/environment'; + import { Injectable } from '@angular/core'; + import { DocumentNode, QueryRef, Apollo, MutationResult, ApolloQueryResult } from 'graphql'; + import { Observable, throwError, catchError, switchMap } from 'rxjs'; + import { MsalService, AuthenticationResult } from '@azure/msal-angular'; + import { environment } from '../../environments/environment';Also applies to: 2-3, 3-4, 4-5, 6-7, 7-8
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/portal/src/app/app.component.ts (2 hunks)
- apps/portal/src/app/graphql/graphql.service.ts (4 hunks)
Additional Context Used
Biome (15)
apps/portal/src/app/app.component.ts (4)
34-34: Decorators are not valid here.
1-1: Some named imports are only used as types.
1-7: Some named imports are only used as types.
7-14: Some named imports are only used as types.
apps/portal/src/app/graphql/graphql.service.ts (11)
35-35: Change to an optional chain.
82-82: Unexpected any. Specify a different type.
112-112: Unexpected any. Specify a different type.
114-122: Change to an optional chain.
118-118: Unexpected any. Specify a different type.
1-2: All these imports are only used as types.
2-3: All these imports are only used as types.
3-4: Some named imports are only used as types.
4-5: All these imports are only used as types.
6-7: All these imports are only used as types.
7-8: All these imports are only used as types.
GitHub Check Runs (1)
Lint failure (3)
apps/portal/src/app/graphql/graphql.service.ts: [warning] 42-42:
Unexpected console statement
Additional comments not posted (1)
apps/portal/src/app/app.component.ts (1)
Line range hint
34-34: Ensure that decorators are used correctly in the Angular context.Verification successful
The decorators
@Componentand@Injectare used correctly in the Angular context within theapp.component.tsfile. The search results show that these decorators are used in various files, indicating consistent usage across the codebase.
@Componentis used to define Angular components.@Injectis used for dependency injection, which is a standard practice in Angular applications.No issues were found with the usage of these decorators in the provided context.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct use of decorators in Angular components. # Test: Search for incorrect decorator usage. Expect: No matches. rg --type typescript $'@Component|@Inject'Length of output: 78
Script:
#!/bin/bash # Description: Verify the correct use of decorators in Angular components. # Test: Search for incorrect decorator usage. Expect: No matches. rg '@Component|@Inject' --glob '*.ts'Length of output: 1518
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
apps/portal/src/app/graphql/graphql.service.ts (2)
Line range hint
109-119: Consider using optional chaining inisUnauthorizedErrormethod for better readability and safety.- error && - error.graphQLErrors && - error.graphQLErrors.some( + error?.graphQLErrors?.some(
Line range hint
1-8: Useimport typefor imports that are only used as types to prevent side effects and improve clarity.- import { DocumentNode } from 'graphql'; - import { QueryRef, Apollo, MutationResult } from 'apollo-angular'; - import { ApolloQueryResult } from '@apollo/client'; + import type { DocumentNode } from 'graphql'; + import type { QueryRef, Apollo, MutationResult } from 'apollo-angular'; + import type { ApolloQueryResult } from '@apollo/client';
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/portal/src/app/graphql/graphql.service.ts (4 hunks)
Additional Context Used
Biome (11)
apps/portal/src/app/graphql/graphql.service.ts (11)
35-35: Change to an optional chain.
79-79: Unexpected any. Specify a different type.
109-109: Unexpected any. Specify a different type.
111-119: Change to an optional chain.
115-115: Unexpected any. Specify a different type.
1-2: All these imports are only used as types.
2-3: All these imports are only used as types.
3-4: Some named imports are only used as types.
4-5: All these imports are only used as types.
6-7: All these imports are only used as types.
7-8: All these imports are only used as types.
Learnings (1)
apps/portal/src/app/graphql/graphql.service.ts (1)
User: Harish-osmosys PR: OsmosysSoftware/osm-transcript-summarizer#40 File: apps/portal/src/app/graphql/graphql.service.ts:0-0 Timestamp: 2024-05-29T11:00:53.632Z Learning: Harish-osmosys removed the `console.error` line from the `getToken` method in `GraphqlService` as suggested for better production code practices.
Additional comments not posted (4)
apps/portal/src/app/graphql/graphql.service.ts (4)
35-35: Consider using optional chaining for better readability and safety.- if (response && response.accessToken) { + if (response?.accessToken) {Likely invalid or redundant comment.
80-105: Refactormutatemethod to usegetTokenand handle token asynchronously.Verification successful
The
handleUnauthorizedErrormethod is indeed present and invoked within themutatemethod, confirming that unauthorized errors are being handled appropriately.
handleUnauthorizedErrormethod is defined and called within themutatemethod.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling in `mutate` method is robust and correctly redirects on unauthorized errors. # Test: Search for error handling patterns in the method. Expect: Proper redirection and error throwing. rg --type typescript $'handleUnauthorizedError' apps/portal/src/app/graphql/graphql.service.tsLength of output: 128
Script:
#!/bin/bash # Description: Verify that error handling in `mutate` method is robust and correctly redirects on unauthorized errors. # Test: Search for error handling patterns in the method. Expect: Proper redirection and error throwing. rg --type ts 'handleUnauthorizedError' apps/portal/src/app/graphql/graphql.service.tsLength of output: 213
20-43: RefactorgetTokenmethod to useacquireTokenSilentand returnObservable<string>.Verification successful
The
getTokenmethod is used within thequeryandmutatemethods of theGraphqlServiceclass, and both usages correctly handle the Observable by using thepipemethod withswitchMap.
query<T>(query: DocumentNode): Observable<ApolloQueryResult<T>>mutate<T>(mutation: DocumentNode, variables: any): Observable<MutationResult<T>>Both methods properly manage the Observable returned by
getToken.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all usages of `getToken` method handle the Observable correctly. # Test: Search for the method usage. Expect: Proper handling of Observable. rg --type typescript $'\\.getToken\\(\\)'Length of output: 74
Script:
#!/bin/bash # Description: Verify that all usages of `getToken` method handle the Observable correctly. # Test: Search for the method usage. Expect: Proper handling of Observable. rg --glob '*.ts' '\.getToken\(\)'Length of output: 195
Script:
#!/bin/bash # Description: Extract lines where `getToken` is used to verify proper Observable handling. # Extract lines where `getToken` is used in `graphql.service.ts` rg --glob '*.ts' '\.getToken\(\)' -A 5 -B 5Length of output: 1791
53-67: Refactorquerymethod to usegetTokenand handle token asynchronously.
|
@Harish-osmosys Add your findings on behaviour of token in description. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
apps/portal/src/app/graphql/graphql.service.ts (1)
Line range hint
113-127: Consider simplifying the condition inisUnauthorizedErrorfor better readability.- error && - error.graphQLErrors && - error.graphQLErrors.some( - (err: any) => - err.extensions && - err.extensions.code === 'UNAUTHENTICATED' && - err.message === 'Unauthorized', - ) + error?.graphQLErrors?.some(err => err.extensions?.code === 'UNAUTHENTICATED' && err.message === 'Unauthorized')
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/portal/src/app/graphql/graphql.service.ts (4 hunks)
Additional Context Used
Biome (11)
apps/portal/src/app/graphql/graphql.service.ts (11)
35-35: Change to an optional chain.
87-87: Unexpected any. Specify a different type.
117-117: Unexpected any. Specify a different type.
119-127: Change to an optional chain.
123-123: Unexpected any. Specify a different type.
1-2: All these imports are only used as types.
2-3: All these imports are only used as types.
3-4: Some named imports are only used as types.
4-5: All these imports are only used as types.
6-7: All these imports are only used as types.
7-8: Some named imports are only used as types.
Learnings (1)
apps/portal/src/app/graphql/graphql.service.ts (1)
User: Harish-osmosys PR: OsmosysSoftware/osm-transcript-summarizer#40 File: apps/portal/src/app/graphql/graphql.service.ts:0-0 Timestamp: 2024-05-29T11:00:53.632Z Learning: Harish-osmosys removed the `console.error` line from the `getToken` method in `GraphqlService` as suggested for better production code practices.
Additional comments not posted (3)
apps/portal/src/app/graphql/graphql.service.ts (3)
61-75: The implementation of thequerymethod looks correct and well-structured.
88-113: The implementation of themutatemethod is correct and handles file uploads appropriately.
52-54: ThehandleUnauthorizedErrormethod correctly handles logout redirection.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/api/src/modules/summary/summary.resolver.ts (1 hunks)
- apps/portal/src/app/graphql/graphql.service.ts (4 hunks)
Additional context used
Learnings (1)
apps/portal/src/app/graphql/graphql.service.ts (1)
User: Harish-osmosys PR: OsmosysSoftware/osm-transcript-summarizer#40 File: apps/portal/src/app/graphql/graphql.service.ts:0-0 Timestamp: 2024-05-29T11:00:53.632Z Learning: Harish-osmosys removed the `console.error` line from the `getToken` method in `GraphqlService` as suggested for better production code practices.
Biome
apps/api/src/modules/summary/summary.resolver.ts
[error] 18-18: Decorators are not valid here.
[error] 19-19: Decorators are not valid here.
[error] 27-27: Decorators are not valid here.
[error] 1-2: All these imports are only used as types.
[error] 3-4: All these imports are only used as types.
apps/portal/src/app/graphql/graphql.service.ts
[error] 35-35: Change to an optional chain.
[error] 86-86: Unexpected any. Specify a different type.
[error] 116-116: Unexpected any. Specify a different type.
[error] 118-126: Change to an optional chain.
[error] 122-122: Unexpected any. Specify a different type.
[error] 1-2: All these imports are only used as types.
[error] 2-3: All these imports are only used as types.
[error] 3-4: Some named imports are only used as types.
[error] 4-5: All these imports are only used as types.
[error] 6-7: All these imports are only used as types.
[error] 7-8: Some named imports are only used as types.
2 participants
API PR Checklist
Pre-requisites
PR Details
PR details have been updated as per the given format (see below)
feat: add admin login endpoint)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Description:
Token Behaviour :
Related changes:
NA
Screenshots:
NA
Query request and response:
NA
Documentation changes:
NA
Test suite output:
NA
Pending actions:
NA
Additional notes:
NA
Summary by CodeRabbit
Bug Fixes
Chores
CurrentUserdecorator in the Summary module.