-
Notifications
You must be signed in to change notification settings - Fork 8
Notebook controls #559
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
Notebook controls #559
Conversation
|
@rgbkrk I added the AI cell toggle because sometimes that's useful. That context code was AI-generated |
d41c42c to
7bc676c
Compare
|
Added the "hide AI cells" for now to see if we want to keep it. It'll |
|
I don't want to put in too much work into AI cell filtering yet. Every query in the UI that asks for cells will likely have to be looked at |
packages/schema/src/index.ts
Outdated
| name: "v1.MultipleExecutionRequested", | ||
| schema: Schema.Struct({ | ||
| requestedBy: Schema.String, | ||
| cellsInfo: Schema.Array( |
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.
Using cellsInfo is a bit awkward. I don't want to call it cells because it would be missing too many cell details
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.
If you make this "v1.RunAllCells" then you don't have to pass so much in.
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 did that initially and then realized it wouldn't be deterministic because of the queueId. But your comment holds for the other ones. Others were AI-generated based on this example
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.
Maybe we need to stop using that field. Like, can we take it out?
packages/schema/src/index.ts
Outdated
| }) | ||
| ), | ||
| }), | ||
| }), |
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 would just make this v1.CancellAllExecutions and then not have to pass in the cell IDs. Same for the others.
2b05c15 to
8a6804d
Compare
This reverts commit 8a6804d.
|
Closing in favor of #600 |
Added controls:
Also showing blue badge when there's something active under the menu
Screen.Recording.2025-09-22.at.7.26.09.AM-for-gh.mov
Screen.Recording.2025-09-22.at.7.27.33.AM-for-gh.mov
Delete all cells shows an alert to confirm:
Screen.Recording.2025-09-22.at.7.34.41.AM.mov
Added some tweaks to communicate that AI cell filtering could break things: