feat(taskflow)!: Per state UI rendering order preservation - #246
chathura-de-silva wants to merge 9 commits into
Conversation
…nt cases out of zone assemblers test file
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe zone view renderer now returns an ordered list of enriched components. State layouts determine section order, and current-state actions determine which handles are included. The assembler forwards the rendered view directly, and documentation describes the configuration and frontend format. ChangesZone view rendering
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The view format changes as intended. No demonstrated issue blocks merging, though the consuming frontend’s compatibility should be confirmed during rollout. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new view format requires consumers to understand an ordered list. Existing action and visibility controls appear preserved, but compatibility with deployed consumers and the update sequence are not established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 60.87% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 5 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…nt cases out of zone assemblers test file
18eb18e to
86d8bd5
Compare
There was a problem hiding this comment.
Don't we need a nsw-srilanka consumer that consumes the ordered list and retires the #504 ZONE_ORDER hack?
Gap: Trader portal still treats view as a map (types.ts, TraderZoneLayout, task handler tests). If we merge this PR and bump core without a portal update, task screens break.
What nsw-srilanka should do:
- Accept
viewas an ordered list (use each entry’sid). - Render top to bottom in that list order.
- Remove the temporary
ZONE_ORDERhack from #504 (status_awaiting/review_historyhardcoding)
It's already done and tested with local core. Waiting to this to merge so I could point and test it with the remote core repo. |
…ttps://github.com/chathura-de-silva/NSW-Core into feat/236-per-state-ui-rendering-order-preservation
Summary
Previously
render.jsons had no way to specify or preserve an order of UI sections. It was workarounded in the frontend side. This introduces a mechanism to specify astatedependant order and preserve it so that frontend can directly render it in the given order.Type of Change
Changes Made
Rendered view (breaking)
viewis now an ordered list of entries instead of a map keyed by section. Position in the list is render order; there is no separate order field.id: the section's key in render.json (unique within a task, stable across states), for tracking entries on the client.title: the section's title, which was previously dropped in the pipeline (omitted when the section has none).Ordering rules
orderreturns its sections sorted by key(alphabetically).$ref(not of the form#/layouts/<name>, or naming an undefined layout) fails with an error instead of silently falling back.Implementation
TaskRenderer.Rendernow builds the whole view: it projects sections viauiprojector, resolves the state's layout, orders the sections, and merges in each section's legal handles.ZoneViewAssembleris now a thin wrapper: it callsRenderwith the task's state, data and the caller's claims, and wraps the result inZoneView.The second parse of the config and
mergeViewwere removed.TaskManager.GetTaskRenderInfoand the zoneview assembler) go throughRender, so both return the new list shape.legalCommandsandfilterLegalHandlesmoved from `zone_as unchanged.Tests
renderer_test.gocovering: layout ordering per state, shared layouts, the key-sorted fallback, unlisted sections going last,$referrors, titles and legal handles, handle declaration order, payload shape per projector, omitted empty titles, the empty config, plus unit tests forresolveLayoutandorderSlots.zone_assembler_test.gotrimmed to the assembler's own concerns. Tests now identify sections byid.Docs
template-reference.md: new "Thezoneviewrenderer" section documentingsections,layouts,states.orderand the ordering rules.frontend-guide.md: new "Thezoneviewconvention" section and its fields.The breaking label in the "Rendered view" section matches the reads view as a map, like the trader-app's zone renderer,needs updating alongside this PR.
Testing
Checklist
Related Issues
Closes #236
Related to OpenNSW/nsw-srilanka#512
Related to https://github.com/LSFLK/lsf-govtech-tnsw/issues/150
Screenshots/Demo
(If applicable, add screenshots or GIFs to help explain your changes)
Additional Notes
Note
Breaking Change
Trader-Portals Task view will fail (on a core version bump without the relavant feature PR there)
OpenNSW/nsw-srilanka#504
Summary by CodeRabbit