Repository navigation
Conversation
Keep tool controls, related links and useful reference sections; remove unreachable educational and homepage catalog padding together. Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 47 files
Confidence score: 3/5
src/ui/home.jsremoves the homepage’s only ad unit along with the editorial block, which could reduce homepage ad inventory beyond the PR’s stated scope — preserve the ad slot if its removal is not intended.src/routes/css-gradient-generator.jsleaves an empty wrapper withpb-12, creating an unnecessary ~48px gap below the main content — remove the empty wrapper or its padding.
Not reviewed (too large): src/i18n/fr.js (~362 lines), src/i18n/ja.js (~362 lines), src/i18n/vi.js (~362 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/routes/css-gradient-generator.js">
<violation number="1" location="src/routes/css-gradient-generator.js:185">
P2: Removing the educational content left an empty `<div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12">...</div>` wrapper behind. With no content, its `pb-12` padding still renders a ~48px blank gap below the main panel, which contradicts the PR's goal of removing the padding. Delete the whole wrapper div (both the opening line and the added `</div>`), matching refactored routes like cidr-calculator.js that place createRelatedToolsSection directly.</violation>
</file>
<file name="src/ui/home.js">
<violation number="1" location="src/ui/home.js:6">
P2: This diff removes more than the editorial padding: it also deletes the homepage's only ad unit, `getAdSlotHTML("home", ...)`, which was inside the removed `home-below-catalog` block. The PR description scopes this change to "removed educational sections" and doesn't mention dropping the ad unit, so if that wasn't intentional it's a silent monetization regression on the site's highest-traffic page. The ad machinery still treats the home path as ad-enabled: `pageAllowsAds("/")` and `pageAllowsAdScript("/")` return true, `createPageTemplate` still emits the AdSense head script via `getAdSenseScript(pagePath)`, and `slotKeyForPath("/")`/`ALLOW_SLOT_KEYS` still map "home" to an active slot (src/utils/ads.js lines 9, 65-70, 202). The result is a homepage that loads adsbygoogle.js but has no `ins.adsbygoogle` unit to fill, and a dead "home" slot key in config. Confirm the ad removal was intended, or restore the unit; if removing it is deliberate, call it out in the PR description and consider also dropping the now-dead "home" slot handling.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| currentLang, | ||
| )} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
P2: Removing the educational content left an empty <div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12">...</div> wrapper behind. With no content, its pb-12 padding still renders a ~48px blank gap below the main panel, which contradicts the PR's goal of removing the padding. Delete the whole wrapper div (both the opening line and the added </div>), matching refactored routes like cidr-calculator.js that place createRelatedToolsSection directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/css-gradient-generator.js, line 185:
<comment>Removing the educational content left an empty `<div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12">...</div>` wrapper behind. With no content, its `pb-12` padding still renders a ~48px blank gap below the main panel, which contradicts the PR's goal of removing the padding. Delete the whole wrapper div (both the opening line and the added `</div>`), matching refactored routes like cidr-calculator.js that place createRelatedToolsSection directly.</comment>
<file context>
@@ -183,33 +182,7 @@ function renderCSSGradientPage(lang = DEFAULT_LANGUAGE) {
- currentLang,
- )}
- </div>
+ </div>
${createRelatedToolsSection(relatedToolsData)}
</file context>
|
|
||
| import { respondHTML } from "../utils/respond.js"; | ||
| import { createPageTemplate, getAdSlotHTML, t } from "../utils/common-ui.js"; | ||
| import { createPageTemplate, t } from "../utils/common-ui.js"; |
There was a problem hiding this comment.
P2: This diff removes more than the editorial padding: it also deletes the homepage's only ad unit, getAdSlotHTML("home", ...), which was inside the removed home-below-catalog block. The PR description scopes this change to "removed educational sections" and doesn't mention dropping the ad unit, so if that wasn't intentional it's a silent monetization regression on the site's highest-traffic page. The ad machinery still treats the home path as ad-enabled: pageAllowsAds("/") and pageAllowsAdScript("/") return true, createPageTemplate still emits the AdSense head script via getAdSenseScript(pagePath), and slotKeyForPath("/")/ALLOW_SLOT_KEYS still map "home" to an active slot (src/utils/ads.js lines 9, 65-70, 202). The result is a homepage that loads adsbygoogle.js but has no ins.adsbygoogle unit to fill, and a dead "home" slot key in config. Confirm the ad removal was intended, or restore the unit; if removing it is deliberate, call it out in the PR description and consider also dropping the now-dead "home" slot handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ui/home.js, line 6:
<comment>This diff removes more than the editorial padding: it also deletes the homepage's only ad unit, `getAdSlotHTML("home", ...)`, which was inside the removed `home-below-catalog` block. The PR description scopes this change to "removed educational sections" and doesn't mention dropping the ad unit, so if that wasn't intentional it's a silent monetization regression on the site's highest-traffic page. The ad machinery still treats the home path as ad-enabled: `pageAllowsAds("/")` and `pageAllowsAdScript("/")` return true, `createPageTemplate` still emits the AdSense head script via `getAdSenseScript(pagePath)`, and `slotKeyForPath("/")`/`ALLOW_SLOT_KEYS` still map "home" to an active slot (src/utils/ads.js lines 9, 65-70, 202). The result is a homepage that loads adsbygoogle.js but has no `ins.adsbygoogle` unit to fill, and a dead "home" slot key in config. Confirm the ad removal was intended, or restore the unit; if removing it is deliberate, call it out in the PR description and consider also dropping the now-dead "home" slot handling.</comment>
<file context>
@@ -3,7 +3,7 @@
import { respondHTML } from "../utils/respond.js";
-import { createPageTemplate, getAdSlotHTML, t } from "../utils/common-ui.js";
+import { createPageTemplate, t } from "../utils/common-ui.js";
import { getToolsForEnvironment, CATEGORIES } from "../utils/tool-registry.js";
import {
</file context>
Six routes kept an empty max-w-7xl pb-12 wrapper while rendering Related Tools outside any width container, so the grid ran edge to edge (0-1440px desktop, 0-390px mobile) with an extra blank 48px gap. Move the section into the wrapper, matching case-converter and the other routes, and drop oauth-debugger's empty mt-8 div. The all-tools content test now asserts a 200 HTML response with the page shell, <main>, and exactly one <h1> before its negative checks, so an error or empty page cannot pass.
There was a problem hiding this comment.
0 issues found across 8 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
- run the content-locale generator --check in the unit suite so stale output fails CI - keep ?lang on FAQ answer links; drop unreachable localized FAQ_CONTENT arrays - security keeps its detailed, localized response text; contact keeps the generic one - drop the About count fallback that duplicated data-public-tool-count - keep the historical changelog entry free of the live tool count - remove / from manual-unit inventory to match #972 and the Terms copy - render-level test binds legal ad-scope markup to getAdPolicySnapshot() - docs: replace the untracked harness reference with a reproducible step
1 participant
Summary
refactor(content): remove generic tool and home padding. This is logical change 3/11 in the dependency-ordered product-audit release stack.
Refs #969
What changed
11badf2276ae92720651992bbef49a1a2006a087; validated tree:96df8aa5399002623dad6f20c72593f78c491fc7.Why
Keep tools and useful references prominent instead of repeating generic educational padding.
Verification
bun run buildpassed on this exact candidate tree.npm test -- --maxWorkers=2passed on this exact candidate tree.48 registered routes returned 200 without removed educational sections; home search/results/empty/clear/query restoration passed.
Final combined tree additionally passed 801 unit tests and all 293 Playwright tests with retries disabled, plus all 48 primary tool workflows at desktop and mobile. The exploratory Color Converter exact-HEX boundary remains a documented pre-existing defect; its runtime is unchanged by this stack.
Risk & rollback
Summary by cubic
Removes the generic educational sections (About / How to Use / Pro Tips) from 30+ tool pages and the editorial and flagship blurb blocks from the home page. Tool pages now render only the tool header, controls, related links, and reference content such as cheatsheets; the home page shows the catalog without padding below it. Refs #969.
Refactors
createEducationalSection()andcreateVisibleArticle()fromsrc/utils/content-ui.js.oauth-debugger's emptymt-8div.Rollout
Written for commit c529bfa. Summary will update on new commits.