Skip to content

Commit bb9bdde

Browse files
committed
fix: resolve 10 critical and quality bugs
High priority fixes: - Fix token count mismatch: token counting now includes file headers and markdown fences to match actual copy output format exactly - Fix missing useEffect dependencies: add includeBinaryPaths, tok, and onBatch to useTokenCounts dependencies - Fix preview file race condition: add request ID tracking to prevent displaying wrong file content when switching quickly - Fix debounce cleanup: add cancel() method and call it on unmount to prevent setState after unmount warnings - Fix Rust rename/copy handling: properly track old_path and new_path for renames and copies instead of misclassifying them - Fix non-UTF8 file handling: use lossy UTF-8 conversion for files with encodings like Latin-1 instead of erroring Medium priority fixes: - Fix template overwrite: add confirmation dialog before replacing user's custom instructions with template - Fix desktop resetRepo semantics: add clarifying comments that dispose() is reusable and service instance persists Quality improvements: - Move PROMPT_TEMPLATES outside component to avoid recreation on every render - Remove SelectedFilesPanel key prop to prevent unnecessary remounts and state loss when selection changes
1 parent ba28612 commit bb9bdde

6 files changed

Lines changed: 159 additions & 57 deletions

File tree

apps/desktop/src-tauri/src/git.rs

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ pub struct LoadRepoResult {
1313
pub struct DiffFile {
1414
pub path: String,
1515
#[serde(rename = "type")]
16-
pub change_type: String, // "modify" | "add" | "remove"
16+
pub change_type: String, // "modify" | "add" | "remove" | "rename" | "copy"
17+
#[serde(skip_serializing_if = "Option::is_none")]
18+
pub old_path: Option<String>, // For renames and copies
1719
}
1820

1921
#[derive(Debug, Serialize, Deserialize)]
@@ -116,24 +118,41 @@ pub fn git_diff(path: &str, base: &str, compare: &str) -> Result<DiffResult, Str
116118

117119
diff.foreach(
118120
&mut |delta, _progress| {
119-
let path = delta.new_file().path()
120-
.or_else(|| delta.old_file().path())
121+
let new_path = delta.new_file().path()
121122
.and_then(|p| p.to_str())
122-
.unwrap_or("")
123-
.to_string();
124-
125-
let change_type = match delta.status() {
126-
git2::Delta::Added => "add",
127-
git2::Delta::Deleted => "remove",
128-
git2::Delta::Modified => "modify",
129-
git2::Delta::Renamed => "modify",
130-
git2::Delta::Copied => "add",
131-
_ => "modify",
123+
.map(|s| s.to_string());
124+
125+
let old_path = delta.old_file().path()
126+
.and_then(|p| p.to_str())
127+
.map(|s| s.to_string());
128+
129+
let (path, change_type, stored_old_path) = match delta.status() {
130+
git2::Delta::Added => {
131+
(new_path.unwrap_or_default(), "add", None)
132+
},
133+
git2::Delta::Deleted => {
134+
(old_path.unwrap_or_default(), "remove", None)
135+
},
136+
git2::Delta::Modified => {
137+
(new_path.unwrap_or_default(), "modify", None)
138+
},
139+
git2::Delta::Renamed => {
140+
// For renames, store the new path as primary and old path separately
141+
(new_path.clone().unwrap_or_default(), "rename", old_path)
142+
},
143+
git2::Delta::Copied => {
144+
// For copies, store the new path as primary and old path separately
145+
(new_path.clone().unwrap_or_default(), "copy", old_path)
146+
},
147+
_ => {
148+
(new_path.or(old_path).unwrap_or_default(), "modify", None)
149+
},
132150
};
133151

134152
files.push(DiffFile {
135153
path,
136154
change_type: change_type.to_string(),
155+
old_path: stored_old_path,
137156
});
138157

139158
true
@@ -297,9 +316,8 @@ pub fn read_file_blob(path: &str, ref_name: &str, file_path: &str) -> Result<Rea
297316
});
298317
}
299318

300-
// Convert to UTF-8 string
301-
let text = String::from_utf8(content.to_vec())
302-
.map_err(|_| "Failed to decode file as UTF-8".to_string())?;
319+
// Convert to UTF-8 string (use lossy conversion for non-UTF8 encodings like Latin-1)
320+
let text = String::from_utf8_lossy(content).into_owned();
303321

304322
Ok(ReadFileResult {
305323
binary: false,

apps/desktop/src/hooks/useGitRepository.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,8 @@ export function useGitRepository(setAppStatus?: (s: AppStatus) => void) {
111111
}, [currentDir, loadRepoFromHandle])
112112

113113
const resetRepo = useCallback(() => {
114+
// Note: dispose() clears the repo path but the service instance remains
115+
// valid and can be reused. This is by design for the singleton pattern.
114116
gitClient.dispose()
115117
setRepoStatus({ state: 'idle' })
116118
setCurrentDir(null)

apps/desktop/src/services/TauriGitService.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,11 @@ export class TauriGitService implements GitService {
111111
return result
112112
}
113113

114+
/**
115+
* Dispose of resources by clearing the repo path.
116+
* Note: This service instance remains valid and can be reused after dispose().
117+
* Call loadRepo() again to load a new repository.
118+
*/
114119
dispose(): void {
115120
this.repoPath = null
116121
}

apps/web/src/App.tsx

Lines changed: 70 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,34 @@ import { clearRepositoryCache } from './utils/cache'
2121
import { logError } from './utils/logger'
2222
import { debounce } from './utils/debounce'
2323

24+
// Prompt templates (moved outside component to avoid recreation on every render)
25+
const PROMPT_TEMPLATES = [
26+
{
27+
id: 'branch-summary',
28+
label: 'Summarize branch diff',
29+
content:
30+
'You are an expert engineer. Summarize the changes between the selected branches and explain the impact of the modifications.',
31+
},
32+
{
33+
id: 'wd-review',
34+
label: 'Review working directory changes',
35+
content:
36+
'You are a code reviewer. Review the current working directory diff for potential issues, bugs, or improvements.',
37+
},
38+
{
39+
id: 'test-plan',
40+
label: 'Suggest tests for diff',
41+
content:
42+
'You are a QA engineer. Based on the provided diff, propose relevant unit or integration tests to cover the changes.',
43+
},
44+
{
45+
id: 'release-notes',
46+
label: 'Draft release notes',
47+
content:
48+
'You are a technical writer. Craft concise release notes that describe the user-facing effects of the diff.',
49+
},
50+
]
51+
2452
function App() {
2553
const [appStatus, setAppStatus] = useState<AppStatus>({ state: 'IDLE' })
2654
// note: we will temporarily set task='tokens' while counting, see effect below
@@ -51,6 +79,9 @@ function App() {
5179
compare?: { binary: boolean; text: string | null; notFound?: boolean }
5280
} | null>(null)
5381

82+
// Track preview request IDs to prevent race conditions
83+
const previewRequestIdRef = useRef(0)
84+
5485
const [theme, setTheme] = useState<'light' | 'dark' | null>(() => {
5586
try {
5687
const saved = localStorage.getItem('gc.theme')
@@ -270,32 +301,6 @@ function App() {
270301
return () => { cancelled = true }
271302
}, [userInstructions])
272303

273-
const PROMPT_TEMPLATES = [
274-
{
275-
id: 'branch-summary',
276-
label: 'Summarize branch diff',
277-
content:
278-
'You are an expert engineer. Summarize the changes between the selected branches and explain the impact of the modifications.',
279-
},
280-
{
281-
id: 'wd-review',
282-
label: 'Review working directory changes',
283-
content:
284-
'You are a code reviewer. Review the current working directory diff for potential issues, bugs, or improvements.',
285-
},
286-
{
287-
id: 'test-plan',
288-
label: 'Suggest tests for diff',
289-
content:
290-
'You are a QA engineer. Based on the provided diff, propose relevant unit or integration tests to cover the changes.',
291-
},
292-
{
293-
id: 'release-notes',
294-
label: 'Draft release notes',
295-
content:
296-
'You are a technical writer. Craft concise release notes that describe the user-facing effects of the diff.',
297-
},
298-
]
299304
const [templateId, setTemplateId] = useState<string>('')
300305

301306
// Model selection: fetched dynamically; derive token limit from the selected model
@@ -370,6 +375,14 @@ function App() {
370375
const diffRangeRef = useRef<HTMLInputElement | null>(null)
371376
const MAX_CONTEXT = 999
372377
const debouncedSetDiffContextLines = useMemo(() => debounce(setDiffContextLines, 250), [])
378+
379+
// Cancel debounced function on unmount to avoid setState after unmount
380+
useEffect(() => {
381+
return () => {
382+
debouncedSetDiffContextLines.cancel()
383+
}
384+
}, [debouncedSetDiffContextLines])
385+
373386
// Collapsible User Instructions
374387
const [instructionsOpen, setInstructionsOpen] = useState<boolean>(true)
375388

@@ -583,6 +596,10 @@ function App() {
583596
setNotif('Binary file preview is not supported.')
584597
return
585598
}
599+
600+
// Increment request ID to track this specific preview request
601+
const requestId = ++previewRequestIdRef.current
602+
586603
try {
587604
const toFetchBase = status !== 'add'
588605
const toFetchCompare = status !== 'remove'
@@ -592,6 +609,11 @@ function App() {
592609
toFetchCompare && compareBranch ? gitClient.readFile(compareBranch, path) : Promise.resolve(undefined),
593610
])
594611

612+
// Check if this request is still the latest one (prevent race condition)
613+
if (requestId !== previewRequestIdRef.current) {
614+
return // A newer preview request has been made, ignore this result
615+
}
616+
595617
// If worker reports binary, bail out as well
596618
const baseBin = (baseRes as any)?.binary
597619
const compareBin = (compareRes as any)?.binary
@@ -607,8 +629,11 @@ function App() {
607629
})
608630
setPreviewOpen(true)
609631
} catch (e) {
610-
const err = e instanceof Error ? e : new Error(String(e))
611-
setNotif(`Failed to read file content: ${err.message}`)
632+
// Only show error if this is still the latest request
633+
if (requestId === previewRequestIdRef.current) {
634+
const err = e instanceof Error ? e : new Error(String(e))
635+
setNotif(`Failed to read file content: ${err.message}`)
636+
}
612637
}
613638
}
614639

@@ -1151,9 +1176,24 @@ function App() {
11511176
value={templateId}
11521177
onChange={(e) => {
11531178
const id = e.target.value
1154-
setTemplateId(id)
11551179
const tmpl = PROMPT_TEMPLATES.find((t) => t.id === id)
1156-
if (tmpl) setUserInstructions(tmpl.content)
1180+
if (tmpl) {
1181+
// If user has custom instructions, confirm before overwriting
1182+
const hasCustomText = userInstructions.trim() && userInstructions.trim() !== tmpl.content
1183+
if (hasCustomText) {
1184+
const confirmed = window.confirm(
1185+
'This will replace your current instructions. Continue?'
1186+
)
1187+
if (!confirmed) {
1188+
// Reset select to previous value
1189+
return
1190+
}
1191+
}
1192+
setTemplateId(id)
1193+
setUserInstructions(tmpl.content)
1194+
} else {
1195+
setTemplateId(id)
1196+
}
11571197
}}
11581198
style={{ flexGrow: 1 }}
11591199
>
@@ -1297,7 +1337,6 @@ function App() {
12971337

12981338
<div className="panel-section">
12991339
<SelectedFilesPanel
1300-
key={`sel-${selectedPaths.size}`}
13011340
selectedPaths={selectedPaths}
13021341
statusByPath={statusByPath}
13031342
onUnselect={(path) => toggleSelect(path)}

apps/web/src/hooks/useTokenCounts.ts

Lines changed: 36 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,19 @@ import { buildUnifiedDiffForStatus } from '../utils/diff'
66
import { isBinaryPath, MAX_CONCURRENT_READS } from '@gitcontext/core'
77
import { mapWithConcurrency } from '../utils/concurrency'
88

9+
// Helper to infer language from file extension for syntax highlighting
10+
function inferLangFromPath(p: string): string {
11+
const ext = p.split('.').pop()?.toLowerCase() || ''
12+
const langMap: Record<string, string> = {
13+
js: 'javascript', jsx: 'javascript', ts: 'typescript', tsx: 'typescript',
14+
py: 'python', rb: 'ruby', go: 'go', rs: 'rust', c: 'c', cpp: 'cpp', java: 'java',
15+
kt: 'kotlin', swift: 'swift', php: 'php', cs: 'csharp', sh: 'bash', yaml: 'yaml',
16+
yml: 'yaml', json: 'json', xml: 'xml', html: 'html', css: 'css', scss: 'scss',
17+
md: 'markdown', sql: 'sql',
18+
}
19+
return langMap[ext] || ''
20+
}
21+
922
export type TokenCounts = Map<string, number>
1023

1124
type Args = {
@@ -80,28 +93,42 @@ export function useTokenCounts({
8093
needBase && baseRef ? gitClient.readFile(baseRef, path) : Promise.resolve(undefined as any),
8194
needCompare && compareRef ? gitClient.readFile(compareRef, path) : Promise.resolve(undefined as any),
8295
])
83-
// Mirror final output generation logic
96+
// Mirror final output generation logic EXACTLY as in copyAllSelected
97+
const header = `## FILE: ${path} (${(status || 'unchanged').toUpperCase()})\n\n`
8498
const MAX_CONTEXT = 999
8599
const ctx = diffContextLines >= MAX_CONTEXT ? Number.MAX_SAFE_INTEGER : diffContextLines
100+
86101
if (status === 'modify' || status === 'add' || status === 'remove') {
87102
const isBinary = Boolean((baseRes as any)?.binary) || Boolean((compareRes as any)?.binary)
88103
if (isBinary) {
89104
// Edge: unknown ext but worker says binary; treat same as looksBinary
90105
if (includeBinaryPaths) {
91-
const header = `## FILE: ${path} (${(status || 'unchanged').toUpperCase()})\n\n`
92-
textForCount = header
106+
textForCount = header // Just the header, no [Binary file] text
93107
} else {
94108
textForCount = ''
95109
}
96-
} else if (status === 'add' && ctx === Number.MAX_SAFE_INTEGER) {
97-
textForCount = (compareRes as { text?: string } | undefined)?.text ?? ''
110+
} else if (status === 'add') {
111+
// Include header + markdown fences + content (matches copyAllSelected line 781)
112+
const newTextRaw = (compareRes as { text?: string } | undefined)?.text ?? ''
113+
const newText = newTextRaw.endsWith('\n') ? newTextRaw.slice(0, -1) : newTextRaw
114+
const lang = inferLangFromPath(path)
115+
textForCount = header + '```' + lang + '\n' + newText + '\n```\n\n'
98116
} else {
99-
textForCount = buildUnifiedDiffForStatus(status, path, baseRes as any, compareRes as any, { context: ctx }) || ''
117+
// modify/remove: include header + diff fences + diff (matches copyAllSelected line 791)
118+
const diffText = buildUnifiedDiffForStatus(status, path, baseRes as any, compareRes as any, { context: ctx }) || ''
119+
if (diffText) {
120+
textForCount = header + '```diff\n' + diffText + '```\n\n'
121+
} else {
122+
// Fallback: no text (matches copyAllSelected line 794)
123+
textForCount = header + '_No textual content available._\n\n'
124+
}
100125
}
101126
} else {
127+
// unchanged: include header + markdown fences + content (matches copyAllSelected line 800)
102128
const isBinary = Boolean((baseRes as any)?.binary)
103-
const oldText = isBinary || (baseRes as any)?.notFound ? '' : (baseRes as any)?.text ?? ''
104-
textForCount = oldText
129+
const text = isBinary || (baseRes as any)?.notFound ? '' : (baseRes as any)?.text ?? ''
130+
const lang = inferLangFromPath(path)
131+
textForCount = header + '```' + lang + '\n' + (text || '') + '\n```\n\n'
105132
}
106133
}
107134
const n = textForCount ? await tok.count(textForCount) : 0
@@ -140,7 +167,7 @@ export function useTokenCounts({
140167
return () => {
141168
abortController.abort()
142169
}
143-
}, [gitClient, baseRef, compareRef, selectedList, statusByPath, diffContextLines])
170+
}, [gitClient, baseRef, compareRef, selectedList, statusByPath, diffContextLines, includeBinaryPaths, tok, onBatch])
144171

145172
const total = useMemo(() => {
146173
let sum = 0

apps/web/src/utils/debounce.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,25 @@ export function debounce<T extends (...args: any[]) => unknown>(
33
delay: number,
44
) {
55
let timeoutId: ReturnType<typeof setTimeout> | null = null
6-
return function(this: ThisParameterType<T>, ...args: Parameters<T>) {
6+
7+
const debounced = function(this: ThisParameterType<T>, ...args: Parameters<T>) {
78
if (timeoutId) {
89
clearTimeout(timeoutId)
910
}
1011
timeoutId = setTimeout(() => {
1112
fn.apply(this, args as unknown as []);
13+
timeoutId = null
1214
}, delay)
15+
} as T & { cancel: () => void }
16+
17+
debounced.cancel = () => {
18+
if (timeoutId) {
19+
clearTimeout(timeoutId)
20+
timeoutId = null
21+
}
1322
}
23+
24+
return debounced
1425
}
1526

1627

0 commit comments

Comments
 (0)