Skip to content

Format#23

Closed
yamijuan wants to merge 7 commits intomainfrom
juan/status
Closed

Format#23
yamijuan wants to merge 7 commits intomainfrom
juan/status

Conversation

@yamijuan
Copy link
Collaborator

@yamijuan yamijuan commented May 14, 2025

PR Type

Enhancement


Description

  • Added status card showing current page info

  • Updated tab change status updates

  • Fixed element selection in archive list

  • Improved code formatting and layout


Changes walkthrough 📝

Relevant files
Formatting
5 files
argo-archive-list.ts
Reorganized CSS styles for card container                               
+3/-4     
browser-recorder.ts
Improved code formatting for better readability                   
+9/-3     
downloader.ts
Formatted multiline string concatenations                               
+6/-2     
ipfsutils.ts
Improved template string formatting                                           
+5/-1     
recproxy.ts
Formatted function parameters for better readability         
+15/-4   
Enhancement
5 files
bg.ts
Added sidepanel port and tab change status updates             
+85/-104
recorder.ts
Added favicon and page title to status updates                     
+44/-21 
sidepanel.ts
Implemented status card and UI improvements                           
+453/-118
utils.ts
Added utility functions for UI display                                     
+26/-0   
sidepanel.html
Restructured HTML for new layout                                                 
+7/-67   

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

    Potential Bug

    The sidepanelHandler function has an early return statement that might prevent proper port handling. The condition at line 72-75 returns without assigning the port to sidepanelPort, which could cause issues with tab status updates.

    if (
      !port.sender ||
      port.sender.url !== chrome.runtime.getURL("sidepanel.html")
    ) {
      return;
    Possible Memory Leak

    The registerMessages method creates a connection to the background script but doesn't handle disconnection or cleanup when the component is destroyed, which could lead to memory leaks or stale connections.

    registerMessages() {
      // @ts-expect-error - TS2339 - Property 'port' does not exist on type 'ArgoViewer'.
      this.port = chrome.runtime.connect({ name: "sidepanel-port" });
    
      this.updateTabInfo();
    
      // @ts-expect-error - TS2339 - Property 'port' does not exist on type 'ArgoViewer'.
      this.port.onMessage.addListener((message) => {
        this.onMessage(message);
      });
    Inefficient Code

    The saveScreenshot method creates multiple duplicate objects with similar properties. The thumbData object creation could be simplified to avoid redundancy and improve maintainability.

    const thumbData = {
      ...fullData,
      url: "urn:thumbnail:" + pageInfo.url,
      respHeaders: {
        "Content-Type": mime,
        "Content-Length": thumbPayload.length + "",
      },
      payload: thumbPayload,
    };

    Comment on lines +352 to 354
    this.archiveList = this.shadowRoot?.getElementById(
    "archive-list",
    ) as ArgoArchiveList;

    Choose a reason for hiding this comment

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

    Suggestion: The code is using optional chaining with shadowRoot?.getElementById() but doesn't handle the case where the element might not be found. Add a null check to prevent potential runtime errors. [possible issue, importance: 7]

    Suggested change
    this.archiveList = this.shadowRoot?.getElementById(
    "archive-list",
    ) as ArgoArchiveList;
    const archiveListElement = this.shadowRoot?.getElementById("archive-list");
    if (!archiveListElement) {
    console.error("Archive list element not found in the shadow DOM");
    return;
    }
    this.archiveList = archiveListElement as ArgoArchiveList;

    Comment on lines +76 to +82
    ,
    md-elevated-card {
    display: block;
    margin: 1rem 0;
    padding: 0;
    overflow: visible;
    }

    Choose a reason for hiding this comment

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

    Suggestion: There's a syntax error in the CSS with a leading comma before md-elevated-card selector, which will cause the style rule to be ignored. Remove the comma to fix the CSS syntax. [possible issue, importance: 9]

    Suggested change
    ,
    md-elevated-card {
    display: block;
    margin: 1rem 0;
    padding: 0;
    overflow: visible;
    }
    md-elevated-card {
    display: block;
    margin: 1rem 0;
    padding: 0;
    overflow: visible;
    }

    Comment on lines +1062 to +1064
    const statusline = `${method} ${url.slice(
    urlParsed.origin.length,
    )} HTTP/1.1`;

    Choose a reason for hiding this comment

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

    Suggestion: The statusline constant is defined but never used in the surrounding code. This variable appears to be important for creating the HTTP request record but is not being passed to the WARCRecord.create function. Ensure this value is properly utilized in the request record creation. [possible issue, importance: 3]

    Suggested change
    const statusline = `${method} ${url.slice(
    urlParsed.origin.length,
    )} HTTP/1.1`;
    const urlParsed = new URL(url);
    const statusline = `${method} ${url.slice(
    urlParsed.origin.length,
    )} HTTP/1.1`;
    const reqRecord = WARCRecord.create(
    {
    warcType: "request",
    warcTargetURI: url,
    warcDate: warcDate,
    warcRecordID: reqId,
    statusline: statusline,
    // other properties...
    },
    // other parameters...
    );

    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