-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(app-platform): Open in stacktrace button #12401
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
src/sentry/static/sentry/app/components/events/interfaces/openInButton.jsx
Outdated
Show resolved
Hide resolved
fetchSentryApps() { | ||
const {api, organization} = this.props; | ||
api | ||
.requestPromise(`/organizations/${organization.slug}/sentry-apps/`) |
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.
I'm counting 3 API calls to get sentry-app data. Should we have a 'kitchen sink' API to push the complexity into the server and allow the client to be simpler/more efficient with network?
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.
My general opinion is that we already design the API to one specific consumer (this React app) too much. I'd like us to move away from that and towards a world where the API is an interface to the primitives of what makes up Sentry, the system.
Consumers can do whatever makes the most sense for their specific data access/format needs (this could mean the whole "back end for front end" idea or a data layer within the app or something else) in the future.
In any case, I guess I'm saying that I'd personally rather have more small API calls over an endpoint that combines three different resources/concepts.
Also, fwiw, I think using the Reflux Store stuff will help a bit here. We won't need to make these requests over and over since a lot of the data is relatively static and only needs to be loaded once when the Issue page is loaded. I have some work for this in another PR and we can refactor this once it's landed.
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.
Ok. My concern was over the additional latency and increased chance for something to fail. I agree that we sometimes build APIs that are more tightly coupled to the react client than they may need to be. We can always come back and add a 'porcelain' API later 😄
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.
I've actually used what matte added and now does some work in the Preparer
so that we don't need all of those calls 😸
src/sentry/static/sentry/app/components/events/interfaces/openInButton.jsx
Outdated
Show resolved
Hide resolved
4ba4369
to
48d076c
Compare
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.
Overall looks pretty good. Some style/syntax suggestions and thoughts, but no show stoppers.
src/sentry/static/sentry/app/components/events/interfaces/openInButton.jsx
Outdated
Show resolved
Hide resolved
src/sentry/static/sentry/app/components/events/interfaces/openInButton.jsx
Outdated
Show resolved
Hide resolved
render() { | ||
const {components, installs, sentryApps} = this.state; | ||
if (!components.length || !installs.length || !sentryApps.length) { | ||
return null; |
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.
Does this cause issues? Do you need to return ''
?
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.
I think you have to return null
src/sentry/static/sentry/app/components/events/interfaces/openInButton.jsx
Outdated
Show resolved
Hide resolved
src/sentry/static/sentry/app/components/events/interfaces/openInButton.jsx
Outdated
Show resolved
Hide resolved
635d071
to
09a7907
Compare
This reverts commit 1acc5c6.
* master: (36 commits) Revert "feat(app-platform): Open in stacktrace button (#12401)" fix(app-platform): Unsubscribe from Store changes (#12445) feat(app-platform): Open in stacktrace button (#12401) ref(project transfer): Add explanatory sentence to modal (#12427) feat(features): Add org saved searches [SEN-355] (#12441) add project to payload (#12407) feat(app-platform): Issue Link UI (#12345) fix(api): Fix bug where parser didn't allow dates ending in `Z`, and didn't correctly report the parse fail to the user (ISSUE-376) Add enhanced privacy to feedback (#12418) fix: Add sentry tag to renormalized events (#12434) dev: Ensure docker binds to an interface EventCommon mixin for common event functionality (#12422) build(webpack): Only show errors in webpack output (#12425) ref(ui): Remove unused utils (#12424) ref: Sample to_python calls 2 (#12375) feat(eventtypes): Keep value retained for synthetic errors (#12355) fix: Fix UI side to deal with invalid values (#12433) ref(grouping): Added initial pass of new grouping algorithm (#12414) build(webpack): Fix incremental webpack builds (upgrade to node@8.15.1) (#12408) Revert "feat: Common functionality for Snuba Events (#11071)" (#12421) ...
Uh oh!
There was an error while loading. Please reload this page.