Merge branch 'main' into es2020

* main:
  fix(insertTab): Render inserted nav html only once (#4179)
  feat: De-duplicate client console messages (#4177)

NEWS.md

@@ -15,6 +15,10 @@
 
 * Fixed a bug with modals where calling `removeModal()` too quickly after `showModal()` would fail to remove the modal if the remove modal message was received while the modal was in the process of being revealed. (#4173)
 
+* The Shiny Client Console (enabled with `shiny::devmode()`) no longer displays duplicate warning or error message. (#4177)
+
+* Updated the JavaScript used when inserting a tab to avoid rendering dynamic UI elements twice when adding the new tab via `insertTab()` or `bslib::nav_insert()`. (#4179)
+
 # shiny 1.10.0
 
 ## New features and improvements

srcts/src/components/errorConsole.ts

@@ -200,6 +200,42 @@ class ShinyErrorConsole extends LitElement {
     });
   }
 
+  static createClientMessageElement({ headline, message }: ShinyClientMessage) {
+    const msg = document.createElement("shiny-error-message");
+    msg.setAttribute("headline", headline || "");
+    msg.setAttribute("message", message);
+    return msg;
+  }
+
+  appendConsoleMessage({ headline, message }: ShinyClientMessage) {
+    const content =
+      this.shadowRoot?.querySelector<HTMLSlotElement>("slot.content");
+
+    if (content) {
+      const nodeKey = (node: Element) => {
+        const headline = node.getAttribute("headline") || "";
+        const message = node.getAttribute("message") || "";
+        return `${headline}::${message}`;
+      };
+      const newKey = `${headline}::${message}`;
+
+      for (const node of content.assignedElements()) {
+        if (node.tagName.toLowerCase() === "shiny-error-message") {
+          if (nodeKey(node) === newKey) {
+            // Do nothing, this message is already in the console
+            // TODO: Increase count of message here
+            return;
+          }
+        }
+      }
+    }
+
+    this.appendChild(
+      ShinyErrorConsole.createClientMessageElement({ headline, message })
+    );
+    return;
+  }
+
   render() {
     return html` <div class="header">
         <span class="title"> Shiny Client Errors </span>
@@ -523,17 +559,13 @@ function showShinyClientMessage({
 
   // Check to see if an Error Console Container element already exists. If it
   // doesn't we need to add it before putting an error on the screen
-  let errorConsoleContainer = document.querySelector("shiny-error-console");
-  if (!errorConsoleContainer) {
-    errorConsoleContainer = document.createElement("shiny-error-console");
-    document.body.appendChild(errorConsoleContainer);
+  let sec = document.querySelector<ShinyErrorConsole>("shiny-error-console");
+  if (!sec) {
+    sec = document.createElement("shiny-error-console") as ShinyErrorConsole;
+    document.body.appendChild(sec);
   }
 
-  const errorConsole = document.createElement("shiny-error-message");
-  errorConsole.setAttribute("headline", headline);
-  errorConsole.setAttribute("message", message);
-
-  errorConsoleContainer.appendChild(errorConsole);
+  sec.appendConsoleMessage({ headline, message });
 }
 
 /**

srcts/src/shiny/bind.ts

@@ -13,6 +13,15 @@ import { sendImageSizeFns } from "./sendImageSize";
 
 type BindScope = HTMLElement | JQuery<HTMLElement>;
 
+/**
+ * Type guard to check if a value is a jQuery object containing HTMLElements
+ * @param value The value to check
+ * @returns A type predicate indicating if the value is a jQuery<HTMLElement>
+ */
+function isJQuery<T = HTMLElement>(value: unknown): value is JQuery<T> {
+  return typeof (value as any).jquery === "string";
+}
+
 // todo make sure allowDeferred can NOT be supplied and still work
 function valueChangeCallback(
   inputs: InputValidateDecorator,
@@ -79,6 +88,10 @@ const bindingsRegistry = (() => {
    * otherwise returns an ok status.
    */
   function checkValidity(scope: BindScope): void {
+    if (!isJQuery(scope) && !(scope instanceof HTMLElement)) {
+      return;
+    }
+
     type BindingCounts = { [T in BindingTypes]: number };
     const duplicateIds = new Map<string, BindingCounts>();
     const problems: Set<string> = new Set();
@@ -146,7 +159,7 @@ const bindingsRegistry = (() => {
     }:\n${duplicateIdMsg}`;
 
     const event = new ShinyClientMessageEvent({ headline, message });
-    const scopeElement = scope instanceof HTMLElement ? scope : scope.get(0);
+    const scopeElement = isJQuery(scope) ? scope.get(0) : scope;
     (scopeElement || window).dispatchEvent(event);
   }
 

srcts/src/shiny/shinyapp.ts

@@ -18,6 +18,7 @@ import {
   getShinyCreateWebsocket,
   getShinyOnCustomMessage,
   setShinyUser,
+  shinyBindAll,
   shinyForgetLastInputValue,
   shinyUnbindAll,
 } from "./initedMethods";
@@ -1053,8 +1054,13 @@ class ShinyApp {
         const $tabContent = getTabContent($tabset);
         let tabsetId = $parentTabset.attr("data-tabsetid");
 
-        const $divTag = $(message.divTag.html);
-        const $liTag = $(message.liTag.html);
+        // Create a virtual element where we'll temporarily hold the rendered
+        // nav controls so we can rewrite some attributes and choose where to
+        // insert the new controls.
+        const $fragLi = $("<div>");
+        await renderContentAsync($fragLi, message.liTag, "afterBegin");
+
+        const $liTag = $($fragLi).find("> li");
         const $aTag = $liTag.find("> a");
 
         // Unless the item is being prepended/appended, the target tab
@@ -1097,13 +1103,16 @@ class ShinyApp {
         // text items (which function as dividers and headers inside
         // navbarMenus) and whole navbarMenus (since those get
         // constructed from scratch on the R side and therefore
-        // there are no ids that need matching)
+        // there are no ids that need matching). In other words, we're
+        // guaranteed to be inserting only one `nav_panel()`.
+        let fixupDivId = "";
         if ($aTag.attr("data-toggle") === "tab") {
           const index = getTabIndex($tabset, tabsetId);
           const tabId = "tab-" + tabsetId + "-" + index;
 
           $liTag.find("> a").attr("href", "#" + tabId);
-          $divTag.attr("id", tabId);
+          // We'll fixup the div ID after we insert it
+          fixupDivId = tabId;
         }
 
         // actually insert the item into the right place
@@ -1120,11 +1129,8 @@ class ShinyApp {
             $tabset.append($liTag);
           }
         }
+        await shinyBindAll($targetLiTag?.parent() || $tabset);
 
-        await renderContentAsync($liTag[0], {
-          html: $liTag.html(),
-          deps: message.liTag.deps,
-        });
         // jcheng 2017-07-28: This next part might look a little insane versus the
         // more obvious `$tabContent.append($divTag);`, but there's a method to the
         // madness.
@@ -1152,40 +1158,30 @@ class ShinyApp {
         // In theory the same problem exists for $liTag but since that content is
         // much less likely to include arbitrary scripts, we're skipping it.
         //
-        // This code could be nicer if we didn't use renderContent, but rather the
-        // lower-level functions that renderContent uses. Like if we pre-process
-        // the value of message.divTag.html for singletons, we could do that, then
-        // render dependencies, then do $tabContent.append($divTag).
-        await renderContentAsync(
-          $tabContent[0],
-          { html: "", deps: message.divTag.deps },
-          // @ts-expect-error; TODO-barret; There is no usage of beforeend
-          "beforeend"
-        );
-        for (const el of $divTag.get()) {
-          // Must not use jQuery for appending el to the doc, we don't want any
-          // scripts to run (since they will run when renderContent takes a crack).
-          $tabContent[0].appendChild(el);
-          // If `el` itself is a script tag, this approach won't work (the script
-          // won't be run), since we're only sending innerHTML through renderContent
-          // and not the whole tag. That's fine in this case because we control the
-          // R code that generates this HTML, and we know that the element is not
-          // a script tag.
-          await renderContentAsync(el, el.innerHTML || el.textContent);
+        // garrick 2025-01-23: Keeping in mind the above, the `shiny-insert-tab`
+        // method was re-written to avoid adding the nav controls (liTag) and
+        // the nav panel contents (divTag) twice. Now, we use
+        // renderContentAsync() to add both sections to the DOM only once.
+
+        await renderContentAsync($tabContent[0], message.divTag, "beforeEnd");
+
+        if (fixupDivId) {
+          // We're inserting one nav_panel() and need to fixup the content ID
+          $tabContent.find('[id="tab-tsid-id"]').attr("id", fixupDivId);
         }
 
         if (message.select) {
           $liTag.find("a").tab("show");
         }
         /* Barbara -- August 2017
-  Note: until now, the number of tabs in a tabsetPanel (or navbarPage
-  or navlistPanel) was always fixed. So, an easy way to give an id to
-  a tab was simply incrementing a counter. (Just like it was easy to
-  give a random 4-digit number to identify the tabsetPanel). Now that
-  we're introducing dynamic tabs, we must retrieve these numbers and
-  fix the dummy id given to the tab in the R side -- there, we always
-  set the tab id (counter dummy) to "id" and the tabset id to "tsid")
-  */
+         * Note: until now, the number of tabs in a tabsetPanel (or navbarPage
+         * or navlistPanel) was always fixed. So, an easy way to give an id to
+         * a tab was simply incrementing a counter. (Just like it was easy to
+         * give a random 4-digit number to identify the tabsetPanel). Now that
+         * we're introducing dynamic tabs, we must retrieve these numbers and
+         * fix the dummy id given to the tab in the R side -- there, we always
+         * set the tab id (counter dummy) to "id" and the tabset id to "tsid")
+         */
         function getTabIndex(
           $tabset: JQuery<HTMLElement>,
           tabsetId: string | undefined