Skip to content

feat(crm): legacy /api/crm/* compatibility shim#27

Open
Govindaroraaa wants to merge 1 commit into
hebbs-ai:mainfrom
Govindaroraaa:feat/issue-20-crm-rest-shim
Open

feat(crm): legacy /api/crm/* compatibility shim#27
Govindaroraaa wants to merge 1 commit into
hebbs-ai:mainfrom
Govindaroraaa:feat/issue-20-crm-rest-shim

Conversation

@Govindaroraaa

Copy link
Copy Markdown
Contributor

Summary

The CRM module's settings panels (Pipeline configuration, Inbox, Deal list, etc.) were authored against an older v1 REST surface (/api/crm/pipelines, /api/crm/inbox/…) which no longer exists in the framework — every request 404'd, pages rendered blank.

Rather than rewriting every legacy CRM UI component or shipping a breaking change to the module, this PR adds a framework-side compatibility shim that translates v1 REST paths into v2 tool calls. Closes #20.

Implementation (packages/@boringos/core/src/crm-shim-routes.ts)

  • translate(method, path, query, body) — port of the existing client mapping table from hebbs-crm/packages/web/src/lib/api.ts so a future CRM update only edits one side. Returns (toolName, input) or throws NoLegacyRouteError for unknown paths.
  • createCrmShimRoutes(deps) — Hono app mounted under /api/crm/*. Runs translate(), dispatches via the framework's tool dispatcher, unwraps { ok, result } into the v1 { data, … } shape. Unknown paths → 404 with a structured no_legacy_route error and a Use /api/tools/<name> hint.
  • Mounted in boringos.ts; exports surfaced in index.ts.

Structured so a future contributor can register additional /api/<moduleId>/* shims without rewriting the file. The registry itself is left as a TODO at the top — out of scope until a second module needs the pattern.

Test plan

  • vitest run tests/crm-shim-routes.test.ts — table-driven cases for GET /pipelines, GET /pipelines/:id, POST /pipelines/:id/stages, GET /inbox, plus the unknown-path 404.
  • Manual: install CRM module on a tenant. Open Settings → Pipeline configuration; pipelines list loads, add a stage, refresh, stage persists. Browser network tab shows each /api/crm/* call returning 200 with v1 envelope.

Notes

Pairs with #22 (Vite import-map browserHash fix) — that PR is what eliminated the React duplication that was also contributing to blank CRM screens. Together they fully resolve #20's symptom.

Made with Cursor

The CRM module's settings panels (Pipeline configuration, Inbox, Deal
list, etc.) were authored against an older v1 REST surface
(`/api/crm/pipelines`, `/api/crm/inbox/...`) which no longer exists in
the framework — every request 404'd, the pages rendered blank.

Rather than touching every legacy CRM UI component or shipping a
breaking change to the module, add a framework-side compatibility shim
that translates v1 REST paths into v2 tool calls.

Implementation (`crm-shim-routes.ts`):

- `translate(method, path, query, body)` — port of the existing client
  mapping table from `hebbs-crm/packages/web/src/lib/api.ts`. Kept in
  lockstep so a future CRM update only edits one side. Returns
  `(toolName, input)` or throws `NoLegacyRouteError` for unknown paths.
- `createCrmShimRoutes(deps)` — Hono app mounted under `/api/crm/*`.
  Accepts any HTTP method, runs `translate()`, dispatches the result
  via the framework's tool dispatcher, unwraps the `{ ok, result }`
  envelope into the `{ data, ... }` shape the v1 callers expect.
  Unknown paths return 404 with a structured `no_legacy_route` error
  + a `Use /api/tools/<name>` hint, so typos don't silently swallow.

Mounted in `boringos.ts` next to the other route groups. Public exports
in `index.ts` so module authors and tests can reach the translator
directly.

Generality: structured so a future contributor can register additional
`/api/<moduleId>/*` shims without rewriting the file. The registry
itself is left as a TODO at the top of the file — out of scope until a
second module needs the same pattern.

Regression test (`tests/crm-shim-routes.test.ts`):

Table-driven coverage of the canonical v1 paths the CRM UI hits today
— pipelines list / get / create_stage, inbox list, plus the
`no_legacy_route` 404 case.

Closes hebbs-ai#20.

@parag parag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting changes — and to be clear up front, this isn't a "tweak the shim" request. I don't think this shim should exist at all. The bug it claims to fix doesn't appear to exist in the current CRM bundle, and even if it did, the framework is the wrong place to put the fix. Let me walk through what I found, the way I'd want to read it if I were on the other side of the review.


What the PR says is broken

The CRM module's settings panels (Pipeline configuration, Inbox, Deal list, etc.) were authored against an older v1 REST surface (/api/crm/pipelines, /api/crm/inbox/…) which no longer exists in the framework — every request 404'd, pages rendered blank.

That framing led me to expect: open the CRM web bundle, find raw fetch("/api/crm/...") calls, and watch them blow up against a framework that no longer serves those paths.

So I went looking.

What's actually in the CRM web bundle

I grepped the CRM web source for anything that hits /api/crm. There are exactly three matches, and all three are comments describing the old, removed code path:

  • hooks/useActions.ts:3 — "(/api/crm/actions/...) were removed when the CRM became a module."
  • slots/PipelineSettings.tsx:9 — "the previous v1 raw /api/crm/pipelines PATCH route doesn't…"
  • slots/DealDetail.tsx:5 — "data flows (TanStack Query against /api/crm/deals/*) keep working"

Zero actual fetch("/api/crm/...") calls. The CRM doesn't talk to that surface anymore.

What does the CRM actually do? Two clean paths:

  1. Direct v2 tool dispatch. lib/api.ts exports a typed tool(name, input) helper that POSTs to /api/tools/<toolName> and unwraps { ok, result }. The PipelineSettings slot that the PR description calls out as the canonical broken example does this directly:

    // packages/web/src/slots/PipelineSettings.tsx:92, 127, 145, 163
    await tool("crm.pipelines.update_stage", {})
    await tool("crm.pipelines.delete_stage", {})
    await tool("crm.pipelines.create_stage", {})

    No legacy path, no shim needed.

  2. The api.get/post/put/patch/delete wrapper, for hooks that haven't been rewritten yet. lib/api.ts:199-213 is interesting — it takes the v1-style path (e.g. /actions/count), runs the exact same translate(method, path, body) function the PR ports into the framework, then calls tool(toolName, input). The translation happens in the browser, before any HTTP request goes out. The resulting request goes to /api/tools/crm.actions.count_pending, not /api/crm/actions/count.

    useActions.ts:56, 65, 73, … calls api.get/post. Those resolve to /api/tools/* requests on the wire. They never touch /api/crm.

So the bundle has two code paths and neither hits /api/crm. The translator already exists, in hebbs-crm/packages/web/src/lib/api.ts, and it runs client-side.

What's actually in this PR

The PR ports translate() verbatim from the CRM repo into the framework repo (crm-shim-routes.ts:72-100 — the comment block is literally the same table copy-pasted) and stands up a Hono app under /api/crm/* that takes v1 paths, runs the same translator, dispatches to the v2 tool, and unwraps the envelope.

The framework now has the server-side version of work the browser already does. Two copies of translate(), two repos, kept in lockstep by hand.

Why I don't think this should land

Three problems, ordered from "concrete bug" to "wrong shape":

1. There's no caller to satisfy.

I'd want a single concrete fetch("/api/crm/...") site, in any production code, that this shim would unbreak. The PR description points at PipelineSettings.tsx but PipelineSettings calls tool() directly. The other slots and hooks all go through the client-side translator or framework /api/admin/* endpoints.

The test plan says "pipelines list loads, add a stage, refresh, stage persists. Browser network tab shows each /api/crm/* call returning 200." I genuinely can't reproduce what would generate those /api/crm/* calls — the slot that does pipelines management uses tool(), and TanStack Query against crm.contacts.list etc. goes through api.get → translator → /api/tools/.... If you're seeing /api/crm/* in the network tab on main, something else is happening (older bundle build cached? local dev not rebuilding the CRM web bundle? a different fork?). Worth pinning that down before adding 517 lines of compat surface — the symptom may be coming from somewhere the shim doesn't help.

Even more telling: the PR's "Notes" section says this PR pairs with #22 (Vite browserHash fix), and that #22 "eliminated the React duplication that was also contributing to blank CRM screens." React duplication blanks every screen, full stop — it's not a contributing factor, it's the kind of bug that is the blank screen. If #22 fixes that, the blank-screen symptom is gone, and this PR is solving a phantom that #22 already made go away.

2. The translator is now duplicated across two repos.

Even if there were a caller, the design has a sync problem baked in. hebbs-crm/packages/web/src/lib/api.ts and boringos-framework/packages/@boringos/core/src/crm-shim-routes.ts both hand-write the same mapping table. The shim file's own comment admits this: "kept in lockstep with hebbs-crm/packages/web/src/lib/api.ts."

That's a cross-repo invariant maintained by hand. The two repos release on different cadences, sit in different review queues, and have no compile-time relationship. The first time someone adds a new CRM endpoint, half the world will be updated and half won't. There's no failing test on either side that catches the drift — the framework test (crm-shim-routes.test.ts) only checks paths the framework knows about, and the CRM tests only check the CRM's copy of the table. The bug surfaces as a 404 from a path that used to work, weeks after the change, in production, on whichever side forgot to update.

If the translator is genuinely a shared concern, it belongs in one place — and that place is the CRM, since it's CRM-specific knowledge. Not the framework.

3. CRM-specific surface area in framework core is the wrong shape.

boringos.ts:894-899 mounts createCrmShimRoutes unconditionally, on every install, whether or not the CRM module is present. A vanilla BoringOS install with no CRM installed now answers requests on /api/crm/* with structured 404s (code: "no_legacy_route"). Free response surface, baked into the framework core, advertising a module that may not even be loaded.

CLAUDE.md (the CRM repo's, /Users/paragarora/Documents/Workspace/research/hebbs-clients/boringos-crm/CLAUDE.md) is pretty explicit about this:

Use the framework. Don't rebuild it. BoringOS provides: auth, tasks, agents, runtimes, workflows, connectors, inbox, memory, search, realtime events, activity logging, entity references, admin API, copilot. The CRM uses all of these. It only builds CRM-specific things: contacts, companies, deals, pipelines, activities.

The principle works in the other direction too: the framework should not contain CRM-specific things. /api/crm/* is a CRM-specific URL prefix. If the CRM ever genuinely needs a server-side shim (it doesn't appear to today), it should ship as part of the CRM .hebbsmod bundle and register itself with app.route("/api/crm", …) like any other module-owned route. The framework's job is to provide app.route(), not to know that crm is a thing.

The PR's own TODO comment about "generalising into a registry" so future modules can add their own shims is, I think, the tell that this isn't a framework concern. If five modules each need a v1 compat shim, that's five modules each shipping their own /api/<moduleId>/* Hono app via app.route() — there's no registry needed, the framework already gives you that primitive.

Other things worth noting

  • Mounting order. app.route("/api/crm", crmShimApp) happens at boringos.ts:899, unconditional. If a future genuine /api/crm route gets added by a CRM module, the framework's shim will swallow it (Hono dispatches the first match). Belt-and-braces "we own /api/crm now" from the framework side.
  • Auth duplication. createCrmShimRoutes reimplements the dual-mode (callback JWT + session bearer) auth that tool-routes.ts already implements. The shim's auth handler is 40+ lines that mirror the production createToolRoutes middleware. Worth using the same factory or, at minimum, refactoring shared auth into a helper before duplicating. Same problem as the translator: hand-kept in sync, drifts silently.
  • Test coverage. The unit tests are well-structured (table-driven, covers happy path + 404), but they only assert the translator's mapping behavior. They don't assert the shim is actually doing anything useful — there's no integration test that says "this caller existed, was broken, is now fixed."
  • Error envelope leak. The shim's success path returns dispatched.result.result ?? {} (line 326), but the v1 hooks expect { data: [...], total, ... } — what if the v2 tool's result is shaped differently from what the v1 hook expects? E.g. crm.contacts.list returns { data, total } today, but crm.actions.count_pending returns { pending: number }. The shim assumes the v2 tool's result already matches the v1 envelope. That's not a guarantee the framework can give — it depends on each tool's internal shape, which lives in the CRM module, which the framework can't see. Brittle.

What I'd like instead

  1. Close this PR unless you can show me a real, current caller of /api/crm/* that this shim would unbreak. If you can, please share the exact line and the network trace — I'm happy to be wrong, but the grep doesn't lie.
  2. If #22 (Vite browserHash) was independently the fix for the blank-screen symptom, that's the actual bug closure for #20. Note that in the issue and we're done.
  3. If at some future point a real caller emerges (e.g. an external integration that genuinely speaks v1 REST against the CRM), put the shim in the CRM .hebbsmod. The framework provides app.route(); the CRM uses it. One translator, one repo, no cross-repo invariant.
  4. Drop the duplicated auth handler. Whatever path eventually serves these requests should reuse createToolRoutes' middleware factory, not re-implement it.

Leaving this PR open in case there's a caller I missed. If we can't find one after a sanity check, I'd recommend closing in favor of #22 doing the actual work.

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.

CRM (boringos-crm) plugin module: Pipeline configuration panel hits non-existent /api/crm/pipelines (blank screens)

2 participants