Skip to content

fix(server-runtime): preserve explicit empty data destinations#1635

Merged
nekomeowww merged 4 commits intomoeru-ai:mainfrom
Gujiassh:fix/data-empty-destinations-override
May 3, 2026
Merged

fix(server-runtime): preserve explicit empty data destinations#1635
nekomeowww merged 4 commits intomoeru-ai:mainfrom
Gujiassh:fix/data-empty-destinations-override

Conversation

@Gujiassh
Copy link
Copy Markdown
Contributor

@Gujiassh Gujiassh commented Apr 11, 2026

Summary

  • treat event.data.destinations the same way as event.route.destinations when the property is explicitly present
  • preserve explicit empty destination arrays ([]) instead of collapsing them to undefined
  • add a regression test for the explicit-empty-data override case

Validation

  • pnpm exec vitest run packages/server-runtime/src/middlewares/route.test.ts

This is a follow-up to the routing edge case discussed on #1621.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 11, 2026

⏳ Approval required for deploying to Cloudflare Workers (Preview) for stage-web.

Name Link
🔭 Waiting for approval For maintainers, approve here

Hey, maintainers, kindly take some time to review and approve this deployment when you are available. Thank you! 🙏

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the collectDestinations function to correctly handle explicit empty destination lists as overrides and adds a corresponding test case. A review comment identifies a potential runtime error when using the in operator on primitive types and suggests a safer check.


const data = event.data as { destinations?: Array<string | RouteTargetExpression> } | undefined
if (data?.destinations?.length) {
if (data && 'destinations' in data) {
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.

high

The in operator will throw a TypeError if data is a primitive value (such as a string or boolean). Since event.data can be any type depending on the specific event, it is safer to verify that data is a non-null object before performing this check to avoid potential runtime crashes.

Suggested change
if (data && 'destinations' in data) {
if (data && typeof data === 'object' && 'destinations' in data) {

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2eef8fb8b8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".


const data = event.data as { destinations?: Array<string | RouteTargetExpression> } | undefined
if (data?.destinations?.length) {
if (data && 'destinations' in data) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Guard destination lookup against primitive payloads

collectDestinations now uses data && 'destinations' in data, but event.data is only parsed/cast (not schema-validated) before routing, so malformed events like {"type":"unknown","data":"x"} can reach this path and make the in operator throw a TypeError on a primitive. That turns an otherwise ignorable invalid message into an uncaught runtime failure during message handling; the previous optional-chaining check did not throw in this scenario.

Useful? React with 👍 / 👎.

@nekomeowww
Copy link
Copy Markdown
Member

@copilot resolve the merge conflicts in this pull request

@Gujiassh
Copy link
Copy Markdown
Contributor Author

Rebased this branch onto current main to resolve the merge conflict, and I also addressed the remaining runtime guard concern from review.\n\nUpdated head: d3677d04\n- rebased the branch onto latest main\n- kept explicit empty data.destinations = [] as the override\n- guarded collectDestinations against truthy primitive event.data values before using the in operator\n- added a regression test for primitive payloads\n\nValidation:\n- pnpm exec vitest run packages/server-runtime/src/middlewares/route.test.ts

@Gujiassh Gujiassh force-pushed the fix/data-empty-destinations-override branch from 2f8564e to d3677d0 Compare April 16, 2026 15:46
@Gujiassh
Copy link
Copy Markdown
Contributor Author

Follow-up update on the current branch head c4be8a3e.

I narrowed the destination override typing so collectDestinations() only returns array-shaped overrides, and removed the duplicate empty-route regression block from the route middleware tests.

Focused verification on the refreshed head:

  • pnpm exec vitest run packages/server-runtime/src/middlewares/route.test.ts
  • pnpm run typecheck (the workspace still has unrelated package-level dependency resolution noise, but packages/server-runtime now completes as Done, which clears the three collectDestinations type errors from the previous CI failure)

@Gujiassh Gujiassh force-pushed the fix/data-empty-destinations-override branch from c4be8a3 to 72f7562 Compare April 28, 2026 03:48
@nekomeowww nekomeowww merged commit a5d45bd into moeru-ai:main May 3, 2026
10 checks passed
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