2025-07-22 - Untitled Projects - CR Feedback from AI.md

Untitled Projects - CR Feedback from Gemini 2.5 Pro 2025-07-22

#file:copilot-instructions.md

I have a draft implementation for a new Untitled Projects feature, described in this GH issue: davidfstr/Crystal-Web-Archiver#102

I would like you to code-review the draft implementation. The diff is in this file: #file:untitled_projects-complete.diff

This is an extremely large diff (6,760 lines), so it may be necessary to break the review process up into multiple steps (i.e. multiple conversational turns).

Suggested review order of files, from highest level to lowest level:

• (The GitHub issue) - Overall feature description
• M RELEASE_NOTES.md - Overall feature description
• A src/crystal/tests/test_untitled_projects.py
• Test names first - Details of feature's intended behaviors. See skeleton with cat src/crystal/tests/test_untitled_projects.py | grep -E '(^(async )?def )|===|(^@skip)', or just read: #file:untitled_tests-outline.py
• Test implementations second
• A src/crystal/tests/util/save_as.py - Key utilities used by test_untitled_projects.py
• M src/crystal/main.py - Now creates new projects as untitled rather than titled
• M src/crystal/browser/init.py - New "Save..." and "Save As..." menuitems
• M src/crystal/model.py
• Core implementation: New Project.{save_as, _save_as_coro, _copytree_of_project_with_progress, _reopen} operations New Project attributes: is_untitled, is_dirty
• M src/crystal/progress.py - New SaveAsProgressDialog
• M src/crystal/tests/util/windows.py - Revised test abstractions used by preexisting tests

---

... (commentary as Gemini examined the files mentioned above)

---

Overall Assessment:

The "Untitled Projects" feature is a significant improvement to the user experience of the Crystal Web Archiver. The implementation is well-designed, robust, and thoroughly tested.

Here are some of the key strengths of the implementation:

• Clean and simple API: The new is_untitled and is_dirty attributes in the Project class provide a clean and simple way to track the state of untitled projects.
• Robust "Save As" functionality: The "Save As" functionality is well-implemented and handles a wide range of scenarios, including moving vs. copying, progress updates, and error handling.
• Thorough test coverage: The feature is well-tested, with a comprehensive suite of unit and integration tests.
• Good user experience: The feature provides a good user experience, with clear and informative dialogs and progress updates.

I have a few minor suggestions for improvement:

• Skipped tests: It would be good to add comments to the skipped tests explaining why they are skipped and if there are plans to implement them in the future.
• TODO comments: There are a few TODO comments in the code that should be addressed.

Overall, this is an excellent implementation of a valuable new feature. I'm confident that it will be a welcome addition to the Crystal Web Archiver.

I'm now ready to proceed with any further instructions you may have.

---

General - From Gemini

I have a few minor suggestions for improvement:

• Skipped tests: It would be good to add comments to the skipped tests explaining why they are skipped and if there are plans to implement them in the future.
• TODO comments: There are a few TODO comments in the code that should be addressed.

Skipped Tests

Please identify any skipped tests you saw that were not clear why they were skipped. Many users of the @skip decorator attempt to explain why skips are done.

• You're right, I did notice a few skipped tests where the reason for the skip could be clearer. Here are the ones that stood out:

  In src/crystal/tests/test_untitled_projects.py:

  1. test_when_save_as_project_with_corrupted_database_then_fails_with_error

     • Reason given: 'not yet automated: not valuable to automate: rare case'
     • Analysis: While it's marked as a "rare case," it's not immediately obvious why it's "not valuable to automate." A corrupted database seems like a significant failure scenario. A more descriptive reason, such as explaining the difficulty in reliably simulating database corruption during a copy, would be more helpful.

  2. test_when_save_as_project_with_missing_revision_files_then_ignores_missing_ revision_files

     • Reason given: 'not yet automated: not valuable to automate: rare case'
     • Analysis: Similar to the above, the value of this test isn't immediately clear from the reason. It seems plausible to automate by deleting a revision file manually from the project directory before the "Save As" operation. A clearer explanation of why this is considered not valuable (e.g., "Project integrity is not guaranteed if manually altered") would be better.

  The other skipped tests had clearer reasons, such as being covered by other tests or being blocked by limitations in the testing framework's ability to interact with native file dialogs.

TODO Comments

Can you tell me about the TODO comments in the code that particularly stood out to you?

• Of course. Here are the TODO comments that stood out to me as particularly noteworthy, categorized by file:

  From src/crystal/tests/test_untitled_projects.py:

  1. # TODO: Add UI support for readonly status of project changing

     • Location: In test_when_save_as_project_and_destination_filesystem_writabl e_generally_but_not_writable_by_sqlite_then_reopens_as_readonly.
     • Why it stands out: This comment indicates a known gap in the UI. The test verifies that the project correctly reopens in a read-only state if the new location is unwritable by SQLite, but it explicitly notes that the UI doesn't yet reflect this change. This is a user-facing inconsistency that could be confusing.

  2. # TODO: Consider altering the behavior such that the copy is opened as writable rather than readonly

     • Location: In test_when_save_as_readonly_project_then_creates_writable_co py_but_opens_as_readonly.
     • Why it stands out: This points to a potential design improvement. The current behavior is that if you "Save As" a read-only project, the new copy is created with writable file permissions, but the application keeps it open in a read-only state. The TODO suggests it might be more intuitive for the newly created project to be opened as writable, which seems like a more user-friendly default.

  From src/crystal/main.py:

  3. # TODO: Alter Project.__init__ to create path internally when is_untitled=True
     • Location: In _create_untitled_project.
     • Why it stands out: This is a good code quality and encapsulation suggestion. Currently, the _create_untitled_project function manually creates a temporary directory path and passes it to the Project constructor. This TODO suggests that the Project class itself should be responsible for creating its own temporary path when it's being initialized as an untitled project, which would make the code cleaner and more self-contained.

  From src/crystal/browser/init.py:

  4. # TODO: Handle ProjectReadOnlyError more gracefully, by providing a more-targeted error message
     • Location: In the on_save_complete callback within _on_save_as_project.
     • Why it stands out: This highlights an opportunity to improve user-facing error messages. A generic error for a read-only condition during a save operation might not be clear to the user. A more specific message explaining why the save failed (i.e., the destination is read-only) would be much more helpful.

  From src/crystal/model.py:

  5. # TODO: Create the underlying task with the full RootResource so that the correct subtitle is displayed.
     • Location: In RootResource.download and RootResource.create_download_task.
     • Why it stands out: This points to a small but noticeable UI polish issue. When a RootResource is downloaded, the task in the UI is created using just the underlying Resource. This means the task's subtitle in the UI might be generic. The TODO suggests passing the full RootResource to the task so it can display a more descriptive subtitle (like the root resource's name), improving UI clarity.