feat(admin-ui): Migrate Home/Core module to unified logger (#2868)#2869
feat(admin-ui): Migrate Home/Core module to unified logger (#2868)#2869faisalsiddique4400 wants to merge 2 commits into
Conversation
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
|
Warning Review limit reached
More reviews will be available in 18 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR replaces the ChangesUnified logger migration across admin-ui
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/app/routes/License/hooks/useLicenseDetails.ts (1)
88-93: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winOptional: Remove redundant
isDevelopmentguard.The new
logger.error('dev', ...)API already gates on development mode internally (per PR objectives: "development-only logs remain gated by development mode"). The explicitif (isDevelopment)check is redundant and inconsistent with the migration pattern used in other files in this PR (e.g.,useProfileDetails.ts,ByeBye.tsx,index.tsx,pagingUtils.ts), which calllogger.error('dev', ...)directly without guards.♻️ Simplify by removing the redundant guard
- if (baseAudit) { - const audit = { - ...baseAudit, - action: DELETION, - resource: API_LICENSE, - message: pendingMessageRef.current, - } - try { - await postUserAction(audit) - } catch (e) { - if (isDevelopment) { - logger.error('dev', - 'License reset audit post failed:', - e instanceof Error ? e : String(e), - ) - } - } - } + if (baseAudit) { + const audit = { + ...baseAudit, + action: DELETION, + resource: API_LICENSE, + message: pendingMessageRef.current, + } + try { + await postUserAction(audit) + } catch (e) { + logger.error('dev', + 'License reset audit post failed:', + e instanceof Error ? e : String(e), + ) + } + }🤖 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/routes/License/hooks/useLicenseDetails.ts` around lines 88 - 93, Remove the redundant development-mode guard: delete the surrounding if (isDevelopment) { ... } and call logger.error('dev', 'License reset audit post failed:', e instanceof Error ? e : String(e)) directly so the logger's internal dev gating is used; look for the isDevelopment check and the logger.error('dev', ...) call inside useLicenseDetails (the catch block that logs the "License reset audit post failed" error) and keep the same error argument handling (e instanceof Error ? e : String(e)).
🤖 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/routes/Apps/Gluu/GluuAppSidebar.tsx`:
- Around line 167-169: The catch in loadMenus currently logs and swallows errors
(logger.error(... resolveApiErrorMessage)) which leaves pluginMenus empty and
isReady false causing an infinite loader; either re-throw the error so the
existing ErrorBoundary (ErrorBoundary) can handle it (remove the catch or add
`throw error` after logging), or set a component error state (e.g.,
pluginMenusError via useState) inside the catch and mark loading complete so the
render can show an error message with a retry button that calls loadMenus again;
update the render logic that checks isReady/pluginMenus to display the error UI
when pluginMenusError is set and wire the retry to the loadMenus function.
In `@admin-ui/app/routes/Apps/Gluu/GluuSessionTimeout.tsx`:
- Line 57: The logout error is currently being logged only to the 'dev' channel
via logger.error in GluuSessionTimeout; update the logger.error call used in the
logout error handling (logger.error(...)) to log to both environments (use
'both' as the target) and keep the same error payload (err instanceof Error ?
err : String(err)) so production will also record these security-sensitive
failures for monitoring and audit.
In `@admin-ui/app/routes/License/hooks/useLicenseDetails.ts`:
- Line 17: Remove the now-unused isDevelopment import from useLicenseDetails.ts:
locate the import statement "import { isDevelopment } from '`@/utils/env`'" and
delete it so the file no longer imports isDevelopment when its guards were
removed.
- Around line 107-109: Remove the redundant isDevelopment guard around the
logger call in useLicenseDetails.ts: inside the reset-license/error handling
block where you currently have "if (isDevelopment && error != null) {
logger.error('dev', 'License reset failed:', error) }", drop the outer
isDevelopment check and simply call logger.error('dev', 'License reset failed:',
error) (you may also omit the null check since logging a null/undefined error is
harmless); this keeps the development-only logging behavior delegated to logger
and simplifies the conditional.
---
Outside diff comments:
In `@admin-ui/app/routes/License/hooks/useLicenseDetails.ts`:
- Around line 88-93: Remove the redundant development-mode guard: delete the
surrounding if (isDevelopment) { ... } and call logger.error('dev', 'License
reset audit post failed:', e instanceof Error ? e : String(e)) directly so the
logger's internal dev gating is used; look for the isDevelopment check and the
logger.error('dev', ...) call inside useLicenseDetails (the catch block that
logs the "License reset audit post failed" error) and keep the same error
argument handling (e instanceof Error ? e : String(e)).
🪄 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: db4fba1d-62dd-4294-9552-4f543bf2465a
📒 Files selected for processing (31)
admin-ui/app/cedarling/hooks/useCedarling.tsadmin-ui/app/components/App/PermissionsPolicyInitializer.tsxadmin-ui/app/components/Wizard/Wizard.tsxadmin-ui/app/context/theme/themeContext.tsxadmin-ui/app/i18n.tsadmin-ui/app/layout/default.tsxadmin-ui/app/redux/api/backend-api.tsadmin-ui/app/redux/listeners/authListener.tsadmin-ui/app/redux/listeners/licenseListener.tsadmin-ui/app/redux/listeners/sessionListener.tsadmin-ui/app/routes/Apps/Gluu/GluuAppSidebar.tsxadmin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsxadmin-ui/app/routes/Apps/Gluu/GluuScriptErrorModal.tsxadmin-ui/app/routes/Apps/Gluu/GluuSessionTimeout.tsxadmin-ui/app/routes/Apps/Gluu/LanguageMenu.tsxadmin-ui/app/routes/Apps/Gluu/ThemeDropdown.tsxadmin-ui/app/routes/Apps/Profile/hooks/useProfileDetails.tsadmin-ui/app/routes/License/hooks/useLicenseDetails.tsadmin-ui/app/routes/Pages/ByeBye.tsxadmin-ui/app/routes/index.tsxadmin-ui/app/utils/AppAuthProvider.tsxadmin-ui/app/utils/apiErrorMessage.tsadmin-ui/app/utils/logger.tsadmin-ui/app/utils/pagingUtils.tsadmin-ui/app/utils/triggerWebhookForFeature.tsadmin-ui/app/utils/types/LoggerTypes.tsadmin-ui/app/utils/types/index.tsadmin-ui/orval/__tests__/interceptors.test.tsadmin-ui/orval/interceptors.tsadmin-ui/plugins/PluginMenuResolver.tsadmin-ui/plugins/internal/loadPluginMetadata.ts
Signed-off-by: faisalsiddique4400 <faisalsiddique10886@gmail.com>
|
duttarnab
left a comment
duttarnab
left a comment
There was a problem hiding this comment.
I think we should follow the standard approach for displaying logs. We have the following log levels: TRACE, DEBUG, INFO, WARN, ERROR, and FATAL. Users should be able to configure the GUI log level from the UI, similar to how they can configure the theme or locale. The Admin UI should then display logs in the browser console according to the configured log level.
Dividing the display mode into dev, prod, and both will not work well when troubleshooting production issues, as there may be a need to view dev-level logs during the investigation.
3 participants
feat(admin-ui): Migrate Home/Core module to unified logger (#2868)
Summary
This PR migrates the Home/Core module to the new unified logging system and replaces all remaining usages of the removed
devLoggerutility.The new logger introduces audience-based logging through
logger.<level>('dev' | 'prod' | 'both', ...), providing a consistent logging API across the Admin UI while preserving the existing behavior. Calls targeting thedevaudience remain gated behind development mode, matching the previousdevLoggerimplementation.No user-facing behavior changes are introduced.
Fix Summary
app/utils/devLogger.tsapp/utils/types/DevLoggerTypes.tsapp/utils/logger.tsapp/utils/types/LoggerTypes.tsdevaudienceVerification
passes successfully.
🔗 Ticket
Closes: #2868
Summary by CodeRabbit
Refactor
Chores