Skip to content

Switch activity graph to canvas#1366

Open
skyfallwastaken wants to merge 3 commits into
mainfrom
ag-canvas
Open

Switch activity graph to canvas#1366
skyfallwastaken wants to merge 3 commits into
mainfrom
ag-canvas

Conversation

@skyfallwastaken
Copy link
Copy Markdown
Member

Summary of the problem

The activity graph currently uses 365 (!!) DOM nodes.

Describe your changes

Switch to a canvas, which reduces the node count to 1.

Screenshots / Media

N/A. Looks the same!

Copilot AI review requested due to automatic review settings May 26, 2026 17:33
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR replaces the signed-in Home “daily activity” grid (previously rendered as many <Link> elements) with a single <canvas> that draws the graph, reducing DOM node count and shifting styling to CSS custom properties.

Changes:

  • Reimplemented ActivityGraph.svelte to render a canvas-based heatmap and handle hover/click interactions in JS.
  • Moved activity “intensity” colors from per-cell CSS classes to :root CSS variables for canvas color sampling.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
app/javascript/pages/Home/signedIn/ActivityGraph.svelte Replaces per-day link grid with canvas drawing + pointer/keyboard handlers.
app/assets/tailwind/main.css Defines --activity-cell-* variables used by the canvas renderer.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/javascript/pages/Home/signedIn/ActivityGraph.svelte
Comment thread app/javascript/pages/Home/signedIn/ActivityGraph.svelte
Comment thread app/javascript/pages/Home/signedIn/ActivityGraph.svelte
Comment thread app/javascript/pages/Home/signedIn/ActivityGraph.svelte Outdated
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 26, 2026

Greptile Summary

This PR replaces the activity graph's 365 individual <Link> DOM nodes with a single <canvas> element, reducing the rendered node count to one. The canvas reads theme colors via getComputedStyle on CSS custom properties and re-draws reactively through $effect.

  • Canvas rendering: drawGraph correctly handles HiDPI via devicePixelRatio, draws rounded rectangles using roundRect, and reads theme-aware colors from CSS custom properties — the CSS side converts five .activity-cell--N rules into :root custom properties for this purpose.
  • Interaction handling: pointer move/leave, click, and keyboard events are wired up, but the onKeydown handler is only useful after mouse interaction (see comment), which is a meaningful accessibility regression from the old <Link> approach.
  • Hit detection: dateFromPointer uses modulo arithmetic to map cursor coordinates to date cells, but has a 1px boundary error and does not guard against a zero-column edge case.

Confidence Score: 3/5

The canvas rendering and CSS color refactor work correctly, but keyboard navigation is broken and there are two smaller correctness bugs in hit-detection and width calculation.

The keyboard handler silently does nothing for keyboard-only users because hoveredDate can only be set via mouse movement, which is a real behavioral regression from the previous Link-per-day approach. The 1px hit-detection error and the negative-width edge case are lower-stakes but present defects in the changed code.

app/javascript/pages/Home/signedIn/ActivityGraph.svelte needs attention, particularly the onKeydown handler and dateFromPointer logic.

Important Files Changed

Filename Overview
app/javascript/pages/Home/signedIn/ActivityGraph.svelte Replaces 365 Link DOM nodes with a single canvas element; introduces canvas drawing, pointer hit-testing, and keyboard handler — but keyboard navigation is broken (requires prior mouse hover to set hoveredDate), hit detection is 1px too wide at each cell edge, and graphWidth can go negative when columns === 0.
app/assets/tailwind/main.css Converts five .activity-cell--N CSS class rules into :root CSS custom properties (--activity-cell-0 through --activity-cell-4) so the canvas can read them at draw time via getComputedStyle; straightforward CSS refactor with no issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Component Mount] --> B[effect drawGraph]
    B --> C[Read graphWidth and graphHeight]
    C --> D[Set canvas dimensions x devicePixelRatio]
    D --> E[activityColors via getComputedStyle]
    E --> F[Iterate dates array]
    F --> G[intensityLevel per date]
    G --> H[Set fillStyle from colors array]
    H --> I[roundRect and fill]
    I --> F

    J[User moves pointer] --> K[onPointerMove]
    K --> L[dateFromPointer modulo hit test]
    L --> M{In cell?}
    M -- Yes --> N[hoveredDate = date]
    M -- No --> O[hoveredDate = null]

    P[User presses Enter or Space] --> Q{hoveredDate set?}
    Q -- Yes --> R[router.visit date param]
    Q -- No --> S[Nothing happens - keyboard regression]
Loading
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
app/javascript/pages/Home/signedIn/ActivityGraph.svelte:121-126
**Keyboard navigation broken for keyboard-only users**

The `onKeydown` handler only navigates if `hoveredDate` is non-null, but `hoveredDate` is exclusively set by `onPointerMove` (mouse/touch input). A keyboard-only user who tabs to the canvas will be unable to activate any date — pressing Enter or Space does nothing. The original `<Link>`-per-day DOM approach gave keyboard users individual focusable, navigable elements for free. This canvas approach needs a separate mechanism (e.g., tracking a `focusedIndex` state that arrow keys move through) to restore that capability.

### Issue 2 of 3
app/javascript/pages/Home/signedIn/ActivityGraph.svelte:103-104
Off-by-one in hit detection: `<= cellSize` includes the first pixel of the gap (pixel index `cellSize` is the start of the gap, not the end of the cell). With `cellSize = 12` and `cellGap = 4`, the stride is 16, so pixels 0–11 are in the cell and pixel 12 starts the gap. The check should be `< cellSize`.

```suggestion
    const inCellX = x % (cellSize + cellGap) < cellSize;
    const inCellY = y % (cellSize + cellGap) < cellSize;
```

### Issue 3 of 3
app/javascript/pages/Home/signedIn/ActivityGraph.svelte:19
When `columns` is 0 (possible if `dates.length === 0`), the formula evaluates to `0 * 12 + (0 - 1) * 4 = -4`, which is a negative canvas width. Setting `canvas.width` to a negative number is invalid and will silently produce a broken canvas.

```suggestion
  const graphWidth = $derived(columns > 0 ? columns * cellSize + (columns - 1) * cellGap : 0);
```

Reviews (1): Last reviewed commit: "Switch to CSS vars" | Re-trigger Greptile

Comment thread app/javascript/pages/Home/signedIn/ActivityGraph.svelte
Comment thread app/javascript/pages/Home/signedIn/ActivityGraph.svelte Outdated
Comment thread app/javascript/pages/Home/signedIn/ActivityGraph.svelte Outdated
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.

2 participants