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.
9 issues found across 47 files
Confidence score: 3/5
src/ui/home.jsremoves the homepage’s only ad unit along with the editorial block, creating a likely monetization regression beyond the stated scope — preservegetAdSlotHTML("home", ...)unless the ad removal is intentional.src/routes/code-minifier.jsrenderscreateRelatedToolsSection(...)outside the page-width wrapper, allowing the grid to span the viewport and lose consistent horizontal padding — keep the section inside the wrapper.src/routes/css-gradient-generator.js,src/routes/color-converter.js,src/routes/image-converter.js,src/routes/unit-converter.js, andsrc/routes/mock-data-generator.jsretain emptypb-12wrappers, whilesrc/routes/oauth-debugger.jsretains an emptymt-8wrapper; these leave unintended vertical gaps — remove the empty containers.src/utils/ads.test.jsonly checks negative conditions in its all-tools loop, so failed, redirected, or empty pages can pass; add a positive assertion for each tool’s main control or a successful non-empty response.
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/image-converter.js">
<violation number="1" location="src/routes/image-converter.js:605">
P3: The removal of the educational section leaves behind an empty `<div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12">...</div>` wrapper that now renders nothing. Delete the empty div entirely instead of keeping it with the closing tag; `createRelatedToolsSection` already supplies its own container.</violation>
</file>
<file name="src/routes/oauth-debugger.js">
<violation number="1" location="src/routes/oauth-debugger.js:237">
P3: The removal left behind an empty `<div class="mt-8"></div>` that no longer wraps anything. Remove the leftover opening and closing div entirely; the related-tools section already sits outside it.</violation>
</file>
<file name="src/routes/unit-converter.js">
<violation number="1" location="src/routes/unit-converter.js:109">
P3: The educational-section removal leaves an empty wrapper div behind: `<div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12">` is now immediately closed by the added `</div>` and contains nothing, while the related-tools section sits outside it. This dead container adds an empty ~3rem (pb-12) gap and clutters the template. Remove both the wrapper div and its closing tag, leaving only `${createRelatedToolsSection(relatedToolsData)}`.</violation>
</file>
<file name="src/routes/mock-data-generator.js">
<violation number="1" location="src/routes/mock-data-generator.js:180">
P3: The educational section was removed but its wrapper div was left behind as an empty element. `<div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12"></div>` contributes an unintended pb-12 vertical gap and is dead markup. Delete the entire empty wrapper div (opening and closing tag) along with the section content.</violation>
</file>
<file name="src/routes/code-minifier.js">
<violation number="1" location="src/routes/code-minifier.js:143">
P2: The related-tools section is now rendered outside the page-width/padding wrapper, so its grid can span the full viewport and lose the horizontal padding used by the rest of the page. Keep `createRelatedToolsSection(relatedToolsData)` inside the `max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12` container, removing only the educational content.</violation>
</file>
<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/routes/color-converter.js">
<violation number="1" location="src/routes/color-converter.js:164">
P3: The removal leaves an empty `<div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12"></div>` wrapper between `</main>` and the related tools. An empty div with `pb-12` still renders 3rem (48px) of bottom padding, so a blank gap remains exactly where this PR intends to remove padding, and the wrapper is dead markup. Delete both the opening div (line 163) and this closing tag. Note `text-diff.js` and other routes reuse this wrapper for the related-tools section; here it wraps nothing.</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>
<file name="src/utils/ads.test.js">
<violation number="1" location="src/utils/ads.test.js:227">
P3: The all-tools loop only makes negative assertions, so an error, redirect, or empty page can pass without rendering tool content. Add a positive assertion for each page's main control or a successful, non-empty response.</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: The related-tools section is now rendered outside the page-width/padding wrapper, so its grid can span the full viewport and lose the horizontal padding used by the rest of the page. Keep createRelatedToolsSection(relatedToolsData) inside the max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12 container, removing only the educational content.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/code-minifier.js, line 143:
<comment>The related-tools section is now rendered outside the page-width/padding wrapper, so its grid can span the full viewport and lose the horizontal padding used by the rest of the page. Keep `createRelatedToolsSection(relatedToolsData)` inside the `max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12` container, removing only the educational content.</comment>
<file context>
@@ -141,33 +140,7 @@ function renderCodeMinifierPage(lang = DEFAULT_LANGUAGE) {
- currentLang,
- )}
- </div>
+ </div>
${createRelatedToolsSection(relatedToolsData)}
`;
</file context>
| 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>
| currentLang, | ||
| )} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
P3: The removal of the educational section leaves behind an empty <div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12">...</div> wrapper that now renders nothing. Delete the empty div entirely instead of keeping it with the closing tag; createRelatedToolsSection already supplies its own container.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/image-converter.js, line 605:
<comment>The removal of the educational section leaves behind an empty `<div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12">...</div>` wrapper that now renders nothing. Delete the empty div entirely instead of keeping it with the closing tag; `createRelatedToolsSection` already supplies its own container.</comment>
<file context>
@@ -603,33 +602,7 @@ function renderImageConverterPage(lang = DEFAULT_LANGUAGE) {
- currentLang,
- )}
- </div>
+ </div>
${createRelatedToolsSection(relatedToolsData)}
`;
</file context>
| currentLang, | ||
| )} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
P3: The removal left behind an empty <div class="mt-8"></div> that no longer wraps anything. Remove the leftover opening and closing div entirely; the related-tools section already sits outside it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/oauth-debugger.js, line 237:
<comment>The removal left behind an empty `<div class="mt-8"></div>` that no longer wraps anything. Remove the leftover opening and closing div entirely; the related-tools section already sits outside it.</comment>
<file context>
@@ -235,51 +234,7 @@ function renderOAuthDebuggerPage(lang = DEFAULT_LANGUAGE) {
- currentLang,
- )}
- </div>
+ </div>
${createRelatedToolsSection(relatedToolsData)}
</file context>
| currentLang, | ||
| )} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
P3: The educational-section removal leaves an empty wrapper div behind: <div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12"> is now immediately closed by the added </div> and contains nothing, while the related-tools section sits outside it. This dead container adds an empty ~3rem (pb-12) gap and clutters the template. Remove both the wrapper div and its closing tag, leaving only ${createRelatedToolsSection(relatedToolsData)}.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/unit-converter.js, line 109:
<comment>The educational-section removal leaves an empty wrapper div behind: `<div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12">` is now immediately closed by the added `</div>` and contains nothing, while the related-tools section sits outside it. This dead container adds an empty ~3rem (pb-12) gap and clutters the template. Remove both the wrapper div and its closing tag, leaving only `${createRelatedToolsSection(relatedToolsData)}`.</comment>
<file context>
@@ -107,33 +106,7 @@ function renderUnitConverterPage(lang = DEFAULT_LANGUAGE) {
- currentLang,
- )}
- </div>
+ </div>
${createRelatedToolsSection(relatedToolsData)}
`;
</file context>
| currentLang, | ||
| )} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
P3: The educational section was removed but its wrapper div was left behind as an empty element. <div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12"></div> contributes an unintended pb-12 vertical gap and is dead markup. Delete the entire empty wrapper div (opening and closing tag) along with the section content.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/mock-data-generator.js, line 180:
<comment>The educational section was removed but its wrapper div was left behind as an empty element. `<div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12"></div>` contributes an unintended pb-12 vertical gap and is dead markup. Delete the entire empty wrapper div (opening and closing tag) along with the section content.</comment>
<file context>
@@ -178,33 +177,7 @@ function renderMockDataPage(lang = DEFAULT_LANGUAGE) {
- currentLang,
- )}
- </div>
+ </div>
${createRelatedToolsSection(relatedToolsData)}
</file context>
| currentLang, | ||
| )} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
P3: The removal leaves an empty <div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12"></div> wrapper between </main> and the related tools. An empty div with pb-12 still renders 3rem (48px) of bottom padding, so a blank gap remains exactly where this PR intends to remove padding, and the wrapper is dead markup. Delete both the opening div (line 163) and this closing tag. Note text-diff.js and other routes reuse this wrapper for the related-tools section; here it wraps nothing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/routes/color-converter.js, line 164:
<comment>The removal leaves an empty `<div class="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 pb-12"></div>` wrapper between `</main>` and the related tools. An empty div with `pb-12` still renders 3rem (48px) of bottom padding, so a blank gap remains exactly where this PR intends to remove padding, and the wrapper is dead markup. Delete both the opening div (line 163) and this closing tag. Note `text-diff.js` and other routes reuse this wrapper for the related-tools section; here it wraps nothing.</comment>
<file context>
@@ -162,33 +161,7 @@ function renderColorConverterPage(lang = DEFAULT_LANGUAGE) {
- currentLang,
- )}
- </div>
+ </div>
${createRelatedToolsSection(relatedToolsData)}
`;
</file context>
| const html = await (await handler(new Request(url), url)).text(); | ||
| expect(html, id).toContain(`<h2>${heading}</h2>`); | ||
| expect(html.indexOf(`<h2>${heading}</h2>`), id).toBeLessThan(html.indexOf(before)); | ||
| expect(html, tool.id).not.toContain('data-section="educational"'); |
There was a problem hiding this comment.
P3: The all-tools loop only makes negative assertions, so an error, redirect, or empty page can pass without rendering tool content. Add a positive assertion for each page's main control or a successful, non-empty response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/ads.test.js, line 227:
<comment>The all-tools loop only makes negative assertions, so an error, redirect, or empty page can pass without rendering tool content. Add a positive assertion for each page's main control or a successful, non-empty response.</comment>
<file context>
@@ -208,32 +209,23 @@ describe("ad rendering", () => {
const html = await (await handler(new Request(url), url)).text();
- expect(html, id).toContain(`<h2>${heading}</h2>`);
- expect(html.indexOf(`<h2>${heading}</h2>`), id).toBeLessThan(html.indexOf(before));
+ expect(html, tool.id).not.toContain('data-section="educational"');
+ expect(html, tool.id).not.toContain(`tools.${tool.id}.edu.`);
}
</file context>
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.Rollout
Written for commit 11badf2. Summary will update on new commits.