Add buttons to Grafana and snapshot - #305
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Grafana snapshot persistence (DB model + migration), server API (GET/POST /snapshots) with password validation, shared DTOs, client route/hooks for snapshots, Analysis tab UI for external Grafana links and a Grafana History iframe, and env example placeholders for dashboard open URLs and server snapshot password. ChangesGrafana snapshots and open-in-new-tab links
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant Database
Client->>Server: GET /snapshots (useSnapshots)
Server->>Database: read grafana_snapshot rows (getSnapshots)
Database-->>Server: snapshot list
Server-->>Client: 200 SnapshotListResponseDTO
Client->>Server: POST /snapshots (useCreateSnapshot with url,label,password)
Server->>Server: validate password and URL hostname/HTTPS
Server->>Database: insert grafana_snapshot (createSnapshot)
Database-->>Server: created row or unique-constraint error
Server-->>Client: 201 created or 409 conflict
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/client/src/components/molecules/GrafanaMolecules/GrafanaFrame.tsx`:
- Around line 24-25: The URL builder in GrafanaFrame currently only sets theme
when missing; change it to always override the theme query param so runtime
theme changes take effect. In the function that constructs the iframe URL (look
for GrafanaFrame / the variable url and the theme variable), replace the
conditional url.searchParams.has("theme") check with an unconditional
url.searchParams.set("theme", theme) so the theme param is always updated before
returning url.toString().
- Around line 35-41: resolvedTheme from useTheme may be undefined on first
render causing grafanaTheme to default to "light" and the iframe to load
prematurely; add a mounted guard state and set it true in a useEffect after
mount, then only compute normalizedEmbedUrl and render the iframe when mounted
is true and resolvedTheme is defined (update the useMemo dependencies to include
mounted or move the withGrafanaEmbedDefaults call behind the mounted check).
Locate the useTheme/resolvedTheme usage and grafanaTheme calculation in
GrafanaFrame, and ensure withGrafanaEmbedDefaults(embedUrl, grafanaTheme) runs
only after mounted is true to prevent the initial light-theme iframe load.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 430827c7-8ed5-416c-88e6-5a018db5185b
📒 Files selected for processing (4)
packages/client/.env.examplepackages/client/src/components/molecules/GrafanaMolecules/GrafanaFrame.tsxpackages/client/src/components/tabs/AnalysisTab.tsxpackages/client/src/objects/TabRoutes.tsx
| if (!url.searchParams.has("theme")) url.searchParams.set("theme", theme); | ||
| return url.toString(); |
There was a problem hiding this comment.
Always override the theme query parameter.
Line 24 only sets theme when absent, so URLs that already contain theme=... won’t follow runtime theme changes. Force-set theme every time.
Proposed fix
- if (!url.searchParams.has("theme")) url.searchParams.set("theme", theme);
+ url.searchParams.set("theme", theme);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!url.searchParams.has("theme")) url.searchParams.set("theme", theme); | |
| return url.toString(); | |
| url.searchParams.set("theme", theme); | |
| return url.toString(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client/src/components/molecules/GrafanaMolecules/GrafanaFrame.tsx`
around lines 24 - 25, The URL builder in GrafanaFrame currently only sets theme
when missing; change it to always override the theme query param so runtime
theme changes take effect. In the function that constructs the iframe URL (look
for GrafanaFrame / the variable url and the theme variable), replace the
conditional url.searchParams.has("theme") check with an unconditional
url.searchParams.set("theme", theme) so the theme param is always updated before
returning url.toString().
| const { resolvedTheme } = useTheme(); | ||
| const grafanaTheme = resolvedTheme === "dark" ? "dark" : "light"; | ||
|
|
||
| const normalizedEmbedUrl = useMemo(() => { | ||
| if (!embedUrl) return ""; | ||
| return withGrafanaEmbedDefaults(embedUrl, grafanaTheme); | ||
| }, [embedUrl, grafanaTheme]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In next-themes, can resolvedTheme be undefined on initial render, and is a mounted guard recommended to avoid initial theme flicker/reload?
💡 Result:
Yes, in next-themes, resolvedTheme (along with other values returned by useTheme) can be undefined on the initial render [1][2]. This occurs because the library cannot access client-side storage (like localStorage) or system preferences during server-side rendering (SSR) or static site generation (SSG) [1][2]. Using a mounted guard is highly recommended to avoid hydration mismatch errors and UI inconsistencies [1][2]. Because the server and client will have different initial values for the theme, attempting to render UI components that depend on the current theme before the component has mounted on the client will trigger a hydration mismatch warning [1][2]. To implement this, you should use a state variable (e.g., mounted) that is initialized to false and updated to true within a useEffect hook, which only runs on the client [1][2]. You can then conditionally render your theme-dependent UI only after the component has mounted [1][2]. Example implementation: const [mounted, setMounted] = useState(false); const { theme, setTheme } = useTheme; useEffect( => { setMounted(true); }, []); if (!mounted) { return null; // Or a skeleton/placeholder to avoid layout shift } return ( <button onClick={ => setTheme(theme === 'dark'? 'light': 'dark')}> Toggle Theme ); While next-themes automatically injects a script to prevent theme flashing on page load by setting the correct attributes on the html element [1][2], the mounted guard is specifically necessary to ensure that your React components remain in sync with the theme state during the hydration process [1][2].
Citations:
Add a mounted guard to prevent iframe reload before theme resolution.
resolvedTheme is undefined on initial render in next-themes, causing the ternary operator to default to "light". This triggers an iframe load before the actual theme is available, resulting in a theme flicker when resolvedTheme resolves. Use a mounted state variable updated in a useEffect to conditionally render the component only after the theme is available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client/src/components/molecules/GrafanaMolecules/GrafanaFrame.tsx`
around lines 35 - 41, resolvedTheme from useTheme may be undefined on first
render causing grafanaTheme to default to "light" and the iframe to load
prematurely; add a mounted guard state and set it true in a useEffect after
mount, then only compute normalizedEmbedUrl and render the iframe when mounted
is true and resolvedTheme is defined (update the useMemo dependencies to include
mounted or move the withGrafanaEmbedDefaults call behind the mounted check).
Locate the useTheme/resolvedTheme usage and grafanaTheme calculation in
GrafanaFrame, and ensure withGrafanaEmbedDefaults(embedUrl, grafanaTheme) runs
only after mounted is true to prevent the initial light-theme iframe load.
fc87723 to
1c578b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/client/src/components/tabs/AnalysisTab.tsx`:
- Around line 65-67: The filter currently uses link.url?.trim() but leaves the
original untrimmed value in the objects, so update the creation/normalization
step for the link list to trim and overwrite link.url (e.g., normalize each item
before filtering) so every link object stored/returned has url =
link.url?.trim(); ensure the change touches the same link objects used by the
filter callback (the function referencing link.url and the .filter(...) check)
so subsequent uses (rendering hrefs) always get the trimmed URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ef1578e-1768-4385-ac18-b9e618327c4d
📒 Files selected for processing (1)
packages/client/src/components/tabs/AnalysisTab.tsx
- Implemented snapshot creation and retrieval in the backend. - Added new API routes for managing Grafana snapshots. - Created a new database table for storing snapshot details. - Developed frontend components for adding and displaying snapshots. - Integrated snapshot functionality into the AnalysisTab component. - Updated environment variables and validation for snapshot management. - Enhanced error handling and logging for snapshot operations.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/server/src/utils/validatePassword.ts (1)
37-43: 💤 Low valueConsider constant-time comparison for the shared secret.
password === snapshotPasswordshort-circuits on the first differing byte, leaking timing information. For a shared snapshot password, prefercrypto.timingSafeEqualover===. Note the existingvalidateDriverUpdatePasswordhas the same pattern, so this is consistency hardening rather than a regression.🔒 Proposed constant-time check
+import { timingSafeEqual } from "crypto"; + export function validateSnapshotPassword(password: string): boolean { const snapshotPassword = process.env.SNAPSHOT_PASSWORD; if (!snapshotPassword) { throw new Error("SNAPSHOT_PASSWORD environment variable is not configured"); } - return password === snapshotPassword; + const a = Buffer.from(password); + const b = Buffer.from(snapshotPassword); + return a.length === b.length && timingSafeEqual(a, b); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/utils/validatePassword.ts` around lines 37 - 43, Replace the direct equality checks with a constant-time comparison: in validateSnapshotPassword (and mirror the change in validateDriverUpdatePassword) read SNAPSHOT_PASSWORD/driver env into a string, convert both inputs to Buffers, return false immediately if lengths differ, otherwise use crypto.timingSafeEqual(bufA, bufB) to compute equality and return that boolean; keep the existing environment-variable missing throw and ensure you import/require Node's crypto at the top of the module.packages/client/src/hooks/useSnapshots.ts (1)
45-53: ⚡ Quick winMultiple error notifications may appear during query retries.
React Query (v5) defaults to 3 retries. Each failed retry produces a new
query.errorreference, triggering thisuseEffectagain. Users could see up to 4 identical "Failed to load Grafana snapshots" toasts for a single network outage.Consider disabling retries for this query or deduplicating notifications:
Option A: Disable retries
const query = useQuery({ gcTime: 1000 * 60 * 5, placeholderData: [], queryFn: fetchSnapshots, queryKey: [SNAPSHOTS_QUERY_KEY] as const, refetchOnWindowFocus: false, + retry: false, staleTime: 1000 * 60 * 2, throwOnError: false, });Option B: Use query.isError with a stable notification id
useEffect(() => { - if (query.error) { + if (query.isError) { notifications.show({ color: "red", + id: "snapshots-load-error", message: "Failed to load Grafana snapshots.", title: "Error", }); } -}, [query.error]); +}, [query.isError]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client/src/hooks/useSnapshots.ts` around lines 45 - 53, The hook useSnapshots triggers duplicate toasts because query.error changes across React Query retries; fix by disabling retries for that query (set retry: false in the useQuery/options where query is created inside useSnapshots) or deduplicate notifications by using query.isError and a stable notification id (call notifications.show with a constant id like "grafana-snapshots-error" when query.isError) so only one toast is shown; update the useQuery options or replace the useEffect error check from if (query.error) to if (query.isError) and use a stable id when calling notifications.show.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/client/src/hooks/useSnapshots.ts`:
- Around line 45-53: The hook useSnapshots triggers duplicate toasts because
query.error changes across React Query retries; fix by disabling retries for
that query (set retry: false in the useQuery/options where query is created
inside useSnapshots) or deduplicate notifications by using query.isError and a
stable notification id (call notifications.show with a constant id like
"grafana-snapshots-error" when query.isError) so only one toast is shown; update
the useQuery options or replace the useEffect error check from if (query.error)
to if (query.isError) and use a stable id when calling notifications.show.
In `@packages/server/src/utils/validatePassword.ts`:
- Around line 37-43: Replace the direct equality checks with a constant-time
comparison: in validateSnapshotPassword (and mirror the change in
validateDriverUpdatePassword) read SNAPSHOT_PASSWORD/driver env into a string,
convert both inputs to Buffers, return false immediately if lengths differ,
otherwise use crypto.timingSafeEqual(bufA, bufB) to compute equality and return
that boolean; keep the existing environment-variable missing throw and ensure
you import/require Node's crypto at the top of the module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86a12a9d-a427-4db9-a7a1-c6e4201f2e4d
📒 Files selected for processing (17)
packages/client/src/components/tabs/AnalysisTab.tsxpackages/client/src/constants/apiRoutes.tspackages/client/src/hooks/useSnapshots.tspackages/client/src/objects/TabRoutes.tsxpackages/db/README.mdpackages/db/prisma/migrations/0_init/migration.sqlpackages/db/prisma/migrations/20260526021109_add_grafana_snapshot/migration.sqlpackages/db/prisma/migrations/migration_lock.tomlpackages/db/prisma/schema.prismapackages/db/src/services/DatabaseService.tspackages/server/.env.examplepackages/server/src/controllers/routeControllers/snapshot.controller.tspackages/server/src/index.tspackages/server/src/routes/snapshot.route.tspackages/server/src/utils/validatePassword.tspackages/shared/src/dto/index.tspackages/shared/src/dto/snapshot.ts
✅ Files skipped from review due to trivial changes (5)
- packages/shared/src/dto/index.ts
- packages/server/.env.example
- packages/db/prisma/migrations/migration_lock.toml
- packages/db/prisma/migrations/20260526021109_add_grafana_snapshot/migration.sql
- packages/client/src/objects/TabRoutes.tsx
dastine0308
changed the title
| ######################## | ||
| DRIVER_NAME_UPDATE_PASSWORD=changeme | ||
| FINISH_LINE_UPDATE_PASSWORD=changeme | ||
| SNAPSHOT_PASSWORD=changeme |
There was a problem hiding this comment.
Lets not use a new password for every single new feature on our frontend.
Instead, we can have a master password that will control the entire ui, and it becomes easy to change and manage.
To implement in a future pr...
For now, just use either of the existing passwords:
DRIVER_NAME_UPDATE_PASSWORD=changeme
FINISH_LINE_UPDATE_PASSWORD=changeme
There was a problem hiding this comment.
we gonna do this in afuture p4
| ######################## | ||
| DRIVER_NAME_UPDATE_PASSWORD=changeme | ||
| FINISH_LINE_UPDATE_PASSWORD=changeme | ||
| SNAPSHOT_PASSWORD=changeme |
There was a problem hiding this comment.
we gonna do this in afuture p4
- Fetch only the most recent snapshot end-to-end instead of all rows
- Extract the inline serialize input type into shared IGrafanaSnapshotRow
- Make AddSnapshotForm fully controlled (drop FormData/formRef)
- Replace hardcoded #F05A28 with a semantic `grafana` Tailwind token
- Use a single {" · "} separator in the snapshot caption
| const baseUrl = latestSnapshot?.url ?? null; | ||
|
|
||
| // The server only stores embeddable snapshots.raintank.io URLs, so we can | ||
| // build the iframe src directly and just append the theme query param. |
There was a problem hiding this comment.
consider deleting this in your next pr. Your code should be self documented (written in a way that doesn't require documentation.
5 participants
Screenshot
Summary by CodeRabbit
New Features
Chores