enhance(packages/shared-components): replace tagcloud with wordcloud and filter user input - #4947
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.
klicker-uzh
 |
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
wordcloud
|
| Run status |
|
| Run duration | 10m 13s |
| Commit |
|
| Committer | Patrick Louis Aldover |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
1
|
 |
0
|
 |
0
|
 |
761
|
| View all changes introduced in this branch ↗︎ | |
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
…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 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 | |
|---|---|---|---|---|---|
| 14050276 | Triggered | Generic Password | 62b9e5b | .github/workflows/playwright-testing.yml | View secret |
| 1509424 | Triggered | Generic Password | 62b9e5b | .github/workflows/playwright-testing.yml | View secret |
| 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 secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- 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.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Confidence Score: 5/5The change is safe to merge; all rendering paths use DOM Node injection rather than innerHTML, the new layout engine is well-tested, and the interactive controls are properly memoised. No defects were found that produce wrong data or break existing behaviour. The tooltip XSS issue flagged in a previous round has been resolved by the DOM Node approach in render.ts. The remaining notes are about test-selector naming, a dead hook export, and a potential main-thread stall for very large Sentences-mode datasets — none of these affect correctness for typical usage. packages/shared-components/src/FontSizeButtons.tsx (duplicate data-cy attributes), packages/shared-components/src/hooks/useTextPresenceObserverWordCloud.ts (unused export), and packages/shared-components/src/charts/NativeD3WordCloud.tsx (synchronous layout with unbounded input in Sentences mode).
|
| Filename | Overview |
|---|---|
| packages/word-cloud/src/layout.ts | New deterministic D3 word-cloud layout engine with Archimedean spiral placement, AABB collision detection, and iterative font-size shrinking. Well-structured with good input sanitization, but the synchronous O(N²) layout loop can block the main thread for large uncapped data sets (e.g. Sentences mode). |
| packages/word-cloud/src/render.ts | D3-based SVG renderer with tooltip via DOM Node injection (replaceChildren), safe against XSS since content is built with textContent/createTextNode in the caller. Clean enter/update/exit join pattern with transitions. |
| packages/shared-components/src/charts/ElementWordcloud.tsx | Major rewrite replacing react-tagcloud with the new native renderer; adds language/split-mode/font-size/display-limit controls. Locale-based language initialisation is now wired up. EMPTY_STOPWORDS constant avoids the previous unstable reference. |
| packages/shared-components/src/charts/NativeD3WordCloud.tsx | React wrapper that drives layout and rendering via hooks. ResizeObserver integration and renderer lifecycle management look correct. The layout is computed synchronously in useMemo which can block the main thread for large Sentences-mode word clouds. |
| packages/shared-components/src/FontSizeButtons.tsx | New reusable font-size +/- control, but both decrease and increase buttons have hardcoded data-cy attributes with 'max' in their names. When instantiated twice (for min and max font sizes), both instances produce identical data-cy values, making E2E selectors ambiguous. |
| packages/shared-components/src/hooks/useTextPresenceObserverWordCloud.ts | New MutationObserver hook exported but never imported anywhere in the codebase — appears to be dead code. Also uses containerRef.current in the useEffect dependency array (React anti-pattern flagged in previous review rounds). |
| packages/word-cloud/src/random.ts | Clean Mulberry32 PRNG with FNV-1a seed hashing for deterministic layout. Correct implementation. |
| packages/i18n/messages/en.ts | Adds translation keys for all new word-cloud UI controls; plural-aware omitted-count messages are well-formed. Previously flagged dead Premium/Standard tooltip keys are not present in the current HEAD. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant EC as ElementChart.tsx
participant EW as ElementWordCloud.tsx
participant ND as NativeD3WordCloud.tsx
participant LY as layout.ts (word-cloud pkg)
participant RD as render.ts (word-cloud pkg)
EC->>EW: render(instance, locale)
EW->>EW: getInitialLanguage(locale) → language state
EW->>EW: useMemo: getWordFrequencies(data, stopwords, splitMode)
EW->>EW: "useMemo: sort & slice → processedData"
EW->>ND: "render(words=processedData, minTextSize, maxTextSize)"
ND->>ND: ResizeObserver → containerSize state
ND->>LY: useMemo: computeWordCloudLayout(words, options) [sync, main thread]
LY-->>ND: "LayoutResult {placed[], omitted[]}"
ND->>EW: onLayoutChange(result) → setOmittedCount
ND->>RD: useEffect: renderWordCloud(container, layoutResult)
RD->>RD: D3 select/join/transition → SVG text elements
RD->>RD: mouseover → replaceTooltipContent(tooltipElement, Node)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant EC as ElementChart.tsx
participant EW as ElementWordCloud.tsx
participant ND as NativeD3WordCloud.tsx
participant LY as layout.ts (word-cloud pkg)
participant RD as render.ts (word-cloud pkg)
EC->>EW: render(instance, locale)
EW->>EW: getInitialLanguage(locale) → language state
EW->>EW: useMemo: getWordFrequencies(data, stopwords, splitMode)
EW->>EW: "useMemo: sort & slice → processedData"
EW->>ND: "render(words=processedData, minTextSize, maxTextSize)"
ND->>ND: ResizeObserver → containerSize state
ND->>LY: useMemo: computeWordCloudLayout(words, options) [sync, main thread]
LY-->>ND: "LayoutResult {placed[], omitted[]}"
ND->>EW: onLayoutChange(result) → setOmittedCount
ND->>RD: useEffect: renderWordCloud(container, layoutResult)
RD->>RD: D3 select/join/transition → SVG text elements
RD->>RD: mouseover → replaceTooltipContent(tooltipElement, Node)
Reviews (13): Last reviewed commit: "enhance: include word-cloud package in v..." | Re-trigger Greptile
55e2d6f to
077d70a
Compare
|
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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.prettierignore (1)
56-59: ⚡ Quick winRe-check the “unsupported” ignores for TOML and
.gitignore.These patterns disable formatter coverage for file types that Prettier may already handle in your configured version. If this is intentional, add a short rationale comment; otherwise consider removing these two entries to keep
format:checkeffective.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.prettierignore around lines 56 - 59, Review whether the **/*.toml and **/.gitignore entries in the .prettierignore file are truly necessary for your configured Prettier version, as Prettier may already have support for these file types. If Prettier does support these formats, remove the two entries (the **/*.toml line and the **/*.gitignore line) to allow format:check to cover these files. If these entries must remain for a specific reason, add a clarifying comment above them explaining why they need to be ignored despite potential Prettier support.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.lintstagedrc.mjs:
- Line 14: The Prettier command construction in the linting configuration
concatenates file paths without proper quoting using nonSymlinks.join(' '),
which causes the command parser to misparsefully handle paths containing spaces
or special characters. Instead of returning a single command string with joined
paths, restructure the return value to be an array of separate commands, where
each file in nonSymlinks gets its own individual Prettier command. This allows
lint-staged to properly handle each file path regardless of special characters
or spaces, eliminating the need for manual quoting.
---
Nitpick comments:
In @.prettierignore:
- Around line 56-59: Review whether the **/*.toml and **/.gitignore entries in
the .prettierignore file are truly necessary for your configured Prettier
version, as Prettier may already have support for these file types. If Prettier
does support these formats, remove the two entries (the **/*.toml line and the
**/*.gitignore line) to allow format:check to cover these files. If these
entries must remain for a specific reason, add a clarifying comment above them
explaining why they need to be ignored despite potential Prettier support.
🪄 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: e890fdb3-1783-43fa-8743-535500737f55
📒 Files selected for processing (5)
.lintstagedrc.mjs.prettierignoreapps/frontend-manage/src/components/evaluation/ElementChart.tsxcypress/cypress.config.tscypress/cypress/e2e/O-live-quiz-workflow.cy.ts
💤 Files with no reviewable changes (3)
- apps/frontend-manage/src/components/evaluation/ElementChart.tsx
- cypress/cypress/e2e/O-live-quiz-workflow.cy.ts
- cypress/cypress.config.ts
| }) | ||
| if (nonSymlinks.length === 0) return [] | ||
| return [ | ||
| `prettier --config .prettierrc.mjs --ignore-unknown --check ${nonSymlinks.join(' ')}`, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the .lintstagedrc.mjs file
find . -name ".lintstagedrc.mjs" -type f | head -20Repository: uzh-bf/klicker-uzh
Length of output: 82
🏁 Script executed:
# Get more context around line 14 in .lintstagedrc.mjs
if [ -f .lintstagedrc.mjs ]; then
echo "=== File: .lintstagedrc.mjs ===" && cat -n .lintstagedrc.mjs
fiRepository: uzh-bf/klicker-uzh
Length of output: 613
🌐 Web query:
lint-staged command string parsing space-separated paths
💡 Result:
In lint-staged, commands are executed by spawning a child process rather than running within a shell [1]. By default, lint-staged automatically appends the list of absolute paths of staged files to the end of your configured command [1][2]. When you provide a simple command string, lint-staged uses the string-argv library to parse it into an array of arguments, separating the command from its arguments based on spaces [3][4]. Because lint-staged controls the process execution via the execa library, filenames containing spaces are generally handled correctly as individual arguments without needing manual quoting, provided your command is correctly parsed [3][5]. If you encounter issues with path parsing or require custom formatting (such as needing file paths to appear in a specific position within a command, or avoiding the default space-separated appending), you should use the function syntax in your configuration file [2][6]. Using the function syntax: When you define a task as a function, you receive the list of staged absolute paths as an array [2]. You can then manually construct the exact command string you need, providing full control over how paths are joined and formatted [2][6]. Example of using the function syntax: //.lintstagedrc.js export default { '*.ts': (absolutePaths) => { // Manually join paths with spaces, or use them as needed return my-command --flag ${absolutePaths.join(' ')}; }, } If you require advanced shell features (like pipes, redirection, or chaining commands with &&/||), the recommended approach is to move those commands into a standalone shell script file and invoke that script from lint-staged [7]. Lint-staged will pass the staged files as arguments to your script, which can then process them using "$@" [7].
Citations:
- 1: Document how file paths are passed to tasks lint-staged/lint-staged#1024
- 2: https://github.com/lint-staged/lint-staged?tab=readme-ov-file
- 3: lint-staged/lint-staged@4cb4dde
- 4: lint-staged stringifies my commands, changing their meaning (and breaking them) lint-staged/lint-staged#539
- 5: fix: running eslint on filenames containing spaces lint-staged/lint-staged#1484
- 6: Make Space between command and files optional lint-staged/lint-staged#1097
- 7: https://github.com/okonet/lint-staged/blob/v17.0.7/MIGRATION.md
🌐 Web query:
lint-staged how to properly quote paths in command strings
💡 Result:
In lint-staged, handling file paths that contain spaces or special characters depends on how you define your tasks. Because lint-staged automatically appends the list of staged files to your command string by default, you may encounter issues when paths are not quoted, as the underlying execution engine (execa) treats spaces as argument delimiters [1][2]. To ensure paths are correctly handled, use the following approaches: 1. Use Function Configuration for Granular Control The most robust way to handle quoting is to use a JavaScript configuration file (.lintstagedrc.js or lint-staged.config.js). This allows you to map over the array of staged filenames and wrap each one in quotes manually [3][4][5]. //.lintstagedrc.js export default { '/.js': (filenames) => filenames.map((filename) => prettier --write '${filename}'), } This ensures that each path is passed as a single, quoted argument to your command [3]. 2. Considerations for Special Characters If your file paths contain characters that may conflict with shell interpretation (like [ or ]), you may need to escape them in addition to quoting. Using a helper library such as shell-quote can help maintain cross-platform compatibility [6]. //.lintstagedrc.js const escape = require('shell-quote').quote; export default { '/.js': (filenames) => filenames.map((filename) => prettier --check "${escape([filename])}"), } 3. Avoid Manual Concatenation with Default Behavior By default, lint-staged automatically appends the list of matched staged files to the end of your command string [3][1]. If you are not using a function and rely on this default behavior, you cannot easily insert quotes around individual paths. Consequently, if your file paths contain spaces, you should migrate to the function syntax (as shown above) to take control of how the filenames are formatted and passed to the task [4]. Note that lint-staged no longer supports the --shell flag [7]. Commands are executed directly as child processes using execa, meaning manual shell-style tricks or relying on shell-specific expansion for quoting will not work [1][7]. Always prefer the function syntax for complex argument requirements.
Citations:
- 1: Document how file paths are passed to tasks lint-staged/lint-staged#1024
- 2: lint-staged fails with eslint when path contains spaces lint-staged/lint-staged#773
- 3: https://github.com/lint-staged/lint-staged?tab=readme-ov-file
- 4: https://www.npmjs.com/package/lint-staged
- 5: https://github.com/okonet/lint-staged/blob/v17.0.4/README.md
- 6: Wrapping arguments (file names) with single quotes lint-staged/lint-staged#635
- 7: https://github.com/okonet/lint-staged/blob/v17.0.7/MIGRATION.md
Quote paths before passing them to Prettier in a single command string.
nonSymlinks.join(' ') concatenates paths without quoting, causing paths with spaces or special characters to be misparsed by string-argv (the parser lint-staged uses), which splits by spaces. This leads to false check failures or skipped files in CI/CD.
The proposed JSON.stringify approach works correctly—it wraps each path in double quotes. However, a more idiomatic solution for lint-staged is to return an array of separate commands, one per file:
Recommended approach
- return [
- `prettier --config .prettierrc.mjs --ignore-unknown --check ${nonSymlinks.join(' ')}`,
- ]
+ return nonSymlinks.map(
+ (f) => `prettier --config .prettierrc.mjs --ignore-unknown --check '${f}'`,
+ )📝 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.
| `prettier --config .prettierrc.mjs --ignore-unknown --check ${nonSymlinks.join(' ')}`, | |
| return nonSymlinks.map( | |
| (f) => `prettier --config .prettierrc.mjs --ignore-unknown --check '${f}'`, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.lintstagedrc.mjs at line 14, The Prettier command construction in the
linting configuration concatenates file paths without proper quoting using
nonSymlinks.join(' '), which causes the command parser to misparsefully handle
paths containing spaces or special characters. Instead of returning a single
command string with joined paths, restructure the return value to be an array of
separate commands, where each file in nonSymlinks gets its own individual
Prettier command. This allows lint-staged to properly handle each file path
regardless of special characters or spaces, eliminating the need for manual
quoting.
# Conflicts: # pnpm-lock.yaml
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| words = words.map((word) => ({ | ||
| ...word, | ||
| value: Math.max(1, Math.floor(Math.random() * 60)), |
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3
|
| Run status |
|
| Run duration | 08m 47s |
| Commit |
|
| Committer | Patrick Louis Aldover |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
761
|
| View all changes introduced in this branch ↗︎ | |
4 participants
Summary by CodeRabbit
Screenshots
ClickUp Links
Primary task: Finalize word cloud rework with deterministic D3 renderer and filtering
Roadmap fit: v3.4 lecturer-facing evaluation improvement. This PR replaces the old tag cloud with the new native/D3 word-cloud renderer and filtering flow. It is currently open and not mergeable, so the task tracks review, verification, and remaining blockers.
Related task: