Skip to content

fix: recognize frontend feature loaders - #205

Closed
hopehadfield wants to merge 1 commit into
redhat-developer:mainfrom
hopehadfield:fe-feature-loader-patch
Closed

hopehadfield wants to merge 1 commit into
redhat-developer:mainfrom
hopehadfield:fe-feature-loader-patch

Conversation

@hopehadfield

Copy link
Copy Markdown
Member

Description

Patch @backstage/cli-node@0.3.4 so FrontendFeatureLoader entry points are recognized in generated feature metadata and Module Federation exposure detection. This allows the TechDocs addons dynamic-plugin /alpha entry point to be packaged as a supported frontend feature while the RHDH 2.1 branch still consumes the older Backstage release.

Test Plan

  • yarn tsc
  • Exported the TechDocs addons plugin and verified ./alpha is detected as @backstage/FrontendFeatureLoader
  • Verified the generated manifest exposes . and alpha

Checklist

  • Tests and type checking completed
  • Documentation updated
  • Screenshots attached

Signed-off-by: Hope Hadfield <hhadfiel@redhat.com>
@sonarqubecloud

Copy link
Copy Markdown

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 15, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:41 PM UTC · Completed 2:56 PM UTC

Commit: af13b12 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $2.58

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 15, 2026
@fullsend-ai-review

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Small dependency-only change (3 files, 51 lines) adding a yarn patch for @backstage/cli-node with no protected paths or CI impact; elevated Tier 2 churn on package.json and yarn.lock reflects expected background noise on those shared files.

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Medium

  • [logic-error] .yarn/patches/@backstage-cli-node-npm-0.3.4-b80a6a686e.patch:20 — The patch updates dist/monorepo/PackageGraph.cjs.js (CJS runtime) and dist/index.d.ts (type declarations) to add "@backstage/FrontendFeatureLoader", but contains no hunk for dist/monorepo/PackageGraph.js, which would be the ESM counterpart in a Rollup dual-build. If that file exists in the published @backstage/cli-node@0.3.4 tarball and is not patched, any ESM consumer that calls import { packageFeatureType } from '@backstage/cli-node' at runtime will still see a 4-element array without FrontendFeatureLoader, while TypeScript compilation succeeds because dist/index.d.ts is already patched — masking the gap.
    Remediation: Unpack the @backstage/cli-node@0.3.4 tarball (e.g. npm pack @backstage/cli-node@0.3.4 && tar -tzf backstage-cli-node-0.3.4.tgz | grep PackageGraph) and confirm whether dist/monorepo/PackageGraph.js exists. If it does, add a third hunk to the patch file targeting that file with the same "@backstage/FrontendFeatureLoader" addition to the packageFeatureType array.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

+++ b/dist/monorepo/PackageGraph.cjs.js
@@ -14,7 +14,8 @@ const packageFeatureType = [
"@backstage/BackendFeature",
"@backstage/BackstagePlugin",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] logic-error

The patch updates dist/monorepo/PackageGraph.cjs.js (CJS runtime) and dist/index.d.ts (type declarations) to add "@backstage/FrontendFeatureLoader", but contains no hunk for dist/monorepo/PackageGraph.js, which would be the ESM counterpart in a Rollup dual-build. If that file exists in the published @backstage/cli-node@0.3.4 tarball, ESM consumers will still see a 4-element packageFeatureType array without FrontendFeatureLoader at runtime while TypeScript compilation succeeds, masking the gap.

Suggested fix: Unpack the @backstage/cli-node@0.3.4 tarball and check for dist/monorepo/PackageGraph.js. If it exists, add a third hunk to the patch file targeting that file with the same "@backstage/FrontendFeatureLoader" addition to the packageFeatureType array.

@fullsend-ai-retro

fullsend-ai-retro Bot commented Sep 15, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 3:55 PM UTC · Completed 4:03 PM UTC

Commit: af13b12 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.91

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #205 — fix: recognize frontend feature loaders

Timeline

  1. 14:40 UTC — Human author hopehadfield opened PR #205 to patch @backstage/cli-node@0.3.4, adding FrontendFeatureLoader support. Three files changed: a new yarn patch file, package.json, and yarn.lock.
  2. 14:41–14:56 UTC — Review agent ran (Sonnet, high effort, $2.58). Found one medium-severity logic-error about the patch covering CJS and type declarations but potentially missing the ESM counterpart (PackageGraph.js). Two other findings were correctly downgraded by the challenger process. Submitted CHANGES_REQUESTED.
  3. CI failures — All 9 Verify Plugin Export jobs failed because package.json uses Yarn's patch: protocol for the @backstage/cli-node dependency, which npm install -g (used in the Verify workflow at line 97) cannot resolve.
  4. PR closed without merging.

What went well

  • The review agent's ESM hunk finding was legitimate and well-framed — conditional on whether the ESM file exists, with a clear verification step.
  • The challenger process correctly filtered two lower-impact findings, keeping signal-to-noise high.
  • Cost was reasonable ($2.58 for a high-effort review of a 3-file, 51-line diff).

Key gap: review missed the CI-breaking incompatibility

The review agent flagged a speculative ESM gap but missed the definite CI-breaking issue: the Yarn patch: protocol in package.json dependencies is incompatible with npm install -g in the verify-plugin-export.yaml workflow. All 9 Verify jobs failed immediately. This is a higher-impact finding because it's a guaranteed breakage (not conditional), affects CI directly, and would block merging.

The root cause is that AGENTS.md has no mention of the Verify Plugin Export workflow, the npm install -g installation step, or the constraint that package.json dependencies must use npm-compatible version specifiers.

Retro dispatch efficiency

This retro ran on a closed, unmerged human-authored PR where the only agent interaction was a single review. This pattern has limited retro value and is already covered by existing upstream issues: fullsend-ai/fullsend#2471 (skip retro for unmerged human PRs with only review-agent interaction) and fullsend-ai/fullsend#7197 (skip retro when no agent stages ran). This retro provides additional supporting evidence for those issues.

Existing issue evidence

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant