Skip to content

Create useSSE.ts#682

Open
onyillto wants to merge 2 commits intoLabsCrypt:mainfrom
onyillto:usse-fixes
Open

Create useSSE.ts#682
onyillto wants to merge 2 commits intoLabsCrypt:mainfrom
onyillto:usse-fixes

Conversation

@onyillto
Copy link
Copy Markdown

created the useSSE hook and ensure the UI correctly reflects the connection state.

created the useSSE hook and ensure the UI correctly reflects the connection state.
@onyillto
Copy link
Copy Markdown
Author

closes: #577

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

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

This hook already exists in the codebase at hooks/useSSE. Adding a second copy in components/providers/ creates duplicate code. Also the file placement doesn't match the project convention (hooks go in the hooks/ directory). I'd recommend closing this PR.

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

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

hey, I see this closes #577 but the useSSE hook already exists in the codebase at hooks/useSSE.ts. adding a second copy in components/providers/ would create two competing versions of the same thing.

if the existing hook is missing the polling fallback from #577, the better approach would be to update the existing hook in-place rather than creating a new file. that way there's one source of truth. want to take that approach instead?

@ogazboiz
Copy link
Copy Markdown
Contributor

hey, I see this closes #577 but the useSSE hook already exists in the codebase at hooks/useSSE.ts. adding a second copy in components/providers/ would create two competing versions of the same thing. if the existing hook is missing the polling fallback from #577, the better approach would be to update the existing hook in-place rather than creating a new file. that way there's one source of truth. want to take that approach instead?

@ogazboiz
Copy link
Copy Markdown
Contributor

heads up, a few important changes just landed on main that affect your PR:

  1. axios pinned to 1.13.5 - there's an active supply chain attack on axios 1.14.1 (pulls in confirmed malware). we added overrides in all package.json files to block it.

  2. CI now runs a supply chain audit before backend/frontend jobs. if your lockfile has a compromised package, CI will fail with a clear error.

  3. backend test fixes - loanEndpoints tests now use valid Stellar addresses and base64 strings. if your PR was failing backend CI but you didn't touch backend code, this should fix it after rebase.

please rebase on latest main:

git fetch upstream
git rebase upstream/main
git push --force-with-lease

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants