fix: code review improvements — regex, perf, UI, parser, and submission fixes#27
Open
kalinichenko88 wants to merge 31 commits intomasterfrom
Open
fix: code review improvements — regex, perf, UI, parser, and submission fixes#27kalinichenko88 wants to merge 31 commits intomasterfrom
kalinichenko88 wants to merge 31 commits intomasterfrom
Conversation
Inline ``` in row names or comments could prematurely close the regex match, causing incorrect block boundaries. Add multiline flag and ^ anchor so only line-start ``` closes the block. Add tests covering inline backticks, multiple blocks, and boundary cases.
Avoid full doc.toString() + regex + parser rebuild on every keystroke. Widget-dispatched changes are tagged with an annotation and handled via RangeSet.map(). External changes that don't contain fence markers also use map(). Full rebuild only runs when block structure may have changed. Extract changesAffectBlockStructure into a testable helper with 10 tests.
findCurrentPosition used doc.toString() + regex re-scan on every save to locate the widget's block — O(doc size) and fragile with adjacent blocks due to closed interval [from, to] matching the wrong block. Now iterates the StateField's DecorationSet with half-open [from, to) intervals. Uses a late-bound registry to share the field reference without circular imports between tableExtension and TableWidget.
editingValue was initialized once via untrack and never updated when the value prop changed externally (undo, sorting, drag-and-drop). Opening a cell after such changes would show stale data and overwrite the current value on blur. Add $effect to sync editingValue with the prop while not in editing mode.
…-lock.json handling Replaced process.exit(0) calls that could kill pending writeFile operations with conditional skips per file. Also update packages[""] .version in package-lock.json. Includes lint/format fixes for test files and auto-formatted docs.
Lines without | that aren't categories were silently parsed as rows, creating phantom entries in the UI. Now require at least one pipe separator, matching the format the formatter always produces.
Category names ending with : produced :: in the formatted markdown, corrupting the name on each round-trip through parser. Now strip trailing colons before appending the delimiter.
eq() only compared Map values, ignoring keys (CategoryId, RowId). Since nanoid regenerates IDs on each parse, the old widget was reused with stale IDs in its Svelte store. Now compares full entries including keys and row IDs, ensuring fresh state on rebuild. Also replace trimEnd() with regex for ES2018 target compatibility.
Leading-edge debounce could fire the write immediately before the Editable component sets isEditing = true, interrupting active editing. Trailing-edge fires 100ms after the last call, giving state time to settle.
…abled Menu was built once at component init, capturing a stale isDeletingEnabled value. Delete item stayed disabled even after adding a second category. Now create the menu fresh on each right-click.
Local state (checked, name, amount, comment) was initialized once via untrack and never synced from the row prop. Add $effect to keep local state in sync with external prop changes, preventing stale data from overwriting fresh values via the updateRow $effect.
Escape set editingValue back to the original but blur still fired handleOnLeave which called onChange, triggering an unnecessary save cycle. Add a cancelled flag so handleOnLeave skips onChange when Escape already handled the exit.
BREAKING: Plugin ID changed from "obsidian-budget-planner-plugin" to "budget-planner" to comply with Community Plugin submission rules (no "obsidian" in ID, can't end with "plugin"). Also removed "for Obsidian" from description. Existing manual installs need to rename their plugin folder.
Rename title to Budget Planner, remove "Obsidian" references, update install path to budget-planner/. Update CLAUDE.md with incremental decoration updates, parser/formatter improvements, new test files, and constants module documentation. Add formatter test for empty name with zero amount.
Add sync $effect for categoryName prop in CategoryRow to prevent stale name from overwriting external changes (same pattern as Editable and Row fixes). Fix formatter test to exact-match empty name + zero amount behavior. Assert NO_CATEGORY_NAME in parser test.
Narrow release workflow trigger from '**' to 'v*' to ignore non-release tags. Align tsconfig target and lib to ESNext to match module setting and unlock modern type definitions (Vite handles actual compilation).
…on file switch Replace debounced commit with immediate dispatch so the CodeMirror document always reflects the current Svelte store state. Previously, editing multiple cells and switching files lost changes because updateRow never triggered a document sync and the destroy-time flush was unreliable. - Add onTableChange call to updateRow (consistent with all other actions) - Replace debounced commitTableChange with immediate syncToDocument - Add oninput handler to Editable so store stays fresh during typing - Track startValue in Editable for proper Escape revert - Remove redundant commitChange call and unused import from Row - Reorder destroy() to unmount before flush so teardown callbacks propagate final values to the store
With immediate document sync, isSaving is never set to true and commitChange has no callers. Remove both along with all disabled guards that depended on isSaving. Also fix double dispatch on checkbox toggle by letting the $effect handle updateRow instead of calling it explicitly in handleOnCheckboxClick.
- Fix release script to create v-prefixed tags matching workflow trigger - Create Row context menu on each open for consistency with CategoryRow - Remove dead disabled prop from AddRow and Editable after isSaving removal - Simplify syncToDocument and actions onTableChange signature - Update CLAUDE.md with v* tag pattern and synchronous state management docs
DnD refresh was triggered on every store change via queueMicrotask, including checkbox toggles. SortableJS destroy/create cycle on the first <tbody> reset input.checked before Svelte could apply updates. Now DnD refresh only runs when table structure changes (row/category IDs), not on data-only changes. Also switch checkbox input to bind:checked with pointer-events:none for reliable DOM sync.
style(table-row): simplify checkbox input and class binding
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive code review fixes across the codebase:
Critical
RangeSet.map()withwidgetChangeAnnotation(avoids full rebuild on every keystroke)posAtDOM+ regex position scan with decoration set iteration and half-open intervalseditingValue/ Row local state with props on external changes (undo, sort, drag-and-drop)process.exit(0)mid-async in version-bump script; updatepackages[""].versionin package-lock.json|separator in parser to prevent phantom rows::corruption on round-tripMajor
TableWidget.eq()to prevent stale store reuseisEditingis setisDeletingEnabledonChangeon Escape by skipping blur handler after cancelbudget-plannerfor Community Plugin submission complianceMinor
MouseEvent→WheelEventtype inhandleOnWheelversion-bump.js→scripts/version-bump.js)Tests added: 30 new tests (regex, structural change detection, parser, formatter)
Test plan