Skip to content

Fix some error log lines that lead to confusion#3679

Merged
toger5 merged 3 commits intolivekitfrom
toger5/fix-unnecassary-error-logs
Jan 19, 2026
Merged

Fix some error log lines that lead to confusion#3679
toger5 merged 3 commits intolivekitfrom
toger5/fix-unnecassary-error-logs

Conversation

@toger5
Copy link
Copy Markdown
Contributor

@toger5 toger5 commented Jan 16, 2026

In the past those log lines often were referenced for issues but they are no real issues.
Either they are just deprecated code running or expected.

@toger5
Copy link
Copy Markdown
Contributor Author

toger5 commented Jan 16, 2026

This PR increases the overall coverage and already introduces a not super helpful test (for a log line on error)

So I will merge it with failing CI once approved!

@toger5 toger5 mentioned this pull request Jan 16, 2026
1 task
[vm],
);

useEffect(() => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't match the PR description. This will lead to confusion in the future when looking at history

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.

Do you want to split this into two PR's? Or update the title.
I think the PR can stay as one cleanup PR. That removes outdated code that causes unwanted logs and also improve logs.

Sth like:
Remove outdated layout widget api messages (reduce and improve error logs)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is this outdated? This is now directly inside EC?
That means that ElementWidgetActions.TileLayout should be deprecated?

});
});

it("logs if callIntent cannot be updated", async () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is indeed testing the code, but looks like the code is talking to us? why are we trying to update call intent if not connected? I also find it strange that the intent is mutable :/ causes traffic everytime you mute/unmute camera

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 also played with checking if we are connected here. But it was somewhat redundant. if we already have a check in the js-sdk if we are connected.

@toger5 toger5 requested a review from BillCarsonFr January 19, 2026 08:36
Copy link
Copy Markdown
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

It looks to me that this PR is doing 2 different things?

  • One is removing deprecated(?) widget actions
  • The other about error logs confusion(?)

I'd prefer we merge it split in 2 PRs

[vm],
);

useEffect(() => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is this outdated? This is now directly inside EC?
That means that ElementWidgetActions.TileLayout should be deprecated?

@toger5
Copy link
Copy Markdown
Contributor Author

toger5 commented Jan 19, 2026

I'd prefer we merge it

split it into two PRs?

@toger5
Copy link
Copy Markdown
Contributor Author

toger5 commented Jan 19, 2026

why is this outdated

It was designed in a way where the layout switch buttons were part of the element web header bar.
Now (as you said) its inside the iframe. So the action is not needed anymore.

@BillCarsonFr
Copy link
Copy Markdown
Member

I'd prefer we merge it

split it into two PRs?

YES

In the past those log lines often were referenced for issues but they
are no real issues.
Either they are just deprecated code running or expected.
@toger5 toger5 force-pushed the toger5/fix-unnecassary-error-logs branch from 1e6114e to a102b3f Compare January 19, 2026 11:20
@toger5
Copy link
Copy Markdown
Contributor Author

toger5 commented Jan 19, 2026

@BillCarsonFr I have split this:

[vm],
);

useEffect(() => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if (ex instanceof MatrixError && ex.httpStatus === 404) {
// Expected, this is an unstable endpoint and it's not required.
logger.debug("Backend does not provide any RTC transports", ex);
logger.debug(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This a bit strange for a log no? looks lie it is a user facing string.
Maybe keep the old log and add that as a comment?

Copy link
Copy Markdown
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

OK, to merge.
It looks like the line markes as not covered by codecov are actually covered by this test Creating preferred transport based?
That is strange, ok to force merge

@toger5 toger5 merged commit a72aae0 into livekit Jan 19, 2026
32 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR-Developer-Experience Release note category. A PR that does not change EC but improves working with the repository.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants