enhance(packages/shared-components): replace tagcloud with wordcloud and filter user input#4947
enhance(packages/shared-components): replace tagcloud with wordcloud and filter user input#4947jabbadizzleCode wants to merge 14 commits intov3from
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new native D3-based word cloud system is introduced via a new Changes
Sequence DiagramsequenceDiagram
participant Client as React Component
participant Layout as Layout Engine
participant Render as D3 Renderer
participant DOM as Browser DOM
participant User as User
Client->>Layout: computeWordCloudLayout(words, options)
activate Layout
Layout->>Layout: Normalize options & sanitize words
Layout->>Layout: Measure text dimensions
Layout->>Layout: Assign font sizes (linear/sqrt/log scale)
Layout->>Layout: Spiral search placement with collision detection
alt Words don't fit
Layout->>Layout: Relayout: shrink font bounds & retry
end
Layout-->>Client: LayoutResult (placed, omitted)
deactivate Layout
Client->>Render: renderWordCloud(container, layoutResult, options)
activate Render
Render->>DOM: Create SVG with root group
Render->>DOM: Bind words to text elements (D3 join)
Render->>DOM: Transition enter/update/exit
Render-->>Client: RendererHandle
deactivate Render
User->>DOM: Hover over word
activate Render
DOM->>Render: mouseover event
Render->>DOM: Show tooltip, scale text
Render->>Client: onWordMouseOver callback
deactivate Render
User->>DOM: Move mouse away
activate Render
DOM->>Render: mouseout event
Render->>DOM: Hide tooltip, reset scale
Render->>Client: onWordMouseOut callback
deactivate Render
Client->>Render: update(newLayoutResult)
activate Render
Render->>DOM: Transition to new layout
deactivate Render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
dosubot
bot
added
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
packages/shared-components/src/charts/ElementWordcloud.tsx (8)
5-5: Avoid deep self-import; prefer local path to keep package boundaries clean.Importing from your own package name with a deep
/srcpath can break consumers and bundlers. Use a relative import.-import { CHART_COLORS } from '@klicker-uzh/shared-components/src/constants' +import { CHART_COLORS } from '../constants'
7-7: Consider lighter or lazy NLP to reduce bundle/SSR cost.
compromiseis sizeable. If possible, switch to a smaller build (e.g.,compromise/one) or lazy‑load it only on the client before processing.Would you like a follow‑up patch that lazy‑loads
compromiseand memoizes the transform?
9-9: Dynamic import is fine; optionally add a loading fallback.Adding a lightweight fallback prevents layout jumps.
-import dynamic from 'next/dynamic' +import dynamic from 'next/dynamic' +const Loading = () => <div className="h-full w-full p-4" />
14-16: Good call disabling SSR for react-wordcloud.This avoids d3-cloud SSR issues. Consider adding a
loadingfallback for UX.-const ReactWordcloud = dynamic(() => import('react-wordcloud'), { - ssr: false, -}) +const ReactWordcloud = dynamic(() => import('react-wordcloud'), { + ssr: false, + loading: () => <Loading />, +})
59-69: Use a Set and hoist ignore list; make it locale-aware later.
- Set lookup is O(1) and avoids array re-allocations per render.
- Hoist to module scope; consider per‑locale stopwords.
- const frequencies: Record<string, number> = {} - const ignoreTags: string[] = [ + const frequencies: Record<string, number> = {} + const IGNORE_TAGS = new Set<string>([ 'Pronoun', 'Determiner', 'Conjunction', 'Preposition', 'Auxiliary', // TODO: adjust tags - ] + ])Happy to provide a German/English stopword preset.
86-90: Cap and sort words to protect layout performance.
d3-cloudscales poorly with large word sets. Sort and limit to the top N (e.g., 150).- const processedData = Object.entries(frequencies).map(([value, count]) => ({ - text: value, - value: count, - })) + const processedData = Object.entries(frequencies) + .map(([value, count]) => ({ text: value, value: count })) + .sort((a, b) => b.value - a.value) + .slice(0, 150)
91-95: Rotation guard looks good; optionally sync “rotations” with it.You already zero‑rotate single words. Also set
rotationsto1when there’s only one word.
104-120: Tweak key/rotations; rely on prop changes for re-layout.
- Using
keytied only to length can miss updates when counts change. Let the component re‑layout onwordschanges, or derive a better key.- Align
rotationswith the single‑word case.- <ReactWordcloud - key={`wc-${processedData.length}`} + <ReactWordcloud words={processedData} options={{ enableTooltip: false, deterministic: true, randomSeed: '42', padding: 5, - rotations: 2, - rotationAngles: rotationAngles, + rotations: processedData.length > 1 ? 2 : 1, + rotationAngles, fontSizes: [textSize.min, textSize.max], colors: CHART_COLORS, transitionDuration: 1000, scale: 'log', spiral: 'archimedean', }} />If you want a stable re‑mount key, hash
processedDatainstead of using its length.packages/shared-components/package.json (5)
37-39: Avoid exposing d3-cloud as a peerDependency; it’s a transitive of react-wordcloud
- react-wordcloud uses d3-cloud internally; consumers shouldn’t need to install it separately. Keeping it as a peer widens your peer surface and risks version skew without benefit. Recommend removing "d3-cloud" from peerDependencies. d3-cloud’s latest is 1.2.7 if you still need it directly. (npm.io)
Apply:
"peerDependencies": { @@ - "d3-cloud": "^1.2.7",
16-21: Clean up deprecated TagCloud dev deps
- If TagCloud was replaced, remove "react-tagcloud" and "@types/react-tagcloud" from devDependencies to reduce install surface and avoid confusion. The package is minimally maintained and no longer used. (npmjs.com)
"devDependencies": { @@ - "@types/react-tagcloud": "^2.3.2", @@ - "react-tagcloud": "~2.3.3",
37-38: Peer strategy check: compromise as peer is fine; consider adding to devDependencies for local builds/storybook
- Using compromise as a peer aligns with your other runtime libs. If you develop this package in isolation, add compromise/react-wordcloud to devDependencies to ensure local TS builds/tests don’t rely on hoisting. Compromise 14.14.4 is the latest line and has no known vulns at that version. (npmjs.com)
Do you build this package outside the app workspace? If yes, I can add the minimal devDependencies in a follow-up.
Also applies to: 48-48
28-53: Supply-chain hygiene nudge (recent npm incidents)
- Given recent npm compromises, consider pinning critical peers in the app, enabling provenance/2FA for publishers, and auditing lockfiles after this change. (cybernews.com)
48-48: Address stale react-wordcloud dependency
- react-wordcloud@1.2.7 (published Aug 8 2020) declares peer "react": "^16.13.0", which may warn or break under React 19; upgrade to @cyberblast/react-wordcloud@1.2.10 (latest Jun 19 2024, peer "react": ">=16.13.0") or thoroughly test runtime and build logs under React 19.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/shared-components/package.json(2 hunks)packages/shared-components/src/charts/ElementWordcloud.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/shared-components/src/charts/ElementWordcloud.tsx (1)
packages/shared-components/src/constants.ts (1)
CHART_COLORS(20-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: format
- GitHub Check: cypress-run-cloud
- GitHub Check: check
- GitHub Check: test
🔇 Additional comments (1)
packages/shared-components/src/charts/ElementWordcloud.tsx (1)
41-49: FreeText responses already shaped withvalueandcountTheIFreeElementEvaluationResults.responsesitems include bothvalue: stringandcount: number, soinstance.results.responsesis already the correct shape and no extra mapping is needed.
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/frontend-manage/src/components/evaluation/filter/WordCloudFilter.tsx (2)
377-385: IncludesetWordCloudTagsin the dependency array.The
useEffectusessetWordCloudTagsfrom props but doesn't include it in the dependency array. While parent components often provide stable callbacks viauseCallback, omitting it can lead to stale closures if the callback changes.Apply this diff:
useEffect(() => { const selectedTags: string[] = [] Object.entries(checked).forEach(([tagName, isChecked]) => { if (isChecked) { selectedTags.push(tagName) } }) setWordCloudTags(selectedTags) - }, [checked]) + }, [checked, setWordCloudTags])
386-386: TODO: Consider multi-language NLP support.The comment suggests exploring
tiny-taggerandnlp.jsfor German/English support, which could expand the filter's utility beyond English-only linguistic tagging.Do you want me to research and propose an implementation approach for multi-language support, or open a new issue to track this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/frontend-manage/src/components/evaluation/ActivityEvaluation.tsx(3 hunks)apps/frontend-manage/src/components/evaluation/ElementChart.tsx(3 hunks)apps/frontend-manage/src/components/evaluation/ElementEvaluation.tsx(9 hunks)apps/frontend-manage/src/components/evaluation/EvaluationFooter.tsx(4 hunks)apps/frontend-manage/src/components/evaluation/elements/ChoicesEvaluation.tsx(3 hunks)apps/frontend-manage/src/components/evaluation/elements/FCEvaluation.tsx(2 hunks)apps/frontend-manage/src/components/evaluation/elements/FTEvaluation.tsx(4 hunks)apps/frontend-manage/src/components/evaluation/elements/NREvaluation.tsx(3 hunks)apps/frontend-manage/src/components/evaluation/elements/SEEvaluation.tsx(3 hunks)apps/frontend-manage/src/components/evaluation/filter/WordCloudFilter.tsx(1 hunks)packages/prisma-data/src/data/data/TEST.ts(2 hunks)packages/prisma-data/src/data/seedTEST.ts(4 hunks)packages/shared-components/src/charts/ElementWordcloud.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/prisma-data/src/data/seedTEST.ts (1)
packages/prisma-data/src/data/data/TEST.ts (1)
QUESTION_ID_TYPE(785-796)
apps/frontend-manage/src/components/evaluation/filter/WordCloudFilter.tsx (1)
packages/graphql/src/schema/elementData.ts (1)
ElementType(25-27)
apps/frontend-manage/src/components/evaluation/EvaluationFooter.tsx (1)
apps/frontend-manage/src/components/evaluation/filter/WordCloudFilter.tsx (1)
WordCloudFilter(367-427)
packages/shared-components/src/charts/ElementWordcloud.tsx (2)
packages/graphql/src/schema/elementData.ts (1)
ElementType(25-27)packages/shared-components/src/constants.ts (1)
CHART_COLORS(20-33)
packages/prisma-data/src/data/data/TEST.ts (2)
packages/graphql/src/schema/elementData.ts (1)
ElementType(25-27)packages/graphql/src/schema/practiceQuiz.ts (1)
PublicationStatus(24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: cypress-run-cloud
- GitHub Check: claude-review
- GitHub Check: Analyze (javascript)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (7)
apps/frontend-manage/src/components/evaluation/elements/SEEvaluation.tsx (1)
10-10: LGTM! Clean prop threading.The
wordCloudTagsprop is correctly added to the interface, function signature, and forwarded toElementChart. The optional typing ensures backward compatibility.Also applies to: 21-21, 31-31
apps/frontend-manage/src/components/evaluation/EvaluationFooter.tsx (1)
41-41: LGTM! WordCloudFilter integration is correct.The
setWordCloudTagscallback is properly threaded through props and the conditional rendering ofWordCloudFilterforWORD_CLOUDchart type is implemented correctly.Also applies to: 57-57, 174-179
apps/frontend-manage/src/components/evaluation/ElementChart.tsx (1)
22-22: LGTM! Correct prop forwarding to ElementWordcloud.The
wordCloudTagsprop is properly added and forwarded only to theElementWordcloudcomponent when renderingWORD_CLOUDchart type, maintaining the existing behavior for other chart types.Also applies to: 33-33, 61-61
apps/frontend-manage/src/components/evaluation/elements/FCEvaluation.tsx (1)
11-11: LGTM! Clean prop threading.The
wordCloudTagsprop is correctly added to the interface, destructured in the function signature, and forwarded toElementChart.Also applies to: 14-19, 36-36
apps/frontend-manage/src/components/evaluation/filter/WordCloudFilter.tsx (1)
13-360: LGTM! Well-structured WordCloudFilter component.The component provides a comprehensive UI for linguistic feature filtering with:
- Clear category organization (Nouns, Verbs, Adjectives, etc.)
- Helpful tooltips with examples
- Proper state management via
useStateanduseEffect- Correct handling of
ElementType.NumericaldisablingThe
checkboxGroupsconstant is large but well-organized and maintainable.Also applies to: 367-427
apps/frontend-manage/src/components/evaluation/ElementEvaluation.tsx (2)
44-44: LGTM! Comprehensive prop threading across evaluation components.The
wordCloudTagsprop is correctly added to the interface and consistently forwarded to all relevant child evaluation components (ChoicesEvaluation,NREvaluation,FTEvaluation,SEEvaluation,FCEvaluation), enabling the word cloud feature throughout the evaluation flow.Also applies to: 63-63, 159-159, 174-174, 189-189, 200-200, 233-233
222-222: TODO comments for case study and content word clouds.The comments note potential future support for word clouds in case study and content evaluations. These are reasonable deferments for the current scope.
Also applies to: 243-243
dosubot
bot
added
size:XXL
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/shared-components/src/charts/ElementWordcloud.tsx (1)
136-145: Avoid double-counting and sanitize tokens in Premium modeCurrent loop matches each tag separately; tokens wearing multiple tags get counted multiple times. Also words aren’t normalized (case/punctuation).
Use a single union pattern and dedupe:
- } else { - for (const tag of tags) { - const doc = nlp(response.value) - doc - .match(tag) - .out('array') - .forEach((word: string) => { - frequencies[word] = (frequencies[word] || 0) + response.count - }) - } - } + } else { + const doc = nlp(response.value) + const pattern = tags.join(' OR ') + const words = doc.match(pattern).out('array') as string[] + const unique = new Set( + words + .map((w) => w.replace(/[.,;:!?]/g, '').toLowerCase().trim()) + .filter(Boolean) + ) + unique.forEach((w) => { + frequencies[w] = (frequencies[w] || 0) + response.count + }) + }
🧹 Nitpick comments (3)
packages/shared-components/src/charts/ElementWordcloud.tsx (2)
121-134: Tokenization perf nit: reuse tokenizerYou reinstantiate wink-tokenizer for each tag/response. Consider creating it once per render.
Example:
- for (const tag of tags) { - const tokenizer = new winkTokenizer() + const tokenizer = winkTokenizer() + for (const tag of tags) {
245-251: Locale check robustness (optional)WordCloudFilter treats only exact 'en' as English. If router.locale can be 'en-US'/'de-CH', prefer startsWith.
- language={locale} + language={locale}And in WordCloudFilter:
- const isEnglish = !language || language === 'en' + const isEnglish = !language || language.toLowerCase().startsWith('en')packages/shared-components/src/WordCloudFilter.tsx (1)
920-924: Locale handling (optional)Broaden English detection to include variants.
- const isEnglish = !language || language === 'en' // default to English + const isEnglish = !language || language.toLowerCase().startsWith('en') // default to English
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/frontend-manage/src/components/evaluation/ElementChart.tsx(3 hunks)apps/frontend-manage/src/components/evaluation/ElementEvaluation.tsx(2 hunks)apps/frontend-manage/src/components/evaluation/EvaluationFooter.tsx(1 hunks)packages/i18n/messages/de.ts(2 hunks)packages/i18n/messages/en.ts(2 hunks)packages/shared-components/package.json(4 hunks)packages/shared-components/src/WordCloudFilter.tsx(1 hunks)packages/shared-components/src/charts/ElementWordcloud.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/frontend-manage/src/components/evaluation/ElementChart.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/shared-components/src/WordCloudFilter.tsx (1)
packages/graphql/src/schema/elementData.ts (1)
ElementType(25-27)
packages/shared-components/src/charts/ElementWordcloud.tsx (3)
packages/graphql/src/schema/elementData.ts (1)
ElementType(25-27)packages/shared-components/src/constants.ts (1)
CHART_COLORS(20-33)packages/shared-components/src/WordCloudFilter.tsx (1)
WordCloudFilter(889-971)
🪛 Biome (2.1.2)
packages/shared-components/src/WordCloudFilter.tsx
[error] 903-903: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 904-904: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 911-911: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 912-912: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
packages/shared-components/src/charts/ElementWordcloud.tsx
[error] 68-68: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 69-69: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 72-72: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 75-75: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 GitHub Actions: Check linting
packages/shared-components/package.json
[error] 1-1: Dependency specifiers do not match the lockfile for 'stopword' and 'wink-tokenizer'.
🪛 GitHub Actions: Check syncpack conformity
packages/shared-components/package.json
[error] 1-1: pnpm recursive install failed due to outdated lockfile. Stopword and wink-tokenizer version specifiers in manifest do not match the lockfile.
🪛 GitHub Actions: Check typescript types
packages/shared-components/package.json
[error] 1-1: Command 'pnpm recursive install' failed with exit code 1 due to frozen-lockfile mismatch. Lockfile is not up to date with manifest. Specifiers in lockfile do not match manifest for dependencies: stopword (lockfile: ^3.1.5, manifest: ~3.1.5), wink-tokenizer (lockfile: ^5.3.0, manifest: ~5.3.0). Run 'pnpm install --no-frozen-lockfile' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: SonarCloud
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/i18n/messages/de.ts (2)
403-405: LGTM: new Word Cloud i18n keysGerman labels/tooltips align with the new filter UI and premium mode behavior.
2590-2600: LGTM: evaluation messages for Word CloudKeys cover mode, language, and empty states; phrasing matches UI logic.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/prisma-data/src/data/data/TEST.ts (1)
963-964: Confirmed: total count issue resolved.The previous review flagged that
total: 10should be30to match the response count. This has been correctly updated tototal: 30, which now matches the 30 response entries provided.
🧹 Nitpick comments (1)
packages/shared-components/src/WordCloudFilter.tsx (1)
920-920: Fix typo in variable name.
tagCategoritesshould betagCategories.- const tagCategorites = + const tagCategories = mode === WordCloudMode.STANDARD ? tagsBasic : tagsAdvanced const isEnglish = !language || language === 'en' // default to English - const numSeparators = tagCategorites.length - 1 + const numSeparators = tagCategories.length - 1And update line 937:
- {tagCategorites.map((filterCategory, categoryIndex) => { + {tagCategories.map((filterCategory, categoryIndex) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/prisma-data/src/data/data/TEST.ts(2 hunks)packages/shared-components/src/WordCloudFilter.tsx(1 hunks)packages/shared-components/src/charts/ElementWordcloud.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/shared-components/src/WordCloudFilter.tsx (1)
packages/graphql/src/schema/elementData.ts (1)
ElementType(25-27)
packages/shared-components/src/charts/ElementWordcloud.tsx (3)
packages/graphql/src/schema/elementData.ts (1)
ElementType(25-27)packages/shared-components/src/constants.ts (1)
CHART_COLORS(20-33)packages/shared-components/src/WordCloudFilter.tsx (1)
WordCloudFilter(889-971)
packages/prisma-data/src/data/data/TEST.ts (2)
packages/graphql/src/schema/elementData.ts (1)
ElementType(25-27)packages/graphql/src/schema/practiceQuiz.ts (1)
PublicationStatus(24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (javascript)
- GitHub Check: cypress-run-cloud
- GitHub Check: SonarCloud
- GitHub Check: build
- GitHub Check: check
- GitHub Check: test
- GitHub Check: format
🔇 Additional comments (2)
packages/shared-components/src/charts/ElementWordcloud.tsx (1)
59-79: Confirmed: hooks ordering issue resolved.The previous review flagged that hooks were called after an early return. This has been correctly fixed—all hooks are now declared before the conditional return statement.
packages/shared-components/src/WordCloudFilter.tsx (1)
896-918: Confirmed: hooks ordering issue resolved.The previous review flagged that the early return for
ElementType.Numericaloccurred before hooks. This has been correctly fixed—all hooks (useTranslations,useState,useEffect) are now invoked before the conditional return.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/shared-components/src/WordCloudFilter.tsx (1)
901-910: Add missing dependency to effect.Include setWordCloudTags in deps to avoid stale closure.
- }, [checked]) + }, [checked, setWordCloudTags])
🧹 Nitpick comments (8)
cypress/cypress/e2e/O-live-quiz-workflow.cy.ts (2)
4773-4774: Brittle assertion on "50".Assert on expected words/tokens (or number of words) instead of a literal “50”.
4823-4865: Replace fixed waits with UI-state assertions to reduce flakiness.Prefer should/contains checks over cy.wait(650) after toggles.
packages/shared-components/src/charts/ElementWordcloud.tsx (3)
69-84: Robust sentence splitting (avoid dropping last sentence).Regex misses the trailing sentence without terminal punctuation. Use compromise.
- const regex = /[^.;:!?]+[.;:!?]+(?=\s|$)/g - const sentences = response.value.match(regex)?.map((s) => s.trim()) + const sentences = nlp(response.value).sentences().out('array').map((s: string) => s.trim())
110-119: Normalize tokens in PREMIUM mode (strip punctuation/whitespace).Lowercasing alone yields duplicates like “word,” vs “word”.
- const words = tags - .flatMap((tag) => doc.match(tag).out('array')) - .map((w) => w.toLowerCase()) + const words = tags + .flatMap((tag) => doc.match(tag).out('array')) + .map((w) => w.replace(/[.,;:!?]/g, '').toLowerCase().trim()) + .filter(Boolean)
209-216: Memoize heavy computations to avoid rework on each render.Wrap frequencies/processedData in useMemo keyed by [data, tags, mode, applyFilter, stopwords].
Also applies to: 218-224
packages/shared-components/src/WordCloudFilter.tsx (1)
861-870: Remove unused prop or use it.descriptionLanguage is defined but never used.
packages/shared-components/src/FontSizeButtons.tsx (1)
31-51: Add accessible names to icon-only buttonsIcon-only controls need an accessible name for screen readers. Provide aria-labels (localized), or titles.
Example:
<Button aria-label={t('manage.evaluation.decreaseFontSize')} ... > <Button.Icon withoutLabel icon={faMinus} /> </Button> <Button aria-label={t('manage.evaluation.increaseFontSize')} ... > <Button.Icon withoutLabel icon={faPlus} /> </Button>If keys don’t exist, consider adding them alongside fontSize.
packages/i18n/messages/de.ts (1)
2590-2603: Polish German wording and Swiss orthography
- “Erweitertes Set an Filter” → “Erweitertes Set von Filtern”
- Use “Schriftgrössen” (consistent with Swiss style used elsewhere like “Schriftgrösse”).
- wordCloudModePremiumTooltip: - 'Premium (nur auf Englisch): Erweitertes Set an Filter, z. B. Nomen, Verben, ...', + wordCloudModePremiumTooltip: + 'Premium (nur auf Englisch): Erweitertes Set von Filtern, z. B. Nomen, Verben, ...', @@ - wordCloudNoResponsesDisplayed: - 'Keine Antworten werden aufgrund der aktuellen Filtereinstellungen oder der angegebenen Schriftgrößen angezeigt 😰.', + wordCloudNoResponsesDisplayed: + 'Keine Antworten werden aufgrund der aktuellen Filtereinstellungen oder der angegebenen Schriftgrössen angezeigt 😰.',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/frontend-manage/src/components/evaluation/ElementChart.tsx(4 hunks)cypress/cypress/e2e/O-live-quiz-workflow.cy.ts(1 hunks)cypress/cypress/fixtures/O-live-quiz.json(1 hunks)cypress/cypress/fixtures/questions.json(2 hunks)packages/i18n/messages/de.ts(2 hunks)packages/i18n/messages/en.ts(2 hunks)packages/shared-components/src/FontSizeButtons.tsx(1 hunks)packages/shared-components/src/WordCloudFilter.tsx(1 hunks)packages/shared-components/src/charts/ElementWordcloud.tsx(4 hunks)packages/shared-components/src/hooks/useTextPresenceObserverWordCloud.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cypress/cypress/fixtures/O-live-quiz.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/i18n/messages/en.ts
- apps/frontend-manage/src/components/evaluation/ElementChart.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-11T15:07:47.117Z
Learnt from: sjschlapbach
PR: uzh-bf/klicker-uzh#4550
File: apps/frontend-manage/src/components/courses/modals/TemplateConversionModal.tsx:132-138
Timestamp: 2025-03-11T15:07:47.117Z
Learning: The translation keys for template conversion error messages (`noInstances`, `resourcesRequiredMissing`, `noResourceAccessRequired`) are properly defined in the translation files (packages/i18n/messages/en.ts and packages/i18n/messages/de.ts) for the KlickerUZH project.
Applied to files:
packages/i18n/messages/de.ts
🧬 Code graph analysis (2)
packages/shared-components/src/WordCloudFilter.tsx (1)
packages/graphql/src/schema/elementData.ts (1)
ElementType(25-27)
packages/shared-components/src/charts/ElementWordcloud.tsx (4)
packages/graphql/src/schema/elementData.ts (1)
ElementType(25-27)packages/shared-components/src/hooks/useTextPresenceObserverWordCloud.ts (1)
useTextPresenceObserverWordCloud(3-28)packages/shared-components/src/constants.ts (1)
CHART_COLORS(20-33)packages/shared-components/src/WordCloudFilter.tsx (1)
WordCloudFilter(872-1027)
🪛 Biome (2.1.2)
packages/shared-components/src/charts/ElementWordcloud.tsx
[error] 253-253: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 254-254: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: cypress-run-cloud
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: build
- GitHub Check: test
- GitHub Check: check
🔇 Additional comments (3)
packages/shared-components/src/charts/ElementWordcloud.tsx (1)
91-100: Verify wink-tokenizer tag set alignment.Confirm tokens provide tags for all “basic” filters (e.g., 'ordinal'). Some tags may not exist in wink-tokenizer.
packages/i18n/messages/de.ts (1)
403-405: LGTM for shared keysNeue Keys shared.filter und shared.listExamples passen und sind konsistent.
cypress/cypress/fixtures/questions.json (1)
285-291: Verify tokenizer-category expectations used in fixturesThe filteredResponses buckets (ordinal, emoticon, emoji, currency, etc.) tightly couple tests to the tokenizer’s output. Upgrading wink-tokenizer or changing options could break these.
- Confirm the tokenizer version/config used in app matches these expectations for EN/DE.
- If variance is possible, consider asserting only on categories used by UI logic (or derive expected tokens at test runtime via the same tokenizer to reduce brittleness).
Also applies to: 361-406
| cy.get(`[data-cy="start-live-quiz-${this.data.modes.name}"]`).click() | ||
| cy.get('[data-cy="next-block-timeline"]').click() |
There was a problem hiding this comment.
Start/navigate the correct Word Cloud quiz (wrong identifier).
Use liveQuizWordCloud.name, not modes.name.
- cy.get(`[data-cy="start-live-quiz-${this.data.modes.name}"]`).click()
+ cy.get(`[data-cy="start-live-quiz-${this.data.liveQuizWordCloud.name}"]`).click()- cy.get(`[data-cy="live-quiz-cockpit-${this.data.modes.name}"]`).click()
+ cy.get(`[data-cy="live-quiz-cockpit-${this.data.liveQuizWordCloud.name}"]`).click()Also applies to: 4761-4761
🤖 Prompt for AI Agents
In cypress/cypress/e2e/O-live-quiz-workflow.cy.ts around lines 4695-4696 (and
also at line 4761), the test is clicking the wrong selector using
this.data.modes.name; replace it with this.data.liveQuizWordCloud.name so the
data-cy attribute targets the Word Cloud live quiz. Update both occurrences to
use liveQuizWordCloud.name and keep the subsequent navigation step
(next-block-timeline) unchanged.
| @@ -0,0 +1,28 @@ | |||
| import { useEffect, useState } from 'react' | |||
There was a problem hiding this comment.
Fix missing React types and strengthen typing
- Using React.RefObject without importing React/types can fail compilation.
- Use proper types and return annotation.
Apply:
-import { useEffect, useState } from 'react'
+import { useEffect, useState } from 'react'
+import type { DependencyList, RefObject } from 'react'
-export function useTextPresenceObserverWordCloud(
- containerRef: React.RefObject<HTMLElement | null>,
- deps: any[] = []
-) {
+export function useTextPresenceObserverWordCloud(
+ containerRef: RefObject<HTMLElement | null>,
+ deps: DependencyList = []
+): boolean {Also applies to: 3-6, 27-28
🤖 Prompt for AI Agents
In packages/shared-components/src/hooks/useTextPresenceObserverWordCloud.ts
around lines 1 and 3-6 and 27-28, add explicit React types and a return
annotation: import React types (e.g., import React, { useEffect, useState, type
RefObject } from 'react' or import type { RefObject } from 'react') at the top,
type the ref variables as RefObject<HTMLElement> (or the appropriate element
type), annotate state with proper types (e.g., useState<boolean> or
useState<YourType>), and add an explicit return type for the hook (e.g., (): {
ref: RefObject<HTMLElement>; isPresent: boolean } or the actual shape returned).
Ensure all usages and variables on lines 27-28 are updated to use the new types.
| useEffect(() => { | ||
| const el = containerRef.current | ||
| if (!el) return | ||
|
|
||
| const read = () => { | ||
| const present = !!el.textContent?.trim().length | ||
| setHasText((prev) => (prev !== present ? present : prev)) | ||
| } | ||
| read() | ||
|
|
||
| const mo = new MutationObserver(() => { | ||
| requestAnimationFrame(read) | ||
| }) | ||
| mo.observe(el, { childList: true, subtree: true, characterData: true }) | ||
|
|
||
| return () => mo.disconnect() | ||
| }, [containerRef, ...deps]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Ensure observer (re)attaches when the underlying element changes
Depending on the ref object won’t re-run the effect when ref.current swaps, leading to a stale or never-attached observer.
Apply:
- }, [containerRef, ...deps])
+ }, [containerRef.current, ...deps])Note: this also covers the case where current is initially null and set later.
📝 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.
| useEffect(() => { | |
| const el = containerRef.current | |
| if (!el) return | |
| const read = () => { | |
| const present = !!el.textContent?.trim().length | |
| setHasText((prev) => (prev !== present ? present : prev)) | |
| } | |
| read() | |
| const mo = new MutationObserver(() => { | |
| requestAnimationFrame(read) | |
| }) | |
| mo.observe(el, { childList: true, subtree: true, characterData: true }) | |
| return () => mo.disconnect() | |
| }, [containerRef, ...deps]) | |
| useEffect(() => { | |
| const el = containerRef.current | |
| if (!el) return | |
| const read = () => { | |
| const present = !!el.textContent?.trim().length | |
| setHasText((prev) => (prev !== present ? present : prev)) | |
| } | |
| read() | |
| const mo = new MutationObserver(() => { | |
| requestAnimationFrame(read) | |
| }) | |
| mo.observe(el, { childList: true, subtree: true, characterData: true }) | |
| return () => mo.disconnect() | |
| }, [containerRef.current, ...deps]) |
🤖 Prompt for AI Agents
In packages/shared-components/src/hooks/useTextPresenceObserverWordCloud.ts
around lines 9 to 25, the effect currently depends on the ref object
(containerRef) which doesn't change when containerRef.current swaps, so the
MutationObserver can become stale or never attach; modify the effect to depend
on containerRef.current (e.g., const el = containerRef.current captured at start
and include that value in the dependency array) so the effect re-runs and
re-attaches the observer whenever the underlying DOM element changes, ensure the
cleanup disconnects the previous observer before returning, and handle the case
where el is null by early-returning so attachment occurs later when current is
set.
…loud # Conflicts: # packages/shared-components/package.json # pnpm-lock.yaml
|
Co-authored-by: Patrick Louis Aldover <patrick.aldover@df.uzh.ch>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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.
Actionable comments posted: 11
♻️ Duplicate comments (1)
packages/shared-components/src/charts/ElementWordcloud.tsx (1)
139-147:⚠️ Potential issue | 🔴 CriticalMove the unsupported-type return below the remaining hooks.
The early return on Line 141 still happens before the
useMemo/useCallbackhooks below, so a type change can execute a different hook order between renders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-components/src/charts/ElementWordcloud.tsx` around lines 139 - 147, The early return that checks supportedElementTypes and instance.type causes hooks order to change; move the supported-type check and its UserNotification render to after the useMemo/useCallback hooks in ElementWordcloud so all hooks always run in the same order, e.g., keep the supportedElementTypes = [ElementType.Numerical, ElementType.FreeText] and instance.type check but defer returning the warning until after the memoized and callback hooks have been declared (so useMemo/useCallback invocations in this component always execute regardless of the element type).
🧹 Nitpick comments (3)
packages/word-cloud/src/shims.d.ts (1)
1-17: Replace theany-based D3 shims with real typings or a narrower typed facade.
select(): anyand the bare transition stub effectively turn the renderer path into unchecked code, so D3 API drift will not be caught at compile time. Please prefer the official@types/d3-*packages, or model the exact fluent surface you use and add a compile-only smoke test aroundrender.ts.
Based on learnings "After making significant changes to types, test the build process for all dependent packages and consider adding TypeScript compilation tests for edge cases." and as per coding guidelines "Use TypeScript strict mode everywhere".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/word-cloud/src/shims.d.ts` around lines 1 - 17, The current D3 shims use any and broad stubs (declare module 'd3-transition', the Scale interface and select(): any) making the renderer untyped; replace these with proper typings by installing/using the official `@types/d3-`* packages (e.g., `@types/d3-selection`, `@types/d3-scale`, `@types/d3-transition`) or narrow the facade to exactly the fluent methods you use (e.g., the Scale methods domain/range and the selection API used in render.ts), update the declarations to return the correct Selection/Transition/Scale types instead of any, and add a TypeScript-only smoke compile test that imports render.ts to catch API drift during CI.cypress/cypress/e2e/O-live-quiz-workflow.cy.ts (1)
4710-4715: Avoid duplicating the English empty-state copy.This assertion will drift as soon as the i18n string changes. Reuse the imported
messagesentry instead of asserting on a raw literal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/cypress/e2e/O-live-quiz-workflow.cy.ts` around lines 4710 - 4715, The test currently hardcodes the English empty-state string in noResponsesReceivedMessage; instead pull the expected text from the imported messages object and use that in the assertion. Replace the literal assignment to noResponsesReceivedMessage with the appropriate messages entry (the empty-state key exported in messages for the word-cloud/student responses empty state) and keep the assertion cy.get('[data-cy="word-cloud"]').should('contain', noResponsesReceivedMessage') so the test uses messages rather than a raw string.packages/shared-components/src/charts/ElementWordcloud.tsx (1)
17-18: Use repo aliases for these new imports.These additions reintroduce relative imports in a TSX file. Please switch them to the project's
@/~aliases.As per coding guidelines, "
**/*.{ts,tsx,js,jsx}: Use@and~path aliases for imports instead of relative paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-components/src/charts/ElementWordcloud.tsx` around lines 17 - 18, Replace the relative imports in ElementWordcloud.tsx with the project's path aliases: change the import of FontSizeButtons and NativeD3WordCloud to use the configured `@` or `~` aliases (e.g., import FontSizeButtons from '@/...' or '~/...') so both FontSizeButtons and NativeD3WordCloud use repo alias paths instead of '../FontSizeButtons' and './NativeD3WordCloud'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/i18n/messages/de.ts`:
- Around line 2600-2601: The translation key wordCloudOmittedWords is
incorrectly referring to omitted "Antwort/Antworten"; update its string to
reference omitted words instead (use "Wort" for one and "Wörter" for other)
while preserving the ICU plural format and the {count} placeholder so it reads
e.g. "{count} {count, plural, one {Wort} other {Wörter}} konnten aufgrund von
Platzmangel nicht angezeigt werden."; modify the value of wordCloudOmittedWords
accordingly.
In `@packages/shared-components/src/charts/ElementWordcloud.tsx`:
- Around line 71-75: The sanitizer currently strips numeric punctuation causing
numbers like "12.5" to become "125"; in the normalization logic that builds
words from response.value (the
.trim().toLowerCase().replace(/[^a-z0-9äöüß\s'-]/g, '')... chain) change the
flow to detect and preserve numeric tokens: either split first on whitespace
into tokens and for each token skip the regex cleanup if it matches a numeric
pattern (allowing digits, decimal separators like "." and "," and
percent/units), or adjust the regex to retain decimal separators and common
numeric symbols; apply the same change to the equivalent normalization block
later (the similar code around the second word-processing section) so numeric
values are not mangled in the wordcloud.
- Around line 71-81: The current token counting loop increments frequencies by 1
per token, ignoring response.count; update the increment in the loop that
processes response.value (the block using response.value, response.count,
frequencies and removeStopwords) so that each token increases
frequencies[filtered] by the response's weight (use response.count when present,
defaulting to 1) — e.g., compute a count = Number(response.count) || 1 and add
that count instead of 1 to frequencies[filtered].
- Around line 190-193: The tooltip HTML in the getWordTooltipHtml useCallback
interpolates unescaped user input (word.text), creating a stored XSS risk;
update getWordTooltipHtml to HTML-escape word.text before interpolation (e.g.,
import and use an escapeHtml utility such as from "html-escaper") so the
returned string uses the escaped text, keeping the surrounding template and
t(...) call intact; ensure the import of the escaper is added at the top of the
file and that only word.text is escaped, not the rest of the static markup.
In `@packages/word-cloud/demo/main.js`:
- Around line 3-17: Add a guard that validates required DOM elements before
using them: create a small helper (e.g., requiredElement or getRequiredEl) and
replace direct uses of document.getElementById for cloudElement, statsElement,
emptyStateElement, renderButton, shuffleButton, clearButton and the inputs
properties (seed, scale, width, height, minFontSize, maxFontSize, rotations)
with calls to that helper; the helper should throw a clear error message and
stop execution if an element is missing so the demo fails fast instead of
crashing later — apply the same guard pattern where those elements are
referenced again around lines noted in the review (uses around
render/shuffle/clear and inputs initialization).
In `@packages/word-cloud/demo/server.mjs`:
- Around line 26-33: readResolvedFile currently returns file contents but when
given a directory it resolves to join(filePath, 'index.html') and the caller
computes extname(resolvedPath) using the original path, which can be empty;
change readResolvedFile (or its callers) so the MIME type is inferred from the
actual served file path: ensure when isDirectory() you set the resolved path to
join(filePath, 'index.html') and propagate that final resolvedPath back (e.g.,
return both buffer and resolvedPath or update the caller to use the joined path)
so extname(resolvedPath) and Content-Type are computed from the real file name;
update the other usage at the extname(resolvedPath) site(s) (lines ~46-48) to
use the final resolved path.
- Around line 21-24: The resolvePath function currently can be bypassed by
absolute segments; fix it by resolving both root and the sanitized requestPath
using path.resolve (use resolve(root, sanitizedPath) or resolve then join) and
then verify the final resolved path is contained within the resolved root
(compare with startsWith(resolvedRoot + path.sep) or use path.relative to ensure
it does not start with '..'); if containment fails, return null or throw an
error instead of returning a path outside the root. Apply the same containment
check to the similar logic used around lines 40-44 so all file-serving paths are
validated against their declared root.
In `@packages/word-cloud/src/layout.ts`:
- Around line 87-112: The numeric option checks treat 0 as falsy and fall back
to defaults; update each numeric initializer (padding, rotations, shrinkFactor,
maxRelayouts, maxAttemptsPerWord, etc.) to only test
Number.isFinite(options.<name>) and then apply clamping/Math.floor/Math.max as
needed (e.g., padding = Number.isFinite(options.padding) ? Math.max(0,
options.padding) : DEFAULT_PADDING; rotations =
Number.isFinite(options.rotations) ? Math.max(1, Math.floor(options.rotations))
: DEFAULT_ROTATIONS; maxAttemptsPerWord =
Number.isFinite(options.maxAttemptsPerWord) ? Math.max(1,
Math.floor(options.maxAttemptsPerWord)) : DEFAULT_MAX_ATTEMPTS_PER_WORD;
similarly clamp shrinkFactor with clampNumber when finite) so that explicit 0
values are honored instead of falling back to defaults.
In `@packages/word-cloud/src/render.ts`:
- Around line 50-58: renderWordCloud currently captures render options (colors,
fontFamily, transitionDuration, tooltipOffset, click/hover callbacks) once and
returns a RendererHandle whose update(layout) only accepts a new layout, so prop
changes in NativeD3WordCloud don't take effect; modify the RendererHandle.update
to accept an optional second parameter newRenderOptions: RenderWordCloudOptions,
merge those into the existing renderer state (update colors, fontFamily,
transitionDuration, tooltip/click callbacks and tooltipOffset), and apply them
before performing the layout update (ensure any D3 selections/transition configs
are updated to use the new values). Also update every other update callsite (see
the other occurrence around the later block mentioned) so callers can pass new
options, and keep backward compatibility by making the second parameter optional
and defaulting to previously captured options.
- Around line 32-33: The tooltip HTML is built by getDefaultTooltip using
unescaped word.text and later injected with innerHTML (also in the code path
around where getWordTooltip is used), allowing XSS; change getDefaultTooltip to
return safe content (either plain text or already-escaped HTML) and update the
tooltip rendering code to avoid innerHTML — instead build the tooltip using DOM
APIs (createElement, textContent, appendChild) or run a strict HTML-escape on
word.text before interpolation; ensure the same fix is applied to the code path
that uses getWordTooltip (the block that sets the tooltip element content) so no
unescaped user text is ever assigned via innerHTML.
In `@project/WORDCLOUD_NATIVE.md`:
- Around line 20-41: The document contains workstation-specific absolute paths
(e.g. entries pointing to /Users/rolandschlaefli/...) that leak local info; edit
WORDCLOUD_NATIVE.md to replace those absolute paths with repo-relative paths
(for example use packages/word-cloud/src/index.ts,
packages/word-cloud/src/layout.ts, packages/word-cloud/demo/index.html, etc.)
and scan the rest of the file for any other /Users/rolandschlaefli/...
occurrences (including the noted additional entries) and convert them to
repository-relative paths so the doc is portable.
---
Duplicate comments:
In `@packages/shared-components/src/charts/ElementWordcloud.tsx`:
- Around line 139-147: The early return that checks supportedElementTypes and
instance.type causes hooks order to change; move the supported-type check and
its UserNotification render to after the useMemo/useCallback hooks in
ElementWordcloud so all hooks always run in the same order, e.g., keep the
supportedElementTypes = [ElementType.Numerical, ElementType.FreeText] and
instance.type check but defer returning the warning until after the memoized and
callback hooks have been declared (so useMemo/useCallback invocations in this
component always execute regardless of the element type).
---
Nitpick comments:
In `@cypress/cypress/e2e/O-live-quiz-workflow.cy.ts`:
- Around line 4710-4715: The test currently hardcodes the English empty-state
string in noResponsesReceivedMessage; instead pull the expected text from the
imported messages object and use that in the assertion. Replace the literal
assignment to noResponsesReceivedMessage with the appropriate messages entry
(the empty-state key exported in messages for the word-cloud/student responses
empty state) and keep the assertion
cy.get('[data-cy="word-cloud"]').should('contain', noResponsesReceivedMessage')
so the test uses messages rather than a raw string.
In `@packages/shared-components/src/charts/ElementWordcloud.tsx`:
- Around line 17-18: Replace the relative imports in ElementWordcloud.tsx with
the project's path aliases: change the import of FontSizeButtons and
NativeD3WordCloud to use the configured `@` or `~` aliases (e.g., import
FontSizeButtons from '@/...' or '~/...') so both FontSizeButtons and
NativeD3WordCloud use repo alias paths instead of '../FontSizeButtons' and
'./NativeD3WordCloud'.
In `@packages/word-cloud/src/shims.d.ts`:
- Around line 1-17: The current D3 shims use any and broad stubs (declare module
'd3-transition', the Scale interface and select(): any) making the renderer
untyped; replace these with proper typings by installing/using the official
`@types/d3-`* packages (e.g., `@types/d3-selection`, `@types/d3-scale`,
`@types/d3-transition`) or narrow the facade to exactly the fluent methods you use
(e.g., the Scale methods domain/range and the selection API used in render.ts),
update the declarations to return the correct Selection/Transition/Scale types
instead of any, and add a TypeScript-only smoke compile test that imports
render.ts to catch API drift during CI.
🪄 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: f9836f0e-c0de-4ef1-b60d-5ea5efdc0c4f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
cypress/cypress/e2e/O-live-quiz-workflow.cy.tspackages/i18n/messages/de.tspackages/i18n/messages/en.tspackages/shared-components/package.jsonpackages/shared-components/src/charts/ElementWordcloud.tsxpackages/shared-components/src/charts/NativeD3WordCloud.tsxpackages/word-cloud/.gitignorepackages/word-cloud/demo/index.htmlpackages/word-cloud/demo/main.jspackages/word-cloud/demo/server.mjspackages/word-cloud/package.jsonpackages/word-cloud/rollup.config.jspackages/word-cloud/src/index.tspackages/word-cloud/src/layout.tspackages/word-cloud/src/random.tspackages/word-cloud/src/render.tspackages/word-cloud/src/shims.d.tspackages/word-cloud/src/types.tspackages/word-cloud/test/layout.test.tspackages/word-cloud/tsconfig.jsonpackages/word-cloud/vitest.config.tsproject/WORDCLOUD_NATIVE.mdturbo.json
✅ Files skipped from review due to trivial changes (7)
- packages/word-cloud/.gitignore
- turbo.json
- packages/word-cloud/rollup.config.js
- packages/word-cloud/demo/index.html
- packages/word-cloud/tsconfig.json
- packages/word-cloud/src/index.ts
- packages/word-cloud/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared-components/package.json
| wordCloudOmittedWords: | ||
| '{count} {count, plural, one {Antwort} other {Antworten}} konnten aufgrund von Platzmangel nicht angezeigt werden.', |
There was a problem hiding this comment.
wordCloudOmittedWords currently says omitted “Antworten”.
In word mode the counter refers to omitted words, not full responses, so this string will misreport what was dropped from the layout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/i18n/messages/de.ts` around lines 2600 - 2601, The translation key
wordCloudOmittedWords is incorrectly referring to omitted "Antwort/Antworten";
update its string to reference omitted words instead (use "Wort" for one and
"Wörter" for other) while preserving the ICU plural format and the {count}
placeholder so it reads e.g. "{count} {count, plural, one {Wort} other {Wörter}}
konnten aufgrund von Platzmangel nicht angezeigt werden."; modify the value of
wordCloudOmittedWords accordingly.
| const words = response.value | ||
| .trim() | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9äöüß\s'-]/g, '') | ||
| .split(/\s+/) |
There was a problem hiding this comment.
Don't run numerical answers through the free-text sanitizer.
Because numerical data is funneled through the same normalization path, the regex on Line 74 removes decimal separators and other numeric notation. For example, 12.5 becomes 125, so the numerical word cloud can display the wrong value entirely.
Also applies to: 149-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-components/src/charts/ElementWordcloud.tsx` around lines 71 -
75, The sanitizer currently strips numeric punctuation causing numbers like
"12.5" to become "125"; in the normalization logic that builds words from
response.value (the .trim().toLowerCase().replace(/[^a-z0-9äöüß\s'-]/g, '')...
chain) change the flow to detect and preserve numeric tokens: either split first
on whitespace into tokens and for each token skip the regex cleanup if it
matches a numeric pattern (allowing digits, decimal separators like "." and ","
and percent/units), or adjust the regex to retain decimal separators and common
numeric symbols; apply the same change to the equivalent normalization block
later (the similar code around the second word-processing section) so numeric
values are not mangled in the wordcloud.
| const words = response.value | ||
| .trim() | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9äöüß\s'-]/g, '') | ||
| .split(/\s+/) | ||
| for (const word of words) { | ||
| if (word.length === 0) continue | ||
| const filtered = | ||
| stopwords.length > 0 ? removeStopwords([word], stopwords)[0] : word | ||
| if (!filtered) continue | ||
| frequencies[filtered] = (frequencies[filtered] || 0) + 1 |
There was a problem hiding this comment.
Preserve response.count when splitting into words.
Lines 76-81 currently add + 1 per token, so five identical responses contribute the same weight as one response. That undercounts repeated answers in the word cloud.
Suggested fix
- frequencies[filtered] = (frequencies[filtered] || 0) + 1
+ frequencies[filtered] =
+ (frequencies[filtered] || 0) + response.count🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-components/src/charts/ElementWordcloud.tsx` around lines 71 -
81, The current token counting loop increments frequencies by 1 per token,
ignoring response.count; update the increment in the loop that processes
response.value (the block using response.value, response.count, frequencies and
removeStopwords) so that each token increases frequencies[filtered] by the
response's weight (use response.count when present, defaulting to 1) — e.g.,
compute a count = Number(response.count) || 1 and add that count instead of 1 to
frequencies[filtered].
| const getWordTooltipHtml = useCallback( | ||
| (word: Pick<LayoutWord, 'text' | 'value'>) => | ||
| `<div style="text-align:center; background-color: ${BACKGROUND_COLOR_TOOLTIP}; padding: 5px 15px; opacity: 0.85; border-radius: 10px; font-size: 22px;"><strong>${word.text}</strong><br/>${t('manage.evaluation.numberOfVotes', { number: word.value })}</div>`, | ||
| [t] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd 'NativeD3WordCloud.tsx' packages/shared-components -x rg -n -C2 'getWordTooltip|dangerouslySetInnerHTML|innerHTML|\.html\(' {}Repository: uzh-bf/klicker-uzh
Length of output: 670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find NativeD3WordCloud implementation and see how it uses getWordTooltip
find packages/shared-components -name '*NativeD3*' -o -name '*wordcloud*' | head -20Repository: uzh-bf/klicker-uzh
Length of output: 121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for where tooltip is actually rendered - look for .html() calls in d3 or tooltip usage
rg -i 'tooltip|\.html\(' packages/shared-components --type ts --type tsx -A 3 -B 1 | head -100Repository: uzh-bf/klicker-uzh
Length of output: 89
🏁 Script executed:
#!/bin/bash
#!/bin/bash
set -euo pipefail
# Look at the full ElementWordcloud file to understand data source and rendering
wc -l packages/shared-components/src/charts/ElementWordcloud.tsxRepository: uzh-bf/klicker-uzh
Length of output: 124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for how getWordTooltip is used in the d3 rendering
rg 'getWordTooltip' packages/shared-components/src/charts/NativeD3WordCloud.tsx -A 5 -B 2Repository: uzh-bf/klicker-uzh
Length of output: 863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for .html() or innerHTML usage in NativeD3WordCloud
rg '\.html|innerHTML' packages/shared-components/src/charts/NativeD3WordCloud.tsx -B 3 -A 3Repository: uzh-bf/klicker-uzh
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pupefail
# Read the ElementWordcloud file around the problematic function
sed -n '180,210p' packages/shared-components/src/charts/ElementWordcloud.tsxRepository: uzh-bf/klicker-uzh
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the ElementWordcloud file around the problematic function
sed -n '180,210p' packages/shared-components/src/charts/ElementWordcloud.tsxRepository: uzh-bf/klicker-uzh
Length of output: 1268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for .html in NativeD3WordCloud - it's a d3 method
rg '\\.html|innerHTML' packages/shared-components/src/charts/NativeD3WordCloud.tsx -B 3 -A 3Repository: uzh-bf/klicker-uzh
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for where the renderer is used - check the RendererHandle and how it uses the tooltip
rg -l 'RendererHandle|class.*Renderer' packages/shared-components --type=tsRepository: uzh-bf/klicker-uzh
Length of output: 121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the NativeD3WordCloud.tsx file to understand how tooltip is rendered
cat -n packages/shared-components/src/charts/NativeD3WordCloud.tsx | head -100Repository: uzh-bf/klicker-uzh
Length of output: 3169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at more of NativeD3WordCloud to find tooltip rendering
sed -n '1,200p' packages/shared-components/src/charts/NativeD3WordCloud.tsx | tail -100Repository: uzh-bf/klicker-uzh
Length of output: 2313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the word-cloud package location
find . -name 'word-cloud' -type d | grep -v node_modulesRepository: uzh-bf/klicker-uzh
Length of output: 83
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the word-cloud package in packages directory
fd -t d 'word-cloud' packages/Repository: uzh-bf/klicker-uzh
Length of output: 82
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for renderWordCloud implementation
rg 'export.*renderWordCloud|function renderWordCloud' packages/ --type tsRepository: uzh-bf/klicker-uzh
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the render.ts file to see how getWordTooltip is used
cat -n packages/word-cloud/src/render.tsRepository: uzh-bf/klicker-uzh
Length of output: 6735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Count lines in render.ts first to determine how to read it
wc -l packages/word-cloud/src/render.tsRepository: uzh-bf/klicker-uzh
Length of output: 99
Tooltip HTML is vulnerable to stored XSS via unescaped user input.
The word.text value is interpolated directly into the HTML template at line 191 without HTML escaping. In packages/word-cloud/src/render.ts line 137, this HTML string is rendered via tooltipElement.innerHTML = tooltipHtml, which means malicious input from users can execute arbitrary JavaScript.
For example, a participant could enter: <img src=x onerror=alert('xss')> as their answer, and this would execute when the tooltip is displayed.
Apply HTML escaping (e.g., using a utility like escapeHtml from html-escaper) to word.text before interpolating it into the template string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-components/src/charts/ElementWordcloud.tsx` around lines 190
- 193, The tooltip HTML in the getWordTooltipHtml useCallback interpolates
unescaped user input (word.text), creating a stored XSS risk; update
getWordTooltipHtml to HTML-escape word.text before interpolation (e.g., import
and use an escapeHtml utility such as from "html-escaper") so the returned
string uses the escaped text, keeping the surrounding template and t(...) call
intact; ensure the import of the escaper is added at the top of the file and
that only word.text is escaped, not the rest of the static markup.
| const cloudElement = document.getElementById('cloud') | ||
| const statsElement = document.getElementById('stats') | ||
| const emptyStateElement = document.getElementById('empty-state') | ||
| const renderButton = document.getElementById('render') | ||
| const shuffleButton = document.getElementById('shuffle') | ||
| const clearButton = document.getElementById('clear') | ||
| const inputs = { | ||
| seed: document.getElementById('seed'), | ||
| scale: document.getElementById('scale'), | ||
| width: document.getElementById('width'), | ||
| height: document.getElementById('height'), | ||
| minFontSize: document.getElementById('minFontSize'), | ||
| maxFontSize: document.getElementById('maxFontSize'), | ||
| rotations: document.getElementById('rotations'), | ||
| } |
There was a problem hiding this comment.
Guard required DOM elements before use to prevent demo startup crashes.
The script assumes all queried elements exist; if any ID changes in index.html, it crashes immediately. Add a small required-element helper and fail fast with a clear error.
Suggested fix
+function requireElement(id) {
+ const element = document.getElementById(id)
+ if (!element) {
+ throw new Error(`Missing required element: #${id}`)
+ }
+ return element
+}
+
-const cloudElement = document.getElementById('cloud')
-const statsElement = document.getElementById('stats')
-const emptyStateElement = document.getElementById('empty-state')
-const renderButton = document.getElementById('render')
-const shuffleButton = document.getElementById('shuffle')
-const clearButton = document.getElementById('clear')
+const cloudElement = requireElement('cloud')
+const statsElement = requireElement('stats')
+const emptyStateElement = requireElement('empty-state')
+const renderButton = requireElement('render')
+const shuffleButton = requireElement('shuffle')
+const clearButton = requireElement('clear')Also applies to: 87-89, 119-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/word-cloud/demo/main.js` around lines 3 - 17, Add a guard that
validates required DOM elements before using them: create a small helper (e.g.,
requiredElement or getRequiredEl) and replace direct uses of
document.getElementById for cloudElement, statsElement, emptyStateElement,
renderButton, shuffleButton, clearButton and the inputs properties (seed, scale,
width, height, minFontSize, maxFontSize, rotations) with calls to that helper;
the helper should throw a clear error message and stop execution if an element
is missing so the demo fails fast instead of crashing later — apply the same
guard pattern where those elements are referenced again around lines noted in
the review (uses around render/shuffle/clear and inputs initialization).
| async function readResolvedFile(filePath) { | ||
| const fileStats = await stat(filePath) | ||
| if (fileStats.isDirectory()) { | ||
| return readResolvedFile(join(filePath, 'index.html')) | ||
| } | ||
|
|
||
| return readFile(filePath) | ||
| } |
There was a problem hiding this comment.
Infer Content-Type from the actual served file path.
When a directory is requested, readResolvedFile serves index.html, but extname(resolvedPath) may be empty, causing wrong MIME type.
Suggested fix
async function readResolvedFile(filePath) {
const fileStats = await stat(filePath)
if (fileStats.isDirectory()) {
return readResolvedFile(join(filePath, 'index.html'))
}
- return readFile(filePath)
+ return {
+ filePath,
+ content: await readFile(filePath),
+ }
}
@@
- const file = await readResolvedFile(resolvedPath)
- const extension = extname(resolvedPath)
+ const { filePath, content } = await readResolvedFile(resolvedPath)
+ const extension = extname(filePath)
@@
- response.end(file)
+ response.end(content)Also applies to: 46-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/word-cloud/demo/server.mjs` around lines 26 - 33, readResolvedFile
currently returns file contents but when given a directory it resolves to
join(filePath, 'index.html') and the caller computes extname(resolvedPath) using
the original path, which can be empty; change readResolvedFile (or its callers)
so the MIME type is inferred from the actual served file path: ensure when
isDirectory() you set the resolved path to join(filePath, 'index.html') and
propagate that final resolvedPath back (e.g., return both buffer and
resolvedPath or update the caller to use the joined path) so
extname(resolvedPath) and Content-Type are computed from the real file name;
update the other usage at the extname(resolvedPath) site(s) (lines ~46-48) to
use the final resolved path.
| const padding = | ||
| Number.isFinite(options.padding) && options.padding | ||
| ? Math.max(0, options.padding) | ||
| : DEFAULT_PADDING | ||
| const rotationAngles = options.rotationAngles ?? DEFAULT_ROTATION_ANGLES | ||
| const rotations = | ||
| Number.isFinite(options.rotations) && options.rotations | ||
| ? Math.max(1, Math.floor(options.rotations)) | ||
| : DEFAULT_ROTATIONS | ||
| const deterministic = options.deterministic ?? true | ||
| const seed = options.seed ?? DEFAULT_SEED | ||
| const fontFamily = options.fontFamily ?? DEFAULT_FONT_FAMILY | ||
| const fontStyle = options.fontStyle ?? DEFAULT_FONT_STYLE | ||
| const fontWeight = options.fontWeight ?? DEFAULT_FONT_WEIGHT | ||
| const shrinkFactor = | ||
| Number.isFinite(options.shrinkFactor) && options.shrinkFactor | ||
| ? clampNumber(options.shrinkFactor, 0.5, 0.99) | ||
| : DEFAULT_SHRINK_FACTOR | ||
| const maxRelayouts = | ||
| Number.isFinite(options.maxRelayouts) && options.maxRelayouts | ||
| ? Math.max(0, Math.floor(options.maxRelayouts)) | ||
| : DEFAULT_MAX_RELAYOUTS | ||
| const maxAttemptsPerWord = | ||
| Number.isFinite(options.maxAttemptsPerWord) && options.maxAttemptsPerWord | ||
| ? Math.max(1, Math.floor(options.maxAttemptsPerWord)) | ||
| : DEFAULT_MAX_ATTEMPTS_PER_WORD |
There was a problem hiding this comment.
Falsy numeric checks make supported 0 values impossible.
These branches treat 0 as "unset", so padding: 0 and maxRelayouts: 0 silently fall back to defaults. maxAttemptsPerWord: 0 also skips the clamp and becomes 1500 instead of 1. Check for finiteness only, then clamp.
💡 Suggested fix
- const padding =
- Number.isFinite(options.padding) && options.padding
- ? Math.max(0, options.padding)
- : DEFAULT_PADDING
+ const padding =
+ Number.isFinite(options.padding)
+ ? Math.max(0, options.padding)
+ : DEFAULT_PADDING
@@
- const maxRelayouts =
- Number.isFinite(options.maxRelayouts) && options.maxRelayouts
- ? Math.max(0, Math.floor(options.maxRelayouts))
- : DEFAULT_MAX_RELAYOUTS
+ const maxRelayouts =
+ Number.isFinite(options.maxRelayouts)
+ ? Math.max(0, Math.floor(options.maxRelayouts))
+ : DEFAULT_MAX_RELAYOUTS
@@
- const maxAttemptsPerWord =
- Number.isFinite(options.maxAttemptsPerWord) && options.maxAttemptsPerWord
- ? Math.max(1, Math.floor(options.maxAttemptsPerWord))
- : DEFAULT_MAX_ATTEMPTS_PER_WORD
+ const maxAttemptsPerWord =
+ Number.isFinite(options.maxAttemptsPerWord)
+ ? Math.max(1, Math.floor(options.maxAttemptsPerWord))
+ : DEFAULT_MAX_ATTEMPTS_PER_WORD📝 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.
| const padding = | |
| Number.isFinite(options.padding) && options.padding | |
| ? Math.max(0, options.padding) | |
| : DEFAULT_PADDING | |
| const rotationAngles = options.rotationAngles ?? DEFAULT_ROTATION_ANGLES | |
| const rotations = | |
| Number.isFinite(options.rotations) && options.rotations | |
| ? Math.max(1, Math.floor(options.rotations)) | |
| : DEFAULT_ROTATIONS | |
| const deterministic = options.deterministic ?? true | |
| const seed = options.seed ?? DEFAULT_SEED | |
| const fontFamily = options.fontFamily ?? DEFAULT_FONT_FAMILY | |
| const fontStyle = options.fontStyle ?? DEFAULT_FONT_STYLE | |
| const fontWeight = options.fontWeight ?? DEFAULT_FONT_WEIGHT | |
| const shrinkFactor = | |
| Number.isFinite(options.shrinkFactor) && options.shrinkFactor | |
| ? clampNumber(options.shrinkFactor, 0.5, 0.99) | |
| : DEFAULT_SHRINK_FACTOR | |
| const maxRelayouts = | |
| Number.isFinite(options.maxRelayouts) && options.maxRelayouts | |
| ? Math.max(0, Math.floor(options.maxRelayouts)) | |
| : DEFAULT_MAX_RELAYOUTS | |
| const maxAttemptsPerWord = | |
| Number.isFinite(options.maxAttemptsPerWord) && options.maxAttemptsPerWord | |
| ? Math.max(1, Math.floor(options.maxAttemptsPerWord)) | |
| : DEFAULT_MAX_ATTEMPTS_PER_WORD | |
| const padding = | |
| Number.isFinite(options.padding) | |
| ? Math.max(0, options.padding) | |
| : DEFAULT_PADDING | |
| const rotationAngles = options.rotationAngles ?? DEFAULT_ROTATION_ANGLES | |
| const rotations = | |
| Number.isFinite(options.rotations) && options.rotations | |
| ? Math.max(1, Math.floor(options.rotations)) | |
| : DEFAULT_ROTATIONS | |
| const deterministic = options.deterministic ?? true | |
| const seed = options.seed ?? DEFAULT_SEED | |
| const fontFamily = options.fontFamily ?? DEFAULT_FONT_FAMILY | |
| const fontStyle = options.fontStyle ?? DEFAULT_FONT_STYLE | |
| const fontWeight = options.fontWeight ?? DEFAULT_FONT_WEIGHT | |
| const shrinkFactor = | |
| Number.isFinite(options.shrinkFactor) && options.shrinkFactor | |
| ? clampNumber(options.shrinkFactor, 0.5, 0.99) | |
| : DEFAULT_SHRINK_FACTOR | |
| const maxRelayouts = | |
| Number.isFinite(options.maxRelayouts) | |
| ? Math.max(0, Math.floor(options.maxRelayouts)) | |
| : DEFAULT_MAX_RELAYOUTS | |
| const maxAttemptsPerWord = | |
| Number.isFinite(options.maxAttemptsPerWord) | |
| ? Math.max(1, Math.floor(options.maxAttemptsPerWord)) | |
| : DEFAULT_MAX_ATTEMPTS_PER_WORD |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/word-cloud/src/layout.ts` around lines 87 - 112, The numeric option
checks treat 0 as falsy and fall back to defaults; update each numeric
initializer (padding, rotations, shrinkFactor, maxRelayouts, maxAttemptsPerWord,
etc.) to only test Number.isFinite(options.<name>) and then apply
clamping/Math.floor/Math.max as needed (e.g., padding =
Number.isFinite(options.padding) ? Math.max(0, options.padding) :
DEFAULT_PADDING; rotations = Number.isFinite(options.rotations) ? Math.max(1,
Math.floor(options.rotations)) : DEFAULT_ROTATIONS; maxAttemptsPerWord =
Number.isFinite(options.maxAttemptsPerWord) ? Math.max(1,
Math.floor(options.maxAttemptsPerWord)) : DEFAULT_MAX_ATTEMPTS_PER_WORD;
similarly clamp shrinkFactor with clampNumber when finite) so that explicit 0
values are honored instead of falling back to defaults.
| function getDefaultTooltip(word: LayoutWord) { | ||
| return `<strong>${word.text}</strong><br/>${word.value}` |
There was a problem hiding this comment.
Escape tooltip content before writing it to innerHTML.
word.text comes from user-visible word-cloud content, and the default tooltip interpolates it straight into HTML before assigning innerHTML. A crafted answer can therefore execute script in the evaluation view. Build the tooltip with DOM nodes / textContent, or sanitize any HTML returned by getWordTooltip before insertion.
Also applies to: 135-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/word-cloud/src/render.ts` around lines 32 - 33, The tooltip HTML is
built by getDefaultTooltip using unescaped word.text and later injected with
innerHTML (also in the code path around where getWordTooltip is used), allowing
XSS; change getDefaultTooltip to return safe content (either plain text or
already-escaped HTML) and update the tooltip rendering code to avoid innerHTML —
instead build the tooltip using DOM APIs (createElement, textContent,
appendChild) or run a strict HTML-escape on word.text before interpolation;
ensure the same fix is applied to the code path that uses getWordTooltip (the
block that sets the tooltip element content) so no unescaped user text is ever
assigned via innerHTML.
| export function renderWordCloud( | ||
| container: HTMLElement, | ||
| initialLayoutResult: LayoutResult, | ||
| renderOptions: RenderWordCloudOptions = {} | ||
| ): RendererHandle { | ||
| const colors = renderOptions.colors ?? DEFAULT_COLORS | ||
| const transitionDuration = | ||
| renderOptions.transitionDuration ?? DEFAULT_TRANSITION_DURATION | ||
| const tooltipOffset = renderOptions.tooltipOffset ?? DEFAULT_TOOLTIP_OFFSET |
There was a problem hiding this comment.
RendererHandle.update() cannot apply changed render options.
renderWordCloud() snapshots colors, fontFamily, transitionDuration, and tooltip/click callbacks once, but update() only accepts a new layout. That means theme/locale/tooltip changes in NativeD3WordCloud keep using the old renderer configuration until the component is fully recreated. Extend update() to accept new render options, or recreate the renderer whenever those props change.
Also applies to: 163-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/word-cloud/src/render.ts` around lines 50 - 58, renderWordCloud
currently captures render options (colors, fontFamily, transitionDuration,
tooltipOffset, click/hover callbacks) once and returns a RendererHandle whose
update(layout) only accepts a new layout, so prop changes in NativeD3WordCloud
don't take effect; modify the RendererHandle.update to accept an optional second
parameter newRenderOptions: RenderWordCloudOptions, merge those into the
existing renderer state (update colors, fontFamily, transitionDuration,
tooltip/click callbacks and tooltipOffset), and apply them before performing the
layout update (ensure any D3 selections/transition configs are updated to use
the new values). Also update every other update callsite (see the other
occurrence around the later block mentioned) so callers can pass new options,
and keep backward compatibility by making the second parameter optional and
defaulting to previously captured options.
| - [x] Introduced standalone package `@klicker-uzh/word-cloud` with layout/render API | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/word-cloud/src/index.ts` | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/word-cloud/src/layout.ts` | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/word-cloud/src/render.ts` | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/word-cloud/src/random.ts` | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/word-cloud/src/types.ts` | ||
| - [x] Added standalone demo + tiny static server for isolated smoke testing | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/word-cloud/demo/index.html` | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/word-cloud/demo/main.js` | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/word-cloud/demo/server.mjs` | ||
| - [x] Added package-level tests for deterministic behavior, collisions, scales, and relayout | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/word-cloud/test/layout.test.ts` | ||
| - [x] Integrated native renderer into `Standard` mode in `ElementWordcloud` | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/shared-components/src/charts/ElementWordcloud.tsx` | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/shared-components/src/charts/NativeD3WordCloud.tsx` | ||
| - [x] Kept `Premium` mode temporarily on legacy renderer as migration fallback | ||
| - [x] Wired workspace package usage and build dependency chain | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/packages/shared-components/package.json` | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/turbo.json` | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/pnpm-lock.yaml` | ||
| - [x] Added temporary Cypress assertion to confirm Premium fallback still renders | ||
| - `/Users/rolandschlaefli/.codex/worktrees/eaff/klicker-uzh/cypress/cypress/e2e/O-live-quiz-workflow.cy.ts` |
There was a problem hiding this comment.
Remove workstation-specific absolute paths from this document.
The /Users/rolandschlaefli/... entries leak a local username and are useless to anyone else cloning the repo. Please switch them to repo-relative paths such as packages/word-cloud/src/layout.ts so the doc stays portable and doesn't commit local filesystem details.
Also applies to: 73-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@project/WORDCLOUD_NATIVE.md` around lines 20 - 41, The document contains
workstation-specific absolute paths (e.g. entries pointing to
/Users/rolandschlaefli/...) that leak local info; edit WORDCLOUD_NATIVE.md to
replace those absolute paths with repo-relative paths (for example use
packages/word-cloud/src/index.ts, packages/word-cloud/src/layout.ts,
packages/word-cloud/demo/index.html, etc.) and scan the rest of the file for any
other /Users/rolandschlaefli/... occurrences (including the noted additional
entries) and convert them to repository-relative paths so the doc is portable.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 26381906 | Triggered | Generic Password | 4210bd9 | .factory/skills/agent-browser/templates/authenticated-session.sh | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
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.
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.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| titleEditedDuplicated: 'Auto Save question title new (Copy)', | ||
| } | ||
|
|
||
| const UPDATE = { |
| // i18n labels | ||
| const LABEL_SC = 'Single Choice (SC)' | ||
| const LABEL_DRAFT = 'Draft' | ||
| const LABEL_READY = 'Ready' |
| title: 'CT Question Title (Version 1)', | ||
| content: 'CT Question Content (Version 1)', | ||
| } | ||
| const COLLECTION = { |
| content: 'SE Question Content 2', | ||
| inputs: 3, | ||
| } | ||
| const CSML = { |
| title: 'CS Title Test 1 (Version 1)', | ||
| content: 'CS Question Test 1', | ||
| } | ||
| const SEML2 = { |
| content: 'SE Question Content 2 (Version 2)', | ||
| inputs: 3, | ||
| } | ||
| const CSML2 = { |
| title: 'CS Title Test 1 (Version 2)', | ||
| content: 'CS Question Test 1 (Version 2)', | ||
| } | ||
| const COLLECTION2 = { |
| /** | ||
| * Grant permission on a microlearning via the share dialog. | ||
| */ | ||
| async function grantMicroLearningPermission( |
| /** | ||
| * Verify microlearning is visible in activities list and check a few key items. | ||
| */ | ||
| async function verifyMicroLearningVisible(page: Page, name: string) { |
| const COURSE = 'Testkurs' | ||
| const SEEDED_COURSE = 'Testkurs' | ||
|
|
||
| const SC = { |
2 participants
Summary by CodeRabbit
New Features
Tests