calva icon indicating copy to clipboard operation
calva copied to clipboard

add initial idea for flares

Open timothypratley opened this issue 1 year ago β€’ 1 comments

DRAFT

What has changed?

  • Evaluate now calls flareHandler.inspect on all values it receives from the REPL server
  • Added flareHandler that identifies flares (requests for IDE behavior) and dispatches them by type
  • Added a webview constructor to handle flares requesting to display html

Fixes #2679

My Calva PR Checklist

I have:

  • [*] Read How to Contribute.
  • [*] Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • [*] Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
  • [*] Made sure there is an issue registered with a clear problem statement that this PR addresses, (created the issue if it was not present).
    • [*] Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • [ ] Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
  • [x] Added to or updated docs in this branch, if appropriate
  • [ ] Tests
    • [ ] Tested the particular change
    • [ ] Figured if the change might have some side effects and tested those as well.
  • [x] Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-format)
  • [x] Confirmed that there are no linter warnings or errors (use the eslint extension, run npm run eslint before creating your PR, or run npm run eslint-watch to eslint as you go).

Ping @pez, @bpringe, @corasaurus-hex, @Cyrik

timothypratley avatar Dec 26 '24 04:12 timothypratley

Deploy Preview for calva-docs ready!

Name Link
Latest commit 65d6fb02ca80a767c033f2f3e9389a11208bc891
Latest deploy log https://app.netlify.com/sites/calva-docs/deploys/67db34e99fec660008064e69
Deploy Preview https://deploy-preview-2680--calva-docs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Dec 26 '24 04:12 netlify[bot]

Hi @PEZ.

I think this proposal by @timothypratley is a great idea that will be very helpful in our plans to make Clojure beginner-friendly.

The Flares approach will allow us to add functionality that will be seamless to a new user (e.g., compared to installing Joyride and adding a Joyride script). The fact that we can ship it as part of our JVM Clojure tools and libraries matters a lot.

Is there anything we can do to help promote this?

daslu avatar Mar 15 '25 13:03 daslu

I thought there was an open question how to implement the flares. Go the same way that @cursive-ide takes it, or some other way. I have to admit I have not fully understood the trade-offs and implications, but if we can share things easier with Cursive by implementing things the same way, than that would be a win.

PEZ avatar Mar 15 '25 14:03 PEZ

Summary

There are 2 proposals for displaying WebViews in Calva; both work well

  1. This PR detects a special value and triggers IDE behavior (WebView) similar to Cursive. There is a small difference that instead of using tagged literal form #cursive/html{...} it uses a normal map {:calva/flare {:type :wevbview ...}}
  2. And another one #2684 that only triggers IDE behavior when the code was invoked from a custom REPL command snippet, and allows any IDE command. It is a flag on the snippet configuration. Because it invokes commands it uses [cmd args....] to represent the request.

I don't think that Colin will change the Cursive implementation, so the only question is what is best for Calva?

Tradeoffs

Safety: Option 1 applies to all data evaluated in a REPL, it could be malicious data from a database or log file. Therefore the capability should be strictly limited. Is showing HTML and external pages from a URL acceptable? Option 2 applies only to code which brings no additional risk (we already trust libraries) so can possibly do more than WebViews and info.

Access: Option 1 allows users and libraries to make use of flares without configuring or exporting snippets.

Functionality: Do we want WebViews? (yes!) Do we want to be able to show Info messages in the IDE? (I think so, but we could leave it out for now) Do we want to be able to run other IDE commands? (Maybe, but there doesn't seem to be a compelling use for it)

IDE Request syntax: Should it be a tagged literal? a map? a vector? I don't think it matters much, but one thing to notice is that in options (2) snippets are configured to expect ONLY a command as the result, so they have a less unique shape where as (1) they can occur anywhere so they need to be more unique. Possibly we can follow Cursive more closely with tagged literals.

Recommendation

Proceed with option 1, but remove the "command" flare for safety

Ask

Yes, we need a decision from you @PEZ on how to move forward here. It's quite a complicated tradeoff question for a very basic feature, but it will be so valuable to be able to show visualizations. I'm available any time to discuss, show or do interactive code review if that helps.

timothypratley avatar Mar 15 '25 19:03 timothypratley

Thanks for clarifying, @PEZ @timothypratley. This helps a lot.

I think the discussion together with Cursive was worth the try (and insightful to read), but since we could not find a way to make that discussion proceed, it makes sense to keep going here and think what is best from the Calva perspective.


~~I like Timothy's recommendation to proceed with option 1. It will allow updating the WebView from within the user program, not only through custom commands. This allows, for example, to open and update the WebView as a response to events such as a file change. This can be relevant to live-reload experiences of various tools (Figwheel, Shadow, Oz, Clerk, Clay).~~

~~We discussed such a behaviour in a couple of recent videos.~~ ~~* Noj Reload Executable~~ ~~* Noj in a JAR~~

Edit: removed my last comments which were misleading (actually unrelated to this feature).

daslu avatar Mar 15 '25 19:03 daslu

I'm sorry, I didn't realize that I have been stalling this.

Since I am not very familiar with the usecase I think combining the judgement of you two: @timothypratley and @daslu here is best for Calva users. Option 1 sounds good to me. I have some questions/comments:

  1. Evaluating code already can wipe your hard disk or whatever. How would allowing IDE commands bring any extra danger to it?
  2. Does it make sense to name it like {:flare/html ...} rather than {:calva/webview ...}? This way any repl client is invited to implement it, rather than pointing it at Calva and how it should be implemented.
  3. I like the suggestion to go with tagged literals.

Was this what you need from me, @timothypratley? Just let me know, I am a big fan of this whole idea, and would hate to be stalling it further. πŸ˜„

PEZ avatar Mar 16 '25 17:03 PEZ

Yes this is great feedback. Thank you.

  1. The extra danger is that enabling commands would enable data to wipe your hard disk. We accept the code risk, but data risk is something new. We have a level of trust in code because it comes from verified sources, but data does not have any trust protection. The extra risk is similar to SQL injection attacks; a users enters data like my "username" is "DROP TABLES" and the result is that a database is destroyed. The only way to protect against it is to treat user input carefully when creating sql queries. In the REPL we produce values, possibly from untrusted sources, sometimes without realizing it. We don't need to execute external code to create a flare, we just read it in data we are looking at. If there is an exploit, bad actors have a motivation to sprinkle bad data around where you will find it, when entering a "username", or in edn files, or in log messages. In my opinion we should limit the capabilities of flares to specific features like webview that are not expected to be harmful.

2/3. Yes; all of the options you mentioned are great, which one should I change it to?

I think the next steps are:

  1. I should remove the "command" flare from this PR if you agree that it doesn't add value for the risk
  2. I should change the shape of flares to

Then it should be ready for a final code review by you and hopefully release πŸ˜„

timothypratley avatar Mar 16 '25 18:03 timothypratley

@PEZ @daslu what do you think the flare syntax should be?

timothypratley avatar Mar 16 '25 18:03 timothypratley

Cool. Let's remove the command flare for now at least.

Syntax... So something like:

#flare/html {:title ... :html ... ...}
#flare/message {:type <info|warning|error> :message ... ... }

WDYT?

PEZ avatar Mar 16 '25 20:03 PEZ

Makes sense to me!

daslu avatar Mar 16 '25 21:03 daslu

@PEZ @daslu O.K. I've updated the code and documentation to the tagged literals you recommended, and removed commands, so it's over to you now πŸ˜„ To try it out, there are some examples in the docs. Please let me know if further changes are required.

timothypratley avatar Mar 17 '25 06:03 timothypratley

Awesome. The code looks easy to understand, extend and maintain.

You think you can create an example project that we can use from the documentation? It would make it easier to grasp the concept if the user can go through some quick steps that are already prepared.

PEZ avatar Mar 17 '25 07:03 PEZ

@PEZ what do you mean by an "example project"?

The examples in the documentation: https://deploy-preview-2680--calva-docs.netlify.app/flares/ don't need any setup, you just past them into your REPL or code and evaluate them. Maybe the docs can be improved if it isn't clear what they do and why it is useful?

Did you have an idea for an "example project"? (I'm happy to provide one if I can understand a little better what you mean)

timothypratley avatar Mar 17 '25 17:03 timothypratley

I mean something like a repo on Github that you can clone and evaluate some stuff in and have these cool things happen. It was the Clay mention in the docs that started me to wish there was an example project that I could try. It'll get more real with some graphs and stuff showing up. Usually we wouldn't actually need to hold off merging this because of this, but it would help me review and test how things work.

Checking the docs again now, I agree with your observation that it could be clearer with that some of the examples can be just copied and evaluated with Calva's repl connection. Maybe a section like β€œtry these in your repl”? Could be a rich comment form. And we could also have some maven coordinates to use for getting Clay into the mix.

PEZ avatar Mar 17 '25 17:03 PEZ

@PEZ @daslu I've updated the docs accordingly:

  • Simplified and clarified that the examples can be tried at the REPL
  • Added a more compelling example that makes an SVG image, and supplied a screenshot
  • Gave more context about Clay and linked to Clay visualization examples
  • Emphasized the flare/html and de-emphasized the flare/message, as for now the main exciting feature is the ability to show HTML.

I didn't create a repo because the examples are small enough to copy+paste.

For a more spectacular demo, Noj has many notebooks which can be used to show interesting visualizations, and Clay has an examples page that I linked to in the documentation.

Please let me know if you have something else in mind.

timothypratley avatar Mar 18 '25 06:03 timothypratley

To explain my confusion about the repo idea: Clay and Noj are all about creating notebooks, so there are already many great examples to choose from if we want to show charts. Maybe @daslu can recommend one in particular, or if you like we can work on something new together if we have a clear idea of what it should contain.

timothypratley avatar Mar 18 '25 06:03 timothypratley

The updated documentation can be viewed here: https://deploy-preview-2680--calva-docs.netlify.app/flares/

timothypratley avatar Mar 18 '25 06:03 timothypratley

Thanks, I hope to try it today.

Here is one repo for example, that has a meaningful data-analysis example. https://github.com/scicloj/noj-v2-getting-started

It may be convenient to explore, because it has a recorded video demonstrating the current experience we have in Clay.

daslu avatar Mar 18 '25 06:03 daslu

If that seems helpful, I'll try creating a version of Clay that uses the flares.

daslu avatar Mar 18 '25 06:03 daslu

I love the updated docs, @timothypratley. ❀️

Some doc comments:

  • We could mention that it works in any type of REPL: Clojure, ClojureScript, Babashka, Joyride. It is sort of given, but may help people to quicker grasp what is going on.
  • Now that we don't have a Clay example in the docs I think we can do without an example project for that. But I do think this will be a good add later. I'll have a look at the project you showed us at, @daslu, and see if I can describe what an example project could be about after having gained some more insight.
  • It would be great with one or two examples how to use #flare/message
  • The docs for #flare/message lack the :message attribute

Quirks/bugs

support for MessageItem objects

This one may be between docs or implementation: I tried to us a vscode.MessageItem object for the :items and it gives me an error:

  (tagged-literal 'flare/message {:type :info
                                  :title "Calva homepage"
                                  :message "Hello world"
                                  :items [{:title "OK"
                                           :isCloseAffordance false}
                                          {:title "Cancel"
                                           :isCloseAffordance true}]
                                  :then 'clojure.core/println})

=> ((resolve 'clojure.core/println) {"title":"Cancel","isCloseAffordance":true})

:true

[line 1, col 43] Invalid keyword: :.
------ WARNING - :undeclared-var -----------------------------------------------
 Resource: <eval>:1:1
 Use of undeclared Var user/Cancel
--------------------------------------------------------------------------------


[line 1, col 2] Unmatched delimiter ).

It renders the buttons as I expect (Cancel being a close/dismiss button).

Here's a similar invokation using Joyride:

(-> (vscode/window.showInformationMessage
      "Hello"
      #js {:title "OK", :isCloseAffordance false}
      #js {:title "Cancel", :isCloseAffordance true, :payload (clj->js {:foo [1 2 3]})})
    (.then (fn [x] (def x x) (println x))))

#<js/Promise[~]>
#js {:title Cancel, :isCloseAffordance true, :payload #js {:foo #js [1 2 3]}}

The .then function gets the full message item, including any arbitrary payload (:payload isn't part of the MessageItem shape, just something I chose).

If it's not trivial to fix now, we can lock it down to strings and provide an error message if the user tries to use something else. And add MessageItem support later.

I note that the QuickPick API is similar, and see #flare/menu as something we can also add later.

Untitled files/tabs

A thing I often do to just try something out at the REPL is to open an untitled file/tab (cmd+n) and set it to be a Clojure file (cmd+k, m). When I tried this with a flare I got an error. But then I couldn't reproduce it, so I really don't know what happened. I don't think we should spend time on chasing this ghost, but something to be aware of, maybe.

PEZ avatar Mar 18 '25 09:03 PEZ

For message items, on the other branch I have some code that attempts to map Clojure data to vscode objects, which mostly worked πŸ˜„

Quickpick sounds interesting.

Other possibilities for flares:

"workbench.action.quickOpen" "app.clj" "simpleBrowser.api.open" "https://calva.io/" "vscode.diff" ["filea.txt" "fileb.txt" "diff title"

Messages etc all seem to be "promising" but with no concrete use right now.

Maybe I should just remove them for now, and we can always add them later if someone comes up with a use case?

timothypratley avatar Mar 18 '25 18:03 timothypratley

Yeah, we could also wait for use cases before adding.

PEZ avatar Mar 18 '25 20:03 PEZ

πŸ‘ O.K. I removed the message flare for now.

timothypratley avatar Mar 19 '25 05:03 timothypratley

@PEZ let me know if I missed anything πŸ˜„

Meanwhile it looks like the current dev branch has a bug so I can't start a REPL anymore... it looks like it should be easy to fix but I don't want to cause merge conflicts, so I won't do anything about it unless you say to.

Screen Shot 2025-03-19 at 11 18 26 AM

timothypratley avatar Mar 19 '25 18:03 timothypratley

Awesome. Thanks a ton for adding this to Calva, @timothypratley ! ❀️

I think the error you see is also in the released version of Calva. Do you have a custom connect sequence with auto-connect or auto-jackin enabled?

PEZ avatar Mar 19 '25 21:03 PEZ

Yes, this is my settings.json where the problem occurs:

{
  "calva.autoStartRepl": true,
  "calva.replConnectSequences": [
    {
      "name": "Workspace",
      "projectType": "deps.edn",
      "autoSelectForJackIn": true,
      "menuSelections": {
        "cljAliases": ["dev", "test"]
      }
    }
  ]
}

timothypratley avatar Mar 19 '25 22:03 timothypratley

I mistakingly made projectRootPath mandatory in the previous release of Calva. It will probably stay that way, but I should fix the error message at least...

PEZ avatar Mar 19 '25 23:03 PEZ

Oh I see, adding "projectRootPath": ".", to settings fixed it. πŸ˜„

timothypratley avatar Mar 20 '25 01:03 timothypratley