Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an enterprise login UI: new ConsoleEnterpriseLogin React component, a conditional ConsoleSettings tab shown for paid non-sub-organization tenants, enum members for the tab, a dependency on Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/admin.console-settings.v1/components/console-settings-tabs.tsx (1)
166-179:⚠️ Potential issue | 🟠 MajorAdd missing dependencies to
consoleTabsmemo to ensure Enterprise Login tab visibility updates correctly.The memo at line 116 uses an empty dependency array while reading
tierName,isLoginFlowEnabled,isSharedAccessDisabled,isSubOrganization,componentId, andt. SincetierNamecomes from the asyncuseSubscription()hook, the memo won't recalculate when the subscription tier loads, keeping the Enterprise Login tab stuck in the wrong visibility state.Fix
!(isSubOrganization() || tierName === TenantTier.FREE) && { className: "console-enterprise-login", "data-componentid": `${componentId}-tab-enterprise-login`, "data-tabid": ConsoleSettingsModes.ENTERPRISE_LOGIN, hidden: false, id: ConsoleSettingsModes.ENTERPRISE_LOGIN, label: t("consoleSettings:enterpriseLogin.tabLabel"), pane: <ConsoleEnterpriseLogin />, value: ConsoleSettingsTabIDs.ENTERPRISE_LOGIN } ] .filter((tab: ConsoleSettingsTabInterface) => tab && !tab?.hidden) .map((tab: ConsoleSettingsTabInterface, index: number) => ({ ...tab, value: index })), - [] + [ componentId, isLoginFlowEnabled, isSharedAccessDisabled, isSubOrganization, t, tierName ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/admin.console-settings.v1/components/console-settings-tabs.tsx` around lines 166 - 179, The memo computing consoleTabs (the useMemo that builds the array including the ConsoleEnterpriseLogin tab) is missing dependencies so it doesn't update when async values change; update the dependency array to include tierName, isLoginFlowEnabled, isSharedAccessDisabled, isSubOrganization, componentId and t so the memo recomputes when the subscription/toggle flags or localization change; ensure the filter/map logic that references ConsoleSettingsTabInterface, ConsoleSettingsModes and ConsoleSettingsTabIDs remains unchanged and only the dependency list is adjusted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@features/admin.console-settings.v1/components/console-enterprise-login/console-enterprise-login.tsx`:
- Around line 56-70: The CTA Button currently builds
href={`mailto:${supportEmail}`} and can render mailto:undefined when
supportEmail is missing; update the console-enterprise-login component to guard
this by checking the supportEmail value (from the supportEmail selector) and
either disable or omit the Button/action when supportEmail is falsy, or replace
href with a safe fallback (e.g., no href and disabled) so the Alert action never
links to mailto:undefined; ensure you modify the Alert action/Button rendering
logic in console-enterprise-login to reference supportEmail and adjust props
accordingly.
In `@features/admin.console-settings.v1/package.json`:
- Line 36: Replace the registry semver for the feature dependency
"@wso2is/admin.subscription.v1" with the workspace protocol; update its version
spec from "^1.5.205" to use "workspace:^1.5.205" (i.e., change the value for the
"@wso2is/admin.subscription.v1" dependency in package.json to start with
"workspace:^").
---
Outside diff comments:
In `@features/admin.console-settings.v1/components/console-settings-tabs.tsx`:
- Around line 166-179: The memo computing consoleTabs (the useMemo that builds
the array including the ConsoleEnterpriseLogin tab) is missing dependencies so
it doesn't update when async values change; update the dependency array to
include tierName, isLoginFlowEnabled, isSharedAccessDisabled, isSubOrganization,
componentId and t so the memo recomputes when the subscription/toggle flags or
localization change; ensure the filter/map logic that references
ConsoleSettingsTabInterface, ConsoleSettingsModes and ConsoleSettingsTabIDs
remains unchanged and only the dependency list is adjusted.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/long-clouds-watch.mdfeatures/admin.console-settings.v1/components/console-enterprise-login/console-enterprise-login.tsxfeatures/admin.console-settings.v1/components/console-settings-tabs.tsxfeatures/admin.console-settings.v1/models/ui.tsfeatures/admin.console-settings.v1/package.jsonmodules/i18n/src/models/namespaces/console-settings-ns.tsmodules/i18n/src/translations/en-US/portals/console-settings.ts
...s/admin.console-settings.v1/components/console-enterprise-login/console-enterprise-login.tsx
Show resolved
Hide resolved
bhagyasakalanka
force-pushed
the
enterprise-login-ui
branch
from
3102948 to
93fcc23
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9671 +/- ##
==========================================
+ Coverage 55.88% 56.01% +0.12%
==========================================
Files 42 42
Lines 1020 1023 +3
Branches 254 231 -23
==========================================
+ Hits 570 573 +3
Misses 450 450
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Hi @bhagyasakalanka, Isn’t having a separate tab just to render this info alert a bit of an overkill? In my opinion, we could simply display the banner right above the tab group instead. We could also make the banner expandable, so users can click to view additional details, such as an explanation of what Enterprise Login is and how it works. Additionally, this banner should be hidden in the on-prem console. I assume this has already been taken into consideration, but it’s worth confirming. |
bhagyasakalanka
force-pushed
the
enterprise-login-ui
branch
from
02cfe50 to
99875ce
Compare
We had a chat with @JKAUSHALYA on this initially. In the future, we will improve this tab to provide self configuration of enterprise IDP. Hence we add this as a new tab instead of a banner above |
bhagyasakalanka
force-pushed
the
enterprise-login-ui
branch
from
99875ce to
1172f94
Compare
Yes.. This is not visible in onprem |
Shall we onboard the tab once the self service UI is available and keep the banner above the tab group for now? Because having a single banner in a separate tab wouldn't look good, IMO. cc: @jeradrutnam |
|
+1 for @pavinduLakshan sugggestion if it will not onboard in near future. Additionally we can improve the messaging a bit. "Contact us to configure an Enterprise login to this organization's console access." |
3 participants
Purpose
Add UI for enteprise login feature.
Initially, we will only show a banner asking customers to contact us for configuration