Conversation
…taRef guard (Slice 5b) (#145) * feat(sync): conflict defer-gate — busy re-check + idle-drain + saveDataRef guard (Slice 5b) Completes the canvas side of the reconciler. The store (4c) already declines to adopt-over-local while isBusy() and holds pendingRemote; 5b makes the canvas honor that safely and closes the render race 5a deferred. No store change (maybeFlush consumes pendingRemote before firing onRemoteUpdate, so flushPending is a no-op after an adopt — the canvas owns the deferred re-render). Two distinct cases, two mechanisms: - CASE 1 — store deferred at its own gate (isBusy true → never adopted, local untouched, onRemoteUpdate never fires). The canvas calls flushPending() on idle to adopt then. - CASE 2 — store adopted, then the canvas went busy in maybeFlush's ~2-IDB-write gap before onRemoteUpdate fires. onRemoteUpdate re-checks busy at APPLY time and, if busy, DEFERS the render (local already == remote on Drive) instead of clobbering in-flight work; the idle-drain re-reads LOCAL (freshest — the adopt, or a local edit the user saved in the window; stashing the remote data would clobber that) and hydrates. Both handled by one idle effect (flushPending + re-read-local-if-pending), whose deps include saveState — the last thing to clear on going idle is usually the debounced save, and it must gate re-hydrate so a committed trace's pending save lands first (CRITICAL-b). onRemoteUpdate nulls saveDataRef on BOTH branches so the unmount flush can't push a pre-adopt payload at a fresh rev over the remote winner. isBusy() completeness (a 4c review gap): the predicate now also reports an active drag (dragRef/ocDragRef), the open inline text editor (editingRef), and an in-flight OCR scan (scanBusyRef) — every interaction a re-hydrate would clobber. Extracted as a PURE, unit-tested isCanvasBusy (lib/canvasBusy.ts); computeBusy reads it via refs so it's always fresh yet stable to capture once (no re-register null window). RESIDUAL (documented, not chased — active-co-editing-only, which the rollout forbids): in the adopt→onRemoteUpdate window, if the user COMMITS a trace and its already- scheduled debounced save fires, it pushes pre-adopt+shape over the winner at remote.rev+1. saveDataRef-null closes the unmount flush, not that pending setTimeout. +7 predicate unit tests (648 total); typecheck/lint/build clean. Anonymous path Playwright-verified unchanged (mount, edit, autosave, effects no-op, 0 errors). The opted-in Case 1/2/idle-drain paths are React-bound → advisor-designed + reasoned; end-to-end needs a Google-configured build. * fix(sync): serialize + make the 5b idle-drain retry-safe (Copilot review) - onRemoteUpdate clears remotePendingRender on an immediate (not-busy) hydrate, so a satisfied deferral doesn't trigger a redundant idle re-read. - idle-drain: await flushPending() BEFORE the Case-2 re-read (so a store-deferred adopt's IDB writes land first, not raced); clear remotePendingRender ONLY after a successful loadAnnotations (a rejected read now retries on the next idle instead of dropping the render); re-check busy/alive after each await (no stale hydrate, no setState on an unmounted tree). * fix(sync): close silent loss of the remote winner in the 5b defer window (local review) The local adversarial review found a HIGH-severity silent-loss path (advisor confirmed must-fix): while a Case-2 render was deferred, local IDB already held the adopted remote winner R (synced_rev advanced) but the canvas still showed superseded content — and an edit on that stale canvas saved + pushed at expectedRev=R.rev, clobbering R on both sides with no snapshot. Two composing bugs: - Bug A: the idle-drain's deps were a strict subset of computeBusy's inputs — the interaction REFS (drag/editor/scan) aren't watched, so a busy→idle edge from a ref clearing (sub-threshold drag, editor dismissed no-change, scan done) didn't re-fire the drain → the deferred render stuck until the next state-dep change. - Bug B: nothing blocked a save while a render was deferred, so a stale-canvas edit pushed over R. Fix (advisor-designed): - PRIMARY — remotePendingRender now GATES autosave: while a render is deferred the canvas shows content the store already superseded, so it must never persist/push. Guarded in both the autosave effect (new scheduling) and the debounced timeout callback (a save scheduled BEFORE the adopt — closes the last pre-scheduled window). Converts silent loss of R into visible supersession (canvas jumps to R on drain); in-window edits are the co-editing casualty the rollout forbids, not silent loss. - SECONDARY — idleTick: a state bumped at each interaction-ref clear site (drag in onPointerUp, editor-close effect, scan finally), added to the drain deps, so the busy→idle edge is always observable and suppression can't wedge saves. - Finding 4: the drain's post-loadAnnotations re-check now also bails if a concurrent onRemoteUpdate already cleared remotePendingRender (no double-hydrate). RESIDUAL now truly nil for R-loss; the only remaining effect is supersession of concurrent-edit-on-stale (visible, co-editing-only). 648 tests green; typecheck/lint/ build clean. Default path Playwright-verified: load, condition autosave, pan-drag (bumpIdle site) — 0 console errors. * fix(sync): gate bumpIdle on syncBridge — restore byte-identical flag-off (review) Second local review noted the flag-off path was no longer byte-identical: bumpIdle fired extra setIdleTick re-renders on drag/editor/scan-end even with no syncBridge (functionally inert, but a deviation from invariant #4). bumpIdle is only meaningful on the opted-in path (the idle-drain no-ops without a bridge), so gate it — the anonymous/legacy path is now free of those re-renders again.
…e (Slice 6) (#146) * feat(sync): gate local-first on a build env var, not a per-user toggle (Slice 6) Turns the local-first + Drive-sync feature on via VITE_CLOUD_SYNC=1 at build time (default off = today's Drive-canonical behavior, byte-for-byte). Replaces the 5a localStorage per-browser flag with a deployment-wide one, and drops the planned Settings toggle/panel entirely. Deliberate, and safer than a per-user toggle: only enabled clients honor the sync rev precondition, so a PARTIAL fleet (some browsers on, some off) is the mixed-fleet clobber hazard. A build flag flips the whole deployment at once — which IS the "don't share a project until the whole collaborator set is opted in" rule, enforced by the deploy instead of left to per-user chance. Rollback is one env change + redeploy. - prefs.js: cloudSyncOptedIn (localStorage) → cloudSyncEnabled (VITE_CLOUD_SYNC), matching the existing import.meta.env pattern; drop the now-dead setter. - main.jsx: call the renamed gate (only consumer). - .env.example: document VITE_CLOUD_SYNC. 648 tests green; typecheck/lint/build clean; default path boots clean (0 console errors). No UI — nothing user-visible by default. * docs(env): header says 'deployment-wide build flag', not 'opt-in' (Copilot review) The .env.example section header still implied a per-user toggle; the flag is deployment-wide/build-time.
…Slice 7) (#147) * docs(sync): local-first rollout + sync architecture / provider seam (Slice 7) Completes the epic's docs. The existing GOOGLE_SETUP.md already covered the Google/ OAuth/Drive setup, so this fills the epic-specific gaps: - GOOGLE_SETUP.md §5: how to enable local-first (VITE_CLOUD_SYNC=1), the whole-fleet rollout rule (why it's deployment-wide, the mixed-fleet clobber it prevents, the process rule + the recoverable snapshot net), and one-env-var rollback. - SYNC_ARCHITECTURE.md (new): dev-facing map — the composite store, the rev-precondition correctness model, the two injected provider interfaces (annotation pull/push; snapshot 6-method file API), how OneDrive/O365 drops in (documented, not built — YAGNI), the cut-line, and a file index. - README + .env.example: point at local-first sync. Docs only — no code change. All file refs, exported names, and internal links verified to resolve. 648 tests unaffected. * docs(sync): correct the cut-line claim — main.jsx imports composite.js (Copilot review) The 'delete sync/* and it still builds' claim was inaccurate: main.jsx has a dynamic import("./lib/sync/composite.js") on the opted-in branch, so deleting the sync files breaks the build unless that one branch is removed too. Reframed to state the accurate one-way-dependency cut-line: the sole importer is main.jsx's opted-in branch; remove it + delete the files → a working local-first app remains.
…evisions rename, canvas fixes, contribution.v2, MCP 0.4 Resolutions: - TakeoffCanvas drag-end: upstream's commit-on-gesture-end command dispatch + our Slice 5b bumpIdle() idle-drain calls - README: upstream restructure + our team-cloud-mode / local-first section restored - docs/SYNC_ARCHITECTURE.md: SnapshotPanel references updated to RevisionsPanel (upstream rename)
….0, use_detected literal(true) (re-apply d2f6aca)
… policy Duplicate CSPs enforce as the intersection: upstream's block lacks accounts.google.com/fonts origins, so Google sign-in and webfonts would break in prod; if it won instead, connect-src * would undo the token- containment policy. web/public/_headers stays the single source.
Post-merge review finding: computeBusy fed isCanvasBusy every pre-merge interaction mode but not the new agent's — on a VITE_CLOUD_SYNC build a remote adopt while the agent was mid-run (or its dashed proposals awaited review) would hydrate() immediately, wiping staged proposals and orphaning mid-run minted conditions, where One-Click review in the same situation defers. agentRunning/agentProposals now gate exactly like One-Click's proposal, and both join the idle-drain deps so the run finishing or the last proposal resolving drains a held remote.
…y.toml as the single header source Reverses 4748074's direction per the new fork strategy: follow the parent wherever possible, diverge only for company-specific features. Upstream's [[headers]] block is the base — connect-src * makes the BYO agent/AI seams work in prod. Divergences, each mapped to shipped code: Google sign-in origins (script-src/frame-src accounts.google.com) for the team cloud mode, the Google Fonts origins upstream's block omits while tokens.css still @imports them, and HSTS/upgrade-insecure-requests retained. web/public/ _headers deleted — one policy, upstream's location. Trade-off accepted with the pattern: connect-src * removes the token- containment pin; the Drive token's protection is now script-src integrity rather than egress restriction.
…intent (60s > observed 50s slow render), listSnapshots counts guard non-array payloads, package.json indent
…eleted deploy.yml IS the production deploy (main → npm run check → Netlify --no-build upload); without it the merge would have left prod stale. copilot-review-gate.yml produces the required 'copilot-review' status — its deletion made branch protection permanently BLOCKED. Upstream has neither file, so the merge took the deletion; both are company-specific and stay fork-only.
Sync upstream: command layer + undo/redo, takeoff agent, Revisions rename, canvas fixes, contribution.v2, MCP 0.4
) * fix(auth): persist Google session to sessionStorage across reloads (#148) Every page refresh dropped the in-memory token/user and forced a fresh sign-in click, even seconds after signing in. Cache the token+profile in sessionStorage (tab-scoped, keyed to the build's client_id) and restore it at module load when still valid, so a reload within the token's lifetime needs no contact with Google at all — sidestepping the GIS/COOP silent-refresh issue this was blamed on. * fix(auth): require a usable email before restoring a persisted session Copilot review on #151: readPersistedSession() accepted any truthy object for user, so a malformed/corrupted sessionStorage blob could restore a "signed in" state with no email. AccountChip/AuthChip/ ReportPanel all read user.email directly — require it non-empty.
… redundant check, test the storage glue (#153) Follow-up to the deep review of #151 (sessionStorage session persistence). Fixes the minor findings that don't need design work (the bigger revoked- token/silent-sync-failure gap is tracked separately in #152): - Gate hydrateSession() behind isGoogleConfigured() — it's a no-op on unconfigured builds, so skip the storage read on every page load. - Drop the redundant Array.isArray(storedUser) check in readPersistedSession() — JSON never round-trips a string .email property onto an array, so the following email check already covers it. - Comment explaining why persistSession() is called from both the GIS token callback and signIn() — they cover disjoint flows (silent refresh vs. interactive sign-in), not redundant despite looking like it. - Export persistSession()/hydrateSession() and add integration tests against a stubbed sessionStorage, so the actual storage glue is covered, not just the pure readPersistedSession() validator.
…tream-tracked Upstream flipped CSP from _headers to netlify.toml and back to _headers within hours of our sync (PR #150). Diffed both directive-by-directive — content is equivalent (same origins, same open connect-src stance) except their Permissions-Policy interest-cohort=() FLoC opt-out, backported here. netlify.toml stays the fork's source of truth going forward regardless of which file upstream prefers this week. Google sign-in origins are called out as first-class fork features, not divergences to reconsider each sync.
fix(deploy): make netlify.toml the fork's permanent CSP home
…113) (#155) * feat(canvas): multi-select mode (#113) — M / mode button, click-toggle + marquee, multi highlight, HUD count A dedicated, visible mode (STACK-style; no hidden chords): the Mode cluster gains a Multi-select button (shortcut M). In-mode, a left click toggles a shape in/out of the selection Set and a left drag rubber-bands a marquee — center-in-rect containment in stage px (new lib/marquee.js reusing zone's shapeCenter), so one lasso spans side-by-side panels. Pan stays on right/middle/Space. Selected shapes wear the cobalt highlight; no vertex handles in multi mode. Entry seeds from the single selection and clears in-progress traces + frozen crosshair aids (useLayoutEffect — no flash); exit aborts a live gesture and collapses a 1-member set back to Select (guarded so a paste's fresh selection is never clobbered). Selection prunes against visibleShapes; Esc clears; the HUD shows the live count and a clear button. Marquee gesture aborts (not commits) on pointercancel. * feat(canvas): bulk actions on the multi-selection — ⌫ delete, condition reassign, label assign (one undo step each) All three ride the ids[]-native commands — zero command-layer changes, one recordCommand entry regardless of N. ⌫/Delete clears the whole selection; a condition chip/strip/panel click (the affordance-bearing paths — dashed cobalt + pluralized titles) bulk-reassigns, with an already-assigned filter so unchanged shapes collect no provenance stamp and a palette-chip double-click (onClick + onDoubleClick both land here) can't double-dispatch — the same guard fixes that latent double-stamp for single-select too. Digits 1–9 still only activate (reassign:false — a keypress has no visual reassign affordance). The Label select decouples from activateLabel in multi mode: it dispatches the batch directly and never moves the active tracing label; heterogeneous selections show a disabled '— mixed —' sentinel (NUL-prefixed value — a real label named that can't collide), and a uniform ad-hoc label that left the vocabulary renders as its own option instead of blanking the control. * chore(canvas): multi-select hardening — mid-marquee joins the Slice 5b busy predicate A live marquee gesture defers a remote adopt exactly like a shape drag (one ref term in computeBusy's dragging); a static selection deliberately does not join — it's view state that survives a re-hydrate, and gating on it would let a held selection starve sync adoption. canvasBusy.ts doc updated to match. (The dblclick toggle guard and pointercancel abort landed with the mode core.) * fix(canvas): PR #155 Copilot review round — visibleShapes passes, height guard, escaped sentinel Multi-select scans (multiLabel, bulk reassign) now iterate visibleShapes instead of the full shapes array, matching the invariant multiSel is already pruned to. Bulk reassign collects changed ids in one Map pass instead of a shapes.find per selected id. marquee.js excludes a zero/missing-height panel the same way it already excluded zero-width, with a regression test for the Y=0 collapse. MIXED_SENTINEL uses the \0 escape sequence instead of an embedded literal NUL byte.
…ue 401 (#157) * fix(auth): force re-auth on a revoked Google token instead of an opaque 401 getAccessToken() only checks the local clock (#151), so a token revoked server-side (admin action, user revokes at myaccount.google.com) restores as "valid" and every Drive call then 401s with no recovery path — silent on the local-first sync composite (best-effort swallow) and a bare "Drive X failed (HTTP 401)" on the legacy cloudStore path. drive.js's assertOk() is the one chokepoint every call from both paths passes through (main.jsx builds a single createDrive() instance and threads it into both createCloudStore and the local-first composite), so adding an onUnauthorized hook there — wired to auth.js's signOut() at all four createDrive() call sites — bounces every surface back to sign-in the same way, with no changes to syncStore's best-effort catch semantics. Fixes #152. * fix(auth): update assertOk's comment for the 401 special case Copilot review on PR #157 — the comment described the pre-401-branch behavior only; clarified that a 401 intentionally skips the status/body detail in favor of a re-auth message.
… true curved length (#76) New Draw-menu tool between Linear and Surface Area. Centripetal Catmull-Rom through the clicked control points (web/src/lib/curve.js); shape = linear + curved:true, control points only; rendering, hit-testing, thickness re-flow, and recompute all measure the flattened spline. Contribution wire carries the flag; docs and flattenCurve contract tests included.
…round-trip, guards on measure_role; fix CHANGELOG's ambiguous upstream PR ref hitShapeC flattened curve points to px, divided back to normalized space, then handed them to hitShape which immediately re-multiplied by w/h — pure waste. Passes the flattened px points straight through with w=1,h=1 instead. Also guards on measure_role === "linear", matching the documented contract that curved only applies to linear runs, not just checking the flag alone. CHANGELOG's "(#76)" auto-links to this repo's own PR #76 (unrelated: Drive sidecar JSON migration), not upstream's Curved Line PR. Spelled out the cross-repo reference so it resolves correctly.
…arget, hitShapeC recomputed the spline every hit test flattenCurve's per-segment minimum (2 steps) could push the total well past maxPts on curves with many closely-spaced control points — 200 points at uniform tiny spacing overshot the 220 cap to ~400. Rewrote the allocator to track a real remaining budget, reserving 1 step for each still-unprocessed segment instead of a fixed per-segment floor. Added a test that exercises the overshoot scenario the old code missed. hitShapeC re-flattened a curved shape's spline on every pointer-move hit test. Shapes are replaced (never mutated) on any edit via the command layer (applyShapeCommand's geom case always returns a fresh object), so a WeakMap keyed by shape identity caches the flattened polyline safely — it invalidates itself the instant a shape is actually edited.
…s as straight chords markedset.js mapped verts_norm straight to px and connected them with line segments for every linear/surface_area shape — for a curved run that's the raw clicked control points, not the spline, so the exported PDF showed angular chords while the live canvas showed the smooth curve. Flattens through the same flattenCurve used everywhere else a curved shape's drawn path matters, guarded by the same curved && measure_role === "linear" check as hitShapeC and the canvas render path.
… px-with-w=1 trick, maxPts uses ?? hitShapeC cached the flattened spline in px space and called hitShape with w=1,h=1 to skip re-scaling — functionally fine, but it left verts_norm holding non-normalized values and relied on hitShape never doing anything with them besides *w/*h. Converts the flattened points back to normalized before caching and calls hitShape with the real w/h instead, so verts_norm stays honest. w/h are a sheet's fixed image dimensions here, not zoom-dependent, so this doesn't change the cache's effective hit rate. flattenCurve's maxPts default used `||`, silently discarding an explicit 0.
…t for segCount > maxPts With more than ~221 control points, segCount exceeds the 220 cap and every segment floors to 1 step anyway via the outer Math.max(1, ...) clamp — the old budget calc let that term go negative and relied on the clamp catching it, working by accident rather than by design. budget now starts at max(min(total, maxPts), segCount) so the floor is explicit: segCount is the honest lower bound (every segment needs >=1 step to reach its control point), not a side effect of clamping a negative number. Output is unchanged — this documents/locks in behavior that already held, with a test pinning the exact count.
…eet mid-load
panelImgs[key] || { w: 0, h: 0 } is a real fallback while a sheet's image is
still loading. hitShapeC's normalize round-trip divides by w/h, which would
yield NaN/Infinity vertices for a curved shape hit-tested during that window.
Falls through to plain hitShape instead, matching how every other shape type
already degrades harmlessly (verts_norm * 0) against an unloaded sheet.
Add Curved Line tool (Q): smooth spline through clicked points
…r view, and Flip H/V Both had drifted since their shipping commits (3adbbbc, dcb879f) never touched these docs. FEATURES.md's "Totals & report" row still described the pre-rename columns and didn't mention the Labor view preset; USER_GUIDE.md's Edit-menu verb list was missing Flip Horizontal/Vertical and Redo. Cross-checked against upstream's equivalent fix (#75) and applied only the parts that describe code we actually ship — skipped the detect_rooms/MCP tool-count parts of that commit, since we deliberately didn't pull that tool in (see PR #156's description).
…hape Finish shape (Enter) sits between Flip Vertical and Undo last point in the real menu — missing from upstream's own equivalent fix too. Reordered the list to match the menu's actual grouping while adding it.
docs: sync FEATURES.md/USER_GUIDE.md for w/Waste, Labor view, Flip H/V
* chore(deploy): remove this repo's production deploy workflow This repo no longer deploys anywhere. Merging to main only builds and tests changes (ci.yml); the deploy workflow and its Netlify secrets are gone. Update AGENTS.md and docs/DEPLOYMENT.md accordingly. * docs(deploy): fix CI description accuracy (per Copilot review) CI runs typecheck/test/build only, not the full npm run check (no lint step). node-version-file resolves relative to the repo root, so CI reads the root .nvmrc, not web/.nvmrc as previously stated.
example-plans/ (real customer plan PDFs used as a local detection corpus) was only ignored via .git/info/exclude, which is machine-local and doesn't travel with clones — a fresh clone or another contributor could commit the corpus to this public repo by accident. Promote the ignore to the tracked .gitignore so the protection is durable.
…icy home again (#163) Restore web/public/_headers and revert netlify.toml to upstream's version; both files are now byte-identical to Kentucky-ai/opentakeoff. Upstream's _headers has since adopted every line this fork's netlify.toml block carried (Google sign-in origins, webfonts, HSTS, FLoC opt-out), so the served policy is unchanged — verified by building and diffing dist/_headers against the old block. Reverses 2026-07-20's 'netlify.toml is the permanent CSP home' decision (#154): now that this repo no longer deploys production (#160) and exists to track upstream with minimal divergence, matching upstream's file layout keeps netlify.toml conflict-free on every future sync. The rule that survives either arrangement: never let both files carry a CSP at once — two CSP headers on one response enforce as their intersection.
…URLs, reserved domains for Workspace examples (#164) Replace every '345 Flooring' / '345flooring.com' / 'takeoff.345flooring.com' / '345constructionco.com' mention. These were illustrative placeholders in docs, .env.example, code comments, and test fixtures — never functional config — so this is a pure neutrality cleanup with no behavior change. Two categories, handled the way the parent's tree does: - App / project URL references (Glide deep-links, the OAuth Authorized-Origin production example, the GOOGLE_SETUP deep-link, the AGENTS.md live-demo callout, the PARENT_FORK_PORTS 'Shipped (production)' heading) -> 'opentakeoff.netlify.app'. This public fork exists to contribute back to the parent (Kentucky-ai/opentakeoff), whose deployment IS opentakeoff.netlify.app, so that's the project URL these docs should name. (The parent's own CHANGELOG re-pointed these same strings there after the 2026-07-13 history merge.) - Google Workspace / email domain examples (VITE_GOOGLE_HD, ALLOWED_HD, the auth/identity/branding/hdDrift test fixtures) -> RFC 2606 reserved domains example.com / example.org. These can't be a netlify.app subdomain — you can't run a Workspace on one. Case variants preserved (e.g. 345FLOORING.COM -> EXAMPLE.COM) so the case-insensitivity assertions still test what they mean. - Generic trade name 'Acme Flooring' for the identity.js / branding examples. - AGENTS.md also notes this fork serves no deployment of its own and merging here does not deploy (true since #160). No git history rewrite. Demo PDFs carry no 345 branding (verified). 764 tests pass.
…161) * feat(revisions): transfer takeoff to a reissued sheet + auto-flag what changed (#149) Closes out #149's remaining scope. The original design assumed a positional diff between two saved revisions on a matching sheet_id, but a reissued sheet is usually a fresh, shapeless PDF page — so the real workflow is transfer, then flag. - shapeCommands.js: new `resheet` command bulk-moves (not copies) shapes onto a new sheet_id, id-preserving, undoable, no provenance stamp. - TakeoffCanvas.jsx: transferShapesToSheet re-keys a source sheet's shapes onto a destination and auto-saves a baseline revision; addMarkups bulk-adds auto-flag clouds under one shared rev number. - PlanNavigator.jsx: "Transfer takeoff..." action on a shapeless sheet card, picks a source sheet from the working set. - revisionClouds.js: per-shape id diff (added/changed/removed) between a baseline and current, since resheet's id continuity makes real pairing possible instead of the fuzzy bbox-overlap heuristic the issue originally floated. Bbox padded so an auto-placed cloud never blocks clicking the shape it's flagging. - RevisionsPanel.jsx: "Auto-flag changes" button, enabled only when comparing a revision against the live takeoff. Known, accepted limitation: a baseline's computed values freeze at the source sheet's scale; auto-flag reads a later scale change on the destination as a false quantity delta on every transferred shape. Documented inline; the common case (reissue keeps the same scale) doesn't hit it. * fix(review): local subagent review round — id-continuity guard, rev race, error handling Applied findings from a 7-angle local review of PR #161 (7 finders, verified before applying): - revisionClouds.js: the auto-flag id-diff had no guard against being run on a baseline that never went through a resheet transfer — zero shape-id overlap between baseline/current would flood a sheet with every shape reading as simultaneously Removed and Added. Now skips a sheet when both sides have shapes but share no ids at all (a genuinely new/emptied sheet still diffs normally). - TakeoffCanvas.jsx addMarkups: nextRev now computed inside the setMarkups functional updater (was reading the closed-over `markups`, so two rapid Auto-flag clicks — no busy guard on that button — could stamp two passes with the same rev number); Math.max(0, ...spread) replaced with a reduce to avoid the engine's argument-count ceiling on a very large markups array. - TakeoffCanvas.jsx transferShapesToSheet: store.saveSnapshot now has error handling matching the file's established convention (was the only store.save* call site without it) — a failed baseline save now surfaces instead of silently leaving Auto-flag with nothing to diff against. Documented the markup-scope decision (markups don't follow a transfer, same as cross-sheet paste) alongside the existing scale-mismatch note. - RevisionsPanel.jsx tagOf: now searches both baseline and current conditions (was picking one array outright, mislabeling a removed shape "?" if its condition existed only in the baseline) with a defensive fallback instead of an unguarded array reference. - revisionClouds.js: round2 now imported from lib/num.js instead of reimplemented locally. - PlanNavigator.jsx: the source-sheet list for the transfer picker is now memoized once per render (was re-derived per shapeless card, per render) and computed once per card instead of twice. - shapeCommands.js: resheet's restore-row now goes through a small sheetSnapshot helper (mirroring reassign's assignSnapshot) instead of two inline object literals that would drift out of sync. Skipped (noted, not applied): the ~2-number threshold duplication between revisionClouds.js and revisions.js (deliberate — the two modules diff different-shaped things, forcing a shared import was already weighed and rejected in the original design); unifying addMarkups with the existing addMarkup (their tab-focus/highlight-handling behavior diverges for good reason — stamp placement and mid-stroke highlighting need different treatment than a bulk auto-flag batch). 778 tests passing (3 new), lint clean, build clean. * fix(review): CI typecheck failure + Copilot round — atomic transfer, O(1) tag lookup - test/revisionClouds.test.ts: fix a tsc failure (CI's `web` job was red) — asserted a `status` property the return type never had; tsx --test transpiles without typechecking so this only surfaced in CI. - transferShapesToSheet: Copilot correctly flagged that dispatching the resheet move before saving the baseline snapshot left shapes moved with no baseline if the save failed. Restructured to compute the transferred array and save IT as the baseline first — the actual dispatchShape (which commits live state and records the undo entry) only runs after the save succeeds, so a failure now leaves shapes completely untouched instead of needing a rollback. - RevisionsPanel.jsx tagOf: was a .find() over the conditions array per shape (O(shapes × conditions) for the whole diff) — now a single Map built once, same baseline-then-current-wins fallback semantics. - The double-click rev-numbering race Copilot also flagged was already fixed in the prior commit (functional-updater nextRev) — confirmed still intact. 778 tests passing, typecheck/lint/build clean.
Adopts the four features this fork was behind on: - detect_rooms — MCP server's 11th tool, batch room detection from a sheet's own labels (web/src/lib/detectRooms.ts + wiring in mcp/src) - MCP per-tool conformance suite (mcp/test/conformance.test.ts), Ubuntu+Windows - opentakeoff-mcp 0.5.0 - fix for jagged rendering of very large ingested images (#77): autoRenderScale's floor no longer overrides the physical panel-budget cap Fork-specific divergences preserved through the merge (verified): - No deploy workflow (#160) — deploy.yml stays absent, AGENTS/DEPLOYMENT keep the "this repo deploys nothing" stance rather than upstream's "merge = deploy" - web/public/_headers is the CSP home (#163); netlify.toml carries no header block — both byte-identical to upstream - De-branded docs (#164) — zero 345flooring/takeoff.345 strings; dropped the orphaned upstream parenthetical that reintroduced the URL into DEPLOYMENT.md - Marquee multi-select (multiselect tool, multiDownRef busy gate, cursor) - Refined flattenCurve/hitShapeC (the vertex-cap + zero-dim-guard fix rounds) - copilot-review-gate.yml fork workflow - RevisionsPanel naming in docs/code (upstream renamed to SnapshotPanel) - Edit-menu "Finish shape" verb in USER_GUIDE Conflict resolution: curve.js/curve.test.ts/canvasBusy.ts add/add -> ours (the refined cherry-pick); TakeoffCanvas/main.jsx content -> ours at each conflict while keeping auto-merged #72/#77 hunks; docs -> ours where the fork diverges deliberately, upstream's detect_rooms/eleven-tools additions kept where they auto-merged; mcp/server.json -> 0.5.0. web: 773 tests pass. mcp: 36 pass (incl 8 conformance covering detect_rooms).
Keeps the sync branch up to date with main, which advanced while this was in review. #161's revision-transfer changes auto-merged with the sync (no conflicts). web: 787 tests pass.
Sync upstream Kentucky-ai/opentakeoff (2026-07-21): detect_rooms, MCP 0.5.0, render fix
Replace the switch(name) dispatch in executeAgentTool with a private name-to-handler map (HANDLERS), extending the existing DEFS_BY_NAME pattern. Every handler is async so awaiting one uniformly preserves the sync-return and throw-to-catch semantics for all 8 tools. Behavior is byte-identical: success and error paths (validation-failure messages, the per-tool sheet-not-open wording, and the unknown-tool message) are unchanged. The HANDLERS entry shape is a private internal detail, explicitly NON-normative for the public plugin agent-tool descriptor (that lives in #167); it is not exported. Introduce an AgentToolCtx typedef so ctx is typed rather than any. Extend agentTools.test.ts to characterize each of the 8 tools on both the success and error paths, asserting the byte-identical error strings verbatim. Closes #170
… through A plain-object HANDLERS map inherits Object.prototype, so a model-emitted tool name colliding with a prototype member (toString, constructor, hasOwnProperty, __proto__, …) resolved HANDLERS[name] to an inherited function and dispatched it — the old switch matched only its string-literal cases and fell through to the unknown-tool default. Object.create(null) restores byte-identical parity: such names now hit the !handler guard and return `Unknown tool: <name>.`, matching the old switch fall-through. Adds a regression test over the prototype-key names (mutation-verified: it goes red against a plain-literal map).
There was a problem hiding this comment.
Pull request overview
Internal refactor of executeAgentTool to replace a switch(name) dispatch with a private name→handler map (mirroring the existing DEFS_BY_NAME pattern), while preserving existing tool behavior and hardening against Object.prototype key collisions.
Changes:
- Replaced
executeAgentTool’s switch-based dispatch with a null-prototypeHANDLERSmap and a handler existence guard. - Added/expanded characterization tests to pin byte-for-byte behavior across all 8 tools (success and error paths), including regression coverage for prototype-key tool names.
- Strengthened
view_regioncoverage to ensure the result is an explicit pick (no accidental leakage of extra fields).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| web/src/lib/agentTools.js | Introduces null-prototype handler-map dispatch and preserves unknown-tool semantics for prototype-key collisions. |
| web/test/agentTools.test.ts | Expands characterization suite to lock behavior and adds regression tests for prototype-key tool names + view_region keyset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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
Pure internal refactor of
executeAgentTool: theswitch(name)dispatch is replaced by a private name→handler map, extending the existingDEFS_BY_NAMEpattern already used inagentTools. This is an internal cleanup only — it introduces no new public or plugin surface. (The naming here is explicitly non-normative; the real plugin descriptor lands in #167.)Behavior parity
Behavior is byte-identical across all 8 agent tools, on both the success and error paths. This is proven by the extended
agentTools.test.tscharacterization suite, which exercises each tool's outputs before and after the refactor.Bug caught + fixed in review
The review round caught a real bug: tool names that collide with
Object.prototypekeys (toString,constructor,hasOwnProperty, …) dispatched the inherited prototype function instead of falling through to the unknown-tool path. A plain object literal for the handler map inherits these keys, soHANDLERS["toString"]would resolve toObject.prototype.toStringand be invoked as a handler.Fixed by building the handler map with a null prototype (
Object.create(null)), so prototype keys are absent and unknown names fall through correctly. A regression test covering prototype-key tool names guards against reintroduction.Verification
No browser surface, so no screenshots — verification is the test suite. Full local gate is green: typecheck, lint, test (861 tests), and build. Branch-only.
Closes #170
Part of #179 (epic)