fix(#736): updates for doc issues found in apsara audit#756
fix(#736): updates for doc issues found in apsara audit#756
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe pull request introduces several updates across documentation demos and component functionality. A new 404 error page component is added with custom styling. The DataTable demo is refactored to support both standard and virtualized rendering modes with tabbed examples. Multiple documentation demos are enhanced with tabbed variants showing different implementation approaches (uncontrolled/controlled Search patterns, dropdown Calendar layout). The Search and Accordion components are updated to support controlled/uncontrolled usage patterns and improved value normalization. InputField documentation is expanded with an interactive chip input example. Various CSS styling adjustments are made to preview and demo containers. Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Shreyag02
requested review from
paanSinghCoder and
rohanchkrabrty
and removed request for
rohanchkrabrty
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/www/src/content/docs/components/input-field/demo.ts (1)
131-159: Consider deduplicating/guarding chip entries.Minor UX polish for the demo: pressing Enter on an already-present label will add a duplicate chip (e.g.,
Tag1twice), and very long input produces a chip that may push the layout. Not a bug, but since this demo is meant to showcase realistic usage, a small guard would make it a better reference implementation for consumers.♻️ Suggested tweak
onKeyDown={(e) => { - if (e.key === "Enter" && input.trim()) { - setChips([...chips, { label: input.trim() }]); - setInput(""); + if (e.key === "Enter" && input.trim()) { + e.preventDefault(); + const label = input.trim(); + if (!chips.some((c) => c.label === label)) { + setChips([...chips, { label }]); + } + setInput(""); } }}The
e.preventDefault()also avoids any accidental form submission if this demo is ever embedded inside a<form>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/content/docs/components/input-field/demo.ts` around lines 131 - 159, In ChipInputDemo, prevent adding duplicate or excessively long chips by enhancing the onKeyDown handler on the InputField: call e.preventDefault(), compute const value = input.trim(), check that value is non-empty, not already present in chips (compare against chips.map(c=>c.label)), and optionally limit length (e.g., value.length <= MAX_LABEL_LENGTH) before calling setChips([...chips, {label: value}]) and clearing setInput(""); update the chips onRemove logic remains the same.apps/www/src/app/not-found.tsx (1)
13-17: Render the 404 copy with semantic elements.
Textdefaults tospan, so this page lacks heading and paragraph semantics for assistive-tech navigation. Use therenderprop to render the status ash1and message aspwhile preserving visual styling.Proposed semantic markup update
- <Text size={10} weight='bold'> + <Text render={<h1 />} size={10} weight='bold' style={{ margin: 0 }}> 404 </Text> <div className={styles.divider} /> - <Text size={7}>This page could not be found.</Text> + <Text render={<p />} size={7} style={{ margin: 0 }}> + This page could not be found. + </Text>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/app/not-found.tsx` around lines 13 - 17, The Text component is currently rendering as a span, so update the two Text usages to use the render prop to provide semantic elements: render the status Text (the one with size={10} weight='bold') as an h1 and render the message Text (size={7}) as a p, preserving their size/weight props and keeping the existing divider (styles.divider) in place; locate the Text usages in not-found.tsx and add render="h1" and render="p" respectively.
🤖 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/www/src/content/docs/components/search/demo.ts`:
- Around line 46-60: The ControlledSearch component is defined but never
rendered; after the ControlledSearch function declaration add an instance of it
(i.e., render <ControlledSearch />) so the controlled demo appears in the live
preview — locate the ControlledSearch function in the diff and append a JSX
mount of ControlledSearch right after its definition.
In `@packages/raystack/components/search/search.tsx`:
- Around line 35-36: The uncontrolled Search initializes internalValue to '' so
a passed defaultValue is ignored and both value and defaultValue get forwarded;
change the uncontrolled initialization in the Search component so internalValue
is initialized from props.defaultValue (e.g., const [internalValue,
setInternalValue] = useState(() => props.defaultValue ?? '')), keep currentValue
= isControlled ? controlledValue : internalValue, and when rendering the
underlying input only pass value when isControlled (so the uncontrolled case
only forwards defaultValue in ...props); update references to internalValue,
setInternalValue, currentValue, isControlled and controlledValue accordingly.
---
Nitpick comments:
In `@apps/www/src/app/not-found.tsx`:
- Around line 13-17: The Text component is currently rendering as a span, so
update the two Text usages to use the render prop to provide semantic elements:
render the status Text (the one with size={10} weight='bold') as an h1 and
render the message Text (size={7}) as a p, preserving their size/weight props
and keeping the existing divider (styles.divider) in place; locate the Text
usages in not-found.tsx and add render="h1" and render="p" respectively.
In `@apps/www/src/content/docs/components/input-field/demo.ts`:
- Around line 131-159: In ChipInputDemo, prevent adding duplicate or excessively
long chips by enhancing the onKeyDown handler on the InputField: call
e.preventDefault(), compute const value = input.trim(), check that value is
non-empty, not already present in chips (compare against chips.map(c=>c.label)),
and optionally limit length (e.g., value.length <= MAX_LABEL_LENGTH) before
calling setChips([...chips, {label: value}]) and clearing setInput(""); update
the chips onRemove logic remains the same.
🪄 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: 64524df2-bf9e-4881-9147-e69994eb7b89
📒 Files selected for processing (15)
apps/www/src/app/not-found.module.cssapps/www/src/app/not-found.tsxapps/www/src/components/datatable-demo.tsxapps/www/src/components/demo/demo-preview.tsxapps/www/src/components/demo/demo.tsxapps/www/src/components/demo/styles.module.cssapps/www/src/components/preview/preview.module.cssapps/www/src/content/docs/components/breadcrumb/demo.tsapps/www/src/content/docs/components/calendar/demo.tsapps/www/src/content/docs/components/datatable/demo.tsapps/www/src/content/docs/components/input-field/demo.tsapps/www/src/content/docs/components/input-field/index.mdxapps/www/src/content/docs/components/search/demo.tspackages/raystack/components/accordion/accordion-root.tsxpackages/raystack/components/search/search.tsx
| function ControlledSearch() { | ||
| const [value, setValue] = React.useState("Searchable text"); | ||
|
|
||
| return ( | ||
| <Flex direction="column" gap="medium" align="center"> | ||
| <Search | ||
| placeholder="Type to search..." | ||
| value={value} | ||
| onChange={(e) => setValue(e.target.value)} | ||
| showClearButton | ||
| onClear={() => setValue("")} | ||
| /> | ||
| </Flex> | ||
| ); | ||
| }` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the controlled Search demo defines and renders the component.
python - <<'PY'
from pathlib import Path
p = Path("apps/www/src/content/docs/components/search/demo.ts")
s = p.read_text()
start = s.index("function ControlledSearch")
snippet = s[start:start + 700]
print(snippet)
print("\nRenders ControlledSearch:", "<ControlledSearch" in snippet)
PYRepository: raystack/apsara
Length of output: 563
Render the controlled demo component.
The ControlledSearch function is defined but never instantiated, so the controlled demo won't display in the live preview. Add <ControlledSearch /> after the function declaration to mount it.
🐛 Proposed fix
function ControlledSearch() {
const [value, setValue] = React.useState("Searchable text");
return (
<Flex direction="column" gap="medium" align="center">
<Search
placeholder="Type to search..."
value={value}
onChange={(e) => setValue(e.target.value)}
showClearButton
onClear={() => setValue("")}
/>
</Flex>
);
- }`
+ }
+
+ <ControlledSearch />`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/content/docs/components/search/demo.ts` around lines 46 - 60,
The ControlledSearch component is defined but never rendered; after the
ControlledSearch function declaration add an instance of it (i.e., render
<ControlledSearch />) so the controlled demo appears in the live preview —
locate the ControlledSearch function in the diff and append a JSX mount of
ControlledSearch right after its definition.
| const [internalValue, setInternalValue] = useState(''); | ||
| const currentValue = isControlled ? controlledValue : internalValue; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm Search currently ignores defaultValue while always passing value to InputField.
rg -n -C4 'defaultValue|value=\{currentValue\}|const \[internalValue' packages/raystack/components/search/search.tsxRepository: raystack/apsara
Length of output: 825
🏁 Script executed:
cd packages/raystack/components/search && cat -n search.tsx | head -100Repository: raystack/apsara
Length of output: 3433
🏁 Script executed:
# Also check the SearchProps interface definition
rg -n 'interface SearchProps|type SearchProps' packages/raystack/components/search/Repository: raystack/apsara
Length of output: 184
🏁 Script executed:
# Check InputFieldProps definition to confirm it includes defaultValue
rg -n 'interface InputFieldProps|type InputFieldProps' -A 15 packages/raystack/components/input-field/Repository: raystack/apsara
Length of output: 1487
🏁 Script executed:
# Also verify what happens when both value and defaultValue are passed to InputField
cat -n packages/raystack/components/input-field/input-field.tsx | head -50Repository: raystack/apsara
Length of output: 1655
🏁 Script executed:
# Check what InputPrimitive.Props includes from `@base-ui/react`
# First, let's see if there are any imports or type definitions we can examine
rg -n "InputPrimitive" packages/raystack/components/input-field/input-field.tsx -B 5 -A 5Repository: raystack/apsara
Length of output: 1115
🏁 Script executed:
# Let's check if there are any test files or examples showing defaultValue usage
fd -e test.tsx -e test.ts -e spec.tsx packages/raystack/components/search/Repository: raystack/apsara
Length of output: 508
🏁 Script executed:
# Check if defaultValue is used anywhere in the codebase with Search component
rg 'defaultValue' packages/raystack/components/search/ -nRepository: raystack/apsara
Length of output: 41
🏁 Script executed:
# Also check the InputField implementation to see how it handles value and defaultValue
rg -n 'defaultValue' packages/raystack/components/input-field/input-field.tsx -C 3Repository: raystack/apsara
Length of output: 41
🏁 Script executed:
# Check the full InputField implementation, especially how it handles props
cat -n packages/raystack/components/input-field/input-field.tsx | sed -n '40,120p'Repository: raystack/apsara
Length of output: 2701
🏁 Script executed:
# Look for any usage examples of Search component in tests or stories
fd . packages/raystack/components/search/ -type fRepository: raystack/apsara
Length of output: 229
🏁 Script executed:
# Check if there's a storybook or test file for Search
find packages/raystack/components/search -type f \( -name "*.stories.*" -o -name "*.test.*" -o -name "*.spec.*" \)Repository: raystack/apsara
Length of output: 120
🏁 Script executed:
# Check the test file for Search to understand expected behavior
cat -n packages/raystack/components/search/__tests__/search.test.tsxRepository: raystack/apsara
Length of output: 9584
Honor defaultValue in uncontrolled Search.
SearchProps accepts defaultValue through InputFieldProps, but uncontrolled state initializes to an empty string while defaultValue is forwarded via ...props. This causes <Search defaultValue="foo" /> to render with value="" and pass both value and defaultValue to the underlying input, making React treat it as controlled and ignoring the default.
🐛 Proposed fix
export function Search({
className,
disabled,
placeholder = 'Search',
size,
showClearButton,
onClear,
value: controlledValue,
onChange,
width = '100%',
variant = 'default',
inputRef: externalRef,
+ defaultValue = '',
...props
}: SearchProps) {
const internalRef = useRef<HTMLInputElement>(null);
const inputRef = externalRef ?? internalRef;
const isControlled = controlledValue !== undefined;
- const [internalValue, setInternalValue] = useState('');
+ const [internalValue, setInternalValue] = useState(() =>
+ defaultValue == null ? '' : String(defaultValue)
+ );
const currentValue = isControlled ? controlledValue : internalValue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/search/search.tsx` around lines 35 - 36, The
uncontrolled Search initializes internalValue to '' so a passed defaultValue is
ignored and both value and defaultValue get forwarded; change the uncontrolled
initialization in the Search component so internalValue is initialized from
props.defaultValue (e.g., const [internalValue, setInternalValue] = useState(()
=> props.defaultValue ?? '')), keep currentValue = isControlled ?
controlledValue : internalValue, and when rendering the underlying input only
pass value when isControlled (so the uncontrolled case only forwards
defaultValue in ...props); update references to internalValue, setInternalValue,
currentValue, isControlled and controlledValue accordingly.
| showClearButton?: boolean; | ||
| onClear?: () => void; | ||
| variant?: 'default' | 'borderless'; | ||
| inputRef?: RefObject<HTMLInputElement | null>; |
There was a problem hiding this comment.
I think we can just name this ref as it will be almost always known that ref will be pointing to the input field.
Moreover, lets add this in the Search docs as well.
| (onValueChange as (value: string | undefined) => void)( | ||
| newValue[0] || undefined | ||
| ); | ||
| (onValueChange as (value: string) => void)(newValue[0] ?? ''); |
There was a problem hiding this comment.
Why did we change newValue[0] || undefined to newValue[0] ?? ''?
| @@ -151,9 +151,9 @@ export const iconsDemo = { | |||
| <Breadcrumb> | |||
| <Breadcrumb.Item href="/" leadingIcon={<BellIcon />}>Home</Breadcrumb.Item> | |||
There was a problem hiding this comment.
href='/' should also be href='#'?
| <Text size={10} weight='bold'> | ||
| 404 | ||
| </Text> | ||
| <div className={styles.divider} /> |
There was a problem hiding this comment.
We have a Separator component in Apsara. It can be used here with the vertical orientation.
https://apsara-raystack.vercel.app/docs/components/separator
| const internalRef = useRef<HTMLInputElement>(null); | ||
| const inputRef = externalRef ?? internalRef; | ||
| const isControlled = controlledValue !== undefined; | ||
| const [internalValue, setInternalValue] = useState(''); |
There was a problem hiding this comment.
The defaultValue prop is probably getting ignored. It is inherited from InputField component. Please check this once.
Previously if the value prop is undefined it treats Search as uncontrolled and defaultValue is rendered.
2 participants
Description
Resolves multiple documentation issues identified in the Apsara UI audit. Fixes include:
Type of Change
How Has This Been Tested?
Checklist:
Related Issues
#736