Copilot commented on code in PR #7600: URL: https://github.com/apache/texera/pull/7600#discussion_r3792785658
########## docs/operator-demo-videos/src/main/scala/org/apache/texera/demovideos/controllers/Utils.scala: ########## @@ -0,0 +1,130 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.demovideos.controllers + +import com.microsoft.playwright.options.WaitForSelectorState +import com.microsoft.playwright.{Locator, Mouse, Page} + +// ═══════════════════════════════════════════════════════════════════ +// Utils +// ═══════════════════════════════════════════════════════════════════ + +object Utils { + def waitVisible(loc: Locator): Locator = { + loc.waitFor(new Locator.WaitForOptions().setState(WaitForSelectorState.VISIBLE)) + loc + } + + def installFakeCursor(page: Page): Unit = { + // Both the styles and the cursor element are wiped on every page navigation + // (e.g., `page.navigate(".../dashboard")` in createNewWorkflow). `addInitScript` + // re-runs after every load so the cursor follows the user across pages. + val script = + """ + () => { + if (document.getElementById('pw-cursor-style')) return; + const style = document.createElement('style'); + style.id = 'pw-cursor-style'; + style.textContent = ` + #pw-cursor { + position: fixed; left: 0; top: 0; + width: 14px; height: 14px; border-radius: 50%; + background: rgba(255, 0, 0, 0.9); + box-shadow: 0 0 0 3px rgba(255, 0, 0, 0.25); + pointer-events: none; z-index: 2147483647; + transform: translate(-50%, -50%); + } + .pw-click { + position: fixed; left: 0; top: 0; + width: 18px; height: 18px; border-radius: 50%; + border: 3px solid rgba(255, 0, 0, 0.85); + pointer-events: none; z-index: 2147483647; + transform: translate(-50%, -50%); + animation: pw-click-pop 600ms ease-out forwards; + } + @keyframes pw-click-pop { + 0% { opacity: 0.9; transform: translate(-50%, -50%) scale(0.6); } + 70% { opacity: 0.6; transform: translate(-50%, -50%) scale(2.2); } + 100% { opacity: 0.0; transform: translate(-50%, -50%) scale(2.8); } + } + `; + (document.head || document.documentElement).appendChild(style); + + const ensureCursor = () => { + if (document.getElementById('pw-cursor')) return document.getElementById('pw-cursor'); + const cursor = document.createElement('div'); + cursor.id = 'pw-cursor'; + (document.body || document.documentElement).appendChild(cursor); + return cursor; + }; + + const move = (x, y) => { + const c = ensureCursor(); + c.style.left = x + 'px'; + c.style.top = y + 'px'; + }; + + document.addEventListener('mousemove', (e) => move(e.clientX, e.clientY), true); + document.addEventListener('pointermove', (e) => move(e.clientX, e.clientY), true); + + const clickRing = (x, y) => { + const ring = document.createElement('div'); + ring.className = 'pw-click'; + ring.style.left = x + 'px'; + ring.style.top = y + 'px'; + (document.body || document.documentElement).appendChild(ring); + setTimeout(() => ring.remove(), 650); + }; + + document.addEventListener('mousedown', (e) => { move(e.clientX, e.clientY); clickRing(e.clientX, e.clientY); }, true); + document.addEventListener('pointerdown', (e) => { move(e.clientX, e.clientY); clickRing(e.clientX, e.clientY); }, true); + } + """ + + // Persistent across navigations. + page.addInitScript(script) + // Run once now so the cursor is visible immediately on the current page + // (addInitScript only fires on subsequent loads, not retroactively). + page.evaluate(script) Review Comment: `addInitScript(String)` evaluates this string as raw JavaScript, so the arrow expression is created but never invoked on later documents. The immediate `page.evaluate` makes the cursor appear only on the current page; after login or `gotoWorkflowList` navigates, `ControllerContext` still marks it installed and the recording loses the cursor. Register an explicitly invoked script for both paths. ########## docs/operator-demo-videos/src/main/scala/org/apache/texera/demovideos/controllers/NavigationControllerBuilder.scala: ########## @@ -0,0 +1,153 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.demovideos.controllers + +import com.microsoft.playwright._ +import com.microsoft.playwright.options.{AriaRole, LoadState, WaitForSelectorState, WaitUntilState} +import org.apache.texera.demovideos.config.TestDataConfig + +// ═══════════════════════════════════════════════════════════════════ +// 2. NavigationControllerBuilder +// new NavigationControllerBuilder(ctx).createNewWorkflow().execute() +// new NavigationControllerBuilder(ctx).importWorkflow("path/to/sample.json").execute() +// ═══════════════════════════════════════════════════════════════════ + +class NavigationControllerBuilder(ctx: ControllerContext) extends ControllerBuilder(ctx) { + + private def gotoWorkflowList(page: Page): Unit = { + page.navigate( + s"${TestDataConfig.baseUrl}/user/workflow", + new Page.NavigateOptions() + .setWaitUntil(WaitUntilState.DOMCONTENTLOADED) + .setTimeout(Timeouts.Long) + ) + try { + page.waitForLoadState( + LoadState.NETWORKIDLE, + new Page.WaitForLoadStateOptions().setTimeout(Timeouts.Medium) + ) + } catch { + case _: Exception => + } + } + + private def waitForCanvas(page: Page): Unit = { + page + .getByTestId("navigation-workflow-canvas") + .first() + .waitFor( + new Locator.WaitForOptions() + .setState(WaitForSelectorState.VISIBLE) + .setTimeout(Timeouts.Long) + ) + } + + def createNewWorkflow(): this.type = + addStep(new ControllerStep { + override def name = "Create New Workflow" + override def run(ctx: ControllerContext): Unit = { + val page = ctx.page + ctx.ensureFakeCursor() + gotoWorkflowList(page) + + val createBtn = page + .getByTestId("navigation-create-workflow-button") + .or( + page.getByRole(AriaRole.BUTTON, new Page.GetByRoleOptions().setName("Create Workflow")) + ) + .first() + Utils.clickWithCursor(page, createBtn) + + try waitForCanvas(page) + catch { + case _: Exception => + if (!page.url().contains("/workflow/")) { + throw new RuntimeException("Create workflow did not open workflow editor.") + } + } Review Comment: Do not treat an editor-looking URL as equivalent to a loaded canvas. If workflow creation navigates to an error/partially rendered editor, this catch suppresses the 20-second canvas failure and reports the step as successful, so later recording actions fail far from the cause. The new stable canvas hook makes the fallback unnecessary. This issue also appears on line 132 of the same file. ########## docs/operator-demo-videos/src/main/scala/org/apache/texera/demovideos/controllers/LoginControllerBuilder.scala: ########## @@ -0,0 +1,149 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.demovideos.controllers + +import com.microsoft.playwright._ +import com.microsoft.playwright.options.{LoadState, WaitForSelectorState} +import org.apache.texera.demovideos.config.TestDataConfig + +// ═══════════════════════════════════════════════════════════════════ +// 1. LoginControllerBuilder +// new LoginControllerBuilder(ctx).login("user","pass").execute() +// ═══════════════════════════════════════════════════════════════════ + +class LoginControllerBuilder(ctx: ControllerContext) extends ControllerBuilder(ctx) { + + def login(username: String, password: String): this.type = Review Comment: The new login controller has no automated tests in this module, despite containing distinct success, already-authenticated, missing-form/screenshot, and rejected-credential paths. The manual upload-flow verification does not exercise these branches; add Playwright-backed or mocked ScalaTest coverage for both successful login and each failure/skip outcome. ########## docs/operator-demo-videos/src/main/scala/org/apache/texera/demovideos/controllers/NavigationControllerBuilder.scala: ########## @@ -0,0 +1,153 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.demovideos.controllers + +import com.microsoft.playwright._ +import com.microsoft.playwright.options.{AriaRole, LoadState, WaitForSelectorState, WaitUntilState} +import org.apache.texera.demovideos.config.TestDataConfig + +// ═══════════════════════════════════════════════════════════════════ +// 2. NavigationControllerBuilder +// new NavigationControllerBuilder(ctx).createNewWorkflow().execute() +// new NavigationControllerBuilder(ctx).importWorkflow("path/to/sample.json").execute() +// ═══════════════════════════════════════════════════════════════════ + +class NavigationControllerBuilder(ctx: ControllerContext) extends ControllerBuilder(ctx) { Review Comment: There are no automated Scala tests for this new navigation controller, so regressions in create/import sequencing, upload failure handling, and both list/card entry selection will not be caught by the frontend tests, which only guard the `data-testid` attributes. Add controller-level tests covering successful and failed creation/import plus both listing views. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
