chore: code review tweaks

v3/cypress/cypress.d.ts

@@ -12,6 +12,7 @@ declare global {
        */
       clickMenuItem(item: string): void
       clickToUnselect(subject: Array<{ x: number, y: number }>, options?: { delay: number }): void
+      clickWhenClickable(selector: string, shouldCondition?: string): void
       checkDragAttributeHighlights(source: string, attribute: string, target: string, exists?: boolean): void
       dragAttributeToTarget(source: string, attribute: string, target: string, targetNumber?: number): void
       mouseMoveBy(subject: JQuery<HTMLElement>, targetRect: DOMRect, options?: { delay: number }): void

v3/cypress/support/commands.ts

@@ -2,6 +2,29 @@ Cypress.Commands.add("clickMenuItem", text => {
   cy.get("button[role=menuitem]").contains(text).click()
 })
 
+Cypress.Commands.add("clickWhenClickable", (selector: string, shouldCondition = "be.visible") => {
+  /* According to ChatGPT:
+
+    Using `.should('be.visible').then(...)` is generally more reliable than chaining actions like
+    `.should('be.visible').click()` directly after an assertion.
+
+    Here's why:
+
+    1. Retry Mechanism
+        Cypress will retry the `.should` assertion until it passes (up to the default timeout).
+        However, when you chain an action like `.click()` directly after `.should('be.visible')`,
+        the `.click()` may not retry if the element’s state changes quickly.
+        This can result in flaky tests if the visibility of the element isn't stable.
+    2. Clear Separation
+        By using `.then(...)` after the `.should`, you separate the assertion
+        (which benefits from Cypress’s retry behavior) from the action,
+        ensuring the action only happens once the assertion is fully satisfied.
+  */
+  cy.get(selector).should(shouldCondition).then($el => {
+    cy.wrap($el).click()
+  })
+})
+
 Cypress.Commands.add("dragAttributeToTarget", (source, attribute, target, targetNumber) => {
   const el = {
     tableColumnHeader:

v3/cypress/support/elements/table-tile.ts

@@ -232,24 +232,22 @@ export const TableTileElements = {
   },
   createNewTableFromToolShelf() {
     c.getIconFromToolShelf("table").click()
-    cy.wait(1000)
-    cy.get("[data-testid=tool-shelf-table-new]").click()
+    cy.clickWhenClickable("[data-testid=tool-shelf-table-new]")
   },
   createNewClipboardTableFromToolShelf() {
     c.getIconFromToolShelf("table").click()
-    cy.wait(1000)
-    cy.get("[data-testid=tool-shelf-table-new-clipboard]").click()
+    cy.clickWhenClickable("[data-testid=tool-shelf-table-new-clipboard]")
   },
   openExistingTableFromToolShelf(name: string) {
     c.getIconFromToolShelf("table").click()
-    cy.wait(1000)
-    cy.get(`[data-testid=tool-shelf-table-${name}]`).click()
+    cy.clickWhenClickable(`[data-testid=tool-shelf-table-${name}]`)
   },
   deleteDataSetFromToolShelf(index = 0) {
     c.getIconFromToolShelf("table").click()
-    cy.wait(1000)
-    cy.get(`.tool-shelf-menu-trash-icon`).eq(index).click()
-    cy.get(`.delete-data-set-button-delete`).click()
+    cy.get(`.tool-shelf-menu-trash-icon`).eq(index).should("be.visible").then($el => {
+      cy.wrap($el).click()
+    })
+    cy.clickWhenClickable(`.delete-data-set-button-delete`)
   },
   getToggleCardView() {
     return cy.get("[data-testid=case-table-toggle-view]")

v3/src/components/common/edit-formula-modal.tsx

@@ -4,6 +4,7 @@ import {
 import React, { useEffect, useState } from "react"
 import { observer } from "mobx-react-lite"
 import { clsx } from "clsx"
+import { isCommandKeyDown } from "../../utilities/platform-utils"
 import { t } from "../../utilities/translation/translate"
 import { FormulaEditor } from "./formula-editor"
 import { FormulaEditorContext, useFormulaEditorState } from "./formula-editor-context"
@@ -37,7 +38,7 @@ export const EditFormulaModal = observer(function EditFormulaModal({
     setFormula(value || "")
   }, [value, setFormula])
 
-  const handleApplyClick = () => {
+  const applyAndClose = () => {
     applyFormula(formula)
     closeModal()
   }
@@ -72,10 +73,20 @@ export const EditFormulaModal = observer(function EditFormulaModal({
     onClick: closeModal
   }, {
     label: t("DG.AttrFormView.applyBtnTitle"),
-    onClick: handleApplyClick,
+    onClick: applyAndClose,
     default: true
   }]
 
+  function handleKeyDown(event: React.KeyboardEvent) {
+    if (event.key === "Enter" && isCommandKeyDown(event)) {
+      applyAndClose()
+    }
+    if (event.key === "Escape") {
+      closeModal()
+    }
+    event.stopPropagation()
+  }
+
   return (
     <FormulaEditorContext.Provider value={formulaEditorState}>
       <CodapModal
@@ -90,7 +101,7 @@ export const EditFormulaModal = observer(function EditFormulaModal({
           <div className="codap-header-title" />
           <ModalCloseButton onClick={closeModal} data-testid="modal-close-button" />
         </ModalHeader>
-        <ModalBody className="formula-modal-body" onKeyDown={e => e.stopPropagation()}>
+        <ModalBody className="formula-modal-body" onKeyDown={handleKeyDown}>
           <FormControl display="flex" flexDirection="column" className="formula-form-control">
             <FormLabel display="flex" flexDirection="row">
               <span className="title-label">{titleLabel}</span>
@@ -105,7 +116,7 @@ export const EditFormulaModal = observer(function EditFormulaModal({
             </FormLabel>
             <FormLabel>
               {formulaPrompt ?? t("DG.AttrFormView.formulaPrompt")}
-              <FormulaEditor onClose={closeModal} onApply={handleApplyClick}/>
+              <FormulaEditor />
             </FormLabel>
           </FormControl>
           <Flex flexDirection="row" justifyContent="flex-start">
@@ -119,7 +130,7 @@ export const EditFormulaModal = observer(function EditFormulaModal({
               }
             </Box>
             <Box position="relative">
-              <Button 
+              <Button
                 className={clsx("formula-editor-button", "insert-function", {"menu-open": showFunctionMenu})}
                 size="xs" ml="5" onClick={handleInsertFunctionsOpen} data-testid="formula-insert-function-button"
               >

v3/src/components/common/formula-editor.tsx

@@ -33,8 +33,6 @@ const kAllOptions: ICompletionOptions = {
 interface IProps {
   // options default to true if not specified
   options?: Partial<ICompletionOptions>
-  onClose?: () => void;
-  onApply?: (formula: string) => void;
 }
 
 /*
@@ -217,7 +215,7 @@ const codapHighlightingViewPlugin = ViewPlugin.fromClass(
 /*
  * editor configuration
  */
-function cmExtensionsSetup(onClose: () => void, onApply: (formula: string) => void) {
+function cmExtensionsSetup() {
   let keymaps: KeyBinding[] = []
   keymaps = keymaps.concat(closeBracketsKeymap)
   keymaps = keymaps.concat(defaultKeymap)
@@ -234,39 +232,20 @@ function cmExtensionsSetup(onClose: () => void, onApply: (formula: string) => vo
     }),
     codapHighlightingViewPlugin,
     keymap.of(keymaps.flat()),
-    Prec.highest( //Overrides CodeMirror's default keymap for Cmd-Enter key
-      keymap.of([
-        { key: "Mod-Enter",
-          run:  (view) => {
-            onApply(view.state.doc.toString())
-            view.dom.closest('.codap-modal')?.classList.add('hidden')
-            onClose()
-            return true
-          }
-        }
-      ])
-    ),
-    keymap.of([
-      { key: "Escape",
-        run: (view) => {
-          console.log("Escape key pressed. Formula not saved.")
-          view.dom.closest('.codap-modal')?.classList.add('hidden')
-          onClose()
-          return false
-        }
-      }
-    ])
+    Prec.highest( // Overrides CodeMirror's default keymap for Cmd-Enter key
+      keymap.of([{ key: "Mod-Enter", run: () => true }])
+    )
   ]
   return extensions.filter(Boolean)
 }
 
-export function FormulaEditor({ options: _options, onClose, onApply }: IProps) {
+export function FormulaEditor({ options: _options }: IProps) {
   const dataSet = useDataSetContext()
   const jsonOptions = JSON.stringify(_options ?? {})
   const options = useMemo(() => JSON.parse(jsonOptions), [jsonOptions])
   const cmRef = useRef<ReactCodeMirrorRef>(null)
+  const extensions = useMemo(() => cmExtensionsSetup(), [])
   const { formula, setFormula, setEditorApi } = useFormulaEditorContext()
-  const extensions = useMemo(() => cmExtensionsSetup(onClose!, onApply!), [onClose, onApply])
 
   // update the editor state field with the appropriate data set
   const handleCreateEditor = useCallback((view: EditorView, state: EditorState) => {

v3/src/utilities/platform-utils.ts

@@ -0,0 +1,7 @@
+export const isMac = navigator.platform.toLowerCase().includes("mac")
+
+export const cmdKey = isMac ? "Meta" : "Control"
+
+export function isCommandKeyDown(event: KeyboardEvent | React.KeyboardEvent) {
+  return (isMac && event.metaKey) || (!isMac && event.ctrlKey)
+}