Conversation
… instead of ternary for readability
angelplusultra
left a comment
There was a problem hiding this comment.
Mostly nits and refactor suggestions
|
Docs PR: Mintplex-Labs/anythingllm-docs#235 |
angelplusultra
left a comment
There was a problem hiding this comment.
Some refactor suggestions and nits
angelplusultra
left a comment
There was a problem hiding this comment.
Note about agent chat path missing memory reranking
angelplusultra
left a comment
There was a problem hiding this comment.
LGTM. Just delete that stale comment.
regen migration so it is latest file
There was a problem hiding this comment.
Reminder!
This needs a documentation PR to inform users how this works, what it does, how to manage memories - basically a short tutorial on our features so that they can be informed about this new feature to take advantage of!
Notes:
- When clicking edit on a memory the cursor goes to the start of the input and either should go to the end
- If i have 5/5 Global memories and I have the global tab focused in sidebar and I click to add another the modal closes like it was successful but nothing happens.
- the system settings check value for memories needs to be moved from
"on"|"off"to"true"|"false"and reading the value should work like other fields. I know that value is a string column so it is storing that as a string but just make a helper in SystemSettings for this like the other boolean/string fields to compensate for it like - https://github.com/Mintplex-Labs/anything-llm/blob/feat/memory/server/models/systemSettings.js#L624-L632 so everything is normalized across all system settings. - See line level comments for more
Look above for QA misses in the UI i noticed
Things I did:
- Refactored how Chat menu was broken up
- Refactored the sidebar to prevent prop drilling and cleanup DOM and nested component rendering waterfalls
| }); | ||
| }, | ||
|
|
||
| demoteToWorkspace: async function (memoryId, workspaceId) { |
There was a problem hiding this comment.
JSDoc all these so we have type hints in UI
| }) | ||
| .then((res) => res.json()) | ||
| .catch((e) => { | ||
| console.error(e); |
There was a problem hiding this comment.
We dont need these error logs since we return error anyway
| "@/pages/WorkspaceSettings" | ||
| ); | ||
| return { element: <ManagerRoute Component={WorkspaceSettings} /> }; | ||
| return { element: <PrivateRoute Component={WorkspaceSettings} /> }; |
There was a problem hiding this comment.
Why did we change scope of this route?
| async function memoryEnabled(_req, response, next) { | ||
| const enabled = | ||
| (await SystemSettings.getValueOrFallback( | ||
| { label: "memory_enabled" }, | ||
| "off" | ||
| )) === "on"; | ||
| if (!enabled) | ||
| return response.status(403).json({ error: "Personalization is disabled." }); | ||
| next(); | ||
| } |
There was a problem hiding this comment.
Rename this memoryFeatureEnabled and once you migrate the check for this to a method in SystemSettings like I mentioned in parent review comment you can then just call await SystemSettings.memoriesEnabled() and throw if not.
| function ownerMatch(memory, user) { | ||
| const memUserId = memory.userId ?? null; | ||
| const reqUserId = user?.id ?? null; | ||
| return memUserId === reqUserId; | ||
| } |
There was a problem hiding this comment.
This should also just be a middleware - it should just be skipped if the system is not in MuM.
Easy to check since the first middleware is validatedRequest which assigns
response.locals.multiUserMode = multiUserMode;
| CREATE TABLE "new_workspace_chats" ( | ||
| "id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, | ||
| "workspaceId" INTEGER NOT NULL, | ||
| "prompt" TEXT NOT NULL, | ||
| "response" TEXT NOT NULL, | ||
| "include" BOOLEAN NOT NULL DEFAULT true, | ||
| "user_id" INTEGER, | ||
| "thread_id" INTEGER, | ||
| "api_session_id" TEXT, | ||
| "createdAt" DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "lastUpdatedAt" DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "feedbackScore" BOOLEAN, | ||
| "memory_processed" BOOLEAN NOT NULL DEFAULT false, | ||
| CONSTRAINT "workspace_chats_user_id_fkey" FOREIGN KEY ("user_id") REFERENCES "users" ("id") ON DELETE CASCADE ON UPDATE CASCADE | ||
| ); | ||
| INSERT INTO "new_workspace_chats" ("api_session_id", "createdAt", "feedbackScore", "id", "include", "lastUpdatedAt", "prompt", "response", "thread_id", "user_id", "workspaceId") SELECT "api_session_id", "createdAt", "feedbackScore", "id", "include", "lastUpdatedAt", "prompt", "response", "thread_id", "user_id", "workspaceId" FROM "workspace_chats"; | ||
| DROP TABLE "workspace_chats"; | ||
| ALTER TABLE "new_workspace_chats" RENAME TO "workspace_chats"; |
There was a problem hiding this comment.
I know this works but i REALLY do not ever want to trust SQLite to do a full drop-and-replace of the most important table in the entire DB - you can avoid this but not adding a default field to memory_processed
| workspaceId Int? @map("workspace_id") | ||
| scope String @default("workspace") | ||
| content String | ||
| sourceThreadId Int? @map("source_thread_id") |
There was a problem hiding this comment.
I looked over the code and done see really any place this field even is used in a way that matters? Where is this coming from and/or how is it used?
| const basePrompt = await Provider.systemPrompt({ | ||
| provider, | ||
| workspace, | ||
| user, | ||
| }); |
There was a problem hiding this comment.
Can we not just augment the prompt directly with memories in this method since we already have the user and workspace potentially in there as well?
| // Compress & Assemble message to ensure prompt passes token limit with room for response | ||
| // and build system messages based on inputs and history. | ||
| const systemPrompt = await promptWithMemories({ | ||
| systemPrompt: await chatPrompt(workspace, user), |
There was a problem hiding this comment.
Same here - can we augment the chat in this already async method?
| const { Memory } = require("../../models/memory"); | ||
| const { SystemSettings } = require("../../models/systemSettings"); | ||
|
|
||
| const INJECTED_WORKSPACE_LIMIT = 5; |
There was a problem hiding this comment.
This needs a description - I did not realize what it did for a while


Pull Request Type
Relevant Issues
resolves #5276
Translations PR: #5277
Description
Visuals (if applicable)
Additional Information
Developer Validations
yarn lintfrom the root of the repo & committed changes