-
-
Notifications
You must be signed in to change notification settings - Fork 32
fix: to check all items in an envelope for errors #880
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #880 +/- ##
=======================================
Coverage 31.31% 31.31%
=======================================
Files 127 127
Lines 9562 9562
Branches 133 133
=======================================
Hits 2994 2994
Misses 6568 6568 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
packages/sidecar/src/mcp/index.ts
Outdated
| const [{ type }, payload] = item; | ||
| if (type === "event" && isErrorEvent(payload)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: The code destructures `type` from an envelope item header. This will crash if the `type` field is missing, a case handled defensively elsewhere in the codebase.
- Description: The code at
packages/sidecar/src/mcp/index.tsuses destructuringconst [{ type }, payload] = item;to extract thetypefrom an envelope item's header. However, other parts of the codebase use defensive patterns like optional chaining (item?.[0].type) and explicit checks (if (itemHeader.type)), indicating that thetypefield can be missing in practice. This new, non-defensive approach will throw aTypeErrorif it processes an envelope item header that lacks atypeproperty, causing theget_errorstool to crash. - Suggested fix: Instead of direct destructuring, access the
typeproperty safely. Check for the existence of the header and thetypeproperty before using it. For example:const type = item?.[0]?.type; if (type) { /* ... */ }.
severity: 0.6, confidence: 0.9
Did we get this right? 👍 / 👎 to inform future reviews.
BYK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/sidecar/src/mcp/index.ts
Outdated
| if (a_sent_at > b_sent_at) return -1; | ||
| return 0; | ||
| }) | ||
| .flatMap(processed => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move .flatMap() before sort so we don't spend time sorting filtered items? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, moving this later makes sorting proper so it should be part of the patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like code was updated after this patch. fixed it again.
you can review now
thanks
MathurAditya724
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! 😄
Just left some small changes/preferences
packages/sidecar/src/mcp/index.ts
Outdated
| return formatEventOutput(processErrorEvent(payload)); | ||
| } | ||
| const formattedErrors = items | ||
| .map(item => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use a for loop, which pushes to an array only when we have the data. Just to remove these 2 loops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we can do that.
thanks
packages/sidecar/src/mcp/index.ts
Outdated
|
|
||
| if (formattedErrors?.length) { | ||
| for (const formattedError of formattedErrors) { | ||
| if (formattedError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the if check again over here? We already have removed the undefined values in the formatErrorEnvelope function.
You can make this a one liner add all the data using push in the content array and formatting then with map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, fixed it
MathurAditya724
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @spotlightjs/overlay@4.0.0 ### Major Changes - Remove integrations architecture in favour of fixed telemetry features and remove the ability to use the package as an overlay on top of apps. ([#917](#917)) - Remove all vite-plugins as they are deprecated ([#937](#937)) ### Minor Changes - - Implement URL-based routing in overlay with BrowserRouter for web apps and HashRouter for Electron ([#921](#921)) - Remove sentry-integration.ts and simplify telemetry implementation - Moved telemetry logic from App.tsx to dedicated Telemetry component ### Patch Changes - Remove local resource handling and vite plugin as overlay feature is not supported ([#924](#924)) - Refactor to remove object destructuring from hooks returning only one value ([#930](#930)) ## @spotlightjs/sidecar@2.0.0 ### Major Changes - Make `setupSidecar` async as it should be ([#936](#936)) - Remove all vite-plugins as they are deprecated ([#937](#937)) ### Minor Changes - Add support for multiple stdio MCP servers to run, as a proxy for the existing main sidecar ([#935](#935)) - Adds stdio based MCP server via a `--stdio-mcp` CLI argument. Also removes the context-based message buffers to be able to achieve this as there's no context-id for the stdio transport. This feature was not used anyway. ([#933](#933)) Moves all log messages to stderr as they should have been to avoid clobbering the MCP stdio transport. ### Patch Changes - - To check all items in an envelope for errors. ([#880](#880)) - restructured the sidecar server and minor improvements ([#918](#918)) - To add a soft reset buffer when its cleared. ([#895](#895)) ## @spotlightjs/spotlight@4.0.0 ### Major Changes - Make `setupSidecar` async as it should be ([#936](#936)) - Remove all vite-plugins as they are deprecated ([#937](#937)) ### Minor Changes - Add support for multiple stdio MCP servers to run, as a proxy for the existing main sidecar ([#935](#935)) - Adds stdio based MCP server via a `--stdio-mcp` CLI argument. Also removes the context-based message buffers to be able to achieve this as there's no context-id for the stdio transport. This feature was not used anyway. ([#933](#933)) Moves all log messages to stderr as they should have been to avoid clobbering the MCP stdio transport. ### Patch Changes - Remove local resource handling and vite plugin as overlay feature is not supported ([#924](#924)) - Updated dependencies \[[`2fe54bb`](2fe54bb), [`20783d0`](20783d0), [`48c6753`](48c6753), [`65f4e9f`](65f4e9f), [`bb85759`](bb85759), [`1f2096e`](1f2096e), [`cec5457`](cec5457), [`5670dd4`](5670dd4), [`605b1e1`](605b1e1), [`69bfd17`](69bfd17), [`3d56a55`](3d56a55)]: - @spotlightjs/sidecar@2.0.0 - @spotlightjs/overlay@4.0.0 ## @spotlightjs/electron@1.8.0 ### Minor Changes - Run electron app in background and add a menu tray ([#929](#929)) - Make app window draggable ([#931](#931)) - Added Auto Updater ([#922](#922)) ### Patch Changes - - Implement URL-based routing in overlay with BrowserRouter for web apps and HashRouter for Electron ([#921](#921)) - Remove sentry-integration.ts and simplify telemetry implementation - Moved telemetry logic from App.tsx to dedicated Telemetry component - Updated dependencies \[[`2fe54bb`](2fe54bb), [`20783d0`](20783d0), [`48c6753`](48c6753), [`65f4e9f`](65f4e9f), [`bb85759`](bb85759), [`1f2096e`](1f2096e), [`cec5457`](cec5457), [`5670dd4`](5670dd4), [`605b1e1`](605b1e1), [`69bfd17`](69bfd17), [`3d56a55`](3d56a55)]: - @spotlightjs/sidecar@2.0.0 - @spotlightjs/overlay@4.0.0 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Before opening this PR:
pnpm changeset:add