Skip to content

Commit 13a1894

Browse files
committed
fix(usage): repair the chart family's shared geometry and the audit export
Follow-up review of the previous commit, across the whole diff. The CSV export ignored the workspace filter. The contract accepted it, the on-screen feed applied it, and the export route dropped it on the floor — every field of `AuditLogFilterParams` is optional, so omitting one still type-checks. An admin exporting a workspace-scoped feed downloaded the whole organization, under a truncation warning that blamed the date range. The route now forwards the parsed query whole and refuses an out-of-organization id the way the list route does, with tests for both. The line chart's derived hover index was not clamped. The previous commit replaced stored state with a derivation, but reproduced the clamp only for the bar chart: `padding.left` follows the axis labels and `chartWidth` follows the container, so a sidebar collapse mid-hover pushed the ratio past 1 and indexed off the end — the dot, the rule and the tooltip all vanished until the cursor moved. Per-chart gutters de-aligned the logs dashboard, where three line charts sit in one row. Deriving each from its own labels put their plot origins at 26, 27 and 32 where they had shared one. The gutter now rounds up to a step, which collapses differences that small and leaves several pixels of slack instead of the sub-pixel margin `Math.ceil` alone gave. The radar chart, reviewed against its siblings: - Its hover targets were triangles, whose far edge is the chord. Along its own spoke a triangle reaches only `reach·cos(π/n)` — at three axes, 50px against a 74px radius — so the largest value's vertex, the one a reader aims at, sat outside every target. They are arc sectors now. - The tooltip was positioned against the scroll container rather than the plot, so below the width floor it stayed nailed while the web slid under it. It now sits beside the hovered vertex through the family's own placer, instead of covering the densest part of the gradient. - Captions below the centreline rode ~3px off the ring rather than the gap they were given, and captions beside the web were misaligned from their own vertex. - Web opacities, stroke width, vertex radius, the per-theme fill relationship and the `screen` blend now match the bar and line charts rather than freelancing. - Its rings read the shared grid fractions instead of dividing into even steps that agreed with the siblings only by coincidence. Also: one `expanded` flag drove both lists in the workspace drill-down, so opening either tail silently opened the other's; the Overview and tab lists rendered an inert `Other` row while the same row two clicks away was a button; the expand chevron knocked the value column out of alignment; row hover had regressed to the chip surface where every other settings row uses the active one; and the radar and the list beside it printed two different `Other (N more)` counts under identical wording. The Overview's two readings of the source data now share one section rather than drawing two half-width rules on one line. Rendered-geometry tests cover the clipping and caption bugs against the real SVG — a unit test of the helpers could not have caught either, since both came from a callsite combining correct helpers wrongly.
1 parent aca5d24 commit 13a1894

23 files changed

Lines changed: 713 additions & 304 deletions

File tree

apps/sim/app/api/audit-logs/export/route.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,4 +164,29 @@ describe('GET /api/audit-logs/export', () => {
164164
expect(response.status).toBe(400)
165165
expect(mockQueryAuditLogs).not.toHaveBeenCalled()
166166
})
167+
168+
/**
169+
* The export has to filter by everything the on-screen feed does. It did not
170+
* forward `workspaceId`, so an admin exporting from a workspace-scoped feed
171+
* downloaded the whole organization — silently, because every field of
172+
* `AuditLogFilterParams` is optional and dropping one still type-checks.
173+
*/
174+
it('forwards the workspace filter the on-screen feed applies', async () => {
175+
mockGetOrgWorkspaceIds.mockResolvedValue(['workspace-1'])
176+
177+
await GET(makeRequest('?workspaceId=workspace-1'))
178+
179+
expect(mockBuildFilterConditions).toHaveBeenCalledWith(
180+
expect.objectContaining({ workspaceId: 'workspace-1' })
181+
)
182+
})
183+
184+
it('rejects a workspaceId outside the organization, as the list route does', async () => {
185+
mockGetOrgWorkspaceIds.mockResolvedValue(['workspace-1'])
186+
187+
const response = await GET(makeRequest('?workspaceId=workspace-elsewhere'))
188+
189+
expect(response.status).toBe(400)
190+
expect(mockQueryAuditLogs).not.toHaveBeenCalled()
191+
})
167192
})

apps/sim/app/api/audit-logs/export/route.ts

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
6666
}
6767

6868
const { organizationId, orgMemberIds } = authResult.context
69-
const { search, action, resourceType, actorId, startDate, endDate, includeDeparted } =
70-
parsed.data.query
69+
const { actorId, workspaceId, includeDeparted } = parsed.data.query
7170

7271
if (actorId && !orgMemberIds.includes(actorId)) {
7372
return NextResponse.json(
@@ -77,20 +76,34 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
7776
}
7877

7978
const orgWorkspaceIds = await getOrgWorkspaceIds(organizationId)
79+
/**
80+
* The same refusal `listAuditLogs` gives. The scope predicate already makes an
81+
* out-of-organization id return nothing, but an empty CSV and a 400 that names the
82+
* problem are very different answers to the same bad request, and the two paths
83+
* disagreeing about which one you get is what an audit trail cannot afford.
84+
*/
85+
if (workspaceId && !orgWorkspaceIds.includes(workspaceId)) {
86+
return NextResponse.json(
87+
{ error: 'workspaceId does not belong to your organization' },
88+
{ status: 400 }
89+
)
90+
}
8091
const scopeCondition = buildOrgScopeCondition({
8192
organizationId,
8293
orgWorkspaceIds,
8394
orgMemberIds,
8495
includeDeparted,
8596
})
86-
const filterConditions = buildFilterConditions({
87-
action,
88-
resourceType,
89-
actorId,
90-
search,
91-
startDate,
92-
endDate,
93-
})
97+
/**
98+
* The whole parsed query, not a hand-listed subset.
99+
*
100+
* Every field of `AuditLogFilterParams` is optional, so dropping one type-checks
101+
* silently — which is how `workspaceId` came to be accepted by the contract,
102+
* honoured by the list route, and ignored here: an admin looking at one
103+
* workspace's feed downloaded the entire organization's, under a truncation
104+
* warning that blamed the date range.
105+
*/
106+
const filterConditions = buildFilterConditions(parsed.data.query)
94107
const conditions = [scopeCondition, ...filterConditions]
95108

96109
const rows: ReturnType<typeof formatAuditLogEntry>[] = []

apps/sim/app/workspace/[workspaceId]/settings/components/billing/components/usage-limit-field/usage-limit-field.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,6 @@ export function UsageLimitField({
135135
: String(dollarsToCredits(currentLimit))
136136
}
137137
disabled={!canEdit}
138-
inputClassName='[appearance:textfield] [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-outer-spin-button]:appearance-none'
139138
/>
140139
</SettingsSection>
141140
)

apps/sim/components/charts/bar-chart.tsx

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -117,12 +117,9 @@ function BarChartComponent({
117117
const barWidth = Math.max(1, Math.min(24, slot * 0.7))
118118

119119
/**
120-
* The hovered bucket, derived from the stored cursor rather than stored beside it.
121-
*
122-
* Bars own a slot, so this is which slot the cursor is in — not the nearest sample,
123-
* which is how a line chart resolves it. Deriving it means a resize mid-hover cannot
124-
* leave an index that disagrees with the slot geometry it was measured against, and
125-
* a mouse move writes one state instead of two.
120+
* Bars own a slot, so the hovered bucket is which slot the cursor is in — not the
121+
* nearest sample, which is how a line chart resolves it. Derived, so a resize
122+
* mid-hover cannot leave an index disagreeing with the slot geometry.
126123
*/
127124
const hoverIndex =
128125
hoverPos === null || data.length === 0 || slot <= 0
@@ -203,12 +200,6 @@ function BarChartComponent({
203200
contradicted the constant's own note that the chart "scrolls rather than
204201
compresses". At or above the floor there is no overflow and nothing changes.
205202
*/
206-
/*
207-
`overflow-y-hidden` is not redundant with `overflow-x-auto`: a computed
208-
`overflow-x` other than `visible` promotes `overflow-y: visible` to `auto`,
209-
so the tooltip's shadow reaching the foot of the box raised a vertical
210-
scrollbar over the chart whenever the cursor neared the axis.
211-
*/
212203
'w-full overflow-x-auto overflow-y-hidden',
213204
!hasExternalWrapper && 'rounded-lg border bg-[var(--surface-1)] p-4 shadow-card'
214205
)}

apps/sim/components/charts/chart-geometry.test.ts

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -83,19 +83,32 @@ describe('chartPlotBand', () => {
8383
})
8484

8585
describe('resolveChartPadding', () => {
86-
/**
87-
* The bug this exists for: a fixed 26px gutter left 18px of drawable width once
88-
* `CHART_AXIS_LABEL_GAP` was taken out, and a right-anchored `7.3k` at 9px is wider
89-
* than that — so its first glyph was cut off at the container's left edge.
90-
*/
9186
it('widens the gutter until the longest label fits beside the axis', () => {
9287
const { left } = resolveChartPadding(['7.3k', '0'])
9388
expect(left).toBeGreaterThanOrEqual(estimateAxisLabelWidth('7.3k') + CHART_AXIS_LABEL_GAP)
9489
})
9590

96-
it('never narrows below the shared padding, so short labels stay aligned', () => {
97-
expect(resolveChartPadding(['0', '0']).left).toBe(CHART_PADDING.left)
98-
expect(resolveChartPadding([]).left).toBe(CHART_PADDING.left)
91+
it('never narrows below the shared padding', () => {
92+
expect(resolveChartPadding(['0', '0']).left).toBeGreaterThanOrEqual(CHART_PADDING.left)
93+
expect(resolveChartPadding([]).left).toBeGreaterThanOrEqual(CHART_PADDING.left)
94+
})
95+
96+
/**
97+
* Three charts sit side by side on the logs dashboard. A gutter derived exactly from
98+
* each one's own labels put their plot origins at 26, 27 and 32 — visibly ragged
99+
* across a row that used to share one origin.
100+
*/
101+
it('resolves labels of similar width to the same gutter', () => {
102+
const gutters = [['5'], ['1.2s'], ['12.3k'], ['0'], ['7.3k']].map(
103+
(labels) => resolveChartPadding(labels).left
104+
)
105+
expect(new Set(gutters).size).toBe(1)
106+
})
107+
108+
it('still grows for a genuinely wider label', () => {
109+
expect(resolveChartPadding(['123456.7m']).left).toBeGreaterThan(
110+
resolveChartPadding(['7.3k']).left
111+
)
99112
})
100113

101114
it('leaves the other three sides on the shared constant', () => {
@@ -104,10 +117,4 @@ describe('resolveChartPadding', () => {
104117
expect(padding.right).toBe(CHART_PADDING.right)
105118
expect(padding.bottom).toBe(CHART_PADDING.bottom)
106119
})
107-
108-
it('grows monotonically with label length', () => {
109-
const short = resolveChartPadding(['1.2k']).left
110-
const long = resolveChartPadding(['123456.7m']).left
111-
expect(long).toBeGreaterThan(short)
112-
})
113120
})

apps/sim/components/charts/chart-geometry.ts

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,38 @@ export type ChartPadding = { top: number; right: number; bottom: number; left: n
1515
/** Matches the loader placeholders callers size themselves against. */
1616
export const CHART_DEFAULT_HEIGHT = 166
1717

18-
/** Below this the axis labels collide, so the chart scrolls rather than compresses. */
18+
/**
19+
* Below this the axis labels collide, so the chart scrolls rather than compresses.
20+
*
21+
* Consumers pair `overflow-x-auto` with `overflow-y-hidden`: a computed `overflow-x`
22+
* other than `visible` promotes `overflow-y: visible` to `auto`, so the tooltip's
23+
* shadow reaching the foot of the box raised a vertical scrollbar over the chart
24+
* whenever the cursor neared the axis.
25+
*/
1926
export const CHART_MIN_WIDTH = 280
2027

2128
export const CHART_TICK_FILL = 'var(--text-tertiary)'
2229
export const CHART_TICK_FONT_SIZE = 9
2330
export const CHART_GRID_FRACTIONS = [0.25, 0.5, 0.75] as const
2431

32+
/** Punctuation and whitespace, which sit near half the width of a digit or letter. */
33+
const NARROW_GLYPH = /[.,:\s]/
34+
2535
/** Gap between a y-axis tick label's right edge and the axis rule. */
2636
export const CHART_AXIS_LABEL_GAP = 8
2737

38+
/**
39+
* The gutter is rounded up to a multiple of this.
40+
*
41+
* Charts are read side by side — the logs dashboard puts three in one row — and a
42+
* gutter derived exactly from each chart's own labels made `5`, `1.2s` and `12.3k`
43+
* resolve to 26, 27 and 32, so three plots that used to share an origin no longer
44+
* did. Quantizing collapses differences this small to one value while still growing
45+
* for a genuinely wider label, and it turns the sub-pixel slack that `Math.ceil`
46+
* alone left into several pixels.
47+
*/
48+
const CHART_AXIS_GUTTER_STEP = 8
49+
2850
/**
2951
* Rendered width of a right-anchored y-axis tick label.
3052
*
@@ -37,7 +59,7 @@ export const CHART_AXIS_LABEL_GAP = 8
3759
export function estimateAxisLabelWidth(text: string): number {
3860
let width = 0
3961
for (const character of text) {
40-
width += /[.,:\s]/.test(character) ? 0.3 : 0.58
62+
width += NARROW_GLYPH.test(character) ? 0.3 : 0.58
4163
}
4264
return width * CHART_TICK_FONT_SIZE
4365
}
@@ -54,9 +76,10 @@ export function estimateAxisLabelWidth(text: string): number {
5476
*/
5577
export function resolveChartPadding(yAxisLabels: readonly string[]): ChartPadding {
5678
const widest = yAxisLabels.reduce((max, label) => Math.max(max, estimateAxisLabelWidth(label)), 0)
79+
const required = Math.max(CHART_PADDING.left, widest + CHART_AXIS_LABEL_GAP)
5780
return {
5881
...CHART_PADDING,
59-
left: Math.max(CHART_PADDING.left, Math.ceil(widest) + CHART_AXIS_LABEL_GAP),
82+
left: Math.ceil(required / CHART_AXIS_GUTTER_STEP) * CHART_AXIS_GUTTER_STEP,
6083
}
6184
}
6285

0 commit comments

Comments
 (0)