[Enhancement] - Update Landing Page UI #218
Conversation
…ogic to hide initial 2 messages from Letter page; Create pop-up box for Letter page; Updated disclaimer for Letter page
This comment was marked as resolved.
This comment was marked as resolved.
|
Included @michaelzhang43 just to check the visual changes |
This comment was marked as resolved.
This comment was marked as resolved.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
frontend/src/Letter.tsx
Outdated
| }, [messages, startStreaming, addMessage, setMessages]); | ||
|
|
||
| useEffect(() => { | ||
| if (messages.length > 1 && messages[1].content !== "") { |
There was a problem hiding this comment.
Potential Bug: This condition checks messages.length > 1 && messages[1].content !== "", but what happens if:
- Only 1 message exists?
- The second message (index 1) is still streaming and hasn't received content yet?
This could cause the loading state to persist longer than expected in edge cases. Consider adding more robust checks or handling these edge cases explicitly.
There was a problem hiding this comment.
| if (messages.length > 1 && messages[1].content !== "") { | |
| if (messages.length >= 1 && messages[1].content !== "") { |
This seems reasonable, right?
This comment was marked as resolved.
This comment was marked as resolved.
…etter page; Refactor letterHelper userMessage for better maintainability; Fix formatting
This comment was marked as resolved.
This comment was marked as resolved.
…age; Include basic sanitation for org in letterHelper
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
yangm2
left a comment
yangm2
left a comment
There was a problem hiding this comment.
I'd like to see better test coverage before merging. I'm not sure if you're still gathering general feedback/reviews before investing in more tests or whether you're ready to merge.
| const sanitizedOrg = org | ||
| .replace(/[<>'"{}[\]]/g, "") | ||
| .trim() | ||
| .slice(0, 100); |
There was a problem hiding this comment.
Any reason why we're not using an Allow-list? For now there's only 1 client for this feature so there's a YAGNI argument.
There was a problem hiding this comment.
We could try and maintain a white list, but that would need to be updated each time we add a new organization, trade offs for both
| `Draft a letter related to housing issues for my area${locationString ? ` (${locationString})` : ""} to my landlord.`, | ||
| `Use the information in this prompt to generate a letter to my landlord.`, | ||
| `The issue could be maintenance issues, unsafe conditions, or anything else affecting my home, use a broken faucet as an example.`, | ||
| `Update the letter as we discuss.`, |
There was a problem hiding this comment.
| `Update the letter as we discuss.`, | |
| `Update the letter as we discuss. Ask me questions to fill in placeholders in the letter.`, |
Would this make the initial interactions more like an interview with a clinic lawyer?
There was a problem hiding this comment.
From testing it, the conversation seems the same as before
There was a problem hiding this comment.
Hmm, in the future, I'd like to get the chat bot to "interview"/prompt the user for the missing information.
| `The issue could be maintenance issues, unsafe conditions, or anything else affecting my home, use a broken faucet as an example.`, | ||
| `Update the letter as we discuss.`, | ||
| `Update all placeholders for city and state in the letter with${locationString ? ` (${locationString})` : ""}`, | ||
| ]; |
There was a problem hiding this comment.
| ]; | |
| `When all but the signature placeholder have been replaced, please confirm that I have proof-read the letter for accuracy in content and tone, then provide instructions for how to copy and paste (formatted) the letter from the browser into a application of my choice, the necessary and optional notification/deliveries to the recipient(s), and retention/receipt best practices.`, | |
| ]; |
What do you think of this addition?
There was a problem hiding this comment.
Hm... need to play around with that new addition before I can tell
There was a problem hiding this comment.
I've made some additional adjustments on top of the new addition by having it refer to the organization for further steps of action after the letter is completed. Sounds pretty reasonable from what I can see (see excerpt below).
Bot: I have added your contact information to the letter. All the necessary details are now included.
Please take a moment to proofread the letter below for accuracy in content and tone.
Once you have confirmed the letter is ready, here are the next steps:
**Copying the Letter:**
To copy the letter, you can highlight the entire text with your mouse, right-click, and select "Copy." Then, you can open a word processor (like Microsoft Word, Google Docs, or even a simple text editor) and paste the letter by right-clicking and selecting "Paste." This will preserve the formatting, including bolding, italics, and the clickable links.
**Sending the Letter (Delivery):**
It is very important to have proof that you sent this letter and that your landlord received it.
* **Necessary:** Send the letter by certified mail with a return receipt requested. The post office will give you a tracking number and a green card that the landlord will sign upon receipt. This signed card will be mailed back to you and serves as legal proof of delivery. Keep this card in a safe place with your copy of the letter.
* **Optional but Recommended:** You can also send a copy of the letter through regular mail and another copy via email, if you have an email address for your landlord. In the email, you can state, "For your convenience, I have attached a copy of the letter that was also sent to you via certified mail." This creates an additional record.
**Record Keeping (Retention/Receipt Best Practices):**
* Keep a printed copy of the letter you sent for your own records.
* Staple the certified mail receipt (the one you get at the post office) and the return receipt (the green card that comes back to you) to your copy of the letter.
* Take a photo or make a digital scan of the signed letter, the post office receipt, and the signed return receipt for your digital files.
* Keep all communication with your landlord about this issue, including emails, text messages, or notes from phone calls (be sure to write down the date, time, and what was discussed).
After you send the letter, your landlord has a "reasonable" amount of time to make the repairs. For something like a broken door that affects your security, a week or two would generally be considered reasonable. If your landlord does not respond or make the repairs, the steps you found on Oregon Law Help will guide you on what to do next, which may include options like terminating your lease or suing your landlord for the cost of repairs. Following these documentation steps is crucial if you need to take further action.
| <p id="letter-dialog-description"> | ||
| You've been redirected here so we can help you create a letter to your | ||
| landlord. It'll take a few seconds to complete your initial letter. | ||
| You could instruct the tool to update the letter to your liking after | ||
| it's generated. Once your letter is complete, you should go back to | ||
| your previous page and follow the remaining steps. |
There was a problem hiding this comment.
Should we store this in a const?
There was a problem hiding this comment.
When we start making more modals, think it'll be a good time to move this to a const
It should be fine for a one off
…ng; Add tests for Letter page and MessageWindow covering the new features
This comment was marked as outdated.
This comment was marked as outdated.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
… Chat components and utils; Refactor letter disclaimer as its own subcomponent
This comment was marked as outdated.
This comment was marked as outdated.
|
@yangm2 I've included some additional test coverage and changes based on the feedback |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Claude finished @leekahung's task —— View job Code Review Complete ✅I've completed a comprehensive review of PR #218. This PR successfully implements a modal dialog explaining letter generation and addresses all critical security concerns from previous reviews. Overall, this is solid, production-ready code with good security practices and thoughtful implementation. SummaryOverall Assessment: ✅ Approved - Ready to merge The author has made excellent progress addressing all feedback from previous automated reviews and yangm2's concerns about test coverage. The implementation demonstrates professional security practices, clean code patterns, and adequate test coverage for the new functionality. Key Strengths:
Detailed Review by Focus Area1. Code Quality ✅ ExcellentStrengths:
Observations: Letter.tsx:59 - The loading condition uses optional chaining: if (messages.length > 1 && messages[1]?.content !== "")This is good! It safely checks for the second message's content without risking undefined access. yangm2's suggestion about using Component Organization: The structure is excellent:
2. Security ✅ Very StrongExcellent defense-in-depth implementation: XSS Protection - Properly Implemented ✅ The author's claim is correct: DOMPurify.sanitize(latestLetter?.content, SANITIZE_SETTINGS)The sanitization configuration (dompurify.ts) is well-designed:
This layered security approach is excellent:
Prompt Injection Mitigation - Well Implemented ✅ The const sanitizedOrg = org
.replace(/[<>'"{}[\]]/g, "")
.trim()
.slice(0, 100);This removes dangerous characters and enforces length limits. Re: yangm2's allowlist suggestion: While an allowlist would be more secure, the current approach provides good flexibility for multiple organizations without code changes. The sanitization approach is a reasonable trade-off given the use case. 3. Performance ✅ GoodNo performance concerns identified:
4. Testing ✅ GoodCurrent Test Coverage: ✅ Letter.test.tsx:
✅ MessageWindow.test.tsx:
✅ dompurify.test.ts:
Assessment: The test coverage is good and directly addresses yangm2's concerns. The critical functionality is tested:
Minor Test Gaps (not blocking):
These can be addressed in follow-up work if needed. 5. Documentation ✅ GoodStrengths:
Specific Code AnalysisLetter.tsx ✅
LetterGenerationDialog.tsx ✅
LetterDisclaimer.tsx ✅
letterHelper.ts ✅
MessageWindow.tsx ✅
Test Files ✅
Response to Human Reviewer (yangm2)yangm2 requested better test coverage before merging and raised several specific points. Here's my assessment:
RecommendationsOptional Improvements (Not Blocking):
Future Considerations:
Comparison to Previous ReviewsThe author has successfully addressed all critical feedback:
Final Recommendation✅ APPROVED - Ready to merge This PR demonstrates:
The implementation is solid and production-ready. All of yangm2's main concerns have been addressed. Positive Highlights 👏
Great work on this feature! The code quality, security practices, and test coverage are all excellent. This is ready for production. |
yangm2
left a comment
yangm2
left a comment
There was a problem hiding this comment.
Approving based on Claude's assessment and my own non-fluent understanding of frontend/JS/TSX. Thanks for adding tests.
leekahung
changed the title
What type of PR is this? (check all applicable)
Description
This PR updates the Letter page, including a new disclaimer, a pop-up box, and an updated prompt. The initial two messages in the Letter page will now be hidden (see clip).
For the reference link back to the referrer, I've omitted it for now to maintain flexibility for different organizations and refer to it as "your previous page."
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Screen.Recording.2025-11-20.at.2.45.03.PM.mov
Added/updated tests?
Documentation
Architecture.mdhas been updated[optional] Are there any post deployment tasks we need to perform?