Skip to content

update the distinct active state for the Job and Job Sets tab#4887

Open
YHines004 wants to merge 7 commits intomasterfrom
fix/Job-JobSets-visual-active-tab
Open

update the distinct active state for the Job and Job Sets tab#4887
YHines004 wants to merge 7 commits intomasterfrom
fix/Job-JobSets-visual-active-tab

Conversation

@YHines004
Copy link
Copy Markdown
Collaborator

What type of PR is this?

Enhancement

What this PR does / why we need it

This PR give the button/tab for Job and Job Sets a distinct active state when one of them are selected. We need this to give a distinctive state of which tab is active.

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

@YHines004 YHines004 force-pushed the fix/Job-JobSets-visual-active-tab branch from 5fb438c to 5877261 Compare April 28, 2026 20:44
@YHines004 YHines004 changed the title chore: update the active button for the Job and Job Sets tab update the active distinct state for the Job and Job Sets tab Apr 28, 2026
@YHines004 YHines004 changed the title update the active distinct state for the Job and Job Sets tab update the distinct active state for the Job and Job Sets tab Apr 28, 2026
@YHines004 YHines004 marked this pull request as ready for review May 4, 2026 15:10
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 4, 2026

Greptile Summary

This PR adds a visible active state to the Jobs and Job Sets nav buttons using a theme-aware CSS custom property (--nav-active-bg) computed from alpha(contrastText, 0.33), which is a solid improvement over a hardcoded colour.

  • P1: JOBS = "/" and NavLinkButton lacks the end prop. React Router v6 NavLink uses prefix matching by default, so to="/" matches every URL — the Jobs button will appear highlighted on all pages (Job Sets, Settings, etc.), making the active indicator misleading everywhere except the root.

Confidence Score: 4/5

One P1 bug makes the active indicator always visible on the Jobs button — needs the end prop fix before merging.

A single P1 logic bug: the missing end prop on NavLinkButton causes JOBS ("/") to match every route, rendering the new active highlight incorrect on all non-root pages. The CSS and theme-variable approach are otherwise clean.

internal/lookoutui/src/app/NavBar.tsx — the NavLinkButton definition at line 13–15 needs end added.

Important Files Changed

Filename Overview
internal/lookoutui/src/app/NavBar.tsx Adds --nav-active-bg CSS custom property via alpha() from the theme — good theme-aware approach — but NavLinkButton is missing the end prop, causing JOBS = "/" to match every route and always appear active.
internal/lookoutui/src/app/NavBar.css Adds transition and active-state rules using the --nav-active-bg CSS custom property; clean implementation, correctly scoped to .toolbar a.MuiButton-root.active.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["NavBar renders"] --> B["StyledAppBar sets --nav-active-bg\nvia alpha(contrastText, 0.33)"]
    B --> C["NavLinkButton wraps NavLink"]
    C --> D{"NavLink prefix match\nagainst current URL"}
    D -->|"JOBS = '/' matches ALL routes\n(missing end prop)"| E["❌ .active class always added\nto Jobs button"]
    D -->|"JOB_SETS = '/job-sets'\nonly matches /job-sets"| F["✅ .active class added correctly"]
    E --> G[".toolbar a.MuiButton-root.active\napplies --nav-active-bg + bold"]
    F --> G
    G --> H["Jobs button always highlighted\nregardless of current page"]
Loading

Comments Outside Diff (1)

  1. internal/lookoutui/src/app/NavBar.tsx, line 13-15 (link)

    P1 Without end, React Router v6's NavLink uses prefix matching, so to="/" (JOBS) is active on every URL. Now that .active has visible CSS, the Jobs button will always appear highlighted regardless of which page the user is on. Adding end enforces exact-path matching for all nav links.

Reviews (2): Last reviewed commit: "Merge branch 'master' into fix/Job-JobSe..." | Re-trigger Greptile

Comment thread internal/lookoutui/src/app/NavBar.css
Comment thread internal/lookoutui/src/app/NavBar.css
YHines004 and others added 6 commits May 4, 2026 12:41
Signed-off-by: Yasmine Hines <yhines004@gmail.com>
…off-by: Yasmine Hines yasmine.hines@nmc2.ai

Signed-off-by: Yasmine Hines <yhines004@gmail.com>
Drops the failure_info jsonb column from job_run. Nothing writes or
reads it after #4843 and #4853, and the column was never populated in
production outside the opt-in flag path anyway.

Also drops the unused FailureInfo field from the queryapi sqlc model.

Only merge after #4843 and #4853 have been deployed long enough that we
are sure no consumer still depends on the column.

Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
Signed-off-by: Yasmine Hines <yhines004@gmail.com>
<!-- Thanks for sending a pull request! Here are some tips for you: -->

#### What type of PR is this?

#### What this PR does / why we need it

Updating Lookout to include a hot/cold flag for utilizing the hot/cold
partitioned jobs database, as well as updating the lookout pruner to
only prune jobs from the `job_terminated` table when hot/cold is in use

#### Which issue(s) this PR fixes
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related
issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

#### Special notes for your reviewer

---------

Signed-off-by: David Slear <david_slear@yahoo.com>
Signed-off-by: Yasmine Hines <yhines004@gmail.com>
<!-- Thanks for sending a pull request! Here are some tips for you: -->

#### What type of PR is this?

feature / observability

#### What this PR does / why we need it

Adds a Prometheus counter metric to the lookout ingester to track job state updates processed by UpdateJobs(). The metric (`lookout_ingester_job_state_updates_total`) is labeled by state, allowing operators to observe transition rates and specifically monitor terminal state updates, which trigger cross-partition row movement in the Lookout database.

Also fixes error categorization schema in `_local/executor` configs that was broken by a prior change.

#### Special notes for your reviewer

The `terminal_state_updates_total` counter was initially added as a standalone metric but was removed in favor of deriving it via PromQL from the per-state counter (sum by state where state is terminal).

---------

Signed-off-by: Ian Hockett <ian@hockett.net>
Signed-off-by: Yasmine Hines <yhines004@gmail.com>
Signed-off-by: Yasmine Hines <yhines004@gmail.com>
@YHines004 YHines004 force-pushed the fix/Job-JobSets-visual-active-tab branch from 7467ad3 to 60bc2e1 Compare May 4, 2026 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants