Skip to content

[ENHANCEMENT] [MER-5212] Dynamic links as iframe source#6289

Open
rgachuhi wants to merge 4 commits intomasterfrom
MER-5212-dynamic-links-as-iframe-source
Open

[ENHANCEMENT] [MER-5212] Dynamic links as iframe source#6289
rgachuhi wants to merge 4 commits intomasterfrom
MER-5212-dynamic-links-as-iframe-source

Conversation

@rgachuhi
Copy link
Contributor

Summary

Implements MER-5212: support adaptive iframe internal page links as first-class dynamic links, including authoring UX, canonical
persistence, runtime rewriting, rewiring, dependency detection, telemetry, and scenario coverage.

This enables authors to choose iframe Source as either:

  • External URL (existing behavior), or
  • Page Link (picker-only, stored as resource_id).

Key Changes

1) Authoring UI: iframe Source editor with mode switching

  • Added a dedicated source editor in the Property Editor:
    • assets/src/apps/authoring/components/PropertyEditor/custom/IframeSourceEditor.tsx
    • wired via PropertyEditor.tsx
  • Added checkbox-style source type selection (Page Link / External URL).
  • Page Link mode loads pages via project link API and uses dropdown-only selection.
  • External URL mode preserves free-text URL behavior.
  • Fixed page loading context resolution for authoring/workspace paths.

2) Iframe model/schema + resolver updates

  • Extended iframe schema transforms to support source config encoding/decoding and mode transitions:
    • assets/src/components/parts/janus-capi-iframe/schema.ts
  • Added shared source resolver for preview/delivery rewrite contexts:
    • assets/src/components/parts/janus-capi-iframe/sourceResolver.ts
  • Applied resolver in authoring and delivery iframe components:
    • CapiIframeAuthor.tsx
    • ExternalActivity.tsx

3) Save-time validation + normalization (backend)

  • Extended adaptive save pipeline to validate iframe internal references:
    • lib/oli/authoring/editing/activity_editor.ex
  • Internal iframe links are normalized to canonical metadata (idref/resource_id, linkType: "page", sourceType: "page").
  • Rejects invalid/out-of-project/out-of-scope targets.
  • Leaves external iframe URLs unaffected.

4) Runtime delivery rewrite + fallback

  • Extended adaptive render rewrite to include iframe page links:
    • lib/oli/rendering/activity/html.ex
  • Resolves internal iframe refs to section lesson URLs.
  • Adds safe iframe-only fallback state when unresolved (about:blank + dynamicLinkFallback payload), while preserving other page
    content.

5) Import/export rewiring + dependency warnings

  • Added iframe page-link rewiring support on export/import:
    • lib/oli/interop/rewire_links.ex
    • lib/oli/interop/ingest/processor/rewiring.ex
  • Extended inbound dependency detection to include iframe references:
    • lib/oli/publishing/authoring_resolver.ex
    • lib/oli_web/live/curriculum/container/container_live.ex
  • Delete-block telemetry now differentiates iframe-driven dependencies.

6) Telemetry and observability

  • Extended dynamic link telemetry for iframe authoring/delivery paths with source metadata:
    • iframe_authoring
    • iframe_delivery_render
    • curriculum_delete_modal_iframe
  • Added AppSignal-facing counters/distributions support via existing telemetry pipeline.
  • Config additions for export backend flexibility (default behavior unchanged):
    • config/config.exs
    • lib/oli/authoring/project_export_worker.ex

7) Scenario infrastructure + scenario test coverage

  • Expanded scenario directive support so create_activity can accept content_format: json:
    • directive_types.ex, directive_parser.ex, activity_handler.ex
    • priv/schemas/v0-1-0/scenario.schema.json
  • Added end-to-end iframe-link scenario + hooks:
    • test/scenarios/delivery/adaptive_iframe_internal_link_resolution.scenario.yaml
    • test/scenarios/delivery/iframe_links_hooks.ex

8) Spec pack + requirements traceability

  • Added/updated feature spec pack and implementation traceability:
    • docs/exec-plans/current/epics/adaptive_page_improvements/iframe_links/{prd,fdd,plan,requirements}.md|yml
  • Phase 6 closure includes operational rollout/rollback checklist and AC proof alignment.

Backward Compatibility

  • No DB migrations.
  • No feature flag.
  • External iframe URL mode remains supported and unchanged.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

Warnings
⚠️ PR is large (4357 LOC changed). Consider splitting.

Risk score: 34 → risk/high

Generated by 🚫 dangerJS against 28e7dd2

@rgachuhi rgachuhi marked this pull request as draft March 11, 2026 14:35
@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

AI Review — security

No issues found

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

AI Review — performance

Re-encoding iframe source JSON on every rewrite

file: lib/oli/interop/rewire_links.ex
line: 242
Description: rewire_iframe_dynamic_link/4 always calls maybe_rewrite_iframe_source/3 for internal iframe parts, and the string variant decodes + re-encodes JSON each time. On large imports with many iframe items, this adds avoidable CPU and allocation overhead.
Suggestion: Short-circuit before decode/encode when "source" already matches the target values (mode: "page", same pageId/pageSlug, empty url), and skip maybe_rewrite_iframe_source/3 in that case.

Multiple full passes to compute matching sources

file: lib/oli/publishing/authoring_resolver.ex
line: 664
Description: refs_mapped |> Enum.filter |> Enum.map |> Enum.uniq performs three traversals per entry. In publishing flows with large reference lists, this adds unnecessary work and memory churn.
Suggestion: Replace the pipeline with a single Enum.reduce/3 accumulating a MapSet of matching sources, then convert once at the end.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

AI Review — ui

Mutually exclusive source choices are implemented as checkboxes

file: assets/src/apps/authoring/components/PropertyEditor/custom/IframeSourceEditor.tsx
line: 154
Description: Page Link and External URL are mutually exclusive modes, but the UI uses checkboxes. This gives incorrect semantics to assistive tech and creates confusing keyboard/form behavior for a single-choice control.
Suggestion: Replace both checkbox inputs with radio inputs sharing a common name (or a semantic <fieldset><legend> + radios), and set mode directly from the selected radio value.

Fallback renders a potentially non-functional link

file: assets/src/components/parts/janus-capi-iframe/ExternalActivity.tsx
line: 1197
Description: The fallback CTA always renders as <a href={fallbackHref}>, but sanitizeAdaptiveIframeFallbackHref can return "#". This creates a misleading interactive element that may just jump the page and provides poor UX/accessibility.
Suggestion: Render the anchor only when fallbackHref !== '#'; otherwise render non-interactive helper text (or a properly disabled button) and provide a valid recovery action when available.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

AI Review — elixir

URL-mode iframes can be incorrectly rejected as internal links

file: lib/oli/authoring/editing/activity_editor.ex
line: 892
Description: Internal-link detection includes not is_nil(idref) without first excluding sourceType == "url". For URL-mode iframes carrying stale idref/resource_id, this path enforces internal validation and can return {:error, {:invalid_update_field}} even though URL mode should bypass internal-link validation.
Suggestion: Add an early guard in validate_iframe_dynamic_link/3 to return :ok when Map.get(part, "sourceType") == "url", before computing is_internal.

URL-mode iframes can be rewritten/fallbacked during delivery

file: lib/oli/rendering/activity/html.ex
line: 402
Description: maybe_rewrite_adaptive_iframe/3 treats any iframe with non-nil idref/resource_id as internal, even when sourceType is "url". This can rewrite or fallback external embeds unexpectedly instead of leaving them untouched.
Suggestion: Short-circuit maybe_rewrite_adaptive_iframe/3 for %{"sourceType" => "url"} (or exclude that mode in is_internal) so URL-mode iframes are always returned unchanged.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

AI Review — typescript

onBlur contract is defined but never honored

file: assets/src/apps/authoring/components/PropertyEditor/custom/IframeSourceEditor.tsx
line: 69
Description: The component props include onBlur?: (id: string, value: string) => void, but onBlur is not destructured or passed to any input/select. In this codebase, property editors typically rely on blur callbacks for commit/validation behavior; skipping it can prevent expected persistence/touched-state updates.
Suggestion: Destructure onBlur and wire it to both the URL input and page select, e.g. onBlur={() => onBlur?.(id, encodeSourceConfig(sourceConfig))}.

Page-loading effect can commit stale async results

file: assets/src/apps/authoring/components/PropertyEditor/custom/IframeSourceEditor.tsx
line: 94
Description: Persistence.pages(projectSlug) resolves asynchronously without cancellation/guarding. If the component unmounts or mode toggles quickly, delayed responses can still call setPagesState, causing racey UI state and potential “set state on unmounted component” warnings.
Suggestion: Add an effect-local cancellation flag (or AbortController if supported) and ignore results after cleanup:
let cancelled = false; ... if (!cancelled) setPagesState(...); return () => { cancelled = true; };.

Mutually-exclusive options implemented as checkboxes

file: assets/src/apps/authoring/components/PropertyEditor/custom/IframeSourceEditor.tsx
line: 151
Description: “Page Link” vs “External URL” is a single-choice mode, but implemented with two checkboxes. This is semantically incorrect for assistive tech and can create confusing keyboard/screen-reader behavior.
Suggestion: Replace with radio inputs sharing the same name (or an accessible radio group component), and update onChange to set mode directly from selected radio value.

@github-actions
Copy link
Contributor

Preview deployed to: https://pr-6289.plasma.oli.cmu.edu

@rgachuhi rgachuhi marked this pull request as ready for review March 11, 2026 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant