Skip to content

Amp ad free #468

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
nandap4790 wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Amp ad free #468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9483cb2
amp-ad-free: Include subscriber key when subscribe param is added
nandap4790 Jul 22, 2025
9e50ff7
Remove optional chaining
nandap4790 Jul 22, 2025
9233d73
chore(release): 7.34.5-amp-ad-free.0
nandap4790 Jul 22, 2025
7c01cbf
Push subscriber state to additionalconfig
nandap4790 Jul 22, 2025
82d2272
chore(release): 7.34.5-amp-ad-free.1
nandap4790 Jul 22, 2025
c3d7120
Upgrade amp
nandap4790 Jul 22, 2025
d84ef3d
chore(release): 7.34.5-amp-ad-free.2
nandap4790 Jul 22, 2025
82c7a7b
fix. add new beta for adfree-visual stories
VeenaYemmiganur Jul 25, 2025
ccbbcf2
chore(release): 7.34.5-amp-ad-free.3
VeenaYemmiganur Jul 25, 2025
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
12 changes: 6 additions & 6 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@quintype/framework",
"version": "7.34.4",
"version": "7.34.5-amp-ad-free.3",
"description": "Libraries to help build Quintype Node.js apps",
"main": "index.js",
"engines": {
Expand Down Expand Up @@ -33,7 +33,7 @@
"@ampproject/toolbox-optimizer": "2.8.3",
"@grpc/grpc-js": "^1.12.5",
"@jsdoc/salty": "^0.2.9",
"@quintype/amp": "^2.22.0",
"@quintype/amp": "2.22.1-ad-free-visual-stories.0",
"@quintype/backend": "^2.7.0",
"@quintype/components": "^3.5.0",
"@quintype/prerender-node": "^3.2.26",
Expand Down
4 changes: 4 additions & 0 deletions server/amp/handlers/story-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,10 @@ async function ampStoryPageHandler(
});
merge(mergedAdditionalConfig, additionalConfig, fetchedAdditionalConfig);
}
// the query appending happens in the worker, this is needed for any publisher who needs ad-free in amp story
if(req && req.query && req.query.subscriber === "true") {
merge(additionalConfig, { subscriber: true })
}
Comment on lines +148 to +150
Copy link

@coderabbitai coderabbitai bot Jul 22, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve condition safety and consider merge target consistency.

The current condition checking and merge operation have a few areas for improvement:

  1. As suggested by static analysis, use optional chaining for safer property access
  2. Consider whether additionalConfig is the correct merge target, given that mergedAdditionalConfig is used later in the function

Apply this diff to address the optional chaining and ensure consistent merge target:

-    if(req && req.query && req.query.subscriber === "true") {
-      merge(additionalConfig, { subscriber: true })
-    }
+    if(req?.query?.subscriber === "true") {
+      merge(mergedAdditionalConfig.length ? mergedAdditionalConfig : additionalConfig, { subscriber: true })
+    }

Alternatively, if additionalConfig is intentionally the target regardless of mergedAdditionalConfig:

-    if(req && req.query && req.query.subscriber === "true") {
+    if(req?.query?.subscriber === "true") {
       merge(additionalConfig, { subscriber: true })
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if(req && req.query && req.query.subscriber === "true") {
merge(additionalConfig, { subscriber: true })
}
if (req?.query?.subscriber === "true") {
merge(
mergedAdditionalConfig.length ? mergedAdditionalConfig : additionalConfig,
{ subscriber: true }
)
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 148-148: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🤖 Prompt for AI Agents 
In server/amp/handlers/story-page.js around lines 148 to 150, update the
condition to use optional chaining (e.g., req?.query?.subscriber) for safer
property access to avoid runtime errors if any part is undefined. Also, verify
if the merge target should be mergedAdditionalConfig instead of additionalConfig
to maintain consistency with later usage; if so, change the merge call to target
mergedAdditionalConfig. Adjust the code accordingly to ensure both safer checks
and consistent merge target.

const optimizeAmpHtml = get(domainSpecificOpts, ["featureConfig", "optimizeAmpHtml"], true);
const ampHtml = ampifyStory({
story,
Expand Down