Repository navigation
Fix frontend correctness & robustness issues found in code review - #13
Open
b-eyselein wants to merge 15 commits into
Open
b-eyselein wants to merge 15 commits into
b-eyselein wants to merge 15 commits into
Conversation
onPartChanged only forwarded the parsed word to the parent when both the pre-lb and post-lb halves parsed successfully; otherwise it just logged to console and left the parent's state untouched. If the parent had a valid parse from an earlier edit, it kept showing that stale result with a working submit button even though the currently visible textarea content was actually invalid - submitting would silently save the old value. Now propagates a proper error result so the parent shows the invalid state and withholds the submit button, matching how WordContentEditor already handles single-part invalid input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
isoDate() parses a datetime-local form value as local time and converts it to UTC on submit, but the three date fields (event date, merge date, nested-merge date) populated their defaultValue with a raw slice of the stored UTC ISO string, displayed verbatim with no local-time conversion. Submitting the form without touching the date field re-interpreted the displayed UTC value as local time, silently shifting the stored timestamp by the browser's UTC offset. Added toDatetimeLocalValue() to convert to local time when populating the field, so the round-trip through isoDate() is consistent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
deleteMorphology unset the mrp attribute directly without removing the corresponding Hurrian concordance attestation, unlike toggleMorphology and updateMorphology which both register a removal (deferred until the global update button is clicked). For a Hurrian word with a selected analysis already in the concordance, deleting the morphology removed the XML attribute but left a stale attestation referencing an analysis that no longer exists. Now registers the same deferred removeAttestation call as the sibling functions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
…riants isSelected() only handled 2 of the 6 MorphologicalAnalysis variants and defaulted to false for the rest, unlike the exhaustive isSelected in nodePostprocessing.ts. This function gates whether a selected Hurrian analysis is added to the concordance, so for a word whose selected analysis was one of the 4 unhandled variants (e.g. SingleMorphAnalysisWithMultiEnclitics), it silently never got added. Mirrored the exhaustive match from nodePostprocessing.ts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
deleteMarkedNodes (bulk delete) never set lbCuneiformDirtyPath, unlike single-node deleteNode and initiateInsert, which both recompute the preceding <lb>'s cuneiform when a 'w' node is added/removed. Bulk- deleting one or more word nodes via multi-select left the preceding <lb>'s cached rendered-cuneiform attribute stale, with no refetch triggered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
…ting reCountNodeNumbers mutated XmlElementNode attributes in place on whatever tree it was given, including the live React-state tree passed directly from XmlDocumentEditor's state.rootNode via writeXml. Because object identity didn't change, React never re-rendered to reflect the new numbering, and any other code holding a reference to the same pre-renumbering tree (e.g. a diff view) could see inconsistent state. It now returns a renumbered copy, and writeXml uses that copy for serialization instead of mutating its input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
The lb-cuneiform refetch ran unconditionally in the component's render body instead of inside a useEffect, with no .catch. Any unrelated re-render before the promise resolved could re-fire a duplicate fetch for the same path, and a rejected fetch left lbCuneiformDirtyPath set forever, so the component kept re-issuing the same failing fetch on every render with no visible error. Moved into a useEffect keyed on the dirty path, and cleared the dirty flag (with a logged error) if the fetch fails. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
addLine reassigned firstLines/secondLines to the return value of splice(), which is the array of *removed* elements - empty, since this is an insert (deleteCount 0), not the mutated array. splice() already mutates in place, so clicking "add line" on either side of the document merger wiped the entire column instead of inserting one line. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
leftTxtId is module-level mutable state written in mergeLine and read in iterateTxtId, but was never reset at the start of a merge. If a merge reached an unmatched left-only line before any mergeLine call had executed in that session, leftTxtId still held a value left over from a previous, unrelated merge performed earlier on the same page, silently populating the newly exported line's txtid attribute with stale data. Reset it at the top of mergeLines(), the single entry point for a merge operation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
onExport downloaded the exported XML before awaiting the approval mutation, and the mutation's error was destructured but commented out - only console.error'd, never shown in the UI. If the mutation then failed (network error, session expiry, FK violation), the executive editor already had the downloaded file and saw no error, and could believe the document was approved when server-side approval never persisted. Now awaits the mutation first and only downloads on success, and renders the mutation error if it occurs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
The Yup schema validated password and passwordRepeat independently
(length only) and submitted both values without comparing them, unlike
ResetPasswordForm which checks the two match before submitting. A user
who mistyped the repeat field got an account created with a password
that differed from what they believed they confirmed, with no error
shown. Added a .oneOf([yup.ref('password')]) check on passwordRepeat,
surfaced through the existing ErrorMessage for that field.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
XmlComparatorContainer colors the left file's name red (leftColorClass) and the right file's name green (rightColorClass). diffLines(left, right) marks a line "added" when it exists only in the right file and "removed" when it exists only in the left file, but DiffDisplay had these swapped, so every diff rendered with right-only lines in red and left-only lines in green - backwards from the legend above it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
The mutation error was rendered with redButtonClasses (solid red button styling) instead of redMessageClasses (the message-banner style used identically by LoginForm, RegisterForm, ForgotPasswordForm, and ResetPasswordForm), producing a visually broken/inconsistent error display compared to every other form in the app. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
loadFromLocalStorage called JSON.parse on stored data with no try/catch, run synchronously during Redux store initialization for every persisted slice, including the user auth slice. A corrupted or non-JSON localStorage entry (partial write, manual edit, browser extension interference) threw during configureStore's initial state construction, crashing the entire app on load with no fallback - the user couldn't even reach the login page without manually clearing localStorage. Now falls back to the default value on a parse failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
Home.tsx and UserManagement.tsx both invoked a GraphQL lazy query directly in the render body (gated by `!query.called`) instead of inside a useEffect. Running a query trigger as a side effect during render violates React's render-purity rules; under React 18 StrictMode's double-invocation of component bodies in development, or any re-render occurring before Apollo's `called` flag propagates back into state, the query could fire more than once for the same mount. Moved both into a mount-only useEffect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3
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.
Fixes for 15 issues found during a review of the frontend (
ui/src/), one commit per finding. No security issues — all findings are correctness/robustness bugs, several involving silent data loss.XML editor (
ui/src/xmlEditor/)faa9bf5f) — editing one half of a word spanning a line break into an invalid state no longer silently leaves the parent showing a stale, submittable result.742b5a0c) — event/merge dates inHeaderEditorno longer silently drift by the browser's UTC offset on save.064b3542) — deleting a morphological analysis now removes its concordance attestation, matching the sibling update/toggle functions.isSelected()variant match (e2d75cee) — now exhaustive across all 6MorphologicalAnalysisvariants; previously silently dropped 4 of them from the concordance.16c95bc3) — multi-select delete now marks the preceding<lb>dirty for refetch, matching single-node delete.9241ec6b) —reCountNodeNumbersnow returns a renumbered copy instead of mutating the shared React-state tree.402207e9) — moved into auseEffect, with failure handling so it can't loop forever on a rejected fetch.Rest of frontend
b863f09b) —Array.prototype.splicemutates in place; the code was discarding the array by reassigning it tosplice's return value.leftTxtIdleaking across merges (8247f221) — reset at the start of each merge instead of persisting as module state.e89d8d11) —DocumentApprovalnow awaits the approval mutation before downloading, and surfaces mutation errors in the UI.8047e413) — added the same match checkResetPasswordFormalready has.ca1a3b9c) —XmlComparatornow colors added/removed lines consistently with the left/right legend.7d64f426) —CreateManuscriptFormnow uses the same message-banner class as every other form.93a0304a) —loadFromLocalStoragenow falls back to the default value on a parse failure instead of crashing store initialization.13b4536f) —Home.tsxandUserManagement.tsxnow trigger their initial lazy query from a mount-onlyuseEffectinstead of directly in the render body.Testing
npm run lint— clean.npm run build— compiles successfully (full TypeScript type-check included).🤖 Generated with Claude Code
https://claude.ai/code/session_01528tdgcQt4KEUmzGX9RAm3