Skip to content

LEAN-3843 - #346

Open
fwaisi wants to merge 1 commit into
masterfrom
LEAN-3843
Open

fwaisi wants to merge 1 commit into
masterfrom
LEAN-3843

Conversation

@fwaisi

@fwaisi fwaisi commented Jan 14, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@fwaisi
fwaisi requested review from Copilot and mbetamony and removed request for Copilot January 14, 2026 10:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR removes a broadcast call for document steps after processing a document update, likely as part of implementing a different broadcasting strategy or removing unnecessary real-time notifications.

Changes:

  • Removed broadcastSteps call in the document update flow

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -163,7 +163,6 @@ export class DocumentRoute extends BaseRoute {
payload,
user
)

Copilot AI Jan 14, 2026

Copy link

Choose a reason for hiding this comment

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

Removing the broadcastSteps call may break real-time synchronization for collaborative editing. If this broadcast is no longer needed, ensure that step broadcasting is handled elsewhere in the application flow, or verify that real-time updates are intentionally being disabled.

Suggested change
)
)
await this.documentController.broadcastSteps(projectID, manuscriptID, payload, user)

Copilot uses AI. Check for mistakes.
Comment on lines -166 to 168
this.documentController.broadcastSteps(manuscriptID, result)
res.status(StatusCodes.OK).send(result)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we getting rid of this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because the worker will be responsible to broadcast the steps.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Even locally? And on dev environments? We currently have the worker only on dev2.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don’t think we need this locally. I think we should enable it across all environments.

As a fallback, we could implement an alternative approach, but I need to figure out how to support that in the API repo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was the fallback, so we don't have to run the worker locally. We do need a way to to send steps back to the FE from the BE, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The new implementation keeps the POST request as a fallback, allowing the FE to send steps to the BE and receive them back without broadcasting.

However, it’s not clear how this acts as a fallback. With the current implementation, it seems to introduce a redundant step.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We send steps back to the FE. This is needed if multiple users are editing the same document.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, now the worker will be responsible to handle this.

But I think we can keep it as a fallback and make it working only when the user send the steps by post request.

here is the worker pr: https://github.com/atypon/manuscripts-cf-worker/pull/5

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