Skip to content

feat(dataweaver): Card overlapping at creation - #474

Open
pablonoel wants to merge 10 commits into
datacommonsorg:mainfrom
pablonoel:ui-cards_overlaping
Open

pablonoel wants to merge 10 commits into
datacommonsorg:mainfrom
pablonoel:ui-cards_overlaping

Conversation

@pablonoel

@pablonoel pablonoel commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Overview

Cards could be placed on top of each other as they were created, and a set of
cards arriving together was positioned no differently from a single card. This
change makes placement aware both of what is already on the canvas and of which
cards belong to the same set.

Placement now checks whether the next slot in the current row is free before
using it, and wraps to a new row when something is already sitting there. A row
that wraps drops below both the row being tracked and the lowest card anywhere
on the canvas, so a card the user has dragged underneath the grid, or a sibling
created moments earlier in the same paste, is cleared rather than covered.

Cards that arrive together now behave as a set. A query result, a paste and a
duplicate each open a row of their own rather than filling whatever slots were
left over from earlier work, and that row is centered beneath the existing
content according to the combined width of the cards it will hold. When a card
opens a new row the camera frames the whole canvas, so an arriving set is seen
in the context of what was already there. PLACEMENT.md describes the full
model.

Changes Made

  • Placement checks the next slot in the row before using it, and wraps when
    another card already occupies that space.
  • A row that wraps drops below both the tracked row and the lowest card on the
    canvas, whichever sits lower.
  • Cards arriving together open a row of their own. A RowStart argument marks
    the first card of a set: the store subscriber supplies it for query results,
    and a wrapper around editor.createShapes supplies it for pastes, drops and
    duplicates. The wrapper restores the editor's own method on cleanup.
  • A new row is centered beneath the canvas's content on the combined width of
    the set that opens it. A set whose widths cannot be determined keeps the
    grid's current horizontal position rather than guessing, because a guess
    biases the row and the bias accumulates across successive rows.
  • The camera frames the whole canvas when a card opens a new row, instead of
    framing only the grid currently being built.
  • PLACEMENT.md now documents how a row picks its horizontal position, the
    store visibility rule that placement depends on during a paste, and the
    afterCreate ordering caveat that affects camera framing.
  • register_card_placement.test.ts covers the wrap floor, per-set rows,
    centering, drift across repeated pastes, clone placement and blocked slots.

No new UI surface is introduced, but where cards land on creation does change.
Before and after recordings for desktop and mobile viewports are attached below.

Testing Done

The following were run from dataweaver/:

nvm use && pnpm lint
nvm use && pnpm test
nvm use && pnpm build

Lint passes across tsc, Biome and Stylelint. The suite passes with 126 tests
across 18 files. The production build completes.

The new tests were also run against the placement code on main, to confirm
that they describe the change rather than merely restating it. Five of the nine
fail there: the new row's clearance of the lowest card, both centering cases,
the horizontal drift across repeated sets, and the blocked-slot check.

The behavior was then checked by hand in the browser:

  1. Run a query returning several cards and confirm they arrive on one row.
  2. Select all of them, copy, and paste. The pasted set appears below the lowest
    card, centered under the existing content, overlapping nothing.
  3. Paste several more times. Each row stays centered on the same point rather
    than creeping left.
  4. Duplicate a mixed selection of chart and table cards, which differ in width,
    and confirm the row is still centered.
  5. Narrow the window below the tablet breakpoint and repeat steps 2 to 4. Cards
    stack into one column with no overlap.
  • Unit tests passed
  • Integration tests passed — not required; there is no integration suite
    covering canvas placement, and the behavior is exercised by unit tests
    against a stubbed editor.
  • Manual verification
  • Any updated goldens or fixtures were reviewed and are intentional — not
    required; no goldens or fixtures changed.

Risk & Rollback

The wrapper around editor.createShapes is the only notable risk. It is
installed when the placement system registers and removed when that system is
cleaned up, so it cannot outlive the canvas that installed it, and a signature
change in a future tldraw release would fail type-checking at build time rather
than misbehaving at runtime.

Nothing here touches configuration, stored data or an API boundary, so rolling
back means reverting the commits.

Checklist

  • I have read AGENTS.md and followed CODING_GUIDELINES.md, plus
    FRONTEND.md for UI changes.
  • I have run the app's lint, test, and build commands, as documented in
    that application's guide.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.

Note: Only Maintainers can approve and merge PRs. Expected initial review
time: 3 business days.

Fix card placement when the tracked cursor row becomes stale after cards are moved or inserted. This change adds a canvas-floor calculation and overlap checks so a new row always starts below the lowest existing card, and in-row inserts are skipped when a card already occupies that slot. This prevents cards from being laid on top of other cards when the row height no longer reflects the current canvas state.
@pablonoel pablonoel changed the title feat(dataweaver): Card overlapping and creation feat(dataweaver): Card overlapping at creation Sep 12, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves card placement logic on the canvas by ensuring new cards do not overlap with existing ones and that new rows start below the lowest card on the canvas. The feedback suggests optimizing performance on larger canvases by refactoring canvasFloorY to accept pre-fetched shapes and lazily fetching and reusing the page shapes list to avoid redundant traversals.

Comment thread dataweaver/apps/web/src/components/scopes/atlas/register_card_placement.ts Outdated
Comment thread dataweaver/apps/web/src/components/scopes/atlas/register_card_placement.ts Outdated
Avoid repeated calls to getCurrentPageShapes while calculating card slots. The placement logic now uses a cached snapshot for canvas floor and collision checks, keeping new rows positioned consistently below existing cards and preventing stale shape data during drag placement.
Add a forceNewRow flag to nextSlot and track placedThisTask so the first card placed during a task always starts a fresh row (prevents batches from splitting into leftover gaps). placedThisTask is reset using queueMicrotask to scope the behavior to the current microtask. Update placement logic and comments to skip row-fit when forceNewRow is true.
@pablonoel
pablonoel marked this pull request as ready for review September 14, 2026 15:28
pablonoel and others added 6 commits September 14, 2026 16:38
Add canvasContentXRange to compute the horizontal extent of existing cards. When a batch starts a new row, compute rowStartX so the batch's full-width row is centered under the canvas content and store it in the cursor.

Introduce lastPlacementStartedNewRow to track when a placement started a new row and pass it into keepInView as frameWholeCanvas so the viewport frames the entire canvas (instead of only the new grid) after such placements. Update keepInView signature and framing logic accordingly.
A new row took its y from the lowest card the store had published, but a
multi-shape `store.put` publishes nothing until it commits, so every
wrapping card in a paste landed on its siblings. Take the y from the max
of the canvas floor and the cursor's tracked row.
Replace the `placedThisTask` microtask flag with an explicit `RowStart`
argument from the store subscriber, which knows the real batch boundary;
an SSE chunk is not a query result. Center a new row on the batch's real
widths rather than assuming a full row of the first card's width, which
sent successive pastes walking left. Narrow collision exclusion to the
current row, merge the two canvas scans into one, and add
`register_card_placement.test.ts`.
Describe batches, clones, and the store visibility rule the placement
code depends on, and record the `afterCreate` ordering caveat behind the
camera behavior.
Clones are positioned in a `beforeCreate` handler, which tldraw hands one
record at a time, so the first clone of a paste could not see its siblings
and its row fell back to the grid's x while query rows were centered. Read
the batch from `editor.createShapes`, the one method every creation passes
through, and center the row on the width it will really occupy.
@pablonoel
pablonoel requested a review from juliawu as a code owner September 15, 2026 16:39

@juliawu juliawu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this looks great!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is now very, very long. I'd suggest in a follow-up to split this into separate files, perhaps split by responsibility: camera controls, grid/geometry calculations, lifecycle place/cleanup functions.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants