fix: stabilize PR272 new-session UX, profiles gating, and env safety#12
fix: stabilize PR272 new-session UX, profiles gating, and env safety#12leeroybrun wants to merge 0 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds profile management and favorites, Gemini model-mode & permission typing and UI wiring, reusable machine/path pickers with search/favorites, machine-backed environment‑variable preview/editing, modal typing changes, extensive i18n updates, new UI components, tests, and removal of the old NewSessionWizard and ProfileSync service. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @leeroybrun, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on stabilizing and refining the new session creation and profile management features introduced in a previous large merge. It addresses critical regressions and safety concerns, enhances user experience by making new functionalities optional and coherent, and improves the underlying architecture for better maintainability and security. The changes aim to restore a familiar workflow while integrating powerful new capabilities in a controlled and robust manner. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive pull request that successfully stabilizes the new-session UX, gates new features behind settings, and hardens the logic for profiles and environment variables. The restoration of the pre-existing "New Session" modal flow while making the new wizard an opt-in feature is a great improvement for user experience. The extensive refactoring to introduce reusable components like MachineSelector, PathSelector, and ItemRowActions significantly improves code maintainability and consistency across the application.
Key improvements I noted include:
- Robustness: The settings parser is now fault-tolerant, preventing a single invalid profile from resetting all user settings. Environment variable parsing has been made much more reliable, correctly distinguishing between unset and empty variables. Message normalization for tool results is also more robust, preventing messages from being dropped.
- Security: The implementation now includes heuristics to avoid fetching secret-like environment variables from remote machines, which is a crucial security enhancement. Logging of potentially sensitive data has also been commendably reduced.
- State Management: The navigation and state management for profile editing have been completely overhauled, moving away from a fragile callback-based system to a much more robust ID-based routing approach that correctly handles unsaved changes.
Overall, this is a very high-quality contribution that addresses the stated goals effectively and substantially improves the stability and maintainability of the codebase. I found no issues that meet the requested severity threshold for comments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
sources/modal/components/WebAlertModal.tsx (1)
6-6: UseStyleSheetfromreact-native-unistylesinstead ofreact-native.Per coding guidelines, all styling should use
StyleSheet.createfromreact-native-unistyles. This enables theme and runtime access in style definitions.♻️ Suggested fix
-import { StyleSheet } from 'react-native'; +import { StyleSheet } from 'react-native-unistyles';sources/app/(app)/new/pick/machine.tsx (1)
11-28: Move the stylesheet to the end of the file.App pages should place styles at the very end of the file; please relocate the
StyleSheet.create(...)block accordingly. As per coding guidelines.sources/sync/storage.ts (1)
808-826: Add persistence formodelModelikepermissionMode.The
applySessionshandler loadssessionPermissionModeson app startup (line 305-306) but does not loadmodelMode. UnlikepermissionMode, which is explicitly saved viasaveSessionPermissionModesand restored during sync,modelModeupdates are lost on app restart. ImplementsaveSessionModelModesand load it inapplySessionsto match thepermissionModepersistence pattern, or remove the load-restoration logic ifmodelModeshould not be persisted.
🤖 Fix all issues with AI agents
In `@sources/app/`(app)/new/pick/profile.tsx:
- Around line 146-351: Multiple inline style objects (e.g., container styles in
renderProfileRowRightElement, renderDefaultEnvironmentRowRightElement, the
ItemList paddingTop, and Item rightElement row wrappers) violate the guideline;
extract them into a Unistyles stylesheet created via StyleSheet.create((theme)
=> ({})) placed at the end of the file and replace the inline style objects with
the corresponding style names. Specifically, replace the View style={{
flexDirection: 'row', alignItems: 'center', gap: 16 }}, the inner View
width/align/justify styles, Ionicons style opacity, the ItemList style={{
paddingTop: 0 }}, and any other inline style occurrences used in Item, ItemGroup
or ItemRowActions with styles from the new stylesheet (e.g., row,
rowIconWrapper, checkIcon) and import/use the theme parameter in the stylesheet
for colors.
In `@sources/components/AgentInput.tsx`:
- Around line 965-972: The Text label in AgentInput.tsx currently renders a
user-visible string without localization and hides a zero count by using a
truthy check; update the rendering that uses props.envVarsCount (the Text
element showing "Env Vars" / `Env Vars (${props.envVarsCount})`) to call the
translation function t(...) for the base label and use an explicit numeric
check/format (e.g., always render the count with props.envVarsCount !==
undefined ? props.envVarsCount : '' or use a pluralized/localized format via t
with a count) so that zero displays as "Env Vars (0)" and the text is localized;
adjust the Text content to use the translated string returned by t(...) and
include the numeric count in the localized output.
In `@sources/components/EnvironmentVariableCard.tsx`:
- Line 103: The fallbackOperator state is only initialized once from
parsed.fallbackOperator and can become stale if variable.value (or parsed)
changes; update the component to derive fallbackOperator from props instead of
fixed state or keep it stateful but sync it when parsed/variable.value changes:
replace the useState<[fallbackOperator]> initialization with a derived value
(e.g., compute fallbackOperator = parsed.fallbackOperator) or add a useEffect
that updates the fallbackOperator state when parsed or variable.value changes;
reference the existing fallbackOperator variable, parsed.fallbackOperator, and
any related hooks like useRemoteVariable/remoteVariableName/defaultValue to
ensure all dependent state stays in sync.
- Around line 112-120: The effect in EnvironmentVariableCard uses onUpdate which
is currently passed as an inline arrow from the parent and thus changes every
render; to fix, memoize the callback in the parent (EnvironmentVariablesList)
with useCallback (e.g. memoize the function that calls handleUpdateVariable) or
change the parent to pass handleUpdateVariable directly via a curried function
so the reference is stable; ensure the prop name onUpdate kept on
EnvironmentVariableCard receives a stable function so the useEffect in
EnvironmentVariableCard (which references useRemoteVariable, remoteVariableName,
defaultValue, fallbackOperator, variable.value and onUpdate) no longer
retriggers unnecessarily.
In `@sources/components/ItemActionsMenuModal.tsx`:
- Around line 33-89: Move all inline style objects in ItemActionsMenuModal (the
outer container View, the header View, the title Text, the ScrollView and its
contentContainerStyle, and the Pressable style function) into a Unistyles
stylesheet using StyleSheet.create((theme) => ({})); create named entries like
container, header, title, scroll, scrollContent, and pressable and replace the
inline style props with styles.container, styles.header, styles.title,
style={styles.scroll} and contentContainerStyle={styles.scrollContent}; for the
Pressable, implement style={({pressed}) => ({...styles.pressable, opacity:
pressed ? 0.7 : 1})} so pressed state is preserved; keep icon props and mapping
logic (props.title, props.actions, closeThen) unchanged.
- Around line 1-70: The "Actions" header in ItemActionsMenuModal is hardcoded;
update the ItemGroup title prop to use the i18n translator (e.g.
t('itemActions.actions') or an appropriate key) instead of the literal string,
and ensure the translation key is added to the locale files; locate the title
usage inside the ItemActionsMenuModal component where ItemGroup is rendered and
replace "Actions" with t(...) while importing/using the same t function or hook
your project uses for translations.
In `@sources/components/newSession/EnvironmentVariablesPreviewModal.tsx`:
- Around line 75-246: Extract all inline style objects used in the
EnvironmentVariablesPreviewModal component (the top-level container View, the
header View, title Text, close Pressable style, ScrollView/content container,
the description Text block, the empty-state View/Text, and any per-ItemGroup
wrapper) into a Unistyles stylesheet created with StyleSheet.create((theme) =>
({})) and replace inline style props with styles.* references; keep using the
existing theme and Platform checks (e.g., the top-level style object, header
block, the description Text that uses Platform.select, and the empty message
block), move complex shared typography merges into the stylesheet but preserve
dynamic bits (like pressed opacity on Pressable and the web onWheel spread)
inline as needed, and update imports/usage so all style lookups reference the
new styles object.
In `@sources/components/newSession/MachineSelector.tsx`:
- Around line 81-86: The current filterItem function in MachineSelector.tsx only
searches displayName and host from machine.metadata, so if metadata is null the
displayed machine id won't match; update filterItem to also normalize and search
machine.id (e.g., const id = (machine.id || '').toLowerCase()) and include
id.includes(search) in the return condition alongside
displayName.includes(search) || host.includes(search).
In `@sources/components/newSession/PathSelector.tsx`:
- Around line 317-574: The component uses hard-coded user strings (e.g., "Search
paths...", ItemGroup titles like "Enter Path", "Recent Paths", "Favorite Paths",
"Suggested Paths", "Paths", placeholders such as "Enter path (e.g.
/home/user/projects)", and empty-state labels like "No matches", "No recent
paths", "No favorite paths", "No suggested paths", "No paths") — update each
visible string to use the translation helper t(...) from '@/text' instead of
literals; search for usages around SearchHeader, ItemGroup, the TextInput
placeholder in PathSelector, and Item title props as well as any calls to
renderRightElement/renderCustomRightElement and setPathAndFocus to ensure all
displayed text is wrapped (e.g., t('pathSelector.searchPlaceholder'),
t('pathSelector.enterPathTitle'), etc.), keeping the same string keys across
similar occurrences and passing translated strings into the existing props
(value/placeholder/title).
- Around line 70-73: Detect and prevent mixed controlled/uncontrolled usage for
the search input by adding an explicit isSearchQueryControlled flag (derived
from controlledSearchQuery and onChangeSearchQueryProp) and assert or throw when
only one of controlledSearchQuery/onChangeSearchQueryProp is provided; use
useCallback to create setSearchQuery that either calls onChangeSearchQueryProp
or setUncontrolledSearchQuery depending on isSearchQueryControlled, and keep
searchQuery sourced from controlledSearchQuery when controlled or
uncontrolledSearchQuery otherwise (update symbols: uncontrolledSearchQuery,
setUncontrolledSearchQuery, controlledSearchQuery, onChangeSearchQueryProp,
searchQuery, setSearchQuery, isSearchQueryControlled, useCallback). Also replace
all hardcoded UI strings ("Search paths...", "No matches", "No recent paths",
"No favorite paths", "No suggested paths", "No paths") with t(...) calls for
i18n and move any inline containerStyle objects passed to SearchHeader into the
Unistyles stylesheet (hoist styles and reference them instead).
In `@sources/components/Switch.web.tsx`:
- Around line 50-58: The thumb View always uses the active color; update the
style in the Switch component so backgroundColor uses the switch state (value) —
i.e., set backgroundColor to value ? theme.colors.switch.thumb.active :
theme.colors.switch.thumb.inactive (keeping styles.thumb and transform: [{
translateX }] intact) so thumb color matches the track logic.
In `@sources/modal/components/CustomModal.tsx`:
- Around line 41-43: The modal is wired to close directly via the raw onClose
prop, bypassing the animation-aware handler; change the CommandPaletteModal prop
from onClose={onClose} to onClose={handleClose} so that all close triggers
(overlay/ESC/button) go through the animation-aware handler (see
CommandPaletteModal, CommandPalette, handleClose and isClosing) and preserve the
existing delayed close animation behavior.
In `@sources/sync/profileGrouping.ts`:
- Around line 44-46: favoriteIds is currently built directly from
favoriteProfileIds which may include IDs that don't resolve to actual profiles,
causing inconsistency with favoriteProfiles (which filters via isProfile);
change the logic so favoriteProfiles is created first using
favoriteProfileIds.map(...).filter(isProfile) (using customById and
getBuiltInProfile) and then derive favoriteIds from that filtered list (e.g.,
new Set(favoriteProfiles.map(p => p.id))) so both collections are consistent and
only contain valid profile IDs.
In `@sources/sync/sync.ts`:
- Line 1175: The call to storage.getState().replaceSettings(mergedSettings,
data.currentVersion) unconditionally overwrites settings and can cause version
downgrades; replace it with storage.getState().applySettings(mergedSettings,
data.currentVersion) so the state only updates when the incoming version is
newer (use applySettings in both places where replaceSettings is currently used,
e.g., the two occurrences at sync logic points), preserving the
version-comparison safety implemented in storage.ts (applySettings vs
replaceSettings).
🧹 Nitpick comments (33)
sources/utils/promptUnsavedChangesAlert.test.ts (1)
5-25: Test coverage could be expanded.The test correctly validates the 'save' path. Consider adding tests for the 'discard' and 'keepEditing' resolution paths to ensure complete coverage of all three decision outcomes.
💡 Additional test cases
it('resolves to discard when the Discard button is pressed', async () => { let lastButtons: AlertButton[] | undefined; const alert = (_title: string, _message?: string, buttons?: AlertButton[]) => { lastButtons = buttons; }; const promise = promptUnsavedChangesAlert(alert, { title: 'Discard changes', message: 'You have unsaved changes.', discardText: 'Discard', saveText: 'Save', keepEditingText: 'Keep editing', }); lastButtons?.find((b) => b.text === 'Discard')?.onPress?.(); await expect(promise).resolves.toBe('discard'); }); it('resolves to keepEditing when the Keep editing button is pressed', async () => { let lastButtons: AlertButton[] | undefined; const alert = (_title: string, _message?: string, buttons?: AlertButton[]) => { lastButtons = buttons; }; const promise = promptUnsavedChangesAlert(alert, { title: 'Discard changes', message: 'You have unsaved changes.', discardText: 'Discard', saveText: 'Save', keepEditingText: 'Keep editing', }); lastButtons?.find((b) => b.text === 'Keep editing')?.onPress?.(); await expect(promise).resolves.toBe('keepEditing'); });sources/components/tools/views/GeminiEditView.tsx (1)
62-73: Unnecessary React Fragment wrapper.The Fragment (
<>...</>) wraps only a singleToolSectionViewchild. It can be removed for cleaner JSX.Proposed simplification
return ( - <> - <ToolSectionView fullWidth> - <ToolDiffView - oldText={oldString} - newText={newString} - showLineNumbers={showLineNumbersInToolViews} - showPlusMinusSymbols={showLineNumbersInToolViews} - /> - </ToolSectionView> - </> + <ToolSectionView fullWidth> + <ToolDiffView + oldText={oldString} + newText={newString} + showLineNumbers={showLineNumbersInToolViews} + showPlusMinusSymbols={showLineNumbersInToolViews} + /> + </ToolSectionView> );sources/components/tools/knownTools.tsx (3)
543-566: Consider extracting shared reasoning tool logic.
GeminiReasoningis nearly identical toCodexReasoning(lines 519-542), differing only in thestatusenum values ('canceled'vs'error'). Consider extracting a factory function or shared base definition to reduce duplication.
657-686: Duplicated parsing logic withGeminiExecuteView.tsx.The title parsing logic in
execute.titleandexecute.extractSubtitle(extracting command before[, description from parentheses) duplicates theextractExecuteInfofunction inGeminiExecuteView.tsx(lines 12-44). Consider extracting a shared utility.
763-817: Significant duplication withCodexPatch.
GeminiPatchshares nearly identicalextractSubtitleandextractDescriptionlogic withCodexPatch(lines 687-740). The main difference isisMutable: true. Consider extracting shared helpers or a factory function.sources/utils/ignoreNextRowPress.test.ts (1)
4-16: Use try/finally (or afterEach) to restore timers.
Ensures real timers are restored even if an assertion fails.♻️ Suggested tweak
it('resets the ignore flag on the next tick', () => { vi.useFakeTimers(); - const ref = { current: false }; - - ignoreNextRowPress(ref); - expect(ref.current).toBe(true); - - vi.runAllTimers(); - expect(ref.current).toBe(false); - - vi.useRealTimers(); + try { + const ref = { current: false }; + ignoreNextRowPress(ref); + expect(ref.current).toBe(true); + vi.runAllTimers(); + expect(ref.current).toBe(false); + } finally { + vi.useRealTimers(); + } });sources/sync/persistence.ts (1)
152-154: Consider validatingmodelModeagainst known values.The current implementation accepts any string as a valid
ModelMode. If corrupted or outdated data contains an invalid mode (e.g., a removed model identifier), it will pass through unchecked and could cause issues downstream.The same pattern exists for
permissionMode(lines 149-151), so this is a pre-existing concern. Consider adding a validation helper that checks against known values and falls back to'default'if invalid.Example validation approach
const VALID_MODEL_MODES: ModelMode[] = [ 'default', 'adaptiveUsage', 'sonnet', 'opus', 'gpt-5-codex-high', 'gpt-5-codex-medium', 'gpt-5-codex-low', 'gpt-5-minimal', 'gpt-5-low', 'gpt-5-medium', 'gpt-5-high', 'gemini-2.5-pro', 'gemini-2.5-flash', 'gemini-2.5-flash-lite' ]; const modelMode: ModelMode = VALID_MODEL_MODES.includes(parsed.modelMode) ? parsed.modelMode : 'default';sources/modal/components/BaseModal.tsx (1)
7-7: Consider usingStyleSheet.createfromreact-native-unistyles.The file imports
StyleSheetfromreact-native, but per coding guidelines,StyleSheet.createshould be imported from'react-native-unistyles'for all styling. This would provide consistent theming capabilities.Suggested import change
import { View, Modal, TouchableWithoutFeedback, Animated, - StyleSheet, KeyboardAvoidingView, Platform } from 'react-native'; +import { StyleSheet } from 'react-native-unistyles';Based on coding guidelines: "Use StyleSheet.create from 'react-native-unistyles' for all styling".
sources/sync/permissionMapping.ts (1)
3-3: Avoid duplicating AgentType.Line 3 redefines
AgentType, which already exists insources/sync/modelOptions.ts. Importing it prevents drift if the agent list changes later.♻️ Proposed refactor
-import type { PermissionMode } from './permissionTypes'; - -type AgentType = 'claude' | 'codex' | 'gemini'; +import type { PermissionMode } from './permissionTypes'; +import type { AgentType } from './modelOptions';sources/components/ItemGroup.tsx (1)
13-15: Prefer the@/alias for new imports/exports.Switching to
@/components/ItemGroup.dividerskeeps new references aligned with the path-alias guideline. As per coding guidelines, ...♻️ Proposed refactor
-import { withItemGroupDividers } from './ItemGroup.dividers'; - -export { withItemGroupDividers } from './ItemGroup.dividers'; +import { withItemGroupDividers } from '@/components/ItemGroup.dividers'; + +export { withItemGroupDividers } from '@/components/ItemGroup.dividers';sources/components/Item.tsx (2)
115-116: Inconsistent indentation detected.Lines 116, 202-205, and 291-307 use tab characters instead of the required 4 spaces for indentation. As per coding guidelines, use 4 spaces for indentation consistently.
🔧 Suggested fix
const { theme } = useUnistyles(); const styles = stylesheet; - const selectionContext = React.useContext(ItemGroupSelectionContext); - + const selectionContext = React.useContext(ItemGroupSelectionContext); + // Platform-specific measurements
291-307: Indentation issues in Pressable block.The tab-based indentation continues here. Consider normalizing to 4 spaces for consistency with the codebase conventions.
🔧 Suggested fix for indentation
- if (isInteractive) { - return ( - <Pressable + if (isInteractive) { + return ( + <Pressable onPress={handlePress} onLongPress={onLongPress} onPressIn={handlePressIn} onPressOut={handlePressOut} - disabled={disabled || loading} - style={({ pressed }) => [ - { - backgroundColor: pressed && isIOS && !isWeb - ? theme.colors.surfacePressedOverlay - : (showSelectedBackground ? theme.colors.surfaceSelected : 'transparent'), - opacity: disabled ? 0.5 : 1 - }, - pressableStyle - ]} + disabled={disabled || loading} + style={({ pressed }) => [ + { + backgroundColor: pressed && isIOS && !isWeb + ? theme.colors.surfacePressedOverlay + : (showSelectedBackground ? theme.colors.surfaceSelected : 'transparent'), + opacity: disabled ? 0.5 : 1 + }, + pressableStyle + ]}sources/components/newSession/ProfileCompatibilityIcon.tsx (1)
31-34: Clarify the purpose of the unuseduseUnistyles()call.The
useUnistyles()hook is called but its return value is not used. If this is intentional for subscribing to theme updates (to trigger re-renders), consider adding a brief comment to clarify this intent for future maintainers.export function ProfileCompatibilityIcon({ profile, size = 32, style }: Props) { - useUnistyles(); + useUnistyles(); // Subscribe to theme changes for re-render const styles = stylesheet;sources/profileRouteParams.test.ts (1)
4-39: Good test coverage forconsumeProfileIdParam.The tests comprehensively cover the main use cases:
- Missing param
- Empty string (deselection)
- Matching current selection
- Different selection
- Array input
Consider adding a test for the empty array edge case to ensure consistent behavior:
📝 Optional: Add empty array test
it('treats empty array as missing param', () => { expect(consumeProfileIdParam({ profileIdParam: [], selectedProfileId: 'abc' })).toEqual({ nextSelectedProfileId: undefined, shouldClearParam: false, }); });sources/sync/modelOptions.ts (1)
11-15: User-visible strings should uset(...)for internationalization.Per coding guidelines, all user-visible strings must use the
t(...)function from@/textfor internationalization. The labels and descriptions here are displayed in the UI.♻️ Suggested refactor
+import { t } from '@/text'; + const GEMINI_MODEL_OPTIONS: readonly ModelOption[] = [ - { value: 'gemini-2.5-pro', label: 'Gemini 2.5 Pro', description: 'Most capable' }, - { value: 'gemini-2.5-flash', label: 'Gemini 2.5 Flash', description: 'Fast & efficient' }, - { value: 'gemini-2.5-flash-lite', label: 'Gemini 2.5 Flash Lite', description: 'Fastest' }, + { value: 'gemini-2.5-pro', label: t('gemini_model_pro'), description: t('gemini_model_pro_desc') }, + { value: 'gemini-2.5-flash', label: t('gemini_model_flash'), description: t('gemini_model_flash_desc') }, + { value: 'gemini-2.5-flash-lite', label: t('gemini_model_flash_lite'), description: t('gemini_model_flash_lite_desc') }, ];Note: You'll need to add the corresponding translation keys to your i18n files.
sources/modal/components/WebAlertModal.tsx (2)
37-114: StyleSheet created inside component causes unnecessary re-creation on every render.Move the stylesheet outside the component using unistyles' function mode to access theme dynamically. This improves performance by avoiding repeated style object creation.
♻️ Suggested approach
// Move outside component, using function mode for theme access const stylesheet = StyleSheet.create((theme) => ({ container: { backgroundColor: theme.colors.surface, // ... rest of styles }, // ... other styles })); // Inside component: const { styles } = useStyles(stylesheet);
28-33: User-visible strings should uset(...)for internationalization.Per coding guidelines, 'Cancel' and 'OK' should use the translation function.
♻️ Suggested fix
+import { t } from '@/text'; + const buttons = isConfirm ? [ - { text: config.cancelText || 'Cancel', style: 'cancel' as const }, - { text: config.confirmText || 'OK', style: config.destructive ? 'destructive' as const : 'default' as const } + { text: config.cancelText || t('cancel'), style: 'cancel' as const }, + { text: config.confirmText || t('ok'), style: config.destructive ? 'destructive' as const : 'default' as const } ] : config.buttons || [{ text: 'OK', style: 'default' as const }];sources/modal/ModalManager.ts (1)
98-114: Acceptable type casting for generic-to-concrete bridging.The
as unknown ascasts on lines 109-110 are necessary because the genericPis erased when storing in the non-genericshowModalFn. The approach is type-safe given the constraintP extends CustomModalInjectedProps.However, consider adding a brief comment explaining why the casts are needed:
📝 Suggested comment
const modalConfig: Omit<CustomModalConfig, 'id'> = { type: 'custom', + // Casts required: generic P erased when stored in non-generic modal state component: config.component as unknown as CustomModalConfig['component'], props: config.props as unknown as CustomModalConfig['props'], };sources/sync/typesRaw.ts (1)
421-445: Robust content normalization helper.The
toolResultContentToTextfunction handles the various content formats well:
- Null/undefined → empty string
- String → passthrough
- Claude text block arrays → concatenation
- Other arrays/objects → JSON stringification with fallback
One minor consideration: the function is defined inside
normalizeRawMessage, which means it's recreated on every call. For performance, consider hoisting it outside the function.♻️ Optional: Hoist helper for performance
+const toolResultContentToText = (content: unknown): string => { + if (content === null || content === undefined) return ''; + if (typeof content === 'string') return content; + if (Array.isArray(content)) { + const maybeTextBlocks = content as Array<{ type?: unknown; text?: unknown }>; + const isTextBlocks = maybeTextBlocks.every((b) => b && typeof b === 'object' && b.type === 'text' && typeof b.text === 'string'); + if (isTextBlocks) { + return maybeTextBlocks.map((b) => b.text as string).join(''); + } + try { return JSON.stringify(content); } catch { return String(content); } + } + try { return JSON.stringify(content); } catch { return String(content); } +}; + export function normalizeRawMessage(id: string, localId: string | null, createdAt: number, raw: RawRecord): NormalizedMessage | null { // ... remove inner definition ...sources/sync/typesRaw.spec.ts (1)
1493-1545: Good test coverage for ACP tool result normalization.The tests verify that both
tool-resultandtool-call-resultACP messages correctly normalize text block arrays to plain strings. This aligns with thetoolResultContentToTextimplementation in typesRaw.ts.Consider adding edge case tests for additional coverage:
📝 Suggested additional test cases
it('normalizes ACP tool-result with string output', () => { const raw = { role: 'agent' as const, content: { type: 'acp' as const, provider: 'gemini' as const, data: { type: 'tool-result' as const, callId: 'call_string', output: 'plain string output', id: 'acp-msg-string', }, }, }; const normalized = normalizeRawMessage('msg-3', null, Date.now(), raw); expect(normalized?.role).toBe('agent'); if (normalized && normalized.role === 'agent') { const item = normalized.content[0]; if (item.type === 'tool-result') { expect(item.content).toBe('plain string output'); } } }); it('normalizes ACP tool-result with JSON object output', () => { const raw = { role: 'agent' as const, content: { type: 'acp' as const, provider: 'codex' as const, data: { type: 'tool-result' as const, callId: 'call_json', output: { status: 'success', data: [1, 2, 3] }, id: 'acp-msg-json', }, }, }; const normalized = normalizeRawMessage('msg-4', null, Date.now(), raw); expect(normalized?.role).toBe('agent'); if (normalized && normalized.role === 'agent') { const item = normalized.content[0]; if (item.type === 'tool-result') { expect(item.content).toBe('{"status":"success","data":[1,2,3]}'); } } });sources/components/ProfileEditForm.tsx (2)
57-113: MachinePreviewModal uses inline styles instead of StyleSheet.Per coding guidelines, styling should use
StyleSheet.createfrom 'react-native-unistyles'. TheMachinePreviewModalcomponent uses extensive inline styles which could be extracted to the stylesheet at the bottom of the file.♻️ Suggested refactor to use StyleSheet
function MachinePreviewModal(props: MachinePreviewModalProps) { const { theme } = useUnistyles(); const { height: windowHeight } = useWindowDimensions(); + const styles = stylesheet; // ... existing logic ... const maxHeight = Math.min(720, Math.max(420, Math.floor(windowHeight * 0.85))); return ( - <View style={{ - width: '92%', - maxWidth: 560, - height: maxHeight, - maxHeight, - backgroundColor: theme.colors.groupped.background, - borderRadius: 16, - overflow: 'hidden', - borderWidth: 1, - borderColor: theme.colors.divider, - flexShrink: 1, - }}> + <View style={[styles.modalContainer, { height: maxHeight, maxHeight }]}> {/* ... header and content ... */} </View> ); } // Add to stylesheet: + modalContainer: { + width: '92%', + maxWidth: 560, + backgroundColor: theme.colors.groupped.background, + borderRadius: 16, + overflow: 'hidden', + borderWidth: 1, + borderColor: theme.colors.divider, + flexShrink: 1, + }, + modalHeader: { + paddingHorizontal: 16, + paddingVertical: 12, + flexDirection: 'row', + alignItems: 'center', + justifyContent: 'space-between', + borderBottomWidth: 1, + borderBottomColor: theme.colors.divider, + }, + modalTitle: { + fontSize: 17, + color: theme.colors.text, + ...Typography.default('semiBold'), + },Based on coding guidelines, use StyleSheet.create from 'react-native-unistyles' for all styling.
466-501: Button container uses inline styles - consider extracting to stylesheet.Similar to the modal, the button container and button styles are inline. While functional, extracting to the stylesheet would be more consistent with the codebase patterns.
♻️ Optional: Extract button styles
- <View style={{ paddingHorizontal: Platform.select({ ios: 16, default: 12 }), paddingTop: 12 }}> - <View style={{ flexDirection: 'row', gap: 12 }}> + <View style={styles.buttonContainer}> + <View style={styles.buttonRow}> // Add to stylesheet: + buttonContainer: { + paddingHorizontal: Platform.select({ ios: 16, default: 12 }), + paddingTop: 12, + }, + buttonRow: { + flexDirection: 'row', + gap: 12, + },sources/text/translations/zh-Hans.ts (2)
15-17: Consider removing unused plural helper.Chinese (Simplified) doesn't have grammatical plural forms like English. The
pluralhelper function is defined but appears unused in the translation strings (Chinese typically uses the same form regardless of count). Consider removing this unused function to keep the file clean.♻️ Suggested removal
-/** - * Chinese plural helper function - * `@param` options - Object containing count, singular, and plural forms - * `@returns` The appropriate form based on count - */ -function plural({ count, singular, plural }: { count: number; singular: string; plural: string }): string { - return count === 1 ? singular : plural; -}
509-509: Plural helper used but may not be linguistically appropriate.The
pluralhelper is used here:${plural({ count, singular: '工具', plural: '工具' })}— but both singular and plural are identical ('工具'), confirming Chinese doesn't distinguish. This works but is redundant.sources/components/ItemRowActions.tsx (1)
54-57: Consider using proper event types instead ofany.The
(e: any)type annotation bypasses TypeScript's type safety. While the optional chaining onstopPropagationhandles cross-platform scenarios, you could use a more specific type.Suggested improvement
- onPress={(e: any) => { - e?.stopPropagation?.(); + onPress={(e) => { + (e as any)?.stopPropagation?.(); action.onPress(); }}Alternatively, import
GestureResponderEventfromreact-nativeand use that type, though the optional chaining pattern suggests cross-platform event differences that may warrant the current approach.Also applies to: 71-74
sources/components/SearchHeader.tsx (1)
105-113: Consider improving touch target for the clear button.The clear icon uses
Ionicons'sonPressdirectly. While functional, wrapping it in aPressablewithhitSlopwould provide a larger, more accessible touch target, consistent with how other action icons are handled inItemRowActions.tsx(lines 50-64).Suggested improvement
{value.length > 0 && ( - <Ionicons - name="close-circle" - size={20} - color={theme.colors.textSecondary} - onPress={() => onChangeText('')} - style={styles.clearIcon} - /> + <Pressable + onPress={() => onChangeText('')} + hitSlop={{ top: 10, bottom: 10, left: 10, right: 10 }} + style={styles.clearIcon} + > + <Ionicons + name="close-circle" + size={20} + color={theme.colors.textSecondary} + /> + </Pressable> )}This would require adding
Pressableto the imports fromreact-native.sources/app/(app)/new/pick/path.tsx (1)
91-94: Consider adding proper type for session instead of casting toany.The
as anycast on Line 93 loses type safety. IfSessiontype is available from storage types, use it directly or create a type guard.Suggested improvement
- const session = item as any; - if (session.metadata?.machineId === params.machineId && session.metadata?.path) { + if (typeof item === 'object' && 'metadata' in item) { + const session = item as Session; + if (session.metadata?.machineId === params.machineId && session.metadata?.path) {sources/components/EnvironmentVariablesList.tsx (1)
180-181: UsingenvVar.nameas key may cause issues if names can be edited.If users can rename environment variables, using
nameas the React key could cause unexpected behavior. Consider using a stable identifier or index-based key with proper handling.Consider using index or stable ID
- <EnvironmentVariableCard - key={envVar.name} + <EnvironmentVariableCard + key={`${index}-${envVar.name}`}sources/app/(app)/new/pick/profile-edit.tsx (1)
157-161: Consider wrapping router.replace in try-catch for navigation safety.If
router.replacethrows (e.g., invalid route), the user would be stuck. While unlikely, defensive error handling could improve robustness.sources/app/(app)/settings/profiles.tsx (1)
233-276: Consider extracting a reusable profile row renderer to reduce duplication.The three profile group sections (favorites, custom, built-in) share nearly identical rendering logic with only minor differences (e.g., whether
onDeleteis passed to actions). A sharedrenderProfileItemhelper would reduce ~120 lines of duplication.♻️ Example extraction
const renderProfileItem = React.useCallback(( profile: AIBackendProfile, options: { showDelete?: boolean } ) => { const isSelected = selectedProfileId === profile.id; const isFavorite = favoriteProfileIdSet.has(profile.id); const actions = buildProfileActions({ profile, isFavorite, favoriteActionColor: selectedIndicatorColor, nonFavoriteActionColor: theme.colors.textSecondary, onToggleFavorite: () => toggleFavoriteProfile(profile.id), onEdit: () => handleEditProfile(profile), onDuplicate: () => handleDuplicateProfile(profile), ...(options.showDelete && { onDelete: () => { void handleDeleteProfile(profile); } }), }); return ( <Item key={profile.id} title={profile.name} subtitle={getProfileBackendSubtitle(profile)} leftElement={<ProfileCompatibilityIcon profile={profile} />} onPress={() => handleSelectProfile(profile.id)} showChevron={false} selected={isSelected} rightElement={/* ... */} /> ); }, [/* deps */]);Also applies to: 281-324, 328-370
sources/components/SearchableListSelector.tsx (1)
363-364: Consider making the fallback title generation more robust.The fallback
config.allSectionTitle ?? config.recentSectionTitle.replace('Recent ', 'All ')assumes the recent section title contains "Recent " which may not hold for all locales.♻️ Suggested improvement
- <ItemGroup title={config.allSectionTitle ?? config.recentSectionTitle.replace('Recent ', 'All ')}> + <ItemGroup title={config.allSectionTitle ?? t('common.allItems')}>Alternatively, make
allSectionTitlerequired in the config whenshowAllis true.sources/components/EnvironmentVariableCard.tsx (1)
71-96: Consider moving platform-specific styles to StyleSheet.create.Per coding guidelines, styles should use
StyleSheet.createfrom 'react-native-unistyles'. TheuseMemoapproach works but doesn't leverage unistyles' optimization.sources/components/newSession/PathSelector.tsx (1)
228-296: Move repeated inline styles into the Unistyles stylesheet.Right-element containers and icon wrappers are styled inline; this conflicts with the “StyleSheet.create for all styling” guideline and recreates objects each render. Consider hoisting these styles into the stylesheet (and apply the same approach to other inline style objects in this file).
♻️ Example refactor
-const stylesheet = StyleSheet.create((theme) => ({ +const ITEM_RIGHT_GAP = 16; + +const stylesheet = StyleSheet.create((theme) => ({ pathInputContainer: { flexDirection: 'row', alignItems: 'center', gap: 8, paddingHorizontal: 16, paddingVertical: 16, }, + itemRightContainer: { + flexDirection: 'row', + alignItems: 'center', + gap: ITEM_RIGHT_GAP, + }, + itemRightIconWrapper: { + width: 24, + alignItems: 'center', + justifyContent: 'center', + }, pathInput: { flex: 1, backgroundColor: theme.colors.input.background, borderRadius: 10, paddingHorizontal: 12, minHeight: 36, position: 'relative', borderWidth: 0.5, borderColor: theme.colors.divider, }, })); - -const ITEM_RIGHT_GAP = 16;- <View style={{ flexDirection: 'row', alignItems: 'center', gap: ITEM_RIGHT_GAP }}> - <View style={{ width: 24, alignItems: 'center', justifyContent: 'center' }}> + <View style={styles.itemRightContainer}> + <View style={styles.itemRightIconWrapper}> ... - <View style={{ flexDirection: 'row', alignItems: 'center', gap: ITEM_RIGHT_GAP }}> - <View style={{ width: 24, alignItems: 'center', justifyContent: 'center' }}> + <View style={styles.itemRightContainer}> + <View style={styles.itemRightIconWrapper}>As per coding guidelines, please keep styling in
StyleSheet.create(including other inline style objects in this file).
| React.useEffect(() => { | ||
| const newValue = useRemoteVariable && remoteVariableName.trim() !== '' | ||
| ? `\${${remoteVariableName}${defaultValue ? `:-${defaultValue}` : ''}}` | ||
| ? formatEnvVarTemplate({ sourceVar: remoteVariableName, fallback: defaultValue, operator: fallbackOperator }) | ||
| : defaultValue; | ||
|
|
||
| if (newValue !== variable.value) { | ||
| onUpdate(newValue); | ||
| } | ||
| }, [useRemoteVariable, remoteVariableName, defaultValue, variable.value, onUpdate]); | ||
| }, [useRemoteVariable, remoteVariableName, defaultValue, fallbackOperator, variable.value, onUpdate]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of EnvironmentVariableCard component
rg -n 'EnvironmentVariableCard' --type tsx --type ts -B2 -A5Repository: leeroybrun/happy
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Find all usages of EnvironmentVariableCard component without file type restrictions
rg -n 'EnvironmentVariableCard' -B2 -A5Repository: leeroybrun/happy
Length of output: 3551
🏁 Script executed:
#!/bin/bash
# Read the EnvironmentVariablesList around the usage of EnvironmentVariableCard
sed -n '170,200p' sources/components/EnvironmentVariablesList.tsxRepository: leeroybrun/happy
Length of output: 1704
🏁 Script executed:
#!/bin/bash
# Check the full component definition and handleUpdateVariable
sed -n '1,50p' sources/components/EnvironmentVariablesList.tsxRepository: leeroybrun/happy
Length of output: 2135
🏁 Script executed:
#!/bin/bash
# Find handleUpdateVariable and other handlers in EnvironmentVariablesList
rg -n 'handleUpdateVariable|handleDeleteVariable|handleDuplicateVariable' sources/components/EnvironmentVariablesList.tsx -B2 -A3Repository: leeroybrun/happy
Length of output: 1273
The effect dependency array is complete but includes a callback that changes on every render.
While the dependency array correctly includes onUpdate, it's defined as an inline arrow function on each parent render (onUpdate={(newValue) => handleUpdateVariable(index, newValue)}), causing the effect to run unnecessarily. Wrap the callback in useCallback in EnvironmentVariablesList to memoize it, or pass handleUpdateVariable directly with a curried approach to prevent re-running this effect on every parent render.
🤖 Prompt for AI Agents
In `@sources/components/EnvironmentVariableCard.tsx` around lines 112 - 120, The
effect in EnvironmentVariableCard uses onUpdate which is currently passed as an
inline arrow from the parent and thus changes every render; to fix, memoize the
callback in the parent (EnvironmentVariablesList) with useCallback (e.g. memoize
the function that calls handleUpdateVariable) or change the parent to pass
handleUpdateVariable directly via a curried function so the reference is stable;
ensure the prop name onUpdate kept on EnvironmentVariableCard receives a stable
function so the useEffect in EnvironmentVariableCard (which references
useRemoteVariable, remoteVariableName, defaultValue, fallbackOperator,
variable.value and onUpdate) no longer retriggers unnecessarily.
| <View | ||
| style={[ | ||
| styles.thumb, | ||
| { | ||
| backgroundColor: theme.colors.switch.thumb.active, | ||
| transform: [{ translateX }], | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a switch.thumb.inactive color defined in the theme
rg -n "switch.*thumb" --type tsRepository: leeroybrun/happy
Length of output: 375
🏁 Script executed:
# Find and examine the Switch.web.tsx file
fd "Switch.web.tsx" --exec cat -n {}Repository: leeroybrun/happy
Length of output: 2360
🏁 Script executed:
# Also search for theme color definitions related to switch
rg -n "switch.*thumb" --type tsxRepository: leeroybrun/happy
Length of output: 87
🏁 Script executed:
# Search for theme definitions related to switch colors
rg -n "switch" -A 5 -B 5 --type ts | head -100Repository: leeroybrun/happy
Length of output: 5806
🏁 Script executed:
# Find theme configuration files
fd "theme" -o fd "*theme*" | grep -E "\.(ts|tsx|js|json)$"Repository: leeroybrun/happy
Length of output: 183
🏁 Script executed:
# Search for where switch theme colors are defined
rg -n "thumb.*inactive" --type ts --type tsxRepository: leeroybrun/happy
Length of output: 87
🏁 Script executed:
# Get the complete switch.thumb definitions from theme.ts
sed -n '75,85p' sources/theme.ts && echo "---DARK THEME---" && sed -n '280,290p' sources/theme.tsRepository: leeroybrun/happy
Length of output: 693
🏁 Script executed:
# Search more broadly for all switch thumb color definitions
rg -n "thumb:" -A 2 sources/theme.tsRepository: leeroybrun/happy
Length of output: 269
Use conditional color for thumb based on switch state.
Line 54 should use value ? theme.colors.switch.thumb.active : theme.colors.switch.thumb.inactive, matching the pattern used for track color on line 46. The theme defines both switch.thumb.active (#FFFFFF) and switch.thumb.inactive (#767577), but the component always uses the active color regardless of state.
Current code
<View
style={[
styles.thumb,
{
backgroundColor: theme.colors.switch.thumb.active,
transform: [{ translateX }],
},
]}
/>
🤖 Prompt for AI Agents
In `@sources/components/Switch.web.tsx` around lines 50 - 58, The thumb View
always uses the active color; update the style in the Switch component so
backgroundColor uses the switch state (value) — i.e., set backgroundColor to
value ? theme.colors.switch.thumb.active : theme.colors.switch.thumb.inactive
(keeping styles.thumb and transform: [{ translateX }] intact) so thumb color
matches the track logic.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sources/modal/components/WebAlertModal.tsx (1)
112-220: Verify 3‑button ordering in thetwoPlusOnelayout.For 3 buttons, the UI renders indices
0and2on the top row and1on the bottom, which reorders the semantic order fromconfig.buttons. If any callers rely on positional meaning, this could swap actions. Consider documenting the expected order or selecting the primary button explicitly before layout.sources/components/AgentInput.tsx (1)
498-557: AddisCodex/isGeminitohandleKeyPressdependencies.The callback uses these flags to build the mode order, but they’re not in the dependency list—if the flavor changes, the handler can stay stale.
🐛 Proposed fix
- }, [suggestions, moveUp, moveDown, selected, handleSuggestionSelect, props.showAbortButton, props.onAbort, isAborting, handleAbortPress, agentInputEnterToSend, props.value, props.onSend, props.permissionMode, props.onPermissionModeChange]); + }, [suggestions, moveUp, moveDown, selected, handleSuggestionSelect, props.showAbortButton, props.onAbort, isAborting, handleAbortPress, agentInputEnterToSend, props.value, props.onSend, props.permissionMode, props.onPermissionModeChange, isCodex, isGemini]);
🤖 Fix all issues with AI agents
In `@sources/app/`(app)/new/pick/profile.tsx:
- Around line 330-336: The built-in row currently calls
setProfileParamAndClose(profile.id) directly which bypasses the
ignoreNextRowPress guard; update the Item's onPress to call
handleProfileRowPress so the suppression logic runs (i.e., route the built-in
profile row press through handleProfileRowPress and pass the profile id and the
existing setter callback such that handleProfileRowPress invokes
setProfileParamAndClose(profile.id) when appropriate). Ensure you replace the
direct call in the Item onPress with a call to handleProfileRowPress for
consistency with other rows.
In `@sources/components/EnvironmentVariablesList.tsx`:
- Around line 42-57: The resolvedEnvVarRefs logic currently only filters by
SECRET_NAME_REGEX; update the useMemo that computes resolvedEnvVarRefs to also
skip variables documented as secrets (e.g., check profileDocs for an entry
matching the env var name or the extracted ref and if that entry has isSecret
=== true then return/skip). In other words, inside the forEach over
environmentVariables in resolvedEnvVarRefs, after extracting ref via
extractVarNameFromValue and before refs.add(ref), add a guard that looks up the
corresponding documented variable in profileDocs (by envVar.name or the
extracted ref) and skips adding when that documented entry has isSecret set.
Keep the existing SECRET_NAME_REGEX checks in place.
In `@sources/sync/sync.ts`:
- Around line 1506-1539: When updating session thinking state in the
applySessions call, also update the thinkingAt timestamp so it reflects the
lifecycle event time; inside the object passed to applySessions (near symbols
session, updateData, isTaskComplete, isTaskStarted, applySessions) add a
thinkingAt: updateData.createdAt when you set thinking to true or false (i.e.,
when isTaskStarted or isTaskComplete is true) so the timeline remains consistent
if activity updates are dropped.
♻️ Duplicate comments (2)
sources/app/(app)/new/pick/profile.tsx (1)
163-168: Extract remaining inline styles into the Unistyles stylesheet.The checkmark opacity style and
ItemListpadding are still inline. Please move them into the existing stylesheet. As per coding guidelines, use Unistyles for all styling.Also applies to: 237-237
sources/components/EnvironmentVariableCard.tsx (1)
100-105: Sync local state whenvariable.valuechanges externally.
useRemoteVariable,remoteVariableName, anddefaultValueare only initialized once. Ifvariable.valueis updated by the parent (profile load/reset), the card can display stale values.🧩 Possible fix
const parsed = parseVariableValue(variable.value); const [useRemoteVariable, setUseRemoteVariable] = React.useState(parsed.useRemoteVariable); const [remoteVariableName, setRemoteVariableName] = React.useState(parsed.remoteVariableName); const [defaultValue, setDefaultValue] = React.useState(parsed.defaultValue); const fallbackOperator = parsed.fallbackOperator; +React.useEffect(() => { + const next = parseVariableValue(variable.value); + if (next.useRemoteVariable !== useRemoteVariable) setUseRemoteVariable(next.useRemoteVariable); + if (next.remoteVariableName !== remoteVariableName) setRemoteVariableName(next.remoteVariableName); + if (next.defaultValue !== defaultValue) setDefaultValue(next.defaultValue); +}, [variable.value]);
🧹 Nitpick comments (7)
sources/utils/promptUnsavedChangesAlert.test.ts (2)
1-3: Use the@/path alias for local imports.*Line 3 should use the configured alias to stay consistent with project conventions.
♻️ Suggested change
-import { promptUnsavedChangesAlert } from './promptUnsavedChangesAlert'; +import { promptUnsavedChangesAlert } from '@/utils/promptUnsavedChangesAlert';
6-24: Reduce duplication and assert button presence before invoking onPress.Optional but helpful: extract a tiny harness to avoid repeating the same setup and to fail fast if the expected button is missing (instead of a timeout).
♻️ Suggested refactor
describe('promptUnsavedChangesAlert', () => { + const setupAlert = () => { + let lastButtons: AlertButton[] | undefined; + + const alert = (_title: string, _message?: string, buttons?: AlertButton[]) => { + lastButtons = buttons; + }; + + const promise = promptUnsavedChangesAlert(alert, { + title: 'Discard changes', + message: 'You have unsaved changes.', + discardText: 'Discard', + saveText: 'Save', + keepEditingText: 'Keep editing', + }); + + const press = (text: string) => { + const button = lastButtons?.find((b) => b.text === text); + expect(button).toBeTruthy(); + button?.onPress?.(); + }; + + return { promise, press }; + }; + it('resolves to save when the Save button is pressed', async () => { - let lastButtons: AlertButton[] | undefined; - - const alert = (_title: string, _message?: string, buttons?: AlertButton[]) => { - lastButtons = buttons; - }; - - const promise = promptUnsavedChangesAlert(alert, { - title: 'Discard changes', - message: 'You have unsaved changes.', - discardText: 'Discard', - saveText: 'Save', - keepEditingText: 'Keep editing', - }); - - lastButtons?.find((b) => b.text === 'Save')?.onPress?.(); + const { promise, press } = setupAlert(); + press('Save'); await expect(promise).resolves.toBe('save'); }); it('resolves to discard when the Discard button is pressed', async () => { - let lastButtons: AlertButton[] | undefined; - - const alert = (_title: string, _message?: string, buttons?: AlertButton[]) => { - lastButtons = buttons; - }; - - const promise = promptUnsavedChangesAlert(alert, { - title: 'Discard changes', - message: 'You have unsaved changes.', - discardText: 'Discard', - saveText: 'Save', - keepEditingText: 'Keep editing', - }); - - lastButtons?.find((b) => b.text === 'Discard')?.onPress?.(); + const { promise, press } = setupAlert(); + press('Discard'); await expect(promise).resolves.toBe('discard'); }); it('resolves to keepEditing when the Keep editing button is pressed', async () => { - let lastButtons: AlertButton[] | undefined; - - const alert = (_title: string, _message?: string, buttons?: AlertButton[]) => { - lastButtons = buttons; - }; - - const promise = promptUnsavedChangesAlert(alert, { - title: 'Discard changes', - message: 'You have unsaved changes.', - discardText: 'Discard', - saveText: 'Save', - keepEditingText: 'Keep editing', - }); - - lastButtons?.find((b) => b.text === 'Keep editing')?.onPress?.(); + const { promise, press } = setupAlert(); + press('Keep editing'); await expect(promise).resolves.toBe('keepEditing'); }); });Also applies to: 26-44, 46-64
sources/components/newSession/MachineSelector.tsx (1)
38-42: Default prop values witht()calls are evaluated at module load time.These default values are evaluated once when the module is loaded, not on each render. If the app supports dynamic language switching, users won't see updated translations until the component remounts.
Consider moving these defaults inside the component body or using nullish coalescing:
♻️ Suggested approach
export function MachineSelector({ machines, selectedMachine, recentMachines = [], favoriteMachines = [], onSelect, onToggleFavorite, showFavorites = true, showRecent = true, showSearch = true, searchPlacement = 'header', - searchPlaceholder = t('newSession.machinePicker.searchPlaceholder'), - recentSectionTitle = t('newSession.machinePicker.recentTitle'), - favoritesSectionTitle = t('newSession.machinePicker.favoritesTitle'), - allSectionTitle = t('newSession.machinePicker.allTitle'), - noItemsMessage = t('newSession.machinePicker.emptyMessage'), + searchPlaceholder, + recentSectionTitle, + favoritesSectionTitle, + allSectionTitle, + noItemsMessage, }: MachineSelectorProps) { const { theme } = useUnistyles(); + + const resolvedSearchPlaceholder = searchPlaceholder ?? t('newSession.machinePicker.searchPlaceholder'); + const resolvedRecentSectionTitle = recentSectionTitle ?? t('newSession.machinePicker.recentTitle'); + const resolvedFavoritesSectionTitle = favoritesSectionTitle ?? t('newSession.machinePicker.favoritesTitle'); + const resolvedAllSectionTitle = allSectionTitle ?? t('newSession.machinePicker.allTitle'); + const resolvedNoItemsMessage = noItemsMessage ?? t('newSession.machinePicker.emptyMessage');sources/sync/modelOptions.ts (1)
12-33: Consider memoizing or extracting Gemini options if called frequently in render paths.The function creates a new array with new objects on each call. If
getModelOptionsForAgentType('gemini')is called during renders, this could cause unnecessary re-renders in consuming components that depend on referential equality.If this becomes a performance concern, consider memoizing or caching the result:
♻️ Optional optimization
// Cache outside the function to maintain referential stability let cachedGeminiOptions: readonly ModelOption[] | null = null; export function getModelOptionsForAgentType(agentType: AgentType): readonly ModelOption[] { if (agentType === 'gemini') { if (!cachedGeminiOptions) { cachedGeminiOptions = [ // ... options ]; } return cachedGeminiOptions; } return []; }sources/components/EnvironmentVariablesList.tsx (1)
64-75: Move inline styles into a Unistyles stylesheet.The new layout blocks (header, list container, add form, inputs, and button) and
webNoOutlineare still inline. Please extract them intoStyleSheet.create(() => ({}))(function mode where theme/runtime is needed) and keep the stylesheet at the end of the file. As per coding guidelines, use Unistyles for all styling.Also applies to: 151-210, 232-288
sources/components/EnvironmentVariableCard.tsx (1)
73-98: PreferStyleSheet.create(function mode) for new styles.
The newuseMemostyle objects/inline blocks should move into Unistyles to match project rules.♻️ Example refactor direction
-import { useUnistyles } from 'react-native-unistyles'; +import { useUnistyles, StyleSheet } from 'react-native-unistyles'; -const webNoOutline = React.useMemo(() => (Platform.select({ - web: { /* ... */ }, - default: {}, -}) as object), []); - -const secondaryTextStyle = React.useMemo(() => ({ - fontSize: Platform.select({ ios: 15, default: 14 }), - lineHeight: 20, - letterSpacing: Platform.select({ ios: -0.24, default: 0.1 }), - ...Typography.default(), -}), []); +const styles = StyleSheet.create(() => ({ + webNoOutline: Platform.select({ + web: { /* ... */ }, + default: {}, + }) as object, + secondaryText: { + fontSize: Platform.select({ ios: 15, default: 14 }), + lineHeight: 20, + letterSpacing: Platform.select({ ios: -0.24, default: 0.1 }), + ...Typography.default(), + }, +}));As per coding guidelines, use
StyleSheet.createfor styling and function mode when runtime/platform access is needed.sources/text/translations/ja.ts (1)
16-18: Unusedpluralhelper function.This function is defined but never called in the file. While Japanese doesn't require grammatical plural forms, consider removing this function to reduce dead code, or add a comment explaining it's kept for consistency with other translation files.
| // Check for task lifecycle events to update thinking state | ||
| // This ensures UI updates even if volatile activity updates are lost | ||
| const rawContent = decrypted.content as { role?: string; content?: { type?: string; data?: { type?: string } } } | null; | ||
| const contentType = rawContent?.content?.type; | ||
| const dataType = rawContent?.content?.data?.type; | ||
|
|
||
| // Debug logging to trace lifecycle events | ||
| const isDev = typeof __DEV__ !== 'undefined' && __DEV__; | ||
| if (isDev && (dataType === 'task_complete' || dataType === 'turn_aborted' || dataType === 'task_started')) { | ||
| console.log(`🔄 [Sync] Lifecycle event detected: contentType=${contentType}, dataType=${dataType}`); | ||
| } | ||
|
|
||
| const isTaskComplete = | ||
| ((contentType === 'acp' || contentType === 'codex') && | ||
| (dataType === 'task_complete' || dataType === 'turn_aborted')); | ||
|
|
||
| const isTaskStarted = | ||
| ((contentType === 'acp' || contentType === 'codex') && dataType === 'task_started'); | ||
|
|
||
| if (isDev && (isTaskComplete || isTaskStarted)) { | ||
| console.log(`🔄 [Sync] Updating thinking state: isTaskComplete=${isTaskComplete}, isTaskStarted=${isTaskStarted}`); | ||
| } | ||
|
|
||
| // Update session | ||
| const session = storage.getState().sessions[updateData.body.sid]; | ||
| if (session) { | ||
| this.applySessions([{ | ||
| ...session, | ||
| updatedAt: updateData.createdAt, | ||
| seq: updateData.seq | ||
| seq: updateData.seq, | ||
| // Update thinking state based on task lifecycle events | ||
| ...(isTaskComplete ? { thinking: false } : {}), | ||
| ...(isTaskStarted ? { thinking: true } : {}) | ||
| }]) |
There was a problem hiding this comment.
Update thinkingAt when lifecycle events toggle thinking.
If activity updates are dropped, thinkingAt can remain stale even though thinking is flipped here. Setting it to updateData.createdAt keeps the timeline consistent.
🐛 Proposed fix
- ...(isTaskComplete ? { thinking: false } : {}),
- ...(isTaskStarted ? { thinking: true } : {})
+ ...(isTaskComplete ? { thinking: false, thinkingAt: updateData.createdAt } : {}),
+ ...(isTaskStarted ? { thinking: true, thinkingAt: updateData.createdAt } : {})📝 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.
| // Check for task lifecycle events to update thinking state | |
| // This ensures UI updates even if volatile activity updates are lost | |
| const rawContent = decrypted.content as { role?: string; content?: { type?: string; data?: { type?: string } } } | null; | |
| const contentType = rawContent?.content?.type; | |
| const dataType = rawContent?.content?.data?.type; | |
| // Debug logging to trace lifecycle events | |
| const isDev = typeof __DEV__ !== 'undefined' && __DEV__; | |
| if (isDev && (dataType === 'task_complete' || dataType === 'turn_aborted' || dataType === 'task_started')) { | |
| console.log(`🔄 [Sync] Lifecycle event detected: contentType=${contentType}, dataType=${dataType}`); | |
| } | |
| const isTaskComplete = | |
| ((contentType === 'acp' || contentType === 'codex') && | |
| (dataType === 'task_complete' || dataType === 'turn_aborted')); | |
| const isTaskStarted = | |
| ((contentType === 'acp' || contentType === 'codex') && dataType === 'task_started'); | |
| if (isDev && (isTaskComplete || isTaskStarted)) { | |
| console.log(`🔄 [Sync] Updating thinking state: isTaskComplete=${isTaskComplete}, isTaskStarted=${isTaskStarted}`); | |
| } | |
| // Update session | |
| const session = storage.getState().sessions[updateData.body.sid]; | |
| if (session) { | |
| this.applySessions([{ | |
| ...session, | |
| updatedAt: updateData.createdAt, | |
| seq: updateData.seq | |
| seq: updateData.seq, | |
| // Update thinking state based on task lifecycle events | |
| ...(isTaskComplete ? { thinking: false } : {}), | |
| ...(isTaskStarted ? { thinking: true } : {}) | |
| }]) | |
| // Check for task lifecycle events to update thinking state | |
| // This ensures UI updates even if volatile activity updates are lost | |
| const rawContent = decrypted.content as { role?: string; content?: { type?: string; data?: { type?: string } } } | null; | |
| const contentType = rawContent?.content?.type; | |
| const dataType = rawContent?.content?.data?.type; | |
| // Debug logging to trace lifecycle events | |
| const isDev = typeof __DEV__ !== 'undefined' && __DEV__; | |
| if (isDev && (dataType === 'task_complete' || dataType === 'turn_aborted' || dataType === 'task_started')) { | |
| console.log(`🔄 [Sync] Lifecycle event detected: contentType=${contentType}, dataType=${dataType}`); | |
| } | |
| const isTaskComplete = | |
| ((contentType === 'acp' || contentType === 'codex') && | |
| (dataType === 'task_complete' || dataType === 'turn_aborted')); | |
| const isTaskStarted = | |
| ((contentType === 'acp' || contentType === 'codex') && dataType === 'task_started'); | |
| if (isDev && (isTaskComplete || isTaskStarted)) { | |
| console.log(`🔄 [Sync] Updating thinking state: isTaskComplete=${isTaskComplete}, isTaskStarted=${isTaskStarted}`); | |
| } | |
| // Update session | |
| const session = storage.getState().sessions[updateData.body.sid]; | |
| if (session) { | |
| this.applySessions([{ | |
| ...session, | |
| updatedAt: updateData.createdAt, | |
| seq: updateData.seq, | |
| // Update thinking state based on task lifecycle events | |
| ...(isTaskComplete ? { thinking: false, thinkingAt: updateData.createdAt } : {}), | |
| ...(isTaskStarted ? { thinking: true, thinkingAt: updateData.createdAt } : {}) | |
| }]) |
🤖 Prompt for AI Agents
In `@sources/sync/sync.ts` around lines 1506 - 1539, When updating session
thinking state in the applySessions call, also update the thinkingAt timestamp
so it reflects the lifecycle event time; inside the object passed to
applySessions (near symbols session, updateData, isTaskComplete, isTaskStarted,
applySessions) add a thinkingAt: updateData.createdAt when you set thinking to
true or false (i.e., when isTaskStarted or isTaskComplete is true) so the
timeline remains consistent if activity updates are dropped.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sources/components/EnvironmentVariableCard.tsx`:
- Around line 93-118: The effect and preview logic use fallbackOperator directly
which can be null and drop the fallback even when defaultValue is non-empty;
derive a single computed operator once (e.g., compute a local operator from
fallbackOperator and defaultValue) and reuse it in React.useEffect,
computedTemplateValue, and resolvedSessionValue where formatEnvVarTemplate is
called (references: useRemoteVariable, remoteVariableName, defaultValue,
fallbackOperator, formatEnvVarTemplate, computedTemplateValue,
resolvedSessionValue, React.useEffect) so that when a user enters a fallback the
template always includes the correct operator/fallback behavior instead of
silently producing ${VAR} without the fallback.
In `@sources/components/EnvironmentVariablesList.tsx`:
- Around line 158-165: The current logic uses getDocumentation(varNameFromValue
|| envVar.name) which ignores documentation attached to envVar.name when the
value references another var; update to fetch both docs: call
getDocumentation(envVar.name) (primaryDocs) and, if varNameFromValue is present,
getDocumentation(varNameFromValue) (refDocs), then compute isSecret as true if
primaryDocs.isSecret || refDocs?.isSecret || SECRET_NAME_REGEX.test(envVar.name)
|| SECRET_NAME_REGEX.test(varNameFromValue || ''); for display/metadata prefer
primaryDocs but include refDocs where needed (e.g., show reference info or
combine flags) so documented secrets on the variable name are preserved even
when the value is a reference.
🧹 Nitpick comments (6)
sources/types/react-test-renderer.d.ts (1)
1-1: Bare module declaration bypasses type checking; note thatreact-test-rendereris deprecated in React 19.This ambient declaration provides no type information, making all imports from
react-test-rendererresolve toany. Per the coding guidelines, code should be properly typed.Additionally, based on the provided React 19 documentation,
react-test-rendereris deprecated—the recommended migration path is to use@testing-library/reactor@testing-library/react-native.Consider one of the following:
- Install
@types/react-test-rendererfor proper typings if you need to keep using this package temporarily.- Migrate test code to React Testing Library and remove this declaration.
sources/app/(app)/new/pick/profile.tsx (3)
3-3: Remove unused import.
Pressableis imported but never used in this component.-import { Pressable, View } from 'react-native'; +import { View } from 'react-native';
23-23: Remove unused variable.
selectedIndicatorColoris defined but never used. The checkmark icons referencetheme.colors.textdirectly.- const selectedIndicatorColor = rt.themeName === 'dark' ? theme.colors.text : theme.colors.button.primary.background;
147-190: Inconsistent indentation in callback.The indentation within
renderProfileRowRightElementis inconsistent—some lines appear to use tabs while others use spaces. Per coding guidelines, use 4 spaces for indentation consistently.sources/components/AgentInput.tsx (1)
689-755: Extract new inline styles intoStyleSheet.create.New UI blocks (model options + action chip rows) introduce multiple inline style objects. Per repo guidance, extract the static styles into
StyleSheet.createand keep only pressed-state overrides inline. As per coding guidelines, all styling should come from Unistyles.♻️ Example refactor (model options)
const stylesheet = StyleSheet.create((theme, runtime) => ({ + modelOptionItem: { + flexDirection: 'row', + alignItems: 'center', + paddingHorizontal: 16, + paddingVertical: 8, + }, + modelOptionItemPressed: { + backgroundColor: theme.colors.surfacePressed, + }, + modelOptionLabel: { + fontSize: 14, + ...Typography.default(), + }, + modelOptionDescription: { + fontSize: 11, + color: theme.colors.textSecondary, + ...Typography.default(), + }, }));- <Pressable - style={({ pressed }) => ({ - flexDirection: 'row', - alignItems: 'center', - paddingHorizontal: 16, - paddingVertical: 8, - backgroundColor: pressed ? theme.colors.surfacePressed : 'transparent' - })} - > + <Pressable + style={({ pressed }) => [ + styles.modelOptionItem, + pressed && styles.modelOptionItemPressed + ]} + > ... - <Text style={{ - fontSize: 14, - color: isSelected ? theme.colors.radio.active : theme.colors.text, - ...Typography.default() - }}> + <Text style={[ + styles.modelOptionLabel, + { color: isSelected ? theme.colors.radio.active : theme.colors.text } + ]}> ... - <Text style={{ - fontSize: 11, - color: theme.colors.textSecondary, - ...Typography.default() - }}> + <Text style={styles.modelOptionDescription}>Also applies to: 840-1195
sources/components/EnvironmentVariableCard.test.ts (1)
1-2: React-test-renderer is deprecated in React 19; consider@testing-library/react-nativefor new tests.The repo is on React 19.1.0 where react-test-renderer is officially deprecated. This affects multiple test files in the codebase (EnvironmentVariablesList.test.ts also uses it). When refactoring test coverage, migrate to
@testing-library/react-nativefor consistency and forward compatibility.
| // Update parent when local state changes | ||
| React.useEffect(() => { | ||
| const newValue = useRemoteVariable && remoteVariableName.trim() !== '' | ||
| ? `\${${remoteVariableName}${defaultValue ? `:-${defaultValue}` : ''}}` | ||
| ? formatEnvVarTemplate({ sourceVar: remoteVariableName, fallback: defaultValue, operator: fallbackOperator }) | ||
| : defaultValue; | ||
|
|
||
| if (newValue !== variable.value) { | ||
| onUpdate(newValue); | ||
| onUpdate(index, newValue); | ||
| } | ||
| }, [useRemoteVariable, remoteVariableName, defaultValue, variable.value, onUpdate]); | ||
| }, [defaultValue, fallbackOperator, index, onUpdate, remoteVariableName, useRemoteVariable, variable.value]); | ||
|
|
||
| // Determine status | ||
| const showRemoteDiffersWarning = remoteValue !== null && expectedValue && remoteValue !== expectedValue; | ||
| const showDefaultOverrideWarning = expectedValue && defaultValue !== expectedValue; | ||
|
|
||
| const computedTemplateValue = | ||
| useRemoteVariable && remoteVariableName.trim() !== '' | ||
| ? formatEnvVarTemplate({ sourceVar: remoteVariableName, fallback: defaultValue, operator: fallbackOperator }) | ||
| : defaultValue; | ||
|
|
||
| const resolvedSessionValue = | ||
| isSecret | ||
| ? (useRemoteVariable && remoteVariableName | ||
| ? t('profiles.environmentVariables.preview.secretValueHidden', { | ||
| value: formatEnvVarTemplate({ sourceVar: remoteVariableName, fallback: defaultValue !== '' ? '***' : '', operator: fallbackOperator }), | ||
| }) |
There was a problem hiding this comment.
Ensure fallback templates include defaults when users enter a fallback.
When fallbackOperator is null (e.g., toggling from a literal value) but defaultValue is non‑empty, you pass operator: null, which forces ${VAR} and drops the fallback while the UI preview still treats defaultValue as a fallback. This can silently misconfigure the session value.
✅ Suggested fix (derive operator once and reuse)
- React.useEffect(() => {
+ const templateOperator =
+ fallbackOperator ?? (defaultValue.trim() !== '' ? ':-' : undefined);
+
+ React.useEffect(() => {
const newValue = useRemoteVariable && remoteVariableName.trim() !== ''
- ? formatEnvVarTemplate({ sourceVar: remoteVariableName, fallback: defaultValue, operator: fallbackOperator })
+ ? formatEnvVarTemplate({ sourceVar: remoteVariableName, fallback: defaultValue, operator: templateOperator })
: defaultValue;
if (newValue !== variable.value) {
onUpdate(index, newValue);
}
- }, [defaultValue, fallbackOperator, index, onUpdate, remoteVariableName, useRemoteVariable, variable.value]);
+ }, [defaultValue, fallbackOperator, index, onUpdate, remoteVariableName, useRemoteVariable, variable.value]);
const computedTemplateValue =
useRemoteVariable && remoteVariableName.trim() !== ''
- ? formatEnvVarTemplate({ sourceVar: remoteVariableName, fallback: defaultValue, operator: fallbackOperator })
+ ? formatEnvVarTemplate({ sourceVar: remoteVariableName, fallback: defaultValue, operator: templateOperator })
: defaultValue;
const resolvedSessionValue =
isSecret
? (useRemoteVariable && remoteVariableName
? t('profiles.environmentVariables.preview.secretValueHidden', {
- value: formatEnvVarTemplate({ sourceVar: remoteVariableName, fallback: defaultValue !== '' ? '***' : '', operator: fallbackOperator }),
+ value: formatEnvVarTemplate({ sourceVar: remoteVariableName, fallback: defaultValue !== '' ? '***' : '', operator: templateOperator }),
})
: (defaultValue ? t('profiles.environmentVariables.preview.hiddenValue') : emptyValue))
: (useRemoteVariable && machineId && remoteValue !== undefined
? (remoteValue === null || remoteValue === '' ? (hasFallback ? defaultValue : emptyValue) : remoteValue)
: (computedTemplateValue || emptyValue));🤖 Prompt for AI Agents
In `@sources/components/EnvironmentVariableCard.tsx` around lines 93 - 118, The
effect and preview logic use fallbackOperator directly which can be null and
drop the fallback even when defaultValue is non-empty; derive a single computed
operator once (e.g., compute a local operator from fallbackOperator and
defaultValue) and reuse it in React.useEffect, computedTemplateValue, and
resolvedSessionValue where formatEnvVarTemplate is called (references:
useRemoteVariable, remoteVariableName, defaultValue, fallbackOperator,
formatEnvVarTemplate, computedTemplateValue, resolvedSessionValue,
React.useEffect) so that when a user enters a fallback the template always
includes the correct operator/fallback behavior instead of silently producing
${VAR} without the fallback.
| {environmentVariables.map((envVar, index) => { | ||
| const varNameFromValue = extractVarNameFromValue(envVar.value); | ||
| const docs = getDocumentation(varNameFromValue || envVar.name); | ||
| const isSecret = | ||
| docs.isSecret || | ||
| SECRET_NAME_REGEX.test(envVar.name) || | ||
| SECRET_NAME_REGEX.test(varNameFromValue || ''); | ||
|
|
There was a problem hiding this comment.
Documented secrets can be lost when the value references another var.
docs is derived from varNameFromValue || envVar.name, so a documented secret on envVar.name is ignored when the value is ${OTHER}. That drops secret handling in the card (lock icon, masked input, preview).
✅ Suggested fix (use envVar.name docs + optional ref docs)
- const docs = getDocumentation(varNameFromValue || envVar.name);
- const isSecret =
- docs.isSecret ||
- SECRET_NAME_REGEX.test(envVar.name) ||
- SECRET_NAME_REGEX.test(varNameFromValue || '');
+ const docs = getDocumentation(envVar.name);
+ const refDocs = varNameFromValue ? getDocumentation(varNameFromValue) : undefined;
+ const isSecret =
+ docs.isSecret ||
+ refDocs?.isSecret ||
+ SECRET_NAME_REGEX.test(envVar.name) ||
+ SECRET_NAME_REGEX.test(varNameFromValue || '');Also applies to: 167-177
🤖 Prompt for AI Agents
In `@sources/components/EnvironmentVariablesList.tsx` around lines 158 - 165, The
current logic uses getDocumentation(varNameFromValue || envVar.name) which
ignores documentation attached to envVar.name when the value references another
var; update to fetch both docs: call getDocumentation(envVar.name) (primaryDocs)
and, if varNameFromValue is present, getDocumentation(varNameFromValue)
(refDocs), then compute isSecret as true if primaryDocs.isSecret ||
refDocs?.isSecret || SECRET_NAME_REGEX.test(envVar.name) ||
SECRET_NAME_REGEX.test(varNameFromValue || ''); for display/metadata prefer
primaryDocs but include refDocs where needed (e.g., show reference info or
combine flags) so documented secrets on the variable name are preserved even
when the value is a reference.
|
Tip For best results, initiate chat on the files or code changes.
✅ New Session UX RestorationExcellent work restoring the original New Session experience:
✅ Feature Gating & SettingsWell-implemented opt-in strategy:
✅ Profile Management ImprovementsSignificant architectural improvements:
Good patterns:
✅ Environment Variable SafetyCritical security improvements:
Well-designed components:
✅ Component RefactoringGood modular design:
✅ Type Safety & SchemaStrong type improvements:
|
1 participant
Context / Goal
This PR is a follow-up to
slopus/happy#272, which introduced a large new-session + profiles + env-var/settings/sync surface.The goal here is to keep the feature work, but restore the pre-#272 “standard New Session” UX and make the new profiles/env work coherent + safe.
I’m trying to keep this factual (no blame): this is mostly stabilization work after a very large merge.
Summary (what this PR does)
What #272 introduced (and what we’re fixing here)
Profiles/editor correctness
id: ''.Navigation coherence (web + native + unmount safety)
Global daemon env fetch (and secret exposure risk)
envVarRefs/daemonEnvfetch in/newthat queried the daemon for all${VAR}references across all profiles (including secrets).New Session (standard flow) regressions
Wizard + pickers inconsistent with existing UI
Profiles “leaked” into default behavior
Non-UI safety/reliability issues (PR272 logic surface)
profileSync.ts, `NewSessionWizard) that wasn’t actually wired.Even if Codex was detected on the machines, the Codex profiles stayed disabled
IMAGE
What we implemented to fix/improve it (what’s in this branch)
1) New Session UX restored (pre-PR272 feel, still compatible with new features)
t('session.inputPlaceholder')) and legacy keyboard behavior.2) Wizard + pickers refactored to reduce duplication and improve coherence
3) Profiles made truly optional (separate feature flag) + coherent selection UX
useProfilesflag (separate fromuseEnhancedSessionWizard).4) Profile identity persisted/displayed safely
profileIdand show a read-only chip in session UI.Non-UI “logic hardening” improvements tied to PR272’s new surface area
These changes are limited to files added/modified by PR272, and fix real issues introduced or made risky by PR272’s new data/features.
A) Prevent leaking secrets via logs
__DEV__.B) Fix tool-result robustness (don’t drop messages; don’t break on structured outputs)
C) Make settings parsing tolerant (avoid “one bad profile nukes all settings”)
settingsParse()to parse fields individually and filter invalid profile entries rather than failing the entire settings object.D) Unify env-var template parsing + preserve correctness in remote env querying
${VAR}and${VAR:-default}consistently.nodewhen available (fallback remains safe)E) Remove unused placeholder sync service added by PR272
profileSync.tswas added but not referenced; removed to prevent future confusion.F) Conservative security: avoid querying remote values for secret-like vars
PASS|PASSWORD|COOKIEin addition to token/key/secret/auth.Links to the detailled issues in slopus/happy#272 (links go to the PR diff)
https://github.com/slopus/happy/pull/272/files#diff-30e51abcbe846d95abc7b9de0aee916aa6a7489e72486ec06388b9ac612f7e6bR322
https://github.com/slopus/happy/pull/272/files#diff-f4b041fa0826ef7d66e7e872b4e23bd490d2dc755c0c879551045a005731f401R1147
https://github.com/slopus/happy/pull/272/files#diff-f4b041fa0826ef7d66e7e872b4e23bd490d2dc755c0c879551045a005731f401R1897
https://github.com/slopus/happy/pull/272/files#diff-f4b041fa0826ef7d66e7e872b4e23bd490d2dc755c0c879551045a005731f401R1007
https://github.com/slopus/happy/pull/272/files#diff-f4b041fa0826ef7d66e7e872b4e23bd490d2dc755c0c879551045a005731f401R1012
https://github.com/slopus/happy/pull/272/files#diff-f4b041fa0826ef7d66e7e872b4e23bd490d2dc755c0c879551045a005731f401R481
https://github.com/slopus/happy/pull/272/files#diff-f4b041fa0826ef7d66e7e872b4e23bd490d2dc755c0c879551045a005731f401R491
https://github.com/slopus/happy/pull/272/files#diff-03a554e2004f08654732692f1487a20f748f4ed90e7e187d90678e7d24abb232R401
https://github.com/slopus/happy/pull/272/files#diff-388510a51fa84edaf57a36644f1a084aeb18ee8bd265a798133f48dc5cee281cR257
https://github.com/slopus/happy/pull/272/files#diff-388510a51fa84edaf57a36644f1a084aeb18ee8bd265a798133f48dc5cee281cR270
https://github.com/slopus/happy/pull/272/files#diff-0275aef060275b84215b2726a2b6c016e929a644b469df6b77915fbf3fa8de48R32
https://github.com/slopus/happy/pull/272/files#diff-f4b041fa0826ef7d66e7e872b4e23bd490d2dc755c0c879551045a005731f401R780
https://github.com/slopus/happy/pull/272/files#diff-f4b041fa0826ef7d66e7e872b4e23bd490d2dc755c0c879551045a005731f401R43
https://github.com/slopus/happy/pull/272/files#diff-0275aef060275b84215b2726a2b6c016e929a644b469df6b77915fbf3fa8de48R25
https://github.com/slopus/happy/pull/272/files#diff-0275aef060275b84215b2726a2b6c016e929a644b469df6b77915fbf3fa8de48R46
https://github.com/slopus/happy/pull/272/files#diff-92a4e9f2312662ce954da5d3c480da187552b897c564bc19e45db5e2b5c5cdabR110
https://github.com/slopus/happy/pull/272/files#diff-cbde5d803bb1053919f1167c4eb4f7a56b48dd93f77e75d0ebef73e4fe1f3f92R92
https://github.com/slopus/happy/pull/272/files#diff-15dcee897d504b4753e80c45148ed21f31394550cf02721bd2641929b4be06b1R136
https://github.com/slopus/happy/pull/272/files#diff-01c0b6096ea1c61ee49415456c4d662f513a21a3e3295d7882fde9b94305137aR62
https://github.com/slopus/happy/pull/272/files#diff-f4b041fa0826ef7d66e7e872b4e23bd490d2dc755c0c879551045a005731f401R563
https://github.com/slopus/happy/pull/272/files#diff-f4b041fa0826ef7d66e7e872b4e23bd490d2dc755c0c879551045a005731f401R1346
https://github.com/slopus/happy/pull/272/files#diff-21ea1e7b314af287b486e284fe47fa9681dc76a38cb14c0e2395d7467a7556e0R48
https://github.com/slopus/happy/pull/272/files#diff-21ea1e7b314af287b486e284fe47fa9681dc76a38cb14c0e2395d7467a7556e0R148
https://github.com/slopus/happy/pull/272/files#diff-92a4e9f2312662ce954da5d3c480da187552b897c564bc19e45db5e2b5c5cdabR372
https://github.com/slopus/happy/pull/272/files#diff-7077cf5c55232633bf3a22097c51d59b3eb25db37a642f78e128197f3425e8c2R86
https://github.com/slopus/happy/pull/272/files#diff-7077cf5c55232633bf3a22097c51d59b3eb25db37a642f78e128197f3425e8c2R92
https://github.com/slopus/happy/pull/272/files#diff-9b2dcf6707736bef944b8a005ff7ff6b4f5856defbe446710027e1ebc25b17d1R143
https://github.com/slopus/happy/pull/272/files#diff-cbde5d803bb1053919f1167c4eb4f7a56b48dd93f77e75d0ebef73e4fe1f3f92R84
(This is relevant because PR107’s tmux util does not implement “current/most recent” when name is empty—see below.)
https://github.com/slopus/happy/pull/272/files#diff-a2f78b18918804b89391c64e0942948bfbd600834e789295a93fec0328365e38R14
https://github.com/slopus/happy/pull/272/files#diff-f754c8a677da1cc9a08309fa83d216afe0b6d2c47561cb89b32fc97c6c970153R12
(Runtime import of English from _default.ts is in sources/text/index.ts, but that file wasn’t changed in #272 so it can’t be linked via the PR /files view.)
newpage):https://github.com/slopus/happy/pull/272/files#diff-51775e520e07fb30ce518a991ca21278568a253304b102071fce5fb4c6e41effR1
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests & Docs
✏️ Tip: You can customize this high-level summary in your review settings.