📌 feat: Add Pin Support for Model Specs#11219
Conversation
berry-13
force-pushed
the
feat/pin-model-specs
branch
from
075546b to
9ea62ad
Compare
berry-13
force-pushed
the
feat/pin-model-specs
branch
from
9ea62ad to
bfeefb9
Compare
There was a problem hiding this comment.
Pull request overview
Adds “pin/favorite” support for model specs end-to-end (schema + API validation + client state/UI), and tightens up submenu padding for a more compact nested menu layout.
Changes:
- Extend favorites data model to support
{ spec: string }entries and validate them server-side. - Update client favorites hook + sidebar favorites rendering to include pinned model specs.
- Add a reusable
useIsActiveItemhook to improve keyboard/focus behavior for pin buttons inside Ariakit composite menus; adjust submenu padding styles.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/data-schemas/src/types/user.ts | Align IUser.favorites typing with shared TUserFavorite. |
| packages/data-schemas/src/schema/user.ts | Add spec to favorites schema and apply maxlength constraints. |
| packages/data-provider/src/types/queries.ts | Introduce exported TUserFavorite type (agent/model/spec variants). |
| packages/data-provider/src/data-service.ts | Switch favorites API typings to use TUserFavorite. |
| client/src/store/favorites.ts | Reuse TUserFavorite for client favorite state type. |
| client/src/locales/en/translation.json | Add com_ui_model_spec label for spec favorites. |
| client/src/hooks/useIsActiveItem.ts | New hook to mirror Ariakit data-active-item into React state. |
| client/src/hooks/index.ts | Export useIsActiveItem. |
| client/src/hooks/tests/useIsActiveItem.spec.tsx | Add unit tests for useIsActiveItem. |
| client/src/hooks/useFavorites.ts | Add spec favorite helpers + clean/normalize favorites to canonical shapes. |
| client/src/hooks/tests/useFavorites.spec.tsx | Add tests for spec favorite methods and cleaning behavior. |
| client/src/components/Nav/Favorites/FavoritesList.tsx | Render spec favorites by resolving pinned spec names from startupConfig. |
| client/src/components/Nav/Favorites/FavoriteItem.tsx | Support rendering/selecting/removing spec favorites in sidebar items. |
| client/src/components/Nav/Favorites/tests/FavoritesList.spec.tsx | Add tests for spec rendering in the favorites list. |
| client/src/components/Nav/Favorites/tests/FavoriteItem.spec.tsx | Add tests for spec-type favorite item behavior and a11y label. |
| client/src/components/Chat/Menus/Endpoints/CustomMenu.tsx | Reduce padding for nested (submenu) dropdowns. |
| client/src/components/Chat/Menus/Endpoints/components/ModelSpecItem.tsx | Add hover/active pin button for model spec items. |
| client/src/components/Chat/Menus/Endpoints/components/EndpointModelItem.tsx | Replace inline MutationObserver with useIsActiveItem and improve pin button focus styles. |
| client/src/components/Chat/Menus/Endpoints/components/tests/ModelSpecItem.test.tsx | Add tests for the model spec pin button and interactions. |
| client/src/components/Chat/Menus/Endpoints/components/tests/EndpointModelItem.test.tsx | Update test mocks to include useIsActiveItem. |
| api/server/controllers/FavoritesController.js | Accept/validate spec favorites and enforce (partial) exclusivity rules. |
| api/server/controllers/FavoritesController.spec.js | Add backend controller test coverage for spec + exclusivity validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -43,16 +44,27 @@ const updateFavoritesController = async (req, res) => { | |||
| .status(400) | |||
| .json({ message: `endpoint exceeds maximum length of ${MAX_STRING_LENGTH}` }); | |||
| } | |||
| if (fav.spec !== undefined && fav.spec !== null) { | |||
| if (typeof fav.spec !== 'string' || fav.spec.length === 0) { | |||
| return res.status(400).json({ message: 'spec must be a non-empty string' }); | |||
| } | |||
| if (fav.spec.length > MAX_STRING_LENGTH) { | |||
| return res | |||
| .status(400) | |||
| .json({ message: `spec exceeds maximum length of ${MAX_STRING_LENGTH}` }); | |||
| } | |||
| } | |||
|
|
|||
| if (!hasAgent && !hasModel) { | |||
| const typeCount = [hasAgent, hasModel, hasSpec].filter(Boolean).length; | |||
| if (typeCount === 0) { | |||
| return res.status(400).json({ | |||
| message: 'Each favorite must have either agentId or model+endpoint', | |||
| message: 'Each favorite must have either agentId, model+endpoint, or spec', | |||
| }); | |||
| } | |||
|
|
|||
| if (hasAgent && hasModel) { | |||
| if (typeCount > 1) { | |||
| return res.status(400).json({ | |||
| message: 'Favorite cannot have both agentId and model/endpoint', | |||
| message: 'Favorite cannot have multiple types (agentId, model/endpoint, or spec)', | |||
| }); | |||
| } | |||
There was a problem hiding this comment.
The mutual-exclusivity logic only counts the model variant when both model and endpoint are set. This allows mixed/invalid entries like { agentId, model } or { spec, endpoint } to pass validation and be persisted with multiple type fields present. Consider rejecting when any non-null field from another variant is present, and/or explicitly requiring model and endpoint together when either is provided (and rejecting partials).
| } else if (fav.spec) { | ||
| const spec = specsMap[fav.spec]; | ||
| if (!spec) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
When a spec favorite is stale (missing from startupConfig.modelSpecs.list), this returns null but the entry remains in safeFavorites. That leaves index “holes” in the rendered list (and in react-dnd indices), which can break drag/drop ordering and makes the stale favorite impossible to remove via the UI. Consider filtering/cleaning spec favorites that aren’t present in specsMap (similar to the stale agent cleanup) before rendering and before DnD indexing/persistence.
| <button | ||
| tabIndex={isActive ? 0 : -1} | ||
| onClick={handleFavoriteClick} | ||
| aria-label={isFavorite ? localize('com_ui_unpin') : localize('com_ui_pin')} |
There was a problem hiding this comment.
This pin/unpin control is a plain <button> without an explicit type. If this menu is ever rendered inside a <form>, the button will default to type="submit" and can cause unintended submissions. Set type="button" here (and in the similar pin button in EndpointModelItem).
| <button | ||
| tabIndex={isActive ? 0 : -1} | ||
| onClick={handleFavoriteClick} | ||
| aria-label={isFavorite ? localize('com_ui_unpin') : localize('com_ui_pin')} | ||
| className={cn( |
There was a problem hiding this comment.
This pin/unpin control is a plain <button> without an explicit type. If this menu is ever rendered inside a <form>, the button will default to type="submit" and can cause unintended submissions. Set type="button" here (and in ModelSpecItem).
| it('initializes isActive=true when data-active-item is already set pre-mount', () => { | ||
| // Render wrapper that sets the attribute BEFORE Probe mounts. | ||
| function Wrapper() { | ||
| const [mounted, setMounted] = React.useState(false); | ||
| return ( | ||
| <div> | ||
| {mounted && <Probe />} | ||
| <button | ||
| data-testid="mount-trigger" | ||
| onClick={() => { | ||
| setMounted(true); | ||
| }} | ||
| /> | ||
| </div> | ||
| ); | ||
| } | ||
| // Simplest: assert default is false (initial mount has no attribute); the pre-mount | ||
| // hydration case is effectively the same code path covered by the update test below. | ||
| const { container } = render(<Wrapper />); | ||
| // Not strictly asserting here — initial-attribute path is exercised below via mutation. | ||
| expect(container).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
This test case is named as if it verifies the pre-mount data-active-item initialization behavior, but it doesn’t assert anything about isActive (it only checks container truthiness). Either implement a real assertion for the pre-mount attribute scenario or remove/rename this test to avoid giving a false sense of coverage.
| /** | ||
| * User Favorites — pinned agents, models, and model specs. | ||
| * Exactly one variant should be set per entry; exclusivity is enforced | ||
| * server-side in FavoritesController. Shape is loose for state-update ergonomics. | ||
| */ | ||
| export type TUserFavorite = { | ||
| agentId?: string; | ||
| model?: string; | ||
| endpoint?: string; | ||
| spec?: string; | ||
| }; |
There was a problem hiding this comment.
PR description mentions bumping the data-provider version to 0.8.211, but this change set only adds TUserFavorite and doesn’t include a version bump (and package.json currently shows a different version). Either update the PR description or include the intended version bump so downstream consumers can rely on the new exported type.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfeefb9878
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!spec) { | ||
| return null; |
There was a problem hiding this comment.
When a persisted favorite points to a spec name that is no longer present in startupConfig, this branch returns null and leaves the stale entry in state/storage. That makes the item impossible to unpin from the UI, still counts toward the 50-favorite limit, and can leave an empty favorites section mounted because favorites.length stays nonzero. This should be cleaned up the same way stale agent favorites are cleaned up (via reorderFavorites(..., true)) instead of only hiding it at render time.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 543a2f66a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!staleSpecNamesKey || specCleanupAttemptedRef.current === staleSpecNamesKey) { | ||
| return; | ||
| } | ||
| const staleSet = new Set(staleSpecNamesKey.split(',')); |
There was a problem hiding this comment.
Using staleSpecNamesKey.split(',') assumes spec names never contain commas, but model spec names are free-form strings, so valid names like "foo,bar" get corrupted during cleanup. In that case the stale spec is not removed (because "foo,bar" is split into "foo"/"bar"), and unrelated favorites with those fragment names can be removed instead, causing incorrect persistence when reorderFavorites(cleaned, true) runs.
Useful? React with 👍 / 👎.
danny-avila
changed the title
- Add canonical TUserFavorite type in data-provider, replace three duplicate definitions (data-service, data-schemas, store/favorites) - Consolidate FavoritesController spec validation into single block, add return on 500 paths, add maxlength to mongoose sub-schema - Fix import order in FavoritesList.tsx, merge namespace type imports in FavoriteItem.tsx and FavoritesList.tsx - Add focus-visible:ring-inset on pin buttons to prevent ring clipping - Add explicit return type and JSDoc on useIsActiveItem hook - Use props.type for narrowing consistency in FavoriteItem getTypeLabel - Add 22 backend tests for FavoritesController (spec validation, typeCount exclusivity, persistence, GET path) - Add 40 frontend tests: useFavorites spec methods, useIsActiveItem observer lifecycle, ModelSpecItem pin button, FavoriteItem all three type branches, FavoritesList spec rendering
- Harden backend validation to reject partial cross-type fields (e.g. spec+endpoint, agentId+model without endpoint) - Add stale-spec auto-cleanup in FavoritesList mirroring agent cleanup - Add type="button" to pin buttons in ModelSpecItem/EndpointModelItem - Fix import order violations in EndpointModelItem and ModelSpecItem - Remove hollow test, dead key prop, inline trivial helpers - Fix misleading test description, add onSelectSpec to test mock - Add return to controller success responses for consistency - Add 6 backend tests for partial cross-type field validation
Prevents race condition where spec favorites are incorrectly deleted on cold start before startupConfig has loaded. Mirrors the existing agentsMap === undefined guard pattern used for stale agent cleanup. Also adds tests for stale-spec cleanup persistence and fixes namespace import pattern in FavoritesList.spec.tsx.
Resolves ESLint no-nested-ternary warnings for name and typeLabel derivations.
danny-avila
force-pushed
the
feat/pin-model-specs
branch
from
b824314 to
ee9380a
Compare
* feat: Enhance favorites functionality to support model specs * refactor(FavoritesList): reorder imports based on lenght * feat: improve favorite modelSpec controller; refactor: useIsActiveItem hook * refactor: consolidate Favorite type, harden controller, add tests - Add canonical TUserFavorite type in data-provider, replace three duplicate definitions (data-service, data-schemas, store/favorites) - Consolidate FavoritesController spec validation into single block, add return on 500 paths, add maxlength to mongoose sub-schema - Fix import order in FavoritesList.tsx, merge namespace type imports in FavoriteItem.tsx and FavoritesList.tsx - Add focus-visible:ring-inset on pin buttons to prevent ring clipping - Add explicit return type and JSDoc on useIsActiveItem hook - Use props.type for narrowing consistency in FavoriteItem getTypeLabel - Add 22 backend tests for FavoritesController (spec validation, typeCount exclusivity, persistence, GET path) - Add 40 frontend tests: useFavorites spec methods, useIsActiveItem observer lifecycle, ModelSpecItem pin button, FavoriteItem all three type branches, FavoritesList spec rendering * fix: address PR review findings for pin model specs - Harden backend validation to reject partial cross-type fields (e.g. spec+endpoint, agentId+model without endpoint) - Add stale-spec auto-cleanup in FavoritesList mirroring agent cleanup - Add type="button" to pin buttons in ModelSpecItem/EndpointModelItem - Fix import order violations in EndpointModelItem and ModelSpecItem - Remove hollow test, dead key prop, inline trivial helpers - Fix misleading test description, add onSelectSpec to test mock - Add return to controller success responses for consistency - Add 6 backend tests for partial cross-type field validation * fix: guard stale-spec cleanup against unloaded startupConfig Prevents race condition where spec favorites are incorrectly deleted on cold start before startupConfig has loaded. Mirrors the existing agentsMap === undefined guard pattern used for stale agent cleanup. Also adds tests for stale-spec cleanup persistence and fixes namespace import pattern in FavoritesList.spec.tsx. * fix: replace nested ternaries with if/else in FavoriteItem Resolves ESLint no-nested-ternary warnings for name and typeLabel derivations. --------- Co-authored-by: Danny Avila <danny@librechat.ai>
Summary
Changes
Frontend:
specfield toFavoritetypeaddFavoriteSpec,removeFavoriteSpec,isFavoriteSpec,toggleFavoriteSpecmethods touseFavoriteshookModelSpecItemcomponent (shows on hover)FavoriteItemandFavoritesListto render pinned specspx-3 py-2topx-0.5 py-0.5Backend:
FavoritesControllervalidation to acceptspecas a valid favorite typeagentId,model+endpoint, andspecData Provider:
specfield toFavoriteItemtypeChange Type
Testing
librechat.yamlChecklist