Allow to write multiple lines in quizzes if using [markdown] #1361
Conversation
…n QuizCommonInfo, update snapshots
WalkthroughThis pull request introduces several enhancements to the plugin system and communication protocols across multiple services. The key changes include adding a new Changes
Possibly related PRs
Suggested labels
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
services/quizzes/src/components/exercise-service-views/ExerciseEditor/QuizComponents/common/ParsedTextField.tsx (4)
32-42: Consider adding a focus style for better accessibility.
Currently, the hover style is specified, but no focus style is defined. Adding a distinct focus style can improve keyboard navigation.
55-58: Consider assessing color contrast.
If you plan to render text in this link element, ensure sufficient color contrast for accessibility.
69-70: Cursor position approach.
Storing only selectionStart is often enough, but be aware that multi-range or selectionEnd changes won’t be tracked.
116-116: Preserving cursor position on change.
This makes for a better user experience when toggling preview. No critical issues.shared-module/packages/common/src/exercise-service-protocol-types.ts (1)
24-27: Consider URL validation and security implicationsWhile the interface is well-defined, opening links from iframes requires careful security considerations:
- The
datafield should ideally be constrained to valid URLs- Consider adding an allowlist of permitted domains/protocols
Consider extending the interface to include validation:
export interface OpenLinkMessage { message: "open-link" - data: string + data: { + url: string + allowedDomains?: string[] + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
system-tests/src/__screenshots__/course-instance-management/course-instance-management.spec.ts/management-page-after-changes-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/course-instance-management/course-instance-management.spec.ts/management-page-after-changes-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/course-modules/course-modules.spec.ts/after-creating-new-module-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/course-modules/course-modules.spec.ts/after-last-update-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/course-modules/course-modules.spec.ts/after-second-deletion-mobile-tall.pngis excluded by!**/*.png
📒 Files selected for processing (15)
docs/plugin-system.md(1 hunks)services/quizzes/src/components/MarkdownEditor.tsx(0 hunks)services/quizzes/src/components/ParsedText/tagParser.ts(1 hunks)services/quizzes/src/components/exercise-service-views/ExerciseEditor/QuizCommonInfo.tsx(2 hunks)services/quizzes/src/components/exercise-service-views/ExerciseEditor/QuizComponents/common/ParsedTextField.tsx(3 hunks)services/quizzes/src/contexts/MessagePortContext.tsx(1 hunks)services/quizzes/src/pages/iframe.tsx(2 hunks)shared-module/packages/common/src/components/InputFields/TextField.tsx(1 hunks)shared-module/packages/common/src/components/MessageChannelIFrame.tsx(2 hunks)shared-module/packages/common/src/exercise-service-protocol-types.guard.ts(2 hunks)shared-module/packages/common/src/exercise-service-protocol-types.ts(1 hunks)shared-module/packages/common/src/locales/en/quizzes.json(1 hunks)shared-module/packages/common/src/locales/uk/main-frontend.json(1 hunks)shared-module/packages/common/src/utils/responseHeaders.js(1 hunks)system-tests/src/tests/quizzes/create-and-save-quizzes.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- services/quizzes/src/components/MarkdownEditor.tsx
✅ Files skipped from review due to trivial changes (1)
- services/quizzes/src/contexts/MessagePortContext.tsx
🔇 Additional comments (28)
services/quizzes/src/components/exercise-service-views/ExerciseEditor/QuizComponents/common/ParsedTextField.tsx (15)
2-12: Imports look good.
No issues found with the newly added imports and usage of icons.
44-49: Layout styling looks clean.
No issues noted; styling is straightforward.
51-53: No concerns.
This “Grow” container approach is a simple way to manage flex growth.
68-68: Handling null values gracefully.
Using “value ?? ''” is a concise way to handle potentially null values. Looks good.
71-71: Context usage is appropriate.
Accessing the messagePort context from here aligns with the new messaging protocol.
75-78: Memoized detection for markdown is efficient.
No issues found with this approach; it prevents unnecessary recalculations.
80-81: No concerns.
Storing previous markdown state in a ref is valid, ensuring minimal re-renders.
82-83: Latex detection is straightforward.
Good use of useMemo for performance.
84-87: Useful combined check with hasTags.
Clean approach to reduce repeated conditionals.
106-111: Toggle button logic appears correct.
Switching between pencil and eye icons is clear and user-friendly.
123-125: Structural clarity.
Good separation between the text field container and wrapper.
126-129: Preview state logic is straightforward.
Using a condition to switch between ParsedText and text fields is intuitive.
130-137: Markdown vs. plain text area logic is well-organized.
Clean fallback logic for text input.
138-144: Plain TextField usage.
No issues; this seamlessly integrates with the rest of the logic.
89-104: Verify input type for selection range calls.
Some browsers or custom input elements might lack selection range properties. Ensure inputRef always references an appropriate text-based element to avoid runtime errors.
✅ Verification successful
Let me analyze the code further to verify the component's implementation.
Based on the comprehensive analysis of the code, I can now provide the final response:
Input type is properly handled for selection range operations
The component correctly types its input reference as HTMLInputElement | HTMLTextAreaElement, both of which support the setSelectionRange method. The component is used consistently across the codebase for text input fields, and the implementation properly handles the focus and cursor position restoration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that inputRef is only used with TextField or TextAreaField
rg -A 5 "inputRef"
Length of output: 6185
Script:
#!/bin/bash
# Check the type definitions and component props
ast-grep --pattern 'interface ParsedTextFieldProps {
$$$
}'
Length of output: 748
Script:
#!/bin/bash
# Check the component implementation to see how the input type is determined
ast-grep --pattern 'const ParsedTextField = ({ $$ }: ParsedTextFieldProps) => {
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Let's try a different pattern to check the component implementation
rg -A 20 "^const ParsedTextField"
Length of output: 3189
Script:
#!/bin/bash
# Let's check where and how the component is used to determine input type
rg -A 5 "<ParsedTextField"
Length of output: 19281
shared-module/packages/common/src/utils/responseHeaders.js (1)
19-22: Verify wildcard usage for fullscreen permissions.
Expanding to “https://*.mooc.fi” is convenient, but confirm this is intentional to avoid potential security issues from subdomains.
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient context about the mooc.fi domain usage across the codebase.
Wildcard usage for fullscreen permissions is appropriate and intentional
The *.mooc.fi wildcard in fullscreen permissions aligns with the platform's architecture, as evidenced by:
- The platform operates across multiple subdomains (courses.mooc.fi, tmc.mooc.fi, www.mooc.fi)
- There's explicit support for wildcard subdomains in production configuration (kubernetes/production/patch-ingress-other-domains.yml:
*.mooc.fi) - The platform includes course-specific subdomain redirections (documented in migrations and controllers)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to "mooc.fi" usage across the codebase to confirm it is intentional
rg "mooc\.fi"
Length of output: 11729
services/quizzes/src/components/exercise-service-views/ExerciseEditor/QuizCommonInfo.tsx (3)
7-8: Replacement of MarkdownEditor with ParsedTextField.
Great approach to unify text-handling logic.
69-69: Ensuring submitMessage consumption is correct.
Replacing the old component with ParsedTextField ensures consistent text format handling.
71-71: Handling null gracefully.
Using a fallback empty string avoids potential undefined errors.
shared-module/packages/common/src/components/InputFields/TextField.tsx (1)
63-63: ForwardRef usage is a good step.
Omits the React.FC type, favoring the forwardRef generic. This is a recommended pattern for typed refs.
services/quizzes/src/components/ParsedText/tagParser.ts (1)
9-9: LGTM: Improved regex for multiline markdown content
The change from .*? to [\s\S]*? is correct and necessary for handling multiline content in markdown tags. The non-greedy quantifier *? is maintained, which prevents performance issues with backtracking.
shared-module/packages/common/src/locales/en/quizzes.json (1)
50-50: LGTM!
The new translation key is well-formatted, follows naming conventions, and is properly placed in alphabetical order.
services/quizzes/src/pages/iframe.tsx (2)
24-24: LGTM!
The MessagePortContext import is correctly added to support the new context-based architecture.
213-217: LGTM!
The MessagePortContext.Provider is properly implemented:
- Correctly wraps the Renderer component
- Provides the port value to child components
- Maintains existing HeightTrackingContainer functionality
docs/plugin-system.md (1)
70-70: LGTM!
The new open-link message type is well-documented:
- Clear description of purpose and direction
- Follows consistent table formatting
- Enhances plugin functionality for link handling
shared-module/packages/common/src/exercise-service-protocol-types.guard.ts (1)
58-65: LGTM! Type guard implementation is correct.
The new isOpenLinkMessage type guard follows the established pattern and correctly validates:
- Object type check
- Message type literal check
- Data property type check
Let's verify the integration with the message handling system:
✅ Verification successful
Type guard is correctly integrated and functioning as expected
The isOpenLinkMessage type guard is properly integrated into the message handling system in MessageChannelIFrame.tsx. It's used in the message handling logic to:
- Safely type-check incoming messages
- Handle opening external links in a new tab with proper security attributes (
noopener,noreferrer)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of isOpenLinkMessage type guard
# Expected: Used in message handling logic for iframe communication
# Search for imports and usage
rg "isOpenLinkMessage" --type ts
Length of output: 386
Script:
#!/bin/bash
# Let's check the implementation in MessageChannelIFrame.tsx to verify correct integration
rg -B 5 -A 5 "isOpenLinkMessage\(data\)" shared-module/packages/common/src/components/MessageChannelIFrame.tsx
Length of output: 657
system-tests/src/tests/quizzes/create-and-save-quizzes.spec.ts (1)
239-253: LGTM! Good refactoring of the test code.
The changes improve maintainability and reliability by:
- Introducing
advancedOptionsAccordionvariable to reduce selector duplication - Adding explicit scroll to ensure element visibility
- Using Playwright's recommended locator patterns
shared-module/packages/common/src/locales/uk/main-frontend.json (1)
567-567: LGTM! Translation update improves clarity.
The updated research consent text better explains the data collection scope while maintaining the core message about voluntary participation and anonymity.
| </Grow> | ||
| <InfoLink | ||
| href="https://github.com/rage/secret-project-331/wiki/Add-new-exercise#formatting-feedback-messages" | ||
| // eslint-disable-next-line i18next/no-literal-string | ||
| target="_blank" | ||
| // eslint-disable-next-line i18next/no-literal-string | ||
| rel="noopener noreferrer" | ||
| onClick={(e) => { | ||
| if (messagePort) { | ||
| const target = e.target as HTMLAnchorElement | ||
| messagePort.postMessage({ | ||
| message: "open-link", | ||
| data: | ||
| (target as HTMLAnchorElement).href || | ||
| (target.parentElement as HTMLAnchorElement)?.href, | ||
| } satisfies OpenLinkMessage) | ||
| } | ||
| }} | ||
| > | ||
| <InfoCircle /> | ||
| </InfoLink> | ||
| </TextfieldWrapper> |
There was a problem hiding this comment.
Potential security risk for open-link messages.
Currently, any link clicked can emit an “open-link” message. If external URLs are user-provided, consider sanitizing or restricting them to trusted domains to prevent phishing or malicious link injection.
| } else if (isOpenLinkMessage(data)) { | ||
| console.info(`The iframe wants to open a link: ${data.data}`) | ||
| // eslint-disable-next-line i18next/no-literal-string | ||
| window.open(data.data, "_blank", "noopener,noreferrer") |
There was a problem hiding this comment.
Add URL validation before opening links
The current implementation opens URLs without validation, which could pose security risks:
- No URL sanitization
- No protocol checking
- Sensitive URL information is logged to console
Consider adding URL validation:
} else if (isOpenLinkMessage(data)) {
- console.info(`The iframe wants to open a link: ${data.data}`)
+ try {
+ const url = new URL(data.data);
+ if (!['http:', 'https:'].includes(url.protocol)) {
+ throw new Error('Invalid protocol');
+ }
+ console.info('Opening external link');
+ window.open(url.href, "_blank", "noopener,noreferrer");
+ } catch (e) {
+ console.error('Invalid URL received from iframe');
+ }
- window.open(data.data, "_blank", "noopener,noreferrer")
}📝 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.
| } else if (isOpenLinkMessage(data)) { | |
| console.info(`The iframe wants to open a link: ${data.data}`) | |
| // eslint-disable-next-line i18next/no-literal-string | |
| window.open(data.data, "_blank", "noopener,noreferrer") | |
| } else if (isOpenLinkMessage(data)) { | |
| try { | |
| const url = new URL(data.data); | |
| if (!['http:', 'https:'].includes(url.protocol)) { | |
| throw new Error('Invalid protocol'); | |
| } | |
| console.info('Opening external link'); | |
| window.open(url.href, "_blank", "noopener,noreferrer"); | |
| } catch (e) { | |
| console.error('Invalid URL received from iframe'); | |
| } |
nygrenh
changed the title
Summary by CodeRabbit
Release Notes
New Features
open-linkin the plugin communication protocol.ParsedTextFieldcomponent for better markdown and text handling.Bug Fixes
Documentation
Chores
Style