Conversation
Refactors the `Slides` component into a more maintainable and composable architecture. This commit introduces: - A new `useSlidesState` hook to centralize all slide state management, including active slide, animation direction, and collapse logic. - `ButtonRow` component for rendering slide navigation buttons, supporting both connected and disconnected styles. - `SlidesHeader` and `MinimalSlidesHeader` components to encapsulate header and button row rendering for default and minimal variants, respectively. - `SlideContent` component (implied for content rendering) to handle slide transitions and collapse animations using Framer Motion. - Extraction of Framer Motion variants into `animations.ts` for better organization and improved animation handling (e.g., `position: absolute` for transitions). - Dedicated `types.ts` file for component-specific types. This refactoring significantly improves the separation of concerns, making the `Slides` component easier to understand, maintain, and extend. It also lays the groundwork for more robust and dynamic slide content and collapse animations.
* feat(multiple): Introduce SlideContent component and enhance Docker client host tracking This commit introduces a new `SlideContent` component for animated UI transitions and enhances the Docker client manager's ability to track host IDs. - **UI Components:** - Added a new `SlideContent` component to `@ui/components/Slides` that utilizes `framer-motion` for animated transitions between different content slides, improving user experience in dynamic interfaces. - Re-exported the `Column` type from `@ui/components/Table` to facilitate easier type imports for consumers. - **Docker Client Manager:** - Implemented `internalListeners` in `DockerClientManagerCore` to proactively manage `WorkerWrapper`'s `hostIds` set. - This new mechanism tracks `host:init`, `host:added`, and `host:removed` events, ensuring the `hostIds` set accurately reflects connected hosts. - The `host:init` event type has been added to `@typings/docker-client`. - **Dockstat Application:** - Refactored `WorkersTable` by removing redundant explicit type annotations from `render` function parameters, leveraging TypeScript's type inference. - Explicit type assertions (`as Type`) were introduced internally within the `render` functions for clarity and type safety where inference was not sufficient. * chore(ci): Lint [skip ci] * Update packages/docker-client/src/manager/core.ts Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: ItsNik <info@itsnik.de> * Update packages/ui/src/components/Slides/SlideContent.tsx Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: ItsNik <info@itsnik.de> * chore(ci): Lint [skip ci] * refactor(docker-client): Remove 'host:init' event and clarify host lifecycle logging The 'host:init' event type has been removed from `DockerClientEvents` and its handling logic from `DockerClientManagerCore`. This simplifies the event API and ensures consistency with host lifecycle management. Additionally, debug logging for 'host:added' and 'host:removed' events has been enhanced to include the specific host ID and the exact Set operation (`.add()` or `.delete()`) being performed, improving observability during host lifecycle changes. --------- Signed-off-by: ItsNik <info@itsnik.de> Co-authored-by: actions-user <its4nik@users.noreply.github.com> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideRefactors the Slides UI component into smaller composable pieces with a shared slide-state hook and updated animation model, adds internal event handling for host add/remove in the Docker client manager, slightly adjusts host tracking in Hosts, loosens table cell render typings and re-exports Column, and makes minor typings/logging tweaks across the app. Sequence diagram for slide change handling and animationsequenceDiagram
actor User
participant ButtonRow
participant Slides
participant useSlidesState
participant SlideContent
participant FramerMotion
User->>ButtonRow: Click slide button (key)
ButtonRow->>useSlidesState: changeSlide(key)
useSlidesState->>useSlidesState: Determine hideable, collapsed, index
alt hideable and key is activeSlide
useSlidesState->>useSlidesState: set isCollapsed = true
useSlidesState->>useSlidesState: set internalSlide = null (uncontrolled)
useSlidesState-->>Slides: onSlideChange(null)
else expanding from collapsed or no active
useSlidesState->>useSlidesState: set isCollapsed = false
useSlidesState->>useSlidesState: set animationDirection = 1
useSlidesState->>useSlidesState: set internalSlide = key (uncontrolled)
useSlidesState-->>Slides: onSlideChange(key)
else switching between slides
useSlidesState->>useSlidesState: compute newIndex, currentIndex
useSlidesState->>useSlidesState: set animationDirection
useSlidesState->>useSlidesState: update internalSlide
useSlidesState-->>Slides: onSlideChange(key)
end
Slides->>SlideContent: Render with updated state
SlideContent->>SlideContent: hasContent and show flags
SlideContent->>FramerMotion: motion.div with collapseVariants(contentHeight)
FramerMotion-->>User: Container expand/collapse animation
par Active slide transition
SlideContent->>FramerMotion: motion.div with slideVariants(animationDirection)
FramerMotion-->>User: Slide-in / slide-out horizontal animation
and Height measurement
SlideContent->>SlideContent: measure active slide DOM height
SlideContent->>useSlidesState: setContentHeight(height)
end
Sequence diagram for Docker host event processingsequenceDiagram
participant DockerWorker as DockerWorker
participant DockerClientManagerCore as DockerClientManagerCore
participant internalListeners as internalListeners
participant WorkerWrapper as WorkerWrapper
participant PluginHooks as PluginHooks
DockerWorker-->>DockerClientManagerCore: EventMessage(type host:added, ctx)
DockerClientManagerCore->>DockerClientManagerCore: listener(message)
alt message is defined
DockerClientManagerCore->>internalListeners: internalListeners(wrapper, message)
alt type is host:added
internalListeners->>WorkerWrapper: wrapper.hostIds.add(hostId)
internalListeners-->>DockerClientManagerCore: updated wrapper
else type is host:removed
internalListeners->>WorkerWrapper: wrapper.hostIds.delete(hostId)
internalListeners-->>DockerClientManagerCore: updated wrapper
end
DockerClientManagerCore->>PluginHooks: triggerHooks(message)
PluginHooks-->>DockerClientManagerCore: Execute server hooks
else message is undefined
DockerClientManagerCore-->>DockerWorker: ignore
end
Class diagram for refactored Slides UI componentsclassDiagram
class SlidesProps {
+Record~string, React.ReactNode~ children
+string header
+string description
+ButtonRowPosition buttonPosition
+boolean connected
+string defaultSlide
+string selectedSlide
+string controlledSlide
+function onSlideChange(slideKey string|null)
+boolean hideable
+SlideVariant variant
+string className
}
class UseSlideReturn {
+string[] slideKeys
+string activeSlide
+number animationDirection
+boolean isCollapsed
+number contentHeight
+Record~string, HTMLDivElement~ contentRefs
+boolean isControlled
+function changeSlide(newSlide string)
}
class Slides {
+function Slides(props SlidesProps)
}
class useSlidesState {
+function useSlidesState(children Record~string, React.ReactNode~, controlledSlide string, defaultSlide string, hideable boolean, onSlideChange function)
}
class SlideContent {
+function SlideContent(state UseSlideReturn, children Record~string, React.ReactNode~)
}
class SlidesHeader {
+function SlidesHeader(header string, description string, buttonPosition ButtonRowPosition, connected boolean, state UseSlideReturn)
}
class MinimalSlidesHeader {
+function MinimalSlidesHeader(header string, description string, connected boolean, state UseSlideReturn)
}
class ButtonRow {
+function ButtonRow(slideKeys string[], activeSlide string, isCollapsed boolean, connected boolean, onSlideChange function, wrapperClass string)
}
class animations {
+function slideVariants(direction number)
+function collapseVariants(contentHeight number)
}
Slides --> SlidesProps
Slides --> UseSlideReturn : uses
Slides ..> useSlidesState : calls
Slides ..> SlidesHeader : renders
Slides ..> MinimalSlidesHeader : renders
Slides ..> SlideContent : renders
SlidesHeader --> ButtonRow : uses
MinimalSlidesHeader --> ButtonRow : uses
SlideContent --> UseSlideReturn : uses
SlideContent ..> animations : uses
useSlidesState --> SlidesProps : derives configuration
ButtonRow --> UseSlideReturn : consumes subset
class ButtonRowPosition {
}
class SlideVariant {
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
SlideContent,slideVariantsis called withstate.contentHeight, but theslideVariantshelper expects a direction (e.g.state.animationDirection), which will cause the animation to behave incorrectly. - The
contentHeightinuseSlidesStateis only measured onactiveSlidechanges, so if the content of a slide changes height while it remains active (e.g. async data loads), the container height and animation may not update; consider using aResizeObserveror recalculating when content changes. - The
SlidesPropstype exposes bothselectedSlideandcontrolledSlide, which can be confusing for consumers; it would be clearer to expose a single controlled prop and keep any aliasing internal to theSlidescomponent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SlideContent`, `slideVariants` is called with `state.contentHeight`, but the `slideVariants` helper expects a direction (e.g. `state.animationDirection`), which will cause the animation to behave incorrectly.
- The `contentHeight` in `useSlidesState` is only measured on `activeSlide` changes, so if the content of a slide changes height while it remains active (e.g. async data loads), the container height and animation may not update; consider using a `ResizeObserver` or recalculating when content changes.
- The `SlidesProps` type exposes both `selectedSlide` and `controlledSlide`, which can be confusing for consumers; it would be clearer to expose a single controlled prop and keep any aliasing internal to the `Slides` component.
## Individual Comments
### Comment 1
<location> `packages/ui/src/components/Slides/SlideContent.tsx:13` </location>
<code_context>
+ children: Record<string, React.ReactNode>
+}) => {
+ const activeKey = state.activeSlide
+ const hasContent = activeKey in children && children[activeKey] != null
+ const show = hasContent || !state.isCollapsed
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `activeSlide` being null before using the `in` operator to avoid runtime errors.
If `state.activeSlide` is `null`, `activeKey in children` will throw a `TypeError` because the left-hand side of `in` cannot be `null` or `undefined`. Consider a pattern like:
```ts
const hasContent =
activeKey != null &&
Object.prototype.hasOwnProperty.call(children, activeKey) &&
children[activeKey] != null
```
and derive `show` from `hasContent` to avoid a possible runtime crash while preserving the current behavior.
</issue_to_address>
### Comment 2
<location> `packages/ui/src/components/Slides/SlideContent.tsx:19-28` </location>
<code_context>
+ return (
+ <AnimatePresence initial={false}>
+ {show && (
+ <motion.div
+ initial="collapsed"
+ animate="expanded"
+ exit="collapsed"
+ variants={collapseVariants(state.contentHeight)}
+ transition={{ duration: 0.3, ease: [0.4, 0, 0.2, 1] }}
+ className="overflow-hidden"
+ >
+ <div className="relative">
+ <AnimatePresence initial={false} custom={state.animationDirection}>
+ {state.activeSlide && !state.isCollapsed && (
+ <motion.div
+ key={state.activeSlide}
+ custom={state.animationDirection}
+ variants={slideVariants(state.contentHeight)}
+ initial="enter"
+ animate="center"
</code_context>
<issue_to_address>
**issue (bug_risk):** The `slideVariants` helper is being called with `contentHeight` instead of a direction, which breaks the `custom`-driven animation logic.
Previously, `slideVariants` consumed `animationDirection` via Framer Motion’s `custom` prop. After the refactor it’s typed as `(direction: number): Variants` but is now called with `state.contentHeight` while `custom={state.animationDirection}` remains on the `motion.div`.
This means:
- The argument passed to `slideVariants` is a height, not a direction, so the slide direction can be incorrect or inconsistent.
- The `custom` value is effectively unused, so changing `animationDirection` no longer affects the animation.
To fix this, either:
- Pass `state.animationDirection` into `slideVariants` and remove reliance on `custom`, or
- Restore the functional variants pattern where `slideVariants` uses `direction` via the `custom` prop, so `animationDirection` again drives the slide direction.
</issue_to_address>
### Comment 3
<location> `packages/ui/src/components/Slides/useSlideState.ts:21` </location>
<code_context>
- const [internalSlide, setInternalSlide] = useState<string | null>(initialSlide)
- const [animationDirection, setAnimationDirection] = useState<1 | -1>(1)
- const [isCollapsed, setIsCollapsed] = useState(hideable)
- const previousSlideIndex = useRef(slideKeys.indexOf(initialSlide || ""))
-
- const isControlled = controlledSlide !== undefined
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the unused `previousSlideIndex` ref and narrowing the hook’s returned API to only the fields and helpers that consumers actually need to reduce mental overhead.
You can trim a bit of complexity without changing behaviour:
1. **Remove `previousSlideIndex` (it's write-only)**
`previousSlideIndex` is never read, only assigned in `changeSlide`, so it can be safely removed:
```ts
// Before
const previousSlideIndex = useRef(slideKeys.indexOf(initialSlide || ""));
const changeSlide = (newSlide: string) => {
// ...
const newIndex = slideKeys.indexOf(newSlide);
const currentIndex = slideKeys.indexOf(activeSlide);
setAnimationDirection(newIndex > currentIndex ? 1 : -1);
previousSlideIndex.current = currentIndex;
if (!isControlled) {
setInternalSlide(newSlide);
}
onSlideChange?.(newSlide);
};
// After
const changeSlide = (newSlide: string) => {
// ...
const newIndex = slideKeys.indexOf(newSlide);
const currentIndex = slideKeys.indexOf(activeSlide);
setAnimationDirection(newIndex > currentIndex ? 1 : -1);
if (!isControlled) {
setInternalSlide(newSlide);
}
onSlideChange?.(newSlide);
};
```
And delete the `useRef` line entirely.
2. **Narrow the hook API to hide internals where possible**
If `Slides.tsx` doesn’t need direct access to some internals (e.g. `isControlled`, `contentRefs`, `contentHeight`), you can keep them inside the hook and expose smaller, intent-focused primitives. For example:
```ts
// Before
return {
slideKeys,
activeSlide,
animationDirection,
isCollapsed,
contentHeight,
contentRefs,
isControlled,
changeSlide,
};
// After (example: only if callers don't need the removed fields)
return {
slideKeys,
activeSlide,
animationDirection,
isCollapsed,
changeSlide,
};
```
If you still need refs for measurement, consider exposing a single `getContentRef` callback instead of the whole `contentRefs` map:
```ts
const contentRefs = useRef<Record<string, HTMLDivElement | null>>({});
const getContentRef = (key: string) => (el: HTMLDivElement | null) => {
contentRefs.current[key] = el;
};
return {
slideKeys,
activeSlide,
animationDirection,
isCollapsed,
contentHeight,
getContentRef,
changeSlide,
};
```
This keeps the same functionality but reduces the surface area and mental load for consumers.
</issue_to_address>
### Comment 4
<location> `packages/ui/src/components/Slides/SlideContent.tsx:9` </location>
<code_context>
+ header?: string
+ description?: string
+ connected: boolean
+ state: UseSlideReturn
+}
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider narrowing the `SlideContent` props to a small, explicit state shape and delegating ref management to the hook to reduce coupling and cognitive load.
You can reduce coupling and cognitive load by narrowing the props and hiding the ref/measurement details behind a small API on the hook, instead of passing the entire `UseSlideReturn` and mutating `contentRefs` in the component.
For example, in your hook, expose only what the UI actually needs:
```ts
// useSlideState.ts
export type SlideState = {
activeKey: string | null
isCollapsed: boolean
animationDirection: 1 | -1
contentHeight: number | null
setContentRef: (key: string, el: HTMLDivElement | null) => void
}
export const useSlideState = (...): SlideState => {
const contentRefs = useRef<Record<string, HTMLDivElement | null>>({})
const [contentHeight, setContentHeight] = useState<number | null>(null)
const setContentRef = (key: string, el: HTMLDivElement | null) => {
contentRefs.current[key] = el
// existing measurement logic using contentRefs + setContentHeight
}
return {
activeKey: activeSlide,
isCollapsed,
animationDirection,
contentHeight,
setContentRef,
}
}
```
Then update `SlideContent` to depend only on these primitives, not the full hook state shape or its internal refs:
```tsx
import { AnimatePresence, motion } from "framer-motion"
import { collapseVariants, slideVariants } from "./animations"
import type { SlideState } from "./useSlideState"
export const SlideContent = ({
state: { activeKey, isCollapsed, animationDirection, contentHeight, setContentRef },
children,
}: {
state: SlideState
children: Record<string, React.ReactNode>
}) => {
const hasContent = activeKey != null && activeKey in children && children[activeKey] != null
const show = hasContent || !isCollapsed
return (
<AnimatePresence initial={false}>
{show && (
<motion.div
initial="collapsed"
animate="expanded"
exit="collapsed"
variants={collapseVariants(contentHeight)}
transition={{ duration: 0.3, ease: [0.4, 0, 0.2, 1] }}
className="overflow-hidden"
>
<div className="relative">
<AnimatePresence initial={false} custom={animationDirection}>
{activeKey && !isCollapsed && (
<motion.div
key={activeKey}
custom={animationDirection}
variants={slideVariants(contentHeight)}
initial="enter"
animate="center"
exit="exit"
transition={{
x: { type: "spring", stiffness: 300, damping: 30 },
}}
>
<div ref={(el) => setContentRef(activeKey, el)}>
{children[activeKey]}
</div>
</motion.div>
)}
</AnimatePresence>
</div>
</motion.div>
)}
</AnimatePresence>
)
}
```
This keeps all existing behavior (measured height, variants parameterization, nested `AnimatePresence`) but:
- Removes direct mutation of `contentRefs.current` from the UI layer.
- Makes `SlideContent` easier to understand by taking a small, explicit state shape.
- Localizes the knowledge of how refs are stored and used for measurement inside the hook.
</issue_to_address>
### Comment 5
<location> `packages/ui/src/components/Slides/animations.ts:3` </location>
<code_context>
+import type { Variants } from "framer-motion"
+
+const slideVariants = (direction: number): Variants => {
+ return {
+ enter: {
</code_context>
<issue_to_address>
**issue (complexity):** Consider tightening the function APIs by using a typed slide direction and an optional content height with defaults to make the animation helpers harder to misuse and clearer at call sites.
You can reduce the risk of mis‑wiring and make the abstractions clearer with stronger typing and safer defaults, without removing the new module.
**1. Make slide direction explicit and hard to misuse**
Constrain `direction` to the intended domain and make its meaning obvious:
```ts
type SlideDirection = 1 | -1;
const slideVariants = (direction: SlideDirection): Variants => ({
enter: {
x: direction > 0 ? "100%" : "-100%",
position: "absolute" as const,
top: 0,
left: 0,
right: 0,
},
center: {
x: 0,
position: "relative" as const,
},
exit: {
x: direction > 0 ? "-100%" : "100%",
position: "absolute" as const,
top: 0,
left: 0,
right: 0,
},
});
```
You can also add a small helper to make call sites self‑documenting:
```ts
export const SLIDE_FORWARD: SlideDirection = 1;
export const SLIDE_BACKWARD: SlideDirection = -1;
// usage:
// slideVariants(SLIDE_FORWARD)
```
This makes it much harder for someone to accidentally pass a height or other numeric value.
**2. Make `contentHeight` optional with a sane default**
Keep the ability to use a measured height, but don’t require it at every call site; default to `"auto"` to preserve the simpler behaviour:
```ts
const collapseVariants = (contentHeight?: number): Variants => ({
collapsed: {
height: 0,
opacity: 0,
},
expanded: {
height: contentHeight ?? "auto",
opacity: 1,
},
});
```
This keeps your current functionality (precise height when provided) while making usage simpler and less coupled to measurement logic in places that don’t need it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@sourcery-ai resolve |
- Refactor `slideVariants` to utilize Framer Motion's `custom` prop for passing animation direction, simplifying its usage in `SlideContent`. - Introduce `mode="popLayout"` to `AnimatePresence` for more robust and smoother transitions, ensuring better layout management during animations. - Enhance the `hasContent` check in `SlideContent.tsx` to explicitly account for `null` `activeKey` and use `Object.hasOwn` for precise property validation. - Implement null safety when updating `contentRefs.current`, preventing potential runtime errors if `activeSlide` is `null`. - Remove the redundant `previousSlideIndex` ref from `useSlideState`, streamlining state management.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Refactor the Slides UI component into smaller, reusable pieces with improved slide animation behavior and internal state management, while adding internal host tracking for Docker client events and tightening typings and exports across UI and Docker client modules.
New Features:
Bug Fixes:
Enhancements: