-
Notifications
You must be signed in to change notification settings - Fork 71
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
Enhance note status #487
base: dev
Are you sure you want to change the base?
Enhance note status #487
Conversation
Nice, a few comments:
|
cc74996
to
d06bef4
Compare
Cool, I like all of this. Maybe left-align the select buttons though. Otherwise, let me know when it's ready for review and I'll take a closer look. |
It should be fine to review now Have created 2 new components, in the view folder, PublishesNotices and PublishesConnections, the plan was to have them link to the router but it proved more challenging than expected with the tabs. So these 2 views are just 2 child components for Publishes. I can move them to another folder if it makes more sense |
c04ad6e
to
7e07291
Compare
} | ||
} | ||
connectionsStatus = newConnectionStatus | ||
}, 800) |
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.
This results in a flash of zeroes on first load. Instead, put this in a throttled function and call it on mount and in an interval.
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.
it is not updated inside the loop, the connectionStatus is updated once every 800 ms, there is no flashing on each connection state
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.
On first page load there is, because the interval doesn't run at first:
https://github.com/user-attachments/assets/2b47224e-536d-4af5-a663-510c9bab2546
9f27fb7
to
5ccb27c
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.
Looking good, a few things:
- Enhance note status #487 (comment) still needs to be resolved
- On the notices page, let's replace the relay selector with a generic "search" box. This should match against relay url, verb (ok/notice), and message.
- The animation on the notices page is janky, use
in
rather thantransition
. - While I like the granularity on the notices page, I agree with your point that storing all this is going to be pretty heavy, for not very much benefit. Let's slim it down to OK and NOTICE.
- Let's factor the thunks page out into its own component as well.
|
||
export let selected: string[] = [] | ||
|
||
$: subNotices = selected.flatMap(s => $subscriptionNotices.get(s)?.map(n => ({...n, url: s}))) |
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.
Maybe derive this in engine/state alongside subscriptionNotices
$: notices = ([...pubNotices, ...subNotices] as (Thunk & (SubscriptionNotice & {url: string}))[]) | ||
.sort((a, b) => { | ||
return a.created_at - b.created_at | ||
}) | ||
.filter(Boolean) |
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.
sortBy(x => -x.created_at, ...)
is a more succinct way to do this. Also, the filter(Boolean)
shouldn't be necessary here, something is wrong upstream if it's needed. (Thunk & (SubscriptionNotice & {url: string}))
is pretty ugly too, SubscriptionNotice should probably include url
in its type, and a |
would be more appropriate instead (although it would require type assertions, so &
is fine here if it works).
export enum ConnectionType { | ||
Connected = "Connected", | ||
Logging = "Logging in", | ||
LoginFailed = "Failed to log in", | ||
ConnectFailed = "Failed to connect", | ||
WaitReconnect = "Wainting to reconnect", | ||
NotConnected = "Not connected", | ||
UnstableConnection = "Unstable connection", | ||
} |
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 prefer to decouple constants and display logic by adding an additional displayConnectionType
function.
connection.on(ConnectionEvent.Receive, function (cxn, [verb, ...args]) { | ||
if (verb == "EVENT") return | ||
subscriptionNotices.update($notices => { | ||
pushToMapKey($notices, connection.url, {created_at: now(), notice: [verb, ...args]}) |
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'd add url
to the object just to make the types easier.
Related to #371
Notices are kind of unclear what should be displayed, I have the event, the status of the request and the relay url, not sure how to display this.
This is a draft, just look at the screenshot and give me some general feedback
Stats box are likely too big, you can see a timeout attached to some relay
Here the relay display how many successful events where posted to it