Skip to content

Conversation

@bfollington
Copy link
Contributor

@bfollington bfollington commented Sep 19, 2024

Minimum viable generative UI.

Some issues with reactivity and sharing the state with the iframe context (see comments in code). I'm using a basic read/write postMessage API.

Copy link
Contributor

@seefeldb seefeldb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added comments to explain context. i think i'll go ahead and do those changes.

@customElement('common-iframe')
export class CommonIframe extends LitElement {
@property({ type: String }) src = '';
@property({ type: Object }) context?: CellProxy<any>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, a little issue here is that the UI framework currently listens to that cell already and then issues property updates to the lit element if anything in that cell changes. it doesn't follow cells however. (meta: the ui framework precedes the proxy stuff, so probably it's the ui framework that needs some changes)

we can do the following:

  • exploit that it only listens to the first level by sending data as .context=${{context: data}} (i.e. an object with a data property that then points to the actual cell.
  • now context.context is going to be a Cell. That's now a "simple cell" in cell.ts, which still doesn't automatically follow references. so we can do three things:
    1. manually "crawl" the cell and add listeners to all levels. add a guard against infinite loops by pointing to existing objects when retrieved. a bit complicated, but safe. eventually we want to add a schema that limits how deep we go.
    2. convert to a CellProxy and deep copy it. We'd have to add a method to the simple cell for this, easy. Beware: This can cause infinite loops, since behind the proxy is a generic graph, not a tree.
    3. Present a simple 'get'/'sink'/'write' interface to the iframe and let the iframe manages how deep it wants to go. that's the most work, but also mirrors how we'd want the JS inside our sandboxes to work. we would have to write a library that runs inside the iframe, so that the LLM writes against a simpler API (and unfortunately that one is then different than the one in the sandbox, because this is long-term reactive vs the synchronous style in the sandbox).

i'd go with 2 for expedience and then see.

@bfollington bfollington self-assigned this Sep 20, 2024
Comment on lines 44 to 49
// TODO(ben): should be subscribing to just the cell, not the whole context
// but doing .getAsProxy([key]) or [key] both seem to be lacking .sink()?
const unsub = this.context.sink(() => {
this.notifySubscribers(key, this.context.get()[key]);
});
this.subscriptions.set(key, unsub);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seefeldb not sure what I'm missing here, this code works because it's triggering for any change but I wanted to scope to just a single key... Is it because the value is a primitive (number)? Am I just holding it wrong?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/me really needs to write that README :)

.getAsProxy() turns it into a proxy that just represents the data, but also traverses cell references, since our data can be a graph.

this.context.key(key).sink() would do what you want, but it won't subscribe to changes in referenced cells.

what we can do is call .getAsProxy([key], log) where log is a ReactivityLog. And then do the deep copy. Then log contains all the cells actually read.

But at this point, we can also use the actual scheduler to push changes. Let me do this quickly, might need a few small tweaks.

@seefeldb seefeldb merged commit 34d4a83 into main Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants