feat: Implement features from prose-core-views 0.4.0...0.9.0

[RemiBardon]

No description provided.

[nesium]

Thanks, Rémi!

A few notes…

At first glimpse introducing WebMessageHandler seemed like a good idea to me. After thinking about this a little more deeply, I'm not so sure anymore.

I believe that introducing this new concept makes reasoning about the code harder. As an example, imagine you'd wanted to change something about the context menu without knowing the code base too much. Without these handlers you'd knew that there is no other way than that the context menu would be presented from the view - now this doesn't hold true anymore. Because it's not exactly clear what they are, these handlers are also prone to misuse. Are these views? Are they allowed to generate side-effects?

The alternative - for this feature set - would be that we'd add an extra messageReducer that receives the JS events as actions. This could be a forEach reducer. The messages should be held in an IdentifiedArray, also because there's a lot of index searching going on already. In case encoding becomes a problem it could be done like that:

struct Obj: Encodable {
  var items: [String] = ["a", "b", "c"]

  func encode(to encoder: Encoder) throws {
    var container = encoder.unkeyedContainer()
    
    for item in self.items {
      try container.encode(item)
    }
  }
}

try print(String(data: JSONEncoder().encode(Obj()), encoding: .utf8)!)
// prints ["a","b","c"]

The MessageMenu could then be a plain struct (akin to AlertState) which is not tied to a specific platform. We'd then have a custom View which interprets the MessageMenu depending on the current platform.

Example for a modified JS event handler, building on your work…

struct ToggleReactionPayload: Codable {
  var ids: [Message.ID]
  var reaction: String
}

enum MessageAction {
  case showReactions([Message.ID])
  case toggleReaction(ToggleReactionPayload)
}

struct JSEventHandler {
  var event: String
  var actionFromBody: (Any) throws -> MessageAction

  init<Payload: Decodable>(event: String, action: @escaping (Payload) -> MessageAction) {
    self.event = event
    self.actionFromBody = { body in
      guard let bodyString: String = body as? String,
            let bodyData: Data = bodyString.data(using: .utf8)
      else {
        logger.fault("JS message body should be serialized as a String")
        // TODO: Throw exception
        fatalError()
      }
      
      guard let payload = try? JSONDecoder().decode(Payload.self, from: bodyData) else {
        logger.warning("JS message body could not be decoded as `Self.Body`. Content: \(bodyString)")
        // TODO: Throw exception
        fatalError()
      }
      
      return action(payload)
    }
  }
}

final class ViewStoreScriptMessageHandler: NSObject, WKScriptMessageHandler {
  let viewStore: ViewStore<Void, MessageAction>
  let handler: JSEventHandler
  
  init(handler: JSEventHandler, viewStore: ViewStore<Void, MessageAction>) {
    self.handler = handler
    self.viewStore = viewStore
    super.init()
  }

  func userContentController(
    _ userContentController: WKUserContentController,
    didReceive message: WKScriptMessage
  ) {
    /// TODO: Handle exception
    try! self.viewStore.send(self.handler.actionFromBody(message.body))
  }
}

extension WKUserContentController {
  func addEventHandler(_ handler: JSEventHandler, viewStore: ViewStore<Void, MessageAction>) {
    let handlerName = "handler_" + handler.event.replacingOccurrences(of: ":", with: "_")
  
    self.add(
      ViewStoreScriptMessageHandler(handler: handler, viewStore: viewStore),
      name: handlerName
    )
    
    let script = """
    function \(handlerName)(content) {
      // We need to send a parameter, or the call will not be forwarded to the `WKScriptMessageHandler`
      window.webkit.messageHandlers.\(handlerName).postMessage(JSON.stringify(content));
    }
    MessagingEvent.on("\(handler.event)", \(handlerName));
    """
    
    self.addUserScript(
      WKUserScript(source: script, injectionTime: .atDocumentEnd, forMainFrameOnly: true)
    )
  }
}

// Usage
let handler = JSEventHandler(event: "message:reactions:view", action: MessageAction.showReactions)

What do you think? Am I missing anything?

[RemiBardon]

What do you think? Am I missing anything?

@nesium The problem with your approach is that you suppose all actions only need the message payload (serialized into the correct type). However, when the payload contains a point, we also need a reference to the WKWebView. I will make it so all actions take a pair of (WKWebView, Payload) as an argument, even if we don't need the web view sometimes. It's already what I was doing with the func handle(_ message: WKScriptMessage, body: Body) protocol requirement anyway.

[nesium]

@RemiBardon Given that the flow is WKScriptMessageHandler > ViewStore > Reducer > ViewStore > View and given that the view is the entity which presents other views and holds a reference to the WKWebView, why do we need to pass the WKWebView through all the way?

[RemiBardon]

Ok, I will try to present the picker from the view then.
(I had answered, but never sent the comment. As you can see in my last commit, I managed to not send the view in the actions)

[RemiBardon]

I think I fixed everything, except

The alternative - for this feature set - would be that we'd add an extra messageReducer that receives the JS events as actions. This could be a forEach reducer. The messages should be held in an IdentifiedArray, also because there's a lot of index searching going on already.

and the few #warnings I added.

I'm not sure a forEach reducer would be our best option here, as some actions can make sense on an array of message IDs and not just a single one. I don't have an example in mind, it's just what the prose-core-views API offers. If you want, I can implement it.

Also, @nesium I need your opinion on this: How should I handle removing duplicates in updateNSView? I have two options in mind, one messy and one cleaner:

• The messy way
  1. Add var previousState: ChatState? to ChatView.Coordinator
  2. Start with nil, and update at the end of if !webView.isLoading { (inside this condition otherwise nothing would update)
  3. Replace every do { I wrote by if context.coordinator.viewStore.previousState?.messages != self.viewStore.messages { for example
• The cleaner way
  • In makeNSView, as we already subscribe to a Publisher, create multiple scoped ViewStores and use viewStore.publisher.sink to run the desired code
  • This means we have to keep strong references to the ViewStores

Do you have another idea? What would you suggest?

[RemiBardon]

After a small talk with @nesium, I'm going for the second option, but using viewStore.publisher.my.keypath to avoid the creation of multiple ViewStores. Thanks for the tip!

[RemiBardon]

As we changed the API in prose-core-views v10.0.1, I think we can delay the forEach reducer to the appropriate PR.

If I didn't miss anything, I fixed everything you asked for @nesium. Is everything good now?

[RemiBardon]

Oh, I forgot three #warning("TODO: Throw an exception instead of fatalError"). How should I handle them?

[RemiBardon]

@valeriansaliou What do you think?

[valeriansaliou]

LGTM, can you also add a report button which would create a pre-filled Markdown-formatted email (for the error trace) that would send this to support+macos [at] (our domain) [dot] org ?

That way it gets into Crisp and we can create a rule based on the macos alias tag in the email. Opening eg. Apple Mail or any default mail program, pre-filling the subject, email and body and letting the user press send is OK for me.

[RemiBardon]

I thought about it, but I didn't want to go super fancy in this PR (and I wanted to ask you how to do it properly). Do you mind if I open an issue for this, and we get on it when we introduce logs sharing?