Repository navigation
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
👋 Thanks for the contribution! LibreChat merges all changes into Nothing is needed from you; your commits, reviews and discussion are unchanged. If the diff now shows files you did not touch, rebase onto git remote add upstream https://github.com/LibreChat-AI/LibreChat.git
git fetch upstream dev
git rebase upstream/dev
git push --force-with-leaseMaintainers: apply the |
AtefBellaaj
left a comment
There was a problem hiding this comment.
Thank you so much for working on this. can you please add some tests for this?
|
Done, added regression tests in packages/data-schemas/src/methods/prompt.spec.ts covering both list endpoints (getPromptGroups and getListPromptGroupsByAccess). Each creates a group with a command value and asserts the returned projection carries it, so either projection dropping command fails the test. |
|
@aniketkrs much appreciated 🙏 can you please check the failing tests and static checks? I would appreciated it also if you rebase with the dev branch. Thank you 😊 |
|
Thanks for the review. I looked at the three failing checks and both problems trace back to the test file I added. The TypeScript type check failed because the new test read The static check failure was formatting in the same block. I cleaned that up. The data schemas test failure came from the same TypeScript error breaking compilation of the spec. I also rebased the branch onto the latest dev as requested. The branch now sits at upstream dev CI is rerunning on the new head. Let me know if anything else needs attention. |
|
I see that you closed the PR. Was this a mistake? |
|
Yes, that close was a mistake on my side. While rebasing onto dev I deleted and recreated the branch, which auto closed this PR, and GitHub does not allow reopening a PR whose branch was recreated. I have opened #17046 with the same fix on the rebased head. It addresses your review: regression tests for both list projections are included, the TS2339 union error is fixed by narrowing on promptGroups, and the spec file passes the repo Prettier config. Continuing the review there; this one can stay closed. |
Impact
A prompt saved with a slash
command(e.g.weather) never reaches the browser through the two prompt-group list endpoints, so:/popover lists the prompt by name only, without the/weather -prefix,/weatherdoes not match the prompt that owns that command,The prompt's own page shows the command correctly because
GET /api/prompts/groups/:groupIdreturns it. Only the list responses lose it.Cause
getListPromptGroupsByAccess(servingGET /api/prompts/all) andgetPromptGroups(servingGET /api/prompts/groups) project list fields with Mongoose.select()but omitcommand:The sibling
getAllPromptGroupsselectscommand, so the field exists on the model. The client expects it (client/src/Providers/PromptGroupsContext.tsxbuilds each popover option asvalue: group.command ?? group.namewith a/${group.command} -label prefix), sogroup.commandis alwaysundefinedin the popover today.Fix
Add
commandto the.select()projection in both list queries (packages/data-schemas/src/methods/prompt.ts). One focused change, no schema or type changes needed (commandis already projected elsewhere and the list responses areRecord<string, unknown>[]).Fixes #16874.