Skip to content

Add LTI page for Canvas iframe (INDG 100 self-assessment)#504

Open
frasermuller wants to merge 14 commits into
mainfrom
feature/lti-self-assessment-embed
Open

Add LTI page for Canvas iframe (INDG 100 self-assessment)#504
frasermuller wants to merge 14 commits into
mainfrom
feature/lti-self-assessment-embed

Conversation

@frasermuller
Copy link
Copy Markdown
Collaborator

@frasermuller frasermuller commented Apr 1, 2026

POST: sister branch in chatbot

https://github.com/ubco-db/chatbot/pull/69

Description

Closes #not an issue

This is a big rework from what I was originally making before. Instead of passing the question text in the URL, this adds a proper iframe question system for Canvas embeds. Profs can create and manage questions (with optional grading criteria) in the course settings, and get a copy-paste iframe embed code. All questions are stored in the database now so the URLs are clean.

Students see the question in the iframe, type their response, and get instant AI feedback right there. The feedback comes from the existing HelpMe chatbot, and the backend builds a prompt with the question + criteria + student response and sends it to the chatbot query endpoint.

The iframe endpoints are public (no HelpMe login required) so students in Canvas don't need to be logged into their HelpMe account to use it (though idk maybe there could be some big changes to this where like it would save the feedback for students so it can like build off previous feedback, idk NOT DOING THIS NOW THO). This was super annoying to figure out because whenever i would put the Iframe link in a canvas quiz it would just take me to the login screen or the main page.

There are two public endpoints:

GET /api/v1/iframe-question/public/:courseId/:questionId
fetches the question text

POST /api/v1/iframe-question/public/:courseId/:questionId/feedback

  • sends the student response, gets AI feedback back

the actual iframe route is at: /lti/iframe/[cid]?q=[questionId] so whenever you make a question, you can go to the specific URL and that question will be there.

I want to get this on staging so I can test the full AI feedback loop and iframe embed in Canvas, since I can’t fully verify the AI part locally.

heres a copilot generated flow which might help:

Student types answer

POST /api/v1/iframe-question/public/7/8/feedback
{ responseText: "student's text" }

Backend looks up question # from DB

Builds combined prompt (question + criteria + response)

Forwards to chatbot API: POST /chatbot/query
{ query: "Question: ...\nCriteria: ...\nStudent's response: ...", type: "default" }

AI generates feedback

Returns { feedback: "Your response..." }

Rendered in the iframe

HERES SOME SCREENSHOTS:

in canvas:
Screenshot 2026-04-08 at 9 22 09 PM

in helpMe from profs perspective
Screenshot 2026-04-08 at 9 22 40 PM

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • This requires a run of yarn install
  • This change requires an addition/change to the production .env variables. These changes are below:
  • This change requires developers to add new .env variables. The file and variables needed are below:
  • This change requires a database query to update old data on production. This query is below:
    new migration added.

How Has This Been Tested?

Tested locally with seeded data. The frontend correctly loads questions, renders the form, and sends requests through to the backend chatbot endpoint. Actual AI feedback still needs to be tested on staging where the chatbot service is running.

  • Iframe page loads at /lti/iframe/[cid]?q=[id] without requiring login

  • Question text and criteria are fetched from the public endpoint

  • Form submission hits the public feedback endpoint correctly

  • Iframe settings page (CRUD) works for profs/TAs

  • Copy embed code generates correct iframe HTML

  • Iframe sizing stays compact in Canvas (no more height blowup)

  • Integration tests pass for all iframe question endpoints

  • End-to-end AI feedback test (needs staging)

Checklist:

  • I have performed a code review of my own code (under the "Files Changed" tab on github) to ensure nothing is committed that shouldn't be (e.g. leftover console.logs, leftover unused logic, or anything else that was accidentally committed)
  • I have commented my code where needed
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that new and existing tests pass locally with my changes
  • Any work that this PR is dependent on has been merged into the main branch
  • Any UI changes have been checked to work on desktop, tablet, and mobile

Replaces the old self-assessment page (which had question text hardcoded
in URL params) with a proper database-backed system. Professors can now
create and manage questions through a settings page, copy an iframe embed
code, and paste it into Canvas. Students see the question, type a response,
and get AI feedback via the existing chatbot endpoint.

- New IframeQuestion entity, migration, service, controller, and module
- Professor settings page (CRUD + copy embed code)
- Student iframe page with AI feedback component
- Seed data for dev environment
- Cleaned up old self-assessment files
The iframe was under the (embed) layout which wraps everything in
LtiContextProvider. That was sending lti.frameResize to Canvas and
blowing up the iframe height. Moved it to /lti/iframe/[cid] outside
of (embed) so it doesn't get that context at all.

Also added public endpoints so students don't need to be logged into
HelpMe to use the iframe:
- GET /iframe-question/public/:courseId/:questionId
- POST /iframe-question/public/:courseId/:questionId/feedback

Other fixes:
- Fixed LtiContext crashing when not actually embedded in Canvas
- Added chatbot/query to LTI restrictPaths
- Exported ChatbotApiService from ChatbotModule
- Updated middleware to allow /lti/iframe/* as public pages
@frasermuller frasermuller marked this pull request as ready for review April 9, 2026 04:27
@AdamFipke
Copy link
Copy Markdown
Collaborator

AdamFipke commented Apr 9, 2026

A few thoughts:

  • The fact that the endpoints are public is... unfortunate. Since technically basically anyone with a link to this will have unrestricted access to our chatbot (potential for abuse)
  • From looking at the code, it doesn't seem like the iframe would show any citations from the chatbot? Though I guess for INDG 100 you probably don't need that
  • So from what i've gathered, each iframe question gets its own little prompt. And iirc if you're just using the /query endpoint and not /ask, then the base course prompt is ignored. One approach would allow profs to have a toggle to include the course prompt in these iframe questions. But if you're strapped for time, tbh I think you can get away with just adding "note that Criteria does NOT include your course prompt - the given Criteria will be the ENTIRE prompt (apart from HelpMe's short system prompt)" (i also kinda forget, I'm fairly certain the /query endpoint includes the system prompt but I could be wrong. @bhunt02 do you remember?)

@frasermuller
Copy link
Copy Markdown
Collaborator Author

A few thoughts:

  • The fact that the endpoints are public is... unfortunate. Since technically basically anyone with a link to this will have unrestricted access to our chatbot (potential for abuse)
  • From looking at the code, it doesn't seem like the iframe would show any citations from the chatbot? Though I guess for INDG 100 you probably don't need that
  • So from what i've gathered, each iframe question gets its own little prompt. And iirc if you're just using the /query endpoint and not /ask, then the base course prompt is ignored. One approach would allow profs to have a toggle to include the course prompt in these iframe questions. But if you're strapped for time, tbh I think you can get away with just adding "note that Criteria does NOT include your course prompt - the given Criteria will be the ENTIRE prompt (apart from HelpMe's short system prompt)" (i also kinda forget, I'm fairly certain the /query endpoint includes the system prompt but I could be wrong. @bhunt02 do you remember?)

Yeah i know its definitely not ideal having it public. Idk i could add some sort of like rate limiting or something that could help with the potential "abuse". but i honestly dont really know how to fix this problem without it being public. For citations, since this was mainly self-assessments where students are reflecting on their own experience, there's nothing to cite really, but if i were to change it to the /ask, im pretty sure it needs a user chat token so like they would have to be logged in which is back to the original problem. i guess a solution might be to make some sort of like general token maybe? like a shared token for this feature? but im not sure if thats even possible or how to do that.

and for the course prompt. i can definitely add that so that it explicitly says it. thats honestly the easiest fix.

@AdamFipke
Copy link
Copy Markdown
Collaborator

Yeah i know its definitely not ideal having it public. Idk i could add some sort of like rate limiting or something that could help with the potential "abuse". but i honestly dont really know how to fix this problem without it being public. For citations, since this was mainly self-assessments where students are reflecting on their own experience, there's nothing to cite really, but if i were to change it to the /ask, im pretty sure it needs a user chat token so like they would have to be logged in which is back to the original problem. i guess a solution might be to make some sort of like general token maybe? like a shared token for this feature? but im not sure if thats even possible or how to do that.

and for the course prompt. i can definitely add that so that it explicitly says it. thats honestly the easiest fix.

Okay I actually went and double checked the chatbot repo. The /query endpoint doesn't seem to use the course prompt, the system prompt, or any uploaded materials.

So yeah I guess maybe adjust the "Criteria" tooltip to say as much. Basically that the text they put in is the entirety of the prompt. No course prompt, no HelpMe system prompt, and the chatbot knowledge base is not used.

If we did want to make it use the /ask endpoint instead, you're right we would need to figure out something for the token. Right now, other parts of the system just create a temporary user with a token and then delete it after, which is kinda turbo jank. I talk a little bit about it here #508 . Maybe if some professors want the iframes questions to be like full mini chatbots we can consider that for the future. But for what you're doing this would probably be fine.

And yeah as for the public endpoint problem, I don't really have a good idea to fix it. If we had some way of making it so that if the iframe detects that it's embedded in a canvas page it will auto log the user into helpme that would be amazing (since then we can also keep track of who is asking what). But something tells me with general web fundamentals that that wouldn't be allowed unless there was a way to establish trust between Canvas and the HelpMe iframe. And also yeah bridgette already spent like multiple months basically doing that for her LTI embed of the chatbot to get it as good as she could and even then it's not perfect (you still need to login once the first time, and I believe trust needs to be established by the prof authorizing a HelpMe page in their course, and i imagine the process is different for just inserted iframes).

Copy link
Copy Markdown
Collaborator

@AdamFipke AdamFipke left a comment

Choose a reason for hiding this comment

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

Finally some code that isn't just completely AI generated slop! There's no huge gaping logic holes or completely unfinished code and I can tell that you actually spent time problem solving here to solve the root issue rather than feed the issue to the AI and watch it pump out a steaming pile of garbage that I'll be spending the next half-day picking apart when it turns out there's an entirely better solution to the root issue that no one will think of because the person that was assigned to the issue never actually put in the thought and effort and since AI is stupid it never would've come up with it. And if it turns out I didn't need to spend the next half-day picking it apart, it's because the issue was so small and simple and would only have taken me a few hours to do (and meanwhile they will spend weeks worth of time to get the same amount done and learn fuckall in the process). It's miserable working with AIs; small talk is eliminated, problems are solved poorly, skills aren't improved, nothing is learnt. And I don't get how people who are reliant on AI will expect to get anywhere except from being mistakenly hired - all they will do is take all the junior-level issues that the seniors were saving for training purposes and then pump out garbage once they get anything harder and waste the seniors time. I'd just fire them since I don't need someone there to prompt the LLM for me. But then you might get seen as rude for calling them out on it, so it's either tolerate AI-parrots and be miserable or get seen as rude (especially from those with management backgrounds who probably lack the domain expertise to see how garbage the work is) and then have your social/career opportunities be hampered and be miserable. I think that's the same reason why you don't see people posting negative comments about other people like on linkdin- people will see you as mean or judgy or afraid to work with you i think. Like it feels wrong to, but maybe people should. Or maybe it just needs to be phrased in the right way, since it does feel disrespectful to just get given a bunch of AI generated garbage since now I'm the one that needs to filter through all of it. Repeat ad infinitum since the person never bothered to learn and thus never became better than the AI, and it just feels like and endless cycle of disrespect.
It's so very lonely out here

Anywho, just some small comments.

Comment on lines +8 to +18
<style>{`
html, body, #html {
height: auto !important;
min-height: 0 !important;
background: transparent !important;
}
body {
display: block !important;
flex-grow: 0 !important;
}
`}</style>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yuck but understandable

Comment on lines +54 to +55
No question specified. The iframe URL should include a question ID
(e.g. ?q=3).
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would maybe go into more detail with this (y'know, just in case). Something like "Please consider refreshing the page if possible (copy anything important elsewhere first), and/or let your professor know"

(although, it's been a while idk what would actually happen if a student were to refresh the page on a canvas quiz. Since like it'd be bad if canvas would like block them from getting back in the quiz if they refresh the page).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I updated that error text so it’s not just “missing question ID” anymore. It still tells them the URL is missing ?q=..., but now it also gives them actual next steps: if possible, refresh after copying anything important first, and if it keeps happening, let their professor know. So it’s a bit more actionable for students instead of just a technical message.

if (error || !question) {
return (
<div className="flex min-h-32 flex-col items-center justify-center px-3 py-2">
<p className="text-zinc-600">{error || 'Question not found.'}</p>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Similarly, I would also add a "Please let your professor know" at the end of this error message

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

when the question fails to load (or isn’t found), the message ends with “Please let your professor know.” So it’s clearer what the student should do instead of just showing a dead-end error.

Comment on lines +74 to +75
@Post('public/:courseId/:questionId/feedback')
async getFeedbackPublic(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would really add a throttler to this. I think our default global throttler is something like 10requests per second (i think it's probably more than that). Example here: https://github.com/ubco-db/helpme/pull/476/changes#diff-6fe8c3cf86e5e0cda0cfb874144a8de2e1b2a4cd390e4daa0756ae6a5c62ed0cR245

For this endpoint in particular, you'd maybe want to throttle it to be something like 2 or 3 requests per minute. Or like 5 requests per 5 minutes.

But since this throttle would be triggerable by our users (by spamming it), you might want to handle the error on the frontend saying like "woah buddy calm down. No need to spam it. We allow x requests every x minutes"

Though, come to think of it, if for some reason someone were to stick a LOT of iframes in a single quiz, maybe 5 requests per 5 minutes might be too little. Maybe 10 requests per 5 minutes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

already replied to a diff comment with what i changed, but basically its now 10 req / 5 mins and added frontend handling for 429 so users get a clear “wait a few minutes” message

Comment on lines +78 to +79
@Body() body: { responseText?: string },
): Promise<{ feedback: string }> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As much as it's literally only an object with a single attribute, the body type should still be put in a DTO so that we can ensure that it's a string (since otherwise someone could pass a number or array etc. and cause the backend to error, or worse yet do sql injection). Details how to do so here: https://github.com/ubco-db/helpme/pull/474/changes#diff-5c8dab1e31177771ccba285054bc148e65cf55192857db8b5e648a280dc8e18fR303

Same goes for the create() endpoint and update() endpoint.

It would also be good to make it so the endpoint has a proper return DTO as well. Not necessarily to validate the response from our backend (though technically that's still a good idea in case our server gets compromised), but moreso for the maintainability benefits (if we ever want the endpoint to return something else, we only have to change the code in 1 spot instead of multiple). But admittedly this is a bit less important compared to a request DTO.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I changed the inline body types into DTOs for create, update, and public feedback, and added the feedback return type too. so now the endpoints enforce the expected shape/types properly instead of just trusting whatever gets sent in.

throw new BadRequestException('responseText is required');
}

const question = await this.iframeQuestionService.findOne(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

as a minor point, I'm getting whiplash a bit here since we already have 3 different types of questions (chatbot, queue, anytime), and this now makes it 4.

I really want to name it something else (on the frontend, backend, everywhere) since I imagine this might also get confusing for professors. But godam what would it be? On every quiz, test, assignment, etc. they're always called "questions" I think.

And when I see the text "iframe questions", idk if it's questions that I (as the professor) would set or student questions that came from an iframe (whatever that would mean, I would just be a silly professor that's new to the system idk what helpme does). Same goes if we call them "quizIframeQuestions" - are they questions that students ask or questions that I set?

Maybe if it's called profSetIframeQuestions? But then that's too long for some areas (like the nav). But maybe generally that would work? Like in the changelog n parts of the UI call them Professor-set Iframe Questions. Maybe. Maybe.

okay i thought of some synonyms but none of them work

  • "Exercise" - they're not really exercises
  • "Prompt" - since we're an AI system that'd get confusing
  • "Problem" - not really problems
  • "Item" - feels to generic, i wouldn't know what that is

man this is kinda bs actually. I blame english for my woes. I will spend the next amount of time pondering this

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

lol yeah, i just kept it the same, i honestly dont really know what to call it either, other than "iframe questions". maybe something with like embedding and AI feedback, like "embeddable question with AI feedback." gives a bit more context as to what it actually is but it sounds pretty bad and is quite long... maybe "embeddable question"? still sounds not great. yeah i haven't got a clue either

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

"embeddable question with AI feedback" I think does sound like the best descriptor of it so far, but yeah unfortunately it's long. Maybe on the page title that's what it could be called instead of "Iframe questions"

Image

I also like the sounds of "embeddable question" more since I think it's less likely to be confused as "question that came from an embeddable" compared to "iframe question" getting confused as "question that came from an iframe" (the latter feels more likely to be a feature I would think). Moreover, I'm also unsure how familiar a typical professor would be with the term "iframe".

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And I guess from even a maintenance perspective, renaming it from "iframeQuestions" to "embeddableQuestions" in the code might not be a bad idea.
image

Though I think Bridgette or I can handle that later so don't worry about it

questionText: string;

// criteria that the AI uses to evaluate the student's response
// if empty, the course-level default criteria can apply
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

From what I could tell, I don't think any default criteria is getting applied anywhere. So without any criteria, the entire prompt to the chatbot would just be the question text.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I updated this. I removed the “course-level default criteria” comment and made criteria required end-to-end, so we’re no longer implying fallback behavior that doesn’t exist and we don’t allow empty/omitted criteria anymore.

'r^\\/api\\/v1\\/semesters\\/[0-9]+$',
'r^\\/api\\/v1\\/chatbot\\/ask\\/[0-9]+$',
'r^\\/api\\/v1\\/chatbot\\/askSuggested\\/[0-9]+$',
'r^\\/api\\/v1\\/chatbot\\/query\\/[0-9]+$',
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

wait i might be a little confused here. But i don't believe the frontend calls this endpoint directly, does it? So you probably don't want to allow this here. But I could also be totally misunderstanding what's actually going on here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah you're right oops. this was from before i moved it to public endpoints and i forgot to delete it.

Add stronger validation and throttling around the public iframe feedback flow, clarify iframe UX copy, and align iframe question criteria handling across frontend, backend, tests, and migration history.
@frasermuller
Copy link
Copy Markdown
Collaborator Author

Yeah i know its definitely not ideal having it public. Idk i could add some sort of like rate limiting or something that could help with the potential "abuse". but i honestly dont really know how to fix this problem without it being public. For citations, since this was mainly self-assessments where students are reflecting on their own experience, there's nothing to cite really, but if i were to change it to the /ask, im pretty sure it needs a user chat token so like they would have to be logged in which is back to the original problem. i guess a solution might be to make some sort of like general token maybe? like a shared token for this feature? but im not sure if thats even possible or how to do that.
and for the course prompt. i can definitely add that so that it explicitly says it. thats honestly the easiest fix.

Okay I actually went and double checked the chatbot repo. The /query endpoint doesn't seem to use the course prompt, the system prompt, or any uploaded materials.

So yeah I guess maybe adjust the "Criteria" tooltip to say as much. Basically that the text they put in is the entirety of the prompt. No course prompt, no HelpMe system prompt, and the chatbot knowledge base is not used.

If we did want to make it use the /ask endpoint instead, you're right we would need to figure out something for the token. Right now, other parts of the system just create a temporary user with a token and then delete it after, which is kinda turbo jank. I talk a little bit about it here #508 . Maybe if some professors want the iframes questions to be like full mini chatbots we can consider that for the future. But for what you're doing this would probably be fine.

And yeah as for the public endpoint problem, I don't really have a good idea to fix it. If we had some way of making it so that if the iframe detects that it's embedded in a canvas page it will auto log the user into helpme that would be amazing (since then we can also keep track of who is asking what). But something tells me with general web fundamentals that that wouldn't be allowed unless there was a way to establish trust between Canvas and the HelpMe iframe. And also yeah bridgette already spent like multiple months basically doing that for her LTI embed of the chatbot to get it as good as she could and even then it's not perfect (you still need to login once the first time, and I believe trust needs to be established by the prof authorizing a HelpMe page in their course, and i imagine the process is different for just inserted iframes).

Okay, so I updated the criteria helper text to make this explicit: for iframe feedback, the prompt uses only the question text + criteria entered there, and does not use course prompt, HelpMe system prompt, or chatbot knowledge base and made it red so its very clear to profs.

Also its still on /query

About the public endpoint thing, for POST /api/v1/iframe-question/public/:courseId/:questionId/feedback, i added route-level throttling so 10 requests every 5 minutes, so if someone (or a script) spams it, they hit 429 instead of being able to spam it.

i also added frontend handling for 429 in the iframe feedback component so users get a clear “wait a few minutes” message.

so basically: still public (since iframe flow needs no-login), but less abusable and less confusing when limits are hit.

Backfill existing null criteriaText rows before applying NOT NULL so local and shared environments can run the migration without failing on legacy data.
Copy link
Copy Markdown
Collaborator

@AdamFipke AdamFipke left a comment

Choose a reason for hiding this comment

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

hey fraser thanks for coming back to work on this, we do really appreciate it, you've done more than enough. From here, bridgette and I will take it over and get it merged on prod before the indg 100 course starts, but you're also welcome to continue working on it. It's just that it might be some time before bridgette will be able to review it

Comment on lines +203 to +205
Create questions that can be embedded as iframes in Canvas or other LMS
platforms. Students will see the question and can submit a response to
get AI feedback.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I realise that we should also probably have a tutorial for this (and maybe a showcase).

Maybe a paragraph just below this one stating "Once you have a embeddable question created, copy the iframe link, go into a Canvas quiz/assignment, click 'new question' -> 'iframe', and paste the link" (I should also note that I'm unfamiliar with Canvas' workflow for this so it should probably be tweaked).

Ideally we also have a little showcase video or gif of it in action, which we can include both here and on the changelog (maybe even on the landing page once we flesh out the feature some more). Though I can be the one to make this, and this will obviously come later.

…pdate modal for q's to its own component and used rc-form to make it prettier and more standardized, removed "public" routes for iframe questions, changed how URL is computed and used for iframe questions, overall refactoring - added 'instructions' as additional part. will come with a sister chatbot PR as it required adding a new query type for better structuring
Copy link
Copy Markdown
Collaborator

@AdamFipke AdamFipke left a comment

Choose a reason for hiding this comment

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

I do realise you're not done I just had a few small comments.

Also, before we merge this, I do really want to rename this whole feature to "Embeddable Questions" instead of "Iframe Questions" so that it won't get misinterpreted as "Questions from Iframes" from both professors and future developers. I could do this too while you work on stuff that is more important (like that refresh_token issue or reviewing other PRs) - though obviously i'm not gonna refactor this PR while you're actively working on it.

Comment thread packages/frontend/app/lti/iframe/[cid]/[qid]/page.tsx Outdated
Comment on lines +33 to +65
useEffect(() => {
if (editingQuestion) {
const values = pick(editingQuestion,['questionText','criteriaText','instructions'])
form.setFieldsValue(values)
setFormValues(values)
} else {
form.resetFields(['questionText','criteriaText','instructions'])
setFormValues({})
}
}, [editingQuestion, form])

const handleSave = async () => {
form
.validateFields()
.then(async (values) => {
Object.keys(values).forEach((k) => {
(values as any)[k] = (values as any)[k]?.trim() ?? undefined
if (typeof (values as any)[k] === 'string' && !(values as any)[k]) (values as any)[k] = null
})
if (editingQuestion) {
await API.lti.iframeQuestion.update(courseId, editingQuestion.id, {
...values,
} as any)
message.success('Successfully updated question!')
} else {
await API.lti.iframeQuestion.create(courseId, {
...values,
} as any)
message.success('Successfully created question!')
}
setOpen(false)
form.resetFields(['questionText','criteriaText','instructions'])
setFormValues({})
Copy link
Copy Markdown
Collaborator

@AdamFipke AdamFipke May 8, 2026

Choose a reason for hiding this comment

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

This could probably be refactored to follow the antd docs a little more closely so it's typesafe all the way through (and a lot simpler). https://ant.design/components/form#form-demo-form-in-modal

CreateAsyncQuestionModal.tsx is a pretty decent example I think (I cut out a bit of the irrelevant stuff).
But basically:

  • Define a type for all of the FormValues
  • Don't manage the form state ourselves. Let antd do that
    • Instead of using a useEffect to set the form state, use initialValues on <Form>
    • on <Form>, use onFinish={(values) => onFinish(values)}
  • Some other <Modal> props:
    • okButtonProps={{ autoFocus: true, htmlType: 'submit', loading: isLoading, }}. This is a critical step, since htmlType: 'submit' will trigger the validation step of the Form automatically and then triggers onFinish on success
    • modalRender={(dom) => (<Form ... > {dom} </Form> idk why. antd example had this and I assume it's important
    • destroyOnHidden so you don't need to worry about clearing the form state when onFinish is done (it was also in the antd example. I think we have a Modal Form or two that lacks this and they are kinda buggy sometimes). If you're worried about a user closing and losing what they were typing, I believe you can use maskClosable = false, keyboard = false , and/or an onClose handler when a user has unsaved changes but meh it probably is fine to go without
    • Optionally, play around with the cancelButtonProps or footer props (in this case, I put the delete button in the footer when editing on mobile)
  • Some other <Form> props:
    • name="form_in_modal" idk if this does anything but it was in antd example
    • clearOnDestroy: same reason and goes with destroyOnHidden
  • And then remove shouldUpdate and value from from items/inputs.

And that should cover most formmodals cases. I think you're probably already aware of this, but if there's a time when you actually need to obtain/set the form state, useWatch, form.getFieldsValue, and form.setFieldsValue are some tools.

Here's the snipbit from CreateAsyncQuestionModal.tsx

interface FormValues {
  QuestionAbstract: string
  questionText: string
  questionTypesInput: number[]
  refreshAIAnswer: boolean
  setVisible: boolean
  setAnonymous: boolean
}

interface CreateAsyncQuestionModalProps {
  ...
  question?: AsyncQuestion // if it's defined, then it's an edit modal
}

const CreateAsyncQuestionModal: React.FC<CreateAsyncQuestionModalProps> = ({
 ...
  question,
}) => {
  const [isLoading, setIsLoading] = useState(false)

  const onFinish = async (values: FormValues) => {
    setIsLoading(true)

    // If editing a question, update the question. Else create a new one
    if (question) {
        await API.asyncQuestions
          .studentUpdate(question.id, {
            questionTypes: newQuestionTypeInput,
            questionText: values.questionText,
            questionAbstract: values.QuestionAbstract,
            isAnonymous: values.setAnonymous,
            authorSetVisible: authorCanSetVisible ? values.setVisible : false,
          })
          .then(() => {
            message.success('Question Updated')
            onCreateOrUpdateQuestion()
          })
          .catch((e) => {
            message.error('Error updating question:' +  getErrorMessage(e))
          })
          .finally(() => {
            setIsLoading(false)
          })
    } else {
      await getAiAnswer(
        formatQuestionForChatbot(
          values.QuestionAbstract,
          values.questionText,
          newQuestionTypeInput,
        ),
      ).then(async (aiAnswer) => {
        await API.asyncQuestions
          .create(
            {
              questionTypes: newQuestionTypeInput,
              questionText: values.questionText,
              aiAnswerText: aiAnswer,
              answerText: aiAnswer,
              questionAbstract: values.QuestionAbstract,
              status: courseFeatures?.asyncCentreAIAnswers
                ? asyncQuestionStatus.AIAnswered
                : asyncQuestionStatus.AIAnsweredNeedsAttention,
              isAnonymous: values.setAnonymous,
              authorSetVisible: authorCanSetVisible ? values.setVisible : false,
            },
            courseId,
          )
          .then(() => {
            message.success('Question Posted')
            onCreateOrUpdateQuestion()
          })
          .catch((e) => {
            message.error('Error creating question:' +  getErrorMessage(e))
          })
          .finally(() => {
            setIsLoading(false)
          })
      })
    }
  }

  return (
    <Modal
      open={open}
      title={question ? 'Edit Question' : 'What do you need help with?'}
      okText="Finish"
      cancelText="Cancel"
      okButtonProps={{
        autoFocus: true,
        htmlType: 'submit',
        loading: isLoading,
      }}
      cancelButtonProps={{
        danger: !question,
      }}
      onCancel={onCancel} // hides the modal
      // display delete button for mobile in footer
      footer={(_, { OkBtn, CancelBtn }) => (
        <div
          className={`flex md:justify-end ${question ? 'justify-between' : 'justify-end'}`}
        >
          {question && (
            <Popconfirm
              className="flex md:hidden"
              title="Are you sure you want to delete your question?"
              okText="Yes"
              cancelText="No"
              getPopupContainer={(trigger) => trigger.parentNode as HTMLElement}
              okButtonProps={{ loading: deleteLoading }}
              onConfirm={async () => {
                setDeleteLoading(true)
                await deleteAsyncQuestion(
                  question.id,
                  false,
                  onCreateOrUpdateQuestion,
                )
                setDeleteLoading(false)
              }}
            >
              <Button danger type="primary" icon={<DeleteOutlined />}>
                Delete Question
              </Button>
            </Popconfirm>
          )}
          <div className="ml-1 flex gap-2">
            <CancelBtn />
            <OkBtn />
          </div>
        </div>
      )}
      destroyOnHidden
      modalRender={(dom) => (
        <Form
          layout="vertical"
          form={form}
          name="form_in_modal"
          initialValues={{
            QuestionAbstract: question?.questionAbstract,
            questionText: question?.questionText,
            questionTypesInput:
              questionTypes && questionTypes.length > 0
                ? question?.questionTypes?.map(
                    (questionType) => questionType.id,
                  )
                : [],
            setVisible: question?.authorSetVisible || false,
            setAnonymous:
              question?.isAnonymous ??
              courseFeatures?.asyncCentreDefaultAnonymous ??
              true,
          }}
          clearOnDestroy
          onFinish={(values) => onFinish(values)}
        >
          {dom}
        </Form>
      )}
    >
      <Form.Item
        name="QuestionAbstract"
        label="Question Abstract"
        tooltip="A short summary/description of the question (or the question itself)"
        required={true}
        rules={[
          { required: true, message: 'Please input your question abstract' },
          {
            max: 100,
            message: 'Question abstract must be less than 100 characters',
          },
        ]}
      >
        <Input
          placeholder="Stuck on Lab 3 part C"
          count={{
            show: true,
            max: 100,
          }}
        />
     ...
    </Modal>
  )
}

export default CreateAsyncQuestionModal

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the reason it's implemented like this is bc rc-form & input.textarea does not work with initialvalues or setvalues by default :(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

wait i'm a little confused, we're using rc-form? Like the package? I don't see any references to it in the code. Also personally I don't remember antd's textarea having issues with initialValues

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

whoops, think it was cuz i based this on my chatbot settings stuff - anyway rc-form is the underlying technology behind antd's forms, if the form element isn't directly under it doesn't observe the input, but i didn't need the extra div in this case (unlike w/ the chatbot settings due to the delete/reset button next to each field)

bhunt02 and others added 4 commits May 10, 2026 10:54
…ings/components/UpsertIFrameQuestionModal.tsx

Co-authored-by: Adam Fipke <adamfipke@gmail.com>
Co-authored-by: Adam Fipke <adamfipke@gmail.com>
…, tested manually (works!), improve upsert form given comments, add more rules to form so backend doesn't have to do all the heavy lifting of input validation
…issue w/ using decode vs. verify for JWT and adjusted tests where needed; added docstrings to embeddable-question controller & service
Copy link
Copy Markdown
Collaborator

@AdamFipke AdamFipke left a comment

Choose a reason for hiding this comment

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

Gave the whole thing another quick lookover. Looks good. All the comments I have are really minor that I don't think i'll need to review it again

Comment on lines +41 to +53
Object.keys(values).forEach((k) => {
(values as any)[k] = (values as any)[k]?.trim() ?? undefined
if (typeof (values as any)[k] === 'string' && !(values as any)[k]) (values as any)[k] = null
})
if (editingQuestion) {
await API.lti.embeddableQuestion.update(courseId, editingQuestion.id, {
...values,
} as any)
message.success('Successfully updated question!')
} else {
await API.lti.embeddableQuestion.create(courseId, {
...values,
} as any)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Object.keys(values).forEach((k) => {
(values as any)[k] = (values as any)[k]?.trim() ?? undefined
if (typeof (values as any)[k] === 'string' && !(values as any)[k]) (values as any)[k] = null
})
if (editingQuestion) {
await API.lti.embeddableQuestion.update(courseId, editingQuestion.id, {
...values,
} as any)
message.success('Successfully updated question!')
} else {
await API.lti.embeddableQuestion.create(courseId, {
...values,
} as any)
if (editingQuestion) {
await API.lti.embeddableQuestion.update(courseId, editingQuestion.id, values)
message.success('Successfully updated question!')
} else {
await API.lti.embeddableQuestion.create(courseId, values)

I think you might be able to simplify this lots now right?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the sanitization at the start is to ensure we can actually delete the instructions from the DB and not be storing a 0-character string

Comment on lines +77 to +84
<p className="text-zinc-600">
You cannot access this resource at this time. Try launching the HelpMe LTI tool - a
link to launch the tool should be visible in your Canvas course&#39;s navigation bar.
Contact your professor if this keeps happening after launching HelpMe.
</p>
<p className="text-zinc-600">
Alternatively, launch HelpMe in a new tab via the button below and log in:
</p>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<p className="text-zinc-600">
You cannot access this resource at this time. Try launching the HelpMe LTI tool - a
link to launch the tool should be visible in your Canvas course&#39;s navigation bar.
Contact your professor if this keeps happening after launching HelpMe.
</p>
<p className="text-zinc-600">
Alternatively, launch HelpMe in a new tab via the button below and log in:
</p>
<p className="text-zinc-600">
You cannot access this Embeddable AI-Feedback Helper at this time.
</p>
<p className="text-zinc-600">
Try launching the HelpMe LTI tool by clicking "HelpMe" in your Canvas course's navigation bar, login if need be, and then come back here.
</p>
<p className="text-zinc-600">
Alternatively, launch HelpMe in a new tab via the button below and log in. Then come back to this tab and refresh the page.
</p>
<p className="text-zinc-600">
Contact your professor if this keeps happening after launching HelpMe.
</p>

I have no idea if I got this correct actually. But I'm assuming that it's supposed to be "you're unauthenticated, please either do A or B to get an auth token and then you can continue". So I adjusted the text and made more paragraphs.

Since it wouldn't make sense to have a direct link to HelpMe since they wouldn't be able to access the embeddable question over there.

Also, I'm assuming that after they get this message they're supposed to refresh the page so I added that too. Though I'll admit it's been a while so idk if refreshing the page will reset any fields inside a Canvas quiz (it would be pretty bad if that were the case). Also, iirc some quizzes will lockout the user from coming back into the quiz but idk if we care to cover that just yet (i doubt the INDG 100 "quizzes"/assignments are going to be this restrictive).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

so it actually re-checks the auth state automatically (set interval of 5s) so i think it's okay... maybe adjust your last point. they don't need to refresh. the iframe can also be reloaded independently if a button is added to do so

Comment thread packages/frontend/middleware.ts
Comment thread packages/server/src/login/login.service.ts
bhunt02 added 2 commits May 11, 2026 19:30
…s pages, added grade prompt (chatbot sister PR changes as well for that), and API endpoints related to new embeddable question feedback. requires further work in appearances & interactivity for human grade & ai grade stuff, then addition of ability to export based on question or groups of questions. also: added properties to the embeddablequestion for open & close dates as well as optional name attribute
…ent' part of embeddable stuff, a bit convoluted. interfaces are fully impl'd and tested, need to write backend unit/integration tests and verify functionality w/ manual testing. creation of assessments allows use of previously existing or on-the-fly creation of embeddable questions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants