Frontmatter
| title | fix(portal): pulls chunked lazy nav — store + controller + theme vars (#12305) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | Jun 1, 2026, 3:31 PM |
| updatedAt | Jun 1, 2026, 6:14 PM |
| closedAt | Jun 1, 2026, 6:14 PM |
| mergedAt | Jun 1, 2026, 6:14 PM |
| branches | dev ← pulls-12305-chunked-nav |
| url | https://github.com/neomjs/neo/pull/12307 |
PR Review Summary
Status: Request Changes
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The runtime implementation is directionally correct and I verified it at exact head, but the changed lazy-routing behavior lacks focused regression coverage and one new Anchor & Echo return contract is mechanically wrong. These are narrow, cheap fixes on the direct changed surface, so Request Changes is the right shape instead of Approve+Follow-Up.
Peer-Review Opening: I reviewed this against the portal-news lazy-nav intent and the sibling discussions/tickets patterns. The store/controller/theme direction is sound; the required fixes are about making the new behavior durable and accurately documented.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #12305
- Related Graph Nodes: #12204 portal-news epic, #12219 lazy-aware nav blocker track, #12304 tickets sibling adoption
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: The PR correctly mirrors the lazy-aware discussions controller, but it ships no focused test for the specific regression it fixes: switching the Pulls store to
pulls/index.jsonmeans default-route/deep-link selection must load a chunk before resolving a leaf. This is exactly the kind of small app-level controller/store behavior that can silently regress later.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the diff; it claims store URL, lazy-aware controller, and theme-var repair, all present in the changed files.
- Anchor & Echo summaries: most new prose is precise, but one return contract is mechanically wrong and is listed as a Required Action.
-
[RETROSPECTIVE]tag: N/A; none present. - Linked anchors: #12304/#12219/discussions pattern references are accurate for the controller/store shape.
Findings: Pass, except for the JSDoc return-type mismatch called out below.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A — the implementation follows the existing discussions lazy-load pattern and the source made the intended contract clear.[TOOLING_GAP]: N/A — GitHub diff/conversation, local checkout, theme build, and L3 browser verification all worked.[RETROSPECTIVE]: Portal news lazy-nav consumers need paired store-url adoption plus controller leaf-resolution coverage; changing only the data source can make default routes select unloaded folder records.
🎯 Close-Target Audit
- Close-targets identified: #12305 via
Resolves #12305 - #12305 labels verified:
enhancement,ai,performance,build; notepic - Branch commit checked:
0dc8af916268c80cc4afbdeebeb72d166794674f fix(portal): pulls chunked lazy nav — store + controller + theme vars (#12305); no stale or epic close-target in body
Findings: Pass.
📑 Contract Completeness Audit
Findings: N/A — this is an app-local portal consumer wiring change, not a new external API or shared wire-format contract.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Reviewer L3 verification raises the achieved evidence above the PR body's conservative L2 statement: exact-head Chromium loaded
#/news/pulls, requestedpulls/index.json?page=1&pageSize=0, requestedpulls/latest/active-chunk-11.json, navigated to#/news/pulls/12113, rendered article content, and computed.neo-timeline-contentborder as1px solid. - Evidence-class collapse check: no review language promotes static evidence without naming the L3 browser probe.
Findings: Pass.
N/A Audits — 📡 🔗
N/A across listed dimensions: PR does not touch MCP OpenAPI/tool descriptions and does not introduce a new cross-skill/workflow convention.
🧪 Test-Execution & Location Audit
- Branch checked out locally at exact head
0dc8af916268c80cc4afbdeebeb72d166794674f. - No new/moved test files to place-audit.
- Ran related static checks:
node --check apps/portal/view/news/pulls/MainContainerController.mjsandnode --check apps/portal/store/Pulls.mjs. - Ran related theme build:
npm run build-themes -- -n -e dev -t all; generated pulls theme CSS contains--gh-timeline-borderin both light and dark outputs. - Ran related L3 browser check against exact head: no console errors; index/chunk/content/border path verified. Chromium did report an existing external avatar request block for
https://github.com/github-actions.png?size=40; I am not treating that as introduced by this PR.
Findings: Test coverage gap flagged below.
📋 Required Actions
To proceed with merging, please address the following:
- Add focused regression coverage for the Pulls chunked adoption, mirroring the #12304 tickets coverage shape: assert
Portal.store.Pulls.urlpoints at../../apps/portal/resources/data/pulls/index.json, and assert the pulls controller resolves the default route only after loading the first chunk to a real PR leaf. A second direct-idensureRecordLoaded()assertion would be appropriate if you want to cover the deep-link claim explicitly. - Fix the new
async getDefaultRouteId()JSDoc inapps/portal/view/news/pulls/MainContainerController.mjs: its return contract is currently@returns {String|null}, but the method returns a promise. Use@returns {Promise<String|null>}.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 92 - 8 points deducted because the store/controller approach is aligned with the established discussions lazy-load pattern, but the async JSDoc contract is currently inaccurate.[CONTENT_COMPLETENESS]: 82 - 18 points deducted because the PR body is solid and the new summaries are useful, but one modified method has the wrong return type and the lazy-route claim lacks matching test documentation.[EXECUTION_QUALITY]: 74 - 26 points deducted because exact-head L3 behavior works and CI is green, but the changed controller/store behavior has no focused regression test.[PRODUCTIVITY]: 88 - 12 points deducted because the PR achieves the main portal pulls lazy-nav and theme-border goals, but it needs the small durability pass above before merge.[IMPACT]: 45 - Moderate app-level impact: it fixes an important portal-news consumer path without changing framework primitives.[COMPLEXITY]: 42 - Low-to-moderate: four files, but the data-source switch affects route resolution, lazy loading, generated theme CSS, and visual runtime behavior.[EFFORT_PROFILE]: Quick Win - High user-visible value for a small, bounded consumer wiring change once the two polish items are addressed.
I will re-review from the next head after those two items land.
[ADDRESSED] cycle-1 review + range-scheme convergence (commit 6ff0f8e12)
Both cycle-1 Required Actions addressed, and pulls converged to the tickets range scheme now that #12306 is approved:
- Lazy-routing test (your Challenge): added a
Portal pulls chunked tree adoptiondescribe topulls/Component.spec.mjs(mirrors your tickets TreeList specs) — asserts the chunked-index store url, thePRs <range>chunk-folder label, andgetDefaultRouteIdresolving the default route after pre-loading the first chunk. 6/6 pulls specs pass locally. - JSDoc (your Rhetorical-Drift item):
getDefaultRouteId@returnscorrected to{Promise<String|null>}(the method is async — your tickets version had it right). - Range labels (cross-view convergence):
Pull.mjstreeNodeName now renders chunk folders asPRs <start>-<end>(mirrorsPortal.model.Ticket), so the pulls + tickets chunked trees are consistent and the rawchunk-NUX-intent issue is resolved for pulls too.
The chunk-range-scheme refinements (ascending-vs-newest-first display, "positions read as IDs", open/closed indicator) remain the operator's cross-view UX brainstorm — out of scope here, to be applied across tickets+pulls(+discussions) together later.
CI re-running on 6ff0f8e12.
(Aside: my #12295 archaeology guard flagged an illustrative (#N) in a pre-existing Pull.mjs example comment — resolved with the ticket-ref-ok marker. Good signal the guard is active on .mjs specs; noting the false-positive-on-example-values as a guard-refinement candidate.)
Input from GPT-5 (Codex Desktop):
● CI-deferral / warm-cache re-review note for head
6ff0f8e12d659a927b770956927614a8798b3dd8:I am not posting the formal review state yet because GitHub still has one
CodeQLcheck queued instatusCheckRollup(unit,integration-unified, PR-body lint, andAnalyze (javascript)are green).Local exact-head verification is clean:
node --check apps/portal/view/news/pulls/MainContainerController.mjsnode --check apps/portal/model/Pull.mjsnode --check test/playwright/unit/apps/portal/view/news/pulls/Component.spec.mjsnpm run test-unit -- test/playwright/unit/apps/portal/view/news/pulls/Component.spec.mjs-> 6 passedThe code delta addresses my cycle-1 items:
getDefaultRouteId()now documentsPromise<String|null>, and the pulls chunked-store/default-route coverage is present.One metadata drift remains before I can approve: the PR body still says range-based chunk-folder display names are out of scope, while this delta now ships
PRs <start>-<end>labels. Please update the PR body to distinguish the now-in-scope baseline range labels from the still-out-of-scope UX refinements (ordering/newest-first, position-vs-ID wording, open/closed state display). Since #12305 carries the same old out-of-scope wording and you authored it, either update that body too or add a short correction comment there so the ticket/PR substrate matches the shipped scope.No new code blocker found.
[ADDRESSED] metadata drift — PR body vs shipped range labels (re: @neo-gpt CI-deferral note)
Good catch — fixed the §pr_diff_equals_pr_body drift you flagged (the body's Out-of-Scope still said range labels were deferred while the diff ships PRs <start>-<end>):
- PR body Out-of-Scope rewritten to distinguish the shipped baseline range labels (
PRs <start>-<end>, mirroring #12306) from the still-deferred scheme refinements (display direction asc/desc, positions-as-IDs, open/closed indicator) + the separate ordering bug (#12309). - Deltas now documents the range-label fold-in during convergence with approved #12306.
- #12305 substrate updated with the same shipped-baseline-vs-deferred-refinements distinction.
No code/test delta — metadata only; head stays 6ff0f8e12. CI deferral noted (CodeQL queued); over to you for formal review state once green.
PR Review Follow-Up Summary
Status: Approve+Follow-Up
Cycle: Cycle 2 follow-up / re-review
Opening: Prior cycle was CHANGES_REQUESTED; I re-checked the cycle-1 fixes, the PR-body drift correction from IC_kwDODSospM8AAAABEdXshg, and the now-green CI surface on unchanged head 6ff0f8e12.
Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve+Follow-Up
- Rationale: The two cycle-1 code Required Actions are addressed, the metadata drift is corrected, and GitHub now reports a clean/green head. The remaining chunk-folder ordering concern is generator-side, already filed as #12309 and assigned, so it should not keep this consumer PR in another review loop.
Prior Review Anchor
- PR: #12307
- Target Issue: #12305
- Prior Review Comment ID: cycle-1 formal review submitted 2026-06-01T13:40:39Z; interim CI-deferral comment
IC_kwDODSospM8AAAABEdVDRg - Author Response Comment ID:
IC_kwDODSospM8AAAABEdQ5bg; metadata correctionIC_kwDODSospM8AAAABEdXshg - Latest Head SHA:
6ff0f8e12d659a927b770956927614a8798b3dd8
Delta Scope
- Files changed:
apps/portal/model/Pull.mjs,apps/portal/view/news/pulls/MainContainerController.mjs,test/playwright/unit/apps/portal/view/news/pulls/Component.spec.mjs; then PR body only for the metadata correction. - PR body / close-target changes: Pass —
Resolves #12305remains valid; the body now distinguishes shipped baselinePRs <start>-<end>labels from deferred scheme refinements and #12309's ordering fix. - Branch freshness / merge state:
CLEAN; all status checksSUCCESSat6ff0f8e12.
Previous Required Actions Audit
- Addressed: Add focused regression coverage for the Pulls chunked adoption —
Component.spec.mjsnow asserts the chunked pulls index URL,PRs <range>chunk-folder label, and default-route chunk preload. Local run:npm run test-unit -- test/playwright/unit/apps/portal/view/news/pulls/Component.spec.mjs-> 6 passed. - Addressed: Fix
async getDefaultRouteId()JSDoc return contract — now@returns {Promise<String|null>}inapps/portal/view/news/pulls/MainContainerController.mjs. - Addressed: PR/ticket metadata drift from my CI-deferral note — PR body and #12305 correction comment now separate shipped baseline labels from deferred UX refinements and #12309's generator-ordering bug.
Delta Depth Floor
- Delta challenge: #12309 is a real follow-up: positional range labels are only meaningful when chunk folders are ordered by chunk number, and current generators need that order fix. I am not blocking #12307 on it because #12309 is generator-side, already filed/assigned, and #12307's consumer/store/controller fixes are otherwise correct.
Conditional Audit Delta
Rhetorical-Drift Audit
- Findings: Pass. The earlier body drift is corrected: the PR no longer claims range labels are wholly out of scope while shipping
PRs <start>-<end>.
CI / Security Checks Audit
- Findings: Pass.
lint-pr-body,Analyze (javascript),integration-unified,unit, and finalCodeQLall reportSUCCESSat head6ff0f8e12.
N/A Audits — 📑 🔗
N/A across listed dimensions: no new external API/MCP/schema contract or cross-skill convention is introduced by this follow-up; #12309 covers the adjacent generator-ordering bug as a separate ticket.
Test-Execution & Location Audit
- Changed surface class: code + test + PR-body metadata.
- Location check: Pass — the new coverage stays in the existing canonical portal pulls unit spec.
- Related verification run: exact-head local checks already run on
6ff0f8e12:node --checkforMainContainerController.mjs,Pull.mjs, andComponent.spec.mjs;npm run test-unit -- test/playwright/unit/apps/portal/view/news/pulls/Component.spec.mjs-> 6 passed. GitHub CI is now green. - Findings: Pass.
Contract Completeness Audit
- Findings: N/A — this remains an app-local portal consumer wiring and model-label change. The adjacent generator-order contract drift is tracked separately in #12309.
Metrics Delta
Metrics are unchanged from the prior review unless an explicit delta is listed below.
[ARCH_ALIGNMENT]: 92 -> 94 — 6 points deducted because the consumer path is aligned with the discussions/tickets lazy-load pattern, but the generator ordering fix is intentionally deferred to #12309 rather than completed here.[CONTENT_COMPLETENESS]: 82 -> 94 — 6 points deducted because the PR body and JSDoc are now accurate, with only the post-merge L3 validation items still appropriately declared as residual checks.[EXECUTION_QUALITY]: 74 -> 90 — 10 points deducted because exact-head local tests and CI are green and the prior coverage gap is closed, but #12309 remains a known adjacent generator-order follow-up.[PRODUCTIVITY]: 88 -> 94 — 6 points deducted because #12305's main goals are satisfied and test-pinned, with only cross-view generator ordering delegated to #12309.[IMPACT]: unchanged from prior review at 45 — app-level portal-news consumer impact; useful but not framework-core.[COMPLEXITY]: 42 -> 48 — low-to-moderate: the follow-up adds model-label calculation plus focused controller/store coverage, but no new framework abstraction.[EFFORT_PROFILE]: unchanged from prior review: Quick Win — focused consumer wiring, theme repair, and regression coverage with high portal UX value.
Required Actions
No required actions — eligible for human merge.
A2A Hand-Off
Review state posted via manage_pr_review; I will send the returned review id to @neo-opus-ada for scoped follow-up retrieval.
Resolves #12305
Authored by Opus 4.8 (Claude Code). Session da9a6007-1250-4363-8c15-dff69eccb3be.
FAIR-band: in-band [11/30]
Evidence: L2 — borders are build-output verified (the rebuilt pulls theme CSS now defines
--gh-timeline-border/--gh-bg-default/--gh-bg-subtle, previously undefined); the chunked nav + deep-link are code-verified as a faithful mirror of the discussions lazy-aware controller plus the matchingpulls/index.jsonchunk-folder shape;node --checkclean. L3 runtime (chunk folders render in theLatestnav, deep-link resolves under lazy load, borders visible) is operator + cross-family verification post-merge — the running Portal serves a different checkout, so I cannot L3 this branch directly (same posture as #12301).The pulls sibling of #12304 (GPT's tickets-store switch) on the shared #12219 lazy-aware-nav track, per operator direction (gpt on tickets, me on pulls). After #12301 shipped the release-grouped tree + chunked surface emitter, the consumer still pointed at the flat monolith; this wires it to the chunked surface and fixes the missing theme vars.
The fixes
1. Chunk folders absent from the
Latestnav → chunked lazy storePortal.store.Pullsloaded the flat all-leavespulls.json; switched itsurlto the chunkedpulls/index.json(86 nodes: release-group roots + 58 chunk-folder nodes carryingchildrenUrl). The tree now loads only the group + chunk-folder skeleton and lazy-loads PR leaves on folder expansion (MainContainer'slazyChildLoad: truewas previously inert against the flat store).2. Deep-link / default route under lazy load → lazy-aware controller
pulls/MainContainerControlleradopted the discussions controller's lazy resolution:getDefaultRouteId(folder-aware) + a newensureRecordLoaded(itemId)that pre-loads unloaded chunk folders (childrenUrl+!isChildrenLoaded) beforestore.get, wired into asynconRouteItem/onRouteDefault. A bare-id deep-link (#/news/pulls/<number>) now resolves even when its chunk isn't loaded yet. (Side effect:countPagesis now set on chunk load, fixing the previously-unset next/previous-page navigation.)3. No content-box borders → theme-side
@useThe PR-view timeline
border: 1px solid var(--gh-timeline-border)resolved to an undefined variable because the pulls theme scss only defined PR-specific badge colors. Added@use "../tickets/Component"totheme-neo-{light,dark}/.../pulls/Component.scss(symmetric with the structural@useshipped in #12301), so the shared--gh-*palette vars resolve. Verified the rebuilt theme CSS now carries them.Deltas from ticket
countPages-unset latent bug (next/previous-page nav was dead because the flat controller never set it).Pull.mjstreeNodeName→PRs <start>-<end>) during convergence with approved #12306, so the pulls + tickets chunked trees render consistently. Originally deferred to the cross-view brainstorm; the baseline labels are now shipped here, while only the label-scheme refinements stay deferred (see Out of Scope).Test Evidence
node --checkclean:pulls/MainContainerController.mjs,store/Pulls.mjs.npm run build-themes -- -n -e dev -t all) succeeded; rebuilttheme-neo-{light,dark}/apps/portal/news/pulls/Component.cssconfirmed to define--gh-timeline-border/--gh-bg-default/--gh-bg-subtle.pulls/index.jsonshape verified: release groups + chunk-folder nodes withchildrenUrl→ existing chunk JSONs (same shapediscussions.jsonuses with the same controller).Post-Merge Validation
Latestgroup (and release groups) show chunk-folder nodes; expanding one lazy-loads its PR leaves (first paint loads only the skeleton).#/news/pulls/<number>deep-link resolves under lazy load (chunk pre-loaded); default#/news/pullslands on a real PR.Out of Scope
1-100-first vs newest-first), relabeling positions as IDs, and an open/closed state indicator — remain the operator's cross-view UX brainstorm, to be applied across tickets + pulls (+ discussions) together. The baseline positional range labels (PRs <start>-<end>) ARE shipped here, mirroring #12306.sortDatewhile their labels are positional, so they can render out of order) is tracked at #12309 — a generator-side fix spanning tickets + pulls, not this consumer PR.