diff --git a/.agents/skills/frontend-code-review/SKILL.md b/.agents/skills/frontend-code-review/SKILL.md index 855f4acec6..5859844e60 100644 --- a/.agents/skills/frontend-code-review/SKILL.md +++ b/.agents/skills/frontend-code-review/SKILL.md @@ -1,6 +1,6 @@ --- name: frontend-code-review -description: "Trigger when the user requests a review of frontend files (e.g., `.tsx`, `.ts`, `.js`). Support both pending-change reviews and focused file reviews while applying the checklist rules." +description: "Trigger when the user requests a review of frontend files (e.g., `.tsx`, `.ts`, `.js`). Support pending-change and focused file reviews while applying checklist rules, shared component reuse checks, and React component structure guidance from how-to-write-component." --- # Frontend Code Review @@ -16,10 +16,12 @@ Stick to the checklist below for every applicable file and mode. ## Checklist See [references/code-quality.md](references/code-quality.md), [references/performance.md](references/performance.md), [references/business-logic.md](references/business-logic.md) for the living checklist split by category—treat it as the canonical set of rules to follow. +When reviewing React/TypeScript components, also apply the repo-local `how-to-write-component` skill as the component architecture checklist. In particular, check ownership boundaries, props and API types, query/mutation usage, navigation choices, effect usage, unnecessary wrappers, and unnecessary memoization. + Flag each rule violation with urgency metadata so future reviewers can prioritize fixes. ## Review Process -1. Open the relevant component/module. Gather lines that relate to class names, styling/CSS imports, file size and component boundaries, i18n keys, behavior-sensitive UI interactions, React Flow hooks, and prop memoization. +1. Open the relevant component/module. Gather lines that relate to shared base/dify-ui component reuse, class names, styling/CSS imports, file size and component boundaries, i18n keys, behavior-sensitive UI interactions, React Flow hooks, and prop memoization. 2. For each rule in the review point, note where the code deviates and capture a representative snippet. 3. Compose the review section per the template below. Group violations first by **Urgent** flag, then by category order (Code Quality, Performance, Business Logic). diff --git a/.agents/skills/frontend-code-review/references/code-quality.md b/.agents/skills/frontend-code-review/references/code-quality.md index df94f4298e..5b5f87be01 100644 --- a/.agents/skills/frontend-code-review/references/code-quality.md +++ b/.agents/skills/frontend-code-review/references/code-quality.md @@ -42,7 +42,17 @@ Category: Code Quality When a frontend file grows large or mixes multiple responsibilities, suggest splitting it into focused components, hooks, or utilities. Prefer shallow local structure that matches existing repo patterns, such as a sibling `components/` folder, and avoid deep folder hierarchies unless the surrounding code already uses them. -Update this file when adding, editing, or removing Code Quality rules so the catalog remains accurate. +## Reuse base and dify-ui components before hand-rolling UI + +Category: Code Quality + +### Description + +Before approving new or modified frontend UI, check whether the code manually recreates behavior or styling already owned by `@langgenius/dify-ui/*` or `web/app/components/base/*`. Common examples include `Button`, `Input`, `ToggleGroup`, `Popover`, `DropdownMenu`, `AlertDialog`, `Switch`, `Avatar`, `ScrollArea`, `toast`, and existing feature components. Prefer composing existing primitives instead of duplicating borders, focus states, disabled states, segmented controls, inputs, overlays, or buttons. + +### Suggested Fix + +Replace hand-written UI chrome with the nearest shared primitive, keeping feature-specific layout, state ownership, labels, and workflow behavior local. ## Classname ordering for easy overrides @@ -59,3 +69,5 @@ const Button = ({ className }) => { return
} ``` + +Update this file when adding, editing, or removing Code Quality rules so the catalog remains accurate. diff --git a/docs/main-nav-gating-follow-ups.md b/docs/main-nav-gating-follow-ups.md index 7e3439dd51..7fd9e963df 100644 --- a/docs/main-nav-gating-follow-ups.md +++ b/docs/main-nav-gating-follow-ups.md @@ -2,13 +2,21 @@ Context: the desktop MainNav rewrite moved several workspace, account, tools, and marketplace entry points out of the old header/account-setting layout. These notes track product-contract questions that should be resolved before treating the rewrite as behavior-complete. +Current status: + +- Open: account-setting modal navigation API, Marketplace install/error status parity, default account language entry. +- Partially resolved: Apps/Datasets quick-switch/create parity. +- Resolved: Integrations sidebar placeholder state, branding-gated Help trigger, workspace plan billing access. + ## 1. Account-setting modal naming and moved destinations +Status: Open. + Current branch behavior: -- `setShowAccountSettingModal(PROVIDER)` routes to `/tools?section=provider`. -- `setShowAccountSettingModal(DATA_SOURCE)` routes to `/tools?section=data-source`. -- `setShowAccountSettingModal(API_BASED_EXTENSION)` routes to `/tools?section=api-based-extension`. +- `setShowAccountSettingModal(PROVIDER)` routes to `/integrations/model-provider`. +- `setShowAccountSettingModal(DATA_SOURCE)` routes to `/integrations/data-source`. +- `setShowAccountSettingModal(API_BASED_EXTENSION)` routes to `/integrations/tools/api-extension`. Old behavior: these calls opened the account-setting modal and switched to the matching tab. @@ -23,22 +31,26 @@ Follow-up decision needed: ## 2. Integrations sidebar disabled entries -Current branch behavior: +Status: Resolved. + +Previous branch behavior: - Integrations includes disabled entries for Trigger, Agent Strategy, and Extension. - These are visible but not actionable. -Status: +Current branch behavior: -- Currently being handled separately. +- Trigger, Agent Strategy, and Extension are no longer disabled placeholders. +- These sections route to `PluginCategoryPage` with the corresponding plugin category. -Follow-up decision needed: +Resolution: -- Decide whether disabled future entries should remain visible, be hidden until supported, or be gated by feature flags/edition/role. -- If they stay visible, define the tooltip or disabled-state copy so users understand why the option is unavailable. +- No remaining disabled-entry gating decision is tracked here. ## 3. Plugin and marketplace status parity +Status: Open. + Old header behavior: - `PluginsNav` showed plugin install progress and error state through the installing icon and red indicator. @@ -56,24 +68,29 @@ Follow-up decision needed: ## 4. Mobile/default account language entry +Status: Open. + Current branch behavior: - Desktop MainNav account menu includes Language and Timezone submenus. -- Mobile still uses the default header/account dropdown path. +- The main app layout now uses MainNav across breakpoints. - The default account dropdown does not expose the Language settings entry. +- The default account dropdown still exists in non-MainNav account/header surfaces such as the account layout. Decision: - Preserve the old language-access contract across breakpoints. -- The desktop MainNav path is acceptable; the missing case is mainly mobile/default account dropdown, including dataset-operator users on mobile. +- The desktop MainNav path is acceptable; the missing case is default account-dropdown parity wherever that path remains active. Follow-up decision needed: -- Add an equivalent language entry to the default/mobile account path, or otherwise ensure mobile users can still reach language settings. +- Add an equivalent language entry to the default account path, or otherwise ensure users in those surfaces can still reach language settings. - Keep this as gate-contract parity, not a visual requirement to recreate the old Account Settings sidebar. ## 5. Apps and Datasets quick-switch/create parity +Status: Partially resolved. + Old header behavior: - `AppNav` could show the current app, list more apps, load more results, and launch create-app flows from the header nav. @@ -81,8 +98,9 @@ Old header behavior: Current MainNav behavior: -- Apps and Datasets are static navigation links. -- App/dataset quick switching and create actions are not present in the desktop MainNav. +- Apps is no longer only a static navigation link: MainNav includes a Web Apps section with installed web app search, pin, delete, and navigation behavior. +- Apps still does not preserve the old `AppNav` current-app switcher, load-more behavior, or create-app flows. +- Datasets is still a navigation link and does not preserve the old `DatasetNav` current-dataset switcher, load-more behavior, or dataset creation entry. Follow-up decision needed: @@ -91,6 +109,8 @@ Follow-up decision needed: ## 6. Branding-gated Help and Support behavior +Status: Resolved. + Old account dropdown behavior: - When `systemFeatures.branding.enabled` is `true`, the whole Dify help/community group is hidden. @@ -98,21 +118,21 @@ Old account dropdown behavior: Current MainNav behavior: -- HelpMenu keeps the trigger visible, but its content is gated by `!systemFeatures.branding.enabled`. -- This can produce an empty Help popup when branding is enabled. +- `HelpMenu` returns `null` when `systemFeatures.branding.enabled` is `true`. +- This prevents the empty Help trigger/popup path. -Open question: +Resolution: -- The old coarse gate also hides Support, but Support can contain instance-specific channels such as configured Zendesk or support email in addition to Dify forum/Discord links. -- Confirm whether branded deployments should hide Support entirely, or keep configured customer support channels while hiding Dify official/community links. +- Current implementation follows strict old parity for MainNav: the whole Help trigger is hidden for branded deployments. -Follow-up decision needed: +Optional future product question: -- If strict old parity is required, hide the HelpMenu trigger when branding is enabled. -- If branded deployments should retain support access, split Support into customer-support and Dify-community items with separate gates. +- If branded deployments should retain configured customer support channels, split Support into customer-support and Dify-community items with separate gates. ## 7. Paid plan Billing access from workspace plan +Status: Resolved. + Old header behavior: - The header plan badge was clickable. @@ -122,17 +142,14 @@ Old header behavior: Current MainNav behavior: - Sandbox/free plans have an explicit Upgrade action in the WorkspaceCard credit row. -- Non-sandbox paid plans show the workspace plan badge, but the badge is display-only. +- Non-sandbox paid plans have an explicit View Plan action in the same plan-action row. +- Both actions open the pricing modal. +- The workspace plan badge is display-only. - The WorkspaceCard Settings menu item routes to Account Settings on the Billing tab. - Invite Members remains the Members entry, so Settings and Invite Members do not duplicate the same destination. -Open question: +Resolution: -- Confirm whether product still wants the plan badge itself to be clickable, or whether the Settings-to-Billing menu item is the intended MainNav access path. -- Avoid making the plan badge itself clickable unless the interaction is explicitly approved, because the WorkspaceCard already uses a button to open the workspace menu. - -Recommended default: - -- Keep sandbox/free behavior as the explicit Upgrade action. -- Keep the plan badge display-only. +- Keep sandbox/free and paid behavior as the explicit plan-action row. +- Keep the workspace plan badge display-only. - Use the WorkspaceCard Settings item as the Billing entry.