Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
kdoberst
added
the
do-not-merge/work-in-progress
Amber reviewStatus: Complete |
![hypershell-delivery[bot]](https://avatars.githubusercontent.com/u/327175833?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
hypershell-delivery
Bot
added
the
amber/changes-requested
ea13039 to
1c5ea58
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
hypershell-delivery
Bot
removed
the
amber/changes-requested
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
kdoberst
removed
the
do-not-merge/work-in-progress
ac48432 to
ff62f30
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dcf2474 to
8bd90be
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Verdict
The prior Major (provision-gateway design-review diverging from the enforced parity surface) is now resolved: the Mockups/ story renders the shell-wrapped page surface, so the approved design and the locked reference match. The swap left ProvisionGatewayMockup and its showLocalDevelopment branch unreferenced, which is the one new (Minor) nit.
Findings
Dead code left behind after the provision-gateway story swap. The fix pointed Mockups/Gateways/Provision gateway at ProvisionGatewayParityMockup (provision-gateway.stories.tsx:3-7), which correctly unifies the design-review and parity surfaces. But ProvisionGatewayMockup (provision-gateway.tsx:25-27) is now exported and referenced nowhere, so the roles of the two mockups simply flipped - the previously-unused ParityMockup is now live and ProvisionGatewayMockup is now the dead export. Along with it, the showLocalDevelopment branch in ProvisionGatewayForm (provision-gateway.tsx:17-22) is only reachable through that dead function, and the LocalDevelopment story was deleted, so the local-kind design-review variant is no longer rendered by any story. Either delete ProvisionGatewayMockup and the now-unreachable showLocalDevelopment path, or re-attach the local-development variant to a live story so it stays reviewable.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Previous concerns
- Header region requires literal
variant="default"- Addressed.gateway-list-page-surface.tsx:10-14andprovision-gateway-page-surface.tsx:14-18writevariant="default"on thedata-page-region="header"<PageSection>, matching the check atvisual_parity_gate.py:161. - No
--mockup-viewtarget satisfies both shell-free and region checks - Addressed. The region-bearing JSX lives in the shell-freegateway-list-page-surface.tsx/provision-gateway-page-surface.tsx(no shell imports), whileMockupShell/MockupTemplateusage lives ingateway-list.tsx/provision-gateway.tsx. - Parity stories omit the mandated
MockupParityFrame- Addressed.gateway-list.parity.stories.tsx:9-13andprovision-gateway.parity.stories.tsx:9-13wrap the surface in<MockupParityFrame>viarender. - Viewport-only screenshots miss below-the-fold parity - Addressed.
visual_parity_gate.py:293passes--full-pageto the capture. - Provision-gateway
Mockups/design-review story diverges from the enforced parity surface - Addressed.provision-gateway.stories.tsx:3-7now rendersProvisionGatewayParityMockup, which wrapsProvisionGatewayPageSurfaceinMockupShell(provision-gateway.tsx:29-31), so the approved design and the enforced reference are the same surface, as they already were forgateway-list. (New residual dead code tracked in Findings above.) - Prior Minor - shared
HealthStatuscaller passes noappearance- Still present.gateway-list.tsx:153renders<HealthStatus label={gateway.status} />with noappearance, so every row falls back to the defaultgood(greensuccess) treatment regardless of the status string. Harmless with the all-Healthyfixtures, but the shared component's warning/danger paths are never exercised by the canonical example.
Findings Summary (ordered by severity, highest first)
- [Minor]
ProvisionGatewayMockupis now unreferenced dead code after the story swap, and itsshowLocalDevelopmentbranch / deletedLocalDevelopmentstory leave the local-kind variant unrendered by any story - Example Consistency (provision-gateway.tsx:25-27,provision-gateway.tsx:17-22). - [Minor] Shared
HealthStatuscaller passes noappearance, so every row renders the greengoodtreatment regardless of status - Example Consistency (gateway-list.tsx:153).
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
N/A (no Go changes) |
| No em dashes in text files | Pass |
| Conventional commit messages | Pass |
| Examples consistent with documented conventions | Pass (prior divergence resolved) |
| No dead/unreferenced example code | Fail (ProvisionGatewayMockup) |
| PatternFly reuse via shared components | Pass |
| Tooling passes against its own shipped fixtures | Pass |
| return <Form aria-label="Provision gateway" isWidthLimited><FormGroup isRequired label="Gateway name" fieldId="gateway-name"><TextInput id="gateway-name" isRequired onChange={(_event, value) => setName(value)} value={name} validated={showValidationErrors && !name ? "error" : "default"} />{showValidationErrors ? <FormHelperText><HelperText><HelperTextItem screenReaderText="error status" variant="error">This field is required.</HelperTextItem></HelperText></FormHelperText> : null}</FormGroup>{!showLocalDevelopment ? <FormGroup isRequired label="Network access" fieldId="network-access"><Gallery hasGutter minWidths={{ default: "250px", md: "300px" }} role="radiogroup"><Choice name="placement" value="public" selected={!localKind && network === "public"} title="Public" description="Accessible through a public endpoint." onChoose={() => { setLocalKind(false); setNetwork("public"); }} /><Choice name="placement" value="vpn" selected={!localKind && network === "vpn"} title="VPN" description="For gateways that need to reach GitLab and other internal Red Hat services." descriptionLabel="Red Hat VPN required" onChoose={() => { setLocalKind(false); setNetwork("vpn"); setProvider("aws"); }} /></Gallery></FormGroup> : <FormGroup isRequired label="Local development" fieldId="local-development"><Gallery hasGutter minWidths={{ default: "250px", md: "300px" }} role="radiogroup"><Choice name="placement" value="local-kind" selected={localKind} title="Use local-kind" description="Use the local Kind cluster for development." onChoose={() => setLocalKind(true)} /></Gallery></FormGroup>}{network ? <FormGroup isRequired label="Cloud provider" fieldId="cloud-provider"><Gallery hasGutter minWidths={{ default: "250px", md: "300px" }} role="radiogroup"><Choice name="provider" value="aws" selected={provider === "aws"} title="Amazon Web Services" description="For workloads that rely heavily on AWS services or data." icon={awsLogo} iconPadding="1rem 0" onChoose={() => setProvider("aws")} /><Choice name="provider" value="ibm" selected={provider === "ibm"} title="IBM Cloud" description="The default home for gateways. General-purpose workloads with no special network or data needs." icon={ibmCloudLogo} isDisabled={network === "vpn"} onChoose={() => setProvider("ibm")} /></Gallery></FormGroup> : null}<p>{showLocalDevelopment ? "The local Kind cluster is used for development." : "A matching managed cluster is selected at random for the chosen network and provider."}</p><ActionGroup><Button type="submit" variant="primary">Provision gateway</Button><Button type="button" variant="link">Cancel</Button></ActionGroup></Form>; | ||
| } | ||
|
|
||
| export function ProvisionGatewayMockup({ showValidationErrors = false, showLocalDevelopment = false }: { showValidationErrors?: boolean; showLocalDevelopment?: boolean }) { |
There was a problem hiding this comment.
[Minor] Dead code after the story swap. With provision-gateway.stories.tsx now rendering ProvisionGatewayParityMockup, this ProvisionGatewayMockup export is referenced nowhere - the dead/live roles of the two mockups simply flipped. Its showLocalDevelopment branch in ProvisionGatewayForm (lines 17-22) is only reachable through this function, and the LocalDevelopment story was removed, so the local-kind variant is no longer rendered by any story. Delete ProvisionGatewayMockup and the now-unreachable showLocalDevelopment path, or re-attach the local-development variant to a live story.
This branch has not been deployed
1 participant
Summary
Breaking change? No
New to this repo
Tracking
#469
Specs and other PRs
Related specs and dependent PRs
Components changed
Changes with impact
Verification
Screenshots / video
Miro diagram of flow
Video demo
Questions for discussion
Details
Technical details (for humans and bots)
The first commit adds the repository-level agent guidance, the
ui-build-gateskill, deterministic Storybook harness guidance, a production/story data-boundary checker, and the visual parity gate script. The gate builds both Storybooks, verifies the required parity stories, captures production and mockup images at1440x900and390x844, checks that requests stay within local Storybook assets, and records hashes and review history invisual-parity.json.The second commit extracts page-owned surfaces from the gateway mockups and adds
Parity/Gateways/Gateway listandParity/Gateways/Provision gatewaystories. This lets reviewers compare page structure, backgrounds, spacing, content, and responsive behavior without treating shared masthead or application shell chrome as page-owned UI.The gate also requires the production route to use its real data source while the Storybook story supplies deterministic props or adapters. A failed visual comparison must include a structured difference ledger and a subsequent source change before another capture can run.