Clear the four abap2ui5lint findings: port 500/501, keep the wizard branch, explain 474 - #822
Merged
Merged
Conversation
…h, explain 474 check-abap2UI5 was red on two frozen-view-builder warnings and carried two control-state-lost-on-rebuild hints. 500 and 501 move off z2ui5_cl_xml_view onto z2ui5_cl_ui5_view_builder. The frozen builder meant nothing about either view was checked - no control, no property, no binding, no render - and the port immediately found what that was hiding: the Save button was `type="Success"`, which is @SInCE 1.73 on a corpus that targets 1.71, so it rendered as a default button on the oldest system the samples promise to run on. It is `Accept` now. The lifecycle goes with the view: the manual `check_initialized` / `mv_init` flags become check_on_init( ), both apps gain the check_on_navigated( ) branch they were missing (500 read the edited table back but never redisplayed it), and the `client->get( )-...` reads become get_event( ), get_event_arg( ) and get_app_prev( ). Both classes now pass the property gate and the render gate. 202 keeps the wizard branch it picked. setNextStep drives an ASSOCIATION, so no binding can carry it, and it was issued only from the event handler - after any later view_display( ) (a navigation back, a draft restore) the rebuilt WizardStep had no next step while the app still believed it was on the chosen branch. The choice is class state now, and view_display( ) re-issues the call, which is what makes it survive the rebuild. 474 is the case the rule cannot decide. The wire is real - the URL validator is live control state a rebuild destroys - but every open of the popover goes through popover_open( ), which re-installs it immediately before openBy( ), so there is no window in which the popover is visible without a validator. That is stated in the source and the rule is disabled for that one statement with the reason, rather than left to the next reader to re-derive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PbKRCwvGA1dqWixknfAWFj
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes align with the stated goal (clearing the linter findings) and only a minor chain-layout nit was found.
Pull request overview
This PR resolves check-abap2UI5 findings by migrating affected samples from the frozen XML builder to z2ui5_cl_ui5_view_builder, adjusting lifecycle handling/control-state persistence where needed, and updating the generated CI badges to reflect the clean linter run.
Changes:
- Port
z2ui5_cl_smp_app_500andz2ui5_cl_smp_app_501fromz2ui5_cl_xml_viewtoz2ui5_cl_ui5_view_builder, and align lifecycle handling withcheck_on_*. - Persist and re-apply the Wizard branching choice in
z2ui5_cl_smp_app_202after redisplay/rebuilds. - Document and locally suppress the
control-state-lost-on-rebuildlinter hint inz2ui5_cl_smp_app_474, and refresh abap2UI5-linter badge outputs.
File summaries
| File | Description |
|---|---|
| src/01/z2ui5_cl_smp_app_474.clas.abap | Adds rationale and a localized linter suppression around re-installed popover URL handler state. |
| src/01/z2ui5_cl_smp_app_202.clas.abap | Persists the selected Wizard branch and re-issues setNextStep after view rebuilds. |
| src/00/98/z2ui5_cl_smp_app_501.clas.abap | Rebuilds the popup UI with the view builder and switches to check_on_* lifecycle/event APIs. |
| src/00/98/z2ui5_cl_smp_app_500.clas.abap | Rebuilds the main table UI with the view builder and restores correct redisplay after navigating back from the popup. |
| .github/badges/check-abap2ui5.json | Updates badge to reflect passing linter status and rule count. |
| .github/badges/abap2ui5.json | Updates badge counts for checked apps/views/controls after the ports. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+123
to
+127
| page->ele( `footer` | ||
| )->ele( `OverflowToolbar` | ||
| )->tag( `ToolbarSpacer` | ||
|
|
||
| )->tag( `Button` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
check-abap2UI5was red. Run 33603262445 reported twofrozen-view-builderwarnings (which fail the build) and twocontrol-state-lost-on-rebuildhints (which do not). All four are gone; the pinned linter now reportsSuccess! No findings detected.500 and 501 — off the frozen builder
Both moved from
z2ui5_cl_xml_viewtoz2ui5_cl_ui5_view_builder. The point of that warning is that nothing about either view was being checked — no control, no property, no binding, no render — and the port immediately found what it was hiding:type="Success"on the Save button is@since 1.73, on a corpus that targets 1.71. On the oldest system these samples promise to run on, it rendered as a plain default button. It isAcceptnow.on_after_popup( )fetched the rows from 501 on every roundtrip whilerender_main( )only ran on the very first init, so the returning user saw the old table. Thecheck_on_navigated( )branch fixes that.The lifecycle moved with the view: the manual
check_initialized/mv_initflags becomecheck_on_init( ), and theclient->get( )-…reads becomeget_event( ),get_event_arg( )andget_app_prev( ). Both classes now pass the property gate and the render gate — the corpus goes from 146 to 148 checked apps and 169 to 171 rendered views.202 — the wizard keeps the branch it was given
setNextStepdrives an association, so no binding can carry it, and it was issued only from the event handler. After any laterview_display( )— a navigation back, a draft restore — the rebuiltWizardStephad no next step while the app still believed it was on the chosen branch. The choice is class state now andview_display( )re-issues the call.Worth recording: writing
nextStepinto the XML the rebuild produces anyway also fixes the behaviour, and is arguably the tidier ABAP — but the rule cannot see a statically written attribute as a re-issue and keeps reporting. The documented remedy is used instead, which is also the better fit for a sample whose subject is the imperative frontend call.474 — the case the rule cannot decide
The finding is correct on its own terms: the URL validator is live control state a rebuild destroys, and
popover_open( )is not on the display path. It is still not a defect here — every open goes through that method, which re-installs the validator immediately beforeopenBy( ), so there is no window in which the popover is visible without one. That reasoning is now in the source, and the rule is disabled for that one statement with the reason attached, rather than left for the next reader to re-derive.Checks
Run locally on this branch:
npm run check:abap2ui5(pinned 0.6.1) —Success! No findings detected., 148 files, 0 failingnpm run lintandnpm run check:cloud— abaplint, 0 issues, 309 filescheck:app-rules,check:keywords,check:strip,check:prose,check:family-nav,check:launchpad,check:catalogue,check:overview,check:docs-links,check:pin— all greenThe two corpus badges are regenerated (146 → 148 apps, 169 → 171 views, 2,162 → 2,202 controls).
🤖 Generated with Claude Code
https://claude.ai/code/session_01PbKRCwvGA1dqWixknfAWFj
Generated by Claude Code