get-model-info-from-model-inventory-json-added-additional-fields#2211
Conversation
WalkthroughAdds four new optional model metadata fields (reference_link, biases, limitations, hosting_provider) across client and server types, models, forms, controllers, SQL utils, tenant creation, and a migration; introduces an Autocomplete model selector in the NewModelInventory modal and minor UI/table rendering tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as NewModelInventory Modal (Client)
participant JSON as model-inventory.json
participant API as ModelInventory Controller
participant Model as ModelInventoryModel
participant DB as DB (tenant tables)
User->>UI: Open modal & load model options
UI->>JSON: Read model list
JSON-->>UI: Provide options
User->>UI: Fill form (incl. reference_link, biases, limitations, hosting_provider)
UI->>API: POST/PUT payload (includes new fields)
API->>Model: create/update(payload)
Model->>DB: INSERT/UPDATE including new columns
DB-->>Model: Persisted row
Model-->>API: JSON response (includes new fields)
API-->>UI: Success response
sequenceDiagram
autonumber
participant Migr as Migration
participant Org as Organizations
participant DB as DB
Migr->>Org: Retrieve organizations
loop per org
Migr->>DB: ALTER TABLE "<tenantHash>".model_inventories ADD columns
Migr->>DB: UPDATE "<tenantHash>".model_inventories SET new columns = ''
end
DB-->>Migr: Commit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (15)
Clients/src/presentation/components/Modals/NewModelRisk/index.tsx (1)
331-331: Prevent off‑screen modal content: restore a viewport‑bounded maxHeight.Setting maxHeight to fit-content can push the modal beyond the viewport when content grows, harming usability/accessibility. Keep the inner scroll but cap modal height.
Apply this diff:
- maxHeight: "fit-content", + maxHeight: { xs: "calc(100vh - 32px)", md: "80vh" }, + overflow: "auto",Servers/controllers/modelInventory.ctrl.ts (3)
121-125: Input validation for new fields (URL + length).Add minimal validation/sanitization before persisting:
- reference_link: valid URL (http/https)
- biases/limitations/hosting_provider: trim, length cap to prevent abuse
Example:
const sanitize = (s?: string) => (typeof s === "string" ? s.trim() : ""); const isValidUrl = (u?: string) => { try { if (!u) return true; const x = new URL(u); return ["http:", "https:"].includes(x.protocol); } catch { return false; } }; if (!isValidUrl(reference_link)) return res.status(400).json(STATUS_CODE[400]("Invalid reference_link URL"));Also applies to: 150-154
121-125: Missing “Used in project(s)” field from issue #2155.The issue requests a multi‑select “Used in project(s)” field; it’s not present here. Confirm if it’s deferred to a follow‑up PR.
I can draft the end‑to‑end changes (interface, controller, utils, migration, and UI multi‑select) if you plan to include it in this PR.
121-125: Contract mismatch: server model requires these fields; UI treats them optional.Domain model marks reference_link, biases, limitations, hosting_provider as required (allowNull: false + validation), but the UI treats them as optional and the create path defaults to "". Decide one:
- Make them truly optional (relax model validations, allowNull: true), or
- Enforce requirement in the controller with 400s.
Also applies to: 209-213
Servers/domain.layer/models/modelInventory/modelInventory.model.ts (2)
170-200: Relax validations or enforce them at the API boundary (currently inconsistent).Validations make the four fields required (non‑empty), conflicting with UI/PR intent (“optional”). Either:
- Make them optional here (only validate format if present), or
- Keep them required and return 400s in controllers when missing.
Example optional validation:
if (this.reference_link && !this.reference_link.trim()) { throw new ValidationException("Reference link cannot be blank", "reference_link", this.reference_link); }And validate URL format only if provided.
300-304: Consistent API shape: avoid mixing null and "" for absent values.toSafeJSON returns null for reference_link but other fields return "" when unset. Pick one convention (prefer nulls) and apply consistently.
- biases: this.biases, - limitations: this.limitations, - hosting_provider: this.hosting_provider, + biases: this.biases?.trim() || null, + limitations: this.limitations?.trim() || null, + hosting_provider: this.hosting_provider?.trim() || null,Servers/database/migrations/20250923174329-add-fields-in-model-inventory-table.js (2)
20-25: Idempotency: guard ALTER with IF NOT EXISTS.Prevents failures on partial runs or reruns.
- `ALTER TABLE "${tenantHash}".model_inventories - ADD COLUMN reference_link VARCHAR, - ADD COLUMN biases VARCHAR, - ADD COLUMN limitations VARCHAR, - ADD COLUMN hosting_provider VARCHAR;`, + `ALTER TABLE "${tenantHash}".model_inventories + ADD COLUMN IF NOT EXISTS reference_link VARCHAR, + ADD COLUMN IF NOT EXISTS biases VARCHAR, + ADD COLUMN IF NOT EXISTS limitations VARCHAR, + ADD COLUMN IF NOT EXISTS hosting_provider VARCHAR;`,
55-60: Down migration: add IF EXISTS to be resilient.- `ALTER TABLE "${tenantHash}".model_inventories - DROP COLUMN reference_link, - DROP COLUMN biases, - DROP COLUMN limitations, - DROP COLUMN hosting_provider;`, + `ALTER TABLE "${tenantHash}".model_inventories + DROP COLUMN IF EXISTS reference_link, + DROP COLUMN IF EXISTS biases, + DROP COLUMN IF EXISTS limitations, + DROP COLUMN IF EXISTS hosting_provider;`,Clients/src/presentation/pages/ModelInventory/modelInventoryTable.tsx (3)
270-273: Make reference link clickable (with security attributes)Render a clickable anchor when present; keep tooltip for full URL.
Apply this diff:
- <TooltipCell value={modelInventory.reference_link} /> + {modelInventory.reference_link ? ( + <Tooltip title={modelInventory.reference_link} arrow> + <a + href={modelInventory.reference_link} + target="_blank" + rel="noopener noreferrer" + > + {modelInventory.reference_link} + </a> + </Tooltip> + ) : ( + <span>-</span> + )}
273-281: Prevent layout breaks for long text (biases/limitations/hosting_provider)Long strings will overflow with nowrap. Add ellipsis; tooltip already reveals full content.
Apply this diff to each of these cells:
-<TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> +<TableCell + sx={{ + ...singleTheme.tableStyles.primary.body.cell, + whiteSpace: "nowrap", + maxWidth: 240, + overflow: "hidden", + textOverflow: "ellipsis", + }} +>
365-365: Make table container responsiveUse 100% width with a minWidth to avoid constraining the layout on larger screens.
Apply this diff:
-<TableContainer sx={{ overflowX: "auto", width: "1150px" }}> +<TableContainer sx={{ overflowX: "auto", width: "100%", minWidth: "1150px" }}>Clients/src/presentation/components/Modals/NewModelInventory/index.tsx (4)
201-208: Ensure approver stays a number
event.target.valueis a string; cast only for numeric fields to keep API contracts consistent.Apply this diff:
- const handleOnSelectChange = useCallback( - (prop: keyof NewModelInventoryFormValues) => (event: any) => { - const value = event.target.value; - setValues((prev) => ({ ...prev, [prop]: value })); + const handleOnSelectChange = useCallback( + (prop: keyof NewModelInventoryFormValues) => (event: any) => { + const raw = event.target.value; + const value = prop === "approver" ? Number(raw) : raw; + setValues((prev) => ({ ...prev, [prop]: value as any })); setErrors((prev) => ({ ...prev, [prop]: "" })); }, [] );
656-695: Biases and Limitations should be multilineThese fields tend to be longer prose; consider multiline inputs for usability.
Apply this diff:
- <Field + <Field id="biases" label="Biases" width={220} value={values.biases} onChange={handleOnTextFieldChange("biases")} sx={fieldStyle} - placeholder="Biases" + placeholder="Biases" + multiline + minRows={2} /> @@ - <Field + <Field id="limitations" label="Limitations" width={220} value={values.limitations} onChange={handleOnTextFieldChange("limitations")} sx={fieldStyle} - placeholder="Limitation" + placeholder="Limitations" + multiline + minRows={2} />
703-709: Clarify placeholderHosting provider examples are typically “AWS, Azure, GCP, Private DC”, not “OpenAI”.
Apply this diff:
- placeholder="eg. OpenAI" + placeholder="e.g., AWS, Azure, GCP, Private DC"
348-381: Modal size: consider maxHeight + scroll for content overflowYou used
maxHeight: "fit-content". For long forms, prefer a viewport-based maxHeight with internal scrolling to preserve the header/actions visible.Example:
- maxHeight: "fit-content", + maxHeight: "80vh", + overflowY: "auto",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Clients/src/domain/interfaces/i.modelInventory.ts(1 hunks)Clients/src/presentation/components/Modals/NewModelInventory/index.tsx(7 hunks)Clients/src/presentation/components/Modals/NewModelRisk/index.tsx(1 hunks)Clients/src/presentation/pages/ModelInventory/index.tsx(1 hunks)Clients/src/presentation/pages/ModelInventory/modelInventoryTable.tsx(5 hunks)Servers/controllers/modelInventory.ctrl.ts(4 hunks)Servers/database/migrations/20250923174329-add-fields-in-model-inventory-table.js(1 hunks)Servers/domain.layer/interfaces/i.modelInventory.ts(1 hunks)Servers/domain.layer/models/modelInventory/modelInventory.model.ts(6 hunks)Servers/utils/modelInventory.utils.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-18T05:28:01.115Z
Learnt from: MuhammadKhalilzadeh
PR: bluewave-labs/verifywise#254
File: Clients/src/presentation/components/Modals/Controlpane/index.tsx:22-24
Timestamp: 2024-11-18T05:28:01.115Z
Learning: In `Clients/src/presentation/components/Modals/Controlpane/index.tsx` (TypeScript React component), avoid changing prop types from specific types like `string` to `any` when the exact type is known. Maintaining explicit types ensures better type safety and code reliability.
Applied to files:
Clients/src/presentation/components/Modals/NewModelRisk/index.tsx
🧬 Code graph analysis (3)
Servers/domain.layer/models/modelInventory/modelInventory.model.ts (1)
Servers/domain.layer/exceptions/custom.exception.ts (1)
ValidationException(94-116)
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx (1)
Clients/src/presentation/components/Reporting/GenerateReport/GenerateReportFrom/styles.ts (1)
fieldStyle(26-32)
Servers/database/migrations/20250923174329-add-fields-in-model-inventory-table.js (3)
Servers/tools/getTenantHash.ts (1)
getTenantHash(3-6)Servers/database/migrations/20250627200402-migrate-existing-data-to-tenant-schema.js (1)
up(6-176)Servers/database/migrations/20250826163806-make-model-inventory-tenant-specific.js (1)
queryInterface(6-83)
🪛 GitHub Actions: Frontend Checks
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx
[error] 485-485: Cannot find name 'KeyboardArrowDown'. During 'npm run build', TS2304: Cannot find name 'KeyboardArrowDown' in src/presentation/components/Modals/NewModelInventory/index.tsx(485,35).
🔇 Additional comments (8)
Servers/utils/modelInventory.utils.ts (1)
59-63: LGTM on wiring new fields into SQL replacements.The new fields are passed safely via replacements and match the column list.
Confirm that empty strings are the intended “unset” value. If nulls are preferred over "", update toSafeJSON/DB defaults accordingly to keep API consistent.
Also applies to: 105-109
Servers/domain.layer/interfaces/i.modelInventory.ts (1)
14-17: LGTM: Interface extended with optional metadata.Additions align with the PR goals and are marked optional.
Clients/src/presentation/pages/ModelInventory/index.tsx (1)
782-786: LGTM: Pass-through of new fields to NewModelInventory initialData.Defaults to empty string to avoid undefined in the form.
Ensure the NewModelInventory form binds and submits these fields so edits persist on update.
Clients/src/domain/interfaces/i.modelInventory.ts (1)
12-15: LGTM: Client interface extended with optional fields.Matches server additions.
Note: Server may return null for reference_link; ensure renderers handle string | null gracefully.
Servers/domain.layer/models/modelInventory/modelInventory.model.ts (1)
456-467: LGTM: Update helper propagates new fields conditionally.Only updates provided fields and bumps updated_at.
Clients/src/presentation/pages/ModelInventory/modelInventoryTable.tsx (2)
78-83: TooltipCell abstraction looks goodSimple and reusable for consistent fallback rendering.
60-63: Confirm domain/DB alignment for new columnsEnsure IModelInventory, API payloads, and migrations match these fields and that null/empty strings are handled on reads.
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx (1)
86-92: Scope check: “Used in project(s) / org frameworks” not presentThe linked issue calls for a multi-select covering projects/org frameworks. Confirm if deferred; if not, add it to the form and table for parity.
| @@ -30,7 +30,7 @@ import { User } from "../../../../domain/types/User"; | |||
| import dayjs, { Dayjs } from "dayjs"; | |||
| import KeyboardArrowDown from "@mui/icons-material/KeyboardArrowDown"; | |||
There was a problem hiding this comment.
Fix build: KeyboardArrowDown not found
Pipeline error TS2304 indicates the symbol isn’t resolved at usage. Use the conventional “Icon” alias and update usages.
Apply this diff:
-import KeyboardArrowDown from "@mui/icons-material/KeyboardArrowDown";
+import KeyboardArrowDownIcon from "@mui/icons-material/KeyboardArrowDown";And replace usages:
- popupIcon={<KeyboardArrowDown />}
+ popupIcon={<KeyboardArrowDownIcon />}Also applies to: 478-479, 626-627
🤖 Prompt for AI Agents
In Clients/src/presentation/components/Modals/NewModelInventory/index.tsx around
lines 31 (and also update occurrences at 478-479 and 626-627), the import uses
KeyboardArrowDown which is unresolved; change the import to the conventional
alias KeyboardArrowDownIcon from "@mui/icons-material/KeyboardArrowDown" and
update all usages of KeyboardArrowDown to KeyboardArrowDownIcon to fix the
TS2304 build error.
| import KeyboardArrowDown from "@mui/icons-material/KeyboardArrowDown"; | ||
| import { useModalKeyHandling } from "../../../../application/hooks/useModalKeyHandling"; | ||
|
|
||
| import modelInventoryOptions from "../../../../../../Servers/templates/model-inventory.json"; |
There was a problem hiding this comment.
Don’t import JSON from Servers/ into Clients/
This crosses package boundaries and is likely blocked by the bundler (and breaks encapsulation). Serve it via an API route, move it to a shared package, or place it under Clients/public and fetch at runtime.
I can sketch either a small GET /model-inventory endpoint on the server or a client-side fetch from public/model-inventory.json—your call.
🤖 Prompt for AI Agents
In Clients/src/presentation/components/Modals/NewModelInventory/index.tsx around
line 33, the file currently imports Servers/templates/model-inventory.json which
crosses package boundaries and will break bundling; replace the direct import by
either (A) moving model-inventory.json into Clients/public and fetch it at
runtime (e.g., fetch('/model-inventory.json') and await JSON) or (B) expose a
server-side GET /model-inventory endpoint and fetch from the client (e.g.,
fetch('/api/model-inventory')), then use the fetched JSON in place of the
imported constant; update the component to handle async loading and errors
(loading state and try/catch) accordingly.
| const modelInventoryList = useMemo(() => { | ||
| return modelInventoryOptions.map((u) => ({ | ||
| _id: u.model, | ||
| name: `${u.provider} - ${u.model}`, | ||
| surname: u.model, | ||
| email: u.model | ||
| })); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enrich model options with provider; drop unused fields
Include provider/model in the option object so selection can auto-fill both fields.
Apply this diff:
- return modelInventoryOptions.map((u) => ({
- _id: u.model,
- name: `${u.provider} - ${u.model}`,
- surname: u.model,
- email: u.model
- }));
+ return modelInventoryOptions.map((u) => ({
+ _id: u.model,
+ name: `${u.provider} - ${u.model}`,
+ provider: u.provider,
+ model: u.model,
+ }));📝 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 modelInventoryList = useMemo(() => { | |
| return modelInventoryOptions.map((u) => ({ | |
| _id: u.model, | |
| name: `${u.provider} - ${u.model}`, | |
| surname: u.model, | |
| email: u.model | |
| })); | |
| }, []); | |
| const modelInventoryList = useMemo(() => { | |
| return modelInventoryOptions.map((u) => ({ | |
| _id: u.model, | |
| name: `${u.provider} - ${u.model}`, | |
| provider: u.provider, | |
| model: u.model, | |
| })); | |
| }, []); |
🤖 Prompt for AI Agents
In Clients/src/presentation/components/Modals/NewModelInventory/index.tsx around
lines 181 to 189, the mapped option objects include unused fields and lack a
provider/model pair for autofill and also the useMemo has an empty dependency
array; change the mapping to return an object that includes provider and model
(e.g. _id built from provider+model, provider: u.provider, model: u.model, name:
`${u.provider} - ${u.model}`) and remove surname/email, and add
modelInventoryOptions to the useMemo dependency array so the list updates when
options change.
|
|
||
| <Box sx={{ display: 'flex', flexDirection: 'column', width: 220 }}> | ||
| <Typography | ||
| variant="body2" | ||
| sx={{ mb: 2, fontWeight: 450, color: theme.palette.text.primary }} | ||
| > | ||
| Model <Typography component="span" color="black">*</Typography> | ||
| </Typography> | ||
| <Autocomplete | ||
| id="model-input" | ||
| size="small" | ||
| freeSolo | ||
| value={values.model} | ||
| options={modelInventoryList || []} | ||
| getOptionLabel={(option) => | ||
| typeof option === "string" ? option : option.name | ||
| } | ||
| onChange={(_event, newValue) => { | ||
| // Handle both option object and free text | ||
| if (typeof newValue === "string") { | ||
| setValues({ ...values, model: newValue }); | ||
| } else if (newValue && typeof newValue === "object") { | ||
| setValues({ ...values, model: newValue.name }); | ||
| } else { | ||
| setValues({ ...values, model: "" }); | ||
| } | ||
| }} | ||
| onInputChange={(_event, newInputValue, reason) => { | ||
| if (reason === "input") { | ||
| setValues({ ...values, model: newInputValue }); | ||
| } | ||
| }} | ||
| renderOption={(props, option) => ( | ||
| <Box component="li" {...props}> | ||
| <Typography sx={{ fontSize: 13, color: theme.palette.text.primary }}> | ||
| {option.name} | ||
| </Typography> | ||
| </Box> | ||
| )} | ||
| popupIcon={<KeyboardArrowDown />} | ||
| renderInput={(params) => ( | ||
| <TextField | ||
| {...params} | ||
| placeholder="Select or enter model" | ||
| error={Boolean(errors.model)} | ||
| helperText={errors.model} | ||
| variant="outlined" | ||
| sx={{ | ||
| "& .MuiInputBase-root": { | ||
| height: 34, | ||
| minHeight: 34, | ||
| borderRadius: 2, | ||
| }, | ||
| "& .MuiInputBase-input": { | ||
| padding: "0 8px", | ||
| fontSize: 13, | ||
| }, | ||
| }} | ||
| /> | ||
| )} | ||
| // noOptionsText="No matching models" | ||
| filterOptions={(options, state) => { | ||
| const filtered = options.filter((option) => | ||
| option.name.toLowerCase().includes(state.inputValue.toLowerCase()) | ||
| ); | ||
|
|
||
| if (filtered.length === 0) { | ||
| return []; | ||
| } | ||
|
|
||
| return filtered; | ||
| }} | ||
| slotProps={{ | ||
| paper: { | ||
| sx: { | ||
| "& .MuiAutocomplete-listbox": { | ||
| "& .MuiAutocomplete-option": { | ||
| fontSize: 13, | ||
| color: theme.palette.text.primary, | ||
| padding: "8px 12px", | ||
| }, | ||
| "& .MuiAutocomplete-option.Mui-focused": { | ||
| backgroundColor: theme.palette.background.accent, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }} | ||
| disabled={isLoadingUsers} | ||
| /> | ||
| </Box> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Selecting a predefined model should also populate provider (and keep free-text flow)
Currently only model updates; align with the inventory JSON by setting provider when an option is chosen. Also, disabling this Autocomplete while users load is unrelated—remove that.
Apply this diff:
- <Autocomplete
+ <Autocomplete
id="model-input"
size="small"
freeSolo
value={values.model}
options={modelInventoryList || []}
getOptionLabel={(option) =>
typeof option === "string" ? option : option.name
}
- onChange={(_event, newValue) => {
+ onChange={(_event, newValue) => {
// Handle both option object and free text
if (typeof newValue === "string") {
- setValues({ ...values, model: newValue });
+ setValues((prev) => ({ ...prev, model: newValue }));
} else if (newValue && typeof newValue === "object") {
- setValues({ ...values, model: newValue.name });
+ // Use structured option fields
+ setValues((prev) => ({
+ ...prev,
+ model: newValue.model ?? newValue.name,
+ provider: newValue.provider ?? prev.provider,
+ }));
} else {
- setValues({ ...values, model: "" });
+ setValues((prev) => ({ ...prev, model: "" }));
}
}}
onInputChange={(_event, newInputValue, reason) => {
if (reason === "input") {
- setValues({ ...values, model: newInputValue });
+ setValues((prev) => ({ ...prev, model: newInputValue }));
}
}}
@@
- disabled={isLoadingUsers}
+ // Keep model selection usable regardless of user-loading state
+ disabled={false}
/>📝 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.
| <Box sx={{ display: 'flex', flexDirection: 'column', width: 220 }}> | |
| <Typography | |
| variant="body2" | |
| sx={{ mb: 2, fontWeight: 450, color: theme.palette.text.primary }} | |
| > | |
| Model <Typography component="span" color="black">*</Typography> | |
| </Typography> | |
| <Autocomplete | |
| id="model-input" | |
| size="small" | |
| freeSolo | |
| value={values.model} | |
| options={modelInventoryList || []} | |
| getOptionLabel={(option) => | |
| typeof option === "string" ? option : option.name | |
| } | |
| onChange={(_event, newValue) => { | |
| // Handle both option object and free text | |
| if (typeof newValue === "string") { | |
| setValues({ ...values, model: newValue }); | |
| } else if (newValue && typeof newValue === "object") { | |
| setValues({ ...values, model: newValue.name }); | |
| } else { | |
| setValues({ ...values, model: "" }); | |
| } | |
| }} | |
| onInputChange={(_event, newInputValue, reason) => { | |
| if (reason === "input") { | |
| setValues({ ...values, model: newInputValue }); | |
| } | |
| }} | |
| renderOption={(props, option) => ( | |
| <Box component="li" {...props}> | |
| <Typography sx={{ fontSize: 13, color: theme.palette.text.primary }}> | |
| {option.name} | |
| </Typography> | |
| </Box> | |
| )} | |
| popupIcon={<KeyboardArrowDown />} | |
| renderInput={(params) => ( | |
| <TextField | |
| {...params} | |
| placeholder="Select or enter model" | |
| error={Boolean(errors.model)} | |
| helperText={errors.model} | |
| variant="outlined" | |
| sx={{ | |
| "& .MuiInputBase-root": { | |
| height: 34, | |
| minHeight: 34, | |
| borderRadius: 2, | |
| }, | |
| "& .MuiInputBase-input": { | |
| padding: "0 8px", | |
| fontSize: 13, | |
| }, | |
| }} | |
| /> | |
| )} | |
| // noOptionsText="No matching models" | |
| filterOptions={(options, state) => { | |
| const filtered = options.filter((option) => | |
| option.name.toLowerCase().includes(state.inputValue.toLowerCase()) | |
| ); | |
| if (filtered.length === 0) { | |
| return []; | |
| } | |
| return filtered; | |
| }} | |
| slotProps={{ | |
| paper: { | |
| sx: { | |
| "& .MuiAutocomplete-listbox": { | |
| "& .MuiAutocomplete-option": { | |
| fontSize: 13, | |
| color: theme.palette.text.primary, | |
| padding: "8px 12px", | |
| }, | |
| "& .MuiAutocomplete-option.Mui-focused": { | |
| backgroundColor: theme.palette.background.accent, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }} | |
| disabled={isLoadingUsers} | |
| /> | |
| </Box> | |
| <Box sx={{ display: 'flex', flexDirection: 'column', width: 220 }}> | |
| <Typography | |
| variant="body2" | |
| sx={{ mb: 2, fontWeight: 450, color: theme.palette.text.primary }} | |
| > | |
| Model <Typography component="span" color="black">*</Typography> | |
| </Typography> | |
| <Autocomplete | |
| id="model-input" | |
| size="small" | |
| freeSolo | |
| value={values.model} | |
| options={modelInventoryList || []} | |
| getOptionLabel={(option) => | |
| typeof option === "string" ? option : option.name | |
| } | |
| onChange={(_event, newValue) => { | |
| // Handle both option object and free text | |
| if (typeof newValue === "string") { | |
| setValues((prev) => ({ ...prev, model: newValue })); | |
| } else if (newValue && typeof newValue === "object") { | |
| // Use structured option fields | |
| setValues((prev) => ({ | |
| ...prev, | |
| model: newValue.model ?? newValue.name, | |
| provider: newValue.provider ?? prev.provider, | |
| })); | |
| } else { | |
| setValues((prev) => ({ ...prev, model: "" })); | |
| } | |
| }} | |
| onInputChange={(_event, newInputValue, reason) => { | |
| if (reason === "input") { | |
| setValues((prev) => ({ ...prev, model: newInputValue })); | |
| } | |
| }} | |
| renderOption={(props, option) => ( | |
| <Box component="li" {...props}> | |
| <Typography sx={{ fontSize: 13, color: theme.palette.text.primary }}> | |
| {option.name} | |
| </Typography> | |
| </Box> | |
| )} | |
| popupIcon={<KeyboardArrowDown />} | |
| renderInput={(params) => ( | |
| <TextField | |
| {...params} | |
| placeholder="Select or enter model" | |
| error={Boolean(errors.model)} | |
| helperText={errors.model} | |
| variant="outlined" | |
| sx={{ | |
| "& .MuiInputBase-root": { | |
| height: 34, | |
| minHeight: 34, | |
| borderRadius: 2, | |
| }, | |
| "& .MuiInputBase-input": { | |
| padding: "0 8px", | |
| fontSize: 13, | |
| }, | |
| }} | |
| /> | |
| )} | |
| filterOptions={(options, state) => { | |
| const filtered = options.filter((option) => | |
| option.name.toLowerCase().includes(state.inputValue.toLowerCase()) | |
| ); | |
| if (filtered.length === 0) { | |
| return []; | |
| } | |
| return filtered; | |
| }} | |
| slotProps={{ | |
| paper: { | |
| sx: { | |
| "& .MuiAutocomplete-listbox": { | |
| "& .MuiAutocomplete-option": { | |
| fontSize: 13, | |
| color: theme.palette.text.primary, | |
| padding: "8px 12px", | |
| }, | |
| "& .MuiAutocomplete-option.Mui-focused": { | |
| backgroundColor: theme.palette.background.accent, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }} | |
| disabled={false} | |
| /> | |
| </Box> |
🧰 Tools
🪛 GitHub Actions: Frontend Checks
[error] 485-485: Cannot find name 'KeyboardArrowDown'. During 'npm run build', TS2304: Cannot find name 'KeyboardArrowDown' in src/presentation/components/Modals/NewModelInventory/index.tsx(485,35).
🤖 Prompt for AI Agents
In Clients/src/presentation/components/Modals/NewModelInventory/index.tsx around
lines 439 to 529, update the Autocomplete onChange/onInputChange handlers to
also set provider when a predefined option object is chosen (e.g. setValues({
...values, model: option.name, provider: option.provider })) and clear provider
when the user types free text or clears the field; remove the
disabled={isLoadingUsers} prop (or set disabled={false}) so loading users does
not block model entry. Ensure getOptionLabel/renderOption handling remains but
adjust onInputChange to explicitly set provider to "" for freeSolo input
changes.
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | ||
| <TooltipCell value={modelInventory.provider} /> | ||
| </TableCell> | ||
| <TableCell sx={singleTheme.tableStyles.primary.body.cell}> | ||
| {modelInventory.model || "-"} | ||
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | ||
| <TooltipCell value={modelInventory.model} /> | ||
| </TableCell> | ||
| <TableCell sx={singleTheme.tableStyles.primary.body.cell}> | ||
| {modelInventory.version || "-"} | ||
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | ||
| <TooltipCell value={modelInventory.version} /> | ||
| </TableCell> | ||
| <TableCell sx={singleTheme.tableStyles.primary.body.cell}> | ||
| {userMap.get(modelInventory.approver.toString())} | ||
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | ||
| <TooltipCell value={userMap.get(modelInventory.approver.toString())} /> | ||
| </TableCell> |
There was a problem hiding this comment.
Guard against null/undefined approver before toString()
modelInventory.approver.toString() will throw if approver is null/undefined. Use a safe lookup.
Apply this diff:
- <TooltipCell value={userMap.get(modelInventory.approver.toString())} />
+ <TooltipCell
+ value={
+ modelInventory.approver != null
+ ? userMap.get(String(modelInventory.approver))
+ : undefined
+ }
+ />📝 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.
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | |
| <TooltipCell value={modelInventory.provider} /> | |
| </TableCell> | |
| <TableCell sx={singleTheme.tableStyles.primary.body.cell}> | |
| {modelInventory.model || "-"} | |
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | |
| <TooltipCell value={modelInventory.model} /> | |
| </TableCell> | |
| <TableCell sx={singleTheme.tableStyles.primary.body.cell}> | |
| {modelInventory.version || "-"} | |
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | |
| <TooltipCell value={modelInventory.version} /> | |
| </TableCell> | |
| <TableCell sx={singleTheme.tableStyles.primary.body.cell}> | |
| {userMap.get(modelInventory.approver.toString())} | |
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | |
| <TooltipCell value={userMap.get(modelInventory.approver.toString())} /> | |
| </TableCell> | |
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | |
| <TooltipCell value={modelInventory.provider} /> | |
| </TableCell> | |
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | |
| <TooltipCell value={modelInventory.model} /> | |
| </TableCell> | |
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | |
| <TooltipCell value={modelInventory.version} /> | |
| </TableCell> | |
| <TableCell sx={{ ...singleTheme.tableStyles.primary.body.cell, whiteSpace: "nowrap" }}> | |
| <TooltipCell | |
| value={ | |
| modelInventory.approver != null | |
| ? userMap.get(String(modelInventory.approver)) | |
| : undefined | |
| } | |
| /> | |
| </TableCell> |
🤖 Prompt for AI Agents
In Clients/src/presentation/pages/ModelInventory/modelInventoryTable.tsx around
lines 240 to 251, the code calls modelInventory.approver.toString() which will
throw if approver is null/undefined; change the lookup to safely handle missing
approver (e.g. use modelInventory.approver != null ?
String(modelInventory.approver) : '' or modelInventory.approver?.toString() ??
''), then pass that safe string into userMap.get(...) and optionally fall back
to a default display like '—' or 'Unknown' when userMap.get returns undefined.
| @@ -0,0 +1,70 @@ | |||
| 'use strict'; | |||
|
|
|||
| const { getTenantHash } = require("../../dist/tools/getTenantHash"); | |||
There was a problem hiding this comment.
Avoid requiring build artifacts in migrations.
Requiring ../../dist/... couples migration execution to a prior build. Inline the helper or require a plain JS util.
Apply this diff:
-const { getTenantHash } = require("../../dist/tools/getTenantHash");
+const crypto = require("crypto");
+const getTenantHash = (tenantId) => {
+ const hash = crypto.createHash("sha256").update(String(tenantId)).digest("base64");
+ return hash.replace(/[^a-zA-Z0-9]/g, "").slice(0, 10);
+};📝 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 { getTenantHash } = require("../../dist/tools/getTenantHash"); | |
| const crypto = require("crypto"); | |
| const getTenantHash = (tenantId) => { | |
| const hash = crypto | |
| .createHash("sha256") | |
| .update(String(tenantId)) | |
| .digest("base64"); | |
| return hash.replace(/[^a-zA-Z0-9]/g, "").slice(0, 10); | |
| }; |
🤖 Prompt for AI Agents
In
Servers/database/migrations/20250923174329-add-fields-in-model-inventory-table.js
around line 3, the migration currently requires a helper from
../../dist/tools/getTenantHash which depends on build artifacts; replace this by
either inlining the getTenantHash implementation directly into the migration
file (copy the minimal pure-JS logic needed) or require it from a non-built util
path (e.g., ../../tools/getTenantHash.js or ../utils/getTenantHash.js) that
contains plain JS and does not depend on the project build; ensure the inlined
or re-required function is synchronous/independent of runtime-only dependencies
and update the require/import path accordingly so migrations run without a prior
build.
| @Column({ | ||
| type: DataType.TEXT, | ||
| allowNull: false, | ||
| }) | ||
| reference_link!: string; | ||
|
|
||
| @Column({ | ||
| type: DataType.TEXT, | ||
| allowNull: false, | ||
| }) | ||
| biases!: string; | ||
|
|
||
| @Column({ | ||
| type: DataType.TEXT, | ||
| allowNull: false, | ||
| }) | ||
| limitations!: string; | ||
|
|
||
| @Column({ | ||
| type: DataType.TEXT, | ||
| allowNull: false, | ||
| }) | ||
| hosting_provider!: string; |
There was a problem hiding this comment.
Align DB schema and model nullability for new fields.
Model uses allowNull: false, but the migration adds nullable columns with no NOT NULL constraint. Either:
- Add NOT NULL in the migration (with a safe default), or
- Switch allowNull to true here if fields are optional.
- @Column({ type: DataType.TEXT, allowNull: false })
+ @Column({ type: DataType.TEXT, allowNull: true })
reference_link!: string;(Apply similarly for biases, limitations, hosting_provider) or update the migration to add NOT NULL.
📝 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.
| @Column({ | |
| type: DataType.TEXT, | |
| allowNull: false, | |
| }) | |
| reference_link!: string; | |
| @Column({ | |
| type: DataType.TEXT, | |
| allowNull: false, | |
| }) | |
| biases!: string; | |
| @Column({ | |
| type: DataType.TEXT, | |
| allowNull: false, | |
| }) | |
| limitations!: string; | |
| @Column({ | |
| type: DataType.TEXT, | |
| allowNull: false, | |
| }) | |
| hosting_provider!: string; | |
| @Column({ | |
| type: DataType.TEXT, | |
| allowNull: true, | |
| }) | |
| reference_link!: string; | |
| @Column({ | |
| type: DataType.TEXT, | |
| allowNull: true, | |
| }) | |
| biases!: string; | |
| @Column({ | |
| type: DataType.TEXT, | |
| allowNull: true, | |
| }) | |
| limitations!: string; | |
| @Column({ | |
| type: DataType.TEXT, | |
| allowNull: true, | |
| }) | |
| hosting_provider!: string; |
🤖 Prompt for AI Agents
In Servers/domain.layer/models/modelInventory/modelInventory.model.ts around
lines 77-99 the model marks reference_link, biases, limitations, and
hosting_provider as allowNull: false but the migration created those columns
nullable; make the schema and model consistent by either updating the migration
to ALTER the columns to NOT NULL with a safe default (e.g., empty string or
appropriate value) and re-run the migration, or change these four columns in
this model to allowNull: true if they are optional; pick one approach and apply
it to all four fields so the DB and model nullability match.
| `INSERT INTO "${tenant}".model_inventories (provider_model, provider, model, version, approver, capabilities, security_assessment, status, status_date, reference_link, biases, limitations, hosting_provider, is_demo, created_at, updated_at) | ||
| VALUES (:provider_model, :provider, :model, :version, :approver, :capabilities, :security_assessment, :status, :status_date, :reference_link, :biases, :limitations, :hosting_provider, :is_demo, :created_at, :updated_at) RETURNING *`, | ||
| { |
There was a problem hiding this comment.
Ensure INSERT returns a single model instance (not an array).
With sequelize.query + RETURNING and mapToModel, result[0] is typically an array of instances. Controllers call toSafeJSON() on the return value, so returning an array would break at runtime.
Apply one of the following fixes:
- Prefer adding plain: true to the query options and returning the single instance; or
- Destructure rows and return rows[0].
Example (adjusting current code outside the string literal):
const result = await sequelize.query(
`INSERT INTO "${tenant}".model_inventories (...columns...)
VALUES (...replacements...) RETURNING *`,
{
replacements: { ... },
mapToModel: true,
model: ModelInventoryModel,
+ plain: true,
transaction,
}
);
- return result[0];
+ return result[0]; // with plain:true, this is the single instanceAlternatively:
// Without plain:true
const [rows] = await sequelize.query(/* same opts without plain */);
return rows[0];Please align the SELECT in updateModelInventoryByIdQuery similarly.
🤖 Prompt for AI Agents
In Servers/utils/modelInventory.utils.ts around lines 44-46, the INSERT query
using sequelize.query with RETURNING and mapToModel currently yields an array of
instances (result[0]) but controllers expect a single model instance; change the
query call to either pass plain: true in the query options so sequelize returns
a single plain instance, or destructure the returned rows (const [rows] = await
sequelize.query(...)) and return rows[0]; apply the same change to the SELECT
used in updateModelInventoryByIdQuery so it also returns a single instance (not
an array) before returning to callers.
| // First update the record | ||
| await sequelize.query( | ||
| `UPDATE "${tenant}".model_inventories SET provider_model = :provider_model, provider = :provider, model = :model, version = :version, approver = :approver, capabilities = :capabilities, security_assessment = :security_assessment, status = :status, status_date = :status_date, is_demo = :is_demo, updated_at = :updated_at WHERE id = :id`, | ||
| `UPDATE "${tenant}".model_inventories SET provider_model = :provider_model, provider = :provider, model = :model, version = :version, approver = :approver, capabilities = :capabilities, security_assessment = :security_assessment, status = :status, status_date = :status_date, reference_link = :reference_link, biases = :biases, limitations = :limitations, hosting_provider = :hosting_provider, is_demo = :is_demo, updated_at = :updated_at WHERE id = :id`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the single‑instance return fix for UPDATE→SELECT.
The follow‑up SELECT likely returns an array of model instances with mapToModel. Return the first instance to match controller usage of toSafeJSON().
Add plain: true or destructure rows:
- const result = await sequelize.query(
+ const result = await sequelize.query(
`SELECT * FROM "${tenant}".model_inventories WHERE id = :id`,
{
replacements: { id },
mapToModel: true,
model: ModelInventoryModel,
+ plain: true,
transaction,
}
);
- return result[0];
+ return result[0]; // single instance with plain:trueCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Servers/utils/modelInventory.utils.ts around line 90, the UPDATE followed by
a SELECT currently returns an array of model instances (because mapToModel is
used), but the controller expects a single instance and calls toSafeJSON() on
it; adjust the SELECT to return a single record by either passing { plain: true
} to the query or destructuring the result rows and returning rows[0] (or the
first mapped model) so the function returns one instance instead of an array.
Ensure the returned value is the single model object the controller expects.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx (1)
43-57: Missing requirement: “Used in project(s)” multi-select not implementedIssue #2155 asks for a multi-selection of projects/org frameworks. It’s absent from the form state, UI, and submit payload.
Proposed additions:
- Add used_in_projects: string[] (or IDs) to NewModelInventoryFormValues (optional).
- Provide an Autocomplete multiple tied to projects (similar to capabilities).
- Persist on submit and display in edit mode.
I can draft the minimal UI block plus repository calls if you confirm the expected entity shape (IDs vs names).
🧹 Nitpick comments (2)
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx (2)
368-368: Modal height: avoid unconstrained growthfit-content can overflow the viewport and defeat the scroll container. Cap it for predictable UX.
- maxHeight: "fit-content", + maxHeight: "80vh",
656-711: Text inputs for Biases/Limitations: use multilineThese are narrative fields; single-line inputs are limiting.
- <Field + <Field id="biases" label="Biases" width={220} value={values.biases} onChange={handleOnTextFieldChange("biases")} sx={fieldStyle} - placeholder="Biases" + placeholder="Biases" + multiline + rows={3} /> @@ - <Field + <Field id="limitations" label="Limitations" width={220} value={values.limitations} onChange={handleOnTextFieldChange("limitations")} sx={fieldStyle} - placeholder="Limitation" + placeholder="Limitations" + multiline + rows={3} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-18T05:28:01.115Z
Learnt from: MuhammadKhalilzadeh
PR: bluewave-labs/verifywise#254
File: Clients/src/presentation/components/Modals/Controlpane/index.tsx:22-24
Timestamp: 2024-11-18T05:28:01.115Z
Learning: In `Clients/src/presentation/components/Modals/Controlpane/index.tsx` (TypeScript React component), avoid changing prop types from specific types like `string` to `any` when the exact type is known. Maintaining explicit types ensures better type safety and code reliability.
Applied to files:
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx
🧬 Code graph analysis (1)
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx (1)
Clients/src/presentation/components/Reporting/GenerateReport/GenerateReportFrom/styles.ts (1)
fieldStyle(26-32)
🔇 Additional comments (6)
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx (6)
181-189: Option shape: include provider/model, stable _id, fix depsCurrent mapping includes unused fields and can collide on model name alone. Also memo has empty deps.
- const modelInventoryList = useMemo(() => { - return modelInventoryOptions.map((u) => ({ - _id: u.model, - name: `${u.provider} - ${u.model}`, - surname: u.model, - email: u.model - })); - }, []); + const modelInventoryList = useMemo(() => { + return modelInventoryOptions.map((u) => ({ + _id: `${u.provider}:${u.model}`, + name: `${u.provider} - ${u.model}`, + provider: u.provider, + model: u.model, + })); + }, [modelInventoryOptions]);
626-626: Icon usage here needs the same import fixUpdate to the default-exported alias as above.
- popupIcon={<KeyboardArrowDown />} + popupIcon={<KeyboardArrowDownIcon />}
31-31: Fix build: use icon default import alias and update usagesMUI icons export defaults per-icon. The named import is unresolved and causes TS2304 at usages.
-import { KeyboardArrowDown } from "@mui/icons-material"; +import KeyboardArrowDownIcon from "@mui/icons-material/KeyboardArrowDown";Also update usages below:
- popupIcon={<KeyboardArrowDown />} + popupIcon={<KeyboardArrowDownIcon />}- popupIcon={<KeyboardArrowDown />} + popupIcon={<KeyboardArrowDownIcon />}
33-33: Don’t import JSON from Servers/ into Clients/This crosses package boundaries and will break bundling; fetch from an API or a public asset instead.
-import modelInventoryOptions from "../../../../../../Servers/templates/model-inventory.json";Add client-side loading (example: if you place model-inventory.json under Clients/public):
// near other state const [modelInventoryOptions, setModelInventoryOptions] = useState<{ provider: string; model: string }[]>([]); // on mount useEffect(() => { let alive = true; (async () => { try { const res = await fetch("/model-inventory.json", { cache: "no-store" }); if (!res.ok) throw new Error(`HTTP ${res.status}`); const data = await res.json(); if (alive) setModelInventoryOptions(Array.isArray(data) ? data : []); } catch (e) { console.error("Failed to load model inventory:", e); } })(); return () => { alive = false; }; }, []);If you prefer server mediation, expose GET /api/model-inventory and fetch that instead. I can draft either approach end-to-end.
53-57: New metadata fields wired—LGTM. Verified migrations, server DTOs, controllers, utils & domain models end-to-end.
439-529: Model Autocomplete should populate provider; avoid stale state; don’t block on user loadingCurrently sets model to option.name (includes provider) and never sets provider. Also uses non-functional setState and disables based on unrelated user loading.
- onChange={(_event, newValue) => { - // Handle both option object and free text - if (typeof newValue === "string") { - setValues({ ...values, model: newValue }); - } else if (newValue && typeof newValue === "object") { - setValues({ ...values, model: newValue.name }); - } else { - setValues({ ...values, model: "" }); - } - }} + onChange={(_event, newValue) => { + // Handle both option object and free text + if (typeof newValue === "string") { + // Free text: set model and clear provider + setValues((prev) => ({ ...prev, model: newValue, provider: "" })); + } else if (newValue && typeof newValue === "object") { + // Structured option: set both model and provider + setValues((prev) => ({ + ...prev, + model: newValue.model ?? newValue.name, + provider: newValue.provider ?? prev.provider, + })); + } else { + // Cleared + setValues((prev) => ({ ...prev, model: "", provider: "" })); + } + }} - onInputChange={(_event, newInputValue, reason) => { - if (reason === "input") { - setValues({ ...values, model: newInputValue }); - } - }} + onInputChange={(_event, newInputValue, reason) => { + if (reason === "input") { + setValues((prev) => ({ ...prev, model: newInputValue, provider: "" })); + } + }} - popupIcon={<KeyboardArrowDown />} + popupIcon={<KeyboardArrowDownIcon />} @@ - disabled={isLoadingUsers} + // Keep usable regardless of user-loading state + disabled={false}
|
swaleha456
added
frontend
|
Hello @HarshP4585 |
I am done with requested changes |
|
@MuhammadKhalilzadeh I checked the backend, and it looks good |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx (5)
54-58: Optional model metadata: make fields optional or strip empty strings before submitThese are optional per PR; sending empty strings can pollute data. Either mark optional in the form type or sanitize payload on submit.
Apply one of:
interface NewModelInventoryFormValues { provider_model?: string; // Keep for backward compatibility provider: string; model: string; version: string; approver: number; capabilities: string[]; security_assessment: boolean; status: ModelInventoryStatus; status_date: string; - reference_link: string, - biases: string, - limitations: string, - hosting_provider: string, + reference_link?: string, + biases?: string, + limitations?: string, + hosting_provider?: string, }Or sanitize in handleSubmit:
const handleSubmit = (event?: React.FormEvent) => { if (event) event.preventDefault(); if (validateForm()) { const payload = { ...values }; (['reference_link','biases','limitations','hosting_provider'] as const).forEach((k) => { if (!payload[k]) delete (payload as any)[k]; }); onSuccess?.(payload); handleClose(); } };
81-85: LGTM: initial defaults for new optional fieldsDefaults to empty strings are consistent with current UI; combine with payload sanitization to avoid persisting empties.
376-376: Modal layout: fit-content maxHeight risks off‑screen contentUsing fit-content can allow the modal to exceed viewport height. Prefer a viewport-capped height with internal scroll.
- maxHeight: "fit-content", + maxHeight: "80vh", + overflowY: "auto",
664-691: Reference link and Biases: add minimal validation and UXAdd URL validation for reference_link; consider multiline input for biases.
Add to validateForm:
const urlOk = (s: string) => { try { new URL(s); return true; } catch { return false; } }; if (values.reference_link && !urlOk(values.reference_link)) { (newErrors as any).reference_link = "Enter a valid URL."; }If Field supports multiline:
-<Field id="biases" label="Biases" width={"50%"} ... /> +<Field id="biases" label="Biases" width={"50%"} multiline rows={3} ... />
693-719: Hosting provider and Limitations: consider textarea for LimitationsLimitations tends to be paragraph text; prefer multiline.
-<Field id="limitations" label="Limitations" width={"50%"} ... /> +<Field id="limitations" label="Limitations" width={"50%"} multiline rows={3} ... />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx(7 hunks)Clients/src/presentation/components/Modals/NewModelRisk/index.tsx(1 hunks)Clients/src/presentation/pages/ModelInventory/index.tsx(1 hunks)Clients/src/presentation/pages/ModelInventory/modelInventoryTable.tsx(5 hunks)Servers/scripts/createNewTenant.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Clients/src/presentation/components/Modals/NewModelRisk/index.tsx
- Clients/src/presentation/pages/ModelInventory/modelInventoryTable.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx (1)
Clients/src/presentation/components/Reporting/GenerateReport/GenerateReportFrom/styles.ts (1)
fieldStyle(26-32)
🔇 Additional comments (5)
Clients/src/presentation/components/Modals/NewModelInventory/index.tsx (4)
182-190: Enrich option shape and fix stale memo depsInclude provider/model on options for autofill; drop unused surname/email. Add dependency so list updates when options load.
-const modelInventoryList = useMemo(() => { - return modelInventoryOptions.map((u) => ({ - _id: u.model, - name: `${u.provider} - ${u.model}`, - surname: u.model, - email: u.model - })); -}, []); +const modelInventoryList = useMemo(() => { + return (modelInventoryOptions || []).map((u) => ({ + _id: `${u.provider}:${u.model}`, + name: `${u.provider} - ${u.model}`, + provider: u.provider, + model: u.model, + })); +}, [modelInventoryOptions]);
31-31: Fix build: KeyboardArrowDown must be imported from icon path and with Icon suffixNamed import from @mui/icons-material is invalid and breaks the build. Import from the concrete icon path and use the conventional Icon alias.
Apply:
-import { KeyboardArrowDown } from "@mui/icons-material"; +import KeyboardArrowDownIcon from "@mui/icons-material/KeyboardArrowDown";Update usage below (Line 486):
- popupIcon={<KeyboardArrowDown />} + popupIcon={<KeyboardArrowDownIcon />}Run to confirm no remaining occurrences:
#!/bin/bash rg -nP --type=ts --type=tsx -C2 '\bKeyboardArrowDown\b|@mui/icons-material/KeyboardArrowDown'
34-34: Don’t import JSON from Servers into Clients; fetch insteadCrossing package boundaries will fail bundling and breaks encapsulation. Serve via API or place JSON under Clients/public and fetch.
Here’s a minimal client-side fetch approach:
// remove the import line const [modelInventoryOptions, setModelInventoryOptions] = useState<{provider:string; model:string}[]>([]); useEffect(() => { (async () => { try { const res = await fetch('/model-inventory.json', { cache: 'no-store' }); if (!res.ok) return; const data = await res.json(); setModelInventoryOptions(Array.isArray(data) ? data : []); } catch (e) { console.error('Failed to load model-inventory.json', e); } })(); }, []);
448-537: Model Autocomplete: populate provider on selection, fix popup icon usage, and avoid unrelated disabled stateSelecting a predefined option should set both model and provider. Also fix the icon usage to KeyboardArrowDownIcon and keep the field enabled regardless of user-loading.
<Autocomplete id="model-input" size="small" freeSolo value={values.model} options={modelInventoryList || []} getOptionLabel={(option) => typeof option === "string" ? option : option.name } + isOptionEqualToValue={(option, value) => + typeof value === 'string' + ? option.name === value || option.model === value + : option._id === value._id + } onChange={(_event, newValue) => { // Handle both option object and free text if (typeof newValue === "string") { - setValues({ ...values, model: newValue }); + setValues((prev) => ({ ...prev, model: newValue })); } else if (newValue && typeof newValue === "object") { - setValues({ ...values, model: newValue.name }); + setValues((prev) => ({ + ...prev, + model: newValue.model ?? newValue.name, + provider: newValue.provider ?? prev.provider, + })); } else { - setValues({ ...values, model: "" }); + setValues((prev) => ({ ...prev, model: "" })); } }} onInputChange={(_event, newInputValue, reason) => { if (reason === "input") { - setValues({ ...values, model: newInputValue }); + setValues((prev) => ({ ...prev, model: newInputValue })); } }} @@ - popupIcon={<KeyboardArrowDown />} + popupIcon={<KeyboardArrowDownIcon />} @@ - disabled={isLoadingUsers} + disabled={false} />Clients/src/presentation/pages/ModelInventory/index.tsx (1)
808-812: LGTM: propagate new optional fields to modal initialData
Pre-filling reference_link, biases, limitations and hosting_provider aligns the edit flow with the new UI. Confirm backend schema and API handle these new fields or ignore absent values gracefully.
| reference_link VARCHAR(255) NOT NULL, | ||
| biases VARCHAR(255) NOT NULL, | ||
| limitations VARCHAR(255) NOT NULL, | ||
| hosting_provider VARCHAR(255) NOT NULL, |
There was a problem hiding this comment.
Align new model inventory columns with migration schema
The migration adds these fields as TEXT NOT NULL (populated with empty strings) across existing tenants, but here we create them as VARCHAR(255) NOT NULL. That cap will reject entries longer than 255 characters—for example, practical bias/limitation descriptions—and leaves new tenants with a schema that disagrees with both the migration and the Sequelize model definition. Please switch these to TEXT (and, if desired, keep the same default of '') so freshly created tenants mirror the migrated shape and avoid hard length failures.
- reference_link VARCHAR(255) NOT NULL,
- biases VARCHAR(255) NOT NULL,
- limitations VARCHAR(255) NOT NULL,
- hosting_provider VARCHAR(255) NOT NULL,
+ reference_link TEXT NOT NULL,
+ biases TEXT NOT NULL,
+ limitations TEXT NOT NULL,
+ hosting_provider TEXT NOT NULL,📝 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.
| reference_link VARCHAR(255) NOT NULL, | |
| biases VARCHAR(255) NOT NULL, | |
| limitations VARCHAR(255) NOT NULL, | |
| hosting_provider VARCHAR(255) NOT NULL, | |
| reference_link TEXT NOT NULL, | |
| biases TEXT NOT NULL, | |
| limitations TEXT NOT NULL, | |
| hosting_provider TEXT NOT NULL, |
🤖 Prompt for AI Agents
In Servers/scripts/createNewTenant.ts around lines 666 to 669, the new model
inventory columns are defined as VARCHAR(255) NOT NULL which conflicts with the
migration and Sequelize model (they should be TEXT NOT NULL and populated with
empty strings); update the column definitions for reference_link, biases,
limitations, and hosting_provider to use TEXT NOT NULL (and set the same default
of '' if the script sets defaults) so freshly created tenants match the migrated
schema and avoid 255-character truncation errors.
MuhammadKhalilzadeh
left a comment
MuhammadKhalilzadeh
left a comment
There was a problem hiding this comment.
Thank you @swaleha456 and @HarshP4585
MuhammadKhalilzadeh
deleted the
sw-015-sep-20-add-list-of-modal-in-model-inventory
branch
3 participants
Get model information from model-inventory.json file
Fixes #2155
Please ensure all items are checked off before requesting a review: