OCPBUGS-85545: Fix guided tour modal flash on page reload#16462
OCPBUGS-85545: Fix guided tour modal flash on page reload#16462Cragsmann wants to merge 1 commit into
Conversation
Prevent the "Welcome to the new OpenShift experience" modal from briefly appearing on every page reload for users who have already completed or skipped the tour. The stale-frame race between useReducer initialization and useEffect-based sync with user preferences caused startTour to default to true before the loaded completion state was applied. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
added
jira/valid-reference
|
@Cragsmann: This pull request references Jira Issue OCPBUGS-85545, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@Cragsmann: This pull request references Jira Issue OCPBUGS-85545, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| useUserPreferenceMock.mockReturnValue([{ dev: { completed: true } }, () => null, true]); | ||
| rerender(); | ||
| const { tourState } = result.current; | ||
| expect(tourState?.startTour).not.toBe(true); |
There was a problem hiding this comment.
nit:
| expect(tourState?.startTour).not.toBe(true); | |
| expect(tourState?.startTour).toBe(false); |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cragsmann, jhadvig The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
📝 WalkthroughWalkthroughThis PR fixes a state initialization race condition in the tour context hook. The change introduces an initialization guard using 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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: 1
🧹 Nitpick comments (1)
frontend/packages/console-app/src/components/tour/tour-context.ts (1)
161-163: 💤 Low valueUpdate the effect comment to reflect the loaded dependency.
The comment states the effect should "only run when the active perspective changes," but the effect now explicitly depends on
loadedand contains conditional logic based on that flag. The fix requires the effect to run whenloadedtransitions to true.📝 Suggested comment update
- // only run effect when the active perspective changes + // run effect when the active perspective or loaded state changes // eslint-disable-next-line react-hooks/exhaustive-deps🤖 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 `@frontend/packages/console-app/src/components/tour/tour-context.ts` around lines 161 - 163, Update the misleading effect comment to mention both dependencies: change the comment above the useEffect that currently references only "active perspective" to state that the effect runs when either activePerspective or loaded changes and that it specifically handles the transition when loaded becomes true; locate the comment immediately above the effect that includes the dependency array [activePerspective, loaded] and update it to accurately reflect that the effect depends on and reacts to loaded as well as activePerspective.
🤖 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
`@frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.ts`:
- Around line 143-156: Update the test for useTourValuesForContext so it asserts
the tour context is actually returned and that startTour is explicitly false:
after rerender, add an assertion that result.current.tour is not null (ensuring
the context was initialized) and change the startTour assertion to expect(false)
(i.e., expect(result.current.tourState?.startTour).toBe(false)) instead of only
checking it is not true; reference useTourValuesForContext, tour, tourState, and
startTour when locating the test to modify.
---
Nitpick comments:
In `@frontend/packages/console-app/src/components/tour/tour-context.ts`:
- Around line 161-163: Update the misleading effect comment to mention both
dependencies: change the comment above the useEffect that currently references
only "active perspective" to state that the effect runs when either
activePerspective or loaded changes and that it specifically handles the
transition when loaded becomes true; locate the comment immediately above the
effect that includes the dependency array [activePerspective, loaded] and update
it to accurately reflect that the effect depends on and reacts to loaded as well
as activePerspective.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b1454efb-1472-4a6f-a695-d933e924125d
📒 Files selected for processing (2)
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
frontend/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/**/*.{ts,tsx,js,jsx}: Never import from package index files (e.g.,@console/shared) in new code, as they can create circular dependencies and slow builds. Import from specific file paths instead.
Do not use backticks int()calls for i18n strings, as the i18n parser cannot extract keys from template literals. Use single or double quotes instead.
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import from deprecated packages or use code with the
@deprecatedTSdoc tag in new code.
**/*.{ts,tsx}: Use React functional components with hooks instead of class components
State Management: Use React hooks and Context API (migrating away from legacy Redux/Immutable.js)
Hooks: Use existing hooks fromconsole-sharedwhen possible (useK8sWatchResource,useUserSettings, etc.)
API calls: Use k8s resource hooks for data fetching,consoleFetchJSONfor HTTP requests
Extensions: Use console extension points for plugin integration
Types: Check existing types inconsole-sharedbefore creating new ones
Dynamic Plugins: Use console extension points for plugin integration
Styling: Use SCSS modules co-located with components, PatternFly design system components, avoid any SCSS/CSS if possible
Accessibility: Follow WCAG 2.1 AA standards, use semantic HTML, ARIA labels where needed, ensure keyboard navigation, test with screen readers
i18n: UseuseTranslation('namespace')hook withkeyformat for translation keys
Error Handling: Use ErrorBoundary components and graceful degradation patterns
Optimize re-renders: UseuseCallbackfor memoized callbacks to avoid function recreation every render
Optimize re-renders: UseuseMemofor expensive computations to avoid recalculating on every render
Lazy loading: UseReact.lazy()to lazy load heavy components
TypeScript type safety: Avoid usinganytype; suggest proper type definitions and verify null/undefined are handled properly
Type component props properly: Reuse existing component prop types instead of duplicating type definitions
Use proper hooks: Use specialized hooks likeusePluginInfofor plugin data instead of generic data fetching patterns
Avoid deprecated components: Check for JSDoc@deprecatedtags, import paths containing/deprecated, andDEPRECATED_file name prefix before using components
Importing from barrel files and circular dependencies: Import directly from specific files instead...
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
frontend/**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Never use absolute URLs or paths in the console code. The console runs behind a proxy under an arbitrary path.
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When writing code for static plugins, ensure that all
$codeRefreference the corresponding extension type from the@console/dynamic-plugin-sdkpackage.
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (TESTING.md)
**/*.{tsx,ts}: Always usepage.getByTestId('x')for Playwright selectors which queries[data-test="x"]. If a React element only has a legacy test attribute, adddata-testto the element. Never remove legacy attributes
Preferdata-testattributes in Cypress selectors (e.g.,cy.get('[data-test="create-deployment"]')) over brittle CSS/ARIA selectorsFile Naming: PascalCase for components, kebab-case for utilities,
*.spec.ts(x)for tests
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
**/*.{go,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Use lowercase dash-separated names for all files to avoid git issues with case-insensitive file systems
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx,js,jsx}: New code MUST be written in TypeScript, not JavaScript
Prefer functional programming patterns and immutable data structures
Run the linter and follow all rules defined in .eslintrc
Never use absolute paths in code - the app should be able to run behind a proxy under an arbitrary path
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Tests should follow a similar 'test tables' convention as used in Go where applicable
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.ts
**/*.ts
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Plugin SDK Changes: Any updates to
console-dynamic-plugin-sdkshould aim to maintain backward compatibility as it's a public API - use theplugin-api-reviewskill to vet changes for public API impact and ensure proper documentation updates
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
frontend/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (README.md)
frontend/**/*.{js,ts,tsx}: Support only the latest versions of Edge, Chrome, Safari, and Firefox browsers; IE 11 and earlier are not supported
CSP violations should be automatically reported to telemetry by parsing dynamic plugin names from securitypolicyviolation events, with throttling to prevent duplicate reports within a day
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (INTERNATIONALIZATION.md)
For dynamic translation keys that cannot be parsed by i18next-parser (t(key), t('key' + id), t(
key${id})), specify possible static values in comments for the parser to extract
Files:
frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.tsfrontend/packages/console-app/src/components/tour/tour-context.ts
🔇 Additional comments (3)
frontend/packages/console-app/src/components/tour/tour-context.ts (3)
2-2: LGTM!Also applies to: 154-154
158-160: LGTM!
165-165: LGTM!
| it('should not flash startTour when loaded transitions to true with completed tour', () => { | ||
| useSelectorMock.mockReturnValue({ A: true, B: false }); | ||
| useResolvedExtensionsMock.mockReturnValue(mockTourExtension); | ||
| // Start with loaded: false (async ConfigMap fetch in progress) | ||
| useUserPreferenceMock.mockReturnValue([{ dev: { completed: false } }, () => null, false]); | ||
| const { result, rerender } = renderHook(() => useTourValuesForContext()); | ||
| expect(result.current.tour).toEqual(null); | ||
|
|
||
| // Simulate ConfigMap load completing with completed: true | ||
| useUserPreferenceMock.mockReturnValue([{ dev: { completed: true } }, () => null, true]); | ||
| rerender(); | ||
| const { tourState } = result.current; | ||
| expect(tourState?.startTour).not.toBe(true); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Strengthen the test assertion to verify the tour context is returned.
The test currently only checks tourState?.startTour is not true, which would pass even if tour remains null (since undefined !== true). To properly verify the initialization guard allows the context through after loading, assert that:
touris not null (context was returned)startTouris explicitlyfalse
🧪 Strengthened test assertions
// Simulate ConfigMap load completing with completed: true
useUserPreferenceMock.mockReturnValue([{ dev: { completed: true } }, () => null, true]);
rerender();
- const { tourState } = result.current;
- expect(tourState?.startTour).not.toBe(true);
+ const { tourState, tour } = result.current;
+ expect(tour).not.toBeNull();
+ expect(tourState?.startTour).toBe(false);📝 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.
| it('should not flash startTour when loaded transitions to true with completed tour', () => { | |
| useSelectorMock.mockReturnValue({ A: true, B: false }); | |
| useResolvedExtensionsMock.mockReturnValue(mockTourExtension); | |
| // Start with loaded: false (async ConfigMap fetch in progress) | |
| useUserPreferenceMock.mockReturnValue([{ dev: { completed: false } }, () => null, false]); | |
| const { result, rerender } = renderHook(() => useTourValuesForContext()); | |
| expect(result.current.tour).toEqual(null); | |
| // Simulate ConfigMap load completing with completed: true | |
| useUserPreferenceMock.mockReturnValue([{ dev: { completed: true } }, () => null, true]); | |
| rerender(); | |
| const { tourState } = result.current; | |
| expect(tourState?.startTour).not.toBe(true); | |
| }); | |
| it('should not flash startTour when loaded transitions to true with completed tour', () => { | |
| useSelectorMock.mockReturnValue({ A: true, B: false }); | |
| useResolvedExtensionsMock.mockReturnValue(mockTourExtension); | |
| // Start with loaded: false (async ConfigMap fetch in progress) | |
| useUserPreferenceMock.mockReturnValue([{ dev: { completed: false } }, () => null, false]); | |
| const { result, rerender } = renderHook(() => useTourValuesForContext()); | |
| expect(result.current.tour).toEqual(null); | |
| // Simulate ConfigMap load completing with completed: true | |
| useUserPreferenceMock.mockReturnValue([{ dev: { completed: true } }, () => null, true]); | |
| rerender(); | |
| const { tourState, tour } = result.current; | |
| expect(tour).not.toBeNull(); | |
| expect(tourState?.startTour).toBe(false); | |
| }); |
🤖 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
`@frontend/packages/console-app/src/components/tour/__tests__/tour-context.spec.ts`
around lines 143 - 156, Update the test for useTourValuesForContext so it
asserts the tour context is actually returned and that startTour is explicitly
false: after rerender, add an assertion that result.current.tour is not null
(ensuring the context was initialized) and change the startTour assertion to
expect(false) (i.e., expect(result.current.tourState?.startTour).toBe(false))
instead of only checking it is not true; reference useTourValuesForContext,
tour, tourState, and startTour when locating the test to modify.
|
@Cragsmann: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
useReducerinitialization (defaultsstartTour: truewhilecompletedisundefined) anduseEffect-based sync with user preferencesloadedbeing trueJira
https://redhat.atlassian.net/browse/OCPBUGS-85545
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests