fix: add shouldIgnoreRowClick guard to all unprotected tables#4562
fix: add shouldIgnoreRowClick guard to all unprotected tables#4562GanJiaKouN16 wants to merge 2 commits into
Conversation
Wire the existing GenerateResetLinkModal and PasswordResetLinkModal into the Actions dropdown in the workspace members table. - Add 'Reset password' menu item for workspace members (not self) - Add resetPassword API function in profile service - Show confirmation dialog before generating the reset link - Display the generated password reset link with copy functionality Closes Agenta-AI#2572
Several tables with row-level click navigation were missing the shouldIgnoreRowClick guard, causing clicks on interactive elements (checkboxes, dropdowns, buttons) to accidentally trigger row navigation. Changes: - Consolidate shouldIgnoreRowClick with broader selector list (merges EvaluationRunsTablePOC's extra selectors: [role='button'], [role='menuitem'], [role='checkbox'], .ant-btn, etc.) - Export INTERACTIVE_ROW_SELECTORS constant for reuse - Add guard to ObservabilityTable (traces) - Add guard to SessionsTable - Add guard to PromptsPage - Add guard to TestcasesTableShell - Add guard to EntityTable - Replace partial data-ivt-stop-row-click check in ScenarioListView with full shouldIgnoreRowClick - Update useEntityTableState to use consolidated selectors - Remove duplicate shouldIgnoreRowClick from navigationActions.ts - Update EvaluationRunsTablePOC to import from shared utility Closes Agenta-AI#3254
dosubot
Bot
added
the
size:L
|
Someone is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughCentralizes interactive-element selectors into INTERACTIVE_ROW_SELECTORS, refactors shouldIgnoreRowClick to use a single closest() check, applies the guard across OSS and package table components, and adds a workspace member reset-password API plus a two-step modal UI. ChangesInteractive Row Click Filtering Refactoring
Workspace Member Reset Password Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as WorkspaceManage Actions
participant Modal1 as GenerateResetLinkModal
participant API as resetPassword Service
participant Modal2 as PasswordResetLinkModal
User->>UI: Click "Reset password" menu item
UI->>Modal1: Show generate modal
User->>Modal1: Confirm generate
Modal1->>API: POST /api/profile/reset-password?user_id=...
API-->>Modal1: Return reset link
Modal1->>UI: onOk callback
UI->>UI: Store link, update modal states
UI->>Modal2: Show reset link modal
Modal2-->>User: Display reset link
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useEntityTableState.ts (1)
232-251: 💤 Low valueConsider refactoring to use single
closest()call for consistency and performance.The current implementation iterates through selectors and calls
closest()multiple times:const isInteractive = interactiveSelectors.some((selector) => target.closest(selector), )For consistency with
shouldIgnoreRowClick(which uses a singleclosest()call against the joined selector string), consider:const interactiveSelectorString = useMemo( () => [...DEFAULT_INTERACTIVE_SELECTORS, ...excludeClickSelectors].join(", "), [excludeClickSelectors], ) // In buildRowHandlers: const isInteractive = Boolean(target.closest(interactiveSelectorString))This matches the pattern in
useTableManager.tsxand avoids multiple DOM traversals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e74ab297-ae30-41a7-ad31-bdd07856a672
📒 Files selected for processing (13)
web/oss/src/components/EvaluationRunsTablePOC/actions/navigationActions.tsweb/oss/src/components/EvaluationRunsTablePOC/components/EvaluationRunsTable/index.tsxweb/oss/src/components/InfiniteVirtualTable/hooks/useTableManager.tsxweb/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsxweb/oss/src/components/pages/observability/components/ObservabilityTable/index.tsxweb/oss/src/components/pages/observability/components/SessionsTable/index.tsxweb/oss/src/components/pages/prompts/PromptsPage.tsxweb/oss/src/components/pages/settings/WorkspaceManage/cellRenderers.tsxweb/oss/src/services/profile/index.tsweb/packages/agenta-annotation-ui/src/components/AnnotationSession/ScenarioListView.tsxweb/packages/agenta-entity-ui/src/shared/EntityTable.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useEntityTableState.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsx
💤 Files with no reviewable changes (1)
- web/oss/src/components/EvaluationRunsTablePOC/actions/navigationActions.ts
| <GenerateResetLinkModal | ||
| open={generateResetLinkOpen} | ||
| username={user.username} | ||
| onCancel={() => setGenerateResetLinkOpen(false)} | ||
| onOk={handleResetPassword} | ||
| confirmLoading={resetLoading} | ||
| /> | ||
|
|
||
| <PasswordResetLinkModal | ||
| open={resetLinkOpen} | ||
| username={user.username} | ||
| generatedLink={resetLink} | ||
| onCancel={() => setResetLinkOpen(false)} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the modal components use EnhancedModal or raw antd Modal
fd -e tsx -e ts 'GenerateResetLinkModal|PasswordResetLinkModal' web/oss/src/components/pages/settings/WorkspaceManage/Modals/ --exec cat {}Repository: Agenta-AI/agenta
Length of output: 4194
Switch WorkspaceManage reset modals to EnhancedModal
web/oss/src/components/pages/settings/WorkspaceManage/Modals/GenerateResetLinkModal.tsximports and rendersModalfromantd(should useEnhancedModalfrom@agenta/ui).web/oss/src/components/pages/settings/WorkspaceManage/Modals/PasswordResetLinkModal.tsximports and rendersModalfromantd(should useEnhancedModalfrom@agenta/ui).
Source: Coding guidelines
| /** | ||
| * Generate a password reset link for a user (admin action). | ||
| * Returns the reset password link string. | ||
| */ | ||
| export const resetPassword = async (userId: string): Promise<string> => { | ||
| const base = getBaseUrl() | ||
| const url = new URL("api/profile/reset-password", base) | ||
| url.searchParams.set("user_id", userId) | ||
| const data = await fetchJson<string>(url, { | ||
| method: "POST", | ||
| }) | ||
| return data | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the Fern-generated client exports a method for password reset
rg -n -C3 'resetUserPassword|reset.*password' --type=ts --iglob '**/agenta-api-client/**'Repository: Agenta-AI/agenta
Length of output: 5484
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find ResetUserPasswordRequest shape and response handling
rg -n --type=ts 'ResetUserPasswordRequest' web/packages/agenta-api-client/src/generated/api -SRepository: Agenta-AI/agenta
Length of output: 809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the request interface
echo "---- ResetUserPasswordRequest.ts ----"
sed -n '1,120p' web/packages/agenta-api-client/src/generated/api/resources/users/client/requests/ResetUserPasswordRequest.ts
# Read the relevant portion of UsersClient.resetUserPassword
echo "---- Client.ts (resetUserPassword) ----"
sed -n '130,260p' web/packages/agenta-api-client/src/generated/api/resources/users/client/Client.tsRepository: Agenta-AI/agenta
Length of output: 4067
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=ts --type=tsx '\.resetUserPassword\(' web/oss web/ee web -SRepository: Agenta-AI/agenta
Length of output: 88
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '\.resetUserPassword\(' -S --glob '**/*.ts' --glob '**/*.tsx' web/oss web/eeRepository: Agenta-AI/agenta
Length of output: 42
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'resetUserPassword' -S --glob '**/*.ts' --glob '**/*.tsx' web/oss web/eeRepository: Agenta-AI/agenta
Length of output: 42
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --glob '**/*.ts' --glob '**/*.tsx' 'getAgentaSdkClient\(\{host: getAgentaApiUrl\(\)\}\)' web/oss web/eeRepository: Agenta-AI/agenta
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' web/oss/src/components/pages/_app/index.tsxRepository: Agenta-AI/agenta
Length of output: 3896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'ResetUserPasswordResponse|resetUserPassword.*Response|reset password' web/packages/agenta-api-client/src/generated/api/resources/users/client -SRepository: Agenta-AI/agenta
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'reset-password|ResetUserPassword' web/packages/agenta-api-client/src/generated/api -SRepository: Agenta-AI/agenta
Length of output: 1139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'profile/reset-password|reset_password|resetUserPassword' web/packages/agenta-api-client/src/generated/api -SRepository: Agenta-AI/agenta
Length of output: 1066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find how the SDK responses are consumed (e.g., `.data`)
rg -n --glob '**/*.ts' --glob '**/*.tsx' \
'\.data(\b|$)|rawResponse|HttpResponsePromise' web/oss/src web/ee/src \
|| trueRepository: Agenta-AI/agenta
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find how the SDK client return values are consumed (e.g., awaiting and reading `.data`)
rg -n --glob '**/*.ts' --glob '**/*.tsx' 'getAgentaSdkClient\(' web/oss web/eeRepository: Agenta-AI/agenta
Length of output: 153
Use the Fern-generated client for resetPassword
resetPassword manually builds the URL and uses fetchJson, but the Fern SDK already exposes client.users.resetUserPassword({ user_id }) for the POST profile/reset-password endpoint. Refactor to call that method via getAgentaSdkClient({ host: getAgentaApiUrl() }) and keep the return value consistent with the current Promise<string> contract.
Source: Coding guidelines
GanJiaKouN16
force-pushed
the
fix/table-row-click-behavior
branch
from
6dfce59 to
445fc3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useEntityTableState.ts (1)
232-251: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRefactor to follow the centralized click detection pattern and improve efficiency.
The inline click detection logic duplicates the pattern already centralized in
shouldIgnoreRowClick. The current implementation callsclosest()multiple times (once per selector), while the centralized helper passes the full comma-separated selector string to a singleclosest()call, which is more efficient and consistent.Since this hook supports custom
excludeClickSelectors, combine them withINTERACTIVE_ROW_SELECTORSinto a single selector string and follow the same pattern asshouldIgnoreRowClick.♻️ Proposed refactor to align with centralized pattern
- // Combine selectors for interactive elements - const interactiveSelectors = useMemo( - () => [...DEFAULT_INTERACTIVE_SELECTORS, ...excludeClickSelectors], - [excludeClickSelectors], + // Combine default and custom selectors into a single selector string for efficient matching + const combinedSelectors = useMemo( + () => + excludeClickSelectors.length > 0 + ? `${INTERACTIVE_ROW_SELECTORS}, ${excludeClickSelectors.join(", ")}` + : INTERACTIVE_ROW_SELECTORS, + [excludeClickSelectors], ) // Build row handlers for click events const buildRowHandlers = useCallback( (record: TRow) => { const isNavigable = !record.__isSkeleton return { onClick: (event: MouseEvent<HTMLTableRowElement>) => { if (!isNavigable || !onRowClick) return - // Check if clicking on interactive elements + // Check if clicking on interactive elements (follows shouldIgnoreRowClick pattern) const target = event.target as HTMLElement - const isInteractive = interactiveSelectors.some((selector) => - target.closest(selector), - ) + if (!target) return + const isInteractive = target.closest(combinedSelectors) if (isInteractive) return onRowClick(record) }, className: "entity-table__row", style: { cursor: isNavigable && onRowClick ? "pointer" : "default", height: rowHeight, minHeight: rowHeight, } as CSSProperties, } }, - [onRowClick, rowHeight, interactiveSelectors], + [onRowClick, rowHeight, combinedSelectors], )This approach:
- Uses a single
closest()call instead of looping through selectors- Follows the same pattern as the centralized
shouldIgnoreRowClick- Adds a defensive null check on
target- Maintains support for custom
excludeClickSelectors
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7741bd1a-d2b5-42c7-981d-2ad00b0172e7
📒 Files selected for processing (13)
web/oss/src/components/EvaluationRunsTablePOC/actions/navigationActions.tsweb/oss/src/components/EvaluationRunsTablePOC/components/EvaluationRunsTable/index.tsxweb/oss/src/components/InfiniteVirtualTable/hooks/useTableManager.tsxweb/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsxweb/oss/src/components/pages/observability/components/ObservabilityTable/index.tsxweb/oss/src/components/pages/observability/components/SessionsTable/index.tsxweb/oss/src/components/pages/prompts/PromptsPage.tsxweb/oss/src/components/pages/settings/WorkspaceManage/cellRenderers.tsxweb/oss/src/services/profile/index.tsweb/packages/agenta-annotation-ui/src/components/AnnotationSession/ScenarioListView.tsxweb/packages/agenta-entity-ui/src/shared/EntityTable.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useEntityTableState.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsx
💤 Files with no reviewable changes (1)
- web/oss/src/components/EvaluationRunsTablePOC/actions/navigationActions.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- web/oss/src/components/pages/observability/components/SessionsTable/index.tsx
- web/oss/src/components/pages/prompts/PromptsPage.tsx
- web/oss/src/services/profile/index.ts
- web/packages/agenta-entity-ui/src/shared/EntityTable.tsx
- web/packages/agenta-annotation-ui/src/components/AnnotationSession/ScenarioListView.tsx
- web/oss/src/components/InfiniteVirtualTable/hooks/useTableManager.tsx
- web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsx
- web/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsx
- web/oss/src/components/pages/observability/components/ObservabilityTable/index.tsx
- web/oss/src/components/EvaluationRunsTablePOC/components/EvaluationRunsTable/index.tsx
| const handleResetPassword = async () => { | ||
| setResetLoading(true) | ||
| try { | ||
| const link = await resetPassword(user.id) | ||
| setGenerateResetLinkOpen(false) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Route reset-password through the Fern SDK client, not the manual fetch path.
This handler now depends on a new API call path, but resetPassword is implemented with manual URL + fetchJson instead of the Fern client contract used in web/**/*.{ts,tsx}. Please migrate that service call to getAgentaSdkClient({host: getAgentaApiUrl()}) and pass params via {queryParams: {...}} before merge.
As per coding guidelines, all new frontend API code must go through the Fern-generated client and use {queryParams} rather than axios/fetch-style params.
Source: Coding guidelines
| <PasswordResetLinkModal | ||
| open={resetLinkOpen} | ||
| username={user.username} | ||
| generatedLink={resetLink} | ||
| onCancel={() => setResetLinkOpen(false)} | ||
| /> |
There was a problem hiding this comment.
Clear the generated reset link when closing the modal.
On Line 219, close only toggles resetLinkOpen; the reset URL remains in state. Since this is a sensitive token, clear it on close (and optionally before a new generation) to reduce in-memory retention.
Suggested patch
<PasswordResetLinkModal
open={resetLinkOpen}
username={user.username}
generatedLink={resetLink}
- onCancel={() => setResetLinkOpen(false)}
+ onCancel={() => {
+ setResetLinkOpen(false)
+ setResetLink("")
+ }}
/>2 participants
Summary
Fixes #3254 — several tables with row-level click navigation were missing the
shouldIgnoreRowClickguard, causing clicks on interactive elements (checkboxes, dropdowns, buttons) to accidentally trigger row navigation.Changes
Consolidated shared utility
packages/agenta-ui/.../useTableManager.tsxandoss/.../useTableManager.tsx: ExpandedshouldIgnoreRowClickwith broader selectors (merges extra selectors from EvaluationRunsTablePOC:[role='button'],[role='menuitem'],[role='checkbox'],.ant-btn,select, etc.). ExportedINTERACTIVE_ROW_SELECTORSconstant for reuse.packages/agenta-ui/.../useEntityTableState.ts: Updated to use consolidated selectors instead of its own narrower list.Tables fixed (6 previously unprotected)
observability/.../ObservabilityTable/index.tsxshouldIgnoreRowClickguardobservability/.../SessionsTable/index.tsxshouldIgnoreRowClickguardprompts/PromptsPage.tsxshouldIgnoreRowClickguardTestcasesTableNew/.../TestcasesTableShell.tsxshouldIgnoreRowClickguardagenta-entity-ui/.../EntityTable.tsxshouldIgnoreRowClickguardagenta-annotation-ui/.../ScenarioListView.tsxdata-ivt-stop-row-clickcheck with fullshouldIgnoreRowClickCleanup
navigationActions.ts: Removed duplicateshouldIgnoreRowClickand unusedMouseEventimport.EvaluationRunsTable/index.tsx: Updated import to use shared utility fromInfiniteVirtualTable.Testing
onClick: (event) => { if (shouldIgnoreRowClick(event)) return; ... }Closes #3254