Skip to content

feat: add vertical input for Stackbit 1248#862

Open
bitcoisas wants to merge 3 commits into
selfcustody:developfrom
bitcoisas:feat/stackbit-1248-vertical-input
Open

feat: add vertical input for Stackbit 1248#862
bitcoisas wants to merge 3 commits into
selfcustody:developfrom
bitcoisas:feat/stackbit-1248-vertical-input

Conversation

@bitcoisas
Copy link
Copy Markdown

@bitcoisas bitcoisas commented May 18, 2026

What is this PR for?

This PR adds vertical layout support for Stackbit 1248 seed input, covering the input/loading side of #834.

A "Vertical" option is added to the "Load from 1248" submenu alongside the existing "Standard" layout. Also, this PR complement this other one: #847 (backup side).

Closes input part of #834

Changes made to:

  • Code
  • Tests
  • Docs
  • CHANGELOG

Did you build the code and tested on device?

e651e85a-b86d-4dfe-9299-7390d7948825

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Docs update
  • Other

Screenshots:

load-mnemonic-via-stackbit-vertical-filled-250 en image

@bitcoisas bitcoisas changed the base branch from main to develop May 18, 2026 23:15
@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.35%. Comparing base (9ef5803) to head (5bb237a).
⚠️ Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #862      +/-   ##
===========================================
+ Coverage    97.30%   97.35%   +0.05%     
===========================================
  Files           83       83              
  Lines        10798    10973     +175     
===========================================
+ Hits         10507    10683     +176     
+ Misses         291      290       -1     

☔ 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.

@bitcoisas bitcoisas force-pushed the feat/stackbit-1248-vertical-input branch 3 times, most recently from 73e2c55 to ab698b7 Compare May 18, 2026 23:50
@bitcoisas bitcoisas marked this pull request as ready for review May 19, 2026 01:28
@bitcoisas
Copy link
Copy Markdown
Author

Should these changes be added to the changelog? If so, for the May release?

@qlrd
Copy link
Copy Markdown
Member

qlrd commented May 19, 2026

Should these changes be added to the changelog? If so, for the May release?

For now make it drat and make changes on CHANGELOG.md

@odudex
Copy link
Copy Markdown
Member

odudex commented May 19, 2026

As the form factor allows, try to increase grid size and better use screen surface, making it easier to type.

@bitcoisas bitcoisas marked this pull request as draft May 19, 2026 14:01
Copy link
Copy Markdown
Member

@qlrd qlrd left a comment

Choose a reason for hiding this comment

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

some nits to DRY:

Comment thread tests/pages/test_stackbit.py Outdated
Comment thread tests/pages/test_stackbit.py Outdated
Comment thread tests/pages/test_stackbit.py Outdated
Comment thread src/krux/pages/mnemonic_loader.py
Comment thread src/krux/pages/mnemonic_loader.py Outdated
@bitcoisas bitcoisas force-pushed the feat/stackbit-1248-vertical-input branch 2 times, most recently from 1e11ac2 to 70f6ffb Compare May 19, 2026 17:52
@bitcoisas
Copy link
Copy Markdown
Author

@qlrd Implemented the DRY refactor suggestion and removed the layout size limiter for larger devices @odudex, now making full use of the available screen. Updated docs and screenshots accordingly.

Still need to test on device, i will update here later today! :)

@bitcoisas bitcoisas marked this pull request as ready for review May 19, 2026 18:07
@bitcoisas
Copy link
Copy Markdown
Author

Still need to test on device, i will update here later today! :)

Done, updated the PR Device image tho!

@bitcoisas bitcoisas requested a review from qlrd May 20, 2026 16:06
Copy link
Copy Markdown
Member

@qlrd qlrd left a comment

Choose a reason for hiding this comment

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

LGTM, but if possible, i would check the uncovered lines. Saw them and found weird that they aren't covered. Either untested by some case or those pieces receive more love.

Copy link
Copy Markdown

@joaozinhom joaozinhom left a comment

Choose a reason for hiding this comment

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

LGTM, there are some uncovered lines to review but i think its a simple mock test work, in my view the uncovered lines are in the following functions:
_toggle_bit_vertical;
_index_vertical;

@qlrd
Copy link
Copy Markdown
Member

qlrd commented May 21, 2026

LGTM, there are some uncovered lines to review but i think its a simple mock test work, in my view the uncovered lines are in the following functions: _toggle_bit_vertical; _index_vertical;

One thing that i learned here is that we have potential dead codes, @bitcoisas check if they're dead codes or just need mock like @joaozinhom says

@bitcoisas
Copy link
Copy Markdown
Author

_toggle_bit_vertical;
_index_vertical;

i will work on the coverage later today, checking if we have dead code and also writing some new unit tests too! Thanks again team learning a lot with your guidance! :)

@bitcoisas bitcoisas force-pushed the feat/stackbit-1248-vertical-input branch 3 times, most recently from e6c5070 to 22e2533 Compare May 22, 2026 02:21
@bitcoisas bitcoisas requested review from joaozinhom and qlrd May 22, 2026 02:27
@bitcoisas
Copy link
Copy Markdown
Author

LGTM, there are some uncovered lines to review but i think its a simple mock test work, in my view the uncovered lines are in the following functions: _toggle_bit_vertical; _index_vertical;

One thing that i learned here is that we have potential dead codes, @bitcoisas check if they're dead codes or just need mock like @joaozinhom says

Good catch @qlrd and @joaozinhom ! The if digits[col] == 0 branch in both clamps is unreachable since those cases are already blocked by VERT_INVALID_CELLS, so I simplified both to new_val = 0.

Added unit tests for the remaining paths: early return for invalid cells and forward navigation in _index_vertical.

Now we have the 100% coverage and a cleaner code. Thank you again for the reviews.

Copy link
Copy Markdown

@joaozinhom joaozinhom left a comment

Choose a reason for hiding this comment

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

ACK 22e2533

@qlrd
Copy link
Copy Markdown
Member

qlrd commented May 23, 2026

Two last commits could go as one

@bitcoisas bitcoisas force-pushed the feat/stackbit-1248-vertical-input branch from 22e2533 to 459abe5 Compare May 24, 2026 00:39
@bitcoisas
Copy link
Copy Markdown
Author

Two last commits could go as one

Done, squashed into one!

Copy link
Copy Markdown
Member

@qlrd qlrd left a comment

Choose a reason for hiding this comment

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

Some nits and a doubt

Comment thread src/krux/pages/stack_1248.py Outdated
Comment thread src/krux/pages/stack_1248.py Outdated
Comment thread src/krux/pages/stack_1248.py Outdated
Comment thread src/krux/pages/stack_1248.py Outdated
@bitcoisas bitcoisas force-pushed the feat/stackbit-1248-vertical-input branch 3 times, most recently from 477bd0e to cffc8af Compare May 26, 2026 00:07
@bitcoisas bitcoisas requested a review from qlrd May 26, 2026 19:16
Comment thread src/krux/pages/stack_1248.py Outdated
Comment thread src/krux/pages/stack_1248.py Outdated
Comment thread tests/pages/test_login.py
Comment on lines +1399 to +1400
[BUTTON_ENTER] # Select "Standard" from Standard/Vertical submenu
+ (
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.

Dont get why change this

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This extra button is required because our PR added a Standard/Vertical orientation selector before entering the 1248 grid

Comment thread tests/pages/test_stackbit.py Outdated
Comment thread tests/pages/test_stackbit.py Outdated
Comment thread tests/pages/test_stackbit.py
Comment thread CHANGELOG.md Outdated
Comment thread docs/getting-started/usage/loading-a-mnemonic.en.md Outdated
@bitcoisas
Copy link
Copy Markdown
Author

Just a FUP, i'm working on the updates that you suggested/asked to change here @qlrd. And thank you very much to make me level up every day! :)

@bitcoisas bitcoisas force-pushed the feat/stackbit-1248-vertical-input branch from cffc8af to 970d311 Compare June 2, 2026 01:14
@bitcoisas bitcoisas force-pushed the feat/stackbit-1248-vertical-input branch 2 times, most recently from 419a5f1 to 70b6152 Compare June 2, 2026 01:28
@bitcoisas bitcoisas force-pushed the feat/stackbit-1248-vertical-input branch from 70b6152 to 5bb237a Compare June 2, 2026 01:39
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