Merged
Conversation
Closed
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces Hub identity provider authentication support with credential-based sign-in, refactors analysis table polling from PodOrc-specific checks to a global 15-second refresh interval, revises log-fetching to support incremental updates with query parameters (start_date, limit), improves log viewer auto-scroll behavior, and extends the API with new log analytics endpoints (netstats, LogQL query, request counts) plus filtering capabilities on existing log endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser
participant HubPage as Hub Auth Page
participant Nuxt as Nuxt Auth
participant HubServer as Hub OAuth Server
participant AppServer as App Server
Client->>HubPage: Opens /auth/hub
Client->>HubPage: Enters username & password
Client->>HubPage: Clicks "Sign in"
HubPage->>Nuxt: signIn("hub", {username, password, callbackUrl: "/"})
Nuxt->>AppServer: POST /api/auth/signin
AppServer->>HubServer: POST /token (credentials grant)
HubServer-->>AppServer: Returns access/refresh tokens
AppServer-->>Nuxt: JWT set (tokens in user object)
Nuxt-->>Client: Redirect to /
rect rgba(100, 150, 200, 0.5)
Note over Client,AppServer: Credentials flow handled by<br/>JWT callback for account.type === "credentials"
end
sequenceDiagram
participant Table as AnalysesTable
participant Timer as Refresh Timer
participant ContainerLogs as ContainerLogs
participant API as API
participant Server as Server
Note over Table,Server: Initial Load
Table->>API: GET /analysis-nodes/:id
API-->>Table: Analysis data
ContainerLogs->>API: GET /logs/:id (limit: null)
API-->>ContainerLogs: All historical logs
ContainerLogs->>ContainerLogs: Set lastFetchedAt
Note over Table,Server: Every 15 seconds (Table) / Per execution (ContainerLogs)
Timer->>Table: Trigger refresh()
Table->>API: GET /analyses
API-->>Table: Updated analyses
ContainerLogs->>API: GET /logs/:id?start_date=<lastFetchedAt>
API-->>ContainerLogs: Incremental logs since timestamp
ContainerLogs->>ContainerLogs: Append new logs
ContainerLogs->>ContainerLogs: Update lastFetchedAt
rect rgba(150, 200, 100, 0.5)
Note over ContainerLogs: Polling driven by<br/>execution_status === Executing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
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. Review rate limit: 0/1 reviews remaining, refill in 8 minutes and 47 seconds.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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/components/analysis/logs/ContainerLogs.vue (1)
81-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
analysis.valueis null whenuseIntervalFninitializes, causingimmediateto always be false.The
useIntervalFnsetup runs synchronously beforefetchAnalysis()completes (line 33). At line 86,analysis.valueis stillnull, soanalysis.value!.execution_statusevaluates toundefined, andimmediatewill always befalse—even when the analysis is currently executing.🐛 Proposed fix
Move the interval setup after fetching, or use a watcher to start/stop polling based on status:
-const { pause, resume, isActive } = useIntervalFn( - () => { - refreshLogs(); - }, - 5000, - { immediate: analysis.value!.execution_status === ProcessStatus.Executing }, -); +const { pause, resume, isActive } = useIntervalFn( + () => { + refreshLogs(); + }, + 5000, + { immediate: false }, +); + +// Start polling after analysis is fetched if currently executing +watch( + () => analysis.value?.execution_status, + (status) => { + if (status === ProcessStatus.Executing) { + resume(); + } else { + pause(); + } + }, + { immediate: true }, +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/analysis/logs/ContainerLogs.vue` around lines 81 - 87, The interval is initialized before fetchAnalysis() completes so analysis.value is null and immediate is always false; change the logic so polling starts/stops based on analysis.execution_status after data is fetched or changes: either move the useIntervalFn setup until after fetchAnalysis() resolves (so call useIntervalFn/create the interval once analysis.value is set) or keep useIntervalFn but remove the immediate check and add a watcher on analysis.value or analysis.value.execution_status that calls resume() when status === ProcessStatus.Executing and pause() otherwise; reference useIntervalFn, refreshLogs, fetchAnalysis(), analysis.value, pause, resume, and isActive to locate and update the code.app/components/landing/IdpAuthBtns.vue (1)
24-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute Hub users to
/auth/hubinstead of invoking provider auth directly.When
idpProvider === "hub", Line 28 skips the new credentials page and immediately starts provider auth. That leaves the landing-page login CTA unable to perform the username/password flow introduced in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/landing/IdpAuthBtns.vue` around lines 24 - 29, The button currently calls signIn(idpProvider) for all providers; change the click handler logic so when idpProvider === "hub" it navigates to "/auth/hub" instead of invoking signIn, otherwise call signIn(idpProvider); locate the template/button using idpProvider and the signIn method in IdpAuthBtns.vue and implement the conditional navigation (e.g., via the component's router push or a wrapper method) so the "hub" provider opens the new credentials page.
🧹 Nitpick comments (1)
app/components/analysis/logs/AnalysisLogCardContent.vue (1)
24-36: 💤 Low valueModule-level state will be shared across all component instances.
The
nginxWasAtBottomandanalysisWasAtBottomvariables are declared at module scope withlet, meaning they'll be shared if multiple instances of this component are mounted simultaneously. While likely only one instance exists in practice, converting to refs would be safer.♻️ Suggested refactor using refs
-let nginxWasAtBottom = true; -let analysisWasAtBottom = true; +const nginxWasAtBottom = ref(true); +const analysisWasAtBottom = ref(true); onBeforeUpdate(() => { if (nginxLogBottom.value) { const c = getScrollContainer(nginxLogBottom.value); - nginxWasAtBottom = c ? isAtBottom(c) : true; + nginxWasAtBottom.value = c ? isAtBottom(c) : true; } if (analysisLogBottom.value) { const c = getScrollContainer(analysisLogBottom.value); - analysisWasAtBottom = c ? isAtBottom(c) : true; + analysisWasAtBottom.value = c ? isAtBottom(c) : true; } });And update
onUpdatedsimilarly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/analysis/logs/AnalysisLogCardContent.vue` around lines 24 - 36, nginxWasAtBottom and analysisWasAtBottom are currently module-level lets and will be shared across component instances; change them to reactive refs inside the component's setup (e.g., const nginxWasAtBottom = ref(true), const analysisWasAtBottom = ref(true)) and update all usages in onBeforeUpdate and onUpdated to read/write .value; ensure you still call getScrollContainer(nginxLogBottom.value) and getScrollContainer(analysisLogBottom.value) and pass the resulting container to isAtBottom, but store the boolean result into the correspondingRef.value so state is instance-scoped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/analysis/AnalysesTable.vue`:
- Around line 149-156: The repeated toast comes from always calling
showConnectionErrorToast in the promise .catch inside AnalysesTable.vue; add a
persisted component state flag (e.g., data/ref name isPoReachable) that you set
to true when the PO fetch succeeds and set to false only when a fetch fails;
change the .catch handlers that call showConnectionErrorToast (the one at the
reported .catch and the similar block at lines 158-159) to only invoke the toast
when isPoReachable is currently true and then set isPoReachable = false, and
ensure successful fetch paths set isPoReachable = true so toasts are only
emitted on transitions reachable → unreachable.
- Around line 264-267: pollTableData is async and can overlap because there’s no
in-flight guard around compileAnalysisTable which updates analysesMap, and
getExecutionStatusesFromPodOrc emits repeated warning toasts on each failure;
add an in-flight guard (e.g., a boolean like isPolling or a Promise lock) around
pollTableData/compileAnalysisTable to skip new invocations while one is running
and ensure compileAnalysisTable/refresh mutate analysesMap only when the guard
is held, and modify getExecutionStatusesFromPodOrc to suppress repeated toasts
(track last error or throttle warnings) so a persistent connection failure
doesn’t show a toast every 15s.
In `@app/components/analysis/logs/ContainerLogs.vue`:
- Around line 46-58: The fetchAnalysis function currently always calls the API
even if analysisNodeId is undefined; add an early-return guard at the top of
fetchAnalysis that checks if analysisNodeId is nullish (undefined or null) and
returns immediately to avoid calling `/analysis-nodes/undefined`. Locate
fetchAnalysis and reference analysisNodeId and analysis.value; only proceed with
the useNuxtApp().$hubApi call when analysisNodeId is present, otherwise skip the
request and leave analysis.value unchanged.
In `@app/pages/auth/hub.vue`:
- Around line 21-31: The signIn call is using redirect: true which causes it to
navigate away and return void, so result?.error is never populated; change the
signIn call in this component (the signIn(...) invocation) to use redirect:
false, await the returned SignInResponse, set errorMsg.value when result?.error
exists, and only call router.push("/") when the response indicates success (no
error / result?.ok), ensuring username.value and password.value are passed as
before.
In `@app/services/hub_adapter_swagger.json`:
- Around line 3010-3031: The /history/{analysis_id} OpenAPI schema currently
declares the query params "limit" and "offset" as non-nullable integers causing
422s when the frontend sends null; update the /history/{analysis_id} parameter
schemas for "limit" and "offset" so they match /logs/{analysis_id] (i.e., allow
nulls) by either adding "nullable": true or changing the type to
["integer","null"] for both parameters; modify the parameter entries named
"limit" and "offset" in the /history/{analysis_id} path in
hub_adapter_swagger.json so the API accepts null values sent by
ContainerLogs.vue.
In `@server/routes/flame/api/auth/`[...].ts:
- Around line 66-96: The Hub provider is defined as an OAuth provider
(hubProvider with type: "oauth") but must be a credentials provider to accept
signIn("hub", { username, password }); change hubProvider.type to "credentials",
add a credentials schema (username, password) and implement an
authorize(credentials) async function on hubProvider that validates the
username/password (e.g., call the Hub auth/token or user endpoint), returns a
user object { id, name } on success or null on failure, and keep pushing
hubProvider into providers so the downstream account.type === "credentials"
branch is reached.
- Around line 149-152: The discovery currently always fetches from clientIssuer
and assigns tokenEndpoint from discovery.token_endpoint, which removes the
previous fallback to the internal Keycloak URL; modify the logic in the refresh
token discovery to try fetching
`${clientIssuer}/.well-known/openid-configuration` and if that fetch fails or
the resulting discovery object has no token_endpoint, retry (or fall back) to
the internal issuer URL (env var NUXT_PUBLIC_INTERNAL_KEYCLOAK_URL) before
assigning tokenEndpoint; update the code paths that use discovery and
tokenEndpoint (references: discovery, tokenEndpoint, clientIssuer) to use the
fallback discovery when necessary.
---
Outside diff comments:
In `@app/components/analysis/logs/ContainerLogs.vue`:
- Around line 81-87: The interval is initialized before fetchAnalysis()
completes so analysis.value is null and immediate is always false; change the
logic so polling starts/stops based on analysis.execution_status after data is
fetched or changes: either move the useIntervalFn setup until after
fetchAnalysis() resolves (so call useIntervalFn/create the interval once
analysis.value is set) or keep useIntervalFn but remove the immediate check and
add a watcher on analysis.value or analysis.value.execution_status that calls
resume() when status === ProcessStatus.Executing and pause() otherwise;
reference useIntervalFn, refreshLogs, fetchAnalysis(), analysis.value, pause,
resume, and isActive to locate and update the code.
In `@app/components/landing/IdpAuthBtns.vue`:
- Around line 24-29: The button currently calls signIn(idpProvider) for all
providers; change the click handler logic so when idpProvider === "hub" it
navigates to "/auth/hub" instead of invoking signIn, otherwise call
signIn(idpProvider); locate the template/button using idpProvider and the signIn
method in IdpAuthBtns.vue and implement the conditional navigation (e.g., via
the component's router push or a wrapper method) so the "hub" provider opens the
new credentials page.
---
Nitpick comments:
In `@app/components/analysis/logs/AnalysisLogCardContent.vue`:
- Around line 24-36: nginxWasAtBottom and analysisWasAtBottom are currently
module-level lets and will be shared across component instances; change them to
reactive refs inside the component's setup (e.g., const nginxWasAtBottom =
ref(true), const analysisWasAtBottom = ref(true)) and update all usages in
onBeforeUpdate and onUpdated to read/write .value; ensure you still call
getScrollContainer(nginxLogBottom.value) and
getScrollContainer(analysisLogBottom.value) and pass the resulting container to
isAtBottom, but store the boolean result into the correspondingRef.value so
state is instance-scoped.
🪄 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: 0a0f2dbe-1ee3-4a6a-8d21-ca199a611cca
📒 Files selected for processing (11)
app/components/analysis/AnalysesTable.vueapp/components/analysis/AnalysisControlButtons.vueapp/components/analysis/logs/AnalysisLogCardContent.vueapp/components/analysis/logs/ContainerLogs.vueapp/components/header/AvatarButton.vueapp/components/landing/IdpAuthBtns.vueapp/composables/useAPIFetch.tsapp/pages/auth/hub.vueapp/services/Api.tsapp/services/hub_adapter_swagger.jsonserver/routes/flame/api/auth/[...].ts
Comment on lines
+149
to
+152
| const discovery = await fetch( | ||
| `${clientIssuer}/.well-known/openid-configuration`, | ||
| ).then((r) => r.json()); | ||
| const tokenEndpoint: string = discovery.token_endpoint; |
There was a problem hiding this comment.
Keep the internal issuer fallback for refresh discovery.
Line 149 now always discovers from NUXT_PUBLIC_IDP_ISSUER. That drops the existing NUXT_PUBLIC_INTERNAL_KEYCLOAK_URL path used elsewhere in this file, so refresh can start failing in deployments where the public issuer is only reachable from the browser.
Suggested fix
const clientSecret = process.env.NUXT_IDP_CLIENT_SECRET ?? "";
const clientIssuer =
process.env.NUXT_PUBLIC_IDP_ISSUER ?? "http://localhost:8080/realms/flame";
+ const discoveryIssuer =
+ process.env.NUXT_PUBLIC_INTERNAL_KEYCLOAK_URL ?? clientIssuer;
- const discovery = await fetch(
- `${clientIssuer}/.well-known/openid-configuration`,
- ).then((r) => r.json());
+ const discoveryResponse = await fetch(
+ `${discoveryIssuer}/.well-known/openid-configuration`,
+ );
+ if (!discoveryResponse.ok) {
+ throw new Error("Failed to load OIDC discovery document");
+ }
+ const discovery = await discoveryResponse.json();
const tokenEndpoint: string = discovery.token_endpoint;📝 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.
Suggested change
| const discovery = await fetch( | |
| `${clientIssuer}/.well-known/openid-configuration`, | |
| ).then((r) => r.json()); | |
| const tokenEndpoint: string = discovery.token_endpoint; | |
| const clientSecret = process.env.NUXT_IDP_CLIENT_SECRET ?? ""; | |
| const clientIssuer = | |
| process.env.NUXT_PUBLIC_IDP_ISSUER ?? "http://localhost:8080/realms/flame"; | |
| const discoveryIssuer = | |
| process.env.NUXT_PUBLIC_INTERNAL_KEYCLOAK_URL ?? clientIssuer; | |
| const discoveryResponse = await fetch( | |
| `${discoveryIssuer}/.well-known/openid-configuration`, | |
| ); | |
| if (!discoveryResponse.ok) { | |
| throw new Error("Failed to load OIDC discovery document"); | |
| } | |
| const discovery = await discoveryResponse.json(); | |
| const tokenEndpoint: string = discovery.token_endpoint; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes/flame/api/auth/`[...].ts around lines 149 - 152, The discovery
currently always fetches from clientIssuer and assigns tokenEndpoint from
discovery.token_endpoint, which removes the previous fallback to the internal
Keycloak URL; modify the logic in the refresh token discovery to try fetching
`${clientIssuer}/.well-known/openid-configuration` and if that fetch fails or
the resulting discovery object has no token_endpoint, retry (or fall back) to
the internal issuer URL (env var NUXT_PUBLIC_INTERNAL_KEYCLOAK_URL) before
assigning tokenEndpoint; update the code paths that use discovery and
tokenEndpoint (references: discovery, tokenEndpoint, clientIssuer) to use the
fallback discovery when necessary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements