Fix Namespace Bug - #123
Conversation
Greptile SummaryThis PR fixes a namespace bug where hints widgets applied in components were evaluating
Confidence Score: 5/5The change is narrowly scoped to namespace context tracking for DefaultExpr evaluation; the logic is correct and handles all hint-layer combinations. Context is correctly initialised in cx_set_hints, propagated through cx_copy_hints and the applyto chain, merged safely in cx_merge_two_hints, and read back in cx_hints_setdefault. All hint-layer combinations are handled correctly and no pre-existing interfaces are broken. No files require special attention.
|
| Filename | Overview |
|---|---|
| centrallix-os/sys/js/ht_utils_hints.js | Adds namespace context tracking to hints expressions; logic in cx_merge_two_hints, cx_set_hints, cx_copy_hints, and cx_hints_setdefault is correct and handles all edge cases (null layers, single-layer, same-context merge, cross-context conflict). |
Reviews (2): Last reviewed commit: "Fix unsafe hint merge behavior (noticed ..." | Re-trigger Greptile
|
This PR is ready for human review. |
gbeeley
left a comment
There was a problem hiding this comment.
logic change needed for default expressions - thanks!
| if (h1.DefaultExpr && h2.DefaultExpr && h1.Context !== h2.Context) | ||
| { | ||
| nh.DefaultExpr = h1.DefaultExpr; | ||
| nh.Context = h1.Context; |
There was a problem hiding this comment.
This logic doesn't account for the situation where both hint sources have a valid DefaultExpr, but h1's expression returns null while h2's expression does not. In that case the value would be set to null and h2's expression ignored completely. We need some mechanism here to merge two hint expressions with different scopes and keep the scopes/contexts intact for both.
Incidentally this situation applies also to other expression based hints. We probably need a construct like cx_eval_in_scope(scope, expression) that can be placed within expression strings.
There was a problem hiding this comment.
I just barely understand how this code works at all, so I'm not sure how to design that in a way that works well with the rest of this system. We might need to have a design discussion.
There was a problem hiding this comment.
I think the cx_eval_in_scope(scope, expression) approach is the best approach here. It would require that new function that explicitly specifies the scope an expression should be evaluated in. Then, the DefaultExpr handling should evaluate h1, and if it is unspecified or the result is null, evaluate h2 similarly. Scopes do have global identifiers and so can be specified directly.
There was a problem hiding this comment.
Ok, I'll give it a shot and we'll see what comes out.
There was a problem hiding this comment.
Just pushed my and Claude's best attempt to implement this despite my minimal understanding of this part of the system.
|
@gbeeley Please let me know what you think of this implementation. |
This PR fixes #120, so hints widgets applied in components now use the correct namespace context.