fix(crm): keep side panel task and deal changes from being reverted by dulsara-manakal · Pull Request #1955 · SkappHQ/skapp

dulsara-manakal · 2026-09-17T17:56:29Z

Metrics refetch on every task and deal mutation, and the side panels wrote metrics in the same effect as the task and deal lists, so each refetch replayed the stale list responses over the change the user had just made.

write metrics in their own effect and keep the entity details, task ids and deal ids in a second one, so a metrics refetch no longer rewrites the lists
preselect the contact in the add task modal and link the created task to its contact, company and deal so it shows up in the side panel right away
show the task form errors only once a field is touched, and revalidate when a value is picked so the error clears
mount the v2 task modal controller on the companies and deals pages so the add task button works there and no longer leaves the modal open for the next page
drop the translateText prop that TaskModalForm never declared

PR checklist

TaskId: (https://github.com/SkappHQ/skapp/issues/[id])

Summary

How to test

Project Checklist

Other

New atomic components added
New molecules added
New pages(routes) added
New dependencies installed

PR Checklist

Pull request is raised from the correct source branch
Pull request is raised to the correct destination branch
Pull request is raised with correct title
Pull request is self reviewed
Pull request is self assigned
Suitable pull request status labels are added (ready-for-code-review)

Additional Information

Metrics refetch on every task and deal mutation, and the side panels wrote metrics in the same effect as the task and deal lists, so each refetch replayed the stale list responses over the change the user had just made. - write metrics in their own effect and keep the entity details, task ids and deal ids in a second one, so a metrics refetch no longer rewrites the lists - preselect the contact in the add task modal and link the created task to its contact, company and deal so it shows up in the side panel right away - show the task form errors only once a field is touched, and revalidate when a value is picked so the error clears - mount the v2 task modal controller on the companies and deals pages so the add task button works there and no longer leaves the modal open for the next page - drop the translateText prop that TaskModalForm never declared

CLAassistant · 2026-09-17T17:56:36Z

All committers have signed the CLA.

…2/F-all-in-one

Read the selected contact through getSelectedContact so its optional id assigns straight into the initial values, keeping them a single object literal.

…sks, and Configurations

sonarqubecloud · 2026-09-18T04:48:20Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ThinuwanW

🤖 Claude Code Review

This PR flips five CRM v2 feature flags to production and adjusts the task modal (contact prefill, entity linking after create, touched-gated validation errors) plus the company/contact side-panel effects. The riskiest items are: required-field errors in the Add Task modal can never render (so Save silently no-ops), the newly split metrics effect in both side panels writes the same store slice from a stale snapshot and can discard the sibling write, and the deals page now renders the v2 task modal controller while the deal side panel still drives the v1 store, leaving its Add Task button dead. Remaining findings are dead code and rollout-safety concerns.

Found 16 new issue(s): 🔴 6 important, 🟡 6 suggestion(s), 🟣 4 nit(s)

ThinuwanW · 2026-09-20T07:24:34Z

              placeholder={translateText(["placeholders", "type"])}
-              errorMessage={errors.typeId}
-              variant={errors.typeId ? "primary-error" : "primary"}
+              errorMessage={touched.typeId ? errors.typeId : undefined}


🤖 Claude · 🔴 Important

Gating error display on touched breaks required-field feedback in the Add Task modal. typeId and dueAt are only settable through the Dropdown/DatePicker handlers, which call setFieldValue (that does not mark a field touched), and neither control wires onBlur (the due-date InputField at line 146 has no onBlur={handleBlur} at all). Formik's submit path sets touched via setNestedObjectValues(values, true), which only produces keys that already exist in values — and AddTaskModalContent's initialValues no longer declare typeId/dueAt (they were removed in this PR). Result: user types a name, clicks Save, Yup rejects on typeId/dueAt required, but touched.typeId/touched.dueAt stay undefined so no error is shown and the button appears to do nothing. Fix either side: restore typeId: undefined, dueAt: undefined (and dealId) in AddTaskModalContent initialValues so submit marks them touched, and/or set touched explicitly in the handlers (setFieldTouched("typeId", true, false) alongside setFieldValue). Note ownerId is required by the schema too and has no error surface anywhere in this form — worth handling in the same pass.

ThinuwanW · 2026-09-20T07:24:35Z

  }, [isCompanyError, isCompanyFetching, isMetricsError, isMetricsFetching]);

+  useEffect(() => {
+    if (fetchedMetrics) {


🤖 Claude · 🔴 Important

The new metrics-only effect and the existing data effect both call setCompanies(updateCompany(companies, ...)) using the same companies value captured from the render that scheduled them. When fetchedMetrics and any of fetchedCompany/fetchedTasks/fetchedDeals/fetchedContacts settle in the same commit (very likely on a warm React Query cache, e.g. reopening a panel, and possible whenever two responses land in the same tick), both effects run back-to-back off the same snapshot: the metrics write lands first, then the second effect overwrites the entry from the pre-metrics snapshot and drops metrics. Because the metrics effect's deps ([companyId, fetchedMetrics]) do not change again, it never re-runs and the metric cards fall back to ?? 0/blank until the next refetch. The same pattern was introduced in ContactSidePanel.tsx (lines 167-174 vs. the effect ending at line 197), where both effects write the contacts slice. Fix by reading fresh state inside the effects (useCrmStoreV2.getState().companies / .contacts) or by adding a store action that merges a single entry, so the two writes cannot clobber each other.

ThinuwanW · 2026-09-20T07:24:36Z

          {selectedDealId !== null && <DealSidePanelV2 />}
          <AddDealSidePanelV2 />
        </SidePanelWrapper>
+        <TaskModalControllerV2 />


🤖 Claude · 🔴 Important

TaskModalControllerV2 reads isTaskModalOpen/taskModalType from useCrmStoreV2, but the deal side panel rendered on this page (DealSidePanelV2 → DealDetailContent) imports the v1 ~community/crm/components/molecules/SidePanelTasksSection, whose handleAddTask writes to the v1 useCrmStore. Since isCrmDealsV2 is now true, the v1 TaskModalController is no longer mounted, so clicking "Add task" inside the deal side panel sets a flag nothing listens to and the modal never opens. Switch DealDetailContent to the v2 ~community/crm/v2/components/molecules/SidePanelTasksSection (passing taskIds from the v2 store instead of the v1 useGetRelatedTasks result), or the deals page will ship a dead button.

ThinuwanW · 2026-09-20T07:24:38Z

      priority: values.priority,
      dueAt: values.dueAt,
      ownerId: values.ownerId,
+      companyId: values.companyId,


🤖 Claude · 🟡 Suggestion

companyId: values.companyId is always undefined: the task form has no company field and nothing calls setFieldValue("companyId", ...) (TaskContactField/TaskDealField only use selectedCompanyId to scope their lookups). The backend create contract has no company field either — CrmTaskCreateRequestDto only accepts name/typeId/priority/dueAt/notes/ownerId/contactId/dealId, and persistNewTask derives the company from the selected contact. So this line is dead payload that suggests a capability the API does not have; drop it. The related functional gap: opening Add Task from the Company side panel prefills nothing (unlike the contact case at line 95, which uses selectedContactId), so a task created there without picking a contact comes back with no companyId, linkTaskToRelatedEntities links nothing, and the task never appears in that company's task list. Consider prefilling contactId/dealId from the panel context, or scoping the company link explicitly once the API supports it.

ThinuwanW · 2026-09-20T07:24:40Z


-    if (Object.keys(companyFields).length === 0) return;
-
    setCompanies(updateCompany(companies, companyId, companyFields));


🤖 Claude · 🟡 Suggestion

Dropping the if (Object.keys(companyFields).length === 0) return; guard means this effect now always writes to the store, including on the first mount when every query is still pending. updateCompany will then create companies[companyId] = {} for an id that is not in the store yet (e.g. a deep link or a company not on the current table page), turning the !company skeleton guard on line 325 into a false negative once isLoading flips, and it costs an extra store write + re-render of every companies subscriber on each mount. Keep a cheap guard (e.g. return early when none of fetchedCompany/fetchedTasks/fetchedDeals/fetchedContacts is defined) while still allowing the intended metrics-independent write.

ThinuwanW · 2026-09-20T07:24:41Z

    tasks,
    taskIds,
    owners,
+    companies,


🤖 Claude · 🟣 Nit

The modal now subscribes to the full companies, contacts and deals records purely so handleSuccess can link the created task. That re-renders the modal on every CRM store write (including the side-panel metrics/task/deal writes happening behind it) and captures snapshots that can be stale by the time the mutation resolves. Reading them at call time inside handleSuccess via useCrmStoreV2.getState() would avoid both the extra subscriptions and the stale-snapshot risk; only the setters need to come from the selector.

ThinuwanW · 2026-09-20T07:24:42Z


 // Flip to true to serve the CRM Companies page from the normalized v2 store surface.
-const isCrmCompaniesV2 = false;
+const isCrmCompaniesV2 = true;


🤖 Claude · 🟡 Suggestion

All five v2 switches are flipped to hardcoded true in this PR (companies, contacts, deals, tasks, configurations). That makes CompaniesV1/ContactsV1/DealsV1/TasksV1 and the v1 CrmConfigurations unreachable dead code that still ships in the bundle, and leaves no runtime kill switch — a rollback needs a code change and redeploy. Given there is no test coverage anywhere under src/community/crm/v2 (no .test.tsx files in that tree), consider driving these from an env flag for this release and deleting the v1 components in a follow-up once the rollout is confirmed.

ThinuwanW · 2026-09-20T07:24:44Z

@@ -104,6 +105,7 @@ const DealsV2 = () => {
          {selectedDealId !== null && <DealSidePanelV2 />}


🤖 Claude · 🟣 Nit

Now that this page serves the v2 surface, it still wraps the panels in the v1 SidePanelWrapper from ~community/crm/components/atoms/... (line 8) while the other v2 pages use SidePanelWrapperV2 from ~community/crm/v2/components/templates/.... The two implementations are currently identical, so there is no behavior difference today, but the v2 page should import the v2 template so the v1 tree can be deleted cleanly later.

ThinuwanW · 2026-09-20T07:24:45Z

      ownerId: defaultOwner?.employeeId,
-      contactId: undefined,
-      dealId: undefined,
+      contactId: selectedContact?.id,


🤖 Claude · 🔴 Important

The contact prefill is derived from the global selectedContactId, but the Add Task modal is now mounted on Companies, Deals and Tasks too (TaskModalControllerV2 added to companies.tsx/deals.tsx). selectedContactId is only written by ContactTable row click and only cleared by closeCrmSidePanel() / DeleteContactModalContent — it is NOT cleared on route change, and the v2 store is a module-level singleton that survives client-side navigation. So: open a contact side panel on /contacts, navigate away via the sidebar without closing the panel, then hit "Add task" on /tasks or from a Company side panel — the new task is silently pre-linked to a completely unrelated contact, and linkTaskToRelatedEntities then writes that task id into that contact's taskIds. getSelectedContact was written for the Edit/Delete Contact modals, which are only reachable while the contact panel is open; this shared modal is not. Fix: scope the prefill to the panel that actually opened the modal. The cheapest fix mirrors the guard TaskContactField already uses for companyScopeId — only prefill when isCrmSidePanelOpen && crmSidePanelType === CrmSidePanelTypes.CONTACT_SIDE_PANEL. The cleaner fix is to have SidePanelTasksSection.handleAddTask push an explicit context object (companyId/contactId/dealId) into the store alongside setTaskModalType(ADD_TASK_MODAL) and have the modal read that, which would also give you the missing companyId/dealId prefills for the company and deal panels.

ThinuwanW · 2026-09-20T07:24:47Z


    if (fetchedTasks) {
-      const taskItems = fetchedTasks.pages.flatMap((page) => page.items ?? []);
+      const taskItems = fetchedTasks.pages.flatMap((page) => page?.items ?? []);


🤖 Claude · 🔴 Important

The page.items → page?.items ?? [] hardening added here (and at 219/226) is correct — fetchTasks/fetchDeals/fetchContacts all return response?.data?.results?.[0], so a page really can be undefined at runtime despite the non-optional return type. But the same hazard was left unguarded in two components this PR just switched on in production: CompanyTable.tsx:70 (data.pages.flatMap((page) => page.items), reached on every /crm/companies load) and DealsSectionV2.tsx:159 (same, on the deals board). Both throw a TypeError on an undefined page, and when only items is missing they produce [undefined], which then blows up in updateCompanyRecord/mergeDeals on company.id. That's a white screen on two of the five newly enabled surfaces. Apply page?.items ?? [] in those two spots as well (DealDetailContent.tsx:83 and TaskSidePanelV2.tsx:114 have the same pattern and are worth the same treatment).

ThinuwanW · 2026-09-20T07:24:49Z

    }
  }, [isContactError, isContactFetching, isMetricsError, isMetricsFetching]);

+  useEffect(() => {


🤖 Claude · 🔴 Important

The metrics-only effect split was applied identically here, and it carries the same defect the first pass raised for CompanySidePanel — worth calling out separately because it needs a second, independent fix in this file. Both this effect and the data effect below read the same contacts value captured in one render and each call setContacts(updateContact(contacts, ...)). When both run in the same commit (the common case: useCreateTask invalidates crmContactQueryKeys.METRICS_ROOT, so fetchedMetrics and fetchedContact/fetchedTasks often settle together), the later effect overwrites the earlier one from its stale snapshot and contact.metrics is dropped, leaving getContactMetricItems rendering zeros until the next unrelated re-render. Use the functional store update (or merge both writes into a single effect) so neither write is based on a stale record, e.g. useCrmStoreV2.setState((s) => ({ contacts: updateContact(s.contacts, contactId, { metrics: fetchedMetrics }) })).

ThinuwanW · 2026-09-20T07:24:50Z

@@ -119,7 +119,6 @@ const EditTaskModalContent: FC<Props> = ({ taskId }) => {
    <TaskModalForm


🤖 Claude · 🟡 Suggestion

Missing paired change: create now re-links the task into the related entities (linkTaskToRelatedEntities in AddTaskModalContent), but update still only calls setTasks(...). Changing a task's contact or deal in the Edit modal leaves the task id in the old entity's taskIds and absent from the new one, so a side panel that's already open keeps showing the task under the wrong contact/deal until its infinite query refetches on remount. Add the symmetric unlink/link (a relinkTaskToRelatedEntities(previousTask, updatedTask, ...) in taskUtil, mirroring linkContactToCompany's previousCompanyId handling) in handleSuccess. Related: getChangedTaskFields in taskUtil has no companyId branch, so even though the create payload now sends companyId, the edit path can never change it — add that branch when the companyId field is actually wired up.

ThinuwanW · 2026-09-20T07:24:51Z

          placeholder={translateText(["placeholders", "notes"])}
-          errorMessage={errors.notes}
-          state={errors.notes ? "error" : "default"}
+          errorMessage={touched.notes ? errors.notes : undefined}


🤖 Claude · 🟡 Suggestion

The touched-gating was applied to all four fields owned by this file (name, typeId, dueAt, notes) but not to the sibling field components it renders. TaskOwnerField.tsx:106-107 still does state={errors.ownerId ? "error" : "default"} / errorMessage={errors.ownerId} with no touched check, and ownerId is required() in the same getTaskValidationSchema. Result is inconsistent behaviour in one form: clear the owner and blur any other field and the owner error appears immediately, while an empty task type or due date stays silent. Either pass touched.ownerId through in TaskOwnerField for consistency, or drop the gating here — but the three fields in this form should behave the same way.

ThinuwanW · 2026-09-20T07:24:53Z

@@ -147,7 +176,6 @@ const AddTaskModalContent: FC = () => {
    <TaskModalForm


🤖 Claude · 🟡 Suggestion

Dropping the translateText prop makes TaskModalForm (and its TaskOwnerField/TaskContactField/TaskDealField children, which already use useTranslator("crmModule", "tasks", "taskModal")) translate exclusively from crmModule.tasks.taskModal. That leaves whole duplicated blocks in crmModule.json unreachable: tasks.addTaskModal.{labels,placeholders,ariaLabels,buttons,taskTypes,priorityOptions,emptyStates} and the identical tasks.editTaskModal.* blocks (only title, validations and the toast keys are still read from those namespaces; priority labels come from crmModule.common.priorityOptions). Please delete the now-dead keys in the same PR — otherwise a translator editing addTaskModal.labels.dueDate will silently see no change in the UI, and the drift compounds once other locales are added.

ThinuwanW · 2026-09-20T07:24:54Z

+        deals
+      );
+
+      setCompanies({ ...companies, ...linked.companies });


🤖 Claude · 🟣 Nit

linkTaskToRelatedEntities already returns full record copies (and returns the same reference when nothing changed — every linkTaskTo* helper short-circuits with if (taskIds === entity.taskIds) return entities). Re-spreading as { ...companies, ...linked.companies } discards that short-circuit: a brand-new object identity is pushed into the store for all three records on every task creation, even when the task has no companyId/dealId at all (which, per the first pass, is currently always the case for companyId). That re-renders every companies/deals subscriber — the company table, the kanban board — for nothing. Just setCompanies(linked.companies); setContacts(linked.contacts); setDeals(linked.deals);. (Same pattern exists in handleDealCreated in both side panels if you want to clean it up consistently.)

ThinuwanW · 2026-09-20T07:24:56Z

    }
  }, [isCompanyError, isCompanyFetching, isMetricsError, isMetricsFetching]);

+  useEffect(() => {


🤖 Claude · 🟣 Nit

📁 File-level observation

Accessibility issue that this PR ships to production on all four CRM pages: SidePanelTasksList renders TaskRow without an onRowClick prop, but TaskRow unconditionally applies role="button", tabIndex={0}, cursor-pointer and aria-label={translateText(["openTaskDetails"], { name })} ("open task details"). So every task row in the Company/Contact/Deal side panels is keyboard-focusable and announced as an actionable button that opens task details, and clicking or pressing Enter does nothing. Either pass a real onRowClick from SidePanelTasksSection, or make the interactive attributes conditional on onRowClick being supplied in TaskRow.

dulsara-manakal requested review from akilarootcode and hasalarootcode September 17, 2026 17:56

dulsara-manakal self-assigned this Sep 17, 2026

dulsara-manakal requested a review from ThinuwanW as a code owner September 17, 2026 17:56

Merge branch 'develop' of https://github.com/SkappHQ/skapp into crm-v…

e73f4c0

…2/F-all-in-one

dulsara-manakal force-pushed the crm-v2/F-all-in-one branch 2 times, most recently from c1062b7 to f4c5577 Compare September 17, 2026 18:08

refactor(crm): take the add task contact id from the selected contact

6ab7d0c

Read the selected contact through getSelectedContact so its optional id assigns straight into the initial values, keeping them a single object literal.

dulsara-manakal force-pushed the crm-v2/F-all-in-one branch from f4c5577 to 6ab7d0c Compare September 17, 2026 18:15

feat(crm): enable v2 store surface for Companies, Contacts, Deals, Ta…

3bb24e7

…sks, and Configurations

akilarootcode approved these changes Sep 18, 2026

View reviewed changes

shakila-kamalasena approved these changes Sep 18, 2026

View reviewed changes

ThinuwanW reviewed Sep 20, 2026

View reviewed changes


		if (Object.keys(companyFields).length === 0) return;

		setCompanies(updateCompany(companies, companyId, companyFields));

		@@ -104,6 +105,7 @@ const DealsV2 = () => {
		{selectedDealId !== null && <DealSidePanelV2 />}

		@@ -119,7 +119,6 @@ const EditTaskModalContent: FC<Props> = ({ taskId }) => {
		<TaskModalForm

		@@ -147,7 +176,6 @@ const AddTaskModalContent: FC = () => {
		<TaskModalForm

Conversation

dulsara-manakal commented Sep 17, 2026

PR checklist

TaskId: (https://github.com/SkappHQ/skapp/issues/[id])

Summary

How to test

Project Checklist

Other

PR Checklist

Additional Information

Uh oh!

CLAassistant commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud Bot commented Sep 18, 2026

Quality Gate passed

Uh oh!

ThinuwanW left a comment

Choose a reason for hiding this comment

🤖 Claude Code Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Sep 17, 2026 •

edited

Loading