Skip to content

fix(dashboards): carry dashboard chat context ids, label the chart readout, isolate repository tests - #8499

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/dashboards-review-followups
Oct 1, 2026
Merged

waleedlatif1 merged 2 commits into
stagingfrom
fix/dashboards-review-followups

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Follow-ups to the dashboard findings in the release review. Three findings were real and are fixed here. Five were checked and need no change; the reasons are below.

Fixed

Dashboard mention loses its dashboardId in user messages (release review: home/types.ts "dashboardId is never populated")

  • Root cause: ChatMessageContext declares dashboardId, and persisted-message.ts stores it, but neither client-side mapping copied it:
    • the optimistic user message in use-chat.ts;
    • toDisplayContexts in display-message.ts, which rebuilds the contexts when a persisted chat is reopened.
  • Fix: both mappings now copy dashboardId, using the same pattern as the other id fields.
  • Behaviour change: a dashboard mention in a sent or reopened user message now carries its id. Today nothing reads that field from a message context, so the chip looks the same. The home.tsx:318 path the finding cites resolves composer ChatContexts, which already carry the id.
  • Test: display-message.test.ts adds "keeps the dashboard id of a reopened dashboard mention". It fails without the display-message.ts line.

Time-series readout aria-label is ignored (release review: time-series-chart.tsx)

  • Root cause: ARIA prohibits naming the generic role, so assistive technology ignores an aria-label on a plain div.
  • Fix: when the readout has values, the row gets role='group', so its "<label> values" name is exposed. With no values it stays an unnamed container, so screen readers never meet an empty named group.
  • No test was added: a test would only restate the attribute.

Repository revision test depends on the test before it (release review: repository.integration.ts)

  • Root cause: the revision test read the ws-a row that the first test inserts. Run on its own, it dereferenced null.
  • Fix: the test now seeds its own ws-c dashboard.

Not changed

  • formatChartValue precision (lib/charts/summary.ts): this is the intended display policy, not a bug. The readout is a compact Total/Avg summary of the plotted buckets. lib/dashboards/README.md and the function's TSDoc document it as two decimals, or three significant digits below 1, so small values never round to 0.
  • Sidebar Dashboard gated on the Files capability (sidebar.tsx): this matches enforcement. dashboardOperations.read and dashboardOperations.save both require capability: 'files.use' (lib/dashboards/application/operations.ts). A Files-restricted member is refused by the route as well, so hiding or locking the entry is correct.
  • Organization chats advertise the dashboards entitlement (payload.ts): organization chats run workspace commands with an explicit --workspace ID per invocation. The worker's extractCliWorkspace and parse.ts require it. The tool then runs against that invocation's trusted workspace, and every dashboard operation re-checks availability for that workspace. The entitlement is therefore meaningful in organization chats.
  • Read contract uses unbounded strings (lib/api/contracts/dashboards.ts): the bounds belong on input. The write path enforces them. The GET response describes stored data, and adding the write bounds there could only turn an already-stored row into a response error. It would not prevent anything.
  • 'Dashboard' missing from GENERIC_RESOURCE_TITLES (resources/types.ts): a dashboard's name is always 'Dashboard' (lib/dashboards/application/dashboards.ts). There is no rename, so there is no more specific title to upgrade to.

Test plan

  • display-message.test.ts: the new test fails before the fix and passes after it.
  • Affected unit suites pass: display-message, use-chat, charts, dashboards (14 files, 154 tests).
  • bun run test:integration lib/dashboards/repository.integration.ts passes (2 tests).
  • bun run lint
  • bun run type-check (apps/sim)
  • bun run check:audits (52 audits)
  • Manual screen-reader check of the readout (not done)

…adout, isolate repository tests

- User message contexts keep a dashboard mention's dashboardId, both in the
  optimistic message and when a persisted message is reopened, matching the
  context the server stores.
- The time-series readout row has role="group", so its aria-label is exposed to
  assistive technology.
- The revision test in the dashboard repository suite seeds its own workspace
  instead of depending on the previous test's row.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@vercel

vercel Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
docs Skipped Skipped Oct 1, 2026 1:36am UTC

Request Review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai 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.

No issues found across 5 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Adds dashboard context tracking to chat and fixes chart labeling.

The PR appears safe to merge; no outstanding or new actionable findings were identified.

Summary

This PR preserves dashboard IDs in sent and reopened chat mentions, names the chart readout only when it contains values, and makes the dashboard revision test independent of another test.

Reviews (2) · Last reviewed commit: "fix(dashboards): name the chart readout ..."

Comment thread apps/sim/components/charts/time-series-chart.tsx Outdated

@cubic-dev-ai cubic-dev-ai 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.

All reported issues were addressed across 5 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/components/charts/time-series-chart.tsx Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai 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.

No issues found across 5 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@waleedlatif1
waleedlatif1 merged commit 7e8d3ff into staging Oct 1, 2026
23 of 24 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/dashboards-review-followups branch October 1, 2026 01:46

This branch was previously deployed

1 inactive deployment
Preview — baef45a1 Deployed Oct 1, 2026 by vercel[bot]
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.

1 participant