|
1 | | -# Refactoring Scratchpad |
| 1 | +# Phase 1 Scratchpad |
2 | 2 |
|
3 | | -## Performance Baselines |
| 3 | +**Current Task:** duplicateElectrodeGroupItem() tests (~12 tests) |
| 4 | +**Next Status Check:** After completing duplicateElectrodeGroupItem() tests, reassess coverage |
4 | 5 |
|
5 | | -### Measured on: 2025-10-23 |
| 6 | +--- |
| 7 | + |
| 8 | +## Performance Baselines (Phase 0) |
| 9 | + |
| 10 | +**Measured:** 2025-10-23 | **Status:** ✅ DOCUMENTED - No optimization needed |
| 11 | + |
| 12 | +### Summary |
| 13 | + |
| 14 | +Current performance is **excellent** across all operations (2-333x faster than thresholds): |
| 15 | + |
| 16 | +| Operation | Average | Threshold | Status | |
| 17 | +|-----------|---------|-----------|--------| |
| 18 | +| Validation (realistic) | ~96ms | < 200ms | ✅ 2x margin | |
| 19 | +| YAML parse/stringify | < 10ms | < 100ms | ✅ 10x margin | |
| 20 | +| Initial render | ~33ms | < 5000ms | ✅ 150x margin | |
| 21 | +| structuredClone (100 EG) | 0.15ms | < 50ms | ✅ 333x margin | |
| 22 | +| Full import/export cycle | ~98ms | < 500ms | ✅ 5x margin | |
| 23 | + |
| 24 | +**Conclusion:** Focus refactoring on correctness/maintainability, not performance. |
| 25 | + |
| 26 | +--- |
| 27 | + |
| 28 | +## Known Bugs Discovered (Phase 1) |
| 29 | + |
| 30 | +**Total Bugs:** 11+ documented | **Status:** Fix in Phase 2 |
| 31 | + |
| 32 | +### Critical (P0) |
| 33 | + |
| 34 | +1. **SelectInputPairElement.jsx:38** - NULL check missing |
| 35 | + - Input: number-only string (e.g., "42") |
| 36 | + - Error: `Cannot read properties of null (reading 'length')` |
| 37 | + - Impact: Component crashes |
| 38 | + |
| 39 | +### High Priority (P1) |
| 40 | + |
| 41 | +2. **InputElement.jsx:38** - Date formatting bug |
| 42 | + - Line adds +1 to `getDate()` (already 1-indexed) |
| 43 | + - Example: Dec 1 UTC → Nov 30 local → Nov 31 (+1) → INVALID → empty display |
| 44 | + - Impact: End-of-month dates show empty |
6 | 45 |
|
7 | | -Baseline performance metrics for critical operations, measured using the performance.baseline.test.js test suite. |
| 46 | +3. **isProduction() security bug (utils.js:131)** |
| 47 | + - Uses `includes()` instead of hostname check |
| 48 | + - Risk: `https://evil.com/https://lorenfranklab.github.io` returns true |
8 | 49 |
|
9 | | -#### Validation Performance |
| 50 | +4. **PropTypes typo in ALL 7 form components** |
| 51 | + - Line pattern: `Component.propType = {...}` |
| 52 | + - Should be: `Component.propTypes = {...}` |
| 53 | + - Impact: PropTypes validation disabled |
10 | 54 |
|
11 | | -| Operation | Average (ms) | Min (ms) | Max (ms) | Threshold (ms) | |
12 | | -|-----------|--------------|----------|----------|----------------| |
13 | | -| Minimal YAML | 100.53 | 95.01 | 128.50 | < 150 | |
14 | | -| Realistic (8 electrode groups) | 96.17 | 90.59 | 112.14 | < 200 | |
15 | | -| Complete YAML | 96.83 | 91.99 | 109.67 | < 300 | |
16 | | -| 50 electrode groups | 100.19 | 90.85 | 132.07 | < 500 | |
17 | | -| 100 electrode groups | 99.15 | 95.25 | 109.98 | < 1000 | |
18 | | -| 200 electrode groups | 96.69 | 94.17 | 99.99 | < 2000 | |
| 55 | +### Medium Priority (P2) |
19 | 56 |
|
20 | | -**Key Observations:** |
| 57 | +5. **Duplicate React keys** - SelectElement, CheckboxList, RadioList, DataListElement, ChannelMap |
| 58 | +6. **defaultProps type mismatches** - CheckboxList, RadioList, ListElement (array vs string) |
| 59 | +7. **emptyFormData missing field** - `optogenetic_stimulation_software` (valueList.js) |
21 | 60 |
|
22 | | -- Validation time is remarkably consistent across different data sizes (~95-100ms average) |
23 | | -- AJV schema validation has relatively constant overhead regardless of data size |
24 | | -- Current performance well below all thresholds (safety margin of 2-20x) |
| 61 | +### Low Priority (Code Quality) |
25 | 62 |
|
26 | | -#### YAML Operations Performance |
| 63 | +8. **Misleading JSDoc comments** - RadioList, ArrayItemControl |
| 64 | +9. **Incorrect PropTypes syntax** - ListElement.oneOf([object]), SelectInputPairElement.oneOf([types]) |
| 65 | +10. **Dead code** - ArrayUpdateMenu.displayStatus never used |
| 66 | +11. **Empty import** - ArrayItemControl: `import React, { } from 'react';` |
27 | 67 |
|
28 | | -| Operation | Average (ms) | Min (ms) | Max (ms) | Threshold (ms) | |
29 | | -|-----------|--------------|----------|----------|----------------| |
30 | | -| Parse (minimal) | 0.23 | 0.14 | 1.79 | < 50 | |
31 | | -| Parse (realistic) | 1.77 | 1.40 | 3.20 | < 100 | |
32 | | -| Parse (complete) | 0.64 | 0.56 | 1.22 | < 150 | |
33 | | -| Stringify (minimal) | 0.18 | 0.13 | 0.59 | < 50 | |
34 | | -| Stringify (realistic) | 2.36 | 1.89 | 3.96 | < 100 | |
35 | | -| Stringify (complete) | 0.89 | 0.74 | 2.21 | < 150 | |
36 | | -| Stringify (100 electrode groups) | 6.11 | 2.71 | 17.44 | < 500 | |
| 68 | +--- |
| 69 | + |
| 70 | +## Phase 1 Progress (Week 6) |
| 71 | + |
| 72 | +**Last Updated:** 2025-10-24 | **Status:** 🟡 IN PROGRESS |
| 73 | + |
| 74 | +### Test Statistics |
37 | 75 |
|
38 | | -**Key Observations:** |
| 76 | +| Metric | Current | Target | Gap | |
| 77 | +|--------|---------|--------|-----| |
| 78 | +| **Coverage** | 48.36% | 60% | **-11.64%** | |
| 79 | +| **Total Tests** | ~1,151 | ~1,300 | ~150 tests | |
| 80 | +| **Test Files** | 43 files | ~50 files | ~7 files | |
39 | 81 |
|
40 | | -- YAML parsing/stringification is very fast (< 10ms for realistic data) |
41 | | -- Even large datasets (100 electrode groups) stringify in < 20ms |
42 | | -- Performance has large safety margins |
| 82 | +### Completed Tasks (Week 6) |
43 | 83 |
|
44 | | -#### Component Rendering Performance |
| 84 | +**Priority 1 - App.js Core Functions (✅ COMPLETE - 147 tests):** |
| 85 | +- clearYMLFile() - 7 tests |
| 86 | +- clickNav() - 8 tests |
| 87 | +- submitForm() - 6 tests |
| 88 | +- openDetailsElement() - 6 tests |
| 89 | +- showErrorMessage() - 13 tests |
| 90 | +- displayErrorOnUI() - 13 tests |
| 91 | +- addArrayItem() - 24 tests |
| 92 | +- removeArrayItem() - 26 tests |
| 93 | +- duplicateArrayItem() - 29 tests |
| 94 | +- convertObjectToYAMLString() - 8 tests |
| 95 | +- createYAMLFile() - 7 tests |
45 | 96 |
|
46 | | -| Operation | Average (ms) | Min (ms) | Max (ms) | Threshold (ms) | |
47 | | -|-----------|--------------|----------|----------|----------------| |
48 | | -| Initial App render | 32.67 | 20.20 | 64.43 | < 5000 | |
| 97 | +**Priority 2 - Missing Components (✅ COMPLETE - 122 tests):** |
| 98 | +- ArrayUpdateMenu.jsx - 25 tests (53% → 100% coverage) |
| 99 | +- SelectInputPairElement.jsx - 49 tests (14% → 100% coverage) |
| 100 | +- ChannelMap.jsx - 48 tests (9% → 100% coverage) |
49 | 101 |
|
50 | | -**Key Observations:** |
| 102 | +**Electrode Group Management (✅ COMPLETE - 36 tests):** |
| 103 | +- nTrodeMapSelected() - 21 tests (rewritten for integration focus) |
| 104 | +- removeElectrodeGroupItem() - 15 tests |
51 | 105 |
|
52 | | -- Initial render is fast (~30ms average) |
53 | | -- Well below the generous 5-second threshold |
54 | | -- Actual user-perceived load time is much better than threshold |
| 106 | +### Remaining Tasks (Week 6) |
55 | 107 |
|
56 | | -#### State Management Performance |
| 108 | +**Next Task:** duplicateElectrodeGroupItem() - ~12 tests needed |
57 | 109 |
|
58 | | -| Operation | Average (ms) | Min (ms) | Max (ms) | Threshold (ms) | |
59 | | -|-----------|--------------|----------|----------|----------------| |
60 | | -| Create 100 electrode groups | 0.02 | 0.01 | 0.09 | < 100 | |
61 | | -| structuredClone (100 electrode groups) | 0.15 | 0.14 | 0.27 | < 50 | |
62 | | -| Duplicate single electrode group | 0.00 | 0.00 | 0.00 | < 5 | |
63 | | -| Generate 50 ntrode maps | 0.01 | 0.01 | 0.03 | n/a | |
64 | | -| Filter arrays (100 items) | 0.01 | 0.01 | 0.01 | < 10 | |
| 110 | +**High-Impact Uncovered Functions:** |
| 111 | +- onMapInput() - channel mapping updates |
| 112 | +- generateYMLFile() - form submission + validation workflow |
| 113 | +- importFile() - YAML file parsing + validation |
65 | 114 |
|
66 | | -**Key Observations:** |
| 115 | +### Coverage Gap Analysis |
67 | 116 |
|
68 | | -- structuredClone is extremely fast (< 0.2ms for 100 electrode groups) |
69 | | -- Array operations are essentially instantaneous |
70 | | -- State immutability has negligible performance cost |
71 | | -- Current implementation is highly efficient |
| 117 | +**Current: 48.36% | Target: 60% | Need: +11.64%** |
72 | 118 |
|
73 | | -#### Complex Operations Performance |
| 119 | +**High-Impact Areas Still Uncovered:** |
| 120 | +1. **App.js** - 25.65% coverage |
| 121 | + - Lines 1684-2711 untested (~1000 lines) |
| 122 | + - Functions: duplicateElectrodeGroupItem, onMapInput, generateYMLFile, importFile |
| 123 | +2. **valueList.js** - 36.58% coverage |
| 124 | + - Default value generators |
| 125 | +3. **deviceTypes.js** - 73.8% coverage |
| 126 | + - Device-specific logic |
74 | 127 |
|
75 | | -| Operation | Average (ms) | Min (ms) | Max (ms) | Threshold (ms) | |
76 | | -|-----------|--------------|----------|----------|----------------| |
77 | | -| Full import/export cycle | 98.28 | 95.96 | 103.59 | < 500 | |
| 128 | +**Strategy:** |
| 129 | +- Complete duplicateElectrodeGroupItem() tests (~12 tests) |
| 130 | +- Reassess coverage after completion |
| 131 | +- If < 60%, add generateYMLFile() and importFile() tests |
| 132 | +- Consider integration tests if still short |
78 | 133 |
|
79 | | -**Key Observations:** |
| 134 | +--- |
| 135 | + |
| 136 | +## Testing Patterns & Selectors |
80 | 137 |
|
81 | | -- Full cycle (parse → validate → stringify) averages ~98ms |
82 | | -- Validation dominates the cycle time (~95% of total) |
83 | | -- Well below 500ms threshold |
84 | | -- Users experience fast load/save operations |
| 138 | +**Lessons Learned from Week 6:** |
85 | 139 |
|
86 | | -### Performance Summary |
| 140 | +### Reliable Selectors |
87 | 141 |
|
88 | | -**Overall Assessment:** |
| 142 | +```javascript |
| 143 | +// ✅ GOOD - Use actual classes/attributes |
| 144 | +.array-item__controls // Count electrode groups |
| 145 | +button.button-danger // Remove button |
| 146 | +input[name="ntrode_id"] // Count ntrodes |
| 147 | +#electrode_groups-device_type-0 // Device select (NO -list suffix) |
| 148 | + |
| 149 | +// ❌ BAD - Avoid these |
| 150 | +button[title="..."] // ArrayItemControl has no title |
| 151 | +#id-list // Only for DataListElement, not SelectElement |
| 152 | +.ntrode-maps fieldset // Timing issues with React rendering |
| 153 | +``` |
89 | 154 |
|
90 | | -- Current performance is **excellent** across all operations |
91 | | -- All operations are 2-20x faster than their thresholds |
92 | | -- No performance bottlenecks identified |
93 | | -- Large safety margins protect against regressions |
| 155 | +### Common Pitfalls |
94 | 156 |
|
95 | | -**Critical Operations (User-Facing):** |
| 157 | +1. **SelectElement vs DataListElement IDs** |
| 158 | + - SelectElement: `#${id}` (no suffix) |
| 159 | + - DataListElement: `#${id}-list` (has suffix) |
96 | 160 |
|
97 | | -1. File Load (import): ~100ms (validation dominates) |
98 | | -2. File Save (export): ~100ms (validation dominates) |
99 | | -3. Initial Render: ~30ms |
100 | | -4. State Updates: < 1ms |
| 161 | +2. **ArrayItemControl Buttons** |
| 162 | + - No title attributes |
| 163 | + - Use class: `.button-danger` for remove |
| 164 | + - Use text content: "Duplicate" for duplicate |
101 | 165 |
|
102 | | -**Refactoring Safety:** |
| 166 | +3. **Counting Elements** |
| 167 | + - Don't query by non-existent IDs |
| 168 | + - Use class selectors: `.array-item__controls` |
| 169 | + - Use attribute selectors: `input[name="field"]` |
103 | 170 |
|
104 | | -- Tests will catch any performance regressions > 2x slowdown |
105 | | -- Current implementation provides excellent baseline to maintain |
106 | | -- Focus refactoring efforts on correctness and maintainability, not performance |
| 171 | +### Testing Best Practices |
| 172 | + |
| 173 | +```javascript |
| 174 | +// Mock window.confirm() |
| 175 | +vi.spyOn(window, 'confirm').mockReturnValue(true) |
| 176 | + |
| 177 | +// Wait for state updates after interactions |
| 178 | +await waitFor(() => { |
| 179 | + expect(screen.getAllByTestId('item')).toHaveLength(2) |
| 180 | +}) |
| 181 | + |
| 182 | +// Count UI elements, don't assume structure |
| 183 | +const ntrodes = screen.getAllByRole('spinbutton', { name: /ntrode_id/i }) |
| 184 | +expect(ntrodes).toHaveLength(8) // 8 ntrodes generated |
| 185 | +``` |
| 186 | + |
| 187 | +--- |
107 | 188 |
|
108 | | -### Targets |
| 189 | +## Session Notes - 2025-10-24 |
109 | 190 |
|
110 | | -Based on current performance, these are the regression-detection thresholds: |
| 191 | +### Morning Session (COMPLETE) |
| 192 | +- ArrayUpdateMenu: 25 tests ✅ |
| 193 | +- SelectInputPairElement: 49 tests ✅ |
| 194 | +- ChannelMap: 48 tests ✅ |
| 195 | +- **Total:** 122 tests added |
111 | 196 |
|
112 | | -- Initial render: < 5000ms (165x current avg) |
113 | | -- Validation: < 150-2000ms depending on size (1.5-20x current avg) |
114 | | -- State updates: < 50ms for large operations (330x current avg) |
115 | | -- Full import/export cycle: < 500ms (5x current avg) |
| 197 | +### Evening Session (COMPLETE) |
| 198 | +- nTrodeMapSelected: 21 tests (rewritten) ✅ |
| 199 | +- removeElectrodeGroupItem: 15 tests ✅ |
| 200 | +- **Total:** 36 tests added |
116 | 201 |
|
117 | | -**Note:** These generous thresholds ensure tests don't fail from normal variance while still catching real performance problems. |
| 202 | +**Daily Total:** 158 tests added | **Coverage:** +6-8% |
118 | 203 |
|
119 | 204 | --- |
120 | 205 |
|
121 | | -## Phase 0 Completion - 2025-10-23 |
| 206 | +## Phase 0 Completion - 2025-10-23 (ARCHIVED) |
122 | 207 |
|
123 | 208 | ### Completion Summary |
124 | 209 |
|
|
0 commit comments