Add fournos-ui dashboard - #97
Merged
Merged
Conversation
Web dashboard for managing Fournos performance testing jobs. Provides job submission, live monitoring, scheduling, and historical results -- built with FastAPI, HTMX, and PostgreSQL. Co-authored-by: Cursor <cursoragent@cursor.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThis PR introduces a complete new "fournos-ui" FastAPI dashboard application for monitoring, submitting, and scheduling FournosJob custom resources. It includes PostgreSQL persistence, Kubernetes/Tekton API integration, a background watcher, Forge project discovery, HTMX/Jinja templates, static assets, Dockerfile, and Kustomize deployment manifests. ChangesFournos Dashboard Application
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant FastAPI
participant Kubernetes
participant PostgreSQL
Browser->>FastAPI: submit job or open job detail
FastAPI->>Kubernetes: create FournosJob or fetch live job data
FastAPI->>PostgreSQL: upsert history row on submit
FastAPI->>Kubernetes: stream logs or resolve PipelineRun data
sequenceDiagram
participant Watcher
participant Kubernetes
participant PostgreSQL
Watcher->>Kubernetes: list/watch FournosJob resources
Watcher->>PostgreSQL: upsert jobs and append job events
Watcher->>Kubernetes: retry watch after stream errors
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fournos-ui/README.md (1)
109-125: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDirectory name typo / mismatch, and missing code-fence language.
fournous_UI/doesn't match the actual project directory (fournos-ui/), which could confuse readers navigating the repo. Also flagged by markdownlint (MD040) for missing fence language.Fix
-``` +```text -fournous_UI/ +fournos-ui/ ├── app/🤖 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 `@fournos-ui/README.md` around lines 109 - 125, The repository tree in README has a directory-name typo and the code fence is missing a language annotation. Update the rendered tree in the README’s project structure section to use the correct top-level name referenced by the `fournos-ui` project, and change the fenced block to a language-tagged fence so it satisfies markdownlint MD040; use the existing README tree snippet as the location to fix.Source: Linters/SAST tools
🟡 Minor comments (6)
fournos-ui/README.md-58-64 (1)
58-64: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing content under "Build and push the dashboard image".
Steps 1, 3, and 4 include concrete commands, but step 2 has no
docker build/pushexample, leaving a gap in the setup instructions.🤖 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 `@fournos-ui/README.md` around lines 58 - 64, The “Build and push the dashboard image” step is missing the actual command examples, leaving the setup guide incomplete. Update the README section under that heading to include a concrete build and push flow, using the same style as the surrounding steps and referencing the dashboard image workflow so readers can follow it without guessing.fournos-ui/README.md-15-25 (1)
15-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to the fenced code block.
Static analysis (markdownlint MD040) flags this diagram block as missing a language tag.
Fix
-``` +```text ┌─────────────┐ ┌──────────────────┐ ┌────────────┐🤖 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 `@fournos-ui/README.md` around lines 15 - 25, The fenced diagram block in README is missing a language tag, which triggers markdownlint MD040. Update the existing markdown fence for the architecture diagram by adding an appropriate language identifier (for example, text) while keeping the diagram content unchanged, and make sure the fenced block in the README remains valid markdown.Source: Linters/SAST tools
fournos-ui/app/static/style.css-16-16 (1)
16-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the Stylelint
value-keyword-caseviolations.Stylelint flags the unquoted font-family names and
currentColor. Quoting font names preserves intent while satisfying the rule.Proposed fix
- --font-sans: -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif; + --font-sans: -apple-system, "BlinkMacSystemFont", 'Segoe UI', "Helvetica", "Arial", sans-serif; @@ - background: currentColor; + background: currentcolor;Also applies to: 256-256
🤖 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 `@fournos-ui/app/static/style.css` at line 16, Stylelint is flagging `value-keyword-case` for the font family values in `style.css`; update the `--font-sans` declaration and the other affected `font-family` usages in this file so the font names are quoted consistently. Also review any `currentColor` occurrences mentioned in the comment and adjust them to satisfy the same rule while keeping the intended styling intact.Source: Linters/SAST tools
fournos-ui/app/templates/submit_job.html-98-100 (1)
98-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEncode the project name before building the API URL.
Project names containing path/query characters will call the wrong route. Also check
resp.okso failed lookups don’t look like empty preset lists.Proposed fix
- const resp = await fetch('/api/project-info/' + projectName); + const resp = await fetch('/api/project-info/' + encodeURIComponent(projectName)); + if (!resp.ok) { + throw new Error('Failed to load project info'); + } const data = await resp.json();🤖 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 `@fournos-ui/app/templates/submit_job.html` around lines 98 - 100, The project-info fetch in submit_job.html builds the API URL with the raw projectName, which can break routing for names containing path or query characters and can treat failed requests as empty results. Update the fetch call in the submit-job flow to URL-encode projectName before concatenating it into the /api/project-info/ request, and add a resp.ok check in the same async block so non-200 responses are handled as failures rather than parsed as empty preset data.fournos-ui/app/templates/schedules.html-227-237 (1)
227-237: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSerialize
cronjobDatawithtojson. Jinja/HTML escaping is not valid JavaScript serialization here, so values likeO'Briencan be mangled or break the script. Usetojsonfor the object keys and all embedded fields, including the booleans.🤖 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 `@fournos-ui/app/templates/schedules.html` around lines 227 - 237, `cronjobData` is being built with Jinja string interpolation, which can produce invalid JavaScript and break on quoted values; update the template in the schedules view to serialize the full object with `tojson` instead of manually quoting fields. Use the existing `cronjobData` block and ensure every embedded property, including `has_resolver` and `suspend`, is emitted through proper JSON serialization so JavaScript receives a valid object literal.fournos-ui/app/templates/jobs_list.html-45-46 (1)
45-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter values aren't URL-encoded before insertion into query strings.
filters.owner/filters.clusterare free-text inputs (lines 38-41). If a value contains&,#,%, or spaces, the hx-get URL (line 45) and pagination links (lines 166, 170) will produce a malformed/incorrect query string. Use Jinja'surlencodefilter for these values.🐛 Suggested fix
-<div hx-get="/api/jobs-table?project={{ filters.project }}&cluster={{ filters.cluster }}&status={{ filters.status }}&owner={{ filters.owner }}" +<div hx-get="/api/jobs-table?project={{ filters.project | urlencode }}&cluster={{ filters.cluster | urlencode }}&status={{ filters.status | urlencode }}&owner={{ filters.owner | urlencode }}"Apply the same
| urlencodetreatment to the paginationhrefvalues at Line 166 and Line 170.Also applies to: 166-166, 170-170
🤖 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 `@fournos-ui/app/templates/jobs_list.html` around lines 45 - 46, The job list template is building query strings from free-text filter values without URL encoding, so special characters can break the requests. Update the hx-get URL in jobs_list.html to pass filters.project, filters.cluster, filters.status, and filters.owner through Jinja’s urlencode filter, and apply the same urlencode treatment to the pagination hrefs in the jobs table controls. Use the existing hx-get block and the pagination link markup as the locations to fix.
🧹 Nitpick comments (8)
fournos-ui/app/db.py (2)
140-164: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider indexing
created_atfor paginated history queries.
list_jobssorts byJob.created_at.desc()withlimit/offsetpagination, and per the referencedmain.pysnippet this backs the frequently-hit history tab. Without an index oncreated_at, this will require a full table scan/sort as thejobstable grows.⚡ Proposed index
- created_at = Column(DateTime(timezone=True), default=lambda: datetime.now(timezone.utc)) + created_at = Column(DateTime(timezone=True), default=lambda: datetime.now(timezone.utc), index=True)🤖 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 `@fournos-ui/app/db.py` around lines 140 - 164, The list_jobs query in the Job history path is ordering by Job.created_at.desc() with limit/offset pagination, so add an index on created_at to avoid full scans and sorts as the jobs table grows. Update the Job model/schema with an index on created_at (ideally alongside any existing filters if appropriate), and ensure the change is reflected in the database migration so list_jobs continues to return the same results while using the index.
85-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlembic dependency appears unused.
init_db()notes "use Alembic in production" andalembicis listed inrequirements.txt, but there's no visible migration setup (alembic.ini,migrations/dir) in this batch. Either wire up Alembic migrations for production use or drop the unused dependency to avoid confusion about the actual schema-management strategy.🤖 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 `@fournos-ui/app/db.py` around lines 85 - 90, The schema-management setup is inconsistent: init_db() in db.py still creates tables directly, while alembic is present but there is no visible migration wiring. Either add the missing Alembic migration setup and switch production schema changes to use it, or remove the Alembic dependency and update the init_db() docstring/logging to reflect direct table creation only. Use init_db(), Base.metadata.create_all, and the alembic dependency reference to locate the related code.fournos-ui/app/k8s_client.py (1)
63-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly
ApiExceptionis caught; other transport errors will propagate.Every read/list function in this module follows a "catch
ApiException, return a safe default" pattern, but transport-level failures (e.g.urllib3.exceptions.MaxRetryError, connection reset) aren'tApiExceptionand will bubble up uncaught, breaking the module's otherwise consistent defensive contract.Also applies to: 82-100, 196-215, 333-376
🤖 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 `@fournos-ui/app/k8s_client.py` around lines 63 - 79, The list/read helpers in k8s_client.py only handle ApiException, so transport failures can still escape and break the module’s safe-default behavior. Update list_fournos_jobs and the other affected read/list functions referenced in this module to also catch underlying transport/connection exceptions (for example urllib3/requests errors used by the Kubernetes client) alongside ApiException, log the failure, and return the same empty/default value. Keep the existing defensive pattern consistent across these functions such as list_fournos_jobs and the other read/list helpers.fournos-ui/kustomize/overlays/ocp/kustomization.yaml.example (1)
26-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIndex-based JSON6902 patches are fragile against base changes.
The
FOURNOS_NAMESPACEpatch targetscontainers/0/env/6/valueby numeric index; any reordering/addition of env vars in the basedashboard-deployment.yamlsilently breaks this overlay (wrong var patched) without any error. The image patches (containers/0/image,initContainers/0/image) have the same fragility, though container-index changes are less likely.Consider a strategic-merge patch instead, which matches array elements by name (the
envlist's merge key):- - target: - kind: Deployment - name: fournos-dashboard - patch: | - - op: replace - path: /spec/template/spec/containers/0/env/6/value - value: YOUR_FOURNOS_NAMESPACE +patches: + - path: fournos-namespace-patch.yaml# fournos-namespace-patch.yaml apiVersion: apps/v1 kind: Deployment metadata: name: fournos-dashboard spec: template: spec: containers: - name: dashboard env: - name: FOURNOS_NAMESPACE value: YOUR_FOURNOS_NAMESPACE🤖 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 `@fournos-ui/kustomize/overlays/ocp/kustomization.yaml.example` around lines 26 - 50, The JSON6902 patches in kustomization.yaml.example are index-based and can silently drift when the base Deployment changes. Update the overlay to use a strategic-merge patch (or name-keyed patching) for fournos-dashboard so the env var FOURNOS_NAMESPACE is matched by name instead of env/6, and apply the same approach for the dashboard and init container image overrides using container names rather than containers/0 and initContainers/0.fournos-ui/kustomize/base/postgresql-statefulset.yaml (1)
21-54: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHarden container
securityContext.Only pod-level
runAsNonRoot: trueis set; thepostgresqlcontainer lacksallowPrivilegeEscalation: false,readOnlyRootFilesystem: true, and dropped capabilities, as flagged by static analysis. Note that enablingreadOnlyRootFilesystemfor the official postgres image will additionally requireemptyDirmounts for paths like/tmpand/var/run/postgresql.🔒️ Proposed container securityContext hardening
containers: - name: postgresql image: postgres:15-alpine + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL ports: - containerPort: 5432 name: postgresql protocol: TCP + volumeMounts: + - name: postgresql-data + mountPath: /var/lib/postgresql/data + - name: tmp + mountPath: /tmp + - name: run + mountPath: /var/run/postgresql🤖 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 `@fournos-ui/kustomize/base/postgresql-statefulset.yaml` around lines 21 - 54, The postgresql container in the StatefulSet only has a pod-level non-root setting, so harden the container-level securityContext for the postgresql container by adding allowPrivilegeEscalation: false and dropping all capabilities, and enable readOnlyRootFilesystem only if you also add the required emptyDir mounts for writable paths such as /tmp and /var/run/postgresql. Update the container spec under the postgresql container block to include these securityContext fields.Source: Linters/SAST tools
fournos-ui/app/templates/components/jobs_table_body.html (2)
56-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winActions-menu SVG markup duplicated with
jobs_list.html.The kebab-trigger button and "Run Again" icon SVGs here are duplicated verbatim in
jobs_list.html(history tab, lines 131-142). Consider extracting a shared macro/include for the actions-menu (trigger + stop/rerun/delete buttons) to avoid divergence between the live-table and history-table rendering.🤖 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 `@fournos-ui/app/templates/components/jobs_table_body.html` around lines 56 - 76, The actions menu markup in jobs_table_body.html is duplicated in jobs_list.html, including the kebab trigger and the Run Again SVG, so extract the shared actions-menu UI into a reusable macro/include and have both templates use it. Use the existing stopJob, rerunJob, and toggleActions hooks as the stable integration points when consolidating the duplicated button markup.
32-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProgress-bar markup duplicated with
job_detail_dynamic.html— already drifting.This progress bar (segments + tooltip) is nearly identical to the one in
components/job_detail_dynamic.html(lines 41-53), except the tooltip there also includes "Skipped" while this one doesn't. Extracting a shared macro (e.g.{% macro progress_bar(progress) %}) would keep both in sync going forward.🤖 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 `@fournos-ui/app/templates/components/jobs_table_body.html` around lines 32 - 42, The progress-bar markup in jobs_table_body.html is duplicated from job_detail_dynamic.html and has already diverged, so extract the shared rendering into a reusable Jinja macro such as progress_bar(progress). Update both templates to call the same macro and make sure the tooltip and segment logic stay consistent, including the Skipped count shown in job_detail_dynamic.html.fournos-ui/app/templates/components/job_detail_dynamic.html (1)
11-13: 📐 Maintainability & Code Quality | 🔵 TrivialPrefer
namespace()over list-append side-effect trick.Using
{% if finally_started.append(1) %}purely for its (falsy) side effect works but is non-idiomatic and duplicated twice in this file. Jinja'snamespace()object is the standard way to mutate state across a loop.♻️ Suggested refactor
-{% set finally_started = [] %} +{% set ns = namespace(started=false) %} {% for s in tl %} -{% if s['finally'] and not finally_started %}{% if finally_started.append(1) %}{% endif %}<div class="ptl-sep"></div>{% endif %} +{% if s['finally'] and not ns.started %}{% set ns.started = true %}<div class="ptl-sep"></div>{% endif %}Apply the equivalent change to the
finally_started2block below.Also applies to: 22-24
🤖 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 `@fournos-ui/app/templates/components/job_detail_dynamic.html` around lines 11 - 13, The loop state tracking in the `finally_started` and `finally_started2` blocks uses a list-append side effect inside an `if`, which is non-idiomatic Jinja. Refactor `job_detail_dynamic.html` to use a `namespace()` object for the cross-loop flag in both blocks, and update the conditionals to set and check that namespace value instead of relying on `{% if ...append(1) %}`. Keep the existing separator rendering logic intact while applying the same change to both occurrences.
🤖 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 `@fournos-ui/app/forge_discovery.py`:
- Around line 92-103: The project parsing loop in discover_projects() should not
assume every item in data["projects"] is a mapping, because a malformed entry
can raise AttributeError and break all routes that depend on it. Update the loop
around the ProjectInfo construction to validate each proj before calling
proj.get(...), and skip or safely ignore non-dict entries so one bad ConfigMap
item does not abort discovery.
In `@fournos-ui/app/k8s_client.py`:
- Around line 26-50: The K8s client setup in _ensure_loaded() only initializes
shared API objects and does not limit how long sync calls can block inside async
FastAPI handlers. Update the actual Kubernetes API call sites used through
_custom_api, _core_api, and _batch_api to pass a request timeout (for example
via _request_timeout), or add a central wrapper around those client methods so
every request is bounded; do not rely on retries alone.
In `@fournos-ui/app/main.py`:
- Around line 40-44: Disable Jinja template caching in production by removing
the hardcoded cache_size=0 from _jinja_env in main.py and letting Environment
use a positive cache size or a config-driven value. Update the _jinja_env setup
so get_template calls reuse compiled templates, while preserving any desired
no-cache behavior only for local development if needed.
- Around line 292-350: The jobs_list handler applies pagination only in the
history branch, so the live tab still renders every filtered job. Update
jobs_list to slice the filtered live jobs by page and per_page using the
existing page parameter, and keep total as the full filtered count so the
pagination UI remains correct. Make sure the change is localized around
jobs_list and preserves the current filters, clusters, and current_steps
behavior for the live tab.
- Around line 577-604: The submission flow in the job-creation handler catches
both Kubernetes creation and DB persistence failures together, so a successful
create_fournos_job followed by a db.upsert_job error is wrongly treated as a
failed submission. Split the error handling around create_fournos_job and the
db.async_session/db.upsert_job block in the main submission path so that once
the K8s job is created, any later DB failure does not re-render submit_job.html
as an error; instead preserve the created_name and return the success redirect
(or an equivalent non-error response) while logging the DB sync failure
separately.
- Around line 658-678: The schedule edit flow in main.py deletes the existing
CronJob before confirming the replacement is created, so a failed create can
leave the schedule missing. Update the edit path around edit_target,
k8s_client.delete_cronjob, and k8s_client.create_cronjob so the new CronJob is
created first (or otherwise validated) and only then remove/replace the old one,
with rollback if creation fails.
- Around line 483-497: The stream_logs SSE handler is iterating a synchronous
follow loop directly on the async event loop, which can block the worker. Update
stream_logs and its inner generate() to offload k8s_client.read_pod_log(...,
follow=True) to a thread/executor, then asynchronously yield each line back to
StreamingResponse without blocking; keep the pod validation and SSE formatting
in place.
- Around line 224-268: The async job routes are still doing blocking Kubernetes
calls, and _compute_current_steps also triggers per-job step lookups that can
stall the event loop under polling load. Move the synchronous k8s_client access
used by jobs_list, jobs_table_partial, job_detail, and job_detail_partial off
the async path by wrapping the relevant calls in a threadpool/async offload, and
keep _compute_current_steps from fanning out synchronously. Also add request
timeouts in k8s_client so list_fournos_jobs, get_current_step_for_job, and
related client methods fail fast instead of blocking.
In `@fournos-ui/app/static/htmx-sse.js`:
- Around line 253-258: The retry backoff in ensureEventSourceOnElement is using
the XOR operator instead of exponentiation, which can produce incorrect delays
and reconnect loops. Update the timeout calculation to use proper
power/exponential backoff with retryCount in the CLOSED readyState branch,
keeping the existing Math.random() jitter and retryCount increment logic intact.
- Around line 190-199: The `hx-trigger="sse:*"` branch in `htmx-sse.js` creates
`listener` but never registers it, so the trigger never fires. In the code path
that defines `listener` for the SSE event handling, make sure
`source.addEventListener(...)` is called for the computed SSE event name and
that the htmx request is triggered as intended when the event arrives. Use the
surrounding SSE setup logic in this listener block to wire the handler
consistently with the other trigger paths.
- Around line 239-267: The ensureEventSource() logic always creates a new
EventSource and overwrites api.getInternalData(elt).sseEventSource, which can
leave duplicate live SSE connections on reprocessing. Update ensureEventSource()
to first check for an existing sseEventSource and reuse it when its readyState
is not CLOSED, or explicitly close the old source before assigning a new one;
keep the change localized to ensureEventSource() and its use of
api.getInternalData(elt).sseEventSource.
In `@fournos-ui/app/templates/job_detail.html`:
- Around line 10-26: Keep the live job header synced with polling: the phase
badge, duration badge, Cancel Job button, and artifact links in the job detail
view are rendered outside the only fragment that refreshes periodically. Update
the `job_detail.html` template so the dynamic header controls are either moved
into `#job-dynamic` or sent as out-of-band swaps with `hx-swap-oob`, and make
sure the same `phase`, `is_history`, and `job.metadata.name` conditions are
applied in the polled update path.
In `@fournos-ui/app/watcher.py`:
- Around line 103-124: `_archive_job` is still creating duplicate `JobEvent`
rows on every sync even when the job’s `status` and `message` haven’t changed.
Update the logic around `_archive_job` and the `db.add_job_event` call to
compare the incoming forge fields against the existing job state before
inserting, or add deduping inside `add_job_event`, so unchanged watch/full-sync
passes only upsert the job and skip event creation.
In `@fournos-ui/Dockerfile`:
- Around line 19-23: The current uvicorn ENTRYPOINT is running multiple web
workers while watcher.start_watcher() is launched from the FastAPI lifespan,
which duplicates watcher threads and downstream sync/write work. Fix this by
ensuring the watcher runs in a separate single-replica process or by changing
the app to a single worker, and keep the watcher startup isolated from the
multi-worker web process in the app lifespan/ENTRYPOINT setup.
In `@fournos-ui/kustomize/base/dashboard-clusterrole.yaml`:
- Around line 1-28: The ClusterRole for the dashboard is granting cluster-wide
write access for ConfigMaps, CronJobs, and Jobs even though the dashboard only
operates in FOURNOS_NAMESPACE and its own namespace. Refactor the RBAC in the
dashboard ClusterRole manifest by moving the namespace-specific rules into Role
objects and binding them with RoleBinding, and only keep permissions in
ClusterRole/ClusterRoleBinding if a true cross-namespace need exists. Use the
existing fournos-dashboard-reader role definition and the rules for configmaps,
cronjobs, and jobs as the symbols to split and scope correctly.
In `@fournos-ui/kustomize/base/dashboard-deployment.yaml`:
- Around line 19-103: The dashboard Deployment is missing restrictive container
security settings on both the dashboard container and the wait-for-postgresql
initContainer. Add a securityContext to each container with
readOnlyRootFilesystem enabled and allowPrivilegeEscalation disabled, and mirror
the same settings on the initContainer as well. If the dashboard process needs
writable storage, use an emptyDir mount for that specific path while keeping the
root filesystem read-only.
In `@fournos-ui/requirements.txt`:
- Around line 1-11: The dashboard dependencies in requirements.txt are all
unpinned, which makes builds non-reproducible and can introduce unexpected
upgrades. Update the dependency list used by the Dockerfile install path to
fixed versions, or switch the install flow to a lockfile/constraints file so
rebuilds resolve the same package set. Make sure every package listed in this
file is versioned consistently and kept in sync with the Docker image build.
---
Outside diff comments:
In `@fournos-ui/README.md`:
- Around line 109-125: The repository tree in README has a directory-name typo
and the code fence is missing a language annotation. Update the rendered tree in
the README’s project structure section to use the correct top-level name
referenced by the `fournos-ui` project, and change the fenced block to a
language-tagged fence so it satisfies markdownlint MD040; use the existing
README tree snippet as the location to fix.
---
Minor comments:
In `@fournos-ui/app/static/style.css`:
- Line 16: Stylelint is flagging `value-keyword-case` for the font family values
in `style.css`; update the `--font-sans` declaration and the other affected
`font-family` usages in this file so the font names are quoted consistently.
Also review any `currentColor` occurrences mentioned in the comment and adjust
them to satisfy the same rule while keeping the intended styling intact.
In `@fournos-ui/app/templates/jobs_list.html`:
- Around line 45-46: The job list template is building query strings from
free-text filter values without URL encoding, so special characters can break
the requests. Update the hx-get URL in jobs_list.html to pass filters.project,
filters.cluster, filters.status, and filters.owner through Jinja’s urlencode
filter, and apply the same urlencode treatment to the pagination hrefs in the
jobs table controls. Use the existing hx-get block and the pagination link
markup as the locations to fix.
In `@fournos-ui/app/templates/schedules.html`:
- Around line 227-237: `cronjobData` is being built with Jinja string
interpolation, which can produce invalid JavaScript and break on quoted values;
update the template in the schedules view to serialize the full object with
`tojson` instead of manually quoting fields. Use the existing `cronjobData`
block and ensure every embedded property, including `has_resolver` and
`suspend`, is emitted through proper JSON serialization so JavaScript receives a
valid object literal.
In `@fournos-ui/app/templates/submit_job.html`:
- Around line 98-100: The project-info fetch in submit_job.html builds the API
URL with the raw projectName, which can break routing for names containing path
or query characters and can treat failed requests as empty results. Update the
fetch call in the submit-job flow to URL-encode projectName before concatenating
it into the /api/project-info/ request, and add a resp.ok check in the same
async block so non-200 responses are handled as failures rather than parsed as
empty preset data.
In `@fournos-ui/README.md`:
- Around line 58-64: The “Build and push the dashboard image” step is missing
the actual command examples, leaving the setup guide incomplete. Update the
README section under that heading to include a concrete build and push flow,
using the same style as the surrounding steps and referencing the dashboard
image workflow so readers can follow it without guessing.
- Around line 15-25: The fenced diagram block in README is missing a language
tag, which triggers markdownlint MD040. Update the existing markdown fence for
the architecture diagram by adding an appropriate language identifier (for
example, text) while keeping the diagram content unchanged, and make sure the
fenced block in the README remains valid markdown.
---
Nitpick comments:
In `@fournos-ui/app/db.py`:
- Around line 140-164: The list_jobs query in the Job history path is ordering
by Job.created_at.desc() with limit/offset pagination, so add an index on
created_at to avoid full scans and sorts as the jobs table grows. Update the Job
model/schema with an index on created_at (ideally alongside any existing filters
if appropriate), and ensure the change is reflected in the database migration so
list_jobs continues to return the same results while using the index.
- Around line 85-90: The schema-management setup is inconsistent: init_db() in
db.py still creates tables directly, while alembic is present but there is no
visible migration wiring. Either add the missing Alembic migration setup and
switch production schema changes to use it, or remove the Alembic dependency and
update the init_db() docstring/logging to reflect direct table creation only.
Use init_db(), Base.metadata.create_all, and the alembic dependency reference to
locate the related code.
In `@fournos-ui/app/k8s_client.py`:
- Around line 63-79: The list/read helpers in k8s_client.py only handle
ApiException, so transport failures can still escape and break the module’s
safe-default behavior. Update list_fournos_jobs and the other affected read/list
functions referenced in this module to also catch underlying
transport/connection exceptions (for example urllib3/requests errors used by the
Kubernetes client) alongside ApiException, log the failure, and return the same
empty/default value. Keep the existing defensive pattern consistent across these
functions such as list_fournos_jobs and the other read/list helpers.
In `@fournos-ui/app/templates/components/job_detail_dynamic.html`:
- Around line 11-13: The loop state tracking in the `finally_started` and
`finally_started2` blocks uses a list-append side effect inside an `if`, which
is non-idiomatic Jinja. Refactor `job_detail_dynamic.html` to use a
`namespace()` object for the cross-loop flag in both blocks, and update the
conditionals to set and check that namespace value instead of relying on `{% if
...append(1) %}`. Keep the existing separator rendering logic intact while
applying the same change to both occurrences.
In `@fournos-ui/app/templates/components/jobs_table_body.html`:
- Around line 56-76: The actions menu markup in jobs_table_body.html is
duplicated in jobs_list.html, including the kebab trigger and the Run Again SVG,
so extract the shared actions-menu UI into a reusable macro/include and have
both templates use it. Use the existing stopJob, rerunJob, and toggleActions
hooks as the stable integration points when consolidating the duplicated button
markup.
- Around line 32-42: The progress-bar markup in jobs_table_body.html is
duplicated from job_detail_dynamic.html and has already diverged, so extract the
shared rendering into a reusable Jinja macro such as progress_bar(progress).
Update both templates to call the same macro and make sure the tooltip and
segment logic stay consistent, including the Skipped count shown in
job_detail_dynamic.html.
In `@fournos-ui/kustomize/base/postgresql-statefulset.yaml`:
- Around line 21-54: The postgresql container in the StatefulSet only has a
pod-level non-root setting, so harden the container-level securityContext for
the postgresql container by adding allowPrivilegeEscalation: false and dropping
all capabilities, and enable readOnlyRootFilesystem only if you also add the
required emptyDir mounts for writable paths such as /tmp and
/var/run/postgresql. Update the container spec under the postgresql container
block to include these securityContext fields.
In `@fournos-ui/kustomize/overlays/ocp/kustomization.yaml.example`:
- Around line 26-50: The JSON6902 patches in kustomization.yaml.example are
index-based and can silently drift when the base Deployment changes. Update the
overlay to use a strategic-merge patch (or name-keyed patching) for
fournos-dashboard so the env var FOURNOS_NAMESPACE is matched by name instead of
env/6, and apply the same approach for the dashboard and init container image
overrides using container names rather than containers/0 and initContainers/0.
🪄 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: a912f49c-385d-45bf-910a-bd65bf6acf2a
⛔ Files ignored due to path filters (1)
fournos-ui/app/static/htmx.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (37)
fournos-ui/.dockerignorefournos-ui/.gitignorefournos-ui/Dockerfilefournos-ui/README.mdfournos-ui/app/__init__.pyfournos-ui/app/config.pyfournos-ui/app/db.pyfournos-ui/app/forge_discovery.pyfournos-ui/app/k8s_client.pyfournos-ui/app/main.pyfournos-ui/app/models.pyfournos-ui/app/static/htmx-sse.jsfournos-ui/app/static/style.cssfournos-ui/app/templates/base.htmlfournos-ui/app/templates/components/job_detail_dynamic.htmlfournos-ui/app/templates/components/jobs_table_body.htmlfournos-ui/app/templates/job_detail.htmlfournos-ui/app/templates/jobs_list.htmlfournos-ui/app/templates/schedule_runs.htmlfournos-ui/app/templates/schedules.htmlfournos-ui/app/templates/submit_job.htmlfournos-ui/app/watcher.pyfournos-ui/kustomize/base/dashboard-clusterrole.yamlfournos-ui/kustomize/base/dashboard-clusterrolebinding.yamlfournos-ui/kustomize/base/dashboard-deployment.yamlfournos-ui/kustomize/base/dashboard-projects-configmap.yamlfournos-ui/kustomize/base/dashboard-service.yamlfournos-ui/kustomize/base/dashboard-serviceaccount.yamlfournos-ui/kustomize/base/kustomization.yamlfournos-ui/kustomize/base/namespace.yamlfournos-ui/kustomize/base/postgresql-secret.env.examplefournos-ui/kustomize/base/postgresql-service.yamlfournos-ui/kustomize/base/postgresql-statefulset.yamlfournos-ui/kustomize/overlays/ocp/kustomization.yaml.examplefournos-ui/kustomize/overlays/ocp/params.env.examplefournos-ui/kustomize/overlays/ocp/projects.yaml.examplefournos-ui/requirements.txt
- Switch ClusterRoleBinding to namespace-scoped RoleBinding for least-privilege access - Add securityContext to init and dashboard containers (drop capabilities, read-only root) - Wrap blocking K8s API calls with asyncio.to_thread to avoid event-loop starvation - Add configurable K8s request timeout (K8S_REQUEST_TIMEOUT env var) - Pin dependency versions in requirements.txt - Use kustomize configMapGenerator for projects ConfigMap instead of static manifest - Add OOB HTMX swaps for live header status, completion banner, and MLflow links - Fix htmx-sse.js exponential backoff (Math.pow instead of bitwise XOR) - Deduplicate watcher archive events when phase/message unchanged - Add resilience to malformed project entries in forge_discovery - Reduce Dockerfile workers to 1 and disable Jinja2 cache_size=0 in favour of auto_reload=False - Update README with corrected deploy instructions and project structure Co-authored-by: Cursor <cursoragent@cursor.com>
- Add optional PULL_PULL_SHA field to submit form, injected into spec.env so the resolve job checks out Forge code from a specific PR - Show the MCP Gateway version field only when mcp_gateway project is selected; hide and clear it for all other projects Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fournos-ui/kustomize/base/kustomization.yaml (1)
6-13: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winThe RoleBinding is never deployed — RBAC will not take effect. The resources list includes
dashboard-clusterrole.yamlbut omitsdashboard-clusterrolebinding.yaml. Without the binding, thefournos-dashboardServiceAccount receives none of the ClusterRole permissions, and all Kubernetes API calls (fournosjobs, configmaps, cronjobs, jobs, pods, logs) will fail withForbiddenat runtime.🔧 Add the binding to the resources list
- dashboard-service.yaml - dashboard-clusterrole.yaml + - dashboard-clusterrolebinding.yaml🤖 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 `@fournos-ui/kustomize/base/kustomization.yaml` around lines 6 - 13, The RBAC resources are incomplete because the Kustomize base includes dashboard-clusterrole.yaml but not dashboard-clusterrolebinding.yaml, so the forunos-dashboard ServiceAccount never gets the ClusterRole permissions. Update the resources list in kustomization.yaml to include the missing RoleBinding manifest alongside dashboard-clusterrole.yaml, keeping the dashboard-serviceaccount and dashboard-clusterrole names consistent so the binding is actually applied.fournos-ui/kustomize/base/dashboard-clusterrolebinding.yaml (1)
2-15: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a RoleBinding in the target jobs namespace when
FOURNOS_NAMESPACEdiffers. This binding only applies in the dashboard namespace; the client sends namespaced calls tosettings.fournos_namespace, so a separate RoleBinding (or a ClusterRoleBinding) is needed there to avoidForbiddenerrors.🤖 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 `@fournos-ui/kustomize/base/dashboard-clusterrolebinding.yaml` around lines 2 - 15, The current RoleBinding only exists in the dashboard namespace, so namespaced calls from the client can still fail with Forbidden when FOURNOS_NAMESPACE points elsewhere. Update the dashboard-clusterrolebinding manifest and related overlay logic so a RoleBinding is also created in the target jobs namespace when FOURNOS_NAMESPACE differs, or switch to a ClusterRoleBinding if that is the intended access model; reference the existing fournos-dashboard binding and its roleRef/subjects to keep the new binding aligned.
🤖 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 `@fournos-ui/app/main.py`:
- Around line 330-332: The cluster filter is being derived from the paginated
`jobs` slice in `main.py`, so `_collect_clusters(jobs)` only sees the current
page instead of all filtered results. Update the job listing flow so clusters
are collected from the full filtered job set before applying
`offset`/`per_page`, using the same `jobs` data path around `total`, slicing,
and `_collect_clusters`. Then remove the later unconditional cluster collection
from the paginated slice so the live tab dropdown includes clusters from every
matching job.
- Around line 508-526: The SSE generator in generate() should stop the
background log reader when the client disconnects and avoid dropping lines via
put_nowait. Add a disconnect-aware stop signal to the _reader loop, use a
blocking handoff into the asyncio.Queue instead of
call_soon_threadsafe(queue.put_nowait, ...), and ensure the executor task is
torn down when StreamingResponse finishes. If kubernetes log streaming exposes a
close/cleanup path in k8s_client.read_pod_log, invoke it during teardown.
In `@fournos-ui/kustomize/base/dashboard-clusterrolebinding.yaml`:
- Line 2: The manifest name and resource kind are mismatched:
dashboard-clusterrolebinding.yaml defines a RoleBinding, so rename it to
dashboard-rolebinding.yaml and update the corresponding kustomization reference
to match. Make the change in the RoleBinding manifest and any Kustomize entry
that points to dashboard-clusterrolebinding so the filename accurately reflects
the resource.
---
Outside diff comments:
In `@fournos-ui/kustomize/base/dashboard-clusterrolebinding.yaml`:
- Around line 2-15: The current RoleBinding only exists in the dashboard
namespace, so namespaced calls from the client can still fail with Forbidden
when FOURNOS_NAMESPACE points elsewhere. Update the dashboard-clusterrolebinding
manifest and related overlay logic so a RoleBinding is also created in the
target jobs namespace when FOURNOS_NAMESPACE differs, or switch to a
ClusterRoleBinding if that is the intended access model; reference the existing
fournos-dashboard binding and its roleRef/subjects to keep the new binding
aligned.
In `@fournos-ui/kustomize/base/kustomization.yaml`:
- Around line 6-13: The RBAC resources are incomplete because the Kustomize base
includes dashboard-clusterrole.yaml but not dashboard-clusterrolebinding.yaml,
so the forunos-dashboard ServiceAccount never gets the ClusterRole permissions.
Update the resources list in kustomization.yaml to include the missing
RoleBinding manifest alongside dashboard-clusterrole.yaml, keeping the
dashboard-serviceaccount and dashboard-clusterrole names consistent so the
binding is actually applied.
🪄 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: 229d8215-c7c9-4626-9f0b-1ecde3974f3a
📒 Files selected for processing (19)
fournos-ui/.gitignorefournos-ui/Dockerfilefournos-ui/README.mdfournos-ui/app/config.pyfournos-ui/app/forge_discovery.pyfournos-ui/app/k8s_client.pyfournos-ui/app/main.pyfournos-ui/app/static/htmx-sse.jsfournos-ui/app/templates/components/job_detail_dynamic.htmlfournos-ui/app/templates/job_detail.htmlfournos-ui/app/templates/submit_job.htmlfournos-ui/app/watcher.pyfournos-ui/kustomize/base/dashboard-clusterrole.yamlfournos-ui/kustomize/base/dashboard-clusterrolebinding.yamlfournos-ui/kustomize/base/dashboard-deployment.yamlfournos-ui/kustomize/base/dashboard-projects-configmap-data.yamlfournos-ui/kustomize/base/kustomization.yamlfournos-ui/kustomize/overlays/ocp/kustomization.yaml.examplefournos-ui/requirements.txt
✅ Files skipped from review due to trivial changes (3)
- fournos-ui/kustomize/base/dashboard-projects-configmap-data.yaml
- fournos-ui/README.md
- fournos-ui/.gitignore
🚧 Files skipped from review as they are similar to previous changes (11)
- fournos-ui/Dockerfile
- fournos-ui/kustomize/overlays/ocp/kustomization.yaml.example
- fournos-ui/app/templates/components/job_detail_dynamic.html
- fournos-ui/kustomize/base/dashboard-deployment.yaml
- fournos-ui/app/config.py
- fournos-ui/app/templates/submit_job.html
- fournos-ui/app/forge_discovery.py
- fournos-ui/app/watcher.py
- fournos-ui/app/templates/job_detail.html
- fournos-ui/app/static/htmx-sse.js
- fournos-ui/app/k8s_client.py
Comment on lines
+508
to
+526
| async def generate(): | ||
| queue: asyncio.Queue[str | None] = asyncio.Queue(maxsize=64) | ||
| loop = asyncio.get_event_loop() | ||
|
|
||
| def _reader(): | ||
| try: | ||
| for line in k8s_client.read_pod_log(pod_name, follow=True): | ||
| loop.call_soon_threadsafe(queue.put_nowait, line) | ||
| finally: | ||
| loop.call_soon_threadsafe(queue.put_nowait, None) | ||
|
|
||
| asyncio.get_event_loop().run_in_executor(None, _reader) | ||
| while True: | ||
| line = await queue.get() | ||
| if line is None: | ||
| break | ||
| yield f"data: {line}\n\n" | ||
|
|
||
| return StreamingResponse(generate(), media_type="text/event-stream") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant files.
git ls-files | rg '(^|/)app/main\.py$|(^|/)k8s_client|(^|/)k8s.*client|(^|/)client.*k8s' || true
# Show nearby context in fournos-ui/app/main.py around the cited lines.
if [ -f fournos-ui/app/main.py ]; then
nl -ba fournos-ui/app/main.py | sed -n '470,560p'
fi
# Search for read_pod_log definitions/usages.
rg -n "read_pod_log|read_namespaced_pod_log|StreamingResponse|async def generate|run_in_executor|call_soon_threadsafe|QueueFull" fournos-ui -SRepository: openshift-psap/fournos
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- fournos-ui/app/main.py (around cited lines) ---'
sed -n '490,545p' fournos-ui/app/main.py
echo
echo '--- fournos-ui/app/k8s_client.py ---'
sed -n '1,260p' fournos-ui/app/k8s_client.py
echo
echo '--- search for any disconnect/cancellation handling around pod log streaming ---'
rg -n "disconnect|cancel|CancelledError|QueueFull|put_nowait|run_coroutine_threadsafe|read_pod_log|read_namespaced_pod_log|stream\(" fournos-ui/app -SRepository: openshift-psap/fournos
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- fournos-ui/app/k8s_client.py (read_pod_log region) ---'
sed -n '360,440p' fournos-ui/app/k8s_client.py
echo
echo '--- asyncio queue/threadsafe backpressure probe ---'
python3 - <<'PY'
import asyncio
import threading
async def main():
q = asyncio.Queue(maxsize=1)
loop = asyncio.get_running_loop()
q.put_nowait("first")
done = threading.Event()
errs = []
def cb():
try:
q.put_nowait("second")
except Exception as e:
errs.append(repr(e))
finally:
done.set()
loop.call_soon_threadsafe(cb)
for _ in range(50):
if done.is_set():
break
await asyncio.sleep(0.01)
print("queue size:", q.qsize())
print("errors:", errs)
print("get:", await q.get())
asyncio.run(main())
PYRepository: openshift-psap/fournos
Length of output: 3142
Stop the SSE reader on disconnect and avoid put_nowait here. fournos-ui/app/main.py:508-526
- The background reader keeps running after
generate()closes, so abandoned streams keep an executor worker busy until the pod log stream ends. queue.put_nowaitscheduled viacall_soon_threadsafecan raiseQueueFull, which drops log lines instead of applying backpressure.
Use a disconnect-aware stop signal and a blocking handoff into the queue; if possible, close the underlying Kubernetes log response during teardown too.
🤖 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 `@fournos-ui/app/main.py` around lines 508 - 526, The SSE generator in
generate() should stop the background log reader when the client disconnects and
avoid dropping lines via put_nowait. Add a disconnect-aware stop signal to the
_reader loop, use a blocking handoff into the asyncio.Queue instead of
call_soon_threadsafe(queue.put_nowait, ...), and ensure the executor task is
torn down when StreamingResponse finishes. If kubernetes log streaming exposes a
close/cleanup path in k8s_client.read_pod_log, invoke it during teardown.
- Stop the background log reader thread on client disconnect via asyncio.Event; catch QueueFull to avoid dropping the executor thread - Collect cluster filter options from the full filtered job set before paginating so the dropdown includes all clusters, not just the current page - Rename dashboard-clusterrolebinding.yaml to dashboard-rolebinding.yaml to match the actual RoleBinding kind inside Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the manual SHA input with a searchable dropdown that lists open PRs from the Forge repo (public API, no token needed). Users can filter by PR number, title or author and the HEAD SHA is filled automatically. Also sort pods by creation timestamp so they appear in execution order. Co-authored-by: Cursor <cursoragent@cursor.com>
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 `@fournos-ui/app/main.py`:
- Around line 573-577: The GitHub fetch in github_open_prs is still using
synchronous urllib.request.urlopen inside an async path, which can block the
event loop. Update the request/response handling in github_open_prs to run the
blocking network call via asyncio.to_thread or switch to an async HTTP client,
and keep the existing HTTPException error handling around the fetch and JSON
parsing.
In `@fournos-ui/app/templates/submit_job.html`:
- Around line 154-169: The selectPR() helper is inserting API-sourced data into
pr-hint via innerHTML, which can create an XSS risk. Update the pr branch in
selectPR() to build the hint using textContent and/or DOM nodes instead of
string concatenation, and keep pr.head_sha as plain text when rendering the HEAD
SHA label. Use the existing selectPR, pr.head_sha, and pr-hint elements to
locate the change.
🪄 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: f24370cf-3c28-4432-a903-f4a6b403bed3
📒 Files selected for processing (6)
fournos-ui/README.mdfournos-ui/app/config.pyfournos-ui/app/k8s_client.pyfournos-ui/app/main.pyfournos-ui/app/templates/submit_job.htmlfournos-ui/kustomize/base/dashboard-rolebinding.yaml
💤 Files with no reviewable changes (1)
- fournos-ui/kustomize/base/dashboard-rolebinding.yaml
✅ Files skipped from review due to trivial changes (1)
- fournos-ui/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- fournos-ui/app/config.py
- fournos-ui/app/k8s_client.py
Comment on lines
+154
to
+169
| function selectPR(pr) { | ||
| const search = document.getElementById('pr_search'); | ||
| const shaInput = document.getElementById('pull_sha'); | ||
| const hint = document.getElementById('pr-hint'); | ||
| const dd = document.getElementById('pr_dropdown'); | ||
| dd.style.display = 'none'; | ||
| if (pr) { | ||
| search.value = _prLabel(pr); | ||
| shaInput.value = pr.head_sha; | ||
| hint.innerHTML = 'HEAD SHA: <code style="font-family:var(--font-mono);font-size:12px;background:var(--bg-tertiary,#21262d);padding:1px 6px;border-radius:3px">' + pr.head_sha + '</code>'; | ||
| } else { | ||
| search.value = ''; | ||
| shaInput.value = ''; | ||
| hint.textContent = 'Forge will build from this commit instead of the default image.'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid innerHTML with API-sourced data.
hint.innerHTML is built by concatenating pr.head_sha from the GitHub API response. Static analysis flags this as a potential XSS vector (CWE-79). Even though GitHub SHAs are normally hex-only, this is untrusted external data reflected without escaping — safer to build the hint with textContent/DOM nodes instead of innerHTML.
🛡️ Proposed fix using DOM nodes instead of innerHTML
if (pr) {
search.value = _prLabel(pr);
shaInput.value = pr.head_sha;
- hint.innerHTML = 'HEAD SHA: <code style="font-family:var(--font-mono);font-size:12px;background:var(--bg-tertiary,`#21262d`);padding:1px 6px;border-radius:3px">' + pr.head_sha + '</code>';
+ hint.textContent = '';
+ hint.append('HEAD SHA: ');
+ const code = document.createElement('code');
+ code.style.cssText = 'font-family:var(--font-mono);font-size:12px;background:var(--bg-tertiary,`#21262d`);padding:1px 6px;border-radius:3px';
+ code.textContent = pr.head_sha;
+ hint.append(code);
} else {📝 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
| function selectPR(pr) { | |
| const search = document.getElementById('pr_search'); | |
| const shaInput = document.getElementById('pull_sha'); | |
| const hint = document.getElementById('pr-hint'); | |
| const dd = document.getElementById('pr_dropdown'); | |
| dd.style.display = 'none'; | |
| if (pr) { | |
| search.value = _prLabel(pr); | |
| shaInput.value = pr.head_sha; | |
| hint.innerHTML = 'HEAD SHA: <code style="font-family:var(--font-mono);font-size:12px;background:var(--bg-tertiary,#21262d);padding:1px 6px;border-radius:3px">' + pr.head_sha + '</code>'; | |
| } else { | |
| search.value = ''; | |
| shaInput.value = ''; | |
| hint.textContent = 'Forge will build from this commit instead of the default image.'; | |
| } | |
| } | |
| function selectPR(pr) { | |
| const search = document.getElementById('pr_search'); | |
| const shaInput = document.getElementById('pull_sha'); | |
| const hint = document.getElementById('pr-hint'); | |
| const dd = document.getElementById('pr_dropdown'); | |
| dd.style.display = 'none'; | |
| if (pr) { | |
| search.value = _prLabel(pr); | |
| shaInput.value = pr.head_sha; | |
| hint.textContent = ''; | |
| hint.append('HEAD SHA: '); | |
| const code = document.createElement('code'); | |
| code.style.cssText = 'font-family:var(--font-mono);font-size:12px;background:var(--bg-tertiary,`#21262d`);padding:1px 6px;border-radius:3px'; | |
| code.textContent = pr.head_sha; | |
| hint.append(code); | |
| } else { | |
| search.value = ''; | |
| shaInput.value = ''; | |
| hint.textContent = 'Forge will build from this commit instead of the default image.'; | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 162-162: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: hint.innerHTML = 'HEAD SHA: ' + pr.head_sha + ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(inner-outer-html)
🤖 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 `@fournos-ui/app/templates/submit_job.html` around lines 154 - 169, The
selectPR() helper is inserting API-sourced data into pr-hint via innerHTML,
which can create an XSS risk. Update the pr branch in selectPR() to build the
hint using textContent and/or DOM nodes instead of string concatenation, and
keep pr.head_sha as plain text when rendering the HEAD SHA label. Use the
existing selectPR, pr.head_sha, and pr-hint elements to locate the change.
Source: Linters/SAST tools
Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
|
@ashtarkb should we merge that? |
Contributor
Author
|
@kpouget I think we can merge it and I will improve it with time and with new features. |
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
2 participants
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.
Web dashboard for managing Fournos performance testing jobs. Provides job submission, live monitoring, scheduling, and historical results -- built with FastAPI, HTMX, and PostgreSQL.
Summary by CodeRabbit
New Features
Bug Fixes