Skip to content

Env by app#3303

Draft
nighca wants to merge 1 commit into
goplus:devfrom
nighca:issue-3299
Draft

Env by app#3303
nighca wants to merge 1 commit into
goplus:devfrom
nighca:issue-3299

Conversation

@nighca

@nighca nighca commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the application configuration by removing the global environment file and introducing explicit configuration functions and environment objects for the 'account' and 'xbuilder' apps. This prevents modules from directly importing global environment variables. The review feedback highlights a bug in the parseSampleRate helper where a valid sample rate of 0 incorrectly triggers the fallback value, an out-of-order variable declaration within imports in EditorNavbar.vue, and a potential memory leak/redundant setup issue in initUserState if called multiple times.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +8 to +10
function parseSampleRate(value: string, fallback: number) {
return parseFloat(value) || fallback
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The parseSampleRate function uses parseFloat(value) || fallback. If the sample rate is explicitly configured as 0 (which is a valid setting to disable tracing/sampling), parseFloat(value) evaluates to 0, causing the function to incorrectly fall back to the default value (0.1).

Use isNaN to check if the parsed value is valid instead.

Suggested change
function parseSampleRate(value: string, fallback: number) {
return parseFloat(value) || fallback
}
function parseSampleRate(value: string, fallback: number) {
const parsed = parseFloat(value)
return isNaN(parsed) ? fallback : parsed
}

Comment on lines +16 to +18
function parseSampleRate(value: string, fallback: number) {
return parseFloat(value) || fallback
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The parseSampleRate function uses parseFloat(value) || fallback. If the sample rate is explicitly configured as 0 (which is a valid setting to disable tracing/sampling), parseFloat(value) evaluates to 0, causing the function to incorrectly fall back to the default value (0.1).

Use isNaN to check if the parsed value is valid instead.

Suggested change
function parseSampleRate(value: string, fallback: number) {
return parseFloat(value) || fallback
}
function parseSampleRate(value: string, fallback: number) {
const parsed = parseFloat(value)
return isNaN(parsed) ? fallback : parsed
}

import EditorCheckoutReleaseButton from './EditorCheckoutReleaseButton.vue'
import { EditMode, type EditorState } from '../editor-state'

const { showTutorialsEntry } = getXBuilderConfig()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The variable declaration const { showTutorialsEntry } = getXBuilderConfig() is placed in the middle of the import statements block. All import statements must precede any other code execution or variable declarations at the module level to adhere to standard style guidelines (such as ESLint's import/first rule) and prevent potential hoisting or initialization issues.

Please move this statement below all the import statements (e.g., right before const props = defineProps(...)).

Comment on lines +39 to +44
export function initUserState(config: UserStateConfig) {
userStateConfig = config
oauthFlow = new OAuthFlow<{ returnTo: string }>(oauthApis, {
clientId: config.accountOAuthClientId,
redirectUri: `${window.location.origin}/sign-in/callback`
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If multiple widgets or apps are loaded on the same page, initUserState may be called multiple times. This will register multiple redundant watchEffect listeners on the global userState reactive object, leading to duplicate writes to localStorage and potential memory leaks.

Add an early return if userStateConfig is already initialized to prevent redundant setup.

Suggested change
export function initUserState(config: UserStateConfig) {
userStateConfig = config
oauthFlow = new OAuthFlow<{ returnTo: string }>(oauthApis, {
clientId: config.accountOAuthClientId,
redirectUri: `${window.location.origin}/sign-in/callback`
})
export function initUserState(config: UserStateConfig) {
if (userStateConfig != null) return
userStateConfig = config
oauthFlow = new OAuthFlow<{ returnTo: string }>(oauthApis, {
clientId: config.accountOAuthClientId,
redirectUri: `${window.location.origin}/sign-in/callback`
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant