Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add message redaction #524
Add message redaction #524
Changes from 4 commits
6f44ee6
d6d45f6
b092e88
0e0f71c
323454d
4efeb02
4962974
4432a2d
15f353d
6328b52
6fe3a60
a70a629
4ba56db
6b2074b
4e0d316
d270474
d5edb9b
06d4c3c
0629dd4
9a97dc1
0258e52
eea9ec4
1680c6b
8ab12e9
dfc1d8b
89e6a3c
8fb0a99
16ccba5
2ef31eb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 would be good to note that
REDACT
messages should be returned as part of regularchathistory
requests too, as described in https://github.com/ircv3/ircv3-specifications/pull/524/files#r1213533699.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.
How does this interact with the
event-playback
cap?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.
As in, redacting JOIN/PART/... messages?
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 question is basically: are
REDACT
messages always returned by chathistory regardless ofevent-playback
?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 say yes. The motivation for
event-playback
is that it's hard to deal with channel state (membership, op list, ...) and not necessarily very useful, right? Here this shouldn't be an issue.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.
Right. I agree.
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 need to be specified in this spec?
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 is at odds with #437 (comment).
Clients have no way to differentiate between a
REDACT
returned implicitly by chathistory due to another message, or a standaloneREDACT
message,Standalone
REDACT
messages are important to return in the case of a mobile client synchronizing its history at regular intervals via chathistory. Example:PRIVMSG
is sent at 20:00PRIVMSG
via chathistoryREDACT
is sent at 20:30The server must return
REDACT
at that point, or else the client misses it.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 clients need to differentiate between the two?
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.
Per the linked discussion above, my clients check whether there is more history to fetch by counting the number of messages. If it's less than the limit, no more messages.
Hm, but now that I've written this, this would only cause the number of messages to exceed the limit, causing needless but harmless chathistory requests in the edge case where there are no more messages. So it should be all fine?
Does a standalone
REDACT
count towards the limit? I'd say yes. This draws a line between "standalone" and "context" messages.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.
didn't the linked discussion settle on the empty batch to signify the end?
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.
The discussion hasn't really settled AFAIU, and my clients are still doing the limit check.
Please note, there is an interleaved discussion about what should happen when a channel has chathistory disabled, and this one has settled on empty batches instead of a
FAIL
. But it's unrelated.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.
ugh, true
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.
There are two ways to fix this issue that I see:
@chathistory-context
tag to all "related"/"context" messages returned by chathistory: chathistory: add chathistory-context tag #526. Clients can ignore these when figuring out the selector for their next query.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 a resolution to this issue require changes to the redaction spec, or just to the chathistory spec? i.e. is this a blocker for merge/ratification?
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.
only the chathistory spec IMO
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.
Just spotted this other comment from @emersion that appears unaddressed https://github.com/ircv3/ircv3-specifications/pull/524/files#r1213547049
This PR has become a bit of a nightmare to review lol.
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.
How is the window communicated to clients?
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.
For now, it's not. It could be an informational mode though.
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's an easy thing to add later if needed. I don't believe it's worth blocking this PR because of this.
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.
Is this just saying that REDACT is allowed in chathistory batches? Are we expecting servers to keep track of whether or not clients have seen redacts?
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.
No, why?
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's just worded as though the server will decide whether or not to send the REDACT in chathistory depending on whether the client was present at the time. Could be reworded more simply I think.
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.
Clarification suggestion here: https://github.com/ircv3/ircv3-specifications/pull/524/files#r1500038542