Skip to content

fix(frontend): reject null payloads in dashboard type predicates#6443

Open
eugenegujing wants to merge 1 commit into
apache:mainfrom
eugenegujing:fix/type-predicates-null-guard
Open

fix(frontend): reject null payloads in dashboard type predicates#6443
eugenegujing wants to merge 1 commit into
apache:mainfrom
eugenegujing:fix/type-predicates-null-guard

Conversation

@eugenegujing

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

The five type guards in dashboard/type/type-predicates.ts checked their nested payload with typeof value.<field> === "object", which also accepts null because typeof null === "object" in JavaScript, so an entry like {workflow: null} passed isDashboardWorkflow and the DashboardEntry constructor then crashed with an unrelated TypeError: Cannot read properties of null while dereferencing the payload, instead of reaching its intentional "Unexpected type in DashboardEntry." error path. The guards also returned the falsy input itself rather than false for null/undefined input, violating their declared boolean type-guard signatures.

This PR adds an isNonNullObject type guard (typeof x === "object" && x !== null) to the shared predicate utility (common/util/predicate.ts, next to isDefined) so the correct non-null object check is discoverable and reusable, uses it in the four object-payload guards, and switches all five guards to !!value && ... so they return strict booleans in every path. isDashboardProject's !value.workflow exclusion is deliberately unchanged: a null workflow field means "no workflow data", so an object with a string name still classifies as a project and no valid entry changes classification.

Any related issues, documentation, discussions?

Fixes #6439. The buggy behavior was pinned by the five "(current behavior)" tests added in #6425 (issue #6400), which this PR flips to assert the corrected behavior.

How was this PR tested?

Added predicate.spec.ts (8 tests) covering isNonNullObject and the previously untested isDefined. Updated type-predicates.spec.ts: the four null-payload pins now assert toBe(false), the null/undefined input cases are tightened from toBeFalsy() to toBe(false), and the isDashboardProject null-workflow case keeps asserting true with a comment marking it as an intentional decision; the 5x5 cross-classification matrix and all of its 25 expected values are untouched, confirming that classification of realistic entries is unaffected. Ran locally via yarn ng test --watch=false --include='**/common/util/predicate.spec.ts' --include='**/dashboard/type/type-predicates.spec.ts' (61/61 pass), plus the full frontend suite whose failure set is identical to the unmodified main baseline (pre-existing, environment-related failures only), and tsc --noEmit with zero errors.

Was this PR authored or co-authored using generative AI tooling?

Co-authored using Claude Code(Fable 5).

Add an `isNonNullObject` guard to `common/util/predicate.ts` (with a new predicate.spec.ts, which also covers the previously untested `isDefined`), use it in the four object-payload guards, and switch all five guards to `!!value` so they always return strict booleans. `isDashboardProject`'s `!value.workflow` exclusion is deliberately unchanged. Spec updated: the four "(current behavior)" null-payload pins now assert false; the 5x5 cross-classification matrix and its 25 expected values are untouched, so no realistic entry changes classification.

Fixes apache#6439
@github-actions github-actions Bot added fix frontend Changes related to the frontend GUI labels Jul 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @aglinxinyuan, @gracecluvohio
    You can notify them by mentioning @aglinxinyuan, @gracecluvohio in a comment.

@codecov-commenter

codecov-commenter commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.17%. Comparing base (aa0bc44) to head (bbbb7b3).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6443      +/-   ##
============================================
- Coverage     70.17%   70.17%   -0.01%     
  Complexity     3388     3388              
============================================
  Files          1142     1142              
  Lines         44844    44846       +2     
  Branches       4949     4950       +1     
============================================
+ Hits          31471    31472       +1     
  Misses        11739    11739              
- Partials       1634     1635       +1     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from aa0bc44
agent-service 76.76% <ø> (ø) Carriedforward from aa0bc44
amber 66.67% <ø> (ø) Carriedforward from aa0bc44
computing-unit-managing-service 9.97% <ø> (ø) Carriedforward from aa0bc44
config-service 52.30% <ø> (ø) Carriedforward from aa0bc44
file-service 65.63% <ø> (ø) Carriedforward from aa0bc44
frontend 68.91% <100.00%> (+<0.01%) ⬆️
notebook-migration-service 78.94% <ø> (ø) Carriedforward from aa0bc44
pyamber 91.19% <ø> (ø) Carriedforward from aa0bc44
workflow-compiling-service 55.14% <ø> (ø) Carriedforward from aa0bc44

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xuang7 xuang7 added the release/v1.2 back porting to release/v1.2 label Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix frontend Changes related to the frontend GUI release/v1.2 back porting to release/v1.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dashboard type predicates accept null nested fields (typeof null === "object"), letting invalid entries crash DashboardEntry construction

3 participants