Skip to content

feat(web): add hide secret control - #443

Merged
Maxerns merged 5 commits into
getsotto:mainfrom
tayfuryldz:fix/185-hide-secret
Oct 12, 2026
Merged

Maxerns merged 5 commits into
getsotto:mainfrom
tayfuryldz:fix/185-hide-secret

Conversation

@tayfuryldz

Copy link
Copy Markdown
Contributor

Fixes #185.

Adds a keyboard-accessible Hide secret action that clears the revealed value and share-link UI, removes aria-current from the secret, and returns focus to its list button. Reveal generations also prevent a pending share response from restoring a dismissed or superseded secret.

Validation:

  • npm run typecheck
  • npm test (61 passed)
  • focused VaultView suite (15 passed)
  • git diff --check

@tayfuryldz
tayfuryldz force-pushed the fix/185-hide-secret branch 4 times, most recently from 468e49a to 449b944 Compare October 1, 2026 20:19
@tayfuryldz

Copy link
Copy Markdown
Contributor Author

Rebased the three feature commits onto current main without conflicts. The focused VaultView suite passes 29/29 and the rebased diff is clean. TypeScript still cannot complete in this checkout because the generated web/src/wasm/sotto_wasm.js artifact is absent; I am not treating that environment prerequisite as a passing typecheck.

@tayfuryldz
tayfuryldz force-pushed the fix/185-hide-secret branch from cf3d5e0 to eb8b070 Compare October 2, 2026 18:16

@Maxerns Maxerns left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The implementation itself looks right.

I verified it in isolation:

  • full suite: 109/109 green
  • typecheck: clean apart from the known WASM issue
  • commits: signed off

I also probed the paths that aren’t currently covered by tests. Hiding a secret correctly drops a late share response, and if secret B is revealed while secret A’s share is still pending, B stays selected and the late link from A never appears. So the generation guard is doing its job. The switch to ID-based aria-current is a nice improvement too.

I’ve just got three test only asks before approving.

First, the issue specifically calls out preserving a newly selected secret when an older share completes. Right now only the hide case is pinned. Could you add the reveal B while A’s share is pending case as well? I confirmed the behaviour already works, so this is just about locking it in.

Second, keyboard activation of Hide isn’t exercised yet; all the coverage uses clicks. Swapping one of the e2e Hide clicks for an Enter keypress would cover the keyboard path the issue asks for without adding much more test code.

Third, the new secret-hiding describe block currently relies on project/org mocks set up by earlier blocks. Running it by itself with -t fails, so it needs its own beforeEach fixture setup to make the tests independent and safe to reorder.

One tiny cleanup while you’re there: there’s also a double blank line between the two funnel tests.

No production changes needed from my side once those tests are tightened up, I’m happy to approve.

@Maxerns Maxerns left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

oops meant to request changes

@tayfuryldz

Copy link
Copy Markdown
Contributor Author

Added the missing stale-share regression (reveal B while A’s share is pending), changed an e2e Hide action to keyboard Enter, and made the secret-hiding unit block independent with its own mock setup. VaultView passes 30/30 and diff-check is clean. Playwright could not run locally because e2e/.fixture.json is missing; I have not claimed a browser pass.

Signed-off-by: tayfuryldzz <yildiztayfur668@gmail.com>
Signed-off-by: tayfuryldz <yildiztayfur668@gmail.com>
Signed-off-by: tayfuryldz <yildiztayfur668@gmail.com>
Signed-off-by: tayfuryldzz <yildiztayfur668@gmail.com>
@tayfuryldz

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and resolved the VaultView overlap by keeping both the upstream secret-search state and this PR’s Hide action. The stale-share/keyboard regression coverage is preserved. Web unit tests pass 138/138 excluding wasmLoader tests that require the missing generated wasm artifact, and git diff --check is clean. The real browser fixture is still unavailable locally.

@tayfuryldz
tayfuryldz force-pushed the fix/185-hide-secret branch from 1bc48d9 to da2f357 Compare October 8, 2026 18:36
@Maxerns

Maxerns commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Re-verified all three follow ups are covered now.

  • the secret-hiding block has its own fixture setup, and the -t isolation run that failed before now passes
  • the supersede test proves a newly selected secret survives a stale share response
  • the e2e Hide path now uses a real Enter keypress rather than a click

VaultView is 36/36 green.

The full suite only shows the known wasmLoader failures from missing generated bindings, which pass in environments where the bindings are present.

@Maxerns
Maxerns merged commit aa49c43 into getsotto:main Oct 12, 2026
22 checks passed
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.

Web: add a Hide secret control to the unlocked vault

2 participants