mirror of
https://github.com/callumalpass/tasknotes.git
synced 2026-07-22 12:50:26 +00:00
analysis: AI analysis for issue #789
[Bug]: Default tasks folder and Archive folder fields too short Generated by ai-issue-analyzer
This commit is contained in:
parent
3602799bac
commit
2273321def
2 changed files with 411 additions and 0 deletions
253
issue-analysis/issue-789.md
Normal file
253
issue-analysis/issue-789.md
Normal file
|
|
@ -0,0 +1,253 @@
|
|||
# Issue #789: Folder Path Truncation Bug
|
||||
|
||||
## Problem Understanding
|
||||
|
||||
**Issue:** The path fields for "Default tasks folder" and "Archive folder" allow entering long paths, but sometimes lose characters at the end of the path when saving.
|
||||
|
||||
**Example:**
|
||||
- Input: `0 System/004 Plugin Data/TaskNotes/Archive`
|
||||
- Saved: `0 System/004 Plugin Data/TaskNotes/Archi`
|
||||
|
||||
**Reported Behavior:**
|
||||
- The user reports it doesn't seem to be strictly about length (not a 40-char limit)
|
||||
- Sometimes chars are lost at the end of the path
|
||||
- This causes tasks to be created in wrong paths
|
||||
|
||||
## Test File Location
|
||||
|
||||
**File:** `/home/calluma/projects/tasknotes/tests/unit/issues/issue-789-folder-path-truncation.test.ts`
|
||||
|
||||
**How to Run:**
|
||||
```bash
|
||||
npm test -- issue-789-folder-path-truncation.test.ts
|
||||
```
|
||||
|
||||
The test demonstrates:
|
||||
1. How debounce with `immediate=true` blocks rapid subsequent calls
|
||||
2. How this can cause data loss when multiple onChange events occur in quick succession
|
||||
3. How a trailing debounce (immediate=false) would prevent this issue
|
||||
|
||||
## Root Cause Analysis
|
||||
|
||||
The bug is caused by the debounce implementation with `immediate=true` in the settings tab:
|
||||
|
||||
### Relevant Code Locations
|
||||
|
||||
1. **TaskNotesSettingTab.ts:22**
|
||||
```typescript
|
||||
private debouncedSave = debounce(() => this.plugin.saveSettings(), 500, true);
|
||||
```
|
||||
- Uses debounce with `immediate=true`
|
||||
- This causes the save to execute immediately on the first onChange
|
||||
- Then blocks subsequent saves for 500ms
|
||||
|
||||
2. **settingHelpers.ts:67-94** - `createTextSetting` function
|
||||
- Lines 74-80: Handles debounced vs immediate onChange
|
||||
- The text settings pass the user's setValue function directly to onChange
|
||||
- When debounceMs is NOT set, onChange fires immediately for each character typed
|
||||
|
||||
3. **generalTab.ts:29-39** - Default tasks folder setting
|
||||
```typescript
|
||||
createTextSetting(container, {
|
||||
name: translate("settings.general.taskStorage.defaultFolder.name"),
|
||||
getValue: () => plugin.settings.tasksFolder,
|
||||
setValue: async (value: string) => {
|
||||
plugin.settings.tasksFolder = value;
|
||||
save(); // <- calls debouncedSave
|
||||
},
|
||||
});
|
||||
```
|
||||
|
||||
4. **generalTab.ts:54-64** - Archive folder setting
|
||||
```typescript
|
||||
createTextSetting(container, {
|
||||
name: translate("settings.general.taskStorage.archiveFolder.name"),
|
||||
getValue: () => plugin.settings.archiveFolder,
|
||||
setValue: async (value: string) => {
|
||||
plugin.settings.archiveFolder = value;
|
||||
save(); // <- calls debouncedSave
|
||||
},
|
||||
});
|
||||
```
|
||||
|
||||
5. **main.ts:1200-1208** - saveSettings function
|
||||
```typescript
|
||||
async saveSettings() {
|
||||
const data = (await this.loadData()) || {};
|
||||
const settingsKeys = Object.keys(DEFAULT_SETTINGS) as (keyof TaskNotesSettings)[];
|
||||
for (const key of settingsKeys) {
|
||||
data[key] = this.settings[key];
|
||||
}
|
||||
await this.saveData(data);
|
||||
}
|
||||
```
|
||||
|
||||
### How the Bug Occurs
|
||||
|
||||
**Scenario 1: Quick Typing**
|
||||
1. User types quickly: "Archive" → each character triggers onChange
|
||||
2. First onChange saves immediately (due to immediate=true)
|
||||
3. Subsequent onChange events are blocked for 500ms
|
||||
4. Only partial path gets saved
|
||||
|
||||
**Scenario 2: Paste + Autocomplete**
|
||||
1. User pastes or uses autocomplete: "Archi" → onChange fired
|
||||
2. Save happens immediately with incomplete path
|
||||
3. Autocomplete finishes: "Archive" → onChange fired
|
||||
4. Blocked by debounce cooldown, never saved
|
||||
|
||||
**Scenario 3: Browser Autofill**
|
||||
1. Browser autofills field with partial text
|
||||
2. onChange fires, saves immediately
|
||||
3. Browser finishes autofilling
|
||||
4. Final onChange is blocked
|
||||
|
||||
## Proposed Solutions
|
||||
|
||||
### Solution 1: Use Trailing Debounce (Recommended)
|
||||
|
||||
**Change:** Switch from `immediate=true` to `immediate=false` in the debounce call.
|
||||
|
||||
**Implementation:**
|
||||
```typescript
|
||||
// In src/settings/TaskNotesSettingTab.ts:22
|
||||
private debouncedSave = debounce(() => this.plugin.saveSettings(), 500, false);
|
||||
// Or simply omit the third parameter (defaults to false)
|
||||
private debouncedSave = debounce(() => this.plugin.saveSettings(), 500);
|
||||
```
|
||||
|
||||
**Pros:**
|
||||
- Simple one-line fix
|
||||
- Ensures the final value is always saved
|
||||
- Reduces number of save operations (better for performance)
|
||||
- Consistent with common debounce patterns (e.g., search inputs)
|
||||
|
||||
**Cons:**
|
||||
- Introduces a 500ms delay before settings are saved
|
||||
- User might close settings before the save completes
|
||||
- Changes are not persisted until user stops typing
|
||||
|
||||
**Risk Level:** Low - Well-tested pattern
|
||||
|
||||
---
|
||||
|
||||
### Solution 2: Add Per-Field Debounce for Text Inputs
|
||||
|
||||
**Change:** Add debounce directly to text inputs instead of the save function.
|
||||
|
||||
**Implementation:**
|
||||
```typescript
|
||||
// In src/settings/tabs/generalTab.ts
|
||||
createTextSetting(container, {
|
||||
name: translate("settings.general.taskStorage.defaultFolder.name"),
|
||||
getValue: () => plugin.settings.tasksFolder,
|
||||
setValue: async (value: string) => {
|
||||
plugin.settings.tasksFolder = value;
|
||||
save();
|
||||
},
|
||||
debounceMs: 500, // Add this to problematic fields
|
||||
});
|
||||
```
|
||||
|
||||
**Pros:**
|
||||
- More granular control over which fields are debounced
|
||||
- Save function can remain immediate for other settings
|
||||
- Can tune debounce time per field type
|
||||
|
||||
**Cons:**
|
||||
- Requires updating multiple field definitions
|
||||
- Dual debouncing (field + save) might cause confusion
|
||||
- More code changes required
|
||||
|
||||
**Risk Level:** Medium - Requires changes in multiple files
|
||||
|
||||
---
|
||||
|
||||
### Solution 3: Hybrid Approach - Trailing Debounce with Blur Event Save
|
||||
|
||||
**Change:** Use trailing debounce + save on blur/unfocus events.
|
||||
|
||||
**Implementation:**
|
||||
```typescript
|
||||
// In src/settings/TaskNotesSettingTab.ts
|
||||
private debouncedSave = debounce(() => this.plugin.saveSettings(), 500, false);
|
||||
|
||||
// In settingHelpers.ts - enhance createTextSetting
|
||||
export function createTextSetting(container: HTMLElement, options: TextSettingOptions): Setting {
|
||||
return new Setting(container)
|
||||
.setName(options.name)
|
||||
.setDesc(options.desc)
|
||||
.addText((text) => {
|
||||
text.setValue(options.getValue());
|
||||
|
||||
if (options.debounceMs && options.debounceMs > 0) {
|
||||
const debouncedSetValue = debounce(options.setValue, options.debounceMs);
|
||||
text.onChange(debouncedSetValue);
|
||||
} else {
|
||||
text.onChange(options.setValue);
|
||||
}
|
||||
|
||||
// Add blur event to ensure save on focus loss
|
||||
text.inputEl.addEventListener('blur', () => {
|
||||
options.setValue(text.getValue());
|
||||
});
|
||||
|
||||
// ... rest of the function
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
**Pros:**
|
||||
- Best of both worlds: debounced during typing, immediate on blur
|
||||
- Guarantees settings are saved when user navigates away
|
||||
- No data loss risk
|
||||
- Responsive user experience
|
||||
|
||||
**Cons:**
|
||||
- Most complex solution
|
||||
- Might save twice in some cases (onChange + blur)
|
||||
- Requires modification to helper function
|
||||
|
||||
**Risk Level:** Medium - More complex but safer
|
||||
|
||||
---
|
||||
|
||||
## Recommended Approach
|
||||
|
||||
**Solution 1: Use Trailing Debounce**
|
||||
|
||||
This is the simplest and most reliable fix. The 500ms delay is acceptable for settings that don't need instant persistence. Most users expect some delay in settings panels.
|
||||
|
||||
**Why this solution:**
|
||||
1. **Minimal code change** - One line modification
|
||||
2. **Proven pattern** - Standard approach used in many applications
|
||||
3. **Fixes the root cause** - Ensures final value is always saved
|
||||
4. **Better performance** - Reduces unnecessary save operations
|
||||
5. **Low risk** - Well-understood behavior
|
||||
|
||||
**Additional Improvement:**
|
||||
Consider reducing the debounce time from 500ms to 300ms for a more responsive feel while still preventing excessive saves.
|
||||
|
||||
**Implementation Plan:**
|
||||
1. Change `TaskNotesSettingTab.ts:22` from `immediate=true` to `immediate=false` (or omit parameter)
|
||||
2. Test with long folder paths (paste, type quickly, autocomplete scenarios)
|
||||
3. Verify settings persist correctly when closing the settings panel
|
||||
4. Consider adding a blur event listener as a safety net (optional)
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
1. **Manual Testing:**
|
||||
- Paste long folder path → verify full path is saved
|
||||
- Type quickly → verify complete path is saved
|
||||
- Use autocomplete → verify completed path is saved
|
||||
- Close settings immediately after typing → verify save completes
|
||||
|
||||
2. **Automated Testing:**
|
||||
- Run existing test: `npm test -- issue-789-folder-path-truncation.test.ts`
|
||||
- Add integration test for settings save behavior
|
||||
- Test debounce timing edge cases
|
||||
|
||||
3. **Regression Testing:**
|
||||
- Verify other text settings still work correctly
|
||||
- Test all tabs in settings panel
|
||||
- Verify performance hasn't degraded
|
||||
158
tests/unit/issues/issue-789-folder-path-truncation.test.ts
Normal file
158
tests/unit/issues/issue-789-folder-path-truncation.test.ts
Normal file
|
|
@ -0,0 +1,158 @@
|
|||
/**
|
||||
* Test for GitHub Issue #789: Folder path truncation bug
|
||||
*
|
||||
* Bug Description:
|
||||
* The path fields for "Default tasks folder" and "Archive folder" allow entering long paths,
|
||||
* but sometimes lose characters at the end of the path when saving.
|
||||
* Example: "0 System/004 Plugin Data/TaskNotes/Archive" becomes
|
||||
* "0 System/004 Plugin Data/TaskNotes/Archi"
|
||||
*
|
||||
* Root Cause Hypothesis:
|
||||
* The debounced save function with immediate=true may cause settings to be saved
|
||||
* before all onChange events are processed, especially when typing quickly or
|
||||
* pasting long paths.
|
||||
*/
|
||||
|
||||
import { debounce } from '../../../src/settings/components/settingHelpers';
|
||||
|
||||
describe('Issue #789: Folder path truncation bug', () => {
|
||||
describe('Debounce behavior with immediate=true', () => {
|
||||
test('debounce with immediate=true should execute function immediately on first call', () => {
|
||||
const mockFn = jest.fn();
|
||||
const debouncedFn = debounce(mockFn, 500, true);
|
||||
|
||||
debouncedFn('first');
|
||||
expect(mockFn).toHaveBeenCalledTimes(1);
|
||||
expect(mockFn).toHaveBeenCalledWith('first');
|
||||
});
|
||||
|
||||
test('debounce with immediate=true should block rapid subsequent calls', () => {
|
||||
const mockFn = jest.fn();
|
||||
const debouncedFn = debounce(mockFn, 500, true);
|
||||
|
||||
debouncedFn('first');
|
||||
debouncedFn('second');
|
||||
debouncedFn('third');
|
||||
|
||||
// Only the first call should execute immediately
|
||||
expect(mockFn).toHaveBeenCalledTimes(1);
|
||||
expect(mockFn).toHaveBeenCalledWith('first');
|
||||
});
|
||||
|
||||
test('debounce with immediate=true allows subsequent calls after timeout', async () => {
|
||||
const mockFn = jest.fn();
|
||||
const debouncedFn = debounce(mockFn, 100, true);
|
||||
|
||||
debouncedFn('first');
|
||||
expect(mockFn).toHaveBeenCalledTimes(1);
|
||||
|
||||
// Wait for debounce timeout
|
||||
await new Promise(resolve => setTimeout(resolve, 150));
|
||||
|
||||
debouncedFn('second');
|
||||
expect(mockFn).toHaveBeenCalledTimes(2);
|
||||
expect(mockFn).toHaveBeenNthCalledWith(2, 'second');
|
||||
});
|
||||
|
||||
test('reproduces the folder path truncation bug: rapid onChange events', () => {
|
||||
// Simulate settings object
|
||||
const settings = {
|
||||
archiveFolder: '',
|
||||
};
|
||||
|
||||
// Simulate the debounced save function (immediate=true like in TaskNotesSettingTab)
|
||||
const saveFn = jest.fn(() => {
|
||||
// Save happens here - captures current settings state
|
||||
});
|
||||
const debouncedSave = debounce(saveFn, 500, true);
|
||||
|
||||
// Simulate typing or pasting a long path with multiple onChange events
|
||||
const path = '0 System/004 Plugin Data/TaskNotes/Archive';
|
||||
const chunks = [
|
||||
'0 System/004 Plugin Data/TaskNotes/Archi',
|
||||
'0 System/004 Plugin Data/TaskNotes/Archive',
|
||||
];
|
||||
|
||||
// Simulate rapid onChange events (like typing quickly or paste + autocomplete)
|
||||
chunks.forEach(chunk => {
|
||||
settings.archiveFolder = chunk;
|
||||
debouncedSave();
|
||||
});
|
||||
|
||||
// The first save happens immediately with incomplete data
|
||||
expect(saveFn).toHaveBeenCalledTimes(1);
|
||||
|
||||
// The second onChange is blocked by the debounce cooldown
|
||||
// This means the full path never gets saved!
|
||||
expect(settings.archiveFolder).toBe('0 System/004 Plugin Data/TaskNotes/Archive');
|
||||
// But saveFn was only called once, with the first (incomplete) value
|
||||
});
|
||||
});
|
||||
|
||||
describe('Debounce behavior with immediate=false (trailing)', () => {
|
||||
test('debounce with immediate=false should wait for timeout before executing', async () => {
|
||||
const mockFn = jest.fn();
|
||||
const debouncedFn = debounce(mockFn, 100, false);
|
||||
|
||||
debouncedFn('first');
|
||||
expect(mockFn).toHaveBeenCalledTimes(0);
|
||||
|
||||
// Wait for debounce timeout
|
||||
await new Promise(resolve => setTimeout(resolve, 150));
|
||||
|
||||
expect(mockFn).toHaveBeenCalledTimes(1);
|
||||
expect(mockFn).toHaveBeenCalledWith('first');
|
||||
});
|
||||
|
||||
test('debounce with immediate=false should use the last value when called rapidly', async () => {
|
||||
const mockFn = jest.fn();
|
||||
const debouncedFn = debounce(mockFn, 100, false);
|
||||
|
||||
debouncedFn('first');
|
||||
debouncedFn('second');
|
||||
debouncedFn('third');
|
||||
|
||||
// No calls yet
|
||||
expect(mockFn).toHaveBeenCalledTimes(0);
|
||||
|
||||
// Wait for debounce timeout
|
||||
await new Promise(resolve => setTimeout(resolve, 150));
|
||||
|
||||
// Should only call once with the last value
|
||||
expect(mockFn).toHaveBeenCalledTimes(1);
|
||||
expect(mockFn).toHaveBeenCalledWith('third');
|
||||
});
|
||||
|
||||
test('trailing debounce would prevent the folder path truncation bug', async () => {
|
||||
// Simulate settings object
|
||||
const settings = {
|
||||
archiveFolder: '',
|
||||
};
|
||||
|
||||
const saveFn = jest.fn();
|
||||
// Use trailing debounce (immediate=false)
|
||||
const debouncedSave = debounce(saveFn, 100, false);
|
||||
|
||||
// Simulate rapid onChange events
|
||||
const chunks = [
|
||||
'0 System/004 Plugin Data/TaskNotes/Archi',
|
||||
'0 System/004 Plugin Data/TaskNotes/Archive',
|
||||
];
|
||||
|
||||
chunks.forEach(chunk => {
|
||||
settings.archiveFolder = chunk;
|
||||
debouncedSave();
|
||||
});
|
||||
|
||||
// No save yet
|
||||
expect(saveFn).toHaveBeenCalledTimes(0);
|
||||
|
||||
// Wait for debounce
|
||||
await new Promise(resolve => setTimeout(resolve, 150));
|
||||
|
||||
// Should save once with the final complete value
|
||||
expect(saveFn).toHaveBeenCalledTimes(1);
|
||||
expect(settings.archiveFolder).toBe('0 System/004 Plugin Data/TaskNotes/Archive');
|
||||
});
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue