-
Notifications
You must be signed in to change notification settings - Fork 61
Project Mode #2048
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: dev
Are you sure you want to change the base?
Project Mode #2048
Conversation
… probably move model/update architecture to hazelcore; root of file system path now displays project name
…based architecture for chats
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #2048 +/- ##
==========================================
- Coverage 51.43% 50.17% -1.26%
==========================================
Files 205 212 +7
Lines 21754 22832 +1078
==========================================
+ Hits 11189 11457 +268
- Misses 10565 11375 +810
🚀 New features to boost your workflow:
|
…errors info messages--they are now buttons
…uite in haz3ltest.
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.
Here's a basic review of the integration layer. I think we should make some changes but probably nothing too substantial i dont think. Short notice for this afternoon's meeting; if you have a chance to glance over we can discuss, otherwise can discuss later in the week or next if you are available. some comments are just questions i'm not totally sure about it, mostly about functionality that has moved.
A few things to break out:
- I think the internal printer changes you made are probably both unnecessary and may have broken the probe tests; or at least something did. lmk if you don't know what my in-file comments about this mean.
- I didn't look at the agent-specific stuff in depth but Agent.re is 1723 lines, effectively a god module containing Model + Update + Store for the agent, chat system, and messages all in one file. Should be split soon if not now
- OLD_ASSISTANT/ directory — 2048 lines of fully commented-out dead code
- 5 accidentally committed JSON chat logs (NewChat_openrouter_*.json) scattered in source directories — debug artifacts, not test fixtures (maybe add to .gitignore if these are getting generated automatically?)
- HighLevelNodeMap is 786 lines with complex diff logic — could use documentation (not necessary immediately tho)
- CSS files are large (~1272 lines for chat messages) — likely fine for now but suggest component-scoped styles later
- empty files committed (Test_Info.re, Test_MatchExp.re, Test_Stepper.re, DebugConsole.re), just mode changes, no content
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.
what is this folder 'Desktop/Programmierung'? there are a few files from it
| ( | ||
| ~holes=" ", | ||
| ~concave_holes=" ", | ||
| ~special_folds=false, |
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.
are these for the code maps? can we see if it's possible to add these by putting folds directly on the segment structure before sending it to the printer to avoid a special case at this level? if it's easier to put them on the term instead this should now be possible, as maketerm and exp_to_segment now retain whitespace/comments with appropriate settings. lmk if there's complexity here.
(also nbd but ideally avoid re-ordering existing params like projector_to_segment to simplify merge conflicts)
| ~refractor_seg_to_seg, | ||
| ~projector_to_segment, | ||
| ~special_folds, | ||
| ~refractors: list((Id.t, _))=[], |
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.
can you say why you added a default here? piece_to_string is an internal function that we should be cautious about calling independently; probably better to be explicit about its arguments. ideally we avoid having to call it at all but sometimes special cases must be made. open to discussion here.
| ~projector_to_segment, | ||
| projector_to_segment(p), | ||
| ) | ||
| if (special_folds) { |
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.
ah okay i get this now... nevermind what i said earlier about putting folds on through seg/term. but instead i think what you're doing here can be done by wrapping projector_to_segment and special-casing what happens with folds. then just call the regular printer with that special projector_to_segment; i don't think any modification to the printer here is necessary. the special projector_to_segment should basically replace projectors which single convex tiles whose label is ["⋱"]; lmk if you don't get what i mean
|
|
||
| // AddToolLabel_1.0: Make the action types (above) and add their cases to the funs (below) | ||
| [@deriving (show({with_path: false}), sexp, yojson, eq)] | ||
| type agent_editor_action = |
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.
we can probably call these something like structural_actions instead as in principle they are not agent specific
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.
what is this suspiciously named file?
| @@ -286,7 +286,7 @@ | |||
| justify-content: center; | |||
| } | |||
|
|
|||
| #top-bar #editor-mode .icon:hover { | |||
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.
why was this removed? not necessarily bad but I wonder if it might have adverse consequences, ie this now adds a hover effect to icons not within the editor-mode element, which is not obviously desirable
| NinjaKeys.initialize(Shortcut.options(schedule_action)); | ||
| JsUtil.focus_clipboard_shim(); | ||
| /* Setup scroll listener for floating elements (backpack) */ | ||
| FloatingElement.setup_scroll_listener(); |
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.
i don't think this should be removed; this will break the backpack z-index i think
| } | ||
| | AgentGlobals(agent_globals_action) => | ||
| // AgentGlobals updates are handled at Page level with proper async scheduling | ||
| // This case should not be reached, but we include it for completeness |
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.
given that this mandates a significant chance to the settings API (including the scheduling callback), we probably shouldn't have this if it isn't reached?
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.
more mode changes
Project Mode with Coding Agent
Main Tasks for Coding Agent
Make this branch merge-and-test-ready (Russ)
Complete Evaluation Framework (Cyrus)
Misc.