fix(firestore-translate-text): emit start and completion events on every write - #3149
Merged
Conversation
…ery write
`handleDocumentWrite` returned before recording anything when the gen2
event arrived with no change data, so neither `onStart` nor
`onCompletion` was published for that invocation. The extension's
`fstranslate` is a gen1 `onWrite` and has no such path: every call
publishes `onStart` first and `onCompletion` last, with each early
return recording completion before it leaves.
The guard now sits after `logs.start` and `recordStartEvent`, and
records `onCompletion` before returning, so the no-data path behaves
like the other no-op branches (delete, unchanged input, missing input).
`onStart` carries `{ data: undefined, params }` there; the payload shape
itself is unchanged and is being restored to the extension's
`{ change, context }` separately.
Every other branch already matched the extension: `onSuccess` comes only
from `updateTranslations`, and a translator failure records one
`onError` per layer (translator, `translateSingle`, handler) before
`onCompletion`. Those sequences are now pinned by a `lifecycle events`
block that asserts the exact event order for each branch, so a future
early return cannot drop the pair unnoticed. 112 tests and
`tsc --noEmit` pass. No live deploy was run.
Fixes #3020
Parity ledger: #2974, firestore-translate-text.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request ensures that onStart and onCompletion lifecycle events are published on every invocation of the fstranslate extension, including early-return paths where write events arrive without change data. This is achieved by moving the start event recording before the check for event.data and recording a completion event prior to returning early. Additionally, the test suite has been updated and expanded with comprehensive lifecycle event sequence assertions. There are no review comments to address, and I have no additional feedback to provide.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The kit's
handleDocumentWritereturned before recording anything when a gen2 write event arrived without change data, so that invocation published neitheronStartnoronCompletion. The extension'sfstranslateis a gen1onWritewith no such path: every call publishesonStartfirst andonCompletionlast, and each early return records completion on its way out.The guard now runs after
logs.startandrecordStartEvent, and recordsonCompletionbefore returning, so the no-data path matches the other no-op branches (delete, unchanged input, missing input). TheonStartpayload carries{ data: undefined, params }there. The payload shape itself is untouched; restoring the extension's{ change, context }shape is PR #3098.I compared every branch of the two handlers. Field-name checks, create, delete, the three update branches, the per-document translation-path return, and the error paths already published the same events in the same order (
onSuccessonly fromupdateTranslations, oneonErrorper layer beforeonCompletion). The one remaining difference is that the kit awaits the handler-levelonErrorwhere the extension does not; that only changes what happens when the publish itself rejects, so I left it alone.A new
lifecycle eventsblock pins the exact event sequence for each branch, and the existing no-data test now asserts the pair is published. 112 tests andtsc --noEmitpass. No live deploy or Eventarc subscription was exercised.Fixes #3020