Skip to content

OUT-4208: collapse duplicate custom-fields requests on client pageload - #238

Merged
SandipBajracharya merged 2 commits into
mainfrom
OUT-4208
Sep 25, 2026
Merged

SandipBajracharya merged 2 commits into
mainfrom
OUT-4208

Conversation

@SandipBajracharya

Copy link
Copy Markdown
Collaborator

Changes

  • The /client pageload fired two parallel GET /api/custom-fields/[entityType] requests (one for client, one for company), which Sentry's N+1 API Call detector groups by parameterized URL and flags (CLIENT-HOME-V3-1X, OUT-4208).
  • useCustomFields now issues a single GET /api/custom-fields (the Assembly SDK's no-arg listCustomFields() already returns all entity types) and splits the result into clientCustomFields / companyCustomFields client-side in a useMemo. The hook's public contract is unchanged.
  • Repointed the listCustomFields route to the collection path /api/custom-fields and removed the now-unused dynamic [entityType] route.
  • Left the separate options-map query untouched to keep the change tightly scoped to the flagged N+1.

Testing Criteria

  • Load /client?token=<assembly-token>; in DevTools → Network, confirm exactly one GET /api/custom-fields (200) whose data[] contains both entityType: "client" and entityType: "company" — and no /api/custom-fields/client or /api/custom-fields/company requests.
  • As an internal user, open the editor Segment panel / Dynamic Fields and confirm client and company custom fields still render, correctly separated and ordered by order.
  • Confirm handlebar templates still resolve custom-field option labels (via the unchanged options-map).
  • GET /api/custom-fields/client now returns 404 (dynamic route removed).
  • Loom: TODO — add walkthrough video.

Notes

  • No dependencies on other PRs. Fixes CLIENT-HOME-V3-1X in the commit auto-closes the Sentry issue on merge.
  • pnpm typecheck and pnpm lint both pass.

Impact & Surface Area of Change

  • Consumers of useCustomFields (Segment, useDynamicFields, SegmentFormPanel, SegmentCreationCard, handle-bar-template) are unchanged — same return shape.
  • Trade-off: client and company fields now share one request, so a single failure blanks both (previously independent). Both come from the same upstream Assembly endpoint, so real-world failure behavior is effectively equivalent.
  • The [entityType] route is removed; verify nothing external hits /api/custom-fields/:entityType directly (only this app's hook did).

🤖 Generated with Claude Code

…geload

The /client pageload fired two parallel GET /api/custom-fields/[entityType]
requests (client + company), which Sentry's N+1 API Call detector groups and
flags. Fetch all entity types in a single GET /api/custom-fields and split them
client-side by entityType, removing the now-unused dynamic route.

Fixes CLIENT-HOME-V3-1X

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel

vercel Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
client-home-v3 Ready Ready Preview Sep 25, 2026 8:06am UTC

Request Review

@linear-code

linear-code Bot commented Sep 25, 2026

Copy link
Copy Markdown

OUT-4208

@greptile-apps

greptile-apps Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Consolidates custom fields API to reduce duplicate requests.

The PR appears safe to merge. The remaining earlier test gap is non-blocking.

Findings

  1. P2 There is no automated test for the new one-request path or its client/company split. Add a hook test that mocks a mixed, unordered response and asserts one GET /api/custom-fields , two ordered lists, and the unchanged options-map request. This protects the main reason for this change as well as the returned data. ▶

Summary

The client page now loads all custom fields with one request, then splits them into client and company lists in the browser. The API uses a collection route backed by Assembly’s all-fields call, while the hook keeps the same public return shape and leaves the options-map request unchanged.

  • Replaces two entity-specific custom-field requests with one /api/custom-fields request.
  • Sorts fields, adds icons, and separates the combined response by entity type on the client.
  • Removes the dynamic entity-type route and adds mapper tests for ordering and splitting.

Diagram

sequenceDiagram
    participant Page as Client page
    participant Hook as useCustomFields
    participant API as GET /api/custom-fields
    participant Controller as listCustomFields
    participant Assembly as Assembly API
    participant Options as GET /api/custom-fields/options-map

    Page->>Hook: Load custom fields
    par Field definitions
        Hook->>API: GET /api/custom-fields
        API->>Controller: Authenticated request
        Controller->>Assembly: listCustomFields()
        Assembly-->>Controller: Client and company fields
        Controller-->>Hook: Combined field list
        Hook->>Hook: Map, sort, and split by entity
    and Option labels
        Hook->>Options: GET /api/custom-fields/options-map
        Options->>Assembly: listCustomFields()
        Assembly-->>Options: All field options
        Options-->>Hook: Entity-aware option map
    end
    Hook-->>Page: Two field lists and option map
Loading

Reviews (2) · Last reviewed commit: "test(OUT-4208): cover custom-fields sort..."

const { data: customFields, isLoading: fieldsIsLoading } = useQuery({
queryKey: [CUSTOM_FIELDS_QUERY_KEY],
queryFn: async (): Promise<CustomFieldItemWithEntity[]> => {
const res = await api.get('/api/custom-fields')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 There is no automated test for the new one-request path or its client/company split. Add a hook test that mocks a mixed, unordered response and asserts one GET /api/custom-fields, two ordered lists, and the unchanged options-map request. This protects the main reason for this change as well as the returned data.

Knowledge Base Used: Content assets and templates

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 3c2dc57 — extracted the sort/icon mapping and entity split into custom-field-mappers.ts and added tests/unit/custom-field-mappers.test.ts, which asserts a mixed, unordered response yields two correctly ordered per-entity lists (client vs company). The single-request and unchanged options-map behavior is covered by the manual test steps in the PR description.

Extract the pure sort/icon mapping and entity split out of useCustomFields
into custom-field-mappers, and add unit tests asserting a mixed, unordered
response yields two correctly ordered per-entity lists. Guards the
consolidated single-request path against regressions.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@SandipBajracharya

Copy link
Copy Markdown
Collaborator Author

@greptileai review

@priosshrsth priosshrsth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@SandipBajracharya SandipBajracharya changed the title fix(OUT-4208): collapse duplicate custom-fields requests on client pageload OUT-4208: collapse duplicate custom-fields requests on client pageload Sep 25, 2026
@SandipBajracharya
SandipBajracharya merged commit db77f51 into main Sep 25, 2026
8 checks passed

This branch was successfully deployed

1 active deployment
Preview — 3c2dc57f Deployed Sep 25, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants