feat(gov-proposals): wizard + status + drafts panel (PR 8 frontend)#12
feat(gov-proposals): wizard + status + drafts panel (PR 8 frontend)#12
Conversation
End-to-end UI for authoring a Syscoin governance proposal, paired
with the sysnode-backend PR 8 surface (/gov/proposals/*).
Scope:
- lib/proposalService.js: thin transport for the /gov/proposals
endpoints with shape assertions, retry-after propagation, and
stable error codes.
- lib/paliProvider.js: Pali dApp detection + typed request wrapper
+ EIP-1193 error normalisation. PSBT-based payWithOpReturn is
intentionally stubbed with 'pali_psbt_builder_not_wired' so the
UI falls back to the manual Syscoin-Qt path; live Pali interop
is deferred to a future PR.
- lib/proposalForm.js: pure validation/normalisation helpers
(name/url/address/amount/epoch rules, 512B payload estimation,
dirty-check, draft/prepare body builders).
- components/UnsavedChangesModal: accessible Save/Discard/Cancel
prompt wired into the wizard's leave-guard (beforeunload +
history.block).
- components/ProposalsCreatedPanel: compact Drafts (N) toggle +
in-flight submissions list above the governance watchlist for
authenticated users.
- pages/NewProposal: four-step wizard (Basics / Payment / Review /
Submit) with server-backed drafts, idempotent /prepare handoff,
explicit 150 SYS burn warning, and a CLI command + OP_RETURN
payload for the manual collateral path.
- pages/ProposalStatus: live status page with adaptive polling
(fast while awaiting_collateral, slow in prepared, stopped in
terminal states) and per-status copy for success/failure.
- App.css: wizard, panel, modal, and status styles reusing the
existing design tokens.
- App.js: new /governance/new and /governance/proposal/:id routes
guarded by PrivateRoute.
Testing: - 43 suites / 624 frontend tests pass; new tests cover wizard
flow end-to-end (prepare + attach-collateral), status page
(awaiting / submitted / failed / delete), drafts panel (CTA,
drafts toggle, in-flight list, delete), unsaved-changes modal
(focus, escape, overlay, loading, error), and paliProvider +
proposalService + proposalForm units.
Made-with: Cursor
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e8dc56e10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| You haven't attached a collateral TXID yet. Return to | ||
| the wizard to pay the 150 SYS burn fee. | ||
| </p> | ||
| <Link to="/governance/new" className="button button--primary"> |
There was a problem hiding this comment.
When submission.status === 'prepared', this branch only links to /governance/new, but the wizard can attach collateral only when its in-memory prepared envelope was set in the same session after prepare(). If a user opens a prepared item from the in-flight list or reloads /governance/proposal/:id, clicking Continue lands on a blank wizard with no way to attach the existing submission’s TXID, so the submission is effectively stuck unless they recreate the proposal flow.
Useful? React with 👍 / 👎.
| if (!submission) return undefined; | ||
| const { status } = submission; | ||
| if (status === 'submitted' || status === 'failed') return undefined; | ||
| const delay = status === 'awaiting_collateral' ? POLL_FAST_MS : POLL_SLOW_MS; | ||
| timerRef.current = window.setTimeout(() => { | ||
| load(); | ||
| }, delay); |
There was a problem hiding this comment.
The adaptive polling timeout is scheduled only from the effect keyed on submission, but load()’s error path updates error without changing submission. If any poll request fails once (temporary network hiccup, 5xx, etc.), no new timeout is scheduled and status updates stop permanently for non-terminal submissions until the user refreshes the page.
Useful? React with 👍 / 👎.
| history.replace({ | ||
| pathname: history.location.pathname, | ||
| search: `?${params.toString()}`, | ||
| }); |
There was a problem hiding this comment.
In the first-save path, history.replace runs before dispatch({ type: 'mark_saved' }), so the route change occurs while dirty is still true. Because history.block is active for dirty state, this replace can be blocked and trigger the unsaved-changes modal, and the ?draft=<id> query may not be applied, which breaks the intended reload/resume behavior for newly created drafts.
Useful? React with 👍 / 👎.
…ft-save ordering) Three review findings from Codex on PR #12 (frontend): P1a (ProposalStatus.js): the prepared-state branch only linked to /governance/new, but that wizard starts empty — a user who reloads /governance/proposal/:id or opens a prepared submission from the in-flight list had no way to attach the existing submission's TXID without recreating the whole flow. Fix: render an inline "Attach collateral" section on the status page itself when status === 'prepared'. It surfaces the OP_RETURN push hex (computed from proposalHash — big-endian display → little-endian bytes), the 150 SYS burn amount, a 64-hex TXID input, and calls attachCollateral directly. The row flips to awaiting_collateral in place and the dispatcher takes over. Tests cover: OP_RETURN reversal (trailing "ff" in hash → leading "ff" in push), malformed-TXID inline error with no RPC call, and the full happy-path transition. P1b (ProposalStatus.js): the polling effect was keyed only on `submission`, but load()'s error path updates `error` without touching `submission`, so a single transient 5xx / network blip cancelled the scheduler and froze status updates until manual reload. Fix: include `error` in the effect's deps and keep the slow cadence when submission is null-but-error-set. Regression test with fake timers: three poll calls (live → error → live) bump the conf counter 2/6 → 4/6 as expected. P2 (NewProposal.js): first-save of a draft called history.replace (to add ?draft=<id>) before dispatch({ type: 'mark_saved' }), but history.block was active for dirty state — the replace could either pop the unsaved-changes modal on a flow that had actually succeeded, or be swallowed by getUserConfirmation, leaving the new id out of the URL and breaking reload-to-resume. Fix: mark the reducer clean BEFORE the history.replace so the block gate sees a clean tree. Full FE suite green (43 suites / 628 tests). Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e109755455
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| history.replace({ | ||
| pathname: history.location.pathname, | ||
| search: `?${params.toString()}`, | ||
| }); |
There was a problem hiding this comment.
When creating a new draft, dispatch({ type: 'mark_saved' }) does not synchronously remove the active history.block guard in React 18, so the immediately-following history.replace(...?draft=<id>) can still be intercepted as a dirty-leave transition. In that case the unsaved-changes modal reopens and the URL may stay without the draft id unless the user goes through an extra modal cycle, which breaks the expected “Save draft” flow for first-time saves.
Useful? React with 👍 / 👎.
| // If the modal is already visible with a pending nav, accept it. | ||
| if (leaveModal.open && leaveModal.pending) return true; | ||
| setLeaveModal({ |
There was a problem hiding this comment.
The route blocker currently returns true for any subsequent transition whenever the modal is open, not just for the originally blocked target. If the user triggers another navigation source while the modal is visible (for example browser back/forward), navigation is allowed without an explicit Save/Discard choice, which can drop unsaved edits.
Useful? React with 👍 / 👎.
Codex PR #12 round 2 flagged two navigation-guard issues in the NewProposal wizard; both stem from the same underlying fact that React 18 batches state updates, so a dispatch() doesn't synchronously flip the `dirty` flag the history.block callback is observing. P2 — first-save URL bump blocked by stale `dirty` dispatch({ type: 'mark_saved' }) followed immediately by history.replace('?draft=<id>') was itself the leaving transition the block was supposed to guard. Because `dirty` was still true at replace-time, the block either re-popped the unsaved-changes modal on a flow that actually succeeded or dropped the draft id from the URL, breaking reload-to-resume. Fix: new `allowedPathRef` pre-authorises this one specific internal replace. The block callback reads the ref and lets exactly that transition through, clears the ref, and continues to prompt for everything else. dispatch still runs so the NEXT render tears the block down cleanly. P3 — whole-app allow-list while modal open The previous `leaveModal.open && leaveModal.pending → true` shortcut allowed ANY transition through while the modal was up, not just the one the user clicked Save/Discard for. A browser back/forward or a react-router push from elsewhere (e.g. SessionExpiredBanner) would sneak past and drop the edits. Fix: new `pendingPathRef` is populated only in the modal's save / discard handlers with the exact target the user confirmed. The block callback checks for an exact string match; any other transition while the ref is live gets re-prompted. Test added The existing "saves a new draft and reflects id in URL" test now also asserts (a) the unsaved-changes modal does NOT appear and (b) the MemoryRouter location actually ends at /governance/new?draft=7. A small LocationDisplay helper surfaces the current MemoryRouter location into the DOM so RTL can assert on it. All 6 NewProposal tests green. Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c0d3e9dc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .then((d) => { | ||
| if (cancelled) return; | ||
| const loaded = fromDraft(d); | ||
| dispatch({ type: 'replace', form: loaded, baseline: loaded }); | ||
| setDraftId(d.id); |
There was a problem hiding this comment.
This effect always replaces the local form with the server copy when ?draft= is present. After the first save, history.replace adds ?draft=<id>, which triggers getDraft again; if the user starts typing before that request resolves, dispatch({ type: 'replace', ... }) overwrites those new edits and silently loses input. Add a guard to skip the replace when the same draft is already loaded and local state has diverged (or avoid refetching on the internal URL sync path).
Useful? React with 👍 / 👎.
| payment_amount: sats | ||
| ? Number(form.paymentAmount).toString() | ||
| : '0', |
There was a problem hiding this comment.
estimatePayloadBytes converts paymentAmount through Number(...).toString(), which turns large valid decimal inputs into scientific notation (or Infinity). That can undercount payload bytes by hundreds of characters, so the review-step size meter and payloadBytes > MAX_DATA_SIZE gate may allow proposals that the backend rejects as oversized. Use the validated decimal string (or sats-derived canonical decimal) directly for byte estimation to keep the client-side limit check accurate.
Useful? React with 👍 / 👎.
| const body = prepareBodyFromForm(form, { | ||
| draftId: draftId || undefined, | ||
| consumeDraft: true, | ||
| }); |
There was a problem hiding this comment.
Prepare is sent with consumeDraft: true, so the server deletes the draft row, but the wizard keeps ?draft=<id> in the URL after success. If the user reloads from Submit, the mount effect tries getDraft(<deleted id>), surfaces a load error, and drops them out of the prepared flow even though a submission already exists. After a successful prepare, remove the draft query param (or redirect to the status route) to keep reload/resume working.
Useful? React with 👍 / 👎.
Adds a minimal GitHub Actions workflow that installs with `npm ci`, runs `npm run test:ci`, and then does a `npm run build` smoke test on Node 20 and Node 22 (matches the `engines: ">=20 <24"` range in package.json). The build step catches ESM/CJS interop regressions in new deps that the unit suite won't see, at the cost of a couple of minutes per matrix leg — worth it for a CRA app where a broken build can't be rolled back in the browser. concurrency: ci-<workflow>-<ref> with cancel-in-progress so a force-push supersedes the previous run rather than queueing. Made-with: Cursor
Made-with: Cursor
`sysnode-info` is a deployed CRA application, not a published npm
library, so pinning the full resolved dependency tree is the
correct default:
- Reproducible builds. package.json ranges let npm re-resolve a
different transitive tree on every run; without a lockfile, CI
and developer machines can silently disagree.
- `npm ci` is deterministic, ~2–3× faster than `npm install`, and
fails loudly if the lockfile and package.json drift — exactly
what we want CI to do.
- Supply-chain posture. The lockfile pins an integrity hash for
every resolved tarball, so a compromised registry publish can't
quietly replace a transitive dep.
- Rollback. `git revert` on a lockfile commit gives a bit-identical
old tree instead of "whichever resolver tree npm happened to
pick that day".
Concretely:
- `.gitignore`: drop the `package-lock.json` line.
- Check in the current lockfile.
- `.github/workflows/ci.yml`: flip the install step from
`npm install` back to `npm ci` and re-enable
`actions/setup-node`'s npm cache (it needs a tracked lockfile).
`sysnode-backend` already tracks its lockfile; this brings the two
repos under the same convention.
Made-with: Cursor
The new FE CI workflow runs react-scripts build, which treats ESLint warnings as errors under CI=true. This surfaced two classes of issues: 1. BigInt / globalThis were reported as undefined. CRA's react-app ESLint preset pins parserOptions to an older ecmaVersion that doesn't register ES2020+ globals. Fix: add env.es2021 to the shared eslintConfig (covers BigInt, globalThis, WeakRef, etc). 2. Redundant inline /* global BigInt */ banners in voteSigner.js and wif.js now conflict with the env-provided global and trigger no-redeclare. Removed the banners (duplicate of env declaration). Also fixed two pre-existing, unrelated lint warnings that the new build step surfaced, both with zero semantic change: - ProposalVoteModal.js: hoist `proposal && proposal.Key` out of the useEffect dep array into a named `proposalKey` const (react-hooks/ exhaustive-deps wants a stable identifier in the deps list; the value and render-to-render behavior are identical). - normalize.js: `[A-Za-z0-9._%+\-]` → `[A-Za-z0-9._%+-]`. The escape on a trailing `-` in a character class is unnecessary; both forms match the exact same set. Verified locally on Node 20.19.4: - CI=true npm run build -> Compiled successfully - CI=true npm run test:ci -> 43 suites / 628 tests passing Made-with: Cursor
Three independent fixes to the governance proposal wizard, each
covering a separate code path the round-3 review flagged:
P1 — Draft refetch clobbering in-progress edits
After createDraft() succeeds, the wizard calls setDraftId(id)
AND history.replace({ ?draft=<id> }). Without a guard, the URL
change re-triggers the draft-load effect, which re-fetches the
draft and dispatches `replace` with the server echo — silently
overwriting any edits the user typed during the round trip.
The load effect now skips the fetch when local `draftId`
already matches `?draft=<id>` (our own URL sync). Cold loads
still fetch because `draftId` starts as null.
P2 — estimatePayloadBytes Number() coercion
Previously used `Number(paymentAmount).toString()`, which
collapses "1234567890123.12345678" into "1.234...e+21". That
undercounted the payload by hundreds of characters, so a
proposal the backend's canonical emitter would reject as
oversized could slip through the 512-byte client-side gate.
Now formats via satsStringToSys(sats), which mirrors the
backend's formatSysAmount (BigInt division + fractional
padding) exactly — so the client gate reflects what the
backend will actually serialize.
P2 — Clear consumed ?draft=<id> after prepare
/prepare is called with consumeDraft: true; the server deletes
the draft row on success. The URL still carried ?draft=<id>,
so a reload on the Submit step re-mounted the wizard, hit
getDraft(<deleted id>), surfaced a not-found error, and
dropped the user out of the prepared flow. Now we
history.replace to drop the query param, pre-authorising the
transition through allowedPathRef so the (about-to-unmount)
history.block can't intercept.
Tests:
- NewProposal.test.js: +3 tests (refetch guard; cold-load still
fetches; URL strip after prepare) — 9 total, all passing.
- proposalForm.test.js: +1 regression asserting large decimal
amounts are counted at full width, not scientific notation —
36 total, all passing.
Full FE suite: 632/632 green on Node 20.19.4.
Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92b19947f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!draftIdFromUrl) return () => {}; | ||
| // Codex PR8 round 3 P1: skip refetch when the draft is already |
There was a problem hiding this comment.
When ?draft is absent this effect exits immediately, but onPrepare later clears ?draft and stores the prepared envelope only in component state. If the user reloads on the Submit step, state is lost and this early return prevents any server rehydration, dropping them back to an empty wizard even though a prepared submission already exists. Persist the prepared submission in the URL (or redirect to /governance/proposal/:id) and reload it on mount.
Useful? React with 👍 / 👎.
| {error && !submission ? ( | ||
| <div className="auth-alert auth-alert--error" role="alert"> | ||
| Could not load submission: {error.code || 'error'} |
There was a problem hiding this comment.
onDelete sets error on failure, but this alert only renders when submission is null. In the common failure case (e.g., transient 5xx/403 while viewing an existing submission), the delete button just re-enables with no error message, so users cannot tell the operation failed. Render an error banner for delete failures even when submission is present.
Useful? React with 👍 / 👎.
… errors (Codex R5 P2) Two related UX fixes: 1. After a successful /prepare the wizard used to setStepIdx(3) and render SubmitStep with the envelope in component state only. A browser reload there dropped the envelope, the draft-load effect saw no ?draft= and exited early, and the user landed back on an empty wizard even though the submission already existed server-side. Redirect to /governance/proposal/:id instead — the canonical reload-safe page that hydrates from the server row. Added the gobject prepare CLI fallback there for full parity with the former SubmitStep (so users paying manually from Syscoin-Qt still get the exact argv). Pre-authorised the internal navigation via allowedPathRef so the unsaved-changes modal does not fire. 2. onDelete on ProposalStatus routed failures into the top-level `error` state, but that banner only renders when `submission` is null. A transient 5xx/403 while the panel was on-screen just re-enabled the Delete button with no user-visible signal. Added a distinct deleteError state rendered inside the submission panel so retry-worthy failures are always surfaced. Tests updated + new regressions added for both behaviours. Made-with: Cursor
|
@codex review. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b62ef2034f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (err && err.code === 'submission_exists' && err.details && err.details.id) { | ||
| history.push(`/governance/proposal/${err.details.id}`); |
There was a problem hiding this comment.
submission_exists
When /prepare returns submission_exists, this branch calls history.push(...) directly while the wizard is often still dirty (e.g., user never saved a draft). In that state the history.block guard intercepts the push, opens the unsaved-changes modal, and prevents the intended automatic handoff to the existing status page. This breaks the duplicate-prepare recovery flow unless the user takes an unrelated save/discard action first; pre-authorizing this internal redirect (as done in the success path) would avoid the false prompt.
Useful? React with 👍 / 👎.
…edPathRef (Codex R5 P2) When `/prepare` returns `submission_exists` the wizard pushes the user to the existing submission's status page. Unlike the happy path, this branch does NOT flip the baseline via `mark_saved` — the draft was not consumed this call — so the form is still dirty relative to pristine. Without whitelisting the target path, `history.block` would pop the unsaved-changes modal in front of what is a legitimate "your prior prepare already took, here's its status page" redirect. Fix: set `allowedPathRef.current = nextPath` before `history.push`, so the block callback lets exactly that transition through and still prompts for any other dirty-leave attempt. Added a regression test. Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93b1d4cb33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <button | ||
| type="button" | ||
| className="button button--ghost" | ||
| onClick={saveDraft} |
There was a problem hiding this comment.
The toolbar binds onClick={saveDraft}, but saveDraft rethrows on failure after setting saveDraftError, so a failed createDraft/updateDraft call (for example, network or 4xx errors) produces an unhandled rejected promise in this click path. React does not catch async event-handler rejections, so this can trigger global unhandledrejection handlers or flaky test failures even though the page already surfaces a user-visible save error.
Useful? React with 👍 / 👎.
saveDraft() deliberately rethrows on failure so onModalSave() (which
awaits it) can distinguish success from failure and keep the
unsaved-changes modal open. A bare `onClick={saveDraft}` binding on
the toolbar button, however, makes that rejection an unhandled
promise — React does not attach a catch to event-handler return
values, so transient network / 4xx errors bubble to the global
`unhandledrejection` listener (and pollute test logs + trigger any
error-reporting hook the host app installs).
Fix: wrap the toolbar's onClick with `saveDraft().catch(() => {})`.
The error is already surfaced to the user via the `saveDraftError`
state-driven banner right next to the button, so there is no UX
change — just no more leaked rejections.
Regression test installs an `unhandledrejection` listener around a
click that is rigged to reject, asserts it never fires, and
asserts the visible error banner is still rendered.
Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d409a8ef4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| } | ||
| case 'mark_saved': { | ||
| return { ...state, baseline: state.form }; |
There was a problem hiding this comment.
mark_saved copies state.form into baseline, but saveDraft() posts a payload snapshot taken before the async request. If the user keeps editing while that request is in flight, those newer edits were never persisted yet still become the new baseline, so dirty turns false and the unsaved-changes guard no longer prompts. On slow networks this can silently lose post-click edits; baseline should reflect the payload actually saved (or inputs should be locked during save).
Useful? React with 👍 / 👎.
…, not live form (Codex R7 P1) `mark_saved` copied `state.form` into `baseline` at reducer-time. `saveDraft()` snapshots the form BEFORE awaiting the server, then dispatches `mark_saved` AFTER. If the user keeps typing during that round-trip, those newer edits end up in `state.form` by the time the reducer runs and silently become the new baseline — `dirty` flips false and the `history.block` leave-guard stops prompting for data that was never persisted (most visibly on slow networks). Fix: saveDraft now captures `savedSnapshot = form` before awaiting and passes it explicitly via `action.baseline`. The reducer installs that snapshot as the new baseline instead of reading the live form state. Callers that omit `action.baseline` still fall back to `state.form` for backwards compatibility. Regression test simulates the exact race: Save click snapshots a form value; user keeps typing before createDraft resolves; the resolved save installs the PRE-edit snapshot as baseline; a non-whitelisted navigation then correctly pops the unsaved-changes modal because the post-click edits are still dirty. Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 621977a433
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (draftId) { | ||
| try { | ||
| await proposalService.deleteDraft(draftId); | ||
| } catch (err) { |
There was a problem hiding this comment.
When a user resumes an existing draft, edits it, and chooses Discard in the leave modal, discardDraft() always calls proposalService.deleteDraft(draftId), which removes the server-side draft entirely instead of only dropping local unsaved edits. This turns a normal “discard my recent changes” action into permanent draft deletion, so users can lose previously saved work and cannot resume that draft later.
Useful? React with 👍 / 👎.
Codex PR8 round 8 P1 (src/pages/NewProposal.js): discardDraft() was unconditionally calling proposalService.deleteDraft(draftId) whenever a draftId was present. The common flow — resume a saved draft, edit it, navigate away, click Discard in the unsaved-changes modal — therefore wiped the entire draft from the server. That turns a "throw away my recent edits" action into permanent data loss and breaks the resume-later flow we promised users. Correct semantics: Discard in the leave modal means "drop my unsaved edits", not "delete my saved draft". If a server-side draft exists (draftId != null), revert the form to `baseline` (which IS that row's content) and leave the row intact. Only clear state entirely when there is no persisted draft. Regression test loads an existing server draft, edits a field, triggers a guarded navigation, clicks Discard, and asserts deleteDraft was never called and the form reverted to the saved baseline value. Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1c92744f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <section className="page-section page-section--tight page-section--last"> | ||
| <div className="site-wrap"> | ||
| {loading && !submission ? <p>Loading…</p> : null} | ||
| {error && !submission ? ( |
There was a problem hiding this comment.
When a later getSubmission poll fails after an initial successful load (for example, the submission is deleted from another session and the API starts returning not_found), load() sets error but keeps the previous submission in state. This guard hides the error banner whenever submission is truthy, so the page can keep showing stale status data with no visible failure signal. Please either clear submission on hard failures or render the error regardless of cached submission state.
Useful? React with 👍 / 👎.
…isible (Codex R9 P2) load() kept the previous submission in state on error, but the only error banner on ProposalStatus was guarded by !submission, so a later failed poll (transient 5xx, offline, not_found because the row was deleted from another session) rendered no feedback at all — users kept reading stale status as if the server were healthy. Two-pronged fix: - Hard-failure codes (not_found, forbidden) clear the cached submission so the existing full-page "Could not load" banner takes over. The row is effectively gone from this user's perspective and must stop rendering as live. - Transient failures (5xx, network) keep the cached panel visible but raise a new inline warning banner (auth-alert--warning variant) that flags the displayed data may be out of date and that we'll retry automatically. Regression tests added for both paths; full ProposalStatus suite 14 passing, full FE suite 640 passing. Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a143e35e92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| allowedPathRef.current = `${history.location.pathname}${nextSearch}${ | ||
| history.location.hash || '' | ||
| }`; |
There was a problem hiding this comment.
When the wizard is loaded on a URL that already has a fragment (for example /governance/new#foo), this whitelist stores pathname + search + hash, but the subsequent history.replace only sets pathname and search. The block callback then compares against a different location key, treats the internal URL sync as untrusted navigation, and opens the unsaved-changes modal instead of allowing the successful save flow to continue. This breaks first-save URL normalization (?draft=...) for hash-bearing URLs.
Useful? React with 👍 / 👎.
| if (!submission && !error) return undefined; | ||
| const delay = | ||
| status === 'awaiting_collateral' ? POLL_FAST_MS : POLL_SLOW_MS; |
There was a problem hiding this comment.
For malformed route params, load() sets error.code = 'invalid_id' and returns early, but the polling effect retries whenever there is any error and no terminal status. That means an invalid URL schedules a new timer forever even though the condition is not recoverable, causing unnecessary repeated state churn for as long as the page remains open. The effect should short-circuit for non-retryable validation errors like invalid_id.
Useful? React with 👍 / 👎.
…itelist (Codex R10 P3)
Two small frontend correctness fixes:
P3 (src/pages/ProposalStatus.js) — short-circuit the polling
effect for non-retryable errors. load() sets error.code =
'invalid_id' for malformed route params and returns early, but
the polling effect queued a new setTimeout for any error + non-
terminal combination. An invalid URL therefore scheduled another
retry every POLL_SLOW_MS for as long as the page stayed open,
burning state churn on a condition that cannot recover. Treat
`invalid_id` and `forbidden` as non-retryable and return early
from the polling effect. Anything else (5xx, network, transient
SQLITE_BUSY) stays retryable.
P3 (src/pages/NewProposal.js) — drop location.hash from the
post-save allowedPathRef whitelist key. history.replace({pathname,
search}) produces a location with no hash, but the whitelist was
storing `pathname + search + (current hash)`. For any wizard
URL that happened to carry a fragment — deep link, anchor jump,
stray '#' from copy/paste — the whitelist key would then be
`path?search#hash` while the post-replace locKey was
`path?search`, so the block callback treated our own internal
URL sync as an untrusted navigation and popped the unsaved-
changes modal on a successful first save (and could drop the
`?draft=<id>`, breaking reload-to-resume). Drop the hash from
the whitelist so the key matches exactly what history.replace
produces.
Regression tests added for both. Full relevant suites 27/27;
full FE suite 640/640 in isolation (1 unrelated pre-existing
flake in ProposalVoteModal.test.js when run in full-parallel).
Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dd177d9dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (code === 'not_found' || code === 'forbidden') { | ||
| setSubmission(null); | ||
| } |
There was a problem hiding this comment.
This catch path only clears submission for not_found/forbidden, so if a user navigates from /governance/proposal/old to a different ID and that fetch fails transiently, the page keeps rendering the old row under the new URL. Because action handlers use submission.id, users can accidentally attach collateral or delete the wrong submission while seeing a stale-data warning. Clear cached submission whenever the requested route ID changes (or before each load), not only on these two error codes.
Useful? React with 👍 / 👎.
| .catch((err) => { | ||
| if (cancelled) return; | ||
| setLoadError(err); | ||
| }) |
There was a problem hiding this comment.
When getDraft(draftIdFromUrl) fails, the component leaves the previous form and draftId untouched. If the user switched to another ?draft= value (or a deleted one), the UI still shows the old draft and saveDraft() continues PATCHing the old draftId, which can overwrite the wrong draft while the URL points elsewhere. On load failure for a different draft target, reset local draft state (or block save) to avoid cross-draft writes.
Useful? React with 👍 / 👎.
ProposalStatus.js — on route-param `id` change, reset submission + error + loading BEFORE the new load fires. Pre-fix, the page stayed mounted with the previous submission in state during the new fetch window; any action handler bound to submission.id (Attach-Collateral, Delete) would then operate on the wrong row if the user acted before the new load completed or if that load then failed transiently. Now the `[id]` effect clears state eagerly, and the catch-branch drops the stale-for-different-id condition (the [id] reset already handles it upstream, so the catch only needs to clear on hard failures). NewProposal.js — when the draft-load effect observes draftId != draftIdFromUrl (URL points to a different draft than the one in local state), reset form to emptyForm() and drop draftId BEFORE the new fetch resolves. If that new fetch fails, the catch also clears draftId so subsequent saveDraft() can only create a new draft, never PATCH a draft the user is no longer viewing. Without this, switching /governance/new?draft=10 → ?draft=20 with a failing /20 fetch kept showing /10's fields AND kept PATCHing /drafts/10 while the URL claimed /20 — a cross-draft write under the user's nose. Regression tests: - ProposalStatus: navigating /governance/proposal/100 → /200 drops #100's distinctive testid immediately on rerender (no stale render window) and only shows #200 after its fetch resolves. - NewProposal: switching /governance/new?draft=10 → ?draft=20 with a failing /20 fetch clears the form, and a follow-up save does NOT call updateDraft(10, ...) — guaranteeing no cross-draft write. Made-with: Cursor
|
@codex review. Don't worry about migrations, not an issue. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 935c8c5c01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const row = await proposalService.getSubmission(id); | ||
| if (!mountedRef.current) return null; | ||
| setSubmission(row); | ||
| setError(null); |
There was a problem hiding this comment.
load() writes every successful response into state without verifying it still belongs to the current route id. If a user navigates from /governance/proposal/100 to /governance/proposal/200 while the first request is still in flight, an out-of-order resolution can overwrite the page with row 100 again, and subsequent actions (attach collateral/delete) can target the wrong submission. Guard responses with a request token/current-id check (or cancellation) before calling setSubmission.
Useful? React with 👍 / 👎.
ProposalStatus.js — request-token guard in `load()`. Each call snapshots `id` into a local `reqId` at start, and a `latestReqIdRef` (kept in sync with the route `id` on every render) tracks the currently-displayed id. After the await, any response whose `reqId` no longer matches `latestReqIdRef.current` is dropped on the floor without writing state. Without this, a slow /100 fetch that resolves AFTER a subsequent /200 fetch would overwrite the page with row 100 while the URL points to 200, and action handlers bound to submission.id would then target the wrong row (Delete /100 when user sees /200). The `[id]` reset effect clears state eagerly on navigation, but doesn't help with response-ordering races because the stale response arrives AFTER the reset + new load have already run. Regression test holds /100's promise pending, resolves /200 first, then releases /100 and asserts the page still renders #200 and never flashes to the stale #100. Made-with: Cursor
|
@codex review. Don't worry about migrations, not an issue. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e2967646a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!draftIdFromUrl) return () => {}; | ||
| // Codex PR8 round 3 P1: skip refetch when the draft is already |
There was a problem hiding this comment.
draft query param is removed
When this effect sees no draft query param it immediately returns, so navigating within the mounted wizard from /governance/new?draft=<id> to /governance/new keeps the previous draftId and form state in memory. In that state, clicking Save draft still calls updateDraft on the old draft even though the URL indicates a new proposal, which can overwrite the wrong draft. Handle the !draftIdFromUrl branch by resetting draftId and replacing the form with emptyForm() so route state and persisted target stay aligned.
Useful? React with 👍 / 👎.
… URL NewProposal.js — the draft-load effect's `!draftIdFromUrl` branch previously early-returned without side effects. If the user navigated /governance/new?draft=<id> → /governance/new (param removed) while the wizard was mounted, `draftId` + `form` stayed in memory tied to the prior draft, and Save Draft would PATCH /drafts/<old id> while the URL claimed we were authoring a new proposal — a silent cross-draft overwrite. Fix: the branch now resets draftId, replaces the form with emptyForm(), and clears loadError, so the route state and the persisted target stay aligned. A subsequent Save Draft can only create a new draft, never update a stale one. Regression test: hydrate a draft via ?draft=50, navigate to /governance/new (no param), assert the form is cleared, then type + save and confirm updateDraft(50, ...) is never called. Made-with: Cursor
|
@codex review. Don't worry about migrations, not an issue. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 664fd8d610
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (draftId != null) { | ||
| const blank = emptyForm(); | ||
| dispatch({ type: 'replace', form: blank, baseline: blank }); | ||
| setDraftId(null); | ||
| setLoadError(null); |
There was a problem hiding this comment.
When a ?draft=<id> load fails, catch sets loadError and also sets draftId to null. If the user then navigates to /governance/new (no draft param), this branch only clears loadError when draftId != null, so the stale "Couldn't load draft" banner persists on a fresh-new-proposal route. This creates a misleading error state and makes the page look broken even though no draft is being loaded.
Useful? React with 👍 / 👎.
| const url = (form.url || '').trim(); | ||
| if (url) out.url = url; |
There was a problem hiding this comment.
draftBodyFromForm drops empty strings, so PATCH updates cannot clear previously saved fields. A user who resumes a draft, deletes a value (for example url), and clicks Save sends no url key, so the backend keeps the old value; meanwhile the UI marks the empty local snapshot as saved. On reload, the old value reappears, which silently discards the user’s explicit clear action.
Useful? React with 👍 / 👎.
|
@codex review. Don't worry about migrations, not an issue. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 664fd8d610
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| setSubmission(null); | ||
| setError(null); | ||
| setLoading(true); | ||
| }, [id]); |
There was a problem hiding this comment.
The id-change reset effect only clears submission, error, and loading, so proposal-specific UI state (deleteError, attachError, and txidInput) leaks across /governance/proposal/:id navigations. After a failed delete/attach on one proposal, opening a different proposal can show stale error banners and a prefilled TXID from the previous row, which is misleading and can trigger accidental attach attempts against the wrong submission.
Useful? React with 👍 / 👎.
| if (errCode === 'invalid_id' || errCode === 'forbidden') { | ||
| return undefined; |
There was a problem hiding this comment.
The polling short-circuit excludes invalid_id and forbidden but not not_found, so a missing submission keeps re-scheduling background fetches every 60 seconds indefinitely. For stale/deleted IDs this creates unnecessary backend traffic and repeated error churn without any user-recoverable path; not_found should be handled like other permanent errors in this guard.
Useful? React with 👍 / 👎.
- FE P2 (src/pages/NewProposal.js): always clear loadError when
the ?draft= query is removed from the URL. Prior code gated
the reset on draftId != null, so a failed load (which clears
draftId in catch) left a stale "Couldn't load draft" banner
visible on a fresh-new-proposal route.
- FE P2 (src/lib/proposalForm.js): draftBodyFromForm now accepts
{ forUpdate } and, when true, emits explicit empty strings for
cleared text fields (name/url/paymentAddress) and null for
cleared epochs. NewProposal passes forUpdate: true to
updateDraft so user clears actually PATCH through. Previously
dropping empty keys silently discarded a user's explicit
delete of e.g. url — the UI marked the blank snapshot as saved
while the backend kept the old value; reload silently restored
it.
- FE P2 (src/pages/ProposalStatus.js): extend the [id] reset
effect to also clear deleteError, attachError, and txidInput.
These are per-submission UI state; leaving them set across
/governance/proposal/:id navigations showed stale error
banners on a different row and — worse — prefilled row B's
attach-collateral textbox with a txid the user pasted into
row A, one click away from an accidental cross-row attach.
- FE P3 (src/pages/ProposalStatus.js): treat not_found as
non-retryable in the polling short-circuit. Previously the
guard excluded only invalid_id and forbidden; a stale/deleted
submission would otherwise keep re-firing getSubmission every
60s indefinitely for every mounted tab.
Full regression tests added for each fix; existing test suites
continue to pass (652/652).
Made-with: Cursor
|
@codex review. Don't worry about migrations, not an issue. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22b59675ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function onKey(e) { | ||
| if (e.key === 'Escape') { | ||
| e.preventDefault(); | ||
| if (!saving && typeof onCancel === 'function') onCancel(); | ||
| } |
There was a problem hiding this comment.
This dialog only handles Escape on keydown and does not trap Tab/Shift+Tab within the modal, so keyboard users can move focus to background controls while the modal is open. That breaks modal semantics and can let users trigger navigation/actions outside the save-discard decision path, which is risky in an unsaved-changes flow. Add a focus trap (or make the background inert) for the modal lifetime.
Useful? React with 👍 / 👎.
| {savingDraft ? 'Saving…' : 'Save draft'} | ||
| </button> | ||
| ) : null} | ||
| {draftSavedAt && !savingDraft && !saveDraftError ? ( |
There was a problem hiding this comment.
The "Saved" indicator is rendered solely from draftSavedAt/savingDraft/saveDraftError, so after one successful save it remains visible even when the form becomes dirty again. This can mislead users into believing their latest edits are persisted when they are not. The badge should be gated by !dirty (or draftSavedAt should be cleared on subsequent field edits).
Useful? React with 👍 / 👎.
…clean state Codex PR8 round 14 P2. Two separate UX correctness fixes: 1. UnsavedChangesModal was missing a Tab/Shift+Tab focus trap. Escape was handled, but keyboard users could advance focus to background controls (wizard inputs, nav, etc.) while the modal was open, breaking modal semantics in the middle of a save/discard decision. Added a keydown handler on window that intercepts Tab while `open` is true: forward-wrap from the last focusable back to the first, reverse-wrap Shift+Tab the other way, and pull focus back to the primary action if it has escaped the panel entirely. Disabled buttons are excluded so during `saving` the modal simply consumes Tab (nothing to cycle between). 2. The "Saved" badge in the NewProposal wizard was rendered solely from `draftSavedAt`/`savingDraft`/`saveDraftError`, so after a successful save it remained visible even after the user made further edits. That was a false-safety signal: users inferred their latest changes were persisted when only the pre-edit state was. Gated the badge on `!dirty`, which already tracks whether the form has diverged from the last saved snapshot. The badge now hides instantly on any post-save keystroke and re-appears on the next successful save (when dirty flips back to false against the new baseline). Regression tests cover both paths: Tab wrap forward/back, Tab pulling focus back from an outside element, non-Tab keys untouched, and the "Saved" badge disappearing immediately after a post-save field edit. Made-with: Cursor
|
@codex review. Don't worry about migrations, not an issue. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
End-to-end UI for authoring a Syscoin governance proposal, paired
with the sysnode-backend PR 8
surface (
/gov/proposals/*).This PR delivers the full user-visible flow: discovery → drafting →
server-side save → canonical prepare → 150 SYS collateral payment
(manual) → automatic on-chain submission after 6 confirmations →
live status updates → success/failure copy. It intentionally defers
the Pali-native PSBT signing path (`payWithOpReturn` is stubbed)
— that path needs live interop testing against the extension and
is scoped to a follow-up PR so this one can ship with a robust,
testable manual fallback first.
What's new
`/gov/proposals` endpoints with shape assertions, Retry-After
propagation, and stable error codes.
`request` wrapper, EIP-1193 error normalisation. The
`payWithOpReturn` stub throws `pali_psbt_builder_not_wired`
on purpose so the UI can feature-detect and fall back cleanly.
helpers shared by Basics/Payment steps and body builders.
Mirrors the backend's `proposalValidate` constants (512-byte
payload limit, 40-char name, 4-char URL, `MAX_PAYMENT_COUNT`).
Save / Discard / Cancel dialog. Focus-trapped, Escape-aware,
overlay click is Cancel.
`Drafts (N)` toggle + in-flight submissions list above the
governance watchlist. No banners; the entry is there when you
need it and invisible when you don't.
OP_RETURN bytes for non-Qt wallets, plus a 64-hex TXID paste box
adaptive polling (10s while awaiting collateral confirmations,
60s in `prepared`, stopped in terminal states) and tailored
copy for each state (`awaiting_collateral`, `submitted`,
`failed` with human-readable reason).
reuse the existing design tokens. Mobile-safe grid collapses.
`/governance/proposal/:id` guarded by `PrivateRoute`.
UX notes
dirty wizard, and browsers get a native `beforeunload` prompt for
tab close / reload.
(before committing) and again on the Submit step (before paying).
page feels native to the rest of Governance.
Out of scope (deferred)
/ chain ops are wired; `payWithOpReturn` needs a fully-formed
PSBT built via `syscoinjs-lib` and a live extension test. Making
the manual path primary first keeps PR 8 shippable.
"Pay with Pali" button; it will light up when the builder lands.
Test plan
tests pass.
UnsavedChangesModal are covered.
(with malformed-TXID client validation + success navigation).
delete confirm, and load-error branches.
full 6-confirmation advance. (Cannot automate without Core.)
Linked
Backend PR: syscoin/sysnode-backend#8
Made with Cursor