enhance: improve embedded PQ display and add postMessage interactions with parent app#5065
Conversation
Use a parent-init handshake and restored completion state so embedded hosts receive stable quiz progress without wildcard postMessage targets. Add a local embed harness and mock API so the iframe contract can be verified against the branch-local PWA instead of production, which is blocked by CSP frame-ancestors.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughAdds an iframe embedding mode for PracticeQuiz: threads an Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as Embed Harness (Parent)
participant IFrame as PracticeQuiz (iframe)
participant Storage as localStorage
Parent->>IFrame: Load iframe with embed=true
Parent->>IFrame: PostMessage "klicker:embed-init" (strict targetOrigin)
IFrame->>IFrame: Validate init message & record parentOrigin
IFrame->>Storage: Read pq-${quizId} completion state
loop quiz progress updates
IFrame->>IFrame: Update currentIx / isCompleted
IFrame->>Parent: PostMessage "klicker:quiz-state" (versioned payload) -> targetOrigin = parentOrigin
Parent->>Parent: Log & display latest payload
end
IFrame->>Storage: Persist completion when finished
IFrame->>Parent: PostMessage final "klicker:quiz-state" (completed=true)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the embedded practice-quiz integration by introducing a parent-initiated postMessage handshake (no wildcard target origins), emitting a versioned klicker:quiz-state payload, and adding a local embed-harness to verify the contract against a branch-local PWA.
Changes:
- Added embedded messaging handshake + versioned quiz-state payload, plus completion restoration from persisted progress.
- Threaded
embeddedmode through the practice-quiz flow and adjusted embedded CTA layout/spacing. - Added
util/embed-harness/(static page + local server + mock GraphQL API + docs) and redirectedutil/embedded.htmlto it.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| util/embedded.html | Redirects the older utility page to the new local embed harness. |
| util/embed-harness/server.mjs | Adds a tiny local HTTP server to serve the harness HTML. |
| util/embed-harness/package.json | Adds scripts to run the harness and mock API. |
| util/embed-harness/mock-api.mjs | Adds a local mock GraphQL server for embed-flow verification. |
| util/embed-harness/index.html | Adds a static parent harness page to load an iframe and inspect postMessage traffic. |
| util/embed-harness/README.md | Documents how to run/verify the local harness against the branch-local PWA. |
| packages/graphql/test/helpers.ts | Adds an explicit return type to fix TS2742 during type-check. |
| apps/frontend-pwa/src/pages/course/[courseId]/practiceQuizzes/[id].tsx | Implements embedded handshake + state emission + completion restoration; documents the protocol. |
| apps/frontend-pwa/src/components/practiceQuiz/PracticeQuiz.tsx | Threads embedded prop through to element stacks. |
| apps/frontend-pwa/src/components/practiceQuiz/ElementStack.tsx | Adjusts embedded CTA positioning and adds bottom padding in embedded mode. |
| AGENTS.md | Documents the embedded messaging trust boundary and harness usage constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| res.setHeader('Access-Control-Allow-Credentials', 'true') | ||
| res.setHeader( | ||
| 'Access-Control-Allow-Headers', | ||
| 'content-type, authorization, x-graphql-yoga-csrf' | ||
| ) | ||
| res.setHeader('Access-Control-Allow-Methods', 'POST, OPTIONS') |
There was a problem hiding this comment.
Access-Control-Allow-Credentials: true is set even when the request Origin is not allowed (i.e., when Access-Control-Allow-Origin is not set). While browsers will still block credentialed reads without a matching Allow-Origin, it's safer and clearer to only emit Allow-Credentials (and potentially the other CORS headers) when origin === allowedOrigin.
| } | |
| res.setHeader('Access-Control-Allow-Credentials', 'true') | |
| res.setHeader( | |
| 'Access-Control-Allow-Headers', | |
| 'content-type, authorization, x-graphql-yoga-csrf' | |
| ) | |
| res.setHeader('Access-Control-Allow-Methods', 'POST, OPTIONS') | |
| res.setHeader('Access-Control-Allow-Credentials', 'true') | |
| res.setHeader( | |
| 'Access-Control-Allow-Headers', | |
| 'content-type, authorization, x-graphql-yoga-csrf' | |
| ) | |
| res.setHeader('Access-Control-Allow-Methods', 'POST, OPTIONS') | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend-pwa/src/components/practiceQuiz/PracticeQuiz.tsx (1)
155-167:⚠️ Potential issue | 🟡 MinorThread
embeddedprop toPracticeQuizOverviewand apply fixed-button styling in the overview CTA.
PracticeQuizOverviewdoes not currently accept anembeddedprop (lines 18–28 of its interface definition). The overview button at lines 164–171 uses only static TailwindCSS utilities and does not opt into the fixed-button/padding behavior for embedded contexts. Without threadingembeddedthrough to this component, the overview screen remains the one step in the embedded flow that cannot adjust its CTA positioning for small iframes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend-pwa/src/components/practiceQuiz/PracticeQuiz.tsx` around lines 155 - 167, The PracticeQuizOverview call inside PracticeQuiz (the currentIx === -1 branch) must pass the embedded boolean down so the overview can adjust its CTA for embedded contexts; update the PracticeQuizOverview props (its interface/type) to include embedded?: boolean, thread embedded={embedded} in the PracticeQuiz JSX where PracticeQuizOverview is rendered, and inside the PracticeQuizOverview component modify the overview CTA/button render logic to apply the fixed-button/padding styling when props.embedded is true (toggle the Tailwind classes used for fixed-button behavior based on embedded).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend-pwa/src/pages/course/`[courseId]/practiceQuizzes/[id].tsx:
- Around line 155-160: The posting effect is emitting an initial "overview"
before persisted completion is restored; fix by ensuring completion is
derived/restored before any post runs: either initialize isCompleted
synchronously using readStoredCompletion when data?.practiceQuiz and id are
present (e.g. useState(() => readStoredCompletion(id, stackIdsDerivedFrom
data.practiceQuiz))) or add a completionRestored flag set true only after
readStoredCompletion updates setIsCompleted in the existing useEffect, then
guard the overview-posting useEffect (the effect that sends the overview payload
around lines 168-192) to run only when completionRestored is true (and embedded
&& data?.practiceQuiz). Reference readStoredCompletion, setIsCompleted and the
posting useEffect when making the change.
In `@util/embed-harness/index.html`:
- Around line 641-673: The loadIframe function currently accepts any normalized
origin from user input; enforce a strict allowlist by validating
normalizedUrl.origin equals "http://127.0.0.1:3101" before setting
state.iframeOrigin/state.iframeUrl and before assigning elements.quizFrame.src;
if the origin does not match, set state.iframeOrigin and state.iframeUrl to
null, call setInitState(false), append an 'error' log describing the forbidden
origin (include the provided URL) and call updateStatus(), thereby
hard-rejecting non-local origins in loadIframe.
In `@util/embed-harness/mock-api.mjs`:
- Around line 30-31: Update the CORS "Access-Control-Allow-Methods" header to
include GET so it matches server behavior: replace occurrences of
res.setHeader('Access-Control-Allow-Methods', 'POST, OPTIONS') with a value that
includes 'GET' (e.g., 'GET, POST, OPTIONS') in both places in
util/embed-harness/mock-api.mjs (the res.setHeader calls around line ~30 and the
other occurrence around lines ~221-224).
In `@util/embed-harness/package.json`:
- Line 2: The package.json "name" field currently uses an unscoped value
("klicker-embed-harness"); update it to follow the org scope pattern by changing
the "name" value to "@klicker-uzh/klicker-embed-harness" and ensure any internal
references (build scripts, import paths or CI/publish configs that reference
"klicker-embed-harness") are updated to the new scoped name; locate the field in
util/embed-harness/package.json and adjust any occurrences in package.json
scripts or repo tooling that reference the old name.
---
Outside diff comments:
In `@apps/frontend-pwa/src/components/practiceQuiz/PracticeQuiz.tsx`:
- Around line 155-167: The PracticeQuizOverview call inside PracticeQuiz (the
currentIx === -1 branch) must pass the embedded boolean down so the overview can
adjust its CTA for embedded contexts; update the PracticeQuizOverview props (its
interface/type) to include embedded?: boolean, thread embedded={embedded} in the
PracticeQuiz JSX where PracticeQuizOverview is rendered, and inside the
PracticeQuizOverview component modify the overview CTA/button render logic to
apply the fixed-button/padding styling when props.embedded is true (toggle the
Tailwind classes used for fixed-button behavior based on embedded).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca7a224d-fe44-47fb-9794-b779f6568778
📒 Files selected for processing (11)
AGENTS.mdapps/frontend-pwa/src/components/practiceQuiz/ElementStack.tsxapps/frontend-pwa/src/components/practiceQuiz/PracticeQuiz.tsxapps/frontend-pwa/src/pages/course/[courseId]/practiceQuizzes/[id].tsxpackages/graphql/test/helpers.tsutil/embed-harness/README.mdutil/embed-harness/index.htmlutil/embed-harness/mock-api.mjsutil/embed-harness/package.jsonutil/embed-harness/server.mjsutil/embedded.html
| useEffect(() => { | ||
| if (!embedded || !data?.practiceQuiz) return | ||
|
|
||
| const stackIds = data.practiceQuiz.stacks?.map((stack) => stack.id) ?? [] | ||
| setIsCompleted(readStoredCompletion(id, stackIds)) | ||
| }, [embedded, id, data?.practiceQuiz]) |
There was a problem hiding this comment.
Avoid sending an initial overview payload before completion is restored.
On a reload of a completed embedded quiz, isCompleted is still false on the first render with practiceQuiz data. The restore effect updates it afterward, but this posting effect runs in the same commit and emits overview once before the rerender emits completed. Any parent that reacts to the first payload still sees the wrong state.
Gate the first emit until persisted completion has been restored, or derive the restored completion state before posting the first message.
Also applies to: 168-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend-pwa/src/pages/course/`[courseId]/practiceQuizzes/[id].tsx
around lines 155 - 160, The posting effect is emitting an initial "overview"
before persisted completion is restored; fix by ensuring completion is
derived/restored before any post runs: either initialize isCompleted
synchronously using readStoredCompletion when data?.practiceQuiz and id are
present (e.g. useState(() => readStoredCompletion(id, stackIdsDerivedFrom
data.practiceQuiz))) or add a completionRestored flag set true only after
readStoredCompletion updates setIsCompleted in the existing useEffect, then
guard the overview-posting useEffect (the effect that sends the overview payload
around lines 168-192) to run only when completionRestored is true (and embedded
&& data?.practiceQuiz). Reference readStoredCompletion, setIsCompleted and the
posting useEffect when making the change.
| function loadIframe() { | ||
| const normalizedUrl = normalizeIframeUrl(elements.quizUrl.value) | ||
|
|
||
| if (!normalizedUrl) { | ||
| state.iframeOrigin = null | ||
| state.iframeUrl = null | ||
| setInitState(false) | ||
| appendLog( | ||
| 'error', | ||
| 'Failed to load iframe because the URL is invalid.', | ||
| { | ||
| input: elements.quizUrl.value, | ||
| } | ||
| ) | ||
| updateStatus() | ||
| return | ||
| } | ||
|
|
||
| state.iframeOrigin = normalizedUrl.origin | ||
| state.iframeUrl = normalizedUrl.toString() | ||
| state.lastAcceptedType = null | ||
| state.latestPayload = null | ||
| setInitState(false) | ||
|
|
||
| elements.quizUrl.value = state.iframeUrl | ||
| elements.quizFrame.src = state.iframeUrl | ||
|
|
||
| appendLog('outbound', 'Loaded iframe URL.', { | ||
| iframeOrigin: state.iframeOrigin, | ||
| iframeUrl: state.iframeUrl, | ||
| }) | ||
| updateStatus() | ||
| } |
There was a problem hiding this comment.
Enforce local-origin-only iframe loading in the harness.
The harness currently accepts any origin from user input, so it can still load non-local targets (including production). Please hard-reject origins other than http://127.0.0.1:3101 in loadIframe.
💡 Proposed fix
function loadIframe() {
const normalizedUrl = normalizeIframeUrl(elements.quizUrl.value)
+ const localOrigin = 'http://127.0.0.1:3101'
if (!normalizedUrl) {
@@
return
}
+
+ if (normalizedUrl.origin !== localOrigin) {
+ state.iframeOrigin = null
+ state.iframeUrl = null
+ setInitState(false)
+ appendLog('error', 'Only the local branch PWA origin is allowed.', {
+ expectedOrigin: localOrigin,
+ receivedOrigin: normalizedUrl.origin,
+ })
+ updateStatus()
+ return
+ }
state.iframeOrigin = normalizedUrl.origin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@util/embed-harness/index.html` around lines 641 - 673, The loadIframe
function currently accepts any normalized origin from user input; enforce a
strict allowlist by validating normalizedUrl.origin equals
"http://127.0.0.1:3101" before setting state.iframeOrigin/state.iframeUrl and
before assigning elements.quizFrame.src; if the origin does not match, set
state.iframeOrigin and state.iframeUrl to null, call setInitState(false), append
an 'error' log describing the forbidden origin (include the provided URL) and
call updateStatus(), thereby hard-rejecting non-local origins in loadIframe.
| res.setHeader('Access-Control-Allow-Methods', 'POST, OPTIONS') | ||
| } |
There was a problem hiding this comment.
CORS allow-methods should include GET to match server behavior.
The server accepts GET but CORS only advertises POST, OPTIONS. Add GET to avoid cross-origin method mismatch.
💡 Proposed fix
- res.setHeader('Access-Control-Allow-Methods', 'POST, OPTIONS')
+ res.setHeader('Access-Control-Allow-Methods', 'GET, POST, OPTIONS')Also applies to: 221-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@util/embed-harness/mock-api.mjs` around lines 30 - 31, Update the CORS
"Access-Control-Allow-Methods" header to include GET so it matches server
behavior: replace occurrences of res.setHeader('Access-Control-Allow-Methods',
'POST, OPTIONS') with a value that includes 'GET' (e.g., 'GET, POST, OPTIONS')
in both places in util/embed-harness/mock-api.mjs (the res.setHeader calls
around line ~30 and the other occurrence around lines ~221-224).
| @@ -0,0 +1,9 @@ | |||
| { | |||
| "name": "klicker-embed-harness", | |||
There was a problem hiding this comment.
Scope the harness package name under @klicker-uzh.
klicker-embed-harness breaks the repo’s package-name convention. Keeping the scope consistent avoids one-off package identities, even for private tooling.
🔧 Proposed fix
- "name": "klicker-embed-harness",
+ "name": "@klicker-uzh/embed-harness",As per coding guidelines, "Package names must follow the pattern @klicker-uzh/<name>."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "name": "klicker-embed-harness", | |
| "name": "@klicker-uzh/embed-harness", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@util/embed-harness/package.json` at line 2, The package.json "name" field
currently uses an unscoped value ("klicker-embed-harness"); update it to follow
the org scope pattern by changing the "name" value to
"@klicker-uzh/klicker-embed-harness" and ensure any internal references (build
scripts, import paths or CI/publish configs that reference
"klicker-embed-harness") are updated to the new scoped name; locate the field in
util/embed-harness/package.json and adjust any occurrences in package.json
scripts or repo tooling that reference the old name.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3-embedded-pq
|
| Run status |
|
| Run duration | 10m 12s |
| Commit |
|
| Committer | Roland Schläfli |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
758
|
| View all changes introduced in this branch ↗︎ | |
…e, adjust icon layout
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/frontend-pwa/src/components/practiceQuiz/ElementStack.tsx (2)
359-373: Consider extracting the toggle callback to reduce re-renders.The inline callback is recreated on every render for each element. While this only affects embedded mode with existing evaluations, extracting it to a stable callback could improve performance if the element list is large.
♻️ Optional: Extract toggle callback
+ const handleToggleEvaluation = (elementId: number) => { + setOpenEvaluations((prev) => { + const next = new Set(prev) + if (next.has(elementId)) { + next.delete(elementId) + } else { + next.add(elementId) + } + return next + }) + } + // ... in the map function: onToggleEvaluation={ embedded && stackStorage?.[element.id]?.evaluation - ? () => - setOpenEvaluations((prev) => { - const next = new Set(prev) - if (next.has(element.id)) { - next.delete(element.id) - } else { - next.add(element.id) - } - return next - }) + ? () => handleToggleEvaluation(element.id) : undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend-pwa/src/components/practiceQuiz/ElementStack.tsx` around lines 359 - 373, The inline onToggleEvaluation lambda causes a new function per element render; extract it into a stable callback to avoid re-renders when embedded and stackStorage?.[element.id]?.evaluation is true. Create a memoized handler (e.g., toggleEvaluation or handleToggleEvaluation) that accepts an elementId and calls setOpenEvaluations(prev => { const next = new Set(prev); next.has(elementId) ? next.delete(elementId) : next.add(elementId); return next; }); then pass a pre-bound version (or use useCallback with element.id) to the onToggleEvaluation prop only when embedded && stackStorage?.[element.id]?.evaluation is truthy, preserving evaluationOpen and the conditional behavior.
381-381: Clarify thepreviewprop semantics.The logic
preview={embedded && !openEvaluations.has(element.id)}means:
- Non-embedded mode:
previewis alwaysfalse- Embedded mode:
previewistrueby default,falsewhen evaluation is toggled openThis naming feels inverted—when the user clicks to "open" the evaluation,
previewbecomesfalse. Consider whether the prop name or the logic should be adjusted for clarity, or add a comment explaining the intent.💡 Consider adding a clarifying comment
<StudentElement element={element} elementIx={elementIx} studentResponse={studentResponse} setStudentResponse={setStudentResponse} stackStorage={stackStorage} + // In embedded mode, show preview (hide evaluation) by default; + // toggle to full view when user clicks the evaluation button preview={embedded && !openEvaluations.has(element.id)} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend-pwa/src/components/practiceQuiz/ElementStack.tsx` at line 381, The prop name/logic is confusing: `preview={embedded && !openEvaluations.has(element.id)}` makes preview true only for embedded elements when their evaluation is closed; either rename the prop to reflect that (e.g., `isPreviewMode` or `showPreviewWhenCollapsed`) and update all consumers, or keep the prop name and add a concise inline comment above this prop explaining the semantics ("preview is true for embedded elements while their evaluation is closed — toggling open sets preview=false"), and ensure the expression uses `embedded && !openEvaluations.has(element.id)` (reference: the `preview` prop, `embedded` variable and `openEvaluations.has(element.id)` in ElementStack).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/frontend-pwa/src/components/practiceQuiz/ElementStack.tsx`:
- Around line 359-373: The inline onToggleEvaluation lambda causes a new
function per element render; extract it into a stable callback to avoid
re-renders when embedded and stackStorage?.[element.id]?.evaluation is true.
Create a memoized handler (e.g., toggleEvaluation or handleToggleEvaluation)
that accepts an elementId and calls setOpenEvaluations(prev => { const next =
new Set(prev); next.has(elementId) ? next.delete(elementId) :
next.add(elementId); return next; }); then pass a pre-bound version (or use
useCallback with element.id) to the onToggleEvaluation prop only when embedded
&& stackStorage?.[element.id]?.evaluation is truthy, preserving evaluationOpen
and the conditional behavior.
- Line 381: The prop name/logic is confusing: `preview={embedded &&
!openEvaluations.has(element.id)}` makes preview true only for embedded elements
when their evaluation is closed; either rename the prop to reflect that (e.g.,
`isPreviewMode` or `showPreviewWhenCollapsed`) and update all consumers, or keep
the prop name and add a concise inline comment above this prop explaining the
semantics ("preview is true for embedded elements while their evaluation is
closed — toggling open sets preview=false"), and ensure the expression uses
`embedded && !openEvaluations.has(element.id)` (reference: the `preview` prop,
`embedded` variable and `openEvaluations.has(element.id)` in ElementStack).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e6a3776-521d-4fb3-ac64-caaad1ed04a4
📒 Files selected for processing (3)
apps/frontend-pwa/src/components/practiceQuiz/ElementStack.tsxapps/frontend-pwa/src/components/practiceQuiz/InstanceHeader.tsxapps/frontend-pwa/src/components/practiceQuiz/PracticeQuiz.tsx
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|



Summary by CodeRabbit
New Features
Documentation