chore: simplify and clarify frontend hooks, stores, services, and utils#400
Conversation
…nodes, constants, and perf Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| const shouldPublish = state.enablePublish; | ||
|
|
There was a problem hiding this comment.
📝 Info: Removed design rationale comments from streamStoreHelpers
Several comments in streamStoreHelpers.ts documented non-obvious design decisions — e.g., why shouldPublish no longer implicitly enables in session mode (old line ~335-338), why screen source health checks look for .video instead of the whole source (old line ~949-951), and why secondary broadcasts are limited to 2. While the code is unchanged, these comments captured architectural decisions that aren't obvious from reading the code alone. The NOTE: Session mode no longer implicitly enables publishing comment near line 309 (new) was particularly important as it documented a deliberate behavioral change from past versions.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Restored the shouldPublish design-decision comment in 56eeebe — condensed to 2 lines explaining that session mode no longer implicitly enables publishing.
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
There was a problem hiding this comment.
📝 Info: Removal of performance-rationale comments on memoization boundaries
Several removed comments explained why specific useMemo/React.memo boundaries exist — e.g. explaining that useStableEntries returns a stable reference so downstream React.memo components bail out during opacity/rotation drags (ui/src/nodes/compositorNodeEntries.ts), or that ConnectedRotationControl won't re-render when opacity changes due to Jotai's Object.is check (ui/src/nodes/compositorNodeInspector.tsx). The latest commit (3a5b902) partially restored some of these, but many are still reduced. These comments serve as guard rails against future refactors that might accidentally break render performance. Their removal is a documentation quality tradeoff, not a bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Good point — restored a concise memoization rationale on useStableEntries in compositorNodeEntries.ts and a field-level atom subscription comment above the connected controls in compositorNodeInspector.tsx in 3a5b902. These document the render-budget invariants without being verbose.
| // In local development it's common to mix `localhost` and `127.0.0.1`. Those are treated as | ||
| // different sites by browsers, which breaks cookie-based auth flows. When both sides are | ||
| // loopback, rewrite the API hostname to match the current UI hostname. | ||
| // Rewrite loopback hostname to match UI so SameSite=Strict cookies work. |
There was a problem hiding this comment.
📝 Info: Removal of SameSite cookie auth rationale in base.ts
The original comment in ui/src/services/base.ts:26-31 explained the specific browser behavior (SameSite=Strict treats localhost and 127.0.0.1 as different sites) that motivated the loopback hostname rewrite. The replacement comment at lines 11-12 is shorter but still captures the key rationale. The latest commit (3a5b902) further improved this particular comment. No functional risk here — the code is unchanged.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fair point — expanded the comment to explain why browsers treat localhost and 127.0.0.1 as different sites in 3a5b902.
…aft lifecycle comment Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| const handleOpenClearModal = useCallback(() => setShowClearModal(true), []); | ||
| const handleCloseClearModal = useCallback(() => setShowClearModal(false), []); | ||
| const handleOpenSaveModal = useCallback(() => setShowSaveModal(true), []); | ||
| const handleCloseSaveModal = useCallback(() => setShowSaveModal(false), []); | ||
| const handleOpenCreateModal = useCallback(() => setShowCreateModal(true), []); | ||
| const handleCloseCreateModal = useCallback(() => setShowCreateModal(false), []); | ||
| const handleOpenLoadSampleModal = useCallback(() => setShowLoadSampleModal(true), []); | ||
| const handleCloseLoadSampleModal = useCallback(() => { | ||
| setShowLoadSampleModal(false); | ||
| setPendingSample(null); | ||
| }, []); | ||
|
|
||
| const handleOpenSaveFragmentModal = useCallback(() => { | ||
| setShowSaveFragmentModal(true); | ||
| }, []); | ||
|
|
||
| const handleCloseSaveFragmentModal = useCallback(() => { | ||
| setShowSaveFragmentModal(false); | ||
| }, []); | ||
| const handleOpenSaveFragmentModal = useCallback(() => setShowSaveFragmentModal(true), []); | ||
| const handleCloseSaveFragmentModal = useCallback(() => setShowSaveFragmentModal(false), []); |
There was a problem hiding this comment.
📝 Info: useDesignViewModals callback reformatting is logically identical
In ui/src/hooks/useDesignViewModals.ts, eight callbacks were reformatted from multi-line useCallback(() => { setX(true); }, []) to single-line useCallback(() => setX(true), []). The logic is identical — arrow functions with a single expression implicitly return the result of setState, which returns void in both forms. The one callback that remained multi-line (handleCloseLoadSampleModal) is the only one with two statements, correctly preserving its block form.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
2 participants
Summary
Comment simplification and code clarification pass across the frontend state/logic layer as part of the v0.5 pre-release cleanup.
Scope: 53 files across
ui/src/hooks/,ui/src/stores/,ui/src/services/,ui/src/utils/,ui/src/nodes/,ui/src/constants/, andui/src/perf/.What changed:
@publicPlaywright API contract tags,React.memoreferential stability warningstiming.tsconstants from multi-paragraph JSDoc to concise single-line commentsuseCallbackwrappers to single-line form inuseDesignViewModalscatch {}block inservices/base.tsthat violated theno-emptylint ruleWhat did NOT change:
views/,panes/,components/,Layout.tsx,App.tsx,index.tsx,types/,test/)Review & Testing Checklist for Human
yamlPipeline.tsedge cases)Notes
ConfigurableNode.tsx, max-lines in files outside scope) remain unchangedjust lintpasses with 0 errors;just testpasses all 554 tests across 38 test files;just knip-uipassesLink to Devin session: https://staging.itsdev.in/sessions/b0d3afd0cae0467990a86fd92e6fc3ac
Requested by: @streamer45
Devin Review
3a5b902