-
Notifications
You must be signed in to change notification settings - Fork 17
feat: added diff to debuggerPanel #175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR adds comprehensive diff visualization functionality to the Cedar OS debugger panel. The changes introduce a sophisticated state diffing system that allows developers to visualize changes between old and new states, compare computed vs registered states, and track state synchronization issues.
The core implementation includes:
- New diff history integration: The debugger panel now extracts
diffHistoryStatesfrom the Cedar store and passes it to theStatesTabcomponent, enabling real-time diff visualization - Enhanced StatesTab component: Adds extensive diff rendering capabilities with multiple view modes (unified, side-by-side, word-level) using the
difflibrary and lodash for deep equality checks - State equivalence checking: Implements automatic correction of the
isDiffModeflag whenoldStateandnewStatebecome equivalent, preventing the system from getting stuck in diff mode - New debugger components: Adds
NetworkTab,MessagesTab, andCollapsibleSectioncomponents to create a comprehensive debugging interface - Dependency additions: Introduces
lodash(^4.17.21) anddiff(^7.0.0) libraries to support state comparison and text diffing functionality
The changes also include proper database file exclusions in .gitignore files for the Mastra SQLite databases used in the examples-backend, and removes legacy diff management functions from the product roadmap example to centralize diff functionality in the debugger.
The system integrates deeply with Cedar OS's existing architecture, building on the DiffHistorySlice state management and extending the debugger to provide developers with powerful tools for understanding how AI agents and other automated processes modify application state.
Confidence score: 3/5
- This PR introduces significant complexity with potential integration issues across multiple packages
- Score reflects concerns about Framer Motion import violations, code duplication, and complex diff rendering logic
- Pay close attention to packages/cedar-os/src/components/debugger/MessagesTab.tsx and packages/cedar-os/src/components/debugger/CollapsibleSection.tsx
18 files reviewed, 12 comments
| examples-backend/mastra.db-shm | ||
| examples-backend/mastra.db-wal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Duplicate entries for .db-shm and .db-wal files (already listed on lines 47-48)
|
|
||
| export interface StatesTabProps extends TabProps { | ||
| states: Record<string, registeredState>; | ||
| diffStates?: Record<string, any>; // DiffHistoryState from cedar-os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using a more specific type instead of any. If DiffHistoryState can't be imported here, consider defining a local interface with the required properties.
|
|
||
| export interface StatesTabProps extends TabProps { | ||
| states: Record<string, registeredState>; | ||
| diffStates?: Record<string, any>; // DiffHistoryState from cedar-os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Using any type loses type safety. Consider importing and using the actual DiffHistoryState type from the diffHistorySlice instead of using any.
| const testState: DiffHistoryState<ComplexData> = { | ||
| diffState: { | ||
| oldState: complexObject, | ||
| newState: { ...complexObject }, // Deep copy to ensure different references but same content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Shallow spread { ...complexObject } doesn't create a deep copy as the comment suggests. Consider using cloneDeep for true deep copying
| {log.data.params && ( | ||
| <CollapsibleSection | ||
| id={`${log.id}-context`} | ||
| title='Additional Context' | ||
| isExpanded={expandedSections.has(`${log.id}-context`)} | ||
| onToggle={() => toggleSection(`${log.id}-context`)} | ||
| badges={[]}> | ||
| <pre className='text-xs bg-gray-100 dark:bg-gray-800 p-2 rounded overflow-x-auto'> | ||
| {safeStringify(log.data.params)} | ||
| </pre> | ||
| </CollapsibleSection> | ||
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: This 'Additional Context' section appears to duplicate the same data that's shown in the 'Request' section below (lines 294-326). Consider removing this redundant section.
| const jsonDiff = diffJson( | ||
| oldObj as string | object, | ||
| newObj as string | object | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Type assertion without proper validation - diffJson expects string | object but oldObj and newObj are unknown. Consider adding type guards.
| const handleCopy = (text: string, id: string) => { | ||
| navigator.clipboard.writeText(text); | ||
| setCopiedId(id); | ||
| setTimeout(() => setCopiedId(null), 2000); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Using navigator.clipboard without checking for availability could throw errors in some browsers or contexts. Consider adding a try-catch or feature detection.
| @@ -0,0 +1,219 @@ | |||
| import React, { useState } from 'react'; | |||
| import { motion, AnimatePresence } from 'framer-motion'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Import should be from 'motion/react' not 'framer-motion' per project rules
| import { motion, AnimatePresence } from 'framer-motion'; | |
| import { motion, AnimatePresence } from 'motion/react'; |
| if (!threadMap.has(threadId)) { | ||
| threadMap.set(threadId, []); | ||
| } | ||
| threadMap.get(threadId)!.push(msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Non-null assertion could throw if threadMap.get returns undefined - consider safer access pattern
| @@ -0,0 +1,74 @@ | |||
| import React from 'react'; | |||
| import { motion, AnimatePresence } from 'framer-motion'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Import violates project rules - should be motion/react instead of framer-motion
| import { motion, AnimatePresence } from 'framer-motion'; | |
| import { motion, AnimatePresence } from 'motion/react'; |
No description provided.