feat(contacts): refactor contacts#3995
feat(contacts): refactor contacts#3995devin-ai-integration[bot] wants to merge 6 commits intomainfrom
Conversation
…n/ordering support Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote-storybook canceled.
|
Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 5 potential issues.
⚠️ 1 issue in files not directly in the diff
⚠️ details.tsx handleTogglePin computes maxOrder from humans table only, not both tables (apps/desktop/src/components/main/body/contacts/details.tsx:123-131)
In the new combined contacts list (contacts-list.tsx), pinning computes maxOrder across both humans and organizations tables so newly-pinned items sort to the end of the combined list. However, the person detail panel in details.tsx:113-132 still computes maxOrder from the humans table alone. A user pinning a person via the detail view gets a pin_order that ignores organizations' pin_order values, causing the item to appear in the wrong position.
Detailed Explanation
The new contacts-list.tsx correctly checks both tables when pinning (e.g., contacts-list.tsx:347-359 for PersonItem):
const maxHumanOrder = Object.values(allHumans).reduce(...);
const maxOrgOrder = Object.values(allOrgs).reduce(...);
pin_order: Math.max(maxHumanOrder, maxOrgOrder) + 1;But details.tsx:123-131 only checks humans:
const allHumans = store.getTable("humans");
const maxOrder = Object.values(allHumans).reduce((max, h) => {
const order = (h.pin_order as number | undefined) ?? 0;
return Math.max(max, order);
}, 0);
store.setPartialRow("humans", selectedHumanId, {
pinned: true,
pin_order: maxOrder + 1,
});Example: Organization "Acme" is pinned at pin_order: 5, max human pin_order is 2. Pinning person "John" from the detail view gives pin_order: 3. In the combined pinned list, John appears before Acme (3 < 5) instead of at the end.
Impact: Newly pinned persons from the detail view may not appear at the end of the combined pinned list, violating the expected "pin adds to end" behavior described in the PR.
View 2 additional findings in Devin Review.
apps/desktop/src/components/main/body/contacts/organizations.tsx
Outdated
Show resolved
Hide resolved
| ...base, | ||
| type: "contacts", | ||
| state: tab.state ?? { | ||
| selectedOrganization: null, | ||
| selectedPerson: null, | ||
| selected: null, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🚩 No tab state migration for persisted ContactsState shape change
The ContactsState shape changed from { selectedOrganization: string | null, selectedPerson: string | null } to { selected: ContactsSelection | null }. Pinned tabs are serialized/deserialized via pinned-persistence.ts:15-35, which uses JSON.stringify/JSON.parse with no schema validation or migration. If a user has a pinned contacts tab saved with the old shape ({ selectedOrganization, selectedPerson }), on next app load restorePinnedTabsToStore will restore that tab with the old state shape. The getDefaultState in schema.ts:130-137 only applies when tab.state is null/undefined — if the old state object is present, it's used as-is. This means tab.state.selected would be undefined at runtime, and tab.state.selectedOrganization/selectedPerson would be present but unused. The auto-select useEffect in contacts/index.tsx:125-129 would then trigger (since selected is falsy), masking the issue. So this likely won't crash but the old persisted selection is silently lost.
(Refers to lines 130-137)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 8f3d029 — details.tsx handleTogglePin now computes maxOrder across both humans and organizations tables, matching the behavior in contacts-list.tsx.
There was a problem hiding this comment.
Acknowledged — the auto-select useEffect masks this so it won't crash, just silently resets the old selection. Noted in the PR description checklist.
…ng, fix details maxOrder Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
| useEffect(() => { | ||
| if (!selectedOrganization && !selectedPerson && humanIds.length > 0) { | ||
| setSelectedPerson(humanIds[0]); | ||
| if (!selected && allHumanIds.length > 0) { | ||
| setSelected({ type: "person", id: allHumanIds[0] }); | ||
| } | ||
| }, [humanIds, selectedPerson, selectedOrganization, setSelectedPerson]); | ||
|
|
||
| const isViewingOrgDetails = selectedOrganization && !selectedPerson; | ||
| }, [allHumanIds, selected, setSelected]); |
There was a problem hiding this comment.
🚩 Auto-select useEffect only selects persons, never organizations
The auto-select useEffect at line 125-129 only queries visibleHumans and auto-selects a person when selected is null. If the contact list contains only organizations and no people, no item will be auto-selected and the detail panel will show "Select a person to view details". This is a design choice (the old code also only auto-selected people), but may be surprising now that organizations are first-class items in the combined list.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
…list Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
1 participant
Summary
Refactors the contacts system by replacing the old
{ selectedOrganization, selectedPerson }state shape with a tagged unionContactsSelection({ type: "person" | "organization", id }), and adds pinning/ordering support for both people and organizations.Key changes:
ContactsSelectionenum in Rust (state.rs) and auto-generated TypeScript bindingscontacts-list.tsx(~600 LOC) with combined pinned list, drag-to-reorder (viamotion/react), search hotkey, and sort optionscreated_at,pinned,pin_orderfields to organization schema (zod + tinybase + queries)created_at,pin_orderto human transform/frontmatter logicContactsState(advanced-search, calendar-view, tab schema, tests)organizations.tsxandpeople.tsx(replaced bycontacts-list.tsx)Updates since initial revision
Addressed bot review comments:
organizations.tsxandpeople.tsx— no longer imported after the switch tocontacts-list.tsx.pin_orderclearing fixed: Changedpin_order: undefinedtopin_order: 0when unpinning incontacts-list.tsxanddetails.tsx. TinyBase'ssetPartialRowignoresundefinedvalues, so the oldpin_orderwas silently persisting on unpinned items.details.tsxmaxOrder bug fixed:handleTogglePinin the detail panel now computesmaxOrderacross bothhumansandorganizationstables (previously only checkedhumans), matching the behavior incontacts-list.tsx.searchValueis non-empty, pinned items render in a plain<div>instead of a<Reorder.Group>, preventingpin_ordercollisions between visible and hidden pinned items. Reorder remains fully functional when no search filter is active.NewOrgFormcode removed: Deleted theshowNewOrgstate, the conditional<NewOrgForm>render block, and the entireNewOrgFormcomponent (~70 lines). ThehandleAddcallback only opens the person form, so the org form was unreachable.Merge conflict resolution
Merged latest
mainto resolve conflicts:calendar-view.tsx: Main refactored this file significantly (removedCalendarSidebarContententirely). The PR's change (removing contacts permission check) is now moot — took main's version.people.tsx: PR deleted this file (replaced bycontacts-list.tsx), main added Facehash avatars to it. Kept deleted and ported the Facehash avatar change tocontacts-list.tsx.contacts-list.tsx: Now uses<Facehash>component for person avatars instead ofgetInitials, matching main's avatar style.Review & Testing Checklist for Human
ContactsStateshape changed from{ selectedOrganization, selectedPerson }to{ selected: ContactsSelection | null }. There is no migration logic. Old persisted tabs will haveselected: undefinedat runtime — the auto-selectuseEffectmasks this, but the old selection is silently lost. Verify no runtime errors on app load with pre-existing contacts tabs.<Facehash>integration was ported from main'speople.tsxchange but has not been visually tested. Verify person avatars render correctly in the contacts list (props:size={32},interactive={false},showInitial={false}).contacts-list.tsx) has zero component tests. Verification is limited to typecheck + transform unit tests. Manual testing is essential.created_atsort for organizations: Existing organizations won't havecreated_atpopulated. Verify "Newest" and "Oldest" sort options produce sensible order for pre-existing data.Suggested test plan: Open the contacts tab, create a few people and organizations, pin some of each, reorder the pinned list via drag, switch sort options, use Cmd+F search (verify reorder is disabled and items remain visible), clear search (verify reorder works again), close and reopen the app, and verify selection state + pin order are preserved. Also try pinning from the detail panel and confirm the item appears at the end of the combined pinned list. Verify person avatars display correctly with Facehash.
Notes
mainto eliminate merge conflict noise.pin_orderround-trip for both human and organization transforms.Link to Devin run: https://app.devin.ai/sessions/20cd5306dbec46ed836175a3e6487227
Requested by: @ComputelessComputer