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

Send snapshot on join/invite #631

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mgcm
Copy link
Contributor

@mgcm mgcm commented Nov 12, 2024

✔️ Checklist

  • A changeset describing the change and affected packages (more info).
  • Added or updated documentation.
  • Tests for new functionality and regression tests for bug fixes.
  • Screenshots or videos attached (for UI changes).
  • All your commits have a Signed-off-by line in the message (more info).

@mgcm mgcm force-pushed the nic/feat/NEO-925-snapshot-on-invite-or-join branch from ab90a22 to 68bc427 Compare November 12, 2024 01:45
Copy link

changeset-bot bot commented Nov 12, 2024

🦋 Changeset detected

Latest commit: 8d6628d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@nordeck/matrix-neoboard-widget Minor
@nordeck/matrix-neoboard-react-sdk Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mgcm mgcm force-pushed the nic/feat/NEO-925-snapshot-on-invite-or-join branch 3 times, most recently from 5168458 to 1f6e508 Compare November 13, 2024 17:11
@mgcm mgcm marked this pull request as ready for review November 13, 2024 17:12
@mgcm mgcm requested a review from a team as a code owner November 13, 2024 17:12
@mgcm mgcm force-pushed the nic/feat/NEO-925-snapshot-on-invite-or-join branch 2 times, most recently from 68fc068 to 99b93dc Compare November 13, 2024 21:20
@weeman1337 weeman1337 self-requested a review November 14, 2024 09:11
packages/react-sdk/src/state/synchronizedDocumentImpl.ts Outdated Show resolved Hide resolved
packages/react-sdk/src/state/synchronizedDocumentImpl.ts Outdated Show resolved Hide resolved
packages/react-sdk/src/state/whiteboardInstanceImpl.ts Outdated Show resolved Hide resolved
);
}

function getMembershipFilter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a Redux selector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be, I guess. Would this be by adding a new query to the roomMemberApi that filters membership events based on type and only return joins+invites? I can see how that would be optimal for rooms with high membership volume but it's also 10x more code... Maybe there's a simpler way I'm missing?

@mgcm mgcm force-pushed the nic/feat/NEO-925-snapshot-on-invite-or-join branch from 99b93dc to f3e0c69 Compare November 18, 2024 15:53
@mgcm mgcm force-pushed the nic/feat/NEO-925-snapshot-on-invite-or-join branch from db0f9c0 to 3c3652c Compare November 19, 2024 18:04
@mgcm mgcm requested a review from weeman1337 November 20, 2024 00:48
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

This needs an in-depth review. Requesting change as a blocker notice.

export const App = ({ layoutProps }: AppProps) => {
const { t } = useTranslation('neoboard');
const { value, loading } = useOwnedWhiteboard();
const whiteboardManager = useWhiteboardManager();
const ownUserId = useWidgetApi().widgetParameters.userId;
const whiteboardInstance = useActiveWhiteboardInstance();
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks NeoBoard standalone. Can you have a look?

validator: isValidWhiteboardDocumentSnapshot,
});

const handlePersist = useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if the entire logic is split out, e.g. into a hook or function. Otherwise App becomes really cluttered.

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