FIX: flicking device status #16873
FIX: flicking device status #16873
Conversation
WalkthroughThe pull request introduces several changes across various components in the project. A new file mock module is added in Possibly related PRs
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
||
| // NOTE: for some reason, the content is still in the DOM but the state is definitely closed | ||
| const parent = triggerBefore.parentElement; | ||
| expect(parent).toHaveAttribute('data-state', 'closed'); |
There was a problem hiding this comment.
this is completely werid though, the tooltip content is still in the dom, it is possibly hidden with some CSS, but rather, I used this state. This test isn't perfect at all, but I guess it is better than nothing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx (1)
32-34: Consider memoizing scroll dimensions.The scroll dimensions could be memoized to prevent unnecessary recalculations.
+ const getScrollDimensions = useCallback(() => ({ + scrollWidth: containerRef.current?.scrollWidth ?? null, + scrollHeight: containerRef.current?.scrollHeight ?? null, + }), []); const resizeObserver = new ResizeObserver(entries => { - const scrollWidth = containerRef.current?.scrollWidth ?? null; - const scrollHeight = containerRef.current?.scrollHeight ?? null; + const { scrollWidth, scrollHeight } = getScrollDimensions();packages/components/src/components/Tooltip/Tooltip.test.tsx (2)
22-37: Consider removing unnecessary act.The
actwrapper on line 29 appears unnecessary asuserEventmethods are already wrapped inact.- await act(() => {});
73-75: Add test for content removal.The comment suggests the content remains in DOM. Consider adding a test to verify content is actually removed after animation.
expect(parent).toHaveAttribute('data-state', 'closed'); + // Wait for animation + await new Promise(resolve => setTimeout(resolve, 300)); + const tooltip = screen.queryByText(tooltipContent); + expect(tooltip).not.toBeInTheDocument();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
packages/components/__mocks__/fileMock.js(1 hunks)packages/components/jest.config.js(1 hunks)packages/components/package.json(1 hunks)packages/components/src/components/Tooltip/Tooltip.test.tsx(1 hunks)packages/components/src/components/Tooltip/Tooltip.tsx(4 hunks)packages/components/src/components/Tooltip/TooltipFloatingUi.tsx(3 hunks)packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.stories.tsx(1 hunks)packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/DeviceSelector/DeviceSelector.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/mocks/fileMock.js
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (15)
packages/suite/src/components/suite/layouts/SuiteLayout/DeviceSelector/DeviceSelector.tsx (2)
80-80: LGTM! Explicit boolean conversion improves code clarity.The explicit
Boolean()conversion makes the type conversion more obvious and improves code readability.
137-141: LGTM! Tooltip visibility control fixes the flickering issue.The addition of
isActiveprop tied todiscoveryInProgresseffectively controls the tooltip visibility, preventing it from flickering during device status updates. This aligns with the PR objective to fix the flickering device status issue.packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.stories.tsx (1)
12-12: LGTM! Good addition for demonstrating truncation.Adding a fixed max-width ensures consistent truncation behavior in the story, which helps demonstrate the tooltip functionality effectively.
packages/components/jest.config.js (1)
7-10: LGTM! Verify mock file exists.The SVG file mock configuration is correctly implemented.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Verify that the SVG mock file exists test -f packages/components/__mocks__/fileMock.js || echo "Mock file missing"Length of output: 51
🏁 Script executed:
#!/bin/bash # Verify that the SVG mock file exists and show a preview of its content if [ -f packages/components/__mocks__/fileMock.js ]; then echo "fileMock.js exists. First 10 lines:" head -n 10 packages/components/__mocks__/fileMock.js else echo "ERROR: fileMock.js is missing." fiLength of output: 225
LGTM! The SVG mock file exists and its content is as expected.
- Verified that
packages/components/__mocks__/fileMock.jsexists.- The file's content contains the standard stub (
module.exports = 'test-file-stub';), which is appropriate for mocking SVG imports in Jest.packages/components/src/components/Tooltip/Tooltip.tsx (4)
49-50: Well-documented prop addition.The
isActiveprop is clearly documented and aligns with the PR's objective to fix flickering issues.
71-73: Good default values and documentation.The default values and comments effectively explain the distinction between
isActiveanddisabledprops.
106-107: Proper prop forwarding.The
isActiveprop is correctly forwarded to theTooltipFloatingUicomponent.
117-117: Improved cursor handling.The cursor logic now correctly handles the disabled state.
packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx (2)
24-24: Better state naming.The state variable name
isEllipsisActivebetter reflects its purpose compared to the previousisTooltipVisible.
53-60: Efficient tooltip rendering.The tooltip is correctly configured with the
isActiveprop and conditional rendering.packages/components/src/components/Tooltip/Tooltip.test.tsx (1)
7-20: Good test coverage for isActive=true.The test effectively verifies tooltip behavior when active.
packages/components/src/components/Tooltip/TooltipFloatingUi.tsx (3)
49-49: LGTM! Well-documented interface addition.The
isActiveprop is well-named and documented, making its purpose clear.
66-66: LGTM! Sensible default value.The default value of
trueensures backward compatibility with existing usage.
78-79: LGTM! Clear and effective implementation.The implementation effectively prevents tooltip display when inactive while maintaining existing controlled/uncontrolled behavior. The comment clearly explains the logic.
packages/components/package.json (1)
62-62: LGTM! Appropriate testing dependency.Adding
@testing-library/user-eventis appropriate for testing user interactions with the tooltip component.
|
🚀 Expo preview is ready!
|
vojtatranta
force-pushed
the
16178-flicking-device-status
branch
2 times, most recently
from
2ad4cc0 to
107802d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/suite/src/views/dashboard/PromoBanner.tsx (1)
151-160: Add debounce to prevent tooltip flickering.The rapid toggling of tooltip state could contribute to flickering issues.
Consider adding debounce to the mouse event handlers:
+import { debounce } from 'lodash'; + +const debouncedSetTooltip = debounce((isOpen: boolean, qrType?: QrType) => { + setIsTooltipOpen(isOpen); + setShowQr(qrType); +}, 100); <span onMouseEnter={() => { - setIsTooltipOpen(true); - setShowQr(type); + debouncedSetTooltip(true, type); }} onMouseLeave={() => { - setIsTooltipOpen(false); - setShowQr(undefined); + debouncedSetTooltip(false, undefined); }} >
🧹 Nitpick comments (4)
packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx (2)
28-49: LGTM! Improved ResizeObserver implementation.The changes improve the code by:
- Moving scroll dimensions inside the callback for accurate resize calculations
- Using null coalescing for type safety
- Simplifying the dependency array
Consider adding type assertion to ensure
entriesis not empty:- const resizeObserver = new ResizeObserver(entries => { + const resizeObserver = new ResizeObserver((entries: ResizeObserverEntry[]) => { + if (entries.length === 0) return;
53-60: Optimize the rendering logic.While the
isActiveprop correctly addresses the flickering issue mentioned in the PR objectives, the conditional rendering ofEllipsisContaineris redundant since the parent already has the same styles.Simplify the rendering:
<Tooltip isActive={Boolean(children) && isEllipsisActive} delayShow={delayShow} content={children ?? null} cursor={cursor} > - {isEllipsisActive ? <EllipsisContainer>{children}</EllipsisContainer> : children} + {children} </Tooltip>packages/suite/src/views/settings/SettingsDevice/FirmwareVersion.tsx (1)
103-106: LGTM! Consider adding a title attribute for better accessibility.The cursor property addition provides clear visual feedback when the tooltip is not interactive.
Consider adding a title attribute when revision is not available to improve accessibility:
<VersionTooltip content={revision} cursor={!revision ? 'not-allowed' : undefined} + title={!revision ? 'Firmware revision not available' : undefined} >packages/suite/src/views/dashboard/PromoBanner.tsx (1)
137-140: LGTM! Consider improving mobile interaction handling.The cursor property addition provides clear visual feedback on mobile layouts.
Consider improving mobile interaction handling by:
- Using touch events alongside mouse events
- Adding a debounce to prevent rapid state changes that could cause flickering
<Tooltip isOpen={isTooltipOpen} cursor={isMobileLayout ? 'not-allowed' : undefined} + onTouchStart={() => { + if (!isMobileLayout) { + setIsTooltipOpen(true); + setShowQr(type); + } + }} + onTouchEnd={() => { + if (!isMobileLayout) { + setIsTooltipOpen(false); + setShowQr(undefined); + } + }} content={
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/components/src/components/Tooltip/Tooltip.test.tsx(1 hunks)packages/components/src/components/Tooltip/Tooltip.tsx(3 hunks)packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.stories.tsx(1 hunks)packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/DeviceSelector/DeviceSelector.tsx(2 hunks)packages/suite/src/views/dashboard/PromoBanner.tsx(1 hunks)packages/suite/src/views/settings/SettingsDevice/FirmwareVersion.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.stories.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/DeviceSelector/DeviceSelector.tsx
- packages/components/src/components/Tooltip/Tooltip.test.tsx
- packages/components/src/components/Tooltip/Tooltip.tsx
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: Other Checks
- GitHub Check: Releases revision Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: build-deploy
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: EAS Update
🔇 Additional comments (1)
packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx (1)
24-24: LGTM! Improved state variable naming.The rename from
isTooltipVisibletoisEllipsisActivebetter reflects the actual state being tracked.
| @@ -101,19 +101,15 @@ export const Tooltip = ({ | |||
| return ( | |||
| <Wrapper $isFullWidth={isFullWidth} className={className} as={elType}> | |||
| <TooltipFloatingUi | |||
| isActive={isActive} | |||
There was a problem hiding this comment.
Turns out, that I would really used this change in this other PR of mine: https://github.com/trezor/trezor-suite/pull/16815/files
vojtatranta
force-pushed
the
16178-flicking-device-status
branch
3 times, most recently
from
38f269b to
6f298a4
Compare
| export const pickAndPrepareFrameProps = ( | ||
| props: Record<string, any>, | ||
| allowedFrameProps: Array<FramePropsKeys>, | ||
| convertToTransient = true, |
There was a problem hiding this comment.
the TS should scream if this was used somewhere, but it doesn't. So seems okay.
| <Translation id="TR_UNAVAILABLE_WHILE_LOADING" /> | ||
| ) : undefined | ||
| } | ||
| cursor="not-allowed" |
There was a problem hiding this comment.
maybe for sake of asurance, I could do cursor={discoveryInProgress ? 'not-allowed' : undefined} , yeah, will do
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx (1)
23-44: Consider adding scroll dimensions to the dependency array.While the current changes improve robustness with proper null checks, the dependency array might miss updates if scroll dimensions change without the children changing.
- }, [children]); + }, [children, containerRef.current?.scrollWidth, containerRef.current?.scrollHeight]);packages/components/src/utils/transientProps.test.ts (1)
3-28: LGTM! Well-structured type tests.The test suite effectively validates the type-safety aspects of
makePropsTransient. The tests cover both successful transformations and expected type errors.Consider adding these test cases to improve coverage:
- Test with nested objects
- Test with array values
- Test with undefined/null values
it('should handle nested objects', () => { const result = makePropsTransient({ a: { nested: 1 }, b: { deep: 2 } } as const); expect(result).toEqual({ $a: { nested: 1 }, $b: { deep: 2 } }); });packages/components/src/utils/frameProps.tsx (1)
86-101: LGTM! Enhanced type safety with generics.The generic type improvements in
pickAndPrepareFramePropsprovide better type inference and validation.Optimize reduce accumulator performance.
The spread operator in the reduce accumulator could lead to O(n²) time complexity.
Apply this optimization to improve performance:
- const selectedProps = allowedFrameProps.reduce<{ - [value in KFP[number]]: TProps[value]; - }>( - (acc, item) => ({ ...acc, [item]: props[item] }), - {} as { [value in KFP[number]]: TProps[value] }, - ); + const selectedProps = allowedFrameProps.reduce<{ + [value in KFP[number]]: TProps[value]; + }>((acc, item) => { + acc[item] = props[item]; + return acc; + }, {} as { [value in KFP[number]]: TProps[value] });🧰 Tools
🪛 Biome (1.9.4)
[error] 96-96: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
packages/components/__mocks__/fileMock.js(1 hunks)packages/components/jest.config.js(1 hunks)packages/components/package.json(1 hunks)packages/components/src/components/Icon/Icon.tsx(1 hunks)packages/components/src/components/Tooltip/Tooltip.stories.tsx(3 hunks)packages/components/src/components/Tooltip/Tooltip.test.tsx(1 hunks)packages/components/src/components/Tooltip/Tooltip.tsx(4 hunks)packages/components/src/components/Tooltip/TooltipFloatingUi.tsx(3 hunks)packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.stories.tsx(1 hunks)packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx(1 hunks)packages/components/src/utils/frameProps.test.ts(1 hunks)packages/components/src/utils/frameProps.tsx(2 hunks)packages/components/src/utils/transientProps.test.ts(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/DeviceSelector/DeviceSelector.tsx(2 hunks)packages/suite/src/views/dashboard/PromoBanner.tsx(1 hunks)packages/suite/src/views/settings/SettingsDevice/FirmwareVersion.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/src/components/Icon/Icon.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.stories.tsx
- packages/components/package.json
- packages/suite/src/views/dashboard/PromoBanner.tsx
- packages/components/jest.config.js
- packages/components/mocks/fileMock.js
- packages/components/src/components/Tooltip/Tooltip.stories.tsx
- packages/suite/src/views/settings/SettingsDevice/FirmwareVersion.tsx
- packages/components/src/components/Tooltip/Tooltip.test.tsx
- packages/components/src/components/Tooltip/TooltipFloatingUi.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/DeviceSelector/DeviceSelector.tsx
- packages/components/src/components/Tooltip/Tooltip.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/src/utils/frameProps.tsx
[error] 96-96: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: Releases revision Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: EAS Update
🔇 Additional comments (5)
packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx (3)
5-5: LGTM! Good type safety improvements.The interface extension with
TooltipAllowedFramePropsproperly enables passing through tooltip configuration, which aligns with the PR's objective of controlling tooltip behavior.Also applies to: 13-16
18-20: Good state management improvements!The state rename to
isEllipsisActivebetter reflects its purpose, and using the rest operator enables proper prop forwarding to the tooltip component.
48-55: Great solution for the flickering issue!The addition of
isActiveprop and conditional rendering based onisEllipsisActiveeffectively addresses the PR's objective of fixing the flickering issue.packages/components/src/utils/frameProps.test.ts (1)
3-59: LGTM! Comprehensive type-safety test coverage.The test suite thoroughly validates the type-safety aspects of frame props handling, including:
- Basic key prefixing
- Individual key access
- Type errors for incorrect usage
- Non-existent key access
- Unprefixed key access
packages/components/src/utils/frameProps.tsx (1)
59-60: LGTM! Added 'inherit' cursor type.The addition of 'inherit' to cursors aligns with the PR's goal of fixing the flickering device status by allowing proper cursor inheritance.
vojtatranta
force-pushed
the
16178-flicking-device-status
branch
from
6f298a4 to
97b2352
Compare
|
|
||
| import { Tooltip } from './Tooltip'; | ||
|
|
||
| describe('Tooltip', () => { |
| props: Record<string, any>, | ||
| allowedFrameProps: Array<FramePropsKeys>, | ||
| convertToTransient = true, | ||
| export const pickAndPrepareFrameProps = < |
| @@ -4,4 +4,8 @@ module.exports = { | |||
| ...baseConfig, | |||
| setupFilesAfterEnv: ['<rootDir>/jest.setup.js'], | |||
| testEnvironment: 'jsdom', | |||
| moduleNameMapper: { | |||
There was a problem hiding this comment.
In the source code, there is import to .svg file and due to that, tests wouldn't start
vojtatranta
force-pushed
the
16178-flicking-device-status
branch
from
97b2352 to
6ca6721
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx (1)
24-40: Consider optimizing the ResizeObserver logic.The current implementation could be more concise and performant.
Consider this optimization:
- if (!containerRef.current) return; - const resizeObserver = new ResizeObserver(entries => { - const scrollWidth = containerRef.current?.scrollWidth ?? null; - const scrollHeight = containerRef.current?.scrollHeight ?? null; + const container = containerRef.current; + if (!container) return; + + const scrollWidth = container.scrollWidth; + const scrollHeight = container.scrollHeight; const borderBoxSize = entries[0].borderBoxSize?.[0]; - if (!borderBoxSize || !scrollWidth || !scrollHeight) { - return; - } + if (!borderBoxSize) return; const { inlineSize: elementWidth, blockSize: elementHeight } = borderBoxSize; - const nextEllipsisActive = - scrollWidth > Math.ceil(elementWidth) || scrollHeight > Math.ceil(elementHeight); - + const nextEllipsisActive = Math.ceil(scrollWidth) > elementWidth || + Math.ceil(scrollHeight) > elementHeight; setEllipsisActive(nextEllipsisActive);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
packages/components/__mocks__/fileMock.js(1 hunks)packages/components/jest.config.js(1 hunks)packages/components/package.json(1 hunks)packages/components/src/components/Icon/Icon.tsx(1 hunks)packages/components/src/components/Tooltip/Tooltip.stories.tsx(3 hunks)packages/components/src/components/Tooltip/Tooltip.test.tsx(1 hunks)packages/components/src/components/Tooltip/Tooltip.tsx(4 hunks)packages/components/src/components/Tooltip/TooltipFloatingUi.tsx(3 hunks)packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.stories.tsx(1 hunks)packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx(1 hunks)packages/components/src/utils/frameProps.test.ts(1 hunks)packages/components/src/utils/frameProps.tsx(2 hunks)packages/components/src/utils/transientProps.test.ts(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/DeviceSelector/DeviceSelector.tsx(2 hunks)packages/suite/src/views/dashboard/PromoBanner.tsx(1 hunks)packages/suite/src/views/settings/SettingsDevice/FirmwareVersion.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/components/mocks/fileMock.js
- packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.stories.tsx
- packages/components/package.json
- packages/components/src/utils/transientProps.test.ts
- packages/components/jest.config.js
- packages/suite/src/views/dashboard/PromoBanner.tsx
- packages/components/src/components/Icon/Icon.tsx
- packages/components/src/components/Tooltip/Tooltip.test.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/DeviceSelector/DeviceSelector.tsx
- packages/suite/src/views/settings/SettingsDevice/FirmwareVersion.tsx
- packages/components/src/components/Tooltip/TooltipFloatingUi.tsx
- packages/components/src/utils/frameProps.test.ts
- packages/components/src/components/Tooltip/Tooltip.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/src/utils/frameProps.tsx
[error] 96-96: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (7)
packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx (3)
5-5: LGTM! Good type extension for prop inheritance.The changes to extend
TooltipAllowedFramePropsimprove type safety and allow better control over tooltip properties.Also applies to: 13-16
18-19: LGTM! Improved state naming.The state variable name
isEllipsisActivebetter reflects its purpose compared to the previousisTooltipVisible.
48-55: LGTM! Improved tooltip control.The addition of the
isActiveprop and conditional rendering effectively addresses the flickering issue mentioned in the PR objectives.packages/components/src/components/Tooltip/Tooltip.stories.tsx (2)
9-9: LGTM!The imports are correctly organized, importing the necessary utilities for frame props configuration.
Also applies to: 16-16
111-111: LGTM!The story configuration correctly integrates frame props using the
getFramePropsStoryutility, enhancing the component's configurability in Storybook.Also applies to: 181-181
packages/components/src/utils/frameProps.tsx (2)
59-60: LGTM!The addition of 'inherit' to cursors is a valid enhancement, providing more flexibility in cursor inheritance.
86-92: Well-typed generic constraints! 👏The type improvements with generics enhance type safety and provide better IntelliSense support.
…d fix type / code error
vojtatranta
force-pushed
the
16178-flicking-device-status
branch
from
6ca6721 to
9b7707c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/components/src/utils/frameProps.tsx (1)
93-98: 🛠️ Refactor suggestionImprove performance of the reduce operation.
The spread operator in the accumulator can lead to O(n²) time complexity.
Apply this diff to improve performance:
- const selectedProps = allowedFrameProps.reduce<{ - [value in KFP[number]]: TProps[value]; - }>( - (acc, item) => ({ ...acc, [item]: props[item] }), - {} as { [value in KFP[number]]: TProps[value] }, - ); + const selectedProps = allowedFrameProps.reduce<{ + [value in KFP[number]]: TProps[value]; + }>( + (acc, item) => Object.assign(acc, { [item]: props[item] }), + {} as { [value in KFP[number]]: TProps[value] }, + );🧰 Tools
🪛 Biome (1.9.4)
[error] 96-96: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🧹 Nitpick comments (1)
packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx (1)
23-44: Consider debouncing the resize observer callback.While the logic is correct, resize events can fire frequently during window resizing. Consider debouncing the callback to optimize performance.
useEffect(() => { if (!containerRef.current) return; + const debounceTimeout = 150; // ms + let timeoutId: NodeJS.Timeout; const resizeObserver = new ResizeObserver(entries => { + clearTimeout(timeoutId); + timeoutId = setTimeout(() => { const scrollWidth = containerRef.current?.scrollWidth ?? null; const scrollHeight = containerRef.current?.scrollHeight ?? null; const borderBoxSize = entries[0].borderBoxSize?.[0]; if (!borderBoxSize || !scrollWidth || !scrollHeight) { return; } const { inlineSize: elementWidth, blockSize: elementHeight } = borderBoxSize; const nextEllipsisActive = scrollWidth > Math.ceil(elementWidth) || scrollHeight > Math.ceil(elementHeight); setEllipsisActive(nextEllipsisActive); + }, debounceTimeout); }); resizeObserver.observe(containerRef.current); - return () => resizeObserver.disconnect(); + return () => { + clearTimeout(timeoutId); + resizeObserver.disconnect(); + }; }, [children]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
packages/components/__mocks__/fileMock.js(1 hunks)packages/components/jest.config.js(1 hunks)packages/components/package.json(1 hunks)packages/components/src/components/Icon/Icon.tsx(1 hunks)packages/components/src/components/Tooltip/Tooltip.stories.tsx(3 hunks)packages/components/src/components/Tooltip/Tooltip.test.tsx(1 hunks)packages/components/src/components/Tooltip/Tooltip.tsx(5 hunks)packages/components/src/components/Tooltip/TooltipFloatingUi.tsx(3 hunks)packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.stories.tsx(1 hunks)packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx(1 hunks)packages/components/src/utils/frameProps.test.ts(1 hunks)packages/components/src/utils/frameProps.tsx(2 hunks)packages/components/src/utils/transientProps.test.ts(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/DeviceSelector/DeviceSelector.tsx(2 hunks)packages/suite/src/views/dashboard/PromoBanner.tsx(1 hunks)packages/suite/src/views/settings/SettingsDevice/FirmwareVersion.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.stories.tsx
- packages/components/jest.config.js
- packages/components/mocks/fileMock.js
- packages/components/package.json
- packages/components/src/components/Icon/Icon.tsx
- packages/components/src/utils/transientProps.test.ts
- packages/components/src/components/Tooltip/Tooltip.stories.tsx
- packages/suite/src/views/settings/SettingsDevice/FirmwareVersion.tsx
- packages/suite/src/views/dashboard/PromoBanner.tsx
- packages/components/src/components/Tooltip/Tooltip.test.tsx
- packages/components/src/components/Tooltip/TooltipFloatingUi.tsx
- packages/suite/src/components/suite/layouts/SuiteLayout/DeviceSelector/DeviceSelector.tsx
- packages/components/src/components/Tooltip/Tooltip.tsx
- packages/components/src/utils/frameProps.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/src/utils/frameProps.tsx
[error] 96-96: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-web
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (5)
packages/components/src/utils/frameProps.tsx (2)
59-59: LGTM! Addition of 'inherit' cursor value.The addition of 'inherit' to cursor options is valid and enables better control over cursor inheritance, which can help prevent flickering issues.
86-92: LGTM! Enhanced type safety with generics.The generic type parameters improve type safety by ensuring:
- Props match the FrameProps structure
- Allowed frame props are valid keys
packages/components/src/components/typography/TruncateWithTooltip/TruncateWithTooltip.tsx (3)
5-5: LGTM! Good improvement in type safety.The interface extension and type imports are well-structured, improving type safety while removing the unnecessary cursor prop.
Also applies to: 13-16
18-20: LGTM! Improved prop handling and state naming.The use of rest operator for prop forwarding and the more descriptive state name enhance code clarity and maintainability.
48-55: LGTM! Effective solution for the flickering issue.The use of
isActiveprop and conditional rendering effectively addresses the flickering issue mentioned in the PR objectives while maintaining proper truncation behavior.
|
QA OK Info:
|
Description
Resolve
Screenshots:
before:
https://github.com/user-attachments/assets/38f9a581-f260-4e5a-97d4-777b057f5d4b
Now:
Screen.Recording.2025-02-06.at.2.24.44.PM.mov