refactor(ui-react): Simplify hooks and reduce code duplication (Vibe Kanban) - #77
Conversation
…ckage. Here's a summary of the changes made:
## Simplifications Applied
### Shared Utilities Added
- Extracted `DEFAULT_HOST_CONTEXT` and `DEFAULT_INSETS` constants to eliminate duplication
- Created `createSizeObserver` helper to consolidate ResizeObserver logic used in multiple hooks
### Hook Simplifications
| Hook | Changes |
|------|---------|
| `useHostContext` | Uses shared constant, simplified callback |
| `useSafeAreaInsets` | Single-line return with shared constant |
| `useOnToolCancelled`, `useOnTeardown`, `useOnToolInputPartial` | Direct cleanup function return |
| `useHostCapabilities` | Simplified initializer and callback |
| `useSizeChangedNotifications` | Uses `createSizeObserver` helper |
| `useFileUpload` | Individual `useState` calls, derived `isSupported`, `finally` block |
| `useFileDownload` | Same pattern as `useFileUpload` |
| `useIntrinsicHeight` | Uses `createSizeObserver` helper |
### Import Cleanup
- Changed to type-only imports where appropriate (`import type { RefObject }`)
- Cleaner JSX type usage
All tests pass (29/29), typecheck passes, and lint passes. The changes preserve all functionality while improving clarity and reducing duplication.
…mments from `packages/ui-react/src/index.ts`: - Line 22: `// Context (placeholder - will be implemented in Phase 6)` → `// Context` - Line 26: `// Hooks (placeholder - will be implemented in Phase 6)` → `// Hooks`
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRefactors ui-react hooks and context: reorganizes imports/types, consolidates subscription and ResizeObserver logic into shared utilities, exposes more granular hook state and RefObject types, and expands the package public exports to include many hooks and related types. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
packages/**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
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 |
gabe4coding
changed the title
Code Review: PR #77 - Simplify ui-react-builderOverviewThis PR successfully refactors the ✅ Strengths
🔍 Issues & Concerns1. Potential Race Condition in
|
| Category | Rating | Notes |
|---|---|---|
| Code Quality | ⭐⭐⭐⭐⭐ | Excellent simplification |
| Potential Bugs | Possible duplicate size notifications | |
| Performance | ✅ Improved | Fewer state updates |
| Security | ✅ No issues | N/A for this refactor |
| Test Coverage | Add tests for new helper | |
| Best Practices | ⭐⭐⭐⭐⭐ | Modern React patterns |
🎬 Recommendation
APPROVE with minor suggestions
The refactoring successfully reduces complexity while maintaining functionality. The one potential issue (duplicate size notifications) is minor and may not even manifest in practice. Consider addressing the suggestions above in a follow-up if needed.
Great work on the simplification! The code is now more maintainable and easier to understand.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/ui-react/src/hooks.ts (4)
409-416: Handler dependency can cause excessive re-subscriptions.Including
handlerin the dependency array means the effect will re-run whenever the handler reference changes. If parent components pass inline functions or recreate the handler on each render, this will cause unnecessary unsubscribe/resubscribe cycles.♻️ Recommended fix: Use a ref to stabilize the handler
export function useOnToolCancelled(handler: (reason?: string) => void): void { const { client } = useAppsContext(); + const handlerRef = useRef(handler); + + useEffect(() => { + handlerRef.current = handler; + }, [handler]); useEffect(() => { if (!client) return; - return client.onToolCancelled(handler); - }, [client, handler]); + return client.onToolCancelled((reason) => handlerRef.current(reason)); + }, [client]); }This pattern ensures the subscription remains stable while always calling the latest handler.
435-442: Handler dependency can cause excessive re-subscriptions.Same issue as
useOnToolCancelled(Line 409): thehandlerin the dependency array will trigger re-subscriptions whenever the handler reference changes.♻️ Recommended fix: Use a ref to stabilize the handler
export function useOnTeardown(handler: (reason?: string) => void): void { const { client } = useAppsContext(); + const handlerRef = useRef(handler); + + useEffect(() => { + handlerRef.current = handler; + }, [handler]); useEffect(() => { if (!client) return; - return client.onTeardown(handler); - }, [client, handler]); + return client.onTeardown((reason) => handlerRef.current(reason)); + }, [client]); }
465-472: Handler dependency can cause excessive re-subscriptions.Same issue as
useOnToolCancelled(Line 409) anduseOnTeardown(Line 435): thehandlerin the dependency array will trigger re-subscriptions whenever the handler reference changes.♻️ Recommended fix: Use a ref to stabilize the handler
export function useOnToolInputPartial(handler: (input: Record<string, unknown>) => void): void { const { client } = useAppsContext(); + const handlerRef = useRef(handler); + + useEffect(() => { + handlerRef.current = handler; + }, [handler]); useEffect(() => { if (!client) return; - return client.onToolInputPartial(handler); - }, [client, handler]); + return client.onToolInputPartial((input) => handlerRef.current(input)); + }, [client]); }
808-839: Missing ResizeObserver availability check.Unlike
useSizeChangedNotifications(Line 589), this hook doesn't verify thatResizeObserveris available before usingcreateSizeObserver. In environments whereResizeObserveris undefined, this will throw a runtime error.🛡️ Proposed fix: Add ResizeObserver availability check
useEffect(() => { const element = containerRef.current; - if (!client?.notifyIntrinsicHeight || !element) return; + if (!client?.notifyIntrinsicHeight || !element || typeof ResizeObserver === "undefined") return; const observer = createSizeObserver(element, (_width, height) => { client.notifyIntrinsicHeight?.(height); }); return () => observer.disconnect(); }, [client]);
🧹 Nitpick comments (1)
packages/ui-react/src/hooks.ts (1)
831-833: Redundant optional chaining on notifyIntrinsicHeight.Line 832 uses optional chaining (
client.notifyIntrinsicHeight?.) but Line 829 already confirms thatclient?.notifyIntrinsicHeightexists, making the optional chaining unnecessary.🔧 Optional cleanup
const observer = createSizeObserver(element, (_width, height) => { - client.notifyIntrinsicHeight?.(height); + client.notifyIntrinsicHeight(height); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/ui-react/src/context.tsxpackages/ui-react/src/hooks.tspackages/ui-react/src/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use Strict TypeScript mode: noanytypes, useunknownand narrow types instead
Remove all unused variables or prefix them with underscore (_)
Useexport typefor type-only exports
Files:
packages/ui-react/src/index.tspackages/ui-react/src/hooks.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API only through
index.tsfiles
Files:
packages/ui-react/src/index.ts
packages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
AppErrorandErrorCodefrom@mcp-apps-kit/corefor error handling
Files:
packages/ui-react/src/index.tspackages/ui-react/src/hooks.ts
🧠 Learnings (2)
📚 Learning: 2026-01-09T14:18:43.501Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:18:43.501Z
Learning: Applies to packages/core/**/*.ts : Always use `defineTool` and `defineUI` for type inference when creating tools and UI widgets
Applied to files:
packages/ui-react/src/index.ts
📚 Learning: 2026-01-09T14:18:43.501Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:18:43.501Z
Learning: Applies to **/index.ts : Export public API only through `index.ts` files
Applied to files:
packages/ui-react/src/index.ts
🧬 Code graph analysis (1)
packages/ui-react/src/context.tsx (1)
packages/ui-react/src/index.ts (1)
AppsProviderProps(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-and-lint
- GitHub Check: claude-review
🔇 Additional comments (11)
packages/ui-react/src/hooks.ts (8)
5-6: LGTM! Proper type-only import usage.The separation of
RefObjectas a type-only import follows TypeScript best practices and the coding guidelines.
21-59: Well-structured shared utilities.The extracted constants and
createSizeObserverhelper consolidate common patterns effectively, reducing duplication across the hooks.
190-202: LGTM! Cleaner subscription pattern.The direct return of the unsubscribe function from
client.onHostContextChange(setContext)simplifies the cleanup logic. The fallback toDEFAULT_HOST_CONTEXTprovides sensible defaults during initialization.
381-384: LGTM! Good use of shared constant.Using
DEFAULT_INSETSimproves consistency across the codebase.
500-517: LGTM! Simplified host capabilities hook.The direct return pattern with inline arrow function is clean and correct.
583-599: LGTM! Good ResizeObserver availability check.The hook correctly checks for
ResizeObserveravailability before usingcreateSizeObserver.
665-699: LGTM! Improved state management and error handling.The separate state variables (
isUploading,error,fileId) provide better granularity, and thefinallyblock ensures cleanup regardless of success or failure.
729-767: LGTM! Consistent with useFileUpload pattern.The separate state management and
finallyblock mirror the improvements inuseFileUpload.packages/ui-react/src/context.tsx (2)
5-6: LGTM! Clean import organization.Separating type-only imports (
ReactNode,ComponentType,JSX) from value imports follows TypeScript best practices and aligns with the coding guidelines.
98-98: LGTM! Consistent JSX type usage.Using
JSX.Elementinstead ofReact.JSX.Elementis cleaner and consistent with the direct import ofJSXfrom React.packages/ui-react/src/index.ts (1)
22-57: LGTM! Well-organized public API expansion.The expanded exports provide a comprehensive public API surface. All exported hooks and types have been verified to exist in the source files, and the organization follows the coding guidelines for index.ts files.
As per coding guidelines: Export public API only through
index.tsfiles.
1. **`useOnToolCancelled`** - Added ref pattern to stabilize handler and prevent excessive re-subscriptions 2. **`useOnTeardown`** - Same fix applied 3. **`useOnToolInputPartial`** - Same fix applied 4. **`useIntrinsicHeight`** - Added `typeof ResizeObserver === "undefined"` check to prevent runtime errors in environments without ResizeObserver All tests pass (29/29), typecheck passes, and lint passes.
Code Review - PR #77: Simplify hooks and reduce code duplicationOverviewThis is an excellent refactoring PR that significantly improves code quality in the ✅ Strengths1. Excellent Code Deduplication
2. Proper Hook StabilizationThe ref pattern in const handlerRef = useRef(handler);
useEffect(() => {
handlerRef.current = handler;
}, [handler]);
useEffect(() => {
if (\!client) return;
return client.onToolCancelled((reason) => handlerRef.current(reason));
}, [client]);This prevents excessive re-subscriptions when handler functions change identity on every render. Excellent fix! ✨ 3. Improved State ManagementThe
4. Better Type ImportsChanged to type-only imports (
5. Clean Simplifications
6. Runtime SafetyAdded 🔍 Observations & Minor Considerations1. ResizeObserver Initial Size Reporting (packages/ui-react/src/hooks.ts:55-56)The observer.observe(element);
// Report initial size
const rect = element.getBoundingClientRect();
onResize(Math.round(rect.width), Math.round(rect.height));This is generally fine, but be aware that:
Not a bug, just a behavioral note. The duplicate report is unlikely to cause issues since the values are identical. 2. Type-Only Import Consistency (packages/ui-react/src/context.tsx:5)Good improvement to use 3. Test CoverageThe PR mentions all 29 tests pass. Given the refactoring scope, this is excellent. The changes are low-risk since:
🎯 Code Quality Assessment
📋 Adherence to CLAUDE.md Guidelines✅ Strict TypeScript: No 🎉 Final VerdictAPPROVE ✅ This is a high-quality refactoring that:
The changes demonstrate excellent understanding of React hooks, proper optimization patterns, and thoughtful code organization. Recommendation: Merge with confidence! 🚀 📚 What I LearnedThe ref pattern for stabilizing event handlers is a textbook example of proper React optimization. This prevents the common mistake of creating new subscriptions on every render when handler functions change identity. Great work! 👏 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ui-react/src/hooks.ts (2)
680-714: Refactoring looks good, but error handling doesn't follow coding guidelines.The refactoring to individual state variables and the
finallyblock for cleanup are excellent improvements. However, the error handling doesn't align with the project's coding guidelines.As per coding guidelines,
packages/**/*.tsfiles should useAppErrorandErrorCodefrom@mcp-apps-kit/corefor error handling. Lines 692 and 704 create plainErrorobjects instead.♻️ Suggested refactor to use AppError
Import AppError and ErrorCode:
+import { AppError, ErrorCode } from "@mcp-apps-kit/core";Update error handling:
if (!client?.uploadFile) { - setError(new Error("File upload not supported on this platform")); + setError(new AppError("File upload not supported on this platform", ErrorCode.UNSUPPORTED_OPERATION)); return null; } // ... } catch (err) { - setError(err instanceof Error ? err : new Error(String(err))); + setError(err instanceof AppError ? err : new AppError(String(err), ErrorCode.UPLOAD_FAILED)); return null;Note: You'll also need to update the interface at line 637 from
error: Error | nulltoerror: AppError | nullfor type consistency.
744-782: Same refactoring quality and error handling concern as useFileUpload.The refactoring pattern with individual state variables and
finallyblock cleanup is well-executed. However, error handling on lines 760 and 772 should useAppErrorandErrorCodefrom@mcp-apps-kit/coreper the coding guidelines forpackages/**/*.tsfiles.♻️ Suggested refactor to align with error handling guidelines
if (!client?.getFileDownloadUrl) { - setError(new Error("File download not supported on this platform")); + setError(new AppError("File download not supported on this platform", ErrorCode.UNSUPPORTED_OPERATION)); return null; } // ... } catch (err) { - setError(err instanceof Error ? err : new Error(String(err))); + setError(err instanceof AppError ? err : new AppError(String(err), ErrorCode.DOWNLOAD_FAILED)); return null;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui-react/src/hooks.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use Strict TypeScript mode: noanytypes, useunknownand narrow types instead
Remove all unused variables or prefix them with underscore (_)
Useexport typefor type-only exports
Files:
packages/ui-react/src/hooks.ts
packages/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
AppErrorandErrorCodefrom@mcp-apps-kit/corefor error handling
Files:
packages/ui-react/src/hooks.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (6)
packages/ui-react/src/hooks.ts (6)
21-59: LGTM! Well-designed shared utilities.The new shared constants and
createSizeObserverhelper effectively reduce code duplication. The pattern of checking for ResizeObserver support at call sites (lines 604, 844) before invoking the helper is appropriate and handles environments lacking ResizeObserver safely.
190-202: LGTM! Cleaner initialization and subscription pattern.Using
DEFAULT_HOST_CONTEXTfor initialization and directly returning the unsubscribe function simplifies the code without changing behavior.
409-487: LGTM! Excellent use of ref pattern for stable subscriptions.The ref pattern in
useOnToolCancelled,useOnTeardown, anduseOnToolInputPartialprevents unnecessary re-subscriptions when handler callbacks change. This improves performance and follows React best practices for event handler stability.
515-532: LGTM! Simplified initialization and subscription.The streamlined initialization and subscription pattern improves code clarity while preserving functionality.
598-614: LGTM! Proper ResizeObserver guard and helper usage.The
typeof ResizeObserver === "undefined"check prevents runtime errors in environments lacking ResizeObserver support (e.g., server-side rendering, older browsers). The use ofcreateSizeObserverreduces duplication effectively.
823-854: LGTM! Consistent pattern with proper environment guards.The ResizeObserver guard and helper usage mirror the improvements in
useSizeChangedNotifications, ensuring consistent, safe behavior across size-tracking hooks.
2 participants
Summary
This PR simplifies the
@mcp-apps-kit/ui-reactpackage by reducing code duplication, improving state patterns, and cleaning up hook implementations while preserving all existing functionality.Changes Made
Shared Utilities Added
DEFAULT_HOST_CONTEXTandDEFAULT_INSETSconstants to eliminate duplicated inline object literalscreateSizeObserverhelper function to consolidate ResizeObserver logic used across multiple hooksHook Simplifications
useHostContext(newContext) => setContext(newContext)tosetContextuseSafeAreaInsetsuseOnToolCancelled,useOnTeardown,useOnToolInputPartialuseHostCapabilitiesuseSizeChangedNotificationsuseFileUploaduseStatecalls, derivedisSupporteddirectly, addedfinallyblockuseFileDownloaduseFileUploaduseIntrinsicHeightcreateSizeObserverhelperImport Cleanup
import type { RefObject })Other Cleanup
Impact
All functionality is preserved while improving code clarity and maintainability.
This PR was written using Vibe Kanban