Skip to content

fix(web): guard undefined hash in commit detail view - #7148

Open
vikash7485 wants to merge 2 commits into
pipe-cd:masterfrom
vikash7485:fix/guard-undefined-commit-hash
Open

fix(web): guard undefined hash in commit detail view#7148
vikash7485 wants to merge 2 commits into
pipe-cd:masterfrom
vikash7485:fix/guard-undefined-commit-hash

Conversation

@vikash7485

Copy link
Copy Markdown

What this PR does:

Guard against undefined hash in the deployment detail commit section by adding a null-coalescing fallback (?? "") before calling .slice(0, 7).

Why we need it:

When deployment.trigger.commit.hash is undefined (possible from older Piped v0 protobuf messages where the field was unpopulated), the current code calls .slice(0, 7) directly on undefined, causing a runtime crash:

TypeError: Cannot read properties of undefined (reading 'slice')

The outer guard at L254 (deployment.trigger?.commit &&) only checks that commit exists, not that hash is populated. This fix adds a safe fallback so the link renders with an empty string instead of crashing.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

No. This prevents a crash for an edge case. Users with valid commit hashes see no difference.

  • How are users affected by this change: Deployments with missing commit hashes no longer crash the detail page.
    • Is this breaking change: No.
    • How to migrate (if breaking change): N/A.

When hash is undefined (e.g. from older Piped v0 messages where the
field was unpopulated), hash.slice(0, 7) throws:
TypeError: Cannot read properties of undefined (reading 'slice')

Add null-coalescing fallback to empty string before slicing.

Related: pipe-cd#6706
Signed-off-by: vikash7485 <vikkiraj073@gmail.com>
@vikash7485
vikash7485 force-pushed the fix/guard-undefined-commit-hash branch from 9fc67c1 to 818473d Compare August 11, 2026 04:02

@rahulshendre rahulshendre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGMT, thanks @vikash7485

one thing I am curious about - commit.hash is typed string from the protobuf (proto3 defaults an unset string to "", not undefined), so I couldn't see where it'd actually be undefined.
If you saw it, please let use know where, mostly so we know if message/url next to it need the same

@vikash7485

Copy link
Copy Markdown
Author

@rahulshendre Thanks for the review and approval

The reason hash was specifically at risk here is that it had a method call (hash.slice(0, 7)), whereas message and url are passed directly into JSX (<Typography> and href), which React safely handles even if they are undefined.

In standard protobuf generated classes, proto3 does default strings to "". However, in frontend tests/mocks, partial payloads, or when objects are deserialized from JSON without emitDefaults: true, omitted properties can end up as undefined in TypeScript. (hash ?? "").slice(0, 7) ensures that even in those edge cases, the component renders safely without throwing a runtime TypeError.

@netlify

netlify Bot commented Aug 22, 2026

Copy link
Copy Markdown

Deploy Preview for pipecd-site canceled.

Name Link
🔨 Latest commit 3a04558
🔍 Latest deploy log https://app.netlify.com/projects/pipecd-site/deploys/6a895c04078bba0008121e43

@khanhtc1202 khanhtc1202 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice catch, thank you 👍

@khanhtc1202
khanhtc1202 enabled auto-merge (squash) August 22, 2026 08:22
@codecov

codecov Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 29.01%. Comparing base (4bb2366) to head (3a04558).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7148      +/-   ##
==========================================
- Coverage   29.68%   29.01%   -0.68%     
==========================================
  Files         601      532      -69     
  Lines       64383    58157    -6226     
==========================================
- Hits        19113    16873    -2240     
+ Misses      43779    39919    -3860     
+ Partials     1491     1365     -126     
Flag Coverage Δ
. 23.58% <ø> (ø)
.-pkg-app-pipedv1-plugin-analysis ?
.-pkg-app-pipedv1-plugin-ecs 31.74% <ø> (ø)
.-pkg-app-pipedv1-plugin-kubernetes 58.39% <ø> (ø)
.-pkg-app-pipedv1-plugin-kubernetes_multicluster 59.62% <ø> (ø)
.-pkg-app-pipedv1-plugin-scriptrun 54.83% <ø> (ø)
.-pkg-app-pipedv1-plugin-terraform ?
.-pkg-app-pipedv1-plugin-wait ?
.-pkg-app-pipedv1-plugin-waitapproval 52.71% <ø> (ø)
.-pkg-plugin-sdk ?
.-tool-actions-gh-release ?
.-tool-actions-plan-preview ?
.-tool-codegen-protoc-gen-auth ?

Flags with carried forward coverage won't be shown. 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:
  • ❄️ 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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants