Skip to content
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

[AXON-41] Add error boundary to JIRA pages #60

Open
wants to merge 1 commit into
base: AXON-41-boundary-everywhere
Choose a base branch
from

Conversation

sdzh-atlassian
Copy link
Member

What is this?

This PR adds AtlascodeErrorBoundary to the remaining 3 pages, implemented with the older architecture and IPC.

How was this tested?

Built the extension, ran things in debug, made sure analytics events are emitted

@@ -77,6 +77,7 @@ module.exports = [
'process.env.ATLASCODE_FX3_ENVIRONMENT': JSON.stringify(process.env.ATLASCODE_FX3_ENVIRONMENT),
'process.env.ATLASCODE_FX3_TARGET_APP': JSON.stringify(process.env.ATLASCODE_FX3_TARGET_APP),
'process.env.ATLASCODE_FX3_TIMEOUT': JSON.stringify(process.env.ATLASCODE_FX3_TIMEOUT),
'process.env.CI': JSON.stringify(process.env.CI),
Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently some transitive dependencies have special behaviour depending on $CI. We probably will too, eventually :D

<AtlascodeErrorBoundary
context={{ view: AnalyticsView.JiraIssuePage }}
postMessageFunc={(e) => {
this.postMessage(e); /* just {this.postMessage} doesn't work */
Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently just putting this.postMessage here doesn't work. I'm guessing it's because postMessage is not a callback, and not memo-ized properly? I'd really rather not mess with these pages more than absolutely necessary :D

PipelineSummaryPage = 'pipelineSummary',

StartWorkPage = 'startWork',
OldStartWorkPage = 'oldStartWork',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we like this naming convention?

Eg: should we be starting or ending with the "old" word?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, forgot I had it here lol :) As per the conversation in the other PR - let's fix this in a follow-up ;)

@@ -206,6 +207,13 @@ export abstract class AbstractReactWebview implements ReactWebview {
}
}
}

// Special case for analytics which are normally routed through the new IPC system
if (a?.type === CommonActionType.SendAnalytics && a?.errorInfo) {
Copy link
Collaborator

@bwieger-atlassian-com bwieger-atlassian-com Jan 2, 2025

Choose a reason for hiding this comment

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

a is defined as any in this function. So this specific line should work.

But will the if (isAction(a)) action work here?

export function isAction(a: any): a is Action {
    return a && (<Action>a).action !== undefined;
}

Not sure myself.

Copy link
Member Author

Choose a reason for hiding this comment

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

It actually won't, I had to look into this :) The reason for this is the (unbelievably frustrating) difference in implementation for the JIRA pages and all the other ones

The older (JIRA) pages use this definition for events from src/ipc:

{
    action: "SOME_STRING",
    ...payload
}

The newer (other) pages use this definition in src/lib/ibc (!!?? what a nice trick to see if we're paying attention, eh?)

{
    type: "SOME_STRING",    // from ReducerAction<K,V>
    ...payload,
}

For this new event, I didn't want to have 2 parallel implementations - and just added handling for a new-style event in the old-style handler - hence the weird out-of-place check ;D

Copy link
Collaborator

@bwieger-atlassian-com bwieger-atlassian-com left a comment

Choose a reason for hiding this comment

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

Couple of questions

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