feat(admin-ui): Migrate Auth Server module to unified logger (#2870)#2871
Open
faisalsiddique4400 wants to merge 1 commit into
Open
feat(admin-ui): Migrate Auth Server module to unified logger (#2870)#2871faisalsiddique4400 wants to merge 1 commit into
faisalsiddique4400 wants to merge 1 commit into
Conversation
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
Contributor
📝 WalkthroughWalkthroughThis PR introduces a unified shared logger utility and systematically migrates all ChangesUnified logger migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mo-auto
added
comp-admin-ui
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@admin-ui/app/utils/logger.ts`:
- Line 37: Update the JSDoc example for logger.error to correctly state that the
'prod' environment outputs to the production console only (not "prod + dev
console"); change the comment on the example line using logger.error so it
reflects the emit logic (see emit handling around 'emit' and logger.error) and
accurately reads that 'prod' targets production console only.
In
`@admin-ui/plugins/auth-server/components/OidcClients/components/ClientCibaParUmaPanel.tsx`:
- Around line 155-159: The fire-and-forget catch in ClientCibaParUmaPanel.tsx
duplicates logging because deleteUmaResource already logs failures; remove the
duplicate logger.error from the .catch handler (or replace the .catch body with
an empty swallow) so the promise is still fire-and-forget but not re-logged;
locate the call using deleteUmaResource(String(selectedUMA.id)) and the
logger.error('dev', 'UMA resource deletion failed:', error) invocation and
remove or clear that logger call, leaving the inline comment if you want to keep
the explanation.
In `@admin-ui/plugins/auth-server/components/Ssa/hooks/useSsaApi.ts`:
- Line 94: In useSsaApi.ts update the logger call in the SSA creation error path
to use the dev-only audience: replace logger.error('both', 'Failed to create
SSA: ' + (error instanceof Error ? error.message : String(error))) with a call
that passes 'dev' as the first argument so the message remains gated to
development logging; locate this inside the create SSA error handling in the
useSsaApi hook and change the audience token from 'both' to 'dev'.
- Line 65: Change the logger audience from 'both' to 'dev' in the error call
inside useSsaApi (the logger.error(...) line) so the SSA fetch failure remains
development-only; locate the logger.error call in
admin-ui/plugins/auth-server/components/Ssa/hooks/useSsaApi.ts and replace the
first argument 'both' with 'dev' to preserve the original isDevelopment-gated
logging behavior.
- Line 123: In useSsaApi.ts update the logger call inside the SSA fetch error
handling (the logger.error(...) invocation in the useSsaApi hook) to use the
'dev' audience instead of 'both' so the message remains development-only; locate
the logger.error('both', 'Failed to fetch SSA JWT: ' + ...) line and change the
first argument to 'dev' preserving the rest of the message and error formatting.
In `@admin-ui/plugins/auth-server/services/jsonPropertiesService.ts`:
- Line 37: The error logs in jsonPropertiesService.ts currently call
logger.error('both', ...) which expands audience from dev-only to both
environments; change both occurrences to logger.error('dev', ...) so detailed
backend error messages remain dev-only (search for the logger.error('both',
'Failed to fetch auth server JSON properties: ' + resolveApiErrorMessage(e as
Error)) and the second similar call and replace the 'both' audience with 'dev').
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 225da67a-8eaf-479e-8299-42c90108c8d6
📒 Files selected for processing (42)
admin-ui/app/utils/logger.tsadmin-ui/app/utils/types/LoggerTypes.tsadmin-ui/app/utils/types/index.tsadmin-ui/plugins/auth-server/__tests__/api/setup.tsadmin-ui/plugins/auth-server/components/AuthServerProperties/components/AuthServerPropertiesPage.tsxadmin-ui/plugins/auth-server/components/AuthServerProperties/hooks/useAuthServerScripts.tsadmin-ui/plugins/auth-server/components/Authentication/Acrs/AcrsEditPage.tsxadmin-ui/plugins/auth-server/components/Authentication/AgamaFlows/AgamaFlows.tsxadmin-ui/plugins/auth-server/components/Authentication/AgamaFlows/AgamaProjectConfigModal.tsxadmin-ui/plugins/auth-server/components/Authentication/Aliases/Aliases.tsxadmin-ui/plugins/auth-server/components/Authentication/DefaultAcr/DefaultAcr.tsxadmin-ui/plugins/auth-server/components/ConfigApiProperties/components/ConfigApiPropertiesForm.tsxadmin-ui/plugins/auth-server/components/ConfigApiProperties/components/ConfigApiPropertiesPage.tsxadmin-ui/plugins/auth-server/components/ConfigApiProperties/utils/valueUtils.tsadmin-ui/plugins/auth-server/components/JsonViewer/components/JsonViewerDialog.tsxadmin-ui/plugins/auth-server/components/Keys/hooks/useJwkApi.tsadmin-ui/plugins/auth-server/components/Logging/components/LoggingPage.tsxadmin-ui/plugins/auth-server/components/Logging/hooks/useLoggingApi.tsadmin-ui/plugins/auth-server/components/OidcClients/__tests__/hooks/useClientUmaResources.test.tsadmin-ui/plugins/auth-server/components/OidcClients/components/ClientAddPage.tsxadmin-ui/plugins/auth-server/components/OidcClients/components/ClientCibaParUmaPanel.tsxadmin-ui/plugins/auth-server/components/OidcClients/components/ClientEditPage.tsxadmin-ui/plugins/auth-server/components/OidcClients/components/ClientListPage.tsxadmin-ui/plugins/auth-server/components/OidcClients/helper/utils.tsadmin-ui/plugins/auth-server/components/OidcClients/hooks/useClientTokens.tsadmin-ui/plugins/auth-server/components/OidcClients/hooks/useClientUmaResources.tsadmin-ui/plugins/auth-server/components/OidcClients/hooks/useCreateClient.tsadmin-ui/plugins/auth-server/components/OidcClients/hooks/useDeleteClient.tsadmin-ui/plugins/auth-server/components/OidcClients/hooks/useUpdateClient.tsadmin-ui/plugins/auth-server/components/Scopes/components/ScopeAddPage.tsxadmin-ui/plugins/auth-server/components/Scopes/components/ScopeEditPage.tsxadmin-ui/plugins/auth-server/components/Scopes/components/ScopeListPage.tsxadmin-ui/plugins/auth-server/components/Scopes/hooks/useScopeMutations.tsadmin-ui/plugins/auth-server/components/Sessions/components/SessionListPage.tsxadmin-ui/plugins/auth-server/components/Sessions/hooks/useSessionMutations.tsadmin-ui/plugins/auth-server/components/Ssa/components/SsaAddPage.tsxadmin-ui/plugins/auth-server/components/Ssa/components/SsaListPage.tsxadmin-ui/plugins/auth-server/components/Ssa/helper/utils.tsadmin-ui/plugins/auth-server/components/Ssa/hooks/useSsaApi.tsadmin-ui/plugins/auth-server/components/Ssa/hooks/useSsaMutations.tsadmin-ui/plugins/auth-server/services/jsonPropertiesService.tsadmin-ui/plugins/auth-server/utils/sessionExpiredRedirect.ts
| * every environment. | ||
| * | ||
| * @example | ||
| * logger.error('prod', 'Cedarling init failed:', message) // prod + dev console |
Contributor
There was a problem hiding this comment.
Correct the JSDoc example comment for 'prod' environment.
The comment states // prod + dev console, but according to the emit logic (lines 23-25), 'prod' logs are suppressed in development and only appear in production. The comment should reflect that 'prod' targets the production console only, not both.
📝 Suggested correction
- logger.error('prod', 'Cedarling init failed:', message) // prod + dev console
+ logger.error('prod', 'Cedarling init failed:', message) // production only📝 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.
Suggested change
| * logger.error('prod', 'Cedarling init failed:', message) // prod + dev console | |
| * logger.error('prod', 'Cedarling init failed:', message) // production only |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@admin-ui/app/utils/logger.ts` at line 37, Update the JSDoc example for
logger.error to correctly state that the 'prod' environment outputs to the
production console only (not "prod + dev console"); change the comment on the
example line using logger.error so it reflects the emit logic (see emit handling
around 'emit' and logger.error) and accurately reads that 'prod' targets
production console only.
Comment on lines
+155
to
+159
| // deleteUmaResource already logs the failure; swallow the re-thrown rejection | ||
| // here (fire-and-forget) so it doesn't surface as an unhandled promise. | ||
| deleteUmaResource(String(selectedUMA.id)).catch((error) => { | ||
| logger.error('dev', 'UMA resource deletion failed:', error) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove duplicate UMA deletion logging in the fire-and-forget catch.
The inline comment says deleteUmaResource already logs failures, but this catch logs again. That creates duplicate dev logs and conflicting intent.
Proposed adjustment
- // deleteUmaResource already logs the failure; swallow the re-thrown rejection
- // here (fire-and-forget) so it doesn't surface as an unhandled promise.
- deleteUmaResource(String(selectedUMA.id)).catch((error) => {
- logger.error('dev', 'UMA resource deletion failed:', error)
- })
+ // deleteUmaResource already logs the failure; swallow the re-thrown rejection
+ // here (fire-and-forget) so it doesn't surface as an unhandled promise.
+ deleteUmaResource(String(selectedUMA.id)).catch(() => undefined)📝 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.
Suggested change
| // deleteUmaResource already logs the failure; swallow the re-thrown rejection | |
| // here (fire-and-forget) so it doesn't surface as an unhandled promise. | |
| deleteUmaResource(String(selectedUMA.id)).catch((error) => { | |
| logger.error('dev', 'UMA resource deletion failed:', error) | |
| }) | |
| // deleteUmaResource already logs the failure; swallow the re-thrown rejection | |
| // here (fire-and-forget) so it doesn't surface as an unhandled promise. | |
| deleteUmaResource(String(selectedUMA.id)).catch(() => undefined) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@admin-ui/plugins/auth-server/components/OidcClients/components/ClientCibaParUmaPanel.tsx`
around lines 155 - 159, The fire-and-forget catch in ClientCibaParUmaPanel.tsx
duplicates logging because deleteUmaResource already logs failures; remove the
duplicate logger.error from the .catch handler (or replace the .catch body with
an empty swallow) so the promise is still fire-and-forget but not re-logged;
locate the call using deleteUmaResource(String(selectedUMA.id)) and the
logger.error('dev', 'UMA resource deletion failed:', error) invocation and
remove or clear that logger call, leaving the inline comment if you want to keep
the explanation.
|
|
||
| return await handleResponse<SsaData[]>(response) | ||
| } catch (error) { | ||
| logger.error('both', 'Failed to fetch SSAs: ' + (error instanceof Error ? error.message : String(error))) |
Contributor
There was a problem hiding this comment.
Change 'both' to 'dev' to preserve dev-only logging behavior.
This logger call uses 'both' as the audience, which logs in both development and production environments. However, the PR objectives state that "Development-only logging is preserved by mapping previous calls to logger.('dev', ...), which remains gated by isDevelopment, maintaining existing runtime and debugging behavior." The original devLogger was gated by isDevelopment and only logged in development.
Using 'both' changes the logging behavior from dev-only to production, contradicting the stated objective of preserving behavior.
🔧 Proposed fix
- logger.error('both', 'Failed to fetch SSAs: ' + (error instanceof Error ? error.message : String(error)))
+ logger.error('dev', 'Failed to fetch SSAs: ' + (error instanceof Error ? error.message : String(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.
Suggested change
| logger.error('both', 'Failed to fetch SSAs: ' + (error instanceof Error ? error.message : String(error))) | |
| logger.error('dev', 'Failed to fetch SSAs: ' + (error instanceof Error ? error.message : String(error))) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@admin-ui/plugins/auth-server/components/Ssa/hooks/useSsaApi.ts` at line 65,
Change the logger audience from 'both' to 'dev' in the error call inside
useSsaApi (the logger.error(...) line) so the SSA fetch failure remains
development-only; locate the logger.error call in
admin-ui/plugins/auth-server/components/Ssa/hooks/useSsaApi.ts and replace the
first argument 'both' with 'dev' to preserve the original isDevelopment-gated
logging behavior.
|
|
||
| return await handleResponse<SsaData>(response) | ||
| } catch (error) { | ||
| logger.error('both', 'Failed to create SSA: ' + (error instanceof Error ? error.message : String(error))) |
Contributor
There was a problem hiding this comment.
Change 'both' to 'dev' to preserve dev-only logging behavior.
This logger call uses 'both' as the audience, which expands logging to production. The PR objectives require mapping devLogger calls to logger.('dev', ...) to maintain the existing isDevelopment-gated dev-only behavior.
🔧 Proposed fix
- logger.error('both', 'Failed to create SSA: ' + (error instanceof Error ? error.message : String(error)))
+ logger.error('dev', 'Failed to create SSA: ' + (error instanceof Error ? error.message : String(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.
Suggested change
| logger.error('both', 'Failed to create SSA: ' + (error instanceof Error ? error.message : String(error))) | |
| logger.error('dev', 'Failed to create SSA: ' + (error instanceof Error ? error.message : String(error))) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@admin-ui/plugins/auth-server/components/Ssa/hooks/useSsaApi.ts` at line 94,
In useSsaApi.ts update the logger call in the SSA creation error path to use the
dev-only audience: replace logger.error('both', 'Failed to create SSA: ' +
(error instanceof Error ? error.message : String(error))) with a call that
passes 'dev' as the first argument so the message remains gated to development
logging; locate this inside the create SSA error handling in the useSsaApi hook
and change the audience token from 'both' to 'dev'.
|
|
||
| return await handleResponse<SsaJwtResponse>(response) | ||
| } catch (error) { | ||
| logger.error('both', 'Failed to fetch SSA JWT: ' + (error instanceof Error ? error.message : String(error))) |
Contributor
There was a problem hiding this comment.
Change 'both' to 'dev' to preserve dev-only logging behavior.
This logger call uses 'both' as the audience, which logs in production. The PR objectives state that the migration should preserve behavior by using logger.('dev', ...) to maintain isDevelopment gating.
🔧 Proposed fix
- logger.error('both', 'Failed to fetch SSA JWT: ' + (error instanceof Error ? error.message : String(error)))
+ logger.error('dev', 'Failed to fetch SSA JWT: ' + (error instanceof Error ? error.message : String(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.
Suggested change
| logger.error('both', 'Failed to fetch SSA JWT: ' + (error instanceof Error ? error.message : String(error))) | |
| logger.error('dev', 'Failed to fetch SSA JWT: ' + (error instanceof Error ? error.message : String(error))) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@admin-ui/plugins/auth-server/components/Ssa/hooks/useSsaApi.ts` at line 123,
In useSsaApi.ts update the logger call inside the SSA fetch error handling (the
logger.error(...) invocation in the useSsaApi hook) to use the 'dev' audience
instead of 'both' so the message remains development-only; locate the
logger.error('both', 'Failed to fetch SSA JWT: ' + ...) line and change the
first argument to 'dev' preserving the rest of the message and error formatting.
| await postUserAction(audit as UserActionPayload) | ||
| return data | ||
| } catch (e) { | ||
| logger.error('both', 'Failed to fetch auth server JSON properties: ' + resolveApiErrorMessage(e as Error)) |
Contributor
There was a problem hiding this comment.
Preserve dev-only audience in migrated error logs.
At Line 37 and Line 70, logger.error('both', ...) broadens logging scope versus prior devLogger behavior. This changes runtime behavior and may leak detailed backend error text to production logs. Use 'dev' here (or explicitly document/approve this policy change in PR scope).
Suggested patch
- logger.error('both', 'Failed to fetch auth server JSON properties: ' + resolveApiErrorMessage(e as Error))
+ logger.error('dev', 'Failed to fetch auth server JSON properties: ' + resolveApiErrorMessage(e as Error))
...
- logger.error('both', 'Failed to patch auth server JSON properties: ' + resolveApiErrorMessage(e as Error))
+ logger.error('dev', 'Failed to patch auth server JSON properties: ' + resolveApiErrorMessage(e as Error))Also applies to: 70-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@admin-ui/plugins/auth-server/services/jsonPropertiesService.ts` at line 37,
The error logs in jsonPropertiesService.ts currently call logger.error('both',
...) which expands audience from dev-only to both environments; change both
occurrences to logger.error('dev', ...) so detailed backend error messages
remain dev-only (search for the logger.error('both', 'Failed to fetch auth
server JSON properties: ' + resolveApiErrorMessage(e as Error)) and the second
similar call and replace the 'both' audience with 'dev').
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
2 participants
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.
feat(admin-ui): Migrate Auth Server module to unified logger (#2870)
Summary
This PR migrates the Auth Server plugin to the new unified logging infrastructure by replacing existing
devLoggerusage with the new audience-based logger API.The migration preserves existing behavior by mapping all development-only logging to
logger.<level>('dev', ...), which remains gated byisDevelopment. This aligns the Auth Server module with the broader logging modernization effort while maintaining consistent runtime behavior.Fix Summary
devLogger.<level>(...)usage throughout the Auth Server pluginapp/utils/logger.tsapp/utils/types/LoggerTypes.tsapp/utils/types/index.tsVerification
passes successfully.
devLogger.tsremains available during migration🔗 Ticket
Closes: #2870
Summary by CodeRabbit
Release Notes