-
Notifications
You must be signed in to change notification settings - Fork 120
only popup the vscode env var dialog once #1066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 4e09b54 in 22 seconds
More details
- Looked at
65
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_TfkOxh1kcYM03uhN
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
<Dialog open={showSettings} onOpenChange={setShowSettings}> | ||
<Dialog open={showSettings} onOpenChange={(open) => { | ||
setShowSettings(open) | ||
setHasClosedEnvVarsDialog(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for setting hasClosedEnvVarsDialog
is incorrect. It should be set to true
only when the dialog is closed, not when it is opened. Consider updating the logic to:
setHasClosedEnvVarsDialog(true) | |
setHasClosedEnvVarsDialog(!open) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 2ac140b in 17 seconds
More details
- Looked at
35
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. typescript/playground-common/src/shared/SettingsDialog.tsx:250
- Draft comment:
Consider addinghasClosedEnvVarsDialog
to the dependency array of theuseEffect
hook to ensure the effect re-runs when it changes. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_ZqYOItH3JtUaXYpF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Ensure the VSCode environment variables dialog only appears once by tracking its closed state with
hasClosedEnvVarsDialogAtom
.hasClosedEnvVarsDialogAtom
inEventListener.tsx
to track if the environment variables dialog has been closed.ShowSettingsButton
andSettingsDialog
inSettingsDialog.tsx
to checkhasClosedEnvVarsDialogAtom
before showing the dialog.atomWithStorage
forhasClosedEnvVarsDialogAtom
to persist the closed state across sessions.SettingsDialog.tsx
, the dialog only opens automatically ifhasClosedEnvVarsDialogAtom
isfalse
.This description was created by
for 2ac140b. It will automatically update as commits are pushed.