Skip to content

fix(autorag): dynamically size topology nodes to fit long task labels#7208

Merged
openshift-merge-bot[bot] merged 2 commits intoopendatahub-io:mainfrom
jefho-rh:autorag-node-gap
Apr 13, 2026
Merged

fix(autorag): dynamically size topology nodes to fit long task labels#7208
openshift-merge-bot[bot] merged 2 commits intoopendatahub-io:mainfrom
jefho-rh:autorag-node-gap

Conversation

@jefho-rh
Copy link
Copy Markdown
Contributor

@jefho-rh jefho-rh commented Apr 13, 2026

https://redhat.atlassian.net/browse/RHOAIUX-2271

Description

Applies the same dynamic node sizing from #7189 (automl) to the autorag topology view. Uses canvas text measurement to compute node widths based on actual label text, preventing long task names from being clipped. Falls back to the default NODE_WIDTH when canvas is unavailable.

Before
Screenshot 2026-04-13 at 9 42 10 AM

After
Screenshot 2026-04-13 at 9 42 44 AM

How Has This Been Tested?

  • Unit tests added for createNode and measureTextWidth (canvas available and unavailable paths)
  • Lint, type-check, and all unit tests pass

Test Impact

  • Added packages/autorag/frontend/src/app/topology/__tests__/utils.spec.ts (6 tests)

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)
  • The code follows our Best Practices (React coding standards, PatternFly usage, performance considerations)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests

    • Added comprehensive test suite for node creation, validating dynamic width calculation, label handling, and fallback behavior.
  • Improvements

    • Nodes now dynamically size based on label text width for better layout presentation, with fallback support when text measurement is unavailable.

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 13, 2026

📝 Walkthrough

Walkthrough

Changes add a test suite for the createNode function with mocked topology dependencies and canvas text-measurement scenarios. The implementation now computes node width dynamically by measuring label text width using a canvas rendering context, with fallback to a constant minimum width. Two new layout constants (NODE_PADDING and NODE_FONT) support the text measurement logic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Actionable issues

Canvas context persistence and invalidation risk — The cached CanvasRenderingContext2D assumes persistent validity once initialized. In edge cases (e.g., canvas element removal, document reflow, or worker/JSDOM environments), the context can become invalid without triggering reinitialization. Consider defensive checking before reuse or document context changes.

Font measurement mismatchNODE_FONT is set statically in constants, but actual DOM rendering may apply different fonts via CSS cascade, media queries, or theme overrides. If rendered font diverges from the measurement font, computed widths will be inaccurate. Verify that NODE_FONT remains synchronized with actual rendered styles or measure from the live DOM node instead.

Missing null safety on document — Lazy initialization checks typeof document, but subsequent canvas operations don't guard against document becoming unavailable or the context failing silently. Wrap context operations to handle null/undefined gracefully.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive PR description references #7189 as prior art but lacks a linked issue tracking the autorag-specific work. Verify that an issue (e.g., RHOAIENG-#####) is linked in the PR to track the autorag node sizing improvement.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: adding dynamic node sizing to the autorag topology view to accommodate longer task labels.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to autorag topology node sizing: test file, two new constants, and dynamic width logic.
Description check ✅ Passed PR description includes all required template sections with clear details: linked issue (RHOAIUX-2271), comprehensive description with before/after screenshots, testing methodology, test impact summary, and a completed self-checklist with 4/4 items checked.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jefho-rh jefho-rh requested review from GAUNSD, chrjones-rh and nickmazzi and removed request for NickGagan April 13, 2026 13:46
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/autorag/frontend/src/app/topology/utils.ts`:
- Around line 5-10: The function getCanvasContext repeatedly recreates a canvas
when getContext('2d') returns null because cachedCtx is initialized to null and
the guard uses !cachedCtx; change the caching logic so a null result is cached:
make cachedCtx start as undefined (type CanvasRenderingContext2D | null |
undefined) and change the condition in getCanvasContext to check cachedCtx ===
undefined so the first lookup stores either a context or null and subsequent
calls (e.g., from createNode) reuse that cached value.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 0713eea4-9823-499e-a2a1-206836e24045

📥 Commits

Reviewing files that changed from the base of the PR and between 8f14bce and 953830b.

📒 Files selected for processing (3)
  • packages/autorag/frontend/src/app/topology/__tests__/utils.spec.ts
  • packages/autorag/frontend/src/app/topology/const.ts
  • packages/autorag/frontend/src/app/topology/utils.ts

Comment thread packages/autorag/frontend/src/app/topology/utils.ts
@nickmazzi nickmazzi self-assigned this Apr 13, 2026
@nickmazzi
Copy link
Copy Markdown
Contributor

Thanks @jefho-rh

AI assisted review

pr-review-7208.md

Overall Assessment

Approve -- this is a clean, well-structured port of an already-merged pattern. The implementation is correct, the tests are thorough, and the fallback behavior is sound. A few non-blocking observations below.

Suggestions

  1. Future: Extract shared topology utilities -- Consider a follow-up task to extract the duplicated topology types and node creation utilities into a shared package. Both automl and autorag (and potentially future packages) use identical code. -- being handled by Anish

  2. Cosmetic: Suppress jsdom canvas warning -- The console.error from jsdom about HTMLCanvasElement.prototype.getContext is harmless but noisy. Could add jest.spyOn(console, 'error').mockImplementation() in an appropriate setup hook if test output cleanliness matters.

  3. Robustness: Max width cap -- There's no upper bound on node width. An extremely long label would produce an extremely wide node. Consider adding a Math.min(MAX_NODE_WIDTH, ...) cap if there are labels that could be arbitrarily long. Looking at TASK_DISPLAY_NAMES in useAutoRAGTaskTopology.ts, the longest name is ~30 characters ("Prepare Responses API Requests"), so this is unlikely to be an issue in practice. Non-blocking.

Test Validation

autorag

Frontend ✅

Test Suites: 38 passed, 38 total
Tests:       634 passed, 634 total
Snapshots:   0 total
Time:        20.28 s

BFF ✅

go fmt ./...
go vet ./...
ok      github.com/opendatahub-io/autorag-library/bff/cmd       (cached)
ok      github.com/opendatahub-io/autorag-library/bff/internal/api      (cached)
?       github.com/opendatahub-io/autorag-library/bff/internal/config   [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/constants        [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/helpers  [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/integrations     [no test files]
ok      github.com/opendatahub-io/autorag-library/bff/internal/integrations/kubernetes  (cached)
?       github.com/opendatahub-io/autorag-library/bff/internal/integrations/kubernetes/k8mocks  [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/integrations/kubernetes/k8smocks [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/integrations/llamastack  [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/integrations/llamastack/lsmocks  [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/integrations/pipelineserver      [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/integrations/pipelineserver/psmocks     [no test files]
ok      github.com/opendatahub-io/autorag-library/bff/internal/integrations/s3  (cached)
?       github.com/opendatahub-io/autorag-library/bff/internal/integrations/s3/s3mocks  [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/mocks    [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/models   [no test files]
?       github.com/opendatahub-io/autorag-library/bff/internal/pipelines        [no test files]
ok      github.com/opendatahub-io/autorag-library/bff/internal/repositories     (cached)

Contract ✅

Test Suites: 1 passed, 1 total
Tests:       105 passed, 105 total
Snapshots:   0 total
Time:        1.251 s, estimated 2 s
Test Suites: 1 passed, 1 total
Tests:       105 passed, 105 total

Manual validation

No obvious regressions

image

@nickmazzi
Copy link
Copy Markdown
Contributor

/lgtm
/approve

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 13, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nickmazzi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot Bot merged commit 93eb293 into opendatahub-io:main Apr 13, 2026
51 checks passed
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.81%. Comparing base (8f14bce) to head (1b6894d).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7208      +/-   ##
==========================================
+ Coverage   64.79%   64.81%   +0.02%     
==========================================
  Files        2447     2441       -6     
  Lines       76059    76004      -55     
  Branches    19172    19159      -13     
==========================================
- Hits        49279    49260      -19     
+ Misses      26780    26744      -36     

see 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e464bcd...1b6894d. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants