diff --git a/.agents/skills/frontend-code-review/SKILL.md b/.agents/skills/frontend-code-review/SKILL.md index 9cb33bfe240..fd445790c97 100644 --- a/.agents/skills/frontend-code-review/SKILL.md +++ b/.agents/skills/frontend-code-review/SKILL.md @@ -5,7 +5,7 @@ description: Use only when the user explicitly requests a bug, regression, corre # Frontend Code Review -Review the requested scope for concrete, reproducible regressions. This skill owns the review phase and routes directly to its bundled rule packs. For a combined review-and-fix request, establish findings before applying implementation or testing guidance. +Review the requested scope for concrete defects and violations of explicit project contracts. This skill owns review decisions; its references route to canonical rules without activating another skill's implementation workflow. ## Evidence First @@ -13,7 +13,7 @@ Review the requested scope for concrete, reproducible regressions. This skill ow 2. Read the changed lines, their behavior owner, and the nearest scoped `AGENTS.md`. 3. For a named component or a diff that changes component ownership, props, or state, read the component-architecture rule pack and complete its Root-To-Leaf State And Props Audit before drawing conclusions. 4. Trace other public consumers, generated contracts, primitive APIs, or runtime configuration only when they decide correctness. -5. Report only findings tied to an observable failure, violated contract, security boundary, or demonstrated maintenance risk. +5. Report findings tied to an observable failure, violated contract, security boundary, or demonstrated maintenance risk. Explicit team conventions are contracts: establish their scope and exceptions, and do not invent user impact to justify a convention finding. ## Rule Routing @@ -34,10 +34,10 @@ Read `packages/dify-ui/README.md`, `packages/dify-ui/AGENTS.md`, `packages/dify- - **P0**: security or privacy leak, data loss, production crash, or inaccessible critical workflow. - **P1**: user-visible regression, invalid API or authorization contract, hydration failure, or broken primary interaction. -- **P2**: concrete maintainability, performance, test, or accessibility defect likely to cause incorrect behavior. +- **P2**: concrete maintainability, performance, test, or accessibility defect, or a material violation of an explicit project contract. - **P3**: minor actionable cleanup; omit unless the user requested a thorough audit. -Lead with findings ordered by severity. Include a tight file and line reference, the failing contract or reproduction path, impact, and a concrete fix direction. If there are no findings, say `No issues found.` and state any material verification gap. Do not add praise sections, speculative risks, or an unsolicited offer to implement fixes. +Lead with findings ordered by severity. Include a tight file and line reference, the observed failure or applicable project rule, and a concrete fix direction. Explain the rule's applicability for convention findings; describe downstream consequences only when supported by evidence. When no findings remain, say so briefly and state any material verification gap. Do not add praise sections, speculative risks, or an unsolicited offer to implement fixes. [accessibility]: references/accessibility-ui.md [code-quality]: references/code-quality.md diff --git a/.agents/skills/frontend-code-review/references/accessibility-ui.md b/.agents/skills/frontend-code-review/references/accessibility-ui.md index f7bb6d6918f..e8a841502e0 100644 --- a/.agents/skills/frontend-code-review/references/accessibility-ui.md +++ b/.agents/skills/frontend-code-review/references/accessibility-ui.md @@ -4,19 +4,15 @@ Accessibility findings are first-class review findings. Treat broken keyboard ac ## Review Evidence -Before finalizing UI or accessibility findings, fetch the latest Web Interface Guidelines as a required baseline: - -```text -https://raw.githubusercontent.com/vercel-labs/web-interface-guidelines/main/command.md -``` - -Do not treat that document as the complete accessibility rule set. Combine it with: +Use the sources relevant to the changed contract: - `packages/dify-ui/README.md`, `packages/dify-ui/AGENTS.md`, and the relevant primitive implementation when code uses `@langgenius/dify-ui/*`. - Base UI docs and local `.d.ts` contracts when primitive semantics, focus target, labels, or popup reachability are unclear. - MDN or relevant WAI-ARIA/browser standards when behavior, compatibility, or deprecation status matters. - The current feature's product semantics, because an accessible primitive can still be used in an inaccessible workflow. +Consult current official documentation or standards when these sources leave a behavior unresolved. The [Web Interface Guidelines] are an optional broader UI reference, not a required fetch for each review. + ## Semantic HTML Flag: @@ -83,7 +79,7 @@ Flag: state ownership and obscures whether `disabled` expresses independent unavailability. - Spinner or decorative loading icon exposed to screen readers. - Disabled controls that hide the reason users cannot proceed. -- `aria-disabled` used without manually blocking click, Space, and Enter. +- Controls marked `aria-disabled` that still activate through supported pointer or keyboard input. Check the primitive's handling before requesting manual event guards. - Toasts, inline validation, or async status changes that are not announced when users need the update to continue. - Icon-only loading/error affordances without text or accessible status where the state matters. @@ -108,7 +104,7 @@ Use Popover for explanatory content, rich help, and infotips. Use Tooltip only a Flag: - Text in flex/grid children without `min-w-0` when it can overflow. -- Names, labels, file names, model names, workspace names, or user content lacking `truncate`, `line-clamp`, or `break-words`. +- Long names, labels, or user content that overflow, obscure adjacent controls, or become unreadable in supported layouts. - Right-side icons, badges, checks, or actions that shrink before the text area. - Empty arrays or empty strings rendering broken layout instead of an empty state. - Button, tab, badge, chip, menu item, or card text that can overlap sibling controls at common viewport widths. @@ -127,3 +123,4 @@ Flag: - Hardcoded dates, times, numbers, or currency formats instead of `Intl.*`. [Accessible names and descriptions]: ../../../../packages/dify-ui/docs/accessible-names-and-descriptions.md +[Web Interface Guidelines]: https://raw.githubusercontent.com/vercel-labs/web-interface-guidelines/main/command.md diff --git a/.agents/skills/frontend-code-review/references/code-quality.md b/.agents/skills/frontend-code-review/references/code-quality.md index 9e014a93229..8ecf4b7b122 100644 --- a/.agents/skills/frontend-code-review/references/code-quality.md +++ b/.agents/skills/frontend-code-review/references/code-quality.md @@ -33,7 +33,7 @@ Flag: - Generic color utilities where Dify semantic tokens exist. - Hardcoded magic class values for colors, spacing, radius, shadow, z-index, or typography when Dify tokens, component variants, or documented radius mappings exist. - `!` important modifiers or important CSS overrides without a narrow, documented reason. -- Manual string concatenation, template strings, array `.join(' ')`, or custom ternaries for conditional or multi-line classes. +- Manual class-list assembly through string concatenation, template strings, array `.join(' ')`, or a custom conditional combiner instead of `cn(...)`. Conditional values passed to `cn(...)` remain valid. - JS conditional class branches for primitive visual states already exposed by Dify UI/Base UI `data-*` selectors. - Incoming `className` placed before default classes in `cn(...)`, preventing call-site overrides. - Arbitrary z-index or one-off layering fixes on overlays. diff --git a/.agents/skills/frontend-code-review/references/component-architecture.md b/.agents/skills/frontend-code-review/references/component-architecture.md index 3ece06099d9..752450f406e 100644 --- a/.agents/skills/frontend-code-review/references/component-architecture.md +++ b/.agents/skills/frontend-code-review/references/component-architecture.md @@ -1,6 +1,13 @@ -# Component Architecture Rules +# Component Architecture Review -Use these rules for React component structure, ownership, state, props, effects, and module organization. +Use the canonical reference for the changed concern. These links share rules; they do not activate the implementation skill or its workflow. + +| Concern | Canonical rules | +| --- | --- | +| Vertical modules, public entrypoints, data/handler placement, wrappers, Props, and types | [Ownership] | +| Local/Jotai state, form drafts, route identity, URL state, and persistence | [State] | +| Effects, navigation, memoization, and subscriptions | [Runtime] | +| Hotkeys, focus, and secondary surfaces | [Interactions] | ## Root-To-Leaf State And Props Audit @@ -15,102 +22,24 @@ For a file-focused component review, use the named component as the root. For a Review the complete paths before concluding that a state or prop is necessary. When several edges share one ownership defect, report one finding at the highest incorrect owner and include a representative path such as `Root -> Section -> Leaf`; do not emit a duplicate finding for every child. -## Ownership -Flag: +## Apply Rules In Their Actual Scope -- State, query, mutation, or handlers hoisted above the lowest component that actually uses them. -- Parent components owning row/item actions that do not coordinate a workflow. -- Prop drilling through multiple pass-through layers. -- A page/tab-level section component becoming the data owner without needing a shared snapshot or shared loading/error/empty UI. -- Feature code promoted to shared only because it appears once or might be reused later. +Explicit team conventions are reviewable contracts, including module organization and public API boundaries. Check the documented exception before reporting a violation. Do not infer an exception solely because the code appears to work, or invent a user-facing failure for a convention finding. -Accept repeated TanStack Query hooks for the same key and input in Client Component siblings under one QueryClient; -shared cache is not a reason to hoist. Separate Server Component QueryClients do not share it, so request-level -deduplication needs an identified request-local cache or verified framework or transport owner. +- For owner placement, trace the consumers and required lifetime. Establish whether the parent coordinates a snapshot, submission, navigation, shared UI, or persistence before asking to move state or handlers. +- For component boundaries, identify the ownership or encapsulation the proposed extraction would improve; file length alone establishes neither. +- For props and types, check the domain contract and public API. Do not report private props typing style alone; declaration/export syntax matters only for a documented package rule or concrete type, export, or framework defect. +- For state and Effects, trace the source of truth, external synchronization target, and mount/reset boundary. Controlledness alone does not prove that a draft is lifted or persisted; follow the form and overlay contracts linked by [State]. +- For navigation, distinguish ordinary links from mutation success, guarded redirects, command flows, and submission side effects. -## Component Boundaries +## Preserve Existing Product Contracts -Flag: +During refactors, trace the interaction being moved through its real consumer. Navigation, sidebar, dropdown, webapp-list, and app-switching changes must preserve expansion controls, hover persistence, pin/delete actions, routing, keyboard/focus handling, and open-state ownership where present. -- React component files over 300 lines when the file mixes multiple responsibilities that can be split into focused colocated components, hooks, or utilities. -- Shallow wrappers that only rename props or hide the real primitive. -- Extra DOM wrappers that do not provide layout, semantics, accessibility, state ownership, or library integration. -- Dialog/dropdown/popover hidden surfaces that obscure the parent flow when they should be extracted into a small local component. -- Business forms, menu bodies, or one-off helpers moved away from their owner without reuse or semantic value. +Check that the changed owner still handles reachable empty, loading, and missing optional-data states, and that primitive wrappers preserve accessible semantics and the public controlled-state contract. Report the actual lost behavior or explicit rule violation; use the package testing policy when assessing regression coverage. -Prefer colocated components split by actual data and state needs. - -## Bad Component Design Patterns - -Flag: - -- Refactors of existing navigation, sidebar, dropdown, webapp list, or app-switching UI that do not preserve behavior-sensitive interactions such as expand/collapse arrows, hover persistence, pin/delete controls, routing, keyboard/focus handling, or open-state ownership. -- Components that mix data fetching, mutation side effects, popup state, form validation, layout, and row rendering without a clear owner. -- Generic components with many boolean props that encode one feature's workflow. -- A shared component that imports feature-specific copy, routes, or API contracts. -- A feature component that accepts pre-rendered fragments only to avoid placing ownership correctly. -- A child component that receives both raw server data and separately derived flags for the same concept. -- A wrapper that changes accessible semantics of the primitive it wraps. -- A component that exposes controlled props but still keeps a competing private state for the same value. -- A component that cannot render empty, loading, or missing optional API fields without caller-side preprocessing. - -When existing components already own interaction logic, prefer reusing or extending them. If a refactor is necessary, preserve the old interaction contract and add or update focused tests for changed behavior. - -## Props And Types - -Flag: - -- Declaration or export rewrites made only for stylistic uniformity, without changing an owned behavior or contract. -- Named `Props` types for trivial one-off props where inline typing is clearer. -- Props named by UI implementation instead of domain/API role. -- API data converted too early or under a generic name that breaks traceability. -- Callers duplicating fallback checks that the lowest rendering component already handles. - -Do not flag `FC`, `React.FC`, function declarations, arrow functions, named exports, or default exports by syntax alone. Report them only when the chosen form causes a concrete type, lifecycle, export, framework, or enforced package-contract defect. - -## Effects - -Flag effects that: - -- Transform props/state for rendering. -- Copy one state value into another representing the same concept. -- Handle user actions that belong in event handlers. -- Reset local state from props or visibility when derivation, a stable semantic identity, or the intended mounted owner already expresses the lifecycle. -- Fetch data that belongs in framework APIs or TanStack Query. - -If an effect remains, it must synchronize with a named external system: browser API, subscription, timer, analytics-on-visibility, non-React widget, or imperative DOM integration. - -## State Modeling - -Flag: - -- Storing derived booleans, disabled flags, default tabs, or loading labels that can be calculated from current query/feature state. -- Per-session state held by a longer-lived visibility coordinator and cleared through an open-state Effect or a generated key when the primitive's mounted-content lifecycle already matches the intended state lifetime. -- A DOM field mirrored into competing prop, default, and React state sources when editing does not require those sources to synchronize. -- Local state used to fake server data or generated contract fields. -- UI state persisted to localStorage when it is live app state. -- Feature-local mock shells wired to unrelated existing APIs before the real API is confirmed. - -Review state lifetime before its storage mechanism. For a hidden surface, distinguish the -visibility coordinator from mounted content. State private to one mounted session belongs in that -content owner; promote it only when the draft must survive that content owner's unmount or another -owner coordinates it. A stable semantic identity key may create a new snapshot when the represented -identity changes; a generated key is not a routine reset command. - -Prefer render-time derivation. Keep true local state for user choices, transient input, controlled -popups, and feature UI state that has no server source. Submit-only DOM fields may remain -uncontrolled; use local controlled state when React must own the current value to drive rendering -or coordination. Observing change events or tracking a derived fact such as dirty state does not -require mirroring the field value. Do not flag controlled state by itself without a concrete -competing-source, stale-state, or ownership defect. - -## Navigation - -Flag: - -- Imperative router navigation for ordinary links. -- Button semantics used for navigation. -- Navigation state hidden in component state when URL state is required for shareable filters, tabs, or pagination. - -Use `Link` for normal navigation. Use router APIs for mutation success, guarded redirects, command flows, or form submission side effects. +[Interactions]: ../../how-to-write-component/references/interactions.md +[Ownership]: ../../how-to-write-component/references/ownership.md +[Runtime]: ../../how-to-write-component/references/runtime.md +[State]: ../../how-to-write-component/references/state.md diff --git a/.agents/skills/frontend-code-review/references/data-query-contracts.md b/.agents/skills/frontend-code-review/references/data-query-contracts.md index c68d7e0d1c4..1dc9edc3e0b 100644 --- a/.agents/skills/frontend-code-review/references/data-query-contracts.md +++ b/.agents/skills/frontend-code-review/references/data-query-contracts.md @@ -1,92 +1,28 @@ -# Data, Query, And Contract Rules +# Data, Query, And Contract Review -Use these rules for generated contracts, TanStack Query, mutations, auth/SSR boundaries, URL state, and client persistence. +[Data and queries] owns generated-client, Query options, mutation/cache, imperative access, SSR, authentication, and tenant rules. [State ownership] owns URL state and persistence. Read only the reference needed by the diff; reading a shared reference does not activate its implementation skill. -## Generated Contracts +## Generated Contracts And Query Conventions -Flag: +Review explicit team conventions as contracts, including direct generated options, `skipToken` for missing required input, and shared cache policy. A lint rule may enforce part of a convention; neither a green lint result nor a suppression proves the full contract is satisfied. -- New legacy service/helper wrappers around generated `queryOptions()` or `mutationOptions()`. -- Continuing to use deprecated contract operations when a ready generated contract exists. -- Assuming a generated file means an operation is ready without checking deprecated markers, schema shape, and the actual UI consumer. -- Re-declaring API DTOs in components. -- Adding compatibility layers instead of migrating the pointed line and deleting the old layer. +- Establish whether the changed call belongs to a new or migrated surface and whether the generated operation is ready. Check deprecated markers, schema shape, and the real UI consumer before prescribing a migration. +- Distinguish a pass-through wrapper from a feature hook with actual orchestration. Check whether an independent execution condition or Promise composition justifies the documented query/mutation exception. +- Trace generated input and output types through their boundaries. Identify the exact DTO mirror, field widening, placeholder input, or lost intentional empty value when reporting a violation. +- Check whether a local mutation callback owns feature feedback or replaces shared invalidation, retry, or cache defaults. Match optimistic changes to the current list/detail owner. -Backend Pydantic and OpenAPI schemas own API shape. Generated clients and schemas under `packages/contracts/generated/*` are authoritative at frontend boundaries and use the `{ params, query?, body? }` input shape. +## Imperative Access And SSR -## Queries +- Check freshness, projection, retries, and the caller's execution condition separately against [Data and queries]. Trace Promise ownership to distinguish an awaited hard gate from soft prefetching with an explicit failure owner. +- For Server Components, identify who renders the data and who may revalidate it. Check dehydration, the same-key client consumer, error handling, and the intended Suspense/server-rendered-content contract when the diff changes streaming. +- For auth, setup, roles, branding, or availability, trace authoritative data and the loading/fallback path. A static redirect or placeholder value cannot stand in for a request-dependent decision. -Flag: +## Tenant, URL, And Persistence Boundaries -- `enabled` used to hide missing required input instead of `input: skipToken`. -- Fake fallback IDs or placeholder inputs used to force a query to run. -- Query results copied into local state for rendering. -- Shared query behavior such as invalidation, stale defaults, or retry rules reimplemented at call sites. -- Deprecated imperative reads such as `fetchQuery`, `prefetchQuery`, `ensureQueryData`, or their infinite variants when - the current `query` or `infiniteQuery` contract applies. +- Trace the current workspace-switch flow and cache lifetime before reporting missing identity in a query key. Verify backend meaning before treating `workspace_id` and `tenant_id` as interchangeable. +- For URL and storage changes, identify whether the value is shareable navigation state, live app state, a one-shot signal, or a low-frequency preference. Apply [State ownership] to that category and verify its write/reset owner. -Use `useQuery(consoleQuery.xxx.queryOptions(...))` or `useQuery(marketplaceQuery.xxx.queryOptions(...))` directly unless a feature hook performs real orchestration. +Report the violated rule and applicable scope or the concrete failing path. Do not invent runtime impact when the finding is a team-convention violation. -For imperative access, treat the choices as independent dimensions: - -- `query` or `infiniteQuery` resolves the generated query and returns its data. -- `staleTime` decides whether cached data satisfies this call: `0` treats it as stale, a finite value accepts a freshness - window, `Infinity` accepts it until invalidation, and `'static'` accepts available data even after invalidation. -- `select` projects the resolved value without replacing cached query-function data. An imperative query defaults to no - retries when `retry` is not configured; `enabled` is observer-only, so guard before a conditional call. -- `await` blocks the current flow, `return` transfers the Promise to the caller, and `void` discards the result without - handling rejection. Handle rejection before discarding a potentially rejecting Promise; use `.catch(noop)` only for - intentional silence or feedback owned elsewhere, and preserve rejection for hard gates. - -## Mutations - -Flag: - -- Deprecated `useInvalid` or `useReset`. -- `mutateAsync` used without a need for Promise semantics. -- Awaited mutations without `try/catch`. -- Components owning shared cache invalidation that belongs in query defaults. -- Optimistic updates that do not match current list/detail ownership. - -Use generated `mutationOptions()` directly when possible. Put shared cache behavior in `createTanstackQueryUtils(...experimental_defaults...)`. - -## SSR, Auth, And Route Boundaries - -Flag: - -- Request-time auth, setup, workspace role, or tenant decisions moved into static `next.config redirects()`. -- Dynamic role gates depending on `workspaces.current` implemented as static path redirects. -- Authorization logic depending on an imperative query whose rejection is swallowed. -- Removing a client fallback before server API unavailable behavior is defined. -- Global placeholder query contracts introduced to solve a route-local Suspense issue. -- Branding-sensitive UI reading placeholder defaults without checking pending/placeholder state. -- A Server Component rendering or passing an imperative query result that the browser can independently revalidate, - leaving server and client output with different owners. -- A non-blocking Server Component query without pending-query dehydration, Next-compatible error redaction, a - `HydrationBoundary` covering the same-key client consumer, or an explicit Suspense and SSR-content decision. - -Hard gates await `query` or `infiniteQuery` and preserve rejection; soft prefetches handle failure at the fallback owner. -Treat Server Components as prefetch-and-dehydrate owners by default, rendering returned data only under exclusive server -ownership or a freshness contract that prevents server/client drift. - -## Workspace And Tenant - -Flag: - -- Treating workspace switch as ordinary CRUD invalidation when the current app flow performs server switch plus full reload. -- Query keys that omit workspace/tenant identity when the query truly varies by workspace and no full reload boundary applies. -- Mixing `workspace_id` and `tenant_id` without tracing the current backend/API contract. - -Current Dify workspace switch should be reviewed as a tenant cache boundary first. - -## URL State And Local Storage - -Flag: - -- Shareable filters, tabs, pagination, selected panels, or search state hidden only in component state. -- One-shot navigation signals modeled as subscribed persistent state. -- Live app state stored in localStorage. -- Direct `window.localStorage`, `globalThis.localStorage`, or raw storage calls in app code. -- High-frequency interaction state persisted on every change instead of on commit/settle. - -Use URL state for shareable UI state, feature/Jotai/store state for live UI state, and `@/hooks/use-local-storage` only for low-frequency client-only preferences, dismissed notices, and UI defaults. +[Data and queries]: ../../how-to-write-component/references/data.md +[State ownership]: ../../how-to-write-component/references/state.md diff --git a/.agents/skills/frontend-code-review/references/performance.md b/.agents/skills/frontend-code-review/references/performance.md index 3bd6e25867a..1e9aaf8ce5d 100644 --- a/.agents/skills/frontend-code-review/references/performance.md +++ b/.agents/skills/frontend-code-review/references/performance.md @@ -71,8 +71,6 @@ Flag: ## React Flow -For workflow React Flow components, keep this Dify-specific rule: +Use [Dify invariants] for React Flow node/edge consumption and provider availability in RAG Pipe template rendering. Callback-only reads or mutations can use `useStoreApi`. -- UI consumption should use React Flow hooks such as `useNodes` / `useEdges`. -- Callback-only reads or mutations can use `useStoreApi`. -- Node components under `web/app/components/workflow/nodes/[nodeName]/node.tsx` must not depend on workflow stores that are absent in RAG Pipe template rendering. +[Dify invariants]: dify-invariants.md diff --git a/.agents/skills/frontend-code-review/references/testing.md b/.agents/skills/frontend-code-review/references/testing.md index 3bda124c01b..93b349b21bb 100644 --- a/.agents/skills/frontend-code-review/references/testing.md +++ b/.agents/skills/frontend-code-review/references/testing.md @@ -1,6 +1,6 @@ # Testing Review Rules -`web/docs/test.md` is the canonical frontend testing policy. Use this file only to translate that policy into review findings. +Use `web/docs/test.md` for tests owned by `web/` and `packages/dify-ui/docs/testing.md` for tests owned by Dify UI. These owners define their test boundaries, environments, and checks; this reference only adds review questions. ## Request Missing Tests When Risk Justifies Them @@ -30,8 +30,7 @@ Flag tests that: - Prefer semantic queries and accessible names. - Prefer real feature components when integration semantics matter. -- Allow intentional child or provider mocks when setup would dominate the test and that boundary is covered independently. -- Do not accept semantically inaccurate mocks of Dify UI or legacy base primitives. +- Check mocks against the owning package's policy; allowed mocks must preserve the public contract and leave the behavior under review real. - Require a real-browser or visual verification plan when `happy-dom` cannot represent the risk. Treat test quality, determinism, and regression value as the review criteria. Do not use test count or coverage percentage as a proxy for quality. diff --git a/.agents/skills/frontend-testing/SKILL.md b/.agents/skills/frontend-testing/SKILL.md index a09aa940b1d..3e1ca6833cc 100644 --- a/.agents/skills/frontend-testing/SKILL.md +++ b/.agents/skills/frontend-testing/SKILL.md @@ -5,12 +5,9 @@ description: Use when writing or changing Vitest or React Testing Library tests # Frontend Testing -`web/docs/test.md` is the single policy owner. Read it before changing frontend tests; this skill adds no separate requirements. +Read the testing policy for the package that owns the changed tests: -1. Identify the observable contract and regression risk. -2. Choose the smallest boundary that includes the behavior owner. -3. Establish the failing case first when practical, then implement one coherent scenario. -4. Run the focused spec before the affected suite and relevant static checks. -5. Report the behavior verified and any remaining browser, visual, or end-to-end risk. +- `web/`: `web/docs/test.md` owns Web test boundaries, environments, and commands. +- `packages/dify-ui/`: `packages/dify-ui/docs/testing.md` owns primitive test boundaries, environments, Storybook, and commands. -Recommend deleting low-value tests as readily as adding missing behavior coverage. Use `web/docs/test.md` for policy and Web commands; use `packages/dify-ui/docs/testing.md` for Dify UI commands. +This skill adds no parallel policy or check sequence. Follow the selected owner's requirements, including its validation commands. Recommend deleting low-value tests as readily as adding missing behavior coverage, and report the contract verified and any material verification gap. diff --git a/.agents/skills/how-to-write-component/SKILL.md b/.agents/skills/how-to-write-component/SKILL.md index 1f19a28f108..3526e8ca426 100644 --- a/.agents/skills/how-to-write-component/SKILL.md +++ b/.agents/skills/how-to-write-component/SKILL.md @@ -37,7 +37,7 @@ Choose the mode from the user's requested outcome: | Is a wrapper needed? | Use the primitive or direct code. | The wrapper owns behavior, validation, state, semantics, or necessary integration. | | Is an Effect needed? | Derive during render or handle the user action. | A named external system must be synchronized. | -## Reference Routing +## Topic Routing After selecting the operating mode, read only the topic references required by the change: diff --git a/.agents/skills/how-to-write-component/references/data.md b/.agents/skills/how-to-write-component/references/data.md index 7307f25a6ba..4a014abe5d5 100644 --- a/.agents/skills/how-to-write-component/references/data.md +++ b/.agents/skills/how-to-write-component/references/data.md @@ -8,9 +8,10 @@ Read this document when a component consumes generated contracts, nullable API v - Backend Pydantic and OpenAPI schemas own API shape. Follow the generated `{ params, query?, body? }` input shape; when it is wrong, fix the backend schema and regenerate `packages/contracts/generated/*`. - Do not hand-write DTO mirrors, widen generated fields or enums, edit generated output, or add a parallel frontend status layer unless it models product state absent from the API. - Check deprecated markers, schema shape, and the actual consumer before assuming that a generated operation is ready to use. +- When a ready generated operation exists for the changed call, migrate deprecated operations and remove the replaced layer instead of adding compatibility wrappers. - Normalize only at real boundaries such as user input, search, URL params, filenames, DOM IDs, or a required legacy adapter. - Preserve `null`, `undefined`, and intentional empty strings until the final boundary. Do not use `value || undefined` when an empty string means clearing a field. -- Build required values in the branch that proves them. Avoid `filter(Boolean)`, truthiness filters, non-null assertions after filters, and placeholder values used only to satisfy types. +- Build required values in the branch that proves them. Do not use truthiness filters, non-null assertions, or placeholders to conceal missing required input or discard valid `0`, `false`, or empty-string values. ## Queries @@ -31,6 +32,7 @@ Read this document when a component consumes generated contracts, nullable API v - Use generated `mutationOptions()` directly for owner-local mutations. - Keep a mutation in `useMutation` when pending and error state belong to one dialog or form. Use a mutation atom only when shared workflow orchestration or graph-derived state consumes it; query and mutation atoms remain unscoped. +- Do not introduce deprecated `useInvalid` or `useReset` APIs. - Put shared invalidation, retries, and cache behavior in `createTanstackQueryUtils(...experimental_defaults...)`. Local callbacks may own toast, close, and navigation effects but must not replace shared cache policy. - Workflow-specific invalidation belongs beside the mutation/command that understands which caches become stale. Construct generated query options or keys from graph identity there instead of receiving keys from a rendering parent. - Prefer `mutate(...)`. Use `mutateAsync(...)` only when Promise composition is required, and catch awaited failures. @@ -42,6 +44,7 @@ Read this document when a component consumes generated contracts, nullable API v - Use `query` or `infiniteQuery` for imperative access. `staleTime` defines cache acceptance; `select` projects the return value without replacing cached query-function data. Imperative queries default to no retries when `retry` is not configured, and observer-only `enabled` does not prevent an imperative call. +- Migrate deprecated `fetchQuery`, `prefetchQuery`, `ensureQueryData`, and their infinite variants when the current `query` or `infiniteQuery` contract applies. A `staleTime` of `0` treats data as stale; a finite value accepts that freshness window; `Infinity` accepts data until invalidation; `'static'` accepts available data even after invalidation. - `await` blocks, `return` transfers the Promise, and `void` discards its value without handling rejection. Handle rejection before discarding a potentially rejecting Promise; use `.catch(noop)` only for intentional silence or feedback owned elsewhere. Hard server gates await the query and preserve rejection. @@ -55,5 +58,7 @@ Read this document when a component consumes generated contracts, nullable API v - Non-blocking RSC streaming requires pending-query dehydration without redacting Next.js server errors, a `HydrationBoundary` around the same-key client consumer, and Suspense when that content must be server-rendered. - Never reuse tenant-scoped state after switching workspaces. Discard it at the switch boundary or isolate it by workspace identity. +- Trace the current switch flow before choosing cache handling: server switch plus full reload is a tenant boundary, not ordinary CRUD invalidation. Include workspace identity in varying query keys when no full-reload boundary applies, and trace the backend contract before interchanging `workspace_id` and `tenant_id`. - Do not make product or authorization decisions from bootstrap defaults. Wait for authoritative data, or render an explicit loading or error state. +- Preserve an existing client fallback until server API-unavailable behavior has an explicit owner. - Keep loading and Suspense behavior inside the feature that owns the request. Do not add fake global data merely to bypass that boundary. diff --git a/.agents/skills/how-to-write-component/references/interactions.md b/.agents/skills/how-to-write-component/references/interactions.md index 715a85e1f5b..822e4f44614 100644 --- a/.agents/skills/how-to-write-component/references/interactions.md +++ b/.agents/skills/how-to-write-component/references/interactions.md @@ -28,8 +28,10 @@ Read this document when a change involves application hotkeys, focus, dialogs, m - Keep overlay open-state ownership separate from content-session ownership. A controlled root does not require controlled fields or root-owned drafts. - Match transient state to the primitive's content mount lifecycle. State below an unmounting content boundary gets a fresh instance after unmount; intentionally kept-mounted content needs an explicit persistence or reset policy. - Keep a controlled overlay root at its coordination owner so the primitive can complete exit transitions, focus restoration, and detached-handle behavior. Do not conditionally remove the root to reset content state, and use keys only for stable semantic identity. +- Use the [overlay contract] to determine content lifetime and preserve the Root's closing lifecycle. Follow [state ownership] for draft placement and semantic identity; portal placement alone does not locate the state owner. - Place query subscriptions and mutation observers at the owner whose lifetime matches when they should run. Mounted-session work may belong inside content; work that must start or stop exactly with `open` needs an explicit open-state condition. - Prefer primitive-owned open state unless another owner must observe or coordinate it. Analytics callbacks and local cleanup alone do not require a controlled root. - Do not merge dialogs with different loading guards, reset policies, or lifetimes into one discriminated `currentDialog` state merely to reduce atom count. [overlay contract]: ../../../../packages/dify-ui/docs/overlays.md +[state ownership]: state.md diff --git a/.agents/skills/how-to-write-component/references/ownership.md b/.agents/skills/how-to-write-component/references/ownership.md index 9a60f60c2d3..70395382e93 100644 --- a/.agents/skills/how-to-write-component/references/ownership.md +++ b/.agents/skills/how-to-write-component/references/ownership.md @@ -13,12 +13,11 @@ Read this document when auditing, adding, moving, splitting, or refactoring Reac ## Component Ownership - Before declaring state, a query, or a workflow hook in a page or parent, identify the direct descendant branches that consume each returned value. -- If only one branch consumes a value, declare it in the lowest owner in that branch. The parent may pass stable identity or the smallest boundary input, but must not own child state merely to construct props. -- A parent may own a value when several sibling branches require one live snapshot and the parent genuinely derives or coordinates submission, selection, navigation, lifecycle, loading, or errors. If it only destructures, renames, and forwards fields, it is a switchboard rather than an owner; put the shared workflow in a feature-local state graph so each surface consumes only its named facts and commands. Use a provider only when it becomes the authoritative input, scope, or external-dependency owner. +- If only one branch consumes a value, declare it in the lowest owner in that branch whose mounted lifetime matches the required persistence. The parent may pass stable identity or the smallest boundary input, but must not own child state merely to construct props. +- A parent may own a value when it must intentionally survive the local owner's unmount, or when several sibling branches require one live snapshot and the parent genuinely derives or coordinates submission, selection, batch behavior, navigation, lifecycle, loading, or errors. If it only destructures, renames, and forwards fields, it is a switchboard rather than an owner; put the shared workflow in a feature-local state graph so each surface consumes only its named facts and commands. Use a provider only when it becomes the authoritative input, scope, or external-dependency owner. - A provider is a real boundary only when it establishes authoritative input, scope/isolation, or a stable external dependency. A provider that calls a large hook and repackages its return value into Context remains a switchboard. - A page or feature root may wire route identity, providers, layout, navigation, and genuine cross-surface coordination. It must not call a child-specific state or query hook merely to assemble that child's props. - Keep child contracts at the ownership boundary: stable domain identity, a small immutable snapshot, placement options, or named cross-boundary commands. Do not pass an internal state machine as separate `data`, `pending`, `error`, `retry`, `open`, setter, and callback props when the parent does not use them, and do not hide the same fan-out in a props bag or hook result object. -- Repeated TanStack Query calls in siblings are acceptable when each sibling independently consumes the data; the cache already deduplicates requests. - Repeated TanStack Query hooks for the same key and input in Client Component siblings under one QueryClient share that cache. Separate Server Component QueryClients do not, so request-level deduplication needs an identified request-local cache or verified framework or transport owner. - Treat parent input according to what the child boundary does with it: - If the child only renders or performs a light local decision, pass the snapshot as props. It does not become a new state owner. @@ -27,24 +26,28 @@ Read this document when auditing, adding, moving, splitting, or refactoring Reac - One pass-through layer is acceptable for stable identity and placement. It is not permission to relay workflow state and handlers through an unrelated component. - Route identity may pass once from a framework route into its feature boundary. If multiple descendants, queries, facts, or commands need it, bridge it into the feature graph and stop passing it as props. - A query snapshot may cross once as immutable display input. Query keys, observer methods, retry/loading/error groups, and invalidation mechanics belong to the query surface or feature graph and must not be decomposed into props. +- Revisit prop forwarding when intermediate components obscure the behavior owner; keep clear data flow rather than introducing shared state merely to avoid passing props. - Do not replace prop drilling with one large view-model hook. Move each query, derived value, and handler to the concrete owner that consumes it. - Keep source selection, defaults, validation, dirty checks, and payload shaping beside the workflow that owns submission. ## Boundaries +- Prefer reusing or extending the component that already owns an interaction over rebuilding that behavior in a parallel component. - State-heavy wizards, drawers, modals, and secondary workflows can form a small vertical surface with an entrypoint, optional feature-local state, and shallow owners matching real visual regions. - The entrypoint owns route integration, provider wiring, placement, and open-state coordination. A content or session owner keeps state scoped to that mounted surface. - Judge hook lifetime by the component that declares the hook and the primitive's mount contract, not only by where its rendered controls appear in JSX. - Separate hidden dialogs, dropdowns, and popovers into small local owners when their content obscures the parent flow. - Keep cohesive forms, menu bodies, and one-off helpers local unless they have their own state, reuse, or semantic boundary. +- Extract components or hooks when they clarify ownership or hide a cohesive implementation; keep logic local when extraction only shortens the file or relocates the same coordination. - Avoid wrapper components and wrapper DOM that only rename props, pass children through, or hide the real primitive. A wrapper must own behavior, validation, state, accessibility, layout, or library integration. +- Keep feature workflows out of shared components: do not encode one feature through many boolean props or import its copy, routes, and API contracts into a generic component. Do not pass pre-rendered fragments merely to avoid assigning the behavior to its owner. - Loading states for page sections, cards, lists, tables, forms, and drawers should use skeletons scoped to the loaded content. Reserve spinners for small inline busy indicators. ## Components And Types - Choose component declaration and export forms from the actual component contract, framework requirements, and enforced package rules. Existing style is context, not authority; do not rewrite unaffected code solely to normalize `FC`, `function`, arrow-function, named-export, or default-export forms. -- Type simple one-off props inline. Name a `Props` type when it is reused, exported, complex, or materially clearer. - Use API-generated or API-returned types at component boundaries. Keep one-off UI refinements and conversions beside their owner. +- Name props and converted data after their domain/API role, preserving traceability to the original contract. - Preserve domain value types for selections. Do not widen enums, unions, booleans, numbers, objects, or nullable values to `string` before a real boundary requires it. - Avoid generic `common.tsx` buckets and aliases that only rename another type. Name files, values, and public types after their domain role. - Put fallback and invariant checks in the lowest component that already renders that state. Do not extract helpers whose only purpose is hiding missing display data. diff --git a/.agents/skills/how-to-write-component/references/state.md b/.agents/skills/how-to-write-component/references/state.md index 24df669fd20..47243e29c2d 100644 --- a/.agents/skills/how-to-write-component/references/state.md +++ b/.agents/skills/how-to-write-component/references/state.md @@ -22,11 +22,12 @@ Keep values out of the graph when they only affect one component's presentation, ## Forms And Sessions -- Keep form state in the narrowest owner whose lifetime matches the draft. A draft scoped to one mounted surface belongs to that content or session owner; a draft that must survive its current owner's unmount belongs to an explicit longer-lived feature owner. -- Prefer uncontrolled fields when values are only read at submit time. Use local controlled state only when React must own the current value to drive dependent UI or linked fields; track derived facts such as dirty state without mirroring the field value. Controlledness does not decide whether a draft is local or persisted. +- Follow the [form contract] for field controlledness and the [overlay contract] for Root/content mounting and state lifetime. These primitive contracts also apply when reviewing a consumer. +- Choose the Web draft owner from the required lifetime. Keep mounted-session drafts in the content owner; start with the lowest shared React owner when another component needs the draft or it must survive unmounting. Use feature-scoped atoms only when their coordination or persistence contract is needed. - For query-backed defaults, establish the form session after the required defaults are available. `defaultValue` initializes the current mount; a stable semantic identity key may create a fresh snapshot when the represented identity changes. Do not use a generated key as a routine reset command. -- Promote drafts beyond the session only when another owner reacts to in-progress values, several workflow steps share one draft, or the draft must intentionally survive unmounting. Start with the lowest shared React owner; use feature-scoped atoms only when their coordination or persistence contract is needed. - Keep validation, source priority, fallback behavior, dirty checks, and payload assembly in the workflow that owns submission. +- Derive booleans, disabled flags, default tabs, and loading labels from current state. Do not mirror one value into competing prop, default, and local sources; controlled state alone is not a competing-source defect. +- Do not use local state to fake server data or generated contract fields, or connect a feature mock shell to an unrelated API before its actual contract is confirmed. ## Route And URL State @@ -36,7 +37,7 @@ Keep values out of the graph when they only affect one component's presentation, - A scoped workflow input is different: initialize it once, key the scope by semantic identity when switching entities should reset it, and do not force later parent refreshes into an in-progress session. Never extend the bridge exception to feature-owned primitives, drafts, query or mutation atoms, or edit-session snapshots. - Within one route-owned feature, choose one route-identity source. Do not hydrate route identity into atoms while also threading the same ID through multiple component layers. - A route parameter may cross the route-to-feature entry edge once. After a route bridge exists, queries, facts, commands, and descendant surfaces must read that bridge instead of accepting the same ID as props. -- Put shareable filters, tabs, pagination, and search state in the URL. Keep one-shot navigation signals and transient UI state out of persistent subscriptions. +- Put shareable filters, tabs, selected panels, pagination, and search state in the URL. Keep one-shot navigation signals and transient UI state out of persistent subscriptions. ## Build A Clean Jotai Graph @@ -82,3 +83,6 @@ Before implementation and again after the final slice, draw the actual graph in - Use feature-owned storage modules built on `createLocalStorageState`; callers should not scatter direct storage access or raw keys. - Persist high-frequency interaction state only on commit or after updates settle. - Do not add ad hoc global event listeners for shared state. Centralize subscriptions through the owning atom, store, or subscription hook. + +[form contract]: ../../../../packages/dify-ui/docs/forms.md +[overlay contract]: ../../../../packages/dify-ui/docs/overlays.md diff --git a/AGENTS.md b/AGENTS.md index d7b7320aa2d..35b1fa3301e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,15 +1,9 @@ # AGENTS.md -Dify is an open-source platform for building LLM applications, agentic workflows, and RAG pipelines. This monorepo contains the backend API (`api/`), frontend application (`web/`), deployment assets (`docker/`), standalone agent backend (`dify-agent/`), CLI (`cli/`), and end-to-end suite (`e2e/`). Follow the nearest scoped `AGENTS.md` for the files being changed. +Dify is an open-source platform for building LLM applications, agentic workflows, and RAG pipelines. This monorepo contains the backend API (`api/`), frontend application (`web/`), deployment assets (`docker/`), standalone agent backend (`dify-agent/`), CLI (`cli/`), and end-to-end suite (`e2e/`). Follow the nearest scoped `AGENTS.md` for the files being changed. Apply its guidance within the user's requested scope; explicit user instructions take precedence over workflow defaults. ## Repository Gotchas - Run backend commands through `uv run --project api `. - Backend integration tests are CI-only and are not expected to run locally. - Keep `docker/.env.example` limited to variables required for a default Docker Compose deployment to start. Put optional and provider-specific settings in the matching `docker/envs/*.env.example` file; `docker/.env` overrides those service-specific env files. - -## Frontend Workflow - -- For truncated text disclosure and native `title` decisions, follow [Truncated Text Disclosure]. - -[Truncated Text Disclosure]: web/docs/truncated-text-disclosure.md diff --git a/api/controllers/web/passport.py b/api/controllers/web/passport.py index 3e4a7960984..46482b9c53e 100644 --- a/api/controllers/web/passport.py +++ b/api/controllers/web/passport.py @@ -5,6 +5,7 @@ from werkzeug.exceptions import NotFound, Unauthorized from constants import HEADER_NAME_APP_CODE from controllers.common.schema import query_params_from_model, register_response_schema_models, register_schema_models +from controllers.console.wraps import model_validate from controllers.web import web_ns from controllers.web.error import WebAppAuthRequiredError from extensions.ext_application_services import application_services @@ -48,12 +49,12 @@ class PassportResource(Resource): } ) @web_ns.response(200, "Passport retrieved successfully", web_ns.models[PassportAccessTokenResponse.__name__]) - def get(self): + @model_validate(PassportQuery) + def get(self, query: PassportQuery): app_code = request.headers.get(HEADER_NAME_APP_CODE) if app_code is None: raise Unauthorized("X-App-Code header is missing.") - query = PassportQuery.model_validate(request.args.to_dict(flat=True)) passport_request = WebPassportRequest( app_code=app_code, user_session_id=query.user_id, diff --git a/api/models/model.py b/api/models/model.py index 050b63ee0b3..953d9112c8b 100644 --- a/api/models/model.py +++ b/api/models/model.py @@ -2124,17 +2124,15 @@ class AppAnnotationHitHistory(TypeBase): annotation_question: Mapped[str] = mapped_column(LongText, nullable=False) annotation_content: Mapped[str] = mapped_column(LongText, nullable=False) - @property - def account(self): - return db.session.scalar( + def account(self, session: Session) -> Account | None: + return session.scalar( select(Account) .join(MessageAnnotation, MessageAnnotation.account_id == Account.id) .where(MessageAnnotation.id == self.annotation_id) ) - @property - def annotation_create_account(self): - return db.session.scalar(select(Account).where(Account.id == self.account_id)) + def annotation_create_account(self, session: Session) -> Account | None: + return session.scalar(select(Account).where(Account.id == self.account_id)) class AppAnnotationSetting(TypeBase): diff --git a/api/models/tools.py b/api/models/tools.py index 67b4929da1b..ceb54207a58 100644 --- a/api/models/tools.py +++ b/api/models/tools.py @@ -9,7 +9,7 @@ from uuid import uuid4 import sqlalchemy as sa from deprecated import deprecated from sqlalchemy import ForeignKey, String, func, select -from sqlalchemy.orm import Mapped, mapped_column +from sqlalchemy.orm import Mapped, Session, mapped_column from core.plugin.entities.plugin_daemon import CredentialType from core.tools.entities.common_entities import I18nObject @@ -203,9 +203,8 @@ class ApiToolProvider(TypeBase): return None return db.session.scalar(select(Account).where(Account.id == self.user_id)) - @property - def tenant(self) -> Tenant | None: - return db.session.scalar(select(Tenant).where(Tenant.id == self.tenant_id)) + def tenant(self, session: Session) -> Tenant | None: + return session.scalar(select(Tenant).where(Tenant.id == self.tenant_id)) class ToolLabelBinding(TypeBase): @@ -277,13 +276,11 @@ class WorkflowToolProvider(TypeBase): init=False, ) - @property - def user(self) -> Account | None: - return db.session.scalar(select(Account).where(Account.id == self.user_id)) + def user(self, session: Session) -> Account | None: + return session.scalar(select(Account).where(Account.id == self.user_id)) - @property - def tenant(self) -> Tenant | None: - return db.session.scalar(select(Tenant).where(Tenant.id == self.tenant_id)) + def tenant(self, session: Session) -> Tenant | None: + return session.scalar(select(Tenant).where(Tenant.id == self.tenant_id)) @property def parameter_configurations(self) -> list[WorkflowToolParameterConfiguration]: @@ -292,9 +289,8 @@ class WorkflowToolProvider(TypeBase): for config in json.loads(self.parameter_configuration) ] - @property - def app(self) -> App | None: - return db.session.scalar(select(App).where(App.id == self.app_id)) + def app(self, session: Session) -> App | None: + return session.scalar(select(App).where(App.id == self.app_id)) class MCPToolProvider(TypeBase): diff --git a/api/tests/unit_tests/controllers/web/test_web_passport.py b/api/tests/unit_tests/controllers/web/test_web_passport.py index 9ac0b731a3a..fd6cd80d062 100644 --- a/api/tests/unit_tests/controllers/web/test_web_passport.py +++ b/api/tests/unit_tests/controllers/web/test_web_passport.py @@ -1,6 +1,5 @@ """Unit tests for the thin web-passport Flask adapter.""" -from inspect import unwrap from types import SimpleNamespace from unittest.mock import MagicMock, patch @@ -31,7 +30,7 @@ def test_passport_resource_parses_input_and_serializes_result(app: Flask) -> Non patch("controllers.web.passport.application_services", return_value=services), patch("controllers.web.passport.extract_webapp_access_token", return_value="login-token"), ): - result = unwrap(PassportResource.get)(PassportResource()) + result = PassportResource().get() assert result == {"access_token": "issued-token"} service.issue.assert_called_once_with( @@ -41,7 +40,7 @@ def test_passport_resource_parses_input_and_serializes_result(app: Flask) -> Non def test_passport_resource_requires_app_code(app: Flask) -> None: with app.test_request_context("/passport"), pytest.raises(Unauthorized, match="X-App-Code"): - unwrap(PassportResource.get)(PassportResource()) + PassportResource().get() @pytest.mark.parametrize( @@ -66,4 +65,4 @@ def test_passport_resource_translates_application_errors( patch("controllers.web.passport.application_services", return_value=services), pytest.raises(http_error), ): - unwrap(PassportResource.get)(PassportResource()) + PassportResource().get() diff --git a/api/tests/unit_tests/models/test_annotation_hit_history_session_accessors.py b/api/tests/unit_tests/models/test_annotation_hit_history_session_accessors.py new file mode 100644 index 00000000000..d4bb5c624e5 --- /dev/null +++ b/api/tests/unit_tests/models/test_annotation_hit_history_session_accessors.py @@ -0,0 +1,93 @@ +"""Regression coverage for the ``@property``→session-parameter refactor on +``AppAnnotationHitHistory`` accessors. + +Covers two of `api/models/model.py`'s legacy `@property` accessors that reached for the global +``db.session`` internally and have been converted to plain methods taking an explicit +``session: Session`` (per the pattern established in #40370/#40797/#41394/#41830, tracked in +#40372): + +- ``AppAnnotationHitHistory.account`` (joins through ``MessageAnnotation.account_id``) +- ``AppAnnotationHitHistory.annotation_create_account`` (looks up ``self.account_id`` directly) + +Each accessor is exercised against the real ``sqlite_session`` fixture (a genuine SQLAlchemy +``Session`` bound to a pristine full-schema SQLite database) so the assertions cover actual +query behaviour rather than a mock's recorded call. +""" + +from uuid import uuid4 + +from sqlalchemy.orm import Session + +from models.account import Account +from models.model import AppAnnotationHitHistory, MessageAnnotation + + +def _persist_account(session: Session) -> Account: + account = Account(name="Test Account", email="test@example.com") + session.add(account) + session.flush() + return account + + +def _persist_annotation(session: Session, *, app_id: str, account_id: str) -> MessageAnnotation: + annotation = MessageAnnotation( + app_id=app_id, + conversation_id=None, + message_id=None, + question="What is AI?", + content="AI stands for Artificial Intelligence.", + account_id=account_id, + ) + session.add(annotation) + session.flush() + return annotation + + +def _hit_history(*, app_id: str, annotation_id: str, account_id: str) -> AppAnnotationHitHistory: + return AppAnnotationHitHistory( + app_id=app_id, + annotation_id=annotation_id, + source="api", + question="What is AI?", + account_id=account_id, + score=0.95, + message_id=str(uuid4()), + annotation_question="What is AI?", + annotation_content="AI stands for Artificial Intelligence.", + ) + + +class TestAppAnnotationHitHistoryAccount: + def test_returns_the_annotation_creator(self, sqlite_session: Session) -> None: + app_id = str(uuid4()) + creator = _persist_account(sqlite_session) + annotation = _persist_annotation(sqlite_session, app_id=app_id, account_id=creator.id) + history = _hit_history(app_id=app_id, annotation_id=annotation.id, account_id=str(uuid4())) + + result = history.account(session=sqlite_session) + + assert result is not None + assert result.id == creator.id + + def test_returns_none_when_annotation_missing(self, sqlite_session: Session) -> None: + history = _hit_history(app_id=str(uuid4()), annotation_id=str(uuid4()), account_id=str(uuid4())) + + assert history.account(session=sqlite_session) is None + + +class TestAppAnnotationHitHistoryAnnotationCreateAccount: + def test_returns_the_hit_requester(self, sqlite_session: Session) -> None: + app_id = str(uuid4()) + requester = _persist_account(sqlite_session) + annotation = _persist_annotation(sqlite_session, app_id=app_id, account_id=str(uuid4())) + history = _hit_history(app_id=app_id, annotation_id=annotation.id, account_id=requester.id) + + result = history.annotation_create_account(session=sqlite_session) + + assert result is not None + assert result.id == requester.id + + def test_returns_none_when_account_missing(self, sqlite_session: Session) -> None: + history = _hit_history(app_id=str(uuid4()), annotation_id=str(uuid4()), account_id=str(uuid4())) + + assert history.annotation_create_account(session=sqlite_session) is None diff --git a/api/tests/unit_tests/models/test_tool_provider_session_accessors.py b/api/tests/unit_tests/models/test_tool_provider_session_accessors.py new file mode 100644 index 00000000000..02506476d05 --- /dev/null +++ b/api/tests/unit_tests/models/test_tool_provider_session_accessors.py @@ -0,0 +1,152 @@ +"""Regression coverage for the ``@property``→session-parameter refactor on tool provider accessors. + +Covers four of `api/models/tools.py`'s legacy `@property` accessors that reached for the global +``db.session`` internally and have been converted to plain methods taking an explicit +``session: Session`` (per the pattern established in #40370/#40797/#41394, tracked in #40372): + +- ``ApiToolProvider.tenant`` +- ``WorkflowToolProvider.user`` +- ``WorkflowToolProvider.tenant`` +- ``WorkflowToolProvider.app`` + +Each accessor is exercised against the real ``sqlite_session`` fixture (a genuine SQLAlchemy +``Session`` bound to a pristine full-schema SQLite database) so the assertions cover actual +query behaviour rather than a mock's recorded call. +""" + +from uuid import uuid4 + +from sqlalchemy.orm import Session + +from core.tools.entities.tool_entities import ApiProviderSchemaType +from models.account import Account, Tenant +from models.model import App, AppMode +from models.tools import ApiToolProvider, WorkflowToolProvider + + +def _persist_account(session: Session) -> Account: + account = Account(name="Test Account", email="test@example.com") + session.add(account) + session.flush() + return account + + +def _persist_tenant(session: Session) -> Tenant: + tenant = Tenant(name="Test Tenant") + session.add(tenant) + session.flush() + return tenant + + +def _persist_app(session: Session, *, tenant_id: str) -> App: + app = App( + tenant_id=tenant_id, + name="Test App", + mode=AppMode.WORKFLOW, + enable_site=True, + enable_api=True, + created_by=str(uuid4()), + ) + session.add(app) + session.flush() + return app + + +def _api_tool_provider(*, tenant_id: str) -> ApiToolProvider: + return ApiToolProvider( + tenant_id=tenant_id, + user_id=str(uuid4()), + name="Test API Provider", + icon="{}", + schema="{}", + schema_type_str=ApiProviderSchemaType.OPENAPI, + description="", + tools_str="[]", + credentials_str="{}", + ) + + +def _workflow_tool_provider(*, tenant_id: str, app_id: str, user_id: str) -> WorkflowToolProvider: + return WorkflowToolProvider( + name="Test Workflow Provider", + label="Test Workflow Provider", + icon="{}", + app_id=app_id, + version="1", + user_id=user_id, + tenant_id=tenant_id, + description="", + ) + + +class TestApiToolProviderTenant: + def test_returns_persisted_tenant(self, sqlite_session: Session) -> None: + tenant = _persist_tenant(sqlite_session) + provider = _api_tool_provider(tenant_id=tenant.id) + + result = provider.tenant(session=sqlite_session) + + assert result is not None + assert result.id == tenant.id + + def test_returns_none_when_tenant_missing(self, sqlite_session: Session) -> None: + provider = _api_tool_provider(tenant_id=str(uuid4())) + + assert provider.tenant(session=sqlite_session) is None + + +class TestWorkflowToolProviderUser: + def test_returns_persisted_user(self, sqlite_session: Session) -> None: + tenant = _persist_tenant(sqlite_session) + app = _persist_app(sqlite_session, tenant_id=tenant.id) + account = _persist_account(sqlite_session) + provider = _workflow_tool_provider(tenant_id=tenant.id, app_id=app.id, user_id=account.id) + + result = provider.user(session=sqlite_session) + + assert result is not None + assert result.id == account.id + + def test_returns_none_when_user_missing(self, sqlite_session: Session) -> None: + tenant = _persist_tenant(sqlite_session) + app = _persist_app(sqlite_session, tenant_id=tenant.id) + provider = _workflow_tool_provider(tenant_id=tenant.id, app_id=app.id, user_id=str(uuid4())) + + assert provider.user(session=sqlite_session) is None + + +class TestWorkflowToolProviderTenant: + def test_returns_persisted_tenant(self, sqlite_session: Session) -> None: + tenant = _persist_tenant(sqlite_session) + app = _persist_app(sqlite_session, tenant_id=tenant.id) + provider = _workflow_tool_provider(tenant_id=tenant.id, app_id=app.id, user_id=str(uuid4())) + + result = provider.tenant(session=sqlite_session) + + assert result is not None + assert result.id == tenant.id + + def test_returns_none_when_tenant_missing(self, sqlite_session: Session) -> None: + tenant = _persist_tenant(sqlite_session) + app = _persist_app(sqlite_session, tenant_id=tenant.id) + provider = _workflow_tool_provider(tenant_id=str(uuid4()), app_id=app.id, user_id=str(uuid4())) + + assert provider.tenant(session=sqlite_session) is None + + +class TestWorkflowToolProviderApp: + def test_returns_persisted_app(self, sqlite_session: Session) -> None: + tenant = _persist_tenant(sqlite_session) + app = _persist_app(sqlite_session, tenant_id=tenant.id) + provider = _workflow_tool_provider(tenant_id=tenant.id, app_id=app.id, user_id=str(uuid4())) + + result = provider.app(session=sqlite_session) + + assert result is not None + assert result.id == app.id + + def test_returns_none_when_app_missing(self, sqlite_session: Session) -> None: + tenant = _persist_tenant(sqlite_session) + provider = _workflow_tool_provider(tenant_id=tenant.id, app_id=str(uuid4()), user_id=str(uuid4())) + + assert provider.app(session=sqlite_session) is None diff --git a/packages/dify-ui/AGENTS.md b/packages/dify-ui/AGENTS.md index 5e17256c562..6c45faca8af 100644 --- a/packages/dify-ui/AGENTS.md +++ b/packages/dify-ui/AGENTS.md @@ -1,12 +1,14 @@ # @langgenius/dify-ui -This file owns the package boundary and routes detailed contracts. Start from the [package index], -then read only the guide for the contract being changed. +This file owns the package boundary. Read the matching contract guide below directly; use the +[package index] when discovering available primitives or usage examples. ## Package boundary - Keep this an independent primitive package. Do not import from application packages or depend on routing, i18n, application state, schemas, data fetching, or business APIs. +- Keep package contracts and contributor guidance self-contained here. Application docs and agent + skills may reference these guides; these guides must not require Web docs or skills to define package behavior. - Prefer `@base-ui/react` when it owns the required headless behavior. Style primitives with `cva`, `cn`, and Dify design tokens. Keep one primitive per `src//` folder with optional colocated stories and tests. diff --git a/packages/dify-ui/README.md b/packages/dify-ui/README.md index 645665fb89e..db32e4e26dd 100644 --- a/packages/dify-ui/README.md +++ b/packages/dify-ui/README.md @@ -49,13 +49,13 @@ Import `styles.css` once from the consumer's root stylesheet or entrypoint. Utilities: -- `./cn` composes conditional classes with `clsx` and `tailwind-merge`. +- `./cn` re-exports `cn` from the `cn` package through Dify UI's public subpath. - `./styles.css` provides design tokens, theme variables, and shared utilities. ## Guides -Start here, then open only the guide for the contract being changed. Component-specific Dify -behavior lives beside the component. Contracts shared by several primitives live in `docs/`. +Open only the guide for the contract being changed. Component-specific Dify behavior lives beside +the component. Contracts shared by several primitives live in `docs/`. Upstream behavior remains owned by the [Base UI documentation]. ### Component guides @@ -80,8 +80,8 @@ Upstream behavior remains owned by the [Base UI documentation]. ## Contributing -Read [component authoring rules] before modifying the package, then open only the matching owner -guide. This index intentionally does not duplicate those contracts. +[Package rules][component authoring rules] own the package boundary and contributor guidance. +For a known contract, go directly to its guide above. [Accessible names and descriptions]: ./docs/accessible-names-and-descriptions.md [Base UI documentation]: https://base-ui.com/llms.txt diff --git a/packages/dify-ui/docs/testing.md b/packages/dify-ui/docs/testing.md index 7063601e01e..90cc1c11680 100644 --- a/packages/dify-ui/docs/testing.md +++ b/packages/dify-ui/docs/testing.md @@ -10,6 +10,9 @@ diagnostics. Run the remaining commands from `packages/dify-ui/`: ## Test boundary +This guide owns the Dify UI testing policy and runtime setup. Add tests for observable Dify +integration behavior or a reproducible regression, not merely because a component or prop exists. + The package has two [Vitest projects]. Both run in Playwright Chromium [Browser Mode]; the project name identifies the behavior owner, not a different runtime. @@ -18,9 +21,10 @@ configured accessibility checks through the [Storybook Vitest addon]. Add `play` also owns visible state changes, user interaction, keyboard paths, overlay flows, form behavior, loading behavior, or controlled-state coordination. -Use regular Vitest tests for lower-level wrapper contracts such as class variants, Base UI -passthrough props, hidden-input serialization, data-attribute hooks, stores, and edge cases that -do not need a documented example. +Use regular Vitest tests for Dify integration behavior that does not need a documented example, +such as submitted values, store behavior, or a known regression reached through a public API. +Prop passthrough alone does not justify a test. Assert the resulting behavior instead of CSS class +names or private structure, and do not duplicate behavior already owned by Base UI or the browser. Storybook [accessibility testing] uses `a11y.test = 'error'`, so enabled violations fail the test. Color contrast is the only globally disabled rule because it is a known design-token gap. Do not @@ -30,22 +34,13 @@ use a `play` test in place of an accessibility fix. ## Animation setup Base UI can wait for `element.getAnimations()` before unmounting transition-driven components. -Set its test flag in a Vitest setup file when a test asserts final DOM state rather than animation -behavior: - -```ts -;( - globalThis as typeof globalThis & { - BASE_UI_ANIMATIONS_DISABLED: boolean - } -).BASE_UI_ANIMATIONS_DISABLED = true -``` - -`vitest.setup.ts` already applies this for primitive tests. Storybook uses its preview setup and -must retain real animation lifecycles. A unit test that intentionally asserts animation behavior -may restore the flag to `false` locally, but must restore the previous value during cleanup. +[`vitest.setup.ts`] sets `BASE_UI_ANIMATIONS_DISABLED = true` for primitive tests +that assert final DOM state. Storybook uses its preview setup and retains real animation lifecycles. +A unit test that intentionally asserts animation behavior may set the flag to `false` locally, +but must restore the previous value during cleanup. [Browser Mode]: https://vitest.dev/guide/browser [Storybook Vitest addon]: https://storybook.js.org/docs/writing-tests/integrations/vitest-addon/index [Vitest projects]: https://vitest.dev/guide/projects.html +[`vitest.setup.ts`]: ../vitest.setup.ts [accessibility testing]: https://storybook.js.org/docs/writing-tests/accessibility-testing diff --git a/packages/dify-ui/src/button/README.md b/packages/dify-ui/src/button/README.md index b8a1e41f43b..aa130b0ea91 100644 --- a/packages/dify-ui/src/button/README.md +++ b/packages/dify-ui/src/button/README.md @@ -43,29 +43,9 @@ button semantics. It is not a link mode. `loading` owns Base UI's disabled interaction, retained focus, and the decorative spinner. The button remains in the tab order with `aria-disabled`, and Dify UI suppresses activation. Pass the -pending state only to `loading`: +pending state only to `loading`; keep independent availability conditions in `disabled`: ```tsx - -``` - -Keep independent availability conditions in `disabled`: - -```tsx - -``` - -Do not repeat the pending state in `disabled`: - -```tsx -// Incorrect: loading already handles isSaving. - - -// Correct. @@ -77,11 +57,7 @@ behavior and may leave the tab order. ### Accessible loading feedback The spinner is decorative. Keep a non-empty visible label throughout loading. If the visible label -stays the same, its text continues to name the button: - -```tsx - -``` +stays the same, its text continues to name the button. When the label changes while the focused button enters loading, give the changing text a stable ID and reference it explicitly. Some browser and screen-reader combinations do not reliably announce diff --git a/packages/dify-ui/src/select/index.tsx b/packages/dify-ui/src/select/index.tsx index 029879ad55a..c273c61b165 100644 --- a/packages/dify-ui/src/select/index.tsx +++ b/packages/dify-ui/src/select/index.tsx @@ -62,7 +62,7 @@ type SelectGroupProps = BaseSelect.Group.Props const selectTriggerVariants = cva( [ - 'group flex w-full items-center border-0 bg-components-input-bg-normal text-start text-components-input-text-filled outline-hidden', + 'group/select-trigger flex w-full items-center border-0 bg-components-input-bg-normal text-start text-components-input-text-filled outline-hidden', 'hover:bg-state-base-hover-alt focus-visible:bg-state-base-hover-alt data-popup-open:bg-state-base-hover-alt', 'focus-visible:ring-2 focus-visible:ring-state-accent-solid', 'data-placeholder:text-components-input-text-placeholder', @@ -94,7 +94,7 @@ function SelectTrigger({ className, children, size, ...props }: SelectTriggerPro return ( {children} - + diff --git a/web/AGENTS.md b/web/AGENTS.md index 23acaaf1628..9153a0dc64d 100644 --- a/web/AGENTS.md +++ b/web/AGENTS.md @@ -6,9 +6,12 @@ ## Package Contracts +Web owns application-specific requirements and consumes shared architecture guidance from skills and primitive contracts from Dify UI. Link to those owners instead of redefining their rules here. + +- For truncated text disclosure and native `title` decisions, follow [Truncated Text Disclosure]. - User-facing strings must use `web/i18n/en-US/` keys. When adding or renaming a key, update every supported locale with the correct localized value. - For new backend calls and migrated surfaces, use generated `consoleQuery` / `consoleClient` APIs from `@/service/client`. Do not add handwritten REST helpers or DTO mirrors, mock-backed app state, or direct edits to generated contracts. -- Prefer `@langgenius/dify-ui/*` primitives, data attributes, and design tokens. Start from the [Dify UI package index] when choosing a primitive or shared contract. Preserve a visible focus indicator on the final focusable element. +- Prefer `@langgenius/dify-ui/*` primitives, data attributes, and design tokens. Use the [Dify UI package index] to find a primitive; read the relevant contract directly when it is already known. Preserve a visible focus indicator on the final focusable element. - Reuse the Web `SearchInput` composite when its search, clear, and IME contract matches the feature; otherwise follow the canonical [Input Group contract]. - Give save and submit flows a real form boundary with visible labels and accessible errors. Use Dify UI `Form` when its structured submission and validation contract is the owner; otherwise use a native form. Follow the canonical [form contract]. - Follow the canonical [Button contract] and [IconButton contract] for action semantics, loading, accessible names, and primitive composition. Do not add a Web wrapper that hides those contracts. @@ -33,4 +36,5 @@ This block is written and re-added by `next dev` — verify at `node_modules/nex [Dify UI package index]: ../packages/dify-ui/README.md [IconButton contract]: ../packages/dify-ui/src/icon-button/README.md [Input Group contract]: ../packages/dify-ui/src/input-group/README.md +[Truncated Text Disclosure]: docs/truncated-text-disclosure.md [form contract]: ../packages/dify-ui/docs/forms.md diff --git a/web/README.md b/web/README.md index cb5abb83bc4..776e5a77016 100644 --- a/web/README.md +++ b/web/README.md @@ -19,10 +19,6 @@ First, install the dependencies: pnpm install ``` -> [!NOTE] -> JavaScript dependencies are managed by the workspace files at the repository root: `package.json`, `pnpm-lock.yaml`, and `pnpm-workspace.yaml`. -> Install dependencies and run the commands below from the repository root. - Then, configure the environment variables. Create `web/.env.local` and copy the contents from `web/.env.example`. Modify the values of these environment variables according to your requirements: @@ -97,25 +93,14 @@ Then follow the [Lint Documentation] to lint the code. ## Test -We use [Vitest] and [React Testing Library] for Unit Testing. - -**📖 Frontend Testing Guide**: See the [Frontend Testing Guide] for the canonical testing policy and workflow. - -> [!IMPORTANT] -> As we are using Vite+, the `vitest` command is not available. -> Please make sure to run tests with `vp` commands. -> For example, use `vp test` instead of `vitest`. - -Run test: +We use [Vitest] and [React Testing Library] through Vite+. Run unit tests in `happy-dom` with: ```bash cd web vp test run --project unit ``` -The standard unit command runs in `happy-dom`. Browser Mode is reserved for behavior that depends on a real browser; see the [Frontend Testing Guide] for its admission criteria and commands. Always select a project explicitly: bare `vp test` runs every registered project, including Browser Mode. - -If a test fails only in CI, inspect the failing job and reproduce it locally when possible. A rerun can help identify a flaky test, but it does not replace diagnosing or reporting the failure. +Select a project explicitly; bare `vp test` also runs Browser Mode. Use `vp` instead of the standalone `vitest` command. The [Frontend Testing Guide] owns test policy, Browser Mode admission, and diagnostic commands. ## Documentation diff --git a/web/app/(commonLayout)/app/(appDetailLayout)/[appId]/overview/time-range-picker/range-selector.tsx b/web/app/(commonLayout)/app/(appDetailLayout)/[appId]/overview/time-range-picker/range-selector.tsx index 9c3113e6df0..3f3cea791d9 100644 --- a/web/app/(commonLayout)/app/(appDetailLayout)/[appId]/overview/time-range-picker/range-selector.tsx +++ b/web/app/(commonLayout)/app/(appDetailLayout)/[appId]/overview/time-range-picker/range-selector.tsx @@ -12,8 +12,8 @@ import { SelectPortal, SelectPositioner, SelectTrigger, + SelectValue, } from '@langgenius/dify-ui/select' -import { RiArrowDownSLine } from '@remixicon/react' import dayjs from 'dayjs' import * as React from 'react' import { useCallback, useMemo, useState } from 'react' @@ -35,7 +35,6 @@ type Props = Readonly<{ const RangeSelector: FC = ({ isCustomRange, ranges, onSelect }) => { const { t } = useTranslation() - const [open, setOpen] = useState(false) const items = useMemo(() => { return ranges.map((range) => ({ ...range, @@ -69,8 +68,6 @@ const RangeSelector: FC = ({ isCustomRange, ranges, onSelect }) => { return ( value={selectedItem?.value ?? null} - open={open} - onOpenChange={setOpen} onValueChange={(nextValue) => { if (nextValue == null) return const nextItem = items.find((item) => item.value === nextValue) @@ -79,15 +76,12 @@ const RangeSelector: FC = ({ isCustomRange, ranges, onSelect }) => { handleSelectRange(nextItem) }} > - -
-
- {isCustomRange - ? t(($) => $['filter.period.custom'], { ns: 'appLog' }) - : selectedItem?.name} -
- -
+ + + {isCustomRange + ? t(($) => $['filter.period.custom'], { ns: 'appLog' }) + : selectedItem?.name} + diff --git a/web/app/components/plugins/install-plugin/install-from-github/steps/__tests__/selectPackage.spec.tsx b/web/app/components/plugins/install-plugin/install-from-github/steps/__tests__/selectPackage.spec.tsx index 33b070166a0..f4e3bdda887 100644 --- a/web/app/components/plugins/install-plugin/install-from-github/steps/__tests__/selectPackage.spec.tsx +++ b/web/app/components/plugins/install-plugin/install-from-github/steps/__tests__/selectPackage.spec.tsx @@ -262,16 +262,28 @@ describe('SelectPackage', () => { expect(onSelectVersion).toHaveBeenCalledWith({ value: 'v0.9.0', name: 'v0.9.0' }) }) - it('should select a valid package option', async () => { + it('should only allow package selection after a version is selected', async () => { const user = userEvent.setup() const onSelectPackage = vi.fn() - renderSelectPackage({ - selectedVersion: 'v1.0.0', + const props = { + ...createDefaultProps(), + updatePayload: createUpdatePayload(), onSelectPackage, - }) + } + const { rerender } = render() - const section = getSection('plugin.installFromGitHub.selectPackage') - await user.click(within(section).getByRole('combobox')) + const packageTrigger = screen.getByRole('combobox', { + name: 'plugin.installFromGitHub.selectPackage', + }) + expect(packageTrigger).toBeDisabled() + await user.click(packageTrigger) + expect(screen.queryByRole('listbox')).not.toBeInTheDocument() + expect(onSelectPackage).not.toHaveBeenCalled() + + rerender() + + expect(packageTrigger).toBeEnabled() + await user.click(packageTrigger) await user.click(await screen.findByRole('option', { name: 'plugin.tar.gz' })) expect(onSelectPackage).toHaveBeenCalledWith({ @@ -472,27 +484,6 @@ describe('SelectPackage', () => { }) }) - // ================================ - // PortalSelect Readonly State Tests - // ================================ - describe('PortalSelect Readonly State', () => { - it('should make package select readonly when no version selected', () => { - renderSelectPackage({ selectedVersion: '' }) - - // When no version is selected, package select should be readonly - const trigger = screen.getAllByRole('combobox')[1] - expect(trigger).toHaveAttribute('aria-readonly', 'true') - }) - - it('should make package select active when version is selected', () => { - renderSelectPackage({ selectedVersion: 'v1.0.0' }) - - // When version is selected, package select should be active - const trigger = screen.getAllByRole('combobox')[1] - expect(trigger).not.toHaveAttribute('aria-readonly', 'true') - }) - }) - // ================================ // installedValue Props Tests // ================================ diff --git a/web/app/components/plugins/install-plugin/install-from-github/steps/selectPackage.tsx b/web/app/components/plugins/install-plugin/install-from-github/steps/selectPackage.tsx index 77be3236f9f..690088bc7d6 100644 --- a/web/app/components/plugins/install-plugin/install-from-github/steps/selectPackage.tsx +++ b/web/app/components/plugins/install-plugin/install-from-github/steps/selectPackage.tsx @@ -128,7 +128,7 @@ const SelectPackage: React.FC = ({