Conversation
📝 WalkthroughWalkthroughAdds a screen calibration protocol and docs, localStorage-backed calibration utilities and tests, integrates calibration values into Item context payload, migrates Helmet to react-helmet-async with HelmetProvider in Root, and updates a few dependencies and a Cypress import. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
package.json (1)
44-44: Git branch reference for@graasp/uimay cause reproducibility issues.Referencing a git branch (
#screen-calibration) instead of a tagged version or commit SHA means the resolved dependency can change without any modification topackage.json. This can lead to non-reproducible builds and unexpected breakages if the branch is updated.Consider pinning to a specific commit SHA (e.g.,
#<commit-sha>) or publishing a versioned release once the feature is stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 44, The dependency entry for "@graasp/ui" currently points to a git branch "#screen-calibration" which can change and break reproducibility; update the package.json dependency for "@graasp/ui" to reference a stable identifier (either a published semver version or a specific commit SHA like "#<commit-sha>") instead of the branch name, then run your lockfile update (e.g., npm/yarn/pnpm install) to regenerate the lockfile so builds use the pinned revision.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 78: Remove the hard-pinned platform-specific Rollup binary
"@rollup/rollup-win32-x64-msvc" from devDependencies in package.json (or move it
into optionalDependencies) so Rollup can let the package manager auto-resolve
the correct platform-specific binary at install time; update package.json to
either delete the "@rollup/rollup-win32-x64-msvc" entry under devDependencies or
relocate the same key into optionalDependencies to avoid forcing a Windows-only
artifact on non-Windows CI/dev environments.
In `@src/modules/item/Item.tsx`:
- Around line 212-213: The calibration getters are not scoped by rootId — update
Item.tsx to read rootId from route params (in addition to itemId) and call
getCalibrationScale(rootId) and getCalibrationFontSize(rootId) instead of the
no-arg versions; then modify src/utils/calibration.ts to change the getter
signatures to accept rootId, incorporate rootId into the localStorage/cache key
(use or add buildCalibrationKey(rootId) when constructing keys), and ensure
saveCalibrationScale(rootId, ...) actually uses the rootId when building its key
so all getters/setters are per-experiment isolated.
In `@src/utils/calibration.ts`:
- Around line 25-36: The saveCalibrationFontSize function is missing a rootId
parameter for per-root scoping; update its signature (export const
saveCalibrationFontSize = (fontSize: Record<string, unknown>, metadata?:
Pick<CalibrationData, 'memberId' | 'calibrationAppId'>, rootId?: string): void)
and ensure you pass rootId into buildCalibrationKey (call
buildCalibrationKey(rootId)) when storing to localStorage, mirroring how
saveCalibrationScale handles root scoping; also update any callers of
saveCalibrationFontSize to supply rootId where available and adjust types if
needed.
- Around line 63-66: The current check wrongly tests for a string; update the
condition to accept object-shaped fontSize values: replace "typeof
parsed.fontSize === 'string'" with a guard like "typeof parsed.fontSize ===
'object' && parsed.fontSize !== null && !Array.isArray(parsed.fontSize')" and
return parsed.fontSize (cast/narrow to Record<string, unknown> if needed). This
change touches the parsed.fontSize check in src/utils/calibration.ts and ensures
valid object fontSize values are returned instead of null.
- Line 9: buildCalibrationKey currently ignores rootId and returns a fixed key,
breaking per-root calibration isolation; change buildCalibrationKey to accept a
rootId parameter and return `lnco_screen_calibration_${rootId}`, then update
every place that calls buildCalibrationKey (and any calibration-related
functions that previously received rootId) to forward the rootId through so
stored/loaded calibration keys are namespaced per rootId as specified in the
protocol.
---
Nitpick comments:
In `@package.json`:
- Line 44: The dependency entry for "@graasp/ui" currently points to a git
branch "#screen-calibration" which can change and break reproducibility; update
the package.json dependency for "@graasp/ui" to reference a stable identifier
(either a published semver version or a specific commit SHA like
"#<commit-sha>") instead of the branch name, then run your lockfile update
(e.g., npm/yarn/pnpm install) to regenerate the lockfile so builds use the
pinned revision.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dce013b-acca-4276-bb15-ba4615488b6a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
docs/graasp-ui-screen-calibration-instructions.mddocs/screen-calibration-protocol.mdpackage.jsonsrc/Root.tsxsrc/modules/item/Item.tsxsrc/modules/pages/HomePage.tsxsrc/utils/calibration.ts
| "@eslint/compat": "1.2.3", | ||
| "@eslint/eslintrc": "3.2.0", | ||
| "@eslint/js": "9.15.0", | ||
| "@rollup/rollup-win32-x64-msvc": "4.59.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the content of package.json around the rollup dependencies
echo "=== Checking rollup entries in package.json ==="
rg -n "rollup" package.json -A 2 -B 2
echo -e "\n=== Looking for all rollup-win32 entries ==="
rg -n "rollup-win32" package.json
echo -e "\n=== Checking for other platform-specific rollup packages ==="
rg -n "@rollup/rollup-" package.json | head -20
echo -e "\n=== Checking markdown files for rollup documentation ==="
fd . --type f --extension md | head -20 | xargs rg -l "rollup" 2>/dev/null || echo "No markdown files with rollup mentioned"Repository: lnco-epfl/graasp-player
Length of output: 762
🏁 Script executed:
#!/bin/bash
# Check CHANGELOG for any mention of this rollup-win32 dependency
echo "=== Searching CHANGELOG.md for rollup-win32 ==="
rg -n "rollup-win32\|win32.*rollup\|platform.*rollup" CHANGELOG.md || echo "No direct mentions found"
echo -e "\n=== Checking for any build configuration files ==="
fd . --maxdepth 2 --type f \( -name "*.config.*" -o -name "rollup*" -o -name "vite*" \) | head -20
echo -e "\n=== Checking if there are comments near this dependency in package.json ==="
sed -n '75,85p' package.json
echo -e "\n=== Checking entire devDependencies section ==="
rg -n "devDependencies|@rollup" package.json | head -30Repository: lnco-epfl/graasp-player
Length of output: 1040
🏁 Script executed:
#!/bin/bash
# Check git history for this dependency (limited to recent commits to avoid heavy operations)
echo "=== Checking recent git history for rollup-win32 ==="
git log --oneline -20 -- package.json | head -10
echo -e "\n=== Checking for CI/workflow configuration ==="
fd . -type f -path "*/.github/workflows/*" 2>/dev/null | head -10
echo -e "\n=== Checking for .npmrc or related npm config ==="
fd . -type f \( -name ".npmrc" -o -name "npmrc" -o -name ".npm*" \) 2>/dev/null
echo -e "\n=== Checking for any npm scripts in package.json ==="
rg -A 10 '"scripts"' package.json
echo -e "\n=== Checking if vite.config or build files exist ==="
fd . -type f \( -name "vite.config*" -o -name "rollup.config*" -o -name "build*" \) 2>/dev/null | grep -v node_modulesRepository: lnco-epfl/graasp-player
Length of output: 748
🏁 Script executed:
#!/bin/bash
# Search for vite config with various extensions
echo "=== Searching for vite config (all extensions) ==="
fd -type f -name "vite.config*"
echo -e "\n=== Checking repository root files ==="
ls -la | grep -E "vite|rollup" || echo "No vite/rollup files in root"
echo -e "\n=== Check the actual commit that added this dependency ==="
git log -p --all -S "@rollup/rollup-win32-x64-msvc" -- package.json | head -50
echo -e "\n=== Check if this is in the current branch ==="
git log --oneline -p -S "@rollup/rollup-win32-x64-msvc" -- package.json | head -30Repository: lnco-epfl/graasp-player
Length of output: 1293
🏁 Script executed:
#!/bin/bash
echo "=== Reading vite.config.ts ==="
cat vite.config.tsRepository: lnco-epfl/graasp-player
Length of output: 1764
🌐 Web query:
What is the purpose of @rollup/rollup-win32-x64-msvc? Is it automatically resolved by Rollup or Vite?
💡 Result:
@rollup/rollup-win32-x64-msvc is Rollup’s precompiled native (Node-API) binary for Windows x64 built with MSVC. Rollup uses these platform-specific packages to load native code for performance-critical work (e.g., parsing/hashing), with a WASM fallback on unsupported platforms. [1]
It’s not something you normally import or pin manually. When Rollup runs, its native.js selects the right binary for the current OS/CPU and require()s @rollup/rollup-${platform-arch} (on Windows x64: @rollup/rollup-win32-x64-msvc). [2]
Who “resolves” it—Rollup or Vite?
- Rollup chooses/loads the correct
@rollup/rollup-…package at runtime. [2] - Your package manager (npm/pnpm/yarn) is what installs the correct platform package when
rollupis installed (directly or transitively, e.g. via Vite). [1] - Vite doesn’t resolve it itself; you typically only see it because Vite depends on Rollup (especially for
vite build). Missing-module errors usually come from install/optional-dependency issues rather than Vite configuration. [3]
Sources
- Rollup cross-platform packaging notes (platform-specific
@rollup/rollup-{platform}-{arch}packages; “npm automatically selects…”). [1] - Rollup
native.jsshowing it loads@rollup/rollup-${packageBase}based on detected platform/arch. [2] - Vite discussion showing the error surfaces in Vite projects and is linked to optional-dependency install issues. [3]
Remove explicit pinning of platform-specific Rollup binary or move to optionalDependencies.
@rollup/rollup-win32-x64-msvc is Rollup's precompiled native binary for Windows and should be auto-resolved by your package manager at install time—it's not meant to be manually pinned in devDependencies. Rollup dynamically selects the correct platform binary (@rollup/rollup-{platform}-{arch}) at runtime, and npm/yarn automatically installs the appropriate variant as an optional dependency.
Explicitly pinning only the Windows variant can cause issues on non-Windows CI environments or developer machines. Either remove it and let the package manager handle platform detection, or move it to optionalDependencies if Windows-specific support is required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 78, Remove the hard-pinned platform-specific Rollup
binary "@rollup/rollup-win32-x64-msvc" from devDependencies in package.json (or
move it into optionalDependencies) so Rollup can let the package manager
auto-resolve the correct platform-specific binary at install time; update
package.json to either delete the "@rollup/rollup-win32-x64-msvc" entry under
devDependencies or relocate the same key into optionalDependencies to avoid
forcing a Windows-only artifact on non-Windows CI/dev environments.
| const scale = getCalibrationScale() ?? null; | ||
| const fontSize = getCalibrationFontSize() ?? null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for rootId usage in the item module
rg -n "rootId" src/modules/item/
# Check for useParams usage in Item.tsx
rg -n "useParams" src/modules/item/Item.tsx -A 3
# Check current calibration.ts implementation
if [ -f "src/utils/calibration.ts" ]; then
echo "=== calibration.ts ==="
cat src/utils/calibration.ts
fiRepository: lnco-epfl/graasp-player
Length of output: 2447
🏁 Script executed:
#!/bin/bash
# Find the documentation file
if [ -f "docs/screen-calibration-protocol.md" ]; then
echo "=== Documentation file found ==="
# Get lines 88-109 if the file exists
sed -n '88,109p' docs/screen-calibration-protocol.md
else
# Search for any calibration protocol documentation
find . -name "*calibration*" -type f | head -20
fiRepository: lnco-epfl/graasp-player
Length of output: 561
🏁 Script executed:
#!/bin/bash
# Get the context around lines 212-233 in Item.tsx with useParams usage
sed -n '1,20p' src/modules/item/Item.tsx | head -20
echo "=== useParams extraction ==="
sed -n '270,280p' src/modules/item/Item.tsx
echo "=== Calibration usage context ==="
sed -n '205,240p' src/modules/item/Item.tsxRepository: lnco-epfl/graasp-player
Length of output: 2242
Calibration getters must receive rootId for per-experiment isolation.
The current implementation uses a constant localStorage key (lnco_screen_calibration), causing all experiments to share the same calibration data. Per the protocol documentation, calibration should be scoped by rootId.
Item.tsx (lines 212-213) needs to:
- Extract
rootIdfrom route params (currently only extractsitemId) - Pass it to the calibration getters:
getCalibrationScale(rootId)andgetCalibrationFontSize(rootId)
Additionally, src/utils/calibration.ts must be updated to:
- Accept
rootIdparameter in getter functions - Incorporate
rootIdinto the cache key (e.g., viabuildCalibrationKey(rootId))
Note: saveCalibrationScale() already includes a rootId parameter but doesn't use it, indicating incomplete implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/item/Item.tsx` around lines 212 - 213, The calibration getters
are not scoped by rootId — update Item.tsx to read rootId from route params (in
addition to itemId) and call getCalibrationScale(rootId) and
getCalibrationFontSize(rootId) instead of the no-arg versions; then modify
src/utils/calibration.ts to change the getter signatures to accept rootId,
incorporate rootId into the localStorage/cache key (use or add
buildCalibrationKey(rootId) when constructing keys), and ensure
saveCalibrationScale(rootId, ...) actually uses the rootId when building its key
so all getters/setters are per-experiment isolated.
There was a problem hiding this comment.
Actually then the documentation is wrong. I have created a single calibration app so that other apps don't have to redo calibration but can use that value
There was a problem hiding this comment.
@ymijsters, thanks for the clarification! That makes perfect sense — a single shared calibration key means any app can read the calibration value without needing to redo the calibration process itself. My concern was misplaced.
In that case, the implementation in src/utils/calibration.ts (fixed key lnco_screen_calibration) and the no-arg getters in Item.tsx are correct by design. The documentation in docs/screen-calibration-protocol.md / docs/graasp-ui-screen-calibration-instructions.md should just be updated to clarify this intent — removing any reference to per-rootId key scoping (e.g., lnco_screen_calibration_<rootId>) and replacing it with the global single-key approach.
I'll note this design decision for future reviews.
✏️ Learnings added
Learnt from: ymijsters
Repo: lnco-epfl/graasp-player PR: 9
File: src/modules/item/Item.tsx:212-213
Timestamp: 2026-03-06T10:25:04.744Z
Learning: In the graasp-player repository, the screen calibration localStorage key (`lnco_screen_calibration`) is intentionally global and not scoped per `rootId`. A single shared calibration app sets the calibration scale once, and all other apps read that same shared value. Per-rootId isolation is NOT the intended design.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| calibrationAppId?: string; | ||
| }; | ||
|
|
||
| const buildCalibrationKey = (): string => `lnco_screen_calibration`; |
There was a problem hiding this comment.
Critical: rootId parameter is ignored, breaking per-root calibration scoping.
The documentation in docs/screen-calibration-protocol.md (lines 16-28) explicitly specifies that calibration should be keyed by rootId using the pattern lnco_screen_calibration_<rootId> so "different experiments can have independent calibrations."
The current implementation uses a fixed key, meaning all calibrations share the same storage slot regardless of root context. This defeats the purpose of per-experiment isolation.
🐛 Proposed fix to incorporate rootId
-const buildCalibrationKey = (): string => `lnco_screen_calibration`;
+const buildCalibrationKey = (rootId: string): string =>
+ `lnco_screen_calibration_${rootId}`;Then update all function signatures and calls to pass rootId through:
export const saveCalibrationScale = (
rootId: string,
scale: number,
metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>,
): void => {
const data: CalibrationData = {
scale,
timestamp: Date.now(),
...metadata,
};
- localStorage.setItem(buildCalibrationKey(), JSON.stringify(data));
+ localStorage.setItem(buildCalibrationKey(rootId), JSON.stringify(data));
};
-export const getCalibrationScale = (): number | null => {
- const stored = localStorage.getItem(buildCalibrationKey());
+export const getCalibrationScale = (rootId: string): number | null => {
+ const stored = localStorage.getItem(buildCalibrationKey(rootId));
// ...
};
-export const clearCalibrationScale = (): void => {
- localStorage.removeItem(buildCalibrationKey());
+export const clearCalibrationScale = (rootId: string): void => {
+ localStorage.removeItem(buildCalibrationKey(rootId));
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/calibration.ts` at line 9, buildCalibrationKey currently ignores
rootId and returns a fixed key, breaking per-root calibration isolation; change
buildCalibrationKey to accept a rootId parameter and return
`lnco_screen_calibration_${rootId}`, then update every place that calls
buildCalibrationKey (and any calibration-related functions that previously
received rootId) to forward the rootId through so stored/loaded calibration keys
are namespaced per rootId as specified in the protocol.
| export const saveCalibrationFontSize = ( | ||
| fontSize: Record<string, unknown>, | ||
| metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>, | ||
| ): void => { | ||
| const data: CalibrationData = { | ||
| fontSize, | ||
| timestamp: Date.now(), | ||
| ...metadata, | ||
| }; | ||
|
|
||
| localStorage.setItem(buildCalibrationKey(), JSON.stringify(data)); | ||
| }; |
There was a problem hiding this comment.
saveCalibrationFontSize is missing rootId parameter.
Unlike saveCalibrationScale, this function doesn't accept a rootId parameter at all. For consistency and to support per-root scoping (once the key is fixed), this function should also accept rootId.
🔧 Proposed fix
export const saveCalibrationFontSize = (
+ rootId: string,
fontSize: Record<string, unknown>,
metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>,
): void => {
const data: CalibrationData = {
fontSize,
timestamp: Date.now(),
...metadata,
};
- localStorage.setItem(buildCalibrationKey(), JSON.stringify(data));
+ localStorage.setItem(buildCalibrationKey(rootId), JSON.stringify(data));
};📝 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.
| export const saveCalibrationFontSize = ( | |
| fontSize: Record<string, unknown>, | |
| metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>, | |
| ): void => { | |
| const data: CalibrationData = { | |
| fontSize, | |
| timestamp: Date.now(), | |
| ...metadata, | |
| }; | |
| localStorage.setItem(buildCalibrationKey(), JSON.stringify(data)); | |
| }; | |
| export const saveCalibrationFontSize = ( | |
| rootId: string, | |
| fontSize: Record<string, unknown>, | |
| metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>, | |
| ): void => { | |
| const data: CalibrationData = { | |
| fontSize, | |
| timestamp: Date.now(), | |
| ...metadata, | |
| }; | |
| localStorage.setItem(buildCalibrationKey(rootId), JSON.stringify(data)); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/calibration.ts` around lines 25 - 36, The saveCalibrationFontSize
function is missing a rootId parameter for per-root scoping; update its
signature (export const saveCalibrationFontSize = (fontSize: Record<string,
unknown>, metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>,
rootId?: string): void) and ensure you pass rootId into buildCalibrationKey
(call buildCalibrationKey(rootId)) when storing to localStorage, mirroring how
saveCalibrationScale handles root scoping; also update any callers of
saveCalibrationFontSize to supply rootId where available and adjust types if
needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utils/calibration.ts (1)
87-89: Function name is misleading.
clearCalibrationScaleremoves the entire calibration entry includingfontSize. Consider renaming toclearCalibrationorclearCalibrationDatafor clarity.♻️ Proposed rename
-export const clearCalibrationScale = (): void => { +export const clearCalibration = (): void => { localStorage.removeItem(buildCalibrationKey()); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/calibration.ts` around lines 87 - 89, The function name clearCalibrationScale is misleading because it removes the entire calibration entry (including fontSize); rename the function to clearCalibration or clearCalibrationData (pick one consistently) and update its export to the new name, then update all references/usages and any imports/tests to use the new identifier; ensure buildCalibrationKey() call and localStorage.removeItem remain unchanged and that the old name is removed or replaced with a deprecation wrapper if backward compatibility is needed.src/utils/calibration.test.ts (1)
39-50: Consolidate duplicatebeforeEachhooks.The static analysis correctly identified two separate
beforeEachhooks that can be merged into one.♻️ Proposed fix
describe('calibration utils', () => { beforeEach(() => { Object.defineProperty(globalThis, 'localStorage', { value: createLocalStorageMock(), configurable: true, writable: true, }); - }); - - beforeEach(() => { localStorage.clear(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/calibration.test.ts` around lines 39 - 50, There are two duplicate beforeEach hooks in the test suite; merge them into a single beforeEach that both defines globalThis.localStorage using createLocalStorageMock() (configurable: true, writable: true) and then calls localStorage.clear(); update the test file's describe('calibration utils', ...) to remove the second beforeEach and consolidate the setup into one block referencing globalThis, createLocalStorageMock, and localStorage.clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/calibration.ts`:
- Around line 24-48: saveCalibrationScale and saveCalibrationFontSize overwrite
the entire stored CalibrationData and thus drop the other field; change both
functions (saveCalibrationScale and saveCalibrationFontSize) to read the current
JSON from localStorage using buildCalibrationKey(), parse it (safely handling
missing/invalid JSON), merge the new field (scale or fontSize) plus timestamp
and metadata into the existing object, then write the merged object back to
localStorage; ensure you preserve any existing memberId/calibrationAppId and any
existing scale/fontSize when merging so getCalibrationScale() and
getCalibrationFontSize() continue to work independently.
---
Nitpick comments:
In `@src/utils/calibration.test.ts`:
- Around line 39-50: There are two duplicate beforeEach hooks in the test suite;
merge them into a single beforeEach that both defines globalThis.localStorage
using createLocalStorageMock() (configurable: true, writable: true) and then
calls localStorage.clear(); update the test file's describe('calibration utils',
...) to remove the second beforeEach and consolidate the setup into one block
referencing globalThis, createLocalStorageMock, and localStorage.clear.
In `@src/utils/calibration.ts`:
- Around line 87-89: The function name clearCalibrationScale is misleading
because it removes the entire calibration entry (including fontSize); rename the
function to clearCalibration or clearCalibrationData (pick one consistently) and
update its export to the new name, then update all references/usages and any
imports/tests to use the new identifier; ensure buildCalibrationKey() call and
localStorage.removeItem remain unchanged and that the old name is removed or
replaced with a deprecation wrapper if backward compatibility is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 459f0288-0b87-4ae1-8b9f-3fd8127e89d0
📒 Files selected for processing (4)
cypress.config.tssrc/modules/item/Item.tsxsrc/utils/calibration.test.tssrc/utils/calibration.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/item/Item.tsx
| export const saveCalibrationScale = ( | ||
| scale: number, | ||
| metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>, | ||
| ): void => { | ||
| const data: CalibrationData = { | ||
| scale, | ||
| timestamp: Date.now(), | ||
| ...metadata, | ||
| }; | ||
|
|
||
| localStorage.setItem(buildCalibrationKey(), JSON.stringify(data)); | ||
| }; | ||
|
|
||
| export const saveCalibrationFontSize = ( | ||
| fontSize: CalibrationFontSize, | ||
| metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>, | ||
| ): void => { | ||
| const data: CalibrationData = { | ||
| fontSize, | ||
| timestamp: Date.now(), | ||
| ...metadata, | ||
| }; | ||
|
|
||
| localStorage.setItem(buildCalibrationKey(), JSON.stringify(data)); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the calibration utilities
rg "saveCalibrationScale|saveCalibrationFontSize|getCalibrationScale|getCalibrationFontSize" -A 3 -B 1 --type ts --type tsxRepository: lnco-epfl/graasp-player
Length of output: 94
🏁 Script executed:
# Also check the CalibrationData type definition and buildCalibrationKey function
rg "type CalibrationData|interface CalibrationData|buildCalibrationKey" -A 5 --type ts --type tsxRepository: lnco-epfl/graasp-player
Length of output: 94
🏁 Script executed:
# Check Item.tsx to understand how calibration is used (referenced in learning)
fd "Item.tsx" --type f -x cat -n {}Repository: lnco-epfl/graasp-player
Length of output: 16597
🏁 Script executed:
# Search for saveCalibrationScale and saveCalibrationFontSize usage
rg "saveCalibrationScale|saveCalibrationFontSize" --type ts -B 2 -A 2Repository: lnco-epfl/graasp-player
Length of output: 1870
🏁 Script executed:
# Also view the full calibration.ts file to see all functions
cat src/utils/calibration.tsRepository: lnco-epfl/graasp-player
Length of output: 2035
🏁 Script executed:
# View the calibration test file to see test coverage
cat src/utils/calibration.test.tsRepository: lnco-epfl/graasp-player
Length of output: 3651
Both saveCalibrationScale and saveCalibrationFontSize overwrite the entire storage entry, losing the other value.
The CalibrationData type intentionally supports both scale and fontSize as optional fields. Item.tsx reads both values independently via getCalibrationScale() and getCalibrationFontSize(), then passes both to the app context. However, the save functions create incomplete objects that erase any previously stored value. Calling saveCalibrationScale(1.5) followed by saveCalibrationFontSize('large') will lose the scale value.
To coexist, the save functions must merge with existing data rather than replace it:
🐛 Proposed fix to preserve existing data
+const getExistingCalibrationData = (): Partial<CalibrationData> => {
+ const stored = localStorage.getItem(buildCalibrationKey());
+ if (!stored) return {};
+ try {
+ return JSON.parse(stored) as CalibrationData;
+ } catch {
+ return {};
+ }
+};
+
export const saveCalibrationScale = (
scale: number,
metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>,
): void => {
+ const existing = getExistingCalibrationData();
const data: CalibrationData = {
+ ...existing,
scale,
timestamp: Date.now(),
...metadata,
};
localStorage.setItem(buildCalibrationKey(), JSON.stringify(data));
};
export const saveCalibrationFontSize = (
fontSize: CalibrationFontSize,
metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>,
): void => {
+ const existing = getExistingCalibrationData();
const data: CalibrationData = {
+ ...existing,
fontSize,
timestamp: Date.now(),
...metadata,
};
localStorage.setItem(buildCalibrationKey(), JSON.stringify(data));
};📝 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.
| export const saveCalibrationScale = ( | |
| scale: number, | |
| metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>, | |
| ): void => { | |
| const data: CalibrationData = { | |
| scale, | |
| timestamp: Date.now(), | |
| ...metadata, | |
| }; | |
| localStorage.setItem(buildCalibrationKey(), JSON.stringify(data)); | |
| }; | |
| export const saveCalibrationFontSize = ( | |
| fontSize: CalibrationFontSize, | |
| metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>, | |
| ): void => { | |
| const data: CalibrationData = { | |
| fontSize, | |
| timestamp: Date.now(), | |
| ...metadata, | |
| }; | |
| localStorage.setItem(buildCalibrationKey(), JSON.stringify(data)); | |
| }; | |
| const getExistingCalibrationData = (): Partial<CalibrationData> => { | |
| const stored = localStorage.getItem(buildCalibrationKey()); | |
| if (!stored) return {}; | |
| try { | |
| return JSON.parse(stored) as CalibrationData; | |
| } catch { | |
| return {}; | |
| } | |
| }; | |
| export const saveCalibrationScale = ( | |
| scale: number, | |
| metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>, | |
| ): void => { | |
| const existing = getExistingCalibrationData(); | |
| const data: CalibrationData = { | |
| ...existing, | |
| scale, | |
| timestamp: Date.now(), | |
| ...metadata, | |
| }; | |
| localStorage.setItem(buildCalibrationKey(), JSON.stringify(data)); | |
| }; | |
| export const saveCalibrationFontSize = ( | |
| fontSize: CalibrationFontSize, | |
| metadata?: Pick<CalibrationData, 'memberId' | 'calibrationAppId'>, | |
| ): void => { | |
| const existing = getExistingCalibrationData(); | |
| const data: CalibrationData = { | |
| ...existing, | |
| fontSize, | |
| timestamp: Date.now(), | |
| ...metadata, | |
| }; | |
| localStorage.setItem(buildCalibrationKey(), JSON.stringify(data)); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/calibration.ts` around lines 24 - 48, saveCalibrationScale and
saveCalibrationFontSize overwrite the entire stored CalibrationData and thus
drop the other field; change both functions (saveCalibrationScale and
saveCalibrationFontSize) to read the current JSON from localStorage using
buildCalibrationKey(), parse it (safely handling missing/invalid JSON), merge
the new field (scale or fontSize) plus timestamp and metadata into the existing
object, then write the merged object back to localStorage; ensure you preserve
any existing memberId/calibrationAppId and any existing scale/fontSize when
merging so getCalibrationScale() and getCalibrationFontSize() continue to work
independently.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
package.json (1)
78-78:⚠️ Potential issue | 🟠 MajorRemove the hard-pinned Windows Rollup binary from
devDependencies.Line 78 still pins a Windows-only Rollup native package and can break installs on non-Windows environments. This was already raised earlier and is still unresolved; prefer letting Rollup/package-manager resolve platform binaries automatically, or move it to
optionalDependencies.💡 Proposed change
- "@rollup/rollup-win32-x64-msvc": "4.59.0",#!/bin/bash # Verify platform-specific rollup packages are not hard-pinned in devDependencies rg -n '"@rollup/rollup-[^"]+"' package.json -C2 rg -n '"devDependencies"|"optionalDependencies"' package.json -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 78, Remove the hard‑pinned Windows Rollup native package from devDependencies: locate the entry "@rollup/rollup-win32-x64-msvc" in package.json and either delete it from devDependencies or move it into optionalDependencies so platform-specific binaries are resolved by the package manager (do not hard‑pin Windows-only packages in devDependencies).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/calibration.test.ts`:
- Around line 40-50: Merge the two duplicate beforeEach hooks into one by
combining the setup of globalThis.localStorage (using createLocalStorageMock())
and the clear call into a single beforeEach; locate the existing beforeEach
blocks that reference createLocalStorageMock, Object.defineProperty(globalThis,
'localStorage', ...) and localStorage.clear(), then move the clear() into the
same beforeEach that defines globalThis.localStorage so there is only one
beforeEach in the test scope.
---
Duplicate comments:
In `@package.json`:
- Line 78: Remove the hard‑pinned Windows Rollup native package from
devDependencies: locate the entry "@rollup/rollup-win32-x64-msvc" in
package.json and either delete it from devDependencies or move it into
optionalDependencies so platform-specific binaries are resolved by the package
manager (do not hard‑pin Windows-only packages in devDependencies).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ceaec68f-5704-441a-9856-cb5ae426feb3
📒 Files selected for processing (4)
package.jsonsrc/modules/item/Item.tsxsrc/utils/calibration.test.tssrc/utils/calibration.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/calibration.ts
| beforeEach(() => { | ||
| Object.defineProperty(globalThis, 'localStorage', { | ||
| value: createLocalStorageMock(), | ||
| configurable: true, | ||
| writable: true, | ||
| }); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| localStorage.clear(); | ||
| }); |
There was a problem hiding this comment.
Merge duplicate beforeEach hooks to satisfy lint.
Line 48-50 duplicates the hook type in the same scope and matches the Biome error (noDuplicateTestHooks). Merge setup + clear into a single beforeEach.
✅ Proposed fix
describe('calibration utils', () => {
beforeEach(() => {
Object.defineProperty(globalThis, 'localStorage', {
value: createLocalStorageMock(),
configurable: true,
writable: true,
});
- });
-
- beforeEach(() => {
localStorage.clear();
});📝 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.
| beforeEach(() => { | |
| Object.defineProperty(globalThis, 'localStorage', { | |
| value: createLocalStorageMock(), | |
| configurable: true, | |
| writable: true, | |
| }); | |
| }); | |
| beforeEach(() => { | |
| localStorage.clear(); | |
| }); | |
| beforeEach(() => { | |
| Object.defineProperty(globalThis, 'localStorage', { | |
| value: createLocalStorageMock(), | |
| configurable: true, | |
| writable: true, | |
| }); | |
| localStorage.clear(); | |
| }); |
🧰 Tools
🪛 Biome (2.4.4)
[error] 48-50: Duplicate beforeEach hook found.
(lint/suspicious/noDuplicateTestHooks)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/calibration.test.ts` around lines 40 - 50, Merge the two duplicate
beforeEach hooks into one by combining the setup of globalThis.localStorage
(using createLocalStorageMock()) and the clear call into a single beforeEach;
locate the existing beforeEach blocks that reference createLocalStorageMock,
Object.defineProperty(globalThis, 'localStorage', ...) and localStorage.clear(),
then move the clear() into the same beforeEach that defines
globalThis.localStorage so there is only one beforeEach in the test scope.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores