feat(contacts): refactor contacts#3818
Conversation
✅ Deploy Preview for hyprnote-storybook canceled.
|
✅ Deploy Preview for hyprnote canceled.
|
30b2317 to
71ba18f
Compare
4e7fd98 to
84386d7
Compare
Implement pinning functionality for organizations with drag and reorder capabilities. Separate pinned and unpinned organizations, add visual divider, and enable custom pinned order management.
84386d7 to
f3e7569
Compare
|
@yujonglee made changes to contacts view that involves adding to tinybase. |
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
…rm.ts Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ss-table pin ordering Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
Co-Authored-By: john@hyprnote.com <john@hyprnote.com>
| const allOrgs = store.getTable("organizations"); | ||
| const maxOrder = Object.values(allOrgs).reduce((max, o) => { | ||
| const order = (o.pin_order as number | undefined) ?? 0; | ||
| return Math.max(max, order); | ||
| }, 0); | ||
| store.setPartialRow("organizations", organizationId, { | ||
| pinned: true, | ||
| pin_order: maxOrder + 1, | ||
| }); |
There was a problem hiding this comment.
Pin order calculation is inconsistent with unified contacts list. This only checks organizations table, but should check both humans and organizations tables since they share a unified pin order space.
When a user pins an organization here, it could get the same pin_order as an already-pinned person, causing ordering conflicts in the unified contacts list.
const allOrgs = store.getTable("organizations");
const allHumans = store.getTable("humans");
const maxOrgOrder = Object.values(allOrgs).reduce((max, o) => {
const order = (o.pin_order as number | undefined) ?? 0;
return Math.max(max, order);
}, 0);
const maxHumanOrder = Object.values(allHumans).reduce((max, h) => {
const order = (h.pin_order as number | undefined) ?? 0;
return Math.max(max, order);
}, 0);
store.setPartialRow("organizations", organizationId, {
pinned: true,
pin_order: Math.max(maxOrgOrder, maxHumanOrder) + 1,
});| const allOrgs = store.getTable("organizations"); | |
| const maxOrder = Object.values(allOrgs).reduce((max, o) => { | |
| const order = (o.pin_order as number | undefined) ?? 0; | |
| return Math.max(max, order); | |
| }, 0); | |
| store.setPartialRow("organizations", organizationId, { | |
| pinned: true, | |
| pin_order: maxOrder + 1, | |
| }); | |
| const allOrgs = store.getTable("organizations"); | |
| const allHumans = store.getTable("humans"); | |
| const maxOrgOrder = Object.values(allOrgs).reduce((max, o) => { | |
| const order = (o.pin_order as number | undefined) ?? 0; | |
| return Math.max(max, order); | |
| }, 0); | |
| const maxHumanOrder = Object.values(allHumans).reduce((max, h) => { | |
| const order = (h.pin_order as number | undefined) ?? 0; | |
| return Math.max(max, order); | |
| }, 0); | |
| store.setPartialRow("organizations", organizationId, { | |
| pinned: true, | |
| pin_order: Math.max(maxOrgOrder, maxHumanOrder) + 1, | |
| }); |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
🚩 Old OrganizationsColumn and PeopleColumn are now dead code
The imports of OrganizationsColumn (from organizations.tsx) and PeopleColumn / useSortedHumanIds (from people.tsx) were removed from apps/desktop/src/components/main/body/contacts/index.tsx. These components are no longer referenced anywhere in the codebase. Notably, people.tsx:250-282 still has the nested <button> HTML validity issue (outer <button> wrapping inner pin <button>), and organizations.tsx:224-232 only checks the organizations table for max pin_order (not cross-table). Neither issue matters at runtime since these components are unreachable, but the files should be cleaned up or removed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| expect(result).toEqual({ | ||
| frontmatter: { | ||
| user_id: "user-1", | ||
| created_at: "2024-01-01T00:00:00Z", | ||
| name: "John Doe", | ||
| emails: ["john@example.com"], | ||
| org_id: "org-1", |
There was a problem hiding this comment.
🟡 Test assertion missing pin_order: 0 for humanToFrontmatter
The storeToFrontmatter function (apps/desktop/src/store/tinybase/persister/human/transform.ts:50) always emits pin_order: store.pin_order ?? 0, so when pin_order is not provided in the input, the actual output frontmatter object contains pin_order: 0. However, the test's expected output does not include pin_order, causing toEqual to fail due to the extra non-undefined property.
Root Cause
The storeToFrontmatter function always includes pin_order with a fallback of 0:
pin_order: store.pin_order ?? 0,When the test input at line 157 omits pin_order, the actual output is { ..., pin_order: 0 }. Vitest's toEqual performs strict deep equality — an extra property with a non-undefined value (0) in the received object that is absent in the expected object causes the assertion to fail.
Impact: This test will fail, blocking CI.
(Refers to lines 168-179)
Was this helpful? React with 👍 or 👎 to provide feedback.
| expect(result).toEqual({ | ||
| frontmatter: { | ||
| user_id: "user-1", | ||
| created_at: "2024-01-01T00:00:00Z", | ||
| name: "Acme Corp", | ||
| pinned: false, | ||
| }, | ||
| body: "", | ||
| }); |
There was a problem hiding this comment.
🟡 Test assertion missing pin_order: 0 for organizationToFrontmatter (unpinned case)
The organizationToFrontmatter function (apps/desktop/src/store/tinybase/persister/organization/transform.ts:30) always emits pin_order: org.pin_order ?? 0, so when the input has no pin_order, the output contains pin_order: 0. The test expected object omits this field, causing toEqual to fail.
Root Cause
Same mechanism as the human transform: organizationToFrontmatter always includes pin_order with a default of 0:
pin_order: org.pin_order ?? 0,The test at line 62-70 expects a frontmatter without pin_order, but the actual output includes pin_order: 0. Vitest's toEqual treats this as a mismatch.
Impact: This test will fail, blocking CI.
| expect(result).toEqual({ | |
| frontmatter: { | |
| user_id: "user-1", | |
| created_at: "2024-01-01T00:00:00Z", | |
| name: "Acme Corp", | |
| pinned: false, | |
| }, | |
| body: "", | |
| }); | |
| expect(result).toEqual({ | |
| frontmatter: { | |
| user_id: "user-1", | |
| created_at: "2024-01-01T00:00:00Z", | |
| name: "Acme Corp", | |
| pinned: false, | |
| pin_order: 0, | |
| }, | |
| body: "", | |
| }); | |
Was this helpful? React with 👍 or 👎 to provide feedback.
| expect(result).toEqual({ | ||
| frontmatter: { | ||
| user_id: "user-1", | ||
| created_at: "", | ||
| name: "Acme Corp", | ||
| pinned: true, | ||
| }, | ||
| body: "", | ||
| }); |
There was a problem hiding this comment.
🟡 Test assertion missing pin_order: 0 for organizationToFrontmatter (pinned case)
Same root cause as the unpinned case: organizationToFrontmatter always outputs pin_order: org.pin_order ?? 0. The pinned test input at line 76 omits pin_order, so the output has pin_order: 0, but the expected object at lines 80-87 does not include it.
Root Cause
The test input is:
{ user_id: "user-1", name: "Acme Corp", pinned: true }Since pin_order is not provided, org.pin_order is undefined, and undefined ?? 0 evaluates to 0. The actual frontmatter output contains pin_order: 0, but the expected output at line 80-87 omits it, causing toEqual to fail.
Impact: This test will fail, blocking CI.
(Refers to lines 79-88)
Was this helpful? React with 👍 or 👎 to provide feedback.
| ...base, | ||
| type: "contacts", | ||
| state: tab.state ?? { | ||
| selectedOrganization: null, | ||
| selectedPerson: null, | ||
| selected: null, | ||
| }, |
There was a problem hiding this comment.
🚩 Tab state migration: persisted old ContactsState shape may cause issues on app load
The ContactsState type changed from { selectedOrganization: string | null, selectedPerson: string | null } to { selected: ContactsSelection | null }. Pinned tabs are serialized/deserialized via apps/desktop/src/store/zustand/tabs/pinned-persistence.ts. If a user has pinned contacts tabs persisted with the old shape, restorePinnedTabsToStore will restore them with the old state structure. The getDefaultState at apps/desktop/src/store/zustand/tabs/schema.ts:132-136 provides a fallback { selected: null } when tab.state is undefined, but if the old serialized state is present, it will be used as-is with the wrong shape. Accessing tab.state.selected would then be undefined (not null), which might work due to optional chaining in most places but could cause subtle differences.
(Refers to lines 131-136)
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Refactors the contacts system with a unified selection model and adds pin/ordering support for organizations.
Core changes:
{ selectedOrganization, selectedPerson }with a tagged unionContactsSelection({ type: "person" | "organization", id })created_at,pinned,pin_orderfields to organization schema (zod + tinybase)created_atto human schemacontacts-list.tsxcomponent with combined pinned list (people + orgs), drag-to-reorder, and sort optionsBug fixes (latest commit):
<button>elements inPersonItemandOrganizationItemcomponents (bothcontacts-list.tsxandorganizations.tsx) — outer buttons replaced with<div role="button">pin_orderto organization and human frontmatter transforms (bothtoFrontmatterandfromFrontmatterdirections) so pin order persists across sessionsReview & Testing Checklist for Human
ContactsStateshape changed from{ selectedOrganization, selectedPerson }to{ selected: ContactsSelection | null }. Verify that existing persisted tab states don't cause runtime errors on app load.<button>→<div role="button">change in three components should still support Enter/Space activation and tab focus. Manually test keyboard navigation through the contacts list.pin_orderdefaults to0inorganizationToFrontmatter/humanToFrontmatterfor unpinned items. After a save+load cycle, unpinned items will havepin_order: 0instead ofundefined. Confirm this doesn't affect sort behavior (pinned items are filtered bypinnedboolean, notpin_order).Suggested test plan: Open the contacts tab, create a few people and organizations, pin some of each, reorder the pinned list via drag, close and reopen the app, and verify the order and selection state are preserved.
Notes
dprint fmtrustfmt errors are pre-existing (missing rustfmt component for the Rust toolchain) and unrelated to this PR.created_atbut don't explicitly assertpin_orderround-trip — may want to add those.Link to Devin run: https://app.devin.ai/sessions/8172945c219e4a7689550c510b911cd7
Requested by: @ComputelessComputer