core&ui: assignment auto complete - #1061
Conversation
|
Thank you for your submission, we really appreciate it. Comment |
WalkthroughAdds DomainApi.groups query to validate permissions and return groups for a domain filtered by exact names or a case-insensitive search. Exposes AssignSelectAutoComplete in packages/ui-default/api.ts. Adds a DOM-attached wrapper class AssignSelectAutoComplete (extends AutoComplete) with a custom value() and global assignment. Introduces a React AssignSelectAutoComplete component that loads users and groups, maps them to unified items, and renders multi-select UI; it is initialized on the contest edit page. Adds try/catch error handling around async fetches in the AutoComplete core to log errors and reset UI state. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
I have read the CLA Document and I hereby sign the CLA |
![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 (1)
packages/ui-default/components/autocomplete/AssignSelectAutoComplete.tsx (1)
7-17: Remove hardcodedmulti: trueto respect caller's configuration.The constructor hardcodes
multi: truein props (line 12) but then spreads options (line 15), allowing the hardcoded value to be overridden. This creates ambiguity about the intended behavior.Since the generic parameter
Multisuggests configurability and the caller incontest_edit.page.tsexplicitly passesmulti: true, the hardcoded value is redundant.Apply this diff to remove the redundancy:
constructor($dom, options: AutoCompleteOptions<Multi> = {}) { super($dom, { classes: 'assign-select', component: AssignSelectAutoCompleteFC, props: { - multi: true, height: 'auto', }, ...options, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/hydrooj/src/handler/domain.ts(1 hunks)packages/ui-default/api.ts(1 hunks)packages/ui-default/components/autocomplete/AssignSelectAutoComplete.tsx(1 hunks)packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx(1 hunks)packages/ui-default/pages/contest_edit.page.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/ui-default/components/autocomplete/AssignSelectAutoComplete.tsx (1)
packages/ui-default/components/autocomplete/index.tsx (1)
AutoCompleteOptions(6-20)
packages/ui-default/pages/contest_edit.page.ts (2)
packages/ui-default/api.ts (1)
AssignSelectAutoComplete(34-34)packages/ui-default/components/autocomplete/AssignSelectAutoComplete.tsx (1)
AssignSelectAutoComplete(4-22)
packages/hydrooj/src/handler/domain.ts (3)
framework/framework/api.ts (1)
Query(29-29)packages/hydrooj/src/libs.ts (1)
Schema(14-14)packages/hydrooj/src/service/layers/user.ts (1)
ctx(5-18)
packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx (3)
packages/ui-default/components/autocomplete/AssignSelectAutoComplete.tsx (1)
AssignSelectAutoComplete(4-22)packages/ui-default/utils/index.ts (1)
api(5-9)packages/hydrooj/src/interface.ts (2)
Udoc(79-93)GDoc(111-116)
🔇 Additional comments (4)
packages/ui-default/pages/contest_edit.page.ts (1)
3-3: LGTM!The integration of
AssignSelectAutoCompletefollows the same pattern as existing autocomplete widgets and is properly initialized with multi-select enabled.Also applies to: 15-15
packages/ui-default/api.ts (1)
27-27: LGTM!The export follows the established pattern for other autocomplete components.
Also applies to: 34-34
packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx (2)
43-75: Add explicit type discrimination for keys
ThefetchItemsimplementation uses numeric regex to separate user IDs from group names (line 48), which misclassifies numeric group names and relies on an undocumented key-format contract. Consider prefixing keys (e.g.,"user:123","group:admins"), passing typed items instead of raw keys, or documenting/enforcing non-numeric group names.
Verify that theusersendpoint supports theautoparameter with the backend/API spec.
20-42: users API endpoint exists with search support
Theusersquery is defined inpackages/hydrooj/src/handler/user.tswithsearch: Schema.string(), matching the UI’sapi('users', { search })call.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/hydrooj/src/handler/domain.ts (1)
440-440: Mark domainId as required in the schema.The
domainIdparameter should be marked as required to prevent potential runtime errors whenargs.domainIdis undefined.Apply this diff:
- domainId: Schema.string(), + domainId: Schema.string().required(),packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx (1)
104-114: RemoveisRequiredfromonChangePropType.The
onChangeprop is marked asisRequired, but the underlyingAutoCompletecomponent makes it optional (defaulting to a no-op). The PropType should reflect this optionality.Based on past review comments.
Apply this diff:
AssignSelectAutoComplete.propTypes = { width: PropTypes.string, height: PropTypes.string, listStyle: PropTypes.object, - onChange: PropTypes.func.isRequired, + onChange: PropTypes.func, multi: PropTypes.bool, selectedKeys: PropTypes.arrayOf(PropTypes.string), allowEmptyQuery: PropTypes.bool, freeSolo: PropTypes.bool, freeSoloConverter: PropTypes.func, };
🧹 Nitpick comments (2)
packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx (2)
73-89: Simplify the renderItem structure.The conditional rendering of the avatar creates an asymmetric structure. Consider refactoring to always render a
media__leftcontainer and conditionally render its content, which improves consistency and maintainability.Based on past review comments.
Apply this diff:
renderItem={(item) => ( <div className="media"> - {item.type === 'user' ? ( - <div className="media__left medium"> + <div className="media__left medium"> + {item.type === 'user' && ( <img className="small user-profile-avatar" alt="" src={item.avatarUrl} width="30" height="30" /> - </div> - ) : null} + )} + </div> <div className="media__body medium"> <div className="assign-select__name"> {item.name}{item.type === 'user' && item.displayName && ` (${item.displayName})`} </div> <div className="assign-select__desc"> {item.type === 'group' ? <>Group • {item.uids?.length || 0} users</> : <>User • UID = {item.key}</>} </div> </div> </div> )}
104-114: Consider removing PropTypes in favor of TypeScript.The component uses both TypeScript types (
AutoCompleteProps<AssignItem>) and runtime PropTypes validation. Since TypeScript provides compile-time type safety, PropTypes add maintenance overhead without significant benefit in a TypeScript codebase. Consider removing PropTypes unless runtime validation is specifically required for this component.If you decide to keep PropTypes, ensure they stay in sync with the TypeScript types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/hydrooj/src/handler/domain.ts(1 hunks)packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/hydrooj/src/handler/domain.ts (2)
packages/hydrooj/src/model/builtin.ts (2)
PERM(9-104)PRIV(242-269)packages/hydrooj/src/error.ts (1)
PermissionError(22-27)
packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx (3)
packages/hydrooj/src/interface.ts (2)
Udoc(79-93)GDoc(111-116)packages/ui-default/components/autocomplete/AssignSelectAutoComplete.tsx (1)
AssignSelectAutoComplete(4-22)packages/ui-default/utils/index.ts (1)
api(5-9)
🔇 Additional comments (1)
packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx (1)
46-48: Verify backend handling of negative user IDs
The regex/^-?[0-9]+$/allows negative IDs; confirm that the users API (api('users', { auto: userIds }, …)) correctly processes stringified negative IDs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx (2)
51-51: Missing avatarUrl in fetchItems projection.The
fetchItemsAPI call on line 51 requests['_id', 'uname', 'displayName']but omits'avatarUrl', which is used bytoUserItem(line 22) and rendered in the UI (line 77). This will cause avatars to be missing when items are fetched by keys.Apply this diff to include avatarUrl:
const [users, groups]: [Udoc[], GDoc[]] = await Promise.all([ - userIds.length > 0 ? api('users', { auto: userIds }, ['_id', 'uname', 'displayName']) : [], + userIds.length > 0 ? api('users', { auto: userIds }, ['_id', 'uname', 'displayName', 'avatarUrl']) : [], groupNames.length > 0 ? api('groups', { names: groupNames }, ['name', 'uids']) : [], ]);
108-108: Remove.isRequiredfrom onChange PropType.The
onChangeprop is marked as required, but the underlyingAutoCompletecomponent makes it optional (defaulting to a no-op function on line 89 of AutoComplete.tsx). The PropType should match the TypeScript interface.Apply this diff:
- onChange: PropTypes.func.isRequired, + onChange: PropTypes.func,
🧹 Nitpick comments (1)
packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx (1)
56-61: Consider visual styling for invalid groups.Lines 56-61 correctly handle missing/deleted groups by marking them with
invalid: true, and line 67 displays "(invalid)" text. However, the invalid state could be more prominent in the rendered item (lines 73-89) to better alert users.Consider adding visual styling for invalid items:
renderItem={(item) => ( - <div className="media"> + <div className={`media${item.invalid ? ' invalid' : ''}`}> {item.type === 'user' && ( <div className="media__left medium"> <img className="small user-profile-avatar" alt="" src={item.avatarUrl} width="30" height="30" /> </div> )} <div className="media__body medium"> <div className="assign-select__name"> {item.name}{item.type === 'user' && item.displayName && ` (${item.displayName})`} + {item.invalid && <span className="invalid-badge"> (invalid)</span>} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/frontend/autocomplete/AutoComplete.tsx(3 hunks)packages/hydrooj/src/handler/domain.ts(2 hunks)packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hydrooj/src/handler/domain.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx (4)
packages/hydrooj/src/interface.ts (2)
Udoc(79-93)GDoc(111-116)packages/ui-default/components/autocomplete/AssignSelectAutoComplete.tsx (1)
AssignSelectAutoComplete(4-22)packages/components/frontend/autocomplete/AutoComplete.tsx (2)
AutoCompleteHandle(38-50)AutoCompleteProps(12-36)packages/ui-default/utils/index.ts (1)
api(5-9)
🔇 Additional comments (4)
packages/components/frontend/autocomplete/AutoComplete.tsx (3)
115-124: LGTM! Robust error handling added.The try/catch block properly handles query failures by logging errors and resetting the UI state (empty list, null selection). This prevents the component from breaking when the query API fails.
144-146: LGTM! Appropriate error handling for effect.The catch handler correctly logs fetch failures without re-throwing, which is appropriate for a useEffect hook. This prevents unhandled promise rejections when pre-loading selected items.
298-304: LGTM! Graceful paste failure handling.The try/catch ensures that paste failures don't break the component. Users can continue interacting with the autocomplete even if batch fetching fails.
packages/ui-default/components/autocomplete/components/AssignSelectAutoComplete.tsx (1)
46-46: Confirm support for negative user IDs
The regex/^-?[0-9]+$/matches negative numbers; if negative IDs aren’t intended, restrict to/^[0-9]+$/.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/frontend/autocomplete/AutoComplete.tsx (1)
115-124: Consider structured error handling for production.While the error logging is functional, consider these optional improvements:
- Structured logging: Replace console.error with a proper logging service for production monitoring
- Error callback API: Add an
onError?: (error: Error, context: string) => voidprop to allow parent components to handle errors (show notifications, track metrics, etc.)- Error recovery: For the paste operation specifically, consider partial success handling (add items that succeeded, notify about failures)
Example API enhancement:
export interface AutoCompleteProps<Item> { // ... existing props onError?: (error: Error, context: 'query' | 'fetch' | 'paste') => void; }Then use it in error handlers:
} catch (e) { console.error('Failed to query items', e); + props.onError?.(e, 'query'); setItemList([]); setCurrentItem(null); }Also applies to: 144-146, 298-304
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/frontend/autocomplete/AutoComplete.tsx(3 hunks)packages/hydrooj/src/handler/domain.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hydrooj/src/handler/domain.ts
🔇 Additional comments (2)
packages/components/frontend/autocomplete/AutoComplete.tsx (2)
115-124: LGTM! Proper error handling with state cleanup.The try-catch block correctly handles query failures by logging the error and resetting the UI state. The cache is only updated after a successful query, preventing inconsistent state.
144-146: LGTM! Prevents unhandled promise rejection.The catch handler appropriately logs fetch failures for pre-selected items. The existing code already handles missing items gracefully by displaying the key when an item isn't in the cache (line 274).
| try { | ||
| const fetched = await props.fetchItems(ids); | ||
| for (const item of fetched) valueCache[itemKey(item)] = item; | ||
| setSelectedKeys([...selectedKeys, ...fetched.map((val) => itemKey(val))]); | ||
| } catch (err) { | ||
| console.error('Failed to fetch items on paste', err); | ||
| } |
There was a problem hiding this comment.
Silent failure on paste may confuse users.
When fetchItems fails during a paste operation, no items are added and users receive no feedback about the failure. This can be confusing, especially when users paste multiple comma-separated values expecting them to appear.
Consider providing user-visible feedback when paste operations fail, such as:
- Displaying a toast notification
- Adding invalid items with a visual indicator
- Showing an error state in the input
Example improvement:
try {
const fetched = await props.fetchItems(ids);
for (const item of fetched) valueCache[itemKey(item)] = item;
setSelectedKeys([...selectedKeys, ...fetched.map((val) => itemKey(val))]);
} catch (err) {
console.error('Failed to fetch items on paste', err);
+ // Consider: Show toast notification or set error state
+ // e.g., props.onError?.('Failed to load pasted items');
}📝 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.
| try { | |
| const fetched = await props.fetchItems(ids); | |
| for (const item of fetched) valueCache[itemKey(item)] = item; | |
| setSelectedKeys([...selectedKeys, ...fetched.map((val) => itemKey(val))]); | |
| } catch (err) { | |
| console.error('Failed to fetch items on paste', err); | |
| } | |
| try { | |
| const fetched = await props.fetchItems(ids); | |
| for (const item of fetched) valueCache[itemKey(item)] = item; | |
| setSelectedKeys([...selectedKeys, ...fetched.map((val) => itemKey(val))]); | |
| } catch (err) { | |
| console.error('Failed to fetch items on paste', err); | |
| // Consider: Show toast notification or set error state | |
| // e.g., props.onError?.('Failed to load pasted items'); | |
| } |
🤖 Prompt for AI Agents
In packages/components/frontend/autocomplete/AutoComplete.tsx around lines
298-304 the paste handler silently logs fetchItems errors to console which
leaves users unaware that their pasted items failed to load; modify the catch
block to surface a user-visible error (e.g., trigger the app's
toast/notification system with a clear message, or add the pasted keys to the
selection with an "invalid" flag that renders a visual error state in the
list/input), and ensure valueCache and selectedKeys are not left in a
partial/incorrect state on failure (use a local temporary array and only update
state on success, or roll back on error) so the UI reflects the failure and
guides the user to retry or correct input.
2 participants
Add auto complete for group and users in contest/homework permission control.
Summary by CodeRabbit
New Features
Chores
Bug Fixes