Skip to content

refactor(desktop): share the feature services context and pass bridge namespaces through - #4590

Merged
Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:refactor/feature-services-context
Sep 3, 2026
Merged

refactor(desktop): share the feature services context and pass bridge namespaces through#4590
Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:refactor/feature-services-context

Conversation

@chihumyum

Copy link
Copy Markdown
Contributor

Summary

Nine feature slices each restate the same twenty-line services context and hand-write Desktop adapter forwarders whose signatures equal the bridge methods they forward to (the per-slice join cost Astro-Han measured on #3439: identical services-context.tsx files, and adapters equivalent to { sessions: bridge.sessions }). This PR removes the restatement without changing any port, consumer, or behavior.

  • application/contracts/feature-services.tsx exports createServicesContext<S>(providerName), returning the Provider/hook pair. application/contracts is the one renderer zone every feature may import. Each slice's services-context.tsx is now four lines; every exported name, type, and error message (<Feature>ServicesProvider is missing) is unchanged, and connection-settings keeps its two extra components.
  • Desktop adapters hand a bridge namespace through where the port is a structural subset of it: sessions: bridge.sessions (Session Navigation), catalog: bridge.newTasks (Task Entry), scheduledTasks: bridge.scheduledTasks (Module Hub), and terminal: bridge.shellRuns, todo: bridge.todo, attachments: bridge.attachments (Workbar) — 33 identity forwarders gone. Blocks that rename, guard, filter, or translate (goal, skills, dailyReview, browser, artifacts, inspector, review, sideChat, sessionCollaboration, peerMesh) stay hand-written. I tried { ...bridge.ns, <adaptation> } for those and backed it out: the adapter tests drive Proxy-based bridge recorders that have no own keys, so a spread copies nothing, and the same would hold for any bridge double built that way. Passing the namespace object itself keeps late binding and works with every double.
  • Preload bridge namespaces are plain objects with no this usage, so passing one through is runtime-safe, and the port types stay narrow, so nothing new is reachable from feature code.
    Net: 91 lines removed across 17 files. composition/desktop-feature-services.tsx is deliberately untouched; the tenth slice from refactor(desktop): move App Update controller below AppShell #4498 can adopt the factory after it lands.

Refs #4582

Verification

On the exact head, under Node 24, all green:

  • npm --workspace @maka/desktop run test:dist — 1969/1969
  • npm --workspace @maka/desktop run typecheck — preload, main, renderer, storybook
  • npm run lint, npm run format:check
  • npm run check:renderer-architecture -- --base upstream/main, npm run check:app-shell-hooks (42 hooks / 78 call sites, unchanged)
  • npm run astryx:surface-inventory (regenerated for the new file), npx knip --workspace apps/desktop, npm run check:asf-headers, git diff --check
  • npm --workspace @maka/desktop run build:renderer

renderer-architecture.json needed no regeneration beyond the rebase: application/contracts is an explicit owner zone, so the new file is governed by zone rules rather than recorded as debt.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code — implementation and local validation. The commit carries a Generated-by: Claude Code trailer.

Checklist

  • Tests cover the change and fail without it — the existing adapter and boundary suites are the contract; no new tests, because the change removes restatement rather than adding behavior
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 2, 2026
@chihumyum
chihumyum force-pushed the refactor/feature-services-context branch from 512b255 to ccc75c1 Compare September 2, 2026 20:08
@chihumyum

Copy link
Copy Markdown
Contributor Author

The first CI run failed only in Desktop e2e, on streaming-remount.spec.ts:133 (the streaming bubble did not appear within 10s after the first send). That spec exercises the send and transcript path, which this PR does not touch, and it passed locally on this head three times in a row (--repeat-each 3, 12/12). The same e2e job has flaked on unrelated PRs today: transcript-scroll.spec.ts:251 on #4440 and #4577's neighbours, streaming-remount.spec.ts:142 and send-message.spec.ts:35 on #4577, code-scroll.spec.ts:22 and transcript-measure.spec.ts:22 elsewhere. I cannot re-run the job from this account, so this push (same content, exact head ccc75c135973c3a4af9b69ae30ce5480a86e73ee) re-runs it.

Automated update from Claude Code on behalf of the PR author.

… namespaces through

Add application/contracts/feature-services.tsx with createServicesContext,
and derive every feature slice's ServicesProvider/useServices pair from it
instead of restating the same createContext boilerplate nine times; every
exported name, type, and error message is unchanged.

Where a Desktop adapter's port is a structural subset of one bridge
namespace, hand the namespace through (sessions, newTasks, scheduledTasks,
shellRuns, todo, attachments) instead of restating each method. Blocks that
rename, guard, filter, or translate stay hand-written: the adapter tests
drive Proxy-based bridge recorders without own keys, so a spread would copy
nothing there, and passing the object keeps late binding everywhere.

Generated-by: Claude Code
@chihumyum
chihumyum force-pushed the refactor/feature-services-context branch from ccc75c1 to d0c8078 Compare September 3, 2026 15:12
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main 68cda0b54 to clear the conflict. Exact head: d0c80782582518164f85cc2aa2e1a93c3f1d64d8. Generated files were regenerated; the full local verification is green on it.

Posted by Claude Code on behalf of the PR author.

@Astro-Han Astro-Han 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.

Approving: behavior is preserved for every consumer, and no second provider is left behind.

What I checked. The exported names in all nine services-context.tsx files are identical to main (compared export lists file by file), and the context objects were module-local consts on main, never exported, so no consumer read anything other than the Provider and the hook. createServicesContext calls createContext per invocation, so the nine slices still get nine distinct contexts, and the <Feature>ServicesProvider is missing strings match. The remaining createContext sites in the renderer are composer-mentions.tsx, goals/ui/goal-provider.tsx, session-collaboration/turn-request-inbox-context.tsx, settings/runtime-host-settings-target.tsx, and features/usage/services-context.tsx. Only the last shares the file name, and it is a different thing: it carries a snapshot, a reload ticket, target invalidation and an imperative fence, so it is correctly left alone rather than being a second copy of the factory.

The namespace pass-through holds up too. The preload has no this. usage, makaBridge is one object literal exposed in a single exposeInMainWorld call, and the E2E latch patches it before expose, so a passed-through namespace still resolves to the patched implementation. The construction-time read of bridge.sessions (previously a call-time read) is safe because createDesktopFeatureServices() runs once at main.tsx:37, after preload, and bridge = window.maka was already eager.

P3: the change made part of the adapter suites tautological. apps/desktop/src/main/__tests__/session-navigation-services-adapter.test.ts drives a Proxy with no own keys and asserts what each of seven methods forwarded; with sessions: bridge.sessions that assertion is now true by construction and can no longer go red on a wrong mapping. The scheduledTasks, catalog, terminal, todo and attachments sections of the other three adapter tests are in the same position. Since this change is what made them redundant, deleting the pure pass-through cases (and keeping the ones that still cover a rename, a guard or a translation) belongs here rather than later. Not blocking.

No P0, P1 or P2. CI test is green on this head and the merge against main is clean.

Evidence boundary: static read of d0c8078 against origin/main, plus the CI check-runs API. No checkout, no build, no tests run locally; the preload and contextBridge reasoning comes from source, not from a running app.

AI-assisted review: drafted with Claude Code.

import type { RuntimeHostManagementServices } from './ports.js';

const RuntimeHostManagementServicesContext = createContext<RuntimeHostManagementServices | null>(null);
const { Provider, useServices } = createServicesContext<RuntimeHostManagementServices>('RuntimeHostManagementServicesProvider');

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.

P3: this line is 128 characters, over the repo's lineWidth: 100 (biome.jsonc:47). CI cannot catch it because apps/desktop/** is excluded from the formatter (biome.jsonc:56), but the rest of desktop keeps to 100, and connection-settings/services-context.tsx in this same PR wraps the identical call. Six of the nine rewritten files are over: module-hub 104, task-entry 104, session-settings 116, session-navigation 120, session-collaboration 126, this one 128. Smallest fix: wrap them the way connection-settings does.

bridge.sessions.remove(sessionId, options),
previewRemoval: (sessionId) => bridge.sessions.previewRemoval(sessionId),
},
sessions: bridge.sessions,

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.

P3, recording the trade-off rather than blocking it. This is the widest of the six pass-throughs: SessionNavigationServices['sessions'] is 7 methods, while the object the rail now holds at runtime is the full window.maka.sessions, 47 methods including send, stop, compact, regenerateTurn, approvePlan and respondToSandboxBoundary. Before this change the adapter object itself was the enforcement; now only the port type is, so a future cast reaches the whole execution surface from a navigation feature. No consumer does that today and the README amendment states the rule, so I am not asking for a change. If you want to keep one adapter hand-written, this is the one that earns it.

@Astro-Han
Astro-Han merged commit cbd4d20 into apache:main Sep 3, 2026
1 check passed
ggbdpq pushed a commit to ggbdpq/maka that referenced this pull request Sep 4, 2026
… namespaces through (apache#4590)

Nine renderer feature slices each restated the same twenty-line services context, and their Desktop adapters hand-wrote forwarders whose signatures equal the bridge methods they forward to. Every new slice paid that join cost again, and the copies could drift apart without any test noticing.

`application/contracts/feature-services.tsx` now exports `createServicesContext<S>(providerName)`, which returns the Provider and hook pair. Each slice's `services-context.tsx` shrinks to a few lines while keeping every exported name, type, and error message. Desktop adapters hand a bridge namespace through where the port is a structural subset of it (`sessions`, `newTasks`, `scheduledTasks`, `shellRuns`, `todo`, `attachments`), removing 33 identity forwarders. Adapter blocks that rename, guard, filter, or translate stay hand-written. Preload namespaces are plain objects with no `this` usage, so passing one through is runtime-safe, and the port types stay narrow, so nothing new is reachable from feature code.

`composition/desktop-feature-services.tsx` is untouched; the tenth slice from apache#4498 can adopt the factory after it lands.

No behavior change. Refs apache#4582

Generated-by: Claude Code

Generated-by: GLM-5.3-Flash (ZCode)
@chihumyum
chihumyum deleted the refactor/feature-services-context branch September 4, 2026 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants