Enhance ModelDetailsCard component: improve description overflow hand… - #14
Merged
Conversation
…ling and layout adjustments
Reviewer's GuideRefines how the ModelDetailsCard handles long model descriptions by improving overflow detection, making layout more robust, and slightly restructuring the collapsible description UI. Sequence diagram for description overflow measurement in ModelDetailsCardsequenceDiagram
actor User
participant Browser
participant ModelDetailsCard
participant ClampedParagraph as ClampedParagraph
participant MeasureParagraph as MeasureParagraph
participant ResizeObserverInstance as ResizeObserver
participant Window
participant DocumentFonts as DocumentFonts
User->>Browser: Navigate to compare page
Browser->>ModelDetailsCard: Render with model
ModelDetailsCard->>ModelDetailsCard: useLayoutEffect(model)
ModelDetailsCard->>ClampedParagraph: Attach clampedRef
ModelDetailsCard->>MeasureParagraph: Attach measureRef
ModelDetailsCard->>ModelDetailsCard: measure()
ModelDetailsCard->>MeasureParagraph: window.getComputedStyle(measureEl)
MeasureParagraph-->>ModelDetailsCard: CSSStyleDeclaration
ModelDetailsCard->>ModelDetailsCard: getLineHeightPx(styles)
ModelDetailsCard->>MeasureParagraph: getBoundingClientRect().height
MeasureParagraph-->>ModelDetailsCard: measuredHeight
ModelDetailsCard->>ModelDetailsCard: compute collapsedHeight
ModelDetailsCard->>ModelDetailsCard: overflowing = measuredHeight > collapsedHeight + 1
ModelDetailsCard->>ModelDetailsCard: setIsDescriptionOverflowing(overflowing)
ModelDetailsCard->>ResizeObserverInstance: new ResizeObserver(measure)
ModelDetailsCard->>ResizeObserverInstance: observe(clampedEl)
ModelDetailsCard->>Window: addEventListener(resize, measure)
ModelDetailsCard->>DocumentFonts: fonts.ready.then(measure)
Window-->>ModelDetailsCard: resize event
ModelDetailsCard->>ModelDetailsCard: measure()
ResizeObserverInstance-->>ModelDetailsCard: size change callback
ModelDetailsCard->>ModelDetailsCard: measure()
User-->>Browser: Navigate away or change model
Browser-->>ModelDetailsCard: Unmount or re-render with new model
ModelDetailsCard->>ResizeObserverInstance: disconnect()
ModelDetailsCard->>Window: removeEventListener(resize, measure)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis pull request adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the
useLayoutEffectsetup, theResizeObserveris attached toclampedElwhile the measurement logic usesmeasureEl; consider observingmeasureRefinstead so the observer is directly tracking the element whose height you use for overflow detection. - The collapsed description height is defined twice with implicit magic numbers (
COLLAPSED_DESCRIPTION_MIN_HEIGHTusingcalc(2 * 1.625 * 0.875rem)and the1.625factor ingetLineHeightPx); it would be clearer and less error-prone to centralize these values or derive the min-height from the same constants used in the JS calculation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `useLayoutEffect` setup, the `ResizeObserver` is attached to `clampedEl` while the measurement logic uses `measureEl`; consider observing `measureRef` instead so the observer is directly tracking the element whose height you use for overflow detection.
- The collapsed description height is defined twice with implicit magic numbers (`COLLAPSED_DESCRIPTION_MIN_HEIGHT` using `calc(2 * 1.625 * 0.875rem)` and the `1.625` factor in `getLineHeightPx`); it would be clearer and less error-prone to centralize these values or derive the min-height from the same constants used in the JS calculation.
## Individual Comments
### Comment 1
<location path="apps/website/app/(models)/compare/model-details-card.tsx" line_range="36-37" />
<code_context>
import { formatNumberCompact } from "../../../lib/format-number-compact";
+const COLLAPSED_DESCRIPTION_LINES = 2;
+const COLLAPSED_DESCRIPTION_MIN_HEIGHT =
+ "calc(2 * 1.625 * 0.875rem)";
+
+function getLineHeightPx(styles: CSSStyleDeclaration) {
</code_context>
<issue_to_address>
**suggestion:** The hardcoded min-height formula duplicates typography assumptions and can drift out of sync with the actual line-height/font-size.
This constant is tightly coupled to the current `text-sm` (0.875rem) and `leading-relaxed` (1.625) values. If those utilities or the base font-size change, the min-height will no longer match the actual line height, causing subtle layout issues. Since `useLayoutEffect` already measures line height, consider deriving the collapsed height from that measurement in JS, or at least centralize the `1.625` multiplier and `COLLAPSED_DESCRIPTION_LINES` into shared constants used both here and in the measurement logic.
Suggested implementation:
```typescript
const COLLAPSED_DESCRIPTION_LINES = 2;
const DESCRIPTION_LINE_HEIGHT_MULTIPLIER = 1.625;
const BASE_FONT_SIZE_REM = 0.875;
const COLLAPSED_DESCRIPTION_MIN_HEIGHT = `calc(${COLLAPSED_DESCRIPTION_LINES} * ${DESCRIPTION_LINE_HEIGHT_MULTIPLIER} * ${BASE_FONT_SIZE_REM}rem)`;
```
```typescript
function getLineHeightPx(styles: CSSStyleDeclaration) {
const lineHeightPx = Number.parseFloat(styles.lineHeight);
if (Number.isFinite(lineHeightPx)) {
return lineHeightPx;
}
const fontSizePx = Number.parseFloat(styles.fontSize);
return Number.isFinite(fontSizePx)
? fontSizePx * DESCRIPTION_LINE_HEIGHT_MULTIPLIER
: 0;
}
```
To fully align with your comment about deriving the collapsed height from the measured line height instead of typography assumptions, you could:
1. Store the measured line height from `getLineHeightPx` in state inside the `useLayoutEffect` that measures the description.
2. Compute the collapsed height as `measuredLineHeight * COLLAPSED_DESCRIPTION_LINES` in JS and apply it via an inline style or a CSS custom property, instead of relying on `COLLAPSED_DESCRIPTION_MIN_HEIGHT`.
3. Remove or narrow the usage of `BASE_FONT_SIZE_REM` once all consumers use the runtime measurement rather than the fallback formula.
These changes require touching the layout/measuring logic further down in the file, which isn't shown in the snippet.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/website/app/`(models)/compare/model-details-card.tsx:
- Around line 131-164: The effect's document.fonts?.ready.then callback can call
measure() after cleanup and mutate state for a different model; fix by tracking
cancellation inside useLayoutEffect (e.g., a local `let mounted = true` or an
AbortController) and only call measure() or setIsDescriptionOverflowing when
still mounted, and ensure the cleanup sets mounted = false (or aborts) so the
deferred promise handler no-ops after the effect has been torn down; update the
code around useLayoutEffect, the measure function, document.fonts?.ready.then,
and the cleanup to implement this guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cfce06b-2575-49be-9767-0419aed55b2e
📒 Files selected for processing (2)
.gitignoreapps/website/app/(models)/compare/model-details-card.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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.
…ling and layout adjustments
Summary by Sourcery
Improve ModelDetailsCard description overflow detection and collapsed layout for more robust rendering across models.
Enhancements:
Summary by CodeRabbit
Bug Fixes
Chores
Summary by cubic
Improves ModelDetailsCard overflow detection and collapsed description layout so the expand/collapse button appears correctly and the card stays steady when switching models. Adds guarded cleanup to prevent stale measurements when the model changes.
Bug Fixes
useLayoutEffect,ResizeObserver, and font-ready checks for accurate detection.model.id, set a min-height for the clamped text, observe both elements, and guard cleanup to avoid updates after unmount/model switch.Refactors
.pnpm-storein.gitignore.Written for commit 7741f43. Summary will update on new commits.