Skip to content

Nikita onboarding flow #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 23 commits into from
Jun 4, 2025
Merged

Nikita onboarding flow #40

merged 23 commits into from
Jun 4, 2025

Conversation

nikitalokhmachev-ai
Copy link
Collaborator

@nikitalokhmachev-ai nikitalokhmachev-ai commented May 30, 2025

User description

  • Added the onboarding flow
  • Fixed a few bugs tied to recording web pages

PR Type

Enhancement, Bug fix


Description

  • Added onboarding flow with tutorial carousel

  • Fixed recording status updates in sidepanel

  • Improved debugger detachment handling

  • Added blocked domains status indicator


Changes walkthrough 📝

Relevant files
Bug fix
bg.ts
Improve recorder status handling and error recovery           

src/ext/bg.ts

  • Added status updates for tabs without recorders
  • Fixed debugger detachment when navigating to skipped domains
  • Added error handling for "already attached" debugger errors
  • Improved status notification when stopping recording
  • +55/-1   
    Enhancement
    globals.d.ts
    Add image file type declarations                                                 

    src/globals.d.ts

    • Added declarations for PNG and JPG image file types
    +2/-0     
    onboarding.ts
    Create onboarding tutorial carousel component                       

    src/onboarding.ts

  • Created new onboarding component with carousel interface
  • Implemented multi-step tutorial with navigation controls
  • Added styling for slides, buttons, and transitions
  • Included introduction to Packrat functionality and data privacy
  • +321/-0 
    settings-page.ts
    Add onboarding toggle in settings                                               

    src/settings-page.ts

  • Added option to toggle onboarding display
  • Implemented settings persistence for onboarding preference
  • Added UI switch and description for the onboarding setting
  • +27/-0   
    sidepanel.ts
    Integrate onboarding and improve status indicators             

    src/sidepanel.ts

  • Added onboarding component integration
  • Implemented blocked status indicator for skip-listed domains
  • Fixed domain skip list updates when exiting settings
  • Added handler for onboarding completion
  • +51/-6   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @pr-agent-monadical
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    CSS Opacity Issue

    The slide opacity is set to 1 in the CSS but there's a comment indicating it was changed from 0 to 1. This might cause all slides to be visible simultaneously during transitions instead of only showing the active slide.

    opacity: 1; /* Changed from 0 to 1 */
    Error Handling

    The error handling in the debugger detachment code could be improved. The code catches errors but doesn't properly distinguish between expected errors (already detached) and unexpected errors.

    try {
      chrome.debugger.detach({ tabId }, () => {
        // Debugger detached, ignore any errors as it might already be detached
      });
    } catch (e) {
      // Ignore errors - debugger might already be detached
    }
    Type Safety

    Multiple @ts-expect-error annotations throughout the code indicate potential type safety issues that should be properly addressed rather than suppressed.

    // @ts-expect-error
    this.skipDomains = await getLocalOption("skipDomains");

    Comment on lines +485 to +495
    // @ts-expect-error
    if (err?.message?.includes("already attached")) {
    // Try to detach and delete the recorder
    try {
    chrome.debugger.detach({ tabId }, () => {
    delete self.recorders[tabId];
    });
    } catch (detachErr) {
    console.warn("Failed to detach debugger:", detachErr);
    }
    }

    Choose a reason for hiding this comment

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

    Suggestion: The error handling only cleans up when the error message includes "already attached", but other errors should also trigger cleanup to prevent resource leaks. The recorder should be deleted for any error during attachment. [possible issue, importance: 8]

    Suggested change
    // @ts-expect-error
    if (err?.message?.includes("already attached")) {
    // Try to detach and delete the recorder
    try {
    chrome.debugger.detach({ tabId }, () => {
    delete self.recorders[tabId];
    });
    } catch (detachErr) {
    console.warn("Failed to detach debugger:", detachErr);
    }
    }
    // Clean up on error
    try {
    if (err?.message?.includes("already attached")) {
    // Try to detach the debugger first for "already attached" errors
    chrome.debugger.detach({ tabId }, () => {
    console.log("Detached debugger after 'already attached' error");
    });
    }
    // Delete the recorder for any error
    delete self.recorders[tabId];
    } catch (detachErr) {
    console.warn("Failed to detach debugger:", detachErr);
    }

    Comment on lines 199 to +203
    if (!this.showingSettings) {
    // re-load the list from storage
    // @ts-expect-error
    this.skipDomains = await getLocalOption("skipDomains");

    Choose a reason for hiding this comment

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

    Suggestion: The code doesn't check if getLocalOption("skipDomains") returns a valid array or string. This could lead to runtime errors if the returned value is null, undefined, or an unexpected type. Add proper type checking and conversion. [possible issue, importance: 9]

    Suggested change
    if (!this.showingSettings) {
    // re-load the list from storage
    // @ts-expect-error
    this.skipDomains = await getLocalOption("skipDomains");
    private async _toggleSettings() {
    this.showingSettings = !this.showingSettings;
    // when toggling *off* settings, reload skip-list and re-query
    if (!this.showingSettings) {
    // re-load the list from storage
    // @ts-expect-error
    const domains = await getLocalOption("skipDomains");
    this.skipDomains = Array.isArray(domains)
    ? domains
    : typeof domains === "string"
    ? domains.split("\n").filter(Boolean)
    : [];
    // re-run your normal "update everything" flow
    this.updateTabInfo();
    // wait for the archive list element to re-render
    await this.updateComplete;
    this.archiveList = this.shadowRoot!.getElementById(
    "archive-list",
    ) as ArgoArchiveList;
    }
    }

    @Shrinks99 Shrinks99 linked an issue Jun 2, 2025 that may be closed by this pull request
    1 task
    @Shrinks99 Shrinks99 self-requested a review June 2, 2025 15:16
    @Shrinks99 Shrinks99 self-assigned this Jun 2, 2025
    @Shrinks99
    Copy link
    Collaborator

    Almost completed some styling changes for this, simplified the transforms a bunch! Pls don't merge just yet will be done tomorrow.

    @Shrinks99
    Copy link
    Collaborator

    Shrinks99 commented Jun 4, 2025

    Okay! This is about where I'd like it for launch! Ready to merge from me :D

    Some screenshots of our new on-boarding illustrations, decided to stick with visual metaphors over screenshots because A: they're more fun, and B: I think our app is pretty simple? There's basically one button. I'm hoping people will get it based on context clues and UI copy alone.

    Screenshot 2025-06-03 at 8 16 15 PM Screenshot 2025-06-03 at 8 16 21 PM Screenshot 2025-06-03 at 8 16 27 PM

    @Shrinks99 Shrinks99 merged commit cac7ef3 into main Jun 4, 2025
    0 of 3 checks passed
    @Shrinks99 Shrinks99 deleted the nikita-onboarding-flow branch June 4, 2025 00:27
    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.

    App Onboarding
    2 participants