diff --git a/.gitignore b/.gitignore index e302d15a46..d775e70a26 100644 --- a/.gitignore +++ b/.gitignore @@ -137,6 +137,7 @@ helm/**/.values.yaml # AI Assistants /.claude/ +/.codex/ /.cursor/ /.copilot/ /.aider/ diff --git a/api/package.json b/api/package.json index 86b0f22c0b..bad538a5fc 100644 --- a/api/package.json +++ b/api/package.json @@ -118,7 +118,7 @@ }, "devDependencies": { "jest": "^30.2.0", - "mongodb-memory-server": "^10.1.4", + "mongodb-memory-server": "^11.0.1", "nodemon": "^3.0.3", "supertest": "^7.1.0" } diff --git a/api/server/routes/__tests__/mcp.spec.js b/api/server/routes/__tests__/mcp.spec.js index f194f361d3..9e3ed7a351 100644 --- a/api/server/routes/__tests__/mcp.spec.js +++ b/api/server/routes/__tests__/mcp.spec.js @@ -311,6 +311,87 @@ describe('MCP Routes', () => { expect(response.headers.location).toBe(`${basePath}/oauth/error?error=access_denied`); }); + describe('OAuth error callback failFlow', () => { + it('should fail the flow when OAuth error is received with valid CSRF cookie', async () => { + const flowId = 'test-user-id:test-server'; + const mockFlowManager = { + failFlow: jest.fn().mockResolvedValue(true), + }; + + getLogStores.mockReturnValueOnce({}); + require('~/config').getFlowStateManager.mockReturnValueOnce(mockFlowManager); + MCPOAuthHandler.resolveStateToFlowId.mockResolvedValueOnce(flowId); + + const csrfToken = generateTestCsrfToken(flowId); + const response = await request(app) + .get('/api/mcp/test-server/oauth/callback') + .set('Cookie', [`oauth_csrf=${csrfToken}`]) + .query({ + error: 'invalid_client', + state: flowId, + }); + const basePath = getBasePath(); + + expect(response.status).toBe(302); + expect(response.headers.location).toBe(`${basePath}/oauth/error?error=invalid_client`); + expect(mockFlowManager.failFlow).toHaveBeenCalledWith( + flowId, + 'mcp_oauth', + 'invalid_client', + ); + }); + + it('should fail the flow when OAuth error is received with valid session cookie', async () => { + const flowId = 'test-user-id:test-server'; + const mockFlowManager = { + failFlow: jest.fn().mockResolvedValue(true), + }; + + getLogStores.mockReturnValueOnce({}); + require('~/config').getFlowStateManager.mockReturnValueOnce(mockFlowManager); + MCPOAuthHandler.resolveStateToFlowId.mockResolvedValueOnce(flowId); + + const sessionToken = generateTestCsrfToken('test-user-id'); + const response = await request(app) + .get('/api/mcp/test-server/oauth/callback') + .set('Cookie', [`oauth_session=${sessionToken}`]) + .query({ + error: 'invalid_client', + state: flowId, + }); + const basePath = getBasePath(); + + expect(response.status).toBe(302); + expect(response.headers.location).toBe(`${basePath}/oauth/error?error=invalid_client`); + expect(mockFlowManager.failFlow).toHaveBeenCalledWith( + flowId, + 'mcp_oauth', + 'invalid_client', + ); + }); + + it('should NOT fail the flow when OAuth error is received without cookies (DoS prevention)', async () => { + const flowId = 'test-user-id:test-server'; + const mockFlowManager = { + failFlow: jest.fn(), + }; + + getLogStores.mockReturnValueOnce({}); + require('~/config').getFlowStateManager.mockReturnValueOnce(mockFlowManager); + MCPOAuthHandler.resolveStateToFlowId.mockResolvedValueOnce(flowId); + + const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({ + error: 'invalid_client', + state: flowId, + }); + const basePath = getBasePath(); + + expect(response.status).toBe(302); + expect(response.headers.location).toBe(`${basePath}/oauth/error?error=invalid_client`); + expect(mockFlowManager.failFlow).not.toHaveBeenCalled(); + }); + }); + it('should redirect to error page when code is missing', async () => { const response = await request(app).get('/api/mcp/test-server/oauth/callback').query({ state: 'test-user-id:test-server', diff --git a/api/server/routes/__tests__/roles.spec.js b/api/server/routes/__tests__/roles.spec.js new file mode 100644 index 0000000000..e78965d460 --- /dev/null +++ b/api/server/routes/__tests__/roles.spec.js @@ -0,0 +1,155 @@ +const express = require('express'); +const request = require('supertest'); +const { SystemRoles, roleDefaults } = require('librechat-data-provider'); + +const mockGetRoleByName = jest.fn(); +const mockHasCapability = jest.fn(); + +jest.mock('~/server/middleware', () => ({ + requireJwtAuth: (_req, _res, next) => next(), +})); + +jest.mock('~/server/middleware/roles/capabilities', () => ({ + hasCapability: (...args) => mockHasCapability(...args), + requireCapability: () => (_req, _res, next) => next(), +})); + +jest.mock('~/models', () => ({ + getRoleByName: (...args) => mockGetRoleByName(...args), + updateRoleByName: jest.fn(), +})); + +const rolesRouter = require('../roles'); + +function createApp(user) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.user = user; + next(); + }); + app.use('/api/roles', rolesRouter); + return app; +} + +const staffRole = { + name: 'STAFF', + permissions: { + PROMPTS: { USE: true, CREATE: false }, + }, +}; + +const userRole = roleDefaults[SystemRoles.USER]; +const adminRole = roleDefaults[SystemRoles.ADMIN]; + +beforeEach(() => { + jest.clearAllMocks(); + mockHasCapability.mockResolvedValue(false); + mockGetRoleByName.mockResolvedValue(null); +}); + +describe('GET /api/roles/:roleName — isOwnRole authorization', () => { + it('allows a custom role user to fetch their own role', async () => { + mockGetRoleByName.mockResolvedValue(staffRole); + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/STAFF'); + + expect(res.status).toBe(200); + expect(res.body.name).toBe('STAFF'); + expect(mockGetRoleByName).toHaveBeenCalledWith('STAFF', '-_id -__v'); + }); + + it('returns 403 when a custom role user requests a different custom role', async () => { + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/MANAGER'); + + expect(res.status).toBe(403); + expect(mockGetRoleByName).not.toHaveBeenCalled(); + }); + + it('returns 403 when a custom role user requests ADMIN', async () => { + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/ADMIN'); + + expect(res.status).toBe(403); + expect(mockGetRoleByName).not.toHaveBeenCalled(); + }); + + it('allows USER to fetch the USER role (roleDefaults key)', async () => { + mockGetRoleByName.mockResolvedValue(userRole); + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get(`/api/roles/${SystemRoles.USER}`); + + expect(res.status).toBe(200); + }); + + it('returns 403 when USER requests the ADMIN role', async () => { + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get(`/api/roles/${SystemRoles.ADMIN}`); + + expect(res.status).toBe(403); + }); + + it('allows ADMIN user to fetch their own ADMIN role via isOwnRole', async () => { + mockHasCapability.mockResolvedValue(false); + mockGetRoleByName.mockResolvedValue(adminRole); + const app = createApp({ id: 'u1', role: SystemRoles.ADMIN }); + + const res = await request(app).get(`/api/roles/${SystemRoles.ADMIN}`); + + expect(res.status).toBe(200); + }); + + it('allows any user with READ_ROLES capability to fetch any role', async () => { + mockHasCapability.mockResolvedValue(true); + mockGetRoleByName.mockResolvedValue(staffRole); + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get('/api/roles/STAFF'); + + expect(res.status).toBe(200); + expect(res.body.name).toBe('STAFF'); + }); + + it('returns 404 when the requested role does not exist', async () => { + mockGetRoleByName.mockResolvedValue(null); + const app = createApp({ id: 'u1', role: 'GHOST' }); + + const res = await request(app).get('/api/roles/GHOST'); + + expect(res.status).toBe(404); + }); + + it('returns 500 when getRoleByName throws', async () => { + mockGetRoleByName.mockRejectedValue(new Error('db error')); + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get(`/api/roles/${SystemRoles.USER}`); + + expect(res.status).toBe(500); + }); + + it('returns 403 for prototype property names like constructor (no prototype pollution)', async () => { + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/constructor'); + + expect(res.status).toBe(403); + expect(mockGetRoleByName).not.toHaveBeenCalled(); + }); + + it('treats hasCapability failure as no capability (does not 500)', async () => { + mockHasCapability.mockRejectedValue(new Error('capability check failed')); + const app = createApp({ id: 'u1', role: 'STAFF' }); + mockGetRoleByName.mockResolvedValue(staffRole); + + const res = await request(app).get('/api/roles/STAFF'); + + expect(res.status).toBe(200); + }); +}); diff --git a/api/server/routes/admin/auth.js b/api/server/routes/admin/auth.js index 72f23b7d52..07306ac4db 100644 --- a/api/server/routes/admin/auth.js +++ b/api/server/routes/admin/auth.js @@ -3,7 +3,12 @@ const passport = require('passport'); const crypto = require('node:crypto'); const { CacheKeys } = require('librechat-data-provider'); const { logger, SystemCapabilities } = require('@librechat/data-schemas'); -const { getAdminPanelUrl, exchangeAdminCode, createSetBalanceConfig } = require('@librechat/api'); +const { + getAdminPanelUrl, + exchangeAdminCode, + createSetBalanceConfig, + storeAndStripChallenge, +} = require('@librechat/api'); const { loginController } = require('~/server/controllers/auth/LoginController'); const { requireCapability } = require('~/server/middleware/roles/capabilities'); const { createOAuthHandler } = require('~/server/controllers/auth/oauth'); @@ -73,11 +78,6 @@ router.get('/oauth/openid/check', (req, res) => { res.status(200).json({ message: 'OpenID check successful' }); }); -/** PKCE challenge cache TTL: 5 minutes (enough for user to authenticate with IdP) */ -const PKCE_CHALLENGE_TTL = 5 * 60 * 1000; -/** Regex pattern for valid PKCE challenges: 64 hex characters (SHA-256 hex digest) */ -const PKCE_CHALLENGE_PATTERN = /^[a-f0-9]{64}$/; - /** * Generates a random hex state string for OAuth flows. * @returns {string} A 32-byte random hex string. @@ -86,27 +86,6 @@ function generateState() { return crypto.randomBytes(32).toString('hex'); } -/** - * Stores a PKCE challenge in cache keyed by state. - * @param {string} state - The OAuth state value. - * @param {string | undefined} codeChallenge - The PKCE code_challenge from query params. - * @param {string} provider - Provider name for logging. - * @returns {Promise} True if stored successfully or no challenge provided. - */ -async function storePkceChallenge(state, codeChallenge, provider) { - if (typeof codeChallenge !== 'string' || !PKCE_CHALLENGE_PATTERN.test(codeChallenge)) { - return true; - } - try { - const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); - await cache.set(`pkce:${state}`, codeChallenge, PKCE_CHALLENGE_TTL); - return true; - } catch (err) { - logger.error(`[admin/oauth/${provider}] Failed to store PKCE challenge:`, err); - return false; - } -} - /** * Middleware to retrieve PKCE challenge from cache using the OAuth state. * Reads state from req.oauthState (set by a preceding middleware). @@ -148,7 +127,8 @@ function retrievePkceChallenge(provider) { router.get('/oauth/openid', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'openid'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'openid'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/openid/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -185,7 +165,8 @@ router.get( router.get('/oauth/saml', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'saml'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'saml'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/saml/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -222,7 +203,8 @@ router.post( router.get('/oauth/google', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'google'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'google'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/google/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -260,7 +242,8 @@ router.get( router.get('/oauth/github', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'github'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'github'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/github/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -298,7 +281,8 @@ router.get( router.get('/oauth/discord', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'discord'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'discord'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/discord/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -336,7 +320,8 @@ router.get( router.get('/oauth/facebook', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'facebook'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'facebook'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/facebook/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -374,7 +359,8 @@ router.get( router.get('/oauth/apple', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'apple'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'apple'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/apple/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, diff --git a/api/server/routes/mcp.js b/api/server/routes/mcp.js index c6496ad4b4..b747e6f5ed 100644 --- a/api/server/routes/mcp.js +++ b/api/server/routes/mcp.js @@ -149,6 +149,29 @@ router.get('/:serverName/oauth/callback', async (req, res) => { if (oauthError) { logger.error('[MCP OAuth] OAuth error received', { error: oauthError }); + // Gate failFlow behind callback validation to prevent DoS via leaked state + if (state && typeof state === 'string') { + try { + const flowsCache = getLogStores(CacheKeys.FLOWS); + const flowManager = getFlowStateManager(flowsCache); + const flowId = await MCPOAuthHandler.resolveStateToFlowId(state, flowManager); + if (flowId) { + const flowParts = flowId.split(':'); + const [flowUserId] = flowParts; + const hasCsrf = validateOAuthCsrf(req, res, flowId, OAUTH_CSRF_COOKIE_PATH); + const hasSession = !hasCsrf && validateOAuthSession(req, flowUserId); + if (hasCsrf || hasSession) { + await flowManager.failFlow(flowId, 'mcp_oauth', String(oauthError)); + logger.debug('[MCP OAuth] Marked flow as FAILED with OAuth error', { + flowId, + error: oauthError, + }); + } + } + } catch (err) { + logger.debug('[MCP OAuth] Could not mark flow as failed', err); + } + } return res.redirect( `${basePath}/oauth/error?error=${encodeURIComponent(String(oauthError))}`, ); diff --git a/api/server/routes/roles.js b/api/server/routes/roles.js index 25ee47854d..456d144af1 100644 --- a/api/server/routes/roles.js +++ b/api/server/routes/roles.js @@ -71,9 +71,7 @@ const createPermissionUpdateHandler = (permissionKey) => { const config = permissionConfigs[permissionKey]; return async (req, res) => { - const { roleName: _r } = req.params; - // TODO: TEMP, use a better parsing for roleName - const roleName = _r.toUpperCase(); + const { roleName } = req.params; const updates = req.body; try { @@ -110,9 +108,7 @@ const createPermissionUpdateHandler = (permissionKey) => { * Get a specific role by name */ router.get('/:roleName', async (req, res) => { - const { roleName: _r } = req.params; - // TODO: TEMP, use a better parsing for roleName - const roleName = _r.toUpperCase(); + const { roleName } = req.params; try { let hasReadRoles = false; @@ -121,7 +117,9 @@ router.get('/:roleName', async (req, res) => { } catch (err) { logger.warn(`[GET /roles/:roleName] capability check failed: ${err.message}`); } - if (!hasReadRoles && (roleName === SystemRoles.ADMIN || !roleDefaults[roleName])) { + const isOwnRole = req.user?.role === roleName; + const isDefaultRole = Object.hasOwn(roleDefaults, roleName); + if (!hasReadRoles && !isOwnRole && (roleName === SystemRoles.ADMIN || !isDefaultRole)) { return res.status(403).send({ message: 'Unauthorized' }); } diff --git a/api/server/services/MCP.js b/api/server/services/MCP.js index dbb44740a9..ccff184d4d 100644 --- a/api/server/services/MCP.js +++ b/api/server/services/MCP.js @@ -23,7 +23,7 @@ const { getFlowStateManager, getMCPManager, } = require('~/config'); -const { findToken, createToken, updateToken } = require('~/models'); +const { findToken, createToken, updateToken, deleteTokens } = require('~/models'); const { getGraphApiToken } = require('./GraphTokenService'); const { reinitMCPServer } = require('./Tools/mcp'); const { getAppConfig } = require('./Config'); @@ -644,6 +644,7 @@ function createToolInstance({ findToken, createToken, updateToken, + deleteTokens, }, oauthStart, oauthEnd, diff --git a/client/package.json b/client/package.json index 5fe9cddcc7..f041629cbb 100644 --- a/client/package.json +++ b/client/package.json @@ -81,7 +81,7 @@ "lodash": "^4.17.23", "lucide-react": "^0.394.0", "match-sorter": "^8.1.0", - "mermaid": "^11.13.0", + "mermaid": "^11.14.0", "micromark-extension-llm-math": "^3.1.0", "qrcode.react": "^4.2.0", "rc-input-number": "^7.4.2", diff --git a/client/src/components/Chat/Messages/Message.tsx b/client/src/components/Chat/Messages/Message.tsx index 53aef812fc..fc2a79fca0 100644 --- a/client/src/components/Chat/Messages/Message.tsx +++ b/client/src/components/Chat/Messages/Message.tsx @@ -47,7 +47,6 @@ export default function Message(props: TMessageProps) { { - logger.log('icon_data', iconData, assistant, agent); const { data: endpointsConfig } = useGetEndpointsQuery(); const agentName = agent?.name ?? ''; diff --git a/client/src/components/Chat/Messages/MessageParts.tsx b/client/src/components/Chat/Messages/MessageParts.tsx index 3d13fa6ae0..2d0e3d512b 100644 --- a/client/src/components/Chat/Messages/MessageParts.tsx +++ b/client/src/components/Chat/Messages/MessageParts.tsx @@ -179,7 +179,6 @@ export default function Message(props: TMessageProps) {
- ); + return ; } else if (message.content) { - return ( - - ); + return ; } - return ( - - ); + return ; } diff --git a/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx b/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx new file mode 100644 index 0000000000..c19142494a --- /dev/null +++ b/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx @@ -0,0 +1,169 @@ +import React from 'react'; +import { render } from '@testing-library/react'; +import { EModelEndpoint } from 'librechat-data-provider'; +import type { Agent } from 'librechat-data-provider'; +import type { TMessageIcon } from '~/common'; + +jest.mock('librechat-data-provider', () => ({ + ...jest.requireActual('librechat-data-provider'), + getEndpointField: jest.fn(() => ''), +})); +jest.mock('~/data-provider', () => ({ + useGetEndpointsQuery: jest.fn(() => ({ data: {} })), +})); +jest.mock('~/utils', () => ({ + getIconEndpoint: jest.fn(() => 'agents'), +})); + +const iconRenderCount = { current: 0 }; + +jest.mock('~/components/Endpoints/ConvoIconURL', () => { + const ConvoIconURL = (props: Record) => { + iconRenderCount.current += 1; + return
; + }; + ConvoIconURL.displayName = 'ConvoIconURL'; + return { __esModule: true, default: ConvoIconURL }; +}); +jest.mock('~/components/Endpoints/Icon', () => { + const Icon = (props: Record) => { + iconRenderCount.current += 1; + return
; + }; + Icon.displayName = 'Icon'; + return { __esModule: true, default: Icon }; +}); + +import MessageIcon from '../MessageIcon'; + +const makeAgent = (overrides?: Partial): Agent => + ({ + id: 'agent_123', + name: 'GitHub Agent', + avatar: { filepath: '/images/agent-avatar.png' }, + ...overrides, + }) as Agent; + +const baseIconData: TMessageIcon = { + endpoint: EModelEndpoint.agents, + model: 'agent_123', + iconURL: undefined, + modelLabel: 'GitHub Agent', + isCreatedByUser: false, +}; + +describe('MessageIcon render cycles', () => { + beforeEach(() => { + iconRenderCount.current = 0; + }); + + it('renders once on initial mount', () => { + render(); + expect(iconRenderCount.current).toBe(1); + }); + + it('does not re-render when parent re-renders with same field values but new object references', () => { + const agent = makeAgent(); + const { rerender } = render(); + iconRenderCount.current = 0; + + rerender(); + + expect(iconRenderCount.current).toBe(0); + }); + + it('does not re-render when agent object reference changes but name and avatar are the same', () => { + const agent1 = makeAgent(); + const { rerender } = render(); + iconRenderCount.current = 0; + + const agent2 = makeAgent({ id: 'agent_456' }); + rerender(); + + expect(iconRenderCount.current).toBe(0); + }); + + it('re-renders when agent avatar filepath changes', () => { + const agent1 = makeAgent(); + const { rerender } = render(); + iconRenderCount.current = 0; + + const agent2 = makeAgent({ avatar: { filepath: '/images/new-avatar.png' } }); + rerender(); + + expect(iconRenderCount.current).toBe(1); + }); + + it('re-renders when agent goes from undefined to defined (name changes from undefined to string)', () => { + const { rerender } = render(); + iconRenderCount.current = 0; + + rerender(); + + expect(iconRenderCount.current).toBe(1); + }); + + describe('simulates message lifecycle', () => { + it('renders exactly twice during new message + streaming start: initial render + modelLabel update', () => { + const initialIconData: TMessageIcon = { + endpoint: EModelEndpoint.agents, + model: 'agent_123', + iconURL: undefined, + modelLabel: '', + isCreatedByUser: false, + }; + const agent = makeAgent(); + + const { rerender } = render(); + + const streamingIconData: TMessageIcon = { + ...initialIconData, + modelLabel: 'GitHub Agent', + }; + + rerender(); + + expect(iconRenderCount.current).toBe(2); + }); + + it('does NOT re-render on subsequent streaming chunks (content changes, isSubmitting stays true)', () => { + const iconData: TMessageIcon = { + endpoint: EModelEndpoint.agents, + model: 'agent_123', + iconURL: undefined, + modelLabel: 'GitHub Agent', + isCreatedByUser: false, + }; + const agent = makeAgent(); + + const { rerender } = render(); + iconRenderCount.current = 0; + + for (let i = 0; i < 5; i++) { + rerender(); + } + + expect(iconRenderCount.current).toBe(0); + }); + + it('does NOT re-render when agentsMap context updates with same agent data', () => { + const iconData: TMessageIcon = { + endpoint: EModelEndpoint.agents, + model: 'agent_123', + iconURL: undefined, + modelLabel: 'GitHub Agent', + isCreatedByUser: false, + }; + + const agent1 = makeAgent(); + const { rerender } = render(); + iconRenderCount.current = 0; + + const agent2 = makeAgent(); + expect(agent1).not.toBe(agent2); + rerender(); + + expect(iconRenderCount.current).toBe(0); + }); + }); +}); diff --git a/client/src/components/Messages/MessageContent.tsx b/client/src/components/Messages/MessageContent.tsx index 977e397022..67865ed397 100644 --- a/client/src/components/Messages/MessageContent.tsx +++ b/client/src/components/Messages/MessageContent.tsx @@ -48,7 +48,6 @@ export default function MessageContent(props: TMessageProps) {
= ({ const PeoplePickerAdminSettings = () => { const localize = useLocalize(); const { showToast } = useToastContext(); - const { user, roles } = useAuthContext(); + const { user } = useAuthContext(); const { mutate, isLoading } = useUpdatePeoplePickerPermissionsMutation({ onSuccess: () => { showToast({ status: 'success', message: localize('com_ui_saved') }); @@ -81,15 +81,14 @@ const PeoplePickerAdminSettings = () => { }); const [isRoleMenuOpen, setIsRoleMenuOpen] = useState(false); - const [selectedRole, setSelectedRole] = useState(SystemRoles.USER); - - const defaultValues = useMemo(() => { - const rolePerms = roles?.[selectedRole]?.permissions; - if (rolePerms) { - return rolePerms[PermissionTypes.PEOPLE_PICKER]; - } - return roleDefaults[selectedRole].permissions[PermissionTypes.PEOPLE_PICKER]; - }, [roles, selectedRole]); + const { + selectedRole, + isSelectedCustomRole, + isCustomRoleLoading, + isCustomRoleError, + defaultValues, + roleDropdownItems, + } = useRoleSelector(PermissionTypes.PEOPLE_PICKER); const { reset, @@ -100,17 +99,15 @@ const PeoplePickerAdminSettings = () => { formState: { isSubmitting }, } = useForm({ mode: 'onChange', - defaultValues, + defaultValues: defaultValues as FormValues, }); useEffect(() => { - const value = roles?.[selectedRole]?.permissions?.[PermissionTypes.PEOPLE_PICKER]; - if (value) { - reset(value); - } else { - reset(roleDefaults[selectedRole].permissions[PermissionTypes.PEOPLE_PICKER]); + if (isSelectedCustomRole && (isCustomRoleLoading || isCustomRoleError)) { + return; } - }, [roles, selectedRole, reset]); + reset(defaultValues as FormValues); + }, [isSelectedCustomRole, isCustomRoleLoading, isCustomRoleError, defaultValues, reset]); if (user?.role !== SystemRoles.ADMIN) { return null; @@ -138,21 +135,6 @@ const PeoplePickerAdminSettings = () => { mutate({ roleName: selectedRole, updates: data }); }; - const roleDropdownItems = [ - { - label: SystemRoles.USER, - onClick: () => { - setSelectedRole(SystemRoles.USER); - }, - }, - { - label: SystemRoles.ADMIN, - onClick: () => { - setSelectedRole(SystemRoles.ADMIN); - }, - }, - ]; - return ( @@ -179,7 +161,7 @@ const PeoplePickerAdminSettings = () => { isOpen={isRoleMenuOpen} setIsOpen={setIsRoleMenuOpen} trigger={ - + {selectedRole} } @@ -207,7 +189,11 @@ const PeoplePickerAdminSettings = () => {
{isSmallScreen && (
; @@ -34,7 +35,7 @@ export interface AdminSettingsDialogProps { menuId: string; /** Mutation function and loading state from the permission update hook */ mutation: { - mutate: (data: { roleName: SystemRoles; updates: Record }) => void; + mutate: (data: { roleName: string; updates: Record }) => void; isLoading: boolean; }; /** Whether to show the admin access warning when ADMIN role and USE permission is displayed (default: true) */ @@ -108,18 +109,18 @@ const AdminSettingsDialog: React.FC = ({ extraContent, }) => { const localize = useLocalize(); - const { user, roles } = useAuthContext(); + const { user } = useAuthContext(); const { mutate, isLoading } = mutation; const [isRoleMenuOpen, setIsRoleMenuOpen] = useState(false); - const [selectedRole, setSelectedRole] = useState(SystemRoles.USER); - - const defaultValues = useMemo(() => { - if (roles?.[selectedRole]?.permissions) { - return roles[selectedRole]?.permissions[permissionType]; - } - return roleDefaults[selectedRole].permissions[permissionType]; - }, [roles, selectedRole, permissionType]); + const { + selectedRole, + isSelectedCustomRole, + isCustomRoleLoading, + isCustomRoleError, + defaultValues, + roleDropdownItems, + } = useRoleSelector(permissionType); const { reset, @@ -134,12 +135,11 @@ const AdminSettingsDialog: React.FC = ({ }); useEffect(() => { - if (roles?.[selectedRole]?.permissions?.[permissionType]) { - reset(roles[selectedRole]?.permissions[permissionType]); - } else { - reset(roleDefaults[selectedRole].permissions[permissionType]); + if (isSelectedCustomRole && (isCustomRoleLoading || isCustomRoleError)) { + return; } - }, [roles, selectedRole, reset, permissionType]); + reset(defaultValues); + }, [isSelectedCustomRole, isCustomRoleLoading, isCustomRoleError, defaultValues, reset]); if (user?.role !== SystemRoles.ADMIN) { return null; @@ -149,21 +149,6 @@ const AdminSettingsDialog: React.FC = ({ mutate({ roleName: selectedRole, updates: data }); }; - const roleDropdownItems = [ - { - label: SystemRoles.USER, - onClick: () => { - setSelectedRole(SystemRoles.USER); - }, - }, - { - label: SystemRoles.ADMIN, - onClick: () => { - setSelectedRole(SystemRoles.ADMIN); - }, - }, - ]; - const defaultTrigger = (