Skip to content

fix(git): materialize draft session for generate#3

Merged
bashrusakh merged 1 commit into
win-artifact-pr-fixes-v2from
fix/git-generate-draft-session
Jun 22, 2026
Merged

fix(git): materialize draft session for generate#3
bashrusakh merged 1 commit into
win-artifact-pr-fixes-v2from
fix/git-generate-draft-session

Conversation

@bashrusakh

Copy link
Copy Markdown
Owner

No description provided.

@bashrusakh bashrusakh merged commit af41883 into win-artifact-pr-fixes-v2 Jun 22, 2026
1 of 2 checks passed

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors session creation and context resolution by introducing a reusable materializeOpenDraftSession helper and transitioning to asynchronous generation session context resolution. Feedback on these changes includes improving type safety and code reuse by leveraging resolveDirectoryKey instead of accessing created.directory directly, simplifying folder assignment logic by reusing the normalized createdDirectory variable, and applying optional chaining to lastChoice.modelID for consistency.

Important

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

Comment on lines +441 to +447
persistDraftTarget({
projectId: draftProjectId,
directory: normalizePath(draftDirectoryOverride ?? created.directory ?? null),
})

const draftSyntheticParts = draft.syntheticParts
const createdDirectory = normalizePath(draftDirectoryOverride ?? created.directory ?? null)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Instead of accessing created.directory directly (which is not type-safe on the Session object and doesn't handle fallback fields like project.worktree), we should leverage the existing resolveDirectoryKey helper. We can define createdDirectory first and reuse it for both persistDraftTarget and subsequent operations to keep the code DRY and robust.

Suggested change
persistDraftTarget({
projectId: draftProjectId,
directory: normalizePath(draftDirectoryOverride ?? created.directory ?? null),
})
const draftSyntheticParts = draft.syntheticParts
const createdDirectory = normalizePath(draftDirectoryOverride ?? created.directory ?? null)
const createdDirectory = normalizePath(draftDirectoryOverride) ?? resolveDirectoryKey(created)
persistDraftTarget({
projectId: draftProjectId,
directory: createdDirectory,
})
const draftSyntheticParts = draft.syntheticParts

Comment on lines +468 to +473
if (draftTargetFolderId) {
const scopeKey = draftDirectoryOverride || created.directory || null
if (scopeKey) {
useSessionFoldersStore.getState().addSessionToFolder(scopeKey, draftTargetFolderId, created.id)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

We can simplify this block by reusing the already resolved and normalized createdDirectory variable instead of duplicating the fallback logic with draftDirectoryOverride || created.directory || null.

  if (draftTargetFolderId && createdDirectory) {
    useSessionFoldersStore.getState().addSessionToFolder(createdDirectory, draftTargetFolderId, created.id)
  }

Comment on lines +438 to +440
const lastChoiceModel = lastChoice?.providerID && lastChoice.modelID
? { providerId: lastChoice.providerID, modelId: lastChoice.modelID }
: null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For better type safety and consistency, use optional chaining on lastChoice.modelID as well. This prevents potential static analysis or TypeScript compiler warnings under strict null-checking configurations.

Suggested change
const lastChoiceModel = lastChoice?.providerID && lastChoice.modelID
? { providerId: lastChoice.providerID, modelId: lastChoice.modelID }
: null;
const lastChoiceModel = lastChoice?.providerID && lastChoice?.modelID
? { providerId: lastChoice.providerID, modelId: lastChoice.modelID }
: null;

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant