Fix room state being updated with old (now overwritten) state and emitting for those updates. #4242
Conversation
…tting for those updates.
toger5
force-pushed
the
toger5/wrong-room-state
branch
from
daae9ca to
035f222
Compare
toger5
force-pushed
the
toger5/wrong-room-state
branch
from
330643c to
6299af2
Compare
Add configuration for toStartOfTimeline
toger5
force-pushed
the
toger5/wrong-room-state
branch
from
6299af2 to
5a35e3b
Compare
|
I added tests to see what this actually does. |
toger5
force-pushed
the
toger5/wrong-room-state
branch
from
bd1afb4 to
d469eb0
Compare
src/models/room-state.ts
Outdated
| @@ -419,13 +428,26 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap> | |||
| this.setBeacon(event); | |||
| } | |||
|
|
|||
| const lastStateEvent = this.getStateEventMatching(event); | |||
| const lastStateEv = this.getStateEventMatching(event); | |||
There was a problem hiding this comment.
Why did this get renamed to something less clear?
There was a problem hiding this comment.
Otherwise, we had a lot more linewraps that made I judged as harder to read overall.
There was a problem hiding this comment.
That's only because on L435 and L436 you're using the array syntax to set variables which is really redundant. It'd be clearer as 4 separate instantiations.
src/models/room-state.ts
Outdated
| /** Whether the event is inserted at the start of the timeline | ||
| * or at the end. This is relevant for doing a sanity check: | ||
| * "Is the event id of the added event the same as the replaces_state id of the current event" | ||
| * We need to do this because sync sometimes feeds previous state events. | ||
| * If set to true the sanity check is inverted | ||
| * "Is the event id of the current event the same as the replaces_state id of the added event" | ||
| */ |
There was a problem hiding this comment.
I don't think this makes sense, as a RoomState is only ever in one direction. This sounds like if toStartOfTimeline=false then you should always drop the state event as they're never live. It should be a property on a RoomState if required
There was a problem hiding this comment.
That is a good observation. The state itself might be the right location to store if it's a start state or an end state.
It should never change and also not be relevant outside the state object it seems logical to belong to the state.
I have trouble understanding the exact definition of start state anyway however:
If we have a state that is only set at the beginning of the room, events from a time-line in a specific interval cannot be enough to compute the start state because the start state needs all state events BEFORE the start of the time-line but the time-line only includes events after the "start".
This is slightly off topic but i still wanted to being it up. Feel free to DM me about this or point me to where i can lookup how start state is defined.
There was a problem hiding this comment.
as I understand it, it is the state at the start of the timeline, and it walks backwards as you backpaginate and load older events which undo the more recent state events.
There was a problem hiding this comment.
That would also be how i understand start state. But this is not what we are doing.
We use the content to set the state keys. What we would need to do is set the state with the previous_content field.
| = start of timeline
|| = end of timeline
--- = time steps
XV> = state event changing the state (the content of the state event is the current state from that time on (X is an arbitrary label for the state event (f.ex. a member event) X is the type and V is the value
X0>-----|X1>----X2>----||
Here we have an example where the state is changed with the first event in the timeline. But the start imo is before that event. So the state at the start is X0.
This becomes more clear when adding another type:
X0>--A0>--|A1>-A2>-X1>--||
At the start of the timeline we have X0 A0 but definitly not X1 which would be the first X type state event.
If we use the previous_content we have <XV as the previous content.
<X0>--<A0>--|<A1>-<A2>-<X1>--||
Then <A1 & <X1 is the same as X0> & A0> and the correct start state.
There was a problem hiding this comment.
I suggest raising this to the wider team as the code & intent predates me
There was a problem hiding this comment.
sure. Sorry for escalating this but I always found the startState to be arguably the correct name for that variable the way its implemented and this seemed to be relevant for this discussion. Somewhat relevant, you comment is just about where to store start/end.
What I do like about making it part of the setStateEvents property is that is makes it testable in a more isolated env.
There was a problem hiding this comment.
To get back to the topic what do you think about the more encapsulated testability with the current approach?
who else do we need to unlock this? Could you pls invite? |
|
My outstanding comments need dealing with, e.g. property of room state and random variable name change |
|
@t3chguy
did not get missed,
implies you still prefer making it part of the state object although it allows the encapsulated testing? |
toger5
force-pushed
the
toger5/wrong-room-state
branch
from
7faee46 to
8ebf04a
Compare
|
@t3chguy I updated the variable names back to what they were. And noticed the test can be done equally simple. |
* WIP support for state_after * Fix sliding sync sdk / embedded tests * Allow both state & state_after to be undefined Since it must have allowed state to be undefined previously: the test had it as such. * Fix limited sync handling * Need to use state_after being undefined if state can be undefined anyway * Make sliding sync sdk tests pass * Remove deprecated interfaces & backwards-compat code * Remove useless assignment * Use updates unstable prefix * Clarify docs * Remove additional semi-backwards compatible overload * Update unstable prefixes * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Fix test Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Add test for MSC4222 behaviour Signed-off-by: Michael Telatynski <[email protected]> * Improve coverage Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Fix tests Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Tidy Signed-off-by: Michael Telatynski <[email protected]> * Add comments to explain why things work as they are. * Fix sync accumulator for state_after sync handling Signed-off-by: Michael Telatynski <[email protected]> * Add tests Signed-off-by: Michael Telatynski <[email protected]> * Revert "Fix room state being updated with old (now overwritten) state and emitting for those updates. (#4242)" This reverts commit 957329b. * Fix Sync Accumulator toJSON putting start timeline state in state_after field Signed-off-by: Michael Telatynski <[email protected]> * Update tests Signed-off-by: Michael Telatynski <[email protected]> * Add test case Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> --------- Signed-off-by: Michael Telatynski <[email protected]> Co-authored-by: Hugh Nimmo-Smith <[email protected]> Co-authored-by: Michael Telatynski <[email protected]> Co-authored-by: Timo <[email protected]>
This is a workaround at best and mostly serves the purpose to show that this issue exist and to have a starting point to look into the underlying problem.
Checklist
public/exportedsymbols have accurate TSDoc documentation.