Conversation
❌ Deploy Preview for determinate-ui failed. Why did it fail? →
|
grahamc
force-pushed
the
push-ynlmnrwpxkql
branch
from
161fa56 to
32bbd3f
Compare
grahamc
force-pushed
the
push-ynlmnrwpxkql
branch
5 times, most recently
from
0be93a7 to
96c7434
Compare
grahamc
force-pushed
the
push-ynlmnrwpxkql
branch
from
96c7434 to
250f7c7
Compare
📝 WalkthroughWalkthroughThis PR significantly refactors the UI library by consolidating CodeBlock and CodeFile components with new styling, introducing a responsive Navigation component, restructuring InstallCTA with version-aware sub-components (FindAwsAMIs, UseWithGitHubActions, InstallForNixOS), expanding SCSS utilities with new mixins and tokens, and improving SSR compatibility across multiple modules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
grahamc
force-pushed
the
push-ynlmnrwpxkql
branch
from
250f7c7 to
c07e350
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (26)
package.json (1)
51-53: Peer dep set to React 19 only—confirm intent; consider allowing React 18 too.If you don’t use React 19–specific APIs, widening helps consumers still on 18.
Apply this diff if you want to support 18 and 19:
- "react": "^19" + "react": ">=18 <20"src/InstallCTA/TabSelector.tsx (2)
4-8: Type the icon as ComponentType rather than FC for better compatibility with react-icons.react-icons exports
IconType = ComponentType<IconBaseProps>;FCis unnecessarily restrictive.- icon: FC<IconBaseProps>; + icon: ComponentType<IconBaseProps>;Add the type import (outside this range):
import { useCallback, type FC, type MouseEvent, type ComponentType } from "react";
38-38: Avoid duplicate accessible names; make the icon decorative.The link already contains text (
{name}), so the icon should be aria-hidden instead of getting a title.- <Icon title={name} /> {name} + <Icon aria-hidden="true" focusable="false" /> {name}src/hooks/useSystemColorScheme.ts (1)
35-52: Add Safari <14 fallback for media query change events.Older Safari only supports
addListener/removeListener.- media.addEventListener("change", handleChange); - return function cleanup() { - media.removeEventListener("change", handleChange); - }; + if ("addEventListener" in media) { + media.addEventListener("change", handleChange); + return () => media.removeEventListener("change", handleChange); + } else { + // Safari <14 + // @ts-expect-error legacy API + media.addListener(handleChange); + return () => { + // @ts-expect-error legacy API + media.removeListener(handleChange); + }; + }src/ColorProvider/index.tsx (4)
97-105: Don’t reassign the destructured prop; compute a resolvedRoot instead.Improves clarity and avoids param shadowing.
- root, + root, children, }) => { - if (!root) { - if (typeof document !== "undefined") { - root = document.body; - } - } + const resolvedRoot = + root ?? (typeof document !== "undefined" ? document.body : undefined);
118-121: Avoid DOM side effects during render; use a layout effect for initial apply.This prevents render-time mutations and still avoids FOUC.
- if (root) { - applyTheme(root, scheme); - } + // Apply ASAP before paint to reduce FOUC + // (import useLayoutEffect at top) + useLayoutEffect(() => { + if (resolvedRoot) { + applyTheme(resolvedRoot, scheme); + } + }, []);Add import (outside this range):
import React, { useEffect, useLayoutEffect, useMemo, useState, type PropsWithChildren } from "react";
138-141: Use the resolved root variable consistently.Keeps semantics consistent with the earlier resolution.
- if (root) { - applyTheme(root, scheme); + if (resolvedRoot) { + applyTheme(resolvedRoot, scheme); }
150-154: Avoid console noise in libraries.Drop or gate the log in production.
- if (classes === undefined) { - console.log("classList on the root is null... probably SSR?"); - return; - } + if (classes === undefined) { + return; + }src/sass/_tokens.scss (1)
104-106: Spacing scale:lgduplicatesbase.
baseandlgare both0.75rem, which defeats the scale. Recommend monotonic increase.Example adjustment:
$spacing: ( xs: 0.25rem, sm: 0.5rem, base: 0.75rem, - lg: 0.75rem, - xl: 1rem, + lg: 1rem, + xl: 1.25rem, );src/sass/_mixins.scss (2)
49-51: Guard radius math against edge cases.If a future radius token drops below
1px,calc(... - 1px)can go negative. Cheap guard withmax().- border-top-left-radius: calc(map.get(tokens.$border-radius, $size) - 1px); - border-top-right-radius: calc(map.get(tokens.$border-radius, $size) - 1px); + border-top-left-radius: max(calc(map.get(tokens.$border-radius, $size) - 1px), 0px); + border-top-right-radius: max(calc(map.get(tokens.$border-radius, $size) - 1px), 0px); @@ - border-bottom-left-radius: calc(map.get(tokens.$border-radius, $size) - 1px); - border-bottom-right-radius: calc(map.get(tokens.$border-radius, $size) - 1px); + border-bottom-left-radius: max(calc(map.get(tokens.$border-radius, $size) - 1px), 0px); + border-bottom-right-radius: max(calc(map.get(tokens.$border-radius, $size) - 1px), 0px);Also applies to: 69-71
82-95: DRY nit: duplicate box-shadow in light mode.Same shadow in both modes—drop the override unless you plan to diverge later.
@mixin shadow { box-shadow: 0 0 #0000, 0 0 #0000, 0 1px 3px 0 rgba(0, 0, 0, 0.1), 0 1px 2px -1px rgba(0, 0, 0, 0.1); - @include light-mode { - box-shadow: - 0 0 #0000, - 0 0 #0000, - 0 1px 3px 0 rgba(0, 0, 0, 0.1), - 0 1px 2px -1px rgba(0, 0, 0, 0.1); - } }src/CodeBlock/CodeBlock.stories.tsx (1)
14-17: Stories read well; consider adding a Nix example and a filename for downloads.
- Add a small Nix snippet to showcase the new language.
- Provide
downloadAs(e.g., ".github/workflows/nix-ci.yaml") so downloads have a friendly filename.Example:
export const GithubCI: Story = { args: { @@ - allowDownload: true, + allowDownload: true, + downloadAs: ".github/workflows/nix-ci.yaml", }, }; @@ +export const NixSample: Story = { + args: { + language: "nix", + code: 'pkgs: pkgs.mkShell { buildInputs = [ pkgs.hello ]; }', + title: "shell.nix", + kind: "file", + allowCopy: true, + }, +};Also applies to: 21-23, 65-93, 95-115
src/InstallCTA/FindAwsAMIs.tsx (2)
12-14: Type the component as FC and drop the unused paramPrevents mismatched prop typing and removes the placeholder arg.
-const FindAwsAMIs: FC = (_: TabProps) => { +const FindAwsAMIs: FC<TabProps> = () => {
6-10: Tighten the AMI search to only available imagesAdds a safe state filter; keeps output deterministic.
const code = `aws ec2 describe-images \ --owners 535002876703 \ --filters "Name=name,Values=determinate/nixos/epoch-1/*" \ - "Name=architecture,Values=x86_64,arm64" \ + "Name=architecture,Values=x86_64,arm64" \ + "Name=state,Values=available" \ --query "Images | sort_by(@, &CreationDate) | [-1]"`;src/InstallCTA/InstallForNixOS.tsx (1)
16-19: Type the component as FCAvoids accepting {} props at the type level.
-const InstallForNixOS: FC = ({ version }: TabProps) => { +const InstallForNixOS: FC<TabProps> = ({ version }) => {src/InstallCTA/index.tsx (1)
71-89: A11y: ensure proper tab semantics in TabSelectorTab lists should expose role="tablist"/"tab" and aria-selected; verify TabSelector does this.
src/CodeBlock/index.tsx (1)
47-58: Prevent rendering an empty icon wrapperOnly show the icons container when at least one control will render.
- {(allowCopy || allowDownload) && ( + {(allowCopy || (allowDownload && isFile)) && ( <span className="code-block__icons"> {allowCopy && <CopyButton data={trimmedCode} />} {allowDownload && isFile && ( <DownloadButton data={trimmedCode} filename={downloadAs.split("/").reverse()[0]} /> )} </span> )}src/InstallCTA/UseWithGitHubActions.tsx (2)
6-7: Type the component as FCTightens prop typing.
-const UseWithGitHubActions: FC = ({ version }: TabProps) => { +const UseWithGitHubActions: FC<TabProps> = ({ version }) => {
7-18: Option: set minimal permissions in the workflowSensible default for least privilege.
const code = ` on: push: +permissions: + contents: read jobs: tests: runs-on: ubuntu-24.04 steps: - uses: actions/checkout@v5 - uses: DeterminateSystems/determinate-nix-action@v${version || "3"} - run: nix build .# `;src/CodeBlock/index.scss (1)
11-15: Naming consistency: prefer a BEM-scoped class over .highlightMinor: consider .code-block__highlight to avoid coupling to global .highlight naming.
src/InstallCTA/InstallCTA.stories.tsx (6)
9-23: Use Storybook controls instead of rawtypein argTypes
type: "string"isn’t a control and may be ignored by Storybook. Recommend proper controls and (optionally) deriving options from the enum for future-proofing.argTypes: { initialTab: { - type: "string", - options: [ + control: { type: "select" }, + options: [ InstallTarget.AWS, InstallTarget.GitHub, InstallTarget.Linux, InstallTarget.MacOS, InstallTarget.NixOS, ], }, version: { - type: "string", + control: { type: "text" }, }, },
33-35: Fix inconsistent story nameMacOSLatest is labeled “macOS Tagged”. Rename to “macOS Latest” to match intent and avoid confusion with MacOSTagged.
export const MacOSLatest: Story = { - name: "macOS Tagged", + name: "macOS Latest",
40-44: Add name for consistencyOther “Latest” stories have readable names; add one here for uniformity.
export const LinuxLatest: Story = { + name: "Linux Latest", args: { initialTab: InstallTarget.Linux, }, };
46-50: Add name for consistencyMatch labeling with GitHub/NixOS Latest.
export const AWSLatest: Story = { + name: "AWS Latest", args: { initialTab: InstallTarget.AWS, }, };
74-79: Add name for consistencyProvide a friendly label like other Tagged stories.
export const LinuxTagged: Story = { + name: "Linux Tagged", args: { initialTab: InstallTarget.Linux, version: "3.8.6", }, };
81-86: Add name for consistencyAlign with GitHubTagged/NixOSTagged.
export const AWSTagged: Story = { + name: "AWS Tagged", args: { initialTab: InstallTarget.AWS, version: "3.8.6", }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
package.json(1 hunks)src/CodeBlock/CodeBlock.stories.tsx(2 hunks)src/CodeBlock/index.scss(1 hunks)src/CodeBlock/index.tsx(2 hunks)src/CodeFile/CodeFile.stories.tsx(0 hunks)src/CodeFile/index.scss(0 hunks)src/CodeFile/index.tsx(0 hunks)src/ColorProvider/index.tsx(4 hunks)src/Highlight/index.scss(1 hunks)src/InstallCTA/FindAwsAMIs.tsx(1 hunks)src/InstallCTA/InstallCTA.stories.tsx(2 hunks)src/InstallCTA/InstallForNixOS.tsx(1 hunks)src/InstallCTA/InstallFromCurl.tsx(1 hunks)src/InstallCTA/NavTab.tsx(0 hunks)src/InstallCTA/TabSelector.tsx(2 hunks)src/InstallCTA/UseWithGitHubActions.tsx(1 hunks)src/InstallCTA/index.scss(2 hunks)src/InstallCTA/index.tsx(3 hunks)src/InstallCTA/types.ts(2 hunks)src/Link/index.scss(1 hunks)src/MacInstaller/index.scss(2 hunks)src/MacInstaller/index.tsx(1 hunks)src/hooks/useHighlight.ts(3 hunks)src/hooks/useSystemColorScheme.ts(2 hunks)src/index.ts(0 hunks)src/sass/_mixins.scss(1 hunks)src/sass/_tokens.scss(2 hunks)
💤 Files with no reviewable changes (5)
- src/CodeFile/index.scss
- src/CodeFile/index.tsx
- src/CodeFile/CodeFile.stories.tsx
- src/InstallCTA/NavTab.tsx
- src/index.ts
🧰 Additional context used
🧬 Code graph analysis (7)
src/InstallCTA/InstallForNixOS.tsx (1)
src/InstallCTA/index.tsx (1)
TabProps(32-37)
src/InstallCTA/types.ts (1)
src/InstallCTA/index.tsx (1)
InstallTarget(17-17)
src/CodeBlock/index.tsx (2)
src/index.ts (2)
CodeBlockProps(15-15)HighlightLanguage(7-7)src/hooks/useHighlight.ts (1)
HighlightLanguage(21-21)
src/InstallCTA/FindAwsAMIs.tsx (1)
src/InstallCTA/index.tsx (1)
TabProps(32-37)
src/InstallCTA/InstallFromCurl.tsx (1)
src/InstallCTA/index.tsx (1)
TabProps(32-37)
src/InstallCTA/UseWithGitHubActions.tsx (1)
src/InstallCTA/index.tsx (1)
TabProps(32-37)
src/InstallCTA/InstallCTA.stories.tsx (1)
src/InstallCTA/index.tsx (1)
InstallTarget(17-17)
🪛 Biome (2.1.2)
src/InstallCTA/index.tsx
[error] 78-83: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (14)
src/InstallCTA/types.ts (2)
8-10: LGTM: targets expanded (AWS, GitHub).Enum additions look consistent with the rest of the PR. Ensure stories and the tab/component map cover these targets.
17-22: SSR safety: avoid defaulting to navigator in the parameter; confirm Windows→Linux mapping.Defaulting userAgent to navigator.userAgent will throw during SSR — make the param optional and read navigator at runtime if available. Also confirm the product decision to map Windows → InstallTarget.Linux.
File: src/InstallCTA/types.ts Lines: 17-22 (also applies to 24-28)
-export function detectInstallTarget( - userAgent = navigator.userAgent, -): InstallTarget { - if (userAgent.includes("Windows")) { +export function detectInstallTarget(userAgent?: string): InstallTarget { + const ua = + userAgent ?? (typeof navigator !== "undefined" ? navigator.userAgent : ""); + if (ua.includes("Windows")) { return InstallTarget.Linux; } - if (userAgent.includes("Linux")) { + if (ua.includes("Linux")) { return InstallTarget.Linux; } return InstallTarget.MacOS; }src/hooks/useSystemColorScheme.ts (1)
5-10: Nice SSR guard on window access.Prevents crashes during SSR while preserving client behavior.
src/Link/index.scss (1)
8-9: Good a11y improvement.Underline + offset improves discoverability beyond color alone.
src/sass/_tokens.scss (1)
44-46: Code bg color change acknowledged.Switching
bg-darkto#231f20aligns with brand black; verify contrast with Shiki dark token colors in real code samples.src/Highlight/index.scss (1)
11-11: Removing CSS var fallbacks is fine with our Shiki config.Given
defaultColor: false, relying solely on token-driven backgrounds is consistent.Also applies to: 14-15
src/sass/_mixins.scss (1)
31-40: Solid base border mixin.Consistent radius and color with light-mode override reads well.
src/InstallCTA/InstallForNixOS.tsx (1)
7-14: Use inputs.determinate for module reference to match the input declarationThe snippet declares inputs.determinate; reference it consistently.
- # Load the Determinate module - determinate.nixosModules.default + # Load the Determinate module + inputs.determinate.nixosModules.defaultIf your docs intentionally show the parameter-style binding (outputs = { self, nixpkgs, determinate, ... }:), ignore this change. Otherwise, this keeps the two snippets consistent.
src/MacInstaller/index.scss (1)
15-26: LGTMNice light-mode theming and spacing tweaks.
Also applies to: 29-32, 38-50, 56-57
src/InstallCTA/InstallCTA.stories.tsx (5)
52-57: LGTMStory name and args look good.
59-64: LGTMConsistent with other “Latest” stories.
66-72: LGTMTagged story and version wiring look good.
88-94: LGTMNaming and args are consistent.
96-102: LGTMLooks consistent with the rest.
| kind: "snippet" | "file"; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make kind optional for backward compatibility
You already default kind to "snippet"; let callers omit it.
- kind: "snippet" | "file";
+ kind?: "snippet" | "file";Also ensure the exported type mirrors this change.
Also applies to: 24-33
🏁 Script executed:
#!/bin/bash
# Verify external exports and callsites for kind
rg -nP 'export\s+(type|interface)\s+CodeBlockProps' src/index.ts
rg -nP --type=tsx '<CodeBlock(?![^>]*\bkind=)' -C1Length of output: 83
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "=== List src/CodeBlock ==="
ls -la src/CodeBlock 2>/dev/null || echo "src/CodeBlock does not exist"
echo
echo "=== Show src/CodeBlock/index.tsx (first 240 lines) ==="
if [[ -f src/CodeBlock/index.tsx ]]; then
sed -n '1,240p' src/CodeBlock/index.tsx | nl -ba -v1
else
echo "MISSING: src/CodeBlock/index.tsx"
fi
echo
echo "=== Search for CodeBlockProps occurrences ==="
rg -n --hidden -S "CodeBlockProps" src || true
echo
echo "=== Search for 'export type|interface CodeBlockProps' ==="
rg -n --hidden -S "export\s+(type|interface)\s+CodeBlockProps" src || true
echo
echo "=== Search for 'kind:' definitions ==="
rg -n --hidden -S "kind\s*:" src || true
echo
echo "=== Find files with <CodeBlock and show context (3 lines) ==="
rg -n --hidden -S "<CodeBlock" -g 'src/**' -C3 || true
echo
echo "=== For each file containing <CodeBlock, check if file contains 'kind=' anywhere ==="
files=$(rg -n --hidden -S "<CodeBlock" -g 'src/**' --files || true)
if [[ -z "${files// /}" ]]; then
echo "No files with <CodeBlock found under src/"
else
for f in $files; do
echo "---- File: $f ----"
rg -n --hidden -S "<CodeBlock" -n -C3 "$f" || true
echo "---- 'kind=' occurrences in $f ----"
rg -n --hidden -S "kind\s*=" -n -C3 "$f" || echo "no kind= in $f"
done
fiLength of output: 455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "=== Show src/CodeBlock/index.tsx ==="
if [[ -f src/CodeBlock/index.tsx ]]; then
sed -n '1,400p' src/CodeBlock/index.tsx || true
else
echo "MISSING: src/CodeBlock/index.tsx"
fi
echo
echo "=== Search for CodeBlockProps occurrences ==="
rg -n --hidden --no-ignore "CodeBlockProps" src || true
echo
echo "=== Search for explicit exports of CodeBlockProps ==="
rg -n --hidden --no-ignore "export type CodeBlockProps" src || true
rg -n --hidden --no-ignore "export interface CodeBlockProps" src || true
echo
echo "=== Search for 'kind' property definitions/usages ==="
rg -n --hidden --no-ignore "kind\s*:" src || true
rg -n --hidden --no-ignore "\"snippet\"\s*\|\s*\"file\"" src || true
echo
echo "=== Find <CodeBlock components and whether they pass kind= ==="
rg -n --hidden --no-ignore "<CodeBlock" -g 'src/**' -C2 || true
files=$(rg --hidden --no-ignore "<CodeBlock" -g 'src/**' --files || true)
if [[ -z "${files// /}" ]]; then
echo "No files with <CodeBlock found under src/"
else
for f in $files; do
echo
echo "---- File: $f ----"
sed -n '1,200p' "$f" || true
echo "---- 'kind=' occurrences in $f ----"
rg -n --hidden --no-ignore "kind\s*=" -n -C2 "$f" || echo "no kind= in $f"
done
fiLength of output: 5026
Make CodeBlockProps.kind optional
Align the exported type with the component default and existing call sites.
- kind: "snippet" | "file";
+ kind?: "snippet" | "file";Edit: src/CodeBlock/index.tsx (interface CodeBlockProps — kind at ~line 18). src/index.ts re-exports the type — no change needed.
📝 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.
| kind: "snippet" | "file"; | |
| } | |
| kind?: "snippet" | "file"; | |
| } |
🤖 Prompt for AI Agents
In src/CodeBlock/index.tsx around lines 18-19, the CodeBlockProps.interface
marks kind as required but the component and existing callers treat it as
optional; change the interface to make kind optional (kind?: "snippet" |
"file"), leaving any re-exports unchanged, and run typecheck to ensure no other
call sites need updates.
| import langShell from "@shikijs/langs/shellscript"; | ||
| import langTerraform from "@shikijs/langs/terraform"; | ||
| import langYaml from "@shikijs/langs/yaml"; | ||
| import langNix from "@shikijs/langs/nix"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Nix support looks good; add a safe no-highlight path for "text".
If "text" isn’t a registered Shiki language, codeToHtml can throw. Short-circuit when language === "text" to return escaped HTML without invoking Shiki. Also consider catching unknown langs to avoid runtime crashes.
Apply inside useHighlight:
function useHighlight(language: HighlightLanguage, code: string): string {
return useMemo(() => {
+ if (language === "text") {
+ const escape = (s: string) =>
+ s.replace(/[&<>"']/g, (ch) => ({ "&":"&","<":"<",">":">",'"':""","'":"'" }[ch]!));
+ return `<pre class="highlight"><code>${escape(code)}</code></pre>`;
+ }
const html = getShiki().codeToHtml(code, {
lang: language,
themes: {
light: "github-light",
dark: "github-dark",
},
defaultColor: false,
structure: "inline",
});
return html;
}, [language, code]);
}Also applies to: 15-20, 28-29
🤖 Prompt for AI Agents
In src/hooks/useHighlight.ts around lines 6, 15-20 and 28-29, add a safe
no-highlight path for language === "text" and guard against unknown Shiki
languages: short-circuit before calling shiki.codeToHtml to return HTML-escaped
code for plain text, and wrap the shiki highlighting call in a try/catch (or
check shiki.getLanguage) to fall back to escaped HTML when the requested
language isn't registered or an error occurs; ensure you import or use an
existing HTML-escape utility and return the same shape as the successful
codeToHtml result.
| display: flex; | ||
| text-align: center; | ||
| align-items: center; | ||
| text-wrap-mode: nowrap; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use white-space: nowrap (text-wrap-mode is nonstandard/unsupported)
Fixes a CSS correctness issue.
- text-wrap-mode: nowrap;
+ white-space: nowrap;📝 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.
| display: flex; | |
| text-align: center; | |
| align-items: center; | |
| text-wrap-mode: nowrap; | |
| display: flex; | |
| text-align: center; | |
| align-items: center; | |
| white-space: nowrap; |
🤖 Prompt for AI Agents
In src/InstallCTA/index.scss around lines 59 to 63, the stylesheet uses the
nonstandard property text-wrap-mode: nowrap; — replace that line with the
standard property white-space: nowrap; so the text will not wrap across
browsers; keep the surrounding declarations (display, text-align, align-items)
as-is.
| {ctaTabs.map(([target, icon]) => ( | ||
| <TabSelector | ||
| name={target} | ||
| icon={icon} | ||
| active={activeTab == target} | ||
| onClick={() => setActiveTab(target)} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a key to mapped elements; fix strict equality
Resolves the Biome error and avoids accidental coercion.
- {ctaTabs.map(([target, icon]) => (
- <TabSelector
+ {ctaTabs.map(([target, icon]) => (
+ <TabSelector
+ key={target}
name={target}
icon={icon}
- active={activeTab == target}
+ active={activeTab === target}
onClick={() => setActiveTab(target)}
/>
))}📝 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.
| {ctaTabs.map(([target, icon]) => ( | |
| <TabSelector | |
| name={target} | |
| icon={icon} | |
| active={activeTab == target} | |
| onClick={() => setActiveTab(target)} | |
| /> | |
| ))} | |
| {ctaTabs.map(([target, icon]) => ( | |
| <TabSelector | |
| key={target} | |
| name={target} | |
| icon={icon} | |
| active={activeTab === target} | |
| onClick={() => setActiveTab(target)} | |
| /> | |
| ))} |
🧰 Tools
🪛 Biome (2.1.2)
[error] 78-83: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🤖 Prompt for AI Agents
In src/InstallCTA/index.tsx around lines 77 to 84, the mapped TabSelector
elements are missing a key prop and use loose equality; add a unique key (e.g.,
key={target}) to the TabSelector returned by the map and change the active
comparison from == to strict === (activeTab === target) to satisfy React/Biome
rules and avoid accidental coercion.
| const installScript = (version: string | undefined) => { | ||
| if (typeof version === "undefined") { | ||
| return "curl -fsSL https://install.determinate.systems/nix | sh -s -- install --determinate"; | ||
| } else { | ||
| return `curl -fsSL https://install.determinate.systems/nix/tag/v${version} | sh -s -- install --determinate`; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize version before embedding in a shell command URL.
Unencoded versions can break the URL users copy-paste; encode for safety.
-const installScript = (version: string | undefined) => {
- if (typeof version === "undefined") {
+const installScript = (version: string | undefined) => {
+ if (typeof version === "undefined" || version === "") {
return "curl -fsSL https://install.determinate.systems/nix | sh -s -- install --determinate";
} else {
- return `curl -fsSL https://install.determinate.systems/nix/tag/v${version} | sh -s -- install --determinate`;
+ const v = encodeURIComponent(version);
+ return `curl -fsSL https://install.determinate.systems/nix/tag/v${v} | sh -s -- install --determinate`;
}
};📝 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 installScript = (version: string | undefined) => { | |
| if (typeof version === "undefined") { | |
| return "curl -fsSL https://install.determinate.systems/nix | sh -s -- install --determinate"; | |
| } else { | |
| return `curl -fsSL https://install.determinate.systems/nix/tag/v${version} | sh -s -- install --determinate`; | |
| } | |
| }; | |
| const installScript = (version: string | undefined) => { | |
| if (typeof version === "undefined" || version === "") { | |
| return "curl -fsSL https://install.determinate.systems/nix | sh -s -- install --determinate"; | |
| } else { | |
| const v = encodeURIComponent(version); | |
| return `curl -fsSL https://install.determinate.systems/nix/tag/v${v} | sh -s -- install --determinate`; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/InstallCTA/InstallFromCurl.tsx around lines 6 to 12, the version string
is interpolated directly into the install URL which can break or introduce
unsafe characters; sanitize the version by URL-encoding it (e.g., use
encodeURIComponent) before embedding into the backtick string, ensure the
undefined branch remains unchanged, and use the encodedVersion variable in the
tag path so the generated curl URL is safe for copy-paste.
| const InstallFromCurl: FC = ({ version }: TabProps) => ( | ||
| <> |
There was a problem hiding this comment.
Props typing bug: component exported as FC<{}> (loses version).
Typing the variable as FC without generics erases the version prop from the public type, causing TS errors for consumers.
Apply:
-const InstallFromCurl: FC = ({ version }: TabProps) => (
+const InstallFromCurl: FC<TabProps> = ({ version }) => (📝 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 InstallFromCurl: FC = ({ version }: TabProps) => ( | |
| <> | |
| const InstallFromCurl: FC<TabProps> = ({ version }) => ( | |
| <> |
🤖 Prompt for AI Agents
In src/InstallCTA/InstallFromCurl.tsx around lines 14 to 15, the component is
typed as FC without generics which erases the version prop; change the component
type to FC<TabProps> and adjust the parameter typing by removing the inline cast
(e.g. keep destructuring { version } without : TabProps) so the exported
component retains the correct public prop type; ensure TabProps is
imported/available and update the signature to use the generic FC<TabProps>.
| const urlPath = version ? `tag/v${version}` : `stable`; | ||
| return ( | ||
| <a | ||
| href={`https://install.determinate.systems/determinate-pkg/${urlPath}/Universal`} | ||
| className="mac-installer" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encode version in URLs to avoid malformed links.
Protect the path segment when version contains spaces or special chars.
- const urlPath = version ? `tag/v${version}` : `stable`;
+ const urlPath = version ? `tag/v${encodeURIComponent(version)}` : `stable`;📝 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 urlPath = version ? `tag/v${version}` : `stable`; | |
| return ( | |
| <a | |
| href={`https://install.determinate.systems/determinate-pkg/${urlPath}/Universal`} | |
| className="mac-installer" | |
| const urlPath = version ? `tag/v${encodeURIComponent(version)}` : `stable`; | |
| return ( | |
| <a | |
| href={`https://install.determinate.systems/determinate-pkg/${urlPath}/Universal`} | |
| className="mac-installer" |
🤖 Prompt for AI Agents
In src/MacInstaller/index.tsx around lines 17 to 21, the URL uses the raw
version string which can produce malformed links if version contains spaces or
special characters; update the code to URL-encode the version segment (e.g., use
encodeURIComponent on the version value when building urlPath or the href) so
the href becomes safe, preserving the "v" prefix only once and leaving the
stable branch unchanged.
c07e350 to
f0f28a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/InstallCTA/types.ts (1)
17-19: Make detectInstallTarget SSR-safe (navigator may be undefined).Calling this during SSR will throw. Guard access to navigator and work off a local ua string.
-export function detectInstallTarget( - userAgent = navigator.userAgent, -): InstallTarget { - if (userAgent.includes("Windows")) { +export function detectInstallTarget(userAgent?: string): InstallTarget { + const ua = + userAgent ?? + (typeof navigator !== "undefined" ? navigator.userAgent : ""); + if (ua.includes("Windows")) { return InstallTarget.Linux; } - if (userAgent.includes("Linux")) { + if (ua.includes("Linux")) { return InstallTarget.Linux; } return InstallTarget.MacOS; }Also applies to: 20-29
♻️ Duplicate comments (1)
src/InstallCTA/index.tsx (1)
77-83: Add a key to mapped elements; use strict equality.Avoid React warnings and accidental coercion.
- {ctaTabs.map(([target, icon]) => ( + {ctaTabs.map(([target, icon]) => ( <TabSelector + key={target} name={target} icon={icon} - active={activeTab == target} + active={activeTab === target} onClick={() => setActiveTab(target)} /> ))}
🧹 Nitpick comments (14)
src/InstallCTA/types.ts (1)
24-26: Optionally recognize Android/ChromeOS as Linux.Many Android/CrOS UAs won’t match "Linux" literally.
- if (ua.includes("Linux")) { + if (ua.includes("Linux") || ua.includes("Android") || ua.includes("CrOS")) { return InstallTarget.Linux; }src/InstallCTA/index.tsx (2)
15-15: Prefer IconType over React.FC for react-icons.IconType is the canonical type for these icons. Validate TabSelector’s prop type before changing.
-import type { IconBaseProps } from "react-icons"; +import type { IconType } from "react-icons"; -const ctaTabs: [InstallTarget, React.FC<IconBaseProps>][] = [ +const ctaTabs: [InstallTarget, IconType][] = [Also applies to: 39-45
1-1: Client-only detection to avoid SSR mismatches.Initialize with a stable default and detect on mount. This also sidesteps any future SSR access pitfalls.
-import { useState, type FC } from "react"; +import { useState, useEffect, type FC } from "react"; @@ -const InstallCTA: FC<InstallCTAProps> = ({ initialTab, version }) => { - const [activeTab, setActiveTab] = useState<InstallTarget>(() => { - if (initialTab) { - return initialTab; - } - - return detectInstallTarget(); - }); +const InstallCTA: FC<InstallCTAProps> = ({ initialTab, version }) => { + const [activeTab, setActiveTab] = useState<InstallTarget>( + initialTab ?? InstallTarget.MacOS, + ); + useEffect(() => { + if (!initialTab) { + setActiveTab(detectInstallTarget()); + } + }, [initialTab]);Also applies to: 60-67
src/InstallCTA/InstallCTA.stories.tsx (2)
33-35: Fix mislabeled story name (Latest vs Tagged).MacOSLatest should display “macOS Latest”.
-export const MacOSLatest: Story = { - name: "macOS Tagged", +export const MacOSLatest: Story = { + name: "macOS Latest",
9-23: Enable proper Storybook controls for args.Use control types instead of argTypes.type to get a select/text UI.
argTypes: { initialTab: { - type: "string", + control: { type: "select" }, options: [ InstallTarget.AWS, InstallTarget.GitHub, InstallTarget.Linux, InstallTarget.MacOS, InstallTarget.NixOS, ], }, version: { - type: "string", + control: { type: "text" }, }, },src/PageLayout/index.tsx (1)
26-32: Per-pane wrapper: add a class and avoid index fallback keys.Wrapping each pane is good. Add a specific class for styling hooks and prefer explicit keys over index to avoid reconciliation glitches when panes reorder.
Apply:
- {panes.map((pane, idx) => ( - <div key={pane.key ?? idx}>{pane}</div> - ))} + {panes.map((pane, idx) => ( + <div key={pane.key ?? idx} className="page-layout__pane"> + {pane} + </div> + ))}Please double-check any CSS that assumed panes were direct children of
.page-layout__panes.src/PageLayout/PageLayout.stories.tsx (1)
64-64: Add a key to the single pane element.Eliminates the “key” lint error and avoids index fallback keys.
- panes: [<Placeholder height="3em" label="Pane 1" />], + panes: [<Placeholder key="pane-1" height="3em" label="Pane 1" />],src/Navigation/index.scss (1)
55-61: Avoid fixed widths that can overflow on very narrow viewports.Use a responsive width to prevent horizontal scrolling.
- .navigation__element { - width: 320px; - - @media screen and (width < 650px) { - width: 268px; - } - } + .navigation__element { + /* Fit within viewport while capping at desktop size */ + width: min(320px, 100vw); + }src/Navigation/index.tsx (1)
8-11: Broadenchildrentype and preserve child keys.Support single-child usage and avoid index keys by normalizing via
React.Children.toArrayand using element keys when present.export interface NavigationProps { initialState?: "open" | "closed"; - children: React.ReactNode[]; + children: React.ReactNode; } @@ - {children.map((child, idx) => ( - <div key={idx} className="navigation__element"> - {child} - </div> - ))} + {React.Children.toArray(children).map((child, idx) => { + const key = + React.isValidElement(child) && child.key != null + ? String(child.key) + : String(idx); + return ( + <div key={key} className="navigation__element"> + {child} + </div> + ); + })}Also applies to: 47-51
src/Navigation/Navigation.stories.tsx (5)
27-29: Add keys to children array elements to satisfy React and lint.- children: [ - <Placeholder height="100%" label="Primary Element" />, - <Placeholder height="100%" label="Navigation Link" />, - ], + children: [ + <Placeholder key="primary" height="100%" label="Primary Element" />, + <Placeholder key="link" height="100%" label="Navigation Link" />, + ],
50-52: Add keys in InHeader story.- children: [ - <Placeholder height="100%" label="Lorem" />, - <Placeholder height="100%" label="Ipsum" />, - ], + children: [ + <Placeholder key="lorem" height="100%" label="Lorem" />, + <Placeholder key="ipsum" height="100%" label="Ipsum" />, + ],
69-71: Add keys in DefaultOpen story.- children: [ - <Placeholder height="100%" label="Lorem" />, - <Placeholder height="100%" label="Ipsum" />, - ], + children: [ + <Placeholder key="lorem" height="100%" label="Lorem" />, + <Placeholder key="ipsum" height="100%" label="Ipsum" />, + ],
94-96: Add keys in InHeaderOpen story.- children: [ - <Placeholder height="100%" label="Lorem" />, - <Placeholder height="100%" label="Ipsum" />, - ], + children: [ + <Placeholder key="lorem" height="100%" label="Lorem" />, + <Placeholder key="ipsum" height="100%" label="Ipsum" />, + ],
23-31: Optional: switch stories to JSX children instead of achildrenprop.This aligns with React conventions and eliminates the no-children-prop lint across stories.
Example for Default:
-export const Default: Story = { - args: { - initialState: undefined, - children: [ - <Placeholder key="primary" height="100%" label="Primary Element" />, - <Placeholder key="link" height="100%" label="Navigation Link" />, - ], - }, - render: (props) => ( +export const Default: Story = { + args: { initialState: undefined }, + render: (props) => ( <div style={{ height: "300px", width: "500px", border: "1px solid black", display: "flex", justifyContent: "end" }}> - <Navigation {...props} /> + <Navigation {...props}> + <Placeholder key="primary" height="100%" label="Primary Element" /> + <Placeholder key="link" height="100%" label="Navigation Link" /> + </Navigation> </div> ), };Apply similarly to the other stories.
Also applies to: 46-63, 65-88, 90-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
package.json(1 hunks)src/CodeBlock/CodeBlock.stories.tsx(2 hunks)src/CodeBlock/index.scss(1 hunks)src/CodeBlock/index.tsx(2 hunks)src/CodeFile/CodeFile.stories.tsx(0 hunks)src/CodeFile/index.scss(0 hunks)src/CodeFile/index.tsx(0 hunks)src/ColorProvider/index.tsx(4 hunks)src/Header/index.tsx(1 hunks)src/Highlight/index.scss(1 hunks)src/InstallCTA/FindAwsAMIs.tsx(1 hunks)src/InstallCTA/InstallCTA.stories.tsx(2 hunks)src/InstallCTA/InstallForNixOS.tsx(1 hunks)src/InstallCTA/InstallFromCurl.tsx(1 hunks)src/InstallCTA/NavTab.tsx(0 hunks)src/InstallCTA/TabSelector.tsx(2 hunks)src/InstallCTA/UseWithGitHubActions.tsx(1 hunks)src/InstallCTA/index.scss(2 hunks)src/InstallCTA/index.tsx(3 hunks)src/InstallCTA/types.ts(2 hunks)src/Link/index.scss(1 hunks)src/MacInstaller/index.scss(2 hunks)src/MacInstaller/index.tsx(1 hunks)src/Navigation/Navigation.stories.tsx(1 hunks)src/Navigation/index.scss(1 hunks)src/Navigation/index.tsx(1 hunks)src/PageLayout/PageLayout.stories.tsx(2 hunks)src/PageLayout/index.tsx(1 hunks)src/hooks/useHighlight.ts(3 hunks)src/hooks/useSystemColorScheme.ts(2 hunks)src/index.ts(1 hunks)src/sass/_mixins.scss(1 hunks)src/sass/_tokens.scss(2 hunks)
💤 Files with no reviewable changes (4)
- src/InstallCTA/NavTab.tsx
- src/CodeFile/index.scss
- src/CodeFile/index.tsx
- src/CodeFile/CodeFile.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- src/MacInstaller/index.tsx
- src/sass/_tokens.scss
- src/CodeBlock/index.tsx
- src/InstallCTA/TabSelector.tsx
- src/InstallCTA/InstallForNixOS.tsx
- src/Link/index.scss
- src/index.ts
- src/InstallCTA/UseWithGitHubActions.tsx
- src/CodeBlock/CodeBlock.stories.tsx
- package.json
- src/CodeBlock/index.scss
- src/hooks/useHighlight.ts
- src/InstallCTA/FindAwsAMIs.tsx
- src/Highlight/index.scss
- src/sass/_mixins.scss
- src/hooks/useSystemColorScheme.ts
- src/MacInstaller/index.scss
- src/InstallCTA/InstallFromCurl.tsx
- src/ColorProvider/index.tsx
- src/InstallCTA/index.scss
🧰 Additional context used
🧬 Code graph analysis (6)
src/Navigation/Navigation.stories.tsx (1)
src/Placeholder/index.tsx (1)
Placeholder(9-29)
src/PageLayout/PageLayout.stories.tsx (1)
src/Placeholder/index.tsx (1)
Placeholder(9-29)
src/Header/index.tsx (1)
src/index.ts (1)
HeaderProps(58-58)
src/Navigation/index.tsx (1)
src/index.ts (1)
NavigationProps(78-78)
src/InstallCTA/types.ts (1)
src/InstallCTA/index.tsx (1)
InstallTarget(17-17)
src/InstallCTA/InstallCTA.stories.tsx (1)
src/InstallCTA/index.tsx (1)
InstallTarget(17-17)
🪛 Biome (2.1.2)
src/Navigation/Navigation.stories.tsx
[error] 27-27: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 28-28: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 50-50: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 51-51: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 69-69: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 70-70: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 94-94: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 95-95: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
src/PageLayout/PageLayout.stories.tsx
[error] 55-55: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 64-64: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
src/InstallCTA/index.tsx
[error] 78-83: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (6)
src/InstallCTA/types.ts (1)
20-22: Confirm product intent: map Windows → Linux.If Windows users shouldn’t be funneled to Linux instructions, consider a neutral landing or explicit Windows guidance.
src/InstallCTA/index.tsx (1)
47-53: All mapped components accept an optionalversionprop.
MacInstaller declares MacInstallerProps{ version?: string }; InstallFromCurl, FindAwsAMIs, UseWithGitHubActions and InstallForNixOS useTabProps { version?: string }. No changes required.src/PageLayout/index.tsx (1)
26-32: LGTM: conditional rendering and per-item wrapping improves reconciliation.No functional concerns with the conditional panes rendering; this should reduce unnecessary DOM churn.
src/Header/index.tsx (1)
8-9: Public API tightening — verify downstream call sites & re-exportselements changed to React.ReactElement[] (src/Header/index.tsx); src/index.ts re-exports HeaderProps. Repo search found no in-repo usages of elements — verify stories and external consumers accept an array of React elements and update call-sites or types if they previously relied on ReactNode/other types.
src/Navigation/index.scss (1)
28-31: Confirm focus-visible styles on the toggle.Ensure
@include mixins.svg-button;provides a clear:focus-visibleoutline for keyboard users. If not, add an explicit focus style.src/Navigation/index.tsx (1)
32-41: A11y details look solid.Good use of useId, aria-controls/expanded, and hiding decorative SVG via
aria-hiddenandfocusable="false".
| const Header: FC<HeaderProps> = ({ logo, elements = [] }) => { | ||
| return ( | ||
| <header className="header" role="banner"> | ||
| <div className="header"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restore semantic landmark: use
(banner).Switching the root to a div loses the banner landmark. Use the semantic element (role optional).
- <div className="header">
+ <header className="header" role="banner">
...
- </div>
+ </header>Also applies to: 25-25
🤖 Prompt for AI Agents
In src/Header/index.tsx around lines 13 and 25 the root element was changed from
a semantic <header> (banner) to a <div>, removing the landmark; replace the <div
className="header"> (and the corresponding closing tag) with a semantic <header
className="header"> to restore the banner landmark, optionally add role="banner"
if you need explicit role support for older assistive tech, and ensure any
props/attributes and CSS selectors continue to work with the header element.
| children={[ | ||
| <Placeholder key="lorem" height="100%" label="Lorem" />, | ||
| <Placeholder key="ipsum" height="100%" label="Ipsum" />, | ||
| ]} | ||
| />, | ||
| ]} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t pass children prop explicitly; use JSX children.
Fixes the lint error and matches React conventions.
- <Navigation
- key="nav"
- children={[
- <Placeholder key="lorem" height="100%" label="Lorem" />,
- <Placeholder key="ipsum" height="100%" label="Ipsum" />,
- ]}
- />,
+ <Navigation key="nav">
+ <Placeholder key="lorem" height="100%" label="Lorem" />
+ <Placeholder key="ipsum" height="100%" label="Ipsum" />
+ </Navigation>,📝 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.
| children={[ | |
| <Placeholder key="lorem" height="100%" label="Lorem" />, | |
| <Placeholder key="ipsum" height="100%" label="Ipsum" />, | |
| ]} | |
| />, | |
| ]} | |
| <Navigation key="nav"> | |
| <Placeholder key="lorem" height="100%" label="Lorem" /> | |
| <Placeholder key="ipsum" height="100%" label="Ipsum" /> | |
| </Navigation>, | |
| ]} |
🧰 Tools
🪛 Biome (2.1.2)
[error] 55-55: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🤖 Prompt for AI Agents
In src/PageLayout/PageLayout.stories.tsx around lines 55 to 60, the story is
passing children via an explicit children prop which triggers a lint error and
goes against React conventions; remove the children={...} prop and instead place
the Placeholder elements as normal JSX children inside the component tag
(keeping their key and props intact), e.g. collapse the children array and nest
the two <Placeholder ... /> elements between the component's opening and closing
tags so the story uses standard JSX children.
f0f28a1 to
4a0338d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/sass/_tokens.scss (1)
101-107: Duplicate spacing values:baseandlgare both0.75rem.The
lgspacing token has the same value asbase, which breaks the expected progression (xs<sm<base<lg<xl). This appears to be a copy-paste oversight.Suggested fix
$spacing: ( xs: 0.25rem, sm: 0.5rem, base: 0.75rem, - lg: 0.75rem, + lg: 1rem, - xl: 1rem, + xl: 1.5rem, );src/theme.ts (1)
1-20: Fix Prettier formatting issues in this fileRun
npx prettier --write src/theme.tsto resolve the code style violations.src/index.ts (1)
25-25: Remove or restore the missingCodeFilemodule—the export at line 25 references a non-existent module and breaks the build.The
CodeFilemodule does not exist in the codebase. Remove the export or restore the module with the matching implementation.
🤖 Fix all issues with AI agents
In `@src/InstallCTA/InstallCTA.stories.tsx`:
- Around line 33-38: The story object MacOSLatest in InstallCTA.stories.tsx has
an incorrect human-readable name ("macOS Tagged") that doesn't match its intent
(no version provided); update the story by either changing the name property to
"macOS Latest" or removing the name field so the framework uses the exported
identifier MacOSLatest as the display name—edit the MacOSLatest object
(references: MacOSLatest, InstallTarget.MacOS) accordingly.
In `@src/PageLayout/PageLayout.stories.tsx`:
- Line 73: Missing key on the Placeholder element in the panes array: add a
unique key prop to the Placeholder in the panes: [Placeholder ...] entry (e.g.,
key="pane-1" or similar) to match the Default story's keyed items and follow
React list-key conventions; update the Placeholder instance used in that story
so it has a stable key property.
In `@src/sass/_mixins.scss`:
- Around line 86-99: The `@mixin` shadow defines identical box-shadow rules both
in the base block and inside the `@include` light-mode block, which is redundant;
either remove the `@include` light-mode block entirely or replace its box-shadow
with the intended light-mode values. Locate the `@mixin` shadow and the `@include`
light-mode inside it, then delete the duplicated box-shadow block if no separate
light-mode styling is needed, or update the box-shadow lines within the `@include`
light-mode to the correct light-mode colors/offsets to reflect the intended
visual difference.
♻️ Duplicate comments (5)
src/MacInstaller/index.tsx (1)
17-21: Encodeversionin URL to prevent malformed links.If
versioncontains special characters (e.g., spaces,+,/), the URL will be malformed. ApplyencodeURIComponentto the version segment.Proposed fix
- const urlPath = version ? `tag/v${version}` : `stable`; + const urlPath = version ? `tag/v${encodeURIComponent(version)}` : `stable`;src/InstallCTA/InstallFromCurl.tsx (2)
6-12: URL-encodeversionto avoid broken URLs.Special characters in
version(e.g.,+, spaces) can break the URL when users copy-paste. Encode for safety.const installScript = (version: string | undefined) => { - if (typeof version === "undefined") { + if (typeof version === "undefined" || version === "") { return "curl -fsSL https://install.determinate.systems/nix | sh -s -- install --determinate"; } else { - return `curl -fsSL https://install.determinate.systems/nix/tag/v${version} | sh -s -- install --determinate`; + const v = encodeURIComponent(version); + return `curl -fsSL https://install.determinate.systems/nix/tag/v${v} | sh -s -- install --determinate`; } };
14-14: Props typing bug:FCwithout generics erases theversionprop.The component is typed as
FCwithout generics, which means TypeScript consumers won't see theversionprop. This should beFC<TabProps>.-const InstallFromCurl: FC = ({ version }: TabProps) => ( +const InstallFromCurl: FC<TabProps> = ({ version }) => (src/CodeBlock/index.tsx (1)
18-18: Makekindoptional for backward compatibility.The prop has a default value in the component (line 31) but is required in the interface, forcing callers to provide it even when they want the default.
- kind: "snippet" | "file"; + kind?: "snippet" | "file";src/InstallCTA/index.tsx (1)
77-84: Addkeyprop and use strict equality.Missing
keyon mapped elements and loose equality==should be===.{ctaTabs.map(([target, icon]) => ( <TabSelector + key={target} name={target} icon={icon} - active={activeTab == target} + active={activeTab === target} onClick={() => setActiveTab(target)} /> ))}
🧹 Nitpick comments (4)
src/MacInstaller/index.tsx (1)
14-16: Consider removing the redundant type annotation.Since
FC<MacInstallerProps>already types the props parameter, the explicit: MacInstallerPropson the destructured object is redundant.Suggested simplification
-const MacInstaller: FC<MacInstallerProps> = ({ - version, -}: MacInstallerProps) => { +const MacInstaller: FC<MacInstallerProps> = ({ version }) => {src/CodeBlock/CodeBlock.stories.tsx (1)
95-115: Consider explicitly settingallowCopyfor consistency.The
AWSstory relies on implicit defaults forallowCopyandallowDownload. While this works, explicit values (like inGithubCI) make the story's intent clearer and serve as better documentation.src/CodeBlock/index.tsx (1)
50-55: Consider using.at(-1)for last element access.
.at(-1)is more idiomatic than.split("/").reverse()[0]for getting the last segment.♻️ Suggested refactor
- <DownloadButton - data={trimmedCode} - filename={downloadAs.split("/").reverse()[0]} - /> + <DownloadButton + data={trimmedCode} + filename={downloadAs.split("/").at(-1) ?? downloadAs} + />src/InstallCTA/InstallCTA.stories.tsx (1)
40-64: Consider adding explicitnameproperties for consistency.
LinuxLatest,AWSLatestlack explicit names while others have them. For consistency, either add names to all or remove from all (using export names as defaults).
| export const MacOSLatest: Story = { | ||
| name: "macOS Tagged", | ||
| args: { | ||
| initialTab: InstallTarget.MacOS, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Story name mismatch: MacOSLatest is named "macOS Tagged".
The MacOSLatest story has name: "macOS Tagged" but doesn't include a version. This appears to be a copy-paste error—it should likely be "macOS Latest" or removed entirely to use the default export name.
export const MacOSLatest: Story = {
- name: "macOS Tagged",
+ name: "macOS Latest",
args: {
initialTab: InstallTarget.MacOS,
},
};📝 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.
| export const MacOSLatest: Story = { | |
| name: "macOS Tagged", | |
| args: { | |
| initialTab: InstallTarget.MacOS, | |
| }, | |
| }; | |
| export const MacOSLatest: Story = { | |
| name: "macOS Latest", | |
| args: { | |
| initialTab: InstallTarget.MacOS, | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In `@src/InstallCTA/InstallCTA.stories.tsx` around lines 33 - 38, The story object
MacOSLatest in InstallCTA.stories.tsx has an incorrect human-readable name
("macOS Tagged") that doesn't match its intent (no version provided); update the
story by either changing the name property to "macOS Latest" or removing the
name field so the framework uses the exported identifier MacOSLatest as the
display name—edit the MacOSLatest object (references: MacOSLatest,
InstallTarget.MacOS) accordingly.
| /> | ||
| ), | ||
| content: <div style={{ height: "100em" }} />, | ||
| panes: [<Placeholder height="3em" label="Pane 1" />], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine its content around line 73
if [ -f "src/PageLayout/PageLayout.stories.tsx" ]; then
echo "=== File found. Displaying lines 65-80 ==="
sed -n '65,80p' src/PageLayout/PageLayout.stories.tsx | cat -n
else
echo "File not found at src/PageLayout/PageLayout.stories.tsx"
# Search for similar files
find . -name "*PageLayout*.stories.tsx" -type f 2>/dev/null
fiRepository: DeterminateSystems/ui
Length of output: 498
🏁 Script executed:
# Check for Biome configuration
echo "=== Checking for Biome config ==="
find . -maxdepth 2 -type f \( -name "biome.json" -o -name "biome.jsonc" \o -name ".biomeignore" \) 2>/dev/null | head -20Repository: DeterminateSystems/ui
Length of output: 98
🏁 Script executed:
# Look for the entire PageLayout.stories.tsx structure to understand context
if [ -f "src/PageLayout/PageLayout.stories.tsx" ]; then
echo "=== Full file structure (first 90 lines) ==="
head -90 src/PageLayout/PageLayout.stories.tsx | cat -n
fiRepository: DeterminateSystems/ui
Length of output: 2739
Add a key to the Placeholder element in the panes array for consistency.
Line 73 renders a Placeholder element inside an array without a key, while the Default story provides keys to similar elements (lines 34–35). Following React conventions, all elements in arrays should have keys to maintain consistency and avoid potential issues during component updates.
🔧 Suggested fix
- panes: [<Placeholder height="3em" label="Pane 1" />],
+ panes: [<Placeholder key="pane-1" height="3em" label="Pane 1" />],📝 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.
| panes: [<Placeholder height="3em" label="Pane 1" />], | |
| panes: [<Placeholder key="pane-1" height="3em" label="Pane 1" />], |
🧰 Tools
🪛 Biome (2.1.2)
[error] 73-73: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🤖 Prompt for AI Agents
In `@src/PageLayout/PageLayout.stories.tsx` at line 73, Missing key on the
Placeholder element in the panes array: add a unique key prop to the Placeholder
in the panes: [Placeholder ...] entry (e.g., key="pane-1" or similar) to match
the Default story's keyed items and follow React list-key conventions; update
the Placeholder instance used in that story so it has a stable key property.
| @mixin shadow { | ||
| box-shadow: | ||
| 0 0 #0000, | ||
| 0 0 #0000, | ||
| 0 1px 3px 0 rgba(0, 0, 0, 0.1), | ||
| 0 1px 2px -1px rgba(0, 0, 0, 0.1); | ||
| @include light-mode { | ||
| box-shadow: | ||
| 0 0 #0000, | ||
| 0 0 #0000, | ||
| 0 1px 3px 0 rgba(0, 0, 0, 0.1), | ||
| 0 1px 2px -1px rgba(0, 0, 0, 0.1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Identical box-shadow values for dark and light modes.
The light-mode block contains the exact same box-shadow as the default. This is either dead code or the light-mode values weren't updated after copy-paste.
If intentional, remove the redundant block
`@mixin` shadow {
box-shadow:
0 0 `#0000`,
0 0 `#0000`,
0 1px 3px 0 rgba(0, 0, 0, 0.1),
0 1px 2px -1px rgba(0, 0, 0, 0.1);
- `@include` light-mode {
- box-shadow:
- 0 0 `#0000`,
- 0 0 `#0000`,
- 0 1px 3px 0 rgba(0, 0, 0, 0.1),
- 0 1px 2px -1px rgba(0, 0, 0, 0.1);
- }
}📝 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.
| @mixin shadow { | |
| box-shadow: | |
| 0 0 #0000, | |
| 0 0 #0000, | |
| 0 1px 3px 0 rgba(0, 0, 0, 0.1), | |
| 0 1px 2px -1px rgba(0, 0, 0, 0.1); | |
| @include light-mode { | |
| box-shadow: | |
| 0 0 #0000, | |
| 0 0 #0000, | |
| 0 1px 3px 0 rgba(0, 0, 0, 0.1), | |
| 0 1px 2px -1px rgba(0, 0, 0, 0.1); | |
| } | |
| } | |
| `@mixin` shadow { | |
| box-shadow: | |
| 0 0 `#0000`, | |
| 0 0 `#0000`, | |
| 0 1px 3px 0 rgba(0, 0, 0, 0.1), | |
| 0 1px 2px -1px rgba(0, 0, 0, 0.1); | |
| } |
🤖 Prompt for AI Agents
In `@src/sass/_mixins.scss` around lines 86 - 99, The `@mixin` shadow defines
identical box-shadow rules both in the base block and inside the `@include`
light-mode block, which is redundant; either remove the `@include` light-mode
block entirely or replace its box-shadow with the intended light-mode values.
Locate the `@mixin` shadow and the `@include` light-mode inside it, then delete the
duplicated box-shadow block if no separate light-mode styling is needed, or
update the box-shadow lines within the `@include` light-mode to the correct
light-mode colors/offsets to reflect the intended visual difference.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.