Skip to content

Additional fixes and polish#246

Merged
AllTerrainDeveloper merged 3 commits into
trunkfrom
additional-fixes-and-polish
May 19, 2026
Merged

Additional fixes and polish#246
AllTerrainDeveloper merged 3 commits into
trunkfrom
additional-fixes-and-polish

Conversation

@AllTerrainDeveloper

@AllTerrainDeveloper AllTerrainDeveloper commented May 19, 2026 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Collaborator

Closes #164 and #241

Open WordPress Playground Preview

@AllTerrainDeveloper

AllTerrainDeveloper commented May 19, 2026

Copy link
Copy Markdown
Collaborator Author

@claude, we are polishing the plugin, can you review these changes?

@claude

claude Bot commented May 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @AllTerrainDeveloper's task in 2m 54s —— View job

Code Review

  • Understand the request
  • Gather context — read changed files and diff
  • Review each changed file
  • Post final review

Overview

PR closes #164 (wpd-table selection rebuild / focus loss) and #241 (hide-if-no-customize items dropped from dock). Both fixes are correct. A handful of style issues noted below against the project's AGENTS.md guidelines.

src/ui/components/wpd-table/wpd-table.ts + test — GH#164

The approach is sound. The root cause was that every selection mutator called _schedulePaint() → _paintBody() → tbody.replaceChildren(), which destroyed the focused checkbox and transiently emptied the scroll container (triggering scroll-anchor snap-back). The new _syncSelectionDom method surgically updates only the affected rows in-place without touching the rest of the DOM.

Key implementation points that check out:

  • data-row-id is stamped in _buildBodyRow (line 1320) alongside the existing data-row-index, giving _syncSelectionDom a direct lookup key.
  • The needle Set uses String(id) which correctly matches the dataset.rowId round-trip for both number and string IDs.
  • Single-mode select() captures previouslySelected before clearing _selection, so both the newly selected and the previously selected row get their DOM updated (line 559–565). ✅
  • The selection setter (line 405–407) still calls _schedulePaint() — this is correct, since bulk external replacement warrants a full repaint.
  • Filtered-out rows are silently skipped because they have no <tr> in the DOM; when filters are cleared and _paintBody reruns, _buildBodyRow reads from _selection to correctly apply is-selected. ✅
  • The test uses reference equality (toBe) to verify DOM node identity — not just value equality — so a destroy+recreate would correctly fail it. ✅

Style issue — issue references in code comments. AGENTS.md says:

"Don't reference the current task, fix, or callers... since those belong in the PR description and rot as the codebase evolves."

The following inline references should be removed:

  • select() JSDoc: "(GH#164)" — keep the semantic explanation of why a rebuild is bad, drop the issue number.
  • _syncSelectionDom JSDoc: "both symptoms in GH#164" and "The rebuild path used to flow through..." — the multi-line history of the old code path is verbose; a single line (// avoids tbody rebuild; see select() for context) is enough.
  • _buildBodyRow comment block on tr.dataset.rowId = String(id) — three lines to explain a one-line stamp; the {@link _syncSelectionDom} jsdoc link on the property set suffices.
  • Test's opening multi-line comment block: "// Repro: in the native Plugins window..." — this is the old "describe the task in the test file" pattern the guidelines prohibit.

Fix comment verbosity →

includes/core/payload.php + PHPUnit test — GH#241

The fix is semantically correct. hide-if-no-customize means "visible by default; hide only when <body class='no-customize-support'> is present." The earlier code read it as "always hide," incorrectly dropping the only path to Additional CSS from the Appearance submenu. Removing the continue is the right fix.

Same issue-reference problem in comments:

  • payload.php line 111: "// Note on \hide-if-no-customize` (issue I cannot edit my website #241):"— drop the(issue I cannot edit my website #241)` reference. The semantic explanation of the class semantics is valuable and should stay.
  • desktopModeBuildDockItems.php docblock: "Issue #241 regression —" should be dropped from the first line.

The test rename from test_skips_hide_if_no_customize_submenu_items → test_preserves_hide_if_no_customize_submenu_items accurately describes the new expected behavior. ✅

Fix PHP comment references →

src/desktop-files/wallpaper-menu.ts + src/desktop.ts + test

Clean removal. The wallpapers item was wired as openWallpapers: () => openOsSettings() — a duplicate of the already-present os-settings entry. Removing it simplifies the menu without losing any functionality.

All three removal sites are consistent (interface, implementation, labels object), and the test stubs and assertions are updated to match. The docs/files-on-desktop.md table has the wallpapers row removed. ✅

assets/css/chromeless.css

Correct selector addition. .edit-post-fullscreen-mode-close__view-mode-toggle is a BEM sub-element of the same fullscreen-close button family. Hiding it alongside .edit-post-fullscreen-mode-close is the right grouping. No doc update needed — this extends an existing suppression rule rather than adding a new compat shim.

Summary

File Verdict
wpd-table.ts / test ✅ Logic correct — comment cleanup recommended
payload.php / PHPUnit ✅ Fix correct — drop issue # from comments
wallpaper-menu.ts / desktop.ts / test ✅ Clean, no concerns
chromeless.css ✅ No concerns

The two fixes are solid. The main polish item before merge is stripping the task/issue number references from inline comments (AGENTS.md: "those belong in the PR description and rot as the codebase evolves").

@github-actions

github-actions Bot commented May 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

✅ WordPress Plugin Check Report

✅ Status: Passed

📊 Report

All checks passed! No errors or warnings found.

🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

@AllTerrainDeveloper AllTerrainDeveloper merged commit ae7f507 into trunk May 19, 2026
5 checks passed
@AllTerrainDeveloper AllTerrainDeveloper deleted the additional-fixes-and-polish branch May 19, 2026 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

While selecting some of the plugins below the fold with their checkboxes, plugins screen is jump snapping scroll to the top

1 participant

@AllTerrainDeveloper