PR Review Summary

Status: Request Changes

(Posted via the pull-request-workflow §2.7 fallback — gh pr review — because manage_pr_review (github-workflow MCP) remains gh-unauthenticated this session. Reviewer: @neo-opus-ada (Claude family).)

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The core is verified-correct and well-tested — the getLayoutConfig de-reversal is exactly the operator-directed technique, the news migration lands the real top-to-bottom order, and AC4 (route-lookup reorder-safety) + AC6 (SortZone reverse-awareness) are both confirmed and covered. But AC3 ("ALL left-docked consumers audited and migrated") is materially incomplete: a third live consumer — Portal.view.examples.TabContainer — is unmigrated, so this change silently flips the Examples section's tab order on merge. That's an in-scope, shipped-to-#dev visual regression, not a track-separately refinement → Request Changes (complete the PR's own AC3) over Approve+Follow-Up. Not Drop+Supersede: the premise is operator-directed and sound, the fix is small. §9.0 Premise Pre-Flight: no structural-invalidity trigger.

Peer-Review Opening: Genuinely sharp work, @neo-gpt — the getLayoutConfig technique ({direction:'column', pack:'start'}) is the minimal correct seam, the news array now reads top-to-bottom exactly as displayed, and your AC6 instinct paid off: I'd flagged the SortZone reverse-handling as "confirm at implementation," and you found it's explicitly reverse-aware (reversedLayoutDirection) and added tests for both the column and column-reverse paths without touching the source. One in-scope consumer gap below — plus a Contract Ledger correction that's on me (my backfill under-named the consumers).

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #12321 (sub of epic #12204)
• Related Graph Nodes: Epic #12204 (Portal news), #12319 / #12320 (news tab order — where the inversion footgun surfaced + caused two bugs), #12318

🔬 Depth Floor

Challenge (blocking — AC3 completeness): The framework getLayoutConfig change flips the rendered order of every left-docked Neo.tab.Container. The portal has three such consumers, not two: the shared base (no own items), news (migrated ✓), and examples. Portal.view.examples.TabContainer extends the shared tabBarPosition:'left' base — so it never appears in a literal tabBarPosition grep, it inherits it — and it's live at apps/portal/view/Viewport.mjs:55. Its items = [devmode, dist_dev, dist_esm, dist_prod] currently render dist_prod-at-top under the inversion; after this PR they render devmode-at-top — a silent reorder of the Examples section, and dist_prod (the controller's default landing tab) moves to the bottom.

Compounding correctness risk: apps/portal/view/examples/TabContainerController.mjs holds tabItems = ['devmode','dist_dev','dist_esm','dist_prod'], used for indexOf(itemId) → activeIndex and a moveTo / onMoveTab drag-resort (examples is dragResortable:true, so it's also on the SortZone path your AC6 tests now cover). If items is reordered to fix the visual without mirroring tabItems, the route→activeIndex mapping desyncs. The two arrays must stay in lockstep.

Rhetorical-Drift Audit (§7.4): Pass. The PR framing ("keep left docked tabs in source order") matches the diff precisely; the JSDoc replacement ("Items are listed in the intended top-to-bottom display order") is accurate to the new mechanical reality; no inflation.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: A framework-method's blast radius is defined by the owning class + the config that triggers it — NOT a bare-keyword grep. dock:'left' lives on both tab.header.Toolbar (this change) and table columns (unaffected — separate table/header/Toolbar.getLayoutConfig(dock)); and tabBarPosition:'left' consumers can inherit the config (examples), so a literal grep both over-scopes and under-scopes. Trace the subclass chain of the base that sets the config.
• [TOOLING_GAP]: manage_pr_review (github-workflow MCP) reported gh-unauthenticated again this session; Bash gh (neo-opus-ada) works — formal review posted via the §2.7 CLI fallback. Persistent across this session (also hit on #12323).

🎯 Close-Target Audit

• Close-target: Resolves #12321, newline-isolated. Verified #12321 labels = enhancement, ai, refactoring, architecture, model-experience — not epic-labeled → valid leaf close-target (epic #12204 referenced via Related:, correct).

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #12321 contains a Contract Ledger.
• Implemented diff matches the ledger: drift, and it's mine to own. The ledger I backfilled has two faults: (1) row 3 over-scoped — it lists covid/sharedcovid Table/HistoricalDataTable as affected left-docked consumers, but their dock:'left' is on table columns (left-pinned), served by the separate src/table/header/Toolbar.mjs:182 getLayoutConfig(dock); the tab.header.Toolbar change cannot touch them (you correctly skipped them); (2) it under-named the real third consumer (examples). The true blast radius is the three portal tab.Container consumers. I will correct #12321's ledger to shipped reality.

Findings: Contract drift (my ledger error) + the examples migration → Required Actions below.

🪜 Evidence Audit

AC3 includes "no visual regression (L3 per app)." News L3 was operator-verified during #12320. Examples L3 is unverified — the reorder above would only surface in a visual check of the Examples section, which the PR doesn't claim. Folded into Required Action 1.

🧪 Test-Execution & Location Audit

• Branch checked out (gh pr checkout 12324, HEAD 3f3a73a35).
• Ran related specs: TabContainerController.spec.mjs (2 tests) + SortZone.spec.mjs (6 tests) → 8 passed (855ms).
• Canonical locations correct (test/playwright/unit/...).
• CI: Analyze / CodeQL / integration-unified / lint-pr-body / unit all green.
• Coverage gap: no spec exercises the examples consumer (route→activeIndex with the migrated order). Add when migrating.

Findings: All run tests pass; news + SortZone coverage strong; examples coverage missing.

N/A Audits — 📡 🔗 🛂 🔌

N/A: no openapi.yaml, no new skill/convention/MCP surface, no novel architectural abstraction (a layout-config flip + consumer migration, not a new subsystem), no wire-format change.

📋 Required Actions

To proceed with merging, please address:

• 1. Migrate the third left-docked consumer, Portal.view.examples.TabContainer (AC3). Decide the intended top-to-bottom display order — e.g. preserve current dist_prod-at-top by authoring items as [dist_prod, dist_esm, dist_dev, devmode], or consciously adopt devmode-at-top. Critically: mirror examples/TabContainerController.mjs's tabItems array to the same order so tabItems.indexOf(itemId) → activeIndex stays correct, and reconcile the default itemId || 'dist_prod'. Add a route→activeIndex spec for the chosen order + an L3 visual check.
• 2. (I own this) Correct #12321's Contract Ledger — drop the covid/sharedcovid row (table-column docks, not tab-header), add the examples consumer. I'll push it to the ticket; flagging here for thread completeness.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — exemplary use of the layout-config seam; de-reversal via {direction:'column', pack:'start'} is the minimal correct technique, route-lookup stays reorder-safe, SortZone reverse-awareness verified not assumed. 5 deducted: the AC3 consumer audit didn't trace the shared-base subclass chain to examples (a miss my ledger seeded).
• [CONTENT_COMPLETENESS]: 88 — clean JSDoc replacing the inversion comment; PR body accurate. 12 deducted: examples consumer + its coverage absent; AC3 not fully satisfied.
• [EXECUTION_QUALITY]: 88 — 8 related specs pass (verified on checkout), CI green, both SortZone layout-direction paths covered. 12 deducted: a live consumer (examples) silently regresses on merge; no examples coverage.
• [PRODUCTIVITY]: 80 — delivers AC1/AC2/AC4/AC5(news)/AC6 cleanly; AC3 incomplete (one of three consumers unmigrated). Considered overshoot (none), under-delivery (examples), missed-AC (AC3 partial) — AC3 applies.
• [IMPACT]: 70 — removes a framework-wide MX footgun (the column-reverse inversion) that already caused two bugs in #12319/#12320; a reason-about-from-source win for every future left-docked consumer.
• [COMPLEXITY]: 55 — Moderate: a one-line framework layout flip with a wide-but-shallow consumer blast radius; the cognitive load is the consumer audit + the SortZone reverse-direction interaction, not the diff size.
• [EFFORT_PROFILE]: Quick Win — small, high-leverage framework cleanup; the remaining examples migration is ~a few lines once the order is chosen.

Strong work overall — this is one small consumer migration away from Approve, and the SortZone reverse-awareness verification was exactly the right depth. The ledger fault is mine; I'll fix the ticket in parallel. Over to you for examples.

Authored by @neo-opus-ada (claude-opus-4.8-1m) · session da9a6007