Conversation
tommaso-ascani
left a comment
tommaso-ascani
left a comment
There was a problem hiding this comment.
Review
Same feature as the sibling PRs (nethesis/nethvoice-cti#515, nethesis/nethcti-middleware#63): split firstname/lastname + new social/phone/email fields. Good news first: this PR avoided the sibling CTI PR's crash pattern — the person/company toggle keeps first name/last name/job inputs permanently mounted and only CSS-hides them (className={contactType === 'company' ? 'hidden' : ''}), so there's no ref-after-unmount dereference and no data loss switching contact type back and forth.
Bugs
1. Missing translation key — src/renderer/src/components/Modules/NethVoice/SearchResults/SearchNumberBox.tsx (~line 209).
The new "Add to phonebook" button calls t('Phonebook.Create contact'), but only Phonebook.Create new contact was added to any of the four locale files (public/locales/{en,it} and src/renderer/public/locales/{en,it}). This key is never defined, so i18next falls back to showing the raw key/fallback text instead of a translated label.
2. "+N other numbers" badge undercounts with the new phone fields — src/renderer/src/components/Modules/NethVoice/SearchResults/SearchNumber.tsx (~line 55, 73).
otherDevicesCount is still computed from the old hardcoded keys = ['extension','cellphone','homephone','workphone'], never extended for workphone2/cellphone2/otherphone added by this PR. A contact whose only extra numbers are in the new fields shows no "+N other numbers" indicator at all, even though it has multiple callable numbers.
3. ModuleTitle lost its truncation tooltip with no replacement, affecting untouched consumers — src/renderer/src/components/ModuleTitle.tsx (~lines 26-33).
The react-tooltip <Tooltip>/data-tooltip-* wiring was removed (title still gets truncate), while ContactNameAndAction.tsx got a proper replacement via the new useIsTruncated hook + CustomThemedTooltip. ModuleTitle itself didn't. Consumers this PR never touched now truncate long titles with no way to see the full text: Parking/ParkingBox.tsx (Parking (N), grows with parked-call count), About/AboutModule.tsx, Speeddials/SpeedDials/SpeedDialFormBox.tsx (localized "Edit/Create speed dial" strings can run long, e.g. in Italian).
Cleanup
4. MultiSelectCombobox and the "Add field" flyout hand-roll what Dropdown/HeadlessUI already provide — src/renderer/src/components/Nethesis/MultiSelectCombobox.tsx, AddToPhonebookBox.tsx (~lines 140-166, 318-337, 838-925).
Own outside-click listener + manual keyboard handling instead of extending the existing Dropdown/DropdownItem (which gained a topLeft position in this very PR) or the HeadlessUI Combobox pattern already used elsewhere. The file's own comment says this was "ported from the NethVoice CTI component" — i.e. the same anti-pattern flagged in the sibling PR was intentionally carried over here too.
5. Zod validation schema rebuilt on every render — AddToPhonebookBox.tsx (~lines 60-116).
The discriminatedUnion/superRefine schema (now ~20+ fields, up from ~3) is constructed inline in the component body instead of useMemo'd or hoisted to module scope, so every re-render (typing, toggling person/company, opening a submenu) reconstructs the whole zod object graph for nothing.
6. i18n already drifted between the two locale trees this PR must keep in sync by hand — public/locales/en/translations.json vs src/renderer/public/locales/en/translations.json. A quick diff shows src/renderer/... already has a "Hold" key absent from the top-level copy — evidence the manual-sync convention is already leaking, on top of finding #1 above.
7. New app-wide DropdownItem text-color class, not visually re-verified on unrelated menus — src/renderer/src/components/Nethesis/dropdown/DropdownItem.tsx (~line 38).
!isRed && !active ? theme?.item?.light : '' applies unconditionally to every non-active, non-red item, added to support the new phonebook dropdowns but with blast radius over PresenceBadge's status menu and both SettingsDevicesDialog/SettingsIncomingCallsDialog device pickers.
8. Per-item truncation hook mounted with no payoff in list views — src/renderer/src/components/Modules/NethVoice/BaseModule/ContactNameAndAction.tsx (~lines 93-95).
useIsTruncated([contact.company]) runs unconditionally per row, but ContactNumber.tsx (used by FavouritesBox/SpeedDialsBox) never passes showCompany, so the ref this hook watches never attaches in those exact render-heavy list contexts — pure per-item hook overhead for zero benefit there.
tommaso-ascani
left a comment
tommaso-ascani
left a comment
There was a problem hiding this comment.
Follow-up on latest fixes (c30556d1)
Fix status of the 8 prior findings:
- Missing translation key — Turns out not actually broken:
Phonebook.Create contactexists in all four locale files (it's distinct fromCreate new contact, the page title). False alarm on my end, sorry for the noise. - "+N other numbers" undercount — Fixed.
SearchNumber.tsx'skeysarray now includesworkphone2/cellphone2/otherphone, deduped viaSet. ModuleTitlelost its tooltip — Fixed. Truncation-gated tooltip reintroduced internally (useIsTruncated+data-tooltip-id+CustomThemedTooltip), soParking/About/SpeedDialFormBoxinherit it automatically with no changes needed on their end.MultiSelectCombobox/"Add field" hand-rolled — Not fixed, left as-is intentionally. Both now have explicit comments justifying the divergence ("ported from the NethVoice CTI component", "the only one with a nested submenu, so hand-rolled like the CTI"). Noting as accepted, not re-flagging.- Zod schema rebuilt every render — Fixed via
useMemo, but see the new bug below — the fix has a side effect. - i18n drift between locale trees — Not fixed, same class persists (e.g.
Parks.Holdstill present insrc/renderer/public/locales/{en,it}but absent from the top-levelpublic/locales/{en,it}copies). - App-wide
DropdownItemtext-color class — Not fixed, unchanged since it was introduced. Still blast-radius overPresenceBadge/SettingsDevicesDialog/SettingsIncomingCallsDialog. - Per-item
useIsTruncatedwith no payoff in list views — Not fixed.ContactNumber.tsx(used byFavouritesBox/SpeedDialsBox) still never passesshowCompany, so the hook mounted inContactNameAndAction.tsxstill has nothing to measure there.
New bug introduced by the zod memoization fix
AddToPhonebookBox.tsx's resultSchema is now useMemo(..., [t]), but t is imported directly from i18next at module scope (import { t } from 'i18next'), not the reactive function useTranslation() returns — its reference never changes. If the app's language is switched while this component stays mounted, the schema's baked-in validation messages (e.g. Common.This field is required) stay frozen in whichever language was active on first render, instead of picking up the new language like they did before this fix (when the schema was rebuilt every render).
Follow-up fix (
|
2 participants
Reference