feat(canvas): web analytics template + date-range picker#2657
feat(canvas): web analytics template + date-range picker#2657adamleithp wants to merge 1 commit into
Conversation
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/core/src/canvas/dashboardQueryService.test.ts:29-108
**Non-parameterised shape tests**
The six happy-path shape cases (`scalar`, `column`, `labels`, `matrix`, `pairs`, `retention`) share the same fixture → run → `toMatchObject` structure and could be collapsed into a single `it.each` table. The two failure tests (lines 82-101) also share `expect(r.ok).toBe(false)` and are a natural second table. Keeping them as individual `it` blocks violates the project's "always prefer parameterised tests" convention and makes the list harder to extend when new shapes are added.
### Issue 2 of 2
packages/ui/src/features/canvas/components/ChannelsList.tsx:58-62
Closed channels write `"0"` to `localStorage`, but the comment above `useChannelOpen` says "only an explicit open is stored." The `"0"` entries are redundant because absent keys are also read as `false`; over many channels they accumulate needless storage entries. Remove the `"0"` write (or remove the stored key entirely on close) to match the documented intent.
```suggestion
try {
if (next) {
localStorage.setItem(key, "1");
} else {
localStorage.removeItem(key);
}
} catch {
// Ignore storage failures (private mode, quota) — state still works in-session.
}
```
Reviews (1): Last reviewed commit: "feat(canvas): web analytics template + d..." | Re-trigger Greptile |
| describe("DashboardQueryService shape mapping", () => { | ||
| it("scalar reads row 0, col 0", async () => { | ||
| const [r] = await serviceReturning([[42]]).run({ | ||
| queries: [query("scalar")], | ||
| }); | ||
| expect(r).toMatchObject({ ok: true, value: 42 }); | ||
| }); | ||
|
|
||
| it("column collects the first cell of every row", async () => { | ||
| const [r] = await serviceReturning([[1], [2], [3]]).run({ | ||
| queries: [query("column")], | ||
| }); | ||
| expect(r).toMatchObject({ ok: true, value: [1, 2, 3] }); | ||
| }); | ||
|
|
||
| it("labels stringifies the first column", async () => { | ||
| const [r] = await serviceReturning([["Jun 4"], ["Jun 5"]]).run({ | ||
| queries: [query("labels")], | ||
| }); | ||
| expect(r).toMatchObject({ ok: true, value: ["Jun 4", "Jun 5"] }); | ||
| }); | ||
|
|
||
| it("matrix keeps every row as an array", async () => { | ||
| const [r] = await serviceReturning([ | ||
| ["/", 10, 20], | ||
| ["/pricing", 5, 8], | ||
| ]).run({ queries: [query("matrix")] }); | ||
| expect(r).toMatchObject({ | ||
| ok: true, | ||
| value: [ | ||
| ["/", 10, 20], | ||
| ["/pricing", 5, 8], | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("pairs maps rows to {label,value}", async () => { | ||
| const [r] = await serviceReturning([ | ||
| ["Direct", 5], | ||
| ["Organic", 3], | ||
| ]).run({ queries: [query("pairs")] }); | ||
| expect(r).toMatchObject({ | ||
| ok: true, | ||
| value: [ | ||
| { label: "Direct", value: 5 }, | ||
| { label: "Organic", value: 3 }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("retention maps rows to {label,size,values}", async () => { | ||
| const [r] = await serviceReturning([["Jun 1", 100, 100, 9]]).run({ | ||
| queries: [query("retention")], | ||
| }); | ||
| expect(r).toMatchObject({ | ||
| ok: true, | ||
| value: [{ label: "Jun 1", size: 100, values: [100, 9] }], | ||
| }); | ||
| }); | ||
|
|
||
| it("fails a scalar that isn't a string/number", async () => { | ||
| const [r] = await serviceReturning([[{ nested: true }]]).run({ | ||
| queries: [query("scalar")], | ||
| }); | ||
| expect(r.ok).toBe(false); | ||
| }); | ||
|
|
||
| it("fails a column whose cells are non-numeric (mis-shaped query)", async () => { | ||
| const [r] = await serviceReturning([["Direct"], ["Organic"]]).run({ | ||
| queries: [query("column")], | ||
| }); | ||
| expect(r.ok).toBe(false); | ||
| }); | ||
|
|
||
| it("treats null column cells as empty buckets (0), not a failure", async () => { | ||
| const [r] = await serviceReturning([[5], [null], [3]]).run({ | ||
| queries: [query("column")], | ||
| }); | ||
| expect(r).toMatchObject({ ok: true, value: [5, 0, 3] }); | ||
| }); |
There was a problem hiding this comment.
The six happy-path shape cases (scalar, column, labels, matrix, pairs, retention) share the same fixture → run → toMatchObject structure and could be collapsed into a single it.each table. The two failure tests (lines 82-101) also share expect(r.ok).toBe(false) and are a natural second table. Keeping them as individual it blocks violates the project's "always prefer parameterised tests" convention and makes the list harder to extend when new shapes are added.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/canvas/dashboardQueryService.test.ts
Line: 29-108
Comment:
**Non-parameterised shape tests**
The six happy-path shape cases (`scalar`, `column`, `labels`, `matrix`, `pairs`, `retention`) share the same fixture → run → `toMatchObject` structure and could be collapsed into a single `it.each` table. The two failure tests (lines 82-101) also share `expect(r.ok).toBe(false)` and are a natural second table. Keeping them as individual `it` blocks violates the project's "always prefer parameterised tests" convention and makes the list harder to extend when new shapes are added.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| try { | ||
| localStorage.setItem(key, next ? "1" : "0"); | ||
| } catch { | ||
| // Ignore storage failures (private mode, quota) — state still works in-session. | ||
| } |
There was a problem hiding this comment.
Closed channels write
"0" to localStorage, but the comment above useChannelOpen says "only an explicit open is stored." The "0" entries are redundant because absent keys are also read as false; over many channels they accumulate needless storage entries. Remove the "0" write (or remove the stored key entirely on close) to match the documented intent.
| try { | |
| localStorage.setItem(key, next ? "1" : "0"); | |
| } catch { | |
| // Ignore storage failures (private mode, quota) — state still works in-session. | |
| } | |
| try { | |
| if (next) { | |
| localStorage.setItem(key, "1"); | |
| } else { | |
| localStorage.removeItem(key); | |
| } | |
| } catch { | |
| // Ignore storage failures (private mode, quota) — state still works in-session. | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/canvas/components/ChannelsList.tsx
Line: 58-62
Comment:
Closed channels write `"0"` to `localStorage`, but the comment above `useChannelOpen` says "only an explicit open is stored." The `"0"` entries are redundant because absent keys are also read as `false`; over many channels they accumulate needless storage entries. Remove the `"0"` write (or remove the stored key entirely on close) to match the documented intent.
```suggestion
try {
if (next) {
localStorage.setItem(key, "1");
} else {
localStorage.removeItem(key);
}
} catch {
// Ignore storage failures (private mode, quota) — state still works in-session.
}
```
How can I resolve this? If you propose a fix, please make it concise.Canvas template system (Dashboard / Web analytics / Blank) with a create
picker, per-template agent prompts + component allow-lists, rename, the
Quill component pass, and the showcase seed — plus the Web analytics
template and its date-range picker.
- Templates: canvasTemplates + templateSchemas + CanvasTemplatesService,
templateId on canvas records, NewCanvasMenu picker, per-template chat
suggestions.
- Web analytics template: KPI row, visitor trends with prior-period
comparison, sources, geography, retention, active hours.
- New catalog components: Heatmap and RetentionGrid.
- DateTimePicker in the toolbar (quill): named ranges roll to now, Custom
pins to absolute; view re-runs queries, edit feeds the agent via a
[Range] prompt line.
- Multi-value query refresh: shape-tagged queries mapped onto props via a
JSON-pointer setter; timezone-safe {date_from}/{date_to}(+_prev) tokens.
- Channels remember collapsed state in localStorage (closed by default).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
75f4d8f to
b9a5559
Compare
|
React Doctor could not complete this scan.
Reviewed by React Doctor for commit |
What
A new Web analytics canvas template that drives the agent to build a PostHog-style board (KPI row, visitor trends with prior-period comparison, sources, geography, retention, active hours) for a selectable date range.
Highlights
Heatmap(day×hour activity) andRetentionGrid(cohort bars) — schema + bodies + registry wiring.[Range]prompt line.shape(scalar/column/labels/matrix/pairs/retention) mapped onto props via a JSON-pointer setter. Mis-shaped numeric columns fail (instead of charting zeros); identical values skip the file write (no poll churn).{date_from}/{date_to}(+_prevfor comparison series) substituted as UTC-instanttoDateTime(<unix>)literals.localStorage(closed by default).stickyHeaderwith a capped scroll viewport; Filter button disabled (not wired yet).Test plan
pnpm --filter @posthog/core test— 1415 pass (incl. newdashboardQueryServiceshape-mapping tests).pnpm typecheck(core/ui/host-router) + biome lint clean.🤖 Generated with Claude Code