feat: add user and team tables with authentication support #591
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRenames migration package to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Store as ConfigStore (RDB)
participant Migr as Migrator
participant DB as GORM_DB
App->>Store: NewConfigStore(...)
activate Store
Store->>Store: triggerMigrations(...)
Store->>Store: migrationTeamsTableUpdates()
Store->>Migr: migrator.New(DB, migrations...).Migrate()
activate Migr
Migr->>DB: ALTER TABLE teams ADD COLUMN profile/config/claims
DB-->>Migr: result (ok/error)
Migr-->>Store: Migrate() returns (ok/error)
deactivate Migr
Store-->>App: initialization complete (ok/error)
deactivate Store
note right of Store: runtime helper: RunMigration(ctx, *migrator.Migration)
App->>Store: RunMigration(ctx, migration)
Store->>Migr: migrator.New(DB, migration).Migrate()
Migr-->>Store: result
Store-->>App: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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
🧹 Nitpick comments (6)
ui/app/_fallbacks/enterprise/lib/store/slices/index.ts (1)
4-4: Consider adding type annotations.For better type safety, consider adding explicit type annotations to the reducer:
-export const scimReducer = (state = {}) => state +export const scimReducer = (state: Record<string, unknown> = {}): Record<string, unknown> => stateui/lib/store/store.ts (2)
11-29: Simplify the nested spreading for better performance.The current implementation spreads
enterpriseReducersmultiple times (lines 16-19 and 22-25), creating intermediate objects unnecessarily. This can be simplified by building the object once.Apply this diff to simplify the logic:
-// Get enterprise reducers if they are available -let enterpriseReducers = {}; -try { - const enterprise = require("@enterprise/lib/store/slices"); - if (enterprise.scimReducer) { - enterpriseReducers = { - ...enterpriseReducers, - scim: enterprise.scimReducer, - }; - } - if (enterprise.userReducer) { - enterpriseReducers = { - ...enterpriseReducers, - user: enterprise.userReducer, - }; - } -} catch (e) { - // Enterprise reducers not available, continue without them -} +// Get enterprise reducers if they are available +const enterpriseReducers: Record<string, any> = {}; +try { + const enterprise = require("@enterprise/lib/store/slices"); + if (enterprise.scimReducer) { + enterpriseReducers.scim = enterprise.scimReducer; + } + if (enterprise.userReducer) { + enterpriseReducers.user = enterprise.userReducer; + } +} catch (e) { + // Enterprise reducers not available, continue without them +}
31-44: Consider removing or clarifying the purpose of enterprise API loading.The API loading block (lines 31-44) only ensures modules are loaded but doesn't use the imported values. According to the comments, APIs are already injected via
injectEndpointsat import time. If the modules have necessary side effects on import, this is fine, but if not, this block may be redundant.If the modules inject APIs via side effects when imported, consider simplifying to:
-// Inject enterprise APIs if they are available -try { - const enterpriseApis = require("@enterprise/lib/store/apis"); - if (enterpriseApis.scimApi) { - // APIs are already injected into baseApi via injectEndpoints - // This just ensures the module is loaded - } - if (enterpriseApis.userApi) { - // APIs are already injected into baseApi via injectEndpoints - // This just ensures the module is loaded - } -} catch (e) { - // Enterprise APIs not available, continue without them -} +// Inject enterprise APIs if they are available (side effects on import) +try { + require("@enterprise/lib/store/apis"); +} catch (e) { + // Enterprise APIs not available, continue without them +}Alternatively, if the API loading serves a specific purpose beyond side effects, consider adding a comment explaining why each API is checked.
framework/configstore/tables.go (2)
646-669: Consider clearing string fields when Parsed fields are nil.*The
BeforeSavehook only serializes whenParsed*fields are non-nil, but it doesn't clear the corresponding string fields when they become nil. This means once a field is set, it cannot be explicitly cleared to NULL in the database.Apply this diff to handle nil cases:
func (u *TableUser) BeforeSave(tx *gorm.DB) error { if u.ParsedProfile != nil { data, err := json.Marshal(u.ParsedProfile) if err != nil { return err } u.Profile = bifrost.Ptr(string(data)) + } else { + u.Profile = nil } if u.ParsedConfig != nil { data, err := json.Marshal(u.ParsedConfig) if err != nil { return err } u.Config = bifrost.Ptr(string(data)) + } else { + u.Config = nil } if u.ParsedClaims != nil { data, err := json.Marshal(u.ParsedClaims) if err != nil { return err } u.Claims = bifrost.Ptr(string(data)) + } else { + u.Claims = nil } return nil }
719-762: Apply the same nil-handling fix for TableTeam.The
BeforeSavehook forTableTeamhas the same issue asTableUserwhere string fields are not cleared whenParsed*fields become nil.Apply the same fix as suggested for
TableUser.BeforeSaveto addelseclauses that set the string fields to nil:func (t *TableTeam) BeforeSave(tx *gorm.DB) error { if t.ParsedProfile != nil { data, err := json.Marshal(t.ParsedProfile) if err != nil { return err } t.Profile = bifrost.Ptr(string(data)) + } else { + t.Profile = nil } if t.ParsedConfig != nil { data, err := json.Marshal(t.ParsedConfig) if err != nil { return err } t.Config = bifrost.Ptr(string(data)) + } else { + t.Config = nil } if t.ParsedClaims != nil { data, err := json.Marshal(t.ParsedClaims) if err != nil { return err } t.Claims = bifrost.Ptr(string(data)) + } else { + t.Claims = nil } return nil }framework/configstore/migrations.go (1)
403-438: Consider adding a Rollback function for consistency.The new migration creates a table and adds columns but doesn't provide a
Rollbackfunction. While rollbacks may not always be necessary, other migrations in this file (such asmigrationAddEnableLiteLLMFallbacksColumnat lines 386-393) provide rollback functions for consistency and to support safe migration rollbacks during development or testing.Consider adding a Rollback function:
func migrationAddUserAndTeamTables(ctx context.Context, db *gorm.DB) error { m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ ID: "add_user_and_team_tables", Migrate: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() if !migrator.HasTable(&TableUser{}) { if err := migrator.CreateTable(&TableUser{}); err != nil { return err } } if !migrator.HasColumn(&TableTeam{}, "profile") { if err := migrator.AddColumn(&TableTeam{}, "profile"); err != nil { return err } } if !migrator.HasColumn(&TableTeam{}, "config") { if err := migrator.AddColumn(&TableTeam{}, "config"); err != nil { return err } } if !migrator.HasColumn(&TableTeam{}, "claims") { if err := migrator.AddColumn(&TableTeam{}, "claims"); err != nil { return err } } return nil }, + Rollback: func(tx *gorm.DB) error { + tx = tx.WithContext(ctx) + migrator := tx.Migrator() + if err := migrator.DropTable(&TableUser{}); err != nil { + return err + } + if err := migrator.DropColumn(&TableTeam{}, "profile"); err != nil { + return err + } + if err := migrator.DropColumn(&TableTeam{}, "config"); err != nil { + return err + } + if err := migrator.DropColumn(&TableTeam{}, "claims"); err != nil { + return err + } + return nil + }, }}) err := m.Migrate() if err != nil { return fmt.Errorf("error while running db migration: %s", err.Error()) } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ui/public/images/ms-entra.pngis excluded by!**/*.pngui/public/images/okta.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
framework/configstore/migrations.go(10 hunks)framework/configstore/migrator/migrator.go(1 hunks)framework/configstore/rdb.go(4 hunks)framework/configstore/store.go(2 hunks)framework/configstore/tables.go(3 hunks)transports/bifrost-http/handlers/server.go(1 hunks)ui/app/_fallbacks/enterprise/components/login/loginView.tsx(1 hunks)ui/app/_fallbacks/enterprise/lib/store/slices/index.ts(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/layout.tsx(2 hunks)ui/app/login/page.tsx(1 hunks)ui/app/logs/page.tsx(2 hunks)ui/app/providers/page.tsx(1 hunks)ui/app/scim/page.tsx(1 hunks)ui/components/sidebar.tsx(6 hunks)ui/lib/store/apis/baseApi.ts(2 hunks)ui/lib/store/slices/appSlice.ts(1 hunks)ui/lib/store/slices/index.ts(1 hunks)ui/lib/store/store.ts(3 hunks)ui/next.config.ts(1 hunks)ui/package.json(1 hunks)ui/tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
ui/app/login/page.tsx (1)
ui/app/_fallbacks/enterprise/components/login/loginView.tsx (1)
LoginView(6-14)
framework/configstore/store.go (1)
framework/configstore/migrator/migrator.go (1)
Migration(62-69)
framework/configstore/tables.go (1)
core/utils.go (1)
Ptr(12-14)
ui/app/layout.tsx (1)
ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(87-109)
ui/components/sidebar.tsx (2)
ui/hooks/useWebSocket.tsx (1)
useWebSocket(93-99)ui/lib/constants/config.ts (1)
IS_ENTERPRISE(70-70)
framework/configstore/rdb.go (1)
framework/configstore/migrator/migrator.go (3)
Migration(62-69)New(131-149)DefaultOptions(100-106)
framework/configstore/migrations.go (2)
framework/configstore/migrator/migrator.go (3)
New(131-149)DefaultOptions(100-106)Migration(62-69)framework/configstore/tables.go (4)
TableUser(623-638)TableUser(641-641)TableTeam(693-715)TableTeam(837-837)
🔇 Additional comments (11)
ui/tsconfig.json (1)
33-35: Consider adding a fallback path for @schemas alias.The new
@schemas/*alias only points to the enterprise schemas directory without a fallback path, unlike@enterprise/*which includes a fallback. This could cause module resolution failures when enterprise features are unavailable.Verify if the webpack config in
next.config.tsprovides the fallback at build time. If not, consider adding a fallback path:"@schemas/*": [ - "./app/enterprise/lib/schemas/*" + "./app/enterprise/lib/schemas/*", + "./app/_fallbacks/enterprise/lib/schemas/*" ]ui/package.json (1)
7-7: LGTM!The new
build-enterprisescript appropriately omits thecopy-buildstep, which is only needed for the standard build that deploys to the transport layer.ui/app/logs/page.tsx (1)
11-12: LGTM!The addition of
useAppSelectorand type imports aligns with the store integration changes in this PR.ui/app/_fallbacks/enterprise/components/login/loginView.tsx (1)
6-14: LGTM!The fallback login view correctly redirects non-enterprise users to the home page. The implementation is clean with proper dependency tracking in the
useEffecthook.ui/next.config.ts (2)
32-34: Verify @schemas fallback path.The fallback for
@schemasresolves toapp/_fallbacks/enterprise/libbut doesn't include theschemassubdirectory (unlike the enterprise path which includes it). Verify this is intentional.Compare with the TypeScript alias in
tsconfig.json(line 34) which only defines the enterprise path without a fallback. If schema files should exist in the fallback, consider:config.resolve.alias["@schemas"] = haveEnterprise ? path.join(__dirname, "app", "enterprise", "lib", "schemas") - : path.join(__dirname, "app", "_fallbacks", "enterprise", "lib"); + : path.join(__dirname, "app", "_fallbacks", "enterprise", "lib", "schemas");
35-42: LGTM!The module resolution and symlink configuration correctly handles the scenario where enterprise features might be symlinked from an external folder.
framework/configstore/store.go (1)
110-114: Confirm only RDBConfigStore implements DB()/RunMigration and restrict raw DB access
RDBConfigStore in framework/configstore/rdb.go provides both methods; interface coverage is complete. Audit your HTTP/API handlers to ensureDB()isn’t used there.ui/app/adaptive-routing/page.tsx (1)
1-1: Enterprise component import paths are consistent.
No@enterpriseimports without/components/found across the codebase.ui/lib/store/store.ts (1)
82-82: LGTM!The RootState type correctly intersects the base state with EnterpriseState. Since the enterprise slices are optional in the type definition (lines 7-8), this properly reflects the runtime behavior where these reducers may or may not be loaded.
framework/configstore/tables.go (1)
672-689: LGTM!The
AfterFindhook correctly deserializes JSON fields only when the string pointers are non-nil, and properly propagates any unmarshaling errors.framework/configstore/migrations.go (1)
7-7: LGTM!The migration package has been consistently renamed from
migrationtomigratoracross all migration functions. All references tomigration.Newandmigration.Migrationhave been properly updated tomigrator.Newandmigrator.Migration.Also applies to: 45-45, 209-209, 249-249, 272-272, 305-305, 328-328, 351-351, 374-374
Pratham-Mishra04
force-pushed
the
10-09-minor_extrensibility_updates_new
branch
from
f5fbeda to
cfcca94
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/logs/page.tsx (1)
11-25: Remove unused user selector.
Line 24definesuserbut nothing consumes it, so lint/TypeScript will fail with a no-unused-vars error. Drop the selector (and theuseAppSelectorimport) unless you actually use the user state.-import { getErrorMessage, useAppSelector, useLazyGetLogsQuery } from "@/lib/store"; +import { getErrorMessage, useLazyGetLogsQuery } from "@/lib/store"; ... - const user = useAppSelector((state) => state.user?.currentUser);
♻️ Duplicate comments (1)
ui/app/layout.tsx (1)
28-43: Skip the core config query on/login.
useGetCoreConfigQuerystill runs before you computeisLoginPage, so/loginvisitors hit the failing request and toast. Move theisLoginPagecheck ahead of the query, pass{ skip: isLoginPage }, and guard the toast effect so the login route stays clean.- const pathname = usePathname(); - const { data: bifrostConfig, error } = useGetCoreConfigQuery({}); - const isLoginPage = useMemo(() => { - return pathname === "/login"; - }, [pathname]); + const pathname = usePathname(); + const isLoginPage = pathname === "/login"; + const { data: bifrostConfig, error } = useGetCoreConfigQuery({}, { skip: isLoginPage }); ... - useEffect(() => { - if (error) { + useEffect(() => { + if (error && !isLoginPage) { toast.error(getErrorMessage(error)); } - }, [error]); + }, [error, isLoginPage]);
🧹 Nitpick comments (2)
ui/lib/store/slices/appSlice.ts (1)
13-18: Consider removing orphaned currentUser state.The
currentUserstate field remains but the actions to modify it (setCurrentUser,clearCurrentUser) were removed. This creates dead state that can never change from its initialnullvalue. TheselectCurrentUserselector (line 220) is also orphaned.If user management has moved to the enterprise store (as suggested by the PR context), consider removing this unused state and its selector to keep the codebase clean.
Apply this diff to remove the orphaned state:
export interface AppState { // UI State sidebarCollapsed: boolean; theme: "light" | "dark" | "system"; // Loading states for global operations isInitializing: boolean; isOnline: boolean; - - // Current user/session info - currentUser: { - id?: string; - name?: string; - email?: string; - } | null; // Global notifications/toasts notifications: {And remove the corresponding initial state value (line 69) and selector (line 220).
framework/configstore/migrations.go (1)
403-438: Consider adding a rollback function for consistency.The migration logic correctly checks for table/column existence before creating/adding them, which prevents errors on re-runs. However, this migration lacks a
Rollbackfunction, which is inconsistent with other migrations in this file (e.g.,migrationInit,migrationMany2ManyJoinTable,migrationAddVirtualKeyProviderConfigTable, etc.).While rollback may not be critical for your deployment workflow, adding it would maintain consistency with the established pattern and provide a safety mechanism for reverting changes if needed.
Apply this diff to add a rollback function:
func migrationAddUserAndTeamTables(ctx context.Context, db *gorm.DB) error { m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ ID: "add_user_and_team_tables", Migrate: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() if !migrator.HasTable(&TableUser{}) { if err := migrator.CreateTable(&TableUser{}); err != nil { return err } } if !migrator.HasColumn(&TableTeam{}, "profile") { if err := migrator.AddColumn(&TableTeam{}, "profile"); err != nil { return err } } if !migrator.HasColumn(&TableTeam{}, "config") { if err := migrator.AddColumn(&TableTeam{}, "config"); err != nil { return err } } if !migrator.HasColumn(&TableTeam{}, "claims") { if err := migrator.AddColumn(&TableTeam{}, "claims"); err != nil { return err } } return nil }, + Rollback: func(tx *gorm.DB) error { + tx = tx.WithContext(ctx) + migrator := tx.Migrator() + // Drop columns in reverse order + if migrator.HasColumn(&TableTeam{}, "claims") { + if err := migrator.DropColumn(&TableTeam{}, "claims"); err != nil { + return err + } + } + if migrator.HasColumn(&TableTeam{}, "config") { + if err := migrator.DropColumn(&TableTeam{}, "config"); err != nil { + return err + } + } + if migrator.HasColumn(&TableTeam{}, "profile") { + if err := migrator.DropColumn(&TableTeam{}, "profile"); err != nil { + return err + } + } + if migrator.HasTable(&TableUser{}) { + if err := migrator.DropTable(&TableUser{}); err != nil { + return err + } + } + return nil + }, }}) err := m.Migrate() if err != nil { return fmt.Errorf("error while running db migration: %s", err.Error()) } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ui/public/images/ms-entra.pngis excluded by!**/*.pngui/public/images/okta.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
framework/configstore/migrations.go(10 hunks)framework/configstore/migrator/migrator.go(1 hunks)framework/configstore/rdb.go(4 hunks)framework/configstore/store.go(2 hunks)framework/configstore/tables.go(3 hunks)transports/bifrost-http/handlers/server.go(1 hunks)ui/app/_fallbacks/enterprise/components/login/loginView.tsx(1 hunks)ui/app/_fallbacks/enterprise/lib/store/slices/index.ts(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/layout.tsx(2 hunks)ui/app/login/page.tsx(1 hunks)ui/app/logs/page.tsx(2 hunks)ui/app/providers/page.tsx(1 hunks)ui/app/scim/page.tsx(1 hunks)ui/components/sidebar.tsx(5 hunks)ui/lib/store/apis/baseApi.ts(2 hunks)ui/lib/store/slices/appSlice.ts(1 hunks)ui/lib/store/slices/index.ts(1 hunks)ui/lib/store/store.ts(3 hunks)ui/next.config.ts(1 hunks)ui/package.json(1 hunks)ui/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ui/app/providers/page.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/app/_fallbacks/enterprise/lib/store/slices/index.ts
- ui/app/scim/page.tsx
- transports/bifrost-http/handlers/server.go
- ui/lib/store/apis/baseApi.ts
- ui/app/cluster/page.tsx
- ui/tsconfig.json
- ui/app/adaptive-routing/page.tsx
- framework/configstore/store.go
- ui/app/_fallbacks/enterprise/components/login/loginView.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
ui/app/layout.tsx (1)
ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(87-109)
framework/configstore/tables.go (1)
core/utils.go (1)
Ptr(12-14)
ui/components/sidebar.tsx (1)
ui/lib/constants/config.ts (1)
IS_ENTERPRISE(70-70)
ui/app/login/page.tsx (1)
ui/app/_fallbacks/enterprise/components/login/loginView.tsx (1)
LoginView(6-14)
framework/configstore/rdb.go (1)
framework/configstore/migrator/migrator.go (3)
Migration(62-69)New(131-149)DefaultOptions(100-106)
framework/configstore/migrations.go (2)
framework/configstore/migrator/migrator.go (3)
New(131-149)DefaultOptions(100-106)Migration(62-69)framework/configstore/tables.go (4)
TableUser(623-638)TableUser(641-641)TableTeam(692-714)TableTeam(835-835)
🔇 Additional comments (10)
framework/configstore/migrator/migrator.go (1)
21-21: LGTM! Package rename aligns with PR objectives.The package rename from
migrationtomigratormakes the migration functionality publicly accessible, which aligns with the PR's stated goal of moving the migration package from internal to public for external usage.framework/configstore/migrations.go (2)
7-7: LGTM! Import updated correctly.The import path has been updated to use the renamed
migratorpackage, maintaining consistency with the package rename.
37-39: LGTM! Migration invoked in correct sequence.The new
migrationAddUserAndTeamTablesmigration is correctly added to the migration sequence and follows the established error-handling pattern.framework/configstore/rdb.go (3)
10-10: LGTM! Import added for migrator package.The import correctly references the renamed
migratorpackage, supporting the newRunMigrationmethod.
45-48: LGTM! DB() method exposes database handle as intended.The new
DB()method exposes the underlying*gorm.DBinstance, which aligns with the interface changes inframework/configstore/store.go. This enables external packages to run migrations and perform other advanced database operations.While exposing the raw database handle reduces encapsulation, this appears to be an intentional design decision to support the migration functionality and other use cases that require direct database access.
1292-1296: LGTM! RunMigration method correctly executes migrations.The implementation correctly creates a migrator instance with the provided migration and executes it using the established migrator pattern. The method signature matches the interface definition in
framework/configstore/store.go.framework/configstore/tables.go (4)
8-8: LGTM! Import added for pointer helper.The
bifrostcore import provides thePtrhelper function used in theBeforeSavehooks for creating string pointers, which is a clean pattern for working with GORM's JSON serialization.
622-641: LGTM! TableUser struct is well-defined.The
TableUserstruct correctly defines JSON-backed fields with separate storage (Profile,Config,Claims) and parsed representations (ParsedProfile,ParsedConfig,ParsedClaims). The GORM tags are properly formatted (the typo mentioned in past review comments has been fixed).
703-711: LGTM! JSON-backed fields added to TableTeam.The Profile, Config, and Claims fields follow the same pattern as TableUser, with separate storage and parsed representations. The GORM tags are correctly formatted.
718-741: LGTM! BeforeSave hook correctly serializes JSON fields.The
BeforeSavehook forTableTeamcorrectly serializes the parsed maps to JSON strings usingbifrost.Ptrhelper, following the same pattern asTableUser.Note: The current implementation only serializes when
Parsed*is not nil. This means setting a field tonilwon't clear the database value. If you need to support clearing fields by setting them tonil, you would need to explicitly set the corresponding string pointer tonilor an empty value.
akshaydeo
force-pushed
the
10-09-minor_extrensibility_updates_new
branch
from
cfcca94 to
76b6a47
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ui/app/logs/page.tsx (1)
24-26: Remove debug logging and unused variable.Two issues identified:
- The
console.logstatement is debugging code that should be removed before merging (as previously flagged).- The
uservariable is retrieved from Redux state but never used in the component logic, making it dead code.If the user state is intended for future authentication UI features, implement the actual usage or defer this change to when it's needed.
-const user = useAppSelector((state) => state.user?.currentUser); - -console.log("user", user);framework/configstore/tables.go (1)
675-692: Handle empty strings in AfterFind deserialization.The
AfterFindhook checks if the JSON field pointer is not nil before unmarshaling, but it doesn't handle the case where the pointer points to an empty string"". An empty string is not valid JSON and will causejson.Unmarshalto fail.Based on learnings: A past review comment flagged this exact issue and it was marked as addressed in commit cfcca94, but the empty string check is still missing.
Apply this diff to handle empty strings safely:
// AfterFind hook for TableTeam to deserialize JSON fields func (t *TableTeam) AfterFind(tx *gorm.DB) error { - if t.Profile != nil { + if t.Profile != nil && *t.Profile != "" { if err := json.Unmarshal([]byte(*t.Profile), &t.ParsedProfile); err != nil { return err } } - if t.Config != nil { + if t.Config != nil && *t.Config != "" { if err := json.Unmarshal([]byte(*t.Config), &t.ParsedConfig); err != nil { return err } } - if t.Claims != nil { + if t.Claims != nil && *t.Claims != "" { if err := json.Unmarshal([]byte(*t.Claims), &t.ParsedClaims); err != nil { return err } } return nil }
🧹 Nitpick comments (2)
framework/configstore/migrations.go (1)
403-433: Consider adding a rollback function for completeness.The migration correctly adds the three new columns to the Team table with proper existence checks. However, consider adding a
Rollbackfunction to match the pattern used in some other migrations (e.g.,migrationAddEnableLiteLLMFallbacksColumnat lines 386-394).Apply this diff to add a rollback function:
func migrationTeamsTableUpdates(ctx context.Context, db *gorm.DB) error { m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ ID: "add_profile_config_claims_columns_to_team_table", Migrate: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() if !migrator.HasColumn(&TableTeam{}, "profile") { if err := migrator.AddColumn(&TableTeam{}, "profile"); err != nil { return err } } if !migrator.HasColumn(&TableTeam{}, "config") { if err := migrator.AddColumn(&TableTeam{}, "config"); err != nil { return err } } if !migrator.HasColumn(&TableTeam{}, "claims") { if err := migrator.AddColumn(&TableTeam{}, "claims"); err != nil { return err } } return nil }, + Rollback: func(tx *gorm.DB) error { + tx = tx.WithContext(ctx) + migrator := tx.Migrator() + if err := migrator.DropColumn(&TableTeam{}, "claims"); err != nil { + return err + } + if err := migrator.DropColumn(&TableTeam{}, "config"); err != nil { + return err + } + if err := migrator.DropColumn(&TableTeam{}, "profile"); err != nil { + return err + } + return nil + }, }}) err := m.Migrate() if err != nil { return fmt.Errorf("error while running db migration: %s", err.Error()) } return nil }framework/configstore/rdb.go (1)
45-48: Exposing the database handle reduces encapsulation.The
DB()method exposes the underlying*gorm.DBhandle, which allows external code to perform arbitrary database operations, potentially bypassing the store's abstractions and safety mechanisms. If this is intentional for advanced use cases, ensure that:
- This is documented as an escape hatch for advanced users
- Callers understand they're responsible for transaction management and error handling
- Consider whether specific methods could be added to cover the intended use cases instead
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ui/public/images/ms-entra.pngis excluded by!**/*.pngui/public/images/okta.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
framework/configstore/migrations.go(10 hunks)framework/configstore/migrator/migrator.go(1 hunks)framework/configstore/rdb.go(4 hunks)framework/configstore/store.go(2 hunks)framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/server.go(1 hunks)ui/app/_fallbacks/enterprise/components/login/loginView.tsx(1 hunks)ui/app/_fallbacks/enterprise/lib/store/slices/index.ts(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/login/page.tsx(1 hunks)ui/app/logs/page.tsx(2 hunks)ui/app/providers/page.tsx(1 hunks)ui/app/scim/page.tsx(1 hunks)ui/components/sidebar.tsx(6 hunks)ui/lib/store/apis/baseApi.ts(2 hunks)ui/lib/store/slices/appSlice.ts(1 hunks)ui/lib/store/slices/index.ts(1 hunks)ui/lib/store/store.ts(3 hunks)ui/next.config.ts(1 hunks)ui/package.json(1 hunks)ui/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- ui/components/sidebar.tsx
- ui/app/cluster/page.tsx
- transports/bifrost-http/handlers/server.go
- ui/lib/store/slices/appSlice.ts
- framework/configstore/store.go
- ui/tsconfig.json
- ui/next.config.ts
- ui/package.json
- ui/app/providers/page.tsx
- ui/app/adaptive-routing/page.tsx
- ui/app/login/page.tsx
- ui/app/_fallbacks/enterprise/components/login/loginView.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
framework/configstore/rdb.go (1)
framework/configstore/migrator/migrator.go (3)
Migration(62-69)New(131-149)DefaultOptions(100-106)
framework/configstore/migrations.go (2)
framework/configstore/migrator/migrator.go (3)
New(131-149)DefaultOptions(100-106)Migration(62-69)framework/configstore/tables.go (2)
TableTeam(623-645)TableTeam(767-767)
framework/configstore/tables.go (1)
core/utils.go (1)
Ptr(12-14)
🔇 Additional comments (4)
framework/configstore/migrator/migrator.go (1)
21-21: LGTM! Clean package rename.The package rename from
migrationtomigratoris consistent with the broader PR objective of making the migration functionality publicly accessible.framework/configstore/migrations.go (1)
7-7: LGTM! Import updated consistently.The import path correctly reflects the package rename from
migrationtomigrator.framework/configstore/tables.go (2)
8-8: LGTM! Import added for Ptr helper.The bifrost core import is correctly added to access the
Ptrhelper function used in theBeforeSavehook.
634-641: LGTM! JSON field structure follows established patterns.The new fields for Profile, Config, and Claims correctly follow the dual-field pattern used elsewhere in this file (e.g.,
TableProvider,TableKey):
- Storage fields use
*stringwithgorm:"type:text"andjson:"-"- Parsed fields use
map[string]interface{}withgorm:"-"and descriptive JSON tags
akshaydeo
force-pushed
the
10-09-minor_extrensibility_updates_new
branch
from
76b6a47 to
ddf4f17
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/lib/store/slices/appSlice.ts (1)
14-18: Fix orphanedcurrentUserstate and selector
currentUserremains inui/lib/store/slices/appSlice.ts(14–18, 69) andselectCurrentUser(220) but no reducers or actions populate it—ui/app/logs/page.tsx(24) still readsstate.user?.currentUser, so it’s alwaysnull. Either restore the removed setters/extraReducers to manage this field or remove it and its selector, and update the page to usestate.app.currentUseror theselectCurrentUserselector.
♻️ Duplicate comments (3)
ui/app/logs/page.tsx (1)
24-26: This debug logging was already flagged; remove unused code.A previous review already identified the
console.logstatement as debug code that should be removed. Additionally, theuservariable is not used anywhere in the component logic beyond this debug statement, suggesting incomplete implementation.Apply this diff to remove the unused code:
- const user = useAppSelector((state) => state.user?.currentUser); - - console.log("user", user);If user state is needed for future functionality, please add it as part of a complete implementation rather than leaving it as unused code.
framework/configstore/tables.go (1)
675-692: Handle empty strings in deserialization.The
AfterFindhook doesn't check for empty strings before unmarshaling. If a pointer is not nil but points to an empty string"",json.Unmarshalwill fail. This issue was flagged in a previous review but remains unaddressed.Apply this diff to handle empty strings safely:
func (t *TableTeam) AfterFind(tx *gorm.DB) error { - if t.Profile != nil { + if t.Profile != nil && *t.Profile != "" { if err := json.Unmarshal([]byte(*t.Profile), &t.ParsedProfile); err != nil { return err } } - if t.Config != nil { + if t.Config != nil && *t.Config != "" { if err := json.Unmarshal([]byte(*t.Config), &t.ParsedConfig); err != nil { return err } } - if t.Claims != nil { + if t.Claims != nil && *t.Claims != "" { if err := json.Unmarshal([]byte(*t.Claims), &t.ParsedClaims); err != nil { return err } } return nil }ui/lib/store/store.ts (1)
6-9: Type import breaks non-enterprise builds (duplicate issue).This is the same issue flagged in the previous review. The type-only import of
SCIMStateandUserStateforces TypeScript to resolve@enterprise/lib/store/slices/*at compile time, causing TS2307 errors in OSS builds where these modules don't exist.Apply the previously suggested fix:
-type EnterpriseState = { - scim?: import("@enterprise/lib/store/slices/scimSlice").SCIMState; - user?: import("@enterprise/lib/store/slices/userSlice").UserState; -}; +type EnterpriseState = { + scim?: unknown; + user?: unknown; +};This change allows TypeScript compilation to succeed while the runtime
require()fallback (lines 12-29) handles module presence gracefully.
🧹 Nitpick comments (4)
ui/app/_fallbacks/enterprise/components/login/loginView.tsx (2)
9-11: Consider adding loading state text or timeout handling.While the immediate redirect and loader display work correctly, consider enhancing the UX for potential edge cases:
useEffect(() => { + // Redirect to home if no enterprise login override is present router.push("/"); }, [router]);Optionally, add a timeout fallback in case navigation fails:
useEffect(() => { const timeout = setTimeout(() => { console.warn("Login redirect took too long, attempting fallback navigation"); router.push("/"); }, 5000); router.push("/"); return () => clearTimeout(timeout); }, [router]);
6-14: Clarify fallback LoginView and document override
- Prepend a JSDoc comment in ui/app/_fallbacks/enterprise/components/login/loginView.tsx explaining that this is a placeholder and how to override it via @enterprise/components/login/loginView.tsx.
- Add a UI‐focused README (e.g. ui/app/_fallbacks/enterprise/README.md) describing the enterprise fallback/override pattern.
framework/configstore/migrations.go (1)
403-433: Consider adding a rollback function.The migration adds profile, config, and claims columns to the Team table with proper defensive checks. However, no rollback function is provided. While optional, adding a rollback would improve migration reversibility in production environments.
Apply this diff to add a rollback function:
func migrationTeamsTableUpdates(ctx context.Context, db *gorm.DB) error { m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ ID: "add_profile_config_claims_columns_to_team_table", Migrate: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() if !migrator.HasColumn(&TableTeam{}, "profile") { if err := migrator.AddColumn(&TableTeam{}, "profile"); err != nil { return err } } if !migrator.HasColumn(&TableTeam{}, "config") { if err := migrator.AddColumn(&TableTeam{}, "config"); err != nil { return err } } if !migrator.HasColumn(&TableTeam{}, "claims") { if err := migrator.AddColumn(&TableTeam{}, "claims"); err != nil { return err } } return nil }, + Rollback: func(tx *gorm.DB) error { + tx = tx.WithContext(ctx) + migrator := tx.Migrator() + if err := migrator.DropColumn(&TableTeam{}, "claims"); err != nil { + return err + } + if err := migrator.DropColumn(&TableTeam{}, "config"); err != nil { + return err + } + if err := migrator.DropColumn(&TableTeam{}, "profile"); err != nil { + return err + } + return nil + }, }}) err := m.Migrate() if err != nil { return fmt.Errorf("error while running db migration: %s", err.Error()) } return nil }ui/lib/store/store.ts (1)
11-29: Consider simplifying the enterprise reducer loading logic.The incremental object spreading (lines 16-19, 22-25) can be streamlined for better readability.
Apply this diff to simplify:
-// Get enterprise reducers if they are available -let enterpriseReducers = {}; +// Get enterprise reducers if they are available +let enterpriseReducers: Record<string, any> = {}; try { const enterprise = require("@enterprise/lib/store/slices"); - if (enterprise.scimReducer) { - enterpriseReducers = { - ...enterpriseReducers, - scim: enterprise.scimReducer, - }; - } - if (enterprise.userReducer) { - enterpriseReducers = { - ...enterpriseReducers, - user: enterprise.userReducer, - }; - } + enterpriseReducers = { + ...(enterprise.scimReducer && { scim: enterprise.scimReducer }), + ...(enterprise.userReducer && { user: enterprise.userReducer }), + }; } catch (e) { // Enterprise reducers not available, continue without them }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ui/public/images/ms-entra.pngis excluded by!**/*.pngui/public/images/okta.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
framework/configstore/migrations.go(10 hunks)framework/configstore/migrator/migrator.go(1 hunks)framework/configstore/rdb.go(4 hunks)framework/configstore/store.go(2 hunks)framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/server.go(1 hunks)ui/app/_fallbacks/enterprise/components/login/loginView.tsx(1 hunks)ui/app/_fallbacks/enterprise/lib/store/slices/index.ts(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/login/page.tsx(1 hunks)ui/app/logs/page.tsx(2 hunks)ui/app/providers/page.tsx(1 hunks)ui/app/scim/page.tsx(1 hunks)ui/components/sidebar.tsx(6 hunks)ui/lib/store/apis/baseApi.ts(2 hunks)ui/lib/store/slices/appSlice.ts(1 hunks)ui/lib/store/slices/index.ts(1 hunks)ui/lib/store/store.ts(3 hunks)ui/next.config.ts(1 hunks)ui/package.json(1 hunks)ui/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ui/app/providers/page.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- ui/app/_fallbacks/enterprise/lib/store/slices/index.ts
- ui/app/adaptive-routing/page.tsx
- ui/lib/store/apis/baseApi.ts
- ui/tsconfig.json
- ui/components/sidebar.tsx
- ui/next.config.ts
- ui/package.json
- ui/app/scim/page.tsx
- transports/bifrost-http/handlers/server.go
- framework/configstore/store.go
🧰 Additional context used
🧬 Code graph analysis (4)
framework/configstore/migrations.go (2)
framework/configstore/migrator/migrator.go (3)
New(131-149)DefaultOptions(100-106)Migration(62-69)framework/configstore/tables.go (2)
TableTeam(623-645)TableTeam(767-767)
framework/configstore/rdb.go (1)
framework/configstore/migrator/migrator.go (3)
Migration(62-69)New(131-149)DefaultOptions(100-106)
framework/configstore/tables.go (1)
core/utils.go (1)
Ptr(12-14)
ui/app/login/page.tsx (1)
ui/app/_fallbacks/enterprise/components/login/loginView.tsx (1)
LoginView(6-14)
🔇 Additional comments (8)
ui/app/login/page.tsx (1)
1-5: LGTM! Clean wrapper implementation.The login page correctly delegates to the enterprise LoginView component. The implementation is straightforward and follows Next.js App Router conventions.
framework/configstore/migrator/migrator.go (1)
21-21: LGTM: Clean package rename.The package rename from
migrationtomigratoris straightforward and aligns with the PR objective of exposing the migration package publicly for external usage.framework/configstore/migrations.go (2)
7-7: LGTM: Import updated correctly.The import path has been updated to reflect the migrator package rename.
37-39: LGTM: New migration added to sequence.The new
migrationTeamsTableUpdatesfunction is correctly integrated into the migration sequence.framework/configstore/rdb.go (2)
45-48: LGTM: Database exposure for external control.The
DB()method intentionally exposes the underlying database connection to support external migration management, aligning with the PR's goal of allowing external usage of the migrator.
1292-1299: LGTM: Nil validation addressed.The
RunMigrationmethod now includes proper nil validation for the migration parameter, addressing the concern raised in the previous review.framework/configstore/tables.go (1)
634-641: LGTM: JSON-backed fields added correctly.The new profile, config, and claims fields follow the established pattern of storing JSON in text columns with corresponding virtual fields for runtime use.
ui/lib/store/slices/index.ts (1)
13-14: Static export of@enterprise/lib/store/slicesis safe. Tsconfig path mapping (@enterprise/*→app/_fallbacks/enterprise/*) and the webpack alias inui/next.config.tsensure this resolves to the stub fallback in non-enterprise builds.
akshaydeo
force-pushed
the
10-09-minor_extrensibility_updates_new
branch
from
ddf4f17 to
81c84fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/lib/store/store.ts (1)
61-79: Typo in serializableCheck option: ignoredActionPathsProperty name should be ignoredActionPaths (singular "Action"). Current ignoredActionsPaths is ignored by RTK.
- // Ignore these field paths in all actions - ignoredActionsPaths: ["meta.arg", "payload.timestamp"], + // Ignore these field paths in all actions + ignoredActionPaths: ["meta.arg", "payload.timestamp"],ui/lib/store/slices/appSlice.ts (1)
220-220: Remove or bridge stale selectCurrentUser selector
app.currentUser is always null (no reducers update it); either derive from the enterprise user slice or remove this selector (and the currentUser field).
ui/lib/store/slices/appSlice.ts:220
♻️ Duplicate comments (2)
ui/lib/store/store.ts (2)
6-9: Type-only imports from @enterprise break non-enterprise builds (duplicate of prior review)This forces TS to resolve enterprise modules (TS2307 in OSS). Use placeholders instead.
-type EnterpriseState = { - scim?: import("@enterprise/lib/store/slices/scimSlice").SCIMState; - user?: import("@enterprise/lib/store/slices/userSlice").UserState; -}; +type EnterpriseState = { + scim?: unknown; + user?: unknown; +};
31-45: Enterprise API injection: unclear intent and likely bug
- The side-effect comment contradicts an explicit call: baseApi.injectEndpoints(enterpriseApis.apis) (property 'apis' likely undefined).
- Keep either explicit injection with concrete endpoint definitions, or just require to trigger side effects. Also see earlier feedback about clarity.
Minimal safe fix (remove the erroneous call):
if (enterpriseApis.userApi) { // APIs are already injected into baseApi via injectEndpoints // This just ensures the module is loaded - baseApi.injectEndpoints(enterpriseApis.apis); }Or, if you want explicit injection, replace both branches with concrete calls like:
baseApi.injectEndpoints(enterpriseApis.userApi); and baseApi.injectEndpoints(enterpriseApis.scimApi);
provided those exports are endpoint definitions.
🧹 Nitpick comments (4)
framework/configstore/migrations.go (1)
404-433: LGTM! Consider adding a Rollback for completeness.The migration implementation is correct and follows the established pattern. It properly checks for column existence and uses context appropriately.
For better maintainability, consider adding a Rollback function:
}, + Rollback: func(tx *gorm.DB) error { + tx = tx.WithContext(ctx) + migrator := tx.Migrator() + if err := migrator.DropColumn(&TableTeam{}, "claims"); err != nil { + return err + } + if err := migrator.DropColumn(&TableTeam{}, "config"); err != nil { + return err + } + if err := migrator.DropColumn(&TableTeam{}, "profile"); err != nil { + return err + } + return nil + }, }})ui/lib/store/apis/baseApi.ts (1)
80-82: New RTK Query tag types: verify consistency and centralize constantsConfirm endpoints use "SCIMProviders" and "User" exactly in providesTags/invalidatesTags to avoid cache misses. Consider centralizing tag strings in a shared const to prevent typos.
ui/lib/store/slices/appSlice.ts (1)
175-177: Reset should return a fresh object to guarantee updatesReturning the same initialState object may skip updates if called twice in a row with no intervening changes. Return a new object.
- resetAppState: () => initialState, + resetAppState: () => ({ + ...initialState, + settings: { ...initialState.settings }, + features: { ...initialState.features }, + selectedItems: { ...initialState.selectedItems }, + }),ui/lib/store/store.ts (1)
83-83: RootState intersection with EnterpriseState is unnecessary after placeholder fixIf EnterpriseState uses unknown placeholders, intersection adds no usable shape. Simplify to avoid confusion.
-export type RootState = ReturnType<typeof store.getState> & EnterpriseState; +export type RootState = ReturnType<typeof store.getState>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ui/public/images/ms-entra.pngis excluded by!**/*.pngui/public/images/okta.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
framework/configstore/migrations.go(10 hunks)framework/configstore/migrator/migrator.go(1 hunks)framework/configstore/rdb.go(4 hunks)framework/configstore/store.go(2 hunks)framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/server.go(1 hunks)ui/app/_fallbacks/enterprise/components/login/loginView.tsx(1 hunks)ui/app/_fallbacks/enterprise/lib/store/slices/index.ts(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/login/page.tsx(1 hunks)ui/app/logs/page.tsx(2 hunks)ui/app/providers/page.tsx(1 hunks)ui/app/scim/page.tsx(1 hunks)ui/components/sidebar.tsx(6 hunks)ui/lib/store/apis/baseApi.ts(2 hunks)ui/lib/store/slices/appSlice.ts(1 hunks)ui/lib/store/slices/index.ts(1 hunks)ui/lib/store/store.ts(3 hunks)ui/next.config.ts(1 hunks)ui/package.json(1 hunks)ui/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ui/app/providers/page.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- transports/bifrost-http/handlers/server.go
- ui/app/_fallbacks/enterprise/components/login/loginView.tsx
- ui/app/cluster/page.tsx
- ui/app/logs/page.tsx
- ui/app/_fallbacks/enterprise/lib/store/slices/index.ts
- framework/configstore/rdb.go
- ui/app/login/page.tsx
- ui/tsconfig.json
- ui/next.config.ts
- framework/configstore/tables.go
- framework/configstore/store.go
- ui/app/scim/page.tsx
- ui/package.json
- ui/components/sidebar.tsx
- framework/configstore/migrator/migrator.go
🧰 Additional context used
🧬 Code graph analysis (2)
ui/lib/store/store.ts (1)
ui/lib/store/apis/baseApi.ts (1)
baseApi(61-84)
framework/configstore/migrations.go (2)
framework/configstore/migrator/migrator.go (3)
New(131-149)DefaultOptions(100-106)Migration(62-69)framework/configstore/tables.go (2)
TableTeam(623-645)TableTeam(773-773)
🔇 Additional comments (5)
ui/app/adaptive-routing/page.tsx (1)
1-1: LGTM! Import path correctly updated.The import path has been properly updated to reflect the new enterprise component structure, aligning with the PR's goal of reorganizing enterprise components for better maintainability.
framework/configstore/migrations.go (2)
7-7: LGTM! Package rename applied consistently.The import change from
migrationtomigratoraligns with the package rename and is consistently applied throughout the file.
37-39: LGTM! Migration call properly sequenced.The new migration is correctly placed at the end of the sequence and follows the established error-handling pattern.
ui/lib/store/apis/baseApi.ts (1)
8-8: Sending cookies with every request: confirm CORS/session settingsWith credentials: 'include', ensure the API sets Access-Control-Allow-Credentials: true, a concrete Access-Control-Allow-Origin (not '*'), and SameSite/secure flags on cookies to avoid leakage across origins.
ui/lib/store/slices/index.ts (1)
13-14: Add missing semicolon for consistencyAlias fallback (
./app/_fallbacks/enterprise/*) is already configured in tsconfig.json.export * from "@enterprise/lib/store/slices";
akshaydeo
force-pushed
the
10-09-minor_extrensibility_updates_new
branch
from
81c84fb to
8e67042
Compare
Merge activity
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
ui/components/sidebar.tsx (1)
399-410: Accessibility regression: logout control is not keyboard-accessible.Despite the previous review comment being marked as addressed, the logout trigger remains a plain
<div>withonClick, which is not keyboard-accessible. Keyboard users and assistive technology cannot activate this control.Apply this diff to make it keyboard-accessible:
- {IS_ENTERPRISE && ( - <div> - <div - className="flex items-center space-x-3" - onClick={() => { - window.location.href = "/api/logout"; - }} - > - <LogOut className="hover:text-primary text-muted-foreground h-4.5 w-4.5" size={20} strokeWidth={1.5} /> - </div> - </div> - )} + {IS_ENTERPRISE && ( + <button + type="button" + className="flex items-center space-x-3 text-muted-foreground hover:text-primary" + onClick={() => { + window.location.href = "/api/logout"; + }} + aria-label="Log out" + > + <LogOut className="h-4.5 w-4.5" size={20} strokeWidth={1.5} /> + </button> + )}ui/lib/store/store.ts (2)
5-9: Enterprise type import still breaks non-enterprise builds.As previously flagged, this type-only import forces TypeScript to resolve enterprise modules at compile time, causing TS2307 errors in OSS builds where those files don't exist. The suggested fix from the previous review (using
unknownplaceholders) has not been applied.Apply this diff to unblock non-enterprise builds:
-type EnterpriseState = { - scim?: import("@enterprise/lib/store/slices/scimSlice").SCIMState; - user?: import("@enterprise/lib/store/slices/userSlice").UserState; -}; +type EnterpriseState = { + scim?: unknown; + user?: unknown; +};
31-45: Inconsistent and incorrect API injection logic.The code has multiple issues:
- The
scimApicheck does nothing (lines 34-37)- Line 41 calls
baseApi.injectEndpoints(enterpriseApis.apis)but checks forenterpriseApis.userApi(line 38), creating a mismatch- The property name
apisdoesn't match eitherscimApioruserApiClarify whether APIs self-inject on import or need explicit injection. If explicit injection is required, apply this diff:
// Inject enterprise APIs if they are available try { const enterpriseApis = require("@enterprise/lib/store/apis"); if (enterpriseApis.scimApi) { - // APIs are already injected into baseApi via injectEndpoints - // This just ensures the module is loaded + baseApi.injectEndpoints(enterpriseApis.scimApi); } if (enterpriseApis.userApi) { - // APIs are already injected into baseApi via injectEndpoints - // This just ensures the module is loaded - baseApi.injectEndpoints(enterpriseApis.apis); + baseApi.injectEndpoints(enterpriseApis.userApi); } } catch (e) { // Enterprise APIs not available, continue without them }If APIs self-inject, remove this entire block to eliminate confusion.
framework/configstore/tables.go (1)
682-698: Guard against empty-string JSON in AfterFind.Unmarshaling "" fails. Check for non-empty before json.Unmarshal.
Apply this diff:
func (t *TableTeam) AfterFind(tx *gorm.DB) error { - if t.Profile != nil { + if t.Profile != nil && *t.Profile != "" { if err := json.Unmarshal([]byte(*t.Profile), &t.ParsedProfile); err != nil { return err } } - if t.Config != nil { + if t.Config != nil && *t.Config != "" { if err := json.Unmarshal([]byte(*t.Config), &t.ParsedConfig); err != nil { return err } } - if t.Claims != nil { + if t.Claims != nil && *t.Claims != "" { if err := json.Unmarshal([]byte(*t.Claims), &t.ParsedClaims); err != nil { return err } } return nil }
🧹 Nitpick comments (4)
transports/bifrost-http/handlers/server.go (1)
559-561: Defensive nil-check improves robustness.The nil-check before invoking
s.cancel()prevents a panic ifStart()is called beforeBootstrap(). This is good defensive programming.Optionally, consider whether
Start()should explicitly validate thatBootstrap()was called (e.g., by checking ifs.Clientors.Serverare nil) and return an error early, rather than silently skipping the cancel. This would make misuse more obvious during development.ui/app/_fallbacks/enterprise/components/login/loginView.tsx (1)
13-13: Improve accessibility of loading state.The
FullPageLoaderprovides no context to screen readers about what's happening. Users with assistive technology won't know they're being redirected or why they're seeing a loading state.Consider passing an accessible label to the loader or wrapping it with an ARIA live region:
- return <FullPageLoader />; + return ( + <div role="status" aria-live="polite"> + <FullPageLoader /> + <span className="sr-only">Redirecting to home page...</span> + </div> + );ui/lib/store/store.ts (1)
11-29: Simplify reducer spreading.The repeated spread operations are unnecessary. You can build the object more efficiently by directly assigning properties.
Apply this diff:
-// Get enterprise reducers if they are available -let enterpriseReducers = {}; +// Get enterprise reducers if they are available +let enterpriseReducers: Record<string, any> = {}; try { const enterprise = require("@enterprise/lib/store/slices"); if (enterprise.scimReducer) { - enterpriseReducers = { - ...enterpriseReducers, - scim: enterprise.scimReducer, - }; + enterpriseReducers.scim = enterprise.scimReducer; } if (enterprise.userReducer) { - enterpriseReducers = { - ...enterpriseReducers, - user: enterprise.userReducer, - }; + enterpriseReducers.user = enterprise.userReducer; } } catch (e) { // Enterprise reducers not available, continue without them }framework/configstore/migrations.go (1)
405-427: Optional: add rollback and avoid name shadowing.
- Provide a Rollback to drop added columns if needed.
- Rename local
migrator := tx.Migrator()togmto avoid shadowing the imported package name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ui/public/images/ms-entra.pngis excluded by!**/*.pngui/public/images/okta.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
framework/configstore/migrations.go(10 hunks)framework/configstore/migrator/migrator.go(1 hunks)framework/configstore/rdb.go(4 hunks)framework/configstore/store.go(2 hunks)framework/configstore/tables.go(2 hunks)transports/bifrost-http/handlers/server.go(1 hunks)ui/app/_fallbacks/enterprise/components/login/loginView.tsx(1 hunks)ui/app/_fallbacks/enterprise/lib/store/slices/index.ts(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/login/page.tsx(1 hunks)ui/app/logs/page.tsx(2 hunks)ui/app/providers/page.tsx(1 hunks)ui/app/scim/page.tsx(1 hunks)ui/components/sidebar.tsx(6 hunks)ui/lib/store/apis/baseApi.ts(2 hunks)ui/lib/store/slices/appSlice.ts(1 hunks)ui/lib/store/slices/index.ts(1 hunks)ui/lib/store/store.ts(3 hunks)ui/next.config.ts(1 hunks)ui/package.json(1 hunks)ui/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ui/lib/store/slices/index.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- ui/app/login/page.tsx
- ui/package.json
- ui/tsconfig.json
- framework/configstore/migrator/migrator.go
- ui/app/scim/page.tsx
- ui/lib/store/slices/appSlice.ts
- ui/lib/store/apis/baseApi.ts
- ui/app/logs/page.tsx
- ui/app/cluster/page.tsx
- framework/configstore/store.go
- ui/app/_fallbacks/enterprise/lib/store/slices/index.ts
🧰 Additional context used
🧬 Code graph analysis (5)
framework/configstore/tables.go (1)
core/utils.go (1)
Ptr(12-14)
ui/lib/store/store.ts (1)
ui/lib/store/apis/baseApi.ts (1)
baseApi(61-84)
framework/configstore/rdb.go (1)
framework/configstore/migrator/migrator.go (3)
Migration(62-69)New(131-149)DefaultOptions(100-106)
ui/components/sidebar.tsx (2)
ui/hooks/useWebSocket.tsx (1)
useWebSocket(93-99)ui/lib/constants/config.ts (1)
IS_ENTERPRISE(70-70)
framework/configstore/migrations.go (2)
framework/configstore/migrator/migrator.go (3)
New(131-149)DefaultOptions(100-106)Migration(62-69)framework/configstore/tables.go (2)
TableTeam(623-645)TableTeam(773-773)
🔇 Additional comments (13)
ui/app/adaptive-routing/page.tsx (1)
1-1: Approve import path refactorNo remaining references to the old
@enterprise/adaptive-routing/adaptiveRoutingViewpath found.ui/components/sidebar.tsx (2)
10-10: LGTM! Imports added correctly.The
LogOuticon andIS_ENTERPRISEflag are properly imported and used throughout the component.Also applies to: 30-30
302-326: LGTM! Conditional promo card logic is appropriate.Correctly hides the production setup help card from enterprise users, as they shouldn't be prompted to book demos.
ui/app/_fallbacks/enterprise/components/login/loginView.tsx (1)
6-14: LoginView is a fallback stub—no auth logic required
This component in_fallbacks/enterpriseis deliberately a stub that redirects to/when enterprise features are disabled; authentication is implemented elsewhere.Likely an incorrect or invalid review comment.
ui/next.config.ts (1)
35-42: LGTM! Symlink handling is appropriate.The module resolution prioritization and symlink configuration correctly support the enterprise folder being a symlink to an external directory. This ensures node_modules dependencies are resolved from the project root first.
ui/app/providers/page.tsx (1)
16-17: LGTM: import formatting only.No behavioral change.
framework/configstore/tables.go (3)
8-8: Import alias looks good.Using core.Ptr is appropriate for pointer assignment.
634-642: Team JSON columns + parsed fields are consistent.Types/tags align with migrations and omit DB-backed fields from JSON.
649-676: BeforeSave logic is correct and clears stale DB values.Serializes when maps are set; nils columns otherwise.
framework/configstore/rdb.go (3)
10-11: Migrator import path change looks correct.
45-48: DB() accessor OK.Exposing the underlying *gorm.DB is intentional for advanced use.
1292-1299: RunMigration implementation is sound.Nil check prevents panic; uses migrator.DefaultOptions.
framework/configstore/migrations.go (1)
37-39: Including teams-table column migration in trigger flow looks good.
## Summary Add user and team management infrastructure to support enterprise authentication and governance features. ## Changes - Moved migration package from internal to public to allow external usage - Added user and team database tables with profile, config, and claims fields - Added migration for user and team tables - Created login page and authentication infrastructure - Reorganized enterprise component structure for better maintainability - Added enterprise store integration with Redux - Enhanced sidebar with logout functionality for enterprise users ## Type of change - [x] Feature - [x] Refactor ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] UI (Next.js) ## How to test ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm dev || npm run dev # Navigate to /login to test authentication flow ``` ## Breaking changes - [x] No ## Security considerations This PR adds authentication infrastructure and user management capabilities, which have security implications. The implementation includes proper handling of user sessions and claims. ## Checklist - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI)
Summary
Add user and team management infrastructure to support enterprise authentication and governance features.
Changes
Type of change
Affected areas
How to test
Breaking changes
Security considerations
This PR adds authentication infrastructure and user management capabilities, which have security implications. The implementation includes proper handling of user sessions and claims.
Checklist