major changes and feature additions #1

Open
myk wants to merge 11 commits from claude_code_work into main
Collaborator

This PR represents a day's work with Claude Code. See todo.md for a list of features added. Additionally some paradigm shifts were integrated, and the filesystem was reorganized. This is too big to properly review but I figure it'd be helpful to treat it as a PR rather than just pushing to master.

This PR represents a day's work with Claude Code. See todo.md for a list of features added. Additionally some paradigm shifts were integrated, and the filesystem was reorganized. This is too big to properly review but I figure it'd be helpful to treat it as a PR rather than just pushing to master.
myk added 11 commits 2025-06-10 12:14:13 -05:00
lentil requested changes 2025-06-11 13:00:29 -05:00
lentil left a comment
Owner

Nicely done! I find it very rewarding that my code was written and structured well enough to enable these extensions. I commented on a few minor points.

Nicely done! I find it very rewarding that my code was written and structured well enough to enable these extensions. I commented on a few minor points.
@ -0,0 +1,247 @@
import * as _RhizomeImports from "../src";
Owner

This line appears to do nothing

This line appears to do nothing
@ -0,0 +58,4 @@
const result = resolver.resolve();
expect(result).toBeDefined();
// Should resolve deterministically (likely based on delta processing order)
Owner

Comment is outdated, this seems to use a more robust tie breaking algorithm, not relying on processing order

Comment is outdated, this seems to use a more robust tie breaking algorithm, not relying on processing order
@ -0,0 +1,492 @@
import * as _RhizomeImports from "../src";
Owner

This line appears to do nothing

This line appears to do nothing
@ -0,0 +443,4 @@
expect(stats.negationDeltas).toBe(0); // No negations for this entity
});
it('should handle multiple negations and un-negations', () => {
Owner

and un-negations

I'm not seeing anything implementing or testing the negation of a negation

> and un-negations I'm not seeing anything implementing or testing the negation of a negation
@ -0,0 +251,4 @@
while (currentView.nestedObjects.next && currentView.nestedObjects.next.length > 0) {
currentView = currentView.nestedObjects.next[0];
depth++;
if (depth >= 5) break; // Prevent infinite loop
Owner

This condition means the subsequent test of depth will always pass, regardless of actual depth

This condition means the subsequent test of depth will always pass, regardless of actual depth
@ -4,0 +337,4 @@
'invalid-operator': [{ 'var': 'age' }, 25]
});
// Should not crash, may return empty results or skip problematic entities
Owner

Seems like we should probably return an error to the caller in this case?

Seems like we should probably return an error to the caller in this case?
@ -0,0 +1,501 @@
import * as _RhizomeImports from "../src";
Owner

This line appears to do nothing

This line appears to do nothing
@ -0,0 +1 @@
MANIFEST-000030
Owner

I think data/ should probably be excluded from the repo by .gitignore

I think `data/` should probably be excluded from the repo by `.gitignore`
@ -0,0 +124,4 @@
const entity = this.lossless.domainEntities.get(entityId);
if (!entity) continue;
// Check if entity has deltas for any required property
Owner

Maybe this should only include items with all required properties?

Maybe this should only include items with _all_ required properties?
@ -0,0 +249,4 @@
}
/**
* Extract reference value (target ID) from a delta for a given property
Owner

for a given property

It seems like a bug that propertyId is not referenced in this function

> for a given property It seems like a bug that `propertyId` is not referenced in this function
@ -0,0 +199,4 @@
}
// Common schema patterns
export const CommonSchemas = {
Owner

It looks like CommonSchemas is only used in __tests__ and should probably be moved there

It looks like `CommonSchemas` is only used in `__tests__` and should probably be moved there
@ -0,0 +270,4 @@
}
resolve(state: { min?: number }): PropertyTypes {
return state.min || 0;
Owner

I wonder if this is safe -- maybe if no values are found the result should be omitted

I wonder if this is safe -- maybe if no values are found the result should be omitted
@ -0,0 +106,4 @@
return res;
};
// Override resolve to build accumulator on-demand if needed
Owner

Maybe this belongs in the Lossy class itself?

Maybe this belongs in the `Lossy` class itself?
This pull request can be merged automatically.
You are not authorized to merge this pull request.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin claude_code_work:claude_code_work
git checkout claude_code_work
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lentil/rhizome-node#1
No description provided.