-
Notifications
You must be signed in to change notification settings - Fork 7
SSR safe IDs, remove PropTypes #1306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Jira Issue: https://appfolio.atlassian.net/browse/FEE-1480 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors ID generation to support Server-Side Rendering (SSR) by making all generated IDs client-side only. The main approach is using a new useUniqueClientId hook that generates IDs in useEffect after the initial render, ensuring SSR and client hydration compatibility.
Key changes:
- Introduced
useUniqueClientIdhook that defers ID generation to client-side viauseEffect - Removed unnecessary IDs where accessibility can be maintained without them (using
titleattributes, wrapping labels, or element refs) - Converted several class components to function components (FormChoice, CheckboxBooleanInput, Icon)
- Consolidated Icon implementation by merging FontAwesomeAPM into Icon component
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hooks/useUniqueClientId.ts | New hook for SSR-compatible client-side ID generation |
| src/components/TruncatedText/TruncatedText.tsx | Replaced ID-based tooltip targeting with refs |
| src/components/Tree/TreeItem.tsx | Removed unnecessary checkbox ID |
| src/components/Tooltip/TooltipButton.tsx | Migrated to useUniqueClientId hook, removed aria-describedby |
| src/components/Table/SortableTable.js | Replaced label+ID pattern with title attributes, moved defaultProps |
| src/components/Status/Status.tsx | Updated import to use IconProps instead of FontAwesomeAPMProps |
| src/components/List/components/SortHeader.tsx | Wrapped label around select input, removed PropTypes |
| src/components/List/components/FilterHeader.tsx | Replaced label+ID with title attribute, removed PropTypes |
| src/components/List/SortableList.tsx | Removed PropTypes, added Item type interface |
| src/components/List/ListItem.tsx | Migrated to useUniqueClientId, replaced label with title |
| src/components/List/List.tsx | Migrated to useUniqueClientId, replaced Row with fragment, removed PropTypes |
| src/components/List/List.spec.js | Updated tests from getByLabelText to getByTitle |
| src/components/Icon/Icon.tsx | Merged FontAwesomeAPM, converted to forwardRef function component |
| src/components/Icon/FontAwesomeAPM.tsx | Deleted, merged into Icon.tsx |
| src/components/HelpBubble/HelpBubble.tsx | Migrated to useUniqueClientId hook |
| src/components/HasManyFields/HasManyFieldsRow.tsx | Replaced ID-based tooltip with ref-based approach |
| src/components/HasManyFields/HasManyFieldsRow.spec.js | Removed tests for disabled tooltip behavior |
| src/components/Form/FormChoice.tsx | Converted from class to function, added useUniqueClientId, fixed cursor style bug |
| src/components/Form/FormChoice.spec.js | Removed ID generation test |
| src/components/Form/FormChoice.js | Deleted, replaced with TypeScript version |
| src/components/Form/FormChoice.d.ts | Deleted, types now in .tsx file |
| src/components/Checkbox/CheckboxBooleanInput.tsx | Converted from class to function, added useUniqueClientId |
Comments suppressed due to low confidence (1)
src/components/HasManyFields/HasManyFieldsRow.tsx:27
- The default props have been moved inline in the function parameters, but the
noopfunction is still defined separately. For consistency, consider either defining all defaults inline (replacingonDelete = noopwith an inline function) or removing the now-unusednoopconstant since it's only used once.
const noop = () => undefined;
interface HasManyFieldsRowProps {
children: React.ReactNode;
className?: string;
onDelete?: React.MouseEventHandler<any>;
deletable?: boolean;
deleteProps?: ConfirmationButtonProps;
disabled?: boolean;
disabledReason?: React.ReactNode;
disabledReasonPlacement?: Placement;
}
const HasManyFieldsRow = ({
children,
className,
disabledReason,
onDelete = noop,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6f8b824 to
f44bc54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 55 out of 57 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3c93c43 to
c7a27d1
Compare
…d for id generation
c7a27d1 to
1d32caf
Compare
|
Released prerelease version |
This pull request adds a server-safe utility for unique ID generation. It uses an incrementing counter scoped to the module that resets after
Number.MAX_SAFE_INTEGER. This prevents an overflow server-side because the counter is server-side state when using SSR.I tried to remove generated IDs but too many Property tests rely on them.
It does not fix hydration mismatches. I could not find a solution that keeps the IDs the same server and client side using an incrementing counter. Incrementing counter ID is needed for Property tests. It's possible to introduce Context to hold the counter and we can use this new context in apps that SSR.
PR removes all prop-types because it's depreciated and noisy.