Skip to content

Conversation

@toyamarinyon
Copy link
Contributor

@toyamarinyon toyamarinyon commented Dec 10, 2025

User description

Remove support for passing app prop directly and the associated type unions and type guards. Always fetch the app via useSWR when the node is configured. This reduces complexity and makes the component's data flow more straightforward.


PR Type

Enhancement


Description

  • Removes union type and type guard functions for props

  • Simplifies component to accept only node prop

  • Always fetches app data via useSWR when configured

  • Eliminates unnecessary useMemo logic for app resolution


Diagram Walkthrough

flowchart LR
  A["AppEntryInputDialog<br/>accepts node prop"] -->|"node.content.status<br/>== configured"| B["useSWR fetches app<br/>via appId"]
  B -->|"app data"| C["Component renders<br/>with app"]
  D["Removed: app prop<br/>support"] -.->|"simplified"| A
  E["Removed: type unions<br/>& type guards"] -.->|"simplified"| A
Loading

File Walkthrough

Relevant files
Enhancement
dialog.tsx
Remove app prop support and type guards                                   

internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx

  • Removed App type import and union type definitions
    (AppEntryInputDialogFromAppEntryNode, AppEntryInputDialogFromApp)
  • Removed type guard functions isFromAppEntryNode() and isFromApp()
  • Simplified component props to accept only node: AppEntryNode parameter
  • Replaced conditional useMemo logic with direct useSWR call based on
    node configuration status
  • Removed unused useMemo import
+7/-60   

Note

Simplifies AppEntryInputDialog to accept only node and always fetch the app via SWR, removing the app prop and related unions/guards.

  • UI: internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
    • Props: Require node; remove support for app prop and associated union types/type guards.
    • Data fetching: Always fetch app with useSWR when node.content.status === "configured".
    • Cleanup: Remove useMemo and unused imports; simplify isLoading/app handling.

Written by Cursor Bugbot for commit fc3fc27. This will update automatically on new commits. Configure here.

Summary by CodeRabbit

  • Refactor
    • Simplified internal dialog component architecture by streamlining prop handling and data fetching logic, reducing code complexity and improving maintainability.

✏️ Tip: You can customize this high-level summary in your review settings.

Remove support for passing `app` prop directly and the associated type
unions and type guards. Always fetch the app via useSWR when the node is
configured. This reduces complexity and makes the component's data flow
more straightforward.
Copilot AI review requested due to automatic review settings December 10, 2025 03:03
@toyamarinyon toyamarinyon requested a review from shige as a code owner December 10, 2025 03:03
@changeset-bot
Copy link

changeset-bot bot commented Dec 10, 2025

⚠️ No Changeset found

Latest commit: fc3fc27

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package or glob expression "giselles-ai" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

@vercel
Copy link

vercel bot commented Dec 10, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
giselle Ready Ready Preview Comment Dec 10, 2025 3:08am
ui Ready Ready Preview Comment Dec 10, 2025 3:08am

@giselles-ai
Copy link

giselles-ai bot commented Dec 10, 2025

Finished running flow.

Step 1
🟢
On Pull Request OpenedStatus: Success Updated: Dec 10, 2025 3:03am
Step 2
🟢
Manual QAStatus: Success Updated: Dec 10, 2025 3:06am
🟢
Prompt for AI AgentsStatus: Success Updated: Dec 10, 2025 3:06am
Step 3
🟢
Create a Comment for PRStatus: Success Updated: Dec 10, 2025 3:08am
Step 4
🟢
Create Pull Request CommentStatus: Success Updated: Dec 10, 2025 3:08am

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR simplifies the AppEntryInputDialog component by removing the ability to pass an app prop directly, requiring all callers to provide only a node prop. The component now always fetches the app data via useSWR when the node is configured, eliminating the need for complex type unions and type guards.

Key Changes:

  • Removed App type import and complex prop type unions (AppEntryInputDialogFromAppEntryNode and AppEntryInputDialogFromApp)
  • Eliminated type guard functions (isFromAppEntryNode and isFromApp) that were used to differentiate between the two prop patterns
  • Simplified the component to accept only node: AppEntryNode as a prop, making the data flow more straightforward

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 10, 2025

Walkthrough

The AppEntryInputDialog component was refactored to remove discriminated union-based props, replacing them with a single explicit node: AppEntryNode parameter. App data fetching was simplified from multi-source memoization to direct SWR usage based on node status. Internal type guards and helper functions were removed.

Changes

Cohort / File(s) Summary
AppEntryInputDialog Component Simplification
internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
Removed union-based props (AppEntryInputDialogFromAppEntryNode | AppEntryInputDialogFromApp) and replaced with explicit { onSubmit, onClose, node: AppEntryNode } signature. Simplified app data fetching from memoization across multiple prop sources to direct SWR call based on node.content.status. Removed internal type guards (isFromAppEntryNode, isFromApp) and associated type machinery. Updated data consumption to use fetched app directly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Prop signature change: Verify the new node: AppEntryNode contract is correctly typed and all callers have been updated
  • SWR integration: Confirm the simplified SWR key logic (node.content.status === "configured") correctly gates data fetching
  • Data usage: Ensure all references to app data now use the direct fetched value and no stale indirection remains

Poem

🐰 A dialog once juggled two paths with great care,
Now clarity reigns—one node declared fair.
No guards, no unions, just straightforward SWR,
The code hops lighter, a leaner concur!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: simplifying AppEntryInputDialog to accept only the node prop, which aligns with removing the union-based props and app prop support.
Description check ✅ Passed The description covers all required template sections with clear explanations of the changes, the rationale, and includes a helpful diagram and detailed file walkthrough for reviewer context.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch toyamarinyon/20251210-120205

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-free-for-open-source-projects

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-free-for-open-source-projects

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Synchronize state with prop changes

The useSWR hook will automatically refetch when the key changes, but consider
adding explicit handling or validation to ensure the component properly resets
state when the node prop changes, especially if there's local state that depends
on the fetched app data.

internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx [30-35]

 const { isLoading, data: app } = useSWR(
 	node.content.status === "configured"
 		? { namespace: "getApp", appId: node.content.appId }
 		: null,
 	({ appId }) => client.getApp({ appId }).then((res) => res.app),
 );
 
+useEffect(() => {
+	// Reset validation errors when node changes
+	setValidationErrors({});
+}, [node.id]);
+
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - When using React hooks like useSWR with props that may change, ensure data is synchronized with prop changes. The component should handle scenarios where the node prop changes (e.g., switching between different nodes or contexts).

Low
  • More

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx (1)

15-35: Props simplification and SWR usage look good; confirm behavior for unconfigured/error states

The move to a single node: AppEntryNode prop and always resolving app via useSWR keeps this component’s contract much simpler and matches the PR goal.

Two things to double‑check:

  • When node.content.status !== "configured" (or when getApp fails and leaves app undefined), the component ends up returning null, so the dialog silently renders nothing. If callers can open this dialog for such nodes or hit transient network errors, consider an explicit “not configured” / “failed to load” UI instead of returning null.
  • Now that the app prop path is gone, this code always fetches via getApp (modulo whatever caching useGiselle/SWR provide). If there are call sites that already have the app object in memory, you might want to reuse that via SWR’s fallbackData or a shared cache key pattern, otherwise this behavior is fine.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d35a729 and fc3fc27.

📒 Files selected for processing (1)
  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js,jsx}: Favor clear, descriptive names and type annotations over clever tricks
If you need a multi-paragraph comment, refactor until intent is obvious

**/*.{ts,tsx,js,jsx}: Use async/await and proper error handling
Variables and functions should use camelCase naming
Booleans and functions should use is, has, can, should prefixes
Function names should clearly indicate purpose

Files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)

**/*.{ts,tsx}: MUST run pnpm biome check --write [filename] after EVERY code modification
All code changes must be formatted using Biome before being committed
Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoid any types
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames
Use PascalCase for React components and classes
Use camelCase for variables, functions, and methods
Use prefixes like is, has, can, should for boolean variables (e.g., isEnabled, hasPermission)
Use prefixes like is, has, can, should for boolean functions (e.g., isTriggerRequiringCallsign(), hasActiveSubscription()) instead of imperative verbs
Use verbs or verb phrases for function naming that clearly indicate purpose (e.g., calculateTotalPrice(), not process())

Use PascalCase for React component and class names

Use TypeScript and avoid any

Files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
**/*.tsx

📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)

Use functional components with React hooks

Files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
**/*.{js,ts,tsx,jsx,py,java,cs,cpp,c,go,rb,php,swift,kt,scala,rs,dart}

📄 CodeRabbit inference engine (.cursor/rules/language-support.mdc)

Write all code comments in English

Files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
**/*

📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)

Use kebab-case for file names (e.g., user-profile.ts, api-client.tsx)

Files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)

**/*.{js,ts,jsx,tsx}: Use camelCase for variable names, functions, and methods
Use verbs or verb phrases for function names to clearly indicate what the function does (e.g., calculateTotalPrice(), validateUserInput())
Use nouns or noun phrases for variable names to describe what the variable represents
Use boolean prefixes (is, has, can, should) for boolean variables and functions returning boolean values (e.g., isEnabled, hasPermission, isTriggerRequiringCallsign())

**/*.{js,ts,jsx,tsx}: Run pnpm biome check --write [filename] after every code change
All code must be formatted with Biome before commit

Files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
**/*.{ts,tsx,js,jsx,css}

📄 CodeRabbit inference engine (AGENTS.md)

Files should use kebab-case naming

Files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
**/*.{tsx,ts}

📄 CodeRabbit inference engine (AGENTS.md)

Components should use PascalCase naming

Files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:19.740Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/**/*.{ts,tsx} : Lift actions into the store (e.g., `updateNode`) and call them from components needing mutations instead of passing mutation callbacks as props
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:19.740Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/components/node/**/*.{ts,tsx} : In Node component, select exactly the node data, derived connection ids, and UI flags needed, providing a custom equality function that combines strict equality for references with `shallow` for arrays
📚 Learning: 2025-11-25T03:07:19.740Z
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:19.740Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/**/*.{ts,tsx} : Lift actions into the store (e.g., `updateNode`) and call them from components needing mutations instead of passing mutation callbacks as props

Applied to files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
📚 Learning: 2025-11-25T03:07:19.740Z
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:19.740Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/**/*.{ts,tsx} : Replace context-wide reads from `src/editor/v2` with fine-grained selectors against the Zustand store

Applied to files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
📚 Learning: 2025-11-25T03:07:19.740Z
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:19.740Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/components/node/**/*.{ts,tsx} : In Node component, select exactly the node data, derived connection ids, and UI flags needed, providing a custom equality function that combines strict equality for references with `shallow` for arrays

Applied to files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
📚 Learning: 2025-11-25T03:07:19.740Z
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:19.740Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/**/*.{ts,tsx} : In selectors, avoid selecting full state; select by id (e.g., `s.nodesById[id]`) and derive small, stable shapes

Applied to files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
📚 Learning: 2025-11-25T03:07:19.740Z
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:19.740Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/**/*.{ts,tsx} : Keep derivations simple and stable in selectors; maintain ordering when mapping to arrays so equality checks can short-circuit

Applied to files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
📚 Learning: 2025-11-25T03:07:19.740Z
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:19.740Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/components/**/*.{ts,tsx} : Do not subscribe to `ui` or `nodesById` wholesale; always pick the narrow fields by id that your component needs

Applied to files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
📚 Learning: 2025-11-25T03:07:19.740Z
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:19.740Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/**/*.{ts,tsx} : Use `updateNode(id, patch)` to patch a single node in `nodesById` without touching other nodes

Applied to files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
📚 Learning: 2025-11-25T03:05:31.051Z
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-11-25T03:05:31.051Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Update the `TourGlobalStyles` component in `workspace-tour.tsx` for animation changes

Applied to files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
📚 Learning: 2025-11-25T03:07:07.498Z
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/editor/properties-panel/trigger-node-properties-panel/providers/github-trigger/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:07.498Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/properties-panel/trigger-node-properties-panel/providers/github-trigger/**/*.tsx : Wrap all state updates in `startTransition` for consistent UI behavior during configuration and reconfiguration flows.

Applied to files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
📚 Learning: 2025-11-25T03:07:19.740Z
Learnt from: CR
Repo: giselles-ai/giselle PR: 0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-11-25T03:07:19.740Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/**/*.{ts,tsx} : Use shallow equality for arrays and objects in selectors to avoid false-positive updates

Applied to files:

  • internal-packages/workflow-designer-ui/src/app/app-entry-input-dialog/dialog.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
  • GitHub Check: Cursor Bugbot
  • GitHub Check: check

@giselles-ai
Copy link

giselles-ai bot commented Dec 10, 2025

🔍 QA Testing Assistant by Giselle

📋 Manual QA Checklist

Based on the changes in this PR, here are the key areas to test manually:

  • AppEntryInputDialog: Verify dialog opens correctly after selecting an "App Entry" node and configuring it with a valid app.
  • AppEntryInputDialog: Confirm a loading spinner appears while the app configuration is being fetched.
  • AppEntryInputDialog: Assert that the dialog displays the correct input fields based on the fetched app configuration.
  • AppEntryInputDialog: Test filling in all input fields with valid data and submitting the form via the "Run" button (▶️ icon).
  • AppEntryInputDialog: Verify the dialog closes upon successful form submission and the workflow proceeds as expected.
  • AppEntryInputDialog: Confirm the dialog closes without submitting data when the 'X' icon is clicked.
  • AppEntryInputDialog: Confirm the dialog closes without submitting data when the Escape key is pressed.

✨ Prompt for AI Agents

Use the following prompts with Cursor or Claude Code to automate E2E testing:

📝 E2E Test Generation Prompt

## **Prompt for AI Agent: Generate Playwright E2E Tests**

**Act as an expert QA engineer specializing in automated E2E testing with Playwright.**

Your task is to generate a comprehensive E2E test suite for the `AppEntryInputDialog` component based on the provided context. The component has been refactored to simplify its props and data fetching logic.

### 1. Context Summary

The pull request refactors the `AppEntryInputDialog` component. Previously, it could accept either an `app` object directly or a `node` object, from which it would then fetch the app details.

The change removes the ability to pass the `app` prop directly. The component now **only accepts a `node: AppEntryNode` prop** and **always uses the `useSWR` hook to fetch the app configuration** from the `getApp` endpoint whenever the node's status is `"configured"`.

*   **Key Functionality Change:** The data flow is now unified. The component is entirely dependent on a remote data fetch (`useSWR`) to render its main content when the associated node is configured.
*   **Affected User Flow:** The primary user flow is a user triggering the opening of this dialog, which should then display a form based on the configuration of an "App".
*   **Critical Paths to Test:**
    1.  The loading state while the app configuration is being fetched.
    2.  The successful rendering of the form after the data fetch completes.
    3.  The behavior when the component is rendered for a node that is *not* configured (i.e., no data fetch should occur).
    4.  The handling of data fetch errors.
    5.  Regression testing of the form's submission functionality.

### 2. Test Scenarios

Generate tests covering the following scenarios. Each test should be atomic and self-contained.

*   **Happy Path Scenarios:**
    1.  **Test 1: Displays Loading State and Renders Form on Success:**
        *   **Given:** The `AppEntryInputDialog` is opened with a `node` prop where `node.content.status` is `"configured"`.
        *   **When:** The `getApp` API call is in flight.
        *   **Then:** A loading indicator (`LoaderIcon`) should be visible within the dialog.
        *   **When:** The `getApp` API call resolves successfully with a valid app configuration.
        *   **Then:** The loading indicator should disappear.
        *   **And:** The form, including input fields defined in the mock API response, should be rendered and visible.

    2.  **Test 2: Form Submission (Regression Test):**
        *   **Given:** The dialog has successfully loaded and rendered the form (as in Test 1).
        *   **When:** The user fills in the form fields with valid data and clicks the "Run" button (the one with `PlayIcon`).
        *   **Then:** The `onSubmit` callback should be triggered with the correct input data structure. (You can test this by asserting a side effect, like a console log or a mocked function call).

*   **Edge Case & Sad Path Scenarios:**
    3.  **Test 3: Handles API Fetch Error:**
        *   **Given:** The `AppEntryInputDialog` is opened with a `configured` node.
        *   **When:** The `getApp` API call is mocked to return a 500 server error.
        *   **Then:** The dialog should display a user-friendly error state (e.g., an error message, a disabled submit button). It should not be stuck in a loading state.

    4.  **Test 4: Non-Configured Node:**
        *   **Given:** The `AppEntryInputDialog` is opened with a `node` prop where `node.content.status` is **not** `"configured"`.
        *   **When:** The component renders.
        *   **Then:** No network request to the `getApp` endpoint should be made.
        *   **And:** The dialog should render in a non-interactive or empty state, as there is no app schema to display. Assert that the main form content area is empty or shows a placeholder.

### 3. Playwright Implementation Instructions

Use the following guidelines to write the tests in TypeScript.

*   **Mocking API Requests:** Use `page.route()` to intercept and mock the `getApp` API call. This is **critical** for testing all scenarios without a real backend. The `useSWR` hook in the component will use the mocked responses.

    ```typescript
    // Example of mocking a successful response
    await page.route('**/api/v1/apps/*', async (route, request) => {
        // Assuming the endpoint is something like /api/v1/apps/{appId}
        if (request.url().includes('getApp')) { // Make this more specific if possible
            const mockAppResponse = {
                app: {
                    id: 'app-123',
                    name: 'My Test App',
                    config: {
                        inputs: [
                            { type: 'text', name: 'prompt', label: 'User Prompt' },
                            { type: 'file', name: 'context_file', label: 'Context File' }
                        ]
                    }
                }
            };
            await route.fulfill({
                status: 200,
                contentType: 'application/json',
                body: JSON.stringify(mockAppResponse),
            });
        }
    });

    // Example of mocking an error response
    await page.route('**/api/v1/apps/*', async (route) => {
        await route.fulfill({
            status: 500,
            contentType: 'application/json',
            body: JSON.stringify({ message: 'Internal Server Error' }),
        });
    });
    ```

*   **Selectors:** Target elements robustly. The diff indicates usage of Radix UI and Lucide icons.
    *   **Dialog:** `page.getByRole('dialog')`
    *   **Loading Icon:** The `LoaderIcon` likely renders as an SVG. A `data-testid` is best. If not present, use a selector like `getByRole('dialog').locator('svg.lucide-loader-icon')`. For the purpose of this task, let's assume a `data-testid="loading-spinner"` is added.
    *   **Form Inputs:** `page.getByLabel('User Prompt')`
    *   **Submit Button:** `page.getByRole('button', { name: 'Run' })` (Assuming the button with `PlayIcon` has accessible text "Run").
    *   **Close Button:** `page.getByRole('button', { name: 'Close' })`

*   **User Interactions:**
    *   Use `await page.click(...)` to open the dialog (this part will be outside the component's test, simulating how a user opens it).
    *   Use `await locator.fill(...)` to enter text into inputs.
    *   Use `await locator.click()` to submit the form.

*   **Assertions:**
    *   `await expect(page.getByTestId('loading-spinner')).toBeVisible();`
    *   `await expect(page.getByTestId('loading-spinner')).toBeHidden();`
    *   `await expect(page.getByRole('dialog')).toBeVisible();`
    *   `await expect(page.getByLabel('User Prompt')).toBeVisible();`
    *   To verify a network call was NOT made, you can use a flag inside `page.route` and assert its state after the interaction.

    ```typescript
    let getAppCalled = false;
    await page.route('**/api/v1/apps/*', () => { getAppCalled = true; /* ... */ });
    // ... perform action ...
    expect(getAppCalled).toBe(false);
    ```

### 4. MCP Integration Guidelines

*   **Command Structure:** The tests should be executable via a standard Playwright MCP command.
    ```bash
    # Example command to run a specific test file
    mcp playwright test -- src/app/app-entry-input-dialog/dialog.spec.ts
    ```
*   **Environment:** The tests should not depend on any specific `.env` variables other than a potential `BASE_URL` which Playwright can be configured to use. Since we are mocking all API calls, no backend-specific variables are needed.

### 5. CI-Ready Code Requirements

*   **Test Organization:**
    *   Create a new test file named `dialog.spec.ts` inside the `src/app/app-entry-input-dialog/` directory.
    *   Use a top-level `describe('AppEntryInputDialog', () => { ... });` block to group all tests for this component.
*   **Naming Conventions:**
    *   Test names should clearly describe the behavior being tested, e.g., `test('should display a loading state while fetching the app configuration')`.
*   **Code Structure:**
    *   Write clean, readable, and well-commented code.
    *   Each `test` block should be independent. Use `test.beforeEach` to perform common setup for a group of tests, such as mocking the route or navigating to a page, but ensure mocks are specific to the test case if they differ.
*   **Parallelization:** Ensure tests are atomic and do not share state. Mocking API responses within each test using `page.route` naturally supports this, allowing tests to run in parallel without interference.


Copy link
Member

@shige shige left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 👍

@toyamarinyon toyamarinyon merged commit 13c15c4 into main Dec 10, 2025
12 checks passed
@toyamarinyon toyamarinyon deleted the toyamarinyon/20251210-120205 branch December 10, 2025 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants