Conversation
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean, focused PR that addresses both acceptance criteria from #5720.
CI passing. All three modal composables (image, link, math) consistently restore editor focus on close, and the new autofocus prop is correctly plumbed through to TipTap's native Editor constructor for new assessment questions.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| modalInitialData.value = {}; | ||
| editingNodePos.value = null; | ||
| closeModalBase(); | ||
| editor.value?.commands.focus(); |
There was a problem hiding this comment.
praise: Good consistent pattern across all three modal composables — editor.value?.commands.focus() with optional chaining ensures safe focus restoration regardless of editor state. This directly addresses the focus-on-modal-close requirement.
| const isFocused = ref(false); | ||
|
|
||
| const initializeEditor = (content, mode = 'edit') => { | ||
| const initializeEditor = (content, mode = 'edit', { autofocus = false } = {}) => { |
There was a problem hiding this comment.
praise: Clean approach — destructuring with a default empty object keeps the existing call sites working while making autofocus opt-in. Leveraging TipTap's native autofocus option rather than rolling a custom solution is the right call.
Add autofocus prop to TipTapEditor, threaded through to the TipTap Editor constructor. AssessmentItemEditor sets shouldAutofocusQuestion when it mounts with an empty question (i.e. a newly created one), which passes autofocus=true to the TipTapEditor so the ProseMirror view receives focus as soon as it initializes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
@nucleogenesis this looks good to me, focus is updated thank you. small request for an admittedly out of scope css update just to reduce the number of PRs.
There was a problem hiding this comment.
this is absolutely out of scope for this fix, but request that we add
border: 1px solid #b4b4b4;
for consistency with the other text boxes, without having to open a separate PR for this
There was a problem hiding this comment.
I've added the line:
nucleogenesis
force-pushed
the
5720--focus-trappin
branch
from
40672ee to
1fba461
Compare
3 participants
Summary
References
Closes #5720
AI usage
Claude was consulted for assistance throughout