Skip to content

pam: use dynamic padding for URL/Code labels alignment#1472

Merged
nooreldeenmansour merged 2 commits intomainfrom
dynamic-padding-layout
Apr 22, 2026
Merged

pam: use dynamic padding for URL/Code labels alignment#1472
nooreldeenmansour merged 2 commits intomainfrom
dynamic-padding-layout

Conversation

@nooreldeenmansour
Copy link
Copy Markdown
Member

Replace hardcoded extra spaces in the URL/Code format strings with a shared formatAlignedFields helper that computes padding at runtime based on label widths. This ensures consistent visual alignment regardless of label length, making the output safe for future translations.

Follow-up to #1437

Comment thread pam/internal/adapter/nativemodel.go Outdated
Comment thread pam/internal/adapter/utils.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR replaces hardcoded spacing in the QR-code URL/Code output with a shared helper that computes padding dynamically, aiming to keep the labels visually aligned even when label text changes (e.g., via translation).

Changes:

  • Add formatAlignedFields helper to align label/value lines via runtime padding.
  • Update QR code rendering in both qrcodeModel.View() and nativeModel.handleQrCode() to use the helper.
  • Add unit tests covering several ASCII-label alignment scenarios.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
pam/internal/adapter/utils.go Adds formatAlignedFields helper used for aligned label/value formatting.
pam/internal/adapter/qrcodemodel.go Switches QR code view label/value lines to use formatAlignedFields.
pam/internal/adapter/nativemodel.go Switches native QR code prompt message label/value lines to use formatAlignedFields.
pam/internal/adapter/utils_test.go Adds unit tests for the new alignment helper.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pam/internal/adapter/utils.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.86%. Comparing base (1f7d986) to head (170cfda).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1472      +/-   ##
==========================================
- Coverage   86.94%   86.86%   -0.09%     
==========================================
  Files          93       93              
  Lines        6366     6379      +13     
  Branches      111      111              
==========================================
+ Hits         5535     5541       +6     
- Misses        775      778       +3     
- Partials       56       60       +4     

☔ View full report in Codecov by Sentry.
📢 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.

Comment thread pam/internal/adapter/utils.go
@nooreldeenmansour nooreldeenmansour changed the title pam: use dynamic padding for QR code URL/Code label alignment pam: use dynamic padding for URL/Code labels alignment Apr 17, 2026
@nooreldeenmansour nooreldeenmansour requested a review from 3v1n0 April 17, 2026 15:09
Comment thread pam/internal/adapter/utils.go Outdated
Comment thread pam/internal/adapter/utils_test.go Outdated
Copy link
Copy Markdown
Contributor

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

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

This a part, looks good to me, we need to update the tests now though.

Replace hardcoded extra spaces in the URL/Code format strings with a shared `formatAlignedFields` helper that computes padding at runtime based on label widths. This ensures consistent visual alignment regardless of label length, making the output safe for future translations.

Follow-up to #1437
@nooreldeenmansour
Copy link
Copy Markdown
Member Author

This a part, looks good to me, we need to update the tests now though.

Which tests are you talking about?

@3v1n0
Copy link
Copy Markdown
Contributor

3v1n0 commented Apr 20, 2026

Which tests are you talking about?

I guess this requires updating the golden files for the SSH tests, no?

@3v1n0
Copy link
Copy Markdown
Contributor

3v1n0 commented Apr 20, 2026

Which tests are you talking about?

I guess this requires updating the golden files for the SSH tests, no?

Oh, if not, well, it may worth adding a case in the example broker were we have a url without code, if did not hit that codepath..

@nooreldeenmansour
Copy link
Copy Markdown
Member Author

nooreldeenmansour commented Apr 20, 2026

I guess this requires updating the golden files for the SSH tests, no?

The CI tests jobs are currently passing, so it doesn't seem to be required? (Please correct me if I am wrong)

it may worth adding a case in the example broker were we have a url without code, if did not hit that codepath..

The unit tests in this PR has the test Single field has no extra padding which contains only URL and no code (but that tests the formatting function)

I'll examine the broker tests to see if there is a one that covers the single field scenario. if not, I'll create a one. This one would be an integration test?

Edit: I've added a test in the examplebroker

@nooreldeenmansour nooreldeenmansour requested a review from 3v1n0 April 20, 2026 14:08
Comment thread examplebroker/broker_test.go Outdated
Copy link
Copy Markdown
Contributor

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Copy Markdown
Contributor

@adombeck adombeck left a comment

Choose a reason for hiding this comment

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

Not sure I agree it was worth spending time on before we actually support translations (not saying that i18n is not important, but there are many other things on our roadmap and we need to prioritize). Anyway, the implementation looks good to me.

@nooreldeenmansour nooreldeenmansour merged commit e9e7bfd into main Apr 22, 2026
38 of 43 checks passed
@nooreldeenmansour nooreldeenmansour deleted the dynamic-padding-layout branch April 22, 2026 13:09
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