Conversation
|
|
ThinuwanW
left a comment
There was a problem hiding this comment.
🤖 Claude Code Review
This PR converts the company industry field from a fixed CrmIndustryEnum dropdown into a searchable, free-form industry picker backed by a Zustand industries record plus a new create-industry mutation. As submitted it does not compile or run: the CrmIndustryEntity type and the industries/setIndustries store slice members it imports do not exist anywhere in the repo, nothing ever fetches the industry list, the backend has no POST /crm/industry endpoint, and CrmCompany.industry is still a Java enum so posting a numeric industry id will be rejected. On top of the missing pieces there is a temporal-dead-zone crash in CompanyModalForm, a silently broken keyboard path for the 'Add industry' option, and a clear-industry action that is a no-op against the PATCH endpoint.
Found 25 new issue(s): 🔴 13 important, 🟡 8 suggestion(s), 🟣 4 nit(s)
|
|
||
| import { authFetchV2 } from "~community/common/utils/axiosInterceptor"; | ||
| import { crmIndustryEndpointsV2 } from "~community/crm/v2/api/utils/ApiEndpoints"; | ||
| import { CrmIndustryEntity } from "~community/crm/v2/types/CrmCommonTypes"; |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
CrmIndustryEntity is imported here (and in AddIndustryOption.tsx:10 and CompanyModalForm.tsx:18) but it is not defined or exported anywhere in the repo — frontend/src/community/crm/v2/types/CrmCommonTypes.ts contains no such interface, and a repo-wide grep finds no declaration. All three new/changed files will fail tsc. Add the interface to CrmCommonTypes.ts alongside the other entities (e.g. export interface CrmIndustryEntity { id: number; name: string; }) plus export type CrmIndustryRecord = Record<number, CrmIndustryEntity>; to match the existing CrmTaskTypeRecord pattern.
| "industryOptions" | ||
| const { industries } = useCrmStoreV2( | ||
| useShallow((store) => ({ | ||
| industries: store.industries |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
store.industries (here) and store.setIndustries (AddIndustryOption.tsx:24-25) do not exist on the CRM v2 store. crmDataSlice.ts defines only companies/contacts/deals/board/tasks/owners/stages/taskTypes, and SliceTypes.ts:CrmDataSliceTypes declares no industries or setIndustries. This is a TypeScript error, and at runtime industries would be undefined, so Object.values(industries) on line 44 throws a TypeError and the whole company modal white-screens. Add industries: CrmIndustryRecord and setIndustries: (industries: CrmIndustryRecord) => void to CrmDataSliceTypes and initialise/implement them in CrmDataSlice the same way taskTypes/setTaskTypes are.
| @@ -0,0 +1,24 @@ | |||
| import { UseMutationResult, useMutation } from "@tanstack/react-query"; | |||
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
📁 File-level observation
There is no query/hook anywhere that fetches the industry list, and nothing writes to the (also missing) industries store slice except the create mutation. Consequence: the dropdown is permanently empty, every search term shows only the 'Add "X" as a new industry' option, duplicates of existing industries can be created because hasExactMatch only checks the empty local list, and selectedIndustryLabel in CompanyModalForm can never resolve an existing company's industry when editing. Add a useGetIndustries query (endpoint + get*QueryKeys factory in api/utils/QueryKeys.ts), hydrate the store from it via the existing useEffect-sync pattern, and have useCreateIndustry call queryClient.invalidateQueries on that key instead of relying solely on the manual store write in AddIndustryOption.handleSuccess.
| }; | ||
|
|
||
| export const crmIndustryEndpointsV2 = { | ||
| CREATE_INDUSTRY: `${moduleAPIPath.CRM}/industry` |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
CREATE_INDUSTRY: ${moduleAPIPath.CRM}/industry posted via authFetchV2 resolves to /v2/crm/industry, but there is no industry controller in the backend at all — backend/src/main/java/com/skapp/community/crmplanner/controller/v1/ contains only CrmTaskType/CrmDealStage/CrmTask/CrmBoard/CrmContact/CrmCompany/CrmDeal controllers, and there is no v2 controller package for industries. CrmIndustry exists only as a JPA model + CrmIndustryDao seeded by CrmConfigServiceImpl. Every 'Add industry' click will 404 and surface the generic error toast. The backend endpoint (and a corresponding list endpoint) must land before this frontend change.
| }; | ||
|
|
||
| const handleIndustrySelected = (industry: CrmIndustryEntity) => { | ||
| setFieldValue("industry", industry.id); |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
setFieldValue("industry", industry.id) writes a numeric industry id, but the backend still models industry as an enum: CrmCompany.industry is @Enumerated com.skapp.community.crmplanner.type.CrmIndustry, and both CrmCompanyCreateDto.industry and CrmCompanyEditDto.industry are typed CrmIndustry. Posting industry: 7 will fail Jackson enum deserialization (400) on both create and edit, and CrmValidations.validateIndustry rejects null. It also violates the declared frontend type — CrmCompanyEntity.industry is still CrmIndustryEnum (CrmCommonTypes.ts:11), so this assignment is only accepted because Formik's setFieldValue is loosely typed. The backend company DTOs/entity must migrate to an industry FK (industryId: number) in the same change set, and CrmCompanyEntity.industry must be retyped accordingly.
| content: ( | ||
| <AddIndustryOption | ||
| name={searchTerm} | ||
| onCreated={handleIndustrySelected} |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
Temporal dead zone: handleIndustrySelected is referenced inside the useMemo factory (which executes synchronously during render at line 82) but is declared with const at line 121, after it. The first render is safe because searchTerm is empty and the branch is skipped, but as soon as the user types a character the memo recomputes, hits this branch, and throws ReferenceError: Cannot access 'handleIndustrySelected' before initialization. Move handleIndustrySelected (and the other handlers it depends on) above the useMemo, wrap it in useCallback, and add it to the dependency array — it is currently missing from [industryOptions, industrySearchTerm] (line 111), which would also produce a stale closure once the ordering is fixed.
| }; | ||
|
|
||
| const handleIndustrySelect = (item: SearchableDropdownItem) => { | ||
| const industry = industries[Number(item.id)]; |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
Keyboard selection of the 'Add industry' row silently does nothing. SearchableDropdown's Enter handler calls handleSelect(items[activeIndex]) → onSelect(item) → this function, where Number(ADD_INDUSTRY_ITEM_ID) is NaN, so industries[NaN] is undefined and the early return fires — then handleClose() runs and the dropdown closes with nothing created. Only a direct mouse click on the nested <button> inside the <li role="option"> works, which is itself an a11y violation (interactive control inside an option). Fix by handling the sentinel id explicitly in onSelect (trigger the create mutation from the parent when item.id === ADD_INDUSTRY_ITEM_ID) and rendering the add row as plain content rather than a nested button — SearchableDropdown already exposes an onEmptyActivate hook for this kind of 'create from query' affordance.
| }; | ||
|
|
||
| const handleClearIndustry = () => { | ||
| setFieldValue("industry", undefined); |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
handleClearIndustry sets the field to undefined. On edit, getChangedCompanyFields sees undefined !== initialValues.industry and adds industry: undefined to changedFields, so the modal submits — but JSON.stringify drops undefined keys, so the PATCH body contains no industry at all, and CrmCompanyServiceImpl line 274 only overwrites when the incoming industry is non-null. Result: the user clears the industry, sees a success toast, and the industry is unchanged on the server. Send an explicit null (and make the backend handle an explicit null clear), or keep the existing CrmIndustryEnum.NONE-equivalent sentinel.
| return items; | ||
| }, [industryOptions, industrySearchTerm]); | ||
|
|
||
| const selectedIndustryLabel = values.industry |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
selectedIndustryLabel does industries[Number(values.industry)], but getCompanyFormInitialValues (companyUtil.ts:87) still initialises industry: company?.industry ?? CrmIndustryEnum.NONE — a string enum. Number("NONE") / Number("RETAIL") is NaN, so the lookup always misses and the field renders as an empty search box even for a company that already has an industry. It also breaks getChangedCompanyFields, which will now compare a string enum against a number. companyUtil.ts and its tests (utils/__test__/companyUtil.test.ts) were not updated in this PR and need to move to the id-based representation together with the type change.
| Dropdown, | ||
| InputField | ||
| } from "@rootcodelabs/skapp-ui"; | ||
| import { ButtonV2, CloseIcon, InputField } from "@rootcodelabs/skapp-ui"; |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
📁 File-level observation
SidePanelCompanyHeader.tsx (lines 22-26, 79) still resolves the industry for display via useTranslator("crmModule", "companies", "industryOptions") and translateText([industry]). Once industry becomes a numeric id created from free-form user input, that lookup can never match a key in the industryOptions block and i18next will render the raw key (e.g. "12") in the side panel. Update that consumer to resolve the name from the industries store/record, and decide whether the now-legacy companies.industryOptions translation block should be retired.
| const response = await authFetchV2.post( | ||
| crmIndustryEndpointsV2.CREATE_INDUSTRY, | ||
| { name } | ||
| ); |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
createIndustry is declared as Promise<CrmIndustryEntity> but response?.data?.results?.[0] can legitimately be undefined (empty/absent results). The undefined then flows into AddIndustryOption.handleSuccess, which dereferences createdIndustry.id as an object key and calls onCreated(createdIndustry) — a TypeError inside the mutation's success path, which React Query will not route to onError, so the user sees no feedback at all. Either narrow the return type to CrmIndustryEntity | undefined and guard in handleSuccess, or throw explicitly when the payload is missing so onError fires and the toast shows.
|
|
||
| const handleClick = (event: MouseEvent<HTMLButtonElement>) => { | ||
| event.stopPropagation(); | ||
| createIndustry(name); |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
The industry name comes straight from the unbounded search input with no client-side constraint: the SearchableDropdown InputField has no maxLength (unlike the company name field, which uses characterLengths.COMPANY_NAME_LENGTH), and only the trimmed string is validated for emptiness. A user can create an industry of arbitrary length or one differing from an existing entry only by surrounding punctuation. Add a maxLength on the industry search input matching the backend crm_industry.name column limit, and rely on a server-side uniqueness check (case-insensitive) rather than the client-only hasExactMatch test in CompanyModalForm.
| @@ -0,0 +1,70 @@ | |||
| import { PlusIcon } from "@rootcodelabs/skapp-ui"; | |||
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
📁 File-level observation
No .test.tsx accompanies this new component, and CompanyModalForm has no test either. Given the component owns a mutation, a toast side effect and a store write, it is worth covering at minimum: happy path (click → mutation called with the trimmed name → store updated → onCreated fired), error path (error toast rendered), and the disabled-while-pending state. Use @testing-library/react with the MockTheme wrapper and screen.getByRole("button", …) per the repo's testing conventions.
| id: industry, | ||
| label: translateIndustryOptions([industry]), | ||
| value: industry | ||
| Object.values(industries).map((industry) => ({ |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
The industryOptions memo is an unnecessary intermediate — it maps the store record to {id, label} only for the second memo (line 82) to filter it and remap it to {id, content}. Collapsing the two into a single useMemo over Object.values(industries) removes an allocation per render and one dependency edge. Note also the round-trip String(industry.id) here and Number(item.id) on line 127, which is what makes the ADD_INDUSTRY_ITEM_ID sentinel silently coerce to NaN.
| type="button" | ||
| onClick={handleClick} | ||
| disabled={isPending} | ||
| className="body3 -mx-4 -my-2 flex h-11 items-center gap-2 rounded-xl bg-primary-background px-3 text-primary-text" |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
The -mx-4 -my-2 negative margins exist purely to cancel the px-4 py-2 padding that SearchableDropdown applies to each <li> (SearchableDropdown.tsx:248). This silently couples this atom to an unrelated component's internal styling — any padding change there will visually break this row. Prefer having SearchableDropdown accept an item-level disablePadding/className override, or render this as full-bleed content the dropdown knows about. Also, isPending only disables the button with no visual busy state — consider a spinner or aria-busy so the pending network call is perceivable.
| type="button" | ||
| onClick={handleClick} | ||
| disabled={isPending} | ||
| className="body3 -mx-4 -my-2 flex h-11 items-center gap-2 rounded-xl bg-primary-background px-3 text-primary-text" |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
The add-row button is missing w-full text-left, so it only occupies its intrinsic (shrink-to-fit) width. A <button> keeps fit-content sizing even with display:flex, and the -mx-4 only shifts it left — it does not stretch it. The parent <li> in SearchableDropdown is full width and has its own onClick={() => handleSelect(item)}, so any click on the row to the right of the button text bypasses stopPropagation() and goes to handleIndustrySelect, which resolves industries[Number("add-industry")] → industries[NaN] → undefined → early return, while SearchableDropdown.handleSelect still calls handleClose(). Net effect: the dropdown closes, no industry is created, no feedback is given. Compare the established option pattern in OwnerOptionItem.tsx (... cursor-pointer w-full text-left). Fix: add w-full text-left so the button covers the whole row. Better still, drop the nested button entirely and handle creation in the parent's onSelect when item.id === ADD_INDUSTRY_ITEM_ID (or use the onEmptyActivate prop SearchableDropdown already exposes for exactly this) — that also removes the invalid interactive-control-inside-role="option" markup and makes the row keyboard-selectable.
| import { ADD_INDUSTRY_ITEM_ID } from "~community/crm/v2/constants/commonConstants"; | ||
| import { useCrmStoreV2 } from "~community/crm/v2/store/store"; | ||
| import { | ||
| CrmCompanyEntity, |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
CrmCompanyEntity.industry is still declared as industry?: CrmIndustryEnum in types/CrmCommonTypes.ts, but the form now assigns a numeric industry id to it. This does not fail type-checking only because Formik's setFieldValue takes any — so TypeScript strict mode gives zero protection here and the drift propagates silently through getTrimmedCompanyValues and getChangedCompanyFields (which now compares an enum string against a number) into the create/edit payloads. The field should be retyped (and ideally renamed to industryId?: number to match what it actually carries), with companyUtil.ts and utils/__test__/companyUtil.test.ts updated in the same change. As it stands the model type and the runtime value disagree with no compile-time signal.
| clearAriaLabel={translateText(["ariaLabels", "clearIndustry"])} | ||
| fieldAriaLabel={translateText(["ariaLabels", "industry"])} | ||
| searchValue={industrySearchTerm} | ||
| onSearchChange={handleIndustrySearchChange} |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
Clearing the search box closes the dropdown and it cannot be reopened without blurring and refocusing. SearchableDropdown.handleInputChange does setIsOpen(e.target.value.trim().length > 0), and the onFocus handler that would reopen it is on the wrapper div, so it will not fire again while the input is already focused. Every other consumer of SelectableSearchField (e.g. TaskModalForm owner/contact/deal) is backed by a server lookup that returns nothing for an empty term, so this never mattered before; this field is the first one where the empty term is expected to show the full list. Fix by making the dropdown stay open when items.length > 0 on an empty term — e.g. pass an explicit open handler, or change handleInputChange to setIsOpen(true) and let isDropdownOpen gate on items/emptyMessage as it already does.
|
|
||
| const items: SearchableDropdownItem[] = matches.map((option) => ({ | ||
| id: option.id, | ||
| content: option.label |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
Dropdown item content is a bare string (content: option.label), unlike every other consumer of SearchableDropdownItem in this module, which wraps it as <div className="w-full truncate" title={name}>{name}</div> (see TaskModalForm's contact/deal item memos). Industry names are now unbounded free text created by users, so a long name will overflow the popper instead of truncating, and there is no title tooltip to reveal the full value. Wrap the label the same way for consistency and to keep the popper width stable.
| onSearchChange={handleIndustrySearchChange} | ||
| items={industryDropdownItems} | ||
| onSelect={handleIndustrySelect} | ||
| emptyMessage={translateText(["emptyStates", "noIndustries"])} |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
The new emptyStates.noIndustries message is effectively unreachable in the scenario it was written for. items is only empty when the search term is empty AND the store holds no industries; as soon as the user types anything that matches nothing, the add-industry row is pushed so items.length === 1 and SearchableDropdown renders the list instead of emptyMessage. So 'No industries found' shows only on an empty search box with an empty store, which reads as a bug rather than an empty state. Either drop the prop or render the message as a non-selectable header above the add row.
| onCreated: (industry: CrmIndustryEntity) => void; | ||
| } | ||
|
|
||
| const AddIndustryOption: FC<AddIndustryOptionProps> = ({ name, onCreated }) => { |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
This component is placed under components/atoms/ but it owns an API mutation, writes to the Zustand store and fires a toast. Per the module convention (atoms / molecules / organisms) and the guideline against components that both fetch data and render, this belongs in molecules/ with the mutation lifted to the parent (CompanyModalForm already owns the industry state and the onCreated callback), leaving this file as a presentational row. It also re-creates the crmModule.companies.companyModal translator that its parent already receives as the translateText prop — passing the existing translator down avoids the namespace being hardcoded in two places.
| <button | ||
| type="button" | ||
| onClick={handleClick} | ||
| disabled={isPending} |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
No loading feedback during creation: isPending only disables the button, with no spinner or label change, so a slow create looks like a dead row. Additionally, the <li key="add-industry"> stays mounted across search-term changes, and the search input is not disabled while pending — so the user can keep typing and the button label will show the new term while the in-flight request is still creating the old one, and onCreated will then select the old industry. Add a loading indicator (or use ButtonV2's isLoading) and either disable/ignore search input while the create is in flight, or key the row on the search term so a new term remounts it.
| content: option.label | ||
| })); | ||
|
|
||
| const hasExactMatch = matches.some( |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
hasExactMatch is the only guard against creating a duplicate industry, and it only checks the industries currently in the local Zustand record. There is no server-side uniqueness check and no exists lookup — unlike company and deal names, which both have CHECK_COMPANY_NAME_EXISTS / CHECK_DEAL_NAME_EXISTS endpoints. Two tabs, two users, or a stale store will each happily create their own 'Retail'. The comparison also doesn't normalise internal whitespace or casing beyond toLowerCase(), so 'Real Estate' and 'Real Estate' become separate rows. Add a uniqueness constraint/exists check server-side (the crm_industry table already exists and is seeded by DefaultCrmIndustryTemplate) and normalise the name before comparing/sending.
| @@ -0,0 +1,24 @@ | |||
| import { UseMutationResult, useMutation } from "@tanstack/react-query"; | |||
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
File naming is inconsistent with its siblings in the same directory — CompanyApi.ts, ContactApi.ts, BoardApi.ts, DealApi.ts, TaskApi.ts all omit the Crm prefix since the module path already scopes them. Rename to IndustryApi.ts to match. (The endpoint constant crmIndustryEndpointsV2 is fine — it matches crmTaskEndpointsV2/crmDealEndpointsV2.)
| "address": "Address", | ||
| "industry": "Industry" | ||
| "industry": "Industry", | ||
| "addNewIndustry": "Add \"{{name}}\" as a new industry" |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
addNewIndustry is an action/CTA string with interpolation but it is placed under labels, which everywhere else in this namespace holds static form-field labels (name, contactNumber, website, ...). Move it under buttons (or a new actions group) so the key path reflects its role, consistent with the module.section.key convention.
Follows the add-company-via-contact pattern (PR #1875) instead of creating the industry up front: - AddIndustryOption is presentational only, matching AddNewCompanyOption - the option row no longer owns a mutation, a toast or a click handler. - Option building moves into companyUtil.getIndustryOptions, mirroring getCompanyOptions, including the name-availability and max-length guards. - The company payload carries industryId | industryName, mirroring the contact's companyId | companyName, so picking "Add x as a new industry" just holds the name until the company is saved. - Seeded industries are translated through getIndustryDisplayName, the same way getStageDisplayName handles default deal stages, so names stored as constants no longer surface as raw ALL_CAPS text. - Drops CrmIndustryApi and its endpoint constant, now unused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The header translated company.industry through the industryOptions namespace, which returned the raw key once that field became a numeric industry id. CompanySidePanel already owns the store subscription, so it resolves the name there - via getIndustryDisplayName, so seeded industries stay translated and user-created ones show as typed - and passes it down. The header keeps no store access or translator of its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…crm-add-industry-via-company-modal-FE
|



PR checklist
TaskId: (https://github.com/SkappHQ/skapp/issues/[id])
Summary
dropdown. Options now come from
store.industries(hydrated from board init-data),so each tenant sees its own industries rather than the hardcoded
CrmIndustryEnum.Add "x" as a new industryrowappears at the bottom of the list. Selecting it creates the industry and selects it —
no modal, no page change.
AddIndustryOptionatom owns that row end to end: the create mutation, writingthe new industry into the store, the error toast, and its disabled state while the
request is in flight. The form only receives
onCreatedand selects the result.CrmIndustryApi.useCreateIndustrycallingPOST /v2/crm/industry, plus theCREATE_INDUSTRYendpoint constant.labels.addNewIndustry,emptyStates.noIndustries,ariaLabels.clearIndustry,toastMessages.addIndustry.errorDescription, and theindustry placeholder becomes "Search or add industry".
How to test
Tourism— the add row appears at the bottom.button.
(it went into the store).
is kept so you can correct it.
Project Checklist
npm run formatnpm run check-lintOther
AddIndustryOptionAdditional Information
Blocking dependency. This branch does not compile on its own. It needs
feat/crm-board-init-data-industriesmerged or rebased in first, which provides theCrmIndustryEntitytype and theindustries/setIndustriesstore slice. Without ittscreports 8 errors (3 for the missing type, 5 for the missing slice) on top of theproject's existing baseline. It also needs the backend PR above for the endpoint.
Two known gaps, both worth a follow-up rather than blocking review:
Enteron the add row does nothing — creation is mouse-only. The dropdown'sEnter path calls the form's
onSelect, while the create handler lives insideAddIndustryOptionwhere the form can't reach it. Fixing it properly means teachingSearchableDropdownabout action rows.CrmCompanyEntity.industryis still typedCrmIndustryEnumbut now receives anumeric industry id.
setFieldValueaccepts it and reads useNumber(...), but thetype is inaccurate until the
industryIdmigration lands.A duplicate name currently shows the generic error toast.
SidePanelAddDeal.tsxhas aprecedent for reading
messageKeyoff the error and showing an inline field messageinstead — left out of scope here.