-
Notifications
You must be signed in to change notification settings - Fork 579
Implemented long term memory #1828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@Kitenite Are you asking for this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 3ea51d5 in 2 minutes and 12 seconds. Click for details.
- Reviewed
255
lines of code in4
files - Skipped
1
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/memory/jest.config.js:8
- Draft comment:
Add a newline at the end of the file for POSIX compliance. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While having a newline at the end of files is a POSIX standard, this is a relatively minor issue. Most modern editors and build tools handle this automatically. It's not causing any functional issues, and it's more of a style/convention point than a critical problem. This kind of feedback could be better handled by an automated linter. The POSIX standard is important for file handling in some systems, and missing newlines can cause issues with certain tools. Maybe this should be kept for consistency? While POSIX compliance is good practice, this is too minor for a PR comment. It's the kind of thing that should be enforced by EditorConfig or similar tooling, not manual review. Delete this comment. It's too minor and would be better handled by automated tooling.
2. packages/memory/src/index.ts:19
- Draft comment:
Calling async initialize() in the constructor without awaiting it may lead to race conditions. - Reason this comment was not posted:
Marked as duplicate.
3. packages/memory/src/index.test.ts:11
- Draft comment:
Test setup might face a race condition due to async init in the constructor; consider awaiting initialization for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% 1. The constructor takes a directory path but doesn't show signs of being async. 2. All subsequent async operations in the tests are properly awaited. 3. The first test is synchronous and passes, suggesting no async init is needed. 4. Without seeing the actual constructor implementation, we can't be certain there's an async init issue. 5. The tests are passing and handling async operations correctly where needed. I don't have access to the constructor implementation, so I can't be 100% certain there isn't an async initialization happening internally. However, the tests are working correctly and show no signs of race conditions, and the synchronous test passes, strongly suggesting there is no async initialization needed. Delete this comment as there's no strong evidence of a race condition, and the tests are handling async operations appropriately where needed.
4. packages/memory/jest.config.js:8
- Draft comment:
There is no newline at the end of the file. This is a trivial formatting issue and can be fixed by adding a newline at the end of the file. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Missing newlines at end of files are typically caught by linters and formatters. This is a very minor formatting issue that would be automatically fixed by tools. The comment doesn't point out a logical issue or code quality problem that requires human attention. The missing newline could cause git diff display issues and is considered a best practice in many style guides. While true, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual code review comments. Delete this comment as it points out a trivial formatting issue that should be handled by automated tools rather than manual review.
Workflow ID: wflow_yvDnPRVUr0vipYYf
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to d2640e2 in 2 minutes and 4 seconds. Click for details.
- Reviewed
260
lines of code in4
files - Skipped
1
files when reviewing. - Skipped posting
13
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/memory/jest.config.js:1
- Draft comment:
Jest config looks standard and correct. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. packages/memory/package.json:1
- Draft comment:
Package.json is well-defined with proper deps and scripts. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. packages/memory/src/index.test.ts:1
- Draft comment:
Test suite comprehensively covers rule CRUD operations. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. packages/memory/src/index.ts:3
- Draft comment:
Crypto is imported correctly; using crypto.randomUUID() is appropriate. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. packages/memory/src/index.ts:22
- Draft comment:
Async initialization via init() correctly defers initialization. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. packages/memory/src/index.ts:26
- Draft comment:
Consider logging more context in loadRules for individual file errors. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. packages/memory/jest.config.js:1
- Draft comment:
Jest config is set up correctly for ts-jest and a Node environment. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. packages/memory/package.json:1
- Draft comment:
Package configuration looks good with proper scripts, dependencies, and devDependencies. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
9. packages/memory/src/index.test.ts:9
- Draft comment:
Test suite is comprehensive and correctly awaits async initialization. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
10. packages/memory/src/index.ts:3
- Draft comment:
Correctly imported the 'crypto' module and uses crypto.randomUUID() for unique IDs. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it only states that thecrypto
module is correctly imported and used. It does not provide any actionable feedback or suggestions for improvement.
11. packages/memory/src/index.ts:22
- Draft comment:
Async initialization via init() and initialize() correctly resolves previous async issues. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the async initialization issue is resolved, which is not allowed according to the rules.
12. packages/memory/src/index.ts:37
- Draft comment:
Consider using concurrent file reads (e.g., Promise.all) in loadRules for improved performance with many files. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
13. packages/memory/jest.config.js:8
- Draft comment:
Minor: It looks like there's no newline at the end of the file. Adding a newline at the end can help maintain consistency with Unix-style text files. This is a trivial, cosmetic issue. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and does not provide a specific code suggestion or highlight a potential issue. It simply suggests adding a newline for consistency, which is not critical to the functionality or correctness of the code.
Workflow ID: wflow_7anroNDXos6FaiaH
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
for (const file of files) { | ||
if (file.endsWith('.json')) { | ||
const rulePath = path.join(this.rulesDir, file); | ||
const ruleData = await fs.readJson(rulePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider validating the structure of ruleData
read from JSON to ensure required fields exist.
}; | ||
|
||
const rulePath = path.join(this.rulesDir, `${id}.json`); | ||
await fs.writeJson(rulePath, updatedRule); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap fs.writeJson
in updateRule
with try/catch for robust error handling on filesystem write failures.
Description
Added script inside packages/memory directory for long term rules
Related Issues
related #1816
Type of Change
Testing
Tested using jest
Screenshots (if applicable)
Important
Introduces
LongTermMemory
class for rule management with Jest tests and package configuration.LongTermMemory
class inindex.ts
for managing rules with methods to add, update, delete, and retrieve rules.index.test.ts
with Jest tests forLongTermMemory
methods: initialization, add, update, delete, and get rules by tag.jest.config.js
for TypeScript testing.package.json
for the@onlook/memory
package with build and test scripts, dependencies, and devDependencies.This description was created by
for d2640e2. You can customize this summary. It will automatically update as commits are pushed.