refactor(app): CMP-59 harden CRM UI foundations - #61
Open
ripgrim wants to merge 1 commit into
Open
Conversation
This was referenced Aug 6, 2026
ripgrim
force-pushed
the
rg/agent-builder-stack-03-foundations
branch
from
August 6, 2026 21:18
bd05498 to
def9723
Compare
ripgrim
force-pushed
the
rg/agent-builder-stack-03-foundations
branch
from
August 6, 2026 21:24
def9723 to
54eb111
Compare
ripgrim
marked this pull request as ready for review
August 6, 2026 21:24
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
13 issues found across 73 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/app/(landing)/onboarding/research/research-form.tsx">
<violation number="1" location="apps/app/app/(landing)/onboarding/research/research-form.tsx:37">
P2: The apiKey field is now cleared right after you press Continue. React 19 auto-resets uncontrolled form fields once a form `action` runs, unlike the previous `onSubmit` handler which kept the DOM input intact. Because `save.mutate` returns void and the action returns non-promise, the reset happens immediately while the mutation is still pending, so the pasted key is wiped from the screen and must be re-entered if the call fails or is slow. The rest of this codebase keeps the `onSubmit` + `event.preventDefault()` pattern (e.g. onboarding-form.tsx, settings/research-key.tsx); consider staying with that, or explicitly restoring the input value.</violation>
</file>
<file name="apps/app/components/local-date-time.tsx">
<violation number="1" location="apps/app/components/local-date-time.tsx:32">
P2: Calendar/date-only values are displayed one day early for users west of UTC because `new Date(date)` interprets midnight-UTC ISO values as an instant; use `LocalDay` or calendar-date parsing for date-only fields instead of routing them through `LocalDateTime`.</violation>
<violation number="2" location="apps/app/components/local-date-time.tsx:82">
P2: Each rendered date component injects its own inline `<script>` element into the body, and since these are used per-row in the deals/contacts/companies tables, a page with many rows balloons into many small inline scripts (each running eager `getElementById` + `Intl` work at parse time). This bloats the HTML stream and adds repetitive client work on initial load. Consider consolidating the per-instance scripts into a single shared hydration script (e.g. one script that scans all `time[data-local-date]` nodes by `data-*` attributes) while keeping the deterministic SSR output.</violation>
<violation number="3" location="apps/app/components/local-date-time.tsx:135">
P2: The `last reply` date can be labeled `today` for a reply that occurred yesterday locally because the day value is based on rounded elapsed milliseconds; compare the local calendar-date components (and mirror that logic in `relativeDateScript`) instead.</violation>
<violation number="4" location="apps/app/components/local-date-time.tsx:153">
P3: Relative timestamps can show `60m ago` or `24h ago`, making the displayed unit misleading at its upper boundary; flooring the value or promoting rounded values at the boundary keeps the label consistent.</violation>
</file>
<file name="apps/app/lib/use-hydrated.ts">
<violation number="1" location="apps/app/lib/use-hydrated.ts:5">
P3: This new hook is currently dead code: nothing in the app imports or calls `useHydrated`. Removing it until its first consumer, or wiring it into the intended hydration-dependent UI, would avoid shipping an unused foundation API.</violation>
</file>
<file name="packages/ui/src/lib/format.ts">
<violation number="1" location="packages/ui/src/lib/format.ts:51">
P2: Non-US users now see mixed locale conventions in the dashboard: compact amounts use `en-US` while chart values use the browser locale through `formatMoney`. Keeping this helper locale-aware, or applying one explicit locale consistently to both money formatters, avoids the regression.</violation>
</file>
<file name="apps/app/components/crm/timeline/timeline.tsx">
<violation number="1" location="apps/app/components/crm/timeline/timeline.tsx:95">
P2: The timeline's section headers now group entries and resolve "Today"/"Yesterday" using the UTC calendar day instead of the viewer's local calendar day. Because the PR's stated goal is deterministic local presentation, entries created during a user's local morning can land under yesterday's header (or vice-versa around midnight) for anyone not in UTC, e.g. a 1am local activity on Mar 5 becomes UTC Mar 4 and is labeled Yesterday even though it happened today locally. This only shows as a heading inconsistency (the entry times themselves render locally via LocalDateTime), but since the rest of the surface is local, the headers now disagree with what the user sees. Consider computing the grouping/labels from the local date (e.g. deriving the day from the same local view the timestamps use) so the headers match the user's calendar day.</violation>
</file>
<file name="apps/app/components/data-table/list-search.tsx">
<violation number="1" location="apps/app/components/data-table/list-search.tsx:18">
P2: The new `key={q}` remounts the whole search input every time the committed query changes. Because the debounce fires 250ms after the last keystroke, pausing to read results or correct a typo causes the input to unmount/remount and lose DOM focus; the next keystroke then goes nowhere until the user clicks back into the field. Previously the hook synced the value via `useEffect(() => setValue(committed), [committed])` without remounting, so focus was preserved. Consider syncing the input to `q` without a `key` remount (or otherwise restoring focus after commit) so typing a refined query remains uninterrupted.</violation>
</file>
<file name="apps/app/app/(landing)/onboarding/onboarding-form.tsx">
<violation number="1" location="apps/app/app/(landing)/onboarding/onboarding-form.tsx:92">
P2: The slug field sanitizes every keystroke through `workspaceSlug`, which strips leading/trailing dashes and collapses separators. Because the dash is removed the moment it is typed as the trailing character, a user can never enter an internal dash into the Workspace URL field by typing. For example, with the field at "company", hitting `-` then `u` yields "companyu" instead of "company-usa"; typing a lone `-` into an empty field even collapses to the fallback "workspace" (DEFAULT_WORKSPACE_SLUG). Manual slug edits that contain dashes are therefore impossible to produce from typing — only the auto-generated value (which already contains dashes from the company name) works. Consider sanitizing on blur/submit instead of on every keystroke, or preserving dashes during typing.</violation>
</file>
<file name="packages/ui/src/components/chart.tsx">
<violation number="1" location="packages/ui/src/components/chart.tsx:193">
P3: The tooltip and legend row keys were switched from the always-unique `key={index}` to a content-derived key built from `dataKey + type`. This is more deterministic, but it is no longer guaranteed unique: if two series in the same chart share the same `dataKey`/`type` (e.g. stacked sub-series or multiple series reading the same field, or a fallback to the literal "value" when `dataKey` is undefined), you'll get duplicate React keys and reconciliation/dedup warnings. Consider deriving a key that keeps per-row uniqueness (e.g. append the row index as a tiebreaker) so the determinism benefit doesn't reintroduce the duplicate-key risk.</violation>
</file>
<file name="apps/app/components/app-header.tsx">
<violation number="1" location="apps/app/components/app-header.tsx:33">
P3: This newly extracted module-level `signOutAndRedirect` is byte-for-byte identical to the same-named function already defined in `apps/app/app/(landing)/grant-access/grant-access.tsx` (also `const { error } = await signOut(); ... toast.error(...); ... window.location.assign("/sign-in")`). Since this PR is explicitly about hardening shared CRM foundations and consolidating component logic, hoisting an identical copy here instead of extracting one shared helper leaves the sign-out flow duplicated across two files, so the two implementations can drift apart when the flow changes (e.g. added post-sign-out cleanup or redirect logic). Consider extracting it into a shared lib module (e.g. `@/lib/sign-out`) and importing it from both `AppHeader` and `GrantAccess`.</violation>
</file>
<file name="apps/app/lib/trpc/cache.ts">
<violation number="1" location="apps/app/lib/trpc/cache.ts:191">
P2: After a conversation is removed, the agent panel's archive/events query for that conversation won't actually be invalidated. `conversationRemoved` targets the key `conversations.events.queryKey({ id, limit: 5000 })` with `exact: true`, but the archive query is fetched with just `{ id }` (agent-panel.tsx), and the `limit` default of 2000 is applied server-side by the zod schema, not in the client key. Since the two keys differ, the exact-match invalidation silently no-ops and the deleted conversation's cached events can linger. Consider invalidating with the same key the consumer actually uses, e.g. `trpc.conversations.events.queryKey({ id })`.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
|
||
| const form = new FormData(event.currentTarget); | ||
|
|
||
| action={(form) => { |
There was a problem hiding this comment.
P2: The apiKey field is now cleared right after you press Continue. React 19 auto-resets uncontrolled form fields once a form action runs, unlike the previous onSubmit handler which kept the DOM input intact. Because save.mutate returns void and the action returns non-promise, the reset happens immediately while the mutation is still pending, so the pasted key is wiped from the screen and must be re-entered if the call fails or is slow. The rest of this codebase keeps the onSubmit + event.preventDefault() pattern (e.g. onboarding-form.tsx, settings/research-key.tsx); consider staying with that, or explicitly restoring the input value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/app/(landing)/onboarding/research/research-form.tsx, line 37:
<comment>The apiKey field is now cleared right after you press Continue. React 19 auto-resets uncontrolled form fields once a form `action` runs, unlike the previous `onSubmit` handler which kept the DOM input intact. Because `save.mutate` returns void and the action returns non-promise, the reset happens immediately while the mutation is still pending, so the pasted key is wiped from the screen and must be re-entered if the call fails or is slow. The rest of this codebase keeps the `onSubmit` + `event.preventDefault()` pattern (e.g. onboarding-form.tsx, settings/research-key.tsx); consider staying with that, or explicitly restoring the input value.</comment>
<file context>
@@ -34,11 +34,7 @@ export function ResearchForm() {
-
- const form = new FormData(event.currentTarget);
-
+ action={(form) => {
save.mutate({ apiKey: String(form.get("apiKey") ?? "").trim() });
}}
</file context>
| } | ||
|
|
||
| function formatRelativeDate(date: string): string { | ||
| const days = Math.round((Date.now() - new Date(date).getTime()) / DAY_MS); |
There was a problem hiding this comment.
P2: The last reply date can be labeled today for a reply that occurred yesterday locally because the day value is based on rounded elapsed milliseconds; compare the local calendar-date components (and mirror that logic in relativeDateScript) instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/components/local-date-time.tsx, line 135:
<comment>The `last reply` date can be labeled `today` for a reply that occurred yesterday locally because the day value is based on rounded elapsed milliseconds; compare the local calendar-date components (and mirror that logic in `relativeDateScript`) instead.</comment>
<file context>
@@ -0,0 +1,180 @@
+}
+
+function formatRelativeDate(date: string): string {
+ const days = Math.round((Date.now() - new Date(date).getTime()) / DAY_MS);
+ return relativeDateFormatter.format(-days, "day");
+}
</file context>
| return ( | ||
| <> | ||
| <time id={id} dateTime={date} suppressHydrationWarning> | ||
| {formatter.format(new Date(date))} |
There was a problem hiding this comment.
P2: Calendar/date-only values are displayed one day early for users west of UTC because new Date(date) interprets midnight-UTC ISO values as an instant; use LocalDay or calendar-date parsing for date-only fields instead of routing them through LocalDateTime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/components/local-date-time.tsx, line 32:
<comment>Calendar/date-only values are displayed one day early for users west of UTC because `new Date(date)` interprets midnight-UTC ISO values as an instant; use `LocalDay` or calendar-date parsing for date-only fields instead of routing them through `LocalDateTime`.</comment>
<file context>
@@ -0,0 +1,180 @@
+ return (
+ <>
+ <time id={id} dateTime={date} suppressHydrationWarning>
+ {formatter.format(new Date(date))}
+ </time>
+ <InlineScript html={dateScript(id, date, options)} />
</file context>
|
|
||
| export function formatMoneyCompact(cents: number, currency = "usd"): string { | ||
| return new Intl.NumberFormat(undefined, { | ||
| return new Intl.NumberFormat("en-US", { |
There was a problem hiding this comment.
P2: Non-US users now see mixed locale conventions in the dashboard: compact amounts use en-US while chart values use the browser locale through formatMoney. Keeping this helper locale-aware, or applying one explicit locale consistently to both money formatters, avoids the regression.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui/src/lib/format.ts, line 51:
<comment>Non-US users now see mixed locale conventions in the dashboard: compact amounts use `en-US` while chart values use the browser locale through `formatMoney`. Keeping this helper locale-aware, or applying one explicit locale consistently to both money formatters, avoids the regression.</comment>
<file context>
@@ -44,7 +48,7 @@ export function formatMoney(cents: number, currency = "usd"): string {
export function formatMoneyCompact(cents: number, currency = "usd"): string {
- return new Intl.NumberFormat(undefined, {
+ return new Intl.NumberFormat("en-US", {
style: "currency",
currency: displayCurrencyCode(currency),
</file context>
Suggested change
| return new Intl.NumberFormat("en-US", { | |
| return new Intl.NumberFormat(undefined, { |
| <time id={id} dateTime={date} suppressHydrationWarning> | ||
| {formatRelativeTime(date)} | ||
| </time> | ||
| <InlineScript html={relativeTimeScript(id, date)} /> |
There was a problem hiding this comment.
P2: Each rendered date component injects its own inline <script> element into the body, and since these are used per-row in the deals/contacts/companies tables, a page with many rows balloons into many small inline scripts (each running eager getElementById + Intl work at parse time). This bloats the HTML stream and adds repetitive client work on initial load. Consider consolidating the per-instance scripts into a single shared hydration script (e.g. one script that scans all time[data-local-date] nodes by data-* attributes) while keeping the deterministic SSR output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/components/local-date-time.tsx, line 82:
<comment>Each rendered date component injects its own inline `<script>` element into the body, and since these are used per-row in the deals/contacts/companies tables, a page with many rows balloons into many small inline scripts (each running eager `getElementById` + `Intl` work at parse time). This bloats the HTML stream and adds repetitive client work on initial load. Consider consolidating the per-instance scripts into a single shared hydration script (e.g. one script that scans all `time[data-local-date]` nodes by `data-*` attributes) while keeping the deterministic SSR output.</comment>
<file context>
@@ -0,0 +1,180 @@
+ <time id={id} dateTime={date} suppressHydrationWarning>
+ {formatRelativeTime(date)}
+ </time>
+ <InlineScript html={relativeTimeScript(id, date)} />
+ </>
+ );
</file context>
| conversationRemoved: (id) => { | ||
| for (const queryKey of [ | ||
| trpc.conversations.builderById.queryKey({ id }), | ||
| trpc.conversations.events.queryKey({ id, limit: 5000 }), |
There was a problem hiding this comment.
P2: After a conversation is removed, the agent panel's archive/events query for that conversation won't actually be invalidated. conversationRemoved targets the key conversations.events.queryKey({ id, limit: 5000 }) with exact: true, but the archive query is fetched with just { id } (agent-panel.tsx), and the limit default of 2000 is applied server-side by the zod schema, not in the client key. Since the two keys differ, the exact-match invalidation silently no-ops and the deleted conversation's cached events can linger. Consider invalidating with the same key the consumer actually uses, e.g. trpc.conversations.events.queryKey({ id }).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/lib/trpc/cache.ts, line 191:
<comment>After a conversation is removed, the agent panel's archive/events query for that conversation won't actually be invalidated. `conversationRemoved` targets the key `conversations.events.queryKey({ id, limit: 5000 })` with `exact: true`, but the archive query is fetched with just `{ id }` (agent-panel.tsx), and the `limit` default of 2000 is applied server-side by the zod schema, not in the client key. Since the two keys differ, the exact-match invalidation silently no-ops and the deleted conversation's cached events can linger. Consider invalidating with the same key the consumer actually uses, e.g. `trpc.conversations.events.queryKey({ id })`.</comment>
<file context>
@@ -184,6 +185,22 @@ export function useCrmCache(): CrmCache {
+ conversationRemoved: (id) => {
+ for (const queryKey of [
+ trpc.conversations.builderById.queryKey({ id }),
+ trpc.conversations.events.queryKey({ id, limit: 5000 }),
+ trpc.conversations.shareStatus.queryKey({ id }),
+ ]) {
</file context>
|
|
||
| const distance = | ||
| absolute < HOUR_MS | ||
| ? `${Math.round(absolute / MINUTE_MS)}m` |
There was a problem hiding this comment.
P3: Relative timestamps can show 60m ago or 24h ago, making the displayed unit misleading at its upper boundary; flooring the value or promoting rounded values at the boundary keeps the label consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/components/local-date-time.tsx, line 153:
<comment>Relative timestamps can show `60m ago` or `24h ago`, making the displayed unit misleading at its upper boundary; flooring the value or promoting rounded values at the boundary keeps the label consistent.</comment>
<file context>
@@ -0,0 +1,180 @@
+
+ const distance =
+ absolute < HOUR_MS
+ ? `${Math.round(absolute / MINUTE_MS)}m`
+ : absolute < DAY_MS
+ ? `${Math.round(absolute / HOUR_MS)}h`
</file context>
|
|
||
| import { useSyncExternalStore } from "react"; | ||
|
|
||
| export function useHydrated() { |
There was a problem hiding this comment.
P3: This new hook is currently dead code: nothing in the app imports or calls useHydrated. Removing it until its first consumer, or wiring it into the intended hydration-dependent UI, would avoid shipping an unused foundation API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/lib/use-hydrated.ts, line 5:
<comment>This new hook is currently dead code: nothing in the app imports or calls `useHydrated`. Removing it until its first consumer, or wiring it into the intended hydration-dependent UI, would avoid shipping an unused foundation API.</comment>
<file context>
@@ -0,0 +1,19 @@
+
+import { useSyncExternalStore } from "react";
+
+export function useHydrated() {
+ return useSyncExternalStore(subscribe, clientSnapshot, serverSnapshot);
+}
</file context>
| const key = `${nameKey ?? item.name ?? item.dataKey ?? "value"}`; | ||
| const itemConfig = getPayloadConfigFromPayload(config, item, key); | ||
| const indicatorColor = color ?? item.payload?.fill ?? item.color; | ||
| const itemKey = `${String(item.dataKey ?? item.name ?? "value")}:${String(item.type ?? "default")}`; |
There was a problem hiding this comment.
P3: The tooltip and legend row keys were switched from the always-unique key={index} to a content-derived key built from dataKey + type. This is more deterministic, but it is no longer guaranteed unique: if two series in the same chart share the same dataKey/type (e.g. stacked sub-series or multiple series reading the same field, or a fallback to the literal "value" when dataKey is undefined), you'll get duplicate React keys and reconciliation/dedup warnings. Consider deriving a key that keeps per-row uniqueness (e.g. append the row index as a tiebreaker) so the determinism benefit doesn't reintroduce the duplicate-key risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui/src/components/chart.tsx, line 193:
<comment>The tooltip and legend row keys were switched from the always-unique `key={index}` to a content-derived key built from `dataKey + type`. This is more deterministic, but it is no longer guaranteed unique: if two series in the same chart share the same `dataKey`/`type` (e.g. stacked sub-series or multiple series reading the same field, or a fallback to the literal "value" when `dataKey` is undefined), you'll get duplicate React keys and reconciliation/dedup warnings. Consider deriving a key that keeps per-row uniqueness (e.g. append the row index as a tiebreaker) so the determinism benefit doesn't reintroduce the duplicate-key risk.</comment>
<file context>
@@ -197,16 +185,16 @@ function ChartTooltipContent({
const key = `${nameKey ?? item.name ?? item.dataKey ?? "value"}`;
const itemConfig = getPayloadConfigFromPayload(config, item, key);
const indicatorColor = color ?? item.payload?.fill ?? item.color;
+ const itemKey = `${String(item.dataKey ?? item.name ?? "value")}:${String(item.type ?? "default")}`;
return (
</file context>
| */ | ||
| export function workspaceLabel(name: string | undefined): string { | ||
| const trimmed = name?.trim(); | ||
| async function signOutAndRedirect() { |
There was a problem hiding this comment.
P3: This newly extracted module-level signOutAndRedirect is byte-for-byte identical to the same-named function already defined in apps/app/app/(landing)/grant-access/grant-access.tsx (also const { error } = await signOut(); ... toast.error(...); ... window.location.assign("/sign-in")). Since this PR is explicitly about hardening shared CRM foundations and consolidating component logic, hoisting an identical copy here instead of extracting one shared helper leaves the sign-out flow duplicated across two files, so the two implementations can drift apart when the flow changes (e.g. added post-sign-out cleanup or redirect logic). Consider extracting it into a shared lib module (e.g. @/lib/sign-out) and importing it from both AppHeader and GrantAccess.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/components/app-header.tsx, line 33:
<comment>This newly extracted module-level `signOutAndRedirect` is byte-for-byte identical to the same-named function already defined in `apps/app/app/(landing)/grant-access/grant-access.tsx` (also `const { error } = await signOut(); ... toast.error(...); ... window.location.assign("/sign-in")`). Since this PR is explicitly about hardening shared CRM foundations and consolidating component logic, hoisting an identical copy here instead of extracting one shared helper leaves the sign-out flow duplicated across two files, so the two implementations can drift apart when the flow changes (e.g. added post-sign-out cleanup or redirect logic). Consider extracting it into a shared lib module (e.g. `@/lib/sign-out`) and importing it from both `AppHeader` and `GrantAccess`.</comment>
<file context>
@@ -26,21 +26,19 @@ import { toast } from "sonner";
- */
-export function workspaceLabel(name: string | undefined): string {
- const trimmed = name?.trim();
+async function signOutAndRedirect() {
+ const { error } = await signOut();
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens shared CRM presentation foundations: derived UI state, deterministic local dates, reusable page shells, and shared data-table and component cleanup.
Why
The builder routes build on the same application shell and primitives. This prerequisite keeps the feature layer focused and independently type-safe.
Stack
Depends on #60 — sandboxed builder and runner runtimes.
Verification