Version select move to header #270
Conversation
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughMoved Version component from Sidebar to Header as an endSlot. Overhauled Version UI from native select to shared UI dropdown with selection migration logic. Introduced button forcedColorScheme prop and variants support. Adjusted dependencies to add UI libs and bump shared UI. Added Version re-export; removed Version CSS. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Header as Header
participant Version as Version Dropdown
participant Router as Location Params
participant Selection as Selection Utils
User->>Header: Open app
Header->>Version: Render endSlot (Version)
User->>Version: Click button to open dropdown
Version->>Version: handleOpenChange(true) -> scroll current item
User->>Version: Select version (vX or Latest)
alt No current text selection
Version->>Router: Update location params with new version
else Active text selection exists
Version->>Selection: migrateSelection(current -> new version)
Selection-->>Version: migrated range
Version->>Router: Update params with new version + adjusted selection
end
Note over Version,Router: Tooltip shown if not latest (text updated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Version/Version.tsx (2)
26-31: Don’t treat 0 as “no selection”Using falsy checks will incorrectly bypass migration when selectionStart or selectionEnd is 0. Use nullish checks instead.
- if (!selectionStart || !selectionEnd) { + if (selectionStart == null || selectionEnd == null) {
30-41: EnsuresetLocationParamsupdates are atomic using functional updatesThe
LocationContextprovider uses React’suseStatesetter directly, so calls likesetLocationParams({ version: newVersion })overwrite the entire params object and the async branch closes over a potentially stalelocationParams. Refactor both branches to use the functional form ofsetLocationParams:• File:
src/components/Version/Version.tsx
– Replace the direct overwrite in the “no selection” branch
– Replace the spread-closure update in the async branch--- a/src/components/Version/Version.tsx +++ b/src/components/Version/Version.tsx @@ handleVersionSelect = useCallback( (newVersion: string) => { const { selectionStart, selectionEnd, version } = locationParams; if (!selectionStart || !selectionEnd) { - setLocationParams({ version: newVersion }); + setLocationParams(prev => ({ + ...prev, + version: newVersion, + })); } else { migrateSelection({ selectionStart, selectionEnd }, version, newVersion) .then((newSelection) => { - setLocationParams({ - ...locationParams, - selectionStart: newSelection?.selectionStart ?? selectionStart, - selectionEnd: newSelection?.selectionEnd ?? selectionEnd, - version: newVersion, - }); + setLocationParams(prev => ({ + ...prev, + selectionStart: newSelection?.selectionStart ?? prev.selectionStart, + selectionEnd: newSelection?.selectionEnd ?? prev.selectionEnd, + version: newVersion, + })); }); } },This guarantees merging with the latest state and avoids TOCTOU/stale-closure issues.
🧹 Nitpick comments (8)
src/components/Header/Header.tsx (1)
3-3: Non-standard scrollIntoView behavior in Version component (“instant”)Since this file integrates Version, one nit in Version.tsx: ScrollIntoViewOptions.behavior accepts "auto" | "smooth". "instant" isn’t standard and may be ignored. Suggest switching to "auto" (or omit behavior).
Consider this change in src/components/Version/Version.tsx:
- currentItemRef.current.scrollIntoView({ block: "center", behavior: "instant" }); + currentItemRef.current.scrollIntoView({ block: "center", behavior: "auto" });src/components/ui/button/button.tsx (2)
12-12: Remove redundant prop declaration; it’s already provided by VariantPropsforcedColorScheme is already part of VariantProps. Declaring it again in ButtonProps is redundant.
export interface ButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement>, VariantProps<typeof buttonVariants> { asChild?: boolean; - forcedColorScheme?: "light" | "dark"; }
18-20: Prefer merging className outside cva for clarityPassing className into the cva resolver works, but it’s more explicit to let cva compute variant classes and then merge the caller’s className separately. This also mirrors common patterns with tailwind-merge.
- return ( - <Comp className={twMerge(buttonVariants({ variant, size, className, forcedColorScheme }))} ref={ref} {...props} /> - ); + return ( + <Comp + className={twMerge(buttonVariants({ variant, size, forcedColorScheme }), className)} + ref={ref} + {...props} + /> + );src/components/ui/button/variants.tsx (1)
24-27: Element-level “dark” class won’t affect dark: styles on the element itselfSetting forcedColorScheme: "dark" to add the "dark" class enables dark: styles for descendants, but Tailwind’s dark variant (compiled as ".dark .selector") won’t target the same element. Your compoundVariants for variant: "outline" correctly supply explicit classes to compensate. If you expect other variants (e.g., outlineBrand) to also respect the forced dark theme at the element level, consider adding analogous compoundVariants or refraining from relying on dark: at the element level.
src/components/Version/Version.tsx (4)
20-21: Sort versions for a stable, intuitive order (latest first)Currently relies on object iteration order. Sorting improves UX and predictability.
- const versions = Object.values(metadata.versions).filter(({ legacy }) => !legacy); + const versions = Object.values(metadata.versions) + .filter(({ legacy }) => !legacy) + .sort((a, b) => new Date(b.date).getTime() - new Date(a.date).getTime());
45-53: DRY up label composition across getCurrentVersionLabel and VersionOptionThe label building logic is duplicated and easy to drift. Centralize it.
- const getCurrentVersionLabel = () => { - const date = new Date(currentVersion.date); - const isLatest = currentVersion.hash === metadata.latest; - let label = isLatest ? "Latest" : "v"; - if (currentVersion.name) { - label += `: ${currentVersion.name}`; - } - return `${label} ${shortHash(currentVersion.hash)} (${date.toLocaleDateString()})`; - }; + const getCurrentVersionLabel = () => formatVersionLabel(currentVersion, metadata.latest);- return ( - <span className="w-full"> - {version.hash === latest ? latestText : versionText} {shortHash(version.hash)} ({date.toLocaleDateString()}) - </span> - ); + return <span className="w-full">{formatVersionLabel(version, latest)}</span>;Add this helper (outside of the selected range):
function formatVersionLabel(version: IVersionInfo, latest: string) { const isLatest = version.hash === latest; const base = isLatest ? "Latest" : "v"; const name = version.name ? `: ${version.name}` : ""; const date = new Date(version.date); return `${base}${name} ${shortHash(version.hash)} (${date.toLocaleDateString()})`; }Also applies to: 106-119
67-76: A11y: Tooltip alone isn’t announced by screen readers—add an aria-label to the warning iconProvide an accessible name so SR users know why the warning is present.
- <span + <span data-tooltip-id="version" data-tooltip-content="The current version is not the latest" data-tooltip-place="top" className="text-amber-500 text-2xl mt-[-2px]" + aria-label="The current version is not the latest" >Also applies to: 98-101
46-46: SSR/hydration: toLocaleDateString can differ across server/client localesIf this app renders on the server, toLocaleDateString can produce different strings, causing hydration mismatches. Consider fixing the timezone/locale for deterministic output (e.g., UTC).
Example:
// Deterministic date (UTC) regardless of host environment new Intl.DateTimeFormat("en-CA", { timeZone: "UTC" }).format(date); // or keep locale default but fix timeZone date.toLocaleDateString(undefined, { timeZone: "UTC" });Would you like me to patch both occurrences to use a deterministic formatter?
Also applies to: 108-118
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.json(1 hunks)src/components/Header/Header.tsx(1 hunks)src/components/Sidebar/Sidebar.tsx(0 hunks)src/components/Version/Version.css(0 hunks)src/components/Version/Version.tsx(4 hunks)src/components/Version/index.ts(1 hunks)src/components/ui/button/button.tsx(1 hunks)src/components/ui/button/variants.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- src/components/Sidebar/Sidebar.tsx
- src/components/Version/Version.css
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/components/Version/Version.tsx (3)
src/components/ui/dropdown-menu.tsx (5)
DropdownMenu(163-163)DropdownMenuTrigger(164-164)DropdownMenuContent(165-165)DropdownMenuRadioGroup(177-177)DropdownMenuRadioItem(168-168)src/components/ui/button/button.tsx (1)
Button(15-22)src/components/MetadataProvider/MetadataProvider.tsx (1)
IVersionInfo(7-12)
src/components/Header/Header.tsx (1)
src/components/Version/Version.tsx (1)
Version(16-104)
src/components/ui/button/button.tsx (1)
src/components/ui/button/variants.tsx (1)
buttonVariants(3-42)
🔇 Additional comments (5)
package.json (1)
21-21: Verification results for UI dependencies
- Lucide-react imports are all named ESM:
- src/components/Version/Version.tsx:
import { ChevronDown } from "lucide-react"- src/components/Header/components/github-dropdown-menu.tsx:
import { ExternalLink } from "lucide-react"- src/components/Header/components/dropdown-button.tsx:
import { LucideChevronDown } from "lucide-react"- src/components/ui/dropdown-menu.tsx:
import { Check, ChevronRight, Circle } from "lucide-react"- No wildcard imports (
import * as … from "lucide-react") detected.<Version />is now only imported/used in Header.tsx; no occurrences in Sidebar.- Header.tsx uses
<FluffyHeader … endSlot={<Version />} />Please confirm that the bumped package (
@fluffylabs/shared-ui@^0.1.1) indeed exportsFluffyHeaderwith anendSlotprop matching this usage.src/components/Version/index.ts (1)
1-1: LGTM: clean re-export to stabilize importsRe-exporting Version via index keeps import sites concise and matches the Header usage.
src/components/Header/Header.tsx (1)
7-9: Moving Version into endSlot reads well; confirm shared-ui supports this propThe composition with FluffyHeader looks good. Please confirm FluffyHeader in @fluffylabs/shared-ui@^0.1.1 exposes endSlot and that its layout handles the Version dropdown correctly on narrow widths.
src/components/ui/button/variants.tsx (1)
29-36: Nice compound variant for forcedColorScheme: "dark" + outlineThis gives you deterministic dark-styled outline buttons regardless of page theme; good use of CSS variables and ring offsets.
src/components/Version/Version.tsx (1)
20-23: No action required: metadata.versions keys are commit hashesWe’ve confirmed that
metadata.versionsis keyed by the full commit SHA, and eachIVersionInfo.hashis that same SHA. Usingversion.hashfor the radio values therefore correctly matches the lookup vialocationParams.version.
currentVersionHashalways equalslocationParams.version, so the selection logic is sound.- (Optional) You could streamline the code by using
locationParams.versiondirectly as the<DropdownMenuRadioGroup value={…}>rather than pulling it back out ofmetadata.versions, but this is purely a stylistic cleanup.
| const handleOpenChange = (open: boolean) => { | ||
| if (open) { | ||
| requestAnimationFrame(() => { | ||
| if (currentItemRef.current && dropdownContentRef.current) { | ||
| currentItemRef.current.scrollIntoView({ block: "center", behavior: "instant" }); | ||
| } | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix invalid scrollIntoView option: "instant" is not supported
ScrollIntoViewOptions.behavior only accepts "auto" or "smooth". "instant" will cause a TS error and is ignored by browsers.
Apply this diff:
- currentItemRef.current.scrollIntoView({ block: "center", behavior: "instant" });
+ currentItemRef.current.scrollIntoView({ block: "center", behavior: "auto" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleOpenChange = (open: boolean) => { | |
| if (open) { | |
| requestAnimationFrame(() => { | |
| if (currentItemRef.current && dropdownContentRef.current) { | |
| currentItemRef.current.scrollIntoView({ block: "center", behavior: "instant" }); | |
| } | |
| }); | |
| } | |
| }; | |
| const handleOpenChange = (open: boolean) => { | |
| if (open) { | |
| requestAnimationFrame(() => { | |
| if (currentItemRef.current && dropdownContentRef.current) { | |
| currentItemRef.current.scrollIntoView({ block: "center", behavior: "auto" }); | |
| } | |
| }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/components/Version/Version.tsx around lines 55 to 63, the call to
scrollIntoView uses an invalid behavior value "instant" which causes a
TypeScript error and is ignored by browsers; replace "instant" with a valid
option ("auto" or "smooth") — for example use { block: "center", behavior:
"auto" } — and ensure the argument matches the ScrollIntoViewOptions type (no
casts needed) so the code compiles and browsers perform the expected scroll.
1 participant
Uh oh!
There was an error while loading. Please reload this page.