unify card/modal
This commit is contained in:
teernisse
382
plans/card-modal-unification.md
Normal file
382
plans/card-modal-unification.md
Normal file
@@ -0,0 +1,382 @@
|
||||
# Card/Modal Unification Plan
|
||||
|
||||
**Status:** Implemented
|
||||
**Date:** 2026-02-26
|
||||
**Author:** Claude + Taylor
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
Unify SessionCard and Modal into a single component with an `enlarged` prop, eliminating 165 lines of duplicated code and ensuring feature parity across both views.
|
||||
|
||||
---
|
||||
|
||||
## 1. Problem Statement
|
||||
|
||||
### 1.1 What's Broken
|
||||
|
||||
The AMC dashboard displays agent sessions as cards in a grid. Clicking a card opens a "modal" for a larger, focused view. These two views evolved independently, creating:
|
||||
|
||||
| Issue | Impact |
|
||||
|-------|--------|
|
||||
| **Duplicated rendering logic** | Modal.js reimplemented header, chat, input from scratch (227 lines) |
|
||||
| **Feature drift** | Card had context usage display; modal didn't. Modal had timestamps; card didn't. |
|
||||
| **Maintenance burden** | Every card change required parallel modal changes (often forgotten) |
|
||||
| **Inconsistent UX** | Users see different information depending on view |
|
||||
|
||||
### 1.2 Why This Matters
|
||||
|
||||
The modal's purpose is simple: **show an enlarged view with more screen space for content**. It should not be a separate implementation with different features. Users clicking a card expect to see *the same thing, bigger* — not a different interface.
|
||||
|
||||
### 1.3 Root Cause
|
||||
|
||||
The modal was originally built as a separate component because it needed:
|
||||
- Backdrop blur with click-outside-to-close
|
||||
- Escape key handling
|
||||
- Body scroll lock
|
||||
- Entrance/exit animations
|
||||
|
||||
These concerns led developers to copy-paste card internals into the modal rather than compose them.
|
||||
|
||||
---
|
||||
|
||||
## 2. Goals and Non-Goals
|
||||
|
||||
### 2.1 Goals
|
||||
|
||||
1. **Zero duplicated rendering code** — Single source of truth for how sessions display
|
||||
2. **Automatic feature parity** — Any card change propagates to modal without extra work
|
||||
3. **Preserve modal behaviors** — Backdrop, escape key, animations, scroll lock
|
||||
4. **Add missing features to both views** — Smart scroll, sending state feedback
|
||||
|
||||
### 2.2 Non-Goals
|
||||
|
||||
- Changing the visual design of either view
|
||||
- Adding new features beyond parity + smart scroll + sending state
|
||||
- Refactoring other components
|
||||
|
||||
---
|
||||
|
||||
## 3. User Workflows
|
||||
|
||||
### 3.1 Current User Journey
|
||||
|
||||
```
|
||||
User sees session cards in grid
|
||||
│
|
||||
├─► Card shows: status, agent, cwd, context usage, last 20 messages, input
|
||||
│
|
||||
└─► User clicks card
|
||||
│
|
||||
└─► Modal opens with DIFFERENT layout:
|
||||
- Combined status badge (dot inside)
|
||||
- No context usage
|
||||
- All messages with timestamps
|
||||
- Different input implementation
|
||||
- Keyboard hints shown
|
||||
```
|
||||
|
||||
### 3.2 Target User Journey
|
||||
|
||||
```
|
||||
User sees session cards in grid
|
||||
│
|
||||
├─► Card shows: status, agent, cwd, context usage, last 20 messages, input
|
||||
│
|
||||
└─► User clicks card
|
||||
│
|
||||
└─► Modal opens with SAME card, just bigger:
|
||||
- Identical header layout
|
||||
- Context usage visible
|
||||
- All messages (not limited to 20)
|
||||
- Same input components
|
||||
- Same everything, more space
|
||||
```
|
||||
|
||||
### 3.3 User Benefits
|
||||
|
||||
| Benefit | Rationale |
|
||||
|---------|-----------|
|
||||
| **Cognitive consistency** | Same information architecture in both views reduces learning curve |
|
||||
| **Trust** | No features "hiding" in one view or the other |
|
||||
| **Predictability** | Click = zoom, not "different interface" |
|
||||
|
||||
---
|
||||
|
||||
## 4. Design Decisions
|
||||
|
||||
### 4.1 Architecture: Shared Component with Prop
|
||||
|
||||
**Decision:** Add `enlarged` prop to SessionCard. Modal renders `<SessionCard enlarged={true} />`.
|
||||
|
||||
**Alternatives Considered:**
|
||||
|
||||
| Alternative | Rejected Because |
|
||||
|-------------|------------------|
|
||||
| Modal wraps Card with CSS transform | Breaks layout, accessibility issues, can't change message limit |
|
||||
| Higher-order component | Unnecessary complexity for single boolean difference |
|
||||
| Render props pattern | Overkill, harder to read |
|
||||
| Separate "CardContent" extracted | Still requires prop to control limit, might as well be on SessionCard |
|
||||
|
||||
**Rationale:** A single boolean prop is the simplest solution that achieves all goals. The `enlarged` prop controls exactly two things: container sizing and message limit. Everything else is identical.
|
||||
|
||||
---
|
||||
|
||||
### 4.2 Message Limit: Card 20, Enlarged All
|
||||
|
||||
**Decision:** Card shows last 20 messages. Enlarged view shows all.
|
||||
|
||||
**Rationale:**
|
||||
- Cards in a grid need bounded height for visual consistency
|
||||
- 20 messages is enough context without overwhelming the card
|
||||
- Enlarged view exists specifically to see more — no artificial limit makes sense
|
||||
- Implementation: `limit` prop on ChatMessages (20 default, null for unlimited)
|
||||
|
||||
---
|
||||
|
||||
### 4.3 Header Layout: Keep Card's Multi-Row Style
|
||||
|
||||
**Decision:** Use the card's multi-row header layout for both views.
|
||||
|
||||
**Modal had:** Single row with combined status badge (dot inside badge)
|
||||
**Card had:** Multi-row with separate dot, status badge, agent badge, cwd badge, context usage
|
||||
|
||||
**Rationale:**
|
||||
- Card layout shows more information (context usage was missing from modal)
|
||||
- Multi-row handles overflow gracefully with `flex-wrap`
|
||||
- Consistent with the "modal = bigger card" philosophy
|
||||
|
||||
---
|
||||
|
||||
### 4.4 Spacing: Keep Tighter (Card Style)
|
||||
|
||||
**Decision:** Use card's tighter spacing (`px-4 py-3`, `space-y-2.5`) for both views.
|
||||
|
||||
**Modal had:** Roomier spacing (`px-5 py-4`, `space-y-4`)
|
||||
|
||||
**Rationale:**
|
||||
- Tighter spacing is more information-dense
|
||||
- Enlarged view gains space from larger container, not wider margins
|
||||
- Consistent visual rhythm between views
|
||||
|
||||
---
|
||||
|
||||
### 4.5 Empty State Text: "No messages to show"
|
||||
|
||||
**Decision:** Standardize on "No messages to show" (neither original).
|
||||
|
||||
**Card had:** "No messages yet"
|
||||
**Modal had:** "No conversation messages"
|
||||
|
||||
**Rationale:** "No messages to show" is neutral and accurate — doesn't imply timing ("yet") or specific terminology ("conversation").
|
||||
|
||||
---
|
||||
|
||||
## 5. Implementation Details
|
||||
|
||||
### 5.1 SessionCard.js Changes
|
||||
|
||||
```
|
||||
BEFORE: SessionCard({ session, onClick, conversation, onFetchConversation, onRespond, onDismiss })
|
||||
AFTER: SessionCard({ session, onClick, conversation, onFetchConversation, onRespond, onDismiss, enlarged = false })
|
||||
```
|
||||
|
||||
**New behaviors controlled by `enlarged`:**
|
||||
|
||||
| Aspect | `enlarged=false` (card) | `enlarged=true` (modal) |
|
||||
|--------|-------------------------|-------------------------|
|
||||
| Container classes | `h-[850px] w-[600px] cursor-pointer hover:...` | `max-w-5xl max-h-[90vh]` |
|
||||
| Click handler | `onClick(session)` | `undefined` (no-op) |
|
||||
| Message limit | 20 | null (all) |
|
||||
|
||||
**New feature: Smart scroll tracking**
|
||||
|
||||
```js
|
||||
// Track if user is at bottom
|
||||
const wasAtBottomRef = useRef(true);
|
||||
|
||||
// On scroll, update tracking
|
||||
const handleScroll = () => {
|
||||
wasAtBottomRef.current = el.scrollHeight - el.scrollTop - el.clientHeight < 50;
|
||||
};
|
||||
|
||||
// On new messages, only scroll if user was at bottom
|
||||
if (hasNewMessages && wasAtBottomRef.current) {
|
||||
el.scrollTop = el.scrollHeight;
|
||||
}
|
||||
```
|
||||
|
||||
**Rationale:** Users reading history shouldn't be yanked to bottom when new messages arrive. Only auto-scroll if they were already at the bottom (watching live updates).
|
||||
|
||||
---
|
||||
|
||||
### 5.2 Modal.js Changes
|
||||
|
||||
**Before:** 227 lines reimplementing header, chat, input, scroll, state management
|
||||
|
||||
**After:** 62 lines — backdrop wrapper only
|
||||
|
||||
```js
|
||||
export function Modal({ session, conversations, onClose, onRespond, onFetchConversation, onDismiss }) {
|
||||
// Closing animation state
|
||||
// Body scroll lock
|
||||
// Escape key handler
|
||||
|
||||
return html`
|
||||
<div class="backdrop...">
|
||||
<${SessionCard}
|
||||
session=${session}
|
||||
conversation=${conversations[session.session_id] || []}
|
||||
onFetchConversation=${onFetchConversation}
|
||||
onRespond=${onRespond}
|
||||
onDismiss=${onDismiss}
|
||||
onClick=${() => {}}
|
||||
enlarged=${true}
|
||||
/>
|
||||
</div>
|
||||
`;
|
||||
}
|
||||
```
|
||||
|
||||
**Preserved behaviors:**
|
||||
- Backdrop blur (`bg-[#02050d]/84 backdrop-blur-sm`)
|
||||
- Click outside to close
|
||||
- Escape key handler
|
||||
- Body scroll lock (`document.body.style.overflow = 'hidden'`)
|
||||
- Entrance/exit animations (CSS classes)
|
||||
|
||||
---
|
||||
|
||||
### 5.3 ChatMessages.js Changes
|
||||
|
||||
```
|
||||
BEFORE: ChatMessages({ messages, status })
|
||||
AFTER: ChatMessages({ messages, status, limit = 20 })
|
||||
```
|
||||
|
||||
**Logic change:**
|
||||
```js
|
||||
// Before: always slice to 20
|
||||
const displayMessages = allDisplayMessages.slice(-20);
|
||||
|
||||
// After: respect limit prop
|
||||
const displayMessages = limit ? allDisplayMessages.slice(-limit) : allDisplayMessages;
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 5.4 SimpleInput.js / QuestionBlock.js Changes
|
||||
|
||||
**New feature: Sending state feedback**
|
||||
|
||||
```js
|
||||
const [sending, setSending] = useState(false);
|
||||
|
||||
const handleSubmit = async (e) => {
|
||||
if (sending) return;
|
||||
setSending(true);
|
||||
try {
|
||||
await onRespond(...);
|
||||
} finally {
|
||||
setSending(false);
|
||||
}
|
||||
};
|
||||
|
||||
// In render:
|
||||
<button disabled=${sending}>
|
||||
${sending ? 'Sending...' : 'Send'}
|
||||
</button>
|
||||
```
|
||||
|
||||
**Rationale:** Users need feedback that their message is being sent. Without this, they might click multiple times or think the UI is broken.
|
||||
|
||||
---
|
||||
|
||||
### 5.5 App.js Changes
|
||||
|
||||
**Removed (unused after refactor):**
|
||||
- `conversationLoading` state — was only passed to Modal
|
||||
- `refreshConversation` callback — was only used by Modal's custom send handler
|
||||
|
||||
**Modified:**
|
||||
- `respondToSession` now refreshes conversation immediately after successful send
|
||||
- Modal receives same props as SessionCard (onRespond, onFetchConversation, onDismiss)
|
||||
|
||||
---
|
||||
|
||||
## 6. Dependency Graph
|
||||
|
||||
```
|
||||
App.js
|
||||
│
|
||||
├─► SessionCard (in grid)
|
||||
│ ├─► ChatMessages (limit=20)
|
||||
│ │ └─► MessageBubble
|
||||
│ ├─► QuestionBlock (with sending state)
|
||||
│ │ └─► OptionButton
|
||||
│ └─► SimpleInput (with sending state)
|
||||
│
|
||||
└─► Modal (backdrop wrapper)
|
||||
└─► SessionCard (enlarged=true)
|
||||
├─► ChatMessages (limit=null)
|
||||
│ └─► MessageBubble
|
||||
├─► QuestionBlock (with sending state)
|
||||
│ └─► OptionButton
|
||||
└─► SimpleInput (with sending state)
|
||||
```
|
||||
|
||||
**Key insight:** Modal no longer has its own rendering tree. It delegates entirely to SessionCard.
|
||||
|
||||
---
|
||||
|
||||
## 7. Metrics
|
||||
|
||||
| Metric | Before | After | Change |
|
||||
|--------|--------|-------|--------|
|
||||
| Modal.js lines | 227 | 62 | -73% |
|
||||
| Total duplicated code | ~180 lines | 0 | -100% |
|
||||
| Features requiring dual maintenance | All | None | -100% |
|
||||
| Prop surface area (Modal) | 6 custom | 6 same as card | Aligned |
|
||||
|
||||
---
|
||||
|
||||
## 8. Verification Checklist
|
||||
|
||||
- [x] Card displays: status dot, status badge, agent badge, cwd, context usage, messages, input
|
||||
- [x] Modal displays: identical to card, just larger
|
||||
- [x] Card limits to 20 messages
|
||||
- [x] Modal shows all messages
|
||||
- [x] Smart scroll works in both views
|
||||
- [x] "Sending..." feedback works in both views
|
||||
- [x] Escape closes modal
|
||||
- [x] Click outside closes modal
|
||||
- [x] Entrance/exit animations work
|
||||
- [x] Body scroll locked when modal open
|
||||
|
||||
---
|
||||
|
||||
## 9. Future Considerations
|
||||
|
||||
### 9.1 Potential Enhancements
|
||||
|
||||
| Enhancement | Rationale | Blocked By |
|
||||
|-------------|-----------|------------|
|
||||
| Keyboard navigation in card grid | Accessibility | None |
|
||||
| Resize modal dynamically | User preference | None |
|
||||
| Pin modal to side (split view) | Power user workflow | Design decision needed |
|
||||
|
||||
### 9.2 Maintenance Notes
|
||||
|
||||
- **Any SessionCard change** automatically applies to modal view
|
||||
- **To add modal-only behavior**: Check `enlarged` prop (but avoid this — keep views identical)
|
||||
- **To change message limit**: Modify the `limit` prop value in SessionCard's ChatMessages call
|
||||
|
||||
---
|
||||
|
||||
## 10. Lessons Learned
|
||||
|
||||
1. **Composition > Duplication** — When two UIs show the same data, compose them from shared components
|
||||
2. **Props for variations** — A single boolean prop is often sufficient for "same thing, different context"
|
||||
3. **Identify the actual differences** — Modal needed only: backdrop, escape key, scroll lock, animations. Everything else was false complexity.
|
||||
4. **Feature drift is inevitable** — Duplicated code guarantees divergence over time. Only shared code stays in sync.
|
||||
Reference in New Issue
Block a user