Add project-work sidebar and worktree polish#269
Conversation
Copy as promptReviewed by reactreview for commit e0806aa. Configure here. |
|
@codex please review this PR and give me 10-20 issues if any. Categorize findings as required, recommended, or optional. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8bcf1c719
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (appliedInitialEmptyScopeRef.current || initialVisibleProjectIds === undefined) return | ||
| appliedInitialEmptyScopeRef.current = true | ||
| if (initialVisibleProjectIds === null || initialVisibleProjectIds.length > 0) return | ||
| onShowView('landing') |
There was a problem hiding this comment.
This effect forces onShowView('landing') whenever the persisted scope is [], but ProjectWorkSection is also reused for extension/skill project-target selection. If a user previously saved an empty scope, opening Extensions/Skills immediately gets redirected away before they can choose a target project, which blocks those workflows until scope state is manually changed elsewhere.
Useful? React with 👍 / 👎.
| confirmLabel: _confirmLabel = 'Yes', | ||
| cancelLabel: _cancelLabel = 'Cancel', |
There was a problem hiding this comment.
confirmLabel and cancelLabel are explicitly renamed to unused variables, so callers can no longer provide action-specific text (for example, “Delete”). This removes contextual confirmation wording across existing call sites and makes destructive confirmations less clear.
Useful? React with 👍 / 👎.
| <button | ||
| type="button" |
There was a problem hiding this comment.
Both controls are icon-only buttons without an accessible name (aria-label/text), so assistive tech announces unlabeled buttons. This is an accessibility regression for all confirmations rendered by this component.
Useful? React with 👍 / 👎.
| onDelete={() => | ||
| onAction('thread.archive', { | ||
| projectId: project.id, | ||
| threadId: thread.id, |
There was a problem hiding this comment.
This row wires the ThreadRow “Delete session” affordance to thread.archive, which only archives/hides the thread instead of deleting persisted session data. Users who invoke delete from active project rows will believe data was removed when it was only archived.
Useful? React with 👍 / 👎.
| function getAssistantUsage(entry: unknown) { | ||
| const message = | ||
| typeof entry === 'object' && entry !== null && 'message' in entry ? entry.message : null | ||
| if ( | ||
| typeof message !== 'object' || | ||
| message === null || | ||
| !('role' in message) || | ||
| message.role !== 'assistant' || | ||
| !('usage' in message) || |
There was a problem hiding this comment.
The new deletion-time usage parser counts any object with message.role === 'assistant', but the existing usage summarizer only counts entries where type === 'message'. If session logs contain non-message records with embedded message payloads, deleted-session totals become inflated versus live/archived summaries.
Useful? React with 👍 / 👎.
| CREATE TABLE IF NOT EXISTS project_usage_totals ( | ||
| cwd TEXT PRIMARY KEY, |
There was a problem hiding this comment.
project_usage_totals is keyed by cwd but has no foreign key to projects and no cascade behavior. After project deletion, totals rows can remain orphaned and will be re-applied if the same path is added again, producing stale/incorrect lifetime usage for the new project entry.
Useful? React with 👍 / 👎.
|
@codex please review this PR and give me 10-20 issues if any. Categorize findings as required, recommended, or optional. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8b7548333
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| projectTargetMode={true} | ||
| projectScopeLockActive={props.projectScopeLockActive} |
There was a problem hiding this comment.
Do not force projectTargetMode on every Extensions/Skills view render; this bypasses projectScopeLockActive and keeps the sidebar in install-target selection mode even when scope locking is not active. That removes normal sidebar navigation for those views and changes behavior from conditional lock mode to always-on target mode.
Useful? React with 👍 / 👎.
| await storeUsageBeforeDelete(threadId, sessionPath) | ||
| } catch (error) { | ||
| if ( | ||
| typeof error !== 'object' || | ||
| error === null || | ||
| !('code' in error) || | ||
| error.code !== 'ENOENT' | ||
| ) { | ||
| throw error | ||
| } | ||
| if (!isMissingFileError(error)) throw error | ||
| } | ||
| } | ||
|
|
||
| if (sessionPath) { | ||
| await unlinkSessionFile(sessionPath) |
There was a problem hiding this comment.
Avoid writing usage totals before the delete is guaranteed to succeed. If storeUsageBeforeDelete() succeeds but unlinkSessionFile() or deleteThreadRecord() fails, a retry will add the same session usage again and permanently overcount project usage/cost totals.
Useful? React with 👍 / 👎.
| const error = result?.result?.error | ||
| if (typeof error === 'string' && error.includes('Worktree is dirty')) onBlocked() | ||
| }) |
There was a problem hiding this comment.
This handler only reacts to the 'Worktree is dirty' substring and ignores other switch errors. Since workspace.switch-branch is excluded from global error surfacing, failures like checkout conflicts or missing refs become effectively silent to users.
Useful? React with 👍 / 👎.
| <button | ||
| type="button" | ||
| className="sidebar-project-work-project-block-heading" | ||
| onClick={() => onFocusProject(project.id)} | ||
| > | ||
| {project.repoOriginUrl ? <GitHubInvertocatMark size={13} /> : <FolderCode size={13} />} | ||
| {editingName ? ( | ||
| <ProjectRenameField |
There was a problem hiding this comment.
Rendering ProjectRenameField inside a <button> creates invalid interactive nesting and can cause keyboard/click side effects while renaming (for example, triggering the parent button action unexpectedly). Use a non-button container while edit mode is active.
Useful? React with 👍 / 👎.
| onToggleExpanded={() => | ||
| onSetExpandedProjectIds((current) => ({ | ||
| ...current, | ||
| [`project:${project.id}`]: expanded, | ||
| })) |
There was a problem hiding this comment.
Project expansion is tracked only in local React state here and never routed through persisted project collapse actions, so expand/collapse preferences are lost after shell reload. The backend still carries persistent collapsed project state, so this creates a UX regression in state durability.
Useful? React with 👍 / 👎.
| const defaultCollapsed = !(group.current || group.id === selectedGroupId) | ||
| const collapsed = normalizedSearchQuery | ||
| ? false | ||
| : (collapsedBranchIds[group.id] ?? defaultCollapsed) |
There was a problem hiding this comment.
Using group.id directly as the collapse key leaks collapse state across projects that share branch names (e.g., main, release). Switching projects can inherit the wrong expanded/collapsed state because keys are not scoped by project id.
Useful? React with 👍 / 👎.
| group={group} | ||
| project={project} | ||
| onAction={onAction} | ||
| onBlocked={() => onSetSwitchErrorBranchId(branchActionKey)} |
There was a problem hiding this comment.
Once switchErrorBranchId is set for a branch, this branch remains in blocked-tooltip mode without a reset path in this flow. After users clean the worktree, the stale blocked state can keep showing misleading dirty-state affordances.
Useful? React with 👍 / 👎.
| { value: 'project', label: `Project ${counts.project}` }, | ||
| { value: 'chat', label: `Chat ${counts.chat}` }, |
There was a problem hiding this comment.
The project scope option is always enabled even when projectScopeAvailable is false. That allows users to enter a scope that immediately errors on install (Select a project first) instead of preventing the invalid state up front.
Useful? React with 👍 / 👎.
| { value: 'project', label: `Project ${counts.project}` }, | ||
| { value: 'chat', label: `Chat ${counts.chat}` }, |
There was a problem hiding this comment.
As in Extensions, the Skills scope switch keeps project selectable without an active project path. This permits an invalid scope selection and pushes failure handling downstream instead of gating in the control.
Useful? React with 👍 / 👎.
|
|
||
| const recentEvents = await env.DB.prepare( | ||
| "SELECT COUNT(*) AS count FROM poll_vote_events WHERE voter_hash = ? AND created_at > datetime('now', '-30 seconds')", | ||
| "SELECT COUNT(*) AS count FROM poll_vote_events WHERE voter_hash = ? AND julianday(created_at) > julianday('now', '-30 seconds')", |
There was a problem hiding this comment.
Wrapping created_at with julianday() makes the predicate non-sargable against poll_vote_events_voter_created_idx (voter_hash, created_at), so the hot-path throttle query can degrade into per-voter scans as event history grows.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
bun run ai:checkChangelog
docs/changelog.mdunder0.1.66.