fix: address all code review feedback from PR reviewer

Copilot · hotlong · Copilot · commit 7dd054768b9d · 2026-03-03T07:53:59.000Z
- RecordDetailView: remove unused _context param, respect action.refreshAfter
- ActionParamDialog: use isMissingValue() for falsy-safe validation, use ?? instead of ||, handle number type with valueAsNumber
- RecordDetailActions.test: remove unused 'within' import, use not.toHaveBeenCalled()
- action-bar: destructure data from rest to prevent DOM prop warnings
- action-button: destructure data from rest, simplify params routing with Array.isArray

Co-authored-by: hotlong &lt;50353452+hotlong@users.noreply.github.com&gt;
diff --git a/apps/console/src/__tests__/RecordDetailActions.test.tsx b/apps/console/src/__tests__/RecordDetailActions.test.tsx
@@ -11,7 +11,7 @@
  */
 
 import { describe, it, expect, vi, beforeEach } from 'vitest';
-import { render, screen, waitFor, within } from '@testing-library/react';
+import { render, screen, waitFor } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import '@testing-library/jest-dom';
 import { MemoryRouter, Routes, Route } from 'react-router-dom';
@@ -217,7 +217,7 @@ describe('RecordDetailView — Action buttons', () => {
     // dataSource.update should NOT be called
     // (wait a tick to ensure no async calls happen)
     await new Promise(r => setTimeout(r, 100));
-    expect(ds.update).not.toHaveBeenCalledWith('opportunity', 'opp-1', expect.anything());
+    expect(ds.update).not.toHaveBeenCalled();
   });
 
   it('shows param collection dialog for Change Stage action', async () => {
diff --git a/apps/console/src/components/ActionParamDialog.tsx b/apps/console/src/components/ActionParamDialog.tsx
@@ -59,11 +59,18 @@ export function ActionParamDialog({ state, onOpenChange }: ActionParamDialogProp
     }
   }, [state.open, state.params]);
 
+  const isMissingValue = (value: unknown): boolean => {
+    if (value === undefined || value === null) return true;
+    if (typeof value === 'string') return value.trim() === '';
+    if (Array.isArray(value)) return value.length === 0;
+    return false;
+  };
+
   const handleSubmit = () => {
     // Validate required fields
     const newErrors: Record<string, boolean> = {};
     for (const param of state.params) {
-      if (param.required && !values[param.name]) {
+      if (param.required && isMissingValue(values[param.name])) {
         newErrors[param.name] = true;
       }
     }
@@ -107,7 +114,7 @@ export function ActionParamDialog({ state, onOpenChange }: ActionParamDialogProp
 
               {param.type === 'select' && param.options ? (
                 <Select
-                  value={values[param.name] || ''}
+                  value={values[param.name] ?? ''}
                   onValueChange={(val) => updateValue(param.name, val)}
                 >
                   <SelectTrigger id={param.name} className={errors[param.name] ? 'border-destructive' : ''}>
@@ -124,16 +131,25 @@ export function ActionParamDialog({ state, onOpenChange }: ActionParamDialogProp
               ) : param.type === 'textarea' ? (
                 <Textarea
                   id={param.name}
-                  value={values[param.name] || ''}
+                  value={values[param.name] ?? ''}
                   onChange={(e) => updateValue(param.name, e.target.value)}
                   placeholder={param.placeholder}
                   className={errors[param.name] ? 'border-destructive' : ''}
                 />
+              ) : param.type === 'number' ? (
+                <Input
+                  id={param.name}
+                  type="number"
+                  value={values[param.name] ?? ''}
+                  onChange={(e) => updateValue(param.name, e.target.valueAsNumber)}
+                  placeholder={param.placeholder}
+                  className={errors[param.name] ? 'border-destructive' : ''}
+                />
               ) : (
                 <Input
                   id={param.name}
-                  type={(['number', 'email', 'url', 'date', 'datetime-local', 'time', 'password'] as string[]).includes(param.type) ? param.type : 'text'}
-                  value={values[param.name] || ''}
+                  type={(['email', 'url', 'date', 'datetime-local', 'time', 'password'] as string[]).includes(param.type) ? param.type : 'text'}
+                  value={values[param.name] ?? ''}
                   onChange={(e) => updateValue(param.name, e.target.value)}
                   placeholder={param.placeholder}
                   className={errors[param.name] ? 'border-destructive' : ''}
diff --git a/apps/console/src/components/RecordDetailView.tsx b/apps/console/src/components/RecordDetailView.tsx
@@ -20,7 +20,7 @@ import { SkeletonDetail } from './skeletons';
 import { ActionConfirmDialog, type ConfirmDialogState } from './ActionConfirmDialog';
 import { ActionParamDialog, type ParamDialogState } from './ActionParamDialog';
 import type { DetailViewSchema, FeedItem } from '@object-ui/types';
-import type { ActionDef, ActionContext as ActionCtx, ActionParamDef } from '@object-ui/core';
+import type { ActionDef, ActionParamDef } from '@object-ui/core';
 
 interface RecordDetailViewProps {
   dataSource: any;
@@ -80,7 +80,7 @@ export function RecordDetailView({ dataSource, objects, onEdit }: RecordDetailVi
   }, [navigate]);
 
   // API action handler — maps logical action targets to dataSource operations
-  const apiHandler = useCallback(async (action: ActionDef, _context: ActionCtx) => {
+  const apiHandler = useCallback(async (action: ActionDef) => {
     try {
       const target = action.target || action.name;
       const params = action.params || {};
@@ -103,8 +103,11 @@ export function RecordDetailView({ dataSource, objects, onEdit }: RecordDetailVi
           break;
       }
 
-      setActionRefreshKey(k => k + 1);
-      return { success: true, reload: true };
+      const shouldRefresh = action.refreshAfter === true;
+      if (shouldRefresh) {
+        setActionRefreshKey(k => k + 1);
+      }
+      return { success: true, reload: shouldRefresh };
     } catch (error) {
       return { success: false, error: (error as Error).message };
     }
diff --git a/packages/components/src/renderers/action/action-bar.tsx b/packages/components/src/renderers/action/action-bar.tsx
@@ -66,6 +66,7 @@ const ActionBarRenderer = forwardRef<HTMLDivElement, { schema: ActionBarSchema;
       'data-obj-id': dataObjId,
       'data-obj-type': dataObjType,
       style,
+      data,
       ...rest
     } = props;
 
@@ -141,7 +142,7 @@ const ActionBarRenderer = forwardRef<HTMLDivElement, { schema: ActionBarSchema;
                 variant: action.variant || schema.variant,
                 size: action.size || schema.size,
               }}
-              data={rest.data}
+              data={data}
             />
           );
         })}
diff --git a/packages/components/src/renderers/action/action-button.tsx b/packages/components/src/renderers/action/action-button.tsx
@@ -41,14 +41,15 @@ const ActionButtonRenderer = forwardRef<HTMLButtonElement, ActionButtonProps>(
       'data-obj-id': dataObjId,
       'data-obj-type': dataObjType,
       style,
+      data,
       ...rest
     } = props;
 
     const { execute } = useAction();
     const [loading, setLoading] = useState(false);
 
     // Record data may be passed from SchemaRenderer (e.g. DetailView passes record data)
-    const recordData = rest.data != null && typeof rest.data === 'object' ? rest.data as Record<string, any> : {};
+    const recordData = data != null && typeof data === 'object' ? data as Record<string, any> : {};
 
     // Evaluate visibility and enabled conditions with record data context
     const isVisible = useCondition(schema.visible ? `\${${schema.visible}}` : undefined, recordData);
@@ -66,10 +67,12 @@ const ActionButtonRenderer = forwardRef<HTMLButtonElement, ActionButtonProps>(
       setLoading(true);
 
       try {
-        // Detect if schema.params are ActionParam definitions (array of {name,type,...})
-        // vs actual param values (Record<string, any>)
-        const isParamDefs = Array.isArray(schema.params) && schema.params.length > 0 &&
-          typeof schema.params[0] === 'object' && 'name' in schema.params[0] && 'type' in schema.params[0];
+        // Route params correctly:
+        // - Array of objects with name+type → ActionParamDef[] → pass as actionParams for collection
+        // - Otherwise → pass as actual param values
+        const paramsPayload = Array.isArray(schema.params)
+          ? { actionParams: schema.params as any }
+          : { params: schema.params as Record<string, any> | undefined };
 
         await execute({
           type: schema.actionType || schema.type,
@@ -78,9 +81,7 @@ const ActionButtonRenderer = forwardRef<HTMLButtonElement, ActionButtonProps>(
           execute: schema.execute,
           endpoint: schema.endpoint,
           method: schema.method,
-          ...(isParamDefs
-            ? { actionParams: schema.params as any }
-            : { params: schema.params as Record<string, any> | undefined }),
+          ...paramsPayload,
           confirmText: schema.confirmText,
           successMessage: schema.successMessage,
           errorMessage: schema.errorMessage,
