Optional welcome dialog #1440
Optional welcome dialog #1440
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces a new dialog management system for a React application, transitioning from a Zustand store-based approach to a custom hook-based solution. The changes include creating a new Changes
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (5)
apps/client/src/features/editors/welcome/WelcomePlacement.tsx (1)
8-10: Verify dialog visibility logic.The condition
showDialog !== 'welcome'might be better expressed as an enum value for type safety.Consider using an enum for dialog types:
- if (!showDialog || showDialog !== 'welcome') { + if (!showDialog || showDialog !== DialogKeys.Welcome) {This would require adding
Welcome = 'welcome'to theDialogKeysenum.apps/client/src/common/hooks/useDialogHook.ts (2)
6-8: Consider expanding DialogKeys enum.Add a
Welcomekey to maintain consistency and type safety across the application.export enum DialogKeys { SkipWelcome = 'ontime-skip-welcome', + Welcome = 'welcome', }
24-26: Simplify boolean expressions.The ternary operators for boolean values can be simplified.
- const skipWelcomeDialog = searchParams.get('skip-welcome') === null ? false : true; - const showWelcomeDialog = searchParams.get('show-welcome') === null ? false : true; + const skipWelcomeDialog = searchParams.get('skip-welcome') !== null; + const showWelcomeDialog = searchParams.get('show-welcome') !== null;🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
[error] 26-26: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
apps/client/src/features/editors/welcome/Welcome.tsx (1)
15-16: Consider adding JSDoc for props interface.Adding documentation for the new
skipDialogprop would improve maintainability.interface WelcomeProps { + /** Callback to permanently dismiss the welcome dialog */ skipDialog: () => void; }apps/client/src/common/utils/socket.ts (1)
125-126: Consider removing or guarding the console.log statement.While logging can be helpful during development, it's generally not recommended to keep debug logs in production code.
Consider either:
- Removing the log statement, or
- Guarding it with the existing
isProductionflag:- console.log('w dialog'); + if (!isProduction) { + console.log('w dialog'); + } addDialog('welcome');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
e2e/tests/000-upload-showfile.spec.tsis excluded by none and included by nonee2e/tests/001-smoke-tests.spec.tsis excluded by none and included by nonee2e/tests/features/203-delay-block.spec.tsis excluded by none and included by nonee2e/tests/features/204-editor-crud.spec.tsis excluded by none and included by nonee2e/tests/features/205-operator.spec.tsis excluded by none and included by nonee2e/tests/features/206-url-preset.spec.tsis excluded by none and included by nonee2e/tests/features/208-auxtimer.spec.tsis excluded by none and included by nonee2e/tests/features/301-spreadsheet-import.spec.tsis excluded by none and included by none
📒 Files selected for processing (6)
apps/client/src/common/hooks/useDialogHook.ts(1 hunks)apps/client/src/common/hooks/useElectronEvent.ts(1 hunks)apps/client/src/common/stores/dialogStore.ts(0 hunks)apps/client/src/common/utils/socket.ts(2 hunks)apps/client/src/features/editors/welcome/Welcome.tsx(2 hunks)apps/client/src/features/editors/welcome/WelcomePlacement.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/client/src/common/stores/dialogStore.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/client/src/common/hooks/useDialogHook.ts
[error] 25-25: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 26-26: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e-test
🔇 Additional comments (7)
apps/client/src/features/editors/welcome/WelcomePlacement.tsx (2)
1-1: LGTM! Clean transition to hook-based approach.The change from
useDialogStoretouseDialoghook improves testability and maintainability.Also applies to: 6-6
15-18: LGTM! Clear skip dialog implementation.The implementation correctly handles both clearing the current dialog and setting the skip preference.
apps/client/src/common/hooks/useElectronEvent.ts (1)
4-4: Verify electron dialog handling with new hook.While the import change is correct, we should ensure the electron dialog handling still works as expected with the new hook-based approach.
✅ Verification successful
Import change maintains dialog handling consistency ✅
The new hook-based approach maintains the same dialog handling pattern across both websocket and electron events, ensuring consistent behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any other electron dialog handlers that might need updating rg -l "dialog.*welcome" --type tsLength of output: 168
Script:
#!/bin/bash # Check the context of dialog usage in these files echo "=== WebsocketAdapter.ts ===" rg "dialog.*welcome" apps/server/src/adapters/WebsocketAdapter.ts -B 2 -A 2 echo -e "\n=== socket.ts ===" rg "dialog.*welcome" apps/client/src/common/utils/socket.ts -B 2 -A 2 echo -e "\n=== useElectronEvent.ts ===" rg "dialog.*welcome" apps/client/src/common/hooks/useElectronEvent.ts -B 2 -A 2Length of output: 874
apps/client/src/common/hooks/useDialogHook.ts (1)
28-35: LGTM! Clear persistence logic.The effect hook correctly handles the persistence of dialog preferences in local storage.
apps/client/src/features/editors/welcome/Welcome.tsx (1)
85-87: LGTM! Clear user interaction for dialog dismissal.The "Do not show again" button is well-placed and provides clear user feedback.
apps/client/src/common/utils/socket.ts (2)
6-6: LGTM! Import change aligns with architectural shift.The change from store-based to hook-based dialog management aligns with the PR's objective of restructuring welcome dialog handling.
Let's verify if this architectural change is consistently applied across the codebase:
✅ Verification successful
Import change verified - consistent architectural shift
The migration from store-based to hook-based dialog management is complete and consistent across the codebase. No remaining references to the old approach were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining imports from dialogStore rg "from.*stores/dialogStore" # Search for new hook usage rg "from.*hooks/useDialogHook"Length of output: 294
Script:
#!/bin/bash # Search for any remaining dialog store references rg -i "dialogstore|dialog[^h].*store" # Search for dialog-related patterns ast-grep --pattern 'dialog' # Check for any store files related to dialogs fd -e ts -e tsx "dialog.*store"Length of output: 612
124-128: Verify interaction between WebSocket and URL parameter controls.Given that the PR introduces URL parameters (
?skip-welcome,?show-welcome) to control the welcome dialog, we should ensure that the WebSocket-triggered dialog respects these settings. The current implementation might show the dialog even when it's meant to be skipped via URL parameters.Consider checking the user's preferences before showing the dialog:
case 'dialog': { if (payload.dialog === 'welcome') { if (!isProduction) { console.log('w dialog'); } - addDialog('welcome'); + // Assuming useDialogHook exports a function to check if dialog should be shown + if (shouldShowDialog('welcome')) { + addDialog('welcome'); + } } break; }
| export function addDialog(name: string): void { | ||
| if (booleanFromLocalStorage(DialogKeys.SkipWelcome, false) && name === 'welcome') return; | ||
|
|
||
| const urlParams = new URLSearchParams(window.location.search); | ||
| urlParams.append('dialog', name); | ||
| window.location.search = urlParams.toString(); | ||
| } |
There was a problem hiding this comment.
Potential race condition in addDialog.
The direct manipulation of window.location.search could lead to lost parameters if multiple dialogs are added simultaneously.
Consider using the URLSearchParams API more safely:
export function addDialog(name: string): void {
if (booleanFromLocalStorage(DialogKeys.SkipWelcome, false) && name === 'welcome') return;
const urlParams = new URLSearchParams(window.location.search);
urlParams.append('dialog', name);
- window.location.search = urlParams.toString();
+ const newUrl = new URL(window.location.href);
+ newUrl.search = urlParams.toString();
+ window.history.pushState({}, '', newUrl);
}📝 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 function addDialog(name: string): void { | |
| if (booleanFromLocalStorage(DialogKeys.SkipWelcome, false) && name === 'welcome') return; | |
| const urlParams = new URLSearchParams(window.location.search); | |
| urlParams.append('dialog', name); | |
| window.location.search = urlParams.toString(); | |
| } | |
| export function addDialog(name: string): void { | |
| if (booleanFromLocalStorage(DialogKeys.SkipWelcome, false) && name === 'welcome') return; | |
| const urlParams = new URLSearchParams(window.location.search); | |
| urlParams.append('dialog', name); | |
| const newUrl = new URL(window.location.href); | |
| newUrl.search = urlParams.toString(); | |
| window.history.pushState({}, '', newUrl); | |
| } |
Restructures the way we handle the welcome dialogs, making it easier to test as it can now be requested with search parameter and also allowing search parameter to diable it.
This is very beneficial as it can sometimes block the e2e test especially if run locally or out of order.
It also gives uses the option to dismis the dialog forever. Their is currently no way to get it back through the UI, it is easy to do I'm just not sure where it should be placed.
?skip-welcomewill save in local storage to not show?show-welcomewill reverse that?dialog=welcomewill force it to show