From 424d559b5ed1706920b9d3c193469e64d2dbe3a9 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 13 Sep 2026 02:48:27 -0400 Subject: [PATCH 01/22] =?UTF-8?q?=F0=9F=AB=82=20fix:=20Scope=20People=20Pi?= =?UTF-8?q?cker=20Searches=20to=20Permitted=20Types=20(#15878)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🧭 fix: Resolve People Picker Search Types Once The people picker access check and the principal search handler each parsed the requested principal types on their own: the check honored both `type` and `types`, while the search read only `types` and treated an absent or unrecognized filter as every type. Searches now use the types the access check resolved, so a request without a usable filter covers only the types the role can view, and the Entra ID lookup follows the same set. Moves the access check into packages/api as createPeoplePickerAccess. * 🧭 refactor: Move Principal Search Handler into packages/api Lifts the search-principals handler into createPrincipalSearch with its database and Microsoft Graph calls injected, leaving the controller as wiring. The people picker access check now takes a plain permissions shape instead of the stored role document. * 🧭 refactor: Type Principal Search Requests Without Stored User Documents The exported request type now names the query, headers and the few user fields the handlers read, instead of extending ServerRequest and its IUser. --- .../controllers/PermissionsController.js | 122 +---- .../__tests__/PermissionsController.spec.js | 91 ++-- .../middleware/checkPeoplePickerAccess.js | 111 +---- .../checkPeoplePickerAccess.spec.js | 431 ---------------- packages/api/src/acl/search.spec.ts | 461 ++++++++++++++++++ packages/api/src/acl/search.ts | 313 ++++++++++++ packages/api/src/index.ts | 1 + .../src/methods/userGroup.spec.ts | 15 + 8 files changed, 828 insertions(+), 717 deletions(-) delete mode 100644 api/server/middleware/checkPeoplePickerAccess.spec.js create mode 100644 packages/api/src/acl/search.spec.ts create mode 100644 packages/api/src/acl/search.ts diff --git a/api/server/controllers/PermissionsController.js b/api/server/controllers/PermissionsController.js index 6bfe9028f97..fabc5610936 100644 --- a/api/server/controllers/PermissionsController.js +++ b/api/server/controllers/PermissionsController.js @@ -7,6 +7,7 @@ const { logger, getTenantId, SYSTEM_TENANT_ID } = require('@librechat/data-schem const { ResourceType, PrincipalType, PermissionBits } = require('librechat-data-provider'); const { enrichRemoteAgentPrincipals, + createPrincipalSearch, backfillRemoteAgentPermissions, auditInsightsPermissionChanges, getInsightsPrincipalState, @@ -471,120 +472,13 @@ const getUserEffectivePermissions = async (req, res) => { * Supports hybrid local database + Entra ID search when configured * @route GET /api/permissions/search-principals */ -const searchPrincipals = async (req, res) => { - try { - const { q: rawQuery, limit = 20, types } = req.query; - - if (typeof rawQuery !== 'string' || rawQuery.trim().length === 0) { - return res.status(400).json({ - error: 'Query parameter "q" is required and must not be empty', - }); - } - - const query = rawQuery.trim(); - - if (query.length < 2) { - return res.status(400).json({ - error: 'Query must be at least 2 characters long', - }); - } - - const searchLimit = Math.min(Math.max(1, parseInt(limit) || 10), 50); - - let typeFilters = null; - if (types) { - const typesArray = Array.isArray(types) ? types : types.split(','); - const validTypes = typesArray.filter((t) => - [PrincipalType.USER, PrincipalType.GROUP, PrincipalType.ROLE].includes(t), - ); - typeFilters = validTypes.length > 0 ? validTypes : null; - } - - const localResults = await db.searchPrincipals(query, searchLimit, typeFilters); - let allPrincipals = [...localResults]; - - const useEntraId = entraIdPrincipalFeatureEnabled(req.user); - - if (useEntraId && localResults.length < searchLimit) { - try { - let graphType = 'all'; - if (typeFilters && typeFilters.length === 1) { - const graphTypeMap = { - [PrincipalType.USER]: 'users', - [PrincipalType.GROUP]: 'groups', - }; - const mappedType = graphTypeMap[typeFilters[0]]; - if (mappedType) { - graphType = mappedType; - } - } - - const authHeader = req.headers.authorization; - const accessToken = - authHeader && authHeader.startsWith('Bearer ') ? authHeader.substring(7) : null; - - if (accessToken) { - const graphResults = await searchEntraIdPrincipals( - accessToken, - req.user.openidId, - query, - graphType, - searchLimit - localResults.length, - ); - - const localEmails = new Set( - localResults.map((p) => p.email?.toLowerCase()).filter(Boolean), - ); - const localGroupSourceIds = new Set( - localResults.map((p) => p.idOnTheSource).filter(Boolean), - ); - - for (const principal of graphResults) { - const isDuplicateByEmail = - principal.email && localEmails.has(principal.email.toLowerCase()); - const isDuplicateBySourceId = - principal.idOnTheSource && localGroupSourceIds.has(principal.idOnTheSource); - - if (!isDuplicateByEmail && !isDuplicateBySourceId) { - allPrincipals.push(principal); - } - } - } - } catch (graphError) { - logger.warn('Graph API search failed, falling back to local results:', graphError.message); - } - } - const scoredResults = allPrincipals.map((item) => ({ - ...item, - _searchScore: db.calculateRelevanceScore(item, query), - })); - - const finalResults = db - .sortPrincipalsByRelevance(scoredResults) - .slice(0, searchLimit) - .map((result) => { - const { _searchScore, ...resultWithoutScore } = result; - return resultWithoutScore; - }); - - res.status(200).json({ - query, - limit: searchLimit, - types: typeFilters, - results: finalResults, - count: finalResults.length, - sources: { - local: finalResults.filter((r) => r.source === 'local').length, - entra: finalResults.filter((r) => r.source === 'entra').length, - }, - }); - } catch (error) { - logger.error('Error searching principals:', error); - res.status(500).json({ - error: 'Failed to search principals', - }); - } -}; +const searchPrincipals = createPrincipalSearch({ + searchPrincipals: db.searchPrincipals, + calculateRelevanceScore: db.calculateRelevanceScore, + sortPrincipalsByRelevance: db.sortPrincipalsByRelevance, + entraIdPrincipalFeatureEnabled, + searchEntraIdPrincipals, +}); /** * Get user's effective permissions for all accessible resources of a type diff --git a/api/server/controllers/__tests__/PermissionsController.spec.js b/api/server/controllers/__tests__/PermissionsController.spec.js index 2a578b75e7e..0b83041a56e 100644 --- a/api/server/controllers/__tests__/PermissionsController.spec.js +++ b/api/server/controllers/__tests__/PermissionsController.spec.js @@ -9,8 +9,9 @@ jest.mock('@librechat/data-schemas', () => ({ SYSTEM_TENANT_ID: '__SYSTEM__', })); -const { AccessRoleIds, ResourceType, PrincipalType, SystemRoles } = +const { AccessRoleIds, ResourceType, PrincipalType, SystemRoles, PermissionTypes, Permissions } = jest.requireActual('librechat-data-provider'); +const { createPeoplePickerAccess } = jest.requireActual('@librechat/api'); jest.mock('librechat-data-provider', () => ({ ...jest.requireActual('librechat-data-provider'), @@ -100,68 +101,32 @@ describe('PermissionsController', () => { db.sortPrincipalsByRelevance.mockImplementation((results) => results); }); - it('rejects non-string query parameters', async () => { - const req = createMockReq({ - query: { q: ['alice'] }, - }); - const res = createMockRes(); - - await searchPrincipals(req, res); - - expect(res.status).toHaveBeenCalledWith(400); - expect(res.json).toHaveBeenCalledWith({ - error: 'Query parameter "q" is required and must not be empty', - }); - expect(db.searchPrincipals).not.toHaveBeenCalled(); - }); - - it('searches with the trimmed literal query', async () => { - db.searchPrincipals.mockResolvedValue([ - { - id: 'user-1', - type: PrincipalType.USER, - name: 'Regex [invalid User', - source: 'local', - }, - ]); - - const req = createMockReq({ - query: { q: ' [invalid ', limit: '5', types: PrincipalType.USER }, - }); - const res = createMockRes(); - - await searchPrincipals(req, res); - - expect(db.searchPrincipals).toHaveBeenCalledWith('[invalid', 5, [PrincipalType.USER]); - expect(db.calculateRelevanceScore).toHaveBeenCalledWith( - expect.objectContaining({ name: 'Regex [invalid User' }), - '[invalid', - ); - expect(res.status).toHaveBeenCalledWith(200); - expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ - query: '[invalid', - limit: 5, - count: 1, - }), - ); - }); - - it('does not expose internal error details on search failures', async () => { - db.searchPrincipals.mockRejectedValue(new Error('database failure with internal detail')); - - const req = createMockReq({ - query: { q: 'alice' }, - }); - const res = createMockRes(); - - await searchPrincipals(req, res); - - expect(res.status).toHaveBeenCalledWith(500); - expect(res.json).toHaveBeenCalledWith({ - error: 'Failed to search principals', - }); - }); + it.each([{ q: 'al', type: PrincipalType.GROUP }, { q: 'al', types: 'foobar' }, { q: 'al' }])( + 'searches only the types the people picker check resolved for %j', + async (query) => { + const checkAccess = createPeoplePickerAccess({ + getRoleByName: async () => ({ + permissions: { + [PermissionTypes.PEOPLE_PICKER]: { + [Permissions.VIEW_USERS]: false, + [Permissions.VIEW_GROUPS]: true, + [Permissions.VIEW_ROLES]: false, + }, + }, + }), + }); + const req = createMockReq({ query }); + const res = createMockRes(); + + await checkAccess(req, res, () => searchPrincipals(req, res)); + + expect(db.searchPrincipals).toHaveBeenCalledWith('al', 20, [PrincipalType.GROUP]); + expect(res.status).toHaveBeenCalledWith(200); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ types: [PrincipalType.GROUP] }), + ); + }, + ); }); describe('getResourcePermissions — principal details', () => { diff --git a/api/server/middleware/checkPeoplePickerAccess.js b/api/server/middleware/checkPeoplePickerAccess.js index 83329f56a31..016c35f082d 100644 --- a/api/server/middleware/checkPeoplePickerAccess.js +++ b/api/server/middleware/checkPeoplePickerAccess.js @@ -1,114 +1,7 @@ -const { logger } = require('@librechat/data-schemas'); -const { - PrincipalType, - PermissionTypes, - Permissions, - SystemRoles, -} = require('librechat-data-provider'); +const { createPeoplePickerAccess } = require('@librechat/api'); const { getRoleByName } = require('~/models'); -const VALID_PRINCIPAL_TYPES = new Set([ - PrincipalType.USER, - PrincipalType.GROUP, - PrincipalType.ROLE, -]); - -/** - * Middleware to check if user has permission to access people picker functionality. - * Validates requested principal types via `type` (singular) and `types` (comma-separated or array) - * query parameters against the caller's role permissions: - * - user: requires VIEW_USERS permission - * - group: requires VIEW_GROUPS permission - * - role: requires VIEW_ROLES permission - * - no type filter (mixed search): requires at least one of the above - */ -const checkPeoplePickerAccess = async (req, res, next) => { - try { - const user = req.user; - if (!user || !user.role) { - return res.status(401).json({ - error: 'Unauthorized', - message: 'Authentication required', - }); - } - - if (user.role === SystemRoles.ADMIN) { - return next(); - } - - const role = await getRoleByName(user.role); - if (!role || !role.permissions) { - return res.status(403).json({ - error: 'Forbidden', - message: 'No permissions configured for user role', - }); - } - - const { type, types } = req.query; - const peoplePickerPerms = role.permissions[PermissionTypes.PEOPLE_PICKER] || {}; - const canViewUsers = peoplePickerPerms[Permissions.VIEW_USERS] === true; - const canViewGroups = peoplePickerPerms[Permissions.VIEW_GROUPS] === true; - const canViewRoles = peoplePickerPerms[Permissions.VIEW_ROLES] === true; - - const permissionChecks = { - [PrincipalType.USER]: { - hasPermission: canViewUsers, - message: 'Insufficient permissions to search for users', - }, - [PrincipalType.GROUP]: { - hasPermission: canViewGroups, - message: 'Insufficient permissions to search for groups', - }, - [PrincipalType.ROLE]: { - hasPermission: canViewRoles, - message: 'Insufficient permissions to search for roles', - }, - }; - - const requestedTypes = new Set(); - - if (type && VALID_PRINCIPAL_TYPES.has(type)) { - requestedTypes.add(type); - } - - if (types) { - const typesArray = Array.isArray(types) ? types : types.split(','); - for (const t of typesArray) { - if (VALID_PRINCIPAL_TYPES.has(t)) { - requestedTypes.add(t); - } - } - } - - for (const requested of requestedTypes) { - const check = permissionChecks[requested]; - if (!check.hasPermission) { - return res.status(403).json({ - error: 'Forbidden', - message: check.message, - }); - } - } - - if (requestedTypes.size === 0 && !canViewUsers && !canViewGroups && !canViewRoles) { - return res.status(403).json({ - error: 'Forbidden', - message: 'Insufficient permissions to search for users, groups, or roles', - }); - } - - next(); - } catch (error) { - logger.error( - `[checkPeoplePickerAccess][${req.user?.id}] error for type=${req.query.type}, types=${req.query.types}`, - error, - ); - return res.status(500).json({ - error: 'Internal Server Error', - message: 'Failed to check permissions', - }); - } -}; +const checkPeoplePickerAccess = createPeoplePickerAccess({ getRoleByName }); module.exports = { checkPeoplePickerAccess, diff --git a/api/server/middleware/checkPeoplePickerAccess.spec.js b/api/server/middleware/checkPeoplePickerAccess.spec.js deleted file mode 100644 index 45097ef2905..00000000000 --- a/api/server/middleware/checkPeoplePickerAccess.spec.js +++ /dev/null @@ -1,431 +0,0 @@ -const { logger } = require('@librechat/data-schemas'); -const { - PrincipalType, - PermissionTypes, - Permissions, - SystemRoles, -} = require('librechat-data-provider'); -const { checkPeoplePickerAccess } = require('./checkPeoplePickerAccess'); -const { getRoleByName } = require('~/models'); - -jest.mock('~/models'); -jest.mock('@librechat/data-schemas', () => ({ - ...jest.requireActual('@librechat/data-schemas'), - logger: { - error: jest.fn(), - }, -})); - -describe('checkPeoplePickerAccess', () => { - let req, res, next; - - beforeEach(() => { - req = { - user: { id: 'user123', role: 'USER' }, - query: {}, - }; - res = { - status: jest.fn().mockReturnThis(), - json: jest.fn(), - }; - next = jest.fn(); - jest.clearAllMocks(); - }); - - it('should return 401 if user is not authenticated', async () => { - req.user = null; - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(401); - expect(res.json).toHaveBeenCalledWith({ - error: 'Unauthorized', - message: 'Authentication required', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should return 403 if role has no permissions', async () => { - getRoleByName.mockResolvedValue(null); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'No permissions configured for user role', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('allows a literal admin regardless of configured people picker permissions', async () => { - req.user.role = SystemRoles.ADMIN; - - await checkPeoplePickerAccess(req, res, next); - - expect(next).toHaveBeenCalled(); - expect(getRoleByName).not.toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - }); - - it('should allow access when searching for users with VIEW_USERS permission', async () => { - req.query.type = PrincipalType.USER; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: true, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(next).toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - }); - - it('should deny access when searching for users without VIEW_USERS permission', async () => { - req.query.type = PrincipalType.USER; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: false, - [Permissions.VIEW_GROUPS]: true, - [Permissions.VIEW_ROLES]: true, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'Insufficient permissions to search for users', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should allow access when searching for groups with VIEW_GROUPS permission', async () => { - req.query.type = PrincipalType.GROUP; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: false, - [Permissions.VIEW_GROUPS]: true, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(next).toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - }); - - it('should deny access when searching for groups without VIEW_GROUPS permission', async () => { - req.query.type = PrincipalType.GROUP; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: true, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: true, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'Insufficient permissions to search for groups', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should allow access when searching for roles with VIEW_ROLES permission', async () => { - req.query.type = PrincipalType.ROLE; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: false, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: true, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(next).toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - }); - - it('should deny access when searching for roles without VIEW_ROLES permission', async () => { - req.query.type = PrincipalType.ROLE; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: true, - [Permissions.VIEW_GROUPS]: true, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'Insufficient permissions to search for roles', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should deny access when using types param to bypass type-specific check', async () => { - req.query.types = PrincipalType.GROUP; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: true, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'Insufficient permissions to search for groups', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should deny access when types contains any unpermitted type', async () => { - req.query.types = `${PrincipalType.USER},${PrincipalType.ROLE}`; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: true, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'Insufficient permissions to search for roles', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should allow access when all requested types are permitted', async () => { - req.query.types = `${PrincipalType.USER},${PrincipalType.GROUP}`; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: true, - [Permissions.VIEW_GROUPS]: true, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(next).toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - }); - - it('should validate types when provided as array (Express qs parsing)', async () => { - req.query.types = [PrincipalType.GROUP, PrincipalType.ROLE]; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: true, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: true, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'Insufficient permissions to search for groups', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should enforce permissions for combined type and types params', async () => { - req.query.type = PrincipalType.USER; - req.query.types = PrincipalType.GROUP; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: true, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'Insufficient permissions to search for groups', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should treat all-invalid types values as mixed search', async () => { - req.query.types = 'foobar'; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: true, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(next).toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - }); - - it('should deny when types is empty string and user has no permissions', async () => { - req.query.types = ''; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: false, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'Insufficient permissions to search for users, groups, or roles', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should treat types=public as mixed search since PUBLIC is not a searchable principal type', async () => { - req.query.types = PrincipalType.PUBLIC; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: true, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(next).toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - }); - - it('should allow mixed search when user has at least one permission', async () => { - // No type specified = mixed search - req.query.type = undefined; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: false, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: true, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(next).toHaveBeenCalled(); - expect(res.status).not.toHaveBeenCalled(); - }); - - it('should deny mixed search when user has no permissions', async () => { - // No type specified = mixed search - req.query.type = undefined; - getRoleByName.mockResolvedValue({ - permissions: { - [PermissionTypes.PEOPLE_PICKER]: { - [Permissions.VIEW_USERS]: false, - [Permissions.VIEW_GROUPS]: false, - [Permissions.VIEW_ROLES]: false, - }, - }, - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'Insufficient permissions to search for users, groups, or roles', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should handle errors gracefully', async () => { - const error = new Error('Database error'); - getRoleByName.mockRejectedValue(error); - - await checkPeoplePickerAccess(req, res, next); - - expect(logger.error).toHaveBeenCalledWith( - '[checkPeoplePickerAccess][user123] error for type=undefined, types=undefined', - error, - ); - expect(res.status).toHaveBeenCalledWith(500); - expect(res.json).toHaveBeenCalledWith({ - error: 'Internal Server Error', - message: 'Failed to check permissions', - }); - expect(next).not.toHaveBeenCalled(); - }); - - it('should handle missing permissions object gracefully', async () => { - req.query.type = PrincipalType.USER; - getRoleByName.mockResolvedValue({ - permissions: {}, // No PEOPLE_PICKER permissions - }); - - await checkPeoplePickerAccess(req, res, next); - - expect(res.status).toHaveBeenCalledWith(403); - expect(res.json).toHaveBeenCalledWith({ - error: 'Forbidden', - message: 'Insufficient permissions to search for users', - }); - expect(next).not.toHaveBeenCalled(); - }); -}); diff --git a/packages/api/src/acl/search.spec.ts b/packages/api/src/acl/search.spec.ts new file mode 100644 index 00000000000..65541c3bf33 --- /dev/null +++ b/packages/api/src/acl/search.spec.ts @@ -0,0 +1,461 @@ +import { logger } from '@librechat/data-schemas'; +import { Permissions, SystemRoles, PrincipalType, PermissionTypes } from 'librechat-data-provider'; +import type { TPrincipalSearchResult } from 'librechat-data-provider'; +import type { Response } from 'express'; +import type { + PrincipalSearchDeps, + PrincipalSearchRequest, + SearchablePrincipalType, +} from './search'; +import { + createPrincipalSearch, + createPeoplePickerAccess, + getRequestedPrincipalTypes, + getEntraPrincipalSearchType, +} from './search'; + +type PickerPermissions = { + [Permissions.VIEW_USERS]?: boolean; + [Permissions.VIEW_GROUPS]?: boolean; + [Permissions.VIEW_ROLES]?: boolean; +}; + +type Query = PrincipalSearchRequest['query']; + +const { USER, GROUP, ROLE } = PrincipalType; + +const permissionsFor = (types: SearchablePrincipalType[]): PickerPermissions => ({ + [Permissions.VIEW_USERS]: types.includes(USER), + [Permissions.VIEW_GROUPS]: types.includes(GROUP), + [Permissions.VIEW_ROLES]: types.includes(ROLE), +}); + +const setup = ({ + query = {}, + role = SystemRoles.USER, + picker, + getRoleByName = jest.fn(async () => ({ + permissions: picker ? { [PermissionTypes.PEOPLE_PICKER]: picker } : {}, + })), +}: { + query?: Query; + role?: string | null; + picker?: PickerPermissions; + getRoleByName?: jest.Mock; +}) => { + const req = { user: role ? { id: 'user123', role } : undefined, query } as PrincipalSearchRequest; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + const next = jest.fn(); + const middleware = createPeoplePickerAccess({ getRoleByName }); + return { + req, + res, + next, + getRoleByName, + run: () => middleware(req, res as unknown as Response, next), + }; +}; + +const outcomeOf = ({ req, res, next }: ReturnType) => ({ + status: res.status.mock.calls[0]?.[0], + body: res.json.mock.calls[0]?.[0], + nextCalls: next.mock.calls.length, + types: req.principalSearchTypes, +}); + +const forbidden = (message: string) => ({ + status: 403, + body: { error: 'Forbidden', message }, + nextCalls: 0, + types: undefined, +}); + +const allowed = (types: SearchablePrincipalType[]) => ({ + status: undefined, + body: undefined, + nextCalls: 1, + types, +}); + +describe('getRequestedPrincipalTypes', () => { + it.each([ + [{}, []], + [{ type: GROUP }, [GROUP]], + [{ types: `${USER},${ROLE}` }, [USER, ROLE]], + [{ types: [GROUP, `${ROLE},${USER}`] }, [GROUP, ROLE, USER]], + [{ type: USER, types: `${GROUP},${USER}` }, [USER, GROUP]], + [{ types: `${PrincipalType.PUBLIC},foobar,` }, []], + [{ types: { nested: USER } }, []], + [{ type: [{ nested: USER }] }, []], + ])('reads %j as %j', (query, expected) => { + expect(getRequestedPrincipalTypes(query)).toEqual(expected); + }); +}); + +describe('getEntraPrincipalSearchType', () => { + it.each([ + [[USER, GROUP, ROLE], 'all'], + [[USER, GROUP], 'all'], + [[USER, ROLE], 'users'], + [[USER], 'users'], + [[GROUP, ROLE], 'groups'], + [[GROUP], 'groups'], + [[ROLE], null], + [[], null], + ])('maps %j to %s', (types, expected) => { + expect(getEntraPrincipalSearchType(types as SearchablePrincipalType[])).toBe(expected); + }); +}); + +describe('createPeoplePickerAccess', () => { + it('returns 401 if user is not authenticated', async () => { + const test = setup({ role: null }); + await test.run(); + + expect(test.res.status).toHaveBeenCalledWith(401); + expect(test.res.json).toHaveBeenCalledWith({ + error: 'Unauthorized', + message: 'Authentication required', + }); + expect(test.next).not.toHaveBeenCalled(); + }); + + it('returns 403 if role has no permissions', async () => { + const test = setup({ getRoleByName: jest.fn(async () => null) }); + await test.run(); + + expect(outcomeOf(test)).toEqual(forbidden('No permissions configured for user role')); + }); + + it('allows a literal admin every type without loading the role', async () => { + const test = setup({ role: SystemRoles.ADMIN }); + await test.run(); + + expect(outcomeOf(test)).toEqual(allowed([USER, GROUP, ROLE])); + expect(test.getRoleByName).not.toHaveBeenCalled(); + }); + + it('narrows a literal admin search to the requested types', async () => { + const test = setup({ role: SystemRoles.ADMIN, query: { type: GROUP } }); + await test.run(); + + expect(outcomeOf(test)).toEqual(allowed([GROUP])); + }); + + it.each<[Query, SearchablePrincipalType[], SearchablePrincipalType[]]>([ + [{ type: USER }, [USER], [USER]], + [{ type: GROUP }, [GROUP], [GROUP]], + [{ type: ROLE }, [ROLE], [ROLE]], + [{ types: `${USER},${GROUP}` }, [USER, GROUP], [USER, GROUP]], + [{ types: GROUP }, [GROUP, ROLE], [GROUP]], + ])('allows %j for a role permitting %j', async (query, permitted, expected) => { + const test = setup({ query, picker: permissionsFor(permitted) }); + await test.run(); + + expect(outcomeOf(test)).toEqual(allowed(expected)); + }); + + it.each<[Query, SearchablePrincipalType[], string]>([ + [{ type: USER }, [GROUP, ROLE], 'users'], + [{ type: GROUP }, [USER, ROLE], 'groups'], + [{ type: ROLE }, [USER, GROUP], 'roles'], + [{ types: GROUP }, [USER], 'groups'], + [{ types: `${USER},${ROLE}` }, [USER], 'roles'], + [{ types: [GROUP, ROLE] }, [USER, ROLE], 'groups'], + [{ type: USER, types: GROUP }, [USER], 'groups'], + [{ type: USER }, [], 'users'], + ])('denies %j for a role permitting %j', async (query, permitted, label) => { + const test = setup({ query, picker: permissionsFor(permitted) }); + await test.run(); + + expect(outcomeOf(test)).toEqual(forbidden(`Insufficient permissions to search for ${label}`)); + }); + + it.each<[Query, SearchablePrincipalType[]]>([ + [{}, [GROUP]], + [{}, [ROLE]], + [{}, [USER, ROLE]], + [{}, [USER, GROUP, ROLE]], + [{ types: '' }, [GROUP]], + [{ types: 'foobar' }, [USER]], + [{ types: PrincipalType.PUBLIC }, [GROUP, ROLE]], + [{ types: { nested: USER } }, [GROUP]], + ])( + 'limits an unfiltered search %j to the %j types the role permits', + async (query, permitted) => { + const test = setup({ query, picker: permissionsFor(permitted) }); + await test.run(); + + expect(outcomeOf(test)).toEqual(allowed(permitted)); + }, + ); + + it.each([{}, { types: '' }, { types: 'foobar' }])( + 'denies an unfiltered search %j when the role permits no types', + async (query) => { + const test = setup({ query, picker: permissionsFor([]) }); + await test.run(); + + expect(outcomeOf(test)).toEqual( + forbidden('Insufficient permissions to search for users, groups, or roles'), + ); + }, + ); + + it('denies a requested type when the role has no people picker permissions', async () => { + const test = setup({ query: { type: USER } }); + await test.run(); + + expect(outcomeOf(test)).toEqual(forbidden('Insufficient permissions to search for users')); + }); + + it('handles errors gracefully', async () => { + const error = new Error('Database error'); + const errorSpy = jest.spyOn(logger, 'error').mockImplementation(() => logger); + const test = setup({ getRoleByName: jest.fn().mockRejectedValue(error) }); + await test.run(); + + expect(errorSpy).toHaveBeenCalledWith( + '[checkPeoplePickerAccess][user123] error for type=undefined, types=undefined', + error, + ); + expect(test.res.status).toHaveBeenCalledWith(500); + expect(test.res.json).toHaveBeenCalledWith({ + error: 'Internal Server Error', + message: 'Failed to check permissions', + }); + expect(test.next).not.toHaveBeenCalled(); + errorSpy.mockRestore(); + }); +}); + +describe('createPrincipalSearch', () => { + type ScoredResults = Parameters[0]; + + const principal = (overrides: Partial): TPrincipalSearchResult => ({ + type: USER, + name: 'Principal', + source: 'local', + ...overrides, + }); + + const setupSearch = ({ + query = { q: 'alice' }, + types, + entraEnabled = false, + authorization = 'Bearer token', + localResults = [], + entraResults = [], + }: { + query?: Query; + types?: SearchablePrincipalType[]; + entraEnabled?: boolean; + authorization?: string; + localResults?: TPrincipalSearchResult[]; + entraResults?: TPrincipalSearchResult[]; + }) => { + const deps = { + searchPrincipals: jest.fn(async () => localResults), + calculateRelevanceScore: jest.fn((item: TPrincipalSearchResult) => item.name.length), + sortPrincipalsByRelevance: jest.fn((results: ScoredResults) => + [...results].sort((a, b) => b._searchScore - a._searchScore), + ), + entraIdPrincipalFeatureEnabled: jest.fn(() => entraEnabled), + searchEntraIdPrincipals: jest.fn(async () => entraResults), + }; + const req = { + query, + headers: { authorization }, + user: { id: 'user123', role: SystemRoles.USER, openidId: 'oid-1' }, + principalSearchTypes: types, + } as PrincipalSearchRequest; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + const search = createPrincipalSearch(deps); + return { deps, req, res, run: () => search(req, res as unknown as Response) }; + }; + + it.each([{}, { q: ['alice'] }, { q: ' ' }])('rejects the query %j', async (query) => { + const test = setupSearch({ query, types: [USER] }); + await test.run(); + + expect(test.res.status).toHaveBeenCalledWith(400); + expect(test.res.json).toHaveBeenCalledWith({ + error: 'Query parameter "q" is required and must not be empty', + }); + expect(test.deps.searchPrincipals).not.toHaveBeenCalled(); + }); + + it('rejects a one-character query', async () => { + const test = setupSearch({ query: { q: ' a ' }, types: [USER] }); + await test.run(); + + expect(test.res.status).toHaveBeenCalledWith(400); + expect(test.res.json).toHaveBeenCalledWith({ + error: 'Query must be at least 2 characters long', + }); + expect(test.deps.searchPrincipals).not.toHaveBeenCalled(); + }); + + it.each([ + [undefined, 20], + ['5', 5], + ['500', 50], + ['0', 10], + ['-3', 1], + ['abc', 10], + ])('searches the trimmed literal query with limit %s as %d', async (limit, expected) => { + const test = setupSearch({ query: { q: ' [invalid ', limit }, types: [USER] }); + await test.run(); + + expect(test.deps.searchPrincipals).toHaveBeenCalledWith('[invalid', expected, [USER]); + expect(test.res.status).toHaveBeenCalledWith(200); + expect(test.res.json).toHaveBeenCalledWith( + expect.objectContaining({ query: '[invalid', limit: expected }), + ); + }); + + it('searches only the resolved types, ignoring the query filter', async () => { + const test = setupSearch({ query: { q: 'alice', type: USER, types: USER }, types: [GROUP] }); + await test.run(); + + expect(test.deps.searchPrincipals).toHaveBeenCalledWith('alice', 20, [GROUP]); + expect(test.res.json).toHaveBeenCalledWith(expect.objectContaining({ types: [GROUP] })); + }); + + it('searches no types when the access check did not run', async () => { + const test = setupSearch({ query: { q: 'alice', types: USER }, entraEnabled: true }); + await test.run(); + + expect(test.deps.searchPrincipals).toHaveBeenCalledWith('alice', 20, []); + expect(test.deps.searchEntraIdPrincipals).not.toHaveBeenCalled(); + expect(test.res.status).toHaveBeenCalledWith(200); + }); + + it.each<[SearchablePrincipalType[], string | null]>([ + [[USER, GROUP, ROLE], 'all'], + [[USER, ROLE], 'users'], + [[GROUP, ROLE], 'groups'], + [[ROLE], null], + ])('scopes the Entra ID search for %j to %s', async (types, graphType) => { + const test = setupSearch({ + types, + entraEnabled: true, + localResults: [principal({ name: 'Alice Local', email: 'alice@local.test' })], + }); + await test.run(); + + if (graphType) { + expect(test.deps.searchEntraIdPrincipals).toHaveBeenCalledWith( + 'token', + 'oid-1', + 'alice', + graphType, + 19, + ); + } else { + expect(test.deps.searchEntraIdPrincipals).not.toHaveBeenCalled(); + } + expect(test.res.status).toHaveBeenCalledWith(200); + }); + + it.each([ + ['Entra ID search is disabled', { entraEnabled: false }], + ['there is no bearer token', { authorization: 'Basic token' }], + [ + 'local results fill the limit', + { query: { q: 'alice', limit: '1' }, localResults: [principal({ name: 'Alice' })] }, + ], + ])('skips Entra ID search when %s', async (_case, overrides) => { + const test = setupSearch({ types: [USER, GROUP], entraEnabled: true, ...overrides }); + await test.run(); + + expect(test.deps.searchEntraIdPrincipals).not.toHaveBeenCalled(); + expect(test.res.status).toHaveBeenCalledWith(200); + }); + + it('merges new Entra ID principals, ranks them, and counts sources', async () => { + const test = setupSearch({ + types: [USER, GROUP], + entraEnabled: true, + localResults: [ + principal({ name: 'Alice', email: 'alice@example.test' }), + principal({ type: GROUP, name: 'Admins', idOnTheSource: 'group-1' }), + ], + entraResults: [ + principal({ name: 'Alice Entra', email: 'ALICE@example.test', source: 'entra' }), + principal({ type: GROUP, name: 'Admins Entra', idOnTheSource: 'group-1', source: 'entra' }), + principal({ name: 'Alexandra', email: 'alexandra@example.test', source: 'entra' }), + ], + }); + await test.run(); + + expect(test.res.json).toHaveBeenCalledWith({ + query: 'alice', + limit: 20, + types: [USER, GROUP], + results: [ + principal({ name: 'Alexandra', email: 'alexandra@example.test', source: 'entra' }), + principal({ type: GROUP, name: 'Admins', idOnTheSource: 'group-1' }), + principal({ name: 'Alice', email: 'alice@example.test' }), + ], + count: 3, + sources: { local: 2, entra: 1 }, + }); + }); + + it('falls back to local results when Entra ID search fails', async () => { + const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + const local = principal({ name: 'Alice' }); + const test = setupSearch({ types: [USER], entraEnabled: true, localResults: [local] }); + test.deps.searchEntraIdPrincipals.mockRejectedValue(new Error('graph unavailable')); + await test.run(); + + expect(warnSpy).toHaveBeenCalledWith( + 'Graph API search failed, falling back to local results:', + 'graph unavailable', + ); + expect(test.res.status).toHaveBeenCalledWith(200); + expect(test.res.json).toHaveBeenCalledWith( + expect.objectContaining({ results: [local], sources: { local: 1, entra: 0 } }), + ); + warnSpy.mockRestore(); + }); + + it('does not expose internal error details on search failures', async () => { + const errorSpy = jest.spyOn(logger, 'error').mockImplementation(() => logger); + const test = setupSearch({ types: [USER] }); + const error = new Error('database failure with internal detail'); + test.deps.searchPrincipals.mockRejectedValue(error); + await test.run(); + + expect(errorSpy).toHaveBeenCalledWith('Error searching principals:', error); + expect(test.res.status).toHaveBeenCalledWith(500); + expect(test.res.json).toHaveBeenCalledWith({ error: 'Failed to search principals' }); + errorSpy.mockRestore(); + }); + + it.each([{ q: 'alice', type: GROUP }, { q: 'alice', types: 'foobar' }, { q: 'alice' }])( + 'searches local and Entra ID groups only for a groups-only role requesting %j', + async (query) => { + const test = setupSearch({ query, entraEnabled: true }); + const search = createPrincipalSearch(test.deps); + const checkAccess = createPeoplePickerAccess({ + getRoleByName: async () => ({ + permissions: { [PermissionTypes.PEOPLE_PICKER]: permissionsFor([GROUP]) }, + }), + }); + await checkAccess(test.req, test.res as unknown as Response, () => + search(test.req, test.res as unknown as Response), + ); + + expect(test.deps.searchPrincipals).toHaveBeenCalledWith('alice', 20, [GROUP]); + expect(test.deps.searchEntraIdPrincipals).toHaveBeenCalledWith( + 'token', + 'oid-1', + 'alice', + 'groups', + 20, + ); + }, + ); +}); diff --git a/packages/api/src/acl/search.ts b/packages/api/src/acl/search.ts new file mode 100644 index 00000000000..48ad57ba6e1 --- /dev/null +++ b/packages/api/src/acl/search.ts @@ -0,0 +1,313 @@ +import { logger } from '@librechat/data-schemas'; +import { Permissions, SystemRoles, PrincipalType, PermissionTypes } from 'librechat-data-provider'; +import type { + TUser, + TPeoplePickerPermissions, + TPrincipalSearchResponse, + TPrincipalSearchResult, +} from 'librechat-data-provider'; +import type { NextFunction, Request, Response } from 'express'; + +export type SearchablePrincipalType = PrincipalType.USER | PrincipalType.GROUP | PrincipalType.ROLE; + +export type EntraPrincipalSearchType = 'all' | 'users' | 'groups'; + +export type PrincipalSearchUser = Partial> & { + openidId?: string; +}; + +export type PrincipalSearchRequest = Pick & { + user?: PrincipalSearchUser; + /** Principal types the caller may search, resolved by the people picker access check. */ + principalSearchTypes?: SearchablePrincipalType[]; +}; + +export type PeoplePickerAccess = ( + req: PrincipalSearchRequest, + res: Response, + next: NextFunction, +) => Promise; + +export type PeoplePickerRole = { + permissions?: { [PermissionTypes.PEOPLE_PICKER]?: Partial }; +}; + +export type PrincipalSearch = (req: PrincipalSearchRequest, res: Response) => Promise; + +type ScoredPrincipal = TPrincipalSearchResult & { _searchScore: number }; + +export interface PrincipalSearchDeps { + searchPrincipals: ( + query: string, + limitPerType: number, + types: SearchablePrincipalType[], + ) => Promise; + calculateRelevanceScore: (item: TPrincipalSearchResult, query: string) => number; + sortPrincipalsByRelevance: (results: ScoredPrincipal[]) => ScoredPrincipal[]; + entraIdPrincipalFeatureEnabled: (user?: PrincipalSearchUser) => boolean; + searchEntraIdPrincipals: ( + accessToken: string, + sub: string | undefined, + query: string, + type: EntraPrincipalSearchType, + limit: number, + ) => Promise; +} + +const SEARCHABLE_PRINCIPAL_TYPES: readonly SearchablePrincipalType[] = [ + PrincipalType.USER, + PrincipalType.GROUP, + PrincipalType.ROLE, +]; + +const PRINCIPAL_SEARCH_PERMISSIONS: Record< + SearchablePrincipalType, + { + permission: Permissions.VIEW_USERS | Permissions.VIEW_GROUPS | Permissions.VIEW_ROLES; + label: string; + } +> = { + [PrincipalType.USER]: { permission: Permissions.VIEW_USERS, label: 'users' }, + [PrincipalType.GROUP]: { permission: Permissions.VIEW_GROUPS, label: 'groups' }, + [PrincipalType.ROLE]: { permission: Permissions.VIEW_ROLES, label: 'roles' }, +}; + +const isSearchablePrincipalType = (value: string): value is SearchablePrincipalType => + (SEARCHABLE_PRINCIPAL_TYPES as readonly string[]).includes(value); + +/** + * Collects the principal types named by the `type` and `types` query parameters, in request order. + * Each value may be a single type, a comma-separated list, or a repeated parameter. + */ +export function getRequestedPrincipalTypes( + query: PrincipalSearchRequest['query'], +): SearchablePrincipalType[] { + const requested = new Set(); + for (const value of [query.type, query.types].flat()) { + if (typeof value !== 'string') { + continue; + } + for (const name of value.split(',')) { + if (isSearchablePrincipalType(name)) { + requested.add(name); + } + } + } + return [...requested]; +} + +/** Maps resolved principal types to the Microsoft Graph search scope, or `null` when none apply. */ +export function getEntraPrincipalSearchType( + types: readonly SearchablePrincipalType[], +): EntraPrincipalSearchType | null { + const users = types.includes(PrincipalType.USER); + const groups = types.includes(PrincipalType.GROUP); + if (users && groups) { + return 'all'; + } + if (users) { + return 'users'; + } + return groups ? 'groups' : null; +} + +/** + * Creates the people picker access check. It resolves the principal types the caller may search — + * the requested types when every one is permitted, otherwise every permitted type — and stores + * them on `req.principalSearchTypes` for the search handler. + */ +export function createPeoplePickerAccess({ + getRoleByName, +}: { + getRoleByName: (roleName: string) => Promise; +}): PeoplePickerAccess { + return async (req, res, next) => { + try { + const user = req.user; + if (!user || !user.role) { + return res.status(401).json({ + error: 'Unauthorized', + message: 'Authentication required', + }); + } + + const requested = getRequestedPrincipalTypes(req.query); + + if (user.role === SystemRoles.ADMIN) { + req.principalSearchTypes = + requested.length > 0 ? requested : [...SEARCHABLE_PRINCIPAL_TYPES]; + return next(); + } + + const role = await getRoleByName(user.role); + if (!role || !role.permissions) { + return res.status(403).json({ + error: 'Forbidden', + message: 'No permissions configured for user role', + }); + } + + const peoplePickerPerms = role.permissions[PermissionTypes.PEOPLE_PICKER] ?? {}; + const canSearch = (type: SearchablePrincipalType) => + peoplePickerPerms[PRINCIPAL_SEARCH_PERMISSIONS[type].permission] === true; + + const denied = requested.find((type) => !canSearch(type)); + if (denied) { + return res.status(403).json({ + error: 'Forbidden', + message: `Insufficient permissions to search for ${PRINCIPAL_SEARCH_PERMISSIONS[denied].label}`, + }); + } + + const types = requested.length > 0 ? requested : SEARCHABLE_PRINCIPAL_TYPES.filter(canSearch); + if (types.length === 0) { + return res.status(403).json({ + error: 'Forbidden', + message: 'Insufficient permissions to search for users, groups, or roles', + }); + } + + req.principalSearchTypes = types; + return next(); + } catch (error) { + logger.error( + `[checkPeoplePickerAccess][${req.user?.id}] error for type=${req.query.type}, types=${req.query.types}`, + error, + ); + return res.status(500).json({ + error: 'Internal Server Error', + message: 'Failed to check permissions', + }); + } + }; +} + +/** Entra ID principals for the resolved types that the local results do not already hold. */ +async function findEntraPrincipals({ + req, + query, + types, + remaining, + localResults, + deps, +}: { + req: PrincipalSearchRequest; + query: string; + types: SearchablePrincipalType[]; + remaining: number; + localResults: TPrincipalSearchResult[]; + deps: Pick; +}): Promise { + const graphType = getEntraPrincipalSearchType(types); + if (remaining <= 0 || !graphType || !deps.entraIdPrincipalFeatureEnabled(req.user)) { + return []; + } + + const authHeader = req.headers.authorization; + if (!authHeader?.startsWith('Bearer ')) { + return []; + } + + try { + const graphResults = await deps.searchEntraIdPrincipals( + authHeader.substring(7), + req.user?.openidId, + query, + graphType, + remaining, + ); + + const localEmails = new Set(); + const localSourceIds = new Set(); + for (const principal of localResults) { + if (principal.email) { + localEmails.add(principal.email.toLowerCase()); + } + if (principal.idOnTheSource) { + localSourceIds.add(principal.idOnTheSource); + } + } + + return graphResults.filter( + (principal) => + !(principal.email && localEmails.has(principal.email.toLowerCase())) && + !(principal.idOnTheSource && localSourceIds.has(principal.idOnTheSource)), + ); + } catch (error) { + logger.warn( + 'Graph API search failed, falling back to local results:', + error instanceof Error ? error.message : error, + ); + return []; + } +} + +/** + * Creates the principal search handler. It searches only `req.principalSearchTypes`, as resolved by + * the people picker access check, so a request that skipped the check searches no types. + */ +export function createPrincipalSearch(deps: PrincipalSearchDeps): PrincipalSearch { + return async (req, res) => { + try { + const { q: rawQuery, limit = 20 } = req.query; + + if (typeof rawQuery !== 'string' || rawQuery.trim().length === 0) { + res.status(400).json({ + error: 'Query parameter "q" is required and must not be empty', + }); + return; + } + + const query = rawQuery.trim(); + + if (query.length < 2) { + res.status(400).json({ + error: 'Query must be at least 2 characters long', + }); + return; + } + + const searchLimit = Math.min(Math.max(1, parseInt(String(limit)) || 10), 50); + const types = req.principalSearchTypes ?? []; + + const localResults = await deps.searchPrincipals(query, searchLimit, types); + const entraResults = await findEntraPrincipals({ + req, + query, + types, + remaining: searchLimit - localResults.length, + localResults, + deps, + }); + + const scoredResults = [...localResults, ...entraResults].map((item) => ({ + ...item, + _searchScore: deps.calculateRelevanceScore(item, query), + })); + + const sources = { local: 0, entra: 0 }; + const results = deps + .sortPrincipalsByRelevance(scoredResults) + .slice(0, searchLimit) + .map(({ _searchScore, ...result }) => { + sources[result.source] += 1; + return result; + }); + + const response: TPrincipalSearchResponse = { + query, + limit: searchLimit, + types, + results, + count: results.length, + sources, + }; + res.status(200).json(response); + } catch (error) { + logger.error('Error searching principals:', error); + res.status(500).json({ + error: 'Failed to search principals', + }); + } + }; +} diff --git a/packages/api/src/index.ts b/packages/api/src/index.ts index 76d96b77e08..aefdd8e1794 100644 --- a/packages/api/src/index.ts +++ b/packages/api/src/index.ts @@ -3,6 +3,7 @@ export * from './acl/accessControlService'; export * from './acl/insightsPermissions'; export * from './acl/middleware'; export * from './acl/principals'; +export * from './acl/search'; export * from './credentials'; /* Artifacts */ export * from './artifacts'; diff --git a/packages/data-schemas/src/methods/userGroup.spec.ts b/packages/data-schemas/src/methods/userGroup.spec.ts index 69d47126e26..46ba4cdc7dc 100644 --- a/packages/data-schemas/src/methods/userGroup.spec.ts +++ b/packages/data-schemas/src/methods/userGroup.spec.ts @@ -1612,6 +1612,21 @@ describe('userGroup methods', () => { expect(results.length).toBeGreaterThanOrEqual(1); }); + it('excludes users from a GROUP and ROLE filter', async () => { + const results = await methods.searchPrincipals('a', 10, [ + PrincipalType.GROUP, + PrincipalType.ROLE, + ]); + expect(new Set(results.map((r) => r.type))).toEqual( + new Set([PrincipalType.GROUP, PrincipalType.ROLE]), + ); + }); + + it('returns no principals for an empty type filter', async () => { + const results = await methods.searchPrincipals('a', 10, []); + expect(results).toEqual([]); + }); + it('respects limitPerType', async () => { const results = await methods.searchPrincipals('a', 1); const userResults = results.filter((r) => r.type === PrincipalType.USER); From e43fed2ed400305333e8c6aec4c2c25c5ef10054 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 13 Sep 2026 03:48:22 -0400 Subject: [PATCH 02/22] =?UTF-8?q?=F0=9F=9A=84=20perf:=20Linearize=20Conver?= =?UTF-8?q?sation=20Import=20Lineage=20(#15881)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🧬 fix: Resolve Large Conversation Import Lineage in Linear Time Parent lookups, timestamp ordering, skipped-ancestor resolution and citation linking during conversation import each rescanned the batch or the text per message, so a large export spent minutes on the request. They now run in a single pass, lifted into packages/api as createChatGptLineage, linkChatGptCitations and orderMessageLineage. Fork cloning and branch collection use id maps for the same lookups. * 🧬 refactor: Lift Fork Clone Lineage Into packages/api cloneLineage assigns clone ids, re-links parents and orders timestamps, and getAllMessagesUpToParent moves over whole with its id map, so fork.js only supplies the id generator and persists the clones. --- api/server/utils/import/fork.js | 95 +----- .../utils/import/importers-timestamp.spec.js | 115 +++++++ api/server/utils/import/importers.js | 201 +------------ api/server/utils/import/importers.spec.js | 20 ++ .../api/src/conversations/chatgpt.spec.ts | 219 ++++++++++++++ packages/api/src/conversations/chatgpt.ts | 196 ++++++++++++ packages/api/src/conversations/index.ts | 2 + .../api/src/conversations/lineage.spec.ts | 282 ++++++++++++++++++ packages/api/src/conversations/lineage.ts | 214 +++++++++++++ 9 files changed, 1065 insertions(+), 279 deletions(-) create mode 100644 packages/api/src/conversations/chatgpt.spec.ts create mode 100644 packages/api/src/conversations/chatgpt.ts create mode 100644 packages/api/src/conversations/lineage.spec.ts create mode 100644 packages/api/src/conversations/lineage.ts diff --git a/api/server/utils/import/fork.js b/api/server/utils/import/fork.js index 467aabc0dea..b777eed80da 100644 --- a/api/server/utils/import/fork.js +++ b/api/server/utils/import/fork.js @@ -1,5 +1,5 @@ const { v4: uuidv4 } = require('uuid'); -const { withoutTraceRefs } = require('@librechat/api'); +const { cloneLineage, withoutTraceRefs, getAllMessagesUpToParent } = require('@librechat/api'); const { logger, tenantStorage } = require('@librechat/data-schemas'); const { EModelEndpoint, Constants, ForkOptions } = require('librechat-data-provider'); const { getConvo, getMessages, getSharedMessages } = require('~/models'); @@ -21,53 +21,12 @@ function cloneMessagesWithTimestamps( importBatchBuilder, { detachSubagentRuntime = false } = {}, ) { - const idMapping = new Map(); - - // First pass: create ID mapping and sort messages by parentMessageId - const sortedMessages = [...messagesToClone].sort((a, b) => { - if (a.parentMessageId === Constants.NO_PARENT) { - return -1; - } - if (b.parentMessageId === Constants.NO_PARENT) { - return 1; - } - return 0; - }); - - // Helper function to ensure date object - const ensureDate = (dateValue) => { - if (!dateValue) { - return new Date(); - } - return dateValue instanceof Date ? dateValue : new Date(dateValue); - }; - - // Second pass: clone messages while maintaining proper timestamps - for (const message of sortedMessages) { - const newMessageId = uuidv4(); - idMapping.set(message.messageId, newMessageId); - - const parentId = - message.parentMessageId && message.parentMessageId !== Constants.NO_PARENT - ? idMapping.get(message.parentMessageId) - : Constants.NO_PARENT; - - // If this message has a parent, ensure its timestamp is after the parent's - let createdAt = ensureDate(message.createdAt); - if (parentId !== Constants.NO_PARENT) { - const parentMessage = importBatchBuilder.messages.find((msg) => msg.messageId === parentId); - if (parentMessage) { - const parentDate = ensureDate(parentMessage.createdAt); - if (createdAt <= parentDate) { - createdAt = new Date(parentDate.getTime() + 1); - } - } - } - + const { entries, idMapping } = cloneLineage(messagesToClone, uuidv4); + for (const { source, messageId, parentMessageId, createdAt } of entries) { const clonedMessage = { - ...withoutTraceRefs(message), - messageId: newMessageId, - parentMessageId: parentId, + ...withoutTraceRefs(source), + messageId, + parentMessageId, createdAt, }; if (detachSubagentRuntime) { @@ -189,48 +148,6 @@ async function forkConversation({ } } -/** - * Retrieves all messages up to the root from the target message. - * @param {TMessage[]} messages - The list of messages to search. - * @param {string} targetMessageId - The ID of the target message. - * @returns {TMessage[]} The list of messages up to the root from the target message. - */ -function getAllMessagesUpToParent(messages, targetMessageId) { - const targetMessage = messages.find((msg) => msg.messageId === targetMessageId); - if (!targetMessage) { - return []; - } - - const pathToRoot = new Set(); - const visited = new Set(); - let current = targetMessage; - - while (current) { - if (visited.has(current.messageId)) { - break; - } - - visited.add(current.messageId); - pathToRoot.add(current.messageId); - - const currentParentId = current.parentMessageId ?? Constants.NO_PARENT; - if (currentParentId === Constants.NO_PARENT) { - break; - } - - current = messages.find((msg) => msg.messageId === currentParentId); - } - - // Include all messages that are in the path or whose parent is in the path - // Exclude children of the target message - return messages.filter( - (msg) => - (pathToRoot.has(msg.messageId) && msg.messageId !== targetMessageId) || - (pathToRoot.has(msg.parentMessageId) && msg.parentMessageId !== targetMessageId) || - msg.messageId === targetMessageId, - ); -} - /** * Retrieves all messages up to the root from the target message and its neighbors. * @param {TMessage[]} messages - The list of messages to search. diff --git a/api/server/utils/import/importers-timestamp.spec.js b/api/server/utils/import/importers-timestamp.spec.js index 268cc74c0d8..90572edd2cb 100644 --- a/api/server/utils/import/importers-timestamp.spec.js +++ b/api/server/utils/import/importers-timestamp.spec.js @@ -530,4 +530,119 @@ describe('Import Timestamp Ordering', () => { ); }); }); + + describe('Large exports', () => { + const baseTime = 1700000000; + const chatGptNode = (parent, role) => ({ + parent, + children: [], + message: { + author: { role }, + create_time: baseTime, + content: { content_type: 'text', parts: [role] }, + metadata: {}, + }, + }); + + /** These sizes import in a few hundred milliseconds; a per-message scan takes ten seconds or more. */ + const importBudgetMs = 3000; + + const importJson = async (jsonData) => { + const importBatchBuilder = new ImportBatchBuilder('user-123'); + const startedAt = performance.now(); + await getImporter(jsonData)(jsonData, 'user-123', () => importBatchBuilder); + return { messages: importBatchBuilder.messages, elapsedMs: performance.now() - startedAt }; + }; + + test('imports a flat LibreChat export whose messages all name an absent parent', async () => { + const count = 60000; + const { messages, elapsedMs } = await importJson({ + conversationId: 'large-flat', + title: 'Large flat export', + messages: Array.from({ length: count }, (_, index) => ({ + messageId: `m${index}`, + parentMessageId: 'absent-root', + text: 'x', + sender: 'user', + isCreatedByUser: true, + })), + }); + + expect(elapsedMs).toBeLessThan(importBudgetMs); + expect(messages).toHaveLength(count); + }); + + test('orders a long LibreChat chain whose timestamps all collide', async () => { + const count = 60000; + const createdAt = '2024-01-01T00:00:00.000Z'; + const { messages, elapsedMs } = await importJson({ + conversationId: 'large-chain', + title: 'Large chain export', + messages: Array.from({ length: count }, (_, index) => ({ + messageId: `m${index}`, + parentMessageId: index === 0 ? Constants.NO_PARENT : `m${index - 1}`, + text: 'x', + sender: 'user', + isCreatedByUser: true, + createdAt, + })), + }); + + expect(elapsedMs).toBeLessThan(importBudgetMs); + expect(messages).toHaveLength(count); + expect(messages[count - 1].parentMessageId).toBe(messages[count - 2].messageId); + expect(messages[count - 1].createdAt.getTime()).toBe( + new Date(createdAt).getTime() + count - 1, + ); + }); + + test('orders a long ChatGPT branch listed deepest-first', async () => { + const count = 10000; + const mapping = {}; + for (let depth = count - 1; depth >= 0; depth--) { + mapping[`n${depth}`] = chatGptNode( + depth === 0 ? null : `n${depth - 1}`, + depth % 2 ? 'assistant' : 'user', + ); + } + + const { messages, elapsedMs } = await importJson([ + { title: 'Deep branch', create_time: baseTime, mapping }, + ]); + + const byId = new Map(messages.map((message) => [message.messageId, message])); + const root = messages.find((message) => message.parentMessageId === Constants.NO_PARENT); + const leaf = messages[0]; + expect(elapsedMs).toBeLessThan(importBudgetMs); + expect(messages).toHaveLength(count); + expect(root.createdAt.getTime()).toBe(baseTime * 1000); + expect(leaf.createdAt.getTime()).toBe(baseTime * 1000 + count - 1); + expect(leaf.createdAt.getTime()).toBeGreaterThan( + byId.get(leaf.parentMessageId).createdAt.getTime(), + ); + }); + + test('attaches many ChatGPT replies behind one long run of system messages', async () => { + const systemCount = 10000; + const replyCount = 10000; + const mapping = { root: chatGptNode(null, 'user') }; + for (let index = 0; index < systemCount; index++) { + mapping[`s${index}`] = chatGptNode(index === 0 ? 'root' : `s${index - 1}`, 'system'); + } + for (let index = 0; index < replyCount; index++) { + mapping[`r${index}`] = chatGptNode(`s${systemCount - 1}`, 'assistant'); + } + + const { messages, elapsedMs } = await importJson([ + { title: 'System run', create_time: baseTime, mapping }, + ]); + + const root = messages.find((message) => message.parentMessageId === Constants.NO_PARENT); + expect(elapsedMs).toBeLessThan(importBudgetMs); + expect(messages).toHaveLength(replyCount + 1); + expect(messages.filter((message) => message.parentMessageId === root.messageId)).toHaveLength( + replyCount, + ); + }); + }); }); diff --git a/api/server/utils/import/importers.js b/api/server/utils/import/importers.js index d1d2edd29df..d496749e5cd 100644 --- a/api/server/utils/import/importers.js +++ b/api/server/utils/import/importers.js @@ -7,7 +7,12 @@ const { stripMessageUIResourceMarkers, } = require('@librechat/data-schemas'); const { EModelEndpoint, Constants, Tools, openAISettings } = require('librechat-data-provider'); -const { withoutTraceRefs } = require('@librechat/api'); +const { + withoutTraceRefs, + orderMessageLineage, + createChatGptLineage, + linkChatGptCitations, +} = require('@librechat/api'); const { getEndpointsConfig } = require('~/server/services/Config'); const { createImportBatchBuilder } = require('./importBatchBuilder'); const { resolveImportDefaultModel } = require('./defaults'); @@ -445,86 +450,7 @@ function processConversation(conv, importBatchBuilder, requestUserId, defaultMod } } - /** - * Finds the nearest valid parent by traversing up through skippable messages - * (system, reasoning_recap, thoughts). Uses iterative traversal to avoid - * stack overflow on deep chains of skippable messages. - * - * @param {string} startId - The ID of the starting parent message. - * @returns {string} The ID of the nearest valid parent message. - */ - const findValidParent = (startId) => { - const visited = new Set(); - let parentId = startId; - - while (parentId) { - if (!messageMap.has(parentId) || visited.has(parentId)) { - return Constants.NO_PARENT; - } - visited.add(parentId); - - const parentMapping = conv.mapping[parentId]; - if (!parentMapping?.message) { - return Constants.NO_PARENT; - } - - const contentType = parentMapping.message.content?.content_type; - const shouldSkip = - parentMapping.message.author?.role === 'system' || - contentType === 'reasoning_recap' || - contentType === 'thoughts'; - - if (!shouldSkip) { - return messageMap.get(parentId); - } - - parentId = parentMapping.parent; - } - - return Constants.NO_PARENT; - }; - - /** - * Helper function to find thinking content from parent chain (thoughts messages) - * @param {string} parentId - The ID of the parent message. - * @param {Set} visited - Set of already-visited IDs to prevent cycles. - * @returns {Array} The thinking content array (empty if not found). - */ - const findThinkingContent = (parentId, visited = new Set()) => { - // Guard against circular references in malformed imports - if (!parentId || visited.has(parentId)) { - return []; - } - visited.add(parentId); - - const parentMapping = conv.mapping[parentId]; - if (!parentMapping?.message) { - return []; - } - - const contentType = parentMapping.message.content?.content_type; - - // If this is a thoughts message, extract the thinking content - if (contentType === 'thoughts') { - const thoughts = parentMapping.message.content.thoughts || []; - const thinkingText = thoughts - .map((t) => t.content || t.summary || '') - .filter(Boolean) - .join('\n\n'); - - if (thinkingText) { - return [{ type: 'think', think: thinkingText }]; - } - return []; - } - - // If this is reasoning_recap, look at its parent for thoughts - if (contentType === 'reasoning_recap') { - return findThinkingContent(parentMapping.parent, visited); - } - - return []; - }; + const lineage = createChatGptLineage(conv.mapping, messageMap); // Create and save messages using the mapped IDs const messages = []; @@ -554,7 +480,7 @@ function processConversation(conv, importBatchBuilder, requestUserId, defaultMod if (!newMessageId) { continue; } - const parentMessageId = findValidParent(mapping.parent); + const parentMessageId = lineage.findValidParent(mapping.parent); const messageText = formatMessageText(mapping.message); @@ -593,7 +519,7 @@ function processConversation(conv, importBatchBuilder, requestUserId, defaultMod // For assistant messages, check if there's thinking content in the parent chain if (!isCreatedByUser) { - const thinkingContent = findThinkingContent(mapping.parent); + const thinkingContent = lineage.findThinkingContent(mapping.parent); if (thinkingContent.length > 0) { // Combine thinking content with the text response message.content = [...thinkingContent, { type: 'text', text: messageText }]; @@ -603,10 +529,7 @@ function processConversation(conv, importBatchBuilder, requestUserId, defaultMod messages.push(message); } - const cycleDetected = adjustTimestampsForOrdering(messages); - if (cycleDetected) { - breakParentCycles(messages); - } + orderMessageLineage(messages); for (const message of messages) { importBatchBuilder.saveMessage(message); @@ -633,28 +556,7 @@ function processAssistantMessage(messageData, messageText) { return messageText; } - const citations = messageData.metadata?.citations ?? []; - - const sortedCitations = [...citations].sort((a, b) => b.start_ix - a.start_ix); - - let result = messageText; - for (const citation of sortedCitations) { - if ( - !citation.metadata?.type || - citation.metadata.type !== 'webpage' || - typeof citation.start_ix !== 'number' || - typeof citation.end_ix !== 'number' || - citation.start_ix >= citation.end_ix - ) { - continue; - } - - const replacement = ` ([${citation.metadata.title}](${citation.metadata.url}))`; - - result = result.slice(0, citation.start_ix) + replacement + result.slice(citation.end_ix); - } - - return result; + return linkChatGptCitations(messageText, messageData.metadata?.citations); } /** @@ -693,85 +595,4 @@ function formatMessageText(messageData) { return messageText; } -/** - * Adjusts message timestamps to ensure children always come after parents. - * Messages are sorted by createdAt and buildTree expects parents to appear before children. - * ChatGPT exports can have slight timestamp inversions (e.g., tool call results - * arriving a few ms before their parent). Uses multiple passes to handle cascading adjustments. - * Capped at N passes (where N = message count) to guarantee termination on cyclic graphs. - * - * @param {Array} messages - Array of message objects with messageId, parentMessageId, and createdAt. - * @returns {boolean} True if cyclic parent relationships were detected. - */ -function adjustTimestampsForOrdering(messages) { - if (messages.length === 0) { - return false; - } - - const timestampMap = new Map(); - for (const msg of messages) { - timestampMap.set(msg.messageId, msg.createdAt); - } - - let hasChanges = true; - let remainingPasses = messages.length; - while (hasChanges && remainingPasses > 0) { - hasChanges = false; - remainingPasses--; - for (const message of messages) { - if (message.parentMessageId && message.parentMessageId !== Constants.NO_PARENT) { - const parentTimestamp = timestampMap.get(message.parentMessageId); - if (parentTimestamp && message.createdAt <= parentTimestamp) { - message.createdAt = new Date(parentTimestamp.getTime() + 1); - timestampMap.set(message.messageId, message.createdAt); - hasChanges = true; - } - } - } - } - - const cycleDetected = remainingPasses === 0 && hasChanges; - if (cycleDetected) { - logger.warn( - '[importers] Detected cyclic parent relationships while adjusting import timestamps', - ); - } - return cycleDetected; -} - -/** - * Severs cyclic parentMessageId back-edges so saved messages form a valid tree. - * Walks each message's parent chain; if a message is visited twice, its parentMessageId - * is set to NO_PARENT to break the cycle. - * - * @param {Array} messages - Array of message objects with messageId and parentMessageId. - */ -function breakParentCycles(messages) { - const parentLookup = new Map(); - for (const msg of messages) { - parentLookup.set(msg.messageId, msg); - } - - const settled = new Set(); - for (const message of messages) { - const chain = new Set(); - let current = message; - while (current && !settled.has(current.messageId)) { - if (chain.has(current.messageId)) { - current.parentMessageId = Constants.NO_PARENT; - break; - } - chain.add(current.messageId); - const parentId = current.parentMessageId; - if (!parentId || parentId === Constants.NO_PARENT) { - break; - } - current = parentLookup.get(parentId); - } - for (const id of chain) { - settled.add(id); - } - } -} - module.exports = { getImporter, processAssistantMessage }; diff --git a/api/server/utils/import/importers.spec.js b/api/server/utils/import/importers.spec.js index aa71772de5f..b9d945af94c 100644 --- a/api/server/utils/import/importers.spec.js +++ b/api/server/utils/import/importers.spec.js @@ -1594,6 +1594,26 @@ describe('processAssistantMessage', () => { }); }); + test('should link tens of thousands of citations in one message', () => { + const count = 40000; + const marker = '【†】'; + const span = `${'word '.repeat(19)}${marker}`; + const citations = Array.from({ length: count }, (_, index) => ({ + start_ix: (index + 1) * span.length - marker.length, + end_ix: (index + 1) * span.length, + metadata: { type: 'webpage', title: 'Source', url: 'https://example.com' }, + })); + + const text = span.repeat(count); + const startedAt = performance.now(); + const result = processAssistantMessage({ metadata: { citations } }, text); + const elapsedMs = performance.now() - startedAt; + + expect(elapsedMs).toBeLessThan(1000); + expect(result).not.toContain(marker); + expect(result.split(' ([Source](https://example.com))')).toHaveLength(count + 1); + }); + test('should handle potential ReDoS attack payloads', () => { // Test with increasing input sizes to check for exponential behavior const sizes = [32, 33, 34]; // Adding more sizes would increase test time diff --git a/packages/api/src/conversations/chatgpt.spec.ts b/packages/api/src/conversations/chatgpt.spec.ts new file mode 100644 index 00000000000..b6b6df94369 --- /dev/null +++ b/packages/api/src/conversations/chatgpt.spec.ts @@ -0,0 +1,219 @@ +import { Constants, ContentTypes } from 'librechat-data-provider'; + +import type { + ChatGptMapping, + ChatGptCitation, + ChatGptMappingNode, + ChatGptExportContent, +} from './chatgpt'; +import { createChatGptLineage, linkChatGptCitations } from './chatgpt'; + +const node = ( + parent: string | null, + role: string, + contentType: string, + extra: ChatGptExportContent = {}, +): ChatGptMappingNode => ({ + parent, + message: { author: { role }, content: { content_type: contentType, ...extra } }, +}); + +/** Export ids map to imported ids with an `imported-` prefix. */ +const importedIds = (mapping: ChatGptMapping): Map => + new Map( + Object.entries(mapping) + .filter(([, entry]) => entry?.message?.content?.content_type) + .map(([id]) => [id, `imported-${id}`]), + ); + +const webpage = (start_ix: number, end_ix: number, title: string): ChatGptCitation => ({ + start_ix, + end_ix, + metadata: { type: 'webpage', title, url: `https://example.com/${title}` }, +}); + +describe('createChatGptLineage', () => { + describe('findValidParent', () => { + it('passes over system, reasoning-recap and thoughts ancestors', () => { + const mapping: ChatGptMapping = { + user: node(null, 'user', 'text'), + system: node('user', 'system', 'text'), + thoughts: node('system', 'assistant', 'thoughts'), + recap: node('thoughts', 'assistant', 'reasoning_recap'), + }; + const lineage = createChatGptLineage(mapping, importedIds(mapping)); + + expect(lineage.findValidParent('recap')).toBe('imported-user'); + expect(lineage.findValidParent('system')).toBe('imported-user'); + expect(lineage.findValidParent('user')).toBe('imported-user'); + }); + + it('roots a message whose ancestry is missing, empty or unmapped', () => { + const mapping: ChatGptMapping = { + empty: { parent: null, message: null }, + system: node('empty', 'system', 'text'), + untyped: { parent: null, message: { author: { role: 'user' }, content: {} } }, + }; + const lineage = createChatGptLineage(mapping, importedIds(mapping)); + + expect(lineage.findValidParent(undefined)).toBe(Constants.NO_PARENT); + expect(lineage.findValidParent('absent')).toBe(Constants.NO_PARENT); + expect(lineage.findValidParent('system')).toBe(Constants.NO_PARENT); + expect(lineage.findValidParent('untyped')).toBe(Constants.NO_PARENT); + }); + + it('roots a message whose passed-over ancestors form a cycle, from any entry point', () => { + const mapping: ChatGptMapping = { + a: node('b', 'system', 'text'), + b: node('a', 'system', 'text'), + }; + const lineage = createChatGptLineage(mapping, importedIds(mapping)); + + expect(lineage.findValidParent('a')).toBe(Constants.NO_PARENT); + expect(lineage.findValidParent('b')).toBe(Constants.NO_PARENT); + }); + + it('resolves many descendants of one long run of system messages', () => { + const depth = 20_000; + const mapping: Record = { root: node(null, 'user', 'text') }; + for (let i = 0; i < depth; i++) { + mapping[`s${i}`] = node(i === 0 ? 'root' : `s${i - 1}`, 'system', 'text'); + } + const lineage = createChatGptLineage(mapping, importedIds(mapping)); + + const parents = new Set(); + const startedAt = performance.now(); + for (let i = 0; i < depth; i++) { + parents.add(lineage.findValidParent(`s${depth - 1}`)); + } + const elapsedMs = performance.now() - startedAt; + + expect(elapsedMs).toBeLessThan(1000); + expect([...parents]).toEqual(['imported-root']); + }); + }); + + describe('findThinkingContent', () => { + it('reads thoughts through reasoning recaps', () => { + const mapping: ChatGptMapping = { + thoughts: node(null, 'assistant', 'thoughts', { + thoughts: [{ content: 'first' }, { summary: 'second' }, { content: '' }], + }), + recap: node('thoughts', 'assistant', 'reasoning_recap'), + }; + const lineage = createChatGptLineage(mapping, importedIds(mapping)); + + expect(lineage.findThinkingContent('recap')).toEqual([ + { type: ContentTypes.THINK, think: 'first\n\nsecond' }, + ]); + }); + + it('returns a separate part for each response sharing the same thoughts', () => { + const mapping: ChatGptMapping = { + thoughts: node(null, 'assistant', 'thoughts', { thoughts: [{ content: 'shared' }] }), + }; + const lineage = createChatGptLineage(mapping, importedIds(mapping)); + + const first = lineage.findThinkingContent('thoughts'); + const second = lineage.findThinkingContent('thoughts'); + expect(second).toEqual(first); + expect(second[0]).not.toBe(first[0]); + }); + + it('returns nothing for other ancestors, malformed thoughts and recap cycles', () => { + const mapping: ChatGptMapping = { + text: node(null, 'user', 'text'), + malformed: node(null, 'assistant', 'thoughts', { thoughts: null }), + a: node('b', 'assistant', 'reasoning_recap'), + b: node('a', 'assistant', 'reasoning_recap'), + }; + const lineage = createChatGptLineage(mapping, importedIds(mapping)); + + expect(lineage.findThinkingContent('text')).toEqual([]); + expect(lineage.findThinkingContent('malformed')).toEqual([]); + expect(lineage.findThinkingContent('a')).toEqual([]); + expect(lineage.findThinkingContent(null)).toEqual([]); + }); + + it('follows a recap run deeper than the call stack', () => { + const depth = 100_000; + const mapping: Record = { + thoughts: node(null, 'assistant', 'thoughts', { thoughts: [{ content: 'deep' }] }), + }; + for (let i = 0; i < depth; i++) { + mapping[`r${i}`] = node(i === 0 ? 'thoughts' : `r${i - 1}`, 'assistant', 'reasoning_recap'); + } + const lineage = createChatGptLineage(mapping, importedIds(mapping)); + + expect(lineage.findThinkingContent(`r${depth - 1}`)).toEqual([ + { type: ContentTypes.THINK, think: 'deep' }, + ]); + const startedAt = performance.now(); + for (let i = 0; i < 1000; i++) { + lineage.findThinkingContent(`r${depth - 1}`); + } + expect(performance.now() - startedAt).toBeLessThan(1000); + }); + }); +}); + +describe('linkChatGptCitations', () => { + const text = 'Intro 【1†a】 middle 【2†b】 end'; + const first = text.indexOf('【1'); + const second = text.indexOf('【2'); + + it('replaces adjacent and separated markers in any listed order', () => { + const citations = [webpage(second, second + 5, 'b'), webpage(first, first + 5, 'a')]; + + expect(linkChatGptCitations(text, citations)).toBe( + 'Intro ([a](https://example.com/a)) middle ([b](https://example.com/b)) end', + ); + }); + + it('clamps an end index past the text as slice would', () => { + const tail = 'Tail 【9†z】'; + expect(linkChatGptCitations(tail, [webpage(5, tail.length + 1, 'z')])).toBe( + 'Tail ([z](https://example.com/z))', + ); + }); + + it('leaves out non-webpage, inverted, fractional, negative and overlapping citations', () => { + const citations: Array = [ + null, + { start_ix: first, end_ix: first + 5, metadata: { type: 'file', title: 'f', url: 'u' } }, + webpage(second + 5, second, 'inverted'), + webpage(first + 0.5, first + 5, 'fractional'), + webpage(-3, 2, 'negative'), + webpage(second, second + 5, 'b'), + webpage(second - 2, second + 1, 'overlap'), + ]; + + expect(linkChatGptCitations(text, citations)).toBe( + 'Intro 【1†a】 middle ([b](https://example.com/b)) end', + ); + }); + + it('returns the text unchanged when there is nothing to link', () => { + expect(linkChatGptCitations(text, undefined)).toBe(text); + expect(linkChatGptCitations(text, [])).toBe(text); + expect(linkChatGptCitations('', [webpage(0, 1, 'a')])).toBe(''); + }); + + it('links tens of thousands of citations in one message', () => { + const count = 20_000; + const marker = '【†】'; + const body = `${'word '.repeat(19)}${marker}`.repeat(count); + const span = body.length / count; + const citations = Array.from({ length: count }, (_, index) => + webpage((index + 1) * span - marker.length, (index + 1) * span, 's'), + ); + + const startedAt = performance.now(); + const linked = linkChatGptCitations(body, citations); + const elapsedMs = performance.now() - startedAt; + + expect(elapsedMs).toBeLessThan(1000); + expect(linked).not.toContain(marker); + expect(linked.split(' ([s](https://example.com/s))')).toHaveLength(count + 1); + }); +}); diff --git a/packages/api/src/conversations/chatgpt.ts b/packages/api/src/conversations/chatgpt.ts new file mode 100644 index 00000000000..b46602d9d05 --- /dev/null +++ b/packages/api/src/conversations/chatgpt.ts @@ -0,0 +1,196 @@ +import { Constants, ContentTypes } from 'librechat-data-provider'; +import type { Agents } from 'librechat-data-provider'; + +export interface ChatGptThought { + content?: string | null; + summary?: string | null; +} + +export interface ChatGptExportContent { + content_type?: string | null; + thoughts?: ChatGptThought[] | null; +} + +export interface ChatGptExportMessage { + author?: { role?: string | null } | null; + content?: ChatGptExportContent | null; +} + +export interface ChatGptMappingNode { + message?: ChatGptExportMessage | null; + parent?: string | null; +} + +export type ChatGptMapping = Readonly>; + +export interface ChatGptCitationMetadata { + type?: string; + title?: string; + url?: string; +} + +export interface ChatGptCitation { + start_ix?: number; + end_ix?: number; + metadata?: ChatGptCitationMetadata | null; +} + +type LinkableCitation = ChatGptCitation & { + start_ix: number; + end_ix: number; + metadata: ChatGptCitationMetadata; +}; + +export interface ChatGptLineage { + /** Nearest imported ancestor, passing over system, reasoning-recap and thoughts nodes. */ + findValidParent: (startId: string | null | undefined) => string; + /** Reasoning from the thoughts node behind a response, reached through any reasoning recaps. */ + findThinkingContent: (parentId: string | null | undefined) => Agents.ReasoningContentText[]; +} + +const THOUGHTS = 'thoughts'; +const REASONING_RECAP = 'reasoning_recap'; + +function isPassedOver(message: ChatGptExportMessage): boolean { + const contentType = message.content?.content_type; + return ( + message.author?.role === 'system' || contentType === REASONING_RECAP || contentType === THOUGHTS + ); +} + +function joinThoughts(thoughts: ChatGptThought[] | null | undefined): string { + if (!Array.isArray(thoughts)) { + return ''; + } + return thoughts + .map((thought) => thought?.content || thought?.summary || '') + .filter(Boolean) + .join('\n\n'); +} + +/** + * Resolves parent and reasoning ancestry for one exported ChatGPT conversation. + * Every node's answer is memoized when its walk finishes, so a long run of passed-over + * ancestors is traversed once for the whole conversation rather than once per descendant. + * `messageIds` maps export node ids to imported message ids and must not change afterward. + */ +export function createChatGptLineage( + mapping: ChatGptMapping, + messageIds: ReadonlyMap, +): ChatGptLineage { + const validParents = new Map(); + const thinkingTexts = new Map(); + + const findValidParent = (startId: string | null | undefined): string => { + const walked = new Set(); + let resolved: string = Constants.NO_PARENT; + let nodeId = startId; + while (nodeId) { + const known = validParents.get(nodeId); + if (known !== undefined) { + resolved = known; + break; + } + const node = mapping[nodeId]; + if (!messageIds.has(nodeId) || walked.has(nodeId) || !node?.message) { + break; + } + walked.add(nodeId); + if (!isPassedOver(node.message)) { + resolved = messageIds.get(nodeId) ?? Constants.NO_PARENT; + break; + } + nodeId = node.parent; + } + for (const id of walked) { + validParents.set(id, resolved); + } + return resolved; + }; + + const findThinkingText = (startId: string | null | undefined): string => { + const walked = new Set(); + let text = ''; + let nodeId = startId; + while (nodeId) { + const known = thinkingTexts.get(nodeId); + if (known !== undefined) { + text = known; + break; + } + const node = mapping[nodeId]; + if (walked.has(nodeId) || !node?.message) { + break; + } + walked.add(nodeId); + const content = node.message.content; + if (content?.content_type === THOUGHTS) { + text = joinThoughts(content.thoughts); + break; + } + if (content?.content_type !== REASONING_RECAP) { + break; + } + nodeId = node.parent; + } + for (const id of walked) { + thinkingTexts.set(id, text); + } + return text; + }; + + return { + findValidParent, + findThinkingContent: (parentId) => { + const think = findThinkingText(parentId); + return think ? [{ type: ContentTypes.THINK, think }] : []; + }, + }; +} + +const isIndex = (value: number | undefined): value is number => Number.isInteger(value); + +function isLinkableCitation( + citation: ChatGptCitation | null | undefined, +): citation is LinkableCitation { + if (citation?.metadata?.type !== 'webpage') { + return false; + } + const { start_ix: start, end_ix: end } = citation; + return isIndex(start) && isIndex(end) && start >= 0 && start < end; +} + +/** + * Replaces webpage citation markers with Markdown links in a single pass over the text. + * Citations apply from the end of the text backward, with indices past the end clamped to it + * as `String.prototype.slice` would; a citation overlapping one already applied is left out. + */ +export function linkChatGptCitations( + text: string, + citations: readonly (ChatGptCitation | null | undefined)[] | null | undefined, +): string { + if (!text || !Array.isArray(citations)) { + return text; + } + + const linkable = citations.filter(isLinkableCitation).sort((a, b) => b.start_ix - a.start_ix); + const pieces: string[] = []; + let boundary = text.length; + for (const citation of linkable) { + const end = Math.min(citation.end_ix, text.length); + if (end > boundary) { + continue; + } + pieces.push( + text.slice(end, boundary), + ` ([${citation.metadata.title}](${citation.metadata.url}))`, + ); + boundary = Math.min(citation.start_ix, text.length); + } + + if (pieces.length === 0) { + return text; + } + pieces.push(text.slice(0, boundary)); + return pieces.reverse().join(''); +} diff --git a/packages/api/src/conversations/index.ts b/packages/api/src/conversations/index.ts index 0795f87e5d4..80329340be5 100644 --- a/packages/api/src/conversations/index.ts +++ b/packages/api/src/conversations/index.ts @@ -1,2 +1,4 @@ export * from './archive'; +export * from './chatgpt'; export * from './import'; +export * from './lineage'; diff --git a/packages/api/src/conversations/lineage.spec.ts b/packages/api/src/conversations/lineage.spec.ts new file mode 100644 index 00000000000..612570acbcd --- /dev/null +++ b/packages/api/src/conversations/lineage.spec.ts @@ -0,0 +1,282 @@ +import { logger } from '@librechat/data-schemas'; +import { Constants } from 'librechat-data-provider'; + +import type { LineageMessage } from './lineage'; +import { + cloneLineage, + breakParentCycles, + orderMessageLineage, + orderParentTimestamps, + getAllMessagesUpToParent, +} from './lineage'; + +const BASE = Date.UTC(2024, 0, 1); +const ROOT: string = Constants.NO_PARENT; + +const at = (offset: number): Date => new Date(BASE + offset); + +const message = (messageId: string, parentMessageId: string, offset = 0): LineageMessage => ({ + messageId, + parentMessageId, + createdAt: at(offset), +}); + +const timeOf = (messages: LineageMessage[], messageId: string): number => + messages.find((entry) => entry.messageId === messageId)?.createdAt.getTime() ?? Number.NaN; + +/** Lists a chain deepest-first, the order that forces one pass per level on a naive fixed point. */ +const reverseChain = (length: number): LineageMessage[] => + Array.from({ length }, (_, index) => { + const depth = length - 1 - index; + return message(`m${depth}`, depth === 0 ? Constants.NO_PARENT : `m${depth - 1}`); + }); + +describe('breakParentCycles', () => { + it('leaves a forest untouched', () => { + const messages = [ + message('a', Constants.NO_PARENT), + message('b', 'a'), + message('c', 'a'), + message('d', 'missing'), + ]; + + expect(breakParentCycles(messages)).toBe(false); + expect(messages.map((entry) => entry.parentMessageId)).toEqual([ + Constants.NO_PARENT, + 'a', + 'a', + 'missing', + ]); + }); + + it('severs the link that closes a two-message cycle', () => { + const messages = [message('a', 'b'), message('b', 'a')]; + + expect(breakParentCycles(messages)).toBe(true); + expect(messages[0].parentMessageId).toBe(Constants.NO_PARENT); + expect(messages[1].parentMessageId).toBe('a'); + }); + + it('severs a self-parented message', () => { + const messages = [message('a', 'a')]; + + expect(breakParentCycles(messages)).toBe(true); + expect(messages[0].parentMessageId).toBe(Constants.NO_PARENT); + }); + + it('keeps the tail that leads into a cycle attached to it', () => { + const messages = [message('tail', 'a'), message('a', 'b'), message('b', 'a')]; + + expect(breakParentCycles(messages)).toBe(true); + expect(messages.map((entry) => entry.parentMessageId)).toEqual(['a', Constants.NO_PARENT, 'a']); + }); +}); + +describe('orderParentTimestamps', () => { + it('moves a child that sorts at or before its parent to one millisecond after it', () => { + const messages = [message('parent', Constants.NO_PARENT, 100), message('child', 'parent', 50)]; + + orderParentTimestamps(messages); + + expect(timeOf(messages, 'child')).toBe(BASE + 101); + expect(timeOf(messages, 'parent')).toBe(BASE + 100); + }); + + it('keeps a child that already sorts after its parent', () => { + const messages = [message('parent', Constants.NO_PARENT, 100), message('child', 'parent', 500)]; + + orderParentTimestamps(messages); + + expect(timeOf(messages, 'child')).toBe(BASE + 500); + }); + + it('cascades through descendants regardless of listing order', () => { + const messages = [ + message('grandchild', 'child', 0), + message('child', 'parent', 0), + message('sibling', 'parent', 300), + message('parent', Constants.NO_PARENT, 200), + ]; + + orderParentTimestamps(messages); + + expect(timeOf(messages, 'parent')).toBe(BASE + 200); + expect(timeOf(messages, 'child')).toBe(BASE + 201); + expect(timeOf(messages, 'grandchild')).toBe(BASE + 202); + expect(timeOf(messages, 'sibling')).toBe(BASE + 300); + }); + + it('does not adjust a message whose parent is not in the set', () => { + const messages = [message('orphan', 'missing', 0)]; + + orderParentTimestamps(messages); + + expect(timeOf(messages, 'orphan')).toBe(BASE); + }); + + it('does not propagate from an invalid parent timestamp', () => { + const parent = message('parent', Constants.NO_PARENT); + parent.createdAt = new Date(Number.NaN); + const messages = [parent, message('child', 'parent', 0)]; + + orderParentTimestamps(messages); + + expect(timeOf(messages, 'child')).toBe(BASE); + }); + + it('orders a deep chain listed deepest-first in a single pass', () => { + const length = 20_000; + const messages = reverseChain(length); + + const startedAt = performance.now(); + orderParentTimestamps(messages); + const elapsedMs = performance.now() - startedAt; + + expect(elapsedMs).toBeLessThan(1000); + expect(timeOf(messages, 'm0')).toBe(BASE); + expect(timeOf(messages, `m${length - 1}`)).toBe(BASE + length - 1); + }); +}); + +describe('orderMessageLineage', () => { + it('breaks cycles before ordering and reports them once', () => { + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + const messages = [message('a', 'b', 10), message('b', 'a', 10), message('c', 'b', 0)]; + + orderMessageLineage(messages); + + expect(warn).toHaveBeenCalledTimes(1); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('cyclic parent relationships')); + expect(messages.map((entry) => entry.parentMessageId)).toEqual([Constants.NO_PARENT, 'a', 'b']); + expect(timeOf(messages, 'a')).toBe(BASE + 10); + expect(timeOf(messages, 'b')).toBe(BASE + 11); + expect(timeOf(messages, 'c')).toBe(BASE + 12); + warn.mockRestore(); + }); + + it('stays silent for an acyclic lineage', () => { + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + orderMessageLineage(reverseChain(3)); + + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); +}); + +describe('cloneLineage', () => { + const sequentialIds = () => { + let next = 0; + return () => `clone-${++next}`; + }; + + it('clones roots first and re-links each child to its parent clone', () => { + const { entries, idMapping } = cloneLineage( + [ + { messageId: 'child', parentMessageId: 'root', createdAt: '2024-01-01T00:00:05.000Z' }, + { messageId: 'root', parentMessageId: ROOT, createdAt: new Date(BASE) }, + ], + sequentialIds(), + ); + + expect(entries.map((entry) => entry.source.messageId)).toEqual(['root', 'child']); + expect(idMapping).toEqual( + new Map([ + ['root', 'clone-1'], + ['child', 'clone-2'], + ]), + ); + expect(entries[0].parentMessageId).toBe(Constants.NO_PARENT); + expect(entries[1].parentMessageId).toBe('clone-1'); + expect(entries[1].createdAt.getTime()).toBe(BASE + 5000); + }); + + it('moves a clone that sorts at or before its parent clone to one millisecond after it', () => { + const { entries } = cloneLineage( + [ + { messageId: 'root', parentMessageId: ROOT, createdAt: at(100) }, + { messageId: 'child', parentMessageId: 'root', createdAt: at(100) }, + { messageId: 'grandchild', parentMessageId: 'child', createdAt: at(0) }, + ], + sequentialIds(), + ); + + expect(entries.map((entry) => entry.createdAt.getTime())).toEqual([ + BASE + 100, + BASE + 101, + BASE + 102, + ]); + }); + + it('leaves the parent link undefined when the parent is cloned later or absent', () => { + const { entries } = cloneLineage( + [ + { messageId: 'early', parentMessageId: 'late', createdAt: at(0) }, + { messageId: 'late', parentMessageId: 'missing', createdAt: at(10) }, + ], + sequentialIds(), + ); + + expect(entries.map((entry) => entry.parentMessageId)).toEqual([undefined, undefined]); + expect(entries.map((entry) => entry.createdAt.getTime())).toEqual([BASE, BASE + 10]); + }); + + it('stamps a missing timestamp with the current time', () => { + const before = Date.now(); + const { entries } = cloneLineage( + [{ messageId: 'root', parentMessageId: ROOT }], + sequentialIds(), + ); + + expect(entries[0].createdAt.getTime()).toBeGreaterThanOrEqual(before); + }); +}); + +describe('getAllMessagesUpToParent', () => { + const tree = [ + { messageId: 'root', parentMessageId: ROOT }, + { messageId: 'a', parentMessageId: 'root' }, + { messageId: 'b', parentMessageId: 'root' }, + { messageId: 'a1', parentMessageId: 'a' }, + { messageId: 'a2', parentMessageId: 'a' }, + { messageId: 'a1x', parentMessageId: 'a1' }, + { messageId: 'b1', parentMessageId: 'b' }, + ]; + + it('returns the path to the root with every sibling along it, but not the target children', () => { + expect(getAllMessagesUpToParent(tree, 'a1').map((entry) => entry.messageId)).toEqual([ + 'root', + 'a', + 'b', + 'a1', + 'a2', + ]); + }); + + it('returns nothing for an unknown target and stops on a parent cycle', () => { + expect(getAllMessagesUpToParent(tree, 'unknown')).toEqual([]); + const cyclic = [ + { messageId: 'x', parentMessageId: 'y' }, + { messageId: 'y', parentMessageId: 'x' }, + ]; + expect(getAllMessagesUpToParent(cyclic, 'x').map((entry) => entry.messageId)).toEqual([ + 'x', + 'y', + ]); + }); + + it('walks a deep chain from its leaf in one pass', () => { + const length = 100_000; + const chain = Array.from({ length }, (_, index) => ({ + messageId: `m${index}`, + parentMessageId: index === 0 ? ROOT : `m${index - 1}`, + })); + + const startedAt = performance.now(); + const branch = getAllMessagesUpToParent(chain, `m${length - 1}`); + const elapsedMs = performance.now() - startedAt; + + expect(elapsedMs).toBeLessThan(1000); + expect(branch).toHaveLength(length); + }); +}); diff --git a/packages/api/src/conversations/lineage.ts b/packages/api/src/conversations/lineage.ts new file mode 100644 index 00000000000..65d2b2500c0 --- /dev/null +++ b/packages/api/src/conversations/lineage.ts @@ -0,0 +1,214 @@ +import { logger } from '@librechat/data-schemas'; +import { Constants } from 'librechat-data-provider'; + +/** The identity, parent link and timestamp of a message whose lineage is being ordered. */ +export interface LineageMessage { + messageId: string; + parentMessageId?: string | null; + createdAt: Date; +} + +/** A message as it is read before cloning, with the timestamp still in its stored form. */ +export interface LineageSource { + messageId: string; + parentMessageId?: string | null; + createdAt?: Date | string | number | null; +} + +/** A source message's clone identity: its new id, re-linked parent and ordered timestamp. */ +export interface ClonedLineageEntry { + source: T; + messageId: string; + /** Undefined when the source's parent had not been cloned yet in clone order. */ + parentMessageId: string | undefined; + createdAt: Date; +} + +export interface ClonedLineage { + entries: ClonedLineageEntry[]; + /** Source message id to cloned message id. */ + idMapping: Map; +} + +function indexById(messages: readonly T[]): Map { + const byId = new Map(); + for (const message of messages) { + byId.set(message.messageId, message); + } + return byId; +} + +function parentOf(message: T, byId: Map): T | undefined { + const { parentMessageId } = message; + if (!parentMessageId || parentMessageId === Constants.NO_PARENT) { + return undefined; + } + return byId.get(parentMessageId); +} + +/** + * Severs the parent link that closes each cycle so the messages form a forest. + * Every message joins at most one walk, so the pass is linear in the message count. + * @returns Whether any cycle was found. + */ +export function breakParentCycles(messages: readonly T[]): boolean { + const byId = indexById(messages); + const settled = new Set(); + let cycleFound = false; + + for (const message of messages) { + const chain = new Set(); + let current: T | undefined = message; + while (current != null && !settled.has(current)) { + if (chain.has(current)) { + current.parentMessageId = Constants.NO_PARENT; + cycleFound = true; + break; + } + chain.add(current); + current = parentOf(current, byId); + } + for (const member of chain) { + settled.add(member); + } + } + + return cycleFound; +} + +/** + * Moves each child's `createdAt` to one millisecond after its parent's whenever the child + * would otherwise sort at or before it. Each chain is resolved once, downward from its + * nearest settled ancestor, so the pass is linear in the message count. A cycle that + * remains is treated as rooted where the walk re-entered it. + */ +export function orderParentTimestamps(messages: readonly T[]): void { + const byId = indexById(messages); + const settled = new Set(); + + for (const message of messages) { + const chain: T[] = []; + const pending = new Set(); + let current: T | undefined = message; + while (current != null && !settled.has(current) && !pending.has(current)) { + chain.push(current); + pending.add(current); + current = parentOf(current, byId); + } + + let parentCreatedAt = current != null && settled.has(current) ? current.createdAt : undefined; + for (let i = chain.length - 1; i >= 0; i--) { + const member = chain[i]; + if (parentCreatedAt != null && member.createdAt <= parentCreatedAt) { + member.createdAt = new Date(parentCreatedAt.getTime() + 1); + } + parentCreatedAt = member.createdAt; + settled.add(member); + } + } +} + +/** + * Makes imported messages a forest whose children sort after their parents: + * cyclic parent links are severed first, then timestamps are ordered in one pass. + */ +export function orderMessageLineage(messages: readonly T[]): void { + if (breakParentCycles(messages)) { + logger.warn( + '[importers] Detected cyclic parent relationships while adjusting import timestamps', + ); + } + orderParentTimestamps(messages); +} + +const toDate = (value: LineageSource['createdAt']): Date => { + if (!value) { + return new Date(); + } + return value instanceof Date ? value : new Date(value); +}; + +/** + * Assigns each message a new id, re-links it to its parent's clone and moves its timestamp + * after that clone's. Root messages are cloned first; a message whose parent is cloned later + * keeps an undefined parent link, as the caller's persistence decides where it attaches. + */ +export function cloneLineage( + messages: readonly T[], + createId: () => string, +): ClonedLineage { + const idMapping = new Map(); + const clonedCreatedAt = new Map(); + const ordered = [...messages].sort((a, b) => { + if (a.parentMessageId === Constants.NO_PARENT) { + return -1; + } + if (b.parentMessageId === Constants.NO_PARENT) { + return 1; + } + return 0; + }); + + const entries = ordered.map((source): ClonedLineageEntry => { + const messageId = createId(); + idMapping.set(source.messageId, messageId); + + const parentMessageId = + source.parentMessageId && source.parentMessageId !== Constants.NO_PARENT + ? idMapping.get(source.parentMessageId) + : Constants.NO_PARENT; + + let createdAt = toDate(source.createdAt); + const parentCreatedAt = + parentMessageId == null ? undefined : clonedCreatedAt.get(parentMessageId); + if (parentCreatedAt != null && createdAt <= parentCreatedAt) { + createdAt = new Date(parentCreatedAt.getTime() + 1); + } + clonedCreatedAt.set(messageId, createdAt); + + return { source, messageId, parentMessageId, createdAt }; + }); + + return { entries, idMapping }; +} + +/** + * Retrieves the target message, its ancestors up to the root, and every sibling along that + * path, excluding the target's own children. The first message listed under an id wins. + */ +export function getAllMessagesUpToParent>( + messages: readonly T[], + targetMessageId: string, +): T[] { + const messagesById = new Map(); + for (const message of messages) { + if (!messagesById.has(message.messageId)) { + messagesById.set(message.messageId, message); + } + } + + const targetMessage = messagesById.get(targetMessageId); + if (!targetMessage) { + return []; + } + + const pathToRoot = new Set(); + let current: T | undefined = targetMessage; + while (current != null && !pathToRoot.has(current.messageId)) { + pathToRoot.add(current.messageId); + const parentId: string = current.parentMessageId ?? Constants.NO_PARENT; + if (parentId === Constants.NO_PARENT) { + break; + } + current = messagesById.get(parentId); + } + + return messages.filter( + (message) => + message.messageId === targetMessageId || + (pathToRoot.has(message.messageId) && message.messageId !== targetMessageId) || + (message.parentMessageId != null && + pathToRoot.has(message.parentMessageId) && + message.parentMessageId !== targetMessageId), + ); +} From 9bf51fc30267ecfa2a430a814b0db5b31cebed60 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Sun, 13 Sep 2026 16:40:00 +0200 Subject: [PATCH 03/22] =?UTF-8?q?=F0=9F=A7=B7=20style:=20Sidebar=20Row=20C?= =?UTF-8?q?ontrols,=20Pinned=20Unpin=20Badge=20and=20Drag=20Rules=20(#1587?= =?UTF-8?q?0)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🔘 style: Seat Sidebar Row Actions One Radius Inside Their Row Every control that sits on a sidebar row took the base `rounded-lg`, the same radius as the row hosting it, so a hovered button filled corner to corner with its row. `row-action` and `section-action` now carry `rounded-md`, one step inside the rows, which stay as they were. Three controls also diverged from the recipe they were built on. `ConvoOptions` and the Projects heading action overrode the variant's hover fill with `surface-hover`; the rename form's save and cancel buttons answered the pointer with `hover:opacity-70` and no fill at all. All four now take the shared hover. The project row's two actions are the exception that earns its own recipe: they sit on a row that fills on hover, where the variant's hover surface reads as a second, weaker hover stacked on the first. They take `surface-active` instead, under the pointer and while the menu one of them owns is open, and sit 4px apart like a pinned chat's own pair. * 🧲 fix: Give Both Pinned Row Kinds One Unpin Badge, and Stop Its Flicker Pinned chats and pinned agents/models sit in one list and are dragged against each other, but their unpin badges were two controls: the chat's was `text-text-primary` and always visible, the favorite's was a hand-rolled copy of the shared row action, `text-text-secondary`, revealed on hover. `UnpinButton` is now the single badge both rows render. It keeps the favorites' reveal, holds open while the row's overflow menu is, and is inert while hidden so it cannot swallow a click meant for the row. The reveal is asked for in JS rather than through an `@media (hover: hover)` variant: the variant has to beat the `group-hover` rules that reveal the badge, and it loses that cascade for `opacity` while winning it for `pointer-events`, which left the badge visible and unclickable. Two things made a row's controls flicker under the pointer: - The overflow slot took its width from its content, and the menu mounts a tick after the row is hovered. The unpin badge beside it slid 28px left at that moment, out from under the pointer, mid-fill; whatever landed there started its own. The slot now reserves its width from the row's hover, and `shrink-0` keeps a growing sibling from squeezing it. - `hasInteracted` unmounted that menu on every leave, so each entry mounted a fresh button one frame late into an already-revealed slot, restarting its fill from transparent. A row that has been reached now keeps its control for as long as the row lives; virtualization still drops the whole row. * 🧺 feat: Unpin a Chat by Dropping It on Chats, and Keep Pinned Kinds Apart Dropping a chat on the Chats section filed it out of its project but left a pinned one pinned, so the section it landed in was not the section it went to. The drop now also unpins, and accepts a pinned chat that is already in the root list; a chat that is both filed and pinned comes out of both, each call a no-op for the half that already holds. Pinned chats and pinned agents/models could be dragged through each other, which the list cannot honour: the two are ordered independently and a row dropped into the other group snapped back. Each row now accepts only its own drag type, so the other kind never previews a displacement, and Alt+Arrow refuses the same step. An order saved while the two interleaved is read back grouped, or a row would be walled in by neighbours it is not allowed to swap with. * 🧹 fix: Unnest the Row Action Width Ternary and Correct the Pinned Doc The rebase onto `dev` left two things the branch's own checks catch: the width branch that reserves a chat row's action slot had grown a nested ternary, which the repository's ESLint forbids, and `dnd.ts` carried an import out of order. The section's own doc comment still promised that favorites and conversations "interleave freely", which this branch stopped being true: a row only reorders against its own kind, and a stored order is read back grouped. * 🪃 fix: Keep a Pinned Reorder That Was Released on a Row Reordering the Pinned section with the pointer never survived the release. The rows shifted under the drag, and the moment the button came up they snapped back to where they had started; nothing was written, so a reload showed the old order too. Only Alt+Arrow could actually reorder the list. A row is a drop target — that is what makes the hover that reorders the list arrive at all, since the HTML5 backend reports hover only for targets that could receive the drag. react-dnd then counts that target as having handled the drop whether or not it carries a `drop` handler, so `monitor.didDrop()` was true for every release on a row, and the drag's `end` reads that as "this chat was filed somewhere else" and discards the arrangement the drag built. The row now answers the release with a result that names it, and `end` treats a handled drop as a filing action only when the result is not that one. The same change stops a row swallowing the drop of an unpinned chat that lands on it: that drop reaches the section underneath, which is what pins the chat. Covered by `dropping-a-chat-on-a-pinned-row-pins-it` and the same-kind control inside `a-pinned-chat-cannot-be-dragged-through-a-pinned-model`, both in `e2e/specs/mock/scenarios/pinned-drag-rules.spec.ts`. * 🧪 test: Drive the Sidebar Row Rules in a Real Browser Twelve scenarios in the mock harness, one per behaviour this branch claims: the radius and fill every row control draws inside its row, the one unpin badge both pinned row kinds carry and when it shows, the controls holding still while the pointer crosses a row, the touch tap that reaches the badge directly, the drop on Chats that unpins, the drop on a pinned row that pins, the kind boundary a drag and Alt+Arrow both refuse, and the stored interleaved order that loads grouped. They seed pinned chats straight into Mongo and set favourites and the pinned order through the routes the star and the drag use, so the server's own cache invalidation runs. Pointer-only scenarios skip on a pointer that cannot hover and the touch scenario skips where it can, which is what lets the same file cover the desktop and mobile projects. `dragRowOnto` steps the pointer rather than jumping: the list reorders on `dragover` once the pointer has crossed the hovered row's midpoint, and a single move never delivers enough of them. * 🧭 fix: Sequence the Two Halves of a Drop on Chats, and Reach the Mobile Drawer Dropping a chat that is both pinned and filed in a project asked for the two writes at once. The pin route answers with the conversation as it stands after its own write, and the sidebar publishes that answer into every list, so a pin that overlapped the project write could put the old `chatProjectId` back over the lists the assignment had just corrected — and a failed assignment still left the chat unpinned, out of the section it had been dragged from and still in the project it was supposed to leave. The unpin now waits for the assignment and runs only if it took, so a failure leaves the chat exactly where it was and the published snapshot is the one taken after both writes. `useAssignDroppedConversation` reports that outcome through a named `AssignDroppedConversation` type, and the project row's drop keeps handing back nothing, since the pinned list reads the drop result to tell a reorder from a filing action. On a narrow viewport the sidebar is a drawer that slides out of view rather than unmounting, so its rows answer every query while nothing on them can be tapped. The scenario helpers open it through the chat header the way a person does, which is what lets the touch and keyboard scenarios run in the mobile project. * 📝 docs: Say What the Kept Overflow Control Actually Costs The comment beside `hasInteracted` claimed virtualization bounded the controls a hovered row leaves mounted. It does for the chats list, which drops a row as it scrolls out, but not for the Pinned section: that one mounts every pinned row at once, so a pointer crossing it leaves one `ConvoOptions` per row it touched, standing until the section unmounts. The bound there is the number of chats the user pinned, which is worth stating rather than implying it is zero. * ⏱️ test: Give the Sidebar Scenario Hooks the Time Their Seeding Takes `test.setTimeout` called inside a test body does not reach the hooks around it, and the slow part of these scenarios is in a hook: seeding a pinned list, then reloading and waiting for the section. On a loaded machine the `beforeEach` hit the default 30s and failed a scenario whose own budget was 60s. The timeout is configured once per file instead, and the per-test calls it duplicated are gone. * ⏳ test: Allow for the Cold Start the First Sidebar Scenario Pays The first test of a run loads the app for the first time against a database that\nmay be a network hop away, and under reviewctl verify that first load pushed the\nseeding hook past 60s. The file budget is 120s and the navigation waits are 30s,\nwhich is the cold start plus room, not a licence for a slow assertion: every\nassertion here still resolves in single-digit seconds once the app is warm. * 🧱 test: Tell an Unlaid Sidebar From a Closed One The helper that brings the sidebar on screen read a missing bounding box as "off\nscreen" and reached for the chat header's opener, which desktop widths do not\nrender: the click waited out the whole test budget while the panel it was waiting\nfor was already open beside it. Placement is now three states — on screen, slid\nout of view, not laid out yet — and only a drawer that is really closed, on a\nviewport that really has an opener, is clicked. * 🧮 fix: Keep Each Pinned Kind in Its Own Slots, and Never Report a Pending Write as Done Two ways the drop and order bookkeeping could still move a row nobody touched. A stored order written before the two kinds were kept apart can interleave them, and `mergeVisibleOrder` substituted visible keys across that interleaving. With part of the pinned list still draining, reordering two visible chats could carry an undelivered chat across the favorite between them, so the row moved within its own group once the rest of the list arrived. The stored order is now grouped the way the section reads it back, and each kind is substituted inside its own run. `useAssignDroppedConversation` treated a pending assignment's destination as an outcome: dropping the same chat on Chats again while its first write was still in flight matched `effectiveProjectId`, reported success, and unpinned the chat — and if that first write then failed, the chat stayed in its project with its pin gone. A write already heading where the drop asks now reports "not filed", so the drop that started it keeps ownership of what follows. Covered by `dnd.spec.ts` (`keeps each kind in its own slots when a legacy order interleaves them`), `unpinDropped.spec.tsx` (`does not unpin again while an assignment to the same place is in flight`) and the `PinnedSection` hidden-key test, which now states the normalized order it expects. The pinned scenarios also wait for the rows to advertise their reorder shortcut before dragging: a move is refused until the saved order has arrived, so a drag issued before that passed or failed on how fast the query answered. * 🔗 fix: Wait for the Write a Drop Asked For, Whoever Started It The pending-assignment guard assumed the write in flight belonged to another\ndrop, which would unpin the chat when it landed. The row menu populates the same\nmap and unfiles a chat without unpinning it, so a drop onto Chats made while that\nmenu action was in flight did nothing at all: an accepted drop left the chat in\nthe Pinned section.\n\nA pending entry now only stops this path from reading \ as\n"already there". The drop issues its own write and waits for that one; the\nmutation queues writes per conversation, so it lands after the one already out\nand repeats what it asked for, which the server takes as the no-op it is. The\nunpin still runs only on a write this drop saw succeed. * 🛰️ fix: Let the Server Say Whether a Dropped Chat Is Out of Its Project Three rounds of review found three ways the same guess went wrong, so the guess is gone. Deciding locally whether the filing half of a Chats drop "already holds" meant reading one of three things that are not the answer: a pending write, which is a request that can still fail; a cached project, which another tab may already have changed; or the drag item's own copy, older still. Each one could let the drop unpin a chat that stayed in its project, and a chat that is pinned nowhere and filed in a project is in neither list the sidebar shows. The drop now always sends the assignment and acts on what comes back. Repeating an assignment the chat already has is the no-op update it looks like, and the mutation queues writes per conversation, so this one lands after anything already out. The success notice reads the two project ids in the response rather than the fact that a request was sent, so a write that moved nothing stays quiet — which is what a pinned chat with no project gets when it is dropped on Chats. `unpinDropped.spec.tsx` covers the three: the unpin waits for the write, a failed write keeps the pin, and a chat already out of every project is still confirmed with the server and unpinned without announcing a move that did not happen. The scenario drag helper also steps its pointer more finely. The list shifts rows as it reorders and reads the pointer offset when the hover fires; a pointer that arrived in one jump got a single reading, sometimes taken mid-render, and the reorder it asked for did not always happen. * 🧷 test: Press Only a Row the List Has Connected for Dragging The pinned list disconnects its drag sources while the saved order reconciles,\nand a press on a disconnected row is not a refused drag — it is a click, so the\nrow opens instead of moving. Against the lab database, where that reconciliation\ntakes long enough to overlap the gesture, the Chats-drop scenario failed with the\ndragged row still pinned and now the active chat.\n\nreact-dnd marks a connected source \, so the helper waits for that\nbefore pressing. --- .../Conversations/Conversations.tsx | 25 +- client/src/components/Conversations/Convo.tsx | 106 +++---- .../ConvoOptions/ConvoOptions.tsx | 4 +- .../Conversations/PinnedSection.tsx | 65 ++-- .../Conversations/ProjectsSection.tsx | 31 +- .../components/Conversations/RenameForm.tsx | 12 +- .../components/Conversations/UnpinButton.tsx | 70 +++++ .../__tests__/Conversations.test.tsx | 1 + .../__tests__/PinnedSection.spec.tsx | 93 ++++-- .../Conversations/__tests__/dnd.spec.ts | 19 ++ .../__tests__/unpinDropped.spec.tsx | 222 ++++++++++++++ client/src/components/Conversations/dnd.ts | 160 +++++++--- .../components/Nav/Favorites/FavoriteItem.tsx | 45 +-- .../mock/scenarios/pinned-drag-rules.spec.ts | 260 ++++++++++++++++ .../scenarios/pinned-row-controls.spec.ts | 256 ++++++++++++++++ e2e/specs/mock/scenarios/pinned.helpers.ts | 289 ++++++++++++++++++ .../sidebar-row-action-fills.spec.ts | 154 ++++++++++ .../client/src/components/Button.spec.tsx | 14 +- packages/client/src/components/Button.tsx | 19 +- 19 files changed, 1633 insertions(+), 212 deletions(-) create mode 100644 client/src/components/Conversations/UnpinButton.tsx create mode 100644 client/src/components/Conversations/__tests__/unpinDropped.spec.tsx create mode 100644 e2e/specs/mock/scenarios/pinned-drag-rules.spec.ts create mode 100644 e2e/specs/mock/scenarios/pinned-row-controls.spec.ts create mode 100644 e2e/specs/mock/scenarios/pinned.helpers.ts create mode 100644 e2e/specs/mock/scenarios/sidebar-row-action-fills.spec.ts diff --git a/client/src/components/Conversations/Conversations.tsx b/client/src/components/Conversations/Conversations.tsx index 0bf3e1a3ac4..82e1254a8b4 100644 --- a/client/src/components/Conversations/Conversations.tsx +++ b/client/src/components/Conversations/Conversations.tsx @@ -22,6 +22,7 @@ import { markExternalHover, useAssignDroppedConversation, useEffectiveProjectId, + useUnpinDroppedConversation, } from './dnd'; import { useLocalize, TranslationKeys, useElementSize } from '~/hooks'; import { groupConversations, cn } from '~/utils'; @@ -195,9 +196,11 @@ const Conversations: FC = ({ const filterTags = useAtomValue(chatFilterTagsAtom); const resetFilters = useSetAtom(resetChatFiltersAtom); const isSmallScreen = useMediaQuery('(max-width: 768px)'); - /* Dropping a project conversation on the Chats section files it back out of - * its project. Root-list chats already live here, so they are rejected. */ + /* Dropping a chat on the Chats section makes it an ordinary chat: out of its + * project, and unpinned. A root-list chat that is not pinned already is one, + * so it is rejected rather than given a drop that would do nothing. */ const assignDropped = useAssignDroppedConversation(); + const unpinDropped = useUnpinDroppedConversation(); const effectiveProjectId = useEffectiveProjectId(); const chatsRegionRef = useRef(null); const [{ isDropOver, canDrop }, dropRef] = useDrop< @@ -206,11 +209,25 @@ const Conversations: FC = ({ { isDropOver: boolean; canDrop: boolean } >({ accept: CONVERSATION_DRAG_TYPE, - canDrop: (item) => effectiveProjectId(item) != null, + canDrop: (item) => effectiveProjectId(item) != null || item.pinned === true, /* Reported even when refused, so a root chat dropped back on Chats does not * save the shift its pointer caused on the way out of the pinned list. */ hover: () => markExternalHover(), - drop: (item) => assignDropped(item, null), + drop: (item) => { + /* Sequenced rather than fired together, for a pinned chat that also sits + * in a project. The pin write answers with the conversation as it stands + * once it has run, so a pin that overlapped the project write would + * publish a row still carrying its old `chatProjectId` into the lists the + * assignment had just corrected. Waiting also gives a failure one shape: + * an assignment that did not take leaves the chat pinned where it was, + * instead of unpinning it out of a project it is still in. Each half is a + * no-op when it already holds. */ + void assignDropped(item, null).then((filed) => { + if (filed) { + unpinDropped(item); + } + }); + }, collect: (monitor) => ({ isDropOver: monitor.isOver(), canDrop: monitor.canDrop() }), }); dropRef(chatsRegionRef); diff --git a/client/src/components/Conversations/Convo.tsx b/client/src/components/Conversations/Convo.tsx index 1dfbb4fc088..40dd7466548 100644 --- a/client/src/components/Conversations/Convo.tsx +++ b/client/src/components/Conversations/Convo.tsx @@ -1,10 +1,10 @@ import React, { memo, useState, useEffect, useRef, useMemo, useCallback } from 'react'; import { useDrag } from 'react-dnd'; +import { Link2 } from 'lucide-react'; import { useRecoilValue } from 'recoil'; -import { Link2, PinOff } from 'lucide-react'; import { useParams } from 'react-router-dom'; import { Constants } from 'librechat-data-provider'; -import { Button, Spinner, TooltipAnchor, useToastContext, useMediaQuery } from '@librechat/client'; +import { Spinner, useToastContext, useMediaQuery } from '@librechat/client'; import type { TConversation } from 'librechat-data-provider'; import type { ConversationDragItem } from './dnd'; import { @@ -20,6 +20,7 @@ import { cn, logger, setDocumentTitle } from '~/utils'; import { NotificationSeverity } from '~/common'; import { CONVERSATION_DRAG_TYPE } from './dnd'; import ConvoActions from './ConvoActions'; +import UnpinButton from './UnpinButton'; import RenameForm from './RenameForm'; import ConvoLink from './ConvoLink'; import store from '~/store'; @@ -166,11 +167,22 @@ function Conversation({ setRenaming(false); }; + /* One-way: a row that has been reached keeps its overflow control mounted for + * as long as the row itself lives. Resetting this on leave unmounted the + * control and remounted it on the next entry, one frame after the row's hover + * had already revealed its slot — so the button arrived a frame late and + * restarted its hover fill from transparent every time the pointer crossed + * the row's edge, which reads as a flicker. + * + * The cost is what the row's own lifetime is: the chats list unmounts a row + * as it scrolls out, while the Pinned section mounts every pinned row at + * once, so there a pointer crossing the list leaves one `ConvoOptions` per + * row it touched, standing until the section unmounts. That is bounded by + * the pin count the user chose, and a control that unmounts instead is what + * this comment's first paragraph describes. */ const handleMouseEnter = useCallback(() => { - if (!hasInteracted) { - setHasInteracted(true); - } - }, [hasInteracted]); + setHasInteracted(true); + }, []); /* Matches the favorites' row-level unpin: one click on the pin badge, no * menu digging. The row unmounts once the pinned refetch lands, so focus is @@ -213,35 +225,8 @@ function Conversation({ ); }, [conversationId, unpinMutation, showToast, localize]); - const handleMouseLeave = useCallback(() => { - if (!isPopoverActive) { - setHasInteracted(false); - } - }, [isPopoverActive]); - - const handleBlur = useCallback( - (e: React.FocusEvent) => { - // Don't reset if focus is moving to a child element within this container - if (e.currentTarget.contains(e.relatedTarget as Node)) { - return; - } - if (!isPopoverActive) { - setHasInteracted(false); - } - }, - [isPopoverActive], - ); - const handlePopoverOpenChange = useCallback((open: boolean) => { setIsPopoverActive(open); - if (!open) { - requestAnimationFrame(() => { - const container = containerRef.current; - if (container && !container.contains(document.activeElement)) { - setHasInteracted(false); - } - }); - } }, []); const handleNavigation = (ctrlOrMetaKey: boolean) => { @@ -291,20 +276,29 @@ function Conversation({ ); + /* The slot takes its width from the row's hover, not from its content. The + * overflow menu mounts a tick after the pointer arrives (see `ConvoActions`), + * and a content-sized slot grew at that moment, pulling the unpin badge a + * button's width leftwards out from under the pointer: the badge's fill, + * already fading in, handed off to whichever control had slid into its place. + * Reserving the width up front leaves every control where it was drawn. */ let actionVisibilityClassName = - 'pointer-events-none max-w-0 scale-x-0 opacity-0 group-focus-within:pointer-events-auto group-focus-within:max-w-[60px] group-focus-within:scale-x-100 group-focus-within:opacity-100 group-hover:pointer-events-auto group-hover:max-w-[60px] group-hover:scale-x-100 group-hover:opacity-100'; + 'pointer-events-none w-0 scale-x-0 opacity-0 group-focus-within:pointer-events-auto group-focus-within:scale-x-100 group-focus-within:opacity-100 group-hover:pointer-events-auto group-hover:scale-x-100 group-hover:opacity-100'; + let actionWidthClassName = isSmallScreen + ? 'group-focus-within:w-9 group-hover:w-9' + : 'group-focus-within:w-7 group-hover:w-7'; if (isGenerating) { actionVisibilityClassName = 'pointer-events-none w-5 scale-x-100 opacity-100'; + actionWidthClassName = ''; } else if (isPopoverActive || isActiveConvo || isSmallScreen) { /** Touch has no hover, so a reveal-on-hover menu is unreachable there. */ actionVisibilityClassName = 'pointer-events-auto scale-x-100 opacity-100'; - } - - let actionWidthClassName = ''; - if (!isGenerating && !isPopoverActive && isActiveConvo && isShiftHeld) { - actionWidthClassName = 'max-w-[60px]'; - } else if (!isGenerating) { - actionWidthClassName = isSmallScreen ? 'max-w-[36px]' : 'max-w-[28px]'; + /** Shift over the active row swaps the menu for archive and delete. */ + if (!isPopoverActive && isActiveConvo && isShiftHeld) { + actionWidthClassName = 'w-[60px]'; + } else { + actionWidthClassName = isSmallScreen ? 'w-9' : 'w-7'; + } } let actionContent: React.ReactNode = null; @@ -331,9 +325,7 @@ function Conversation({ onPointerLeave={() => setIsHovered(false)} onPointerCancel={() => setIsHovered(false)} onMouseEnter={handleMouseEnter} - onMouseLeave={handleMouseLeave} onFocus={handleMouseEnter} - onBlur={handleBlur} onClick={(e) => { if (renaming) { return; @@ -372,29 +364,19 @@ function Conversation({