Skip to content

Comments

refactor(browser): migrate Browser view to TypeScript#322

Open
devin-ai-integration[bot] wants to merge 2 commits intomainfrom
devin/1771446034-browser-js-to-ts
Open

refactor(browser): migrate Browser view to TypeScript#322
devin-ai-integration[bot] wants to merge 2 commits intomainfrom
devin/1771446034-browser-js-to-ts

Conversation

@devin-ai-integration
Copy link

@devin-ai-integration devin-ai-integration bot commented Feb 18, 2026

refactor(browser): migrate Browser view to TypeScript

Summary

  • Migrated app/components/Views/Browser/index.jsindex.tsx with explicit TS types for BrowserProps, tab shape, and route params.
  • Removed PropTypes and tightened a few runtime checks uncovered by TS (e.g., guarding activeTab lookup before screenshot, guarding currentSelectedAccount before Solana check).
  • Updated toast payloads to include required hasNoTimeout field.
  • Placed solanaLogo static import inside the ///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps) conditional compilation block so it is stripped alongside its only usage — avoids unused-variable lint errors during non-keyring-snaps bundle builds.
  • Renamed action imports with aliases (createNewTabAction, etc.) and local functions (handleShowTabs, handleCloseTab, handleCloseAllTabs) to satisfy ESLint no-shadow rules.
  • Adjusted BrowserTabProps in app/components/Views/BrowserTab/types.ts by making a few props optional (activeTab, defaultProtocol, addBookmark, searchEngine) to align with actual usage.
  • Fixed a test type mismatch: timestamp: '987'timestamp: 987.

Review & Testing Checklist for Human

  • Verify the browserUrl as string casts (lines ~245, ~270) are safe — browserUrl is string | undefined, and while there's a truthiness guard before one usage, confirm no edge case can reach URLParse with undefined.
  • Confirm that linkType ?? '' in mapDispatchToProps doesn't change behavior vs. passing undefined — check if createNewTab action treats empty string and undefined differently.
  • Confirm captureScreen options use width/height (not THUMB_WIDTH/THUMB_HEIGHT as direct keys) — the old keys don't exist in CaptureOptions.
  • Sanity check that making the BrowserTabProps fields optional (activeTab, defaultProtocol, addBookmark, searchEngine) doesn't mask a real missing-prop issue in BrowserTab.
  • Smoke test in-app Browser screen: open browser, create new tab, switch tabs, close tab(s), close all tabs.
  • (keyring-snaps build) Verify Solana "coming soon" toast still renders correctly — the solanaLogo import now lives inside the conditional block.

Notes

  • CI caveat: lint, lint:tsc, and test:depcheck were cancelled (not passed) because they share a workflow with audit:ci, which fails due to pre-existing dependency vulnerabilities unrelated to this PR. Both tsc --noEmit and eslint were verified locally with no new errors. Unit tests (all 10 shards) and js-bundle-size-check passed in CI.
  • Devin run: https://app.devin.ai/sessions/eb740015f5c344e9b9331896ff831ec6
  • Requested by: @shayanshafii

Open with Devin

Co-Authored-By: shayan@cognition.ai <shayan@cognition.ai>
@devin-ai-integration
Copy link
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Author

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

Open in Devin Review

updateTab: (id, url) => dispatch(updateTab(id, url)),
const mapDispatchToProps = (dispatch: Dispatch) => ({
createNewTab: (url: string, linkType?: string) =>
dispatch(createNewTabAction(url, linkType ?? '')),
Copy link
Author

Choose a reason for hiding this comment

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

🟡 linkType ?? '' coerces undefined to empty string, changing stored tab data

When createNewTab is called without a linkType (e.g., when opening a new blank tab at app/components/Views/Browser/index.tsx:304), the mapDispatchToProps wrapper now coerces undefined to '' before dispatching.

Root Cause and Impact

The original JS code at app/actions/browser/index.js:75-81 passed linkType through directly:

createNewTab: (url, linkType) => dispatch(createNewTab(url, linkType)),

So the Redux action would contain linkType: undefined, and the tab in the store would have linkType: undefined.

The new code coerces undefined to '':

createNewTab: (url: string, linkType?: string) =>
    dispatch(createNewTabAction(url, linkType ?? '')),

Now the Redux action contains linkType: '', and the tab in the store has linkType: ''.

This means every tab created without an explicit link type (the common case—home page, user-initiated new tabs) now stores linkType: '' instead of linkType: undefined. Any downstream code that checks tab.linkType !== undefined or uses strict equality to distinguish "no link type" from "has link type" will behave differently. The ?? is unnecessary here since createNewTabAction (plain JS) happily accepts undefined.

Suggested change
dispatch(createNewTabAction(url, linkType ?? '')),
dispatch(createNewTabAction(url, linkType)),
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Co-Authored-By: shayan@cognition.ai <shayan@cognition.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant