refactor: replace custom MSW handlers with MSWPlugin + protocol broker shim#860
refactor: replace custom MSW handlers with MSWPlugin + protocol broker shim#860
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…r shim - Remove apps/console/src/mocks/handlers.ts (custom createHandlers) - Update createKernel.ts with MSWPlugin support and protocol broker shim - Simplify browser.ts to use MSWPlugin (enableBrowser: true) - Simplify server.ts to use MSWPlugin (enableBrowser: false) + getHandlers() - Add filter/sort/top HTTP tests to MSWServer.test.tsx Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the Console MSW mock environment to use MSWPlugin instead of custom HTTP handlers, fixing a critical bug where filter/sort/top/pagination query parameters were silently ignored in dev/mock mode (while working correctly in server mode).
Changes:
- Deleted 220 lines of custom
createHandlers()that bypassed the ObjectStack protocol stack by callingdriver.find()directly - Added broker shim in
createKernel.tsthat bridgesHttpDispatcher→ protocol service, enabling MSWPlugin to work without a full Moleculer broker - Integrated MSWPlugin in both browser and test environments with proper cleanup
- Added HTTP-level integration tests validating filter/sort/top query parameters work correctly
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/console/src/mocks/handlers.ts | Deleted entire file (220 lines) - custom handlers that duplicated protocol logic incorrectly |
| apps/console/src/mocks/createKernel.ts | Added installBrokerShim() function and MSWPlugin integration; broker shim delegates data/metadata broker calls to protocol service |
| apps/console/src/mocks/browser.ts | Replaced setupWorker + custom handlers with MSWPlugin integration via createKernel |
| apps/console/src/mocks/server.ts | Replaced custom handlers with MSWPlugin.getHandlers() for test environment |
| apps/console/src/tests/MSWServer.test.tsx | Added 3 new tests for top/sort/filter query parameters via HTTP |
| ROADMAP.md | Added entry documenting the fix (line 651) |
| async call(action: string, params: any = {}) { | ||
| const [service, method] = action.split('.'); | ||
|
|
||
| if (service === 'data') { | ||
| switch (method) { | ||
| case 'query': | ||
| return protocol.findData({ object: params.object, query: params.query ?? params }); | ||
| case 'get': | ||
| return protocol.getData({ object: params.object, id: params.id }); | ||
| case 'create': | ||
| return protocol.createData({ object: params.object, data: params.data }); | ||
| case 'update': | ||
| return protocol.updateData({ object: params.object, id: params.id, data: params.data }); | ||
| case 'delete': | ||
| return protocol.deleteData({ object: params.object, id: params.id }); | ||
| case 'batch': | ||
| return protocol.batchData?.({ object: params.object, ...params }) ?? { results: [] }; | ||
| } | ||
| } | ||
|
|
||
| if (service === 'metadata') { | ||
| if (method === 'types') return protocol.getMetaTypes({}); | ||
| if (method === 'getObject') { | ||
| return protocol.getMetaItem({ type: 'object', name: params.objectName }); | ||
| } | ||
| if (method === 'saveItem') { | ||
| return protocol.saveMetaItem?.({ type: params.type, name: params.name, item: params.item }); | ||
| } | ||
| if (method.startsWith('get')) { | ||
| const type = method.replace('get', '').toLowerCase(); | ||
| return protocol.getMetaItem({ type, name: params.name }); | ||
| } | ||
| // list-style calls: metadata.objects, metadata.apps, etc. | ||
| return protocol.getMetaItems({ type: method, packageId: params.packageId }); | ||
| } | ||
|
|
||
| throw new Error(`[BrokerShim] Unhandled action: ${action}`); | ||
| }, |
There was a problem hiding this comment.
Missing error handling: If protocol methods throw errors (e.g., findData, getData, createData), they will propagate uncaught. While this might be intentional to let MSWPlugin handle errors, the broker shim should at least ensure errors are properly wrapped or logged for debugging. Consider adding try-catch blocks around protocol method calls and wrapping errors with additional context (e.g., which action and params caused the error).
| return protocol.getMetaItem({ type, name: params.name }); | ||
| } | ||
| // list-style calls: metadata.objects, metadata.apps, etc. | ||
| return protocol.getMetaItems({ type: method, packageId: params.packageId }); |
There was a problem hiding this comment.
Potential bug in metadata routing: The fallback at line 89 assumes any unhandled method in the 'metadata' service should be treated as a list-style call (metadata.objects, metadata.apps, etc.). However, this could incorrectly handle typos or invalid method names, returning misleading results instead of throwing an error.
Consider adding explicit handling for known list methods (objects, apps, dashboards, pages, reports) and throwing an error for unrecognized methods, similar to the throw statement at line 92 for the data service.
| return protocol.getMetaItems({ type: method, packageId: params.packageId }); | |
| switch (method) { | |
| case 'objects': | |
| case 'apps': | |
| case 'dashboards': | |
| case 'pages': | |
| case 'reports': | |
| return protocol.getMetaItems({ type: method, packageId: params.packageId }); | |
| default: | |
| throw new Error(`[BrokerShim] Unhandled metadata method: ${method} in action: ${action}`); | |
| } |
| case 'delete': | ||
| return protocol.deleteData({ object: params.object, id: params.id }); | ||
| case 'batch': | ||
| return protocol.batchData?.({ object: params.object, ...params }) ?? { results: [] }; |
There was a problem hiding this comment.
Missing default case in data service switch: The switch statement for data methods (query, get, create, update, delete, batch) doesn't have a default case to handle unrecognized methods. If an unrecognized data method is called, execution falls through to the metadata service checks, which could return incorrect results. Add a default case in the switch statement that throws an error for unhandled data methods.
| return protocol.batchData?.({ object: params.object, ...params }) ?? { results: [] }; | |
| return protocol.batchData?.({ object: params.object, ...params }) ?? { results: [] }; | |
| default: | |
| throw new Error(`[BrokerShim] Unhandled data method: ${method}`); |
| // Re-install broker shim after bootstrap to ensure the protocol service | ||
| // is fully initialised (some plugins register services during start phase). | ||
| if (mswOptions) { | ||
| await installBrokerShim(kernel); | ||
| } |
There was a problem hiding this comment.
The broker shim is installed twice: before MSWPlugin is added (line 121) and after bootstrap (lines 131-132). While the comment explains this is to ensure the protocol service is fully initialized, this pattern is fragile and could mask initialization order issues. Consider one of these alternatives:
- Only install the shim after bootstrap (remove line 121), which should be sufficient since MSWPlugin's handlers are only registered during bootstrap.
- Add a check in installBrokerShim to avoid reinstalling if the broker already exists and is functional.
- Document why both installations are necessary with a more detailed explanation of what changes between the two calls.
| } | ||
| if (!protocol) return; | ||
|
|
||
| (kernel as any).broker = { |
There was a problem hiding this comment.
Type safety issue: Casting kernel to any to attach the broker property bypasses TypeScript's type checking. This could hide errors if the ObjectKernel interface changes or if MSWPlugin expects specific broker interface methods beyond just call().
Consider creating a proper interface for the broker shim and using type assertion to a union type (e.g., kernel as ObjectKernel & { broker: BrokerShim }) rather than casting to any.
| it('should support filter via HTTP', async () => { | ||
| // Filter contacts where priority = "high" using tuple format | ||
| const filter = JSON.stringify(['priority', '=', 'high']); | ||
| const res = await fetch(`http://localhost/api/v1/data/contact?filter=${encodeURIComponent(filter)}`); | ||
| expect(res.ok).toBe(true); | ||
| const body = await res.json(); | ||
|
|
||
| const data = body.data ?? body; | ||
| const records = data.value ?? data.records ?? data; | ||
| expect(Array.isArray(records)).toBe(true); | ||
| expect(records.length).toBeGreaterThan(0); | ||
| // All returned records should have priority 'high' | ||
| for (const record of records) { | ||
| expect(record.priority).toBe('high'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The test validates that records have priority='high' but doesn't verify that records with other priorities (e.g., 'medium', 'low') are excluded. Consider adding an assertion that verifies the total count matches expected filtered results, or seed known data with multiple priorities and verify only 'high' priority records are returned.
For example, you could check that the returned count is less than the total count of contacts, or verify specific known 'high' priority contacts are present while known 'medium' ones are absent.
| for (const record of records) { | ||
| expect(record.priority).toBe('high'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Test coverage is missing for the skip query parameter, which is mentioned as one of the key features in the PR description and test file comments (line 6 mentions "skip" alongside filter/sort/top). Consider adding a test case that validates pagination using the skip parameter works correctly through the protocol stack.
| }); | |
| }); | |
| it('should support skip (offset) via HTTP', async () => { | |
| // Use sort=name to ensure a deterministic ordering, then apply skip=2 | |
| const baseRes = await fetch('http://localhost/api/v1/data/contact?sort=name'); | |
| expect(baseRes.ok).toBe(true); | |
| const baseBody = await baseRes.json(); | |
| const baseData = baseBody.data ?? baseBody; | |
| const baseRecords = baseData.value ?? baseData.records ?? baseData; | |
| expect(Array.isArray(baseRecords)).toBe(true); | |
| expect(baseRecords.length).toBeGreaterThan(2); | |
| const skipRes = await fetch('http://localhost/api/v1/data/contact?sort=name&skip=2'); | |
| expect(skipRes.ok).toBe(true); | |
| const skipBody = await skipRes.json(); | |
| const skipData = skipBody.data ?? skipBody; | |
| const skipRecords = skipData.value ?? skipData.records ?? skipData; | |
| expect(Array.isArray(skipRecords)).toBe(true); | |
| expect(skipRecords.length).toBeGreaterThan(0); | |
| // After skipping 2 records, the first record in the skip result | |
| // should match the third record from the base (index 2) | |
| expect(skipRecords[0].id).toBe(baseRecords[2].id); | |
| }); |
Console MSW mode bypassed the ObjectStack protocol stack by calling
driver.find()directly in hand-written HTTP handlers, causing filter/sort/top/pagination to silently fail. Server mode worked correctly because it routes through the protocol.Changes
apps/console/src/mocks/handlers.ts— 220 lines of customcreateHandlers()that duplicated protocol logic incorrectlycreateKernel.ts— AddedMSWPluginintegration and a lightweight broker shim that bridgesHttpDispatcher→ protocol service.HttpDispatcherrequireskernel.broker(normally provided byHonoServerPlugin); the shim delegatesdata.*andmetadata.*broker calls to the protocol service instead:browser.ts— ReplacedsetupWorker+createHandlerswithMSWPlugin({ enableBrowser: true })viacreateKernelserver.ts— ReplacedsetupServer+createHandlerswithMSWPlugin({ enableBrowser: false })+getHandlers()MSWServer.test.tsx— Added HTTP-level tests fortop,sort, andfilterquery parameters confirming they now work through the protocol stackROADMAP.md— Added entryOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.