fix: eliminate layout shift on tools pages - #84
Conversation
Reviewer's GuideRefactors YouTube Music integration to a richer, cache-aware API with diagnostics, stabilizes media tools via persistent sessions and smarter ffmpeg probing, replaces generic skeletons and usage tracking on tools pages with per-tool skeleton components and more accurate Suspense fallbacks, and improves accessibility and focus management across activity and tool UIs while removing FAQ content/structured data from tools and package pages. Sequence diagram for media tool session restore and persistencesequenceDiagram
actor User
participant SendableVideo
participant Store as useSendableVideoStore
participant MediaSession as useMediaSession
participant Persistence as media_persistence
participant IndexedDB
SendableVideo->>Store: useSendableVideoStore()
Store->>MediaSession: useMediaSession(STORAGE_KEY, onRestore)
MediaSession->>Persistence: loadMediaSession(toolKey)
Persistence->>IndexedDB: openDatabase / get(input, trim, output)
IndexedDB-->>Persistence: TPersistedSession
Persistence-->>MediaSession: TPersistedSession
MediaSession-->>Store: onRestore(session)
Store->>Store: setFile / setFileUrl / setOutput
User->>SendableVideo: selectFile(File)
SendableVideo->>Store: selectFile(File)
Store->>MediaSession: persistFile(File)
MediaSession->>Persistence: saveMediaFile(toolKey, File)
Persistence->>IndexedDB: put(inputKey, File)
User->>SendableVideo: adjustTrim
SendableVideo->>Store: updateTrim(trim)
Store->>MediaSession: persistTrim(trim)
MediaSession->>Persistence: saveMediaTrim(toolKey, trim)
User->>SendableVideo: exportMp4 / exportGif
Store->>Store: setOutput
Store->>MediaSession: persistOutput(output)
MediaSession->>Persistence: saveMediaOutput(toolKey, output)
Persistence->>IndexedDB: put(outputKey, blob)
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.
|
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (71)
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 |
- render breadcrumbs server-side by scoping the useSearchParams Suspense boundary to individual links instead of the whole nav - make the /tools/[slug] Suspense fallback mirror the real page chrome, including the actual static ToolQuickNav - replace the single generic tool skeleton with per-tool skeletons that reproduce each tool's first-paint layout at all breakpoints - reuse those skeletons for the internal hydration gates of find-replace, diff-checker, hemelsbreed and coordinate-marker so chunk-load, hydration and ready states share one stable frame
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
ActivityContributionGraph, the keyboard navigation moves by 7 cells on horizontal arrows (treating columns as weeks), which may feel inverted compared to the visual grid; consider aligning ArrowUp/Down/Left/Right movement to the actual row/column layout so keyboard users traverse the calendar predictably. - The HTTP status mapping in
src/app/api/ytmusic/recent/route.tsis string-based and non-exhaustive; consider switching on theYTMusicStatusunion (e.g. aswitchor helper withneverhandling) so adding new statuses cannot silently fall back to 200.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ActivityContributionGraph`, the keyboard navigation moves by 7 cells on horizontal arrows (treating columns as weeks), which may feel inverted compared to the visual grid; consider aligning ArrowUp/Down/Left/Right movement to the actual row/column layout so keyboard users traverse the calendar predictably.
- The HTTP status mapping in `src/app/api/ytmusic/recent/route.ts` is string-based and non-exhaustive; consider switching on the `YTMusicStatus` union (e.g. a `switch` or helper with `never` handling) so adding new statuses cannot silently fall back to 200.
## Individual Comments
### Comment 1
<location path="src/components/landing/activity/contribution-graph.tsx" line_range="435-444" />
<code_context>
+ const activeCellIndex = focusedCellIndex ?? todayCellIndex
+
+ function handleGridKeyDown(event: React.KeyboardEvent) {
+ const totalCells = totalWeeks * 7
+ const moveByKey: Record<string, number> = {
+ ArrowRight: 7,
+ ArrowLeft: -7,
+ ArrowDown: 1,
+ ArrowUp: -1
+ }
+
+ let nextIndex: number
+ if (event.key === 'Home') {
+ nextIndex = 0
+ } else if (event.key === 'End') {
+ nextIndex = totalCells - 1
+ } else if (event.key in moveByKey) {
+ nextIndex = Math.min(
+ totalCells - 1,
+ Math.max(0, activeCellIndex + moveByKey[event.key])
+ )
+ } else {
+ return
+ }
+
+ event.preventDefault()
+ setFocusedCellIndex(nextIndex)
+ cellRefs.current.get(nextIndex)?.focus()
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Keyboard navigation assumes a full `totalWeeks * 7` grid, which can desync from actual rendered cells.
Because `totalCells` is derived from `totalWeeks * 7`, navigation can target indices for which no button exists (e.g. weeks with leading/trailing blanks). In those cases `cellRefs.current.get(nextIndex)` is undefined while `tabIndex` still treats that index as focused, so keyboard focus has no real DOM target. Consider deriving bounds from `cellRefs.current.size` or the actual `weeks` data so keyboard navigation only lands on rendered cells.
Suggested implementation:
```typescript
const activeCellIndex = focusedCellIndex ?? todayCellIndex
function handleGridKeyDown(event: React.KeyboardEvent) {
const totalCells = cellRefs.current.size
if (totalCells === 0) return
const moveByKey: Record<string, number> = {
ArrowRight: 7,
ArrowLeft: -7,
ArrowDown: 1,
ArrowUp: -1
}
let nextIndex: number
if (event.key === 'Home') {
nextIndex = 0
} else if (event.key === 'End') {
nextIndex = totalCells - 1
} else if (event.key in moveByKey) {
const currentIndex = Math.min(activeCellIndex, totalCells - 1)
nextIndex = Math.min(
totalCells - 1,
Math.max(0, currentIndex + moveByKey[event.key])
)
} else {
return
}
// Only move focus to indices that have a rendered cell
if (!cellRefs.current.has(nextIndex)) {
return
}
event.preventDefault()
setFocusedCellIndex(nextIndex)
cellRefs.current.get(nextIndex)?.focus()
}
const cellRefs = useRef<Map<number, HTMLButtonElement>>(new Map())
const [focusedCellIndex, setFocusedCellIndex] = useState<number | null>(null)
const todayCellIndex = useMemo(() => {
const todayStr = new Date().toISOString().split('T')[0]
for (let weekIndex = 0; weekIndex < weeks.length; weekIndex++) {
const dayIndex = weeks[weekIndex].findIndex(
day => day.date === todayStr
)
if (dayIndex !== -1) return weekIndex * 7 + dayIndex
```
If `totalWeeks` is now unused after this change, it can be safely removed to avoid dead code. Also ensure that any code creating `cellRefs` entries uses zero-based, contiguous indices so that `cellRefs.current.size` accurately reflects the highest valid index; if indices can be sparse, consider normalizing them or adjusting navigation to use an ordered list of valid keys instead of numeric ranges.
</issue_to_address>
### Comment 2
<location path="src/app/api/ytmusic/recent/route.ts" line_range="44-48" />
<code_context>
}
}
+
+function getHttpStatus(status: string): number {
+ if (status === 'unauthorized') return 401
+ if (status === 'unconfigured') return 503
+ if (status === 'error') return 502
+ return 200
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `string` instead of the `YTMusicStatus` type for `getHttpStatus` weakens type safety.
`status` comes from `YTMusicResult.status`, which is a discriminated union (`YTMusicStatus`). Accepting a plain `string` allows unsupported values to be passed without compile-time errors. Typing the parameter as `YTMusicStatus` (and using `assertNever` in the default case) would keep the mapping aligned with the domain model and prevent invalid statuses at the type level.
Suggested implementation:
```typescript
function getHttpStatus(status: YTMusicStatus): number {
if (status === 'unauthorized') return 401
if (status === 'unconfigured') return 503
if (status === 'error') return 502
if (status === 'ok') return 200
return assertNever(status)
}
```
To fully implement this change, you will also need to:
1. Ensure `YTMusicStatus` is imported or available in this file (e.g., `import type { YTMusicStatus } from '...';`).
2. Ensure `assertNever` is imported from your shared utilities (e.g., `import { assertNever } from '...';`) or implement it if it does not yet exist:
`export function assertNever(x: never): never { throw new Error(\`Unexpected value: \${x}\`) }`.
3. Confirm that `'ok'` (or the equivalent success status) is one of the union members of `YTMusicStatus`; adjust the success-case branch accordingly if your union uses a different literal for the successful state.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const totalCells = totalWeeks * 7 | ||
| const moveByKey: Record<string, number> = { | ||
| ArrowRight: 7, | ||
| ArrowLeft: -7, | ||
| ArrowDown: 1, | ||
| ArrowUp: -1 | ||
| } | ||
|
|
||
| let nextIndex: number | ||
| if (event.key === 'Home') { |
There was a problem hiding this comment.
suggestion: Keyboard navigation assumes a full totalWeeks * 7 grid, which can desync from actual rendered cells.
Because totalCells is derived from totalWeeks * 7, navigation can target indices for which no button exists (e.g. weeks with leading/trailing blanks). In those cases cellRefs.current.get(nextIndex) is undefined while tabIndex still treats that index as focused, so keyboard focus has no real DOM target. Consider deriving bounds from cellRefs.current.size or the actual weeks data so keyboard navigation only lands on rendered cells.
Suggested implementation:
const activeCellIndex = focusedCellIndex ?? todayCellIndex
function handleGridKeyDown(event: React.KeyboardEvent) {
const totalCells = cellRefs.current.size
if (totalCells === 0) return
const moveByKey: Record<string, number> = {
ArrowRight: 7,
ArrowLeft: -7,
ArrowDown: 1,
ArrowUp: -1
}
let nextIndex: number
if (event.key === 'Home') {
nextIndex = 0
} else if (event.key === 'End') {
nextIndex = totalCells - 1
} else if (event.key in moveByKey) {
const currentIndex = Math.min(activeCellIndex, totalCells - 1)
nextIndex = Math.min(
totalCells - 1,
Math.max(0, currentIndex + moveByKey[event.key])
)
} else {
return
}
// Only move focus to indices that have a rendered cell
if (!cellRefs.current.has(nextIndex)) {
return
}
event.preventDefault()
setFocusedCellIndex(nextIndex)
cellRefs.current.get(nextIndex)?.focus()
}
const cellRefs = useRef<Map<number, HTMLButtonElement>>(new Map())
const [focusedCellIndex, setFocusedCellIndex] = useState<number | null>(null)
const todayCellIndex = useMemo(() => {
const todayStr = new Date().toISOString().split('T')[0]
for (let weekIndex = 0; weekIndex < weeks.length; weekIndex++) {
const dayIndex = weeks[weekIndex].findIndex(
day => day.date === todayStr
)
if (dayIndex !== -1) return weekIndex * 7 + dayIndexIf totalWeeks is now unused after this change, it can be safely removed to avoid dead code. Also ensure that any code creating cellRefs entries uses zero-based, contiguous indices so that cellRefs.current.size accurately reflects the highest valid index; if indices can be sparse, consider normalizing them or adjusting navigation to use an ordered list of valid keys instead of numeric ranges.
| function getHttpStatus(status: string): number { | ||
| if (status === 'unauthorized') return 401 | ||
| if (status === 'unconfigured') return 503 | ||
| if (status === 'error') return 502 | ||
| return 200 |
There was a problem hiding this comment.
suggestion (bug_risk): Using string instead of the YTMusicStatus type for getHttpStatus weakens type safety.
status comes from YTMusicResult.status, which is a discriminated union (YTMusicStatus). Accepting a plain string allows unsupported values to be passed without compile-time errors. Typing the parameter as YTMusicStatus (and using assertNever in the default case) would keep the mapping aligned with the domain model and prevent invalid statuses at the type level.
Suggested implementation:
function getHttpStatus(status: YTMusicStatus): number {
if (status === 'unauthorized') return 401
if (status === 'unconfigured') return 503
if (status === 'error') return 502
if (status === 'ok') return 200
return assertNever(status)
}To fully implement this change, you will also need to:
- Ensure
YTMusicStatusis imported or available in this file (e.g.,import type { YTMusicStatus } from '...';). - Ensure
assertNeveris imported from your shared utilities (e.g.,import { assertNever } from '...';) or implement it if it does not yet exist:
export function assertNever(x: never): never { throw new Error(\Unexpected value: ${x}`) }`. - Confirm that
'ok'(or the equivalent success status) is one of the union members ofYTMusicStatus; adjust the success-case branch accordingly if your union uses a different literal for the successful state.
![Screenshot of /packages/[slug]](https://80hoebpheaklxk73.public.blob.vercel-storage.com/remcostoeten-jgh2sm43t-remcostoetens-projects.vercel.app_packages_[slug].jpeg)

![Screenshot of /tools/[slug]](https://80hoebpheaklxk73.public.blob.vercel-storage.com/remcostoeten-jgh2sm43t-remcostoetens-projects.vercel.app_tools_[slug].jpeg)
Summary
Speed Insights showed CLS of 0.11 on
/tools/[slug](and 0.09 on/tools). The shift came from three stacked layers, all fixed here:useSearchParams-forced<Suspense fallback={null}>, appearing only after hydration and pushing every page down. The Suspense boundary is now scoped to the individual links (only the?langparam needs it), so the nav is in the server HTML. Also benefits/blog/[...slug]and/, which share the component./tools/[slug]Suspense fallback now mirrors the real header geometry and renders the actual staticToolQuickNav, making the page chrome pixel-identical at every breakpoint.tool-skeletons.tsxhas a per-tool skeleton reproducing each tool's real first-paint layout (containers, borders, panel heights, responsive variants). Wired into thenext/dynamicloaders, and reused by the internal hydration gates of find-replace, diff-checker, hemelsbreed and coordinate-marker so chunk-load → hydration → ready render one stable frame.Verification
tsc --noEmitclean,next buildpasses with all tool routes prerenderedSummary by Sourcery
Improve tools pages stability and YouTube Music integration, adding richer diagnostics, persistent media sessions, and accessibility refinements while simplifying some marketing content.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: