Skip to content

Gates console.warn statements behind debug flag in Duck Player Native #1861

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Gates console.warn statements behind debug flag in Duck Player Native #1861

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions injected/src/features/duck-player-native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export class DuckPlayerNativeFeature extends ContentFeature {

const selectors = this.getFeatureSetting('selectors');
if (!selectors) {
console.warn('No selectors found. Check remote config. Feature will not be initialized.');
if (this.isDebug) console.warn('No selectors found. Check remote config. Feature will not be initialized.');
return;
}

Expand All @@ -64,7 +64,7 @@ export class DuckPlayerNativeFeature extends ContentFeature {
try {
initialSetup = await messages.initialSetup();
} catch (e) {
console.warn('Failed to get initial setup', e);
if (this.isDebug) console.warn('Failed to get initial setup', e);
return;
}

Expand Down Expand Up @@ -103,7 +103,7 @@ export class DuckPlayerNativeFeature extends ContentFeature {
break;
case 'UNKNOWN':
default:
console.warn('No known pageType');
logger.warn('No known pageType');
Copy link
Preview

Copilot AI Jul 29, 2025

Choose a reason for hiding this comment

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

This line uses logger.warn while the other warning statements in this PR use console.warn with debug checks. For consistency, this should either be if (this.isDebug) console.warn('No known pageType'); or all warning statements should use the logger approach with appropriate debug level configuration.

Suggested change
logger.warn('No known pageType');
if (this.isDebug) console.warn('No known pageType');

Copilot uses AI. Check for mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mgurgel following this advise, is there a reason not to switch to the logger you have? (assuming that is gated)

Copy link

Choose a reason for hiding this comment

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

Bug: Inconsistent Debug Gating for Warnings

Inconsistent gating of console.warn statements. While two warnings are gated with if (this.isDebug), a third is changed to logger.warn(). This is inconsistent with the PR's goal of debug-gating warnings. logger.warn() may not respect this.isDebug (e.g., if it uses env.isTestMode()), potentially logging warnings in non-debug environments. Furthermore, logger.warn() might not exist or behave as expected, as other logger calls in the function use logger.log().

Locations (1)
Fix in Cursor Fix in Web

}

if (this.currentPage) {
Expand Down
Loading