Skip to content

Conversation

PhilBastian
Copy link
Contributor

right flow only, and no message selection

PhilBastian and others added 30 commits March 6, 2025 14:56
# Conflicts:
#	src/Frontend/src/components/messages/MessageView.vue
#	src/Frontend/src/stores/AuditStore.ts
Copy link
Member

@johnsimons johnsimons left a comment

Choose a reason for hiding this comment

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

A few comments added but overall LGTM

async function deleteAllInstances() {
try {
await Promise.all(sortedInstances.value.filter((instance) => instance.name === endpointName).map((instance) => store.deleteEndpointInstance(instance)));
await Promise.all(sortedInstances.value.filter((instance: EndpointsView) => instance.name === endpointName).map((instance: EndpointsView) => store.deleteEndpointInstance(instance)));
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed?
Isn't the array typed already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it started failing my build. Will try removing it again

try {
await store.updateEndpointSettings(
filteredEndpoints.value.filter((endpoint) => (dialogWarningOperation.value === Operation.Track && !endpoint.track_instances) || (dialogWarningOperation.value === Operation.DoNotTrack && endpoint.track_instances))
filteredEndpoints.value.filter((endpoint: LogicalEndpoint) => (dialogWarningOperation.value === Operation.Track && !endpoint.track_instances) || (dialogWarningOperation.value === Operation.DoNotTrack && endpoint.track_instances))
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed?
Isn't the array typed already?


<template>
<div class="outer">
<svg class="sequence-diagram" :width="`max(100%, ${isNaN(maxWidth) ? 0 : maxWidth}px)`" :height="maxHeight + 20">
Copy link
Member

Choose a reason for hiding this comment

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

Should we be rendering at all if isNaN(maxWidth) is true?
I have created a spinner for exactly this purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will add this to the latest branch rather than this one

@mouseover="() => store.setHighlightId(arrow.id)"
@mouseleave="() => store.setHighlightId()"
>
<rect v-if="arrow.highlight && arrow.messageTypeOffset" :width="arrow.highlightTextWidth + 19 + 4 + 4" :height="arrow.highlightTextHeight + 4 + 4" fill="var(--highlight-background)" />
Copy link
Member

Choose a reason for hiding this comment

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

What are the magic numbers?
+ 19 + 4 + 4
+ 4 + 4

Copy link
Member

Choose a reason for hiding this comment

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

Why are we using underscores for private variables?
You can use private visibility - https://www.typescriptlang.org/docs/handbook/2/classes.html#private
Or you can set it at the cstructor - https://www.typescriptlang.org/docs/handbook/2/classes.html#parameter-properties

Copy link
Contributor Author

@PhilBastian PhilBastian Apr 7, 2025

Choose a reason for hiding this comment

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

why not use underscore? I am using private visibility where required

Copy link
Member

@johnsimons johnsimons Apr 7, 2025

Choose a reason for hiding this comment

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

The code base does not use _ to mean "private" anywhere, are we planning to make this a rule now?

Copy link
Contributor Author

@PhilBastian PhilBastian Apr 7, 2025

Choose a reason for hiding this comment

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

no, it's private _variableName, not just _variableName. Since the variable name is the same as what's exposed publicly (as readonly) then I'm using underscore to be able to differentiate

Edit: there was one place where it looks like there was an erroneous _variableName, fixed

Comment on lines +21 to +23
#endpoints: Endpoint[];
#handlers: Handler[];
#processingRoutes: MessageProcessingRoute[];
Copy link
Member

Choose a reason for hiding this comment

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

Declare them as private instead? Easier to understand for c# devs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not truly private though. I was only reverting to the typescript private where necessary due to use of proxies

);

const names = () => filteredInstances.value.map((value) => value.host_display_name);
const names = () => filteredInstances.value.map((value: EndpointsView) => value.host_display_name);
Copy link
Member

Choose a reason for hiding this comment

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

is this needed?

);

const names = () => filteredHealthyEndpoints.value.map((value) => value.name);
const names = () => filteredHealthyEndpoints.value.map((value: LogicalEndpoint) => value.name);
Copy link
Member

Choose a reason for hiding this comment

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

is this needed?

PhilBastian and others added 2 commits April 7, 2025 15:44
…Component.vue

Co-authored-by: John Simons <john.simons@particular.net>
@PhilBastian PhilBastian merged commit 27d4787 into master Apr 7, 2025
5 checks passed
@PhilBastian PhilBastian deleted the sequence_diagram branch April 7, 2025 07:57
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.

3 participants